fix(mfa): ignore verify-disabled factors in recovery codes guard (#2824)

When a user has a second factor enrolled but verification has been
disabled at the configuration level, we should not allow the enrollment
of recovery codes as that would make the recovery codes the only usable
second factor.

Author: fadymak

internal/api/mfa.go

@@ -134,9 +134,30 @@ func validateFactors(db *storage.Connection, user *models.User, newFactorName st
 	return nil
 }
 
-func hasVerifiedNonRecoveryFactor(factors []models.Factor, unenrollingID uuid.UUID) bool {
+func isFactorTypeVerifyEnabled(config *conf.GlobalConfiguration, factorType string) bool {
+	switch factorType {
+	case models.TOTP:
+		return config.MFA.TOTP.VerifyEnabled
+	case models.Phone:
+		return config.MFA.Phone.VerifyEnabled
+	case models.WebAuthn:
+		return config.MFA.WebAuthn.VerifyEnabled
+	case models.RecoveryCode:
+		return config.MFA.RecoveryCodes.VerifyEnabled
+	default:
+		return false
+	}
+}
+
+// hasUsableNonRecoveryFactor reports whether the user has a second factor,
+// other than recovery codes, that can actually be used at sign-in (verified and enabled in config).
+func hasUsableNonRecoveryFactor(config *conf.GlobalConfiguration, factors []models.Factor, excludeID uuid.UUID) bool {
 	for _, f := range factors {
-		if f.ID != unenrollingID && f.IsVerified() && !f.IsRecoveryCodeFactor() {
+		if f.ID == excludeID || !f.IsVerified() || f.IsRecoveryCodeFactor() {
+			continue
+		}
+
+		if isFactorTypeVerifyEnabled(config, f.FactorType) {
 			return true
 		}
 	}
@@ -1066,7 +1087,7 @@ func (a *API) UnenrollFactor(w http.ResponseWriter, r *http.Request) error {
 					return apierrors.NewInternalServerError("Database error loading factors").WithInternalError(terr)
 				}
 
-				if !hasVerifiedNonRecoveryFactor(user.Factors, factor.ID) {
+				if !hasUsableNonRecoveryFactor(config, 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) {

internal/api/mfa_test.go

@@ -54,6 +54,57 @@ func TestMFA(t *testing.T) {
 	suite.Run(t, ts)
 }
 
+func TestIsFactorTypeVerifyEnabled(t *testing.T) {
+	config := &conf.GlobalConfiguration{}
+	config.MFA.TOTP.VerifyEnabled = true
+	config.MFA.Phone.VerifyEnabled = false
+	config.MFA.WebAuthn.VerifyEnabled = true
+	config.MFA.RecoveryCodes.VerifyEnabled = false
+
+	cases := map[string]bool{
+		models.TOTP:         true,
+		models.Phone:        false,
+		models.WebAuthn:     true,
+		models.RecoveryCode: false,
+		"unknown":           false,
+	}
+	for factorType, want := range cases {
+		require.Equal(t, want, isFactorTypeVerifyEnabled(config, factorType), factorType)
+	}
+}
+
+func TestHasUsableNonRecoveryFactor(t *testing.T) {
+	config := &conf.GlobalConfiguration{}
+	config.MFA.TOTP.VerifyEnabled = true
+	config.MFA.Phone.VerifyEnabled = false
+	config.MFA.RecoveryCodes.VerifyEnabled = true
+
+	factor := func(factorType string, state models.FactorState) models.Factor {
+		return models.Factor{ID: uuid.Must(uuid.NewV4()), FactorType: factorType, Status: state.String()}
+	}
+	verifiedTOTP := factor(models.TOTP, models.FactorStateVerified)
+
+	cases := []struct {
+		name    string
+		factors []models.Factor
+		exclude uuid.UUID
+		want    bool
+	}{
+		{name: "no factors", want: false},
+		{name: "verified factor of a verify-enabled type", factors: []models.Factor{verifiedTOTP}, want: true},
+		{name: "unverified factor", factors: []models.Factor{factor(models.TOTP, models.FactorStateUnverified)}, want: false},
+		{name: "verified factor of a verify-disabled type", factors: []models.Factor{factor(models.Phone, models.FactorStateVerified)}, want: false},
+		{name: "recovery-code factor never counts even when verify is enabled", factors: []models.Factor{factor(models.RecoveryCode, models.FactorStateVerified)}, want: false},
+		{name: "excluded factor is skipped", factors: []models.Factor{verifiedTOTP}, exclude: verifiedTOTP.ID, want: false},
+		{name: "another usable factor besides the excluded one", factors: []models.Factor{verifiedTOTP, factor(models.TOTP, models.FactorStateVerified)}, exclude: verifiedTOTP.ID, want: true},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			require.Equal(t, tc.want, hasUsableNonRecoveryFactor(config, tc.factors, tc.exclude))
+		})
+	}
+}
+
 func (ts *MFATestSuite) SetupTest() {
 	models.TruncateAll(ts.API.db)
 

internal/api/recovery_codes.go

@@ -147,7 +147,7 @@ func (a *API) RecoveryCodesGenerate(w http.ResponseWriter, r *http.Request) erro
 	}
 
 	// Recovery codes can never be the user's only second factor.
-	if !hasVerifiedNonRecoveryFactor(user.Factors, uuid.Nil) {
+	if !hasUsableNonRecoveryFactor(config, 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

@@ -320,6 +320,18 @@ func (ts *RecoveryCodesTestSuite) TestRecoveryCodesGenerateSoleFactor() {
 	ts.requireErrorCode(w, http.StatusUnprocessableEntity, apierrors.ErrorCodeMFARecoveryCodesSoleFactor)
 }
 
+func (ts *RecoveryCodesTestSuite) TestRecoveryCodesGenerateVerifyDisabledFactorNotCounted() {
+	token := ts.aal2Token()
+	// The verified TOTP factor cannot be used at sign-in once TOTP verification
+	// is disabled, so recovery codes would become the only usable factor.
+	prev := ts.Config.MFA.TOTP.VerifyEnabled
+	defer func() { ts.Config.MFA.TOTP.VerifyEnabled = prev }()
+	ts.Config.MFA.TOTP.VerifyEnabled = false
+
+	w := ts.serveRequest(http.MethodPost, "http://localhost/factors/recovery-codes", token, nil)
+	ts.requireErrorCode(w, http.StatusUnprocessableEntity, apierrors.ErrorCodeMFARecoveryCodesSoleFactor)
+}
+
 func (ts *RecoveryCodesTestSuite) TestRecoveryCodesGenerateAtFactorLimits() {
 	token := ts.aal2Token()
 
@@ -1019,6 +1031,15 @@ func (ts *RecoveryCodesTestSuite) createTOTPFactor(friendlyName string, state mo
 	return f
 }
 
+func (ts *RecoveryCodesTestSuite) createPhoneFactor(friendlyName string, state models.FactorState) *models.Factor {
+	f := models.NewPhoneFactor(ts.TestUser, "+15555555555", friendlyName)
+	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)
@@ -1093,6 +1114,32 @@ func (ts *RecoveryCodesTestSuite) TestRecoveryCodesUnenrollOtherSecondFactorAllo
 	ts.requireErrorCode(w, http.StatusUnprocessableEntity, apierrors.ErrorCodeMFARecoveryCodesSoleFactor)
 }
 
+func (ts *RecoveryCodesTestSuite) TestRecoveryCodesUnenrollVerifyDisabledFactorNotCounted() {
+	token := ts.aal2Token()
+	ts.performGenerate(token, nil)
+	phone := ts.createPhoneFactor("phone_factor", models.FactorStateVerified)
+	prev := ts.Config.MFA.Phone.VerifyEnabled
+	defer func() { ts.Config.MFA.Phone.VerifyEnabled = prev }()
+
+	// A verified phone factor does not count while phone verification is disabled,
+	// so the TOTP factor is still the last usable second factor.
+	ts.Config.MFA.Phone.VerifyEnabled = false
+	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)
+
+	// Enabling phone verification makes it a usable second factor and lifts the guard.
+	ts.Config.MFA.Phone.VerifyEnabled = true
+	w = ts.performUnenroll(token, ts.TestFactor.ID)
+	require.Equal(ts.T(), http.StatusOK, w.Code)
+	_, err = models.FindFactorByFactorID(ts.API.db, ts.TestFactor.ID)
+	require.EqualError(ts.T(), err, models.FactorNotFoundError{}.Error())
+	_, err = models.FindFactorByFactorID(ts.API.db, phone.ID)
+	require.NoError(ts.T(), err)
+	ts.recoveryCodeSetState()
+}
+
 func (ts *RecoveryCodesTestSuite) TestRecoveryCodesUnenrollUnverifiedFactorAllowed() {
 	token := ts.aal2Token()
 	ts.performGenerate(token, nil)