feat(mfa): behavior redesign — withheld-token first-time setup, WebAuthn/Email/SMS as MFA factors, lockout + admin recovery#686
Merged
Conversation
signup.go only set IsMultiFactorAuthEnabled when EnforceMFA was on or the caller passed it explicitly - a bare signup on a server with MFA available but not enforced left the flag unset, so resolveMFAGate's first condition (userMFAEnabled) was always false and the optional- setup-with-skip offer never triggered for anyone. Now defaults to true whenever EnableMFA is true (TOTP, or email/SMS OTP with its provider configured) and the caller didn't explicitly opt in or out. EnforceMFA and an explicit caller-supplied value both still take precedence, matching existing behavior.
signup.go's default only applies at account creation, so users who signed up before it shipped (or before MFA was configured at all) were permanently stuck with IsMultiFactorAuthEnabled unset - the offer-with-skip flow silently never triggered for them, forever. Added ensureMFADefaultSet, called from both login.go's password path and webauthn.go's passkey primary-login path, right before either one reads the flag: if it was never explicitly set and MFA is available server-wide, default it to true and persist it on that login. Same end state as a fresh signup, triggered by the user's own next login instead of a bulk migration across every DB backend. An explicit false (opt-out) and EnforceMFA's own forcing are both untouched.
…h resolveMFAGate mfaGateOfferSetup renamed to mfaGateOfferAll and moved into the token-withhold group, matching mfaGateBlockVerify/mfaGateBlockEnroll. WebauthnLoginVerify no longer hand-rolls its own EnforceMFA-only check -- passkey primary login now goes through the same 5-way gate password login does, so an optional-MFA user isn't silently exempted from the first-time setup offer just because they logged in with a passkey. Adds should_offer_webauthn_mfa_setup to AuthResponse (schema regen) -- needed for the offer response, since ShouldOfferEmailOtpMfaSetup/ ShouldOfferSmsOtpMfaSetup are Task 6's job but WebAuthn's own flag isn't. should_offer_mfa_setup is deprecated in place (still read by skip_mfa_setup_test.go) rather than deleted, since it's never written anymore once PendingTOTPOffer is gone. Also fixes a crash: webauthn.go's offer case unconditionally generated a TOTP enrollment, unlike its sibling block cases which guard on EnableTOTPLogin -- panicked on nil AuthenticatorProvider whenever TOTP login was disabled server-wide.
skip_mfa_setup now authenticates via the MFA session cookie plus email/phone_number (same pattern as verify_otp), not a bearer token -- none exists yet when the offer screen withholds it. Issues the previously-withheld access token on success.
Adds MFALockedAt, lock_mfa (MFA-session-cookie-authenticated, refused when a verified OTP fallback exists), and reset_mfa on the existing _update_user admin mutation -- clears lock/opt-in/skip state and deletes all enrolled authenticators/passkeys, landing the user back on first-time setup on next login.
Adds email_otp_mfa_setup/sms_otp_mfa_setup (bearer-token authenticated, send a code and create an unverified Authenticator row) and marks that row verified on a successful verify_otp. Retrofits login.go's previously-ungated email/SMS-OTP-as-MFA branches to require a verified enrollment, not just config+contact-info presence -- closing the gap where every optional-MFA user with a phone number was silently OTP-challenged without ever choosing that method.
A brand-new signup with MFA available now has its token withheld and is offered the first-time setup screen, matching login/passkey -- previously SignUp always issued a token immediately regardless of MFA. Guarded behind EnableMFA && EnableTOTPLogin, mirroring login.go's own guard around the same resolveMFAGate/generateTOTPEnrollment call: AuthenticatorProvider is nil whenever EnableTOTPLogin is off, so calling generateTOTPEnrollment unconditionally would panic on a webauthn-only-enforced-MFA signup. Also fixes three pre-existing tests whose fixtures assumed SignUp never creates MFA artifacts and always returns AuthResponse.User: - mfa_gate_login_test.go: addVerifiedAuthenticator now upserts instead of relying on AddAuthenticator's create-only-if-absent semantics, since signUpUser's own SignUp call (cfg.EnableMFA=true) already leaves an unverified TOTP row behind via the new gate. - mfa_service_availability_test.go, admin_reset_mfa_test.go: look the signed-up user up by email instead of reading SignUpResponse.User, which the withheld-token gate path doesn't set (matching login.go's own mfaGateOfferAll/BlockEnroll responses).
Consumer social logins (Google/GitHub/etc.) now go through the same resolveMFAGate decision as password/passkey login before the browser session cookie is set -- matches verified Auth0 behavior (MFA policy applies on top of social connections, not exempted). Enterprise SSO (saml_sp.go) and Session()'s routine token refresh are untouched.
login.go and signup.go only ran resolveMFAGate when isTOTPLoginEnabled was also true, so a WebAuthn-only enforced-MFA server (EnableTOTPLogin off) skipped the gate entirely and issued tokens unconditionally to unenrolled users -- no offer, no enforcement. WebauthnLoginVerify was already correctly gated on such a server (Task 3), which is what exposed the gap. Broaden both guards to isMFAEnabled / EnableMFA alone, matching webauthn.go's pattern: call resolveMFAGate unconditionally and condition only the TOTP-specific parts of the response (enrollment generation, TOTP screen/fields) on EnableTOTPLogin. WebAuthn/email/SMS offer flags are now set from config in mfaGateBlockEnroll too, not just mfaGateOfferAll, so a WebAuthn-only server has something actionable to offer.
EmailOTPMFASetup/SMSOTPMFASetup required a bearer token, but a user in the token-withheld first-time MFA offer has none yet -- only the MFA session cookie. Add a resolveOTPSetupCaller helper that tries the bearer token first (unchanged) and falls back to the cookie + email/phone_number pattern already used by SkipMFASetup/LockMFA. Schema gains an optional OtpMfaSetupRequest (email/phone_number, mirrors SkipMfaSetupRequest minus state) on both mutations. Also confirmed webauthn_registration_options/verify have the same bearer-token-only gap -- out of scope here, left for a follow-up.
…, M3) authenticatorVerified in oauth_mfa_gate.go only checked TOTP+WebAuthn, so a user whose only enrolled factor was a verified email/SMS OTP authenticator got routed to mfaGateOfferAll (re-offered setup) instead of mfaGateBlockVerify (asked to verify what they already have). Fold email/SMS OTP verification into the same computation login.go already uses for its own enrollment checks, and list email_otp/sms_otp in the mfa_methods hint on both the verify and offer/enroll branches. Also tighten mfaGateBlockVerify's test assertion: Contains(...,"totp") never actually distinguished it from the offer/enroll branches. Enable every other configured MFA method in that subtest and assert mfa_methods is exactly "totp" -- the only value the verify branch (lists what's verified) can produce vs. what an offer/enroll branch (lists everything configured) would.
SetState wrote the code/authToken bridge entry before EvaluateMFAGateForOAuth ran. Not exploitable as-is (code is never disclosed to the browser on a withheld redirect, so the entry just self-expires unreachable), but there's no reason to write it before we know the login actually proceeds. Move the write to after withheld=false; derivation of code/codeChallenge/nonce/ authorizeRedirectURI is unchanged, and code/nonce are still needed earlier to build params for the non-withheld response.
TestSMSOTPMFASetupViaMfaSessionCookie stopped after the setup leg (asserted the unverified Authenticator row was created, never verified, never checked a token was issued). Extend it to close the same withheld-login -> cookie-authenticated-setup -> verify_otp -> token-issued chain the email-OTP twin already proves.
…M4b) login.go's local generateOTP closure, resend_otp.go's local generateOTP closure, and otp_mfa_setup.go's generateAndStoreOTP method all did the same thing: generate an OTP, HMAC it, UpsertOTP, restore plaintext on the returned struct. Verified this before consolidating. All three call sites (login's email/phone-verification and TOTP-alternative branches, resend, and OTP MFA setup) now call the one generateAndStoreOTP method. No behavior change -- the closures' internal error logging was redundant, every call site already logs on error.
Was hardcoded true from before --disable-webauthn-mfa existed, so an operator disabling WebAuthn as an MFA factor still saw it advertised as available via the meta query.
GraphQL-only MFA redesign (skip_mfa_setup, lock_mfa, email/sms_otp_mfa_setup, plus AuthResponse/Meta/User MFA fields, UpdateUserRequest.reset_mfa) had no REST/gRPC surface. Mirrors VerifyOtp's pattern exactly: public=true RPCs, service layer resolves the caller itself (MFA session cookie and/or bearer token) rather than the auth interceptor. - types.proto: AuthResponse +4 should_offer_* fields, Meta +is_mfa_enforced, User +has_skipped_mfa_setup_at/+mfa_locked_at - admin.proto: UpdateUserRequest +reset_mfa - authorizer.proto: SkipMfaSetup, LockMfa, EmailOtpMfaSetup, SmsOtpMfaSetup RPCs + request/response messages - gen/: regenerated (buf.build BSR token in this env is expired; generated locally with version-pinned protoc-gen-go/-go-grpc/-grpc-gateway/-openapiv2 matching the committed buf.gen.yaml's remote plugin versions, diff verified scoped to only the touched proto files) - handlers: 4 new gRPC handlers, Meta/UpdateUser/projectUser/ projectAuthResponse updated to carry the new fields through - tests: end-to-end gRPC coverage for SkipMfaSetup, LockMfa, and UpdateUser.reset_mfa
974cc85 added these gRPC handlers with only go build/go vet as coverage. Port the two auth-mode scenarios already proven at the GraphQL layer (otp_mfa_setup_test.go) onto the gRPC transport: MFA session cookie + email/phone_number fallback, and ordinary bearer token, plus the unauthenticated-caller rejection case.
govulncheck flagged GO-2026-5856 (crypto/tls ECH privacy leak), fixed upstream in go1.26.5. No code changes needed.
trivy flagged 15 CVEs (2 CRITICAL, 13 HIGH) in libssl3/libcrypto3 3.5.5-r0 baked into the alpine:3.23.3 base image. The stable repo already carries 3.5.7-r0; a plain apk upgrade picks it up without touching the edge busybox pin. Re-scan: 0 vulnerabilities.
…sion mint
ResendOTP and the mobile branch of ForgotPassword are unauthenticated
(only need a victim's email/phone) yet minted the same MFA session
cookie that login/signup/webauthn/oauth mint after actually verifying
a first factor. SkipMFASetup and LockMFA trusted any valid session for
a user ID as proof of that first factor, with no OTP code check:
resend_otp{email:victim} -> session cookie for victim.ID
skip_mfa_setup{email:victim} + cookie -> victim's access/refresh/id tokens
The same chain into lock_mfa gave an unauthenticated account-lockout DoS.
Fixes:
- tag MFA sessions with a purpose (verified vs challenge); ResendOTP and
ForgotPassword's mobile branch mint challenge, everything else mints
verified
- SkipMFASetup/LockMFA reject challenge sessions
- SkipMFASetup recomputes the MFA gate and only proceeds on a genuine
mfaGateOfferAll, so a user with an already-verified second factor
can't skip past it
- EnforceMFA is now absolute: a user's persisted opt-out no longer
exempts them once the org enforces MFA, and admin UpdateUser can no
longer persist that opt-out while enforcement is on (update_profile
already guarded this; admin_users.go did not)
- MFA sessions are single-use (deleted on consumption)
New regression tests exercise the attack chain directly: a
ResendOTP/ForgotPassword-minted session is rejected by SkipMFASetup and
LockMFA, a verified-factor user can't skip, and admin UpdateUser can't
disable MFA under enforcement.
TestReleaseSmoke asserts FGA permission checks, not MFA — but MFA is on by default (TOTP/WebAuthn need no external provider configured), so signup's token was withheld behind the MFA-setup gate instead of returned directly, panicking on the nil access_token type assertion. Failing since a26fa82, unrelated to the session/CVE work in this PR. --disable-mfa keeps WebAuthn/passkey as a primary login method while turning off its MFA-factor role, matching what this scenario needs.
x/net v0.55.0->v0.56.0 fixes CVE-2026-46600 (panic parsing an invalid SVCB/HTTPS RR in dns/dnsmessage). x/text v0.37.0->v0.39.0 fixes CVE-2026-56852 (infinite loop on invalid input). go mod tidy pulled in matching transitive bumps (x/crypto, x/mod, x/sys, x/tools). golang.org/x/crypto flags GO-2026-5932 (openpgp unmaintained, no fix) but we don't import x/crypto/openpgp anywhere and govulncheck already confirms it's unreachable from our code — left as-is.
oauth_mfa_gate.go offered TOTP as an MFA option even when --disable-totp-mfa was set -- every other method checked its own config flag, TOTP didn't. Gate it the same way. login.go's inline email/SMS-OTP MFA challenge required the login identifier to match the enrolled method: a user who signs up with email, later verifies a phone number, and enrolls SMS-OTP as their only factor was never challenged for it on an email+password login -- it fell through to "offer fresh setup" instead of being blocked to verify the factor they already opted into. Not a corner case: any basic-auth signup that later verifies a phone and picks SMS-OTP hits this. The challenge now fires on enrollment alone, sending the code to the account's own stored contact (user.Email/user.PhoneNumber) rather than the login params, which are empty for the non-matching identifier. Also closes coverage gaps for already-correct but unverified behavior: lock_mfa's refusal to lock when a verified email/SMS-OTP fallback exists had zero test coverage, and updates its doc comment (referenced an unlanded "Task 6" that has since shipped).
The OAuth callback redirect only carries mfa_required=1&mfa_methods=... on a withheld gate outcome -- no email/phone, since embedding an identifier in a redirect URL risks referrer leakage to third-party scripts, CDN/proxy access logs, and browser history. But VerifyOTP/SkipMFASetup/LockMFA/resolveOTPSetupCaller (shared by EmailOTPMFASetup/SMSOTPMFASetup) all required an email/phone param to resolve the account before checking the MFA session -- a dead end for an OAuth-return caller, who never has one. Add MemoryStoreProvider.GetMfaSessionOwner(key), a reverse lookup (session key -> userID + purpose) across all three backends, and wire it into each of the four resolvers as a fallback used only when the caller supplies no identifier: resolve the owning user directly from the session, applying the same purpose checks (Verified-only for skip/lock) the identifier path already enforces. ResendOTP gets the same fallback, gated to a Verified session only -- a bare Challenge session still can't spawn further resends without an identifier, preserving the existing account-lockout-DoS guarantee. Every identifier-supplied path is byte-for-byte unchanged; this only adds a new path taken when email and phone are both empty.
VerifyEmail (magic-link click-through and signup email-verification completion) had its own ad-hoc, TOTP-only MFA check instead of the shared resolveMFAGate every other entry point (login.go, signup.go, oauth_mfa_gate.go, webauthn.go) uses. It silently skipped: - MFALockedAt entirely - a locked account could complete a magic-link login with zero MFA challenge. - WebAuthn and email/SMS-OTP as configured MFA factors - only TOTP was ever checked. - EnforceMFA and HasSkippedMFASetupAt - first-time setup was only offered when EnableTOTPLogin happened to be on. - effectiveMFAEnabled's per-user default backfill - it read user.IsMultiFactorAuthEnabled directly instead. Replaced with the same resolveMFAGate call and response construction login.go uses, gated behind the same MFALockedAt check. Regression tests: TestMagicLinkLoginMFAGate (locked account blocked, MFA-offer withholds the token instead of logging in directly).
GORM's naming strategy split "OAuth" into "O"+"Auth" for the schemas.OAuthState struct, so SQL providers (postgres/mysql/sqlite) created authorizer_o_auth_states instead of authorizer_oauth_states, the name every other storage provider already uses. Pin OAuthState.TableName() to schemas.Collections.OAuthState. No migration: the table only holds short-lived OAuth `state` CSRF values consumed within one login round-trip, so existing rows are safe to drop; AutoMigrate creates the correctly named table on next startup. BREAKING CHANGE: SQL installs get a fresh authorizer_oauth_states table; the old authorizer_o_auth_states is left behind and can be dropped manually.
SessionData conflated "when this session token was minted" (IssuedAt,
which legitimately refreshes on every silent rollover) with "when the
user actually authenticated" (auth_time, which must not). Every
silent /authorize continuation and every token refresh reset both to
the same value, so:
- the ID token's auth_time changed on every silent re-authorization
instead of staying fixed at the real login time
- the max_age staleness check compared against the rollover
timestamp, so a client could keep a session alive past max_age
indefinitely by hitting prompt=none faster than the max_age window
Adds a separate AuthTime field to SessionData (falling back to
IssuedAt via EffectiveAuthTime() for pre-fix session cookies), threads
it through all rollover call sites in authorize.go, the code-exchange
and refresh_token paths in token.go, and the GraphQL session resolver,
and adds it to the refresh token's own JWT claims so a refresh doesn't
lose it either. The max_age check now measures against AuthTime.
Caught by the OIDF conformance suite's oidcc-prompt-none-logged-in and
oidcc-max-age-10000 tests (auth_time mismatch across two id_tokens for
the same unbroken session).
authorize's redirect_uri check only validated scheme+host+port against the global AllowedOrigins allowlist, silently dropping the path. Any path under an allowed host — including a suffix appended to another client's registered callback — received a real authorization code. Caught by OIDF conformance test oidcc-ensure-registered-redirect-uri. Clients with registered RedirectURIs now require an exact string match (RFC 6749 §3.1.2.3, OAuth 2.0 Security BCP/RFC 9700). Clients with no registered URIs keep the existing origin-only check.
…client RFC 6749 mandates POST support on the authorization endpoint and per-client refresh token binding; OIDC Core requires aud to identify the actual requesting client. The OpenID Foundation conformance suite caught all of these, plus a JAR-parameter gap and a Basic-auth secret decoding bug that broke on any client_secret containing "!" or similar. - authorize: accept POST (form-urlencoded) alongside GET, exempt CSRF - authorize: reject request/request_uri params with request_not_supported per OIDCC-3.1.2.6 instead of falling through to a confusing error - token: form-urldecode client_secret_basic credentials (RFC 6749 S2.3.1) - token: bind refresh tokens to the client they were issued to (RFC 6749 S6), enforced via the standard aud claim/audience check - token: id_token/access_token/refresh_token aud now identifies the actual OAuth client, not the reserved bootstrap client, for every /authorize and /oauth/token issuance path - token: invalid_grant responses use HTTP 400 per RFC 6749 S5.2 (was 401) - discovery: advertise the "phone" scope (phone_number claims already exist) Verified against the OpenID Foundation conformance suite live, and go test across every supported DB backend (make test-all-db).
…race A security audit of the admin dashboard (Users, Clients, Organizations, Tuples/Model, Webhooks, EmailTemplates, AuditLogs, TrustedIssuers, auth session handling) turned up two real bugs, both fixed here: - Tuples.tsx: creating or deleting an OpenFGA relationship tuple fired the mutation immediately on click, with no confirmation step - including the one-click "Public - all users" pattern (user:*), the single broadest possible grant. Both add and delete now require confirming in a dialog; wildcard grants get an extra explicit warning. - Users.tsx: updateUserList had no guard against out-of-order responses. A fast pagination/search change firing while an earlier request was still in flight could let the earlier response overwrite the list with stale results after the later one resolved. Now guarded by a per-call request id, matching the pattern already used in ViewUserModal. Everything else audited (auth/session cookie handling, CSRF, client secret exposure, org-boundary enforcement, XSS, SSRF) came back clean - see the audit notes for what was checked. One functional (non-security) gap was found separately: org-scoped admins can't use the Organizations page for their own org because the resolver is superadmin-only; tracked for a follow-up. Added aria-labels to icon-only buttons (pagination, tuple revoke) purely so tests could target them - a real, if minor, accessibility fix too.
Organization (singular fetch-by-id) required super-admin, so an org-scoped admin got rejected loading the dashboard's OrganizationDetail page for their own org - the SSO/SCIM sub-cards it also fetches were already org-admin-gated and worked fine, only the top-level org lookup blocked the page. Switched to requireOrgAdmin, the same gating already used by OrgMembers/AddOrgMember, so a different org is still denied. Found during the Phase 6 dashboard audit; Organizations (the list-all page) stays super-admin-only on purpose - browsing/creating/deleting orgs across the instance is not something an org-scoped admin should do.
- login.tsx: render AuthorizerPasskeyLogin above the social/basic-auth block, matching AuthorizerRoot's own reference ordering. It self-hides when WebAuthn isn't supported or the org enforces MFA, so this is a pure addition with no behavior change for anyone who doesn't see it. - New /app/settings page (AuthorizerPasskeyRegister with showCredentials) so a signed-in user can view and add passkeys. Wired into Root.tsx's authenticated route table; linked from the dashboard via "Manage passkeys". Both AuthorizerPasskeyLogin/AuthorizerPasskeyRegister already existed in authorizer-react and are already exported by web/app's installed dependency - this only wires them into the app that wasn't using them yet. Verified end-to-end in a live browser: login page renders the passkey button, dashboard shows the settings link, /app/settings loads directly (hard navigation) and on client-side navigation, and logout still works cleanly.
WebauthnRegistrationOptions/Verify required a bearer token, which doesn't exist yet during a token-withheld MFA offer (mfaGateOfferAll/ BlockEnroll) - so passkey could never actually be offered as a setup method there, despite should_offer_webauthn_mfa_setup already signaling it was available. Add resolveWebauthnSetupCaller: falls back to the MFA session cookie (same pattern as VerifyOTP/SkipMFASetup) when there's no bearer token, gated to the genuine offer/enroll states only - never mfaGateBlockVerify, or a caller who only proved a password could mint a brand-new passkey to bypass challenging an existing second factor. On success via the session path, WebauthnRegistrationVerify now also completes the gate and issues the withheld token, same as totp_mfa_setup + verify_otp(is_totp: true) does for TOTP - so its GraphQL return type changes from Response to AuthResponse. Also close the OAuth/magic-link MFA-redirect gap this branch's earlier work left open: the redirect only ever said mfa_required=1, with no way to tell a first-time offer from a challenge for an already-configured factor, so Root.tsx always rendered the setup screen. EvaluateMFAGateForOAuth now adds mfa_gate=offer|verify, and Root.tsx branches to AuthorizerVerifyOtp for the verify case. web/app/Root.tsx also stops hardcoding passkey: false in the MFA offer it renders.
Contributor
Author
|
Pushed a follow-up commit closing two gaps left open by this PR's earlier work:
Given the new session-cookie auth path on a credential-registration endpoint, flagging for Full Go test suite green, 5 new tests added covering the session path (happy path + the blockVerify bypass guard + missing-session + challenge-session + settings-page-unaffected regression). Passkey-setup-screen fix and email-OTP-via-mailpit flow verified live against a local build; |
2 tasks
This was referenced Jul 21, 2026
lakhansamani
added a commit
that referenced
this pull request
Jul 22, 2026
* chore(storage): bring provider_template to full Provider parity Template only stubbed 60 of 122 storage.Provider methods — files for client, organization, org_membership, org_domain, saml_idp, scim_endpoint, scim_group, trusted_issuer, webauthn_credential, federated_identity, audit_log, and health_check were never added after those features landed on the real providers (#580, #686, #691, #694). make generate-db-template handed contributors a struct that didn't compile against the interface. - add the 12 missing feature files as stubs, matching the existing file-per-feature layout of sql/mongodb - add GetUserByExternalID stub to user.go - add var _ storage.Provider = (*provider)(nil) to provider.go so future interface drift fails the build immediately instead of silently shipping an incomplete template Verified: make generate-db-template dbname=x now produces a package that builds and satisfies storage.Provider out of the box. * fix(storage): move Provider assertion out of provider_template.go var _ storage.Provider = (*provider)(nil) in provider.go created an import cycle the moment a generated provider gets wired into storage.New() per CONTRIBUTING.md step 4: internal/storage would import the new db package, which imported internal/storage right back. Caught by simulating the full generate -> wire -> build flow, not just building the template in isolation. Move the check into interface_test.go as an external _test package (provider_template_test), which can import internal/storage without being part of the same build graph storage.New() pulls in. Verified: - go build ./... clean with a provider actually wired into storage.New() - go test ./internal/storage/db/provider_template/... fails to compile (not just fails at runtime) when a method is removed
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
1 participant
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.
Summary
Redesigns Authorizer's MFA behavior on top of the just-merged EnforceMFA/passkey-gate work (#685):
--disable-webauthn-mfa), folded into theEnableMFAderivation alongside TOTP/Email-OTP/SMS-OTP.email_otp_mfa_setup/sms_otp_mfa_setupmutations, withverify_otpnow marking enrollment complete. Previously-existing login-time OTP branches (which fired for any optional-MFA user with a phone/email, no enrollment check at all) now require a verified enrollment first.effectiveMFAEnabled:IsMultiFactorAuthEnabledis no longer written as an inferred/persisted default at signup or login — it's either an explicit user/admin choice or computed live from server config, every call.skip_mfa_setupreworked from bearer-token to MFA-session-cookie authentication (no bearer token exists yet under the withheld-token model).lock_mfa, refused when a verified Email/SMS OTP fallback exists) and admin recovery (reset_mfaon the existing_update_usermutation — clears all MFA state and deletes every enrolled factor across all 7 storage backends).Design doc:
docs/superpowers/specs/2026-07-14-mfa-behavior-redesign-design.md(local only, gitignored in this repo).Plan:
docs/superpowers/plans/2026-07-14-mfa-behavior-redesign-backend.md.Process
Built via subagent-driven development: 8 plan tasks, each implemented and independently reviewed (task-scoped spec + quality gate); a final whole-branch review across all 9 commits (opus) found no Critical issues but 2 Important findings, both fixed and re-reviewed:
Login()/SignUp()when TOTP was disabled — closed by broadening the gate to run whenever any MFA method is available, not just when TOTP specifically is.5 additional Minor findings (OAuth method-hint parity, defensive event-ordering, a weakened test assertion, missing SMS coverage, a small refactor) were also fixed and re-reviewed.
Known follow-up (not blocking)
Verified-MFA-factor detection (
authenticatorVerified) doesn't yet fold in Email/SMS-OTP forwebauthn.go(any login channel) or forlogin.goin the cross-channel case (e.g. email-primary login with SMS-OTP enrolled as the MFA method) — an enrolled user in these cases is routed to re-enroll instead of verify. Confirmed non-security-bypassing (the token is still withheld either way); worth a follow-up ticket to fold Email/SMS-OTP into everyauthenticatorVerifiedcomputation site, matching what the OAuth gate now does.Test plan
go build ./...,go vet ./...cleaninternal/service,internal/integration_tests,internal/http_handlers,internal/config,internal/graph,internal/cookie— all passingTestEvaluateMFAGateForOAuth, but the actual/oauth_callback/:providerredirect behavior with a live Google/GitHub login has not been driven end-to-end)Update: proto/gRPC/REST parity
The redesign above shipped GraphQL-only. Added as follow-up commits on this same branch:
AuthResponse,Meta,Userproto messages and admin'sUpdateUserRequestwith the fields the GraphQL layer already had (should_offer_*_mfa_*,is_mfa_enforced,has_skipped_mfa_setup_at,mfa_locked_at,reset_mfa).SkipMfaSetup,LockMfa,EmailOtpMfaSetup,SmsOtpMfaSetup) with REST routes auto-generated via the existing gRPC-gateway, mirroringVerifyOtp's exact pattern — pure transport wiring, zerointernal/servicechanges.Meta.is_webauthn_enabled, which predated--disable-webauthn-mfaand was hardcodedtrue.AuthResponse.should_offer_mfa_setup(the deprecated, unqualified field) has no proto counterpart either.