diff --git a/apps/login/src/app/(login)/mfa/set/page.tsx b/apps/login/src/app/(login)/mfa/set/page.tsx index 2fd9b5e20c..f4d48c00da 100644 --- a/apps/login/src/app/(login)/mfa/set/page.tsx +++ b/apps/login/src/app/(login)/mfa/set/page.tsx @@ -27,7 +27,7 @@ export async function generateMetadata(): Promise { return { title: t("set.title") }; } -function isSessionValid(session: Partial): { +function isSessionValidForMfaSet(session: Partial): { valid: boolean; verifiedAt?: Timestamp; } { @@ -111,7 +111,7 @@ export default async function Page(props: { searchParams: Promise void }) { @@ -33,7 +33,7 @@ export function SessionClearItem({ session, reload }: { session: Session; reload return response; } - const { valid, verifiedAt } = isSessionValid(session); + const { valid, verifiedAt } = isSessionPrimaryFactorAndLifetimeValid(session); const [_error, setError] = useState(null); diff --git a/apps/login/src/components/session-item.tsx b/apps/login/src/components/session-item.tsx index 77b0858d99..f6e85f98f5 100644 --- a/apps/login/src/components/session-item.tsx +++ b/apps/login/src/components/session-item.tsx @@ -15,7 +15,7 @@ import { AutoSubmitForm } from "./auto-submit-form"; import { Avatar } from "./avatar"; import { Translated } from "./translated"; -export function isSessionValid(session: Partial): { +export function isSessionPrimaryFactorAndLifetimeValid(session: Partial): { valid: boolean; verifiedAt?: Timestamp; } { @@ -53,7 +53,7 @@ export function SessionItem({ session, reload, requestId }: { session: Session; return response; } - const { valid, verifiedAt } = isSessionValid(session); + const { valid, verifiedAt } = isSessionPrimaryFactorAndLifetimeValid(session); const [samlData, setSamlData] = useState<{ url: string; fields: Record } | null>(null); const [_error, setError] = useState(null); diff --git a/apps/login/src/lib/server/session.ts b/apps/login/src/lib/server/session.ts index 60f813a602..f694283ae6 100644 --- a/apps/login/src/lib/server/session.ts +++ b/apps/login/src/lib/server/session.ts @@ -25,7 +25,9 @@ import { removeSessionFromCookie, } from "../cookies"; import { getServiceConfig } from "../service-url"; +import { isSessionValid } from "../session"; import { getPublicHost } from "./host"; +import { sendLoginname } from "./loginname"; const logger = createLogger("session"); @@ -79,9 +81,39 @@ export async function continueWithSession({ requestId, ...session }: ContinueWit const t = await getTranslations("error"); - const loginSettings = await getLoginSettings({ serviceConfig, organization: session.factors?.user?.organizationId }); + if (!session.factors?.user) { + return { error: t("couldNotContinueSession") }; + } - if (requestId && session.id && session.factors?.user) { + const loginSettings = await getLoginSettings({ serviceConfig, organization: session.factors.user.organizationId }); + + // Validate session (including MFA) before completing the flow + const valid = await isSessionValid({ serviceConfig, session: session as Session }); + + if (!valid) { + logger.warn("continueWithSession: session is not valid (e.g. MFA not completed), redirecting to re-authenticate", { + sessionId: session.id, + }); + + // Redirect user to re-authenticate (will route to MFA page if password is still valid) + const res = await sendLoginname({ + loginName: session.factors.user.loginName, + organization: session.factors.user.organizationId, + requestId: requestId, + }); + + if (res && "redirect" in res && res.redirect) { + return { redirect: res.redirect }; + } + + if (res && "samlData" in res && res.samlData) { + return { samlData: res.samlData }; + } + + return { error: t("couldNotContinueSession") }; + } + + if (requestId && session.id) { return completeFlowOrGetUrl( { sessionId: session.id, @@ -90,18 +122,15 @@ export async function continueWithSession({ requestId, ...session }: ContinueWit }, loginSettings?.defaultRedirectUri, ); - } else if (session.factors?.user) { - return completeFlowOrGetUrl( - { - loginName: session.factors.user.loginName, - organization: session.factors.user.organizationId, - }, - loginSettings?.defaultRedirectUri, - ); } - // Fallback error if we couldn't determine where to redirect - return { error: t("couldNotContinueSession") }; + return completeFlowOrGetUrl( + { + loginName: session.factors.user.loginName, + organization: session.factors.user.organizationId, + }, + loginSettings?.defaultRedirectUri, + ); } export type UpdateSessionCommand = { diff --git a/apps/login/src/lib/session.test.ts b/apps/login/src/lib/session.test.ts index e64f4f59ed..c24b561c99 100644 --- a/apps/login/src/lib/session.test.ts +++ b/apps/login/src/lib/session.test.ts @@ -376,18 +376,22 @@ describe("isSessionValid", () => { vi.mocked(verifyHelperModule.shouldEnforceMFA).mockReturnValue(false); + // User has no MFA methods configured + vi.mocked(zitadelModule.listAuthenticationMethodTypes).mockResolvedValue({ + authMethodTypes: [AuthenticationMethodType.PASSWORD], + } as any); + const result = await isSessionValid({ serviceConfig: { baseUrl: mockServiceUrl }, session }); expect(result).toBe(true); }); - test("should return true when user has PASSWORD and TOTP configured but only password verified and MFA not required", async () => { - // This test specifically covers the original bug scenario: - // - User has PASSWORD and TOTP configured (would show up in listAuthenticationMethodTypes) - // - User has only verified password, not TOTP - // - MFA is not required by policy - // - Session should be valid (this was the bug - it was returning false) + test("should return false when user has PASSWORD and TOTP configured but only password verified and MFA not required by policy", async () => { + // User has TOTP configured as an MFA method. + // Even though MFA is not required by policy, the user's configured MFA + // factor must be verified because they have it set up. + const consoleSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); const verifiedTimestamp = createMockTimestamp(); const session = createMockSession({ factors: { @@ -401,8 +405,7 @@ describe("isSessionValid", () => { password: { verifiedAt: verifiedTimestamp, }, - // TOTP is configured but NOT verified - this is the key part - // totp: undefined (no verifiedAt) + // TOTP is configured but NOT verified }, }); @@ -413,9 +416,16 @@ describe("isSessionValid", () => { vi.mocked(verifyHelperModule.shouldEnforceMFA).mockReturnValue(false); + // User has TOTP configured + vi.mocked(zitadelModule.listAuthenticationMethodTypes).mockResolvedValue({ + authMethodTypes: [AuthenticationMethodType.PASSWORD, AuthenticationMethodType.TOTP], + } as any); + const result = await isSessionValid({ serviceConfig: { baseUrl: mockServiceUrl }, session }); - expect(result).toBe(true); + expect(result).toBe(false); + expect(consoleSpy).toHaveBeenCalledWith("[Session] MFA is required but not valid"); + consoleSpy.mockRestore(); }); test("should return false when user has PASSWORD and TOTP configured but only password verified and MFA IS required", async () => { @@ -459,10 +469,8 @@ describe("isSessionValid", () => { }); test("REGRESSION TEST: user with only PASSWORD factor should be valid when MFA not required", async () => { - // This test specifically verifies the original bug is fixed - // Original bug: A user with only PASSWORD authentication would be invalid - // because the code checked if authMethods.length > 0 (which included PASSWORD) - // and then required MFA verification even when MFA was not required by policy + // User has only PASSWORD configured, no MFA methods at all. + // Session should be valid since no MFA factors need verification. const verifiedTimestamp = createMockTimestamp(); const session = createMockSession({ @@ -493,20 +501,23 @@ describe("isSessionValid", () => { vi.mocked(verifyHelperModule.shouldEnforceMFA).mockReturnValue(false); + // User has only PASSWORD, no MFA methods + vi.mocked(zitadelModule.listAuthenticationMethodTypes).mockResolvedValue({ + authMethodTypes: [AuthenticationMethodType.PASSWORD], + } as any); + const result = await isSessionValid({ serviceConfig: { baseUrl: mockServiceUrl }, session }); - // This should be true - if it's false, the original bug still exists + // This should be true - user has no MFA methods configured expect(result).toBe(true); }); - test("DEMONSTRATION: how the original bug would manifest with old logic", async () => { - // This test demonstrates the original problematic scenario: - // 1. listAuthenticationMethodTypes returns [PASSWORD, TOTP] - // 2. Old logic would check authMethods.length > 0 (true because PASSWORD is included) - // 3. Old logic would then require MFA verification regardless of policy - // 4. User has only password verified, no TOTP - // 5. Session would be marked invalid even though MFA is not required + test("should return false when user has TOTP configured but not verified, even if MFA not required by policy", async () => { + // User has TOTP configured as an MFA method. + // Even though MFA is not required by policy, the configured MFA + // factor must be verified. + const consoleSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); const verifiedTimestamp = createMockTimestamp(); const session = createMockSession({ factors: { @@ -533,10 +544,58 @@ describe("isSessionValid", () => { vi.mocked(verifyHelperModule.shouldEnforceMFA).mockReturnValue(false); + // User has TOTP configured + vi.mocked(zitadelModule.listAuthenticationMethodTypes).mockResolvedValue({ + authMethodTypes: [AuthenticationMethodType.PASSWORD, AuthenticationMethodType.TOTP], + } as any); + + const result = await isSessionValid({ serviceConfig: { baseUrl: mockServiceUrl }, session }); + + // With the new logic, MFA must be verified if user has MFA methods + expect(result).toBe(false); + expect(consoleSpy).toHaveBeenCalledWith("[Session] MFA is required but not valid"); + consoleSpy.mockRestore(); + }); + + test("should return true when user has TOTP configured and verified, even if MFA not required by policy", async () => { + // User has TOTP configured as an MFA method and it is verified on the session. + // Even though MFA is not required by policy, the configured MFA factor + // is verified so the session should be valid. + + const verifiedTimestamp = createMockTimestamp(); + const session = createMockSession({ + factors: { + user: { + id: mockUserId, + organizationId: mockOrganizationId, + loginName: "test@example.com", + displayName: "Test User", + verifiedAt: verifiedTimestamp, + }, + password: { + verifiedAt: verifiedTimestamp, + }, + totp: { + verifiedAt: verifiedTimestamp, + }, + }, + }); + + // MFA is NOT required by policy + vi.mocked(zitadelModule.getLoginSettings).mockResolvedValue({ + forceMfa: false, + forceMfaLocalOnly: false, + } as any); + + vi.mocked(verifyHelperModule.shouldEnforceMFA).mockReturnValue(false); + + // User has TOTP configured + vi.mocked(zitadelModule.listAuthenticationMethodTypes).mockResolvedValue({ + authMethodTypes: [AuthenticationMethodType.PASSWORD, AuthenticationMethodType.TOTP], + } as any); + const result = await isSessionValid({ serviceConfig: { baseUrl: mockServiceUrl }, session }); - // With our fix, this should be true (session is valid) - // With the old logic, this would have been false (bug) expect(result).toBe(true); }); }); @@ -984,7 +1043,8 @@ describe("isSessionValid", () => { }); }); - test("should return true when authenticated with IDP intent and forceMfaLocalOnly (IDP bypasses local-only MFA)", async () => { + test("should return false when authenticated with IDP intent and forceMfaLocalOnly but user has unverified MFA methods", async () => { + const consoleSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); const verifiedTimestamp = createMockTimestamp(); const session = createMockSession({ factors: { @@ -1017,14 +1077,17 @@ describe("isSessionValid", () => { const result = await isSessionValid({ serviceConfig: { baseUrl: mockServiceUrl }, session }); - expect(result).toBe(true); + // MFA methods must be verified regardless of policy + expect(result).toBe(false); expect(verifyHelperModule.shouldEnforceMFA).toHaveBeenCalledWith(session, expect.any(Object)); expect(zitadelModule.getLoginSettings).toHaveBeenCalledWith({ serviceConfig: { baseUrl: mockServiceUrl }, organization: mockOrganizationId, }); - // Should not call listAuthenticationMethodTypes since shouldEnforceMFA returned false - expect(zitadelModule.listAuthenticationMethodTypes).not.toHaveBeenCalled(); + // listAuthenticationMethodTypes is always called now + expect(zitadelModule.listAuthenticationMethodTypes).toHaveBeenCalled(); + expect(consoleSpy).toHaveBeenCalledWith("[Session] MFA is required but not valid"); + consoleSpy.mockRestore(); }); test("should return true when authenticated with IDP intent and MFA required and satisfied", async () => { @@ -1065,7 +1128,8 @@ describe("isSessionValid", () => { }); describe("passkey authentication", () => { - test("should return true when authenticated with passkey and MFA required (passkey satisfies MFA)", async () => { + test("should return false when authenticated with passkey but user has unverified MFA methods", async () => { + const consoleSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); const verifiedTimestamp = createMockTimestamp(); const session = createMockSession({ factors: { @@ -1079,7 +1143,7 @@ describe("isSessionValid", () => { webAuthN: { verifiedAt: verifiedTimestamp, }, - // No password factor, no additional MFA factors + // No password factor, no additional MFA factors verified }, }); @@ -1091,21 +1155,24 @@ describe("isSessionValid", () => { forceMfaLocalOnly: false, } as any); - // User has MFA methods configured but none verified (passkey should satisfy MFA) + // User has TOTP configured but not verified vi.mocked(zitadelModule.listAuthenticationMethodTypes).mockResolvedValue({ authMethodTypes: [AuthenticationMethodType.TOTP], } as any); const result = await isSessionValid({ serviceConfig: { baseUrl: mockServiceUrl }, session }); - expect(result).toBe(true); + // MFA methods must be verified regardless of policy or passkey + expect(result).toBe(false); expect(verifyHelperModule.shouldEnforceMFA).toHaveBeenCalledWith(session, expect.any(Object)); expect(zitadelModule.getLoginSettings).toHaveBeenCalledWith({ serviceConfig: { baseUrl: mockServiceUrl }, organization: mockOrganizationId, }); - // Should not call listAuthenticationMethodTypes since shouldEnforceMFA returned false - expect(zitadelModule.listAuthenticationMethodTypes).not.toHaveBeenCalled(); + // listAuthenticationMethodTypes is always called now + expect(zitadelModule.listAuthenticationMethodTypes).toHaveBeenCalled(); + expect(consoleSpy).toHaveBeenCalledWith("[Session] MFA is required but not valid"); + consoleSpy.mockRestore(); }); }); diff --git a/apps/login/src/lib/session.ts b/apps/login/src/lib/session.ts index b43f1446d3..f20cf0fabc 100644 --- a/apps/login/src/lib/session.ts +++ b/apps/login/src/lib/session.ts @@ -80,43 +80,38 @@ export async function isSessionValid({ // Use the existing shouldEnforceMFA function to determine if MFA is required const isMfaRequired = shouldEnforceMFA(session, loginSettings); - // Only enforce MFA validation if MFA is required by policy - if (isMfaRequired) { - const authMethodTypes = await listAuthenticationMethodTypes({ serviceConfig, userId: session.factors.user.id }); + // Always check auth methods to see if the user has MFA factors configured + const authMethodTypes = await listAuthenticationMethodTypes({ serviceConfig, userId: session.factors.user.id }); - const authMethods = authMethodTypes.authMethodTypes; - // Filter to only MFA methods (exclude PASSWORD and PASSKEY) - const mfaMethods = authMethods?.filter( - (method) => - method === AuthenticationMethodType.TOTP || - method === AuthenticationMethodType.OTP_EMAIL || - method === AuthenticationMethodType.OTP_SMS || - method === AuthenticationMethodType.U2F, - ); + const authMethods = authMethodTypes.authMethodTypes; + // Filter to only MFA methods (exclude PASSWORD and PASSKEY) + const mfaMethods = authMethods?.filter( + (method) => + method === AuthenticationMethodType.TOTP || + method === AuthenticationMethodType.OTP_EMAIL || + method === AuthenticationMethodType.OTP_SMS || + method === AuthenticationMethodType.U2F, + ); - if (mfaMethods && mfaMethods.length > 0) { - // Check if any of the configured MFA methods have been verified - const totpValid = mfaMethods.includes(AuthenticationMethodType.TOTP) && !!session.factors.totp?.verifiedAt; - const otpEmailValid = - mfaMethods.includes(AuthenticationMethodType.OTP_EMAIL) && !!session.factors.otpEmail?.verifiedAt; - const otpSmsValid = mfaMethods.includes(AuthenticationMethodType.OTP_SMS) && !!session.factors.otpSms?.verifiedAt; - const u2fValid = mfaMethods.includes(AuthenticationMethodType.U2F) && !!session.factors.webAuthN?.verifiedAt; + if (mfaMethods && mfaMethods.length > 0) { + // User has MFA methods configured — they must be verified regardless of policy + const totpValid = mfaMethods.includes(AuthenticationMethodType.TOTP) && !!session.factors.totp?.verifiedAt; + const otpEmailValid = mfaMethods.includes(AuthenticationMethodType.OTP_EMAIL) && !!session.factors.otpEmail?.verifiedAt; + const otpSmsValid = mfaMethods.includes(AuthenticationMethodType.OTP_SMS) && !!session.factors.otpSms?.verifiedAt; + const u2fValid = mfaMethods.includes(AuthenticationMethodType.U2F) && !!session.factors.webAuthN?.verifiedAt; - mfaValid = totpValid || otpEmailValid || otpSmsValid || u2fValid; - } else { - // No specific MFA methods configured, but MFA is forced - check for any verified MFA factors - // (excluding IDP which should be handled separately) - const otpEmail = session.factors.otpEmail?.verifiedAt; - const otpSms = session.factors.otpSms?.verifiedAt; - const totp = session.factors.totp?.verifiedAt; - const webAuthN = session.factors.webAuthN?.verifiedAt; - // Note: Removed IDP (session.factors.intent?.verifiedAt) as requested + mfaValid = totpValid || otpEmailValid || otpSmsValid || u2fValid; + } else if (isMfaRequired) { + // No MFA methods configured, but MFA is forced by policy — check for any verified MFA factors + const otpEmail = session.factors.otpEmail?.verifiedAt; + const otpSms = session.factors.otpSms?.verifiedAt; + const totp = session.factors.totp?.verifiedAt; + const webAuthN = session.factors.webAuthN?.verifiedAt; - mfaValid = !!(otpEmail || otpSms || totp || webAuthN); - } + mfaValid = !!(otpEmail || otpSms || totp || webAuthN); } - // If MFA is not required by policy, mfaValid remains true + // If user has no MFA methods and MFA is not required by policy, mfaValid remains true const stillValid = session.expirationDate ? timestampDate(session.expirationDate).getTime() > new Date().getTime() : true;