code-dot-org · Hamms · Sep 8, 2020 · Sep 1, 2020 · Sep 2, 2020 · Sep 2, 2020
diff --git 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
@@ -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
@@ -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
@@ -29,15 +29,15 @@ 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

  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
@@ -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