diff --git a/internal/api/signup.go b/internal/api/signup.go index 54c384e7e9..2408b40fc5 100644 --- a/internal/api/signup.go +++ b/internal/api/signup.go @@ -175,32 +175,72 @@ func (a *API) Signup(w http.ResponseWriter, r *http.Request) error { return apierrors.NewInternalServerError("Database error finding user").WithInternalError(err) } - var signupUser *models.User - if user == nil { - // always call this outside of a database transaction as this method - // can be computationally hard and block due to password hashing - signupUser, err = params.ToUserModel(false /* <- isSSOUser */) - if err != nil { - return err - } - if err := a.triggerBeforeUserCreated(r, db, signupUser); err != nil { - return err + if user != nil { + if (params.Provider == EmailProvider && user.IsConfirmed()) || (params.Provider == PhoneProvider && user.IsPhoneConfirmed()) { + err = db.Transaction(func(tx *storage.Connection) error { + if terr := models.NewAuditLogEntry(config.AuditLog, r, tx, user, models.UserRepeatedSignUpAction, "", map[string]any{ + "provider": params.Provider, + }); terr != nil { + return terr + } + return nil + }) + if err != nil { + return err + } + if config.Mailer.Autoconfirm || config.Sms.Autoconfirm { + return apierrors.NewUnprocessableEntityError(apierrors.ErrorCodeUserAlreadyExists, "User already registered") + } + sanitizedUser, err := sanitizeUser(user, params) + if err != nil { + return err + } + return sendJSON(w, http.StatusOK, sanitizedUser) } } + // always call this outside of a database transaction as this method + // can be computationally hard and block due to password hashing + signupUser, err := params.ToUserModel(false /* <- isSSOUser */) + if err != nil { + return err + } + if err := a.triggerBeforeUserCreated(r, db, signupUser); err != nil { + return err + } + err = db.Transaction(func(tx *storage.Connection) error { var terr error + + if user != nil { + tuser, terr := models.FindUserByIDForUpdate(tx, user.ID) + if terr == nil { + user = tuser + } else if !models.IsNotFoundError(terr) { + return terr + } + } + if user != nil { if (params.Provider == EmailProvider && user.IsConfirmed()) || (params.Provider == PhoneProvider && user.IsPhoneConfirmed()) { return UserExistsError } - // do not update the user because we can't be sure of their claimed identity + + // Check user hasn't finished registration. + if !user.IsConfirmed() && !user.IsPhoneConfirmed() { + user.EncryptedPassword = signupUser.EncryptedPassword + terr = user.UpdatePassword(tx, nil) + if terr != nil { + return terr + } + } } 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) { @@ -357,6 +397,8 @@ func sanitizeUser(u *models.User, params *SignupParams) (*models.User, error) { u.Identities = make([]models.Identity, 0) u.UserMetaData = params.Data u.Aud = params.Aud + isSuperAdmin := false + u.IsSuperAdmin = &isSuperAdmin // sanitize app_metadata u.AppMetaData = map[string]interface{}{ diff --git a/internal/api/signup_test.go b/internal/api/signup_test.go index e07c33dd1d..99b9b69bb1 100644 --- a/internal/api/signup_test.go +++ b/internal/api/signup_test.go @@ -10,6 +10,7 @@ import ( "testing" "time" + "github.com/supabase/auth/internal/crypto" mail "github.com/supabase/auth/internal/mailer" "github.com/stretchr/testify/assert" @@ -169,3 +170,101 @@ 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"]) } + +// Check that each new signup attempt updates the unregistered user's password. +func (ts *SignupTestSuite) TestConsecutiveSignupAttempts() { + var attempt1Buffer bytes.Buffer + require.NoError(ts.T(), json.NewEncoder(&attempt1Buffer).Encode(map[string]interface{}{ + "email": "test@example.com", + "password": "password1", + })) + + // Setup request + req := httptest.NewRequest(http.MethodPost, "/signup", &attempt1Buffer) + req.Header.Set("Content-Type", "application/json") + + // Setup response recorder + w := httptest.NewRecorder() + ts.API.handler.ServeHTTP(w, req) + + require.Equal(ts.T(), http.StatusOK, w.Code) + + var attempt2Buffer bytes.Buffer + require.NoError(ts.T(), json.NewEncoder(&attempt2Buffer).Encode(map[string]interface{}{ + "email": "test@example.com", + "password": "password2", + })) + + // Setup request + req = httptest.NewRequest(http.MethodPost, "/signup", &attempt2Buffer) + req.Header.Set("Content-Type", "application/json") + + w = httptest.NewRecorder() + ts.API.handler.ServeHTTP(w, req) + require.Equal(ts.T(), http.StatusOK, w.Code) + + user, err := models.FindUserByEmailAndAudience(ts.API.db, "test@example.com", ts.Config.JWT.Aud) + require.NoError(ts.T(), err) + + err = crypto.CompareHashAndPassword(ts.T().Context(), *user.EncryptedPassword, "password1") + require.Error(ts.T(), err) + + err = crypto.CompareHashAndPassword(ts.T().Context(), *user.EncryptedPassword, "password2") + require.NoError(ts.T(), err) +} + +// Check an email signup attempt doesn't overwrite password of an email-confirmed user. +func (ts *SignupTestSuite) TestEmailSignupForEmailConfirmedUser() { + user, err := models.NewUser("", "test@example.com", "password1", ts.Config.JWT.Aud, nil) + require.NoError(ts.T(), err) + require.NoError(ts.T(), ts.API.db.Create(user)) + require.NoError(ts.T(), user.Confirm(ts.API.db)) + + var buffer bytes.Buffer + require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]interface{}{ + "email": "test@example.com", + "password": "password2", + })) + + req := httptest.NewRequest(http.MethodPost, "/signup", &buffer) + req.Header.Set("Content-Type", "application/json") + + w := httptest.NewRecorder() + ts.API.handler.ServeHTTP(w, req) + + require.Equal(ts.T(), http.StatusOK, w.Code) + + user, err = models.FindUserByEmailAndAudience(ts.API.db, "test@example.com", ts.Config.JWT.Aud) + require.NoError(ts.T(), err) + + err = crypto.CompareHashAndPassword(ts.T().Context(), *user.EncryptedPassword, "password1") + require.NoError(ts.T(), err) +} + +// Check an email signup attempt doesn't overwrite password of an phone-confirmed user. +func (ts *SignupTestSuite) TestEmailSignupForPhoneConfirmedUser() { + user, err := models.NewUser("123456789", "test@example.com", "password1", ts.Config.JWT.Aud, nil) + require.NoError(ts.T(), err) + require.NoError(ts.T(), ts.API.db.Create(user)) + require.NoError(ts.T(), user.ConfirmPhone(ts.API.db)) + + var buffer bytes.Buffer + require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]interface{}{ + "email": "test@example.com", + "password": "password2", + })) + + req := httptest.NewRequest(http.MethodPost, "/signup", &buffer) + req.Header.Set("Content-Type", "application/json") + + w := httptest.NewRecorder() + ts.API.handler.ServeHTTP(w, req) + + require.Equal(ts.T(), http.StatusOK, w.Code) + + user, err = models.FindUserByEmailAndAudience(ts.API.db, "test@example.com", ts.Config.JWT.Aud) + require.NoError(ts.T(), err) + + err = crypto.CompareHashAndPassword(ts.T().Context(), *user.EncryptedPassword, "password1") + require.NoError(ts.T(), err) +} diff --git a/internal/models/user.go b/internal/models/user.go index 661c206670..a8bd6a39d4 100644 --- a/internal/models/user.go +++ b/internal/models/user.go @@ -73,6 +73,8 @@ type User struct { DeletedAt *time.Time `json:"deleted_at,omitempty" db:"deleted_at"` IsAnonymous bool `json:"is_anonymous" db:"is_anonymous"` + IsSuperAdmin *bool `json:"-" db:"is_super_admin"` + DONTUSEINSTANCEID uuid.UUID `json:"-" db:"instance_id"` } @@ -186,7 +188,7 @@ func (u *User) BeforeSave(tx *pop.Connection) error { return nil } -// IsConfirmed checks if a user has already been +// IsConfirmed checks if a user's email has already been // registered and confirmed. func (u *User) IsConfirmed() bool { return u.EmailConfirmedAt != nil @@ -772,6 +774,19 @@ func FindUserByID(tx *storage.Connection, id uuid.UUID) (*User, error) { return findUser(tx, "instance_id = ? and id = ?", uuid.Nil, id) } +func FindUserByIDForUpdate(tx *storage.Connection, id uuid.UUID) (*User, error) { + obj := &User{} + query := fmt.Sprintf(`SELECT * FROM %q WHERE id = ? FOR UPDATE`, obj.TableName()) + if err := tx.RawQuery(query, id).First(obj); err != nil { + if errors.Cause(err) == sql.ErrNoRows { + return nil, UserNotFoundError{} + } + return nil, errors.Wrap(err, "error finding user") + } + + return obj, nil +} + // FindUserWithRefreshToken finds a user from the provided refresh token. If // forUpdate is set to true, then the SELECT statement used by the query has // the form SELECT ... FOR UPDATE SKIP LOCKED. This means that a FOR UPDATE