-
Notifications
You must be signed in to change notification settings - Fork 755
feat(otp): switch one_time_tokens table to source of truth #2788
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
annabkr
wants to merge
14
commits into
annabaker/auth-1553-ott-query-helpers
Choose a base branch
from
annabaker/auth-1553-switch-one_time_tokens-table-to-source-of-truth
base: annabaker/auth-1553-ott-query-helpers
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 12 commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
0121d3d
feat(otp): add config feature flag to EnableOTTAsSourceOfTruth
annabkr 301ec41
feat(otp): use one_time_tokens table as source of truth for verifyUse…
annabkr f42bd06
feat(otp): clean up and add tess
annabkr d7423a4
fix(otp): resolve verifying a user from the one_time_tokens row
annabkr 4caedac
fix: reorder functions to make diff more readable
annabkr ebdd96e
feat: validate user for ott
annabkr 0820b2b
chore: refactor
annabkr 869d961
chore(otp): change flagging logic to improve readability
annabkr 6d15a97
chore(otp): defer test OTP and Twilio Verify handling to a follow-up PR
annabkr a313e08
docs: clarify
annabkr 324860d
chore: refactor tests
annabkr d4cfac6
chore: clean up tests and move things around
annabkr 7cdc428
chore: refactor to make clearer
annabkr 7cea4ca
chore: improve test coverage
annabkr File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,133 @@ | ||
| package api | ||
|
|
||
| import ( | ||
| "strings" | ||
|
|
||
| "github.com/supabase/auth/internal/api/apierrors" | ||
| mail "github.com/supabase/auth/internal/mailer" | ||
| "github.com/supabase/auth/internal/models" | ||
| "github.com/supabase/auth/internal/storage" | ||
| ) | ||
|
|
||
| // verifyUserAndTokenFromOTT is the EnableOTTAsSourceOfTruth path. It finds the | ||
| // challenge in the one_time_tokens table and derives the user from that row, | ||
| // instead of finding the user by identifier and comparing the users.*_token | ||
| // 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) { | ||
|
|
||
| // TODO AUTH-1553: Add support for test OTPs and Twilio Verify on this path. | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Separating this out to make it easier to review, but they'll be merged together. |
||
| ott, err := verifyOneTimeToken(conn, params) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| user, err := models.FindUserByID(conn, ott.UserID) | ||
| if models.IsNotFoundError(err) { | ||
| return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err) | ||
| } else if err != nil { | ||
| return nil, apierrors.NewInternalServerError("Database error finding user").WithInternalError(err) | ||
| } | ||
|
|
||
| if err := validateUserForOTT(params, ott, user, aud); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if user.IsBanned() { | ||
| return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeUserBanned, "User is banned") | ||
| } | ||
| return user, nil | ||
| } | ||
|
|
||
| func verifyOneTimeToken(conn *storage.Connection, params *VerifyParams) (*models.OneTimeToken, error) { | ||
| tokenTypes := verifyTypeToTokenTypes(params.Type) | ||
| if len(tokenTypes) == 0 { | ||
| return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalMessage("unknown verification type") | ||
| } | ||
|
|
||
| ott, err := models.FindOneTimeTokenWithPKCEFallback(conn, params.TokenHash, tokenTypes...) | ||
|
annabkr marked this conversation as resolved.
|
||
| if models.IsNotFoundError(err) { | ||
| return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalMessage("one time token not found") | ||
| } else if err != nil { | ||
| return nil, apierrors.NewInternalServerError("Database error finding one time token").WithInternalError(err) | ||
| } | ||
|
|
||
| if ott.IsExpired() { | ||
| return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalMessage("one time token has expired") | ||
| } | ||
|
|
||
| // The generic email type needs to match to the flow that issues the token, so the caller runs the right post-verify step | ||
| if params.Type == mail.EmailOTPVerification { | ||
| switch ott.TokenType { | ||
| case models.ConfirmationToken: | ||
| params.Type = mail.SignupVerification | ||
| case models.RecoveryToken: | ||
| params.Type = mail.MagicLinkVerification | ||
| } | ||
| } | ||
|
|
||
| return ott, nil | ||
| } | ||
|
|
||
| func verifyTypeToTokenTypes(verifyType string) []models.OneTimeTokenType { | ||
| switch verifyType { | ||
| case mail.EmailOTPVerification: | ||
| return []models.OneTimeTokenType{models.ConfirmationToken, models.RecoveryToken} | ||
| case mail.SignupVerification, mail.InviteVerification: | ||
| return []models.OneTimeTokenType{models.ConfirmationToken} | ||
| case mail.RecoveryVerification, mail.MagicLinkVerification: | ||
| return []models.OneTimeTokenType{models.RecoveryToken} | ||
| case mail.EmailChangeVerification: | ||
| return []models.OneTimeTokenType{models.EmailChangeTokenCurrent, models.EmailChangeTokenNew} | ||
| case phoneChangeVerification: | ||
| return []models.OneTimeTokenType{models.PhoneChangeToken} | ||
| case smsVerification: | ||
| return []models.OneTimeTokenType{models.ConfirmationToken} | ||
| default: | ||
| return nil | ||
| } | ||
| } | ||
|
|
||
| // validateUserForOTT checks that the user found from a one_time_tokens row is | ||
| // the one the request is entitled to act on. | ||
| // | ||
| // The legacy path gets these guarantees for free from its identifier-keyed lookups (which also filter on | ||
| // aud and is_sso_user), so a mismatch there is a not-found. | ||
| // | ||
| // The one_time_tokens path finds the user by token hash, so it has to check the binding itself. | ||
| func validateUserForOTT(params *VerifyParams, ott *models.OneTimeToken, user *models.User, aud string) error { | ||
| mismatch := apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid") | ||
|
|
||
| if user.IsSSOUser { | ||
| return mismatch.WithInternalMessage("SSO users cannot be verified with one time tokens") | ||
| } | ||
|
|
||
| if user.Aud != aud { | ||
| return mismatch.WithInternalMessage("user audience does not match") | ||
| } | ||
|
|
||
| switch params.Type { | ||
| case smsVerification: | ||
| if params.Phone == "" || user.GetPhone() != params.Phone { | ||
| return mismatch.WithInternalMessage("user phone does not match") | ||
| } | ||
| case phoneChangeVerification: | ||
| if params.Phone == "" || user.PhoneChange != params.Phone { | ||
| return mismatch.WithInternalMessage("user phone does not match") | ||
| } | ||
| case mail.EmailChangeVerification: | ||
| expected := user.EmailChange | ||
| if ott.TokenType == models.EmailChangeTokenCurrent { | ||
| expected = user.GetEmail() | ||
| } | ||
| if params.Email == "" || !strings.EqualFold(expected, params.Email) { | ||
| return mismatch.WithInternalMessage("user email does not match") | ||
| } | ||
| default: // Signup, Invite, Recovery, MagicLink | ||
| if params.Email == "" || !strings.EqualFold(user.GetEmail(), params.Email) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: is |
||
| return mismatch.WithInternalMessage("user email does not match") | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
The summary is that we:
type=emailrequest param type to the actual token type for post-verification steps