diff --git a/Gemfile b/Gemfile index 1f6f0d1bb..ffe1245b5 100644 --- a/Gemfile +++ b/Gemfile @@ -38,6 +38,7 @@ gem 'paper_trail' gem 'pg', '~> 1.6' gem 'postmark-rails' gem 'propshaft' +gem 'public_suffix', '~> 7.0' gem 'puma', '~> 7.2' gem 'rack_content_type_default', '~> 1.1' gem 'rack-cors' diff --git a/Gemfile.lock b/Gemfile.lock index 1a57687fb..728c9924d 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -614,6 +614,7 @@ DEPENDENCIES postmark-rails propshaft pry-byebug + public_suffix (~> 7.0) puma (~> 7.2) rack-cors rack_content_type_default (~> 1.1) diff --git a/app/controllers/api/join_controller.rb b/app/controllers/api/join_controller.rb new file mode 100644 index 000000000..bd85a35ae --- /dev/null +++ b/app/controllers/api/join_controller.rb @@ -0,0 +1,78 @@ +# frozen_string_literal: true + +module Api + class JoinController < ApiController + before_action :authorize_user, only: :create + before_action :find_school_and_class + + def show + @status = show_status + render :show, formats: [:json], status: :ok + end + + def create + case action_status + when :wrong_school, :domain_mismatch, :not_a_student + render json: { error: action_status.to_s }, status: :forbidden + when :already_member + render json: { redirect_url: class_redirect_path }, status: :ok + else + add_user_to_school_and_class + render json: { redirect_url: class_redirect_path }, status: :ok + end + end + + private + + def find_school_and_class + normalized = params[:join_code].to_s.upcase.gsub(/[^A-Z0-9]/, '') + @school_class = SchoolClass.find_by!(join_code: normalized) + @school = @school_class.school + end + + def show_status + return :unauthenticated unless current_user + + action_status + end + + def action_status + @action_status ||= compute_action_status + end + + def compute_action_status + return :already_member if user_is_member_of_class? + return :joinable if user_is_student_of_school? + return :not_a_student if user_has_non_student_role? + return :wrong_school if user_in_different_school? + return :domain_mismatch unless @school.valid_email?(current_user.email) + + :joinable + end + + def class_redirect_path + "/school/#{@school.code}/class/#{@school_class.code}" + end + + def user_is_member_of_class? + ClassStudent.exists?(school_class: @school_class, student_id: current_user.id) + end + + def user_is_student_of_school? + Role.exists?(school: @school, user_id: current_user.id, role: Role.roles[:student]) + end + + def user_has_non_student_role? + Role.where(user_id: current_user.id).where.not(role: Role.roles[:student]).exists? + end + + def user_in_different_school? + Role.where(user_id: current_user.id).where.not(school_id: @school.id).exists? + end + + def add_user_to_school_and_class + Role.create!(school: @school, user_id: current_user.id, role: :student) unless Role.exists?(school: @school, user_id: current_user.id) + ClassStudent.create!(school_class: @school_class, student_id: current_user.id) + end + end +end diff --git a/app/controllers/api/school_classes_controller.rb b/app/controllers/api/school_classes_controller.rb index 7774044be..17d9baae7 100644 --- a/app/controllers/api/school_classes_controller.rb +++ b/app/controllers/api/school_classes_controller.rb @@ -81,6 +81,14 @@ def destroy end end + def regenerate_join_code + @school_class.regenerate_join_code! + @school_class_with_teachers = @school_class.with_teachers + render :show, formats: [:json], status: :ok + rescue ActiveRecord::RecordInvalid => e + render json: { error: e.message }, status: :unprocessable_content + end + private def render_student_index(school_classes) diff --git a/app/models/ability.rb b/app/models/ability.rb index 011460a18..8365cf7a3 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -62,7 +62,7 @@ def define_authenticated_non_student_abilities(user) def define_school_owner_abilities(school:) can(%i[read update destroy], School, id: school.id) can(%i[read], :school_member) - can(%i[read create import update destroy], SchoolClass, school: { id: school.id }) + can(%i[read create import update destroy regenerate_join_code], SchoolClass, school: { id: school.id }) can(%i[read show_context], Project, school_id: school.id, lesson: { visibility: %w[teachers students] }) can(%i[read create create_batch destroy], ClassStudent, school_class: { school: { id: school.id } }) can(%i[read create destroy], :school_owner) @@ -78,7 +78,7 @@ def define_school_teacher_abilities(user:, school:) can(%i[read], School, id: school.id) can(%i[read], :school_member) can(%i[create import], SchoolClass, school: { id: school.id }) - can(%i[read update destroy], SchoolClass, school: { id: school.id }, teachers: { teacher_id: user.id }) + can(%i[read update destroy regenerate_join_code], SchoolClass, school: { id: school.id }, teachers: { teacher_id: user.id }) can(%i[read create create_batch destroy], ClassStudent, school_class: { school: { id: school.id }, teachers: { teacher_id: user.id } }) can(%i[read], :school_owner) can(%i[read], :school_teacher) diff --git a/app/models/school.rb b/app/models/school.rb index fe35f00fa..5ee72ae4c 100644 --- a/app/models/school.rb +++ b/app/models/school.rb @@ -6,6 +6,7 @@ class School < ApplicationRecord has_many :projects, dependent: :nullify has_many :roles, dependent: :nullify has_many :school_projects, dependent: :nullify + has_many :school_email_domains, dependent: :destroy VALID_URL_REGEX = %r{\A(?:https?://)?(?:www.)?[a-z0-9]+([-.]{1}[a-z0-9]+)*\.[a-z]{2,63}(\.[a-z]{2,63})*(/.*)?\z}ix @@ -116,6 +117,19 @@ def import_in_progress? .exists?(description: id) end + def valid_domain?(candidate_domain) + school_email_domains.exists?(domain: candidate_domain) + end + + def valid_email?(email) + return false if email.blank? + + local, separator, domain = email.to_s.rpartition('@') + return false if separator.empty? || local.blank? || domain.blank? + + valid_domain?(domain.strip.downcase) + end + private # Ensure the reference is nil, not an empty string diff --git a/app/models/school_class.rb b/app/models/school_class.rb index e35f9b4f6..3d8c373ff 100644 --- a/app/models/school_class.rb +++ b/app/models/school_class.rb @@ -10,9 +10,11 @@ class SchoolClass < ApplicationRecord scope :with_teachers, ->(user_id) { joins(:teachers).where(teachers: { id: user_id }) } before_validation :assign_class_code, on: %i[create import] + before_validation :assign_join_code, on: %i[create import] validates :name, presence: true validates :code, uniqueness: { scope: :school_id }, presence: true, format: { with: /\d\d-\d\d-\d\d/, allow_nil: false } + validates :join_code, uniqueness: true, presence: true, format: { with: JoinCodeGenerator::FORMAT_REGEX, allow_nil: false } validate :code_cannot_be_changed validate :school_class_has_at_least_one_teacher @@ -58,6 +60,21 @@ def assign_class_code errors.add(:code, 'could not be generated') end + def assign_join_code + return if join_code.present? + + loop do + self.join_code = JoinCodeGenerator.generate + break if join_code_is_unique? + end + end + + def regenerate_join_code! + self.join_code = nil + assign_join_code + save! + end + def submitted_count return 0 if lessons.empty? @@ -88,4 +105,8 @@ def code_cannot_be_changed def code_is_unique_within_school? code.present? && SchoolClass.where(code:, school:).none? end + + def join_code_is_unique? + join_code.present? && SchoolClass.where(join_code:).where.not(id:).none? + end end diff --git a/app/models/school_email_domain.rb b/app/models/school_email_domain.rb new file mode 100644 index 000000000..dc4ea8010 --- /dev/null +++ b/app/models/school_email_domain.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +class SchoolEmailDomain < ApplicationRecord + belongs_to :school + + validates :domain, presence: true + validates :domain, uniqueness: { scope: :school_id } + + before_validation :validate_domain + + private + + def validate_domain + return if domain.blank? + + value = domain.strip.downcase + # Add a scheme unless it already has one, so URI can parse it + value = "http://#{value}" unless %r{\A[a-z][a-z0-9+\-.]*://}i.match?(value) + uri = URI.parse(value) + host = uri.host&.delete_suffix('.') + + validate_host(host) + rescue URI::InvalidURIError + errors.add(:domain, :invalid) + end + + def validate_host(host) + accounts_host_format = + /\A\s*(?:[A-Za-z0-9](?:[A-Za-z0-9-]{0,61}[A-Za-z0-9])?\.)+[A-Za-z]{2,63}\s*\z/i + + unless host&.match?(accounts_host_format) + errors.add(:domain, :invalid) + return + end + + if PublicSuffix.valid?(host) + self.domain = host + else + errors.add(:domain, :invalid) + end + end +end diff --git a/app/views/api/join/show.json.jbuilder b/app/views/api/join/show.json.jbuilder new file mode 100644 index 000000000..57053fe3d --- /dev/null +++ b/app/views/api/join/show.json.jbuilder @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +json.status @status.to_s +json.school do + json.code @school.code + json.name @school.name +end +json.school_class do + json.code @school_class.code + json.name @school_class.name +end diff --git a/app/views/api/school_classes/_school_class.json.jbuilder b/app/views/api/school_classes/_school_class.json.jbuilder index 135ae733f..93f03f44c 100644 --- a/app/views/api/school_classes/_school_class.json.jbuilder +++ b/app/views/api/school_classes/_school_class.json.jbuilder @@ -9,7 +9,8 @@ json.call( :created_at, :updated_at, :import_origin, - :import_id + :import_id, + :join_code ) json.teachers(teachers) do |teacher| diff --git a/config/routes.rb b/config/routes.rb index c64e4d46d..23d279083 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -70,6 +70,7 @@ resources :members, only: %i[index], controller: 'school_members' resources :classes, only: %i[index show create update destroy], controller: 'school_classes' do post :import, on: :collection + post :regenerate_join_code, on: :member resources :members, only: %i[index create destroy], controller: 'class_members' do post :batch, on: :collection, to: 'class_members#create_batch' end @@ -100,6 +101,9 @@ resources :profile_auth_check, only: %i[index] resources :subscriptions, only: %i[create] + + get '/join/:join_code', to: 'join#show' + post '/join/:join_code', to: 'join#create' end resource :github_webhooks, only: :create, defaults: { formats: :json } diff --git a/db/migrate/20260420104937_create_school_email_domains.rb b/db/migrate/20260420104937_create_school_email_domains.rb new file mode 100644 index 000000000..0101e995c --- /dev/null +++ b/db/migrate/20260420104937_create_school_email_domains.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +class CreateSchoolEmailDomains < ActiveRecord::Migration[7.2] + def change + create_table :school_email_domains, id: :uuid do |t| + t.references :school, null: false, foreign_key: true, type: :uuid + t.string :domain, null: false + + t.timestamps + end + + add_index :school_email_domains, %i[school_id domain], unique: true + end +end diff --git a/db/migrate/20260420104938_add_join_code_to_school_classes.rb b/db/migrate/20260420104938_add_join_code_to_school_classes.rb new file mode 100644 index 000000000..8cf99c09f --- /dev/null +++ b/db/migrate/20260420104938_add_join_code_to_school_classes.rb @@ -0,0 +1,6 @@ +class AddJoinCodeToSchoolClasses < ActiveRecord::Migration[7.2] + def change + add_column :school_classes, :join_code, :string + add_index :school_classes, :join_code, unique: true + end +end diff --git a/db/migrate/20260420104939_backfill_join_code_for_school_classes.rb b/db/migrate/20260420104939_backfill_join_code_for_school_classes.rb new file mode 100644 index 000000000..0d67204a7 --- /dev/null +++ b/db/migrate/20260420104939_backfill_join_code_for_school_classes.rb @@ -0,0 +1,12 @@ +class BackfillJoinCodeForSchoolClasses < ActiveRecord::Migration[7.2] + def up + SchoolClass.find_each do |school_class| + school_class.assign_join_code + school_class.save!(validate: false) + end + end + + def down + # No need to revert - join codes can stay + end +end diff --git a/db/schema.rb b/db/schema.rb index eb22dba99..5495c6c1e 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.2].define(version: 2026_04_10_110000) do +ActiveRecord::Schema[7.2].define(version: 2026_04_20_104939) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" @@ -264,10 +264,21 @@ t.string "code" t.integer "import_origin" t.string "import_id" + t.string "join_code" t.index ["code", "school_id"], name: "index_school_classes_on_code_and_school_id", unique: true + t.index ["join_code"], name: "index_school_classes_on_join_code", unique: true t.index ["school_id"], name: "index_school_classes_on_school_id" end + create_table "school_email_domains", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| + t.uuid "school_id", null: false + t.string "domain", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["school_id", "domain"], name: "index_school_email_domains_on_school_id_and_domain", unique: true + t.index ["school_id"], name: "index_school_email_domains_on_school_id" + end + create_table "school_import_results", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| t.uuid "job_id", null: false t.uuid "user_id", null: false @@ -398,6 +409,7 @@ add_foreign_key "projects", "schools" add_foreign_key "roles", "schools" add_foreign_key "school_classes", "schools" + add_foreign_key "school_email_domains", "schools" add_foreign_key "school_project_transitions", "school_projects" add_foreign_key "school_projects", "projects" add_foreign_key "school_projects", "schools" diff --git a/lib/join_code_generator.rb b/lib/join_code_generator.rb new file mode 100644 index 000000000..a18af1db6 --- /dev/null +++ b/lib/join_code_generator.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +class JoinCodeGenerator + # Removed letters that commonly form offensive words: + # Removed vowels: None (need all 5 for readability) + # Removed consonants: K (dick, fuck), X (sex), Z (rarely used anyway) + CONSONANTS = %w[B C D F G H J L M N P Q R S T V W Y].freeze + VOWELS = %w[A E I O U].freeze + + # Format: CVDDCVDD (e.g., CE18LI80) + # C = Consonant, V = Vowel, D = Digit + FORMAT_REGEX = /\A[A-Z][A-Z0-9]{7}\z/ + + cattr_accessor :random + + self.random ||= Random.new + + # List of offensive letter patterns to avoid (consonant-vowel pairs) + OFFENSIVE_PATTERNS = %w[ + AS BA BO BU DA DI FU HO PO SH TA TI VA + ].freeze + + def self.generate + max_attempts = 100 + max_attempts.times do + code = [ + CONSONANTS.sample(random: random), + VOWELS.sample(random: random), + format('%02d', random.rand(100)), + CONSONANTS.sample(random: random), + VOWELS.sample(random: random), + format('%02d', random.rand(100)) + ].join + + # Extract the CV patterns (positions 0-1 and 4-5) + first_cv = code[0, 2] + second_cv = code[4, 2] + + # Check if either CV pair matches offensive patterns + next if OFFENSIVE_PATTERNS.include?(first_cv) + next if OFFENSIVE_PATTERNS.include?(second_cv) + + return code + end + + # Fallback if we couldn't generate a clean code after max_attempts + raise 'Unable to generate non-offensive join code' + end +end diff --git a/spec/factories/school_class.rb b/spec/factories/school_class.rb index 9dfcad3c1..4c27ed0c1 100644 --- a/spec/factories/school_class.rb +++ b/spec/factories/school_class.rb @@ -4,6 +4,7 @@ factory :school_class do sequence(:name) { |n| "Class #{n}" } code { ForEducationCodeGenerator.generate } + join_code { JoinCodeGenerator.generate } transient do teacher_ids { [SecureRandom.uuid] } diff --git a/spec/features/school_class/regenerating_join_code_spec.rb b/spec/features/school_class/regenerating_join_code_spec.rb new file mode 100644 index 000000000..810365ff1 --- /dev/null +++ b/spec/features/school_class/regenerating_join_code_spec.rb @@ -0,0 +1,84 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe 'Regenerating a school class join code', type: :request do + before do + authenticated_in_hydra_as(teacher) + end + + let(:headers) { { Authorization: UserProfileMock::TOKEN } } + let(:school) { create(:school) } + let(:teacher) { create(:teacher, school:, name: 'School Teacher') } + let!(:school_class) { create(:school_class, name: 'Test School Class', school:, teacher_ids: [teacher.id]) } + + it 'responds 200 OK' do + post("/api/schools/#{school.id}/classes/#{school_class.id}/regenerate_join_code", headers:) + expect(response).to have_http_status(:ok) + end + + it 'responds 200 OK when the user is the school-teacher for the class' do + authenticated_in_hydra_as(teacher) + + post("/api/schools/#{school.id}/classes/#{school_class.id}/regenerate_join_code", headers:) + expect(response).to have_http_status(:ok) + end + + it 'generates a new join code' do + old_join_code = school_class.join_code + + post("/api/schools/#{school.id}/classes/#{school_class.id}/regenerate_join_code", headers:) + data = JSON.parse(response.body, symbolize_names: true) + + expect(data[:join_code]).not_to eq(old_join_code) + expect(data[:join_code]).to match(JoinCodeGenerator::FORMAT_REGEX) + end + + it 'responds with the updated school class JSON including the new join code' do + old_join_code = school_class.join_code + + post("/api/schools/#{school.id}/classes/#{school_class.id}/regenerate_join_code", headers:) + data = JSON.parse(response.body, symbolize_names: true) + + expect(data[:id]).to eq(school_class.id) + expect(data[:name]).to eq('Test School Class') + expect(data[:join_code]).to be_present + expect(data[:join_code]).not_to eq(old_join_code) + end + + it 'responds with the teacher JSON' do + post("/api/schools/#{school.id}/classes/#{school_class.id}/regenerate_join_code", headers:) + data = JSON.parse(response.body, symbolize_names: true) + + expect(data[:teachers].first[:name]).to eq('School Teacher') + end + + it 'responds 401 Unauthorized when no token is given' do + post "/api/schools/#{school.id}/classes/#{school_class.id}/regenerate_join_code" + expect(response).to have_http_status(:unauthorized) + end + + it 'responds 403 Forbidden when the user is a school-owner for a different school' do + other_school = create(:school) + other_class = create(:school_class, school: other_school) + + post("/api/schools/#{other_school.id}/classes/#{other_class.id}/regenerate_join_code", headers:) + expect(response).to have_http_status(:forbidden) + end + + it 'responds 403 Forbidden when the user is not the school-teacher for the class' do + other_teacher = create(:teacher, school:) + authenticated_in_hydra_as(other_teacher) + + post("/api/schools/#{school.id}/classes/#{school_class.id}/regenerate_join_code", headers:) + expect(response).to have_http_status(:forbidden) + end + + it 'responds 403 Forbidden when the user is a school-student' do + student = create(:student, school:) + authenticated_in_hydra_as(student) + + post("/api/schools/#{school.id}/classes/#{school_class.id}/regenerate_join_code", headers:) + expect(response).to have_http_status(:forbidden) + end +end diff --git a/spec/lib/join_code_generator_spec.rb b/spec/lib/join_code_generator_spec.rb new file mode 100644 index 000000000..560b6eb65 --- /dev/null +++ b/spec/lib/join_code_generator_spec.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe JoinCodeGenerator do + describe '.generate' do + it 'generates a string in CVDDCVDD format' do + expect(described_class.generate).to match(JoinCodeGenerator::FORMAT_REGEX) + end + + it 'generates consonant-vowel-digit-digit-consonant-vowel-digit-digit pattern' do + code = described_class.generate + consonants = JoinCodeGenerator::CONSONANTS + vowels = JoinCodeGenerator::VOWELS + + # Check pattern: C-V-DD-C-V-DD + expect(consonants).to include(code[0]) + expect(vowels).to include(code[1]) + expect(code[2]).to match(/\d/) + expect(code[3]).to match(/\d/) + expect(consonants).to include(code[4]) + expect(vowels).to include(code[5]) + expect(code[6]).to match(/\d/) + expect(code[7]).to match(/\d/) + end + + it 'generates a different code each time' do + codes = 10.times.map { described_class.generate } + expect(codes.uniq.length).to eq(10) + end + + it 'does not generate codes with offensive patterns' do + # Generate many codes to check filtering works + codes = 100.times.map { described_class.generate } + + codes.each do |code| + first_cv = code[0, 2] + second_cv = code[4, 2] + + expect(JoinCodeGenerator::OFFENSIVE_PATTERNS).not_to include(first_cv) + expect(JoinCodeGenerator::OFFENSIVE_PATTERNS).not_to include(second_cv) + end + end + end +end diff --git a/spec/models/school_class_spec.rb b/spec/models/school_class_spec.rb index d90034d28..a8cdeebe7 100644 --- a/spec/models/school_class_spec.rb +++ b/spec/models/school_class_spec.rb @@ -262,6 +262,82 @@ end end + describe '#assign_join_code' do + it 'assigns a join code if not already present' do + school_class = build(:school_class, join_code: nil, school:) + school_class.assign_join_code + expect(school_class.join_code).to match(JoinCodeGenerator::FORMAT_REGEX) + end + + it 'does not assign a join code if already present' do + school_class = build(:school_class, join_code: 'BAFA1234', school:) + school_class.assign_join_code + expect(school_class.join_code).to eq('BAFA1234') + end + + it 'retries until a unique join code is found' do + create(:school_class, join_code: 'BAFA1234', school:) + allow(JoinCodeGenerator).to receive(:generate).and_return('BAFA1234', 'BAFA1234', 'CAFE5678') + + new_class = build(:school_class, join_code: nil, school:) + new_class.assign_join_code + + expect(new_class.join_code).to eq('CAFE5678') + expect(JoinCodeGenerator).to have_received(:generate).exactly(3).times + end + end + + describe '#regenerate_join_code!' do + it 'generates a new join code' do + school_class = create(:school_class, teacher_ids: [teacher.id], school:) + old_join_code = school_class.join_code + school_class.regenerate_join_code! + expect(school_class.join_code).not_to eq(old_join_code) + expect(school_class.join_code).to match(JoinCodeGenerator::FORMAT_REGEX) + end + + it 'persists the new join code' do + school_class = create(:school_class, teacher_ids: [teacher.id], school:) + school_class.regenerate_join_code! + expect(school_class.reload.join_code).to match(JoinCodeGenerator::FORMAT_REGEX) + end + end + + describe 'join_code validations' do + subject(:school_class) { build(:school_class, teacher_ids: [teacher.id, second_teacher.id], school:) } + + it 'assigns join code before validating' do + school_class.join_code = nil + school_class.valid? + expect(school_class.join_code).to match(JoinCodeGenerator::FORMAT_REGEX) + end + + it 'requires a globally unique join code' do + school_class.save! + other_school = create(:school) + school_class_with_duplicate = build(:school_class, school: other_school, join_code: school_class.join_code) + school_class_with_duplicate.valid? + expect(school_class_with_duplicate.errors[:join_code]).to include('has already been taken') + end + + it 'requires a valid join code format' do + school_class.join_code = 'invalid' + expect(school_class).not_to be_valid + end + + it 'accepts a valid join code format' do + school_class.join_code = 'BAFA1234' + expect(school_class).to be_valid + end + + it 'allows the join code to be changed' do + school_class.join_code = 'BAFA1234' + school_class.save! + school_class.join_code = 'CAFE5678' + expect(school_class).to be_valid + end + end + describe '#submitted_count' do it 'returns 0 if there are no lessons' do school_class = create(:school_class, teacher_ids: [teacher.id], school:) diff --git a/spec/models/school_email_domain_spec.rb b/spec/models/school_email_domain_spec.rb new file mode 100644 index 000000000..0ca313e00 --- /dev/null +++ b/spec/models/school_email_domain_spec.rb @@ -0,0 +1,153 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe SchoolEmailDomain do + subject(:school_email_domain) { described_class.create!(school:, domain:) } + + let(:school) { create(:school, creator_id: SecureRandom.uuid) } + let(:domain) { 'example.edu' } + + describe 'associations' do + it { is_expected.to belong_to(:school) } + it { is_expected.to validate_presence_of(:domain) } + it { is_expected.to be_valid } + end + + context 'with a valid domain' do + describe 'domain normalisation' do + context 'when given a full url' do + let(:domain) { 'http://mail.school.edu/path?query=1' } + + it 'extracts the host' do + expect(school_email_domain.domain).to eq('mail.school.edu') + end + end + + context 'when given a full https url' do + let(:domain) { 'https://mail.school.edu/path' } + + it 'extracts the host' do + expect(school_email_domain.domain).to eq('mail.school.edu') + end + end + + context 'when given a domain with a trailing dot' do + let(:domain) { 'EXAMPLE.EDU.' } + + it 'stores the host without the trailing dot' do + expect(school_email_domain.domain).to eq('example.edu') + end + end + + context 'when given a capitalised host' do + let(:domain) { 'EXAMPLE.EDU' } + + it 'downcases the host' do + expect(school_email_domain.domain).to eq('example.edu') + end + end + + context 'with a leading @' do + let(:domain) { '@example.edu' } + + it 'removes the @' do + expect(school_email_domain.domain).to eq('example.edu') + end + end + end + + describe 'public suffix list validation' do + context 'when there is at least one registrable label before the public suffix' do + let(:domain) { 'example.edu' } + + it 'accepts the domain' do + expect(school_email_domain.domain).to eq('example.edu') + end + end + + context 'when there is a subdomain before a valid public suffix' do + let(:domain) { 'mail.example.edu' } + + it 'accepts the domain' do + expect(school_email_domain.domain).to eq('mail.example.edu') + end + end + + context 'when there is a hostname under a multi-part public suffix' do + let(:domain) { 'school.example.co.uk' } + + it 'accepts the domain' do + expect(school_email_domain.domain).to eq('school.example.co.uk') + end + end + + context 'when given a district-style host with a multi-part public suffix' do + let(:domain) { 'school.k12.tx.us' } + + it 'accepts the domain' do + expect(school_email_domain.domain).to eq('school.k12.tx.us') + end + end + end + + describe 'domain uniqueness' do + context 'when the proposed domain matches the existing record' do + subject(:school_email_domain) { described_class.new(school:, domain:) } + + let(:domain) { 'example.edu' } + + before do + described_class.create!(school:, domain: 'example.edu') + school_email_domain.valid? + end + + it 'rejects the duplicate' do + expect(school_email_domain).not_to be_valid + end + + it 'records :taken on domain' do + expect(school_email_domain.errors.of_kind?(:domain, :taken)).to be(true) + end + end + + context 'when the proposed domain matches after normalisation' do + subject(:school_email_domain) { described_class.new(school:, domain:) } + + let(:domain) { 'http://EXAMPLE.EDU' } + + before do + described_class.create!(school:, domain: 'example.edu') + school_email_domain.valid? + end + + it 'rejects the duplicate' do + expect(school_email_domain).not_to be_valid + end + + it 'records :taken on domain' do + expect(school_email_domain.errors.of_kind?(:domain, :taken)).to be(true) + end + end + + it 'allows the same domain for a different school' do + described_class.create!(school:, domain: 'example.edu') + other_school = create(:school, creator_id: SecureRandom.uuid) + other_school_email_domain = described_class.new(school: other_school, domain: 'example.edu') + + expect(other_school_email_domain).to be_valid + end + end + end + + context 'with an invalid domain' do + it { is_expected.not_to allow_value('').for(:domain) } + it { is_expected.not_to allow_value(' ').for(:domain) } + it { is_expected.not_to allow_value('http://').for(:domain) } + it { is_expected.not_to allow_value('edu').for(:domain) } + it { is_expected.not_to allow_value('com').for(:domain) } + it { is_expected.not_to allow_value('co.uk').for(:domain) } + it { is_expected.not_to allow_value('http://invalid uri').for(:domain) } + it { is_expected.not_to allow_value('-wrong.edu').for(:domain) } + end +end diff --git a/spec/models/school_spec.rb b/spec/models/school_spec.rb index 7aec97db1..9ed796612 100644 --- a/spec/models/school_spec.rb +++ b/spec/models/school_spec.rb @@ -35,6 +35,12 @@ expect(school.roles.size).to eq(2) end + it 'has many school email domains' do + SchoolEmailDomain.create!(school:, domain: 'example.edu') + SchoolEmailDomain.create!(school:, domain: 'other.edu') + expect(school.school_email_domains.size).to eq(2) + end + context 'when a school is destroyed' do let!(:school_class) { create(:school_class, school:, teacher_ids: [teacher.id]) } let!(:lesson_1) { create(:lesson, user_id: teacher.id, school_class:) } @@ -88,6 +94,11 @@ school.destroy! expect(role.reload.school_id).to be_nil end + + it 'also destroys school email domains' do + SchoolEmailDomain.create!(school:, domain: 'example.edu') + expect { school.destroy! }.to change(SchoolEmailDomain, :count).by(-1) + end end end @@ -671,4 +682,53 @@ expect(school.reopen).to be(false) end end + + describe '#valid_domain?' do + let(:valid_domain) { 'valid.edu' } + let(:invalid_domain) { 'invalid.edu' } + + before do + SchoolEmailDomain.create!(school:, domain: valid_domain) + end + + it 'returns true when school has registered the email domain' do + expect(school.valid_domain?(valid_domain)).to be(true) + end + + it 'returns false when school has not registered the email domain' do + expect(school.valid_domain?(invalid_domain)).to be(false) + end + end + + describe '#valid_email?' do + before do + SchoolEmailDomain.create!(school:, domain: 'valid.edu') + end + + it 'returns true when the email domain is registered for the school' do + expect(school.valid_email?('user@valid.edu')).to be(true) + end + + it 'returns false when the email domain is not registered for the school' do + expect(school.valid_email?('user@other.edu')).to be(false) + end + + it 'normalizes case and surrounding whitespace on the domain' do + expect(school.valid_email?('User@VALID.EDU ')).to be(true) + end + + it 'returns false when the email is blank' do + expect(school.valid_email?('')).to be(false) + expect(school.valid_email?(nil)).to be(false) + end + + it 'returns false when the email has no @ separator' do + expect(school.valid_email?('not-an-email')).to be(false) + end + + it 'returns false when the local part or domain is missing' do + expect(school.valid_email?('@valid.edu')).to be(false) + expect(school.valid_email?('user@')).to be(false) + end + end end diff --git a/spec/requests/join_controller_spec.rb b/spec/requests/join_controller_spec.rb new file mode 100644 index 000000000..e2a4dbede --- /dev/null +++ b/spec/requests/join_controller_spec.rb @@ -0,0 +1,235 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe 'Join endpoint' do + let(:school) { create(:school, code: '12-34-56') } + let(:school_class) { create(:school_class, school:, join_code: 'BAFA2345') } + let(:student) { create(:user, email: 'student@example.edu') } + let(:headers) { { Authorization: UserProfileMock::TOKEN } } + + before do + SchoolEmailDomain.create!(school:, domain: 'example.edu') + end + + describe 'GET /api/join/:join_code' do + it 'responds with 404 when the join code does not exist' do + get '/api/join/INVALID123' + expect(response).to have_http_status(:not_found) + end + + it 'normalizes the join code by stripping non-alphanumerics' do + school_class # force creation before the request + + get '/api/join/BAFA-2345' + + expect(response).to have_http_status(:ok) + data = JSON.parse(response.body, symbolize_names: true) + expect(data[:school_class][:code]).to eq(school_class.code) + end + + context 'when the user is not authenticated' do + it 'returns status: unauthenticated with school and class details' do + get "/api/join/#{school_class.join_code}" + + expect(response).to have_http_status(:ok) + data = JSON.parse(response.body, symbolize_names: true) + expect(data[:status]).to eq('unauthenticated') + expect(data[:school]).to include(code: school.code, name: school.name) + expect(data[:school_class]).to include(code: school_class.code, name: school_class.name) + end + end + + context 'when the user is authenticated' do + before { authenticated_in_hydra_as(student) } + + it 'returns status: joinable when the user can join' do + get "/api/join/#{school_class.join_code}", headers: headers + + data = JSON.parse(response.body, symbolize_names: true) + expect(data[:status]).to eq('joinable') + end + + it 'returns status: already_member when the user is already in the class' do + create(:student_role, school:, user_id: student.id) + ClassStudent.create!(school_class:, student_id: student.id) + + get "/api/join/#{school_class.join_code}", headers: headers + + data = JSON.parse(response.body, symbolize_names: true) + expect(data[:status]).to eq('already_member') + end + + it 'returns status: wrong_school when the user belongs to a different school' do + other_school = create(:school) + create(:student_role, school: other_school, user_id: student.id) + + get "/api/join/#{school_class.join_code}", headers: headers + + data = JSON.parse(response.body, symbolize_names: true) + expect(data[:status]).to eq('wrong_school') + end + + it 'returns status: not_a_student when the user is a teacher of this school' do + create(:teacher_role, school:, user_id: student.id) + + get "/api/join/#{school_class.join_code}", headers: headers + + data = JSON.parse(response.body, symbolize_names: true) + expect(data[:status]).to eq('not_a_student') + end + + it 'returns status: not_a_student when the user is an owner of this school' do + create(:owner_role, school:, user_id: student.id) + + get "/api/join/#{school_class.join_code}", headers: headers + + data = JSON.parse(response.body, symbolize_names: true) + expect(data[:status]).to eq('not_a_student') + end + + it 'returns status: not_a_student for a teacher of a different school (not wrong_school)' do + other_school = create(:school) + create(:teacher_role, school: other_school, user_id: student.id) + + get "/api/join/#{school_class.join_code}", headers: headers + + data = JSON.parse(response.body, symbolize_names: true) + expect(data[:status]).to eq('not_a_student') + end + + context 'when the email domain is not registered for the school' do + let(:student) { create(:user, email: 'student@other.edu') } + + it 'returns status: domain_mismatch' do + get "/api/join/#{school_class.join_code}", headers: headers + + data = JSON.parse(response.body, symbolize_names: true) + expect(data[:status]).to eq('domain_mismatch') + end + + it 'returns status: joinable when the user is already a student of the school' do + create(:student_role, school:, user_id: student.id) + + get "/api/join/#{school_class.join_code}", headers: headers + + data = JSON.parse(response.body, symbolize_names: true) + expect(data[:status]).to eq('joinable') + end + end + end + end + + describe 'POST /api/join/:join_code' do + context 'when the user is not authenticated' do + it 'responds with 401' do + post "/api/join/#{school_class.join_code}" + expect(response).to have_http_status(:unauthorized) + end + end + + context 'when the user is authenticated' do + before { authenticated_in_hydra_as(student) } + + it 'adds the user to the school and class and returns a redirect URL' do + expect do + post "/api/join/#{school_class.join_code}", headers: headers + end.to change(ClassStudent, :count).by(1).and change(Role, :count).by(1) + + expect(response).to have_http_status(:ok) + data = JSON.parse(response.body, symbolize_names: true) + expect(data[:redirect_url]).to eq("/school/#{school.code}/class/#{school_class.code}") + + created_role = Role.find_by(user_id: student.id, school:) + expect(created_role.role).to eq('student') + end + + it 'is idempotent when the user is already in the class' do + create(:student_role, school:, user_id: student.id) + ClassStudent.create!(school_class:, student_id: student.id) + + expect do + post "/api/join/#{school_class.join_code}", headers: headers + end.not_to change(ClassStudent, :count) + + expect(response).to have_http_status(:ok) + data = JSON.parse(response.body, symbolize_names: true) + expect(data[:redirect_url]).to eq("/school/#{school.code}/class/#{school_class.code}") + end + + it 'does not duplicate the school role if the user is already in the school' do + create(:student_role, school:, user_id: student.id) + + expect do + post "/api/join/#{school_class.join_code}", headers: headers + end.to change(ClassStudent, :count).by(1) + + expect(Role.where(user_id: student.id, school:).count).to eq(1) + end + + it 'responds with 403 wrong_school when the user belongs to a different school' do + other_school = create(:school) + create(:student_role, school: other_school, user_id: student.id) + + post "/api/join/#{school_class.join_code}", headers: headers + + expect(response).to have_http_status(:forbidden) + data = JSON.parse(response.body, symbolize_names: true) + expect(data[:error]).to eq('wrong_school') + end + + context 'when the email domain is not registered for the school' do + let(:student) { create(:user, email: 'student@other.edu') } + + it 'responds with 403 domain_mismatch and does not enroll the user' do + expect do + post "/api/join/#{school_class.join_code}", headers: headers + end.not_to change(ClassStudent, :count) + + expect(response).to have_http_status(:forbidden) + data = JSON.parse(response.body, symbolize_names: true) + expect(data[:error]).to eq('domain_mismatch') + end + + it 'enrolls the user when they are already a student of the school' do + create(:student_role, school:, user_id: student.id) + + expect do + post "/api/join/#{school_class.join_code}", headers: headers + end.to change(ClassStudent, :count).by(1) + + expect(response).to have_http_status(:ok) + data = JSON.parse(response.body, symbolize_names: true) + expect(data[:redirect_url]).to eq("/school/#{school.code}/class/#{school_class.code}") + end + end + + it 'responds with 403 not_a_student when the user is a teacher of the school' do + create(:teacher_role, school:, user_id: student.id) + + expect do + post "/api/join/#{school_class.join_code}", headers: headers + end.not_to change(ClassStudent, :count) + + expect(response).to have_http_status(:forbidden) + data = JSON.parse(response.body, symbolize_names: true) + expect(data[:error]).to eq('not_a_student') + end + + it 'responds with 403 not_a_student when the user is an owner of the school' do + create(:owner_role, school:, user_id: student.id) + + post "/api/join/#{school_class.join_code}", headers: headers + + expect(response).to have_http_status(:forbidden) + data = JSON.parse(response.body, symbolize_names: true) + expect(data[:error]).to eq('not_a_student') + end + + it 'responds with 404 when the join code does not exist' do + post '/api/join/INVALID123', headers: headers + expect(response).to have_http_status(:not_found) + end + end + end +end