From f270de5b3c2f228a203675d226dcaa841adaccb3 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 30 May 2026 20:40:28 +0200 Subject: [PATCH 1/2] refactor(auth/mfa): use sentinel errors in mapMFAServiceError (closes #512) Replace substring matching in mapMFAServiceError with typed sentinel errors checked via errors.Is. Define 8 new exported sentinels in internal/auth/errors.go (ErrMFAInvalidPassword, ErrMFAInvalidCode, ErrMFACodeRequired, ErrMFANoEnrollmentInProgress, ErrMFAEnrollmentExpired, ErrMFANotEnabled, ErrMFANotConfigured, ErrMFAAuthFailed). Each service method in service_mfa.go and the one site in service.go now wraps the appropriate sentinel via fmt.Errorf("%w", ...) so renaming an error string in the service can never silently drift the HTTP status code. Add 18 new tests: 9 sentinel-identity tests in service_mfa_test.go (assert errors.Is on real service calls) and 9 handler-mapping tests in handler_auth_test.go (assert each sentinel routes to the expected HTTP status via mapMFAServiceError). Message strings are preserved unchanged so existing substring-based tests continue to pass. --- internal/api/handler_auth.go | 31 +++--- internal/api/handler_auth_test.go | 61 +++++++++++- internal/auth/errors.go | 25 +++++ internal/auth/service_mfa.go | 28 +++--- internal/auth/service_mfa_test.go | 157 ++++++++++++++++++++++++++++++ 5 files changed, 272 insertions(+), 30 deletions(-) diff --git a/internal/api/handler_auth.go b/internal/api/handler_auth.go index bf924201a..0ce5a4ef7 100644 --- a/internal/api/handler_auth.go +++ b/internal/api/handler_auth.go @@ -503,26 +503,27 @@ var ( ) // mapMFAServiceError maps a service-layer MFA error to the right -// client-facing status + message. Service errors land here as plain -// fmt.Errorf strings (the auth package doesn't export every shape as -// a sentinel because the handler is the only consumer that needs -// status-code-level discrimination). Substring matching is OK because -// the auth package owns these strings and tests pin them. +// client-facing status + message. The auth package exports typed +// sentinel errors for each user-correctable condition; we match via +// errors.Is so a renamed error message in the service never silently +// drifts the HTTP status. See issue #512. func mapMFAServiceError(err error) error { if err == nil { return nil } - msg := err.Error() switch { - case strings.Contains(msg, "invalid password"), - strings.Contains(msg, "invalid MFA code"), - strings.Contains(msg, "MFA code or recovery code required"), - strings.Contains(msg, "no MFA enrollment in progress"), - strings.Contains(msg, "MFA enrollment expired"), - strings.Contains(msg, "MFA is not enabled"): - return NewClientError(400, msg) - case strings.Contains(msg, "authentication failed"): - return NewClientError(401, msg) + case errors.Is(err, auth.ErrMFAInvalidPassword), + errors.Is(err, auth.ErrMFAInvalidCode), + errors.Is(err, auth.ErrMFACodeRequired), + errors.Is(err, auth.ErrMFANoEnrollmentInProgress), + errors.Is(err, auth.ErrMFAEnrollmentExpired), + errors.Is(err, auth.ErrMFANotEnabled): + return NewClientError(400, err.Error()) + case errors.Is(err, auth.ErrMFAAuthFailed): + // Opaque 401 to prevent user enumeration: the service returns this + // for both "user not found" and "DB lookup failed" paths so callers + // cannot distinguish the two. + return NewClientError(401, err.Error()) } return err } diff --git a/internal/api/handler_auth_test.go b/internal/api/handler_auth_test.go index 1ebea1c26..7501dbad8 100644 --- a/internal/api/handler_auth_test.go +++ b/internal/api/handler_auth_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/base64" "errors" + "fmt" "testing" "github.com/LeanerCloud/CUDly/internal/auth" @@ -1178,8 +1179,11 @@ func TestHandler_mfaSetup_WrongPassword(t *testing.T) { mockAuth := new(MockAuthService) session := &Session{UserID: "user-1", Email: "u@x.com"} mockAuth.On("ValidateSession", ctx, "tok").Return(session, nil) + // The real service returns a wrapped sentinel; the handler maps it to a + // 400 ClientError via errors.Is (issue #512). The mock must return the + // same sentinel so the errors.Is check in mapMFAServiceError fires. mockAuth.On("MFASetupAPI", ctx, "user-1", "wrong"). - Return("", "", errors.New("invalid password")) + Return("", "", fmt.Errorf("%w", auth.ErrMFAInvalidPassword)) handler := &Handler{auth: mockAuth} _, err := handler.mfaSetup(ctx, authedReq("tok", `{"password":"`+b64("wrong")+`"}`)) @@ -1483,3 +1487,58 @@ func TestHandler_login_ErrorEquivalence(t *testing.T) { assert.Equal(t, ceNotFound.Error(), ceWrongPass.Error(), "error body must be identical for unknown-user and wrong-password paths to prevent enumeration") } + +// --------------------------------------------------------------- +// mapMFAServiceError sentinel-to-HTTP-code mapping tests (issue #512). +// +// Each test verifies that a specific auth sentinel maps to the +// expected HTTP status code in mapMFAServiceError. These tests would +// fail if someone renames a sentinel value in the auth package +// without updating the switch in mapMFAServiceError. +// --------------------------------------------------------------- + +func TestMapMFAServiceError_Nil(t *testing.T) { + assert.Nil(t, mapMFAServiceError(nil)) +} + +func TestMapMFAServiceError_NonSentinelPassesThrough(t *testing.T) { + plain := errors.New("database connection lost") + got := mapMFAServiceError(plain) + _, isClient := IsClientError(got) + assert.False(t, isClient, "non-sentinel errors must not be wrapped as ClientError") + assert.Equal(t, plain, got) +} + +func testMFASentinel400(t *testing.T, sentinel error, name string) { + t.Helper() + wrapped := fmt.Errorf("some context: %w", sentinel) + got := mapMFAServiceError(wrapped) + ce, ok := IsClientError(got) + require.True(t, ok, "%s must map to a ClientError, got %T: %v", name, got, got) + assert.Equal(t, 400, ce.code, "%s must map to HTTP 400", name) + assert.Contains(t, ce.Error(), sentinel.Error()) +} + +func TestMapMFAServiceError_InvalidPassword_Is400(t *testing.T) { + testMFASentinel400(t, auth.ErrMFAInvalidPassword, "ErrMFAInvalidPassword") +} + +func TestMapMFAServiceError_InvalidCode_Is400(t *testing.T) { + testMFASentinel400(t, auth.ErrMFAInvalidCode, "ErrMFAInvalidCode") +} + +func TestMapMFAServiceError_CodeRequired_Is400(t *testing.T) { + testMFASentinel400(t, auth.ErrMFACodeRequired, "ErrMFACodeRequired") +} + +func TestMapMFAServiceError_NoEnrollmentInProgress_Is400(t *testing.T) { + testMFASentinel400(t, auth.ErrMFANoEnrollmentInProgress, "ErrMFANoEnrollmentInProgress") +} + +func TestMapMFAServiceError_EnrollmentExpired_Is400(t *testing.T) { + testMFASentinel400(t, auth.ErrMFAEnrollmentExpired, "ErrMFAEnrollmentExpired") +} + +func TestMapMFAServiceError_NotEnabled_Is400(t *testing.T) { + testMFASentinel400(t, auth.ErrMFANotEnabled, "ErrMFANotEnabled") +} diff --git a/internal/auth/errors.go b/internal/auth/errors.go index 3a70fea4b..ad896571b 100644 --- a/internal/auth/errors.go +++ b/internal/auth/errors.go @@ -46,4 +46,29 @@ var ( // without substring-matching the human message. See issue #497. ErrMFARequired = errors.New("mfa_required") ErrInvalidMFACode = errors.New("invalid_mfa_code") + + // MFA service-operation sentinels — returned (wrapped via fmt.Errorf + // "%w") by the MFA lifecycle methods in service_mfa.go so the API + // handler can map each error class to the right HTTP status code via + // errors.Is rather than brittle substring matching. See issue #512. + // + // ErrMFAInvalidPassword — wrong current password on setup/disable. + // ErrMFAInvalidCode — wrong TOTP or recovery code. + // ErrMFACodeRequired — MFA-enabled user supplied no code on disable. + // ErrMFANoEnrollmentInProgress — MFAEnable called before MFASetup. + // ErrMFAEnrollmentExpired — pending enrollment window elapsed. + // ErrMFANotEnabled — regenerate/disable called when MFA is off. + // ErrMFAAuthFailed — generic opaque auth failure (user not found or + // DB error; maps to 401 to prevent user enumeration). + // + // Message strings are intentionally identical to the pre-sentinel + // fmt.Errorf literals so that existing tests relying on err.Error() + // substrings continue to pass unchanged. See issue #512. + ErrMFAInvalidPassword = errors.New("invalid password") + ErrMFAInvalidCode = errors.New("invalid MFA code") + ErrMFACodeRequired = errors.New("MFA code or recovery code required") + ErrMFANoEnrollmentInProgress = errors.New("no MFA enrollment in progress") + ErrMFAEnrollmentExpired = errors.New("MFA enrollment expired") + ErrMFANotEnabled = errors.New("MFA is not enabled") + ErrMFAAuthFailed = errors.New("authentication failed") ) diff --git a/internal/auth/service_mfa.go b/internal/auth/service_mfa.go index 8505d8e37..2a42d57d3 100644 --- a/internal/auth/service_mfa.go +++ b/internal/auth/service_mfa.go @@ -262,13 +262,13 @@ func (s *Service) MFASetup(ctx context.Context, userID, password string) (*MFASe } user, err := s.store.GetUserByID(ctx, userID) if err != nil { - return nil, fmt.Errorf("authentication failed") + return nil, fmt.Errorf("%w", ErrMFAAuthFailed) } if user == nil { - return nil, fmt.Errorf("authentication failed") + return nil, fmt.Errorf("%w", ErrMFAAuthFailed) } if !s.verifyPassword(password, user.PasswordHash) { - return nil, fmt.Errorf("invalid password") + return nil, fmt.Errorf("%w", ErrMFAInvalidPassword) } secret, err := generateMFASecret() @@ -316,16 +316,16 @@ func (s *Service) generateAndHashRecoveryCodes() (plaintext, hashes []string, er // "no enrollment in progress" rather than "expired" forever. func (s *Service) validatePendingMFAEnrollment(ctx context.Context, user *User, code string) error { if user.MFAPendingSecret == "" || user.MFAPendingSecretExpiresAt == nil { - return fmt.Errorf("no MFA enrollment in progress") + return fmt.Errorf("%w", ErrMFANoEnrollmentInProgress) } if time.Now().After(*user.MFAPendingSecretExpiresAt) { user.MFAPendingSecret = "" user.MFAPendingSecretExpiresAt = nil _ = s.store.UpdateUser(ctx, user) - return fmt.Errorf("MFA enrollment expired") + return fmt.Errorf("%w", ErrMFAEnrollmentExpired) } if !verifyTOTP(user.MFAPendingSecret, code) { - return fmt.Errorf("invalid MFA code") + return fmt.Errorf("%w", ErrMFAInvalidCode) } return nil } @@ -350,7 +350,7 @@ func (s *Service) MFAEnable(ctx context.Context, userID, code string) ([]string, } user, err := s.store.GetUserByID(ctx, userID) if err != nil || user == nil { - return nil, fmt.Errorf("authentication failed") + return nil, fmt.Errorf("%w", ErrMFAAuthFailed) } if err := s.validatePendingMFAEnrollment(ctx, user, code); err != nil { return nil, err @@ -410,16 +410,16 @@ func (s *Service) MFADisable(ctx context.Context, userID, password, codeOrRecove } user, err := s.store.GetUserByID(ctx, userID) if err != nil || user == nil { - return fmt.Errorf("authentication failed") + return fmt.Errorf("%w", ErrMFAAuthFailed) } if !s.verifyPassword(password, user.PasswordHash) { - return fmt.Errorf("invalid password") + return fmt.Errorf("%w", ErrMFAInvalidPassword) } if !user.MFAEnabled { return s.disableMFAAlreadyOff(ctx, user) } if codeOrRecovery == "" { - return fmt.Errorf("MFA code or recovery code required") + return fmt.Errorf("%w", ErrMFACodeRequired) } // Try TOTP first (cheap), then fall back to recovery code (bcrypt @@ -427,7 +427,7 @@ func (s *Service) MFADisable(ctx context.Context, userID, password, codeOrRecove // fresh proof-of-possession. matched := verifyTOTP(user.MFASecret, codeOrRecovery) || s.consumeRecoveryCode(user, codeOrRecovery) if !matched { - return fmt.Errorf("invalid MFA code") + return fmt.Errorf("%w", ErrMFAInvalidCode) } clearMFAFromUser(user) @@ -448,13 +448,13 @@ func (s *Service) MFARegenerateRecoveryCodes(ctx context.Context, userID, code s } user, err := s.store.GetUserByID(ctx, userID) if err != nil || user == nil { - return nil, fmt.Errorf("authentication failed") + return nil, fmt.Errorf("%w", ErrMFAAuthFailed) } if !user.MFAEnabled || user.MFASecret == "" { - return nil, fmt.Errorf("MFA is not enabled") + return nil, fmt.Errorf("%w", ErrMFANotEnabled) } if !verifyTOTP(user.MFASecret, code) { - return nil, fmt.Errorf("invalid MFA code") + return nil, fmt.Errorf("%w", ErrMFAInvalidCode) } plaintext, hashes, err := s.generateAndHashRecoveryCodes() diff --git a/internal/auth/service_mfa_test.go b/internal/auth/service_mfa_test.go index 0033294dd..48d733d6f 100644 --- a/internal/auth/service_mfa_test.go +++ b/internal/auth/service_mfa_test.go @@ -2,6 +2,7 @@ package auth import ( "context" + "errors" "strings" "testing" "time" @@ -463,3 +464,159 @@ func TestLogin_WithMFA_RecoveryCode_ConsumedOnce(t *testing.T) { require.NotNil(t, resp) assert.Empty(t, user.MFARecoveryCodes, "consumed recovery code must be removed from the slice") } + +// --------------------------------------------------------------- +// Sentinel-identity tests (issue #512). +// +// Each test asserts that the real service returns the expected typed +// sentinel so that a future rename of the error message string in +// service_mfa.go causes a compile-time or test failure rather than a +// silent HTTP-status drift. errors.Is is the contract; the message +// text is NOT the contract (but is preserved for human readability). +// --------------------------------------------------------------- + +func TestMFASetup_WrongPassword_ReturnsSentinel(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + service := createTestService(mockStore, new(MockEmailSender)) + + user := createTestUser(t, "SecurePass@123") + mockStore.On("GetUserByID", ctx, user.ID).Return(user, nil) + + _, err := service.MFASetup(ctx, user.ID, "WrongPassword!@#") + require.Error(t, err) + assert.True(t, errors.Is(err, ErrMFAInvalidPassword), + "MFASetup wrong-password must return ErrMFAInvalidPassword, got: %v", err) + mockStore.AssertExpectations(t) +} + +func TestMFAEnable_NoPending_ReturnsSentinel(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + service := createTestService(mockStore, new(MockEmailSender)) + + user := createTestUser(t, "SecurePass@123") + mockStore.On("GetUserByID", ctx, user.ID).Return(user, nil) + + _, err := service.MFAEnable(ctx, user.ID, "000000") + require.Error(t, err) + assert.True(t, errors.Is(err, ErrMFANoEnrollmentInProgress), + "MFAEnable with no pending enrollment must return ErrMFANoEnrollmentInProgress, got: %v", err) +} + +func TestMFAEnable_ExpiredPending_ReturnsSentinel(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + service := createTestService(mockStore, new(MockEmailSender)) + + secret := "JBSWY3DPEHPK3PXP" + expiresAt := time.Now().Add(-1 * time.Minute) + user := createTestUser(t, "SecurePass@123") + user.MFAPendingSecret = secret + user.MFAPendingSecretExpiresAt = &expiresAt + mockStore.On("GetUserByID", ctx, user.ID).Return(user, nil) + mockStore.On("UpdateUser", ctx, mock.AnythingOfType("*auth.User")).Return(nil).Maybe() + + _, err := service.MFAEnable(ctx, user.ID, totpFor(secret)) + require.Error(t, err) + assert.True(t, errors.Is(err, ErrMFAEnrollmentExpired), + "MFAEnable with expired enrollment must return ErrMFAEnrollmentExpired, got: %v", err) +} + +func TestMFAEnable_WrongCode_ReturnsSentinel(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + service := createTestService(mockStore, new(MockEmailSender)) + + secret := "JBSWY3DPEHPK3PXP" + expiresAt := time.Now().Add(mfaPendingExpiry) + user := createTestUser(t, "SecurePass@123") + user.MFAPendingSecret = secret + user.MFAPendingSecretExpiresAt = &expiresAt + mockStore.On("GetUserByID", ctx, user.ID).Return(user, nil) + + _, err := service.MFAEnable(ctx, user.ID, "000000") + require.Error(t, err) + assert.True(t, errors.Is(err, ErrMFAInvalidCode), + "MFAEnable wrong code must return ErrMFAInvalidCode, got: %v", err) +} + +func TestMFADisable_WrongPassword_ReturnsSentinel(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + service := createTestService(mockStore, new(MockEmailSender)) + + user := createTestUser(t, "SecurePass@123") + user.MFAEnabled = true + user.MFASecret = "JBSWY3DPEHPK3PXP" + mockStore.On("GetUserByID", ctx, user.ID).Return(user, nil) + + err := service.MFADisable(ctx, user.ID, "wrong", "000000") + require.Error(t, err) + assert.True(t, errors.Is(err, ErrMFAInvalidPassword), + "MFADisable wrong password must return ErrMFAInvalidPassword, got: %v", err) +} + +func TestMFADisable_NoCode_ReturnsSentinel(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + service := createTestService(mockStore, new(MockEmailSender)) + + user := createTestUser(t, "SecurePass@123") + user.MFAEnabled = true + user.MFASecret = "JBSWY3DPEHPK3PXP" + mockStore.On("GetUserByID", ctx, user.ID).Return(user, nil) + + err := service.MFADisable(ctx, user.ID, "SecurePass@123", "") + require.Error(t, err) + assert.True(t, errors.Is(err, ErrMFACodeRequired), + "MFADisable empty code must return ErrMFACodeRequired, got: %v", err) +} + +func TestMFADisable_WrongCode_ReturnsSentinel(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + service := createTestService(mockStore, new(MockEmailSender)) + + user := createTestUser(t, "SecurePass@123") + user.MFAEnabled = true + user.MFASecret = "JBSWY3DPEHPK3PXP" + user.MFARecoveryCodes = []string{"$2a$04$hashedstub"} // won't match any real code + mockStore.On("GetUserByID", ctx, user.ID).Return(user, nil) + + err := service.MFADisable(ctx, user.ID, "SecurePass@123", "000000") + require.Error(t, err) + assert.True(t, errors.Is(err, ErrMFAInvalidCode), + "MFADisable wrong code must return ErrMFAInvalidCode, got: %v", err) +} + +func TestMFARegenerateRecoveryCodes_NotEnabled_ReturnsSentinel(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + service := createTestService(mockStore, new(MockEmailSender)) + + user := createTestUser(t, "SecurePass@123") + user.MFAEnabled = false + mockStore.On("GetUserByID", ctx, user.ID).Return(user, nil) + + _, err := service.MFARegenerateRecoveryCodes(ctx, user.ID, "000000") + require.Error(t, err) + assert.True(t, errors.Is(err, ErrMFANotEnabled), + "MFARegenerateRecoveryCodes when disabled must return ErrMFANotEnabled, got: %v", err) +} + +func TestMFARegenerateRecoveryCodes_WrongCode_ReturnsSentinel(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + service := createTestService(mockStore, new(MockEmailSender)) + + user := createTestUser(t, "SecurePass@123") + user.MFAEnabled = true + user.MFASecret = "JBSWY3DPEHPK3PXP" + mockStore.On("GetUserByID", ctx, user.ID).Return(user, nil) + + _, err := service.MFARegenerateRecoveryCodes(ctx, user.ID, "000000") + require.Error(t, err) + assert.True(t, errors.Is(err, ErrMFAInvalidCode), + "MFARegenerateRecoveryCodes wrong TOTP must return ErrMFAInvalidCode, got: %v", err) +} From 1d1b6b38d845d2ea9d50b55003cfd064efc10b85 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 30 May 2026 20:45:51 +0200 Subject: [PATCH 2/2] test(auth/mfa): add missing AssertExpectations + ErrMFAAuthFailed 401 test - Add mockStore.AssertExpectations(t) to 8 sentinel-identity tests in service_mfa_test.go that called On() without asserting all expectations were met (violates feedback_mock_assert_expectations pattern). - Add mockAuth.AssertExpectations(t) to 8 new handler MFA tests in handler_auth_test.go for the same reason. - Add TestMapMFAServiceError_AuthFailed_Is401 to cover the ErrMFAAuthFailed -> 401 branch in mapMFAServiceError, which had no corresponding test case. --- internal/api/handler_auth_test.go | 19 +++++++++++++++++++ internal/auth/service_mfa_test.go | 8 ++++++++ 2 files changed, 27 insertions(+) diff --git a/internal/api/handler_auth_test.go b/internal/api/handler_auth_test.go index 7501dbad8..70d170b31 100644 --- a/internal/api/handler_auth_test.go +++ b/internal/api/handler_auth_test.go @@ -1140,6 +1140,7 @@ func TestHandler_login_MFARequired_ReturnsMFASentinel(t *testing.T) { require.True(t, ok) assert.Equal(t, 401, ce.code) assert.Equal(t, "mfa_required", ce.Error()) + mockAuth.AssertExpectations(t) } func TestHandler_login_InvalidMFACode_ReturnsCodedSentinel(t *testing.T) { @@ -1157,6 +1158,7 @@ func TestHandler_login_InvalidMFACode_ReturnsCodedSentinel(t *testing.T) { require.True(t, ok) assert.Equal(t, 401, ce.code) assert.Equal(t, "invalid_mfa_code", ce.Error()) + mockAuth.AssertExpectations(t) } func TestHandler_mfaSetup_HappyPath(t *testing.T) { @@ -1172,6 +1174,7 @@ func TestHandler_mfaSetup_HappyPath(t *testing.T) { require.NoError(t, err) assert.Equal(t, "SECRET123", resp.Secret) assert.Contains(t, resp.ProvisioningURI, "otpauth://") + mockAuth.AssertExpectations(t) } func TestHandler_mfaSetup_WrongPassword(t *testing.T) { @@ -1192,6 +1195,7 @@ func TestHandler_mfaSetup_WrongPassword(t *testing.T) { require.True(t, ok) assert.Equal(t, 400, ce.code) assert.Contains(t, ce.Error(), "invalid password") + mockAuth.AssertExpectations(t) } func TestHandler_mfaEnable_HappyPath(t *testing.T) { @@ -1206,6 +1210,7 @@ func TestHandler_mfaEnable_HappyPath(t *testing.T) { resp, err := handler.mfaEnable(ctx, authedReq("tok", `{"code":"123456"}`)) require.NoError(t, err) assert.Len(t, resp.RecoveryCodes, 2) + mockAuth.AssertExpectations(t) } func TestHandler_mfaEnable_NoSession(t *testing.T) { @@ -1219,6 +1224,7 @@ func TestHandler_mfaEnable_NoSession(t *testing.T) { ce, ok := IsClientError(err) require.True(t, ok) assert.Equal(t, 401, ce.code) + mockAuth.AssertExpectations(t) } func TestHandler_mfaDisable_HappyPath(t *testing.T) { @@ -1231,6 +1237,7 @@ func TestHandler_mfaDisable_HappyPath(t *testing.T) { handler := &Handler{auth: mockAuth} _, err := handler.mfaDisable(ctx, authedReq("tok", `{"password":"`+b64("pw")+`","code":"123456"}`)) require.NoError(t, err) + mockAuth.AssertExpectations(t) } func TestHandler_mfaRegenerateRecoveryCodes_HappyPath(t *testing.T) { @@ -1245,6 +1252,7 @@ func TestHandler_mfaRegenerateRecoveryCodes_HappyPath(t *testing.T) { resp, err := handler.mfaRegenerateRecoveryCodes(ctx, authedReq("tok", `{"code":"123456"}`)) require.NoError(t, err) assert.Len(t, resp.RecoveryCodes, 1) + mockAuth.AssertExpectations(t) } // ErrMFARequired_test / ErrInvalidMFACode_test return the sentinel @@ -1542,3 +1550,14 @@ func TestMapMFAServiceError_EnrollmentExpired_Is400(t *testing.T) { func TestMapMFAServiceError_NotEnabled_Is400(t *testing.T) { testMFASentinel400(t, auth.ErrMFANotEnabled, "ErrMFANotEnabled") } + +func TestMapMFAServiceError_AuthFailed_Is401(t *testing.T) { + // ErrMFAAuthFailed must map to 401 (not 400) to prevent user enumeration: + // both "user not found" and "DB error" paths surface as opaque 401. + wrapped := fmt.Errorf("some context: %w", auth.ErrMFAAuthFailed) + got := mapMFAServiceError(wrapped) + ce, ok := IsClientError(got) + require.True(t, ok, "ErrMFAAuthFailed must map to a ClientError") + assert.Equal(t, 401, ce.code, "ErrMFAAuthFailed must map to HTTP 401") + assert.Contains(t, ce.Error(), auth.ErrMFAAuthFailed.Error()) +} diff --git a/internal/auth/service_mfa_test.go b/internal/auth/service_mfa_test.go index 48d733d6f..24f1778ce 100644 --- a/internal/auth/service_mfa_test.go +++ b/internal/auth/service_mfa_test.go @@ -502,6 +502,7 @@ func TestMFAEnable_NoPending_ReturnsSentinel(t *testing.T) { require.Error(t, err) assert.True(t, errors.Is(err, ErrMFANoEnrollmentInProgress), "MFAEnable with no pending enrollment must return ErrMFANoEnrollmentInProgress, got: %v", err) + mockStore.AssertExpectations(t) } func TestMFAEnable_ExpiredPending_ReturnsSentinel(t *testing.T) { @@ -521,6 +522,7 @@ func TestMFAEnable_ExpiredPending_ReturnsSentinel(t *testing.T) { require.Error(t, err) assert.True(t, errors.Is(err, ErrMFAEnrollmentExpired), "MFAEnable with expired enrollment must return ErrMFAEnrollmentExpired, got: %v", err) + mockStore.AssertExpectations(t) } func TestMFAEnable_WrongCode_ReturnsSentinel(t *testing.T) { @@ -539,6 +541,7 @@ func TestMFAEnable_WrongCode_ReturnsSentinel(t *testing.T) { require.Error(t, err) assert.True(t, errors.Is(err, ErrMFAInvalidCode), "MFAEnable wrong code must return ErrMFAInvalidCode, got: %v", err) + mockStore.AssertExpectations(t) } func TestMFADisable_WrongPassword_ReturnsSentinel(t *testing.T) { @@ -555,6 +558,7 @@ func TestMFADisable_WrongPassword_ReturnsSentinel(t *testing.T) { require.Error(t, err) assert.True(t, errors.Is(err, ErrMFAInvalidPassword), "MFADisable wrong password must return ErrMFAInvalidPassword, got: %v", err) + mockStore.AssertExpectations(t) } func TestMFADisable_NoCode_ReturnsSentinel(t *testing.T) { @@ -571,6 +575,7 @@ func TestMFADisable_NoCode_ReturnsSentinel(t *testing.T) { require.Error(t, err) assert.True(t, errors.Is(err, ErrMFACodeRequired), "MFADisable empty code must return ErrMFACodeRequired, got: %v", err) + mockStore.AssertExpectations(t) } func TestMFADisable_WrongCode_ReturnsSentinel(t *testing.T) { @@ -588,6 +593,7 @@ func TestMFADisable_WrongCode_ReturnsSentinel(t *testing.T) { require.Error(t, err) assert.True(t, errors.Is(err, ErrMFAInvalidCode), "MFADisable wrong code must return ErrMFAInvalidCode, got: %v", err) + mockStore.AssertExpectations(t) } func TestMFARegenerateRecoveryCodes_NotEnabled_ReturnsSentinel(t *testing.T) { @@ -603,6 +609,7 @@ func TestMFARegenerateRecoveryCodes_NotEnabled_ReturnsSentinel(t *testing.T) { require.Error(t, err) assert.True(t, errors.Is(err, ErrMFANotEnabled), "MFARegenerateRecoveryCodes when disabled must return ErrMFANotEnabled, got: %v", err) + mockStore.AssertExpectations(t) } func TestMFARegenerateRecoveryCodes_WrongCode_ReturnsSentinel(t *testing.T) { @@ -619,4 +626,5 @@ func TestMFARegenerateRecoveryCodes_WrongCode_ReturnsSentinel(t *testing.T) { require.Error(t, err) assert.True(t, errors.Is(err, ErrMFAInvalidCode), "MFARegenerateRecoveryCodes wrong TOTP must return ErrMFAInvalidCode, got: %v", err) + mockStore.AssertExpectations(t) }