diff --git a/example.env b/example.env index 39c207ea34..58745a06fe 100644 --- a/example.env +++ b/example.env @@ -286,3 +286,7 @@ GOTRUE_MFA_RECOVERY_CODES_LOCKOUT_DURATION="15m" # between 1m and 24h # account that didn't have one (e.g. a user who signed up with an external # provider and later sets a password). GOTRUE_EXPERIMENTAL_CREATE_EMAIL_IDENTITY_ON_PASSWORD_SET_ENABLED="false" + +# Reads one-time tokens from the one_time_tokens table instead of the users +# table when verifying typed OTPs. A miss is rejected as an expired or invalid token. +GOTRUE_EXPERIMENTAL_ENABLE_OTT_AS_SOURCE_OF_TRUTH="false" diff --git a/internal/api/verify.go b/internal/api/verify.go index 4386a1dcda..b5693f75a4 100644 --- a/internal/api/verify.go +++ b/internal/api/verify.go @@ -699,85 +699,249 @@ func (a *API) verifyTokenHash(conn *storage.Connection, params *VerifyParams) (* func (a *API) verifyUserAndToken(conn *storage.Connection, params *VerifyParams, aud string) (*models.User, error) { config := a.config + if config.Experimental.EnableOTTAsSourceOfTruth { + // 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) + } + } + + ott, err := a.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 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") + } + return user, nil + } else { + var user *models.User + var err error + tokenHash := params.TokenHash + + var isValid bool + switch params.Type { + case phoneChangeVerification: + user, err = models.FindUserByPhoneChangeAndAudience(conn, params.Phone, aud) + case smsVerification: + user, err = models.FindUserByPhoneAndAudience(conn, params.Phone, aud) + case mail.EmailChangeVerification: + // Since the email change could be trigger via the implicit or PKCE flow, + // the query used has to also check if the token saved in the db contains the pkce_ prefix + user, err = models.FindUserForEmailChange(conn, params.Email, tokenHash, aud, config.Mailer.SecureEmailChangeEnabled) + default: + user, err = models.FindUserByEmailAndAudience(conn, params.Email, aud) + } + + if err != nil { + if models.IsNotFoundError(err) { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err) + } + return nil, apierrors.NewInternalServerError("Database error finding user").WithInternalError(err) + } + + if user.IsBanned() { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeUserBanned, "User is banned") + } + + smsProvider, _ := sms_provider.GetSmsProvider(*config) + switch params.Type { + case mail.EmailOTPVerification: + // if the type is emailOTPVerification, we'll check both the confirmation_token and recovery_token columns + if isOtpValid(tokenHash, user.ConfirmationToken, user.ConfirmationSentAt, config.Mailer.OtpExp) { + isValid = true + params.Type = mail.SignupVerification + } else if isOtpValid(tokenHash, user.RecoveryToken, user.RecoverySentAt, config.Mailer.OtpExp) { + isValid = true + params.Type = mail.MagicLinkVerification + } else { + isValid = false + } + case mail.SignupVerification, mail.InviteVerification: + isValid = isOtpValid(tokenHash, user.ConfirmationToken, user.ConfirmationSentAt, config.Mailer.OtpExp) + case mail.RecoveryVerification, mail.MagicLinkVerification: + isValid = isOtpValid(tokenHash, user.RecoveryToken, user.RecoverySentAt, config.Mailer.OtpExp) + case mail.EmailChangeVerification: + isValid = isOtpValid(tokenHash, user.EmailChangeTokenCurrent, user.EmailChangeSentAt, config.Mailer.OtpExp) || + isOtpValid(tokenHash, user.EmailChangeTokenNew, user.EmailChangeSentAt, config.Mailer.OtpExp) + case phoneChangeVerification, smsVerification: + // Check if test OP, if so skip validation and return user + if testOTP, ok := config.Sms.GetTestOTP(params.Phone, time.Now()); ok { + if params.Token == testOTP { + return user, nil + } + } + + phone := params.Phone + sentAt := user.ConfirmationSentAt + expectedToken := user.ConfirmationToken + if params.Type == phoneChangeVerification { + phone = user.PhoneChange + sentAt = user.PhoneChangeSentAt + expectedToken = user.PhoneChangeToken + } + + if !config.Hook.SendSMS.Enabled && config.Sms.IsTwilioVerifyProvider() { + if err := smsProvider.(*sms_provider.TwilioVerifyProvider).VerifyOTP(phone, params.Token); err != nil { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err) + } + return user, nil + } + isValid = isOtpValid(tokenHash, expectedToken, sentAt, config.Sms.OtpExp) + } + if !isValid { + return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalMessage("token has expired or is invalid") + } + 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 // Should we use GetPhoneChange? + } + 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 - tokenHash := params.TokenHash switch params.Type { case phoneChangeVerification: user, err = models.FindUserByPhoneChangeAndAudience(conn, params.Phone, aud) case smsVerification: user, err = models.FindUserByPhoneAndAudience(conn, params.Phone, aud) - case mail.EmailChangeVerification: - // Since the email change could be trigger via the implicit or PKCE flow, - // the query used has to also check if the token saved in the db contains the pkce_ prefix - user, err = models.FindUserForEmailChange(conn, params.Email, tokenHash, aud, config.Mailer.SecureEmailChangeEnabled) default: - user, err = models.FindUserByEmailAndAudience(conn, params.Email, aud) + // 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 err != nil { - if models.IsNotFoundError(err) { - return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err) - } + 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 +} - var isValid bool +func (a *API) 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") + } - smsProvider, _ := sms_provider.GetSmsProvider(*config) - switch params.Type { - case mail.EmailOTPVerification: - // if the type is emailOTPVerification, we'll check both the confirmation_token and recovery_token columns - if isOtpValid(tokenHash, user.ConfirmationToken, user.ConfirmationSentAt, config.Mailer.OtpExp) { - isValid = true + ott, err := models.FindOneTimeTokenWithPKCEFallback(conn, params.TokenHash, tokenTypes...) + 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 - } else if isOtpValid(tokenHash, user.RecoveryToken, user.RecoverySentAt, config.Mailer.OtpExp) { - isValid = true + case models.RecoveryToken: params.Type = mail.MagicLinkVerification - } else { - isValid = false - } - case mail.SignupVerification, mail.InviteVerification: - isValid = isOtpValid(tokenHash, user.ConfirmationToken, user.ConfirmationSentAt, config.Mailer.OtpExp) - case mail.RecoveryVerification, mail.MagicLinkVerification: - isValid = isOtpValid(tokenHash, user.RecoveryToken, user.RecoverySentAt, config.Mailer.OtpExp) - case mail.EmailChangeVerification: - isValid = isOtpValid(tokenHash, user.EmailChangeTokenCurrent, user.EmailChangeSentAt, config.Mailer.OtpExp) || - isOtpValid(tokenHash, user.EmailChangeTokenNew, user.EmailChangeSentAt, config.Mailer.OtpExp) - case phoneChangeVerification, smsVerification: - if testOTP, ok := config.Sms.GetTestOTP(params.Phone, time.Now()); ok { - if params.Token == testOTP { - return user, nil - } } + } - phone := params.Phone - sentAt := user.ConfirmationSentAt - expectedToken := user.ConfirmationToken - if params.Type == phoneChangeVerification { - phone = user.PhoneChange - sentAt = user.PhoneChangeSentAt - expectedToken = user.PhoneChangeToken - } + return ott, nil +} - if !config.Hook.SendSMS.Enabled && config.Sms.IsTwilioVerifyProvider() { - if err := smsProvider.(*sms_provider.TwilioVerifyProvider).VerifyOTP(phone, params.Token); err != nil { - return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalError(err) - } - return user, nil - } - isValid = isOtpValid(tokenHash, expectedToken, sentAt, config.Sms.OtpExp) +// 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 +} - if !isValid { - return nil, apierrors.NewForbiddenError(apierrors.ErrorCodeOTPExpired, "Token has expired or is invalid").WithInternalMessage("token has expired or is invalid") +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 } - return user, nil } // isOtpValid checks the actual otp sent against the expected otp and ensures that it's within the valid window diff --git a/internal/api/verify_ott_parity_test.go b/internal/api/verify_ott_parity_test.go new file mode 100644 index 0000000000..5811cbd414 --- /dev/null +++ b/internal/api/verify_ott_parity_test.go @@ -0,0 +1,502 @@ +package api + +import ( + "bytes" + "encoding/json" + "net/http" + "net/http/httptest" + "time" + + "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" +) + +// The typed-OTP verify path can read the challenge from either the legacy +// users.*_token columns or, with EnableOTTAsSourceOfTruth, from the +// one_time_tokens table. These tests run every flow once per store and require +// the observable outcome to be identical. Both stores are seeded the way the +// send paths seed them, so a failure here is a divergence in verify logic, not +// in the fixtures. + +const ( + 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 +// POST /verify: the HTTP result, the user state it left, and the audit action +// it recorded. +type otpParityOutcome struct { + Status int + ErrorCode string + Msg string + Action string + EmailConfirmed bool + PhoneConfirmed bool + Email string + Phone string +} + +type otpParityCase struct { + desc string + // seed writes the challenge to both stores and returns the request body. + // It receives a freshly created, unconfirmed user. + seed func(u *models.User) map[string]interface{} + // configure applies per-case config and returns a function that undoes + // it. It runs once per store, so consumable mocks are re-armed each time. + configure func() func() + expected otpParityOutcome +} + +func (ts *VerifyTestSuite) TestVerifyOTPParityEmailFlows() { + now := time.Now() + expired := now.Add(-48 * time.Hour) + emailHash := crypto.GenerateTokenHash(parityEmail, parityOTP) + + baseline := otpParityOutcome{Email: parityEmail, Phone: parityPhone} + forbidden := baseline + forbidden.Status = http.StatusForbidden + forbidden.ErrorCode = apierrors.ErrorCodeOTPExpired + forbidden.Msg = parityForbidden + + signedUp := baseline + signedUp.Status = http.StatusOK + signedUp.Action = string(models.UserSignedUpAction) + signedUp.EmailConfirmed = true + + loggedIn := baseline + loggedIn.Status = http.StatusOK + loggedIn.Action = string(models.LoginAction) + loggedIn.EmailConfirmed = true + + cases := []otpParityCase{ + { + desc: "signup with a valid code confirms the user", + seed: func(u *models.User) map[string]interface{} { + ts.seedChallenge(u, models.ConfirmationToken, parityEmail, emailHash, now, time.Hour) + return emailOTPBody(mail.SignupVerification, parityEmail) + }, + expected: signedUp, + }, + { + desc: "signup with an expired code is rejected", + seed: func(u *models.User) map[string]interface{} { + ts.seedChallenge(u, models.ConfirmationToken, parityEmail, emailHash, expired, -time.Hour) + return emailOTPBody(mail.SignupVerification, parityEmail) + }, + expected: forbidden, + }, + { + desc: "signup with the wrong code is rejected", + seed: func(u *models.User) map[string]interface{} { + ts.seedChallenge(u, models.ConfirmationToken, parityEmail, crypto.GenerateTokenHash(parityEmail, "999999"), now, time.Hour) + return emailOTPBody(mail.SignupVerification, parityEmail) + }, + expected: forbidden, + }, + { + desc: "invite with a valid code confirms the user", + seed: func(u *models.User) map[string]interface{} { + u.InvitedAt = &now + ts.seedChallenge(u, models.ConfirmationToken, parityEmail, emailHash, now, time.Hour) + return emailOTPBody(mail.InviteVerification, parityEmail) + }, + expected: signedUp, + }, + { + desc: "magiclink with a valid code signs a confirmed user in", + seed: func(u *models.User) map[string]interface{} { + u.EmailConfirmedAt = &now + ts.seedChallenge(u, models.RecoveryToken, parityEmail, emailHash, now, time.Hour) + return emailOTPBody(mail.MagicLinkVerification, parityEmail) + }, + expected: loggedIn, + }, + { + desc: "recovery with a valid code signs a confirmed user in", + seed: func(u *models.User) map[string]interface{} { + u.EmailConfirmedAt = &now + ts.seedChallenge(u, models.RecoveryToken, parityEmail, emailHash, now, time.Hour) + return emailOTPBody(mail.RecoveryVerification, parityEmail) + }, + expected: loggedIn, + }, + { + // Tokens issued through the PKCE flow are stored with a pkce_ + // prefix. A plain code must still match them. + desc: "magiclink with a pkce_ prefixed stored hash accepts the plain code", + seed: func(u *models.User) map[string]interface{} { + u.EmailConfirmedAt = &now + ts.seedChallenge(u, models.RecoveryToken, parityEmail, PKCEPrefix+emailHash, now, time.Hour) + return emailOTPBody(mail.MagicLinkVerification, parityEmail) + }, + expected: loggedIn, + }, + { + // The generic "email" type must resolve to the signup flow when the + // stored challenge is a confirmation token. + desc: "email type with a confirmation token runs the signup flow", + seed: func(u *models.User) map[string]interface{} { + ts.seedChallenge(u, models.ConfirmationToken, parityEmail, emailHash, now, time.Hour) + return emailOTPBody(mail.EmailOTPVerification, parityEmail) + }, + expected: signedUp, + }, + { + // The generic "email" type must resolve to the magiclink flow when + // the stored challenge is a recovery token. + desc: "email type with a recovery token runs the magiclink flow", + seed: func(u *models.User) map[string]interface{} { + u.EmailConfirmedAt = &now + ts.seedChallenge(u, models.RecoveryToken, parityEmail, emailHash, now, time.Hour) + return emailOTPBody(mail.EmailOTPVerification, parityEmail) + }, + expected: loggedIn, + }, + { + desc: "email type with no matching challenge is rejected", + seed: func(u *models.User) map[string]interface{} { + return emailOTPBody(mail.EmailOTPVerification, parityEmail) + }, + expected: forbidden, + }, + { + desc: "email change with a valid code moves the user to the new address", + configure: func() func() { + previous := ts.Config.Mailer.SecureEmailChangeEnabled + ts.Config.Mailer.SecureEmailChangeEnabled = false + return func() { ts.Config.Mailer.SecureEmailChangeEnabled = previous } + }, + seed: func(u *models.User) map[string]interface{} { + u.EmailChange = parityNewEmail + ts.seedChallenge(u, models.EmailChangeTokenNew, parityNewEmail, crypto.GenerateTokenHash(parityNewEmail, parityOTP), now, time.Hour) + return emailOTPBody(mail.EmailChangeVerification, parityNewEmail) + }, + expected: otpParityOutcome{ + Status: http.StatusOK, + Action: string(models.UserModifiedAction), + EmailConfirmed: true, + Email: parityNewEmail, + Phone: parityPhone, + }, + }, + { + desc: "a banned user is rejected before the challenge is checked", + seed: func(u *models.User) map[string]interface{} { + bannedUntil := now.Add(time.Hour) + u.BannedUntil = &bannedUntil + ts.seedChallenge(u, models.ConfirmationToken, parityEmail, emailHash, now, time.Hour) + return emailOTPBody(mail.SignupVerification, parityEmail) + }, + expected: otpParityOutcome{ + Status: http.StatusForbidden, + ErrorCode: apierrors.ErrorCodeUserBanned, + Msg: "User is banned", + Email: parityEmail, + Phone: parityPhone, + }, + }, + { + desc: "an unknown verification type is rejected", + seed: func(u *models.User) map[string]interface{} { + ts.seedChallenge(u, models.ConfirmationToken, parityEmail, emailHash, now, time.Hour) + return emailOTPBody("bogus", parityEmail) + }, + expected: forbidden, + }, + } + + ts.runOTPParityCases(cases) +} + +func (ts *VerifyTestSuite) TestVerifyOTPParityPhoneFlows() { + now := time.Now() + expired := now.Add(-48 * time.Hour) + phoneHash := crypto.GenerateTokenHash(parityPhone, parityOTP) + + baseline := otpParityOutcome{Email: parityEmail, Phone: parityPhone} + forbidden := baseline + forbidden.Status = http.StatusForbidden + forbidden.ErrorCode = apierrors.ErrorCodeOTPExpired + forbidden.Msg = parityForbidden + + phoneSignedUp := baseline + phoneSignedUp.Status = http.StatusOK + phoneSignedUp.Action = string(models.UserSignedUpAction) + phoneSignedUp.PhoneConfirmed = true + + phoneChanged := otpParityOutcome{ + Status: http.StatusOK, + Action: string(models.UserModifiedAction), + PhoneConfirmed: true, + Email: parityEmail, + Phone: parityNewPhone, + } + + cases := []otpParityCase{ + { + desc: "sms with a valid code confirms the phone", + seed: func(u *models.User) map[string]interface{} { + ts.seedChallenge(u, models.ConfirmationToken, parityPhone, phoneHash, now, time.Hour) + return phoneOTPBody(smsVerification, parityPhone) + }, + expected: phoneSignedUp, + }, + { + desc: "sms with an expired code is rejected", + seed: func(u *models.User) map[string]interface{} { + ts.seedChallenge(u, models.ConfirmationToken, parityPhone, phoneHash, expired, -time.Hour) + return phoneOTPBody(smsVerification, parityPhone) + }, + expected: forbidden, + }, + { + desc: "sms with the wrong code is rejected", + seed: func(u *models.User) map[string]interface{} { + ts.seedChallenge(u, models.ConfirmationToken, parityPhone, crypto.GenerateTokenHash(parityPhone, "999999"), now, time.Hour) + return phoneOTPBody(smsVerification, parityPhone) + }, + expected: forbidden, + }, + { + desc: "phone change with a valid code moves the user to the new number", + seed: func(u *models.User) map[string]interface{} { + u.PhoneChange = parityNewPhone + ts.seedChallenge(u, models.PhoneChangeToken, parityNewPhone, crypto.GenerateTokenHash(parityNewPhone, parityOTP), now, time.Hour) + return 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. + desc: "sms with a test OTP succeeds with no stored challenge", + configure: func() func() { + return ts.configureTestOTP(parityPhone, parityOTP) + }, + seed: func(u *models.User) map[string]interface{} { + return phoneOTPBody(smsVerification, parityPhone) + }, + expected: phoneSignedUp, + }, + { + desc: "sms with a wrong code falls through the test OTP check and is rejected", + configure: func() func() { + return ts.configureTestOTP(parityPhone, "000000") + }, + seed: func(u *models.User) map[string]interface{} { + return 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. + desc: "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) map[string]interface{} { + ts.seedChallenge(u, models.ConfirmationToken, parityPhone, crypto.GenerateTokenHash(parityPhone, "999999"), now, time.Hour) + return phoneOTPBody(smsVerification, parityPhone) + }, + expected: phoneSignedUp, + }, + { + desc: "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) map[string]interface{} { + ts.seedChallenge(u, models.ConfirmationToken, parityPhone, phoneHash, now, time.Hour) + return phoneOTPBody(smsVerification, parityPhone) + }, + expected: forbidden, + }, + } + + ts.runOTPParityCases(cases) +} + +// runOTPParityCases runs each case against both stores and asserts that both +// produce the expected outcome and agree with each other. +func (ts *VerifyTestSuite) runOTPParityCases(cases []otpParityCase) { + originalFlag := ts.Config.Experimental.EnableOTTAsSourceOfTruth + defer func() { ts.Config.Experimental.EnableOTTAsSourceOfTruth = originalFlag }() + + modes := []struct { + name string + flag bool + }{ + {name: "legacy users columns", flag: false}, + {name: "one_time_tokens", flag: true}, + } + + for _, caseItem := range cases { + c := caseItem + ts.Run(c.desc, func() { + outcomes := make(map[string]otpParityOutcome, len(modes)) + + for _, mode := range modes { + m := mode + ts.Run(m.name, func() { + ts.SetupTest() + ts.Config.Experimental.EnableOTTAsSourceOfTruth = m.flag + if c.configure != nil { + restore := c.configure() + defer restore() + } + + u, err := models.FindUserByEmailAndAudience(ts.API.db, parityEmail, ts.Config.JWT.Aud) + require.NoError(ts.T(), err) + body := c.seed(u) + + since := time.Now() + w := ts.postVerify(body) + outcome := ts.observeOutcome(w, u.ID, since) + require.Equal(ts.T(), c.expected, outcome) + outcomes[m.name] = outcome + }) + } + + require.Equal(ts.T(), outcomes[modes[0].name], outcomes[modes[1].name], + "legacy and one_time_tokens paths must produce the same outcome") + }) + } +} + +// 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; the Twilio Verify path finds the row by it. +// Any other pending change on u is persisted at the same time. +func (ts *VerifyTestSuite) seedChallenge(u *models.User, tokenType models.OneTimeTokenType, relatesTo, hash string, sentAt time.Time, validity time.Duration) { + switch tokenType { + case models.ConfirmationToken: + u.ConfirmationToken = hash + u.ConfirmationSentAt = &sentAt + case models.RecoveryToken: + u.RecoveryToken = hash + u.RecoverySentAt = &sentAt + case models.EmailChangeTokenNew: + u.EmailChangeTokenNew = hash + u.EmailChangeSentAt = &sentAt + case models.PhoneChangeToken: + u.PhoneChangeToken = hash + u.PhoneChangeSentAt = &sentAt + default: + ts.T().Fatalf("seedChallenge does not support token type %s", tokenType) + } + + require.NoError(ts.T(), ts.API.db.Update(u)) + require.NoError(ts.T(), models.CreateOneTimeToken(ts.API.db, u.ID, relatesTo, hash, tokenType, validity)) +} + +func (ts *VerifyTestSuite) postVerify(body map[string]interface{}) *httptest.ResponseRecorder { + var buffer bytes.Buffer + require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(body)) + + req := httptest.NewRequest(http.MethodPost, "http://localhost/verify", &buffer) + req.Header.Set("Content-Type", "application/json") + + w := httptest.NewRecorder() + ts.API.handler.ServeHTTP(w, req) + return w +} + +// observeOutcome collects the response and the resulting user state. Only +// audit entries written after since are considered, so earlier entries in the +// same test run cannot leak into the result. +func (ts *VerifyTestSuite) observeOutcome(w *httptest.ResponseRecorder, userID uuid.UUID, since time.Time) otpParityOutcome { + outcome := otpParityOutcome{Status: w.Code} + + if w.Code != http.StatusOK { + var body struct { + ErrorCode string `json:"error_code"` + Msg string `json:"msg"` + } + require.NoError(ts.T(), json.NewDecoder(w.Body).Decode(&body)) + outcome.ErrorCode = body.ErrorCode + outcome.Msg = body.Msg + } + + u, err := models.FindUserByID(ts.API.db, userID) + require.NoError(ts.T(), err) + outcome.EmailConfirmed = u.EmailConfirmedAt != nil + outcome.PhoneConfirmed = u.PhoneConfirmedAt != nil + outcome.Email = u.GetEmail() + outcome.Phone = u.GetPhone() + + logs, err := models.FindAuditLogEntries(ts.API.db, nil, "", nil) + require.NoError(ts.T(), err) + if len(logs) > 0 && !logs[0].CreatedAt.Before(since) { + outcome.Action, _ = logs[0].Payload["action"].(string) + } + + return outcome +} + +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, + "token": parityOTP, + "email": email, + } +} + +func phoneOTPBody(verifyType, phone string) map[string]interface{} { + return map[string]interface{}{ + "type": verifyType, + "token": parityOTP, + "phone": phone, + } +} diff --git a/internal/conf/configuration.go b/internal/conf/configuration.go index 7484c1a610..5a4ec186e6 100644 --- a/internal/conf/configuration.go +++ b/internal/conf/configuration.go @@ -412,6 +412,13 @@ type ExperimentalConfiguration struct { // one (e.g. a user who signed up with an external provider and later sets a password). // Env: GOTRUE_EXPERIMENTAL_CREATE_EMAIL_IDENTITY_ON_PASSWORD_SET_ENABLED=true CreateEmailIdentityOnPasswordSetEnabled bool `split_words:"true" default:"false"` + + // EnableOTTAsSourceOfTruth makes the typed-OTP verification path read the + // challenge from the one_time_tokens table instead of the users.*_token + // columns. A lookup miss is rejected as an expired or invalid token; there is + // no fallback to the users columns. + // Env: GOTRUE_EXPERIMENTAL_ENABLE_OTT_AS_SOURCE_OF_TRUTH=true + EnableOTTAsSourceOfTruth bool `split_words:"true" default:"false"` } // ReloadingConfiguration holds the configuration values for runtime diff --git a/internal/models/one_time_token.go b/internal/models/one_time_token.go index 7bec1c6743..bc0e466925 100644 --- a/internal/models/one_time_token.go +++ b/internal/models/one_time_token.go @@ -118,6 +118,11 @@ type OneTimeToken struct { ExpiresAt *time.Time `json:"expires_at" db:"expires_at"` } +// 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) +} + func (OneTimeToken) TableName() string { return "one_time_tokens" } @@ -189,6 +194,34 @@ func FindOneTimeToken(tx *storage.Connection, tokenHash string, tokenTypes ...On return oneTimeToken, nil } +// FindOneTimeTokenWithPKCEFallback finds the one time token of the given type by a token hash. +// If the token is not found, it will try to find a token with the "pkce_" prefix. +// It returns OneTimeTokenNotFoundError when no row exists. +func FindOneTimeTokenWithPKCEFallback(tx *storage.Connection, tokenHash string, tokenTypes ...OneTimeTokenType) (*OneTimeToken, error) { + oneTimeToken, err := FindOneTimeToken(tx, tokenHash, tokenTypes...) + if IsNotFoundError(err) { + oneTimeToken, err = FindOneTimeToken(tx, "pkce_"+tokenHash, tokenTypes...) + } + return oneTimeToken, err +} + +// FindOneTimeTokenByRelatesTo finds the newest one time token of the given token type by the relatesTo field. +// It returns OneTimeTokenNotFoundError when no row exists. +func FindOneTimeTokenByRelatesTo(tx *storage.Connection, relatesTo string, tokenType OneTimeTokenType) (*OneTimeToken, error) { + oneTimeToken := &OneTimeToken{} + + err := tx.Eager().Q(). + Where("token_type = ? and relates_to = ?", tokenType, strings.ToLower(relatesTo)). + Order("created_at desc"). + First(oneTimeToken) + if errors.Cause(err) == sql.ErrNoRows { + return nil, OneTimeTokenNotFoundError{} + } else if err != nil { + return nil, errors.Wrap(err, "error finding one time token") + } + return oneTimeToken, nil +} + // FindUserByOneTimeToken finds the user holding the one-time token matching // tokenHash for any of the given token types. func FindUserByOneTimeToken(tx *storage.Connection, tokenHash string, tokenTypes ...OneTimeTokenType) (*User, error) { diff --git a/internal/models/one_time_token_test.go b/internal/models/one_time_token_test.go index 929998e763..baf713c877 100644 --- a/internal/models/one_time_token_test.go +++ b/internal/models/one_time_token_test.go @@ -97,3 +97,61 @@ func (ts *OneTimeTokenTestSuite) TestCreateOneTimeTokenResendReplacesWindow() { require.True(ts.T(), second.ExpiresAt.After(*first.ExpiresAt), "resend must move expires_at forward, first=%s second=%s", first.ExpiresAt, second.ExpiresAt) } + +func (ts *OneTimeTokenTestSuite) TestFindOneTimeTokenWithPKCEFallback() { + ts.Run("exact hash match", func() { + TruncateAll(ts.db) + u := ts.createUser() + require.NoError(ts.T(), CreateOneTimeToken(ts.db, u.ID, u.GetEmail(), "hash", ConfirmationToken, time.Minute)) + + ott, err := FindOneTimeTokenWithPKCEFallback(ts.db, "hash", ConfirmationToken) + require.NoError(ts.T(), err) + require.Equal(ts.T(), "hash", ott.TokenHash) + require.Equal(ts.T(), u.ID, ott.UserID) + }) + + ts.Run("falls back to pkce_ prefixed hash", func() { + TruncateAll(ts.db) + u := ts.createUser() + require.NoError(ts.T(), CreateOneTimeToken(ts.db, u.ID, u.GetEmail(), "pkce_hash", ConfirmationToken, time.Minute)) + + ott, err := FindOneTimeTokenWithPKCEFallback(ts.db, "hash", ConfirmationToken) + require.NoError(ts.T(), err) + require.Equal(ts.T(), "pkce_hash", ott.TokenHash) + require.Equal(ts.T(), u.ID, ott.UserID) + }) + + ts.Run("prefers exact match over pkce_ prefixed hash", func() { + TruncateAll(ts.db) + u := ts.createUser() + + // (user_id, token_type) is unique, so the two candidates have to be + // different types. Both types are passed so both are eligible. + require.NoError(ts.T(), CreateOneTimeToken(ts.db, u.ID, u.GetEmail(), "hash", ConfirmationToken, time.Minute)) + require.NoError(ts.T(), CreateOneTimeToken(ts.db, u.ID, u.GetEmail(), "pkce_hash", RecoveryToken, time.Minute)) + + ott, err := FindOneTimeTokenWithPKCEFallback(ts.db, "hash", ConfirmationToken, RecoveryToken) + require.NoError(ts.T(), err) + require.Equal(ts.T(), "hash", ott.TokenHash) + require.Equal(ts.T(), ConfirmationToken, ott.TokenType) + }) + + ts.Run("not found when neither hash exists", func() { + TruncateAll(ts.db) + ts.createUser() + + ott, err := FindOneTimeTokenWithPKCEFallback(ts.db, "missing", ConfirmationToken) + require.True(ts.T(), IsNotFoundError(err), "expected not found error, got %v", err) + require.Nil(ts.T(), ott) + }) + + ts.Run("token type filter applies to the pkce_ fallback", func() { + TruncateAll(ts.db) + u := ts.createUser() + require.NoError(ts.T(), CreateOneTimeToken(ts.db, u.ID, u.GetEmail(), "pkce_hash", RecoveryToken, time.Minute)) + + ott, err := FindOneTimeTokenWithPKCEFallback(ts.db, "hash", ConfirmationToken) + require.True(ts.T(), IsNotFoundError(err), "expected not found error, got %v", err) + require.Nil(ts.T(), ott) + }) +}