Merge commit from fork

* add mfa check to session validity check

* change session check to check for already set up mfa methods

* lint

* improve naming

* test changes

* cleanup continueWithSession fcn

apps/login/src/app/(login)/mfa/set/page.tsx

@@ -27,7 +27,7 @@ export async function generateMetadata(): Promise<Metadata> {
   return { title: t("set.title") };
 }
 
-function isSessionValid(session: Partial<Session>): {
+function isSessionValidForMfaSet(session: Partial<Session>): {
   valid: boolean;
   verifiedAt?: Timestamp;
 } {
@@ -111,7 +111,7 @@ export default async function Page(props: { searchParams: Promise<Record<string
     organization: sessionWithData?.factors?.user?.organizationId,
   });
 
-  const { valid } = sessionWithData ? isSessionValid(sessionWithData) : { valid: false };
+  const { valid } = sessionWithData ? isSessionValidForMfaSet(sessionWithData) : { valid: false };
 
   if (force === "true" && valid && sessionWithData?.factors?.user?.loginName && loginSettings) {
     const emailVerified = sessionWithData.emailVerified ?? false;

apps/login/src/components/session-clear-item.tsx

@@ -8,7 +8,7 @@ import { useLocale } from "next-intl";
 import { useRouter } from "next/navigation";
 import { useState } from "react";
 import { Avatar } from "./avatar";
-import { isSessionValid } from "./session-item";
+import { isSessionPrimaryFactorAndLifetimeValid } from "./session-item";
 import { Translated } from "./translated";
 
 export function SessionClearItem({ session, reload }: { session: Session; reload: () => 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<string | null>(null);
 

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<Session>): {
+export function isSessionPrimaryFactorAndLifetimeValid(session: Partial<Session>): {
   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<string, string> } | null>(null);
 
   const [_error, setError] = useState<string | null>(null);

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 = {

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();
     });
   });
 

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;