diff --git a/internal/api/errors.go b/internal/api/errors.go index 2f8156098..67ba5bf6a 100644 --- a/internal/api/errors.go +++ b/internal/api/errors.go @@ -2,15 +2,19 @@ package api import ( "context" + "errors" "fmt" "net/http" "os" "runtime/debug" "time" - "github.com/pkg/errors" + "github.com/jackc/pgconn" + "github.com/jackc/pgerrcode" + pkgerr "github.com/pkg/errors" "github.com/sirupsen/logrus" "github.com/supabase/auth/internal/api/apierrors" + "github.com/supabase/auth/internal/models" "github.com/supabase/auth/internal/observability" "github.com/supabase/auth/internal/utilities" ) @@ -22,9 +26,29 @@ const ( ) var ( - UserExistsError error = errors.New("user already exists") + UserExistsError error = pkgerr.New("user already exists") ) +// isUniqueConstraintError reports whether err is (or wraps) a Postgres unique +// violation, including HTTPError wrappers from signupNewUser. +func isUniqueConstraintError(err error) bool { + if err == nil { + return false + } + if models.IsUniqueConstraintViolatedError(err) { + return true + } + var httpErr *apierrors.HTTPError + if errors.As(err, &httpErr) && httpErr.InternalError != nil { + err = httpErr.InternalError + } + var pgErr *pgconn.PgError + if errors.As(err, &pgErr) { + return pgErr.Code == pgerrcode.UniqueViolation + } + return false +} + const InvalidChannelError = "Invalid channel, supported values are 'sms' or 'whatsapp'. 'whatsapp' is only supported if Twilio or Twilio Verify is used as the provider." var oauthErrorMap = map[int]string{ diff --git a/internal/api/otp_test.go b/internal/api/otp_test.go index 7a99f3d9c..40d1efcd2 100644 --- a/internal/api/otp_test.go +++ b/internal/api/otp_test.go @@ -310,3 +310,37 @@ func (ts *OtpTestSuite) TestSubsequentOtp() { require.Empty(ts.T(), user.RecoverySentAt) require.Empty(ts.T(), user.EmailConfirmedAt) } + +// TestConcurrentOtpSignupSameEmail ensures a race on users_email_partial_key +// does not surface as a raw 500 (issue #2675). +func (ts *OtpTestSuite) TestConcurrentOtpSignupSameEmail() { + ts.Config.SMTP.MaxFrequency = 0 + email := "concurrent-otp-race@example.com" + + const n = 8 + codes := make(chan int, n) + for i := 0; i < n; i++ { + go func() { + var buffer bytes.Buffer + require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]interface{}{ + "email": email, + "create_user": true, + })) + req := httptest.NewRequest(http.MethodPost, "/otp", &buffer) + req.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + ts.API.handler.ServeHTTP(w, req) + codes <- w.Code + }() + } + + for i := 0; i < n; i++ { + code := <-codes + require.NotEqual(ts.T(), http.StatusInternalServerError, code, "losing concurrent OTP must not return 500") + require.Equal(ts.T(), http.StatusOK, code) + } + + user, err := models.FindUserByEmailAndAudience(ts.API.db, email, ts.Config.JWT.Aud) + require.NoError(ts.T(), err) + require.NotNil(ts.T(), user) +} diff --git a/internal/api/signup.go b/internal/api/signup.go index 0af5a7c48..633fa7a85 100644 --- a/internal/api/signup.go +++ b/internal/api/signup.go @@ -188,94 +188,114 @@ func (a *API) Signup(w http.ResponseWriter, r *http.Request) error { } } - err = db.Transaction(func(tx *storage.Connection) error { - var terr error - if user != nil { - if (params.Provider == "email" && user.IsConfirmed()) || (params.Provider == "phone" && user.IsPhoneConfirmed()) { - return UserExistsError - } - // do not update the user because we can't be sure of their claimed identity - } else { - user, terr = a.signupNewUser(tx, signupUser) - if terr != nil { - return terr - } - } - identity, terr := models.FindIdentityByIdAndProvider(tx, user.ID.String(), params.Provider) - if terr != nil { - if !models.IsNotFoundError(terr) { - return terr - } - identityData := structs.Map(provider.Claims{ - Subject: user.ID.String(), - Email: user.GetEmail(), - }) - for k, v := range params.Data { - if _, ok := identityData[k]; !ok { - identityData[k] = v + for attempt := 0; attempt < 2; attempt++ { + err = db.Transaction(func(tx *storage.Connection) error { + var terr error + if user != nil { + if (params.Provider == "email" && user.IsConfirmed()) || (params.Provider == "phone" && user.IsPhoneConfirmed()) { + return UserExistsError } - } - identity, terr = a.createNewIdentity(tx, user, params.Provider, identityData) - if terr != nil { - return terr - } - if terr := user.RemoveUnconfirmedIdentities(tx, identity); terr != nil { - return terr - } - } - user.Identities = []models.Identity{*identity} - - if params.Provider == "email" && !user.IsConfirmed() { - if config.Mailer.Autoconfirm { - if terr = models.NewAuditLogEntry(config.AuditLog, r, tx, user, models.UserSignedUpAction, "", map[string]interface{}{ - "provider": params.Provider, - }); terr != nil { + // do not update the user because we can't be sure of their claimed identity + } else { + user, terr = a.signupNewUser(tx, signupUser) + if terr != nil { return terr } - if terr = user.Confirm(tx); terr != nil { - return apierrors.NewInternalServerError("Database error updating user").WithInternalError(terr) - } - } else { - if terr = models.NewAuditLogEntry(config.AuditLog, r, tx, user, models.UserConfirmationRequestedAction, "", map[string]interface{}{ - "provider": params.Provider, - }); terr != nil { + } + identity, terr := models.FindIdentityByIdAndProvider(tx, user.ID.String(), params.Provider) + if terr != nil { + if !models.IsNotFoundError(terr) { return terr } - if isPKCEFlow(flowType) { - _, terr := generateFlowState(tx, params.Provider, models.EmailSignup, params.CodeChallengeMethod, params.CodeChallenge, &user.ID) - if terr != nil { - return terr + identityData := structs.Map(provider.Claims{ + Subject: user.ID.String(), + Email: user.GetEmail(), + }) + for k, v := range params.Data { + if _, ok := identityData[k]; !ok { + identityData[k] = v } } - if terr = a.sendConfirmation(r, tx, user, flowType); terr != nil { + identity, terr = a.createNewIdentity(tx, user, params.Provider, identityData) + if terr != nil { return terr } - } - } else if params.Provider == "phone" && !user.IsPhoneConfirmed() { - if config.Sms.Autoconfirm { - if terr = models.NewAuditLogEntry(config.AuditLog, r, tx, user, models.UserSignedUpAction, "", map[string]interface{}{ - "provider": params.Provider, - "channel": params.Channel, - }); terr != nil { + if terr := user.RemoveUnconfirmedIdentities(tx, identity); terr != nil { return terr } - if terr = user.ConfirmPhone(tx); terr != nil { - return apierrors.NewInternalServerError("Database error updating user").WithInternalError(terr) - } - } else { - if terr = models.NewAuditLogEntry(config.AuditLog, r, tx, user, models.UserConfirmationRequestedAction, "", map[string]interface{}{ - "provider": params.Provider, - }); terr != nil { - return terr + } + user.Identities = []models.Identity{*identity} + + if params.Provider == "email" && !user.IsConfirmed() { + if config.Mailer.Autoconfirm { + if terr = models.NewAuditLogEntry(config.AuditLog, r, tx, user, models.UserSignedUpAction, "", map[string]interface{}{ + "provider": params.Provider, + }); terr != nil { + return terr + } + if terr = user.Confirm(tx); terr != nil { + return apierrors.NewInternalServerError("Database error updating user").WithInternalError(terr) + } + } else { + if terr = models.NewAuditLogEntry(config.AuditLog, r, tx, user, models.UserConfirmationRequestedAction, "", map[string]interface{}{ + "provider": params.Provider, + }); terr != nil { + return terr + } + if isPKCEFlow(flowType) { + _, terr := generateFlowState(tx, params.Provider, models.EmailSignup, params.CodeChallengeMethod, params.CodeChallenge, &user.ID) + if terr != nil { + return terr + } + } + if terr = a.sendConfirmation(r, tx, user, flowType); terr != nil { + return terr + } } - if _, terr := a.sendPhoneConfirmation(r, tx, user, params.Phone, phoneConfirmationOtp, params.Channel); terr != nil { - return terr + } else if params.Provider == "phone" && !user.IsPhoneConfirmed() { + if config.Sms.Autoconfirm { + if terr = models.NewAuditLogEntry(config.AuditLog, r, tx, user, models.UserSignedUpAction, "", map[string]interface{}{ + "provider": params.Provider, + "channel": params.Channel, + }); terr != nil { + return terr + } + if terr = user.ConfirmPhone(tx); terr != nil { + return apierrors.NewInternalServerError("Database error updating user").WithInternalError(terr) + } + } else { + if terr = models.NewAuditLogEntry(config.AuditLog, r, tx, user, models.UserConfirmationRequestedAction, "", map[string]interface{}{ + "provider": params.Provider, + }); terr != nil { + return terr + } + if _, terr := a.sendPhoneConfirmation(r, tx, user, params.Phone, phoneConfirmationOtp, params.Channel); terr != nil { + return terr + } } } - } - return nil - }) + return nil + }) + + if err == nil || errors.Is(err, UserExistsError) || !isUniqueConstraintError(err) { + break + } + // Concurrent signup for the same email/phone won the insert. Reload the + // user and retry once on the existing-user path (same as a later client retry). + var findErr error + switch params.Provider { + case "email": + user, findErr = models.FindUserByEmailAndAudience(db, params.Email, params.Aud) + case "phone": + user, findErr = models.FindUserByPhoneAndAudience(db, params.Phone, params.Aud) + default: + return err + } + if findErr != nil { + return apierrors.NewInternalServerError("Database error finding user").WithInternalError(findErr) + } + } if err != nil { if errors.Is(err, UserExistsError) { @@ -383,6 +403,9 @@ func (a *API) signupNewUser(conn *storage.Connection, user *models.User) (*model err := conn.Transaction(func(tx *storage.Connection) error { var terr error if terr = tx.Create(user); terr != nil { + if isUniqueConstraintError(terr) { + return models.UserEmailUniqueConflictError{} + } return apierrors.NewInternalServerError("Database error saving new user").WithInternalError(terr) } if terr = user.SetRole(tx, config.JWT.DefaultGroupName); terr != nil { diff --git a/internal/api/signup_test.go b/internal/api/signup_test.go index 606de1fe1..1166cca1a 100644 --- a/internal/api/signup_test.go +++ b/internal/api/signup_test.go @@ -169,3 +169,16 @@ func (ts *SignupTestSuite) TestSignupRequestBodyTooLarge() { require.NoError(ts.T(), json.NewDecoder(w.Body).Decode(&data)) require.Equal(ts.T(), "request_entity_too_large", data["error_code"]) } + +func (ts *SignupTestSuite) TestSignupNewUserUniqueEmailConflict() { + u1, err := models.NewUser("", "dup@example.com", "password123", ts.Config.JWT.Aud, nil) + require.NoError(ts.T(), err) + _, err = ts.API.signupNewUser(ts.API.db, u1) + require.NoError(ts.T(), err) + + u2, err := models.NewUser("", "dup@example.com", "password123", ts.Config.JWT.Aud, nil) + require.NoError(ts.T(), err) + _, err = ts.API.signupNewUser(ts.API.db, u2) + require.True(ts.T(), models.IsUniqueConstraintViolatedError(err), "got: %v", err) + require.True(ts.T(), isUniqueConstraintError(err)) +}