mirror of
https://github.com/andwati/zitadel.git
synced 2026-10-18 05:52:51 +00:00
fix(login): forward login_hint to the external IdP (#12760)
# Which Problems Are Solved Login V2 never forwards the OIDC `login_hint` to an external identity provider. The user enters their identifier once at ZITADEL, is redirected to the IdP, and has to type it again because the IdP's authorize request contains no `login_hint`. Login V1 has done this since #6899 (`sessionParamsFromAuthRequest`), and the backend half for V2 exists since #11552 (`RedirectURLs.login_hint` on `StartIdentityProviderIntent`), but none of the three places in the login app that start an IdP intent set it. # How the Problems Are Solved Every call to `startIdentityProviderFlow` now sets `urls.loginHint`, mirroring the priority Login V1 uses: - `sendLoginname`: when the user is already known and linked to exactly one IdP, the hint is the username the IdP knows the user by (`IDPLink.userName`), falling back to the typed login name (with the org domain suffix re-attached). When an org's single active IdP triggers the redirect, the hint is the login name. - `handleOIDCFlowInitiation`: when the auth request selects an IdP via `urn:zitadel:iam:org:idp🆔<id>`, the hint is `authRequest.loginHint`. - `redirectToIdp` (the user clicks an IdP button): `SignInWithIdp` gets an optional `loginHint` prop rendered as a hidden form field. The loginname page passes the login name it was prefilled with from `login_hint`; the authenticator setup page passes the session's login name when linking an IdP to an existing user. Unit tests cover all three call sites, including the linked-username priority and the empty-hint case. Closes #12080 Based on #12683 by @aarontraynor
This commit is contained in:
@@ -186,6 +186,7 @@ export default async function Page(props: { searchParams: Promise<Record<string
|
||||
requestId={requestId}
|
||||
organization={sessionWithData.factors?.user?.organizationId}
|
||||
sessionId={sessionWithData.id} // tell the callback function to link the IDP
|
||||
loginHint={sessionWithData.factors?.user?.loginName}
|
||||
></SignInWithIdp>
|
||||
</>
|
||||
)}
|
||||
|
||||
@@ -23,6 +23,11 @@ export default async function Page(props: { searchParams: Promise<Record<string
|
||||
const orgDomain = searchParams?.orgDomain;
|
||||
const submit: boolean = searchParams?.submit === "true";
|
||||
|
||||
// With an org domain suffix the login name may only be the local part (the
|
||||
// form shows the suffix separately), so put it back together for the IdP
|
||||
// login hint the same way sendLoginname does for the username form.
|
||||
const idpLoginHint = loginName && orgDomain && !loginName.includes("@") ? `${loginName}@${orgDomain}` : loginName;
|
||||
|
||||
const _headers = await headers();
|
||||
const { serviceConfig } = getServiceConfig(_headers);
|
||||
|
||||
@@ -78,6 +83,7 @@ export default async function Page(props: { searchParams: Promise<Record<string
|
||||
requestId={requestId}
|
||||
organization={organization}
|
||||
postErrorRedirectUrl="/loginname"
|
||||
loginHint={idpLoginHint}
|
||||
showLabel={loginSettings?.allowLocalAuthentication}
|
||||
></SignInWithIdp>
|
||||
</div>
|
||||
|
||||
@@ -23,6 +23,8 @@ export interface SignInWithIDPProps {
|
||||
organization?: string;
|
||||
sessionId?: string;
|
||||
postErrorRedirectUrl?: string;
|
||||
/** Forwarded to the IdP as login_hint so the user does not have to type the identifier again. */
|
||||
loginHint?: string;
|
||||
showLabel?: boolean;
|
||||
}
|
||||
|
||||
@@ -32,6 +34,7 @@ export function SignInWithIdp({
|
||||
organization,
|
||||
sessionId,
|
||||
postErrorRedirectUrl,
|
||||
loginHint,
|
||||
showLabel = true,
|
||||
}: Readonly<SignInWithIDPProps>) {
|
||||
const [state, action, _isPending] = useActionState(redirectToIdp, {});
|
||||
@@ -64,6 +67,7 @@ export function SignInWithIdp({
|
||||
<input type="hidden" name="organization" value={organization} />
|
||||
{sessionId && <input type="hidden" name="sessionId" value={sessionId} />}
|
||||
{postErrorRedirectUrl && <input type="hidden" name="postErrorRedirectUrl" value={postErrorRedirectUrl} />}
|
||||
{loginHint && <input type="hidden" name="loginHint" value={loginHint} />}
|
||||
<Component key={id} name={name} />
|
||||
</form>
|
||||
) : null;
|
||||
|
||||
@@ -1061,4 +1061,34 @@ describe("handleOIDCFlowInitiation — idp scope (urn:zitadel:iam:org:idp:id)",
|
||||
expect(html).toContain('action="https://adfs.example.com/adfs/ls"');
|
||||
expect(html).toContain('name="SAMLRequest"');
|
||||
});
|
||||
|
||||
test("should pass the login_hint of the auth request on to the scoped IdP", async () => {
|
||||
const { IdentityProviderType } = await import("@zitadel/proto/zitadel/settings/v2/login_settings_pb");
|
||||
|
||||
mockGetAuthRequest.mockResolvedValue({
|
||||
authRequest: {
|
||||
id: "abc123",
|
||||
uiLocales: [],
|
||||
scope: ["urn:zitadel:iam:org:idp:id:idp-1"],
|
||||
prompt: [],
|
||||
loginHint: "[email protected]",
|
||||
},
|
||||
});
|
||||
|
||||
mockGetActiveIdentityProviders.mockResolvedValue({
|
||||
identityProviders: [{ id: "idp-1", type: IdentityProviderType.AZURE_AD }],
|
||||
});
|
||||
|
||||
mockStartIdentityProviderFlow.mockResolvedValue({
|
||||
url: "https://login.microsoftonline.com/tenant/oauth2/v2.0/authorize?client_id=xyz",
|
||||
});
|
||||
|
||||
await handleOIDCFlowInitiation(makeBaseParams({ sessions: [] }));
|
||||
|
||||
expect(mockStartIdentityProviderFlow).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
urls: expect.objectContaining({ loginHint: "[email protected]" }),
|
||||
}),
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -329,6 +329,7 @@ export async function handleOIDCFlowInitiation(params: FlowInitiationParams): Pr
|
||||
urls: {
|
||||
successUrl: constructUrl(request, `/idp/${provider}/process?${params.toString()}`).toString(),
|
||||
failureUrl: constructUrl(request, `/idp/${provider}/failure?${params.toString()}`).toString(),
|
||||
loginHint: authRequest.loginHint,
|
||||
},
|
||||
});
|
||||
|
||||
|
||||
@@ -269,6 +269,55 @@ describe("redirectToIdp", () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe("loginHint parameter handling", () => {
|
||||
test("should forward the loginHint to the IDP flow when provided", async () => {
|
||||
const formData = new FormData();
|
||||
formData.append("id", "idp123");
|
||||
formData.append("provider", "google");
|
||||
formData.append("loginHint", "[email protected]");
|
||||
|
||||
mockStartIdentityProviderFlow.mockResolvedValue({ url: "https://idp.example.com/auth" });
|
||||
|
||||
try {
|
||||
await redirectToIdp(undefined, formData);
|
||||
} catch (error: any) {
|
||||
// Redirect throws in tests
|
||||
expect(error.message).toContain("REDIRECT:");
|
||||
}
|
||||
|
||||
expect(mockStartIdentityProviderFlow).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
idpId: "idp123",
|
||||
urls: expect.objectContaining({ loginHint: "[email protected]" }),
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
test("should not set a loginHint when it is missing or empty", async () => {
|
||||
for (const loginHint of [undefined, ""]) {
|
||||
mockStartIdentityProviderFlow.mockReset();
|
||||
mockStartIdentityProviderFlow.mockResolvedValue({ url: "https://idp.example.com/auth" });
|
||||
|
||||
const formData = new FormData();
|
||||
formData.append("id", "idp123");
|
||||
formData.append("provider", "google");
|
||||
if (loginHint !== undefined) {
|
||||
formData.append("loginHint", loginHint);
|
||||
}
|
||||
|
||||
try {
|
||||
await redirectToIdp(undefined, formData);
|
||||
} catch (error: any) {
|
||||
// Redirect throws in tests
|
||||
expect(error.message).toContain("REDIRECT:");
|
||||
}
|
||||
|
||||
const callArgs = mockStartIdentityProviderFlow.mock.calls[0][0];
|
||||
expect(callArgs.urls.loginHint).toBeUndefined();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe("General redirect behavior", () => {
|
||||
test("should return error when IDP flow returns null", async () => {
|
||||
const formData = new FormData();
|
||||
|
||||
@@ -35,6 +35,7 @@ export async function redirectToIdp(prevState: RedirectToIdpState, formData: For
|
||||
const idpId = formData.get("id") as string;
|
||||
const provider = formData.get("provider") as string;
|
||||
const postErrorRedirectUrl = formData.get("postErrorRedirectUrl") as string;
|
||||
const loginHint = formData.get("loginHint") as string;
|
||||
|
||||
if (sessionId) {
|
||||
try {
|
||||
@@ -72,6 +73,7 @@ export async function redirectToIdp(prevState: RedirectToIdpState, formData: For
|
||||
idpId,
|
||||
successUrl: `/idp/${provider}/process?` + params.toString(),
|
||||
failureUrl: `/idp/${provider}/failure?` + params.toString(),
|
||||
loginHint: loginHint || undefined,
|
||||
});
|
||||
|
||||
if (!response) {
|
||||
@@ -99,6 +101,7 @@ export type StartIDPFlowCommand = {
|
||||
idpId: string;
|
||||
successUrl: string;
|
||||
failureUrl: string;
|
||||
loginHint?: string;
|
||||
};
|
||||
|
||||
async function startIDPFlow(command: StartIDPFlowCommand) {
|
||||
@@ -110,6 +113,7 @@ async function startIDPFlow(command: StartIDPFlowCommand) {
|
||||
urls: {
|
||||
successUrl: `${command.host.includes("localhost") ? "http://" : "https://"}${command.host}${basePath}${command.successUrl}`,
|
||||
failureUrl: `${command.host.includes("localhost") ? "http://" : "https://"}${command.host}${basePath}${command.failureUrl}`,
|
||||
loginHint: command.loginHint,
|
||||
},
|
||||
});
|
||||
|
||||
|
||||
@@ -279,6 +279,70 @@ describe("sendLoginname", () => {
|
||||
});
|
||||
});
|
||||
|
||||
test("should pass the login name to the user's IDP as a login hint", async () => {
|
||||
mockGetLoginSettings.mockResolvedValue({ allowLocalAuthentication: false });
|
||||
mockListAuthenticationMethodTypes.mockResolvedValue({
|
||||
authMethodTypes: [AuthenticationMethodType.PASSWORD],
|
||||
});
|
||||
mockListIDPLinks.mockResolvedValue({
|
||||
result: [{ idpId: "idp123" }],
|
||||
});
|
||||
mockStartIdentityProviderFlow.mockResolvedValue({ url: "https://idp.example.com/auth" });
|
||||
|
||||
await sendLoginname({
|
||||
loginName: "[email protected]",
|
||||
});
|
||||
|
||||
expect(mockStartIdentityProviderFlow).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
urls: expect.objectContaining({ loginHint: "[email protected]" }),
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
test("should prefer the username known by the linked IDP over the login name as a login hint", async () => {
|
||||
mockGetLoginSettings.mockResolvedValue({ allowLocalAuthentication: false });
|
||||
mockListAuthenticationMethodTypes.mockResolvedValue({
|
||||
authMethodTypes: [AuthenticationMethodType.PASSWORD],
|
||||
});
|
||||
mockListIDPLinks.mockResolvedValue({
|
||||
result: [{ idpId: "idp123", userName: "[email protected]" }],
|
||||
});
|
||||
mockStartIdentityProviderFlow.mockResolvedValue({ url: "https://idp.example.com/auth" });
|
||||
|
||||
await sendLoginname({
|
||||
loginName: "[email protected]",
|
||||
});
|
||||
|
||||
expect(mockStartIdentityProviderFlow).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
urls: expect.objectContaining({ loginHint: "[email protected]" }),
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
test("should add the suffix back to the login hint when the user only typed the local part", async () => {
|
||||
mockGetLoginSettings.mockResolvedValue({ allowLocalAuthentication: false });
|
||||
mockListAuthenticationMethodTypes.mockResolvedValue({
|
||||
authMethodTypes: [AuthenticationMethodType.PASSWORD],
|
||||
});
|
||||
mockListIDPLinks.mockResolvedValue({
|
||||
result: [{ idpId: "idp123" }],
|
||||
});
|
||||
mockStartIdentityProviderFlow.mockResolvedValue({ url: "https://idp.example.com/auth" });
|
||||
|
||||
await sendLoginname({
|
||||
loginName: "user",
|
||||
suffix: "example.com",
|
||||
});
|
||||
|
||||
expect(mockStartIdentityProviderFlow).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
urls: expect.objectContaining({ loginHint: "[email protected]" }),
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
test("should return error when password not allowed and no IDP links available", async () => {
|
||||
mockGetLoginSettings.mockResolvedValue({ allowLocalAuthentication: false });
|
||||
mockListAuthenticationMethodTypes.mockResolvedValue({
|
||||
@@ -319,6 +383,29 @@ describe("sendLoginname", () => {
|
||||
});
|
||||
});
|
||||
|
||||
test("should pass the login name to the organization's single active IDP as a login hint", async () => {
|
||||
mockGetLoginSettings.mockResolvedValue({ allowLocalAuthentication: false });
|
||||
mockListAuthenticationMethodTypes.mockResolvedValue({
|
||||
authMethodTypes: [AuthenticationMethodType.PASSWORD],
|
||||
});
|
||||
mockListIDPLinks.mockResolvedValue({ result: [] });
|
||||
mockGetActiveIdentityProviders.mockResolvedValue({
|
||||
identityProviders: [{ id: "org-idp-123", type: 0, options: { isAutoCreation: true } }],
|
||||
});
|
||||
mockIdpTypeToSlug.mockReturnValue("google");
|
||||
mockStartIdentityProviderFlow.mockResolvedValue({ url: "https://org-idp.example.com/auth" });
|
||||
|
||||
await sendLoginname({
|
||||
loginName: "[email protected]",
|
||||
});
|
||||
|
||||
expect(mockStartIdentityProviderFlow).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
urls: expect.objectContaining({ loginHint: "[email protected]" }),
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
test("should redirect to passkey when user has only passkey method and it's allowed", async () => {
|
||||
mockGetLoginSettings.mockResolvedValue({ passkeysType: PasskeysType.ALLOWED, allowLocalAuthentication: true });
|
||||
mockListAuthenticationMethodTypes.mockResolvedValue({
|
||||
|
||||
@@ -115,6 +115,10 @@ export async function sendLoginname(command: SendLoginnameCommand) {
|
||||
return { error: t("errors.userNotFound") };
|
||||
};
|
||||
|
||||
// With an org domain suffix the user types only the local part, so put the
|
||||
// login name back together for the checks below and for the IdP login hint.
|
||||
const concatLoginname = command.suffix ? `${command.loginName}@${command.suffix}` : command.loginName;
|
||||
|
||||
const redirectUserToIDP = async (userId?: string, organization?: string) => {
|
||||
// If userId is provided, check for user-specific IDP links first
|
||||
let identityProviders: IDPLink[] = [];
|
||||
@@ -165,6 +169,7 @@ export async function sendLoginname(command: SendLoginnameCommand) {
|
||||
failureUrl:
|
||||
`${host.includes("localhost") ? "http://" : "https://"}${host}${basePath}/idp/${provider}/failure?` +
|
||||
new URLSearchParams(params),
|
||||
loginHint: concatLoginname,
|
||||
},
|
||||
});
|
||||
|
||||
@@ -224,6 +229,9 @@ export async function sendLoginname(command: SendLoginnameCommand) {
|
||||
failureUrl:
|
||||
`${host.includes("localhost") ? "http://" : "https://"}${host}${basePath}/idp/${provider}/failure?` +
|
||||
new URLSearchParams(params),
|
||||
// The user is already linked to this IdP, so prefer the username the
|
||||
// IdP knows the user by over the ZITADEL login name (same as Login V1).
|
||||
loginHint: identityProviders[0].userName || concatLoginname,
|
||||
},
|
||||
});
|
||||
|
||||
@@ -251,9 +259,6 @@ export async function sendLoginname(command: SendLoginnameCommand) {
|
||||
|
||||
const userLoginSettings = await getLoginSettings({ serviceConfig, organization: user.details?.resourceOwner });
|
||||
|
||||
// compare with the concatenated suffix when set
|
||||
const concatLoginname = command.suffix ? `${command.loginName}@${command.suffix}` : command.loginName;
|
||||
|
||||
const humanUser = users[0].type.case === "human" ? users[0].type.value : undefined;
|
||||
|
||||
// recheck login settings after user discovery, as the search might have been done without org scope
|
||||
|
||||
@@ -0,0 +1,75 @@
|
||||
import { beforeEach, describe, expect, test, vi } from "vitest";
|
||||
import { startIdentityProviderFlow } from "./zitadel";
|
||||
|
||||
vi.mock("./service", () => ({
|
||||
createServiceForHost: vi.fn(),
|
||||
}));
|
||||
|
||||
vi.mock("next-intl/server", () => ({
|
||||
getTranslations: vi.fn(),
|
||||
}));
|
||||
|
||||
vi.mock("./fingerprint", () => ({
|
||||
getUserAgent: vi.fn(),
|
||||
}));
|
||||
|
||||
describe("startIdentityProviderFlow", () => {
|
||||
const serviceConfig = { baseUrl: "https://api.example.com" } as any;
|
||||
let startIdentityProviderIntent: ReturnType<typeof vi.fn>;
|
||||
|
||||
beforeEach(async () => {
|
||||
vi.clearAllMocks();
|
||||
const { createServiceForHost } = await import("./service");
|
||||
startIdentityProviderIntent = vi.fn().mockResolvedValue({
|
||||
nextStep: { case: "authUrl", value: "https://idp.example.com/auth" },
|
||||
});
|
||||
vi.mocked(createServiceForHost).mockResolvedValue({ startIdentityProviderIntent } as any);
|
||||
});
|
||||
|
||||
test("forwards the login hint to the API", async () => {
|
||||
const result = await startIdentityProviderFlow({
|
||||
serviceConfig,
|
||||
idpId: "idp123",
|
||||
urls: { successUrl: "https://login/success", failureUrl: "https://login/failure", loginHint: "[email protected]" },
|
||||
});
|
||||
|
||||
expect(result).toEqual({ url: "https://idp.example.com/auth" });
|
||||
expect(startIdentityProviderIntent).toHaveBeenCalledWith({
|
||||
idpId: "idp123",
|
||||
content: {
|
||||
case: "urls",
|
||||
value: { successUrl: "https://login/success", failureUrl: "https://login/failure", loginHint: "[email protected]" },
|
||||
},
|
||||
});
|
||||
});
|
||||
|
||||
test("drops a login hint the API would reject instead of failing the IdP flow", async () => {
|
||||
const loginHint = "a".repeat(201);
|
||||
|
||||
const result = await startIdentityProviderFlow({
|
||||
serviceConfig,
|
||||
idpId: "idp123",
|
||||
urls: { successUrl: "https://login/success", failureUrl: "https://login/failure", loginHint },
|
||||
});
|
||||
|
||||
expect(result).toEqual({ url: "https://idp.example.com/auth" });
|
||||
expect(startIdentityProviderIntent).toHaveBeenCalledWith({
|
||||
idpId: "idp123",
|
||||
content: {
|
||||
case: "urls",
|
||||
value: { successUrl: "https://login/success", failureUrl: "https://login/failure" },
|
||||
},
|
||||
});
|
||||
});
|
||||
|
||||
test("does not set an empty login hint", async () => {
|
||||
await startIdentityProviderFlow({
|
||||
serviceConfig,
|
||||
idpId: "idp123",
|
||||
urls: { successUrl: "https://login/success", failureUrl: "https://login/failure", loginHint: "" },
|
||||
});
|
||||
|
||||
const value = startIdentityProviderIntent.mock.calls[0][0].content.value;
|
||||
expect(value).not.toHaveProperty("loginHint");
|
||||
});
|
||||
});
|
||||
@@ -925,6 +925,9 @@ export async function getOrgsByDomain({ serviceConfig, domain }: WithServiceConf
|
||||
);
|
||||
}
|
||||
|
||||
// RedirectURLs.login_hint is validated with max_len 200 by the API.
|
||||
const MAX_LOGIN_HINT_LENGTH = 200;
|
||||
|
||||
export async function startIdentityProviderFlow({
|
||||
serviceConfig,
|
||||
idpId,
|
||||
@@ -939,12 +942,19 @@ export async function startIdentityProviderFlow({
|
||||
publicHost: "",
|
||||
});
|
||||
|
||||
// The login hint only improves the UX at the IdP, so a hint the API would
|
||||
// reject (e.g. an overlong login_hint sent by the RP) is dropped instead of
|
||||
// failing the whole IdP flow.
|
||||
const { loginHint, ...redirectUrls } = urls;
|
||||
const content: RedirectURLsJson =
|
||||
loginHint && loginHint.length <= MAX_LOGIN_HINT_LENGTH ? { ...redirectUrls, loginHint } : redirectUrls;
|
||||
|
||||
return userService
|
||||
.startIdentityProviderIntent({
|
||||
idpId,
|
||||
content: {
|
||||
case: "urls",
|
||||
value: urls,
|
||||
value: content,
|
||||
},
|
||||
})
|
||||
.then(async (resp) => {
|
||||
|
||||
Reference in New Issue
Block a user