mirror of
https://github.com/stack-auth/stack.git
synced 2026-06-04 21:04:37 +08:00
fix(saml): address PR review — absolute SP URLs, domain normalization, port-prefix-aware tests
- Dashboard SSO page now renders absolute SP metadata + ACS URLs from
NEXT_PUBLIC_BROWSER_STACK_API_URL so values are paste-ready into Okta
/ Azure AD / Google Workspace consoles.
- Trim + lowercase email domain on write so trailing whitespace can't
silently break discovery (matching is case-insensitive but not trim).
- Guard DeleteDialog displayName against stale deleteId after a config
refresh removed the entry.
- signInWithSso gains the same browser-only guard as signInWithSaml so
SSR callers get a coherent error from the right method.
- SAML e2e tests now derive redirect_uri from localhostUrl("01") and
the mock IdP base from suffix 42 (matches mock-saml-idp default port);
align both wait-on URLs in the workflows. Round-trip happy path now
exchanges the OAuth code, asserts is_new_user + the JIT-created
user's primary_email and display_name, and asserts the callback origin
matches the redirect_uri so a custom-port job can no longer pass while
wired to a non-running dashboard.
This commit is contained in:
Bilal Godil
parent
e37e5f37b5
commit
be4560b416
@ -145,7 +145,7 @@ jobs:
|
||||
with:
|
||||
run: pnpm run start:mock-saml-idp --log-order=stream &
|
||||
wait-on: |
|
||||
http://localhost:8115/idp
|
||||
http://localhost:8142/idp
|
||||
tail: true
|
||||
wait-for: 30s
|
||||
log-output-if: true
|
||||
|
||||
@ -141,7 +141,7 @@ jobs:
|
||||
with:
|
||||
run: pnpm run start:mock-saml-idp --log-order=stream &
|
||||
wait-on: |
|
||||
http://localhost:6715/idp
|
||||
http://localhost:6742/idp
|
||||
tail: true
|
||||
wait-for: 30s
|
||||
log-output-if: true
|
||||
|
||||
@ -3,12 +3,18 @@
|
||||
import { SmartFormDialog } from "@/components/form-dialog";
|
||||
import { ActionDialog, Alert, Button, Card, CardContent, CardHeader, Typography } from "@/components/ui";
|
||||
import { useUpdateConfig } from "@/lib/config-update";
|
||||
import { getPublicEnvVar } from "@/lib/env";
|
||||
import React, { useState } from "react";
|
||||
import * as yup from "yup";
|
||||
import { AppEnabledGuard } from "../app-enabled-guard";
|
||||
import { PageLayout } from "../page-layout";
|
||||
import { useAdminApp } from "../use-admin-app";
|
||||
|
||||
function getBrowserApiBase(): string {
|
||||
const url = getPublicEnvVar("NEXT_PUBLIC_BROWSER_STACK_API_URL") ?? getPublicEnvVar("NEXT_PUBLIC_STACK_API_URL") ?? "";
|
||||
return url.replace(/\/+$/, "");
|
||||
}
|
||||
|
||||
/**
|
||||
* Dashboard for managing SAML SSO connections on the current project.
|
||||
*
|
||||
@ -40,6 +46,7 @@ function PageContent() {
|
||||
const [deleteId, setDeleteId] = useState<string | null>(null);
|
||||
|
||||
const connectionEntries = Object.entries(connections);
|
||||
const apiBase = getBrowserApiBase();
|
||||
|
||||
return (
|
||||
<PageLayout
|
||||
@ -93,12 +100,12 @@ function PageContent() {
|
||||
</Typography>
|
||||
<DetailRow
|
||||
label="SP metadata URL"
|
||||
value={`/api/v1/auth/saml/metadata/${id}?project_id=${project.id}`}
|
||||
value={`${apiBase}/api/v1/auth/saml/metadata/${id}?project_id=${project.id}`}
|
||||
mono
|
||||
/>
|
||||
<DetailRow
|
||||
label="ACS URL"
|
||||
value={`/api/v1/auth/saml/acs/${id}`}
|
||||
value={`${apiBase}/api/v1/auth/saml/acs/${id}`}
|
||||
mono
|
||||
/>
|
||||
</div>
|
||||
@ -111,7 +118,9 @@ function PageContent() {
|
||||
<DeleteDialog
|
||||
connectionId={deleteId}
|
||||
onClose={() => setDeleteId(null)}
|
||||
displayName={deleteId ? connections[deleteId].displayName : null}
|
||||
// Guard against stale deleteId after a config refresh removed the
|
||||
// entry — index types claim non-undefined but the key may be gone.
|
||||
displayName={deleteId && Object.hasOwn(connections, deleteId) ? connections[deleteId].displayName : null}
|
||||
/>
|
||||
</PageLayout>
|
||||
);
|
||||
@ -160,13 +169,17 @@ function CreateDialog({ open, onOpenChange }: { open: boolean, onOpenChange: (op
|
||||
// config normalization when the parent record entry doesn't yet
|
||||
// exist — same convention as auth.oauth.providers in the
|
||||
// auth-methods page.
|
||||
// Normalize domain on write — discovery does case-insensitive
|
||||
// matching but does not trim, so trailing whitespace from a
|
||||
// pasted value would silently break lookups.
|
||||
const normalizedDomain = values.domain?.trim().toLowerCase() || undefined;
|
||||
await updateConfig({
|
||||
adminApp: stackAdminApp,
|
||||
configUpdate: {
|
||||
[`auth.saml.connections.${values.id}`]: {
|
||||
displayName: values.displayName,
|
||||
allowSignIn: values.allowSignIn,
|
||||
domain: values.domain || undefined,
|
||||
domain: normalizedDomain,
|
||||
idpEntityId: values.idpEntityId,
|
||||
idpSsoUrl: values.idpSsoUrl,
|
||||
idpCertificate: (values.idpCertificate ?? "").replace(/-----BEGIN CERTIFICATE-----|-----END CERTIFICATE-----|\s+/g, ""),
|
||||
|
||||
@ -1,4 +1,5 @@
|
||||
import { it } from "../../../../../../helpers";
|
||||
import { localhostUrl } from "../../../../../../helpers/ports";
|
||||
import { InternalApiKey, Project, backendContext, niceBackendFetch } from "../../../../../backend-helpers";
|
||||
|
||||
/**
|
||||
@ -36,7 +37,7 @@ function loginQuery() {
|
||||
return {
|
||||
client_id: !branchId ? projectKeys.projectId : `${projectKeys.projectId}#${branchId}`,
|
||||
client_secret: projectKeys.publishableClientKey ?? "",
|
||||
redirect_uri: "http://localhost:8101/handler/oauth-callback",
|
||||
redirect_uri: localhostUrl("01", "/handler/oauth-callback"),
|
||||
scope: "legacy",
|
||||
state: "this-is-some-state",
|
||||
grant_type: "authorization_code",
|
||||
|
||||
@ -2,7 +2,7 @@
|
||||
* Full SAML SP-initiated round-trip e2e tests.
|
||||
*
|
||||
* Drives the entire flow against the running mock-saml-idp service on
|
||||
* port 8115:
|
||||
* the mock IdP port (default suffix `42`, e.g. 8142 with prefix 81):
|
||||
*
|
||||
* 1. Setup project with a SAML connection pointing at the mock IdP
|
||||
* (cert fetched from mock metadata so it matches the mock's
|
||||
@ -26,8 +26,9 @@ import { it, niceFetch } from "../../../../../../helpers";
|
||||
import { localhostUrl } from "../../../../../../helpers/ports";
|
||||
import { InternalApiKey, Project, backendContext, niceBackendFetch } from "../../../../../backend-helpers";
|
||||
|
||||
const MOCK_SAML_BASE = localhostUrl("15");
|
||||
const MOCK_SAML_BASE = localhostUrl("42");
|
||||
const BACKEND_BASE = localhostUrl("02");
|
||||
const DASHBOARD_REDIRECT_URI = localhostUrl("01", "/handler/oauth-callback");
|
||||
|
||||
// ---------- helpers ----------
|
||||
|
||||
@ -38,7 +39,7 @@ async function fetchMockIdpCertificate(tenantSlug: string): Promise<{
|
||||
}> {
|
||||
const res = await niceFetch(`${MOCK_SAML_BASE}/idp/${tenantSlug}/metadata`);
|
||||
if (res.status !== 200) {
|
||||
throw new Error(`Mock IdP returned ${res.status} for ${tenantSlug} metadata — is mock-saml-idp running on port 8115?`);
|
||||
throw new Error(`Mock IdP returned ${res.status} for ${tenantSlug} metadata — is mock-saml-idp running on ${MOCK_SAML_BASE}?`);
|
||||
}
|
||||
// application/xml content-type makes niceFetch return ArrayBuffer; decode it.
|
||||
const xml = typeof res.body === "string"
|
||||
@ -74,6 +75,8 @@ async function setupProjectWithMockSamlConnection(connectionId: string, tenantSl
|
||||
return { connectionId, tenantSlug };
|
||||
}
|
||||
|
||||
const ROUND_TRIP_CODE_VERIFIER = "round-trip-code-challenge";
|
||||
|
||||
function loginQuery() {
|
||||
const projectKeys = backendContext.value.projectKeys;
|
||||
if (projectKeys === "no-project") throw new Error("No project keys");
|
||||
@ -81,12 +84,13 @@ function loginQuery() {
|
||||
return {
|
||||
client_id: !branchId ? projectKeys.projectId : `${projectKeys.projectId}#${branchId}`,
|
||||
client_secret: projectKeys.publishableClientKey ?? "",
|
||||
redirect_uri: "http://localhost:8101/handler/oauth-callback",
|
||||
redirect_uri: DASHBOARD_REDIRECT_URI,
|
||||
scope: "legacy",
|
||||
state: "round-trip-test-state",
|
||||
grant_type: "authorization_code",
|
||||
code_challenge: "round-trip-code-challenge",
|
||||
code_challenge_method: "S256",
|
||||
// Use plain so the verifier used at token exchange equals the challenge.
|
||||
code_challenge: ROUND_TRIP_CODE_VERIFIER,
|
||||
code_challenge_method: "plain",
|
||||
response_type: "code",
|
||||
};
|
||||
}
|
||||
@ -175,8 +179,45 @@ it("full SP-initiated round trip: new user JIT-created", async ({ expect }) => {
|
||||
const location = acsRes.headers.get("location");
|
||||
expect(location).toBeTruthy();
|
||||
const callbackUrl = new URL(location!);
|
||||
// Should contain an OAuth `code` query param.
|
||||
expect(callbackUrl.searchParams.get("code")).toBeTruthy();
|
||||
// Callback origin must match the dashboard redirect_uri the test
|
||||
// submitted, not whatever the default port is — otherwise this test
|
||||
// can pass under a custom port prefix while wired to a non-running app.
|
||||
expect(callbackUrl.origin).toBe(new URL(DASHBOARD_REDIRECT_URI).origin);
|
||||
const code = callbackUrl.searchParams.get("code");
|
||||
expect(code).toBeTruthy();
|
||||
|
||||
// Exchange the code for tokens and verify the JIT user was actually
|
||||
// persisted with the email + display name from the SAML assertion —
|
||||
// otherwise the test only proves "ACS issued a redirect", not "the
|
||||
// SAML account/user link was saved".
|
||||
const projectKeys = backendContext.value.projectKeys;
|
||||
if (projectKeys === "no-project") throw new Error("No project keys");
|
||||
const branchId = backendContext.value.currentBranchId;
|
||||
const tokenRes = await niceBackendFetch("/api/v1/auth/oauth/token", {
|
||||
method: "POST",
|
||||
accessType: "client",
|
||||
body: {
|
||||
client_id: !branchId ? projectKeys.projectId : `${projectKeys.projectId}#${branchId}`,
|
||||
client_secret: projectKeys.publishableClientKey ?? "",
|
||||
code,
|
||||
redirect_uri: DASHBOARD_REDIRECT_URI,
|
||||
code_verifier: ROUND_TRIP_CODE_VERIFIER,
|
||||
grant_type: "authorization_code",
|
||||
},
|
||||
});
|
||||
expect(tokenRes.status).toBe(200);
|
||||
const accessToken = (tokenRes.body as { access_token: string }).access_token;
|
||||
expect(accessToken).toBeTruthy();
|
||||
expect((tokenRes.body as { is_new_user: boolean }).is_new_user).toBe(true);
|
||||
|
||||
const meRes = await niceBackendFetch("/api/v1/users/me", {
|
||||
accessType: "client",
|
||||
headers: { "x-stack-access-token": accessToken },
|
||||
});
|
||||
expect(meRes.status).toBe(200);
|
||||
const me = meRes.body as { primary_email: string | null, display_name: string | null };
|
||||
expect(me.primary_email).toBe("alice@acme.test");
|
||||
expect(me.display_name).toBe("Alice");
|
||||
});
|
||||
|
||||
it("rejects an assertion with the wrong audience", async ({ expect }) => {
|
||||
|
||||
@ -21,7 +21,7 @@ export default function SamlDemoPage() {
|
||||
<Typography className="mb-6 text-gray-600">
|
||||
Manual end-to-end check for the SAML round-trip against the local mock IdP. Seed the dummy
|
||||
project with <code>STACK_SEED_ENABLE_SAML=true</code> first; that pre-creates two
|
||||
connections (acme + globex) pointing at <code>localhost:8115</code>.
|
||||
connections (acme + globex) pointing at <code>localhost:8142</code>.
|
||||
</Typography>
|
||||
|
||||
<div className="grid gap-6">
|
||||
|
||||
@ -2895,6 +2895,9 @@ export class _StackClientAppImplIncomplete<HasTokenStore extends boolean, Projec
|
||||
email: string,
|
||||
returnTo?: string,
|
||||
}) {
|
||||
if (typeof window === "undefined") {
|
||||
throw new Error("signInWithSso can currently only be called in a browser environment");
|
||||
}
|
||||
const connection = await this._interface.discoverSamlConnection(options.email);
|
||||
if (!connection) {
|
||||
throw new Error(`No SSO connection configured for email domain "${options.email.split("@").pop()}"`);
|
||||
|
||||
Loading…
Reference in New Issue
Block a user