Skip to content

Remove track_mfa_submit analytics pass-through method #10679

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

Merged
aduth merged 1 commit into from
May 23, 2024
Merged

Remove track_mfa_submit analytics pass-through method #10679

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions app/controllers/two_factor_authentication/backup_code_verification_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,7 @@ def show
def create
@backup_code_form = BackupCodeVerificationForm.new(current_user)
result = @backup_code_form.submit(backup_code_params)
analytics.track_mfa_submit_event(
result.to_h.merge(new_device: new_device?),
)
analytics.multi_factor_auth(**result.to_h.merge(new_device: new_device?))
irs_attempts_api_tracker.mfa_login_backup_code(success: result.success?)
handle_result(result)
end
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/two_factor_authentication/otp_verification_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ def post_analytics(result)
properties = result.to_h.merge(analytics_properties, new_device: new_device?)
analytics.multi_factor_auth_setup(**properties) if context == 'confirmation'

analytics.track_mfa_submit_event(properties)
analytics.multi_factor_auth(**properties)

if UserSessionContext.reauthentication_context?(context)
irs_attempts_api_tracker.mfa_login_phone_otp_submitted(
Expand Down
5 changes: 4 additions & 1 deletion app/controllers/two_factor_authentication/personal_key_verification_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,10 @@ def track_analytics(result)
new_device: new_device?,
)

analytics.track_mfa_submit_event(analytics_hash)
analytics.multi_factor_auth(
**analytics_hash,
pii_like_keypaths: [[:errors, :personal_key], [:error_details, :personal_key]],
)
end

def check_personal_key_enabled
Expand Down
4 changes: 1 addition & 3 deletions app/controllers/two_factor_authentication/piv_cac_verification_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,7 @@ def redirect_to_piv_cac_service

def process_token
result = piv_cac_verification_form.submit
analytics.track_mfa_submit_event(
result.to_h.merge(analytics_properties),
)
analytics.multi_factor_auth(**result.to_h.merge(analytics_properties))
irs_attempts_api_tracker.mfa_login_piv_cac(
success: result.success?,
subject_dn: piv_cac_verification_form.x509_dn,
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/two_factor_authentication/totp_verification_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ def show

def create
result = TotpVerificationForm.new(current_user, params.require(:code).strip).submit
analytics.track_mfa_submit_event(result.to_h.merge(new_device: new_device?))
analytics.multi_factor_auth(**result.to_h.merge(new_device: new_device?))
irs_attempts_api_tracker.mfa_login_totp(success: result.success?)

if result.success?
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/two_factor_authentication/webauthn_verification_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ def show

def confirm
result = form.submit
analytics.track_mfa_submit_event(
analytics.multi_factor_auth(
**result.to_h,
**analytics_properties,
multi_factor_auth_method_created_at:
Expand Down
7 changes: 0 additions & 7 deletions app/services/analytics.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,13 +52,6 @@ def first_event_this_session?
session[:first_event]
end

def track_mfa_submit_event(attributes)
multi_factor_auth(
**attributes,
pii_like_keypaths: [[:errors, :personal_key], [:error_details, :personal_key]],
)
end

def request_attributes
attributes = {
user_ip: request.remote_ip,
Expand Down
41 changes: 23 additions & 18 deletions app/services/analytics_events.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3730,6 +3730,7 @@ def logout_initiated(

# @param [Boolean] success Whether authentication was successful
# @param [Hash] errors Authentication error reasons, if unsuccessful
# @param [Hash] error_details Details for error that occurred in unsuccessful submission
# @param [String] context
# @param [Boolean] new_device
# @param [String] multi_factor_auth_method
Expand All @@ -3748,6 +3749,7 @@ def logout_initiated(
def multi_factor_auth(
success:,
errors: nil,
error_details: nil,
context: nil,
new_device: nil,
multi_factor_auth_method: nil,
Expand All @@ -3767,24 +3769,27 @@ def multi_factor_auth(
)
track_event(
'Multi-Factor Authentication',
success: success,
errors: errors,
context: context,
new_device: new_device,
multi_factor_auth_method: multi_factor_auth_method,
multi_factor_auth_method_created_at: multi_factor_auth_method_created_at,
auth_app_configuration_id: auth_app_configuration_id,
piv_cac_configuration_id: piv_cac_configuration_id,
key_id: key_id,
webauthn_configuration_id: webauthn_configuration_id,
confirmation_for_add_phone: confirmation_for_add_phone,
phone_configuration_id: phone_configuration_id,
pii_like_keypaths: pii_like_keypaths,
area_code: area_code,
country_code: country_code,
phone_fingerprint: phone_fingerprint,
frontend_error:,
**extra,
{
success: success,
errors: errors,
error_details: error_details,
context: context,
new_device: new_device,
multi_factor_auth_method: multi_factor_auth_method,
multi_factor_auth_method_created_at: multi_factor_auth_method_created_at,
auth_app_configuration_id: auth_app_configuration_id,
piv_cac_configuration_id: piv_cac_configuration_id,
key_id: key_id,
webauthn_configuration_id: webauthn_configuration_id,
confirmation_for_add_phone: confirmation_for_add_phone,
phone_configuration_id: phone_configuration_id,
pii_like_keypaths: pii_like_keypaths,
area_code: area_code,
country_code: country_code,
phone_fingerprint: phone_fingerprint,
frontend_error:,
**extra,
}.compact,

@aduth aduth May 22, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason for this change is that have_logged_event is much stricter about comparing logged properties, and was failing on method-specific properties like auth_app_configuration_id being nil when logged in another method's controller.

I still think we should probably do an automatic .compact on all properties logged, but that's a future problem to address.

)
end

Expand Down
74 changes: 36 additions & 38 deletions spec/controllers/two_factor_authentication/backup_code_verification_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,16 +27,6 @@
sign_in_before_2fa(user)
stub_analytics
stub_attempts_tracker
analytics_hash = {
success: true,
errors: {},
multi_factor_auth_method: 'backup_code',
multi_factor_auth_method_created_at: Time.zone.now.strftime('%s%L'),
new_device: true,
}

expect(@analytics).to receive(:track_mfa_submit_event).
with(analytics_hash)

expect(@irs_attempts_api_tracker).to receive(:track_event).
with(:mfa_login_backup_code, success: true)
Expand All @@ -47,6 +37,15 @@

post :create, params: payload

expect(@analytics).to have_logged_event(
'Multi-Factor Authentication',
success: true,
errors: {},
multi_factor_auth_method: 'backup_code',
multi_factor_auth_method_created_at: Time.zone.now.strftime('%s%L'),
new_device: true,
)

expect(subject.user_session[:auth_events]).to eq(
[
auth_method: TwoFactorAuthenticatable::AuthMethod::BACKUP_CODE,
Expand Down Expand Up @@ -93,22 +92,23 @@
stub_analytics
stub_attempts_tracker

expect(@analytics).to receive(:track_mfa_submit_event).
with({
success: true,
errors: {},
multi_factor_auth_method: 'backup_code',
multi_factor_auth_method_created_at: Time.zone.now.strftime('%s%L'),
new_device: true,
})

expect(@irs_attempts_api_tracker).to receive(:track_event).
with(:mfa_login_backup_code, success: true)

expect(@analytics).to receive(:track_event).
with('User marked authenticated', authentication_type: :valid_2fa)

post :create, params: payload

expect(@analytics).to have_logged_event(
'Multi-Factor Authentication',
success: true,
errors: {},
multi_factor_auth_method: 'backup_code',
multi_factor_auth_method_created_at: Time.zone.now.strftime('%s%L'),
new_device: true,
)
expect(@analytics).to have_logged_event(
'User marked authenticated',
authentication_type: :valid_2fa,
)
end
end
end
Expand All @@ -122,10 +122,12 @@
stub_analytics
stub_sign_in_before_2fa(user)

expect(@analytics).to receive(:track_mfa_submit_event).
with(hash_including(new_device: false))

post :create, params: payload

expect(@analytics).to have_logged_event(
'Multi-Factor Authentication',
hash_including(new_device: false),
)
end
end

Expand Down Expand Up @@ -171,33 +173,29 @@
user.second_factor_attempts_count =
IdentityConfig.store.login_otp_confirmation_max_attempts - 1
user.save
properties = {
success: false,
errors: {},
multi_factor_auth_method: 'backup_code',
multi_factor_auth_method_created_at: nil,
new_device: true,
}

stub_analytics
stub_attempts_tracker

expect(@analytics).to receive(:track_mfa_submit_event).
with(properties)

expect(@irs_attempts_api_tracker).to receive(:track_event).
with(:mfa_login_backup_code, success: false)

expect(@analytics).to receive(:track_event).
with('Multi-Factor Authentication: max attempts reached')

expect(@irs_attempts_api_tracker).to receive(:mfa_login_rate_limited).
with(mfa_device_type: 'backup_code')

expect(PushNotification::HttpPush).to receive(:deliver).
with(PushNotification::MfaLimitAccountLockedEvent.new(user: subject.current_user))

post :create, params: payload

expect(@analytics).to have_logged_event(
'Multi-Factor Authentication',
success: false,
errors: {},
multi_factor_auth_method: 'backup_code',
new_device: true,
)
expect(@analytics).to have_logged_event('Multi-Factor Authentication: max attempts reached')
end

it 'records unsuccessful 2fa event' do
Expand Down
Loading