From b6cbb684287e755873c6cc5ede4276cb902feabe Mon Sep 17 00:00:00 2001 From: Max Peintner Date: Thu, 11 Sep 2025 10:36:39 +0200 Subject: [PATCH] fix(login): MFA session validation to support multiple authentication methods (#10610) Fixed an issue in `isSessionValid()` where users with multiple configured MFA methods (e.g., TOTP and U2F) would have their sessions incorrectly invalidated. The function previously used exclusive if-else logic that only checked the first matching method, causing validation to fail even when other configured methods were successfully verified. Closes #10529 # Which Problems Are Solved [#10529](https://github.com/zitadel/zitadel/issues/10529) # How the Problems Are Solved - Replaced exclusive if-else if chain with inclusive validation logic - Session is now considered valid if ANY configured MFA method has been verified - Improved error logging to show all configured methods and their verification status Example: A user with both TOTP and U2F configured can now successfully authenticate using either method, whereas previously the session would be invalid if they used U2F but TOTP was checked first. --- apps/login/src/lib/session.test.ts | 685 +++++++++++++++++++++++++++++ apps/login/src/lib/session.ts | 33 +- 2 files changed, 700 insertions(+), 18 deletions(-) create mode 100644 apps/login/src/lib/session.test.ts diff --git a/apps/login/src/lib/session.test.ts b/apps/login/src/lib/session.test.ts new file mode 100644 index 0000000000..80ca32854f --- /dev/null +++ b/apps/login/src/lib/session.test.ts @@ -0,0 +1,685 @@ +/** + * Unit tests for the isSessionValid function. + * + * This test suite covers the comprehensive session validation logic including: + * - Session expiration checks + * - User presence validation + * - Authentication factor verification (password, passkey, IDP) + * - MFA validation with configured authentication methods (TOTP, OTP Email/SMS, U2F) + * - MFA validation with login settings (forceMfa, forceMfaLocalOnly) + * - Email verification when EMAIL_VERIFICATION environment variable is enabled + * - Edge cases like sessions without expiration date + */ + +import { timestampDate } from "@zitadel/client"; +import { AuthenticationMethodType } from "@zitadel/proto/zitadel/user/v2/user_service_pb"; +import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; +import { isSessionValid } from "./session"; +import * as zitadelModule from "./zitadel"; + +// Mock the zitadel client timestampDate function +vi.mock("@zitadel/client", () => ({ + timestampDate: vi.fn(), +})); + +// Mock the zitadel module +vi.mock("./zitadel", () => ({ + listAuthenticationMethodTypes: vi.fn(), + getLoginSettings: vi.fn(), + getUserByID: vi.fn(), +})); + +// Mock environment variables +const originalEnv = process.env; + +describe("isSessionValid", () => { + const mockServiceUrl = "https://zitadel-abc123.zitadel.cloud"; + const mockUserId = "test-user-id"; + const mockOrganizationId = "test-org-id"; + + beforeEach(() => { + vi.clearAllMocks(); + process.env = { ...originalEnv }; + // @ts-ignore - delete is OK for test environment variables + delete process.env.EMAIL_VERIFICATION; + + // Setup timestampDate mock to return valid dates + vi.mocked(timestampDate).mockImplementation((timestamp: any) => { + if (!timestamp || !timestamp.seconds) { + return new Date(); // Return current date for invalid timestamps + } + return new Date(Number(timestamp.seconds) * 1000); + }); + }); + + afterEach(() => { + process.env = originalEnv; + }); + + const createMockTimestamp = (offsetMs = 3600000): any => ({ + seconds: BigInt(Math.floor((Date.now() + offsetMs) / 1000)), + }); + + const createMockSession = (overrides: any = {}): any => { + const futureTimestamp = createMockTimestamp(); + + const defaultSession = { + id: "session-id", + expirationDate: futureTimestamp, + factors: { + user: { + id: mockUserId, + organizationId: mockOrganizationId, + loginName: "test@example.com", + displayName: "Test User", + verifiedAt: futureTimestamp, + }, + password: { + verifiedAt: futureTimestamp, + }, + }, + ...overrides, + }; + + return defaultSession; + }; + + describe("when session has no user", () => { + test("should return false and log warning", async () => { + const consoleSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + const session = createMockSession({ + factors: undefined, + }); + + const result = await isSessionValid({ serviceUrl: mockServiceUrl, session }); + + expect(result).toBe(false); + expect(consoleSpy).toHaveBeenCalledWith("Session has no user"); + consoleSpy.mockRestore(); + }); + }); + + describe("when session is expired", () => { + test("should return false and log warning", async () => { + const consoleSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + const session = createMockSession({ + expirationDate: createMockTimestamp(-3600000), // 1 hour ago + }); + + vi.mocked(zitadelModule.listAuthenticationMethodTypes).mockResolvedValue({ + authMethodTypes: [], + } as any); + + const result = await isSessionValid({ serviceUrl: mockServiceUrl, session }); + + expect(result).toBe(false); + expect(consoleSpy).toHaveBeenCalledWith( + "Session is expired", + expect.any(String) + ); + consoleSpy.mockRestore(); + }); + }); + + describe("when session has no valid authentication factors", () => { + test("should return false when no password, passkey, or IDP verification", async () => { + const session = createMockSession({ + factors: { + user: { + id: mockUserId, + organizationId: mockOrganizationId, + loginName: "test@example.com", + displayName: "Test User", + verifiedAt: createMockTimestamp(), + }, + // No password, webAuthN, or intent factors + }, + }); + + vi.mocked(zitadelModule.listAuthenticationMethodTypes).mockResolvedValue({ + authMethodTypes: [], + } as any); + + const result = await isSessionValid({ serviceUrl: mockServiceUrl, session }); + + expect(result).toBe(false); + }); + }); + + describe("MFA validation with configured authentication methods", () => { + test("should return true when TOTP is configured and verified", async () => { + 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, + }, + }, + }); + + vi.mocked(zitadelModule.listAuthenticationMethodTypes).mockResolvedValue({ + authMethodTypes: [AuthenticationMethodType.TOTP], + } as any); + + const result = await isSessionValid({ serviceUrl: mockServiceUrl, session }); + + expect(result).toBe(true); + }); + + test("should return false when TOTP is configured but not verified", async () => { + const consoleSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + const verifiedTimestamp = createMockTimestamp(); + const session = createMockSession({ + factors: { + user: { + id: mockUserId, + organizationId: mockOrganizationId, + loginName: "test@example.com", + displayName: "Test User", + verifiedAt: verifiedTimestamp, + }, + password: { + verifiedAt: verifiedTimestamp, + }, + // No TOTP verification + }, + }); + + vi.mocked(zitadelModule.listAuthenticationMethodTypes).mockResolvedValue({ + authMethodTypes: [AuthenticationMethodType.TOTP], + } as any); + + const result = await isSessionValid({ serviceUrl: mockServiceUrl, session }); + + expect(result).toBe(false); + expect(consoleSpy).toHaveBeenCalledWith( + "Session has no valid MFA factor. Configured methods:", + [AuthenticationMethodType.TOTP], + "Session factors:", + expect.objectContaining({ + totp: undefined, + otpEmail: undefined, + otpSms: undefined, + webAuthN: undefined, + }) + ); + consoleSpy.mockRestore(); + }); + + test("should return true when OTP Email is configured and verified", async () => { + const verifiedTimestamp = createMockTimestamp(); + const session = createMockSession({ + factors: { + user: { + id: mockUserId, + organizationId: mockOrganizationId, + loginName: "test@example.com", + displayName: "Test User", + verifiedAt: verifiedTimestamp, + }, + password: { + verifiedAt: verifiedTimestamp, + }, + otpEmail: { + verifiedAt: verifiedTimestamp, + }, + }, + }); + + vi.mocked(zitadelModule.listAuthenticationMethodTypes).mockResolvedValue({ + authMethodTypes: [AuthenticationMethodType.OTP_EMAIL], + } as any); + + const result = await isSessionValid({ serviceUrl: mockServiceUrl, session }); + + expect(result).toBe(true); + }); + + test("should return true when U2F is configured and verified", async () => { + const verifiedTimestamp = createMockTimestamp(); + const session = createMockSession({ + factors: { + user: { + id: mockUserId, + organizationId: mockOrganizationId, + loginName: "test@example.com", + displayName: "Test User", + verifiedAt: verifiedTimestamp, + }, + password: { + verifiedAt: verifiedTimestamp, + }, + webAuthN: { + verifiedAt: verifiedTimestamp, + }, + }, + }); + + vi.mocked(zitadelModule.listAuthenticationMethodTypes).mockResolvedValue({ + authMethodTypes: [AuthenticationMethodType.U2F], + } as any); + + const result = await isSessionValid({ serviceUrl: mockServiceUrl, session }); + + expect(result).toBe(true); + }); + + test("should return true when multiple auth methods are configured and one is verified", async () => { + const verifiedTimestamp = createMockTimestamp(); + const session = createMockSession({ + factors: { + user: { + id: mockUserId, + organizationId: mockOrganizationId, + loginName: "test@example.com", + displayName: "Test User", + verifiedAt: verifiedTimestamp, + }, + password: { + verifiedAt: verifiedTimestamp, + }, + otpEmail: { + verifiedAt: verifiedTimestamp, + }, + // TOTP not verified + }, + }); + + vi.mocked(zitadelModule.listAuthenticationMethodTypes).mockResolvedValue({ + authMethodTypes: [AuthenticationMethodType.TOTP, AuthenticationMethodType.OTP_EMAIL], + } as any); + + const result = await isSessionValid({ serviceUrl: mockServiceUrl, session }); + + expect(result).toBe(true); + }); + }); + + describe("MFA validation with login settings (no configured auth methods)", () => { + test("should return true when MFA is not forced and no auth methods configured", async () => { + const verifiedTimestamp = createMockTimestamp(); + const session = createMockSession({ + factors: { + user: { + id: mockUserId, + organizationId: mockOrganizationId, + loginName: "test@example.com", + displayName: "Test User", + verifiedAt: verifiedTimestamp, + }, + password: { + verifiedAt: verifiedTimestamp, + }, + }, + }); + + vi.mocked(zitadelModule.listAuthenticationMethodTypes).mockResolvedValue({ + authMethodTypes: [], + } as any); + + vi.mocked(zitadelModule.getLoginSettings).mockResolvedValue({ + forceMfa: false, + forceMfaLocalOnly: false, + } as any); + + const result = await isSessionValid({ serviceUrl: mockServiceUrl, session }); + + expect(result).toBe(true); + }); + + test("should return false when MFA is forced but no factors are verified", async () => { + const consoleSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + const verifiedTimestamp = createMockTimestamp(); + const session = createMockSession({ + factors: { + user: { + id: mockUserId, + organizationId: mockOrganizationId, + loginName: "test@example.com", + displayName: "Test User", + verifiedAt: verifiedTimestamp, + }, + password: { + verifiedAt: verifiedTimestamp, + }, + // No MFA factors verified + }, + }); + + vi.mocked(zitadelModule.listAuthenticationMethodTypes).mockResolvedValue({ + authMethodTypes: [], + } as any); + + vi.mocked(zitadelModule.getLoginSettings).mockResolvedValue({ + forceMfa: true, + forceMfaLocalOnly: false, + } as any); + + const result = await isSessionValid({ serviceUrl: mockServiceUrl, session }); + + expect(result).toBe(false); + expect(consoleSpy).toHaveBeenCalledWith( + "Session has no valid multifactor", + expect.any(Object) + ); + consoleSpy.mockRestore(); + }); + + test("should return true when MFA is forced and TOTP is verified", async () => { + 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, + }, + }, + }); + + vi.mocked(zitadelModule.listAuthenticationMethodTypes).mockResolvedValue({ + authMethodTypes: [], + } as any); + + vi.mocked(zitadelModule.getLoginSettings).mockResolvedValue({ + forceMfa: true, + forceMfaLocalOnly: false, + } as any); + + const result = await isSessionValid({ serviceUrl: mockServiceUrl, session }); + + expect(result).toBe(true); + }); + + test("should return true when forceMfaLocalOnly is enabled and WebAuthn is verified", async () => { + const verifiedTimestamp = createMockTimestamp(); + const session = createMockSession({ + factors: { + user: { + id: mockUserId, + organizationId: mockOrganizationId, + loginName: "test@example.com", + displayName: "Test User", + verifiedAt: verifiedTimestamp, + }, + password: { + verifiedAt: verifiedTimestamp, + }, + webAuthN: { + verifiedAt: verifiedTimestamp, + }, + }, + }); + + vi.mocked(zitadelModule.listAuthenticationMethodTypes).mockResolvedValue({ + authMethodTypes: [], + } as any); + + vi.mocked(zitadelModule.getLoginSettings).mockResolvedValue({ + forceMfa: false, + forceMfaLocalOnly: true, + } as any); + + const result = await isSessionValid({ serviceUrl: mockServiceUrl, session }); + + expect(result).toBe(true); + }); + }); + + describe("email verification", () => { + test("should return false when EMAIL_VERIFICATION is enabled and email is not verified", async () => { + process.env.EMAIL_VERIFICATION = "true"; + const consoleSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + + const verifiedTimestamp = createMockTimestamp(); + const session = createMockSession({ + factors: { + user: { + id: mockUserId, + organizationId: mockOrganizationId, + loginName: "test@example.com", + displayName: "Test User", + verifiedAt: verifiedTimestamp, + }, + password: { + verifiedAt: verifiedTimestamp, + }, + }, + }); + + vi.mocked(zitadelModule.listAuthenticationMethodTypes).mockResolvedValue({ + authMethodTypes: [], + } as any); + + vi.mocked(zitadelModule.getLoginSettings).mockResolvedValue({ + forceMfa: false, + forceMfaLocalOnly: false, + } as any); + + vi.mocked(zitadelModule.getUserByID).mockResolvedValue({ + user: { + type: { + case: "human", + value: { + email: { + email: "test@example.com", + isVerified: false, + }, + }, + }, + }, + } as any); + + const result = await isSessionValid({ serviceUrl: mockServiceUrl, session }); + + expect(result).toBe(false); + expect(consoleSpy).toHaveBeenCalledWith( + "Session invalid: Email not verified and EMAIL_VERIFICATION is enabled", + mockUserId + ); + consoleSpy.mockRestore(); + }); + + test("should return true when EMAIL_VERIFICATION is enabled and email is verified", async () => { + process.env.EMAIL_VERIFICATION = "true"; + + const verifiedTimestamp = createMockTimestamp(); + const session = createMockSession({ + factors: { + user: { + id: mockUserId, + organizationId: mockOrganizationId, + loginName: "test@example.com", + displayName: "Test User", + verifiedAt: verifiedTimestamp, + }, + password: { + verifiedAt: verifiedTimestamp, + }, + }, + }); + + vi.mocked(zitadelModule.listAuthenticationMethodTypes).mockResolvedValue({ + authMethodTypes: [], + } as any); + + vi.mocked(zitadelModule.getLoginSettings).mockResolvedValue({ + forceMfa: false, + forceMfaLocalOnly: false, + } as any); + + vi.mocked(zitadelModule.getUserByID).mockResolvedValue({ + user: { + type: { + case: "human", + value: { + email: { + email: "test@example.com", + isVerified: true, + }, + }, + }, + }, + } as any); + + const result = await isSessionValid({ serviceUrl: mockServiceUrl, session }); + + expect(result).toBe(true); + }); + + test("should return true when EMAIL_VERIFICATION is disabled", async () => { + // EMAIL_VERIFICATION is not set, so it's disabled by default + + const verifiedTimestamp = createMockTimestamp(); + const session = createMockSession({ + factors: { + user: { + id: mockUserId, + organizationId: mockOrganizationId, + loginName: "test@example.com", + displayName: "Test User", + verifiedAt: verifiedTimestamp, + }, + password: { + verifiedAt: verifiedTimestamp, + }, + }, + }); + + vi.mocked(zitadelModule.listAuthenticationMethodTypes).mockResolvedValue({ + authMethodTypes: [], + } as any); + + vi.mocked(zitadelModule.getLoginSettings).mockResolvedValue({ + forceMfa: false, + forceMfaLocalOnly: false, + } as any); + + const result = await isSessionValid({ serviceUrl: mockServiceUrl, session }); + + expect(result).toBe(true); + // getUserByID should not be called when EMAIL_VERIFICATION is disabled + expect(zitadelModule.getUserByID).not.toHaveBeenCalled(); + }); + }); + + describe("passkey authentication", () => { + test("should return true when authenticated with passkey (WebAuthn)", async () => { + const verifiedTimestamp = createMockTimestamp(); + const session = createMockSession({ + factors: { + user: { + id: mockUserId, + organizationId: mockOrganizationId, + loginName: "test@example.com", + displayName: "Test User", + verifiedAt: verifiedTimestamp, + }, + webAuthN: { + verifiedAt: verifiedTimestamp, + }, + // No password factor + }, + }); + + vi.mocked(zitadelModule.listAuthenticationMethodTypes).mockResolvedValue({ + authMethodTypes: [], + } as any); + + vi.mocked(zitadelModule.getLoginSettings).mockResolvedValue({ + forceMfa: false, + forceMfaLocalOnly: false, + } as any); + + const result = await isSessionValid({ serviceUrl: mockServiceUrl, session }); + + expect(result).toBe(true); + }); + }); + + describe("IDP authentication", () => { + test("should return true when authenticated with IDP intent", async () => { + const verifiedTimestamp = createMockTimestamp(); + const session = createMockSession({ + factors: { + user: { + id: mockUserId, + organizationId: mockOrganizationId, + loginName: "test@example.com", + displayName: "Test User", + verifiedAt: verifiedTimestamp, + }, + intent: { + verifiedAt: verifiedTimestamp, + }, + // No password factor + }, + }); + + vi.mocked(zitadelModule.listAuthenticationMethodTypes).mockResolvedValue({ + authMethodTypes: [], + } as any); + + vi.mocked(zitadelModule.getLoginSettings).mockResolvedValue({ + forceMfa: false, + forceMfaLocalOnly: false, + } as any); + + const result = await isSessionValid({ serviceUrl: mockServiceUrl, session }); + + expect(result).toBe(true); + }); + }); + + describe("edge cases", () => { + test("should handle session without expiration date", async () => { + const verifiedTimestamp = createMockTimestamp(); + const session = createMockSession({ + expirationDate: undefined, // No expiration date + factors: { + user: { + id: mockUserId, + organizationId: mockOrganizationId, + loginName: "test@example.com", + displayName: "Test User", + verifiedAt: verifiedTimestamp, + }, + password: { + verifiedAt: verifiedTimestamp, + }, + }, + }); + + vi.mocked(zitadelModule.listAuthenticationMethodTypes).mockResolvedValue({ + authMethodTypes: [], + } as any); + + vi.mocked(zitadelModule.getLoginSettings).mockResolvedValue({ + forceMfa: false, + forceMfaLocalOnly: false, + } as any); + + const result = await isSessionValid({ serviceUrl: mockServiceUrl, session }); + + expect(result).toBe(true); + }); + }); +}); \ No newline at end of file diff --git a/apps/login/src/lib/session.ts b/apps/login/src/lib/session.ts index f250da64cf..45c3274c51 100644 --- a/apps/login/src/lib/session.ts +++ b/apps/login/src/lib/session.ts @@ -50,25 +50,22 @@ export async function isSessionValid({ serviceUrl, session }: { serviceUrl: stri }); const authMethods = authMethodTypes.authMethodTypes; - if (authMethods && authMethods.includes(AuthenticationMethodType.TOTP)) { - mfaValid = !!session.factors.totp?.verifiedAt; + if (authMethods && authMethods.length > 0) { + // Check if any of the configured authentication methods have been verified + const totpValid = authMethods.includes(AuthenticationMethodType.TOTP) && !!session.factors.totp?.verifiedAt; + const otpEmailValid = authMethods.includes(AuthenticationMethodType.OTP_EMAIL) && !!session.factors.otpEmail?.verifiedAt; + const otpSmsValid = authMethods.includes(AuthenticationMethodType.OTP_SMS) && !!session.factors.otpSms?.verifiedAt; + const u2fValid = authMethods.includes(AuthenticationMethodType.U2F) && !!session.factors.webAuthN?.verifiedAt; + + mfaValid = totpValid || otpEmailValid || otpSmsValid || u2fValid; + if (!mfaValid) { - console.warn("Session has no valid totpEmail factor", session.factors.totp?.verifiedAt); - } - } else if (authMethods && authMethods.includes(AuthenticationMethodType.OTP_EMAIL)) { - mfaValid = !!session.factors.otpEmail?.verifiedAt; - if (!mfaValid) { - console.warn("Session has no valid otpEmail factor", session.factors.otpEmail?.verifiedAt); - } - } else if (authMethods && authMethods.includes(AuthenticationMethodType.OTP_SMS)) { - mfaValid = !!session.factors.otpSms?.verifiedAt; - if (!mfaValid) { - console.warn("Session has no valid otpSms factor", session.factors.otpSms?.verifiedAt); - } - } else if (authMethods && authMethods.includes(AuthenticationMethodType.U2F)) { - mfaValid = !!session.factors.webAuthN?.verifiedAt; - if (!mfaValid) { - console.warn("Session has no valid u2f factor", session.factors.webAuthN?.verifiedAt); + console.warn("Session has no valid MFA factor. Configured methods:", authMethods, "Session factors:", { + totp: session.factors.totp?.verifiedAt, + otpEmail: session.factors.otpEmail?.verifiedAt, + otpSms: session.factors.otpSms?.verifiedAt, + webAuthN: session.factors.webAuthN?.verifiedAt + }); } } else { // only check settings if no auth methods are available, as this would require a setup