Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Move failed account takeover logging to redshift, include error #24817

Merged
merged 3 commits into from Sep 17, 2018
Merged

Move failed account takeover logging to redshift, include error #24817

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
008dab1
Move failed account takeover logging to redshift, include error
maddiedierker Sep 14, 2018
7648abb
Log separate redshift event for failed account takeovers
maddiedierker Sep 14, 2018
2f379b9
Update unit test to expect FirehoseClient called rather than Honeybadger
maddiedierker Sep 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 14 additions & 13 deletions dashboard/app/helpers/users_helper.rb
View file
Open in desktop
Expand Up @@ -19,13 +19,18 @@ 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.
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

Expand All @@ -40,12 +45,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
Expand Down Expand Up @@ -94,7 +94,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
Expand All @@ -103,6 +103,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
Expand Down
2 changes: 1 addition & 1 deletion dashboard/test/controllers/omniauth_callbacks_controller_test.rb
View file
Open in desktop
Expand Up @@ -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)
Expand Down