feat(mfa): optional MFA setup, passkey as a second factor, and close the EnforceMFA passkey bypass#685
Merged
Merged
Conversation
…up when MFA isn't enforced
login.go's TOTP branch switches on resolveMFAGate's 5-way decision. mfaGateBlockVerify/mfaGateBlockEnroll must never reach the AccessToken code path, but no test exercised the switch wiring itself (only the pure decision function in mfa_gate_test.go). Add TestLoginMFAGateTokenWithholding, one subtest per gate outcome, driven through the real service.Provider.Login via GraphQLProvider.Login. Verified each block case actually fails the test when its `return` is removed, then reverted the drill.
Authenticated users can now decline the optional MFA setup prompt shown at login. Server re-checks EnforceMFA before touching the caller token, so a client can never forge this call to bypass org-enforced MFA. Covers happy path (sets HasSkippedMFASetupAt, quiets next login) and the EnforceMFA rejection path (FailedPrecondition even with a valid, freshly-verified TOTP token) via integration test.
An unauthenticated caller got FailedPrecondition when EnforceMFA was true but Unauthenticated when false, leaking org-wide MFA enforcement via response code alone with zero credentials required. Check authentication first; EnforceMFA is still re-checked before any state mutation, so HasSkippedMFASetupAt is never set while it's true. Add coverage for a fully credential-less caller under both EnforceMFA states, proving the reorder closed the leak.
WebauthnLoginVerify issued tokens unconditionally, ignoring EnforceMFA entirely — a user could always bypass password + MFA verification just by using their passkey instead. Reuse the same TOTP-verify/enrollment gate login.go's password path already has, when EnforceMFA is on for a user who has MFA individually enabled.
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
Three layered pieces of MFA work, landed together:
--enforce-mfakeeps working exactly as before — no token until MFA is complete, never skippable.webauthn_login_options/webauthn_login_verifyflow and MFA-session-cookie mechanism rather than adding new mutations.EnforceMFAcheck at all — letting a user skip password + MFA verification entirely.webauthn_login_verifynow reuses the same TOTP-verify/enrollment gate the password path already has when MFA is enforced for that user.Test plan
go build ./...andgo vet ./...cleanTestResolveMFAGate— 9 subtests, pure decision-table coverageTestLoginMFAGateTokenWithholding— 7 subtests covering all gate outcomes through the real login path, including TOTP-only, passkey-only, and dual-enrolled usersTestWebauthnLoginVerifyEnforceMFA— 5 subtests covering EnforceMFA on/off, TOTP verified/not-enrolled, and TOTP-disabled-server-wideTestMeta— reflectsis_mfa_enforcedskip_mfa_setupmutation integration tests, including the auth-before-EnforceMFA ordering fixinternal/integration_tests+internal/servicesuites pass with zero regressions