Merge pull request #24725 from code-dot-org/revert-24658-google-roste…

…r-sync-fix

Revert "Implement silent takeover for Google Classroom imported students"

dashboard/app/controllers/omniauth_callbacks_controller.rb

@@ -81,13 +81,12 @@ def login
       redirect_to '/home?open=rosterDialog'
     elsif User::OAUTH_PROVIDERS_UNTRUSTED_EMAIL.include?(provider) && @user.persisted?
       handle_untrusted_email_signin(@user, provider)
-    elsif allows_silent_takeover(@user, auth_hash) || google_classroom_student_takeover(@user)
-      silent_takeover(@user, auth_hash)
-      sign_in_user
     elsif @user.persisted?
       # If email is already taken, persisted? will be false because of a validation failure
       check_and_apply_oauth_takeover(@user)
       sign_in_user
+    elsif allows_silent_takeover(@user, auth_hash)
+      silent_takeover(@user, auth_hash)
     elsif (looked_up_user = User.find_by_email_or_hashed_email(@user.email))
       # Note that @user.email is populated by User.from_omniauth even for students
       if looked_up_user.provider == 'clever'
@@ -187,28 +186,14 @@ def just_authorized_google_classroom(user, params)
       scopes.include?('classroom.rosters.readonly')
   end
 
-  def google_classroom_student_takeover(user)
-    # Google Classroom does not provide student email addresses, so we want to perform
-    # silent takeover on these accounts, but *only if* the student hasn't made progress
-    # with the account created during the Google Classroom import.
-    user.persisted? && user.google_classroom_student? &&
-      user.email.blank? && user.hashed_email.blank? &&
-      !user.has_activity?
-  end
-
   def silent_takeover(oauth_user, auth_hash)
     # Copy oauth details to primary account
-    lookup_email = oauth_user.email.presence || auth_hash.info.email
-    @user = User.find_by_email_or_hashed_email(lookup_email)
-    return unless @user.present?
-    if google_classroom_student_takeover(oauth_user)
-      return unless move_sections_and_destroy_source_user(oauth_user, @user)
-    end
-
+    @user = User.find_by_email_or_hashed_email(oauth_user.email)
     if @user.migrated?
       success = AuthenticationOption.create(
         user: @user,
-        email: lookup_email,
+        email: oauth_user.email,
+        hashed_email: oauth_user.hashed_email,
         credential_type: auth_hash.provider.to_s,
         authentication_id: auth_hash.uid,
         data: {
@@ -221,25 +206,17 @@ def silent_takeover(oauth_user, auth_hash)
         # This should never happen if other logic is working correctly, so notify
         Honeybadger.notify(
           error_class: 'Failed to create AuthenticationOption during silent takeover',
-          error_message: "Could not create AuthenticationOption during silent takeover for user with email #{lookup_email}"
+          error_message: "Could not create AuthenticationOption during silent takeover for user with email #{oauth_user.email}"
         )
       end
     else
-      success = @user.update(
-        provider: auth_hash.provider.to_s,
-        uid: auth_hash.uid,
-        oauth_token: auth_hash.credentials&.token,
-        oauth_token_expiration: auth_hash.credentials&.expires_at,
-        oauth_refresh_token: auth_hash.credentials&.refresh_token
-      )
-      unless success
-        # This should never happen if other logic is working correctly, so notify
-        Honeybadger.notify(
-          error_class: 'Failed to update User during silent takeover',
-          error_message: "Could not update user during silent takeover for user with email #{lookup_email}"
-        )
-      end
+      @user.provider = oauth_user.provider
+      @user.uid = oauth_user.uid
+      @user.oauth_refresh_token = oauth_user.oauth_refresh_token
+      @user.oauth_token = oauth_user.oauth_token
+      @user.oauth_token_expiration = oauth_user.oauth_token_expiration
     end
+    sign_in_user
   end
 
   def sign_in_user
@@ -251,8 +228,7 @@ def allows_silent_takeover(oauth_user, auth_hash)
     allow_takeover = auth_hash.provider.present?
     allow_takeover &= AuthenticationOption::SILENT_TAKEOVER_CREDENTIAL_TYPES.include?(auth_hash.provider.to_s)
     lookup_user = User.find_by_email_or_hashed_email(oauth_user.email)
-
-    allow_takeover && lookup_user && !oauth_user.persisted?
+    allow_takeover && lookup_user
   end
 
   def should_connect_provider?

dashboard/app/helpers/users_helper.rb

@@ -12,37 +12,6 @@ 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.
-  # Returns a boolean - true if all steps were successful, false otherwise.
-  def move_sections_and_destroy_source_user(source_user, destination_user)
-    # No-op if source_user is nil
-    return true unless source_user.present?
-
-    if source_user.has_activity?
-      # We don't want to destroy an account with progress.
-      # In theory this should not happen, so we log a Honeybadger error and return.
-      Honeybadger.notify(
-        error_class: 'Oauth takeover called for user with progress',
-        errors_message: "Attempted takeover for account with progress. Cancelling takeover of account with id #{source_user.id} by id #{destination_user.id}"
-      )
-      return false
-    end
-
-    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)
-        end
-      end
-
-      source_user.destroy!
-      true
-    end
-  rescue
-    false
-  end
-
   # If Clever takeover flags are present, the current account (user) is the one that the person just
   # logged into (to prove ownership), and all the Clever details are migrated over, including sections.
   def check_and_apply_oauth_takeover(user)
@@ -53,9 +22,24 @@ def check_and_apply_oauth_takeover(user)
       clear_takeover_session_variables
 
       existing_account = User.find_by_credential(type: provider, id: uid)
-      # No-op if move_sections_and_destroy_source_user fails
-      return unless move_sections_and_destroy_source_user(existing_account, user)
+      if existing_account.has_activity?
+        # We don't want to destroy an account with progress.
+        # In theory this should not happen, so we log a Honeybadger error and return.
+        Honeybadger.notify(
+          error_class: 'Oauth takeover called for user with progress',
+          error_message: "Attempted to take over account with id #{existing_account.id}, which has activity"
+        )
+        return
+      end
+
+      # Move over sections that students follow
+      if user.student? && existing_account
+        Follower.where(student_user_id: existing_account.id).each do |follower|
+          follower.update(student_user_id: user.id)
+        end
+      end
 
+      existing_account.destroy! if existing_account
       if user.migrated?
         success = user.add_credential(
           type: provider,

dashboard/test/controllers/omniauth_callbacks_controller_test.rb

@@ -463,25 +463,6 @@ class OmniauthCallbacksControllerTest < ActionController::TestCase
     assert_equal user.uid, uid
   end
 
-  test 'login: google_oauth2 silently takes over unmigrated Google Classroom student with matching email' do
-    email = 'test@foo.xyz'
-    uid = '654321'
-    user = create(:student, email: email)
-    google_classroom_student = create(:student, :imported_from_google_classroom, uid: uid)
-    google_classroom_section = google_classroom_student.sections_as_student.find {|s| s.login_type == Section::LOGIN_TYPE_GOOGLE_CLASSROOM}
-    auth = generate_auth_user_hash(provider: 'google_oauth2', uid: uid, user_type: User::TYPE_STUDENT, email: email)
-    @request.env['omniauth.auth'] = auth
-    @request.env['omniauth.params'] = {}
-
-    assert_destroys(User) do
-      get :google_oauth2
-    end
-    user.reload
-    assert_equal 'google_oauth2', user.provider
-    assert_equal user.uid, uid
-    assert_equal [google_classroom_section&.id], user.sections_as_student.pluck(:id)
-  end
-
   test 'login: google_oauth2 silently takes over unmigrated teacher with matching email' do
     email = 'test@foo.xyz'
     uid = '654321'
@@ -513,26 +494,6 @@ class OmniauthCallbacksControllerTest < ActionController::TestCase
     assert found_google
   end
 
-  test 'login: google_oauth2 silently takes over migrated Google Classroom student with matching email' do
-    email = 'test@foo.xyz'
-    uid = '654321'
-    user = create(:student, :with_migrated_email_authentication_option, email: email)
-    google_classroom_student = create(:student, :migrated_imported_from_google_classroom, uid: uid)
-    google_classroom_section = google_classroom_student.sections_as_student.find {|s| s.login_type == Section::LOGIN_TYPE_GOOGLE_CLASSROOM}
-    auth = generate_auth_user_hash(provider: 'google_oauth2', uid: uid, user_type: User::TYPE_STUDENT, email: email)
-    @request.env['omniauth.auth'] = auth
-    @request.env['omniauth.params'] = {}
-
-    assert_destroys(User) do
-      get :google_oauth2
-    end
-    user.reload
-    assert_equal 'migrated', user.provider
-    found_google = user.authentication_options.any? {|auth_option| auth_option.credential_type == AuthenticationOption::GOOGLE}
-    assert found_google
-    assert [google_classroom_section&.id], user.sections_as_student.pluck(:id)
-  end
-
   test 'login: google_oauth2 silently adds authentication_option to migrated teacher with matching email' do
     email = 'test@foo.xyz'
     uid = '654321'

dashboard/test/factories/factories.rb

@@ -252,26 +252,6 @@
         end
       end
 
-      trait :imported_from_google_classroom do
-        unmigrated_google_sso
-        after(:create) do |user|
-          user.update!(email: '', hashed_email: '')
-          section = create :section, login_type: Section::LOGIN_TYPE_GOOGLE_CLASSROOM
-          create :follower, student_user: user, section: section
-          user.reload
-        end
-      end
-
-      trait :migrated_imported_from_google_classroom do
-        with_migrated_google_authentication_option
-        after(:create) do |user|
-          user.primary_contact_info.update!(email: '', hashed_email: '')
-          section = create :section, login_type: Section::LOGIN_TYPE_GOOGLE_CLASSROOM
-          create :follower, student_user: user, section: section
-          user.reload
-        end
-      end
-
       trait :without_email do
         email ''
         hashed_email nil
@@ -365,7 +345,7 @@
           email: user.email,
           hashed_email: user.hashed_email,
           credential_type: AuthenticationOption::GOOGLE,
-          authentication_id: user.uid,
+          authentication_id: 'abcd123',
           data: {
             oauth_token: 'some-google-token',
             oauth_refresh_token: 'some-google-refresh-token',