diff --git a/CHANGELOG.md b/CHANGELOG.md index 9b8b1b0..7676425 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,29 @@ to follow [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Added + +- **`c1i entitlements create`.** Modelling a manually-managed app took three + raw `api` calls -- resource type, resource, then entitlement -- with the ids + hand-carried between them. One command now does it, and reuses objects you + already have: pass `--resource-type-id` to skip the first call, and + `--resource-id` as well to skip the second. `--owner-id` is set + inline rather than needing a follow-up call, and `--duration-grant` is a flag + rather than a hand-written body field. + + On a partial failure nothing is rolled back; the error names what that run + created, the flags to re-run with, and the create-only flags that retry has + to drop, so the command it prints is one that works. `--dry-run` previews + all three requests. + + Empty values are usage errors (exit 2) before anything is sent, rather than + silent fallbacks: `--owner-id ""` from an unset shell variable would have + created an ownerless entitlement at exit 0, and an empty + `--resource-type-display-name`/`--resource-display-name` would have quietly + reused `--display-name`. Only a `CUSTOM` resource type can repeat on one + app; the help now says so, quoting the server's own + `app resource type already exists`. + ### Changed - **Fixtures and command documentation now use placeholder identifiers**, and diff --git a/README.md b/README.md index b6354dc..ffcd97d 100644 --- a/README.md +++ b/README.md @@ -99,8 +99,54 @@ c1i accounts set-owner --app-id --user-id ```sh c1i entitlements list [--app-id ] [--query ] [--page-size N] [--page-token TOKEN] [--limit N] c1i entitlements get --app-id + +# Create one on a manually-managed app, with its resource type and resource +c1i entitlements create --app-id --display-name "Payroll admin" \ + [--description ] [--slug member] [--alias payroll_admin] [--owner-id ] \ + [--duration-grant 3600s] [--resource-type CUSTOM] \ + [--resource-type-display-name "Payroll role"] [--resource-display-name "Payroll admins"] + +# Reuse an existing resource type (or an existing resource) instead of creating one +c1i entitlements create --app-id --display-name "Payroll viewer" --resource-type-id +c1i entitlements create --app-id --display-name "Payroll viewer (RO)" \ + --resource-type-id --resource-id ``` +An entitlement points at an app resource, which lives under an app resource +type, so `entitlements create` is up to three `POST`s: the resource type, the +resource, then the entitlement. `--resource-type-id` and `--resource-id` skip +whichever of the first two steps you already have — one resource type can carry +many resources, and one resource many entitlements. The server requires both +ids on the entitlement even though the OpenAPI schema marks only `displayName` +required, so `--resource-id` without `--resource-type-id` is rejected at exit +`2` before anything is sent. + +`--resource-type` is `ROLE`, `GROUP`, `LICENSE`, `PROJECT`, `CATALOG`, +`CUSTOM`, `VAULT` or `PROFILE_TYPE` (case-insensitive here, uppercase on the +wire) and describes the resource type this command creates, so passing it +together with `--resource-type-id` is a usage error rather than a silently +ignored flag. Only `CUSTOM` can repeat on one app: a second resource type of +any other kind fails with a 500 (exit `6`, though retrying never helps) saying +`app resource type already exists`, so reuse the existing one with +`--resource-type-id` and drop both `--resource-type` and +`--resource-type-display-name` — either one alongside the id is a usage error. +Reusing a resource with `--resource-id` likewise means you drop +`--resource-display-name`. `--owner-id` is repeatable and goes inline in the create +request, so no follow-up call is needed; an empty one is a usage error rather +than an owner quietly dropped. `--duration-grant` takes a protobuf duration — +seconds with an `s` suffix, e.g. `3600s`; a Go-style `1h` is refused by the +server. Omit it for standing access. + +`--dry-run` previews all three requests, printing +`NEW_APP_RESOURCE_TYPE_ID`/`NEW_APP_RESOURCE_ID` where an id only exists after +a real preceding step. There is no rollback: if a later step fails, the objects +the earlier ones created still exist, and the error names them along with the +flags that reuse them and the create-only flags the retry has to drop. The +created entitlement comes back as pretty JSON under `appEntitlementView` +(`--fields` is never applied to mutation output); it echoes +`appResourceTypeId`/`appResourceId` and expands both objects, so every id the +command touched is in that one payload. + ### Grants ("who has access") ```sh diff --git a/cmd/agents.md b/cmd/agents.md index 8e742ab..17448e9 100644 --- a/cmd/agents.md +++ b/cmd/agents.md @@ -245,6 +245,20 @@ Two things are irreversible in ways their `--help` doesn't make obvious: each toolset's app entitlement, is soft-deleted with it. Anyone whose access came through one of those entitlements is affected. +One command sends more than one write: `entitlements create` POSTs a resource +type, a resource, then the entitlement, skipping the steps whose id you supply +via `--resource-type-id`/`--resource-id`. Its `--dry-run` previews all three. +There is no rollback, so a failure part-way through leaves the earlier objects +behind; the error names them, the flags that reuse them, and the create-only +flags the retry has to drop, and re-running without those flags creates +duplicates. Only a `CUSTOM` resource type can repeat on one app: a second +`--resource-type` of any other kind fails with a 500 (exit `6`, though +retrying never helps) saying `app resource type already exists` -- reuse the +existing type with `--resource-type-id` and drop both `--resource-type` and +`--resource-type-display-name`; either alongside the id is exit 2. Reusing a +resource with `--resource-id` likewise means you drop +`--resource-display-name`. + ## Things that will surprise you - A **repeatable** flag takes one value per occurrence; a comma is literal, not @@ -316,6 +330,10 @@ Two things are irreversible in ways their `--help` doesn't make obvious: - Entitlement ids are unique only within an app — some system-builtin entitlements reuse the same id across every app that has one. Always key on `(app_id, id)` together, never `id` alone. +- `POST /api/v1/apps/{app_id}/entitlements` requires `appResourceTypeId` and + `appResourceId` even though its OpenAPI schema lists only `displayName` as + required; omitting them 400s on the id regex. `entitlements create` handles + this for you. - `mcp servers test-connection` returns `toolCount` as a JSON string, not a number. The `tool_count` in NDJSON list rows is a real number. - `mcp servers search` only includes `tool_count` when you pass diff --git a/cmd/docs_guide.go b/cmd/docs_guide.go index da706ad..4290dd5 100644 --- a/cmd/docs_guide.go +++ b/cmd/docs_guide.go @@ -346,11 +346,10 @@ provisionerPolicy.delegated update is the actual provisioning trigger. ` // guideConfigureNewApp walks through creating a manually-managed app -// container, setting its owners, and creating a custom entitlement for it -// via the 3-call resource-type/resource/entitlement sequence (no first-class -// "entitlements create" exists). Derived from cmd/apps_create.go, -// cmd/apps_set_owners.go, cmd/entitlements_get.go, cmd/entitlements_list.go, -// and cmd/api.go. +// container, setting its owners, and creating a custom entitlement for it. +// Derived from cmd/apps_create.go, cmd/apps_set_owners.go, +// cmd/entitlements_create.go, cmd/entitlements_get.go, +// cmd/entitlements_list.go, and cmd/api.go. const guideConfigureNewApp = `# Configure a new app Stand up an app container, assign the C1 users who administer it, and give it @@ -412,32 +411,66 @@ here removes yourself. Use "apps add-owner" instead to add without replacing. ### 3. Create a custom entitlement to grant -There is no "entitlements create" — only "entitlements get"/"list". Creating -one for a manually-managed app is a 3-call sequence instead: a resource -type, a resource under it, then the entitlement pointing at both. No -first-class command covers this, so each call goes through "c1i api": +An entitlement points at an app resource, which lives under an app resource +type, so creating one is three POSTs. "entitlements create" sends all three: - c1i api --path=/api/v1/apps/$APP_ID/resource_types --body='{"displayName":"Payroll role","resourceType":"CUSTOM"}' - RT_ID= - - c1i api --path=/api/v1/apps/$APP_ID/resource_types/$RT_ID/resources --body='{"displayName":"Payroll admin"}' - RES_ID= - - c1i api --path=/api/v1/apps/$APP_ID/entitlements --body='{"displayName":"Payroll admin","slug":"member","alias":"payroll_admin","appResourceTypeId":"'$RT_ID'","appResourceId":"'$RES_ID'"}' + c1i entitlements create --app-id "$APP_ID" \ + --display-name "Payroll admin" --resource-type-display-name "Payroll role" \ + --slug member --alias payroll_admin --owner-id "$OWNER_USER_ID" ENT_ID= -resourceType is one of ROLE|GROUP|LICENSE|PROJECT|CATALOG|CUSTOM|VAULT|PROFILE_TYPE. -Omitting a duration defaults the entitlement to standing access -(durationUnset); pass a durationGrant field (e.g. 3600s) instead for -time-boxed access. +The response echoes the ids of the other two objects in that same payload: + + RT_ID= + RES_ID= + +Reuse them for the next entitlement rather than minting a duplicate resource +type per entitlement: + + c1i entitlements create --app-id "$APP_ID" --display-name "Payroll viewer" \ + --resource-type-id "$RT_ID" # new resource under an existing type + c1i entitlements create --app-id "$APP_ID" --display-name "Payroll viewer (RO)" \ + --resource-type-id "$RT_ID" --resource-id "$RES_ID" # entitlement only + +--resource-type (the kind of resource type to create, default CUSTOM) is one +of ROLE, GROUP, LICENSE, PROJECT, CATALOG, CUSTOM, VAULT, PROFILE_TYPE. Only +CUSTOM can repeat on one app: a second resource type of any other kind fails +with a 500, + app resource type already exists +which maps to exit 6 even though retrying never helps. Reuse the one that +exists by passing --resource-type-id and drop both --resource-type and +--resource-type-display-name: each describes a type this command would create, +so either alongside the id is refused at exit 2. Reusing a resource with +--resource-id likewise means you drop --resource-display-name. + +--owner-id is repeatable and rides along in the create request, so entitlement +owners need no follow-up call -- but the read lags the write the same way app +owners do (one measured create took 116s to show up on +"GET .../entitlements/$ENT_ID/ownerids"), so don't read that as a failure. + +The three writes are not atomic and nothing is rolled back: if a later one +fails, the objects the earlier ones created still exist, and the error names +them, the flags that reuse them, and the flags to drop, e.g. "(already +created: re-run with --resource-type-id , dropping +--resource-type-display-name, to reuse instead of duplicating)". Following it +verbatim is what makes the retry succeed: keeping a flag that describes an +object which now already exists is refused. "--dry-run" previews all three +requests. + +Omitting --duration-grant defaults the entitlement to standing access +(durationUnset). For time-boxed access pass a protobuf duration -- seconds +with an "s" suffix, e.g. "--duration-grant 3600s"; a Go-style "1h" is refused +by the server with "invalid google.protobuf.Duration value". ## Verify c1i entitlements list --app-id "$APP_ID" Auto-paginates to completion; expect the builtin "Access" row plus your new -entitlement, both present immediately — the entitlement search index is not -lagged the way owners are. +entitlement. The entitlement search index is not lagged the way owners are -- +three measured creates were listed within a second -- but it isn't +transactional either: a fourth was missing from a list issued immediately +after it. Re-run before concluding the create failed. c1i entitlements get "$ENT_ID" --app-id "$APP_ID" @@ -519,8 +552,9 @@ you'd take it back. c1i auth whoami -- An app and an entitlement that already exist to request against. There is - no "entitlements create" — find real ones: +- An app and an entitlement that already exist to request against. Find real + ones — "entitlements create" only makes manually-managed ones, which is not + what this workflow is for (see "c1i docs guide configure-new-app"): c1i apps list APP_ID= diff --git a/cmd/entitlements_create.go b/cmd/entitlements_create.go new file mode 100644 index 0000000..a23a4e1 --- /dev/null +++ b/cmd/entitlements_create.go @@ -0,0 +1,453 @@ +package cmd + +import ( + "encoding/json" + "fmt" + "strings" + + "github.com/ConductorOne/c1i/internal/client" + "github.com/spf13/cobra" +) + +// resourceTypeKinds is the AppResourceType enum, and the single source for +// every place this repo names it. TestResourceTypeKindsAreDocumented holds the +// help text, README.md and the configure-new-app guide to this list. Not used +// to validate --resource-type: the server stays authoritative if the enum +// grows, and normalizeResourceType passes an unknown name through. +var resourceTypeKinds = []string{"ROLE", "GROUP", "LICENSE", "PROJECT", "CATALOG", "CUSTOM", "VAULT", "PROFILE_TYPE"} + +// Stand-ins printed by --dry-run for ids only a preceding response can supply. +// Alphanumeric + "_" so client.Path leaves them unescaped and the previewed +// path stays readable. +const ( + newResourceTypeIDPlaceholder = "NEW_APP_RESOURCE_TYPE_ID" + newResourceIDPlaceholder = "NEW_APP_RESOURCE_ID" +) + +var entitlementsCreateCmd = &cobra.Command{ + Use: "create", + Short: "Create an app entitlement, with its resource type and resource (pretty JSON)", + Long: `Create an app entitlement, plus the app resource type and resource it +points at. The app itself need not be manually managed -- the entitlement this +creates is (isManuallyManaged is true on it, false on the app it was created +on). + +An entitlement points at an app resource, which lives under an app resource +type, so this is up to three POSTs in order: + + 1. POST /api/v1/apps/{app-id}/resource_types + 2. POST /api/v1/apps/{app-id}/resource_types/{app-resource-type-id}/resources + 3. POST /api/v1/apps/{app-id}/entitlements + +Steps 1 and 2 are skipped for whichever of --resource-type-id/--resource-id +you already have, so one resource type can carry many resources and one +resource many entitlements: + + --app-id --display-name all three (a new type and resource) + ... --resource-type-id steps 2 and 3 (a new resource + under an existing type) + ... --resource-type-id --resource-id step 3 only + +Both ids are required by the server even though the OpenAPI schema marks only +displayName required — omitting them fails with: + invalid CreateAppEntitlementRequest.AppResourceTypeId: value does not match regex pattern "^[a-zA-Z0-9]{27}$" +so --resource-id without --resource-type-id is rejected here, at exit 2, +before anything is sent. + +--resource-type describes the resource type this command creates, so passing +it together with --resource-type-id is a usage error rather than a silently +ignored flag. Case-insensitive here; the API itself takes only the uppercase +form. One of: + ` + strings.Join(resourceTypeKinds, ", ") + ` +Only CUSTOM can repeat on one app: a second resource type of any other kind +fails with a 500 (exit 6, though retrying never helps): + app resource type already exists +Reuse the one that exists with --resource-type-id and drop both +--resource-type and --resource-type-display-name; either alongside the id is a +usage error. Likewise, pass --resource-id to reuse a resource and drop +--resource-display-name. + +Omitting --duration-grant leaves the entitlement at standing access +(durationUnset in the response). It takes a protobuf duration, not a Go one -- +seconds with an "s" suffix, e.g. 3600s. "1h" is refused by the server with: + invalid google.protobuf.Duration value "1h" + +Assign owners inline with --owner-id: the create request carries +appEntitlementOwnerIds, so no follow-up call is needed. An empty --owner-id is +a usage error, not an owner quietly dropped. Owner provisioning is +asynchronous — one measured create took 116s to read back on +"GET /api/v1/apps/{app-id}/entitlements/{id}/ownerids". + +--dry-run previews every request in the sequence. The ids that only exist +after a real step 1 or 2 print as ` + newResourceTypeIDPlaceholder + `/` + + newResourceIDPlaceholder + `. + +There is no rollback. The three creates are independent, so if step 2 or 3 +fails the objects earlier steps created still exist; the error names them, the +flags that reuse them, and the create-only flags a retry has to drop (a reused +id makes those a usage error), verbatim: + (already created: re-run with --resource-type-id , dropping --resource-type-display-name, to reuse instead of duplicating) + +Prints the created entitlement as pretty JSON under "appEntitlementView" +(--fields is not applied to mutation output). The response echoes +appResourceTypeId/appResourceId and expands both objects, so every id this +command touched is in that one payload. + +Example: + c1i entitlements create --app-id "$APP_ID" \ + --display-name "Payroll admin" --resource-type-display-name "Payroll role" \ + --slug member --alias payroll_admin`, + RunE: func(cmd *cobra.Command, args []string) error { + if err := requireNonEmpty(cmd, "app-id", "display-name"); err != nil { + return err + } + + plan, err := buildEntitlementCreatePlan(cmd) + if err != nil { + // Already a *usageError from the flag helpers; flags.go documents + // that callers must not re-wrap. + return err + } + + baseURL, err := GetBaseURL() + if err != nil { + return err + } + + if dryRunActive() { + return plan.previewRequests(cmd) + } + + c, err := newClient(cmd, baseURL) + if err != nil { + return fmt.Errorf("authentication failed: %w", err) + } + + return plan.run(cmd, c) + }, +} + +// entitlementCreatePlan is the resolved create sequence: a step's body is nil +// when the caller supplied that object's id and the step is skipped. +type entitlementCreatePlan struct { + appID string + + resourceTypeID string // reuse this type when non-empty + resourceTypeBody map[string]any // otherwise create one from this + + resourceID string + resourceBody map[string]any + + // Create-only flags this invocation passed, per object. A retry that + // reuses the object has to drop them; rejectFlagsForReusedObject refuses + // them alongside the id createdSoFar hands back. + typeOnlyFlags []string + resourceOnlyFlags []string + + // entitlementBody carries everything except appResourceTypeId and + // appResourceId, which are only known once the steps above have run. + entitlementBody map[string]any +} + +// typeCreateOnlyFlags describe a resource type this command would create, so +// each is refused alongside --resource-type-id. resourceCreateOnlyFlags is the +// same for a resource and --resource-id. Named once each: the reject call, the +// retry message and the docs that tell a reader what to drop all read these. +var ( + typeCreateOnlyFlags = []string{"resource-type", "resource-type-display-name"} + resourceCreateOnlyFlags = []string{"resource-display-name"} +) + +// buildEntitlementCreatePlan resolves flags into the request sequence. Pure (no +// network / auth) so --dry-run and unit tests exercise the same bodies the live +// requests send. Optional fields are omitted when empty rather than sent as +// empty strings/arrays. +func buildEntitlementCreatePlan(cmd *cobra.Command) (*entitlementCreatePlan, error) { + appID, _ := cmd.Flags().GetString("app-id") + displayName, _ := cmd.Flags().GetString("display-name") + + resourceTypeID, err := requireNonEmptyIfSet(cmd, "resource-type-id") + if err != nil { + return nil, err + } + resourceID, err := requireNonEmptyIfSet(cmd, "resource-id") + if err != nil { + return nil, err + } + if resourceID != "" && resourceTypeID == "" { + return nil, &usageError{fmt.Errorf("--resource-id requires --resource-type-id: the entitlement carries both ids, and the server rejects a missing one with " + + `invalid CreateAppEntitlementRequest.AppResourceTypeId: value does not match regex pattern "^[a-zA-Z0-9]{27}$"`)} + } + + if err := rejectFlagsForReusedObject(cmd, resourceTypeID, "--resource-type-id", typeCreateOnlyFlags...); err != nil { + return nil, err + } + if err := rejectFlagsForReusedObject(cmd, resourceID, "--resource-id", resourceCreateOnlyFlags...); err != nil { + return nil, err + } + + // --resource-type has a default, so an explicit "" is a mistake. Checked + // after the reuse rejection above, whose message is the more useful one when + // --resource-type-id is also set. + resourceType, err := requireNonEmptyIfSet(cmd, "resource-type") + if err != nil { + return nil, err + } + + // Same for the display-name overrides: an explicit "" would fall back to + // --display-name and mis-name the object rather than fail. + for _, n := range []string{"resource-type-display-name", "resource-display-name"} { + if _, err := requireNonEmptyIfSet(cmd, n); err != nil { + return nil, err + } + } + + p := &entitlementCreatePlan{ + appID: appID, + resourceTypeID: resourceTypeID, + resourceID: resourceID, + entitlementBody: map[string]any{"displayName": displayName}, + } + + if resourceTypeID == "" { + p.resourceTypeBody = map[string]any{ + "displayName": flagOrDefault(cmd, "resource-type-display-name", displayName), + "resourceType": normalizeResourceType(resourceType), + } + p.typeOnlyFlags = changedFlags(cmd, typeCreateOnlyFlags...) + } + if resourceID == "" { + p.resourceBody = map[string]any{ + "displayName": flagOrDefault(cmd, "resource-display-name", displayName), + } + p.resourceOnlyFlags = changedFlags(cmd, resourceCreateOnlyFlags...) + } + + // Flag name -> request field, equal except where the wire key is camelCase. + for flag, key := range map[string]string{ + "description": "description", + "slug": "slug", + "alias": "alias", + "duration-grant": "durationGrant", + } { + if v, _ := cmd.Flags().GetString(flag); v != "" { + p.entitlementBody[key] = v + } + } + // An owner id from an unset shell variable would otherwise create the + // entitlement with fewer owners than asked for, and owner reads are async, + // so nothing downstream can tell that apart from "not provisioned yet". + owners, err := repeatableStringFlag(cmd, "owner-id") + if err != nil { + return nil, err + } + if len(owners) > 0 { + p.entitlementBody["appEntitlementOwnerIds"] = owners + } + + return p, nil +} + +// rejectFlagsForReusedObject fails when flags that only describe an object this +// command would create are passed alongside the id of an existing one, rather +// than ignoring them and creating something the caller didn't ask for. +func rejectFlagsForReusedObject(cmd *cobra.Command, reusedID, reusedFlag string, names ...string) error { + if reusedID == "" { + return nil + } + if passed := changedFlags(cmd, names...); len(passed) > 0 { + return &usageError{fmt.Errorf("%s only applies when this command creates that object; drop it or drop %s", passed[0], reusedFlag)} + } + return nil +} + +// changedFlags returns the named flags the caller actually passed, "--"-prefixed. +func changedFlags(cmd *cobra.Command, names ...string) []string { + var passed []string + for _, n := range names { + if cmd.Flags().Changed(n) { + passed = append(passed, "--"+n) + } + } + return passed +} + +// normalizeResourceType upper-cases the enum name so "custom" and +// "profile-type" work; anything unrecognized is passed through for the server +// to reject, keeping it authoritative if the enum grows. +func normalizeResourceType(s string) string { + return strings.ToUpper(strings.ReplaceAll(s, "-", "_")) +} + +func (p *entitlementCreatePlan) resourceTypesPath() string { + return client.Path("/api/v1/apps/%s/resource_types", p.appID) +} + +func (p *entitlementCreatePlan) resourcesPath(resourceTypeID string) string { + return client.Path("/api/v1/apps/%s/resource_types/%s/resources", p.appID, resourceTypeID) +} + +func (p *entitlementCreatePlan) entitlementsPath() string { + return client.Path("/api/v1/apps/%s/entitlements", p.appID) +} + +// fullEntitlementBody returns the step-3 body with the resource ids filled in, +// leaving p.entitlementBody untouched so a caller can build it more than once. +func (p *entitlementCreatePlan) fullEntitlementBody(resourceTypeID, resourceID string) map[string]any { + body := make(map[string]any, len(p.entitlementBody)+2) + for k, v := range p.entitlementBody { + body[k] = v + } + body["appResourceTypeId"] = resourceTypeID + body["appResourceId"] = resourceID + return body +} + +// run issues the planned requests in order and prints the entitlement the last +// one returns. +func (p *entitlementCreatePlan) run(cmd *cobra.Command, c *client.Client) error { + ctx := cmd.Context() + + resourceTypeID := p.resourceTypeID + if p.resourceTypeBody != nil { + data, err := c.Post(ctx, p.resourceTypesPath(), p.resourceTypeBody) + if err != nil { + return fmt.Errorf("API error creating the app resource type: %w", err) + } + if resourceTypeID, err = createdObjectID(data, "appResourceType"); err != nil { + return err + } + } + + resourceID := p.resourceID + if p.resourceBody != nil { + data, err := c.Post(ctx, p.resourcesPath(resourceTypeID), p.resourceBody) + if err != nil { + return fmt.Errorf("API error creating the app resource%s: %w", p.createdSoFar(resourceTypeID, ""), err) + } + if resourceID, err = createdObjectID(data, "appResource"); err != nil { + return err + } + } + + data, err := c.Post(ctx, p.entitlementsPath(), p.fullEntitlementBody(resourceTypeID, resourceID)) + if err != nil { + return fmt.Errorf("API error creating the entitlement%s: %w", p.createdSoFar(resourceTypeID, resourceID), err) + } + + return writeRawObject(cmd, data) +} + +// createdSoFar names the objects THIS invocation created before failing, the +// flags that reuse them, and the create-only flags the retry must drop. +// Nothing is rolled back: a compensating delete can fail too, which would +// leave a worse state described by a less honest message. +func (p *entitlementCreatePlan) createdSoFar(resourceTypeID, resourceID string) string { + var parts, drop []string + if p.resourceTypeBody != nil && resourceTypeID != "" { + parts = append(parts, "--resource-type-id "+resourceTypeID) + drop = append(drop, p.typeOnlyFlags...) + } + if p.resourceBody != nil && resourceID != "" { + parts = append(parts, "--resource-id "+resourceID) + drop = append(drop, p.resourceOnlyFlags...) + } + if len(parts) == 0 { + return "" + } + msg := " (already created: re-run with " + strings.Join(parts, " ") + if len(drop) > 0 { + // Named, not merely implied: the reused id makes these a usage error, + // so a retry that kept them would exit 2. + msg += ", dropping " + strings.Join(drop, " ") + "," + } + return msg + " to reuse instead of duplicating)" +} + +// previewRequests previews every request in the sequence, not just the first — +// two of the three are writes the caller would otherwise not see coming. +func (p *entitlementCreatePlan) previewRequests(cmd *cobra.Command) error { + resourceTypeID, resourceID := p.resourceTypeID, p.resourceID + + var used []string + if p.resourceTypeBody != nil { + resourceTypeID = newResourceTypeIDPlaceholder + used = append(used, newResourceTypeIDPlaceholder) + } + if p.resourceBody != nil { + resourceID = newResourceIDPlaceholder + used = append(used, newResourceIDPlaceholder) + } + if len(used) > 0 { + subject := "stands in for an id" + if len(used) > 1 { + subject = "stand in for ids" + } + _, _ = fmt.Fprintf(cmd.OutOrStdout(), + "[dry-run] requests are sent in order; %s %s the preceding response returns\n", + strings.Join(used, "/"), subject) + } + + if p.resourceTypeBody != nil { + if err := printDryRun(cmd, "POST", p.resourceTypesPath(), p.resourceTypeBody); err != nil { + return err + } + } + if p.resourceBody != nil { + if err := printDryRun(cmd, "POST", p.resourcesPath(resourceTypeID), p.resourceBody); err != nil { + return err + } + } + return printDryRun(cmd, "POST", p.entitlementsPath(), p.fullEntitlementBody(resourceTypeID, resourceID)) +} + +// createdObjectID pulls .id out of a create response. A 200 that carries +// no id would otherwise read as success while the next request in the chain +// addresses a path with an empty segment. +func createdObjectID(data []byte, key string) (string, error) { + var resp map[string]json.RawMessage + if err := json.Unmarshal(data, &resp); err != nil { + return "", &nonJSONResponseError{fmt.Errorf("failed to parse %s response: %w", key, err)} + } + raw, ok := resp[key] + if !ok { + return "", &nonJSONResponseError{fmt.Errorf("response carried no %q object", key)} + } + var obj struct { + ID string `json:"id"` + } + if err := json.Unmarshal(raw, &obj); err != nil { + return "", &nonJSONResponseError{fmt.Errorf("failed to parse %s response: %w", key, err)} + } + if obj.ID == "" { + return "", &nonJSONResponseError{fmt.Errorf("response carried no %s.id", key)} + } + return obj.ID, nil +} + +// addEntitlementCreateFlags registers the flag set. Shared with the tests so a +// flag can't be added to the command yet missed by what exercises it. +func addEntitlementCreateFlags(cmd *cobra.Command) { + f := cmd.Flags() + f.String("app-id", "", "Application ID to create the entitlement on") + f.String("display-name", "", "Display name for the new entitlement (also names the resource type and resource this command creates)") + f.String("description", "", "Description for the new entitlement") + f.String("slug", "", "Slug for the new entitlement (e.g. member)") + f.String("alias", "", "Alias for the new entitlement; exact-match queryable") + f.String("duration-grant", "", "Maximum grant duration as a protobuf duration, e.g. 3600s; omit for standing access") + // StringArray, not StringSlice: the slice parser drops an empty value + // outright, so `--owner-id "" --owner-id U` would lose the empty one + // before this command could reject it. + addRepeatableStringFlag(cmd, "owner-id", "C1 user ID to own the new entitlement (repeatable)") + f.String("resource-type", "CUSTOM", "Kind of resource type to create: "+strings.Join(resourceTypeKinds, ", ")) + f.String("resource-type-id", "", "Existing app resource type to reuse instead of creating one") + f.String("resource-type-display-name", "", "Display name for the resource type this command creates (default: --display-name)") + f.String("resource-id", "", "Existing app resource to reuse instead of creating one; requires --resource-type-id") + f.String("resource-display-name", "", "Display name for the resource this command creates (default: --display-name)") + markRequired(cmd, "app-id", "display-name") +} + +func init() { + addEntitlementCreateFlags(entitlementsCreateCmd) + entitlementsCmd.AddCommand(entitlementsCreateCmd) +} diff --git a/cmd/entitlements_create_test.go b/cmd/entitlements_create_test.go new file mode 100644 index 0000000..ddb5913 --- /dev/null +++ b/cmd/entitlements_create_test.go @@ -0,0 +1,752 @@ +package cmd + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "reflect" + "slices" + "strings" + "testing" + + "github.com/ConductorOne/c1i/internal/client" + "github.com/spf13/cobra" +) + +// newEntitlementCreateCmd builds a throwaway command carrying the real flag +// set, so a test never drifts from what the command actually registers. +func newEntitlementCreateCmd(t *testing.T, args ...string) *cobra.Command { + t.Helper() + cmd := &cobra.Command{Use: "create", RunE: func(*cobra.Command, []string) error { return nil }} + addEntitlementCreateFlags(cmd) + cmd.SetArgs(args) + cmd.SetOut(new(bytes.Buffer)) + cmd.SetErr(new(bytes.Buffer)) + cmd.SetContext(context.Background()) + if err := cmd.Execute(); err != nil { + t.Fatalf("parsing %v: %v", args, err) + } + return cmd +} + +func mustPlan(t *testing.T, args ...string) *entitlementCreatePlan { + t.Helper() + p, err := buildEntitlementCreatePlan(newEntitlementCreateCmd(t, args...)) + if err != nil { + t.Fatalf("buildEntitlementCreatePlan(%v): %v", args, err) + } + return p +} + +// TestEntitlementCreatePlanCreatesAllThree pins the default sequence: a +// resource type and resource are created, both named from --display-name +// unless separately overridden, and unset optional fields are omitted rather +// than sent empty. +func TestEntitlementCreatePlanCreatesAllThree(t *testing.T) { + p := mustPlan(t, "--app-id", "app1", "--display-name", "Payroll admin") + + if p.resourceTypeID != "" || p.resourceID != "" { + t.Errorf("nothing was supplied to reuse, got type=%q resource=%q", p.resourceTypeID, p.resourceID) + } + wantType := map[string]any{"displayName": "Payroll admin", "resourceType": "CUSTOM"} + if !reflect.DeepEqual(p.resourceTypeBody, wantType) { + t.Errorf("resource type body = %v, want %v", p.resourceTypeBody, wantType) + } + wantResource := map[string]any{"displayName": "Payroll admin"} + if !reflect.DeepEqual(p.resourceBody, wantResource) { + t.Errorf("resource body = %v, want %v", p.resourceBody, wantResource) + } + wantEnt := map[string]any{"displayName": "Payroll admin"} + if !reflect.DeepEqual(p.entitlementBody, wantEnt) { + t.Errorf("entitlement body = %v, want %v", p.entitlementBody, wantEnt) + } +} + +// TestEntitlementCreatePlanOptionalFields pins that every optional flag reaches +// the body it belongs to, under the wire key the API expects. +func TestEntitlementCreatePlanOptionalFields(t *testing.T) { + p := mustPlan(t, + "--app-id", "app1", "--display-name", "Payroll admin", + "--description", "pays people", "--slug", "member", "--alias", "payroll_admin", + "--owner-id", "u1", "--owner-id", "u2", + "--resource-type", "profile-type", + "--resource-type-display-name", "Payroll role", + "--resource-display-name", "Payroll admins", + ) + + if got := p.resourceTypeBody["displayName"]; got != "Payroll role" { + t.Errorf("resource type displayName = %v", got) + } + // Lower case and "-" are normalized; the API takes only PROFILE_TYPE. + if got := p.resourceTypeBody["resourceType"]; got != "PROFILE_TYPE" { + t.Errorf("resourceType = %v, want PROFILE_TYPE", got) + } + if got := p.resourceBody["displayName"]; got != "Payroll admins" { + t.Errorf("resource displayName = %v", got) + } + want := map[string]any{ + "displayName": "Payroll admin", + "description": "pays people", + "slug": "member", + "alias": "payroll_admin", + "appEntitlementOwnerIds": []string{"u1", "u2"}, + } + if !reflect.DeepEqual(p.entitlementBody, want) { + t.Errorf("entitlement body = %v, want %v", p.entitlementBody, want) + } +} + +// TestEntitlementCreatePlanReusesSuppliedIDs pins that a supplied id skips +// exactly the step that would have created that object, and no more. +func TestEntitlementCreatePlanReusesSuppliedIDs(t *testing.T) { + t.Run("resource type only", func(t *testing.T) { + p := mustPlan(t, "--app-id", "app1", "--display-name", "e", "--resource-type-id", "rt1") + if p.resourceTypeBody != nil { + t.Errorf("resource type body = %v, want nil (reusing rt1)", p.resourceTypeBody) + } + if p.resourceBody == nil { + t.Error("resource body is nil; a resource must still be created") + } + }) + t.Run("both", func(t *testing.T) { + p := mustPlan(t, "--app-id", "app1", "--display-name", "e", "--resource-type-id", "rt1", "--resource-id", "r1") + if p.resourceTypeBody != nil || p.resourceBody != nil { + t.Errorf("bodies = %v/%v, want both nil (entitlement only)", p.resourceTypeBody, p.resourceBody) + } + body := p.fullEntitlementBody(p.resourceTypeID, p.resourceID) + if body["appResourceTypeId"] != "rt1" || body["appResourceId"] != "r1" { + t.Errorf("entitlement body = %v, want the supplied ids", body) + } + }) +} + +// TestEntitlementCreatePlanUsageErrors pins the combinations refused before any +// request is sent. Each would otherwise either 400 on the server or silently +// create something the caller did not ask for. +func TestEntitlementCreatePlanUsageErrors(t *testing.T) { + cases := []struct { + name string + args []string + want string + }{ + { + "resource id without its type", + []string{"--app-id", "app1", "--display-name", "e", "--resource-id", "r1"}, + "--resource-id requires --resource-type-id", + }, + { + // --resource-type has a default, so an explicit "" is a mistake. + // It used to reach the wire as an empty enum. + "explicitly empty resource type", + []string{"--app-id", "app1", "--display-name", "e", "--resource-type", ""}, + "flag --resource-type requires a non-empty value", + }, + { + // With --resource-type-id set, the reuse rejection is the more + // useful message: the flag does not belong at all. + "empty resource type alongside a reused type", + []string{"--app-id", "app1", "--display-name", "e", "--resource-type-id", "rt1", "--resource-type", ""}, + "--resource-type only applies when this command creates that object", + }, + { + "resource type kind alongside a reused type", + []string{"--app-id", "app1", "--display-name", "e", "--resource-type-id", "rt1", "--resource-type", "ROLE"}, + "--resource-type only applies", + }, + { + "resource type name alongside a reused type", + []string{"--app-id", "app1", "--display-name", "e", "--resource-type-id", "rt1", "--resource-type-display-name", "x"}, + "--resource-type-display-name only applies", + }, + { + "resource name alongside a reused resource", + []string{"--app-id", "app1", "--display-name", "e", "--resource-type-id", "rt1", "--resource-id", "r1", "--resource-display-name", "x"}, + "--resource-display-name only applies", + }, + { + "empty resource type id", + []string{"--app-id", "app1", "--display-name", "e", "--resource-type-id", ""}, + "flag --resource-type-id requires a non-empty value", + }, + { + "empty resource id", + []string{"--app-id", "app1", "--display-name", "e", "--resource-type-id", "rt1", "--resource-id", ""}, + "flag --resource-id requires a non-empty value", + }, + { + // Both display-name overrides would otherwise fall back to + // --display-name and mis-name the object. + "empty resource type display name", + []string{"--app-id", "app1", "--display-name", "e", "--resource-type-display-name", ""}, + "flag --resource-type-display-name requires a non-empty value", + }, + { + "empty resource display name", + []string{"--app-id", "app1", "--display-name", "e", "--resource-display-name", ""}, + "flag --resource-display-name requires a non-empty value", + }, + { + // An unset shell variable: the entitlement would otherwise be + // created ownerless at exit 0, and an async owner read cannot tell + // that apart from "not provisioned yet". + "empty owner id", + []string{"--app-id", "app1", "--display-name", "e", "--owner-id", ""}, + "--owner-id requires a non-empty value for every occurrence", + }, + { + "empty owner id alongside a real one", + []string{"--app-id", "app1", "--display-name", "e", "--owner-id", "", "--owner-id", "u1"}, + "--owner-id requires a non-empty value for every occurrence", + }, + { + "whitespace-only owner id", + []string{"--app-id", "app1", "--display-name", "e", "--owner-id", " "}, + "--owner-id requires a non-empty value for every occurrence", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + _, err := buildEntitlementCreatePlan(newEntitlementCreateCmd(t, tc.args...)) + if err == nil { + t.Fatalf("expected a usage error for %v", tc.args) + } + if !strings.Contains(err.Error(), tc.want) { + t.Errorf("error = %q, want it to mention %q", err, tc.want) + } + if code := exitCode(err); code != exitUsage { + t.Errorf("exit code = %d, want %d", code, exitUsage) + } + }) + } +} + +// entitlementCreateServer is a stub of the three create endpoints. It records +// each (path, body) it receives and answers with ids the next call must pick +// up, so a break in the chain shows up as a wrong path or a wrong id. +type entitlementCreateServer struct { + srv *httptest.Server + paths []string + bodies []map[string]any + failEnt bool + failRes bool +} + +func newEntitlementCreateServer(t *testing.T, failEnt bool) *entitlementCreateServer { + t.Helper() + s := &entitlementCreateServer{failEnt: failEnt} + s.srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var body map[string]any + if err := json.NewDecoder(r.Body).Decode(&body); err != nil { + t.Errorf("decoding body for %s: %v", r.URL.Path, err) + } + s.paths = append(s.paths, r.URL.Path) + s.bodies = append(s.bodies, body) + w.Header().Set("Content-Type", "application/json") + switch { + case strings.HasSuffix(r.URL.Path, "/resource_types"): + _, _ = w.Write([]byte(`{"appResourceType":{"id":"rt-new"},"expanded":[]}`)) + case strings.HasSuffix(r.URL.Path, "/resources"): + if s.failRes { + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte(`{"code":3,"message":"nope"}`)) + return + } + _, _ = w.Write([]byte(`{"appResource":{"id":"res-new"}}`)) + case strings.HasSuffix(r.URL.Path, "/entitlements"): + if s.failEnt { + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte(`{"code":3,"message":"nope"}`)) + return + } + _, _ = w.Write([]byte(`{"appEntitlementView":{"appEntitlement":{"id":"ent-new"}}}`)) + default: + t.Errorf("unexpected path %q", r.URL.Path) + w.WriteHeader(http.StatusNotFound) + } + })) + t.Cleanup(s.srv.Close) + return s +} + +// TestEntitlementCreateRunChainsIDs drives the wired end-to-end path: each +// response's id must address the next request, and the entitlement body must +// carry both ids the server requires. +func TestEntitlementCreateRunChainsIDs(t *testing.T) { + s := newEntitlementCreateServer(t, false) + p := mustPlan(t, "--app-id", "app1", "--display-name", "Payroll admin") + + var out bytes.Buffer + cmd := &cobra.Command{} + cmd.SetOut(&out) + cmd.SetContext(context.Background()) + + if err := p.run(cmd, client.NewForTesting(s.srv.URL, s.srv.Client())); err != nil { + t.Fatalf("run: %v", err) + } + + wantPaths := []string{ + "/api/v1/apps/app1/resource_types", + "/api/v1/apps/app1/resource_types/rt-new/resources", + "/api/v1/apps/app1/entitlements", + } + if !reflect.DeepEqual(s.paths, wantPaths) { + t.Errorf("paths = %v, want %v", s.paths, wantPaths) + } + ent := s.bodies[2] + if ent["appResourceTypeId"] != "rt-new" || ent["appResourceId"] != "res-new" { + t.Errorf("entitlement body = %v, want the ids from the two preceding responses", ent) + } + if !strings.Contains(out.String(), `"id": "ent-new"`) { + t.Errorf("output = %q, want the raw entitlement response", out.String()) + } +} + +// TestEntitlementCreateRunReportsPartialCreates pins that a failure part-way +// through names what already exists and how to reuse it -- the objects are not +// rolled back, so an unnamed id would be an orphan the caller can't find. +func TestEntitlementCreateRunReportsPartialCreates(t *testing.T) { + s := newEntitlementCreateServer(t, true) + p := mustPlan(t, "--app-id", "app1", "--display-name", "Payroll admin") + + cmd := &cobra.Command{} + cmd.SetOut(new(bytes.Buffer)) + cmd.SetContext(context.Background()) + + err := p.run(cmd, client.NewForTesting(s.srv.URL, s.srv.Client())) + if err == nil { + t.Fatal("expected the entitlement create to fail") + } + for _, want := range []string{"--resource-type-id rt-new", "--resource-id res-new"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error = %q, want it to name %q", err, want) + } + } + // The 400 must still classify as a usage error, not be masked by the wrap. + if code := exitCode(err); code != exitUsage { + t.Errorf("exit code = %d, want %d", code, exitUsage) + } +} + +// TestEntitlementCreateRunReusedObjectsAreNotClaimed pins that the failure +// message only names objects THIS run created: reporting a caller-supplied id +// as "already created" would invite them to delete their own resource type. +func TestEntitlementCreateRunReusedObjectsAreNotClaimed(t *testing.T) { + s := newEntitlementCreateServer(t, true) + p := mustPlan(t, "--app-id", "app1", "--display-name", "e", "--resource-type-id", "rt-mine", "--resource-id", "res-mine") + + cmd := &cobra.Command{} + cmd.SetOut(new(bytes.Buffer)) + cmd.SetContext(context.Background()) + + err := p.run(cmd, client.NewForTesting(s.srv.URL, s.srv.Client())) + if err == nil { + t.Fatal("expected the entitlement create to fail") + } + if strings.Contains(err.Error(), "already created") { + t.Errorf("error = %q, but this run created nothing", err) + } + if len(s.paths) != 1 || s.paths[0] != "/api/v1/apps/app1/entitlements" { + t.Errorf("paths = %v, want the entitlement create only", s.paths) + } +} + +// TestEntitlementCreateDryRunPreviewsEveryRequest pins that a dry run shows all +// three writes, not just the first -- the other two are writes the caller would +// otherwise not see coming. +func TestEntitlementCreateDryRunPreviewsEveryRequest(t *testing.T) { + p := mustPlan(t, "--app-id", "app1", "--display-name", "Payroll admin") + + var out bytes.Buffer + cmd := &cobra.Command{} + cmd.SetOut(&out) + if err := p.previewRequests(cmd); err != nil { + t.Fatalf("printDryRun: %v", err) + } + + got := out.String() + for _, want := range []string{ + "[dry-run] POST /api/v1/apps/app1/resource_types\n", + "[dry-run] POST /api/v1/apps/app1/resource_types/" + newResourceTypeIDPlaceholder + "/resources\n", + "[dry-run] POST /api/v1/apps/app1/entitlements\n", + `"appResourceId": "` + newResourceIDPlaceholder + `"`, + } { + if !strings.Contains(got, want) { + t.Errorf("dry run output missing %q:\n%s", want, got) + } + } + if n := strings.Count(got, "[dry-run] POST"); n != 3 { + t.Errorf("previewed %d requests, want 3:\n%s", n, got) + } + + // A fully-specified run has nothing to stand in for, so it must not print + // placeholders at all. + out.Reset() + single := mustPlan(t, "--app-id", "app1", "--display-name", "e", "--resource-type-id", "rt1", "--resource-id", "r1") + if err := single.previewRequests(cmd); err != nil { + t.Fatalf("printDryRun: %v", err) + } + if strings.Contains(out.String(), "NEW_APP_RESOURCE") { + t.Errorf("placeholder leaked into a fully-specified preview:\n%s", out.String()) + } +} + +// TestCreatedObjectIDRejectsUnusableResponse pins that a 200 carrying no id is +// a server failure (exit 6), not a success that sends the next request to a +// path with an empty segment. +func TestCreatedObjectIDRejectsUnusableResponse(t *testing.T) { + cases := []struct { + name string + data string + }{ + {"not json", `nope`}, + {"key missing", `{"expanded":[]}`}, + {"id empty", `{"appResourceType":{"id":""}}`}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + id, err := createdObjectID([]byte(tc.data), "appResourceType") + if err == nil { + t.Fatalf("id = %q, want an error", id) + } + var nonJSON *nonJSONResponseError + if !errors.As(err, &nonJSON) { + t.Errorf("error %v is not a *nonJSONResponseError, so it would exit 1", err) + } + if code := exitCode(err); code != exitServer { + t.Errorf("exit code = %d, want %d", code, exitServer) + } + }) + } +} + +// TestResourceTypeKindsAreDocumented holds the one enum list to every place +// that restates it. The command deliberately does NOT validate --resource-type +// against this list -- the server stays authoritative if the enum grows -- but +// the help text and docs freeze it either way, and a contract restated in four +// places drifts unless something fails when it does. +func TestResourceTypeKindsAreDocumented(t *testing.T) { + docs := map[string]string{ + "entitlements create --help": entitlementsCreateCmd.Long, + "--resource-type flag usage": entitlementsCreateCmd.Flags().Lookup("resource-type").Usage, + "README.md": readDocFile(t, "../README.md"), + "guideConfigureNewApp": guideConfigureNewApp, + } + for name, doc := range docs { + for _, kind := range resourceTypeKinds { + if !strings.Contains(doc, kind) { + t.Errorf("%s does not name the %q resource type kind", name, kind) + } + } + } + + // The default must be one of them, or --help promises a value that 400s. + def := entitlementsCreateCmd.Flags().Lookup("resource-type").DefValue + if !slices.Contains(resourceTypeKinds, def) { + t.Errorf("--resource-type defaults to %q, which is not in resourceTypeKinds", def) + } +} + +// TestEntitlementCreateDryRunBannerNamesOnlyUsedPlaceholders pins that the +// banner announces exactly the stand-ins the preview goes on to use. Naming +// one that never appears is the one place a dry run could overclaim. +func TestEntitlementCreateDryRunBannerNamesOnlyUsedPlaceholders(t *testing.T) { + cases := []struct { + name string + args []string + want []string + notWant []string + }{ + { + "creates both", + []string{"--app-id", "app1", "--display-name", "e"}, + []string{newResourceTypeIDPlaceholder, newResourceIDPlaceholder, "stand in for ids"}, + []string{"stands in for an id"}, + }, + { + "reuses the resource type", + []string{"--app-id", "app1", "--display-name", "e", "--resource-type-id", "rt1"}, + []string{newResourceIDPlaceholder, "stands in for an id"}, + []string{newResourceTypeIDPlaceholder}, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + var out bytes.Buffer + cmd := &cobra.Command{} + cmd.SetOut(&out) + if err := mustPlan(t, tc.args...).previewRequests(cmd); err != nil { + t.Fatalf("previewRequests: %v", err) + } + banner, _, _ := strings.Cut(out.String(), "\n") + for _, w := range tc.want { + if !strings.Contains(banner, w) { + t.Errorf("banner %q does not name %s, which the preview uses", banner, w) + } + } + for _, w := range tc.notWant { + if strings.Contains(banner, w) { + t.Errorf("banner %q claims %q, which does not describe this preview", banner, w) + } + // A placeholder must not appear anywhere in a preview whose + // id was supplied, banner or request. + if strings.HasPrefix(w, "NEW_") && strings.Contains(out.String(), w) { + t.Errorf("preview used %s despite the id being supplied:\n%s", w, out.String()) + } + } + }) + } +} + +// TestEntitlementCreateDurationGrantWireKey pins the flag-name-to-field +// mapping: --duration-grant is the one flag whose wire key differs from its +// name, and a mismatch would be silently dropped by the server. +func TestEntitlementCreateDurationGrantWireKey(t *testing.T) { + p := mustPlan(t, "--app-id", "app1", "--display-name", "e", "--duration-grant", "3600s") + if got := p.entitlementBody["durationGrant"]; got != "3600s" { + t.Errorf("durationGrant = %v, want 3600s (body: %v)", got, p.entitlementBody) + } + if _, ok := p.entitlementBody["duration-grant"]; ok { + t.Error("the flag name leaked into the body as a wire key") + } + + // Omitted means standing access; an empty key would pick the wrong arm of + // the max_grant_duration oneof. + bare := mustPlan(t, "--app-id", "app1", "--display-name", "e") + if _, ok := bare.entitlementBody["durationGrant"]; ok { + t.Errorf("durationGrant sent without the flag: %v", bare.entitlementBody) + } +} + +// TestNormalizeResourceType pins case/separator normalization, and that an +// unknown value is passed through for the server to rule on rather than being +// rejected against a list that can go stale. +func TestNormalizeResourceType(t *testing.T) { + cases := map[string]string{ + "custom": "CUSTOM", + "CUSTOM": "CUSTOM", + "profile-type": "PROFILE_TYPE", + "profile_type": "PROFILE_TYPE", + "something_new": "SOMETHING_NEW", + } + for in, want := range cases { + if got := normalizeResourceType(in); got != want { + t.Errorf("normalizeResourceType(%q) = %q, want %q", in, got, want) + } + } +} + +// TestEntitlementCreateRetryRemediationIsAccepted pins that the retry the +// failure message hands back is a command that works: the ids it names make +// the create-only flags of the original invocation a usage error, so the +// message has to name those for dropping too. +func TestEntitlementCreateRetryRemediationIsAccepted(t *testing.T) { + s := newEntitlementCreateServer(t, true) + args := []string{ + "--app-id", "app1", "--display-name", "e", + "--resource-type", "ROLE", + "--resource-type-display-name", "rt name", + "--resource-display-name", "res name", + } + + cmd := &cobra.Command{} + cmd.SetOut(new(bytes.Buffer)) + cmd.SetContext(context.Background()) + runErr := mustPlan(t, args...).run(cmd, client.NewForTesting(s.srv.URL, s.srv.Client())) + if runErr == nil { + t.Fatal("expected the entitlement create to fail") + } + + add, drop := parseRemediation(t, runErr.Error()) + retry := append(withoutFlags(args, drop), add...) + if _, err := buildEntitlementCreatePlan(newEntitlementCreateCmd(t, retry...)); err != nil { + t.Fatalf("the remediation %q produces %v, so the retry it advises exits 2", runErr, err) + } +} + +// parseRemediation pulls the flags to add and the flags to drop out of the +// "(already created: ...)" tail. +func parseRemediation(t *testing.T, msg string) (add, drop []string) { + t.Helper() + _, rest, ok := strings.Cut(msg, "re-run with ") + if !ok { + t.Fatalf("error %q names no retry", msg) + } + rest, _, ok = strings.Cut(rest, " to reuse instead of duplicating)") + if !ok { + t.Fatalf("error %q has no remediation tail", msg) + } + addPart, dropPart, hasDrop := strings.Cut(rest, ", dropping ") + if hasDrop { + drop = strings.Fields(strings.TrimSuffix(dropPart, ",")) + } + return strings.Fields(addPart), drop +} + +// withoutFlags removes each named flag and its value from an argument list of +// "--flag value" pairs. +func withoutFlags(args, remove []string) []string { + var kept []string + for i := 0; i+1 < len(args); i += 2 { + if !slices.Contains(remove, args[i]) { + kept = append(kept, args[i], args[i+1]) + } + } + return kept +} + +// TestResourceTypeSingletonIsDocumented holds the server's own error string in +// every doc that promises the restriction, so it stays greppable from both +// directions. Reproduced live: a second ROLE, GROUP or VAULT resource type on +// entitlementCreateDocs are the four sources that carry this command's +// behavioural claims. The other guards in this file deliberately use narrower +// sets -- the kinds list also checks the flag's own usage string, and the +// remediation quote lives in only two docs -- so those keep their own. +func entitlementCreateDocs(t *testing.T) map[string]string { + t.Helper() + return map[string]string{ + "entitlements create --help": entitlementsCreateCmd.Long, + "README.md": readDocFile(t, "../README.md"), + "cmd/agents.md (embedded)": agentsTemplate, + "docs guide configure-new-app": guideConfigureNewApp, + } +} + +// one app 500s, while a second CUSTOM one succeeds. +func TestResourceTypeSingletonIsDocumented(t *testing.T) { + const serverError = "app resource type already exists" + docs := entitlementCreateDocs(t) + for name, doc := range docs { + if !strings.Contains(doc, serverError) { + t.Errorf("%s does not quote the server's %q error", name, serverError) + } + } +} + +// TestRemediationStringIsDocumentedVerbatim pins the docs that quote the +// partial-failure message to what createdSoFar actually emits. The message +// changed to name the flags a retry must drop, and the guide kept quoting the +// old form in the same commit — a retry copied from it would be refused. +func TestRemediationStringIsDocumentedVerbatim(t *testing.T) { + // Built by the real planner, so a regression that stopped populating + // typeOnlyFlags fails here instead of leaving a hand-made plan green. + plan := mustPlan(t, "--app-id", "a", "--display-name", "e", + "--resource-type-display-name", "rt") + msg := strings.TrimSpace(plan.createdSoFar("", "")) + if !strings.Contains(msg, "dropping --resource-type-display-name") { + t.Fatalf("createdSoFar produced %q, which names no flag to drop; "+ + "this guard would then pass on any doc", msg) + } + docs := map[string]string{ + "entitlements create --help": entitlementsCreateCmd.Long, + "docs guide configure-new-app": guideConfigureNewApp, + } + for name, doc := range docs { + // Verbatim, not a keyword: renaming the flag must break the docs that + // quote it, which a check for the word "dropping" alone would not. + if !strings.Contains(flatten(doc), msg) { + t.Errorf("%s does not quote the partial-failure message verbatim.\nwant: %s", name, msg) + } + } +} + +// TestRemediationOnResourceFailureKeepsResourceName covers the step-2 failure: +// the resource type exists, the resource does not. The retry must drop the +// type-only flags but KEEP --resource-display-name, since the object it names +// was never created. Getting that wrong is silent — the resource would be +// created under --display-name instead — where the step-3 case fails loudly. +func TestRemediationOnResourceFailureKeepsResourceName(t *testing.T) { + s := newEntitlementCreateServer(t, false) + s.failRes = true + + args := []string{ + "--app-id", "app1", "--display-name", "e", + "--resource-type", "ROLE", + "--resource-type-display-name", "rt name", + "--resource-display-name", "res name", + } + cmd := &cobra.Command{} + cmd.SetOut(new(bytes.Buffer)) + cmd.SetContext(context.Background()) + runErr := mustPlan(t, args...).run(cmd, client.NewForTesting(s.srv.URL, s.srv.Client())) + if runErr == nil { + t.Fatal("expected the resource create to fail") + } + msg := runErr.Error() + + // The advised retry must also be accepted, same as the step-3 case. + add, drop := parseRemediation(t, msg) + retry := append(withoutFlags(args, drop), add...) + if _, err := buildEntitlementCreatePlan(newEntitlementCreateCmd(t, retry...)); err != nil { + t.Fatalf("the step-2 remediation %q produces %v, so the retry it advises exits 2", msg, err) + } + if !strings.Contains(msg, "--resource-type-id") { + t.Errorf("message does not name the created resource type: %q", msg) + } + if strings.Contains(msg, "--resource-id") { + t.Errorf("message names a resource id, but the resource was never created: %q", msg) + } + if strings.Contains(msg, "--resource-display-name") { + t.Errorf("message tells the caller to drop --resource-display-name, but the resource "+ + "it names does not exist yet; the retry would create it under --display-name: %q", msg) + } +} + +// reuseDropClauses are the canonical instructions, required in every doc that +// advises reusing an object. Fixed clauses, not pattern-matched prose: earlier +// heuristic versions were each satisfiable by the wrong text. +var reuseDropClauses = map[string]string{ + "--resource-type-id": "drop both --resource-type and --resource-type-display-name", + "--resource-id": "drop --resource-display-name", +} + +// reusePairs ties each id flag to the list the code actually refuses beside it. +var reusePairs = map[string][]string{ + "--resource-type-id": typeCreateOnlyFlags, + "--resource-id": resourceCreateOnlyFlags, +} + +// TestReuseAdviceIsDocumented holds every doc that advises reusing an existing +// object to the full list of flags the code refuses alongside its id. Advice +// naming only one of two refused flags exited 2, which is what this prevents. +func TestReuseAdviceIsDocumented(t *testing.T) { + for idFlag, flags := range reusePairs { + clause, ok := reuseDropClauses[idFlag] + if !ok { + t.Fatalf("%s refuses flags but has no documented clause", idFlag) + } + for _, f := range flags { + // docMentionsFlag, not Contains: --resource-type is a prefix of + // --resource-type-display-name, so a substring check here is + // satisfied by the other flag in the very same clause. + if !docMentionsFlag(clause, f) { + t.Fatalf("the clause for %s omits --%s, which the code refuses", idFlag, f) + } + } + } + + docs := entitlementCreateDocs(t) + for name, doc := range docs { + // Backticks stripped so a doc can format flags as code, which is these + // files' own convention; requiring the bare form banned correct markdown. + flat := strings.ReplaceAll(flatten(doc), "`", "") + for idFlag, clause := range reuseDropClauses { + if !docMentionsFlag(flat, strings.TrimPrefix(idFlag, "--")) { + t.Errorf("%s never names %s, so its reuse advice cannot be found", name, idFlag) + continue + } + at := strings.Index(flat, clause) + if at < 0 { + t.Errorf("%s does not carry the reuse clause for %s.\nwant: %s", name, idFlag, clause) + continue + } + // A nearby negation inverts the instruction while satisfying it. + lead := strings.ToLower(flat[max(0, at-24):at]) + if strings.Contains(lead, "not ") || strings.Contains(lead, "never ") { + t.Errorf("%s negates the reuse clause for %s: %q", name, idFlag, flat[max(0, at-24):at+len(clause)]) + } + } + } +} diff --git a/cmd/flags.go b/cmd/flags.go index 2c8ee7a..4cd9c89 100644 --- a/cmd/flags.go +++ b/cmd/flags.go @@ -245,6 +245,28 @@ func requireNonEmpty(cmd *cobra.Command, names ...string) error { } } +// requireNonEmptyIfSet errors when a flag was passed with an empty value, while +// leaving an unset flag alone. Distinct from requireNonEmpty, which is for +// flags that must always be present: use this for an *optional* flag whose +// empty value would change behavior rather than fail — an id flag selecting an +// existing object (`--resource-type-id "$RT_ID"` with RT_ID unset would +// silently create a second one), or one that scopes a read. +func requireNonEmptyIfSet(cmd *cobra.Command, name string) (string, error) { + v, _ := cmd.Flags().GetString(name) + if v == "" && cmd.Flags().Changed(name) { + return "", &usageError{fmt.Errorf("flag --%s requires a non-empty value", name)} + } + return v, nil +} + +// flagOrDefault returns a string flag's value, falling back to def when unset. +func flagOrDefault(cmd *cobra.Command, name, def string) string { + if v, _ := cmd.Flags().GetString(name); v != "" { + return v + } + return def +} + // addRepeatableStringFlag registers a repeatable string flag. It always uses // StringArray, never StringSlice: StringSlice CSV-splits every occurrence, so // `--user-id "" --user-id REAL` reaches the command as ["REAL"] — the empty