From 0b0224037598f10f43c0d049b36ff6a4d738559e Mon Sep 17 00:00:00 2001 From: Elijah Hamovitz Date: Tue, 1 Sep 2020 15:23:37 -0700 Subject: [PATCH 1/8] actually validate uniqueness of authentication options, not just uniqueness between users --- dashboard/app/models/authentication_option.rb | 15 ++------------- .../test/models/authentication_option_test.rb | 16 +++++++++++++--- 2 files changed, 15 insertions(+), 16 deletions(-) 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/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 From 4c726bbb8d3ccff6423db6b612008f24600fd318 Mon Sep 17 00:00:00 2001 From: Elijah Hamovitz Date: Wed, 2 Sep 2020 11:08:01 -0700 Subject: [PATCH 2/8] use uuids for authentication_ids in FactoryGirl, so the new uniqueness logic doesn't interfere with tests --- dashboard/test/factories/factories.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/dashboard/test/factories/factories.rb b/dashboard/test/factories/factories.rb index 8071a723ce0ac..29544320d9a02 100644 --- a/dashboard/test/factories/factories.rb +++ b/dashboard/test/factories/factories.rb @@ -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 From 25f5b0cc92a2d135a9dba78ae515fafaec76c029 Mon Sep 17 00:00:00 2001 From: Elijah Hamovitz Date: Wed, 2 Sep 2020 15:26:08 -0700 Subject: [PATCH 3/8] return to using a custom method to check for uniqueness, in order to easily exclude deleted AOs --- dashboard/app/models/authentication_option.rb | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/dashboard/app/models/authentication_option.rb b/dashboard/app/models/authentication_option.rb index 817021fcdbb11..59af9414990e9 100644 --- a/dashboard/app/models/authentication_option.rb +++ b/dashboard/app/models/authentication_option.rb @@ -33,8 +33,7 @@ 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} - - validates :authentication_id, uniqueness: {scope: [:credential_type, :deleted_at]} + validate :cred_type_and_auth_id_must_be_unique after_create :set_primary_contact_info @@ -184,4 +183,15 @@ 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 if authentication_id.blank? || deleted_at.present? + + # note that here we are deliberately not querying deleted AOs + others = AuthenticationOption. + where(credential_type: credential_type, authentication_id: authentication_id). + where('id != ?', id) + errors.add(:credential_type, I18n.t('errors.messages.taken')) unless others.empty? + end end From 97b1678d71774e484f47085c7e6aeccd7aeb48ec Mon Sep 17 00:00:00 2001 From: Elijah Hamovitz Date: Wed, 2 Sep 2020 15:38:34 -0700 Subject: [PATCH 4/8] use a UUID for automatic test user emails, to more robustly prevent collisions --- dashboard/test/factories/factories.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dashboard/test/factories/factories.rb b/dashboard/test/factories/factories.rb index 29544320d9a02..971299eb13119 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")} + email {("#{user_type}_#{SecureRandom.uuid}@code.org")} password "00secret" locale 'en-US' sequence(:name) {|n| "User#{n} Codeberg"} From 763f6ebd8cd2b6e18407c9ee00a9db4c6b5f5a42 Mon Sep 17 00:00:00 2001 From: Elijah Hamovitz Date: Wed, 2 Sep 2020 15:39:07 -0700 Subject: [PATCH 5/8] Revert "return to using a custom method to check for uniqueness, in order to easily exclude deleted AOs" This reverts commit 25f5b0cc92a2d135a9dba78ae515fafaec76c029. --- dashboard/app/models/authentication_option.rb | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/dashboard/app/models/authentication_option.rb b/dashboard/app/models/authentication_option.rb index 59af9414990e9..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,15 +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 if authentication_id.blank? || deleted_at.present? - - # note that here we are deliberately not querying deleted AOs - others = AuthenticationOption. - where(credential_type: credential_type, authentication_id: authentication_id). - where('id != ?', id) - errors.add(:credential_type, I18n.t('errors.messages.taken')) unless others.empty? - end end From 8b6fdca25ca751a75d57dc4edbc33bc04b5a5995 Mon Sep 17 00:00:00 2001 From: Elijah Hamovitz Date: Wed, 2 Sep 2020 15:54:33 -0700 Subject: [PATCH 6/8] use built-in sequence helper rather than trying to roll our own version --- dashboard/test/factories/factories.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dashboard/test/factories/factories.rb b/dashboard/test/factories/factories.rb index 971299eb13119..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}_#{SecureRandom.uuid}@code.org")} + sequence(:email) {|n| "#{user_type}_#{n}@code.org"} password "00secret" locale 'en-US' sequence(:name) {|n| "User#{n} Codeberg"} From c6d494559a76d1bbbcef94f69796a0374ae5bb2d Mon Sep 17 00:00:00 2001 From: Elijah Hamovitz Date: Thu, 3 Sep 2020 11:45:28 -0700 Subject: [PATCH 7/8] only build test users, don't persist them. For some reason we're getting invalid authentication option errors when we try to persist --- dashboard/test/models/user_test.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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 From 5607c4f2ae5df08760c0a7652c89b7a25960ca23 Mon Sep 17 00:00:00 2001 From: Elijah Hamovitz Date: Fri, 4 Sep 2020 13:49:10 -0700 Subject: [PATCH 8/8] be explicit about authentication ids in test to prevent duplicates --- .../test/lib/user_multi_auth_helper_test.rb | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) 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