feat(otp): switch one_time_tokens table to source of truth - #2788
Conversation
2ed1e5b to
0665647
Compare
0665647 to
8bf17f6
Compare
7577774 to
fc7f405
Compare
8bf17f6 to
0820b2b
Compare
Remove the phone-provider branches from the one_time_tokens verify path so this PR covers only the row-based flow. In OTT mode a test OTP or Twilio Verify code now misses the hash lookup and is rejected. A follow-up PR stacked on this branch restores both.
31b4e74 to
6d15a97
Compare
| // NOTE: Test OTPs and Twilio Verify are not handled yet on this path; a follow-up PR will add them. | ||
| func verifyUserAndTokenFromOTT(conn *storage.Connection, params *VerifyParams, aud string) (*models.User, error) { | ||
|
|
||
| // TODO AUTH-1553: Add support for test OTPs and Twilio Verify on this path. |
There was a problem hiding this comment.
Separating this out to make it easier to review, but they'll be merged together.
|
|
||
| // IsExpired treats nil ExpiresAt as expired. This is a security measure to avoid accidentally treating a token with no expiration as valid. | ||
| func (o OneTimeToken) IsExpired() bool { | ||
| return o.ExpiresAt == nil || time.Now().After(*o.ExpiresAt) |
There was a problem hiding this comment.
We have two options here:
- Treat a nil
expiresAtas expired. - Treat a nil
expiresAtas non-expired.
expiresAt will be nil for all OTTs created before expiresAt was written to.
We can reduce the likelihood by staggering the release of these changes + the write-path changes so the OTTs without expiries age out, but eventually we plan to make this path on by default. So, if a self-hosted customer doesn't upgrade until we've made it the default, it would cause all of their recently issued tokens to expire.
I went this direction because having to re-request a OTT seems like a minor inconvenience, but I wanted to point that out so we can discuss if there are any concerns.
| // columns. A lookup miss is rejected as an expired or invalid token. | ||
| // | ||
| // NOTE: Test OTPs and Twilio Verify are not handled yet on this path; a follow-up PR will add them. | ||
| func verifyUserAndTokenFromOTT(conn *storage.Connection, params *VerifyParams, aud string) (*models.User, error) { |
There was a problem hiding this comment.
The summary is that we:
- Verify the OTT by looking it up and checking if it's expired
- Resolve the generic
type=emailrequest param type to the actual token type for post-verification steps - Find the user by the OTT's user ID
- Ensure that the user associated with the OTT is eligible to verify it, and validate identifier binding
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
xlgmokha
left a comment
xlgmokha
left a comment
There was a problem hiding this comment.
Nicely done! Nothing stood out for me as blockers. I left one question but it's not a blocker.
I'm not as familiar with this part of the codebase so you might choose to wait for another review from someone else or not. I trust your discretion.
| return mismatch.WithInternalMessage("user email does not match") | ||
| } | ||
| default: // Signup, Invite, Recovery, MagicLink | ||
| if params.Email == "" || !strings.EqualFold(user.GetEmail(), params.Email) { |
There was a problem hiding this comment.
question: is user.GetEmail() a safe fallback?
2 participants
What kind of change does this PR introduce?
Feat
What is the current behavior?
We dual-write token data to the
one_time_tokensanduserstable, but read token data from them inconsistently.What is the new behavior?
When a flag
GOTRUE_EXPERIMENTAL_ENABLE_OTT_AS_SOURCE_OF_TRUTHis enabled, we treatone_time_tokensas the source of truth for token data, and only lookup the user to validate eligibility and confirm identifier binding.Additional context
I've intentionally split out the Twilio Verify flow + Test OTP handling into a separate PR, but will wait until both are merged to release.
Closes AUTH-1553.