feat(mfa): behavior redesign — withheld-token first-time setup, WebAuthn/Email/SMS as MFA factors, lockout + admin recovery#686
Open
lakhansamani wants to merge 45 commits into
Open
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.
TOTP enrollment (secret/QR/recovery-codes) could previously only ever be generated inline as a side effect of the login/signup MFA gate response. An already-authenticated user had no way to add TOTP from account settings the way EmailOTPMFASetup/SMSOTPMFASetup already allow - AuthorizerMFASetup.tsx's own comment describes a "host fetches the enrollment payload" fallback that had no backend/SDK support to build on. Adds TOTPMFASetup (service layer, mirroring EmailOTPMFASetup/ SMSOTPMFASetup's dual-mode resolveOTPSetupCaller auth: bearer token OR MFA session cookie + email/phone_number) plus the totp_mfa_setup GraphQL mutation and resolver wiring. Reuses generateTOTPEnrollment (login.go) and its underlying AuthenticatorProvider.Generate, which already persists the unverified Authenticator row - this endpoint just returns the same enrollment payload shape the login-gate response already uses, no new persistence logic. Tests: full enroll-then-use cycle (setup -> verify_otp -> a later login correctly challenges instead of re-offering), plus the unauthenticated-caller rejection case extended to cover TOTP.
verify_otp unconditionally requires a valid MFA session cookie, but EmailOTPMFASetup/SMSOTPMFASetup/TOTPMFASetup never established one for the bearer-token caller (the realistic settings-screen scenario: an ordinary logged-in user visiting Settings with no leftover session from a login-time MFA gate). Setup itself would succeed, but the immediately-following verify_otp call failed with "invalid session" - enrollment could never actually be completed end to end. Caught by browser end-to-end testing, not the existing Go integration tests - every one of them synthesized its own MFA session directly via MemoryStoreProvider.SetMfaSession before calling verify_otp, which happens to bypass exactly this gap. Fixed those same tests to use the cookie setup itself now returns instead of bypassing it, so they actually prove the fix. resolveOTPSetupCaller now re-arms a fresh 3-minute "Verified" MFA session unconditionally after resolving the caller, regardless of auth mode - uniform behavior, and harmless for the cookie-mode caller (who already had a valid session, just gets it refreshed).
…ubject handleTokenExchangeGrant's revocation check only covered a user subject (RevokedTimestamp) - a service-account subject took a separate branch that never looked the account up at all. validateExchangeToken only checks the JWT's own signature/exp with no session-store lookup, so a still-unexpired token from a just-deactivated service account could keep seeding fresh delegated tokens through a willing downstream agent, extending its effective lifetime past deactivation. Mirrors the user branch: look up the Client by the subject's surrogate ID and reject if !IsActive, same fail-closed contract (a subject we cannot load or confirm active must not seed a delegation). Regression test: mint a service-account subject_token, deactivate the account, confirm the still-valid token is rejected at exchange time.
…ssertion TestTokenExchangeMultiHopDelegation exercises a real 4-hop chain through actual HTTP round-trips (each hop's token is a real JWT that gets re-parsed as the next hop's subject_token) - confirms the nested act chain and progressive scope attenuation both survive the JSON marshal/parse round-trip intact, and that the depth cap rejects exactly at hop 5 (maxActChainDepth=4). Only single-hop delegation had coverage before this; the design doc explicitly describes multi-hop chains as the intended shape. Also fixes a real bug in the existing attenuation_cannot_widen test: it asserted claims["scope"].(string), but the scope claim is a JSON array ([]string encoded via encoding/json), so the type assertion failed silently (empty string, no panic) and the subsequent NotContains check on an always-empty slice passed trivially regardless of the token's actual scope content. The test never verified what its name claims. Added claimScope() to decode it correctly and fixed both call sites.
loadJWKS had no test asserting anything about the cache itself: that two TrustedIssuer rows sharing a JWKS URL get independent cache entries (H7), that a cache hit skips a refetch, and that an evicted entry refetches. All three hold against the current code — closes the coverage gap, no product bug found.
TestSAML_WrongRecipientRejected varied Recipient only, leaving Destination at its correct default, so a broken/missing Destination check would have passed unnoticed. Add TestSAML_WrongDestinationRejected to close that gap. Audited CSRF ACS exemption scope, signature/expiry/InResponseTo/ Destination validation, and the replay guard per task-2 brief; no code defect found. See .superpowers/sdd/task-2-report.md.
…users SCIM deactivate() (and account deactivation) stamp RevokedTimestamp and synchronously wipe memory-store sessions, but a failed/missed session delete on an instance was never caught elsewhere: login and refresh_token already re-check RevokedTimestamp as defense-in-depth, but the general request-serving auth path (ValidateAccessToken/ValidateBrowserSession, used by every GraphQL/REST resolver and the gRPC interceptor) relied solely on the session-store lookup, with no DB fallback. Add the same fail-open (demote-only) RevokedTimestamp re-check there, mirroring introspect.go's existing pattern. Requires threading StorageProvider into the token package as a new dependency. Add a regression test that isolates this path from the pre-existing session-deletion coverage by revoking via RevokedTimestamp only (no DeleteAllUserSessions) and confirming a held access token is rejected.
web/app (the bundled universal login UI at /app) was pinned to authorizer-react@2.2.0-rc.0, predating every MFA fix from this session's audit (hasCodeFactor regression, TOTP settings-mode session-arming, locked-account screen, OAuth mfa_required redirect detection). Points at authorizer-js/authorizer-react's main branches instead, now that both carry the fixes and both build cleanly from a fresh git-based install (verified via a standalone clone + npm install of each, and a `web/app` build served through this backend's live /app route).
GET /verify_email is the literal URL the "verify your email" and magic-link-login emails send users to (utils.GetEmailVerificationURL) - a separate implementation from the GraphQL verify_email mutation, which already went through resolveMFAGate. This REST handler issued full access/refresh/ID tokens unconditionally: no MFA gate check, no RevokedTimestamp check. A user could complete signup email verification or magic-link login and walk away with working tokens regardless of MFA enrollment or account revocation - the GraphQL-side fix earlier in this branch never covered this path since it's not the same code. Reuses EvaluateMFAGateForOAuth (already proven at oauth_callback.go) for the gate-then-redirect-with-mfa_required shape, and adds the missing RevokedTimestamp check mirroring every other login entry point. Added TestVerifyEmailRESTEndpointMFAGate, which hits the real HTTP route (now registered in the integration test harness - it previously only exposed /graphql, which is exactly why this bypass had no test coverage despite three prior MFA-audit passes on this branch). Verified non-vacuous: reverting the handler fix makes the gate/revocation subtests fail.
web/app composes login primitives directly in login.tsx/signup.tsx rather than using authorizer-react's AuthorizerRoot wrapper, so it never picked up parseMfaRedirectParams/AuthorizerMFASetup handling for the mfa_required redirect - the mechanism the backend uses whenever a login completes via a redirect rather than an inline SDK call response. That's three flows, not one: the OAuth /authorize continuation this was originally built for, and (per the previous commit's fix) the magic-link and signup-email-verification click-through URLs, which now redirect the same way. Before this fix, all three landed on a login/signup screen with no way to complete MFA - a dead end for any deployment relying on the default bundled login UI, given MFA is on by default. Mirrors AuthorizerRoot.tsx's existing, proven handling exactly. Verified against the live dev server: /app?mfa_required=1&mfa_methods=totp,email_otp renders the MFA setup screen with the right methods offered.
Same bug already found and fixed once for the authenticator (user_id,
method) index: the driver silently rejects a multi-key bson.M for a
compound index's Keys ("multi-key map passed in for ordered parameter
keys"), so CreateIndexes never actually created it - the fix wasn't
applied to the other three compound indexes in this file.
verification_requests(email, identifier) is the one that mattered:
declared unique but never enforced, so MongoDB silently accumulated
duplicate pending verification requests for the same identity (SQL
avoids this via an explicit upsert-on-conflict; every non-SQL backend
either lacks the constraint or, for Mongo, declared but never created
it). session_token/mfa_session(user_id, key_name) were non-unique -
performance-only, fixed for consistency with the same pattern.
Added a regression assertion to the shared cross-backend test, branched
per backend since the correct behavior differs: SQL upserts in place,
Mongo must now reject a genuine duplicate, and the four backends with
neither behavior yet (Cassandra/DynamoDB/ArangoDB/Couchbase) are a
known, separate, not-fixed-here gap.
Also adds .github/workflows/storage-matrix.yml: CI only ever ran `make
test` (SQLite-only) despite a genuine 2000+ line parameterized suite
(provider_test.go) covering all 6 other backends via `make test-all-db`
- nothing in .github/workflows ever invoked it, so this bug (and
whatever else it would have caught) had zero CI protection. New
workflow runs on schedule, manual dispatch, and PRs touching storage
code; kept off the general PR path since it's slow (multiple DB
containers). Verified end-to-end: make test-all-db passes clean across
couchbase/postgres/sqlite/mongodb/arangodb/scylladb/dynamodb.
SSOLoginHandler checks org.Enabled only at dispatch time (resolveActiveOIDCConnection). SSOCallbackHandler re-fetches the connection by ID directly and checked conn.IsActive but never the owning org's Enabled flag. An admin disabling an org during the state TTL window (~30-90s) let an in-flight login still complete and mint a session. Re-check org.Enabled via conn.OrgID right after the connection lookup, before secret decrypt or any upstream call - mirrors the existing org_discovery.go / saml_sp.go org-gate idiom. jitProvisionFederatedUser's returning-user path already rejects RevokedTimestamp users; added regression coverage since none existed. Also adds missing SSOLoginHandler coverage (unknown org, disabled org, no connection, missing redirect_uri all reject without redirecting to the IdP).
org_scoped_admin_test.go's confused-deputy matrix never exercised inbound SCIM (GET/PUT/PATCH/DELETE /scim/v2/Users/:id, filter probe on /scim/v2/Users) — only unit tests with a fake service existed. Add a real HTTP round-trip test: two orgs, two SCIM tokens, org A's token attempts every verb against org B's provisioned user. requireMember's GetOrgMembership(orgID, userID) gate already blocks all of it (verified by temporarily bypassing the gate — every case flipped from 404 to 200/204 and org B's user got deactivated). No product bug; closes the test-coverage gap the design doc claimed but never proved.
…skey login Final whole-branch review found the exact bug class this session kept finding in sibling code paths, still alive in two more: both service.VerifyEmail (backs the GraphQL verify_email mutation AND the gRPC handler) and WebauthnLoginVerify computed authenticatorVerified from TOTP/WebAuthn only, completely omitting Email-OTP and SMS-OTP. A user whose only enrolled factor was Email-OTP or SMS-OTP, with HasSkippedMFASetupAt set (reachable: skip while unenrolled, then later enroll Email/SMS-OTP via settings without touching TOTP/WebAuthn), resolved to mfaGateSkippedSetup on these two paths and got a full token with zero MFA challenge - despite login.go correctly challenging the identical account on a password login. login.go's own equivalent gate is correct only because it has dedicated early-return branches that actively send the Email/SMS-OTP code before ever reaching its TOTP-only resolveMFAGate call; verify_email.go and webauthn.go had no such branches at all. Ports login.go's exact pattern into both: send the code and return the matching ShouldShow*OtpScreen hint before falling through to the TOTP/WebAuthn gate, which is now correctly scoped (reaching it means neither email nor SMS-OTP is the user's factor). webauthn.go's gate uses effectiveMFAEnabled alone, not the additional cfg.EnableMFA login.go requires - matches its own pre-existing TOTP gate immediately below, which already supports pure per-user MFA opt-in independent of the global flag. Also stops discarding the MongoDB verification_requests(email, identifier) unique-index creation error: a database that already accumulated duplicates from the earlier bson.M bug will fail this index build and silently stay unprotected forever without a loud signal telling an operator to dedupe. New tests verified non-vacuous by reverting the fix and confirming both fail on pre-fix code.
spaBuildCacheMiddleware only exempted "index.js"/"main.css" from the year-long immutable cache branch. web/dashboard's Vite config emits main.css, matching the check - but web/app's emits index.css (its assetFileNames names the CSS after the entry chunk, not a fixed "main"), so it silently fell into the immutable branch. Any CSS fix shipped to web/app - like a layout bug - would leave a browser that had already visited once stuck on the stale, broken stylesheet for up to a year, deploy notwithstanding. Found by reproducing a user-reported misaligned MFA setup screen: a hard refresh fixed it in my test tab, which pointed straight at the cache header rather than the component markup/CSS itself (already correct).
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.