From 52d641d6d589cba7cb1f3dad7af7bc7d1144cf64 Mon Sep 17 00:00:00 2001 From: joel Date: Mon, 28 Oct 2024 15:19:15 +0800 Subject: [PATCH 1/3] fix: check for phone identity --- internal/api/user.go | 2 +- internal/models/user.go | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) 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/models/user.go b/internal/models/user.go index 228e6e962..0b3df4589 100644 --- a/internal/models/user.go +++ b/internal/models/user.go @@ -759,6 +759,26 @@ func IsDuplicatedPhone(tx *storage.Connection, phone, aud string) (bool, error) return true, nil } +// TODO: Simplify this, check if it is safe to do this in admin create user too +// HasPhoneIdentity checks if the phone number already exists in the identities table +func HasPhoneIdentity(tx *storage.Connection, phone, aud string) (bool, error) { + user, err := FindUserByPhoneAndAudience(tx, phone, aud) + if err != nil { + if IsNotFoundError(err) { + return false, nil + } + return false, err + } + _, err = FindIdentityByIdAndProvider(tx, user.ID.String(), "phone") + if err != nil { + if IsNotFoundError(err) { + return false, nil + } + return false, err + } + return true, nil +} + // Ban a user for a given duration. func (u *User) Ban(tx *storage.Connection, duration time.Duration) error { if duration == time.Duration(0) { From 09af278ff885ed6438e32c0af01349c6dd0d3422 Mon Sep 17 00:00:00 2001 From: joel Date: Tue, 29 Oct 2024 14:00:40 +0800 Subject: [PATCH 2/3] fix: adjust check to use phone identity --- internal/api/user_test.go | 6 ++++++ internal/models/user.go | 22 ++++++++++------------ 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/internal/api/user_test.go b/internal/api/user_test.go index ed6c585af..310979d80 100644 --- a/internal/api/user_test.go +++ b/internal/api/user_test.go @@ -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 diff --git a/internal/models/user.go b/internal/models/user.go index 0b3df4589..5fb9a9171 100644 --- a/internal/models/user.go +++ b/internal/models/user.go @@ -762,21 +762,19 @@ func IsDuplicatedPhone(tx *storage.Connection, phone, aud string) (bool, error) // TODO: Simplify this, check if it is safe to do this in admin create user too // HasPhoneIdentity checks if the phone number already exists in the identities table func HasPhoneIdentity(tx *storage.Connection, phone, aud string) (bool, error) { - user, err := FindUserByPhoneAndAudience(tx, phone, aud) + query := `SELECT 1 FROM users + JOIN identities ON users.id = identities.user_id + WHERE users.phone = ? AND users.aud = ? AND identities.provider = ? + LIMIT 1` + + var exists bool + q := tx.RawQuery(query, phone, aud, "phone") + exists, err := q.Exists(&exists) if err != nil { - if IsNotFoundError(err) { - return false, nil - } return false, err } - _, err = FindIdentityByIdAndProvider(tx, user.ID.String(), "phone") - if err != nil { - if IsNotFoundError(err) { - return false, nil - } - return false, err - } - return true, nil + + return exists, nil } // Ban a user for a given duration. From 51523e88c15ab578640ab0ecf4756212d737aaf6 Mon Sep 17 00:00:00 2001 From: joel Date: Tue, 12 Nov 2024 22:34:41 +0800 Subject: [PATCH 3/3] fix: add tests --- internal/api/user_test.go | 50 ++++++++++++++++++++++++++++++++++++++- internal/models/user.go | 19 +++++++-------- 2 files changed, 58 insertions(+), 11 deletions(-) diff --git a/internal/api/user_test.go b/internal/api/user_test.go index 310979d80..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) @@ -269,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 5fb9a9171..9bf577af3 100644 --- a/internal/models/user.go +++ b/internal/models/user.go @@ -759,21 +759,20 @@ func IsDuplicatedPhone(tx *storage.Connection, phone, aud string) (bool, error) return true, nil } -// TODO: Simplify this, check if it is safe to do this in admin create user too // HasPhoneIdentity checks if the phone number already exists in the identities table func HasPhoneIdentity(tx *storage.Connection, phone, aud string) (bool, error) { - query := `SELECT 1 FROM users - JOIN identities ON users.id = identities.user_id - WHERE users.phone = ? AND users.aud = ? AND identities.provider = ? - LIMIT 1` - - var exists bool - q := tx.RawQuery(query, phone, aud, "phone") - exists, err := q.Exists(&exists) + 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 }