From 6ef271a60d1ae422d9fb8ff18b6203c5a917f401 Mon Sep 17 00:00:00 2001 From: Matthew Trew Date: Wed, 26 Aug 2026 14:26:08 +0100 Subject: [PATCH 1/4] Reject unassociated teacher accounts in Join Code flow Also adds tests, updates the user factory to be aware of the new sub field, and updates some existing tests to take advantage of the factory. --- app/models/user.rb | 5 ++++ app/services/join_status_service.rb | 6 ++++- spec/factories/user.rb | 2 ++ spec/models/user_spec.rb | 22 +++++++++++++++ spec/requests/join_controller_spec.rb | 4 +-- spec/services/join_status_service_spec.rb | 33 ++++++++++++++--------- 6 files changed, 56 insertions(+), 16 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index 5bc5d8fff..1c6538d0b 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -21,6 +21,7 @@ class User username roles sso_providers + sub ].freeze attr_accessor(*ATTRIBUTES) @@ -53,6 +54,10 @@ def student? Role.student.exists?(user_id: id) end + def student_account_type? + @sub.starts_with?('student') + end + def admin? parsed_roles.include?('editor-admin') end diff --git a/app/services/join_status_service.rb b/app/services/join_status_service.rb index 2117764f4..e9ac1a3ad 100644 --- a/app/services/join_status_service.rb +++ b/app/services/join_status_service.rb @@ -38,7 +38,7 @@ def existing_user_join_status # The user has no role in this school yet: may they join as a new student? def new_user_join_status - return :not_a_student if user_has_non_student_role? + return :not_a_student if user_has_non_student_role? || user_has_non_student_account_type? return :wrong_school if user_in_different_school? return :domain_mismatch unless @school.email_domain_in_school_domains?(@user.email) @@ -66,6 +66,10 @@ def user_has_non_student_role? Role.where(user_id: @user.id).where.not(role: Role.roles[:student]).exists? end + def user_has_non_student_account_type? + !@user.student_account_type? + end + def user_in_different_school? Role.where(user_id: @user.id).where.not(school_id: @school.id).exists? end diff --git a/spec/factories/user.rb b/spec/factories/user.rb index a2a943e53..e3240bd69 100644 --- a/spec/factories/user.rb +++ b/spec/factories/user.rb @@ -7,6 +7,7 @@ email { Faker::Internet.email } username { nil } sso_providers { [] } + sub { id } factory :admin_user do roles { 'editor-admin' } @@ -20,6 +21,7 @@ email { nil } username { Faker::Internet.username } sso_providers { [] } # standard students have no SSO providers + sub { "student:#{id}" } trait :sso do email { Faker::Internet.email } diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 7b8a76568..33cac47ae 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -74,6 +74,10 @@ expect(user.username).to be_nil end + it 'returns a user with the correct sub' do + expect(user.sub).to eq owner.id + end + context 'when BYPASS_OAUTH is true' do around do |example| ClimateControl.modify(BYPASS_OAUTH: 'true') do @@ -118,6 +122,10 @@ it 'returns a user without an email' do expect(user.email).to be_nil end + + it 'returns a user with the correct sub' do + expect(user.sub).to eq "student:#{student.id}" + end end it 'returns nil when the access token is invalid' do @@ -266,6 +274,20 @@ end end + describe '#student_account_type?' do + it 'returns true for a student account' do + expect(build(:student)).to be_student_account_type + end + + it 'returns false for a non-student account' do + expect(build(:user)).not_to be_student_account_type + end + + it 'returns false for a teacher account' do + expect(build(:teacher)).not_to be_student_account_type + end + end + describe '#parsed_roles' do it 'returns array of role names when roles is set to comma-separated string' do user = build(:user, roles: 'role-1,role-2') diff --git a/spec/requests/join_controller_spec.rb b/spec/requests/join_controller_spec.rb index a82c18487..562da202e 100644 --- a/spec/requests/join_controller_spec.rb +++ b/spec/requests/join_controller_spec.rb @@ -41,7 +41,7 @@ end context 'when the user is authenticated' do - before { authenticated_in_hydra_as(student) } + before { authenticated_in_hydra_as(student, :student) } it 'returns status: joinable when the user can join' do get "/api/join/#{school_class.join_code}", headers: headers @@ -139,7 +139,7 @@ end context 'when the user is authenticated' do - before { authenticated_in_hydra_as(student) } + before { authenticated_in_hydra_as(student, :student) } it 'adds the user to the school and class and returns a redirect URL' do expect do diff --git a/spec/services/join_status_service_spec.rb b/spec/services/join_status_service_spec.rb index 80b2dafea..65528cea1 100644 --- a/spec/services/join_status_service_spec.rb +++ b/spec/services/join_status_service_spec.rb @@ -5,7 +5,7 @@ describe JoinStatusService do let(:school) { create(:school) } let(:school_class) { create(:school_class, school:) } - let(:user) { create(:user, email: 'user@example.edu') } + let(:user) { build(:student, email: 'user@example.edu') } let(:service) { described_class.new(school:, school_class:, user:) } before do @@ -14,10 +14,9 @@ describe '#call' do context 'when the user is already a student of the class' do - before do - create(:student_role, school:, user_id: user.id) - ClassStudent.create!(school_class:, student_id: user.id) - end + let(:user) { create(:student, school:) } + + before { ClassStudent.create!(school_class:, student_id: user.id) } it 'returns :already_member' do expect(service.call).to eq(:already_member) @@ -25,10 +24,9 @@ end context 'when the user is already a teacher of the class' do - before do - create(:teacher_role, school:, user_id: user.id) - ClassTeacher.create!(school_class:, teacher_id: user.id) - end + let(:user) { create(:teacher, school:) } + + before { ClassTeacher.create!(school_class:, teacher_id: user.id) } it 'returns :already_member' do expect(service.call).to eq(:already_member) @@ -36,6 +34,8 @@ end context 'when the user owns the school' do + let(:user) { create(:user, email: 'user@example.edu') } + before { create(:owner_role, school:, user_id: user.id) } it 'returns :owner' do @@ -44,7 +44,7 @@ end context 'when the user is a teacher of the school but not in this class' do - before { create(:teacher_role, school:, user_id: user.id) } + let(:user) { create(:teacher, school:) } it 'returns :joinable_as_teacher' do expect(service.call).to eq(:joinable_as_teacher) @@ -52,7 +52,7 @@ end context 'when the user is already a student of the school but not in this class' do - before { create(:student_role, school:, user_id: user.id) } + let(:user) { create(:student, school:) } it 'returns :joinable' do expect(service.call).to eq(:joinable) @@ -61,8 +61,15 @@ context 'when the user has a non-student role in a different school' do let(:other_school) { create(:school) } + let(:user) { create(:teacher, school: other_school) } + + it 'returns :not_a_student' do + expect(service.call).to eq(:not_a_student) + end + end - before { create(:teacher_role, school: other_school, user_id: user.id) } + context 'when the user does not have a student account type' do + let(:user) { build(:teacher, email: 'user@example.edu') } it 'returns :not_a_student' do expect(service.call).to eq(:not_a_student) @@ -80,7 +87,7 @@ end context "when the user's email domain is not registered for the school" do - let(:user) { create(:user, email: 'user@other.edu') } + let(:user) { build(:student, email: 'user@other.edu') } it 'returns :domain_mismatch' do expect(service.call).to eq(:domain_mismatch) From a876f9f8942e4e4fcb9c642c79f5413d9b457ad7 Mon Sep 17 00:00:00 2001 From: Matthew Trew Date: Wed, 26 Aug 2026 14:44:39 +0100 Subject: [PATCH 2/4] Make student sub check more robust --- app/models/user.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/user.rb b/app/models/user.rb index 1c6538d0b..a3fcc0870 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -55,7 +55,7 @@ def student? end def student_account_type? - @sub.starts_with?('student') + sub.to_s.starts_with?('student:') end def admin? From 0a87cd6ed70a0c020b389057479acbbc3b3c5f16 Mon Sep 17 00:00:00 2001 From: Matthew Trew Date: Wed, 26 Aug 2026 17:00:29 +0100 Subject: [PATCH 3/4] Use account type to identify students in place of role --- app/services/join_status_service.rb | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/app/services/join_status_service.rb b/app/services/join_status_service.rb index e9ac1a3ad..ae4402556 100644 --- a/app/services/join_status_service.rb +++ b/app/services/join_status_service.rb @@ -38,7 +38,7 @@ def existing_user_join_status # The user has no role in this school yet: may they join as a new student? def new_user_join_status - return :not_a_student if user_has_non_student_role? || user_has_non_student_account_type? + return :not_a_student if user_has_non_student_account_type? return :wrong_school if user_in_different_school? return :domain_mismatch unless @school.email_domain_in_school_domains?(@user.email) @@ -62,10 +62,6 @@ def user_has_role_in_school? Role.exists?(school: @school, user_id: @user.id) end - def user_has_non_student_role? - Role.where(user_id: @user.id).where.not(role: Role.roles[:student]).exists? - end - def user_has_non_student_account_type? !@user.student_account_type? end From cbbbcd87fc7f20c560a82e539b960c6541c7aeac Mon Sep 17 00:00:00 2001 From: Matthew Trew Date: Wed, 26 Aug 2026 17:01:06 +0100 Subject: [PATCH 4/4] Use new factories to correct join_controller_spec issues --- spec/requests/join_controller_spec.rb | 132 +++++++++++++++----------- 1 file changed, 75 insertions(+), 57 deletions(-) diff --git a/spec/requests/join_controller_spec.rb b/spec/requests/join_controller_spec.rb index 562da202e..3ebc56083 100644 --- a/spec/requests/join_controller_spec.rb +++ b/spec/requests/join_controller_spec.rb @@ -5,7 +5,9 @@ RSpec.describe 'Join endpoint' do let(:school) { create(:school, code: '12-34-56') } let(:school_class) { create(:school_class, school:, join_code: 'B123-C456') } - let(:student) { create(:user, email: 'student@example.edu') } + let(:student) { build(:student, email: 'student@example.edu') } + let(:teacher) { build(:teacher, email: 'teacher@example.edu') } + let(:owner) { build(:owner, email: 'owner@example.edu') } let(:headers) { { Authorization: UserProfileMock::TOKEN } } before do @@ -40,7 +42,7 @@ end end - context 'when the user is authenticated' do + context 'when the user is authenticated as a student' do before { authenticated_in_hydra_as(student, :student) } it 'returns status: joinable when the user can join' do @@ -70,62 +72,70 @@ expect(data[:status]).to eq('wrong_school') end - it 'returns status: joinable_as_teacher when the user is a teacher of this school not yet in the class' do - create(:teacher_role, school:, user_id: student.id) + context 'when the email domain is not registered for the school' do + let(:student) { build(:student, email: 'student@other.edu') } - get "/api/join/#{school_class.join_code}", headers: headers + 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('joinable_as_teacher') + 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 - it 'returns status: already_member when the user is already a teacher in the class' do - create(:teacher_role, school:, user_id: student.id) - ClassTeacher.create!(school_class:, teacher_id: student.id) + context 'when the user is authenticated as a teacher' do + before { authenticated_in_hydra_as(teacher) } + + it 'returns status: joinable_as_teacher when the user is a teacher of this school not yet in the class' do + create(:teacher_role, school:, user_id: teacher.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') + expect(data[:status]).to eq('joinable_as_teacher') end - it 'returns status: owner when the user is an owner of this school' do - create(:owner_role, school:, user_id: student.id) + it 'returns status: already_member when the user is already a teacher in the class' do + create(:teacher_role, school:, user_id: teacher.id) + ClassTeacher.create!(school_class:, teacher_id: teacher.id) get "/api/join/#{school_class.join_code}", headers: headers data = JSON.parse(response.body, symbolize_names: true) - expect(data[:status]).to eq('owner') + expect(data[:status]).to eq('already_member') 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) + create(:teacher_role, school: other_school, user_id: teacher.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 + 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 + context 'when the user is authenticated as an owner' do + before { authenticated_in_hydra_as(owner) } - it 'returns status: joinable when the user is already a student of the school' do - create(:student_role, school:, user_id: student.id) + it 'returns status: owner when the user is an owner of this school' do + create(:owner_role, school:, user_id: owner.id) - get "/api/join/#{school_class.join_code}", headers: headers + get "/api/join/#{school_class.join_code}", headers: headers - data = JSON.parse(response.body, symbolize_names: true) - expect(data[:status]).to eq('joinable') - end + data = JSON.parse(response.body, symbolize_names: true) + expect(data[:status]).to eq('owner') end end end @@ -138,7 +148,7 @@ end end - context 'when the user is authenticated' do + context 'when the user is authenticated as a student' do before { authenticated_in_hydra_as(student, :student) } it 'adds the user to the school and class and returns a redirect URL' do @@ -188,8 +198,24 @@ expect(data[:error]).to eq('wrong_school') 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 + + # rubocop:disable RSpec/AnyInstance + it 'responds with 500 when action_status returns an unexpected value' do + allow_any_instance_of(Api::JoinController).to receive(:action_status).and_return(:something_unexpected) + + post "/api/join/#{school_class.join_code}", headers: headers + + expect(response).to have_http_status(:internal_server_error) + expect(response.body).to include('Unexpected join action_status') + end + # rubocop:enable RSpec/AnyInstance + context 'when the email domain is not registered for the school' do - let(:student) { create(:user, email: 'student@other.edu') } + let(:student) { build(:student, email: 'student@other.edu') } it 'responds with 403 domain_mismatch and does not enroll the user' do expect do @@ -213,9 +239,13 @@ expect(data[:redirect_url]).to eq("/school/#{school.code}/class/#{school_class.code}") end end + end + + context 'when the user is authenticated as a teacher' do + before { authenticated_in_hydra_as(teacher) } it 'adds the user to the class as a teacher and returns a redirect URL' do - create(:teacher_role, school:, user_id: student.id) + create(:teacher_role, school:, user_id: teacher.id) school_class # force creation before the request expect do @@ -225,14 +255,14 @@ 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}") - expect(ClassTeacher.exists?(school_class:, teacher_id: student.id)).to be(true) - expect(ClassStudent.exists?(school_class:, student_id: student.id)).to be(false) - expect(Role.where(user_id: student.id, school:).pluck(:role)).to eq(['teacher']) + expect(ClassTeacher.exists?(school_class:, teacher_id: teacher.id)).to be(true) + expect(ClassStudent.exists?(school_class:, student_id: teacher.id)).to be(false) + expect(Role.where(user_id: teacher.id, school:).pluck(:role)).to eq(['teacher']) end it 'is idempotent when the user is already a teacher in the class' do - create(:teacher_role, school:, user_id: student.id) - ClassTeacher.create!(school_class:, teacher_id: student.id) + create(:teacher_role, school:, user_id: teacher.id) + ClassTeacher.create!(school_class:, teacher_id: teacher.id) expect do post "/api/join/#{school_class.join_code}", headers: headers @@ -242,9 +272,13 @@ data = JSON.parse(response.body, symbolize_names: true) expect(data[:redirect_url]).to eq("/school/#{school.code}/class/#{school_class.code}") end + end + + context 'when the user is authenticated as an owner' do + before { authenticated_in_hydra_as(owner) } it 'redirects an owner into the class without adding them to it' do - create(:owner_role, school:, user_id: student.id) + create(:owner_role, school:, user_id: owner.id) expect do post "/api/join/#{school_class.join_code}", headers: headers @@ -253,25 +287,9 @@ 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}") - expect(ClassTeacher.exists?(school_class:, teacher_id: student.id)).to be(false) - expect(Role.where(user_id: student.id, school:).pluck(:role)).to eq(['owner']) + expect(ClassTeacher.exists?(school_class:, teacher_id: owner.id)).to be(false) + expect(Role.where(user_id: owner.id, school:).pluck(:role)).to eq(['owner']) 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 - - # rubocop:disable RSpec/AnyInstance - it 'responds with 500 when action_status returns an unexpected value' do - allow_any_instance_of(Api::JoinController).to receive(:action_status).and_return(:something_unexpected) - - post "/api/join/#{school_class.join_code}", headers: headers - - expect(response).to have_http_status(:internal_server_error) - expect(response.body).to include('Unexpected join action_status') - end - # rubocop:enable RSpec/AnyInstance end end end