LG-14261 Add attempt count to mfa setup auth events #11293

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.

aduth · 2024-10-04T15:10:14Z

app/controllers/concerns/mfa_setup_concern.rb

This file is a concern related to MFA setup, but the ticket should include attempts for verification as well. It might make sense to put these methods somewhere else more common, maybe app/controllers/concerns/two_factor_authenticatable_methods.rb.

aduth · 2024-10-04T15:10:34Z

app/controllers/concerns/mfa_setup_concern.rb

@@ -64,6 +64,15 @@ def in_account_creation_flow?
    user_session[:in_account_creation_flow] || false
  end

+  def mfa_selection_attempt_count
+    session[:mfa_attempts] ||= 0


Should we be using user_session so that it gets cleaned up when the user is logged out?

aduth · 2024-10-04T15:11:07Z

app/controllers/concerns/mfa_setup_concern.rb

+  end
+
+  def reset_mfa_selection_attempt_count
+    session[:mfa_attempts] = nil


We should probably delete the key altogether, rather than keeping it as a nil value.

Suggested change

session[:mfa_attempts] = nil

session.delete(:mfa_attempts)

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

aduth · 2024-10-08T18:56:43Z

app/controllers/concerns/two_factor_authenticatable_methods.rb

@@ -18,6 +18,7 @@ def handle_verification_for_authentication_context(result:, auth_method:, extra_
      enabled_mfa_methods_count: mfa_context.enabled_mfa_methods_count,
      new_device: new_device?,
      **extra_analytics.to_h,
+      mfa_attempts: mfa_attempts_hash(auth_method),


Similar to how we're sharing the logging code here in handle_verification_for_authentication_context, I think it'd also make sense to handle the incrementing code as well? Rather than in each individual method's controller.

app/controllers/concerns/two_factor_authenticatable_methods.rb

kevinsmaster5 · 2024-10-11T17:19:08Z

app/controllers/concerns/two_factor_authenticatable_methods.rb

@@ -124,6 +127,25 @@ def handle_remember_device_preference(remember_device_preference)
    save_remember_device_preference(remember_device_preference)
  end

+  def mfa_selection_attempt_count(auth_method)
+    user_session[:mfa_attempts] ||= {}
+    auth_method = auth_method.gsub('personal_key', 'personal-key')


Adding 'personal_key' to a session would trigger SessionEncryptor::SensitiveKeyError even though here we're just counting attempts with that as the key.
Interested in a more idiomatic way of handling this edge case.

Maybe we could build support for something similar to FakeAnalytics' pii_like_keypaths to bypass the default error handling.

aduth · 2024-10-15T11:51:05Z

app/controllers/concerns/two_factor_authenticatable_methods.rb

+  def reset_mfa_selection_attempt_count
+    user_session.delete(:mfa_attempts)
+  end
+
+  def mfa_attempts_hash(auth_method)
+    return nil if user_session[:mfa_attempts].nil?
+    {
+      attempts: user_session[:mfa_attempts],
+      auth_method: auth_method,
+    }
+  end
+


These methods appears to be unused.

Suggested change

def reset_mfa_selection_attempt_count

user_session.delete(:mfa_attempts)

end

def mfa_attempts_hash(auth_method)

return nil if user_session[:mfa_attempts].nil?

{

attempts: user_session[:mfa_attempts],

auth_method: auth_method,

}

end

Those are removed

aduth · 2024-10-15T11:53:57Z

spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb

+          mfa_attempts: {
+            'otp' => 1,
+            'sms' => 1,
+          },


Evidenced by this test, I think we're double-counting attempts for verification. Maybe related to the comment I had asking about conditionally incrementing count for confirmation context?

Luckily they seem to be a different name, but I'd still only expect one or the other here per attempt.

I have that worked out now using the suggested condition check

aduth · 2024-10-15T11:56:56Z

app/controllers/concerns/two_factor_authenticatable_methods.rb

@@ -124,6 +127,25 @@ def handle_remember_device_preference(remember_device_preference)
    save_remember_device_preference(remember_device_preference)
  end

+  def mfa_selection_attempt_count(auth_method)


Nit: Since this method is doing a thing, I'd expect its name to include a verb (e.g. "increment").

Revised to incorporate increment

aduth · 2024-10-15T12:01:16Z

app/controllers/concerns/two_factor_authenticatable_methods.rb

@@ -124,6 +127,25 @@ def handle_remember_device_preference(remember_device_preference)
    save_remember_device_preference(remember_device_preference)
  end

+  def mfa_selection_attempt_count(auth_method)
+    user_session[:mfa_attempts] ||= {}
+    auth_method = auth_method.gsub('personal_key', 'personal-key')


Maybe we could build support for something similar to FakeAnalytics' pii_like_keypaths to bypass the default error handling.

aduth · 2024-10-15T12:02:21Z

app/controllers/two_factor_authentication/otp_verification_controller.rb

@@ -21,6 +21,7 @@ def show
    end

    def create
+      mfa_selection_attempt_count('otp')


I think since these values are "known", it makes more sense to use symbol values rather than strings.

(same comment across the board for the hash keys)

Suggested change

mfa_selection_attempt_count('otp')

mfa_selection_attempt_count(:otp)

Related: https://fullstackheroes.com/tutorials/ruby/symbols-vs-strings/

That makes sense. Thanks!

aduth · 2024-10-15T12:04:21Z

app/controllers/concerns/two_factor_authenticatable_methods.rb

+    user_session[:mfa_attempts] ||= {}
+    auth_method = auth_method.gsub('personal_key', 'personal-key')
+    attempt = { auth_method => 1 }
+    user_session[:mfa_attempts].merge!(attempt) { |_key, old_val, new_val| old_val + new_val }


Feels like this could be simplified?

Suggested change

user_session[:mfa_attempts] ||= {}

auth_method = auth_method.gsub('personal_key', 'personal-key')

attempt = { auth_method => 1 }

user_session[:mfa_attempts].merge!(attempt) { |_key, old_val, new_val| old_val + new_val }

user_session[:mfa_attempts] ||= {}

user_session[:mfa_attempts][auth_method] ||= 0

user_session[:mfa_attempts][auth_method] += 1

Revised that to simplify.

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

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

…ics event

… prev iteration

…ate specs to sync with that

aduth · 2024-10-18T12:13:03Z

app/controllers/concerns/two_factor_authenticatable_methods.rb

@@ -124,6 +132,18 @@ def handle_remember_device_preference(remember_device_preference)
    save_remember_device_preference(remember_device_preference)
  end

+  def mfa_selection_attempt_count(increment_auth_method)


My previous suggestion was to rename the method name to include a verb action, not the argument name.

Something like:

Suggested change

def mfa_selection_attempt_count(increment_auth_method)

def increment_mfa_selection_attempt_count(auth_method)

Oops. Fixed that in b9b2843

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

app/controllers/concerns/two_factor_authenticatable_methods.rb

+        [:errors, :personal_key],
+        [:error_details, :personal_key],


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]]

aduth · 2024-10-18T13:33:01Z

app/controllers/concerns/two_factor_authenticatable_methods.rb

+    if increment_auth_method == 'personal_key'
+      increment_auth_method = increment_auth_method.gsub(
+        'personal_key',
+        'personal-key',
+      )
+    end


Continuing the previous discussion about trying to exempt from SessionEncryptor errors, the suggestion to explore pii_like_keypaths might not work so well since it's not implemented by default, but moreso because SessionEncryptor is concerned with the session and not the log detail.

Instead, I wonder if we could create a set of "safe" paths, following the existing pattern of the SENSITIVE_PATHS in SessionEncryptor, which would be exempt from the default error handling.

I played around with this locally and this seemed to work okay:

class SessionEncryptor # ... # Paths which would trigger sensitive key detection, but which don't contain PII SAFE_PATHS = [ ['warden.user.user.session', 'mfa_attempts', 'personal_key'], ].freeze # ... def alert_or_raise_if_contains_sensitive_keys!(hash) except_nested(hash, SAFE_PATHS).deep_transform_keys do |key| # ... end end def except_nested(hash, key_paths) hash = hash.deep_dup key_paths.each do |key_path| nested_hash = hash.dig(*key_path[..-2]) nested_hash&.delete!(key_path.last) end hash end end

it 'does not raise if exempted PII key appears' do session = { 'warden.user.user.session' => { 'mfa_attempts' => { 'personal_key' => 1 } } } session = session.deep_transform_values(&:freeze) expect { subject.dump(session) }.not_to raise_error(SessionEncryptor::SensitiveKeyError) end it 'maintains all values from the original session' do session = { 'warden.user.user.session' => { 'idv_new' => { 'nested' => {} }, 'mfa_attempts' => { 'personal_key' => 1 }, }, } dumped = subject.dump(session.deep_dup) loaded = subject.load(dumped) expect(loaded).to eq(session) end

It's starting to get a bit complex and I'd have some reservations about the performance impact of extra logic to deep-duplicate the session hash for every request on a session that's very unlikely to ever contain the value we care about. But I also don't feel very good about the renaming approach, since it seems to be intentionally circumventing the intent of the session encryptor in identifying a "personal key" key within the session.

Another thought, if we're okay to just increment the count of attempts for the current method but want to ensure that we reset when switching to another method, we could track it something like...

user_session = { mfa_attempts: { attempts: 0, auth_method: 'personal_key' } }

...then make sure to reset attempts if the auth method is different from what's in the session

user_session[:mfa_attempts][:attempts] = 0 if user_session[:mfa_attempts][:auth_method] != auth_method

I made a run at this before I saw your comment with b92b9a6
I wanted to try to make it work programmatically and not hard-code the session key so I ended up adding another session value. Not sure if that's a great approach though. It doesn't work exactly the same as how

identity-idp/spec/support/fake_analytics.rb

Line 49 in 74b19c4

if pii_attr_names.include?(key) && !pii_like_keypaths.include?(current_keypath)

was solved however.

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

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

aduth reviewed Oct 15, 2024

View reviewed changes

kevinsmaster5 and others added 10 commits October 17, 2024 15:04

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

76dcf08

…ics event

add piv attempts, use common name in analytics

c6402f9

add logging to otp and totp setup

ca53ef1

reset attempts count on success

6542fe3

add totp reset and refactor reset on webauthn

587da36

update otp specs

ce6927a

update totp specs

a5b8825

update piv_cac_setup specs

afb95c5

add otp spec that confirms incremented mfa_attempts analytics log

f9e035d

add specs for piv and totp

563173a

kevinsmaster5 added 22 commits October 17, 2024 15:04

add mfa count for piv authentication

ff1564c

clear user_session token when changing mfa after a failed attempt

c5d3fbe

add params to piv analytics event

0572f73

fix piv verification spec. reset mfa account for setup failure

d5a4d20

express auth attempts as a hash consisting of attempt count and method

54f296b

group all attempts into a hash

f57e9cd

sync rspec up with changes made

a692ecf

add mfa attempt to event expectation

ab1fe0e

add mfa attempt to event expectation

5d92095

fixes specs to catch missing events

25a70ff

fix webauthn spec to correct user flow

c6f08f0

fix mfa label in spec

a97a0ac

remove private_key gsub

02ec7f7

leverage symbols for mfa methods, remove no longer needed method from…

097a114

… prev iteration

revise spec to use symbol

1dcea81

address keypath warnings from spec

e1ae532

fix how otp verification controller generates the attempts count, upd…

9500700

…ate specs to sync with that

gsub personal_key for mfa_attempts

1800e9b

convert to sym

39b18a9

convert key to sym with correct method

6528f70

rename incrementing method param. repair rspec to correct exptected mfa

1f9ae4c

fix rspec expected mfa types

da9afdf

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

place gsub behind a conditional

65f3232

aduth reviewed Oct 18, 2024

View reviewed changes

kevinsmaster5 added 5 commits October 18, 2024 11:05

set up a programmatic way of protecting pii keys in sessions

b92b9a6

refactor increment verb

b9b2843

fix broken logic

ee762d5

put expected attempt key back

7a58848

add testing for change to session_encryptor

2f17d79

	mfa_selection_attempt_count('otp')
	mfa_selection_attempt_count(:otp)

	def mfa_selection_attempt_count(increment_auth_method)
	def increment_mfa_selection_attempt_count(auth_method)

LG-14261 Add attempt count to mfa setup auth events #11293

Are you sure you want to change the base?

LG-14261 Add attempt count to mfa setup auth events #11293

Conversation

kevinsmaster5 commented Sep 27, 2024

🎫 Ticket

🛠 Summary of changes

📜 Testing Plan

👀 Screenshots

kevinsmaster5 commented Sep 27, 2024

kevinsmaster5 commented Sep 27, 2024

mdiarra3 left a comment

Choose a reason for hiding this comment

kevinsmaster5 commented Oct 2, 2024

kevinsmaster5 commented Oct 3, 2024

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Oct 7, 2024

kevinsmaster5 commented Oct 7, 2024 • edited Loading

aduth commented Oct 8, 2024

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

voidlily commented Oct 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinsmaster5 Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinsmaster5 commented Oct 7, 2024 •

edited

Loading

kevinsmaster5 Oct 18, 2024 •

edited

Loading