Skip to content

Commit 00c7dc6

Browse files
committed
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.
1 parent 31b4e74 commit 00c7dc6

2 files changed

Lines changed: 190 additions & 9 deletions

File tree

internal/api/verify.go

Lines changed: 96 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -789,9 +789,20 @@ func (a *API) verifyUserAndToken(conn *storage.Connection, params *VerifyParams,
789789
// challenge in the one_time_tokens table and derives the user from that row,
790790
// instead of finding the user by identifier and comparing the users.*_token
791791
// columns. A lookup miss is rejected as an expired or invalid token; there is
792-
// no fallback to the users columns. Test OTPs and Twilio Verify are not handled
793-
// yet on this path; a follow-up PR adds them.
792+
// no fallback to the users columns.
794793
func (a *API) verifyUserAndTokenFromOTT(conn *storage.Connection, params *VerifyParams, aud string) (*models.User, error) {
794+
config := a.config
795+
796+
// Twilio Verify and test OTPs are verified without a local challenge
797+
if params.Type == smsVerification || params.Type == phoneChangeVerification {
798+
if testOTP, ok := config.Sms.GetTestOTP(params.Phone, time.Now()); ok && params.Token == testOTP {
799+
return a.findUserForTestOTP(conn, params, aud)
800+
}
801+
if !config.Hook.SendSMS.Enabled && config.Sms.IsTwilioVerifyProvider() {
802+
return a.verifyPhoneWithTwilio(conn, params, aud)
803+
}
804+
}
805+
795806
ott, err := a.verifyOneTimeToken(conn, params)
796807
if err != nil {
797808
return nil, err
@@ -814,6 +825,72 @@ func (a *API) verifyUserAndTokenFromOTT(conn *storage.Connection, params *Verify
814825
return user, nil
815826
}
816827

828+
func (a *API) verifyPhoneWithTwilio(conn *storage.Connection, params *VerifyParams, aud string) (*models.User, error) {
829+
tokenType := models.ConfirmationToken
830+
if params.Type == phoneChangeVerification {
831+
tokenType = models.PhoneChangeToken
832+
}
833+
834+
ott, err := models.FindOneTimeTokenByRelatesTo(conn, params.Phone, tokenType)
835+
if models.IsNotFoundError(err) {
836+
return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err)
837+
} else if err != nil {
838+
return nil, apierrors.NewInternalServerError("Database error finding one time token").WithInternalError(err)
839+
}
840+
841+
user, err := models.FindUserByID(conn, ott.UserID)
842+
if models.IsNotFoundError(err) {
843+
return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err)
844+
} else if err != nil {
845+
return nil, apierrors.NewInternalServerError("Database error finding user").WithInternalError(err)
846+
}
847+
848+
pendingPhone := user.GetPhone()
849+
if params.Type == phoneChangeVerification {
850+
pendingPhone = user.PhoneChange
851+
}
852+
if pendingPhone != params.Phone {
853+
return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalMessage("user phone does not match")
854+
}
855+
if user.Aud != aud {
856+
return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalMessage("user audience does not match")
857+
}
858+
if user.IsBanned() {
859+
return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeUserBanned, "User is banned")
860+
}
861+
if err := a.verifyOTPWithTwilio(params.Phone, params.Token); err != nil {
862+
return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err)
863+
}
864+
return user, nil
865+
}
866+
867+
// findUserForTestOTP resolves the user for a phone verification whose code
868+
// matched a configured test OTP. A test OTP has no local challenge.
869+
func (a *API) findUserForTestOTP(conn *storage.Connection, params *VerifyParams, aud string) (*models.User, error) {
870+
var user *models.User
871+
var err error
872+
873+
switch params.Type {
874+
case phoneChangeVerification:
875+
user, err = models.FindUserByPhoneChangeAndAudience(conn, params.Phone, aud)
876+
case smsVerification:
877+
user, err = models.FindUserByPhoneAndAudience(conn, params.Phone, aud)
878+
default:
879+
// The caller only routes phone types here, so in practice this should never happen.
880+
return nil, apierrors.NewInternalServerError("Test OTP lookup called for non-phone verification type %q", params.Type)
881+
}
882+
if models.IsNotFoundError(err) {
883+
return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err)
884+
} else if err != nil {
885+
return nil, apierrors.NewInternalServerError("Database error finding user").WithInternalError(err)
886+
}
887+
888+
if user.IsBanned() {
889+
return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeUserBanned, "User is banned")
890+
}
891+
return user, nil
892+
}
893+
817894
func (a *API) verifyOneTimeToken(conn *storage.Connection, params *VerifyParams) (*models.OneTimeToken, error) {
818895
tokenTypes := verifyTypeToTokenTypes(params.Type)
819896
if len(tokenTypes) == 0 {
@@ -844,6 +921,23 @@ func (a *API) verifyOneTimeToken(conn *storage.Connection, params *VerifyParams)
844921
return ott, nil
845922
}
846923

924+
// verifyOTPWithTwilio asks Twilio Verify to check the code. Twilio generates
925+
// and delivers its own code, so there is no local challenge to compare.
926+
func (a *API) verifyOTPWithTwilio(phone, code string) error {
927+
smsProvider, err := sms_provider.GetSmsProvider(*a.config)
928+
if err != nil {
929+
return apierrors.NewInternalServerError("Failed to get SMS provider").WithInternalError(err)
930+
}
931+
twilioVerify, ok := smsProvider.(*sms_provider.TwilioVerifyProvider)
932+
if !ok {
933+
return apierrors.NewInternalServerError("SMS provider is not Twilio Verify")
934+
}
935+
if err := twilioVerify.VerifyOTP(phone, code); err != nil {
936+
return apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err)
937+
}
938+
return nil
939+
}
940+
847941
func verifyTypeToTokenTypes(verifyType string) []models.OneTimeTokenType {
848942
switch verifyType {
849943
case mail.EmailOTPVerification:

internal/api/verify_ott_parity_test.go

Lines changed: 94 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,11 @@ import (
99

1010
"github.com/gofrs/uuid"
1111
"github.com/stretchr/testify/require"
12+
"gopkg.in/h2non/gock.v1"
1213

1314
"github.com/supabase/auth/internal/api/apierrors"
15+
"github.com/supabase/auth/internal/api/sms_provider"
16+
"github.com/supabase/auth/internal/conf"
1417
"github.com/supabase/auth/internal/crypto"
1518
mail "github.com/supabase/auth/internal/mailer"
1619
"github.com/supabase/auth/internal/models"
@@ -24,12 +27,13 @@ import (
2427
// in the fixtures.
2528

2629
const (
27-
parityOTP = "123456"
28-
parityEmail = "test@example.com"
29-
parityPhone = "12345678"
30-
parityNewEmail = "new@example.com"
31-
parityNewPhone = "1234567890"
32-
parityForbidden = "Token has expired or is invalid"
30+
parityOTP = "123456"
31+
parityEmail = "test@example.com"
32+
parityPhone = "12345678"
33+
parityNewEmail = "new@example.com"
34+
parityNewPhone = "1234567890"
35+
parityForbidden = "Token has expired or is invalid"
36+
twilioServiceSid = "VA-parity-test"
3337
)
3438

3539
// otpParityOutcome is everything a client or an operator can observe after a
@@ -276,6 +280,53 @@ func (ts *VerifyTestSuite) TestVerifyOTPParityPhoneFlows() {
276280
},
277281
expected: phoneChanged,
278282
},
283+
{
284+
// A test OTP is accepted without any stored challenge. This is the
285+
// path app store reviewers and CI rely on.
286+
desc: "sms with a test OTP succeeds with no stored challenge",
287+
configure: func() func() {
288+
return ts.configureTestOTP(parityPhone, parityOTP)
289+
},
290+
seed: func(u *models.User) map[string]interface{} {
291+
return phoneOTPBody(smsVerification, parityPhone)
292+
},
293+
expected: phoneSignedUp,
294+
},
295+
{
296+
desc: "sms with a wrong code falls through the test OTP check and is rejected",
297+
configure: func() func() {
298+
return ts.configureTestOTP(parityPhone, "000000")
299+
},
300+
seed: func(u *models.User) map[string]interface{} {
301+
return phoneOTPBody(smsVerification, parityPhone)
302+
},
303+
expected: forbidden,
304+
},
305+
{
306+
// Twilio Verify generates and delivers its own code, so the locally
307+
// stored hash never matches what the user types. Twilio's answer
308+
// is the only thing that counts.
309+
desc: "sms with Twilio Verify accepts a code Twilio approves",
310+
configure: func() func() {
311+
return ts.configureTwilioVerify(map[string]interface{}{"status": "approved", "valid": true})
312+
},
313+
seed: func(u *models.User) map[string]interface{} {
314+
ts.seedChallenge(u, models.ConfirmationToken, parityPhone, crypto.GenerateTokenHash(parityPhone, "999999"), now, time.Hour)
315+
return phoneOTPBody(smsVerification, parityPhone)
316+
},
317+
expected: phoneSignedUp,
318+
},
319+
{
320+
desc: "sms with Twilio Verify rejects a code Twilio does not approve",
321+
configure: func() func() {
322+
return ts.configureTwilioVerify(map[string]interface{}{"status": "pending", "valid": false})
323+
},
324+
seed: func(u *models.User) map[string]interface{} {
325+
ts.seedChallenge(u, models.ConfirmationToken, parityPhone, phoneHash, now, time.Hour)
326+
return phoneOTPBody(smsVerification, parityPhone)
327+
},
328+
expected: forbidden,
329+
},
279330
}
280331

281332
ts.runOTPParityCases(cases)
@@ -330,7 +381,7 @@ func (ts *VerifyTestSuite) runOTPParityCases(cases []otpParityCase) {
330381

331382
// seedChallenge stores hash in the users column and the one_time_tokens row
332383
// for tokenType, mirroring what the send paths write. relatesTo is the address
333-
// or number the code was sent to.
384+
// or number the code was sent to; the Twilio Verify path finds the row by it.
334385
// Any other pending change on u is persisted at the same time.
335386
func (ts *VerifyTestSuite) seedChallenge(u *models.User, tokenType models.OneTimeTokenType, relatesTo, hash string, sentAt time.Time, validity time.Duration) {
336387
switch tokenType {
@@ -398,6 +449,42 @@ func (ts *VerifyTestSuite) observeOutcome(w *httptest.ResponseRecorder, userID u
398449
return outcome
399450
}
400451

452+
func (ts *VerifyTestSuite) configureTestOTP(phone, otp string) func() {
453+
previous := ts.Config.Sms.TestOTP
454+
ts.Config.Sms.TestOTP = map[string]string{phone: otp}
455+
return func() { ts.Config.Sms.TestOTP = previous }
456+
}
457+
458+
// configureTwilioVerify switches the SMS provider to Twilio Verify and arms a
459+
// single mocked VerificationCheck response.
460+
func (ts *VerifyTestSuite) configureTwilioVerify(response map[string]interface{}) func() {
461+
previousProvider := ts.Config.Sms.Provider
462+
previousTwilio := ts.Config.Sms.TwilioVerify
463+
previousMock := sms_provider.MockProvider
464+
465+
ts.Config.Sms.Provider = "twilio_verify"
466+
ts.Config.Sms.TwilioVerify = conf.TwilioVerifyProviderConfiguration{
467+
AccountSid: "AC-parity-test",
468+
AuthToken: "parity-test-token",
469+
MessageServiceSid: twilioServiceSid,
470+
}
471+
// The mock provider would short-circuit GetSmsProvider and never reach
472+
// the Twilio Verify type assertion.
473+
sms_provider.MockProvider = nil
474+
475+
gock.New("https://verify.twilio.com/v2/Services/" + twilioServiceSid + "/VerificationCheck").
476+
Post("").
477+
Reply(http.StatusOK).
478+
JSON(response)
479+
480+
return func() {
481+
gock.OffAll()
482+
sms_provider.MockProvider = previousMock
483+
ts.Config.Sms.TwilioVerify = previousTwilio
484+
ts.Config.Sms.Provider = previousProvider
485+
}
486+
}
487+
401488
func emailOTPBody(verifyType, email string) map[string]interface{} {
402489
return map[string]interface{}{
403490
"type": verifyType,

0 commit comments

Comments
 (0)