diff --git a/apps/cli/src/legacy/commands/sso/update/SIDE_EFFECTS.md b/apps/cli/src/legacy/commands/sso/update/SIDE_EFFECTS.md index 961ea33239..4fa58fb871 100644 --- a/apps/cli/src/legacy/commands/sso/update/SIDE_EFFECTS.md +++ b/apps/cli/src/legacy/commands/sso/update/SIDE_EFFECTS.md @@ -20,13 +20,13 @@ ## API Routes -| Method | Path | Auth | Request body | Response (used fields) | -| ------ | ------------------------------------------------------------ | ------------ | ------------------------------------------------------------------------------- | ------------------------------------------------------------------ | -| `GET` | `/v1/projects/{ref}/config/auth/sso/providers/{provider_id}` | Bearer token | none | `{id, saml?, domains?, created_at?, updated_at?}` | -| `PUT` | `/v1/projects/{ref}/config/auth/sso/providers/{provider_id}` | Bearer token | `{metadata_xml?, metadata_url?, domains?, attribute_mapping?, name_id_format?}` | `{id, saml?, domains?, ...}` (parsed loosely) | -| `GET` | `` | none | `Accept: application/xml`, 10s timeout | XML body (UTF-8) — validation when `--skip-url-validation` not set | -| `GET` | `/v1/projects/{ref}` | Bearer token | none | `{organization_slug}` — upgrade-gate side-call on 4xx | -| `GET` | `/v1/organizations/{slug}/entitlements` | Bearer token | none | `{entitlements[].feature.key, .hasAccess}` — upgrade-gate | +| Method | Path | Auth | Request body | Response (used fields) | +| ------ | ------------------------------------------------------------ | ------------ | ------------------------------------------------------------------------------ | ------------------------------------------------------------------ | +| `GET` | `/v1/projects/{ref}/config/auth/sso/providers/{provider_id}` | Bearer token | none | `{id, saml?, domains?, created_at?, updated_at?}` | +| `PUT` | `/v1/projects/{ref}/config/auth/sso/providers/{provider_id}` | Bearer token | `{metadata_xml?, metadata_url?, domains, attribute_mapping?, name_id_format?}` | `{id, saml?, domains?, ...}` (parsed loosely) | +| `GET` | `` | none | `Accept: application/xml`, 10s timeout | XML body (UTF-8) — validation when `--skip-url-validation` not set | +| `GET` | `/v1/projects/{ref}` | Bearer token | none | `{organization_slug}` — upgrade-gate side-call on 4xx | +| `GET` | `/v1/organizations/{slug}/entitlements` | Bearer token | none | `{entitlements[].feature.key, .hasAccess}` — upgrade-gate | Bypasses the typed Management API client for the PUT so user-supplied keys inside `attribute_mapping.keys.` (e.g. `default`) are preserved verbatim. The initial @@ -83,6 +83,7 @@ Single `success` event with the parsed response as data. - `--metadata-file` and `--metadata-url` are mutually exclusive. - Always performs the GET pre-check (matches Go's `update.go:42`), regardless of whether `--add-domains` / `--remove-domains` are used. - Domain merge: removals are applied first, then additions. Go uses a `map[string]bool` so the resulting order is **unordered**; consumers must sort if comparing. +- **`domains` is always present in the PUT body** (CLI-1981): Go's `--add-domains`/`--remove-domains` default to a non-nil `[]string{}` (`cmd/sso.go:171-172`), so `update.go:84`'s `!= nil` merge gate is always true from the CLI. With no domain flags (or an explicit empty `--domains=`) the body carries the recomputed existing set; when the provider has no domains it is the literal `"domains":[]` (non-nil `*[]string` under `omitempty` — verified by live capture against the Go binary). - Metadata URL validation error message: `only HTTPS Metadata URLs are supported Use --skip-url-validation to suppress this error.` (single trailing period — matches Go's `update.go:69`, which wraps with `%w Use --skip-url-validation to suppress this error.`; differs from `sso add`'s variant which omits the trailing period). - The `## Attribute Mapping` / `## SAML 2.0 Metadata XML` sections are emitted as plain markdown (heading + fence). Visual styling of the headings does not match Go's Glamour-rendered output; the table portion and the XML body inside the fence are byte-parity (see `formatSsoMetadataXml`). - **PUT failure message reuses the GET error string**: a non-2xx PUT response produces `unexpected error fetching identity provider: ` — note "fetching" not "updating". This matches Go's `update.go:133` verbatim. diff --git a/apps/cli/src/legacy/commands/sso/update/update.handler.ts b/apps/cli/src/legacy/commands/sso/update/update.handler.ts index aba1d6833f..8df976a366 100644 --- a/apps/cli/src/legacy/commands/sso/update/update.handler.ts +++ b/apps/cli/src/legacy/commands/sso/update/update.handler.ts @@ -115,13 +115,15 @@ function mergeDomains( add: ReadonlyArray, remove: ReadonlyArray, ): ReadonlyArray { - // Mirrors Go's `update.go:93-117` — seed from current domains, apply + // Mirrors Go's `update.go:84-109` — seed from current domains, apply // removals, then add new entries. Go uses a `map[string]bool`, so iteration - // order is unspecified; integration tests sort before asserting. + // order is unspecified; integration tests sort before asserting. Go's seed + // check is nil-ness only (`domain.Domain != nil`), so an empty-string domain + // from the GET response is kept, not filtered. const set = new Set(); if (existing !== undefined) { for (const item of existing) { - if (typeof item.domain === "string" && item.domain.length > 0) { + if (typeof item.domain === "string") { set.add(item.domain); } } @@ -228,7 +230,16 @@ export const legacySsoUpdate = Effect.fn("legacy.sso.update")(function* ( if (flags.domains.length > 0) { body["domains"] = [...flags.domains]; - } else if (flags.addDomains.length > 0 || flags.removeDomains.length > 0) { + } else { + // Go's `update.go:84` reads as gating the merge on + // `params.AddDomains != nil || params.RemoveDomains != nil`, but + // `cmd/sso.go:171-172` declares both flags with a non-nil `[]string{}` + // default and passes them unconditionally — so from the CLI that + // condition is always true and every `sso update` PUT recomputes and + // sends `domains`, even when no domain flag was passed. `body.Domains` + // is a non-nil `*[]string` under `json:"domains,omitempty"`, so an + // empty merged set serializes as `"domains":[]`, never omitted + // (CLI-1981; live-captured against the Go binary). body["domains"] = mergeDomains(existing.domains, flags.addDomains, flags.removeDomains); } diff --git a/apps/cli/src/legacy/commands/sso/update/update.integration.test.ts b/apps/cli/src/legacy/commands/sso/update/update.integration.test.ts index c76206a29b..757e7b46de 100644 --- a/apps/cli/src/legacy/commands/sso/update/update.integration.test.ts +++ b/apps/cli/src/legacy/commands/sso/update/update.integration.test.ts @@ -476,12 +476,84 @@ describe("legacy sso update integration", () => { }).pipe(Effect.provide(layer)); }); - it.live("no domain flag set → body.domains omitted", () => { + it.live("no domain flag set → PUT still sends the recomputed existing domain set", () => { + // Go's `--add-domains`/`--remove-domains` default to a non-nil `[]string{}` + // (`cmd/sso.go:171-172`), so `update.go:84`'s `!= nil` gate is always true + // from the CLI — every `sso update` enters the merge and sends `domains`, + // even when no domain flag was passed (CLI-1981). Live-captured Go PUT: + // `{"domains":["old1.com","old2.com"]}`. const { layer, api } = setup(); return Effect.gen(function* () { yield* legacySsoUpdate(defaultFlags); const putReq = api.requests.find((r) => r.method === "PUT"); - expect((putReq?.body as { domains?: string[] })?.domains).toBeUndefined(); + const domains = (putReq?.body as { domains: string[] })?.domains; + // Go map iteration is unordered — sort before asserting. + expect([...domains].sort()).toEqual(["old1.com", "old2.com"]); + }).pipe(Effect.provide(layer)); + }); + + it.live( + "no domain flags + provider with no domains → PUT sends domains: [] (not omitted)", + () => { + // Go sets `body.Domains` to a non-nil pointer to `make([]string, 0)`, and + // `json:"domains,omitempty"` never omits a non-nil pointer — live-captured + // Go PUT body is exactly `{"domains":[]}`. + const { layer, api } = setup({ getBody: { ...EXISTING_PROVIDER, domains: [] } }); + return Effect.gen(function* () { + yield* legacySsoUpdate(defaultFlags); + const putReq = api.requests.find((r) => r.method === "PUT"); + const body = putReq?.body as Record; + expect(Object.keys(body)).toContain("domains"); + expect(body["domains"]).toEqual([]); + }).pipe(Effect.provide(layer)); + }, + ); + + it.live("no domain flags + GET response missing domains entirely → PUT sends domains: []", () => { + // Go's seed loop is skipped when `getResp.JSON200.Domains == nil`, leaving + // the merged set empty — same `{"domains":[]}` bytes as the empty-list case. + const { domains: _omitted, ...providerWithoutDomains } = EXISTING_PROVIDER; + const { layer, api } = setup({ getBody: providerWithoutDomains }); + return Effect.gen(function* () { + yield* legacySsoUpdate(defaultFlags); + const putReq = api.requests.find((r) => r.method === "PUT"); + const body = putReq?.body as Record; + expect(Object.keys(body)).toContain("domains"); + expect(body["domains"]).toEqual([]); + }).pipe(Effect.provide(layer)); + }); + + it.live("explicit empty --domains= falls into the merge and resends the existing set", () => { + // `--domains=` parses to an empty slice, so Go's `len(params.Domains) != 0` + // replace gate is false and the merge branch runs with no add/remove — + // live-captured Go PUT resends the existing domains, it does NOT replace + // them with an empty list. + const { layer, api } = setup({ + cliArgs: ["sso", "update", VALID_PROVIDER_ID, "--domains="], + }); + return Effect.gen(function* () { + yield* legacySsoUpdate({ ...defaultFlags, domains: [] }); + const putReq = api.requests.find((r) => r.method === "PUT"); + const domains = (putReq?.body as { domains: string[] })?.domains; + expect([...domains].sort()).toEqual(["old1.com", "old2.com"]); + }).pipe(Effect.provide(layer)); + }); + + it.live("merge keeps empty-string domains and skips entries without a domain field", () => { + // Go's seed check is nil-ness only (`domain.Domain != nil`, + // `update.go:89`): an empty-string domain from the GET response stays in + // the merged set, while an entry missing the field entirely is skipped. + const { layer, api } = setup({ + getBody: { + ...EXISTING_PROVIDER, + domains: [{ id: "d1", domain: "" }, { id: "d2", domain: "old1.com" }, { id: "d3" }], + }, + }); + return Effect.gen(function* () { + yield* legacySsoUpdate(defaultFlags); + const putReq = api.requests.find((r) => r.method === "PUT"); + const domains = (putReq?.body as { domains: string[] })?.domains; + expect([...domains].sort()).toEqual(["", "old1.com"]); }).pipe(Effect.provide(layer)); });