Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
16 changes: 13 additions & 3 deletions app/controllers/concerns/two_factor_authenticatable_methods.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,19 @@ def authenticate_user
authenticate_user!(force: true)
end

def handle_second_factor_locked_user(type)
def handle_second_factor_locked_user(type:, context: nil)
analytics.multi_factor_auth_max_attempts
event = PushNotification::MfaLimitAccountLockedEvent.new(user: current_user)
PushNotification::HttpPush.deliver(event)
handle_max_attempts(type + '_login_attempts')

if context
if UserSessionContext.authentication_context?(context)
irs_attempts_api_tracker.mfa_login_rate_limited(type: type)
elsif UserSessionContext.confirmation_context?(context)
irs_attempts_api_tracker.mfa_enroll_rate_limited(type: type)
end
end
end

def handle_too_many_otp_sends
Expand Down Expand Up @@ -108,13 +116,13 @@ def two_factor_authentication_method
# Method will be renamed in the next refactor.
# You can pass in any "type" with a corresponding I18n key in
# two_factor_authentication.invalid_#{type}
def handle_invalid_otp(type: 'otp')
def handle_invalid_otp(type:, context: nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

I am weirdly delighted that we're able to get rid of the hard-coded 'otp' default here. 👏

update_invalid_user

flash.now[:error] = invalid_otp_error(type)

if decorated_user.locked_out?
handle_second_factor_locked_user(type)
handle_second_factor_locked_user(context: context, type: type)
else
render_show_after_invalid
end
Expand All @@ -124,6 +132,8 @@ def invalid_otp_error(type)
case type
when 'otp'
t('two_factor_authentication.invalid_otp')
when 'totp'
t('two_factor_authentication.invalid_otp')
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it's worth calling out for others: the code previously passed in "otp" for both phone-based OTP and TOTP, but we wanted to distinguish between them as distinct events.

This way we pass in extra detail, but don't change the user experience.

when 'personal_key'
t('two_factor_authentication.invalid_personal_key')
when 'piv_cac'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ def handle_invalid_backup_code
flash.now[:error] = t('two_factor_authentication.invalid_backup_code')

if decorated_user.locked_out?
handle_second_factor_locked_user('backup_code')
handle_second_factor_locked_user(context: context, type: 'backup_code')
else
render_show_after_invalid
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ def create
if result.success?
handle_valid_otp
else
handle_invalid_otp
handle_invalid_otp(context: context, type: 'otp')
end
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ def handle_result(result)
generate_new_personal_key_for_verified_users_otherwise_retire_the_key_and_ensure_two_mfa
handle_valid_otp
else
handle_invalid_otp(type: 'personal_key')
handle_invalid_otp(context: context, type: 'personal_key')
end
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ def handle_valid_piv_cac

def handle_invalid_piv_cac
clear_piv_cac_information
handle_invalid_otp(type: 'piv_cac')
handle_invalid_otp(context: context, type: 'piv_cac')
end

# This overrides the method in TwoFactorAuthenticatable so that we
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ def create
if result.success?
handle_valid_otp
else
handle_invalid_otp
handle_invalid_otp(context: context, type: 'totp')
end
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ def locked_reason
t('two_factor_authentication.max_otp_login_attempts_reached')
when 'otp_requests'
t('two_factor_authentication.max_otp_requests_reached')
when 'totp_login_attempts'
t('two_factor_authentication.max_otp_login_attempts_reached')
when 'totp_requests'
t('two_factor_authentication.max_otp_requests_reached')
when 'personal_key_login_attempts'
t('two_factor_authentication.max_personal_key_login_attempts_reached')
when 'piv_cac_login_attempts'
Expand Down
22 changes: 21 additions & 1 deletion app/services/irs_attempts_api/tracker_events.rb
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,16 @@ def mfa_enroll_piv_cac(
)
end

# @param [String] type - the type of multi-factor authentication used
# The user has exceeded the rate limit during enrollment
# and account has been locked
def mfa_enroll_rate_limited(type:)
track_event(
:mfa_enroll_rate_limited,
type: type,
)
end

# Tracks when the user has attempted to enroll the TOTP MFA method to their account
# @param [Boolean] success
def mfa_enroll_totp(success:)
Expand Down Expand Up @@ -158,8 +168,8 @@ def mfa_login_phone_otp_submitted(reauthentication:, success:)
end

# Tracks when the user has attempted to log in with the piv cac MFA method to their account
# @param [String] subject_dn
# @param [Boolean] success
# @param [String] subject_dn
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO I wouldn't fix this unless you need to edit something else in this PR anyway, but I think in the merge conflict this line got moved from above to below success.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, the merge conflict removed the top two lines entirely 😨. I put them back in a commit, but noticed that the params were out of order, so I moved subject_dn below success on purpose, to match the function definition. Was that the right thing to do?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, my bad! Yes, good catch. 👍

# @param [Hash<Symbol,Array<Symbol>>] failure_reason
def mfa_login_piv_cac(
success:,
Expand All @@ -174,6 +184,16 @@ def mfa_login_piv_cac(
)
end

# @param [String] type - the type of multi-factor authentication used
# The user has exceeded the rate limit during verification
# and account has been locked
def mfa_login_rate_limited(type:)
track_event(
:mfa_login_rate_limited,
type: type,
)
end

# Tracks when the user has attempted to log in with the TOTP MFA method to access their account
# @param [Boolean] success
def mfa_login_totp(success:)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,10 @@

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(type: 'backup_code')

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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,9 @@
expect(@irs_attempts_api_tracker).to receive(:mfa_login_phone_otp_submitted).
with({ reauthentication: false, success: false })

expect(@irs_attempts_api_tracker).to receive(:mfa_login_rate_limited).
with(type: 'otp')

post :create, params:
{ code: '12345',
otp_delivery_preference: 'sms' }
Expand Down Expand Up @@ -235,7 +238,7 @@
with('User marked authenticated', authentication_type: :valid_2fa)

expect(@irs_attempts_api_tracker).to receive(:mfa_login_phone_otp_submitted).
with({ reauthentication: false, success: true })
with(reauthentication: false, success: true)

post :create, params: {
code: subject.current_user.reload.direct_otp,
Expand Down Expand Up @@ -380,7 +383,7 @@
controller.user_session[:phone_id] = phone_id

expect(@irs_attempts_api_tracker).to receive(:mfa_enroll_phone_otp_submitted).
with({ success: true })
with(success: true)

post(
:create,
Expand Down Expand Up @@ -410,7 +413,7 @@
context 'user enters an invalid code' do
before do
expect(@irs_attempts_api_tracker).to receive(:mfa_enroll_phone_otp_submitted).
with({ success: false })
with(success: false)

post(
:create,
Expand Down Expand Up @@ -461,6 +464,19 @@
expect(@analytics).to have_received(:track_event).
with('Multi-Factor Authentication Setup', properties)
end

context 'user has exceeded the maximum number of attempts' do
it 'tracks the attempt event' do
allow_any_instance_of(User).to receive(:max_login_attempts?).and_return(true)
sign_in_before_2fa

stub_attempts_tracker
expect(@irs_attempts_api_tracker).to receive(:mfa_enroll_rate_limited).
with(type: 'otp')

post :create, params: { code: '12345', otp_delivery_preference: 'sms' }
end
end
end

context 'user does not include a code parameter' do
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,11 +152,16 @@
}

stub_analytics
stub_attempts_tracker

expect(@analytics).to receive(:track_mfa_submit_event).
with(properties)
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(type: 'personal_key')

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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,9 @@
expect(@analytics).to receive(:track_event).
with('Multi-Factor Authentication: enter PIV CAC visited', attributes)

expect(@irs_attempts_api_tracker).to receive(:mfa_login_rate_limited).
with(type: 'piv_cac')

submit_attributes = {
success: false,
errors: { type: 'user.piv_cac_mismatch' },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,10 @@
with('Multi-Factor Authentication: max attempts reached')
expect(@irs_attempts_api_tracker).to receive(:track_event).
with(:mfa_login_totp, success: false)

expect(@irs_attempts_api_tracker).to receive(:mfa_login_rate_limited).
with(type: 'totp')

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

Expand Down