From 008dab1b0592bc17c40add9f11824b715b14c9c8 Mon Sep 17 00:00:00 2001 From: Maddie Kasula Date: Fri, 14 Sep 2018 16:21:54 -0700 Subject: [PATCH 1/3] Move failed account takeover logging to redshift, include error --- dashboard/app/helpers/users_helper.rb | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/dashboard/app/helpers/users_helper.rb b/dashboard/app/helpers/users_helper.rb index 8eca47c4678d0..0213aaf570d20 100644 --- a/dashboard/app/helpers/users_helper.rb +++ b/dashboard/app/helpers/users_helper.rb @@ -19,13 +19,16 @@ def move_sections_and_destroy_source_user(source_user:, destination_user:, takeo # No-op if source_user is nil return true unless source_user.present? + firehose_params = { + source_user: source_user, + destination_user: destination_user, + type: takeover_type, + provider: destination_user.provider, + } + 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}" - ) + # We don't want to destroy an account with progress. Log to Redshift and return false. + log_account_takeover_to_firehose(firehose_params.merge(error: "Attempted takeover for account with progress.")) return false end @@ -40,12 +43,7 @@ def move_sections_and_destroy_source_user(source_user:, destination_user:, takeo source_user.destroy! end - log_account_takeover_to_firehose( - source_user: source_user, - destination_user: destination_user, - type: takeover_type, - provider: destination_user.provider - ) + log_account_takeover_to_firehose(firehose_params) true rescue false @@ -94,7 +92,7 @@ def check_and_apply_oauth_takeover(user) end end - def log_account_takeover_to_firehose(source_user:, destination_user:, type:, provider:) + def log_account_takeover_to_firehose(source_user:, destination_user:, type:, provider:, error: nil) FirehoseClient.instance.put_record( study: 'user-soft-delete-audit', event: "#{type}-account-takeover", # Silent or OAuth takeover @@ -103,6 +101,7 @@ def log_account_takeover_to_firehose(source_user:, destination_user:, type:, pro data_string: provider, # OAuth provider data_json: { user_type: destination_user.user_type, + error: error, }.to_json ) end From 7648abb13ea5f3294e56561870a618347f25c7d7 Mon Sep 17 00:00:00 2001 From: Maddie Kasula Date: Fri, 14 Sep 2018 16:39:35 -0700 Subject: [PATCH 2/3] Log separate redshift event for failed account takeovers --- dashboard/app/helpers/users_helper.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/dashboard/app/helpers/users_helper.rb b/dashboard/app/helpers/users_helper.rb index 0213aaf570d20..54981da4375a4 100644 --- a/dashboard/app/helpers/users_helper.rb +++ b/dashboard/app/helpers/users_helper.rb @@ -28,7 +28,9 @@ def move_sections_and_destroy_source_user(source_user:, destination_user:, takeo if source_user.has_activity? # We don't want to destroy an account with progress. Log to Redshift and return false. - log_account_takeover_to_firehose(firehose_params.merge(error: "Attempted takeover for account with progress.")) + firehose_params[:type] = "cancelled-#{takeover_type}" + firehose_params[:error] = "Attempted takeover for account with progress." + log_account_takeover_to_firehose(firehose_params) return false end From 2f379b945d84b5032e767953b28cd6c6792ddba1 Mon Sep 17 00:00:00 2001 From: Maddie Kasula Date: Mon, 17 Sep 2018 09:47:53 -0700 Subject: [PATCH 3/3] Update unit test to expect FirehoseClient called rather than Honeybadger --- .../test/controllers/omniauth_callbacks_controller_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dashboard/test/controllers/omniauth_callbacks_controller_test.rb b/dashboard/test/controllers/omniauth_callbacks_controller_test.rb index 6548a8463bc8e..d207dab9e854b 100644 --- a/dashboard/test/controllers/omniauth_callbacks_controller_test.rb +++ b/dashboard/test/controllers/omniauth_callbacks_controller_test.rb @@ -449,7 +449,7 @@ class OmniauthCallbacksControllerTest < ActionController::TestCase assert oauth_student.has_activity? - Honeybadger.expects(:notify).at_least_once + FirehoseClient.any_instance.expects(:put_record).at_least_once set_oauth_takeover_session_variables(provider, oauth_student) check_and_apply_oauth_takeover(student)