diff --git a/internal/api/signup.go b/internal/api/signup.go index 9b18ecf119..54c384e7e9 100644 --- a/internal/api/signup.go +++ b/internal/api/signup.go @@ -175,72 +175,32 @@ func (a *API) Signup(w http.ResponseWriter, r *http.Request) error { return apierrors.NewInternalServerError("Database error finding user").WithInternalError(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) + 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 } - } - - // 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 } - - // 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 - } - } + // 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) { @@ -397,7 +357,6 @@ func sanitizeUser(u *models.User, params *SignupParams) (*models.User, error) { u.Identities = make([]models.Identity, 0) u.UserMetaData = params.Data u.Aud = params.Aud - u.IsSuperAdmin = false // sanitize app_metadata u.AppMetaData = map[string]interface{}{ diff --git a/internal/api/signup_test.go b/internal/api/signup_test.go index 99b9b69bb1..e07c33dd1d 100644 --- a/internal/api/signup_test.go +++ b/internal/api/signup_test.go @@ -10,7 +10,6 @@ import ( "testing" "time" - "github.com/supabase/auth/internal/crypto" mail "github.com/supabase/auth/internal/mailer" "github.com/stretchr/testify/assert" @@ -170,101 +169,3 @@ 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 8d1ed74b60..661c206670 100644 --- a/internal/models/user.go +++ b/internal/models/user.go @@ -73,8 +73,6 @@ 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"` } @@ -188,7 +186,7 @@ func (u *User) BeforeSave(tx *pop.Connection) error { return nil } -// IsConfirmed checks if a user's email has already been +// IsConfirmed checks if a user has already been // registered and confirmed. func (u *User) IsConfirmed() bool { return u.EmailConfirmedAt != nil @@ -774,19 +772,6 @@ 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