diff --git a/dashboard/app/models/authentication_option.rb b/dashboard/app/models/authentication_option.rb index 9a78026b09b4d..817021fcdbb11 100644 --- a/dashboard/app/models/authentication_option.rb +++ b/dashboard/app/models/authentication_option.rb @@ -33,7 +33,8 @@ class AuthenticationOption < ApplicationRecord validates_email_format_of :email, allow_blank: true, if: :email_changed?, unless: -> {email.to_s.utf8mb4?} validate :email_must_be_unique, :hashed_email_must_be_unique, unless: -> {UNTRUSTED_EMAIL_CREDENTIAL_TYPES.include? credential_type} - validate :cred_type_and_auth_id_must_be_unique + + validates :authentication_id, uniqueness: {scope: [:credential_type, :deleted_at]} after_create :set_primary_contact_info @@ -183,16 +184,4 @@ def update_oauth_credential_tokens(credentials) errors.add :email, I18n.t('errors.messages.taken') end end - - private def cred_type_and_auth_id_must_be_unique - # skip the db lookup if possible - return unless authentication_id.present? && - (credential_type_changed? || authentication_id_changed?) && - errors.blank? - - other = User.find_by_credential(type: credential_type, id: authentication_id) - if other && other != user - errors.add :credential_type, I18n.t('errors.messages.taken') - end - end end diff --git a/dashboard/test/factories/factories.rb b/dashboard/test/factories/factories.rb index 8071a723ce0ac..2781d2dbb9e5b 100644 --- a/dashboard/test/factories/factories.rb +++ b/dashboard/test/factories/factories.rb @@ -72,7 +72,7 @@ factory :user do birthday Time.zone.today - 21.years - email {("#{user_type}_#{(User.maximum(:id) || 0) + 1}@code.org")} + sequence(:email) {|n| "#{user_type}_#{n}@code.org"} password "00secret" locale 'en-US' sequence(:name) {|n| "User#{n} Codeberg"} @@ -386,7 +386,7 @@ email: user.email, hashed_email: user.hashed_email, credential_type: AuthenticationOption::GOOGLE, - authentication_id: "abcd#{user.id}", + authentication_id: SecureRandom.uuid, data: { oauth_token: 'some-google-token', oauth_refresh_token: 'some-google-refresh-token', @@ -403,7 +403,7 @@ email: user.email, hashed_email: user.hashed_email, credential_type: AuthenticationOption::CLEVER, - authentication_id: '456efgh', + authentication_id: SecureRandom.uuid, data: { oauth_token: 'some-clever-token' }.to_json @@ -447,7 +447,7 @@ association :user sequence(:email) {|n| "testuser#{n}@example.com.xx"} credential_type AuthenticationOption::EMAIL - authentication_id {User.hash_email email} + authentication_id SecureRandom.uuid factory :google_authentication_option do credential_type AuthenticationOption::GOOGLE diff --git a/dashboard/test/lib/user_multi_auth_helper_test.rb b/dashboard/test/lib/user_multi_auth_helper_test.rb index 7446d8323daf1..28d07b19276cd 100644 --- a/dashboard/test/lib/user_multi_auth_helper_test.rb +++ b/dashboard/test/lib/user_multi_auth_helper_test.rb @@ -20,13 +20,17 @@ class UserMultiAuthHelperTest < ActiveSupport::TestCase test 'oauth_tokens_for_provider returns most recently updated tokens for migrated teacher' do Timecop.freeze do user = create :teacher - create :authentication_option, credential_type: AuthenticationOption::CLEVER, user: user, data: { - oauth_token: 'old-clever-token' - }.to_json + create :authentication_option, + authentication_id: "old-auth-id", + credential_type: AuthenticationOption::CLEVER, + user: user, + data: {oauth_token: 'old-clever-token'}.to_json Timecop.travel(1.minute) do - create :authentication_option, credential_type: AuthenticationOption::CLEVER, user: user, data: { - oauth_token: 'newer-clever-token' - }.to_json + create :authentication_option, + authentication_id: "newer-auth-id", + credential_type: AuthenticationOption::CLEVER, + user: user, + data: {oauth_token: 'newer-clever-token'}.to_json clever_token = user.oauth_tokens_for_provider(AuthenticationOption::CLEVER)[:oauth_token] assert_equal 'newer-clever-token', clever_token end diff --git a/dashboard/test/models/authentication_option_test.rb b/dashboard/test/models/authentication_option_test.rb index f49cb1202af1c..db034ddf004fb 100644 --- a/dashboard/test/models/authentication_option_test.rb +++ b/dashboard/test/models/authentication_option_test.rb @@ -29,7 +29,7 @@ class AuthenticationOptionTest < ActiveSupport::TestCase teacher_email = 'TESTcaseSANITIZATION@test.com' sanitized = 'testcasesanitization@test.com' teacher = create(:teacher, email: teacher_email) - email_auth = create(:authentication_option, user: teacher, email: teacher_email) + email_auth = teacher.primary_contact_info assert_equal sanitized, email_auth.email assert_equal email_auth.hashed_email, AuthenticationOption.hash_email(sanitized) end @@ -37,7 +37,7 @@ class AuthenticationOptionTest < ActiveSupport::TestCase test 'student email is not stored but hashed_email is' do student_email = 'teststudent@test.com' student = create(:student, email: student_email) - email_auth = create(:authentication_option, user: student, email: student_email) + email_auth = student.primary_contact_info assert email_auth.user.student? assert_equal '', email_auth.email assert_equal student.hashed_email, email_auth.hashed_email @@ -49,7 +49,17 @@ class AuthenticationOptionTest < ActiveSupport::TestCase create :authentication_option, credential_type: cred_type, authentication_id: auth_id new_auth_option = build :authentication_option, credential_type: cred_type, authentication_id: auth_id refute new_auth_option.valid? - assert_equal ['Credential type has already been taken'], new_auth_option.errors.full_messages + assert_equal ['Authentication has already been taken'], new_auth_option.errors.full_messages + end + + test 'deleted authentication options do not affect uniqueness' do + cred_type = AuthenticationOption::GOOGLE + auth_id = '54321' + first_auth_option = create :authentication_option, credential_type: cred_type, authentication_id: auth_id + new_auth_option = build :authentication_option, credential_type: cred_type, authentication_id: auth_id + refute new_auth_option.valid? + first_auth_option.delete + assert new_auth_option.valid? end test 'user can have multiple authentication options' do diff --git a/dashboard/test/models/user_test.rb b/dashboard/test/models/user_test.rb index ddd12226e82ce..3e471a9c3af86 100644 --- a/dashboard/test/models/user_test.rb +++ b/dashboard/test/models/user_test.rb @@ -2933,10 +2933,10 @@ def track_progress(user_id, script_level, result, pairings: nil) end test "age is set exactly for Google OAuth users between ages 4 and 20" do - four_year_old = create :user, birthday: (Date.today - 4.years), provider: 'google_oauth2' + four_year_old = build :user, birthday: (Date.today - 4.years), provider: 'google_oauth2' assert_equal 4, four_year_old.age - twenty_year_old = create :user, birthday: (Date.today - 20.years), provider: 'google_oauth2' + twenty_year_old = build :user, birthday: (Date.today - 20.years), provider: 'google_oauth2' assert_equal 20, twenty_year_old.age end @@ -2958,10 +2958,10 @@ def track_progress(user_id, script_level, result, pairings: nil) end test "age is set exactly for Clever users between ages 4 and 20" do - four_year_old = create :user, birthday: (Date.today - 4.years), provider: 'clever' + four_year_old = build :user, birthday: (Date.today - 4.years), provider: 'clever' assert_equal 4, four_year_old.age - twenty_year_old = create :user, birthday: (Date.today - 20.years), provider: 'clever' + twenty_year_old = build :user, birthday: (Date.today - 20.years), provider: 'clever' assert_equal 20, twenty_year_old.age end