mirror of
https://github.com/andwati/zitadel.git
synced 2026-10-18 05:52:51 +00:00
fix(login): update login logic to correctly handle email and phone re… (#11618)
## Problem Enabling `Disable Phone Login` (or `Disable Email Login`) in the login policy caused some users to receive "User not found in the system" even when logging in with a valid, non-phone identifier. Users whose preferred login name looked like an email address (e.g. `[email protected]` in an org-scoped context) were rejected incorrectly. Toggling the setting back off immediately restored login for those users. Fixes #11518 ## Root Cause In `loginname.ts` and `password.ts`, the post-lookup guard that enforces the disable-phone/email restriction used `||` instead of `&&`: ```ts // BEFORE (buggy): rejects if preferredLoginName doesn't match OR email doesn't match // → any user whose preferredLoginName ≠ raw email is rejected if (user.preferredLoginName !== concatLoginname || humanUser?.email?.email !== command.loginName) { return preventUserEnumeration(...); } // AFTER (correct): rejects only if preferredLoginName doesn't match AND email doesn't match // → user passes through if they matched by either their preferred login name or their email if (user.preferredLoginName !== concatLoginname && humanUser?.email?.email !== command.loginName) { return preventUserEnumeration(...); } ``` The intent is to allow the user through if they matched by their preferred login name **or** by the still-permitted identifier. By De Morgan's law, the rejection condition must be `¬A ∧ ¬B`, not `¬A ∨ ¬B`. ## Changes - **`loginname.ts`**: fixed 2 `||` → `&&` in the `disableLoginWithEmail` and `disableLoginWithPhone` guards - **`password.ts`**: fixed 4 `||` → `&&` across the same guards in both `resetPassword` and `sendPassword` - **`loginname.test.ts`**: added two regression tests: - `disableLoginWithPhone=true`, user logs in with their email-format preferred login name → must succeed (was broken) - `disableLoginWithPhone=true`, user logs in with their actual phone number → must be blocked (feature still works) ## Testing All 654 unit tests pass. Co-authored-by: Max Peintner <[email protected]>
This commit is contained in:
co-authored by
Max Peintner
parent
f3b578f19f
commit
11d48edd84
@@ -731,6 +731,75 @@ describe("sendLoginname", () => {
|
|||||||
expect((result as any).redirect).toMatch(/^\/password\?/);
|
expect((result as any).redirect).toMatch(/^\/password\?/);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test("should allow login with email when disableLoginWithPhone is true and preferred login name differs from email", async () => {
|
||||||
|
// Regression test for: https://github.com/zitadel/zitadel/issues/11518
|
||||||
|
// When preferredLoginName (e.g. username@org-domain) differs from the user's email, logging
|
||||||
|
// in with the email while disableLoginWithPhone=true must NOT be blocked.
|
||||||
|
const mockUser = {
|
||||||
|
userId: "user123",
|
||||||
|
preferredLoginName: "[email protected]", // org-domain scoped login name
|
||||||
|
details: { resourceOwner: "org123" },
|
||||||
|
type: {
|
||||||
|
case: "human",
|
||||||
|
value: { email: { email: "[email protected]" }, phone: { phone: "+1234567890" } },
|
||||||
|
},
|
||||||
|
state: UserState.ACTIVE,
|
||||||
|
};
|
||||||
|
|
||||||
|
const mockSession = {
|
||||||
|
factors: {
|
||||||
|
user: {
|
||||||
|
id: "user123",
|
||||||
|
loginName: "[email protected]",
|
||||||
|
organizationId: "org123",
|
||||||
|
},
|
||||||
|
},
|
||||||
|
};
|
||||||
|
|
||||||
|
mockSearchUsers.mockResolvedValue({ result: [mockUser] });
|
||||||
|
mockGetLoginSettings.mockResolvedValue({
|
||||||
|
disableLoginWithPhone: true,
|
||||||
|
allowLocalAuthentication: true,
|
||||||
|
});
|
||||||
|
mockCreateSessionAndUpdateCookie.mockResolvedValue({ session: mockSession, sessionCookie: {} });
|
||||||
|
mockListAuthenticationMethodTypes.mockResolvedValue({
|
||||||
|
authMethodTypes: [AuthenticationMethodType.PASSWORD],
|
||||||
|
});
|
||||||
|
|
||||||
|
const result = await sendLoginname({
|
||||||
|
loginName: "[email protected]", // logging in with email, not phone
|
||||||
|
});
|
||||||
|
|
||||||
|
// Must NOT return "User not found" — email-based login must be allowed when only phone is disabled
|
||||||
|
expect(result).not.toEqual({ error: "errors.userNotFound" });
|
||||||
|
expect(mockCreateSessionAndUpdateCookie).toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
|
test("should block login with phone number when disableLoginWithPhone is true", async () => {
|
||||||
|
const mockUser = {
|
||||||
|
userId: "user123",
|
||||||
|
preferredLoginName: "[email protected]",
|
||||||
|
details: { resourceOwner: "org123" },
|
||||||
|
type: {
|
||||||
|
case: "human",
|
||||||
|
value: { email: { email: "[email protected]" }, phone: { phone: "+1234567890" } },
|
||||||
|
},
|
||||||
|
state: UserState.ACTIVE,
|
||||||
|
};
|
||||||
|
|
||||||
|
mockSearchUsers.mockResolvedValue({ result: [mockUser] });
|
||||||
|
mockGetLoginSettings.mockResolvedValue({
|
||||||
|
disableLoginWithPhone: true,
|
||||||
|
});
|
||||||
|
|
||||||
|
const result = await sendLoginname({
|
||||||
|
loginName: "+1234567890", // logging in with phone — must be blocked
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(result).toEqual({ error: "errors.userNotFound" });
|
||||||
|
expect(mockCreateSessionAndUpdateCookie).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
test("should redirect to password when ignoreUnknownUsernames is true and more than one user found", async () => {
|
test("should redirect to password when ignoreUnknownUsernames is true and more than one user found", async () => {
|
||||||
mockGetLoginSettings.mockResolvedValue({
|
mockGetLoginSettings.mockResolvedValue({
|
||||||
ignoreUnknownUsernames: true,
|
ignoreUnknownUsernames: true,
|
||||||
|
|||||||
@@ -250,11 +250,11 @@ export async function sendLoginname(command: SendLoginnameCommand) {
|
|||||||
return preventUserEnumeration(command.organization);
|
return preventUserEnumeration(command.organization);
|
||||||
}
|
}
|
||||||
} else if (userLoginSettings?.disableLoginWithEmail) {
|
} else if (userLoginSettings?.disableLoginWithEmail) {
|
||||||
if (user.preferredLoginName !== concatLoginname || humanUser?.phone?.phone !== command.loginName) {
|
if (user.preferredLoginName !== concatLoginname && humanUser?.phone?.phone !== command.loginName) {
|
||||||
return preventUserEnumeration(command.organization);
|
return preventUserEnumeration(command.organization);
|
||||||
}
|
}
|
||||||
} else if (userLoginSettings?.disableLoginWithPhone) {
|
} else if (userLoginSettings?.disableLoginWithPhone) {
|
||||||
if (user.preferredLoginName !== concatLoginname || humanUser?.email?.email !== command.loginName) {
|
if (user.preferredLoginName !== concatLoginname && humanUser?.email?.email !== command.loginName) {
|
||||||
return preventUserEnumeration(command.organization);
|
return preventUserEnumeration(command.organization);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -90,7 +90,7 @@ export async function resetPassword(command: ResetPasswordCommand) {
|
|||||||
return { error: t("errors.couldNotSendResetLink") };
|
return { error: t("errors.couldNotSendResetLink") };
|
||||||
}
|
}
|
||||||
} else if (userLoginSettings?.disableLoginWithEmail) {
|
} else if (userLoginSettings?.disableLoginWithEmail) {
|
||||||
if (user.preferredLoginName !== command.loginName || humanUser?.phone?.phone !== command.loginName) {
|
if (user.preferredLoginName !== command.loginName && humanUser?.phone?.phone !== command.loginName) {
|
||||||
if (userLoginSettings?.ignoreUnknownUsernames) {
|
if (userLoginSettings?.ignoreUnknownUsernames) {
|
||||||
await new Promise((resolve) => setTimeout(resolve, 2000));
|
await new Promise((resolve) => setTimeout(resolve, 2000));
|
||||||
return {};
|
return {};
|
||||||
@@ -98,7 +98,7 @@ export async function resetPassword(command: ResetPasswordCommand) {
|
|||||||
return { error: t("errors.couldNotSendResetLink") };
|
return { error: t("errors.couldNotSendResetLink") };
|
||||||
}
|
}
|
||||||
} else if (userLoginSettings?.disableLoginWithPhone) {
|
} else if (userLoginSettings?.disableLoginWithPhone) {
|
||||||
if (user.preferredLoginName !== command.loginName || humanUser?.email?.email !== command.loginName) {
|
if (user.preferredLoginName !== command.loginName && humanUser?.email?.email !== command.loginName) {
|
||||||
if (userLoginSettings?.ignoreUnknownUsernames) {
|
if (userLoginSettings?.ignoreUnknownUsernames) {
|
||||||
await new Promise((resolve) => setTimeout(resolve, 2000));
|
await new Promise((resolve) => setTimeout(resolve, 2000));
|
||||||
return {};
|
return {};
|
||||||
@@ -221,14 +221,14 @@ export async function sendPassword(
|
|||||||
return { error: t("errors.couldNotVerifyPassword") };
|
return { error: t("errors.couldNotVerifyPassword") };
|
||||||
}
|
}
|
||||||
} else if (userLoginSettings?.disableLoginWithEmail) {
|
} else if (userLoginSettings?.disableLoginWithEmail) {
|
||||||
if (user.preferredLoginName !== command.loginName || humanUser?.phone?.phone !== command.loginName) {
|
if (user.preferredLoginName !== command.loginName && humanUser?.phone?.phone !== command.loginName) {
|
||||||
if (loginSettingsByContext?.ignoreUnknownUsernames) {
|
if (loginSettingsByContext?.ignoreUnknownUsernames) {
|
||||||
return { error: t("errors.failedToAuthenticateNoLimit") };
|
return { error: t("errors.failedToAuthenticateNoLimit") };
|
||||||
}
|
}
|
||||||
return { error: t("errors.couldNotVerifyPassword") };
|
return { error: t("errors.couldNotVerifyPassword") };
|
||||||
}
|
}
|
||||||
} else if (userLoginSettings?.disableLoginWithPhone) {
|
} else if (userLoginSettings?.disableLoginWithPhone) {
|
||||||
if (user.preferredLoginName !== command.loginName || humanUser?.email?.email !== command.loginName) {
|
if (user.preferredLoginName !== command.loginName && humanUser?.email?.email !== command.loginName) {
|
||||||
if (loginSettingsByContext?.ignoreUnknownUsernames) {
|
if (loginSettingsByContext?.ignoreUnknownUsernames) {
|
||||||
return { error: t("errors.failedToAuthenticateNoLimit") };
|
return { error: t("errors.failedToAuthenticateNoLimit") };
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user