diff --git a/app/models/user.rb b/app/models/user.rb index 5bc5d8fff..a3fcc0870 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.to_s.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..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? + 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,8 +62,8 @@ 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? + def user_has_non_student_account_type? + !@user.student_account_type? end def user_in_different_school? 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..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,8 +42,8 @@ end end - context 'when the user is authenticated' do - before { authenticated_in_hydra_as(student) } + 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 get "/api/join/#{school_class.join_code}", headers: headers @@ -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,8 +148,8 @@ end end - context 'when the user is authenticated' do - before { authenticated_in_hydra_as(student) } + 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 expect 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 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)