fix: move load user into validate factors

supabase · Sep 30, 2024 · 1578d18 · 1578d18
1 parent dabd447
commit 1578d18
Show file tree

Hide file tree

Showing 4 changed files with 11 additions and 9 deletions.
diff --git a/internal/api/debug.test4262789671 b/internal/api/debug.test4262789671
diff --git a/internal/api/mfa.go b/internal/api/mfa.go
@@ -70,10 +70,14 @@ const (
 	QRCodeGenerationErrorMessage = "Error generating QR Code"
 )
 
-func validateFactors(user *models.User, newFactorName string, config *conf.GlobalConfiguration, session *models.Session) error {
+func validateFactors(db *storage.Connection, user *models.User, newFactorName string, config *conf.GlobalConfiguration, session *models.Session) error {
 	factorCount := len(user.Factors)
 	numVerifiedFactors := 0
 
+	if err := db.Load(user, "Factors"); err != nil {
+		return err
+	}
+
 	for _, factor := range user.Factors {
 		if factor.FriendlyName == newFactorName {
 			return unprocessableEntityError(
@@ -137,7 +141,7 @@ func (a *API) enrollPhoneFactor(w http.ResponseWriter, r *http.Request, params *
 		return internalServerError("Database error deleting unverified phone factors").WithInternalError(err)
 	}
 
-	if err := validateFactors(user, params.FriendlyName, a.config, session); err != nil {
+	if err := validateFactors(db, user, params.FriendlyName, a.config, session); err != nil {
 		return err
 	}
 
@@ -186,10 +190,7 @@ func (a *API) enrollTOTPFactor(w http.ResponseWriter, r *http.Request, params *E
 		return err
 	}
 
-	if err := db.Load(user, "Factors"); err != nil {
-		return err
-	}
-	if err := validateFactors(user, params.FriendlyName, config, session); err != nil {
+	if err := validateFactors(db, user, params.FriendlyName, config, session); err != nil {
 		return err
 	}
 

diff --git a/internal/api/mfa_test.go b/internal/api/mfa_test.go
@@ -367,7 +367,7 @@ func (ts *MFATestSuite) TestMultipleEnrollsCleanupExpiredFactors() {
 	var w *httptest.ResponseRecorder
 	token := accessTokenResp.Token
 	for i := 0; i < numFactors; i++ {
-		w = performEnrollFlow(ts, token, "non-empty-name", models.TOTP, "https://issuer.com", "", http.StatusOK)
+		w = performEnrollFlow(ts, token, "first-name", models.TOTP, "https://issuer.com", "", http.StatusOK)
 	}
 
 	enrollResp := EnrollFactorResponse{}
@@ -377,7 +377,7 @@ func (ts *MFATestSuite) TestMultipleEnrollsCleanupExpiredFactors() {
 	_ = performChallengeFlow(ts, enrollResp.ID, token)
 
 	// Enroll another Factor (Factor 3)
-	_ = performEnrollFlow(ts, token, "non-empty-names", models.TOTP, "https://issuer.com", "", http.StatusOK)
+	_ = performEnrollFlow(ts, token, "second-name", models.TOTP, "https://issuer.com", "", http.StatusOK)
 	require.NoError(ts.T(), ts.API.db.Eager("Factors").Find(ts.TestUser, ts.TestUser.ID))
 	require.Equal(ts.T(), 3, len(ts.TestUser.Factors))
 }

diff --git a/internal/models/factor.go b/internal/models/factor.go
@@ -117,7 +117,8 @@ func ParseAuthenticationMethod(authMethod string) (AuthenticationMethod, error)
 }
 
 type Factor struct {
-	ID               uuid.UUID          `json:"id" db:"id"`
+	ID uuid.UUID `json:"id" db:"id"`
+	// TODO: Consider removing this nested user field. We don't use it.
 	User             User               `json:"-" belongs_to:"user"`
 	UserID           uuid.UUID          `json:"-" db:"user_id"`
 	CreatedAt        time.Time          `json:"created_at" db:"created_at"`