LG-7201 - Added attempt events - MFA submitted rate limited#6783
LG-7201 - Added attempt events - MFA submitted rate limited#6783Rwolfe-Nava merged 9 commits intomainfrom
Conversation
Rwolfe-Nava
changed the title
Rwolfe-Nava
changed the title
Rwolfe-Nava
changed the title
Rwolfe-Nava
changed the title
zachmargolis
left a comment
zachmargolis
left a comment
There was a problem hiding this comment.
LGTM, small changes
spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb
Outdated
Show resolved
Hide resolved
Rwolfe-Nava
force-pushed
the
lg7200-mfa-phone-otp-send-rate-limited
branch
from
72e7701 to
440da0f
Compare
Rwolfe-Nava
changed the title
n1zyy
left a comment
n1zyy
left a comment
There was a problem hiding this comment.
One minor suggestion, but overall this looks good.
I'm going to hold off on approving since I paired a bit on this, but LGTM.
There was a problem hiding this comment.
I guess it's worth calling out for others: the code previously passed in "otp" for both phone-based OTP and TOTP, but we wanted to distinguish between them as distinct events.
This way we pass in extra detail, but don't change the user experience.
Rwolfe-Nava
force-pushed
the
lg7200-mfa-phone-otp-send-rate-limited
branch
from
03aea3d to
bc7effb
Compare
changelog: Internal, Attempts API, Track additional events
Rwolfe-Nava
force-pushed
the
lg7200-mfa-phone-otp-send-rate-limited
branch
from
bc7effb to
ad561de
Compare
n1zyy
left a comment
n1zyy
left a comment
There was a problem hiding this comment.
Looks good to me!
The absolute nittiest of nits about a comment that got moved in the merge conflict; I'd ignore it unless you find yourself editing stuff anyway since it doesn't change anything.
| # You can pass in any "type" with a corresponding I18n key in | ||
| # two_factor_authentication.invalid_#{type} | ||
| def handle_invalid_otp(type: 'otp') | ||
| def handle_invalid_otp(type:, context: nil) |
There was a problem hiding this comment.
I am weirdly delighted that we're able to get rid of the hard-coded 'otp' default here. 👏
| # Tracks when the user has attempted to log in with the piv cac MFA method to their account | ||
| # @param [String] subject_dn | ||
| # @param [Boolean] success | ||
| # @param [String] subject_dn |
There was a problem hiding this comment.
IMHO I wouldn't fix this unless you need to edit something else in this PR anyway, but I think in the merge conflict this line got moved from above to below success.
There was a problem hiding this comment.
So, the merge conflict removed the top two lines entirely 😨. I put them back in a commit, but noticed that the params were out of order, so I moved subject_dn below success on purpose, to match the function definition. Was that the right thing to do?
There was a problem hiding this comment.
Oh, my bad! Yes, good catch. 👍
3 participants
Summary of changes
mfa_enroll_rate_limitedattempt event.mfa_verify_rate_limitedattempt event. EDIT: nowmfa_login_rate_limitedtwo_factor_authenticationspec files for the above events.handle_invalid_otpin order to support the above events.changelog: Internal, Attempts API, Track additional events