Skip to content

Commit e19ea65

Browse files
BilalG1BilalG1
committed
fix(saml): use whole-entry config writes; correct test request shapes
E2E tests + dashboard SSO page were writing per-field deep dot-keys like `auth.saml.connections.X.displayName`, which the config normalizer drops because the parent record entry doesn't yet exist when the connection is being created. Match the existing auth.oauth.providers convention: write the whole connection entry as a single value on create. Also fixed two test-harness issues uncovered while running the suite: - Round-trip ACS POST was using niceBackendFetch which always JSON.stringifies the body. Switched to plain niceFetch so URLSearchParams gets sent as application/x-www-form-urlencoded. - Mock IdP /metadata returns application/xml, which makes niceFetch return ArrayBuffer; added a TextDecoder pass before regex matching.
1 parent 18a4d19 commit e19ea65

5 files changed

Lines changed: 72 additions & 44 deletions

File tree

‎apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/sso/page-client.tsx‎

Lines changed: 15 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -144,19 +144,23 @@ function CreateDialog({ open, onOpenChange }: { open: boolean, onOpenChange: (op
144144
okButton={{ label: "Create" }}
145145
cancelButton
146146
onSubmit={async (values) => {
147-
const overlay: Record<string, unknown> = {
148-
[`auth.saml.connections.${values.id}.displayName`]: values.displayName,
149-
[`auth.saml.connections.${values.id}.allowSignIn`]: values.allowSignIn,
150-
[`auth.saml.connections.${values.id}.idpEntityId`]: values.idpEntityId,
151-
[`auth.saml.connections.${values.id}.idpSsoUrl`]: values.idpSsoUrl,
152-
[`auth.saml.connections.${values.id}.idpCertificate`]: (values.idpCertificate ?? "").replace(/-----BEGIN CERTIFICATE-----|-----END CERTIFICATE-----|\s+/g, ""),
153-
};
154-
if (values.domain) {
155-
overlay[`auth.saml.connections.${values.id}.domain`] = values.domain;
156-
}
147+
// Set the whole connection entry as a single value. Deep dot-keys
148+
// (e.g. `auth.saml.connections.X.displayName`) get dropped during
149+
// config normalization when the parent record entry doesn't yet
150+
// exist — same convention as auth.oauth.providers in the
151+
// auth-methods page.
157152
await updateConfig({
158153
adminApp: stackAdminApp,
159-
configUpdate: overlay as Parameters<typeof updateConfig>[0]["configUpdate"],
154+
configUpdate: {
155+
[`auth.saml.connections.${values.id}`]: {
156+
displayName: values.displayName,
157+
allowSignIn: values.allowSignIn,
158+
domain: values.domain || undefined,
159+
idpEntityId: values.idpEntityId,
160+
idpSsoUrl: values.idpSsoUrl,
161+
idpCertificate: (values.idpCertificate ?? "").replace(/-----BEGIN CERTIFICATE-----|-----END CERTIFICATE-----|\s+/g, ""),
162+
},
163+
} as Parameters<typeof updateConfig>[0]["configUpdate"],
160164
pushable: true,
161165
});
162166
}}

‎apps/e2e/tests/backend/endpoints/api/v1/auth/saml/discover.test.ts‎

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -15,16 +15,21 @@ import { Project, niceBackendFetch } from "../../../../../backend-helpers";
1515

1616
async function createProjectWithSamlConnection(slug: string, domain: string) {
1717
const { projectId } = await Project.createAndSwitch();
18-
// Push the SAML connection at the environment level — that's where the
19-
// IdP-side fields live. The discovery endpoint reads from the rendered
20-
// organization config which folds in env overrides.
18+
// Set the entire connection entry as a single value. The override
19+
// system handles `auth.saml.connections.{id}: {full object}` cleanly,
20+
// but per-field deep dot-keys (e.g. .displayName) on a record entry
21+
// that doesn't yet exist get dropped during config normalization with
22+
// onDotIntoNonObject="ignore" — same convention as auth.oauth.providers
23+
// (see auth-methods/page-client.tsx).
2124
await Project.updateConfig({
22-
[`auth.saml.connections.${slug}.displayName`]: `${slug} SSO`,
23-
[`auth.saml.connections.${slug}.allowSignIn`]: true,
24-
[`auth.saml.connections.${slug}.domain`]: domain,
25-
[`auth.saml.connections.${slug}.idpEntityId`]: `https://idp.${domain}/saml/metadata`,
26-
[`auth.saml.connections.${slug}.idpSsoUrl`]: `https://idp.${domain}/saml/sso`,
27-
[`auth.saml.connections.${slug}.idpCertificate`]: "MIICertificatePlaceholderForDiscoveryTest=",
25+
[`auth.saml.connections.${slug}`]: {
26+
displayName: `${slug} SSO`,
27+
allowSignIn: true,
28+
domain,
29+
idpEntityId: `https://idp.${domain}/saml/metadata`,
30+
idpSsoUrl: `https://idp.${domain}/saml/sso`,
31+
idpCertificate: "MIICertificatePlaceholderForDiscoveryTest=",
32+
},
2833
});
2934
return { projectId };
3035
}

‎apps/e2e/tests/backend/endpoints/api/v1/auth/saml/login.test.ts‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,11 +18,13 @@ async function setupProjectWithSamlConnection(slug: string, idpHost: string) {
1818
await Project.createAndSwitch();
1919
await InternalApiKey.createAndSetProjectKeys();
2020
await Project.updateConfig({
21-
[`auth.saml.connections.${slug}.displayName`]: `${slug} SSO`,
22-
[`auth.saml.connections.${slug}.allowSignIn`]: true,
23-
[`auth.saml.connections.${slug}.idpEntityId`]: `https://${idpHost}/saml/metadata`,
24-
[`auth.saml.connections.${slug}.idpSsoUrl`]: `https://${idpHost}/saml/sso`,
25-
[`auth.saml.connections.${slug}.idpCertificate`]: "MIICertificatePlaceholderForLoginTest=",
21+
[`auth.saml.connections.${slug}`]: {
22+
displayName: `${slug} SSO`,
23+
allowSignIn: true,
24+
idpEntityId: `https://${idpHost}/saml/metadata`,
25+
idpSsoUrl: `https://${idpHost}/saml/sso`,
26+
idpCertificate: "MIICertificatePlaceholderForLoginTest=",
27+
},
2628
});
2729
}
2830

@@ -91,6 +93,8 @@ it("returns 404 for an unknown connection ID", async ({ expect }) => {
9193

9294
it("returns 403 when allowSignIn is false on the connection", async ({ expect }) => {
9395
await setupProjectWithSamlConnection("acme", "idp.acme.test");
96+
// The connection already exists, so deep-key updates work (the parent
97+
// record entry is navigable now).
9498
await Project.updateConfig({ "auth.saml.connections.acme.allowSignIn": false });
9599

96100
const response = await niceBackendFetch("/api/v1/auth/saml/login/acme", {

‎apps/e2e/tests/backend/endpoints/api/v1/auth/saml/metadata.test.ts‎

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -12,11 +12,13 @@ import { Project, niceBackendFetch } from "../../../../../backend-helpers";
1212
async function setupSamlConnection(slug: string) {
1313
const { projectId } = await Project.createAndSwitch();
1414
await Project.updateConfig({
15-
[`auth.saml.connections.${slug}.displayName`]: `${slug} SSO`,
16-
[`auth.saml.connections.${slug}.allowSignIn`]: true,
17-
[`auth.saml.connections.${slug}.idpEntityId`]: `https://idp.${slug}.test/saml/metadata`,
18-
[`auth.saml.connections.${slug}.idpSsoUrl`]: `https://idp.${slug}.test/saml/sso`,
19-
[`auth.saml.connections.${slug}.idpCertificate`]: "MIICertificatePlaceholderForMetadataTest=",
15+
[`auth.saml.connections.${slug}`]: {
16+
displayName: `${slug} SSO`,
17+
allowSignIn: true,
18+
idpEntityId: `https://idp.${slug}.test/saml/metadata`,
19+
idpSsoUrl: `https://idp.${slug}.test/saml/sso`,
20+
idpCertificate: "MIICertificatePlaceholderForMetadataTest=",
21+
},
2022
});
2123
return { projectId };
2224
}
@@ -54,11 +56,13 @@ it("returns 404 for an unknown connection ID", async ({ expect }) => {
5456

5557
it("returns 404 when the connection exists but has no IdP cert configured", async ({ expect }) => {
5658
const { projectId } = await Project.createAndSwitch();
57-
// Create a connection at branch level but skip the env-level IdP fields.
59+
// Create a connection but skip the IdP-side fields.
5860
await Project.updateConfig({
59-
"auth.saml.connections.partial.displayName": "Partial",
60-
"auth.saml.connections.partial.allowSignIn": true,
61-
// No idpEntityId / idpSsoUrl / idpCertificate.
61+
"auth.saml.connections.partial": {
62+
displayName: "Partial",
63+
allowSignIn: true,
64+
// No idpEntityId / idpSsoUrl / idpCertificate.
65+
},
6266
});
6367

6468
const response = await niceBackendFetch(

‎apps/e2e/tests/backend/endpoints/api/v1/auth/saml/round-trip.test.ts‎

Lines changed: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import { localhostUrl } from "../../../../../../helpers/ports";
2727
import { InternalApiKey, Project, backendContext, niceBackendFetch } from "../../../../../backend-helpers";
2828

2929
const MOCK_SAML_BASE = localhostUrl("15");
30+
const BACKEND_BASE = localhostUrl("02");
3031

3132
// ---------- helpers ----------
3233

@@ -39,7 +40,10 @@ async function fetchMockIdpCertificate(tenantSlug: string): Promise<{
3940
if (res.status !== 200) {
4041
throw new Error(`Mock IdP returned ${res.status} for ${tenantSlug} metadata — is mock-saml-idp running on port 8115?`);
4142
}
42-
const xml = res.body as string;
43+
// application/xml content-type makes niceFetch return ArrayBuffer; decode it.
44+
const xml = typeof res.body === "string"
45+
? res.body
46+
: new TextDecoder("utf-8").decode(res.body as ArrayBuffer);
4347
const entityIdMatch = xml.match(/entityID="([^"]+)"/);
4448
const ssoMatch = xml.match(/Binding="urn:oasis:names:tc:SAML:2\.0:bindings:HTTP-Redirect"[^>]*Location="([^"]+)"/);
4549
const certMatch = xml.match(/<X509Certificate>([\s\S]+?)<\/X509Certificate>/);
@@ -58,11 +62,13 @@ async function setupProjectWithMockSamlConnection(connectionId: string, tenantSl
5862
await InternalApiKey.createAndSetProjectKeys();
5963
const idp = await fetchMockIdpCertificate(tenantSlug);
6064
await Project.updateConfig({
61-
[`auth.saml.connections.${connectionId}.displayName`]: `${connectionId} SSO`,
62-
[`auth.saml.connections.${connectionId}.allowSignIn`]: true,
63-
[`auth.saml.connections.${connectionId}.idpEntityId`]: idp.entityId,
64-
[`auth.saml.connections.${connectionId}.idpSsoUrl`]: idp.ssoUrl,
65-
[`auth.saml.connections.${connectionId}.idpCertificate`]: idp.certificate,
65+
[`auth.saml.connections.${connectionId}`]: {
66+
displayName: `${connectionId} SSO`,
67+
allowSignIn: true,
68+
idpEntityId: idp.entityId,
69+
idpSsoUrl: idp.ssoUrl,
70+
idpCertificate: idp.certificate,
71+
},
6672
});
6773
return { connectionId, tenantSlug };
6874
}
@@ -120,13 +126,18 @@ async function runSamlRoundTrip(options: {
120126
}
121127

122128
// Step 3: extract SAMLResponse from the auto-POST HTML.
123-
const html = idpLoginRes.body as string;
129+
const html = typeof idpLoginRes.body === "string"
130+
? idpLoginRes.body
131+
: new TextDecoder("utf-8").decode(idpLoginRes.body as ArrayBuffer);
124132
const samlResponseMatch = html.match(/name="SAMLResponse" value="([^"]+)"/);
125133
if (!samlResponseMatch) throw new Error(`Mock IdP did not return a SAMLResponse form: ${html.slice(0, 200)}`);
126134
const samlResponse = samlResponseMatch[1].replace(/&#x2F;/g, "/").replace(/&#x3D;/g, "=").replace(/&amp;/g, "&");
127135

128-
// Step 4: POST to ACS.
129-
const acsRes = await niceBackendFetch(`/api/v1/auth/saml/acs/${options.connectionId}`, {
136+
// Step 4: POST to ACS — use niceFetch directly so URLSearchParams gets
137+
// sent as application/x-www-form-urlencoded. niceBackendFetch always
138+
// JSON.stringifies the body, which doesn't work for the IdP-style
139+
// form POST that ACS expects.
140+
const acsRes = await niceFetch(`${BACKEND_BASE}/api/v1/auth/saml/acs/${options.connectionId}`, {
130141
method: "POST",
131142
redirect: "manual",
132143
body: new URLSearchParams({
@@ -223,7 +234,7 @@ it("rejects replay of a previously-consumed assertion", async ({ expect }) => {
223234
// Replay the same SAMLResponse — backend should reject because the
224235
// SamlOuterInfo row was deleted at the end of the first ACS call, so
225236
// the InResponseTo lookup misses.
226-
const replayRes = await niceBackendFetch(`/api/v1/auth/saml/acs/acme`, {
237+
const replayRes = await niceFetch(`${BACKEND_BASE}/api/v1/auth/saml/acs/acme`, {
227238
method: "POST",
228239
redirect: "manual",
229240
body: new URLSearchParams({

0 commit comments

Comments
 (0)