feat(otp): switch one_time_tokens table to source of truth

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"

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