diff --git a/.env.example b/.env.example index bcf9830dc..d1df03173 100644 --- a/.env.example +++ b/.env.example @@ -24,6 +24,7 @@ HYDRA_CLIENT_ID=editor-dashboard-dev HYDRA_CLIENT_SECRET=secret IDENTITY_URL=http://host.docker.internal:3002 # Internal docker address +PROFILE_URL=http://localhost:3002 # External address for browser redirects SMEE_TUNNEL=https://smee.io/MLq0n9kvAes2vydX diff --git a/app/controllers/api/join_controller.rb b/app/controllers/api/join_controller.rb new file mode 100644 index 000000000..7c8a289f1 --- /dev/null +++ b/app/controllers/api/join_controller.rb @@ -0,0 +1,68 @@ +# frozen_string_literal: true + +module Api + class JoinController < ApiController + before_action :find_school_and_class + before_action :authorize_user, only: [:create] + + rescue_from ActiveRecord::RecordNotFound, with: :not_found + + def show + # Public endpoint to get school code for a join code (for login autofill) + render json: { school_code: @school.code }, status: :ok + end + + def create + # If already a member of this class, just return success + if ClassStudent.exists?(school_class: @school_class, student_id: current_user.id) + render json: { redirect_url: class_page_url, school_code: @school.code }, status: :ok + return + end + + if user_in_different_school? + render json: { error: 'You are already a member of a different school' }, status: :forbidden + return + end + + unless @school.valid_email?(current_user.email) + render json: { error: 'Your email domain does not match the school' }, status: :forbidden + return + end + + add_user_to_school_and_class + + render json: { redirect_url: class_page_url, school_code: @school.code }, status: :created + end + + private + + def find_school_and_class + @normalized_join_code = params[:join_code].to_s.upcase.gsub(/[^A-Z0-9]/, '') + @school_class = SchoolClass.find_by!(join_code: @normalized_join_code) + @school = @school_class.school + end + + def class_page_url + "/en/school/#{@school.code}/class/#{@school_class.code}" + end + + def user_in_different_school? + existing_school = Role.find_by(user_id: current_user.id)&.school + existing_school.present? && existing_school.id != @school.id + end + + def add_user_to_school_and_class + # Add to school if not already a member + Role.create!(school: @school, user_id: current_user.id, role: :student) unless Role.exists?(school: @school, user_id: current_user.id) + + # Add to class if not already a member + return if ClassStudent.exists?(school_class: @school_class, student_id: current_user.id) + + ClassStudent.create!(school_class: @school_class, student_id: current_user.id) + end + + def not_found + render json: { error: 'Join code not found' }, status: :not_found + 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..bd5e4538b 100644 --- a/app/models/school.rb +++ b/app/models/school.rb @@ -116,6 +116,12 @@ def import_in_progress? .exists?(description: id) end + def valid_email?(_email) + # TODO: Implement actual domain verification + # This stub always returns true for now + true + 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..3deba6ff2 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: /\A[A-Z][A-Z0-9]{7}\z/, 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/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 24aaca4bf..ea60cd29e 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -28,6 +28,8 @@ post '/test/reseed', to: 'test_utilities#reseed' + get '/join/:join_code', to: 'join#show', as: 'join' + post '/graphql', to: 'graphql#execute' mount GraphiQL::Rails::Engine, at: '/graphql', graphql_path: '/graphql#execute' unless Rails.env.production? @@ -70,6 +72,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 @@ -99,6 +102,9 @@ resources :features, only: %i[index] resources :profile_auth_check, only: %i[index] + + 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/20260416124302_add_join_code_to_school_classes.rb b/db/migrate/20260416124302_add_join_code_to_school_classes.rb new file mode 100644 index 000000000..8cf99c09f --- /dev/null +++ b/db/migrate/20260416124302_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/20260416124324_backfill_join_code_for_school_classes.rb b/db/migrate/20260416124324_backfill_join_code_for_school_classes.rb new file mode 100644 index 000000000..0d67204a7 --- /dev/null +++ b/db/migrate/20260416124324_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..fd00d8caa 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_16_124324) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" @@ -264,7 +264,9 @@ 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 diff --git a/lib/join_code_generator.rb b/lib/join_code_generator.rb new file mode 100644 index 000000000..8d6d7b315 --- /dev/null +++ b/lib/join_code_generator.rb @@ -0,0 +1,45 @@ +# 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 + + 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..a378a8498 --- /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(/\A[A-Z]{4}\d{4}\z/) + 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..d4ea2115a --- /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(/\A[A-Z][A-Z0-9]{7}\z/) + 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..6767d3d45 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(/\A[A-Z]{4}\d{4}\z/) + 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(/\A[A-Z]{4}\d{4}\z/) + 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(/\A[A-Z]{4}\d{4}\z/) + 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(/\A[A-Z]{4}\d{4}\z/) + 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/requests/join_controller_spec.rb b/spec/requests/join_controller_spec.rb new file mode 100644 index 000000000..3a4f7147e --- /dev/null +++ b/spec/requests/join_controller_spec.rb @@ -0,0 +1,118 @@ +# 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) } # Don't create role yet - let join endpoint do it + let(:headers) { { Authorization: UserProfileMock::TOKEN } } + + before do + stub_const('ENV', ENV.to_hash.merge('EDITOR_PUBLIC_URL' => 'https://editor.example.com')) + end + + describe 'GET /join/:join_code' do + context 'when user is not logged in' do + it 'redirects to Profile student-login with school code and return URL' do + get "/join/#{school_class.join_code}" + + expect(response).to have_http_status(:redirect) + redirect_url = response.location + expect(redirect_url).to include("#{ENV.fetch('EDITOR_PUBLIC_URL')}/en/school") + expect(redirect_url).to include(school.code.to_s) + expect(redirect_url).to include('redirect_url=') + expect(redirect_url).to include('join') + expect(redirect_url).to include(school_class.join_code) + end + + it 'normalizes join code by removing non-alphanumeric characters' do + # Create a class with the normalized code + school_class.update!(join_code: 'BAFA2345') + + get '/join/BAFA-2345' + + expect(response).to have_http_status(:redirect) + redirect_url = response.location + expect(redirect_url).to include('BAFA2345') + expect(redirect_url).not_to include('BAFA-2345') + expect(redirect_url).to include(school.code.to_s) + end + + it 'responds with 404 when join code does not exist' do + get '/join/INVALID123' + expect(response).to have_http_status(:not_found) + end + end + + context 'when user is logged in' do + before do + authenticated_in_hydra_as(student) + end + + it 'adds the user to the school and class and redirects to class page' do + expect do + get "/join/#{school_class.join_code}", headers: + end.to change(ClassStudent, :count).by(1).and change(Role, :count).by(1) + + expect(response).to have_http_status(:redirect) + expect(response.location).to include("#{ENV.fetch('EDITOR_PUBLIC_URL')}/en/school/#{school.code}/class/#{school_class.code}") + + created_role = Role.find_by(user_id: student.id, school:) + expect(created_role).to be_present + expect(created_role.role).to eq('student') + end + + it 'does not create duplicate school role if user already in school' do + create(:student_role, school:, user_id: student.id) + + expect do + get "/join/#{school_class.join_code}", headers: + end.not_to change(Role, :count) + end + + it 'does not create duplicate class membership if user already in class' do + create(:student_role, school:, user_id: student.id) + ClassStudent.create!(school_class:, student_id: student.id) + + expect do + get "/join/#{school_class.join_code}", headers: + end.not_to change(ClassStudent, :count) + + expect(response).to have_http_status(:redirect) + end + + context 'when user email domain does not match school' do + before do + allow(school_class).to receive(:school).and_return(school) + allow(school).to receive(:valid_email?).and_return(false) + allow(SchoolClass).to receive(:find_by!).and_return(school_class) + end + + it 'responds with 403 Forbidden' do + get "/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('Your email domain does not match the school') + end + end + + context 'when user is already a member of a different school' do + let(:other_school) { create(:school) } + + before do + create(:student_role, school: other_school, user_id: student.id) + end + + it 'responds with 403 Forbidden' do + get "/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('You are already a member of a different school') + end + end + end + end +end