mirror of
https://github.com/zitadel/zitadel.git
synced 2026-08-17 16:35:14 -05:00
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.
This commit is contained in:
@@ -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;
|
||||
}
|
||||
|
||||
|
||||
@@ -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 () => {
|
||||
|
||||
@@ -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" };
|
||||
});
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user