[cristim]

What

A live session plus the password could replace an enabled authenticator. MFASetup never checked MFAEnabled, and MFAEnable overwrote the active secret and every recovery code. Disable, by contrast, demands a current TOTP or recovery code.

• MFASetup and MFAEnable return ErrMFAAlreadyEnabled (HTTP 409) while MFA is on. Setup checks the password first, so a wrong password still gets the existing 400.
• The setup, enable and password-only cancel writes (disableMFAAlreadyOff) are now column-scoped compare-and-swap writes in store_postgres_credentials.go, following the UpdateUserCredentials / ErrUserChanged pattern:
  • SetPendingMFASecret writes only while mfa_enabled = false.
  • EnableMFA writes only while mfa_enabled = false AND mfa_pending_secret = <the secret whose TOTP was verified>.
  • ClearPendingMFASecret writes only while mfa_enabled = false.
    A lost race returns ErrUserChanged (409, mapped in mapMFAServiceError). Storage errors still surface as 500s.
• When an enrollment has expired, enable now rejects it without writing (before this change it wrote the stale whole row back). The next setup overwrites the expired pending secret.

Policy and where it comes from

Option A: no setup or enable while MFA is enabled. To replace a factor, the user disables MFA first with the password plus a current TOTP or recovery code (unchanged), then enrolls again. A user who has lost their authenticator uses a recovery code to disable. No admin MFA-reset path exists, and this PR adds none. This is the shape in the reviewed #227 lifecycle plan that the owner's issue comment refers to. It is also the issue's second suggested fix. The UI already hides "Set up" when mfa_enabled is true. A stale tab displays the server's 409 message through the existing flow error element, so the frontend needs no change.

Verification

Commands: GOTOOLCHAIN=go1.26.6 GOWORK=off GOFLAGS='-p=2 -count=1' AWS_EC2_METADATA_DISABLED=true, run on macOS with testcontainers PG16.

• New internal/auth/service_mfa_db_test.go (integration tag, real Postgres, uses the existing credentialReadBarrier read-barrier fixture):
  • sequential attack: setup, and enable with planted pending material, are both refused; the row is unchanged; the victim's TOTP and recovery codes still log in
  • stale setup, stale password-only disable, and an expired enable, each racing a victim enrollment
  • stale enable against a concurrent setup, and two enables of the same pending secret
  • control: re-enroll after disabling with the current factor
• Before the fix (main logic, with only the new sentinel added so the tests compile): 6 of 7 subtests FAIL. Example failure: Expected error with "MFA is already enabled; ..." in chain but got nil. In the expired case the stored row ends up with MFAEnabled:false, MFASecret:"", so the victim's factor is erased. The control passes.
• After the fix: all 7 PASS. Full internal/auth integration suite: 913 PASS, 0 FAIL, 0 SKIP.
• Mutations, each reverted afterwards:
  • dropping mfa_enabled = false from SetPendingMFASecret fails stale-setup-...
  • dropping the exact-secret predicate from EnableMFA fails stale-enable-after-concurrent-setup
  • removing the service guard in MFASetup fails the error-class assertions; the CAS still keeps the row safe
• go build ./..., go vet (with and without -tags integration), and golangci-lint (with and without the integration tag) on auth/api/server/mocks: clean. Unit tests for auth, api, server and mocks: pass. Handler test TestHandler_mfaSetupAndEnable_ConflictsAre409 covers the HTTP mapping.
• TestIntegration_LoginLockoutExpiry sometimes fails locally on a sub-millisecond Go-vs-container clock skew. It reproduces on unmodified main (-count=5) and this PR does not touch it.

Deliberately left out

• The SQL expiry predicate and the row-lock-first transaction from the earlier plan. Expiry is checked in Go on the read snapshot. Takeover needs a pending secret, and that secret only comes from a setup that passed the password check and the mfa_enabled = false CAS. Reviewers can push back on this.
• Whole-row writes in factor-authenticated disable (service_mfa.go MFADisable enabled branch), recovery-code regeneration, and admin UpdateUser: these stay with sec(auth): stale whole-row user writes can still erase MFA fields #493.
• Per the owner's note, this PR uses Refs #227 rather than closing it, because sec(auth): stale whole-row user writes can still erase MFA fields #493's residual stale writers can still undo an enrollment.

Refs #227

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 11 billable files and costs up to $2.75.}

• Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 12 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used all 2 included reviews currently available. Your 50 included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Essentials
• Run ID: 451f1b72-c884-4c1d-b3b7-694ceb852c13

📥 Commits

Reviewing files that changed from the base of the PR and between e65a8a3 and 5583f43.

📒 Files selected for processing (11)

• internal/api/handler_auth.go
• internal/api/handler_auth_test.go
• internal/auth/errors.go
• internal/auth/interfaces.go
• internal/auth/service_mfa.go
• internal/auth/service_mfa_db_test.go
• internal/auth/service_mfa_test.go
• internal/auth/store_postgres_credentials.go
• internal/auth/test_helpers.go
• internal/mocks/stores.go
• internal/server/health_test.go

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

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