[IAmJSD]

Why

#1571 added the {email, mfa_token, mfa_code} login step. That step looked up the user by the submitted email. As a result, the answer to a token that cannot complete a sign-in depended on the account the email named:

• no account, or an inactive or locked account: invalid credentials
• an active account other than the token's user: invalid_mfa_token

This PR makes every token-mismatch case return the same response.

What

• internal/auth/service_mfa_login_token.go: claimMFALoginToken now loads the user with GetUserByID(claim.UserID), using the user the token is bound to, and no longer calls GetUserByEmail(email). The new mfaTokenUserMayContinue helper checks that:

  • the submitted email equals that user's email (exact match, the same rule GetUserByEmail uses),
  • the account is active and not locked,
  • MFA is still on,
  • the password version still matches.

  Every case that cannot lead to a sign-in now returns ErrInvalidMFAToken (401 invalid_mfa_token): an unknown, expired or used token, a mismatched email, a failed or empty user lookup, and an inactive or locked token user.

• internal/auth/service.go: the lock-window check moves into a small accountLocked helper, which the password path and the token path both use. Password-path behavior is unchanged.

• Unchanged:

  • Rate limiting: the per-IP login bucket and the per-email login_mfa bucket, still keyed by the submitted email.
  • Lockout accounting: a wrong code still calls RecordFailedLogin on the token's user, and that is only reached once the email matches.
  • The password and password + code forms.

• internal/auth/errors.go and internal/api/openapi.yaml: the invalid_mfa_token description now also covers an account that can no longer sign in.

• Tests:

  • TestLoginWithMFAToken_EveryMismatchSameAnswer (unit, MockStore) covers five cases: a non-existent email, an inactive user's email, a different active user's email, the token's user inactive, and the token's user locked. Each must give ErrInvalidMFAToken. The test also asserts that no code is checked, no failure is recorded and GetUserByEmail is never called.
  • TestLoginWithMFAToken_UserLookupErrorRefused (unit): a store error, or a missing token user, gives ErrInvalidMFAToken.
  • TestIntegration_MFALoginToken/every-mismatch-same-answer (real Postgres) covers the same five cases with real accounts. It also checks that no failed attempt was recorded and that no session was created.
  • The existing unit tests and internal/server/adapter_mfa_token_test.go now mock GetUserByID for the token step. The locked-account unit case was folded into the new test, because its expected error is now invalid_mfa_token.

Verification

Linux, Go 1.26, AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true, head 7c78a0b7 (origin/main f50f1d3e plus one commit). Postgres via testcontainers.

Fail-before. I put origin/main's internal/auth/service.go and service_mfa_login_token.go back under the new tests:

• Unit: TestLoginWithMFAToken_EveryMismatchSameAnswer fails in all 5 subtests. Non-existent email, inactive user email, token user inactive and token user locked get invalid credentials instead of invalid_mfa_token. Other user email gets invalid_mfa_token, but only after a lookup by email.
• Integration: every-mismatch-same-answer fails for non-existent email, inactive user email, token user inactive and token user locked (in chain: "Check your email address and password and try again"). Other user email already gave invalid_mfa_token.

Pass-after: both tests pass on this branch.

Checks:

• go build ./... passed.
• go vet passed on internal/auth, internal/api and internal/server, with and without -tags integration.
• go test -count=1 passed on the same three packages.
• go test -tags integration -count=1 -run TestIntegration_MFALoginToken ./internal/auth/ passed (all 10 subtests).
• golangci-lint found 0 issues on those packages, with and without --build-tags integration.
• gocyclo -over 10 reports nothing on the changed non-test files.

Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree: queue/platform @ 6144e322 = origin/main f50f1d3e plus these heads, merged in this order:

Branch	Tested head
fix/issue-712-gcp-azure	9569af9
fix/issue-134-mfa-token-lookup	7c78a0b
chore/review-followups-1028	b358cf4

Linux, Go 1.26.9, AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers.

• Passed: go build ./..., go vet ./..., go vet -tags integration ./... and the full go test -race -short -count=1 ./....
• Static checks: golangci-lint v2.10.1 found 0 issues on every changed package, with and without --build-tags integration. gocyclo -over 10 is silent on the changed non-test files.
• Integration tests, every changed package that has them, one package at a time (-tags integration -count=1): internal/analytics, internal/auth, internal/config and internal/server all pass.
• Secrets: git-secrets on the changed files found nothing, and a planted fake key was flagged. scripts/test-git-secrets-allowlist.sh passes.
• Terraform: terraform fmt -check and validate (init -backend=false) on the touched modules; .tftest.hcl files run in CI. Nothing was applied to any cloud.

Refs #134

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 59 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: main
• File: .coderabbit.yaml
• Commit: f50f1d3
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 55ba57ae-d110-4da0-8972-fd4a81b7ea05

Commits

Reviewing files that changed from the base of the PR and between f50f1d3 and 7c78a0b.

Files selected for processing: 7

• internal/api/openapi.yaml
• internal/auth/errors.go
• internal/auth/service.go
• internal/auth/service_mfa_login_token.go
• internal/auth/service_mfa_login_token_db_test.go
• internal/auth/service_mfa_login_token_test.go
• internal/server/adapter_mfa_token_test.go

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

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 7c78a0b768118f27ea5c07f7d87b537cf827dd98

Blocking:

• none

Non-blocking:

• internal/auth/service_mfa_login_token.go:123-129: the token step no longer logs anything when it refuses a locked token user. The password path still logs Login attempt for locked account, and the old token path did too, via getUserAndValidateStatus. A one-line logging.Warnf on the locked or inactive branch, without the user ID, would keep that signal for operators. A mismatched email is also refused silently. That is fine for the response, but a log line would help spot token probing.
• A store error on GetUserByID used to return invalid credentials and now returns invalid_mfa_token, which sends the client back to the password step after a transient DB error. That is the intended uniform answer and costs at most one re-entry of the password. Noting it only because the PR body's list of refusals mentions it without calling it a behavior change.

Attacked:

• Enumeration and scope. With a live token, every email that can't complete the sign-in now gets invalid_mfa_token from one GetUserByID path, with no GetUserByEmail call. That closes the "no or inactive account vs another active account" oracle. Only the token owner's correct email can reach verifyLoginMFACode and so RecordFailedLogin, so you can't lock out another account by naming it.
• Email matching. The exact Go == against parsed.Address matches the old lookup, because users.email is VARCHAR(255) with the default deterministic collation (WHERE email = $1, 000001), and the email is normalized by mail.ParseAddress before the branch, as before.
• Attempt spending, rate limits and lockout: the claim still runs before the user check, and the per-email bucket in handler_auth.go is unchanged. mapLoginError maps the sentinel to 401 invalid_mfa_token. No frontend code consumes mfa_token yet, so the UI doesn't change.
• Ran at 7c78a0b: go build ./...; go vet on internal/auth, internal/api and internal/server, with and without -tags integration; go test -race -short -count=1 on those three packages (ok); go test -race -tags integration -count=1 -run 'TestIntegration_MFALoginToken|Login' ./internal/auth (ok); golangci-lint on internal/auth, with and without integration tags (0 issues); gocyclo -over 10 on the changed files (silent).
• Fail-before: I restored origin/main's service.go and service_mfa_login_token.go under the new tests. TestLoginWithMFAToken_EveryMismatchSameAnswer failed in all 5 subtests: 4 got the wrong error, and "other user email" called GetUserByEmail. Integration every-mismatch-same-answer failed too. Both pass with the fix restored.

Body-stale:

• none

_{Reviewer: rJ1. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 7c78a0b768118f27ea5c07f7d87b537cf827dd98

Blocking:

• none

Non-blocking:

• internal/auth/service_mfa_login_token.go:119-127: the token path no longer logs anything when it refuses a locked or inactive token user. The password path still logs Login attempt for locked account in getUserAndValidateStatus. If operators watch that line, a logging.Warnf on the accountLocked / !Active branch would keep the two paths consistent. The client answer would not change.
• internal/auth/service_mfa_login_token_test.go:114: the other-user case in TestLoginWithMFAToken_BindingMismatchRefused now rewrites the user's ID and email, so the email mismatch is what refuses it. That makes it the same as other user email in the new test. It's harmless, since the ID binding now holds by construction (GetUserByID(claim.UserID)), but the case name no longer says what it tests.

Attacked:

• Oracle. Before this change, someone holding a valid token (so already knowing one account's password) could try a different email on each of the token's 3 attempts. invalid credentials meant no account or an inactive one, and invalid_mfa_token meant another active account. Now every refusal goes through the same ClaimMFALoginTokenAttempt + GetUserByID and returns the same sentinel, so the answer and the DB work no longer depend on the submitted email. An unknown token still returns before any user read. Rate limits (login_mfa per submitted email) and lockout accounting can't be sidestepped: a mismatched email only burns a token attempt and never reaches a code check.
• Email equality. users.email is plain VARCHAR(255) (000001), and GetUserByEmail is WHERE email = $1. Both steps normalise the email with mail.ParseAddress in Login. So the Go user.Email != email check behaves exactly like the old lookup, and a real user's login won't break on letter case. Nil-user and store-error results from GetUserByID fail closed. The checks the old path made (active, locked, MFA on, password version) are all still there.
• Ran: go build ./.... go vet on internal/auth, api and server, with and without -tags integration. go test -race -short -count=1 on the same three packages: pass. go test -race -tags integration -count=1 ./internal/auth/: pass. golangci-lint on internal/auth, with and without --build-tags integration: 0 issues. gocyclo -over 10 on the changed files: nothing reported.
• Fail-before: I checked out origin/main's service.go and service_mfa_login_token.go under the new tests. The unit test TestLoginWithMFAToken_EveryMismatchSameAnswer failed in all 5 subtests, and TestIntegration_MFALoginToken/every-mismatch-same-answer failed. Both pass again after restoring the PR's files, and the worktree is clean.

Body-stale:

• none. The PR body's fail-before description matches what I reproduced. The PR says Refs #134, which is right, because chore(frontend/core): medium/low findings from the 2026-07-28 full review #134 is a multi-item tracking issue and this PR is a follow-up to its MFA-continuation-token item.

_{Reviewer: rJ2. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Local verification at 7c78a0b768118f27ea5c07f7d87b537cf827dd98 merged onto main (f50f1d3e), together with #1573, #1574, #1575 in that order (combined tree 1f343bb5, Linux, Go 1.26.10): go build ./..., go vet with and without -tags integration, the full go test -race -short -count=1 ./..., and go test -race -tags integration -count=1 on every changed package, against Docker Postgres. All pass. CI Success and pre-commit are green on this head, with 2 approving independent review(s) on it. Nothing was run against any cloud.

[IAmJSD]

Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 2 approving independent review(s) on 7c78a0b7 (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly 7c78a0b768118f27ea5c07f7d87b537cf827dd98.