feat: clean up expired factors (#1371)

## What kind of change does this PR introduce?

Currently, unverified MFA factors can build up in the database quickly.
Supabase developers can toggle Maximum unverified factors ( `Maximum
number of per-user MFA factors`) via the dashboard but developers will
have to look for the toggle. Developers can also call `unenroll` but it
requires an additional step.


This PR proposes periodic cleanup of stale factors on each request. A
stale factor is:

- Unverified
- Has no associated Challenges 
- Older than five minutes

Why five minutes? 
- Most enrolment or verification flow should be completed within the
five minute window

Factors which are unverified but have associated challenges will be
cleaned up [after the developer makes successful
verification](https://github.com/supabase/gotrue/blob/master/internal/api/mfa.go#L314)

Alternatives considered:
- Return the same factor and QR code if the same user calls `/enroll`
twice. We unfortunately can't reuse the QR code as it poses a security
risk.
- Increase the initial number of default unverified factors (currently
10)
- Drop the unverified factor check. I think this was initially
introduced to prevent a malicious user from creating excessive entries
in the database


Address #979.

---------

Co-authored-by: joel@joellee.org <joel@joellee.org>
Co-authored-by: joel <joel@joels-MacBook-Pro.local>

internal/api/mfa.go

@@ -64,12 +64,16 @@ func (a *API) EnrollFactor(w http.ResponseWriter, r *http.Request) error {
 	session := getSession(ctx)
 	config := a.config
 
+	if session == nil || user == nil {
+		return internalServerError("A valid session and a registered user are required to enroll a factor")
+	}
+
 	params := &EnrollFactorParams{}
 	if err := retrieveRequestParams(r, params); err != nil {
 		return err
 	}
-	issuer := ""
 
+	issuer := ""
 	if params.FactorType != models.TOTP {
 		return badRequestError("factor_type needs to be totp")
 	}
@@ -84,25 +88,26 @@ func (a *API) EnrollFactor(w http.ResponseWriter, r *http.Request) error {
 		issuer = params.Issuer
 	}
 
-	// Read from DB for certainty
-	factors, err := models.FindFactorsByUser(a.db, user)
-	if err != nil {
-		return internalServerError("error validating number of factors in system").WithInternalError(err)
-	}
+	factors := user.Factors
 
-	if len(factors) >= int(config.MFA.MaxEnrolledFactors) {
-		return forbiddenError("Enrolled factors exceed allowed limit, unenroll to continue")
+	factorCount := len(factors)
+	numVerifiedFactors := 0
+	if err := models.DeleteExpiredFactors(a.db, config.MFA.FactorExpiryDuration); err != nil {
+		return err
 	}
 
-	numVerifiedFactors := 0
 	for _, factor := range factors {
 		if factor.IsVerified() {
 			numVerifiedFactors += 1
 		}
 	}
 
+	if factorCount >= int(config.MFA.MaxEnrolledFactors) {
+		return forbiddenError("Enrolled factors exceed allowed limit, unenroll to continue")
+	}
+
 	if numVerifiedFactors >= config.MFA.MaxVerifiedFactors {
-		return forbiddenError("Maximum number of enrolled factors reached, unenroll to continue")
+		return forbiddenError("Maximum number of verified factors reached, unenroll to continue")
 	}
 
 	if numVerifiedFactors > 0 && !session.IsAAL2() {
@@ -116,6 +121,7 @@ func (a *API) EnrollFactor(w http.ResponseWriter, r *http.Request) error {
 	if err != nil {
 		return internalServerError(QRCodeGenerationErrorMessage).WithInternalError(err)
 	}
+
 	var buf bytes.Buffer
 	svgData := svg.New(&buf)
 	qrCode, _ := qr.Encode(key.String(), qr.H, qr.Auto)

internal/api/mfa_test.go

@@ -180,6 +180,34 @@ func (ts *MFATestSuite) TestDuplicateEnrollsReturnExpectedMessage() {
 
 }
 
+func (ts *MFATestSuite) TestMultipleEnrollsCleanupExpiredFactors() {
+	// All factors are deleted when a subsequent enroll is made
+	ts.API.config.MFA.FactorExpiryDuration = 0 * time.Second
+	// Verified factor should not be deleted (Factor 1)
+	resp := performTestSignupAndVerify(ts, ts.TestEmail, ts.TestPassword, true /* <- requireStatusOK */)
+	numFactors := 5
+	accessTokenResp := &AccessTokenResponse{}
+	require.NoError(ts.T(), json.NewDecoder(resp.Body).Decode(&accessTokenResp))
+
+	token := accessTokenResp.Token
+	for i := 0; i < numFactors; i++ {
+		_ = performEnrollFlow(ts, token, "", models.TOTP, "https://issuer.com", http.StatusOK)
+	}
+
+	// All Factors except last factor should be expired
+	factors, err := models.FindFactorsByUser(ts.API.db, ts.TestUser)
+	require.NoError(ts.T(), err)
+
+	// Make a challenge so last, unverified factor isn't deleted on next enroll (Factor 2)
+	_ = performChallengeFlow(ts, factors[len(factors)-1].ID, token)
+
+	// Enroll another Factor (Factor 3)
+	_ = performEnrollFlow(ts, token, "", models.TOTP, "https://issuer.com", http.StatusOK)
+	factors, err = models.FindFactorsByUser(ts.API.db, ts.TestUser)
+	require.NoError(ts.T(), err)
+	require.Equal(ts.T(), 3, len(factors))
+}
+
 func (ts *MFATestSuite) TestChallengeFactor() {
 	f := ts.TestUser.Factors[0]
 	token := ts.generateAAL1Token(ts.TestUser, &ts.TestSession.ID)
@@ -431,7 +459,7 @@ func performTestSignupAndVerify(ts *MFATestSuite, email, password string, requir
 
 func performEnrollFlow(ts *MFATestSuite, token, friendlyName, factorType, issuer string, expectedCode int) *httptest.ResponseRecorder {
 	var buffer bytes.Buffer
-	require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]string{"friendly_name": friendlyName, "factor_type": factorType, "issuer": issuer}))
+	require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(EnrollFactorParams{FriendlyName: friendlyName, FactorType: factorType, Issuer: issuer}))
 	w := ServeAuthenticatedRequest(ts, http.MethodPost, "http://localhost/factors/", token, buffer)
 	require.Equal(ts.T(), expectedCode, w.Code)
 	return w

internal/conf/configuration.go

@@ -18,6 +18,7 @@ import (
 
 const defaultMinPasswordLength int = 6
 const defaultChallengeExpiryDuration float64 = 300
+const defaultFactorExpiryDuration time.Duration = 300 * time.Second
 const defaultFlowStateExpiryDuration time.Duration = 300 * time.Second
 
 // See: https://www.postgresql.org/docs/7.0/syntax525.htm
@@ -102,11 +103,12 @@ type JWTConfiguration struct {
 
 // MFAConfiguration holds all the MFA related Configuration
 type MFAConfiguration struct {
-	Enabled                     bool    `default:"false"`
-	ChallengeExpiryDuration     float64 `json:"challenge_expiry_duration" default:"300" split_words:"true"`
-	RateLimitChallengeAndVerify float64 `split_words:"true" default:"15"`
-	MaxEnrolledFactors          float64 `split_words:"true" default:"10"`
-	MaxVerifiedFactors          int     `split_words:"true" default:"10"`
+	Enabled                     bool          `default:"false"`
+	ChallengeExpiryDuration     float64       `json:"challenge_expiry_duration" default:"300" split_words:"true"`
+	FactorExpiryDuration        time.Duration `json:"factor_expiry_duration" default:"300s" split_words:"true"`
+	RateLimitChallengeAndVerify float64       `split_words:"true" default:"15"`
+	MaxEnrolledFactors          float64       `split_words:"true" default:"10"`
+	MaxVerifiedFactors          int           `split_words:"true" default:"10"`
 }
 
 type APIConfiguration struct {
@@ -711,6 +713,9 @@ func (config *GlobalConfiguration) ApplyDefaults() error {
 	if config.MFA.ChallengeExpiryDuration < defaultChallengeExpiryDuration {
 		config.MFA.ChallengeExpiryDuration = defaultChallengeExpiryDuration
 	}
+	if config.MFA.FactorExpiryDuration < defaultFactorExpiryDuration {
+		config.MFA.FactorExpiryDuration = defaultFactorExpiryDuration
+	}
 	if config.External.FlowStateExpiryDuration < defaultFlowStateExpiryDuration {
 		config.External.FlowStateExpiryDuration = defaultFlowStateExpiryDuration
 	}

internal/models/factor.go

@@ -217,3 +217,17 @@ func DeleteFactorsByUserId(tx *storage.Connection, userId uuid.UUID) error {
 	}
 	return nil
 }
+
+func DeleteExpiredFactors(tx *storage.Connection, validityDuration time.Duration) error {
+	totalSeconds := int64(validityDuration / time.Second)
+	validityInterval := fmt.Sprintf("interval '%d seconds'", totalSeconds)
+
+	factorTable := (&pop.Model{Value: Factor{}}).TableName()
+	challengeTable := (&pop.Model{Value: Challenge{}}).TableName()
+
+	query := fmt.Sprintf(`delete from %q where status != 'verified' and not exists (select * from %q where %q.id = %q.factor_id ) and created_at + %s < current_timestamp;`, factorTable, challengeTable, factorTable, challengeTable, validityInterval)
+	if err := tx.RawQuery(query).Exec(); err != nil {
+		return err
+	}
+	return nil
+}