LG-14261 Add attempt count to mfa setup auth events by kevinsmaster5 · Pull Request #11293 · 18F/identity-idp

kevinsmaster5 · 2024-09-27T13:21:49Z

🎫 Ticket

Link to the relevant ticket:
LG-14261

🛠 Summary of changes

Adds a session token to track number of attempts when setting up an mfa type.
Total sum of attempts gets passed to analytics_events.
Backup codes are excluded since during setup they're added instantly.

📜 Testing Plan

Checklist of steps to confirm the changes.

In a separate console run make watch_events
http://localhost:3000 with existing or new account add MFA.
Observe in the watch_events feed 'Multi-Factor Authentication Setup' that a predictable value for
properties > event_properties > mfa_attempts is registered

Attempt count will increment by failing and retrying to add mfa for example:

Webauthn (Face|Touch or security key) clicking cancel at the browser dialog before completing setup
PIV test screen select a failing condition
SMS/Voice enter (change) supplied auth code
Auth App enter wrong auth code

👀 Screenshots

kevinsmaster5 · 2024-09-27T14:05:07Z

Taking a look at second_factor_attempts_count to see if that might take the place of creating a session token.

kevinsmaster5 · 2024-09-27T14:59:18Z

Taking a look at second_factor_attempts_count to see if that might take the place of creating a session token.

seems to only work for OTP.

mdiarra3

Looks good and seems to work locally, but wondering if theres a way for us to push this out to a helper or concern for the verification controllers.

kevinsmaster5 · 2024-10-02T12:25:16Z

Looks good and seems to work locally, but wondering if theres a way for us to push this out to a helper or concern for the verification controllers.

Do you mean something like abstracting this sort of thing into a concern?

session[:piv_cac_attempts] ||= 0
session[:piv_cac_attempts] += 1

It does look repetitive since it's sort of identical in 4 places.

kevinsmaster5 · 2024-10-03T16:45:38Z

Added suggestion from #11293 (review)

aduth

The ticket wants this to be included for both setup and verification attempts, but the pull request is currently focused on setup only.

app/controllers/concerns/mfa_setup_concern.rb

app/controllers/two_factor_authentication/otp_verification_controller.rb

aduth · 2024-10-07T14:44:18Z

If someone fails authenticating or setting up with one authentication method and then switches to another, what do we expect the mfa_attempts to be for that second MFA method? I think we wouldn't want to unfairly bias attempts for that second method based on failures with another method.

I think we should either only log the attempts for that individual method, or the log value should be a hash of attempts for each method. For both cases, I'd imagine the session value would be a hash of method-to-attempts.

kevinsmaster5 · 2024-10-07T20:23:32Z

I think we should either only log the attempts for that individual method, or the log value should be a hash of attempts for each method. For both cases, I'd imagine the session value would be a hash of method-to-attempts.

So rather than
"event_properties": {
"success": true,
"errors": {},
"multi_factor_auth_method": "piv_cac",
"in_account_creation_flow": true,
"enabled_mfa_methods_count": 1,
"mfa_attempts": 1
},

Would we want to see something like
"event_properties": {
"success": true,
"errors": {},
"multi_factor_auth_method": "piv_cac",
"in_account_creation_flow": true,
"enabled_mfa_methods_count": 1,
"mfa_attempts": {
"piv_cac": 1
}
},

aduth · 2024-10-08T11:53:05Z

I think we could keep the logged value a single integer if we wanted, but in order to make sure that we're only logging attempts for that method, we'd need to track the attempts in the session as per-method (a hash).

So the session value would look something like the hash in your example, and getting the value from the session is something like user_session[:mfa_attempts][method].

aduth

I'm wondering if it'd be better to implement the incrementing

app/controllers/two_factor_authentication/options_controller.rb

app/controllers/two_factor_authentication/otp_verification_controller.rb

app/controllers/concerns/two_factor_authenticatable_methods.rb

spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb

app/controllers/concerns/two_factor_authenticatable_methods.rb

app/controllers/two_factor_authentication/otp_verification_controller.rb

app/controllers/concerns/two_factor_authenticatable_methods.rb

voidlily · 2024-10-17T18:56:42Z

can you rebase this branch to pick up changes to the reviewapp deploy process?

app/controllers/concerns/two_factor_authenticatable_methods.rb

aduth · 2024-10-18T12:14:06Z

app/controllers/concerns/two_factor_authenticatable_methods.rb

Do we log errors.personal_key and error_details.personal_key here? I'm surprised this would come up just now.

It was in response to an error I was getting. I can take another look for more clarity.

It was in the same spec that was exhibiting the session sensitive key error
./spec/features/legacy_passwords_spec.rb:61

legacy passwords signing in with an incorrect uak personal key digest does not grant access
Failure/Error:
raise PiiDetected, <<~ERROR
track_event received pii key path: #{current_keypath.inspect}
event: #{event} (#{constant_name})
full event: #{attributes.inspect}
allowlisted keypaths: #{pii_like_keypaths.inspect}
ERROR

FakeAnalytics::PiiDetected:
track_event received pii key path: [:errors, :personal_key]
event: Multi-Factor Authentication ()
full event: {:success=>false, :errors=>{:personal_key=>["Incorrect personal key"]}, :error_details=>{:personal_key=>{:personal_key_incorrect=>true}}, :new_device=>true, :mfa_attempts=>{"personal_key"=>1}, :multi_factor_auth_method=>"personal-key", :enabled_mfa_methods_count=>1}
allowlisted keypaths: [[:mfa_attempts, :otp], [:error_details, :personal_key]]

I'm not able to reproduce that test failure locally in your branch when removing this pii_like_keypaths code. Also, mfa_attempts.otp would never exist in the hash structure since you changed that in an earlier commit.

app/controllers/concerns/two_factor_authenticatable_methods.rb

lib/session_encryptor.rb

aduth · 2024-10-22T11:53:52Z

spec/controllers/concerns/two_factor_authenticatable_methods_spec.rb

I think now that we're only tracking attempts for the current method, and because auth_method is a redundant property, it'd be nice if we logged the numeric value of attempts instead.

Suggested change

mfa_attempts: {

attempts: 1,

auth_method: 'sms',

},

attempts: 1,

Revised expected analytics event with e6ab87d

aduth · 2024-10-22T11:56:00Z

app/controllers/concerns/two_factor_authenticatable_methods.rb

I'm not able to reproduce that test failure locally in your branch when removing this pii_like_keypaths code. Also, mfa_attempts.otp would never exist in the hash structure since you changed that in an earlier commit.

app/controllers/concerns/two_factor_authenticatable_methods.rb

aduth · 2024-10-22T15:20:23Z

app/controllers/users/webauthn_setup_controller.rb

I don't think this is accurately counting attempts. If I sat on the WebAuthn setup page and refreshed the page, it would count that as an attempt, but that's not really an attempt. I think we'd want this inside the result.errors.present? check above.

I moved it there. I don't honestly know why that works. Do we expect errors to be present every time?

aduth · 2024-10-22T15:21:07Z

spec/lib/session_encryptor_spec.rb

I don't think we need these changes anymore?

That's right. Those are removed.

…ics event

…, improve method count helper

app/controllers/users/totp_setup_controller.rb

app/services/analytics_events.rb

app/controllers/users/webauthn_setup_controller.rb

kevinsmaster5 · 2024-10-23T12:53:25Z

app/controllers/concerns/two_factor_authenticatable_methods.rb

    user_session[:mfa_attempts] ||= {}
    user_session[:mfa_attempts][:attempts] ||= 0
-    if user_session[:mfa_attempts][:auth_method] != auth_method
+    if user_session[:mfa_attempts][:auth_method].to_s != auth_method.to_s


webauthn gets converted to a string when it's added to the session
[2] pry(#Users::WebauthnSetupController)> auth_method
=> :webauthn.1 |
[3] pry(#Users::WebauthnSetupController)> user_session[:mfa_attempts]

it would always set the increment to zero because the 2 values were coming out inequal

Thinking it's because the auth method may or may not be set to "_platform" 🤔

Yes, that's a good call-out. The WebAuthn controller handles both Security Key and Face or Touch Unlock, and we probably want to differentiate with webauthn_platform vs. webauthn.

This reminds me that we have constants defined for each of the expected auth_method values. Using these might also help with issues normalizing to string, since session value should always be a string.

identity-idp/app/controllers/concerns/two_factor_authenticatable.rb

Lines 18 to 26 in f7fcb4c

BACKUP_CODE = 'backup_code'

PERSONAL_KEY = 'personal_key'

PIV_CAC = 'piv_cac'

REMEMBER_DEVICE = 'remember_device'

SMS = 'sms'

TOTP = 'totp'

VOICE = 'voice'

WEBAUTHN = 'webauthn'

WEBAUTHN_PLATFORM = 'webauthn_platform'

kevinsmaster5 · 2024-10-23T15:00:14Z

app/controllers/two_factor_authentication/otp_verification_controller.rb


    def create
+      if UserSessionContext.confirmation_context?(context)
+        increment_mfa_selection_attempt_count(:otp)


@aduth should this (and others relative to their constants) be

Suggested change

increment_mfa_selection_attempt_count(:otp)

increment_mfa_selection_attempt_count(TwoFactorAuthenticatable::AuthMethod::PIV_CAC)

for example regarding #11293 (comment)

Yeah like that, as appropriate for each method.

Got that in there. It also made the change here #11293 (comment) no longer needed.

I still see the symbol :otp used here. Would we want to switch between TwoFactorAuthenticatable::AuthMethod::SMS or TwoFactorAuthenticatable::AuthMethod::VOICE like we're doing with WebAuthn, to be able to better differentiate what delivery method they're failing with?

aduth · 2024-10-23T17:58:07Z

app/controllers/users/webauthn_setup_controller.rb

    end

    def confirm
+      increment_mfa_selection_attempt_count(:webauthn)


We'll want to use the constant here as well, and incorporate 'webauthn_platform'.

Maybe we want a helper method to avoid noisy if else ?

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

That works great - also for sms/voice.

aduth

One minor suggestion, but LGTM otherwise 👍

aduth · 2024-10-23T18:56:01Z

app/controllers/users/webauthn_setup_controller.rb

+        if @platform_authenticator
+          increment_mfa_selection_attempt_count(
+            TwoFactorAuthenticatable::AuthMethod::WEBAUTHN_PLATFORM,
+          )
+        else
+          increment_mfa_selection_attempt_count(
+            TwoFactorAuthenticatable::AuthMethod::WEBAUTHN,
+          )
+        end


We can use your new helper method to simplify

Suggested change

if @platform_authenticator

increment_mfa_selection_attempt_count(

TwoFactorAuthenticatable::AuthMethod::WEBAUTHN_PLATFORM,

)

else

increment_mfa_selection_attempt_count(

TwoFactorAuthenticatable::AuthMethod::WEBAUTHN,

)

end

increment_mfa_selection_attempt_count(webauthn_auth_method)

oh right! thanks :)

kevinsmaster5 added the status - work in progress label Sep 27, 2024

kevinsmaster5 removed the status - work in progress label Sep 30, 2024

kevinsmaster5 marked this pull request as ready for review September 30, 2024 13:31

kevinsmaster5 requested a review from a team September 30, 2024 14:53

mdiarra3 approved these changes Oct 1, 2024

View reviewed changes

kevinsmaster5 requested a review from mdiarra3 October 3, 2024 16:41

aduth reviewed Oct 4, 2024

View reviewed changes

kevinsmaster5 requested a review from aduth October 7, 2024 20:36

aduth reviewed Oct 8, 2024

View reviewed changes

kevinsmaster5 commented Oct 11, 2024

View reviewed changes

app/controllers/concerns/two_factor_authenticatable_methods.rb Outdated Show resolved Hide resolved

kevinsmaster5 requested a review from aduth October 11, 2024 19:45

aduth reviewed Oct 15, 2024

View reviewed changes

kevinsmaster5 force-pushed the kmas-lg-14261-add-attempt-count-to-mfa-setup-auth branch from df8b957 to da9afdf Compare October 17, 2024 19:04

aduth reviewed Oct 18, 2024

View reviewed changes

aduth reviewed Oct 21, 2024

View reviewed changes

lib/session_encryptor.rb Outdated Show resolved Hide resolved

aduth reviewed Oct 22, 2024

View reviewed changes

kevinsmaster5 added 5 commits October 22, 2024 15:29

changelog: Internal, MFA setup, Add attempt count to MFA setup analyt…

6c05942

…ics event

add piv attempts, use common name in analytics

ba3cd02

add logging to otp and totp setup

2c49e86

reset attempts count on success

cae6055

add totp reset and refactor reset on webauthn

bd3c92c

kevinsmaster5 added 16 commits October 22, 2024 15:29

convert to sym

39169f3

convert key to sym with correct method

0453e0a

rename incrementing method param. repair rspec to correct exptected mfa

462630f

fix rspec expected mfa types

1c2db74

place gsub behind a conditional

2309190

set up a programmatic way of protecting pii keys in sessions

e426e1c

refactor increment verb

347c9b6

fix broken logic

347b58e

put expected attempt key back

d852f84

add testing for change to session_encryptor

0e9d865

change to more sensitive session detection approach

b87472e

revise specs with change to attempt log structure

80d4a1e

follow up on remaining specs

be26643

reorder check for method changeup

5dfaa8c

restructure analytics for attempts and update tests

985114a

make webauthn setup count more accurate, remove previously added spec…

cb69a2c

…, improve method count helper

kevinsmaster5 force-pushed the kmas-lg-14261-add-attempt-count-to-mfa-setup-auth branch from 8f17b01 to cb69a2c Compare October 22, 2024 19:30

aduth reviewed Oct 22, 2024

View reviewed changes

app/controllers/users/totp_setup_controller.rb Outdated Show resolved Hide resolved

app/services/analytics_events.rb Outdated Show resolved Hide resolved

app/controllers/users/webauthn_setup_controller.rb Outdated Show resolved Hide resolved

increment webauthn setup also at confirm

66eb15f

kevinsmaster5 commented Oct 23, 2024

View reviewed changes

utilize constant values for mfa methods

8797df4

aduth reviewed Oct 23, 2024

View reviewed changes

select phone/voice and webauthn/_platform submethods

274f409

aduth approved these changes Oct 23, 2024

View reviewed changes

use webauth_auth_method function

30fdc23

kevinsmaster5 merged commit 7837514 into main Oct 23, 2024

kevinsmaster5 deleted the kmas-lg-14261-add-attempt-count-to-mfa-setup-auth branch October 23, 2024 20:15

mitchellhenke mentioned this pull request Oct 24, 2024

Deploy RC 425 to Production #11392

Merged

-          mfa_attempts: {
-            attempts: 1,
-            auth_method: 'sms',
-          },
+          attempts: 1,

	BACKUP_CODE = 'backup_code'
	PERSONAL_KEY = 'personal_key'
	PIV_CAC = 'piv_cac'
	REMEMBER_DEVICE = 'remember_device'
	SMS = 'sms'
	TOTP = 'totp'
	VOICE = 'voice'
	WEBAUTHN = 'webauthn'
	WEBAUTHN_PLATFORM = 'webauthn_platform'

	increment_mfa_selection_attempt_count(:otp)
	increment_mfa_selection_attempt_count(TwoFactorAuthenticatable::AuthMethod::PIV_CAC)

Conversation

kevinsmaster5 commented Sep 27, 2024

🎫 Ticket

🛠 Summary of changes

📜 Testing Plan

👀 Screenshots

Uh oh!

kevinsmaster5 commented Sep 27, 2024

Uh oh!

kevinsmaster5 commented Sep 27, 2024

Uh oh!

mdiarra3 left a comment

Choose a reason for hiding this comment

Uh oh!

kevinsmaster5 commented Oct 2, 2024

Uh oh!

kevinsmaster5 commented Oct 3, 2024

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aduth commented Oct 7, 2024

Uh oh!

kevinsmaster5 commented Oct 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduth commented Oct 8, 2024

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

voidlily commented Oct 17, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinsmaster5 Oct 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kevinsmaster5 commented Oct 7, 2024 •

edited

Loading

kevinsmaster5 Oct 18, 2024 •

edited

Loading

kevinsmaster5 Oct 23, 2024 •

edited

Loading