From 45310c3f7d7a8f2aee75d914e7044c9a84938a59 Mon Sep 17 00:00:00 2001 From: Anna Baker Date: Thu, 10 Sep 2026 10:43:14 -0400 Subject: [PATCH 1/2] feat(otp): handle test OTPs and Twilio Verify on the one_time_tokens path Restore the phone-provider branches removed from the parent PR. Test OTPs keep the identifier lookup because they have no stored challenge. Twilio Verify finds the challenge row by relates_to and asks Twilio to check the code. --- internal/api/verify.go | 2 +- internal/api/verify_ott.go | 101 ++++++++++++++++++++++++- internal/api/verify_ott_parity_test.go | 95 +++++++++++++++++++++-- 3 files changed, 185 insertions(+), 13 deletions(-) diff --git a/internal/api/verify.go b/internal/api/verify.go index 1009384d6f..ae0d05c42b 100644 --- a/internal/api/verify.go +++ b/internal/api/verify.go @@ -701,7 +701,7 @@ func (a *API) verifyUserAndToken(conn *storage.Connection, params *VerifyParams, config := a.config if config.Experimental.EnableOTTAsSourceOfTruth { - return verifyUserAndTokenFromOTT(conn, params, aud) + return a.verifyUserAndTokenFromOTT(conn, params, aud) } var user *models.User diff --git a/internal/api/verify_ott.go b/internal/api/verify_ott.go index c5383366ed..8516a0a39a 100644 --- a/internal/api/verify_ott.go +++ b/internal/api/verify_ott.go @@ -2,8 +2,10 @@ package api import ( "strings" + "time" "github.com/supabase/auth/internal/api/apierrors" + "github.com/supabase/auth/internal/api/sms_provider" mail "github.com/supabase/auth/internal/mailer" "github.com/supabase/auth/internal/models" "github.com/supabase/auth/internal/storage" @@ -13,11 +15,19 @@ import ( // 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) { +func (a *API) verifyUserAndTokenFromOTT(conn *storage.Connection, params *VerifyParams, aud string) (*models.User, error) { + config := a.config + + // Twilio Verify and test OTPs are verified without a local challenge + if params.Type == smsVerification || params.Type == phoneChangeVerification { + if testOTP, ok := config.Sms.GetTestOTP(params.Phone, time.Now()); ok && params.Token == testOTP { + return a.findUserForTestOTP(conn, params, aud) + } + if !config.Hook.SendSMS.Enabled && config.Sms.IsTwilioVerifyProvider() { + return a.verifyPhoneWithTwilio(conn, params, aud) + } + } - // TODO AUTH-1553: Add support for test OTPs and Twilio Verify on this path. ott, err := verifyOneTimeToken(conn, params) if err != nil { return nil, err @@ -43,6 +53,89 @@ func verifyUserAndTokenFromOTT(conn *storage.Connection, params *VerifyParams, a return user, nil } +func (a *API) verifyPhoneWithTwilio(conn *storage.Connection, params *VerifyParams, aud string) (*models.User, error) { + tokenType := models.ConfirmationToken + if params.Type == phoneChangeVerification { + tokenType = models.PhoneChangeToken + } + + ott, err := models.FindOneTimeTokenByRelatesTo(conn, params.Phone, tokenType) + 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 one time token").WithInternalError(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) + } + + pendingPhone := user.GetPhone() + if params.Type == phoneChangeVerification { + pendingPhone = user.PhoneChange + } + if pendingPhone != params.Phone { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalMessage("user phone does not match") + } + if user.Aud != aud { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalMessage("user audience does not match") + } + if user.IsBanned() { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeUserBanned, "User is banned") + } + if err := a.verifyOTPWithTwilio(params.Phone, params.Token); err != nil { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err) + } + return user, nil +} + +// findUserForTestOTP resolves the user for a phone verification whose code +// matched a configured test OTP. A test OTP has no local challenge. +func (a *API) findUserForTestOTP(conn *storage.Connection, params *VerifyParams, aud string) (*models.User, error) { + var user *models.User + var err error + + switch params.Type { + case phoneChangeVerification: + user, err = models.FindUserByPhoneChangeAndAudience(conn, params.Phone, aud) + case smsVerification: + user, err = models.FindUserByPhoneAndAudience(conn, params.Phone, aud) + default: + // The caller only routes phone types here, so in practice this should never happen. + return nil, apierrors.NewInternalServerError("Test OTP lookup called for non-phone verification type %q", params.Type) + } + 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 user.IsBanned() { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeUserBanned, "User is banned") + } + return user, nil +} + +// verifyOTPWithTwilio asks Twilio Verify to check the code. Twilio generates +// and delivers its own code, so there is no local challenge to compare. +func (a *API) verifyOTPWithTwilio(phone, code string) error { + smsProvider, err := sms_provider.GetSmsProvider(*a.config) + if err != nil { + return apierrors.NewInternalServerError("Failed to get SMS provider").WithInternalError(err) + } + twilioVerify, ok := smsProvider.(*sms_provider.TwilioVerifyProvider) + if !ok { + return apierrors.NewInternalServerError("SMS provider is not Twilio Verify") + } + if err := twilioVerify.VerifyOTP(phone, code); err != nil { + return apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err) + } + return nil +} + func verifyOneTimeToken(conn *storage.Connection, params *VerifyParams) (*models.OneTimeToken, error) { tokenTypes := verifyTypeToTokenTypes(params.Type) if len(tokenTypes) == 0 { diff --git a/internal/api/verify_ott_parity_test.go b/internal/api/verify_ott_parity_test.go index 07d63e55eb..467bbf32bf 100644 --- a/internal/api/verify_ott_parity_test.go +++ b/internal/api/verify_ott_parity_test.go @@ -9,8 +9,11 @@ import ( "github.com/gofrs/uuid" "github.com/stretchr/testify/require" + "gopkg.in/h2non/gock.v1" "github.com/supabase/auth/internal/api/apierrors" + "github.com/supabase/auth/internal/api/sms_provider" + "github.com/supabase/auth/internal/conf" "github.com/supabase/auth/internal/crypto" mail "github.com/supabase/auth/internal/mailer" "github.com/supabase/auth/internal/models" @@ -21,12 +24,13 @@ import ( // // These tests run every flow once per store with identical seeding and ensure the outcome is equal. const ( - parityOTP = "123456" - parityEmail = "test@example.com" - parityPhone = "12345678" - parityNewEmail = "new@example.com" - parityNewPhone = "1234567890" - parityForbidden = "Token has expired or is invalid" + parityOTP = "123456" + parityEmail = "test@example.com" + parityPhone = "12345678" + parityNewEmail = "new@example.com" + parityNewPhone = "1234567890" + parityForbidden = "Token has expired or is invalid" + twilioServiceSid = "VA-parity-test" ) // otpParityOutcome is everything a client or an operator can observe after a @@ -260,6 +264,45 @@ func (ts *VerifyTestSuite) TestVerifyOTPParityPhoneFlows() { requestBody: phoneOTPBody(phoneChangeVerification, parityNewPhone), expected: phoneChanged, }, + // A test OTP is accepted without any stored challenge. This is the + // path app store reviewers and CI rely on. + "sms with a test OTP succeeds with no stored challenge": { + configure: func() func() { + return ts.configureTestOTP(parityPhone, parityOTP) + }, + requestBody: phoneOTPBody(smsVerification, parityPhone), + expected: phoneSignedUp, + }, + "sms with a wrong code falls through the test OTP check and is rejected": { + configure: func() func() { + return ts.configureTestOTP(parityPhone, "000000") + }, + requestBody: phoneOTPBody(smsVerification, parityPhone), + expected: forbidden, + }, + // Twilio Verify generates and delivers its own code, so the locally + // stored hash never matches what the user types. Twilio's answer is the + // only thing that counts. + "sms with Twilio Verify accepts a code Twilio approves": { + configure: func() func() { + return ts.configureTwilioVerify(map[string]interface{}{"status": "approved", "valid": true}) + }, + seed: func(u *models.User) { + ts.seedChallenge(u, models.ConfirmationToken, parityPhone, crypto.GenerateTokenHash(parityPhone, "999999"), now, time.Hour) + }, + requestBody: phoneOTPBody(smsVerification, parityPhone), + expected: phoneSignedUp, + }, + "sms with Twilio Verify rejects a code Twilio does not approve": { + configure: func() func() { + return ts.configureTwilioVerify(map[string]interface{}{"status": "pending", "valid": false}) + }, + seed: func(u *models.User) { + ts.seedChallenge(u, models.ConfirmationToken, parityPhone, phoneHash, now, time.Hour) + }, + requestBody: phoneOTPBody(smsVerification, parityPhone), + expected: forbidden, + }, } ts.runOTPParityCases(cases) @@ -336,8 +379,8 @@ func (ts *VerifyTestSuite) saveUser(u *models.User) { // seedChallenge stores hash in the users column and the one_time_tokens row // for tokenType, mirroring what the send paths write. relatesTo is the address -// or number the code was sent to. It persists u, so it also saves any other -// change the case made. +// or number the code was sent to; the Twilio Verify path finds the row by it. +// It persists u, so it also saves any other change the case made. func (ts *VerifyTestSuite) seedChallenge(u *models.User, tokenType models.OneTimeTokenType, relatesTo, hash string, sentAt time.Time, validity time.Duration) { switch tokenType { case models.ConfirmationToken: @@ -488,6 +531,42 @@ func (ts *VerifyTestSuite) responseMsg(w *httptest.ResponseRecorder) string { return body.Msg } +func (ts *VerifyTestSuite) configureTestOTP(phone, otp string) func() { + previous := ts.Config.Sms.TestOTP + ts.Config.Sms.TestOTP = map[string]string{phone: otp} + return func() { ts.Config.Sms.TestOTP = previous } +} + +// configureTwilioVerify switches the SMS provider to Twilio Verify and arms a +// single mocked VerificationCheck response. +func (ts *VerifyTestSuite) configureTwilioVerify(response map[string]interface{}) func() { + previousProvider := ts.Config.Sms.Provider + previousTwilio := ts.Config.Sms.TwilioVerify + previousMock := sms_provider.MockProvider + + ts.Config.Sms.Provider = "twilio_verify" + ts.Config.Sms.TwilioVerify = conf.TwilioVerifyProviderConfiguration{ + AccountSid: "AC-parity-test", + AuthToken: "parity-test-token", + MessageServiceSid: twilioServiceSid, + } + // The mock provider would short-circuit GetSmsProvider and never reach + // the Twilio Verify type assertion. + sms_provider.MockProvider = nil + + gock.New("https://verify.twilio.com/v2/Services/" + twilioServiceSid + "/VerificationCheck"). + Post(""). + Reply(http.StatusOK). + JSON(response) + + return func() { + gock.OffAll() + sms_provider.MockProvider = previousMock + ts.Config.Sms.TwilioVerify = previousTwilio + ts.Config.Sms.Provider = previousProvider + } +} + func emailOTPBody(verifyType, email string) map[string]interface{} { return map[string]interface{}{ "type": verifyType, From 0e3e5232bd35a27318d87010436802d07c027192 Mon Sep 17 00:00:00 2001 From: Anna Baker Date: Fri, 11 Sep 2026 15:46:14 -0400 Subject: [PATCH 2/2] chore: clean up, add test coverage for SSO users --- internal/api/verify_ott.go | 51 ++++++++++++++++---------- internal/api/verify_ott_parity_test.go | 13 +++++++ 2 files changed, 44 insertions(+), 20 deletions(-) diff --git a/internal/api/verify_ott.go b/internal/api/verify_ott.go index 8516a0a39a..7679165fe3 100644 --- a/internal/api/verify_ott.go +++ b/internal/api/verify_ott.go @@ -54,9 +54,10 @@ func (a *API) verifyUserAndTokenFromOTT(conn *storage.Connection, params *Verify } func (a *API) verifyPhoneWithTwilio(conn *storage.Connection, params *VerifyParams, aud string) (*models.User, error) { - tokenType := models.ConfirmationToken - if params.Type == phoneChangeVerification { - tokenType = models.PhoneChangeToken + tokenType, ok := verifyTypeToTokenType(params.Type) + if !ok { + // The caller only routes phone types here, so in practice this should never happen. + return nil, apierrors.NewInternalServerError("Twilio Verify lookup called for unknown verification type %q", params.Type) } ott, err := models.FindOneTimeTokenByRelatesTo(conn, params.Phone, tokenType) @@ -73,16 +74,10 @@ func (a *API) verifyPhoneWithTwilio(conn *storage.Connection, params *VerifyPara return nil, apierrors.NewInternalServerError("Database error finding user").WithInternalError(err) } - pendingPhone := user.GetPhone() - if params.Type == phoneChangeVerification { - pendingPhone = user.PhoneChange - } - if pendingPhone != params.Phone { - return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalMessage("user phone does not match") - } - if user.Aud != aud { - return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalMessage("user audience does not match") + if err := validateUserForOTT(params, ott, user, aud); err != nil { + return nil, err } + if user.IsBanned() { return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeUserBanned, "User is banned") } @@ -156,22 +151,38 @@ func verifyOneTimeToken(conn *storage.Connection, params *VerifyParams) (*models return ott, nil } +// verifyTypeToTokenTypes returns nil for an unknown verification type. 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} + } + + tokenType, ok := verifyTypeToTokenType(verifyType) + if !ok { + return nil + } + return []models.OneTimeTokenType{tokenType} +} + +// verifyTypeToTokenType maps a verification type that has exactly one token +// type. ok is false for an unknown type. ConfirmationToken is the zero value of +// OneTimeTokenType, so callers must check ok instead of the returned type. +func verifyTypeToTokenType(verifyType string) (models.OneTimeTokenType, bool) { + switch verifyType { + case mail.SignupVerification, mail.InviteVerification: + return models.ConfirmationToken, true + case mail.RecoveryVerification, mail.MagicLinkVerification: + return models.RecoveryToken, true case smsVerification: - return []models.OneTimeTokenType{models.ConfirmationToken} + // phone signup codes are stored as confirmation tokens + return models.ConfirmationToken, true + case phoneChangeVerification: + return models.PhoneChangeToken, true default: - return nil + return 0, false } } diff --git a/internal/api/verify_ott_parity_test.go b/internal/api/verify_ott_parity_test.go index 467bbf32bf..fd3e3f4efb 100644 --- a/internal/api/verify_ott_parity_test.go +++ b/internal/api/verify_ott_parity_test.go @@ -303,6 +303,19 @@ func (ts *VerifyTestSuite) TestVerifyOTPParityPhoneFlows() { requestBody: phoneOTPBody(smsVerification, parityPhone), expected: forbidden, }, + // An SSO user has no local credentials to verify. Twilio approves the + // code here, so only the SSO check can reject the request. + "sms with Twilio Verify rejects an SSO user": { + configure: func() func() { + return ts.configureTwilioVerify(map[string]interface{}{"status": "approved", "valid": true}) + }, + seed: func(u *models.User) { + u.IsSSOUser = true + ts.seedChallenge(u, models.ConfirmationToken, parityPhone, phoneHash, now, time.Hour) + }, + requestBody: phoneOTPBody(smsVerification, parityPhone), + expected: forbidden, + }, } ts.runOTPParityCases(cases)