From 89ddf77000e9118515c6673e575b217cd2705f33 Mon Sep 17 00:00:00 2001 From: Mehal Shah Date: Tue, 11 Sep 2018 09:28:02 -0700 Subject: [PATCH] Revert "Implement silent takeover for Google Classroom imported students" --- .../omniauth_callbacks_controller.rb | 50 +++++-------------- dashboard/app/helpers/users_helper.rb | 50 +++++++------------ .../omniauth_callbacks_controller_test.rb | 39 --------------- dashboard/test/factories/factories.rb | 22 +------- 4 files changed, 31 insertions(+), 130 deletions(-) diff --git a/dashboard/app/controllers/omniauth_callbacks_controller.rb b/dashboard/app/controllers/omniauth_callbacks_controller.rb index e46b096118e3d..f3563d417a887 100644 --- a/dashboard/app/controllers/omniauth_callbacks_controller.rb +++ b/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? diff --git a/dashboard/app/helpers/users_helper.rb b/dashboard/app/helpers/users_helper.rb index 25e56538c927d..ee7fe0714b2e0 100644 --- a/dashboard/app/helpers/users_helper.rb +++ b/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, diff --git a/dashboard/test/controllers/omniauth_callbacks_controller_test.rb b/dashboard/test/controllers/omniauth_callbacks_controller_test.rb index bff11daccf17c..efdafd786e194 100644 --- a/dashboard/test/controllers/omniauth_callbacks_controller_test.rb +++ b/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' diff --git a/dashboard/test/factories/factories.rb b/dashboard/test/factories/factories.rb index e5d5994f06495..119a494a21a35 100644 --- a/dashboard/test/factories/factories.rb +++ b/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',