Repository navigation
fix(auth): a started but unfinished MFA enrolment can never be retried — setup answers 400 "duplicated key" #714
Description
Problem
A user who opens MFA enrolment and does not finish it can never enrol again.
The first POST /api/v1/auth/mfa/setup stores an unverified secret. Every later call tries to INSERT a second row, hits the UNIQUE index on mfa_secrets.user_id, and answers 400 "failed to store MFA secret: duplicated key not allowed". Closing the dialog, reloading the page, losing the phone before scanning, or a network blip after the first call all leave the account in this state. The UI then shows "Impossible de démarrer l'activation. Réessayez…"; retrying cannot succeed.
This matters most for a role under the MFA policy. The banner says that once the grace period ends, "l'accès sera bloqué jusqu'à l'activation du MFA", and activation is exactly what the account can no longer do. The only way out is a manual DB delete.
The frontend already knows the endpoint is not idempotent and guards against React's double effect: frontend/src/features/auth/MFAEnrollmentDialog.tsx:42 and frontend/src/features/auth/AuthScreen.tsx:395. That guard hides the defect in the common case; the backend was never fixed.
Found on 2026-09-17 while capturing visual diffs for #524.
Reproduction (verified 2026-09-17, local stack, master 53c5f29)
POST /auth/register → 201 (fresh account)
POST /auth/login → 200
POST /auth/mfa/setup → 200 (QR code returned)
POST /auth/mfa/setup → 400 {"error":"failed to store MFA secret: duplicated key not allowed"}
Cause: backend/internal/application/auth/mfa_usecase.go SetupMFAUseCase.Execute refuses only when existingSecret.IsVerified. When a secret exists but is unverified it still calls CreateMFASecret (a plain GORM Create), and mfa_secrets has idx_mfa_secrets_user_id UNIQUE (user_id).
Two related defects in the same function:
- The secret write and the backup-code writes (
DeleteBackupCodes,SaveBackupCodes) are separate statements with no transaction (ABSOLUTE RULE 7). A failure in the middle leaves a secret with no backup codes. - The raw storage error reaches the client as a 400 instead of a typed error (ABSOLUTE RULE 3).
Acceptance criteria
- Given an account with an unverified MFA secret, when
POST /auth/mfa/setupis called again, then it answers 200 with a new secret and QR code. The stored secret is replaced, so only the newest QR code verifies. - Given an account with a verified MFA secret, when
POST /auth/mfa/setupis called, then it still answers with the typed conflict error (ErrConflict,already_enabled), unchanged. - Given setup is called N times before verification, then exactly one
mfa_secretsrow exists for the user, and exactly one set of unused backup codes (the latest). - Given a failure while storing backup codes, then no new secret is persisted: secret and codes are written in one transaction.
- Given any storage failure, the response is a typed error and never includes the database message.
- Every query stays scoped by
tenant_id, as today.
Definition of Done
- tenant_id filtering on every query touched
-
TestSetupMFA_Success,TestSetupMFA_NotFound,TestSetupMFA_Unauthorized, plusTestSetupMFA_ReplacesUnverifiedSecretandTestSetupMFA_VerifiedConflict - Transaction across
mfa_secretsandmfa_backup_codes - FR + EN strings: N/A unless the error copy changes
- Security review (devsecops): this touches auth. It is a fix, not a redesign of the MFA flow
- The frontend "not idempotent" guard comments are updated, or removed if no longer needed
- Claim matrix: no-op
Scope OUT
- Changing the TOTP, encryption or backup-code scheme.
- The MFA policy and grace period.
Milestone chosen by the reporting agent (auth reliability belongs with trust); the PO may re-milestone.
Activity
alex-dembele commentedon Oct 7, 2026
backend — 2026-10-07
Done — PR #898, branch
fix/714-mfa-setup-atomic(worktreeOpenRisk-wt-714). #889 had already fixed criteria 1 and 2. This PR adds:MFARepository.StartMFAEnrolmentwrites the secret (upsert, unverified rows of the same tenant only), the code delete and the code insert in one transaction —backend/internal/infrastructure/repository/gorm_mfa_repository.go,mfa_repository_interface.go.ReplaceUnverifiedMFASecretremoved (no caller left).backend/internal/application/auth/mfa_usecase.go.mapAuthError: an untyped error now answers 500 with the generic FR/EN text and is logged —backend/internal/handler/auth/mfa_handler.go.AuthScreen.tsx,MFAEnrollmentDialog.tsx.docs/714_MFA_SETUP_ATOMIC.md; D-066 raised indocs/DECISIONS.md; follow-up fix(auth): a person in two organisations can enrol MFA in only one, because mfa_secrets.user_id is unique #897.Verified
go test ./...→ 80 packages ok, no FAIL.DATABASE_URL=<throwaway pg16> go test -raceonapplication/auth,infrastructure/repository,handler/auth→ ok ×3, includingStartMFAEnrolment_{,ReusesATombstone,RollsBackWhenTheCodesFail,ConcurrentFirstSetup}_Postgres.is_verifiedguard, the upsert or the tombstone clause each fails a test.golangci-lint v1.64.8 --new-from-rev=origin/masteron the touched packages → clean;npx tsc -b --noEmit→ clean.Criteria — 1 ✅ (#889) · 2 ✅ (#889) · 3 ✅ · 4 ✅ · 5 ✅ · 6 ✅
DoD: tests ✅. #889's
…ReplacesAnUnverifiedSecret/…VerifiedSecretIsKeptare kept as this issue's…ReplacesUnverifiedSecret/…VerifiedConflict(owner's call). Transaction ✅ · FR/EN: no new strings, existinggenericFailurereused ✅ · frontend comments ✅ · claim matrix no-op ✅ · devsecops review ❌ not run as a separate agent pass. The diff was self-reviewed for tenant scope, logging and the verified path.Next — Owner review of PR #898; optionally a devsecops pass first. Then the merge.
Blocked on — nothing for #714. #897 is blocked on the owner's decision D-066.
Merge pull request #898 from opendefender/fix/714-mfa-setup-atomic
alex-dembele commentedon Oct 10, 2026
devsecops review — 2026-10-11
Done — Security review of PR #898 (merged; head 3fe842b), the last open DoD item. Read-only review of the diff
1b5be5d6^..3fe842b6, plus the repository, use cases, handler, middlewares and route wiring. Live run on a throwaway Postgres and Redis.Verified
tenant_idmatches the caller's, the code delete and insert are scoped by(user_id, tenant_id), and IDs come only from the verified JWT.MFA_ENROLLMENTtoken, anMFA_REQUIREDtoken gets 401, and a PAT can't pass./auth/mfa/*. The new log line can't carry a secret: pgconn errors leave out the Detail field.DisableMFA's, so there's no deadlock path. The 7 repository tests pass on Postgres.Pre-existing findings outside the diff, filed:
MFA_REQUIREDtoken (password only) is accepted on every route behindProtected. Reproduced live: a*PAT minted, and MFA turned off, without the second factor.tenant_idfilter) — a backup code can be used twice.MFA_ENCRYPTION_KEYis also used in production.Saveoverwrites a setup that lands meanwhile.errors.AsinmapAuthError, refused setups not audited.Criteria — DoD "Security review (devsecops)" ✅ for #714.
Next — #944 first; it blocks any release. Then #946.
Blocked on — nothing. D-066 has been decided (one factor per organisation), and #897 is
status:ready.