Merge pull request #27657 from code-dot-org/section-ownership-transfe…

…r-on-takeover

Accounts: Account takeover edge cases

dashboard/app/controllers/omniauth_callbacks_controller.rb

@@ -65,8 +65,9 @@ def connect_provider
     end
 
     # Credential is already held by another user with activity
+    #   Or a student is trying to take over a teacher account
     # Display an error explaining that the credential is already in use.
-    if existing_credential_holder&.has_activity?
+    if existing_credential_holder&.has_activity? || (current_user.student? && existing_credential_holder&.teacher?)
       flash.alert = I18n.t('auth.already_in_use', provider: I18n.t("auth.#{provider}"))
       return redirect_to edit_user_registration_path
     end

dashboard/app/helpers/users_helper.rb

@@ -13,7 +13,8 @@ module UsersHelper
   ACCT_TAKEOVER_OAUTH_TOKEN = 'clever_takeover_token'
   ACCT_TAKEOVER_FORCE_TAKEOVER = 'force_clever_takeover'
 
-  # Move followed sections from source_user to destination_user and destroy source_user.
+  # Move section membership and (for teachers) section ownership from source_user to
+  # destination_user and destroy source_user.
   # Returns a boolean - true if all steps were successful, false otherwise.
   def move_sections_and_destroy_source_user(source_user:, destination_user:, takeover_type:, provider:)
     # No-op if source_user is nil
@@ -36,9 +37,14 @@ def move_sections_and_destroy_source_user(source_user:, destination_user:, takeo
 
     ActiveRecord::Base.transaction do
       # Move over sections that source_user follows
-      if destination_user.student?
-        Follower.where(student_user_id: source_user.id).each do |followed|
-          followed.update!(student_user_id: destination_user.id)
+      Follower.where(student_user_id: source_user.id).each do |followed|
+        followed.update!(student_user_id: destination_user.id)
+      end
+
+      # If both users are teachers, transfer ownership of sections
+      if source_user.teacher? && destination_user.teacher?
+        Section.where(user: source_user).each do |owned_section|
+          owned_section.update! user: destination_user
         end
       end
 

dashboard/test/controllers/omniauth_callbacks_controller_test.rb

@@ -1372,6 +1372,85 @@ class OmniauthCallbacksControllerTest < ActionController::TestCase
     refute_includes section.students, other_user
   end
 
+  test "connect_provider: Teacher takeover of student transfers section enrollment" do
+    # Given I am a teacher
+    user = create :teacher
+
+    # And there exists a student
+    #   having credential X
+    #   and has no activity
+    other_user = create :student
+    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: Successful takeover transfers ownership of sections" do
+    # Given I am a teacher
+    user = create :teacher
+
+    # And there exists another teacher
+    #   having credential X
+    #   and who owns section Y
+    #   and has no activity
+    other_user = create :teacher
+    credential = create :google_authentication_option, user: other_user
+    section = create :section, teacher: other_user
+    refute other_user.has_activity?
+
+    # When I add credential X
+    link_credential user,
+      type: credential.credential_type,
+      id: credential.authentication_id
+
+    # Then I should own section Y instead of the other user
+    section.reload
+    refute section.deleted?
+    assert_equal user, section.teacher
+    refute_equal other_user, section.teacher
+  end
+
+  test "connect_provider: Refuses to link credential if student is taking over teacher account" do
+    # Given I am a student
+    user = create :student
+
+    # And there exists a teacher
+    #   having credential X
+    #   and has no activity
+    other_user = create :teacher
+    credential = create :google_authentication_option, user: other_user
+    refute 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_equal 1, user.authentication_options.count
+
+    # 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: Refuses to link credential if there is an account with matching credential that has activity" do
     user = create :user