LG-14261 Add attempt count to mfa setup auth events #11293
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -12,16 +12,24 @@ def auth_methods_session | |||||
| end | ||||||
|
|
||||||
| def handle_verification_for_authentication_context(result:, auth_method:, extra_analytics: nil) | ||||||
| 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, | ||||||
| mfa_attempts: user_session[:mfa_attempts], | ||||||
| pii_like_keypaths: [ | ||||||
| [:mfa_attempts, :otp], | ||||||
| [:errors, :personal_key], | ||||||
| [:error_details, :personal_key], | ||||||
| ], | ||||||
| ) | ||||||
|
|
||||||
| if result.success? | ||||||
| handle_valid_verification_for_authentication_context(auth_method:) | ||||||
| user_session.delete(:mfa_attempts) | ||||||
| else | ||||||
| handle_invalid_verification_for_authentication_context | ||||||
| end | ||||||
|
|
@@ -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) | ||||||
|
There was a problem hiding this comment. My previous suggestion was to rename the method name to include a verb action, not the argument name. Something like:
Suggested change
There was a problem hiding this comment. Oops. Fixed that in b9b2843 |
||||||
| user_session[:mfa_attempts] ||= {} | ||||||
| if increment_auth_method == 'personal_key' | ||||||
| increment_auth_method = increment_auth_method.gsub( | ||||||
| 'personal_key', | ||||||
| 'personal-key', | ||||||
| ) | ||||||
| end | ||||||
|
There was a problem hiding this comment. Continuing the previous discussion about trying to exempt from SessionEncryptor errors, the suggestion to explore Instead, I wonder if we could create a set of "safe" paths, following the existing pattern of the 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)
endIt'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. There was a problem hiding this comment. 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... ...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_methodThere was a problem hiding this comment. I made a run at this before I saw your comment with b92b9a6 |
||||||
| user_session[:mfa_attempts][increment_auth_method] ||= 0 | ||||||
| user_session[:mfa_attempts][increment_auth_method] += 1 | ||||||
| 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} | ||||||
|
|
||||||
aduth marked this conversation as resolved.
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we log
errors.personal_keyanderror_details.personal_keyhere? I'm surprised this would come up just now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was in response to an error I was getting. I can take another look for more clarity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]]