fix(auth): give OIDC/SAML logins a real MFA challenge instead of a hard block (#536)

Fixes #533, found while working on #529/#531.

OIDC and SAML logins hard-blocked on the MFA policy with zero check of whether the user actually enrolled TOTP, and no challenge step at all. Once an admin turned on an MFA-required policy, every SSO user was permanently locked out regardless of enrollment status.

- Extract the post-auth MFA decision (challenge / enrollment-required / proceed) into a shared, unit-tested function so OIDC and SAML can't independently diverge again
- An already-enrolled user now gets a real challenge (reusing the existing, auth-method-agnostic MFA completion flow) instead of being blocked
- An unenrolled user under a required policy gets a distinct, correctly mapped error instead of the old generic one
- Fix a real fail-open regression caught in review: a transient DB error during the enrollment-status check could have silently skipped MFA entirely for an enrolled user; now it fails closed and logs
- Strip the one-time challenge token from the URL after consuming it
This commit is contained in:
SnapOtter
2026-07-16 18:08:24 +08:00
committed by GitHub
parent 190d4c2a00
commit bbfcbe9c82
7 changed files with 591 additions and 22 deletions
+32
View File
@@ -4,6 +4,7 @@ import {
createTotp,
hashRecoveryCodes,
isMfaRequiredForUser,
resolveExternalLoginMfaOutcome,
verifyRecoveryCode,
verifyTotpCode,
} from "../../../apps/api/src/plugins/mfa.js";
@@ -188,4 +189,35 @@ describe("MFA", () => {
expect(isMfaRequiredForUser("admins_only", "user")).toBe(false);
});
});
describe("resolveExternalLoginMfaOutcome", () => {
// Exhaustive over the full input space: 3 policies x 2 roles x 2
// totpEnabled states. Role only matters via isMfaRequiredForUser, which
// branches solely on role === "admin", so {admin, user} covers it.
it.each([
["optional", "user", false, "proceed"],
["optional", "user", true, "challenge"],
["optional", "admin", false, "proceed"],
["optional", "admin", true, "challenge"],
["admins_only", "user", false, "proceed"],
["admins_only", "user", true, "challenge"],
["admins_only", "admin", false, "enrollment_required"],
["admins_only", "admin", true, "challenge"],
// required+user+enrolled is the exact shape of the #533 bug: an
// enrolled non-admin user under a required policy must be challenged,
// not hard-blocked.
["required", "user", false, "enrollment_required"],
["required", "user", true, "challenge"],
["required", "admin", false, "enrollment_required"],
["required", "admin", true, "challenge"],
] as const)("policy=%s role=%s totpEnabled=%s -> %s", (policy, role, totpEnabled, expected) => {
expect(resolveExternalLoginMfaOutcome(policy, role, totpEnabled)).toBe(expected);
});
it("enrollment takes priority: an enrolled user is challenged even under a required policy", () => {
expect(resolveExternalLoginMfaOutcome("required", "admin", true)).not.toBe(
"enrollment_required",
);
});
});
});