fix(auth): require the password to turn MFA off (#754) - #848
Merged
alex-dembele merged 9 commits intoOct 1, 2026
Merged
alex-dembele merged 9 commits into
alex-dembele merged 9 commits into
Conversation
…754) Disabling MFA ran two writes outside a transaction: a failure on the second left backup codes alive after their secret was gone. Both now go in one transaction, and the secret is hard-deleted so re-enrolment does not trip the unique user_id constraint on a tombstone. Signed-off-by: alex-dembele <alexandredembele16@gmail.com>
An open session was enough to remove the second factor. The disable endpoint now re-verifies the current password (wrong or missing: 401, generic body), counts every attempt against a per-account budget of 5 per 15 minutes on top of the per-IP limiter, refuses roles that login requires MFA for (403), writes an mfa_disable audit entry with a reason code on failure, and notifies the owner by email and in-app. Signed-off-by: alex-dembele <alexandredembele16@gmail.com>
The deferred initial focus moved focus to the first focusable element, the close button, a frame after an autoFocus field had taken it. A user typing straight away typed into nothing. It now stays put when focus is already in the panel. Signed-off-by: alex-dembele <alexandredembele16@gmail.com>
…#754) A dialog asks for the current password, reports a wrong one on the field and refusals it cannot fix plainly, and hides the button for roles that require MFA. The mutation never retries: every request is a password guess counted against the server's budget, and the app-wide retry of 3 spent four of five attempts on one typo. Signed-off-by: alex-dembele <alexandredembele16@gmail.com>
The sqlite tests fail the second write through a GORM callback. This one, gated on DATABASE_URL, makes Postgres refuse it with a trigger and checks the secret delete rolls back, the tenant scope holds, and re-enrolment works after a hard delete. Signed-off-by: alex-dembele <alexandredembele16@gmail.com>
#754) An account that signs in through an identity provider has no local password, so the disable endpoint refused it outright. It now confirms with a current TOTP code instead (401 wrong_code otherwise, same per-account budget). Backup codes do not count, and a code never replaces the password of an account that has one. Without the key wired, the refusal stays: it fails closed. /auth/me returns has_password so the client asks for the right proof, and the deactivation notice no longer claims a password was confirmed. Owner decision D-061. Signed-off-by: alex-dembele <alexandredembele16@gmail.com>
… MFA off (#754) The dialog reads has_password when it opens and shows either the password field or a six-digit code field, with copy that matches. If it guessed wrong (the read failed), a wrong_code answer to a password switches it to the code. Signed-off-by: alex-dembele <alexandredembele16@gmail.com>
…a code (#754) Signed-off-by: alex-dembele <alexandredembele16@gmail.com>
Neither route was in the spec, so has_password and the disable body and error codes existed only in the code. The generated client types are not refreshed here: the committed file already drifts from the spec on unrelated routes. Signed-off-by: alex-dembele <alexandredembele16@gmail.com>
This was referenced Sep 30, 2026
alex-dembele
deleted the
754-securityauth-disabling-mfa-does-not-re-verify-the-password
branch
October 1, 2026 12:21
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #754
Turning MFA off used to need nothing more than an open session. It now needs the current password, is refused for roles that must keep MFA, happens in one transaction, and is audited and reported to the account owner.
What changed
POST /auth/mfa/disableverifies the current password withPasswordHasher.Verify. A wrong or missing password gets 401 with a generic body (code: wrong_password). Accounts that sign in through an identity provider have no local password and get 409no_local_password. Code:backend/internal/application/auth/mfa_usecase.go,backend/internal/handler/auth/mfa_handler.go.authRateLimitalso applies to the route. Over the limit: 429too_many_attempts.mfaRequiredRoles/mfaRequiredBusinessRolesget 403mfa_required_by_role, checked against both the stored membership and the token's org role. If the membership lookup fails, the request is refused.GormMFARepository.DisableMFAdeletes the secret and the backup codes in a single transaction. The secret is hard-deleted becauseuser_idis UNIQUE, so a soft-deleted row would block re-enrolment.mfa_disableaudit entry, with a reason code when it fails. The password is never logged. The owner gets an email (FR/EN, through the existing async security mailer) and an in-appmfa_disablednotification.MFADisableDialogasks for the password (validated with Zod), shows a wrong password as a field error and other refusals as an inline message. Roles that can't turn MFA off see no button.Two defects found in the live pass and fixed here
mutations.retry: 3resent the request, so one typo used 4 of the 5 attempts and the next try got a 429.useDisableMFAnow setsretry: false. The test fails without the fix ("called 4 times").useDismissableLayer's deferred initial focus overrodeautoFocus, so typing went nowhere. This also causedmfaDisable.test.tsxto fail 1–3 of its 5 tests per run. The fix is 4 lines in the design system: when focus is already inside the panel, leave it there. No API change.DeclareIncidentModalandCreateAssetModalget the same fix, since they also useautoFocusinside aModal.Verification
Tests named in the acceptance criteria:
TestDisableMFA_Success,TestDisableMFA_NotFound,TestDisableMFA_Unauthorized(wrong password),TestGormMFARepository_DisableMFA_RollsBackWhenTheSecondWriteFails, and the handler E2E inmfa_disable_e2e_test.go.Live pass: the real
MFAAccountPanelrendered in Chromium, with the API mocked by Playwright:aria-invalid=true). Before the fix: 4 requests, about 7 s{password, locale}, and after success the dialog closes and the toast shows.playwright-mcp/754-disable-wrong-password.pngSSO accounts (owner decision D-061)
An account that signs in through an identity provider has no local password to re-check. It now confirms with a current TOTP code from its authenticator app:
wrong_codeand counts against the same 5-per-15-minute budget. Backup codes are not accepted. A code never replaces the password of an account that has one./auth/mereturnshas_password(a boolean). The dialog uses it to show the password field or a 6-digit code field, with matching copy. If that read fails, awrong_codeanswer switches the dialog to the code field.Live run on real Postgres and Redis
Docker still wasn't reachable, so I used the local PostgreSQL 18 binaries (the stack targets 16) in a throwaway cluster on :55754, plus a throwaway
redis-serveron :56754. The branch server was booted from the repo root, so every migration ran. All of it has since been stopped.mfa_required_by_rolewrong_password/ 401wrong_passwordmfa_secrets1→0 andmfa_backup_codes8→0 (hard delete)not_enrolledtoo_many_attempts; Redis keyratelimit:mfa-disable:<user>, TTL 900 sauth_audit_logs: 8mfa_disablerows, each with its reason code; chainedaudit_events:create disablenotificationsrowmfa_disabled(in_app); email logged with subject "Two-factor authentication was turned off…"users.passwordblanked)000000gives "Code incorrect." and an auditwrong_code; the live code gives 200, both tables at 0, toast shown, panel back to "Activer le MFA"; request body{code, locale}Screenshots:
.playwright-mcp/754-sso-wrong-code.png,754-sso-dialog-light.png,754-sso-dialog-dark.png.A new test,
TestGormMFARepository_DisableMFA_Postgres(gated onDATABASE_URL), uses a real Postgres trigger to make the backup-code delete fail, then checks that the secret delete rolled back. It fails when the transaction is removed. WithDATABASE_URLset, all 7 Postgres-gated tests pass, and so does the fullgo test ./....PostgreSQL 16 (target version)
Docker was reachable again, so the whole run was repeated on
postgres:16-alpine(16.15) andredis:7-alpine:create disableentry and the notification are all presenthas_password: false; password only → 401wrong_code; wrong code → 401; valid code → 200, tables at 0DATABASE_URL=<pg16> go test ./...: no failures, all 7 Postgres-gated tests PASS.OpenAPI
docs/openapi.yamlnow describesGET /auth/me(MeResponse, includinghas_password) andPOST /auth/mfa/disable(DisableMFAInput,DisableMFAErrorwith every error code), in commit4c568540.Follow-ups opened
Not done
frontend/src/types/openapi.generated.tsis not regenerated. On master it already drifts from the spec on unrelated routes (933 diff lines), and regenerating it here would pull those changes into this PR. The client keeps hand-written types for these two routes.react-hooks/set-state-in-effectlint error inMFAPolicyPanel.tsx:44was already on master and is left as it was.🤖 Generated with Claude Code