Skip to content

fix(auth): let an unfinished MFA enrolment start over (#889) - #892

Merged
alex-dembele merged 1 commit into
masterfrom
fix/889-reenrol-after-unfinished-setup
Oct 6, 2026
Merged

alex-dembele merged 1 commit into
masterfrom
fix/889-reenrol-after-unfinished-setup

Conversation

@alex-dembele

Copy link
Copy Markdown
Member

Closes #889

A member whose role requires MFA and whose enrolment was ever left unfinished (tab closed, or the 15-minute enrolment token expired) could never sign in again.

SetupMFAUseCase refused only a verified secret, then always inserted a new row. The unverified row from the earlier attempt made that insert fail on the unique mfa_secrets.user_id, so /auth/mfa/setup answered 400 on every later attempt, and every sign-in landed on that broken enrolment.

What changes

  • Unverified secret: it is now replaced in place by one conditional UPDATE … WHERE user_id AND tenant_id AND is_verified = false, in the new MFARepository.ReplaceUnverifiedMFASecret. If a verification lands at the same moment, the update touches nothing and setup answers 409, as it always did for a verified secret.
  • Replay state: last_totp_step and last_used_at belonged to the abandoned key, so they are reset explicitly rather than through a Save, which security(auth): a TOTP code can be replayed within its validity window #849's contract forbids.
  • Verified secret: still refused with 409, and never touched.

Verification

Run on this branch plus #888's one-file fix, because master does not compile until #888 is merged (#886), with a real Postgres:

go build ./... && go vet ./... && DATABASE_URL=… go test ./... -race → exit 0, 80 packages ok, 0 FAIL

New tests:

  • TestSetupMFA_ReplacesAnUnverifiedSecret and TestSetupMFA_VerifiedSecretIsKept, with a mock that enforces the unique user_id. The old mock overwrote on insert, which is why this never showed up. Before the fix, the first test failed with duplicated key not allowed.
  • TestGormMFARepository_ReplaceUnverifiedMFASecret, on sqlite and on Postgres. It checks tenant scoping, the reset of the replay step, and that a verified secret is left untouched.

Live, reproducing the issue:

  1. An invited admin, under a 0-day grace policy, starts enrolment at 12:18 (POST /auth/mfa/setup → 200).
  2. The token expires.
  3. Signing in again: before this change, setup answered 400. With it, setup answers 200 with a new key, the code from that key answers 200, and the user lands on the dashboard.

Merge order

After #888: CI cannot build the backend until it is in.

Setup refused only a verified secret, then always inserted a new row.
An enrolment left unfinished (tab closed, token expired) leaves an
unverified row behind, and mfa_secrets.user_id is unique, so every later
setup failed with 400. For a role that requires MFA that is a permanent
lockout: each sign-in lands on enrolment, and enrolment cannot start.

An unverified secret is now replaced in place by one conditional UPDATE
(is_verified = false, tenant-scoped). It loses cleanly against a
verification landing at the same moment, which then answers 409 as a
verified secret always did. The abandoned key's last_totp_step and
last_used_at are reset explicitly, not through a Save, as #849 requires.

Signed-off-by: alex-dembele <alexandredembele16@gmail.com>
@alex-dembele alex-dembele added type:bug Something is broken area:backend Go, /internal, /pkg priority:P1-high Blocks a milestone labels Oct 2, 2026
@alex-dembele alex-dembele added the tier:0-trust Trust: security, isolation, evidence integrity label Oct 2, 2026
@alex-dembele
alex-dembele merged commit cefe453 into master Oct 6, 2026
15 of 30 checks passed
@alex-dembele
alex-dembele deleted the fix/889-reenrol-after-unfinished-setup branch October 6, 2026 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:backend Go, /internal, /pkg priority:P1-high Blocks a milestone tier:0-trust Trust: security, isolation, evidence integrity type:bug Something is broken

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(auth): an unfinished MFA enrolment locks a privileged account out for good — setup answers 400 forever

1 participant