From 119646034b52936b7ae596371a48593d6806c7ca Mon Sep 17 00:00:00 2001 From: fadymak Date: Fri, 4 Sep 2026 09:13:43 +0200 Subject: [PATCH] feat(mfa): add recovery codes unenroll guards --- internal/api/mfa.go | 33 ++++ internal/api/recovery_codes.go | 9 +- internal/api/recovery_codes_test.go | 264 ++++++++++++++++++++++++++++ 3 files changed, 298 insertions(+), 8 deletions(-) diff --git a/internal/api/mfa.go b/internal/api/mfa.go index 2d9f2fa7b2..19db81968d 100644 --- a/internal/api/mfa.go +++ b/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 } diff --git a/internal/api/recovery_codes.go b/internal/api/recovery_codes.go index bd39ee9d93..44585c6fbf 100644 --- a/internal/api/recovery_codes.go +++ b/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") } diff --git a/internal/api/recovery_codes_test.go b/internal/api/recovery_codes_test.go index c83f4d1725..842f090d35 100644 --- a/internal/api/recovery_codes_test.go +++ b/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) + }) +}