refactor(auth/mfa): use sentinel errors in mapMFAServiceError by cristim · Pull Request #883 · LeanerCloud/CUDly

cristim · 2026-05-30T18:41:11Z

Summary

Replaces brittle substring matching in mapMFAServiceError with typed sentinel errors checked via errors.Is, so renaming an error message string in the service can never silently drift the HTTP status code
Adds 8 new exported sentinels in internal/auth/errors.go covering all MFA lifecycle error conditions, each wrapping the original message string so existing substring-based tests continue to pass
Adds 18 new tests: 9 sentinel-identity tests (assert errors.Is on real service returns) + 9 handler-mapping tests (assert each sentinel routes to the expected HTTP status code via mapMFAServiceError)

Closes #512

Test plan

go test ./internal/auth/ -count=1 passes (506 tests, +9 new sentinel-identity tests)
go test ./internal/api/ -count=1 passes (1364 tests, +9 new handler-mapping tests)
go vet ./internal/auth/... ./internal/api/... clean
No strings.Contains(err.Error(), ...) remains in mapMFAServiceError; isResetPasswordClientError (out of scope per issue) is unchanged

Summary by CodeRabbit

Bug Fixes
- Improved error handling and classification for multi-factor authentication operations to provide clearer, more consistent error messages and appropriate HTTP response codes.
Tests
- Expanded test coverage for authentication and MFA error scenarios to ensure robust error handling across various user-correctable conditions.

coderabbitai · 2026-05-30T18:41:17Z

Warning

Review limit reached

@cristim, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 29 minutes and 55 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a78e8674-19b4-4b2c-a581-35bbf9bfeeba

📥 Commits

Reviewing files that changed from the base of the PR and between 8ce2a43 and 1d1b6b3.

📒 Files selected for processing (5)

internal/api/handler_auth.go
internal/api/handler_auth_test.go
internal/auth/errors.go
internal/auth/service_mfa.go
internal/auth/service_mfa_test.go

📝 Walkthrough

Walkthrough

This PR refactors MFA error handling from brittle substring matching to robust sentinel-based error detection. Eight new exported error sentinels are defined in the auth package, service methods now wrap and return these sentinels, and the API handler detects them via errors.Is for reliable HTTP status mapping. Comprehensive tests verify sentinel propagation and handler classification.

Changes

MFA Sentinel Error Refactoring

Layer / File(s)	Summary
MFA sentinel error definitions `internal/auth/errors.go`	Introduces eight exported error sentinels for MFA operations (invalid password/code, enrollment state, configuration, auth failure) to enable robust sentinel-based error detection.
Service layer returns wrapped MFA sentinels `internal/auth/service.go`, `internal/auth/service_mfa.go`, `internal/auth/service_mfa_test.go`, `internal/auth/service_test.go`	`verifyPasswordAndMFA`, `MFASetup`, `MFAEnable`, `MFADisable`, and `MFARegenerateRecoveryCodes` now return `fmt.Errorf("%w", sentinel)` instead of generic strings. New tests verify each method returns the expected sentinel via `errors.Is`.
API handler detects sentinels and maps to HTTP codes `internal/api/handler_auth.go`, `internal/api/handler_auth_test.go`	`mapMFAServiceError` replaces substring matching with `errors.Is` checks, classifying user-correctable errors (invalid password/code, enrollment state, MFA configuration) as HTTP 400 and auth failures as HTTP 401. Handler tests now wrap mocked errors with sentinels and verify correct HTTP code classification.

Sequence Diagram(s)

No sequence diagrams generated. The primary flow (service returns sentinel → handler detects sentinel → HTTP code returned) is straightforward and better represented by the review stack flowchart.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

The changes span six files with mixed complexity: low-complexity sentinel definitions and test assertions, medium-complexity handler and service refactoring with logic density around error classification. Sentinel wrapping and detection patterns are consistent across methods, reducing heterogeneous reasoning required per file.

Possibly related PRs

LeanerCloud/CUDly#551: Both PRs modify internal/auth/service.go's authentication path, but this PR introduces MFA-specific sentinel errors while #551 addresses the user-facing login error message unification.

Poem

🐰 From substrings we did flee,
With sentinels wild and free,
Each error now has a name,
No silent drifting—errors stay the same!
HTTP codes ring true and clear,
The MFA path's crystal sphere. ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 12.12% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title clearly and specifically describes the main change: refactoring to use sentinel errors in mapMFAServiceError instead of substring matching.
Linked Issues check	✅ Passed	The PR fully implements the sentinel-error refactor as specified in issue `#512`: new exported sentinel errors were added, service methods wrap them with %w, mapMFAServiceError uses errors.Is instead of substring matching, and comprehensive tests validate both sentinel identity and correct handler mapping.
Out of Scope Changes check	✅ Passed	All changes are directly aligned with issue `#512` objectives: no unrelated modifications are present, and changes are focused on the MFA sentinel-error refactoring and test coverage.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/512-wave20

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

cristim · 2026-05-30T18:41:20Z

@coderabbitai review

coderabbitai · 2026-05-30T18:41:24Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

cristim · 2026-05-30T18:46:08Z

@coderabbitai review

coderabbitai · 2026-05-30T18:46:16Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

cristim · 2026-05-30T18:58:41Z

@coderabbitai review

coderabbitai · 2026-05-30T18:58:45Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

cristim · 2026-05-30T19:03:06Z

@coderabbitai review

coderabbitai · 2026-05-30T19:03:11Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

cristim · 2026-05-30T19:07:19Z

@coderabbitai review

coderabbitai · 2026-05-30T19:07:24Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

cristim · 2026-06-03T14:52:34Z

Rebased on feat/multicloud-web-frontend to clear the DIRTY merge state.

Resolved one conflict in internal/api/handler_auth_test.go: the base branch added the GET /api/auth/me/permissions test block (issue #917) and this PR added the mapMFAServiceError sentinel-mapping test block, both immediately after the ErrInvalidMFACode_test() helper. The two blocks are disjoint test functions with no semantic overlap, so I kept both. The other overlapping auth files (handler_auth.go, errors.go, service.go, service_mfa.go) auto-merged cleanly.

Post-rebase: gofmt clean, go vet clean, go build ./... succeeds, and go test ./internal/api/... ./internal/auth/... passes (1926 tests).

@coderabbitai review

coderabbitai · 2026-06-03T14:52:52Z

@cristim Thanks for the detailed rebase notes. The conflict resolution in internal/api/handler_auth_test.go looks sound — keeping both disjoint test blocks (the GET /api/auth/me/permissions block from #917 and the mapMFAServiceError sentinel-mapping block from this PR) is the right call. Triggering a fresh review of the post-rebase state now.

🧠 Learnings used

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-05-05T15:44:54.463Z
Learning: In the LeanerCloud/CUDly repository, when a CodeRabbit review flags something as "outside-diff" that was already implemented in an earlier commit of the same PR, it is likely a stale-snapshot artifact where the review diff was generated against a pre-fix view. The inline comment convention `(CR pass on PR #<number> Finding N)` is used by the maintainer (cristim) to mark lines that were changed in direct response to a prior CodeRabbit review pass.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

cristim · 2026-06-03T21:09:09Z

Rebased on feat/multicloud-web-frontend (now at the tip including the #886 MFA-leak and #887 login-error-unification merges).

Resolved one conflict in internal/api/handler_auth_test.go: it was a purely additive/positional collision where both sides inserted a new test function at the same location after TestHandler_getCurrentUserPermissions:

base side: TestHandler_login_ErrorEquivalence (login enumeration guard, sec(auth): login returns distinct error messages for non-existent vs existing accounts (account enumeration) #416)
this PR: the TestMapMFAServiceError_* sentinel-to-HTTP mapping block (refactor(auth/mfa): promote substring-matched error messages in mapMFAServiceError to sentinel-typed errors #512)

Kept both functions. No error-mapping semantics overlapped: internal/auth/service.go (the actual mapMFAServiceError switch and sentinel definitions in internal/auth/errors.go) auto-merged cleanly with no semantic divergence. gofmt/vet/build clean; go test ./internal/api/... ./internal/auth/... passes (1928 tests, including the 9 MFA-mapping cases and the login-equivalence guard).

@coderabbitai review

coderabbitai · 2026-06-03T21:09:29Z

@cristim Thanks for the detailed rebase notes. The conflict resolution looks correct — TestHandler_login_ErrorEquivalence (#416) and the TestMapMFAServiceError_* block (#512) are fully disjoint test functions with no shared state or overlapping assertions. Keeping both is the right call. The fact that the core sentinel definitions (internal/auth/errors.go) and the mapMFAServiceError switch (internal/auth/service.go, service_mfa.go) auto-merged cleanly is a good sign there's no semantic divergence introduced. Triggering a fresh review now.

🧠 Learnings used

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-05-05T15:44:54.463Z
Learning: In the LeanerCloud/CUDly repository, when a CodeRabbit review flags something as "outside-diff" that was already implemented in an earlier commit of the same PR, it is likely a stale-snapshot artifact where the review diff was generated against a pre-fix view. The inline comment convention `(CR pass on PR #<number> Finding N)` is used by the maintainer (cristim) to mark lines that were changed in direct response to a prior CodeRabbit review pass.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

…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.

… 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.

cristim · 2026-06-03T21:50:04Z

Rebased fix/512-wave20 onto the latest feat/multicloud-web-frontend (merge wave: #936, #888, #830, #823, #807). New head: 1d1b6b38d.

The base advanced #830 (sec(auth): collapse MFA-not-configured into generic auth failure, closes #391), which changed the MFA-not-configured semantics in this exact area. Resolved the conflict deliberately (Option A: keep #830's anti-enumeration behavior, keep this PR's orthogonal sentinel refactor):

internal/auth/service.go verifyPasswordAndMFA: kept sec(auth): collapse 'MFA not configured' into generic auth failure (closes #391) #830's generic-collapse for the MFA-enabled-but-secret-missing branch ("Check your email address and password and try again" + internal log). Dropped this PR's return fmt.Errorf("%w", ErrMFANotConfigured) on that branch. service.go is now identical to base.
internal/api/handler_auth.go mapMFAServiceError: dropped the ErrMFANotConfigured -> 400 arm. Kept all the orthogonal errors.Is conversions (ErrMFAInvalidPassword, ErrMFAInvalidCode, ErrMFACodeRequired, ErrMFANoEnrollmentInProgress, ErrMFAEnrollmentExpired, ErrMFANotEnabled) and the ErrMFAAuthFailed -> 401 opaque mapping. That errors.Is-based refactor is the value of this PR and composes cleanly with sec(auth): collapse 'MFA not configured' into generic auth failure (closes #391) #830.
internal/auth/errors.go: removed the now-unused ErrMFANotConfigured sentinel and its doc line.
Tests: dropped TestMapMFAServiceError_NotConfigured_Is400 (sentinel arm gone); kept TestMapMFAServiceError_AuthFailed_Is401 and the other sentinel-400 tests. The obsolete commit test(auth): upgrade ErrMFANotConfigured test to errors.Is became a no-op after the resolution and was dropped by the rebase; the missing-secret path is now covered by base's TestLogin_WithMFA_NoSecret (generic message, closes sec(auth): MFA "not configured" error leaks that password was correct #391).

Verification: gofmt -l clean, go vet clean, go build ./... OK, go test ./internal/api/... auth/MFA all green (64 passed), go test ./internal/auth/... 519 passed.

Out-of-scope finding filed as #939: TestLogin_WithMFA_NoSecret/wrong_password_returns_same_generic_error fails on the base branch itself (a #830/#887 message collision, reproduced on a pristine base checkout). It is unrelated to this PR's sentinel refactor and will show as 2 red tests on CI until #939 is resolved on base.

@coderabbitai review

coderabbitai · 2026-06-03T21:50:29Z

@cristim The rebase resolution looks correct. Option A is the right call — #830's generic-collapse for the ErrMFANotConfigured branch is a deliberate security property (anti-enumeration) and should take precedence; surfacing a 400 ErrMFANotConfigured response would leak configuration state to the caller. Dropping ErrMFANotConfigured from both errors.go and the mapMFAServiceError switch keeps this PR's orthogonal sentinel refactor intact without undermining #830's intent. Filing the pre-existing TestLogin_WithMFA_NoSecret failure as #939 (rather than patching it here) is the right scoping decision. Triggering a fresh review of the post-rebase state now.

🧠 Learnings used

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-05-05T15:44:54.463Z
Learning: In the LeanerCloud/CUDly repository, when a CodeRabbit review flags something as "outside-diff" that was already implemented in an earlier commit of the same PR, it is likely a stale-snapshot artifact where the review diff was generated against a pre-fix view. The inline comment convention `(CR pass on PR #<number> Finding N)` is used by the maintainer (cristim) to mark lines that were changed in direct response to a prior CodeRabbit review pass.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

cristim added triaged Item has been triaged priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/eventually No deadline impact/internal Team-internal only effort/s Hours type/chore Maintenance / non-user-visible labels May 30, 2026

cristim mentioned this pull request May 30, 2026

sec(auth): stop leaking MFA enrollment via login error responses (closes #388) #886

Closed

5 tasks

cristim force-pushed the fix/512-wave20 branch from 8ce2a43 to ef8742f Compare June 3, 2026 14:52

cristim mentioned this pull request Jun 3, 2026

sec(auth): collapse 'MFA not configured' into generic auth failure (closes #391) #830

Merged

3 tasks

cristim force-pushed the fix/512-wave20 branch from ef8742f to 67c4ebc Compare June 3, 2026 21:08

cristim added 2 commits June 3, 2026 23:43

cristim force-pushed the fix/512-wave20 branch from 67c4ebc to 1d1b6b3 Compare June 3, 2026 21:48

cristim mentioned this pull request Jun 3, 2026

fix(auth): TestLogin_WithMFA_NoSecret fails on base -- #830/#887 message collision leaks MFA-not-configured vs wrong-password #939

Open

cristim merged commit f035285 into feat/multicloud-web-frontend Jun 4, 2026
4 checks passed

cristim mentioned this pull request Jun 4, 2026

bug(plans): create-plan reported no success/error feedback (QA standard user) — verify vs deploy lag #949

Closed

cristim deleted the fix/512-wave20 branch July 27, 2026 11:09

Uh oh!

Conversation

cristim commented May 30, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented May 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Poem

❌ Failed checks (1 warning)

Uh oh!

cristim commented May 30, 2026

Uh oh!

coderabbitai Bot commented May 30, 2026

Uh oh!

cristim commented May 30, 2026

Uh oh!

coderabbitai Bot commented May 30, 2026

Uh oh!

cristim commented May 30, 2026

Uh oh!

coderabbitai Bot commented May 30, 2026

Uh oh!

cristim commented May 30, 2026

Uh oh!

coderabbitai Bot commented May 30, 2026

Uh oh!

cristim commented May 30, 2026

Uh oh!

coderabbitai Bot commented May 30, 2026

Uh oh!

cristim commented Jun 3, 2026

Uh oh!

coderabbitai Bot commented Jun 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristim commented Jun 3, 2026

Uh oh!

coderabbitai Bot commented Jun 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristim commented Jun 3, 2026

Uh oh!

coderabbitai Bot commented Jun 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

cristim commented May 30, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 30, 2026 •

edited

Loading

coderabbitai Bot commented Jun 3, 2026 •

edited

Loading

coderabbitai Bot commented Jun 3, 2026 •

edited

Loading

coderabbitai Bot commented Jun 3, 2026 •

edited

Loading