From 00481bd2dc746fd3e348f20ef4f0668dadfd6f0b Mon Sep 17 00:00:00 2001 From: Max Peintner Date: Tue, 20 Jan 2026 10:15:13 +0100 Subject: [PATCH] fix(login): prevent eventual consistency issues on /password/change, missing permissions (#11371) Closes #11345 # Which Problems Are Solved When changing password, users could have run into a race condition / eventual consistency issue which resulted in: - Verification failures in `sendPassword` - The user receiving a `couldNotCreateSession` or `couldNotCreateSessionForUser` error despite providing a valid password. # How the Problems Are Solved - Previously, `checkSessionAndSetPassword` was fired without await, causing the code to proceed immediately to `sendPassword`. This resulted in `sendPassword` trying to verify the user's session with the new password before the password update had actually completed on the server. - Now the password change call is executed by the login service user only, ommitting eventual `membership not found (AUTHZ-cdgFk)` errors from the API by using the user session itself. The login checks for a recent password change (within 5 minutes) as well to ensure session freshness. --- .../src/components/change-password-form.tsx | 17 ++-- apps/login/src/lib/server/password.test.ts | 26 +----- apps/login/src/lib/server/password.ts | 92 ++----------------- 3 files changed, 21 insertions(+), 114 deletions(-) diff --git a/apps/login/src/components/change-password-form.tsx b/apps/login/src/components/change-password-form.tsx index ade359dda..7560c0404 100644 --- a/apps/login/src/components/change-password-form.tsx +++ b/apps/login/src/components/change-password-form.tsx @@ -51,25 +51,24 @@ export function ChangePasswordForm({ passwordComplexitySettings, sessionId, logi async function submitChange(values: Inputs) { setLoading(true); - const changeResponse = checkSessionAndSetPassword({ + const changeResponse = await checkSessionAndSetPassword({ sessionId, password: values.password, - }) - .catch(() => { - setError(t("change.errors.couldNotChangePassword")); - return; - }) - .finally(() => { - setLoading(false); - }); + }).catch(() => { + setError(t("change.errors.couldNotChangePassword")); + setLoading(false); + return; + }); if (changeResponse && "error" in changeResponse && changeResponse.error) { setError(typeof changeResponse.error === "string" ? changeResponse.error : t("change.errors.unknownError")); + setLoading(false); return; } if (!changeResponse) { setError(t("change.errors.couldNotChangePassword")); + setLoading(false); return; } diff --git a/apps/login/src/lib/server/password.test.ts b/apps/login/src/lib/server/password.test.ts index ad9fe0811..6f9462f6e 100644 --- a/apps/login/src/lib/server/password.test.ts +++ b/apps/login/src/lib/server/password.test.ts @@ -19,10 +19,6 @@ vi.mock("@zitadel/client", () => ({ timestampDate: (ts: any) => new Date(ts.seconds * 1000), })); -vi.mock("@zitadel/client/v2", () => ({ - createUserServiceClient: vi.fn(), -})); - vi.mock("../service-url", () => ({ getServiceConfig: vi.fn(), })); @@ -74,8 +70,6 @@ describe("checkSessionAndSetPassword", () => { let mockListAuthenticationMethodTypes: any; let mockGetLoginSettings: any; let mockSetPassword: any; // Service account - let mockCreateUserServiceClient: any; // User session - let mockSetPasswordUser: any; beforeEach(async () => { vi.clearAllMocks(); @@ -84,7 +78,6 @@ describe("checkSessionAndSetPassword", () => { const { getServiceConfig } = await import("../service-url"); const { getSessionCookieById } = await import("../cookies"); const { getSession, listAuthenticationMethodTypes, getLoginSettings, setPassword } = await import("../zitadel"); - const { createUserServiceClient } = await import("@zitadel/client/v2"); mockHeaders = vi.mocked(headers); mockGetServiceConfig = vi.mocked(getServiceConfig); @@ -93,7 +86,6 @@ describe("checkSessionAndSetPassword", () => { mockListAuthenticationMethodTypes = vi.mocked(listAuthenticationMethodTypes); mockGetLoginSettings = vi.mocked(getLoginSettings); mockSetPassword = vi.mocked(setPassword); - mockCreateUserServiceClient = vi.mocked(createUserServiceClient); mockHeaders.mockResolvedValue({}); mockGetServiceConfig.mockReturnValue({ serviceConfig: { baseUrl: "https://api.example.com" } }); @@ -118,20 +110,14 @@ describe("checkSessionAndSetPassword", () => { mockGetLoginSettings.mockResolvedValue({ forceMfa: false }); // Mock user service client - mockSetPasswordUser = vi.fn().mockResolvedValue({}); - mockCreateUserServiceClient.mockReturnValue({ - setPassword: mockSetPasswordUser, - }); mockSetPassword.mockResolvedValue({}); }); - test("should use user session when no MFA is configured", async () => { + test("should use service account when no MFA is configured", async () => { await checkSessionAndSetPassword({ sessionId: "session123", password: "newpassword" }); - expect(mockCreateUserServiceClient).toHaveBeenCalled(); - expect(mockSetPasswordUser).toHaveBeenCalled(); - expect(mockSetPassword).not.toHaveBeenCalled(); + expect(mockSetPassword).toHaveBeenCalled(); }); test("should use service account when MFA is configured but NOT verified in session", async () => { @@ -154,13 +140,10 @@ describe("checkSessionAndSetPassword", () => { await checkSessionAndSetPassword({ sessionId: "session123", password: "newpassword" }); - // EXPECTATION: Should use service account (mockSetPassword) - // CURRENTLY: Will fail this test and use user session expect(mockSetPassword).toHaveBeenCalled(); - expect(mockCreateUserServiceClient).not.toHaveBeenCalled(); }); - test("should use user session when MFA is configured AND verified in session", async () => { + test("should use service account when MFA is configured AND verified in session", async () => { // User has TOTP configured mockListAuthenticationMethodTypes.mockResolvedValue({ authMethodTypes: [AuthenticationMethodType.PASSWORD, AuthenticationMethodType.TOTP], @@ -179,8 +162,7 @@ describe("checkSessionAndSetPassword", () => { await checkSessionAndSetPassword({ sessionId: "session123", password: "newpassword" }); - expect(mockCreateUserServiceClient).toHaveBeenCalled(); - expect(mockSetPassword).not.toHaveBeenCalled(); + expect(mockSetPassword).toHaveBeenCalled(); }); test("should fail when MFA is configured but not verified, and password verification is too old", async () => { diff --git a/apps/login/src/lib/server/password.ts b/apps/login/src/lib/server/password.ts index 485f2cc50..311bd207c 100644 --- a/apps/login/src/lib/server/password.ts +++ b/apps/login/src/lib/server/password.ts @@ -10,16 +10,14 @@ import { listAuthenticationMethodTypes, passwordReset, searchUsers, - ServiceConfig, setPassword, setUserPassword, } from "@/lib/zitadel"; -import { ConnectError, create, Duration, timestampDate } from "@zitadel/client"; -import { createUserServiceClient } from "@zitadel/client/v2"; +import { create, Duration, timestampDate } from "@zitadel/client"; import { Checks, ChecksSchema } from "@zitadel/proto/zitadel/session/v2/session_service_pb"; import { LoginSettings } from "@zitadel/proto/zitadel/settings/v2/login_settings_pb"; import { User, UserState } from "@zitadel/proto/zitadel/user/v2/user_pb"; -import { AuthenticationMethodType, SetPasswordRequestSchema } from "@zitadel/proto/zitadel/user/v2/user_service_pb"; +import { SetPasswordRequestSchema } from "@zitadel/proto/zitadel/user/v2/user_service_pb"; import { getTranslations } from "next-intl/server"; import { headers } from "next/headers"; import { completeFlowOrGetUrl } from "../client"; @@ -31,7 +29,6 @@ import { checkPasswordChangeRequired, checkUserVerification, } from "../verify-helper"; -import { createServerTransport } from "../zitadel"; import { getPublicHostWithProtocol } from "./host"; type ResetPasswordCommand = { @@ -111,7 +108,6 @@ export async function resetPassword(command: ResetPasswordCommand) { } const userId = user.userId; - const basePath = process.env.NEXT_PUBLIC_BASE_PATH ?? ""; return passwordReset({ @@ -497,44 +493,6 @@ export async function checkSessionAndSetPassword({ sessionId, password }: CheckS }, }); - // check if the user has no password set in order to set a password - let authmethods; - try { - authmethods = await listAuthenticationMethodTypes({ serviceConfig, userId: session.factors.user.id }); - } catch (error) { - console.error("Error getting auth methods:", error); - return { error: "Could not load auth methods" }; - } - - if (!authmethods) { - return { error: t("errors.couldNotLoadAuthMethods") }; - } - - let loginSettings; - try { - loginSettings = await getLoginSettings({ serviceConfig, organization: session.factors.user.organizationId }); - } catch (error) { - console.error("Error getting login settings:", error); - return { error: "Could not load login settings" }; - } - - const forceMfa = !!(loginSettings?.forceMfa || loginSettings?.forceMfaLocalOnly); - - const mfaFactors = authmethods.authMethodTypes.filter( - (m) => - m === AuthenticationMethodType.TOTP || - m === AuthenticationMethodType.OTP_SMS || - m === AuthenticationMethodType.OTP_EMAIL || - m === AuthenticationMethodType.U2F, - ); - - const hasMfa = mfaFactors.length > 0; - const mfaVerified = - !!session.factors?.totp?.verifiedAt || - !!session.factors?.otpSms?.verifiedAt || - !!session.factors?.otpEmail?.verifiedAt || - !!session.factors?.webAuthN?.verifiedAt; - // check if the password factor is set and not older than 5 minutes const passwordVerifiedAt = session.factors?.password?.verifiedAt; if (!passwordVerifiedAt) { @@ -549,43 +507,11 @@ export async function checkSessionAndSetPassword({ sessionId, password }: CheckS return { error: t("errors.passwordVerificationTooOld") }; } - // if the user has no MFA but MFA is enforced, we can set a password otherwise we use the token of the user - // also if the user has no MFA but it is not verified, we use the service account - if (forceMfa || (hasMfa && !mfaVerified)) { - console.log("Set password using service account due to enforced MFA without existing MFA methods"); - return setPassword({ serviceConfig, payload }).catch((error) => { - // throw error if failed precondition (ex. User is not yet initialized) - if (error.code === 9 && error.message) { - return { error: t("errors.failedPrecondition") }; - } - return { error: "Could not set password" }; - }); - } else { - const transport = async (serviceConfig: ServiceConfig, token: string) => { - return createServerTransport(token, serviceConfig); - }; - - const myUserService = async (serviceConfig: ServiceConfig, sessionToken: string) => { - const transportPromise = await transport(serviceConfig, sessionToken); - return createUserServiceClient(transportPromise); - }; - - const selfService = await myUserService(serviceConfig, sessionCookie.token); - - return selfService - .setPassword( - { - userId: session.factors.user.id, - newPassword: { password, changeRequired: false }, - }, - {}, - ) - .catch((error: ConnectError) => { - console.log(error); - if (error.code === 7) { - return { error: t("errors.sessionNotValid") }; - } - return { error: "Could not set the password" }; - }); - } + return setPassword({ serviceConfig, payload }).catch((error) => { + // throw error if failed precondition (ex. User is not yet initialized) + if (error.code === 9 && error.message) { + return { error: t("errors.failedPrecondition") }; + } + return { error: "Could not set password" }; + }); }