feat(mfa): add recovery codes unenroll guards

internal/api/mfa.go

@@ -134,6 +134,16 @@ func validateFactors(db *storage.Connection, user *models.User, newFactorName st
 	return nil
 }
 
+func hasVerifiedNonRecoveryFactor(factors []models.Factor, unenrollingID uuid.UUID) bool {
+	for _, f := range factors {
+		if f.ID != unenrollingID && f.IsVerified() && !f.IsRecoveryCodeFactor() {
+			return true
+		}
+	}
+
+	return false
+}
+
 func (a *API) enrollPhoneFactor(w http.ResponseWriter, r *http.Request, params *EnrollFactorParams) error {
 	ctx := r.Context()
 	config := a.config
@@ -1035,6 +1045,10 @@ func (a *API) UnenrollFactor(w http.ResponseWriter, r *http.Request) error {
 		return apierrors.NewInternalServerError("A valid session and factor are required to unenroll a factor")
 	}
 
+	if factor.IsRecoveryCodeFactor() {
+		return apierrors.NewUnprocessableEntityError(apierrors.ErrorCodeValidationFailed, "Recovery codes cannot be unenrolled with this endpoint, use DELETE /factors/recovery-codes")
+	}
+
 	if factor.IsVerified() && !session.IsAAL2() {
 		return apierrors.NewUnprocessableEntityError(apierrors.ErrorCodeInsufficientAAL, "AAL2 required to unenroll verified factor")
 	}
@@ -1043,6 +1057,25 @@ func (a *API) UnenrollFactor(w http.ResponseWriter, r *http.Request) error {
 
 	err = db.Transaction(func(tx *storage.Connection) error {
 		var terr error
+
+		// Recovery codes can never be a user's only second factor.
+		if factor.IsVerified() {
+			_, terr := models.FindRecoveryCodeSetForUpdate(tx, user.ID)
+			if terr == nil {
+				if terr := tx.Load(user, "Factors"); terr != nil {
+					return apierrors.NewInternalServerError("Database error loading factors").WithInternalError(terr)
+				}
+
+				if !hasVerifiedNonRecoveryFactor(user.Factors, factor.ID) {
+					return apierrors.NewUnprocessableEntityError(apierrors.ErrorCodeMFARecoveryCodesSoleFactor, "Recovery codes cannot be the only verified factor. Please enroll another factor before unenrolling this one or delete your recovery codes.")
+				}
+			} else if !models.IsNotFoundError(terr) {
+				return apierrors.NewInternalServerError("Database error locking recovery code set").WithInternalError(terr)
+			}
+
+			// no recovery codes are enrolled, nothing to protect.
+		}
+
 		if terr := tx.Destroy(factor); terr != nil {
 			return terr
 		}

internal/api/recovery_codes.go

@@ -133,14 +133,7 @@ func (a *API) RecoveryCodesGenerate(w http.ResponseWriter, r *http.Request) erro
 	}
 
 	// Recovery codes can never be the user's only second factor.
-	hasOtherVerifiedFactor := false
-	for _, f := range user.Factors {
-		if f.IsVerified() && !f.IsRecoveryCodeFactor() {
-			hasOtherVerifiedFactor = true
-			break
-		}
-	}
-	if !hasOtherVerifiedFactor {
+	if !hasVerifiedNonRecoveryFactor(user.Factors, uuid.Nil) {
 		return apierrors.NewUnprocessableEntityError(apierrors.ErrorCodeMFARecoveryCodesSoleFactor, "At least one other verified factor is required to generate recovery codes")
 	}
 

internal/api/recovery_codes_test.go

@@ -968,3 +968,267 @@ func (ts *RecoveryCodesTestSuite) TestRecoveryCodesDeleteUnenrolledNotificationD
 
 	require.Empty(ts.T(), mockMailer.MFAFactorUnenrolledMailCalls, "Expected no MFA factor unenrolled notification email to be sent")
 }
+
+// adminToken mints a supabase_admin JWT for the admin factor endpoints.
+func (ts *RecoveryCodesTestSuite) adminToken() string {
+	claims := &AccessTokenClaims{Role: "supabase_admin"}
+	token, err := jwt.NewWithClaims(jwt.SigningMethodHS256, claims).SignedString([]byte(ts.Config.JWT.Secret))
+	require.NoError(ts.T(), err, "Error generating admin jwt")
+	return token
+}
+
+func (ts *RecoveryCodesTestSuite) createTOTPFactor(friendlyName string, state models.FactorState) *models.Factor {
+	f := models.NewTOTPFactor(ts.TestUser, friendlyName)
+	require.NoError(ts.T(), f.SetSecret("secretkey", ts.Config.Security.DBEncryption.Encrypt, ts.Config.Security.DBEncryption.EncryptionKeyID, ts.Config.Security.DBEncryption.EncryptionKey))
+	require.NoError(ts.T(), ts.API.db.Create(f))
+	if state == models.FactorStateVerified {
+		require.NoError(ts.T(), f.UpdateStatus(ts.API.db, models.FactorStateVerified))
+	}
+	return f
+}
+
+// performUnenroll hits the generic DELETE /factors/{factor_id} endpoint.
+func (ts *RecoveryCodesTestSuite) performUnenroll(token string, factorID uuid.UUID) *httptest.ResponseRecorder {
+	return ts.serveRequest(http.MethodDelete, fmt.Sprintf("http://localhost/factors/%s", factorID), token, nil)
+}
+
+// performEnrollTOTP hits the generic POST /factors endpoint with a TOTP factor.
+func (ts *RecoveryCodesTestSuite) performEnrollTOTP(token, friendlyName string) *httptest.ResponseRecorder {
+	var buffer bytes.Buffer
+	require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(EnrollFactorParams{
+		FriendlyName: friendlyName,
+		FactorType:   models.TOTP,
+		Issuer:       "supabase.com",
+	}))
+	return ts.serveRequest(http.MethodPost, "http://localhost/factors/", token, &buffer)
+}
+
+func (ts *RecoveryCodesTestSuite) TestRecoveryCodesUnenrollLastSecondFactorBlocked() {
+	token := ts.aal2Token()
+	generateResp := ts.performGenerate(token, nil)
+
+	// The TOTP factor is the only second factor; unenrolling it would strand the codes.
+	w := ts.performUnenroll(token, ts.TestFactor.ID)
+	ts.requireErrorCode(w, http.StatusUnprocessableEntity, apierrors.ErrorCodeMFARecoveryCodesSoleFactor)
+
+	_, err := models.FindFactorByFactorID(ts.API.db, ts.TestFactor.ID)
+	require.NoError(ts.T(), err)
+	_, err = models.FindFactorByFactorID(ts.API.db, generateResp.ID)
+	require.NoError(ts.T(), err)
+	ts.recoveryCodeSetState()
+
+	logs, err := models.FindAuditLogEntries(ts.API.db, []string{"action"}, string(models.UnenrollFactorAction), nil)
+	require.NoError(ts.T(), err)
+	require.Empty(ts.T(), logs, "a blocked unenroll must not be audited")
+
+	// Deleting the recovery codes first lifts the guard.
+	w = ts.performDelete(token)
+	require.Equal(ts.T(), http.StatusOK, w.Code)
+
+	w = ts.performUnenroll(token, ts.TestFactor.ID)
+	require.Equal(ts.T(), http.StatusOK, w.Code)
+	resp := UnenrollFactorResponse{}
+	require.NoError(ts.T(), json.NewDecoder(w.Body).Decode(&resp))
+	require.Equal(ts.T(), ts.TestFactor.ID, resp.ID)
+	_, err = models.FindFactorByFactorID(ts.API.db, ts.TestFactor.ID)
+	require.EqualError(ts.T(), err, models.FactorNotFoundError{}.Error())
+
+	logs, err = models.FindAuditLogEntries(ts.API.db, []string{"action"}, string(models.UnenrollFactorAction), nil)
+	require.NoError(ts.T(), err)
+	require.Len(ts.T(), logs, 1)
+}
+
+func (ts *RecoveryCodesTestSuite) TestRecoveryCodesUnenrollOtherSecondFactorAllowed() {
+	token := ts.aal2Token()
+	ts.performGenerate(token, nil)
+	second := ts.createTOTPFactor("second_factor", models.FactorStateVerified)
+
+	// Another verified second factor remains, so this unenroll is unaffected.
+	w := ts.performUnenroll(token, second.ID)
+	require.Equal(ts.T(), http.StatusOK, w.Code)
+	resp := UnenrollFactorResponse{}
+	require.NoError(ts.T(), json.NewDecoder(w.Body).Decode(&resp))
+	require.Equal(ts.T(), second.ID, resp.ID)
+	_, err := models.FindFactorByFactorID(ts.API.db, second.ID)
+	require.EqualError(ts.T(), err, models.FactorNotFoundError{}.Error())
+
+	ts.recoveryCodeSetState()
+	w = ts.serveRequest(http.MethodGet, "http://localhost/factors/recovery-codes", token, nil)
+	require.Equal(ts.T(), http.StatusOK, w.Code)
+
+	// The remaining TOTP factor is now the last second factor.
+	w = ts.performUnenroll(token, ts.TestFactor.ID)
+	ts.requireErrorCode(w, http.StatusUnprocessableEntity, apierrors.ErrorCodeMFARecoveryCodesSoleFactor)
+}
+
+func (ts *RecoveryCodesTestSuite) TestRecoveryCodesUnenrollUnverifiedFactorAllowed() {
+	token := ts.aal2Token()
+	ts.performGenerate(token, nil)
+	unverified := ts.createTOTPFactor("pending_factor", models.FactorStateUnverified)
+
+	// An unverified factor is not a usable second factor, so the guard does not apply
+	w := ts.performUnenroll(token, unverified.ID)
+	require.Equal(ts.T(), http.StatusOK, w.Code)
+	_, err := models.FindFactorByFactorID(ts.API.db, unverified.ID)
+	require.EqualError(ts.T(), err, models.FactorNotFoundError{}.Error())
+	ts.recoveryCodeSetState()
+}
+
+func (ts *RecoveryCodesTestSuite) TestRecoveryCodesGenericUnenrollRejected() {
+	generateResp, _, _ := ts.enrollForVerify()
+	aal2Token := ts.token(ts.TestUser, &ts.TestSession.ID)
+
+	ts.Run("AAL2", func() {
+		w := ts.performUnenroll(aal2Token, generateResp.ID)
+		ts.requireErrorCode(w, http.StatusUnprocessableEntity, apierrors.ErrorCodeValidationFailed)
+	})
+
+	_, err := models.FindFactorByFactorID(ts.API.db, generateResp.ID)
+	require.NoError(ts.T(), err)
+	ts.recoveryCodeSetState()
+
+	for _, action := range []models.AuditAction{models.RecoveryCodesDeletedAction, models.UnenrollFactorAction} {
+		logs, err := models.FindAuditLogEntries(ts.API.db, []string{"action"}, string(action), nil)
+		require.NoError(ts.T(), err)
+		require.Empty(ts.T(), logs)
+	}
+}
+
+func (ts *RecoveryCodesTestSuite) TestRecoveryCodesGenericEndpointsRejectRecoveryFactor() {
+	token := ts.aal2Token()
+	generateResp := ts.performGenerate(token, nil)
+
+	ts.Run("Enroll", func() {
+		var buffer bytes.Buffer
+		require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(EnrollFactorParams{FriendlyName: "codes", FactorType: models.RecoveryCode}))
+		w := ts.serveRequest(http.MethodPost, "http://localhost/factors/", token, &buffer)
+		ts.requireErrorCode(w, http.StatusBadRequest, apierrors.ErrorCodeValidationFailed)
+	})
+
+	ts.Run("Challenge", func() {
+		w := ts.serveRequest(http.MethodPost, fmt.Sprintf("http://localhost/factors/%s/challenge", generateResp.ID), token, nil)
+		ts.requireErrorCode(w, http.StatusBadRequest, apierrors.ErrorCodeValidationFailed)
+	})
+
+	ts.Run("Verify", func() {
+		var buffer bytes.Buffer
+		require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]any{"code": generateResp.Codes[0]}))
+		w := ts.serveRequest(http.MethodPost, fmt.Sprintf("http://localhost/factors/%s/verify", generateResp.ID), token, &buffer)
+		ts.requireErrorCode(w, http.StatusBadRequest, apierrors.ErrorCodeValidationFailed)
+	})
+
+	// The rejected generic verify did not consume the real code it was given.
+	require.Equal(ts.T(), generateResp.Total, ts.unusedCodeCount())
+}
+
+func (ts *RecoveryCodesTestSuite) TestRecoveryCodesOccupyFactorSlot() {
+	token := ts.aal2Token()
+	ts.performGenerate(token, nil)
+	// The user now holds two verified factors: TOTP and recovery codes.
+
+	ts.Run("MaxEnrolledFactors", func() {
+		ts.Config.MFA.MaxEnrolledFactors = 2
+		defer func() { ts.Config.MFA.MaxEnrolledFactors = 10 }()
+
+		w := ts.performEnrollTOTP(token, "another_factor")
+		ts.requireErrorCode(w, http.StatusUnprocessableEntity, apierrors.ErrorCodeTooManyEnrolledMFAFactors)
+	})
+
+	ts.Run("MaxVerifiedFactors", func() {
+		ts.Config.MFA.MaxVerifiedFactors = 2
+		defer func() { ts.Config.MFA.MaxVerifiedFactors = 10 }()
+
+		w := ts.performEnrollTOTP(token, "another_factor")
+		ts.requireErrorCode(w, http.StatusUnprocessableEntity, apierrors.ErrorCodeTooManyEnrolledMFAFactors)
+	})
+
+	ts.Run("WithinLimits", func() {
+		w := ts.performEnrollTOTP(token, "another_factor")
+		require.Equal(ts.T(), http.StatusOK, w.Code)
+	})
+}
+
+func (ts *RecoveryCodesTestSuite) TestRecoveryCodesAdminDelete() {
+	generateResp, verifySession, verifyToken := ts.enrollForVerify()
+	setID := ts.recoveryCodeSetState().ID
+
+	// Upgrade the AAL1 session with a recovery code so deletion has a session to downgrade.
+	w := ts.performVerify(verifyToken, generateResp.Codes[0])
+	require.Equal(ts.T(), http.StatusOK, w.Code)
+	upgraded, err := models.FindSessionByID(ts.API.db, verifySession.ID, false)
+	require.NoError(ts.T(), err)
+	require.True(ts.T(), upgraded.IsAAL2())
+	require.True(ts.T(), hasRecoveryCodeAMRClaim(upgraded))
+
+	w = ts.serveRequest(http.MethodDelete, fmt.Sprintf("http://localhost/admin/users/%s/factors/%s/", ts.TestUser.ID, generateResp.ID), ts.adminToken(), nil)
+	require.Equal(ts.T(), http.StatusOK, w.Code)
+
+	// The factor is gone and the FK cascade removed the set and every code.
+	_, err = models.FindFactorByFactorID(ts.API.db, generateResp.ID)
+	require.EqualError(ts.T(), err, models.FactorNotFoundError{}.Error())
+	_, err = models.FindRecoveryCodeSetByUser(ts.API.db, ts.TestUser.ID)
+	require.True(ts.T(), models.IsNotFoundError(err))
+	total, remaining, err := models.CountRecoveryCodes(ts.API.db, setID)
+	require.NoError(ts.T(), err)
+	require.Equal(ts.T(), 0, total)
+	require.Equal(ts.T(), 0, remaining)
+
+	// The session upgraded by a recovery code is downgraded and loses its AMR claim.
+	downgraded, err := models.FindSessionByID(ts.API.db, verifySession.ID, false)
+	require.NoError(ts.T(), err)
+	require.Equal(ts.T(), models.AAL1.String(), downgraded.GetAAL())
+	require.Nil(ts.T(), downgraded.FactorID)
+	require.False(ts.T(), hasRecoveryCodeAMRClaim(downgraded))
+
+	// The TOTP-backed session is untouched.
+	totpSession, err := models.FindSessionByID(ts.API.db, ts.TestSession.ID, false)
+	require.NoError(ts.T(), err)
+	require.True(ts.T(), totpSession.IsAAL2())
+	require.NotNil(ts.T(), totpSession.FactorID)
+	require.Equal(ts.T(), ts.TestFactor.ID, *totpSession.FactorID)
+}
+
+func (ts *RecoveryCodesTestSuite) TestRecoveryCodesFactorListings() {
+	token := ts.aal2Token()
+	generateResp := ts.performGenerate(token, nil)
+
+	// Sensitive fields are not serialized
+	forbiddenKeys := []string{"secret", "code_hash", "codes", "failed_verification_count", "verification_locked_until"}
+
+	assertRecoveryFactorListed := func(factors []map[string]any) {
+		var recovery map[string]any
+		for _, f := range factors {
+			if f["factor_type"] == models.RecoveryCode {
+				require.Nil(ts.T(), recovery, "only one recovery-code factor expected")
+				recovery = f
+			}
+		}
+		require.NotNil(ts.T(), recovery, "recovery-code factor missing from listing")
+		require.Equal(ts.T(), generateResp.ID.String(), recovery["id"])
+		require.Equal(ts.T(), models.FactorStateVerified.String(), recovery["status"])
+		require.Equal(ts.T(), models.DefaultRecoveryCodeFriendlyName, recovery["friendly_name"])
+		for _, key := range forbiddenKeys {
+			require.NotContains(ts.T(), recovery, key)
+		}
+	}
+
+	ts.Run("AdminGetFactors", func() {
+		w := ts.serveRequest(http.MethodGet, fmt.Sprintf("http://localhost/admin/users/%s/factors/", ts.TestUser.ID), ts.adminToken(), nil)
+		require.Equal(ts.T(), http.StatusOK, w.Code)
+		var factors []map[string]any
+		require.NoError(ts.T(), json.NewDecoder(w.Body).Decode(&factors))
+		require.Len(ts.T(), factors, 2)
+		assertRecoveryFactorListed(factors)
+	})
+
+	ts.Run("UserGet", func() {
+		w := ts.serveRequest(http.MethodGet, "http://localhost/user", token, nil)
+		require.Equal(ts.T(), http.StatusOK, w.Code)
+		var user struct {
+			Factors []map[string]any `json:"factors"`
+		}
+		require.NoError(ts.T(), json.NewDecoder(w.Body).Decode(&user))
+		require.Len(ts.T(), user.Factors, 2)
+		assertRecoveryFactorListed(user.Factors)
+	})
+}