fix: refine error message for sign up (#237)

* refine error message for sign up Signed-off-by: Bariq <[email protected]> * avoid information leak in signup Signed-off-by: Bariq <[email protected]> * resolve reviews Signed-off-by: Bariq <[email protected]> * make new error, sanitize user upon error user already exists Signed-off-by: Bariq <[email protected]> * sanitize confirmed_at, email_confirmed_at, phone_confirmed_at, and last_sign_in_at Signed-off-by: Bariq <[email protected]> * remove email change confirm status Signed-off-by: Bariq <[email protected]> * sanitize identities Signed-off-by: Bariq <[email protected]> * modularize function Signed-off-by: Bariq <[email protected]> * manually check response from gotrue-js Signed-off-by: Bariq <[email protected]> * fix test case and change aud Signed-off-by: Bariq <[email protected]> * refactor: santizeUser function Co-authored-by: Kang Ming <[email protected]>
supabase · Oct 27, 2021 · 5bc665b · 5bc665b
1 parent 8bfde09
commit 5bc665b
Show file tree

Hide file tree

Showing 3 changed files with 65 additions and 7 deletions.
diff --git a/api/errors.go b/api/errors.go
@@ -6,11 +6,15 @@ import (
 	"net/http"
 	"os"
 	"runtime/debug"
+
+	"github.com/netlify/gotrue/conf"
+	"github.com/pkg/errors"
 )
 
 // Common error messages during signup flow
 var (
-	DuplicateEmailMsg = "A user with this email address has already been registered"
+	DuplicateEmailMsg       = "A user with this email address has already been registered"
+	UserExistsError   error = errors.New("User already exists")
 )
 
 var oauthErrorMap = map[int]string{
@@ -56,6 +60,21 @@ func (e *OAuthError) Cause() error {
 	return e
 }
 
+func invalidSignupError(config *conf.Configuration) *HTTPError {
+	var msg string
+	if config.External.Email.Enabled && config.External.Phone.Enabled {
+		msg = "To signup, please provide your email or phone number"
+	} else if config.External.Email.Enabled {
+		msg = "To signup, please provide your email"
+	} else if config.External.Phone.Enabled {
+		msg = "To signup, please provide your phone number"
+	} else {
+		// 3rd party OAuth signups
+		msg = "To signup, please provide required fields"
+	}
+	return unprocessableEntityError(msg)
+}
+
 func oauthError(err string, description string) *OAuthError {
 	return &OAuthError{Err: err, Description: description}
 }

diff --git a/api/signup.go b/api/signup.go
@@ -78,7 +78,7 @@ func (a *API) Signup(w http.ResponseWriter, r *http.Request) error {
 		}
 		user, err = models.FindUserByPhoneAndAudience(a.db, instanceID, params.Phone, params.Aud)
 	default:
-		return unprocessableEntityError("Signup provider must be either email or phone")
+		return invalidSignupError(config)
 	}
 
 	if err != nil && !models.IsNotFoundError(err) {
@@ -89,11 +89,11 @@ func (a *API) Signup(w http.ResponseWriter, r *http.Request) error {
 		var terr error
 		if user != nil {
 			if params.Provider == "email" && user.IsConfirmed() {
-				return badRequestError("Thanks for registering, now check your email to complete the process.")
+				return UserExistsError
 			}
 
 			if params.Provider == "phone" && user.IsPhoneConfirmed() {
-				return badRequestError("A user with this phone number has already been registered")
+				return UserExistsError
 			}
 
 			if err := user.UpdateUserMetaData(tx, params.Data); err != nil {
@@ -160,6 +160,12 @@ func (a *API) Signup(w http.ResponseWriter, r *http.Request) error {
 		if errors.Is(err, MaxFrequencyLimitError) {
 			return tooManyRequestsError("For security purposes, you can only request this once every minute")
 		}
+		if errors.Is(err, UserExistsError) {
+			sanitizedUser := sanitizeUser(user, params)
+
+			// return sanitized user
+			return sendJSON(w, http.StatusOK, sanitizedUser)
+		}
 		return err
 	}
 
@@ -198,6 +204,34 @@ func (a *API) Signup(w http.ResponseWriter, r *http.Request) error {
 	return sendJSON(w, http.StatusOK, user)
 }
 
+func sanitizeUser(user *models.User, params *SignupParams) *models.User {
+	u := user
+	now := time.Now()
+
+	u.CreatedAt, u.UpdatedAt, u.ConfirmationSentAt, u.LastSignInAt, u.ConfirmedAt = now, now, &now, &now, &now
+	u.Identities = make([]models.Identity, 0)
+	u.UserMetaData = params.Data
+	u.Aud = params.Aud
+
+	// sanitize app_metadata
+	u.AppMetaData = map[string]interface{}{
+		"provider":  params.Provider,
+		"providers": []string{params.Provider},
+	}
+
+	// sanitize param fields
+	switch params.Provider {
+	case "email":
+		u.PhoneConfirmedAt, u.EmailConfirmedAt, u.Phone = nil, &now, ""
+	case "phone":
+		u.PhoneConfirmedAt, u.EmailConfirmedAt, u.Email = &now, nil, ""
+	default:
+		u.Phone, u.EmailConfirmedAt, u.PhoneConfirmedAt, u.Email = "", nil, nil, ""
+	}
+
+	return u
+}
+
 func (a *API) signupNewUser(ctx context.Context, conn *storage.Connection, params *SignupParams) (*models.User, error) {
 	instanceID := getInstanceID(ctx)
 	config := a.getConfig(ctx)

diff --git a/api/signup_test.go b/api/signup_test.go
@@ -219,11 +219,16 @@ func (ts *SignupTestSuite) TestSignupTwice() {
 	encode()
 	ts.API.handler.ServeHTTP(w, req)
 
-	data := make(map[string]interface{})
+	data := models.User{}
 	require.NoError(ts.T(), json.NewDecoder(w.Body).Decode(&data))
 
-	assert.Equal(ts.T(), http.StatusBadRequest, w.Code)
-	assert.Equal(ts.T(), float64(http.StatusBadRequest), data["code"])
+	require.Equal(ts.T(), http.StatusOK, w.Code)
+
+	assert.Equal(ts.T(), "[email protected]", data.GetEmail())
+	assert.Equal(ts.T(), ts.Config.JWT.Aud, data.Aud)
+	assert.Equal(ts.T(), 1.0, data.UserMetaData["a"])
+	assert.Equal(ts.T(), "email", data.AppMetaData["provider"])
+	assert.Equal(ts.T(), []interface{}{"email"}, data.AppMetaData["providers"])
 }
 
 func (ts *SignupTestSuite) TestVerifySignup() {