From 22e230f1b38b41ae2632871191e4c9b5af8ae3bf Mon Sep 17 00:00:00 2001 From: joel Date: Sat, 28 Sep 2024 10:59:00 +0200 Subject: [PATCH 1/2] fix: refactor mfa validation --- internal/api/mfa.go | 107 +++++++++++++++++--------------------- internal/api/mfa_test.go | 8 ++- internal/models/factor.go | 4 -- 3 files changed, 52 insertions(+), 67 deletions(-) diff --git a/internal/api/mfa.go b/internal/api/mfa.go index 357eea34c..7f1eef3bc 100644 --- a/internal/api/mfa.go +++ b/internal/api/mfa.go @@ -15,6 +15,7 @@ import ( "github.com/pquerna/otp" "github.com/pquerna/otp/totp" "github.com/supabase/auth/internal/api/sms_provider" + "github.com/supabase/auth/internal/conf" "github.com/supabase/auth/internal/crypto" "github.com/supabase/auth/internal/hooks" "github.com/supabase/auth/internal/metering" @@ -69,6 +70,40 @@ const ( QRCodeGenerationErrorMessage = "Error generating QR Code" ) +func validateFactors(db *storage.Connection, user *models.User, newFactorName string, config *conf.GlobalConfiguration, session *models.Session) error { + if err := db.Load(user, "Factors"); err != nil { + return err + } + factorCount := len(user.Factors) + numVerifiedFactors := 0 + + for _, factor := range user.Factors { + if factor.FriendlyName == newFactorName { + return unprocessableEntityError( + ErrorCodeMFAFactorNameConflict, + fmt.Sprintf("A factor with the friendly name %q for this user already exists", newFactorName), + ) + } + if factor.IsVerified() { + numVerifiedFactors++ + } + } + + if factorCount >= int(config.MFA.MaxEnrolledFactors) { + return unprocessableEntityError(ErrorCodeTooManyEnrolledMFAFactors, "Maximum number of verified factors reached, unenroll to continue") + } + + if numVerifiedFactors >= config.MFA.MaxVerifiedFactors { + return unprocessableEntityError(ErrorCodeTooManyEnrolledMFAFactors, "Maximum number of verified factors reached, unenroll to continue") + } + + if numVerifiedFactors > 0 && session != nil && !session.IsAAL2() { + return forbiddenError(ErrorCodeInsufficientAAL, "AAL2 required to enroll a new factor") + } + + return nil +} + func (a *API) enrollPhoneFactor(w http.ResponseWriter, r *http.Request, params *EnrollFactorParams) error { ctx := r.Context() config := a.config @@ -83,37 +118,21 @@ func (a *API) enrollPhoneFactor(w http.ResponseWriter, r *http.Request, params * if err != nil { return badRequestError(ErrorCodeValidationFailed, "Invalid phone number format (E.164 required)") } - factors := user.Factors - - factorCount := len(factors) - numVerifiedFactors := 0 if err := models.DeleteExpiredFactors(db, config.MFA.FactorExpiryDuration); err != nil { return err } + var factorsToDelete []models.Factor for _, factor := range user.Factors { - switch { - case factor.FriendlyName == params.FriendlyName: - return unprocessableEntityError( - ErrorCodeMFAFactorNameConflict, - fmt.Sprintf("A factor with the friendly name %q for this user already exists", factor.FriendlyName), - ) - - case factor.IsPhoneFactor(): - if factor.Phone.String() == phone { - if factor.IsVerified() { - return unprocessableEntityError( - ErrorCodeMFAVerifiedFactorExists, - "A verified phone factor already exists, unenroll the existing factor to continue", - ) - } else if factor.IsUnverified() { - factorsToDelete = append(factorsToDelete, factor) - } - + if factor.IsPhoneFactor() && factor.Phone.String() == phone { + if factor.IsVerified() { + return unprocessableEntityError( + ErrorCodeMFAVerifiedFactorExists, + "A verified phone factor already exists, unenroll the existing factor to continue", + ) + } else if factor.IsUnverified() { + factorsToDelete = append(factorsToDelete, factor) } - - case factor.IsVerified(): - numVerifiedFactors++ } } @@ -121,17 +140,10 @@ func (a *API) enrollPhoneFactor(w http.ResponseWriter, r *http.Request, params * return internalServerError("Database error deleting unverified phone factors").WithInternalError(err) } - if factorCount >= int(config.MFA.MaxEnrolledFactors) { - return unprocessableEntityError(ErrorCodeTooManyEnrolledMFAFactors, "Maximum number of verified factors reached, unenroll to continue") - } - - if numVerifiedFactors >= config.MFA.MaxVerifiedFactors { - return unprocessableEntityError(ErrorCodeTooManyEnrolledMFAFactors, "Maximum number of verified factors reached, unenroll to continue") + if err := validateFactors(db, user, params.FriendlyName, a.config, session); err != nil { + return err } - if numVerifiedFactors > 0 && !session.IsAAL2() { - return forbiddenError(ErrorCodeInsufficientAAL, "AAL2 required to enroll a new factor") - } factor := models.NewPhoneFactor(user, phone, params.FriendlyName) err = db.Transaction(func(tx *storage.Connection) error { if terr := tx.Create(factor); terr != nil { @@ -173,31 +185,14 @@ func (a *API) enrollTOTPFactor(w http.ResponseWriter, r *http.Request, params *E issuer = params.Issuer } - factors := user.Factors - - factorCount := len(factors) - numVerifiedFactors := 0 if err := models.DeleteExpiredFactors(db, config.MFA.FactorExpiryDuration); err != nil { return err } - for _, factor := range factors { - if factor.IsVerified() { - numVerifiedFactors += 1 - } - } - - if factorCount >= int(config.MFA.MaxEnrolledFactors) { - return forbiddenError(ErrorCodeTooManyEnrolledMFAFactors, "Maximum number of verified factors reached, unenroll to continue") - } - - if numVerifiedFactors >= config.MFA.MaxVerifiedFactors { - return forbiddenError(ErrorCodeTooManyEnrolledMFAFactors, "Maximum number of verified factors reached, unenroll to continue") + if err := validateFactors(db, user, params.FriendlyName, config, session); err != nil { + return err } - if numVerifiedFactors > 0 && !session.IsAAL2() { - return forbiddenError(ErrorCodeInsufficientAAL, "AAL2 required to enroll a new factor") - } var factor *models.Factor var buf bytes.Buffer var key *otp.Key @@ -225,13 +220,9 @@ func (a *API) enrollTOTPFactor(w http.ResponseWriter, r *http.Request, params *E err = db.Transaction(func(tx *storage.Connection) error { if terr := tx.Create(factor); terr != nil { - pgErr := utilities.NewPostgresError(terr) - if pgErr.IsUniqueConstraintViolated() { - return unprocessableEntityError(ErrorCodeMFAFactorNameConflict, fmt.Sprintf("A factor with the friendly name %q for this user likely already exists", factor.FriendlyName)) - } return terr - } + if terr := models.NewAuditLogEntry(r, tx, user, models.EnrollFactorAction, r.RemoteAddr, map[string]interface{}{ "factor_id": factor.ID, }); terr != nil { diff --git a/internal/api/mfa_test.go b/internal/api/mfa_test.go index 557b0ab13..87767f085 100644 --- a/internal/api/mfa_test.go +++ b/internal/api/mfa_test.go @@ -290,9 +290,7 @@ func (ts *MFATestSuite) TestDuplicateTOTPEnrollsReturnExpectedMessage() { err := json.NewDecoder(response.Body).Decode(&errorResponse) require.NoError(ts.T(), err) - // Convert the response body to a string and check for the expected error message - expectedErrorMessage := fmt.Sprintf("A factor with the friendly name %q for this user likely already exists", friendlyName) - require.Contains(ts.T(), errorResponse.Message, expectedErrorMessage) + require.Contains(ts.T(), errorResponse.ErrorCode, ErrorCodeMFAFactorNameConflict) } func (ts *MFATestSuite) AAL2RequiredToUpdatePasswordAfterEnrollment() { @@ -369,7 +367,7 @@ func (ts *MFATestSuite) TestMultipleEnrollsCleanupExpiredFactors() { var w *httptest.ResponseRecorder token := accessTokenResp.Token for i := 0; i < numFactors; i++ { - w = performEnrollFlow(ts, token, "", models.TOTP, "https://issuer.com", "", http.StatusOK) + w = performEnrollFlow(ts, token, "first-name", models.TOTP, "https://issuer.com", "", http.StatusOK) } enrollResp := EnrollFactorResponse{} @@ -379,7 +377,7 @@ func (ts *MFATestSuite) TestMultipleEnrollsCleanupExpiredFactors() { _ = performChallengeFlow(ts, enrollResp.ID, token) // Enroll another Factor (Factor 3) - _ = performEnrollFlow(ts, token, "", 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 index 24cda188f..730965330 100644 --- a/internal/models/factor.go +++ b/internal/models/factor.go @@ -269,10 +269,6 @@ func (f *Factor) DowngradeSessionsToAAL1(tx *storage.Connection) error { return updateFactorAssociatedSessions(tx, f.UserID, f.ID, AAL1.String()) } -func (f *Factor) IsOwnedBy(user *User) bool { - return f.UserID == user.ID -} - func (f *Factor) IsVerified() bool { return f.Status == FactorStateVerified.String() } From 8a65eb8e4abdc4599d6ec8006f4d87e74a02e05d Mon Sep 17 00:00:00 2001 From: joel Date: Mon, 30 Sep 2024 22:13:15 +0200 Subject: [PATCH 2/2] fix: move expiry check into validate factors --- internal/api/mfa.go | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/internal/api/mfa.go b/internal/api/mfa.go index 7f1eef3bc..d6158e964 100644 --- a/internal/api/mfa.go +++ b/internal/api/mfa.go @@ -71,6 +71,9 @@ const ( ) func validateFactors(db *storage.Connection, user *models.User, newFactorName string, config *conf.GlobalConfiguration, session *models.Session) error { + if err := models.DeleteExpiredFactors(db, config.MFA.FactorExpiryDuration); err != nil { + return err + } if err := db.Load(user, "Factors"); err != nil { return err } @@ -106,7 +109,6 @@ func validateFactors(db *storage.Connection, user *models.User, newFactorName st func (a *API) enrollPhoneFactor(w http.ResponseWriter, r *http.Request, params *EnrollFactorParams) error { ctx := r.Context() - config := a.config user := getUser(ctx) session := getSession(ctx) db := a.db.WithContext(ctx) @@ -118,9 +120,6 @@ func (a *API) enrollPhoneFactor(w http.ResponseWriter, r *http.Request, params * if err != nil { return badRequestError(ErrorCodeValidationFailed, "Invalid phone number format (E.164 required)") } - if err := models.DeleteExpiredFactors(db, config.MFA.FactorExpiryDuration); err != nil { - return err - } var factorsToDelete []models.Factor for _, factor := range user.Factors { @@ -185,10 +184,6 @@ func (a *API) enrollTOTPFactor(w http.ResponseWriter, r *http.Request, params *E issuer = params.Issuer } - if err := models.DeleteExpiredFactors(db, config.MFA.FactorExpiryDuration); err != nil { - return err - } - if err := validateFactors(db, user, params.FriendlyName, config, session); err != nil { return err }