Merge pull request code-dot-org#24870 from code-dot-org/multi-auth-reverse-takeover

Handle linking credential already claimed by another account

Author: islemaster

dashboard/app/controllers/omniauth_callbacks_controller.rb

@@ -27,6 +27,31 @@ def connect_provider
     provider = auth_hash.provider.to_s
     return head(:bad_request) unless AuthenticationOption::OAUTH_CREDENTIAL_TYPES.include? provider
 
+    existing_credential_holder = User.find_by_credential type: provider, id: auth_hash.uid
+
+    # Credential is already held by the current user
+    # Notify of no-op.
+    if existing_credential_holder&.==(current_user)
+      flash.notice = I18n.t('auth.already_linked', provider: I18n.t("auth.#{provider}"))
+      return redirect_to edit_user_registration_path
+    end
+
+    # Credential is already held by another user with activity
+    # Display an error explaining that the credential is already in use.
+    if existing_credential_holder&.has_activity?
+      flash.alert = I18n.t('auth.already_in_use', provider: I18n.t("auth.#{provider}"))
+      return redirect_to edit_user_registration_path
+    end
+
+    # Credential is already held by an unused account.
+    # Take over the unused account.
+    if existing_credential_holder
+      move_sections_and_destroy_source_user \
+        source_user: existing_credential_holder,
+        destination_user: current_user,
+        takeover_type: 'connect_provider'
+    end
+
     # TODO: some of this won't work right for non-Google providers, because info comes in differently
     new_data = nil
     if auth_hash.credentials && (auth_hash.credentials.token || auth_hash.credentials.expires_at || auth_hash.credentials.refresh_token)

dashboard/config/locales/en.yml

@@ -117,6 +117,8 @@ en:
     not_linked: "Sorry, your Code.org account is not linked to your %{provider}. You'll have to sign in by typing your password below."
     use_clever: "Sorry, your Code.org account is not linked to your %{provider}. Please sign in through your Clever Portal instead."
     unable_to_connect_provider: "Unable to connect your Code.org account to your %{provider}. Please try again."
+    already_in_use: "Your %{provider} is already connected to another Code.org account."
+    already_linked: "Your %{provider} is already linked to your Code.org account."
   signup_form:
     student_count: '%{count} students have already signed up.'
     teacher_count: '%{count} teachers have already signed up.'

dashboard/test/controllers/omniauth_callbacks_controller_test.rb

@@ -822,6 +822,106 @@ class OmniauthCallbacksControllerTest < ActionController::TestCase
     end
   end
 
+  test "connect_provider: Performs takeover of an account with matching credential that has no activity" do
+    user = create :user, :multi_auth_migrated
+
+    # Given there exists another user
+    #   having credential X
+    #   and having no activity
+    other_user = create :user, :multi_auth_migrated
+    credential = create :google_authentication_option, user: other_user
+    refute other_user.has_activity?
+
+    # When I attempt to add credential X
+    auth = link_credential user,
+      type: credential.credential_type,
+      id: credential.authentication_id
+
+    # Then I should successfully add credential X
+    user.reload
+    assert_redirected_to 'http://test.host/users/edit'
+    assert_auth_option(user, auth)
+
+    # And the other user should be destroyed
+    other_user.reload
+    assert other_user.deleted?
+  end
+
+  test "connect_provider: Successful takeover also enrolls in replaced user's sections" do
+    user = create :user, :multi_auth_migrated
+
+    # Given there exists another user
+    #   having credential X
+    #   and having no activity
+    #   and enrolled in section Y
+    other_user = create :user, :multi_auth_migrated
+    credential = create :google_authentication_option, user: other_user
+    refute other_user.has_activity?
+    section = create :section
+    section.students << other_user
+
+    # When I add credential X
+    link_credential user,
+      type: credential.credential_type,
+      id: credential.authentication_id
+
+    # Then I should be enrolled in section Y instead of the other user
+    section.reload
+    assert_includes section.students, user
+    refute_includes section.students, other_user
+  end
+
+  test "connect_provider: Refuses to link credential if there is an account with matching credential that has activity" do
+    user = create :user, :multi_auth_migrated
+
+    # Given there exists another user
+    #   having credential X
+    #   and having activity
+    other_user = create :user, :multi_auth_migrated
+    credential = create :google_authentication_option, user: other_user
+    create :user_level, user: other_user, best_result: ActivityConstants::MINIMUM_PASS_RESULT
+    assert other_user.has_activity?
+
+    # When I attempt to add credential X
+    link_credential user,
+      type: credential.credential_type,
+      id: credential.authentication_id
+
+    # Then the other user should not be destroyed
+    other_user.reload
+    refute other_user.deleted?
+
+    # And I should fail to add credential X
+    user.reload
+    assert_empty user.authentication_options
+
+    # And receive a helpful error message about the credential already being in use.
+    assert_redirected_to 'http://test.host/users/edit'
+    expected_error = I18n.t('auth.already_in_use', provider: I18n.t("auth.google_oauth2"))
+    assert_equal expected_error, flash.alert
+  end
+
+  test "connect_provider: Presents no-op message if the provided credentials are already linked to user's account" do
+    # Given the current user already has credential X
+    user = create :user, :multi_auth_migrated
+    credential = create :google_authentication_option, user: user
+    assert_equal 1, user.authentication_options.count
+
+    # When I attempt to add credential X
+    link_credential user,
+      type: credential.credential_type,
+      id: credential.authentication_id
+
+    # Then I should have the same authentication options
+    user.reload
+    assert_equal 1, user.authentication_options.count
+
+    # And receive a friendly notice about already having the credential
+    assert_redirected_to 'http://test.host/users/edit'
+    expected_notice = I18n.t('auth.already_linked', provider: I18n.t("auth.google_oauth2"))
+    assert_equal expected_notice, flash.notice
+  end
+
   private
 
   def set_oauth_takeover_session_variables(provider, user)
@@ -831,6 +931,17 @@ def set_oauth_takeover_session_variables(provider, user)
     @request.session[ACCT_TAKEOVER_OAUTH_TOKEN] = '54321'
   end
 
+  # Try to link a credential to the provided user
+  # @return [OmniAuth::AuthHash] the auth hash, useful for validating
+  #   linked credentials with assert_auth_option
+  def link_credential(user, type:, id:)
+    auth = generate_auth_user_hash(provider: type, uid: id)
+    @request.env['omniauth.auth'] = auth
+    setup_should_connect_provider(user, 2.days.from_now)
+    get :google_oauth2
+    auth
+  end
+
   def generate_auth_user_hash(args)
     OmniAuth::AuthHash.new(
       uid: args[:uid] || '1111',
@@ -863,7 +974,7 @@ def assert_auth_option(user, oauth_hash)
       user: user,
       hashed_email: User.hash_email(oauth_hash.info.email),
       credential_type: oauth_hash.provider,
-      authentication_id: user.uid,
+      authentication_id: oauth_hash.uid,
       data: {
         oauth_token: oauth_hash.credentials.token,
         oauth_token_expiration: oauth_hash.credentials.expires_at,