Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
55 commits
Select commit Hold shift + click to select a range
6c05942
changelog: Internal, MFA setup, Add attempt count to MFA setup analyt…
kevinsmaster5 Sep 24, 2024
ba3cd02
add piv attempts, use common name in analytics
kevinsmaster5 Sep 25, 2024
2c49e86
add logging to otp and totp setup
kevinsmaster5 Sep 26, 2024
cae6055
reset attempts count on success
kevinsmaster5 Sep 26, 2024
bd3c92c
add totp reset and refactor reset on webauthn
kevinsmaster5 Sep 26, 2024
7d9c411
update otp specs
kevinsmaster5 Sep 26, 2024
60e800d
update totp specs
kevinsmaster5 Sep 26, 2024
32c271b
update piv_cac_setup specs
kevinsmaster5 Sep 26, 2024
7af6595
add otp spec that confirms incremented mfa_attempts analytics log
kevinsmaster5 Sep 27, 2024
1940b4a
add specs for piv and totp
kevinsmaster5 Sep 27, 2024
952830d
refactor slightly session key setter
kevinsmaster5 Sep 30, 2024
54eb134
add testing for attempts on webauthn setup
kevinsmaster5 Sep 30, 2024
155cc0a
refactor pulling commonly use function into mfa setup concern
kevinsmaster5 Oct 3, 2024
90b0d15
move mfa attempts to 2fa methods concern, change to user_session
kevinsmaster5 Oct 4, 2024
a65cdcf
equip otp with mfa attempt logging at authentication
kevinsmaster5 Oct 7, 2024
7901753
update spec because of session token change, add attempt count to web…
kevinsmaster5 Oct 7, 2024
640dba0
add mfa attempt count for totp authentication
kevinsmaster5 Oct 7, 2024
b4f3f75
add mfa count for piv authentication
kevinsmaster5 Oct 7, 2024
51d153c
clear user_session token when changing mfa after a failed attempt
kevinsmaster5 Oct 7, 2024
d7891eb
add params to piv analytics event
kevinsmaster5 Oct 7, 2024
e051696
fix piv verification spec. reset mfa account for setup failure
kevinsmaster5 Oct 7, 2024
49f670f
express auth attempts as a hash consisting of attempt count and method
kevinsmaster5 Oct 8, 2024
173e619
group all attempts into a hash
kevinsmaster5 Oct 10, 2024
32b1134
sync rspec up with changes made
kevinsmaster5 Oct 10, 2024
eb4d673
add mfa attempt to event expectation
kevinsmaster5 Oct 10, 2024
37db890
add mfa attempt to event expectation
kevinsmaster5 Oct 10, 2024
0f588f5
fixes specs to catch missing events
kevinsmaster5 Oct 11, 2024
98f0974
fix webauthn spec to correct user flow
kevinsmaster5 Oct 11, 2024
4b78810
fix mfa label in spec
kevinsmaster5 Oct 11, 2024
ae714e7
remove private_key gsub
kevinsmaster5 Oct 15, 2024
ae695cc
leverage symbols for mfa methods, remove no longer needed method from…
kevinsmaster5 Oct 15, 2024
75707d3
revise spec to use symbol
kevinsmaster5 Oct 15, 2024
b4212f0
address keypath warnings from spec
kevinsmaster5 Oct 15, 2024
591fb38
fix how otp verification controller generates the attempts count, upd…
kevinsmaster5 Oct 15, 2024
3cb1711
gsub personal_key for mfa_attempts
kevinsmaster5 Oct 17, 2024
39169f3
convert to sym
kevinsmaster5 Oct 17, 2024
0453e0a
convert key to sym with correct method
kevinsmaster5 Oct 17, 2024
462630f
rename incrementing method param. repair rspec to correct exptected mfa
kevinsmaster5 Oct 17, 2024
1c2db74
fix rspec expected mfa types
kevinsmaster5 Oct 17, 2024
2309190
place gsub behind a conditional
kevinsmaster5 Oct 17, 2024
e426e1c
set up a programmatic way of protecting pii keys in sessions
kevinsmaster5 Oct 18, 2024
347c9b6
refactor increment verb
kevinsmaster5 Oct 18, 2024
347b58e
fix broken logic
kevinsmaster5 Oct 18, 2024
d852f84
put expected attempt key back
kevinsmaster5 Oct 18, 2024
0e9d865
add testing for change to session_encryptor
kevinsmaster5 Oct 18, 2024
b87472e
change to more sensitive session detection approach
kevinsmaster5 Oct 21, 2024
80d4a1e
revise specs with change to attempt log structure
kevinsmaster5 Oct 21, 2024
be26643
follow up on remaining specs
kevinsmaster5 Oct 21, 2024
5dfaa8c
reorder check for method changeup
kevinsmaster5 Oct 21, 2024
985114a
restructure analytics for attempts and update tests
kevinsmaster5 Oct 22, 2024
cb69a2c
make webauthn setup count more accurate, remove previously added spec…
kevinsmaster5 Oct 22, 2024
66eb15f
increment webauthn setup also at confirm
kevinsmaster5 Oct 23, 2024
8797df4
utilize constant values for mfa methods
kevinsmaster5 Oct 23, 2024
274f409
select phone/voice and webauthn/_platform submethods
kevinsmaster5 Oct 23, 2024
30fdc23
use webauth_auth_method function
kevinsmaster5 Oct 23, 2024
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
17 changes: 17 additions & 0 deletions app/controllers/concerns/two_factor_authenticatable_methods.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,19 @@ def auth_methods_session
end

def handle_verification_for_authentication_context(result:, auth_method:, extra_analytics: nil)
increment_mfa_selection_attempt_count(auth_method)
analytics.multi_factor_auth(
**result.to_h,
multi_factor_auth_method: auth_method,
enabled_mfa_methods_count: mfa_context.enabled_mfa_methods_count,
new_device: new_device?,
**extra_analytics.to_h,
attempts: mfa_attempts_count,
)

if result.success?
handle_valid_verification_for_authentication_context(auth_method:)
user_session.delete(:mfa_attempts)
else
handle_invalid_verification_for_authentication_context
end
Expand Down Expand Up @@ -113,6 +116,20 @@ def handle_remember_device_preference(remember_device_preference)
save_remember_device_preference(remember_device_preference)
end

def increment_mfa_selection_attempt_count(auth_method)
user_session[:mfa_attempts] ||= {}
user_session[:mfa_attempts][:attempts] ||= 0
if user_session[:mfa_attempts][:auth_method] != auth_method
user_session[:mfa_attempts][:attempts] = 0
end
user_session[:mfa_attempts][:attempts] += 1
user_session[:mfa_attempts][:auth_method] = auth_method
end

def mfa_attempts_count
user_session.dig(:mfa_attempts, :attempts)
end

# 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}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ def show
end

def create
if UserSessionContext.confirmation_context?(context)
increment_mfa_selection_attempt_count(otp_auth_method)
end
result = otp_verification_form.submit
post_analytics(result)

Expand All @@ -41,13 +44,22 @@ def create
end

reset_otp_session_data
user_session.delete(:mfa_attempts)
else
handle_invalid_otp(type: 'otp')
end
end

private

def otp_auth_method
if params[:otp_delivery_preference] == 'sms'
TwoFactorAuthenticatable::AuthMethod::SMS
else
TwoFactorAuthenticatable::AuthMethod::VOICE
end
end

def handle_valid_confirmation_otp
assign_phone
track_mfa_added
Expand Down Expand Up @@ -155,6 +167,7 @@ def analytics_properties
phone_configuration_id: phone_configuration&.id,
in_account_creation_flow: user_session[:in_account_creation_flow] || false,
enabled_mfa_methods_count: mfa_context.enabled_mfa_methods_count,
attempts: mfa_attempts_count,
}
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ def create
result:,
auth_method: TwoFactorAuthenticatable::AuthMethod::TOTP,
)

if result.success?
handle_remember_device_preference(params[:remember_device])
redirect_to after_sign_in_path_for(current_user)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,11 +66,13 @@ def piv_cac_service_url_with_redirect
end

def process_piv_cac_setup
increment_mfa_selection_attempt_count(TwoFactorAuthenticatable::AuthMethod::PIV_CAC)
result = user_piv_cac_form.submit
properties = result.to_h.merge(analytics_properties)
analytics.multi_factor_auth_setup(**properties)
if result.success?
process_valid_submission
user_session.delete(:mfa_attempts)
else
process_invalid_submission
end
Expand Down Expand Up @@ -126,6 +128,7 @@ def analytics_properties
{
in_account_creation_flow: user_session[:in_account_creation_flow] || false,
enabled_mfa_methods_count: mfa_context.enabled_mfa_methods_count,
attempts: mfa_attempts_count,
}
end

Expand Down
4 changes: 3 additions & 1 deletion app/controllers/users/totp_setup_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,13 @@ def new

def confirm
result = totp_setup_form.submit

increment_mfa_selection_attempt_count(TwoFactorAuthenticatable::AuthMethod::TOTP)
properties = result.to_h.merge(analytics_properties)
analytics.multi_factor_auth_setup(**properties)

if result.success?
process_valid_code
user_session.delete(:mfa_attempts)
else
process_invalid_code
end
Expand Down Expand Up @@ -118,6 +119,7 @@ def analytics_properties
{
in_account_creation_flow: in_account_creation_flow?,
pii_like_keypaths: [[:mfa_method_counts, :phone]],
attempts: mfa_attempts_count,
}
end
end
Expand Down
13 changes: 12 additions & 1 deletion app/controllers/users/webauthn_setup_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ def new
@need_to_set_up_additional_mfa = need_to_set_up_additional_mfa?

if result.errors.present?
increment_mfa_selection_attempt_count(webauthn_auth_method)
analytics.webauthn_setup_submitted(
platform_authenticator: form.platform_authenticator?,
errors: result.errors,
Expand All @@ -54,6 +55,7 @@ def new
end

def confirm
increment_mfa_selection_attempt_count(webauthn_auth_method)
form = WebauthnSetupForm.new(
user: current_user,
user_session: user_session,
Expand All @@ -71,9 +73,9 @@ def confirm
)
properties = result.to_h.merge(analytics_properties)
analytics.multi_factor_auth_setup(**properties)

if result.success?
process_valid_webauthn(form)
user_session.delete(:mfa_attempts)
else
flash.now[:error] = result.first_error_message
render :new
Expand All @@ -89,6 +91,14 @@ def validate_existing_platform_authenticator
end
end

def webauthn_auth_method
if @platform_authenticator
TwoFactorAuthenticatable::AuthMethod::WEBAUTHN_PLATFORM
else
TwoFactorAuthenticatable::AuthMethod::WEBAUTHN
end
end

def platform_authenticator?
params[:platform] == 'true'
end
Expand Down Expand Up @@ -141,6 +151,7 @@ def process_valid_webauthn(form)
def analytics_properties
{
in_account_creation_flow: user_session[:in_account_creation_flow] || false,
attempts: mfa_attempts_count,
}
end

Expand Down
21 changes: 20 additions & 1 deletion app/services/analytics_events.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4905,6 +4905,7 @@ def logout_initiated(
# @param [Boolean] new_device Whether the user is authenticating from a new device
# @param [String] multi_factor_auth_method Authentication method used
# @param [String] multi_factor_auth_method_created_at When the authentication method was created
# @param [Integer] attempts number of MFA setup attempts
# @param [Integer] auth_app_configuration_id Database ID of authentication app configuration
# @param [Integer] piv_cac_configuration_id Database ID of PIV/CAC configuration
# @param [String] piv_cac_configuration_dn_uuid PIV/CAC X509 distinguished name UUID
Expand All @@ -4928,6 +4929,7 @@ def multi_factor_auth(
errors: nil,
error_details: nil,
context: nil,
attempts: nil,
multi_factor_auth_method_created_at: nil,
auth_app_configuration_id: nil,
piv_cac_configuration_id: nil,
Expand All @@ -4951,6 +4953,7 @@ def multi_factor_auth(
error_details:,
context:,
new_device:,
attempts:,
multi_factor_auth_method:,
multi_factor_auth_method_created_at:,
auth_app_configuration_id:,
Expand Down Expand Up @@ -4998,17 +5001,20 @@ def multi_factor_auth_added_phone(
# @param [Integer] enabled_mfa_methods_count Number of enabled MFA methods on the account
# @param [Boolean] in_account_creation_flow whether user is going through creation flow
# @param ['piv_cac'] method_name Authentication method added
# @param [Integer] attempts number of MFA setup attempts
def multi_factor_auth_added_piv_cac(
enabled_mfa_methods_count:,
in_account_creation_flow:,
method_name: :piv_cac,
attempts: nil,
**extra
)
track_event(
:multi_factor_auth_added_piv_cac,
method_name:,
enabled_mfa_methods_count:,
in_account_creation_flow:,
attempts:,
**extra,
)
end
Expand Down Expand Up @@ -5048,6 +5054,7 @@ def multi_factor_auth_enter_backup_code_visit(context:, **extra)
end

# @param ["authentication", "reauthentication", "confirmation"] context User session context
# @param [Integer] attempts number of MFA setup attempts
# @param [String] multi_factor_auth_method
# @param [Boolean] confirmation_for_add_phone
# @param [Integer] phone_configuration_id
Expand All @@ -5067,11 +5074,13 @@ def multi_factor_auth_enter_otp_visit(
phone_fingerprint:,
in_account_creation_flow:,
enabled_mfa_methods_count:,
attempts: nil,
**extra
)
track_event(
'Multi-Factor Authentication: enter OTP visited',
context:,
attempts:,
multi_factor_auth_method:,
confirmation_for_add_phone:,
phone_configuration_id:,
Expand Down Expand Up @@ -5247,6 +5256,7 @@ def multi_factor_auth_phone_setup(
# @param [String, nil] key_id PIV/CAC key_id from PKI service
# @param [Hash] mfa_method_counts Hash of MFA method with the number of that method on the account
# @param [Hash] authenticator_data_flags WebAuthn authenticator data flags
# @param [Integer] attempts number of MFA setup attempts
# @param [String, nil] aaguid AAGUID value of WebAuthn device
# @param [String[], nil] unknown_transports Array of unrecognized WebAuthn transports, intended to
# be used in case of future specification changes.
Expand All @@ -5270,6 +5280,7 @@ def multi_factor_auth_setup(
key_id: nil,
mfa_method_counts: nil,
authenticator_data_flags: nil,
attempts: nil,
aaguid: nil,
unknown_transports: nil,
**extra
Expand All @@ -5295,6 +5306,7 @@ def multi_factor_auth_setup(
key_id:,
mfa_method_counts:,
authenticator_data_flags:,
attempts:,
aaguid:,
unknown_transports:,
**extra,
Expand Down Expand Up @@ -5947,11 +5959,18 @@ def piv_cac_recommended_visited
# Tracks when user's piv cac setup
# @param [Boolean] in_account_creation_flow Whether user is going through account creation
# @param [Integer] enabled_mfa_methods_count Number of enabled MFA methods on the account
def piv_cac_setup_visited(in_account_creation_flow:, enabled_mfa_methods_count: nil, **extra)
# @param [Integer] attempts number of MFA setup attempts
def piv_cac_setup_visited(
in_account_creation_flow:,
enabled_mfa_methods_count: nil,
attempts: nil,
**extra
)
track_event(
:piv_cac_setup_visited,
in_account_creation_flow:,
enabled_mfa_methods_count:,
attempts:,
**extra,
)
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
multi_factor_auth_method: TwoFactorAuthenticatable::AuthMethod::REMEMBER_DEVICE,
enabled_mfa_methods_count: 0,
new_device: true,
attempts: 1,
)
end

Expand Down Expand Up @@ -189,6 +190,7 @@
multi_factor_auth_method: TwoFactorAuthenticatable::AuthMethod::SMS,
enabled_mfa_methods_count: 1,
new_device: true,
attempts: 1,
)
end

Expand All @@ -197,5 +199,33 @@
expect(user.events.last.event_type).to eq('sign_in_unsuccessful_2fa')
end
end

context 'user switches mfa after unsuccessful attempt' do
let(:user) { create(:user, :fully_registered) }
let(:auth_method) { TwoFactorAuthenticatable::AuthMethod::SMS }
before do
allow(controller).to receive(:user_session).and_return(
mfa_attempts: {
auth_method: 'piv_cac', attempts: 2
},
)
end

it 'tracks multi-factor authentication event with the expected number of attempts' do
stub_analytics

result

expect(@analytics).to have_logged_event(
'Multi-Factor Authentication',
success: true,
errors: {},
multi_factor_auth_method: TwoFactorAuthenticatable::AuthMethod::SMS,
enabled_mfa_methods_count: 1,
new_device: true,
attempts: 1,
)
end
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
multi_factor_auth_method_created_at: Time.zone.now.strftime('%s%L'),
enabled_mfa_methods_count: 1,
new_device: true,
attempts: 1,
)

expect(subject.user_session[:auth_events]).to eq(
Expand Down Expand Up @@ -97,6 +98,7 @@
multi_factor_auth_method_created_at: Time.zone.now.strftime('%s%L'),
enabled_mfa_methods_count: 1,
new_device: true,
attempts: 1,
)
expect(@analytics).to have_logged_event(
'User marked authenticated',
Expand Down Expand Up @@ -175,6 +177,7 @@
multi_factor_auth_method: 'backup_code',
enabled_mfa_methods_count: 1,
new_device: true,
attempts: 1,
)
expect(@analytics).to have_logged_event('Multi-Factor Authentication: max attempts reached')
end
Expand Down
Loading