feat: refactor one-time tokens for performance (supabase#1558)

Refactors all One-Time Tokens (OTP) used for sign-in with email, SMS, email confirmation, phone confirmation, change... to achieve: - Performance (as current method does not use an index due to the use of [partial indexes](https://github.com/supabase/auth/blob/master/migrations/20220429102000_add_unique_idx.up.sql#L10-L14) which [cannot be used in practice](https://www.postgresql.org/docs/current/indexes-partial.html)) - Future enhancements (such as OTP verification counters, adaptive OTP lengths, etc.) Summary of the change: - A new `one_time_tokens` table is added which uses a double-write mechanism with `users`. - Each new OTP is both written in the corresponding `users` column and as a new row in `one_time_tokens`. - Lookup for an OTP hash is performed first in `one_time_tokens` and if not found, using the traditional `users` approach. - In a few days, once all OTPs using the `users` columns have expired, a new change will be deployed which removes the `users` lookup. This completely solves the performance issue for looking up OTPs. - In a future change, the `one_time_tokens` table can be used to add a verification counter based on lookups on the `relates_to` (email or phone number) column, enabling new security features. --------- Co-authored-by: Joel Lee <[email protected]>
uxodb · May 6, 2024 · d1cf8d9 · d1cf8d9
1 parent bfe4d98
commit d1cf8d9
Show file tree

Hide file tree

Showing 8 changed files with 837 additions and 285 deletions.
diff --git a/internal/api/mail.go b/internal/api/mail.go
@@ -1,12 +1,13 @@
 package api
 
 import (
-	"github.com/supabase/auth/internal/hooks"
-	mail "github.com/supabase/auth/internal/mailer"
 	"net/http"
 	"strings"
 	"time"
 
+	"github.com/supabase/auth/internal/hooks"
+	mail "github.com/supabase/auth/internal/mailer"
+
 	"github.com/badoux/checkmail"
 	"github.com/fatih/structs"
 	"github.com/pkg/errors"
@@ -123,6 +124,13 @@ func (a *API) adminGenerateLink(w http.ResponseWriter, r *http.Request) error {
 			terr = tx.UpdateOnly(user, "recovery_token", "recovery_sent_at")
 			if terr != nil {
 				terr = errors.Wrap(terr, "Database error updating user for recovery")
+				return terr
+			}
+
+			terr = models.CreateOneTimeToken(tx, user.ID, user.GetEmail(), user.RecoveryToken, models.RecoveryToken)
+			if terr != nil {
+				terr = errors.Wrap(terr, "Database error creating recovery token in admin")
+				return terr
 			}
 		case mail.InviteVerification:
 			if user != nil {
@@ -170,6 +178,12 @@ func (a *API) adminGenerateLink(w http.ResponseWriter, r *http.Request) error {
 			terr = tx.UpdateOnly(user, "confirmation_token", "confirmation_sent_at", "invited_at")
 			if terr != nil {
 				terr = errors.Wrap(terr, "Database error updating user for invite")
+				return terr
+			}
+			terr = models.CreateOneTimeToken(tx, user.ID, user.GetEmail(), user.ConfirmationToken, models.ConfirmationToken)
+			if terr != nil {
+				terr = errors.Wrap(terr, "Database error creating confirmation token for invite in admin")
+				return terr
 			}
 		case mail.SignupVerification:
 			if user != nil {
@@ -202,6 +216,12 @@ func (a *API) adminGenerateLink(w http.ResponseWriter, r *http.Request) error {
 			terr = tx.UpdateOnly(user, "confirmation_token", "confirmation_sent_at")
 			if terr != nil {
 				terr = errors.Wrap(terr, "Database error updating user for confirmation")
+				return terr
+			}
+			terr = models.CreateOneTimeToken(tx, user.ID, user.GetEmail(), user.ConfirmationToken, models.ConfirmationToken)
+			if terr != nil {
+				terr = errors.Wrap(terr, "Database error creating confirmation token for signup in admin")
+				return terr
 			}
 		case mail.EmailChangeCurrentVerification, mail.EmailChangeNewVerification:
 			if !config.Mailer.SecureEmailChangeEnabled && params.Type == "email_change_current" {
@@ -228,6 +248,21 @@ func (a *API) adminGenerateLink(w http.ResponseWriter, r *http.Request) error {
 			terr = tx.UpdateOnly(user, "email_change_token_current", "email_change_token_new", "email_change", "email_change_sent_at", "email_change_confirm_status")
 			if terr != nil {
 				terr = errors.Wrap(terr, "Database error updating user for email change")
+				return terr
+			}
+			if user.EmailChangeTokenCurrent != "" {
+				terr = models.CreateOneTimeToken(tx, user.ID, user.GetEmail(), user.EmailChangeTokenCurrent, models.EmailChangeTokenCurrent)
+				if terr != nil {
+					terr = errors.Wrap(terr, "Database error creating email change token current in admin")
+					return terr
+				}
+			}
+			if user.EmailChangeTokenNew != "" {
+				terr = models.CreateOneTimeToken(tx, user.ID, user.EmailChange, user.EmailChangeTokenNew, models.EmailChangeTokenNew)
+				if terr != nil {
+					terr = errors.Wrap(terr, "Database error creating email change token new in admin")
+					return terr
+				}
 			}
 		default:
 			return badRequestError(ErrorCodeValidationFailed, "Invalid email action link type requested: %v", params.Type)
@@ -290,6 +325,11 @@ func (a *API) sendConfirmation(r *http.Request, tx *storage.Connection, u *model
 		return errors.Wrap(err, "Database error updating user for confirmation")
 	}
 
+	err = models.CreateOneTimeToken(tx, u.ID, u.GetEmail(), u.ConfirmationToken, models.ConfirmationToken)
+	if err != nil {
+		return errors.Wrap(err, "Database error creating confirmation token")
+	}
+
 	return nil
 }
 
@@ -317,6 +357,11 @@ func (a *API) sendInvite(r *http.Request, tx *storage.Connection, u *models.User
 		return errors.Wrap(err, "Database error updating user for invite")
 	}
 
+	err = models.CreateOneTimeToken(tx, u.ID, u.GetEmail(), u.ConfirmationToken, models.ConfirmationToken)
+	if err != nil {
+		return errors.Wrap(err, "Database error creating confirmation token for invite")
+	}
+
 	return nil
 }
 
@@ -349,6 +394,11 @@ func (a *API) sendPasswordRecovery(r *http.Request, tx *storage.Connection, u *m
 		return errors.Wrap(err, "Database error updating user for recovery")
 	}
 
+	err = models.CreateOneTimeToken(tx, u.ID, u.GetEmail(), u.RecoveryToken, models.RecoveryToken)
+	if err != nil {
+		return errors.Wrap(err, "Database error creating recovery token")
+	}
+
 	return nil
 }
 
@@ -381,6 +431,11 @@ func (a *API) sendReauthenticationOtp(r *http.Request, tx *storage.Connection, u
 		return errors.Wrap(err, "Database error updating user for reauthentication")
 	}
 
+	err = models.CreateOneTimeToken(tx, u.ID, u.GetEmail(), u.ReauthenticationToken, models.ReauthenticationToken)
+	if err != nil {
+		return errors.Wrap(err, "Database error creating reauthentication token")
+	}
+
 	return nil
 }
 
@@ -416,6 +471,11 @@ func (a *API) sendMagicLink(r *http.Request, tx *storage.Connection, u *models.U
 		return errors.Wrap(err, "Database error updating user for recovery")
 	}
 
+	err = models.CreateOneTimeToken(tx, u.ID, u.GetEmail(), u.RecoveryToken, models.RecoveryToken)
+	if err != nil {
+		return errors.Wrap(err, "Database error creating recovery token")
+	}
+
 	return nil
 }
 
@@ -469,6 +529,20 @@ func (a *API) sendEmailChange(r *http.Request, tx *storage.Connection, u *models
 		return errors.Wrap(err, "Database error updating user for email change")
 	}
 
+	if u.EmailChangeTokenCurrent != "" {
+		err = models.CreateOneTimeToken(tx, u.ID, u.GetEmail(), u.EmailChangeTokenCurrent, models.EmailChangeTokenCurrent)
+		if err != nil {
+			return errors.Wrap(err, "Database error creating email change token current")
+		}
+	}
+
+	if u.EmailChangeTokenNew != "" {
+		err = models.CreateOneTimeToken(tx, u.ID, u.EmailChange, u.EmailChangeTokenNew, models.EmailChangeTokenNew)
+		if err != nil {
+			return errors.Wrap(err, "Database error creating email change token new")
+		}
+	}
+
 	return nil
 }
 

diff --git a/internal/api/phone.go b/internal/api/phone.go
@@ -95,6 +95,7 @@ func (a *API) sendPhoneConfirmation(ctx context.Context, r *http.Request, tx *st
 		if err != nil {
 			return "", err
 		}
+
 		// Hook should only be called if SMS autoconfirm is disabled
 		if !config.Sms.Autoconfirm && config.Hook.SendSMS.Enabled {
 			input := hooks.SendSMSInput{
@@ -109,7 +110,6 @@ func (a *API) sendPhoneConfirmation(ctx context.Context, r *http.Request, tx *st
 				return "", err
 			}
 		} else {
-
 			messageID, err = smsProvider.SendMessage(phone, message, channel, otp)
 			if err != nil {
 				return messageID, err
@@ -128,7 +128,26 @@ func (a *API) sendPhoneConfirmation(ctx context.Context, r *http.Request, tx *st
 		user.ReauthenticationSentAt = &now
 	}
 
-	return messageID, errors.Wrap(tx.UpdateOnly(user, includeFields...), "Database error updating user for confirmation")
+	if err := tx.UpdateOnly(user, includeFields...); err != nil {
+		return messageID, errors.Wrap(err, "Database error updating user for phone")
+	}
+
+	switch otpType {
+	case phoneConfirmationOtp:
+		if err := models.CreateOneTimeToken(tx, user.ID, user.GetPhone(), user.ConfirmationToken, models.ConfirmationToken); err != nil {
+			return messageID, errors.Wrap(err, "Database error creating confirmation token for phone")
+		}
+	case phoneChangeVerification:
+		if err := models.CreateOneTimeToken(tx, user.ID, user.PhoneChange, user.PhoneChangeToken, models.PhoneChangeToken); err != nil {
+			return messageID, errors.Wrap(err, "Database error creating phone change token")
+		}
+	case phoneReauthenticationOtp:
+		if err := models.CreateOneTimeToken(tx, user.ID, user.GetPhone(), user.ReauthenticationToken, models.ReauthenticationToken); err != nil {
+			return messageID, errors.Wrap(err, "Database error creating reauthentication token for phone")
+		}
+	}
+
+	return messageID, nil
 }
 
 func generateSMSFromTemplate(SMSTemplate *template.Template, otp string) (string, error) {

diff --git a/internal/api/verify.go b/internal/api/verify.go
@@ -479,11 +479,28 @@ func (a *API) emailChangeVerify(r *http.Request, conn *storage.Connection, param
 	config := a.config
 	if config.Mailer.SecureEmailChangeEnabled && user.EmailChangeConfirmStatus == zeroConfirmation && user.GetEmail() != "" {
 		err := conn.Transaction(func(tx *storage.Connection) error {
+			currentOTT, terr := models.FindOneTimeToken(tx, params.TokenHash, models.EmailChangeTokenCurrent)
+			if terr != nil && !models.IsNotFoundError(terr) {
+				return terr
+			}
+
+			newOTT, terr := models.FindOneTimeToken(tx, params.TokenHash, models.EmailChangeTokenNew)
+			if terr != nil && !models.IsNotFoundError(terr) {
+				return terr
+			}
+
 			user.EmailChangeConfirmStatus = singleConfirmation
-			if params.Token == user.EmailChangeTokenCurrent || params.TokenHash == user.EmailChangeTokenCurrent {
+
+			if params.Token == user.EmailChangeTokenCurrent || params.TokenHash == user.EmailChangeTokenCurrent || (currentOTT != nil && params.TokenHash == currentOTT.TokenHash) {
 				user.EmailChangeTokenCurrent = ""
-			} else if params.Token == user.EmailChangeTokenNew || params.TokenHash == user.EmailChangeTokenNew {
+				if terr := models.ClearOneTimeTokenForUser(tx, user.ID, models.EmailChangeTokenCurrent); terr != nil {
+					return terr
+				}
+			} else if params.Token == user.EmailChangeTokenNew || params.TokenHash == user.EmailChangeTokenNew || (newOTT != nil && params.TokenHash == newOTT.TokenHash) {
 				user.EmailChangeTokenNew = ""
+				if terr := models.ClearOneTimeTokenForUser(tx, user.ID, models.EmailChangeTokenNew); terr != nil {
+					return terr
+				}
 			}
 			if terr := tx.UpdateOnly(user, "email_change_confirm_status", "email_change_token_current", "email_change_token_new"); terr != nil {
 				return terr