Skip to content

fix(auth): store the MFA enrolment in one transaction and never return database text (#714) - #898

Merged
alex-dembele merged 7 commits into
masterfrom
fix/714-mfa-setup-atomic
Oct 7, 2026
Merged

alex-dembele merged 7 commits into
masterfrom
fix/714-mfa-setup-atomic

Conversation

@alex-dembele

Copy link
Copy Markdown
Member

Closes #714

#889 already fixed the lockout in this issue's title: an unfinished enrolment can start over. This PR does what #714 still asked for, plus two defects found on the way. Spec: docs/714_MFA_SETUP_ATOMIC.md.

One transaction. Setup wrote the secret, deleted the old backup codes and inserted the new ones as three statements, so a failure on the last left a new secret with no backup codes. MFARepository.StartMFAEnrolment now does all three in one transaction. The secret goes through an upsert on user_id that only applies to an unverified row of the same tenant, which also closes the race #889 left: two first setups at the same moment used to send the second one into the unique index (400). Codes are bcrypt-hashed before the transaction opens. ReplaceUnverifiedMFASecret lost its only caller and is removed.

No database text in responses. mapAuthError sent any untyped error back as 400 {"error": err.Error()}. It now answers 500 with the generic FR/EN message and logs the real error. This applies to setup and verify; typed errors (validation, not found, conflict, forbidden) answer exactly as before.

Tombstones from before #754. Turning MFA off used to soft-delete the secret. That hidden row still holds the unique user_id, so on master such an account got 400 on every setup. It is now brought back as a fresh, unverified enrolment, still within its own tenant.

Frontend. Only the two guard comments change. The guards stay, for a new reason: each call now voids the previous key, so a double effect could leave an unusable QR code on screen.

Escalated, not fixed. user_id is unique across tenants while every query is per tenant, so a person in two organisations can enrol in only one. Raised as D-066, tracked in #897.

Tests

  • Repository, SQLite plus Postgres twins: create, replace N times (one row, last 8 codes), verified refusal, tenant scope, tombstones, rollback when the code insert fails, 10 concurrent first setups. Removing the transaction, the is_verified guard, the upsert or the tombstone clause each makes a test fail (checked by mutation).
  • Use case: TestSetupMFA_Success (one store call, 8 scoped bcrypt codes), _NotFound, _Unauthorized, _StoreFailureIsNotAConflict. fix(auth): an unfinished MFA enrolment locks a privileged account out for good — setup answers 400 forever #889's _ReplacesAnUnverifiedSecret and _VerifiedSecretIsKept are kept under those names (owner's call): they are this issue's _ReplacesUnverifiedSecret and _VerifiedConflict.
  • HTTP: setup three times → 200 each, one row; forced store failure → 500, generic body, no secret stored.

Verified

go test ./...                                     → 80 packages ok, no FAIL
DATABASE_URL=… go test -race ./internal/application/auth/ ./internal/infrastructure/repository/ ./internal/handler/auth/  → ok ×3
golangci-lint v1.64.8 run --new-from-rev=origin/master (touched packages) → no issues
npx tsc -b --noEmit · eslint on the two .tsx files → clean

Live, branch server on a throwaway Postgres 16:

setup #1/#2/#3 → 200 · mfa_secrets rows 1 · unused codes 8
verify, first QR code → 400 invalid TOTP code · last QR code → 200 · setup again → 409
code insert forced to fail (trigger) → 500 "Something went wrong…" / "Une erreur est survenue…"; secret unchanged, codes 8 → 8
verified tombstone → setup 200 → verify 200, row live and verified

Not done

  • The devsecops review in the DoD has not run as a separate pass. I reviewed the diff for tenant scope, logging and the verified-secret path myself, and the owner may want the agent pass before merging.

#889 already lets an unfinished enrolment start over. What #714 still
asks for is the transaction across the secret and the backup codes, and a
response that never carries database text. The spec records what master
already covers and what this branch adds.

Signed-off-by: alex-dembele <alexandredembele16@gmail.com>
…on (#714)

Setup wrote the secret, deleted the old backup codes and inserted the new
ones as three statements. If the insert failed, the user kept a new
secret and no backup codes at all.

StartMFAEnrolment now does all three in one transaction. The secret is
written by an upsert on user_id that only applies to an unverified row of
the same tenant, so it also closes the race #889 left: two first setups
at the same moment both land instead of the second hitting the unique
index. A verified secret affects no row, the call rolls back and setup
answers 409 as before.

The codes are hashed before the transaction opens, so bcrypt never runs
under the row lock. ReplaceUnverifiedMFASecret had no other caller and
is gone; its cases move to the new repository tests, which run on SQLite
and, with DATABASE_URL, on Postgres, including a concurrent first setup.

Signed-off-by: alex-dembele <alexandredembele16@gmail.com>
)

mapAuthError sent any untyped error back as 400 with err.Error(), so a
store failure showed the user a driver message and a constraint name.
Untyped errors are server faults the caller cannot fix: they now answer
500 with the generic FR/EN message, and the real error goes to the log.

This covers setup and verify. Errors a user can cause are typed and
answer as before.

Signed-off-by: alex-dembele <alexandredembele16@gmail.com>
Both guards said a second setup call answers 400. It no longer does:
each call issues a new key and voids the previous one. The guards stay,
because a second call could now leave a QR code on screen that the
server no longer accepts.

Signed-off-by: alex-dembele <alexandredembele16@gmail.com>
mfa_secrets.user_id is unique on its own while every query is scoped by
tenant, so a member of two organisations can enrol in only one. Choosing
the model is tenant-isolation design, so it goes to the owner. Tracked
in #897.

Signed-off-by: alex-dembele <alexandredembele16@gmail.com>
…again (#714)

Before #754, turning MFA off soft-deleted the secret. Such a tombstone is
invisible to every read but still holds the unique user_id, so the setup
upsert landed on it: an unverified tombstone got a new key nobody could
see, and setup answered 200 with a QR code that could never verify; a
verified one answered 409 to a user with MFA off.

A soft-deleted row is now replaced like an unverified one and brought
back as a fresh enrolment, still only within its own tenant.

Signed-off-by: alex-dembele <alexandredembele16@gmail.com>
Signed-off-by: alex-dembele <alexandredembele16@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(auth): a started but unfinished MFA enrolment can never be retried — setup answers 400 "duplicated key"

1 participant