LG-9984: Add mfa_created_at to analytics#8718
Conversation
aduth
left a comment
aduth
left a comment
There was a problem hiding this comment.
Couple comments and a lingering question about data querying, but functionally I tested this and it works great 👍
| private | ||
|
|
||
| def track_analytics(result) | ||
| mfa_created_at = current_user&.encrypted_recovery_code_digest_generated_at |
There was a problem hiding this comment.
Do we need the nil-safe operator here? Not sure I'd expect current_user to be nil here, and don't see a corresponding spec.
| mfa_created_at = current_user&.encrypted_recovery_code_digest_generated_at | |
| mfa_created_at = current_user.encrypted_recovery_code_digest_generated_at |
| # TODO Note: This is a mirror of the style found in: | ||
| # app/forms/totp_verification_form.rb | ||
| # @return [BackupCodeConfiguration, nil] | ||
| def if_valid_code_return_config(plaintext_code) |
There was a problem hiding this comment.
With the case TOTP, and the method if_valid_totp_code_return_config, I prefer the verbose descriptiveness. Changing that method, and the method in question, to config hides the validation which is generally more important than the config itself.
There was a problem hiding this comment.
Yeah, that's fair. I feel like the method had been doing a lot that wasn't obvious by its name, and my previous suggestion was based on an assumption that it's largely a "lookup", which is only partially true.
I'm a bit surprised about the side-effect to update the used_at property, and worry that that it's not obvious that the code is effectively consumed any time we call this if_valid_code_return_config method.
I'd wonder if we should split this up somehow between the validation/consumption/retrieval. I'm not sure we need all the same validation for the created_at property, for example.
| true | ||
| end | ||
|
|
||
| # TODO Note: This is a mirror of the style found in: |
There was a problem hiding this comment.
Can we remove the TODO remark?
| # TODO Note: This is a mirror of the style found in: | |
| # Note: This is a mirror of the style found in: |
| return unless plaintext_code.present? | ||
| backup_code = RandomPhrase.normalize(plaintext_code) | ||
| config = BackupCodeConfiguration.find_with_code(code: backup_code, user_id: @user.id) | ||
| return unless code_usable?(config) | ||
| config.update!(used_at: Time.zone.now) | ||
| config |
There was a problem hiding this comment.
To avoid duplicating this logic between here and in verify, can we refactor verify to use this new method?
i.e.
def verify(plaintext_code)
if_valid_code_return_config(plaintext_code).present?
endThere was a problem hiding this comment.
Can we include a test case in this file that yields a value other than nil ?
| # @param [Hash] errors Authentication error reasons, if unsuccessful | ||
| # @param [String] context | ||
| # @param [String] multi_factor_auth_method | ||
| # @param [DateTime] multi_factor_auth_method_created_at time auth method was created |
There was a problem hiding this comment.
For awareness: In trying to verify how we'd query this, I wasn't able to get CloudWatch to produce results for date string filtering. Worst case we might need to convert this to another format like a timestamp number, but hopefully I'm just doing something wrong. Have you been able to devise a query using this type of data?
Slack thread here: https://gsa-tts.slack.com/archives/C5E7EJWF7/p1688563099123109
changelog: Internal, Analytics, Add mfa_created_at to Multi-Factor Authentication event
jc-gsa
force-pushed
the
LG-9984-add-mfa-created-at-to-analytics
branch
from
8c8bd3f to
bf29282
Compare
2 participants
🎫 Ticket
LG-9984
Adds value
mfa_created_atto eventMulti-Factor Authentication.