From bb94ec7ee83faa35135cf6b968be75f35a5e1f48 Mon Sep 17 00:00:00 2001 From: Colum Ferry Date: Tue, 28 Jul 2026 14:49:26 +0100 Subject: [PATCH] fix(cli): use cobra mutual-exclusivity template in sso add (CLI-1982) --- .../legacy/commands/sso/add/SIDE_EFFECTS.md | 2 +- .../legacy/commands/sso/add/add.handler.ts | 63 ++++++- .../commands/sso/add/add.integration.test.ts | 161 +++++++++++++++--- 3 files changed, 194 insertions(+), 32 deletions(-) diff --git a/apps/cli/src/legacy/commands/sso/add/SIDE_EFFECTS.md b/apps/cli/src/legacy/commands/sso/add/SIDE_EFFECTS.md index 350a2d34c9..91ee760cce 100644 --- a/apps/cli/src/legacy/commands/sso/add/SIDE_EFFECTS.md +++ b/apps/cli/src/legacy/commands/sso/add/SIDE_EFFECTS.md @@ -78,7 +78,7 @@ Single `success` event with the parsed response as data. ## Notes - `--type saml` is **required** (Go's `MarkFlagRequired("type")`). -- `--metadata-file` and `--metadata-url` are mutually exclusive. +- `--metadata-file` and `--metadata-url` are mutually exclusive (Go's `MarkFlagsMutuallyExclusive`, `cmd/sso.go:164`). Violations emit cobra's exact template: `if any flags in the group [metadata-file metadata-url] are set none of the others can be; [metadata-file metadata-url] were all set`. "Set" follows `pflag.Changed` semantics — an explicit empty value (`--metadata-file=`) still counts. - `--skip-url-validation` skips the HTTPS-only + 10s GET + UTF-8 body validation against the metadata URL. - Metadata URL validation error message: `only HTTPS Metadata URLs are supported Use --skip-url-validation to suppress this error` (no trailing period — matches Go's `create.go:47`; differs from `sso update`'s variant). - 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 XML body inside the fence is byte-parity via `formatSsoMetadataXml`. diff --git a/apps/cli/src/legacy/commands/sso/add/add.handler.ts b/apps/cli/src/legacy/commands/sso/add/add.handler.ts index 29b3592ba9..3a25dd346c 100644 --- a/apps/cli/src/legacy/commands/sso/add/add.handler.ts +++ b/apps/cli/src/legacy/commands/sso/add/add.handler.ts @@ -1,10 +1,14 @@ -import { Effect, Option } from "effect"; +import { Effect, Option, Stdio } from "effect"; import * as HttpClient from "effect/unstable/http/HttpClient"; import * as HttpClientRequest from "effect/unstable/http/HttpClientRequest"; import { LegacyCliConfig } from "../../../config/legacy-cli-config.service.ts"; import { LegacyProjectRefResolver } from "../../../config/legacy-project-ref.service.ts"; import { LegacyOutputFlag } from "../../../../shared/legacy/global-flags.ts"; +import { + cobraMutuallyExclusiveErrorMessage, + hasExplicitValueFlag, +} from "../../../../shared/cli/cobra-flag-groups.ts"; import { Output } from "../../../../shared/output/output.service.ts"; import { encodeGoJson, @@ -42,6 +46,37 @@ const readAttributeMapping = readAttributeMappingFile({ openError: (args) => new LegacySsoAddAttributeMappingFileError(args), }); +const SSO_ADD_COMMAND_PATH = ["sso", "add"] as const; + +/** + * `sso add`'s single mutually-exclusive group, in Go's registration order + * (`cmd/sso.go:164` — `MarkFlagsMutuallyExclusive("metadata-file", + * "metadata-url")`). Registration order determines the first bracket of + * cobra's error template; only the violating subset gets sorted. + */ +const SSO_ADD_MUTEX_GROUP = ["metadata-file", "metadata-url"] as const; + +/** + * Every value-taking (non-boolean) flag `sso add` declares + * (`add.command.ts`) — tells `hasExplicitValueFlag` which bare tokens + * consume the next argv token as their value. `--skip-url-validation` is + * this command's only boolean flag and is deliberately excluded; booleans + * never consume a following token. `--type`'s `-t` shorthand is not covered + * — the raw-argv scan only understands long flags, same limitation as + * `sso update`'s scan (a shorthand's consumed value could in principle be + * misread, but pflag would fail `-t`'s enum validation on any dash-prefixed + * value before flag groups are even checked). + */ +const SSO_ADD_VALUE_FLAG_NAMES = new Set([ + "project-ref", + "type", + "domains", + "metadata-file", + "metadata-url", + "attribute-mapping-file", + "name-id-format", +]); + export const legacySsoAdd = Effect.fn("legacy.sso.add")(function* (flags: LegacySsoAddFlags) { const output = yield* Output; const goOutputFlag = yield* LegacyOutputFlag; @@ -50,12 +85,34 @@ export const legacySsoAdd = Effect.fn("legacy.sso.add")(function* (flags: Legacy const resolver = yield* LegacyProjectRefResolver; const linkedProjectCache = yield* LegacyLinkedProjectCache; const telemetryState = yield* LegacyTelemetryState; + const stdio = yield* Stdio.Stdio; + const rawArgs = yield* stdio.args; yield* Effect.gen(function* () { - if (Option.isSome(flags.metadataFile) && Option.isSome(flags.metadataUrl)) { + // cobra runs `ValidateFlagGroups` (`command.go:1010`) before `RunE` + // (`command.go:1014`), so the mutex check must precede everything Go does + // inside `RunE` — project-ref resolution included. Keep this block first. + // + // "Set" follows cobra's `pflag.Changed` — whether the flag was passed at + // all — not the resulting value: `--metadata-file= --metadata-url x` must + // still trip the error even though the file path is empty. Scanning raw + // argv (like `sso update` does) keeps detection aligned with pflag's + // semantics rather than with whatever the TS parser produced — e.g. a + // bare `--metadata-file --metadata-url` parses to two `none`s here but + // is a single consumed value in pflag (CLI-1982). + // + // `hasExplicitValueFlag` (not the simpler `hasExplicitLongFlag`) is + // required here because both flags in the group take a value: a bare + // `--metadata-file --metadata-url` is pflag consuming `--metadata-url` as + // `metadata-file`'s (oddly named) value, not two flags being set — see + // that function's doc comment. + const changed = SSO_ADD_MUTEX_GROUP.filter((flagName) => + hasExplicitValueFlag(rawArgs, SSO_ADD_COMMAND_PATH, SSO_ADD_VALUE_FLAG_NAMES, flagName), + ); + if (changed.length > 1) { return yield* Effect.fail( new LegacySsoMutexFlagError({ - message: "only one of --metadata-file or --metadata-url may be set", + message: cobraMutuallyExclusiveErrorMessage(SSO_ADD_MUTEX_GROUP, changed), }), ); } diff --git a/apps/cli/src/legacy/commands/sso/add/add.integration.test.ts b/apps/cli/src/legacy/commands/sso/add/add.integration.test.ts index 5ee8f835cd..966f5db1cc 100644 --- a/apps/cli/src/legacy/commands/sso/add/add.integration.test.ts +++ b/apps/cli/src/legacy/commands/sso/add/add.integration.test.ts @@ -2,7 +2,7 @@ import { writeFileSync } from "node:fs"; import { join } from "node:path"; import { describe, expect, it } from "@effect/vitest"; -import { Effect, Exit, Option } from "effect"; +import { Effect, Exit, Layer, Option, Stdio } from "effect"; import * as HttpClientResponse from "effect/unstable/http/HttpClientResponse"; import { mockAnalytics, mockOutput } from "../../../../../tests/helpers/mocks.ts"; @@ -39,6 +39,13 @@ interface SetupOpts { upgradeGate?: "gated" | "notGated"; // Metadata-URL fetch responses keyed by URL prefix. metadataUrlResponse?: { status: number; body: string }; + /** + * Raw argv the handler sees via `Stdio.Stdio` — drives the + * `hasExplicitValueFlag`-based mutex check. Defaults to a bare invocation + * with neither of the mutually-exclusive metadata flags present; tests + * that exercise the check must pass the matching flags explicitly here. + */ + cliArgs?: ReadonlyArray; } function jsonResponse( @@ -132,15 +139,20 @@ function setup(opts: SetupOpts = {}) { }); const cliConfig = mockLegacyCliConfig({ workdir: tempRoot.current }); - const layer = buildLegacyTestRuntime({ - out, - api: { layer: api.layer, httpClientLayer: api.httpClientLayer }, - cliConfig, - telemetry: telemetry.layer, - linkedProjectCache: cache.layer, - analytics, - goOutput: opts.goOutput === undefined ? Option.none() : Option.some(opts.goOutput), - }); + const layer = Layer.mergeAll( + buildLegacyTestRuntime({ + out, + api: { layer: api.layer, httpClientLayer: api.httpClientLayer }, + cliConfig, + telemetry: telemetry.layer, + linkedProjectCache: cache.layer, + analytics, + goOutput: opts.goOutput === undefined ? Option.none() : Option.some(opts.goOutput), + }), + Stdio.layerTest({ + args: Effect.succeed(opts.cliArgs ?? ["sso", "add", "--type", "saml"]), + }), + ); return { layer, out, api, analytics, telemetry, cache }; } @@ -173,27 +185,109 @@ describe("legacy sso add integration", () => { }).pipe(Effect.provide(layer)); }); - it.live("fails with mutex-flag error when both metadata flags set", () => { - const { layer } = setup(); - return Effect.gen(function* () { - const exit = yield* Effect.exit( - legacySsoAdd({ - ...defaultFlags, - metadataFile: Option.some("/tmp/missing.xml"), - metadataUrl: Option.some("https://idp.example.com/m"), - }), - ); - expect(Exit.isFailure(exit)).toBe(true); - if (Exit.isFailure(exit)) { - expect(JSON.stringify(exit.cause)).toContain("LegacySsoMutexFlagError"); - } - }).pipe(Effect.provide(layer)); - }); + it.live( + "mutex check: --metadata-file + --metadata-url fails with cobra's exact error text", + () => { + const { layer } = setup({ + cliArgs: [ + "sso", + "add", + "--type", + "saml", + "--metadata-file", + "/tmp/missing.xml", + "--metadata-url", + "https://idp.example.com/m", + ], + }); + return Effect.gen(function* () { + const exit = yield* Effect.exit( + legacySsoAdd({ + ...defaultFlags, + metadataFile: Option.some("/tmp/missing.xml"), + metadataUrl: Option.some("https://idp.example.com/m"), + }), + ); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const dump = JSON.stringify(exit.cause); + expect(dump).toContain("LegacySsoMutexFlagError"); + // Byte-matches cobra's `validateExclusiveFlagGroups` template + // (`flag_groups.go:204`): group in Go's registration order + // (`cmd/sso.go:164`), changed flags sorted alphabetically. + expect(dump).toContain( + "if any flags in the group [metadata-file metadata-url] are set none of the others can be; [metadata-file metadata-url] were all set", + ); + } + }).pipe(Effect.provide(layer)); + }, + ); + + it.live( + "mutex check: an explicit but empty --metadata-file= still conflicts with --metadata-url (changed, not truthy)", + () => { + // `--metadata-file=` parses to an empty string, but cobra's + // `pflag.Changed` tracks that the flag was passed at all, not the + // resulting value — the mutex must trip on an explicit empty value, + // and with cobra's exact template, not the old hand-written message. + const { layer } = setup({ + cliArgs: [ + "sso", + "add", + "--type", + "saml", + "--metadata-file=", + "--metadata-url", + "https://idp.example.com/m", + ], + }); + return Effect.gen(function* () { + const exit = yield* Effect.exit( + legacySsoAdd({ + ...defaultFlags, + metadataFile: Option.some(""), + metadataUrl: Option.some("https://idp.example.com/m"), + }), + ); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const dump = JSON.stringify(exit.cause); + expect(dump).toContain("LegacySsoMutexFlagError"); + expect(dump).toContain( + "if any flags in the group [metadata-file metadata-url] are set none of the others can be; [metadata-file metadata-url] were all set", + ); + } + }).pipe(Effect.provide(layer)); + }, + ); + + it.live( + "mutex check: a bare --metadata-file followed by --metadata-url is not a violation", + () => { + // pflag's `--flag arg` branch consumes the very next argv token as the + // value unconditionally (`flag.go:1013-1031`), so real cobra parses + // this as `metadata-file` receiving the literal value + // `"--metadata-url"` — `metadata-url` is never parsed as its own flag + // and stays unset. The raw-argv mutex scan must reach the same "not a + // violation" conclusion pflag does, not double-count the + // `--metadata-url` token as a second explicit flag. + const { layer } = setup({ + cliArgs: ["sso", "add", "--type", "saml", "--metadata-file", "--metadata-url"], + }); + return Effect.gen(function* () { + const exit = yield* Effect.exit(legacySsoAdd(defaultFlags)); + expect(Exit.isSuccess(exit)).toBe(true); + }).pipe(Effect.provide(layer)); + }, + ); it.live("reads metadata file and sends as metadata_xml", () => { const path = join(tempRoot.current, "good.xml"); writeFileSync(path, ''); - const { layer, api } = setup(); + // A single metadata flag on the raw argv must sail through the mutex scan. + const { layer, api } = setup({ + cliArgs: ["sso", "add", "--type", "saml", "--metadata-file", path], + }); return Effect.gen(function* () { yield* legacySsoAdd({ ...defaultFlags, metadataFile: Option.some(path) }); const req = api.requests.find((r) => r.method === "POST"); @@ -217,7 +311,18 @@ describe("legacy sso add integration", () => { }); it.live("sends metadata_url verbatim when --skip-url-validation", () => { - const { layer, api } = setup(); + // A single metadata flag on the raw argv must sail through the mutex scan. + const { layer, api } = setup({ + cliArgs: [ + "sso", + "add", + "--type", + "saml", + "--metadata-url", + "https://idp.example.com/m", + "--skip-url-validation", + ], + }); return Effect.gen(function* () { yield* legacySsoAdd({ ...defaultFlags,