feat(mfa): behavior redesign — withheld-token first-time setup, WebAuthn/Email/SMS as MFA factors, lockout + admin recovery#686
Open
lakhansamani wants to merge 20 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.
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.