diff --git a/internal/api/user.go b/internal/api/user.go index 8588ce319..a4b587541 100644 --- a/internal/api/user.go +++ b/internal/api/user.go @@ -136,7 +136,7 @@ func (a *API) UserUpdate(w http.ResponseWriter, r *http.Request) error { } if params.Phone != "" && user.GetPhone() != params.Phone { - if exists, err := models.IsDuplicatedPhone(db, params.Phone, aud); err != nil { + if exists, err := models.HasPhoneIdentity(db, params.Phone, aud); err != nil { return internalServerError("Database error checking phone").WithInternalError(err) } else if exists { return unprocessableEntityError(ErrorCodePhoneExists, DuplicatePhoneMsg) diff --git a/internal/api/user_test.go b/internal/api/user_test.go index ed6c585af..ed74a9ee2 100644 --- a/internal/api/user_test.go +++ b/internal/api/user_test.go @@ -195,8 +195,8 @@ func (ts *UserTestSuite) TestUserUpdateEmail() { require.NoError(ts.T(), ts.API.db.Destroy(u)) }) } - } + func (ts *UserTestSuite) TestUserUpdatePhoneAutoconfirmEnabled() { u, err := models.FindUserByEmailAndAudience(ts.API.db, "test@example.com", ts.Config.JWT.Aud) require.NoError(ts.T(), err) @@ -205,6 +205,12 @@ func (ts *UserTestSuite) TestUserUpdatePhoneAutoconfirmEnabled() { require.NoError(ts.T(), err) require.NoError(ts.T(), ts.API.db.Create(existingUser)) + idPhone, err := models.NewIdentity(existingUser, "phone", map[string]interface{}{ + "sub": "+29382983298", + }) + require.NoError(ts.T(), err) + require.NoError(ts.T(), ts.API.db.Create(idPhone)) + cases := []struct { desc string userData map[string]string @@ -263,6 +269,54 @@ func (ts *UserTestSuite) TestUserUpdatePhoneAutoconfirmEnabled() { } +func (ts *UserTestSuite) TestUserAllowPhoneUpdateIfPhoneNotConfirmed() { + // Sign up first user with Phone A + existingUser, err := models.NewUser("333333333", "", "", ts.Config.JWT.Aud, nil) + require.NoError(ts.T(), err) + require.NoError(ts.T(), ts.API.db.Create(existingUser)) + + var buffer bytes.Buffer + // Sign up second user with phone B + phoneB := "1234567890" + + require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]interface{}{ + "phone": phoneB, + "password": "testpassword", + })) + + ts.Config.External.Phone.Enabled = true + ts.Config.Sms.Autoconfirm = true + 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) + + // Try to update Phone A user's phone number to Phone B + buffer.Reset() + require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]interface{}{ + "phone": phoneB, + })) + + data := AccessTokenResponse{} + + require.NoError(ts.T(), json.NewDecoder(w.Body).Decode(&data)) + + // Make update request + req = httptest.NewRequest(http.MethodPut, "/user", &buffer) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", data.Token)) + w = httptest.NewRecorder() + ts.API.handler.ServeHTTP(w, req) + + // Should succeed since the original phone number isn't confirmed + require.Equal(ts.T(), http.StatusOK, w.Code) + + updatedUser := &models.User{} + require.NoError(ts.T(), json.NewDecoder(w.Body).Decode(updatedUser)) + require.Equal(ts.T(), phoneB, updatedUser.GetPhone()) +} + func (ts *UserTestSuite) TestUserUpdatePassword() { u, err := models.FindUserByEmailAndAudience(ts.API.db, "test@example.com", ts.Config.JWT.Aud) require.NoError(ts.T(), err) diff --git a/internal/models/user.go b/internal/models/user.go index 228e6e962..9bf577af3 100644 --- a/internal/models/user.go +++ b/internal/models/user.go @@ -759,6 +759,23 @@ func IsDuplicatedPhone(tx *storage.Connection, phone, aud string) (bool, error) return true, nil } +// HasPhoneIdentity checks if the phone number already exists in the identities table +func HasPhoneIdentity(tx *storage.Connection, phone, aud string) (bool, error) { + usersTable := (&pop.Model{Value: User{}}).TableName() + + exists, err := tx.Q(). + Join(usersTable, fmt.Sprintf("%s.id = identities.user_id", usersTable)). + Where("users.phone = ? AND users.aud = ? AND identities.provider = ?", + phone, aud, "phone"). + Limit(1). + Exists(Identity{}) + + if err != nil { + return false, err + } + return exists, nil +} + // Ban a user for a given duration. func (u *User) Ban(tx *storage.Connection, duration time.Duration) error { if duration == time.Duration(0) {