Repository: supabase/gotrue
Pull request #1374 (2024-01-12 06:57:05)

feat: make error message in factor creation more obvious
## What kind of change does this PR introduce?

This PR proposes two main changes:

1. Removes the return of `error` in `NewFactor` and `NewChallenge` . As constructed right now,  these two constructs are guaranteed to succeed and won't return an error. They are unlikely to change and removing the `err` field will cut back on complexity
2. Checks for occasions where an attempt is made to insert a factor with a friendly name which is already registered in the DB. In such instances, we translate the error into a message which highlights that there is already another factor with the same name registered in the system.





Patch:
diff --git a/internal/api/mfa.go b/internal/api/mfa.go
index 66505d0d..c4f60016 100644
--- a/internal/api/mfa.go
+++ b/internal/api/mfa.go
@@ -3,6 +3,7 @@ package api
 import (
 	"bytes"
 	"encoding/json"
+	"fmt"
 	"net/http"
 	"net/url"
 
@@ -130,13 +131,16 @@ func (a *API) EnrollFactor(w http.ResponseWriter, r *http.Request) error {
 	}
 	svgData.End()
 
-	factor, err := models.NewFactor(user, params.FriendlyName, params.FactorType, models.FactorStateUnverified, key.Secret())
-	if err != nil {
-		return internalServerError("database error creating factor").WithInternalError(err)
-	}
+	factor := models.NewFactor(user, params.FriendlyName, params.FactorType, models.FactorStateUnverified, key.Secret())
+
 	err = a.db.Transaction(func(tx *storage.Connection) error {
 		if terr := tx.Create(factor); terr != nil {
+			pgErr := utilities.NewPostgresError(terr)
+			if pgErr.IsUniqueConstraintViolated() {
+				return internalServerError(fmt.Sprintf("a factor with the friendly name %q for this user likely already exists", factor.FriendlyName))
+			}
 			return terr
+
 		}
 		if terr := models.NewAuditLogEntry(r, tx, user, models.EnrollFactorAction, r.RemoteAddr, map[string]interface{}{
 			"factor_id": factor.ID,
@@ -169,12 +173,9 @@ func (a *API) ChallengeFactor(w http.ResponseWriter, r *http.Request) error {
 	user := getUser(ctx)
 	factor := getFactor(ctx)
 	ipAddress := utilities.GetIPAddress(r)
-	challenge, err := models.NewChallenge(factor, ipAddress)
-	if err != nil {
-		return internalServerError("Database error creating challenge").WithInternalError(err)
-	}
+	challenge := models.NewChallenge(factor, ipAddress)
 
-	err = a.db.Transaction(func(tx *storage.Connection) error {
+	err := a.db.Transaction(func(tx *storage.Connection) error {
 		if terr := tx.Create(challenge); terr != nil {
 			return terr
 		}
diff --git a/internal/models/challenge.go b/internal/models/challenge.go
index c69a1cb5..99758e63 100644
--- a/internal/models/challenge.go
+++ b/internal/models/challenge.go
@@ -22,7 +22,7 @@ func (Challenge) TableName() string {
 	return tableName
 }
 
-func NewChallenge(factor *Factor, ipAddress string) (*Challenge, error) {
+func NewChallenge(factor *Factor, ipAddress string) *Challenge {
 	id := uuid.Must(uuid.NewV4())
 
 	challenge := &Challenge{
@@ -30,7 +30,7 @@ func NewChallenge(factor *Factor, ipAddress string) (*Challenge, error) {
 		FactorID:  factor.ID,
 		IPAddress: ipAddress,
 	}
-	return challenge, nil
+	return challenge
 }
 
 func FindChallengeByChallengeID(tx *storage.Connection, challengeID uuid.UUID) (*Challenge, error) {
diff --git a/internal/models/factor.go b/internal/models/factor.go
index e4162a0b..5af45772 100644
--- a/internal/models/factor.go
+++ b/internal/models/factor.go
@@ -124,7 +124,7 @@ func (Factor) TableName() string {
 	return tableName
 }
 
-func NewFactor(user *User, friendlyName string, factorType string, state FactorState, secret string) (*Factor, error) {
+func NewFactor(user *User, friendlyName string, factorType string, state FactorState, secret string) *Factor {
 	id := uuid.Must(uuid.NewV4())
 
 	factor := &Factor{
@@ -135,7 +135,7 @@ func NewFactor(user *User, friendlyName string, factorType string, state FactorS
 		Secret:       secret,
 		FactorType:   factorType,
 	}
-	return factor, nil
+	return factor
 }
 
 // FindFactorsByUser returns all factors belonging to a user ordered by timestamp
diff --git a/internal/utilities/postgres.go b/internal/utilities/postgres.go
index 08a57b6a..4d7fde8d 100644
--- a/internal/utilities/postgres.go
+++ b/internal/utilities/postgres.go
@@ -34,6 +34,10 @@ func NewPostgresError(err error) *PostgresError {
 
 	return nil
 }
+func (pg *PostgresError) IsUniqueConstraintViolated() bool {
+	// See https://www.postgresql.org/docs/current/errcodes-appendix.html for list of error codes
+	return pg.Code == "23505"
+}
 
 // isPubliclyAccessiblePostgresError checks if the Postgres error should be
 // made accessible.


Test patch:
diff --git a/internal/api/admin_test.go b/internal/api/admin_test.go
index b7a361dd..35447bcf 100644
--- a/internal/api/admin_test.go
+++ b/internal/api/admin_test.go
@@ -721,9 +721,7 @@ func (ts *AdminTestSuite) TestAdminUserDeleteFactor() {
 	require.NoError(ts.T(), err, "Error making new user")
 	require.NoError(ts.T(), ts.API.db.Create(u), "Error creating user")
 
-	f, err := models.NewFactor(u, "testSimpleName", models.TOTP, models.FactorStateVerified, "secretkey")
-
-	require.NoError(ts.T(), err, "Error creating test factor model")
+	f := models.NewFactor(u, "testSimpleName", models.TOTP, models.FactorStateVerified, "secretkey")
 	require.NoError(ts.T(), ts.API.db.Create(f), "Error saving new test factor")
 
 	// Setup request
@@ -746,8 +744,7 @@ func (ts *AdminTestSuite) TestAdminUserGetFactors() {
 	require.NoError(ts.T(), err, "Error making new user")
 	require.NoError(ts.T(), ts.API.db.Create(u), "Error creating user")
 
-	f, err := models.NewFactor(u, "testSimpleName", models.TOTP, models.FactorStateUnverified, "secretkey")
-	require.NoError(ts.T(), err, "Error creating test factor model")
+	f := models.NewFactor(u, "testSimpleName", models.TOTP, models.FactorStateUnverified, "secretkey")
 	require.NoError(ts.T(), ts.API.db.Create(f), "Error saving new test factor")
 
 	// Setup request
@@ -768,8 +765,7 @@ func (ts *AdminTestSuite) TestAdminUserUpdateFactor() {
 	require.NoError(ts.T(), err, "Error making new user")
 	require.NoError(ts.T(), ts.API.db.Create(u), "Error creating user")
 
-	f, err := models.NewFactor(u, "testSimpleName", models.TOTP, models.FactorStateUnverified, "secretkey")
-	require.NoError(ts.T(), err, "Error creating test factor model")
+	f := models.NewFactor(u, "testSimpleName", models.TOTP, models.FactorStateUnverified, "secretkey")
 	require.NoError(ts.T(), ts.API.db.Create(f), "Error saving new test factor")
 
 	var cases = []struct {
diff --git a/internal/api/mfa_test.go b/internal/api/mfa_test.go
index 9aa57605..c1b107aa 100644
--- a/internal/api/mfa_test.go
+++ b/internal/api/mfa_test.go
@@ -57,8 +57,7 @@ func (ts *MFATestSuite) SetupTest() {
 	require.NoError(ts.T(), err, "Error creating test user model")
 	require.NoError(ts.T(), ts.API.db.Create(u), "Error saving new test user")
 	// Create Factor
-	f, err := models.NewFactor(u, "test_factor", models.TOTP, models.FactorStateUnverified, "secretkey")
-	require.NoError(ts.T(), err, "Error creating test factor model")
+	f := models.NewFactor(u, "test_factor", models.TOTP, models.FactorStateUnverified, "secretkey")
 	require.NoError(ts.T(), ts.API.db.Create(f), "Error saving new test factor")
 	// Create corresponding session
 	s, err := models.NewSession()
@@ -120,7 +119,6 @@ func (ts *MFATestSuite) TestEnrollFactor() {
 			issuer:       ts.TestDomain,
 			expectedCode: http.StatusBadRequest,
 		},
-
 		{
 			desc:         "TOTP: Factor has friendly name",
 			friendlyName: testFriendlyName,
@@ -158,6 +156,22 @@ func (ts *MFATestSuite) TestEnrollFactor() {
 			}
 		})
 	}
+}
+
+func (ts *MFATestSuite) TestDuplicateEnrollsReturnExpectedMessage() {
+	friendlyName := "mary"
+	token, _, err := ts.API.generateAccessToken(context.Background(), ts.API.db, ts.TestUser, nil, models.TOTPSignIn)
+	require.NoError(ts.T(), err)
+	_ = performEnrollFlow(ts, token, friendlyName, models.TOTP, "https://issuer.com", http.StatusOK)
+	response := performEnrollFlow(ts, token, friendlyName, models.TOTP, "https://issuer.com", http.StatusInternalServerError)
+
+	var errorResponse HTTPError
+	err = json.NewDecoder(response.Body).Decode(&errorResponse)
+	require.NoError(ts.T(), err)
+
+	// Convert the response body to a string and check for the expected error message
+	expectedErrorMessage := fmt.Sprintf("a factor with the friendly name %q for this user likely already exists", friendlyName)
+	require.Contains(ts.T(), errorResponse.Message, expectedErrorMessage)
 
 }
 
@@ -221,8 +235,7 @@ func (ts *MFATestSuite) TestMFAVerifyFactor() {
 			req := httptest.NewRequest(http.MethodPost, fmt.Sprintf("/factors/%s/verify", f.ID), &buffer)
 			testIPAddress := utilities.GetIPAddress(req)
 			req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token))
-			c, err := models.NewChallenge(f, testIPAddress)
-			require.NoError(ts.T(), err, "Error creating test Challenge model")
+			c := models.NewChallenge(f, testIPAddress)
 			require.NoError(ts.T(), ts.API.db.Create(c), "Error saving new test challenge")
 			if !v.validChallenge {
 				// Set challenge creation so that it has expired in present time.
@@ -650,5 +663,4 @@ func cleanupHook(ts *MFATestSuite, hookName string) {
 	cleanupHookSQL := fmt.Sprintf("drop function if exists %s", hookName)
 	err := ts.API.db.RawQuery(cleanupHookSQL).Exec()
 	require.NoError(ts.T(), err)
-
 }
diff --git a/internal/models/factor_test.go b/internal/models/factor_test.go
index 63f04e0a..eb5dccb9 100644
--- a/internal/models/factor_test.go
+++ b/internal/models/factor_test.go
@@ -50,8 +50,7 @@ func (ts *FactorTestSuite) createFactor() *Factor {
 	err = ts.db.Create(user)
 	require.NoError(ts.T(), err)
 
-	factor, err := NewFactor(user, "asimplename", TOTP, FactorStateUnverified, "topsecret")
-	require.NoError(ts.T(), err)
+	factor := NewFactor(user, "asimplename", TOTP, FactorStateUnverified, "topsecret")
 
 	err = ts.db.Create(factor)
 	require.NoError(ts.T(), err)
@@ -63,8 +62,7 @@ func (ts *FactorTestSuite) TestUpdateStatus() {
 	u, err := NewUser("", "", "", "", nil)
 	require.NoError(ts.T(), err)
 
-	f, err := NewFactor(u, "", TOTP, FactorStateUnverified, "some-secret")
-	require.NoError(ts.T(), err)
+	f := NewFactor(u, "", TOTP, FactorStateUnverified, "some-secret")
 	require.NoError(ts.T(), f.UpdateStatus(ts.db, newFactorStatus))
 	require.Equal(ts.T(), newFactorStatus.String(), f.Status)
 }
@@ -74,8 +72,7 @@ func (ts *FactorTestSuite) TestUpdateFriendlyName() {
 	u, err := NewUser("", "", "", "", nil)
 	require.NoError(ts.T(), err)
 
-	f, err := NewFactor(u, "A1B2C3", TOTP, FactorStateUnverified, "some-secret")
-	require.NoError(ts.T(), err)
+	f := NewFactor(u, "A1B2C3", TOTP, FactorStateUnverified, "some-secret")
 	require.NoError(ts.T(), f.UpdateFriendlyName(ts.db, newSimpleName))
 	require.Equal(ts.T(), newSimpleName, f.FriendlyName)
 }
@@ -84,8 +81,7 @@ func (ts *FactorTestSuite) TestEncodedFactorDoesNotLeakSecret() {
 	u, err := NewUser("", "", "", "", nil)
 	require.NoError(ts.T(), err)
 
-	f, err := NewFactor(u, "A1B2C3", TOTP, FactorStateUnverified, "some-secret")
-	require.NoError(ts.T(), err)
+	f := NewFactor(u, "A1B2C3", TOTP, FactorStateUnverified, "some-secret")
 	encodedFactor, err := json.Marshal(f)
 	require.NoError(ts.T(), err)
 	decodedFactor := Factor{}