Add validation error when mail OTP is expired #1614
Conversation
|
This PR uses the current generic message for an expired OTP: Tagging cc: @mkhandekar |
|
I recommend this to match the link below “send me another letter”:
|
| def most_recent_otp_expired? | ||
| return false unless any_mail_sent? | ||
|
|
||
| user_mail_events.first.updated_at < USPS_CONFIRMATION_WINDOW_DAYS.days.ago |
There was a problem hiding this comment.
Maybe not in this PR, but I think it's worth it to order user_mail_events in ASC order because everywhere it's being used, we're naming things the opposite of the meaning. For example, here we are referring to the most recent event, but we are fetching it by calling first, which is counterintuitive. I would expect it to be last. Same with updated_within_last_month?. I would expect it to refer to first since it's the oldest mail event that is compared to MAIL_EVENTS_WINDOW_DAYS. Thoughts?
There was a problem hiding this comment.
(Hi, I know this is already merged, but the code is here, so I'm just gonna comment anyways)
I don't think that updated_at is a good column to use because a lot of things could update this record. I think we should add a migration for an explicit sent_at (or technically queued_at) column that indicates when the mail event was sent, so we have a clear value that should expire?
There was a problem hiding this comment.
Hmm, that seems like it could get a little bit sticky since the Event model is a generic for a number of different user events. So, sent_at or queued_at may make sense for usps mail events, but maybe not for things like account created events or phone confirmed events.
With that in mind, what is the best way for us to arrange events based on when the events occurred? I would think event.created_at makes sense for that, but went with updated_at because that is what I saw in the code for mail_spammed?.
There was a problem hiding this comment.
FWIW when I was thinking of implementing this, I would have put the otp_created_at in the PII payload next to the OTP itself in the encrypted PII. I don't think there's a clearly right answer here, but updated_at is a little ambiguous here for my comfort.
Maybe the mail_spammed should be using created_at? (since in cases we create the event for every attempt....updating shouldn't matter?) I don't feel as strongly about that since it's for rate limiting and I expect that to be more fluid compared to a really clear expiration point for the code.
**Why**: So that users can't use an expired mail OTP to verify an identity.
jmhooper
force-pushed
the
jmhooper-mail-otp-expiration
branch
from
5fba205 to
913ac58
Compare
Why: So that users can't use an expired mail OTP to verify their
identity.