From e5f98cb9e24ecebb0b7dc88c495fd456cc73fcba Mon Sep 17 00:00:00 2001 From: Kang Ming Date: Mon, 13 May 2024 23:32:53 +0800 Subject: [PATCH] fix: sms verify should update is_anonymous field (#1580) ## What kind of change does this PR introduce? * verifying the phone number of a user should update the `is_anonymous` field to false * add test to prevent any future regression --------- Co-authored-by: Joel Lee --- internal/api/anonymous_test.go | 165 +++++++++++++++++++++------------ internal/api/verify.go | 7 ++ 2 files changed, 113 insertions(+), 59 deletions(-) diff --git a/internal/api/anonymous_test.go b/internal/api/anonymous_test.go index 1260774e3..fdee4cc07 100644 --- a/internal/api/anonymous_test.go +++ b/internal/api/anonymous_test.go @@ -12,6 +12,7 @@ import ( "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" "github.com/supabase/auth/internal/conf" + mail "github.com/supabase/auth/internal/mailer" "github.com/supabase/auth/internal/models" ) @@ -77,66 +78,112 @@ func (ts *AnonymousTestSuite) TestAnonymousLogins() { func (ts *AnonymousTestSuite) TestConvertAnonymousUserToPermanent() { ts.Config.External.AnonymousUsers.Enabled = true - // Request body - var buffer bytes.Buffer - require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]interface{}{})) - - 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) - - signupResponse := &AccessTokenResponse{} - require.NoError(ts.T(), json.NewDecoder(w.Body).Decode(&signupResponse)) - - // Add email to anonymous user - require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]interface{}{ - "email": "test@example.com", - })) - - req = httptest.NewRequest(http.MethodPut, "/user", &buffer) - req.Header.Set("Content-Type", "application/json") - req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", signupResponse.Token)) - - w = httptest.NewRecorder() - ts.API.handler.ServeHTTP(w, req) - require.Equal(ts.T(), http.StatusOK, w.Code) - - // Check if anonymous user is still anonymous - user, err := models.FindUserByID(ts.API.db, signupResponse.User.ID) - require.NoError(ts.T(), err) - require.NotEmpty(ts.T(), user) - require.True(ts.T(), user.IsAnonymous) - - // Verify email change - require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]interface{}{ - "token_hash": user.EmailChangeTokenNew, - "type": "email_change", - })) - - req = httptest.NewRequest(http.MethodPost, "/verify", &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) - - data := &AccessTokenResponse{} - require.NoError(ts.T(), json.NewDecoder(w.Body).Decode(&data)) - - // User is a permanent user and not anonymous anymore - assert.Equal(ts.T(), signupResponse.User.ID, data.User.ID) - assert.Equal(ts.T(), ts.Config.JWT.Aud, data.User.Aud) - assert.Equal(ts.T(), "test@example.com", data.User.GetEmail()) - assert.Equal(ts.T(), models.JSONMap(models.JSONMap{"provider": "email", "providers": []interface{}{"email"}}), data.User.AppMetaData) - assert.False(ts.T(), data.User.IsAnonymous) - assert.NotEmpty(ts.T(), data.User.EmailConfirmedAt) + ts.Config.Sms.TestOTP = map[string]string{"1234567890": "000000"} + // test OTPs still require setting up an sms provider + ts.Config.Sms.Provider = "twilio" + ts.Config.Sms.Twilio.AccountSid = "fake-sid" + ts.Config.Sms.Twilio.AuthToken = "fake-token" + ts.Config.Sms.Twilio.MessageServiceSid = "fake-message-service-sid" + + cases := []struct { + desc string + body map[string]interface{} + verificationType string + }{ + { + desc: "convert anonymous user to permanent user with email", + body: map[string]interface{}{ + "email": "test@example.com", + }, + verificationType: "email_change", + }, + { + desc: "convert anonymous user to permanent user with phone", + body: map[string]interface{}{ + "phone": "1234567890", + }, + verificationType: "phone_change", + }, + } - // User should have an email identity - assert.Len(ts.T(), data.User.Identities, 1) + for _, c := range cases { + ts.Run(c.desc, func() { + // Request body + var buffer bytes.Buffer + require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]interface{}{})) + + 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) + + signupResponse := &AccessTokenResponse{} + require.NoError(ts.T(), json.NewDecoder(w.Body).Decode(&signupResponse)) + + // Add email to anonymous user + require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(c.body)) + + req = httptest.NewRequest(http.MethodPut, "/user", &buffer) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", signupResponse.Token)) + + w = httptest.NewRecorder() + ts.API.handler.ServeHTTP(w, req) + require.Equal(ts.T(), http.StatusOK, w.Code) + + // Check if anonymous user is still anonymous + user, err := models.FindUserByID(ts.API.db, signupResponse.User.ID) + require.NoError(ts.T(), err) + require.NotEmpty(ts.T(), user) + require.True(ts.T(), user.IsAnonymous) + + switch c.verificationType { + case mail.EmailChangeVerification: + require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]interface{}{ + "token_hash": user.EmailChangeTokenNew, + "type": c.verificationType, + })) + case phoneChangeVerification: + require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]interface{}{ + "phone": "1234567890", + "token": "000000", + "type": c.verificationType, + })) + } + + req = httptest.NewRequest(http.MethodPost, "/verify", &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) + + data := &AccessTokenResponse{} + require.NoError(ts.T(), json.NewDecoder(w.Body).Decode(&data)) + + // User is a permanent user and not anonymous anymore + assert.Equal(ts.T(), signupResponse.User.ID, data.User.ID) + assert.Equal(ts.T(), ts.Config.JWT.Aud, data.User.Aud) + assert.False(ts.T(), data.User.IsAnonymous) + + // User should have an identity + assert.Len(ts.T(), data.User.Identities, 1) + + switch c.verificationType { + case mail.EmailChangeVerification: + assert.Equal(ts.T(), "test@example.com", data.User.GetEmail()) + assert.Equal(ts.T(), models.JSONMap(models.JSONMap{"provider": "email", "providers": []interface{}{"email"}}), data.User.AppMetaData) + assert.NotEmpty(ts.T(), data.User.EmailConfirmedAt) + case phoneChangeVerification: + assert.Equal(ts.T(), "1234567890", data.User.GetPhone()) + assert.Equal(ts.T(), models.JSONMap(models.JSONMap{"provider": "phone", "providers": []interface{}{"phone"}}), data.User.AppMetaData) + assert.NotEmpty(ts.T(), data.User.PhoneConfirmedAt) + } + }) + } } func (ts *AnonymousTestSuite) TestRateLimitAnonymousSignups() { diff --git a/internal/api/verify.go b/internal/api/verify.go index f48494b0d..121cb4e2c 100644 --- a/internal/api/verify.go +++ b/internal/api/verify.go @@ -406,6 +406,13 @@ func (a *API) smsVerify(r *http.Request, conn *storage.Connection, user *models. } } + if user.IsAnonymous { + user.IsAnonymous = false + if terr := tx.UpdateOnly(user, "is_anonymous"); terr != nil { + return terr + } + } + if terr := tx.Load(user, "Identities"); terr != nil { return internalServerError("Error refetching identities").WithInternalError(terr) }