diff --git a/CHANGELOG.md b/CHANGELOG.md index 7676425..6faaa15 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,32 @@ to follow [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### Added +- **`c1i tasks restart`, `reset`, `skip-step`, `process` and + `update-grant-duration`.** The task action family was five commands wrapping + thirteen server routes; these add the five whose behaviour could be + demonstrated. All ten now share one runner — `approve`, `deny`, `comment`, + `close` and `reassign` each hand-rolled the same request-and-confirm + sequence, and their help text and dry-run output are unchanged. + + Verified by effect rather than exit code. `restart`, `reset` and `skip-step` + all rotate the task's current policy step; `restart` and `skip-step` add one + history entry, `reset` four, because it restarts the policy rather than the + step. Neither `restart` nor `reset` reopens a closed task — the state stays + `TASK_STATE_CLOSED`. `process` changes nothing observable on a healthy task; + the stalled case was not reproduced. `update-grant-duration` lands as + `grantDuration` on the task, takes a protobuf duration (`3600s`, not `1h`), + and is refused once the task reaches provisioning with `cannot update grant + duration for a ticket in a provision step`. + + Which actions a task accepts depends on its state; the rest are refused with + `action not permitted`. Read the task's own list with + `c1i api --path /api/v1/tasks/ --fields actions`. + + Not wrapped: `escalate` could not be demonstrated even with emergency grants + enabled on the entitlement, `update-request-data` takes a free-form object + that wants a body-file flag, and `approve-with-step-up` needs a step-up + transaction id the CLI cannot obtain. + - **`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 diff --git a/README.md b/README.md index ffcd97d..4fdba44 100644 --- a/README.md +++ b/README.md @@ -204,8 +204,33 @@ c1i tasks deny [--policy-step-id ] [--comment ] c1i tasks comment --comment c1i tasks close [--comment ] c1i tasks reassign --to-user-id [--to-user-id ...] [--policy-step-id ] [--comment ] +c1i tasks restart [--policy-step-id ] [--comment ] +c1i tasks reset [--comment ] +c1i tasks skip-step [--policy-step-id ] [--comment ] +c1i tasks process +c1i tasks update-grant-duration --duration ``` +`restart`, `reset` and `skip-step` each rotate the task's current policy step, +so a `--policy-step-id` captured before one of them goes stale — the server +answers `this action is no longer available: the request has advanced to a new +approval step`. Omit the flag to act on whatever step is current. + +Which actions a task accepts depends on its state; the server refuses the rest +with `action not permitted`. Read the task's own list with +`c1i api --path /api/v1/tasks/ --fields actions`. + +`restart` re-runs the current approval step; `reset` restarts the whole policy. +Neither reopens a closed task. `process` changes nothing observable on a +healthy task — it is intended for one that has stalled, which was not +reproduced here. `update-grant-duration` takes a +protobuf duration (`3600s`, not `1h`) and only applies before the task reaches +provisioning, after which the server answers `cannot update grant duration for +a ticket in a provision step`; the value lands as `grantDuration`. + +`escalate`, `update-request-data` and `approve-with-step-up` are not wrapped; +reach them through `c1i api`. + `approve`/`deny`/`reassign` target a specific policy step. If `--policy-step-id` is omitted, the task's currently executing step is fetched and used automatically for all three — but `approve` and `reassign` require a resolvable diff --git a/cmd/agents.md b/cmd/agents.md index 17448e9..d4f65f2 100644 --- a/cmd/agents.md +++ b/cmd/agents.md @@ -326,7 +326,22 @@ resource with `--resource-id` likewise means you drop close`/`reassign` therefore never print a state (`close` reports `task_id`, `reassign` also the `policy_step_id`); if you call these actions through `api`, read the task back rather than trusting the response's - `state`. + `state`. The same holds for `restart`, `reset`, `skip-step`, `process` and + `update-grant-duration`. +- Which actions a task accepts depends on its state, and the server refuses the + rest with `action not permitted`. Read the task's own list first: + `c1i api --path /api/v1/tasks/ --fields actions`. `restart`, `reset` and + `skip-step` each rotate the current policy step (measured), so a + `--policy-step-id` captured before one of them is stale and answers `this + action is no longer available: the request has advanced to a new approval + step` -- omit the flag to act on whatever step is current. +- `restart` re-runs the current approval step (one new history entry); `reset` + restarts the whole policy (four, measured). Neither reopens a closed task -- + the state stays `TASK_STATE_CLOSED`. `process` changes nothing observable on + a healthy task. `update-grant-duration` needs a protobuf duration (`3600s`, + not `1h`) and only works before provisioning, after which the server says + `cannot update grant duration for a ticket in a provision step`; the value + lands as `grantDuration`. - 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. diff --git a/cmd/tasks.go b/cmd/tasks.go index cfa4f73..c649885 100644 --- a/cmd/tasks.go +++ b/cmd/tasks.go @@ -157,14 +157,15 @@ func parseCurrentPolicyStepID(data []byte) (string, error) { return resp.TaskView.Task.Policy.Current.ID, nil } -// resolvePolicyStepID returns the policy step ID to use for an approve/deny -// action. If the user supplied one explicitly it is used as-is; otherwise the -// task is fetched and its currently executing step ID is used. +// resolvePolicyStepID returns the policy step id an action should target: +// the explicit --policy-step-id when given, otherwise the task's currently +// executing step, fetched with a GET. // -// approve requires policyStepId, so callers pass required=true to turn an -// underivable step into an error. deny treats it as optional (the API does -// not require it), so it passes required=false and simply omits the field -// when no current step can be derived. +// required distinguishes the two modes callers need. Actions the server +// rejects without a step (approve, skip-step) and reassign, which we refuse to +// send ambiguously, pass true and get an error. deny and restart pass false: +// when the step cannot be derived the field is omitted rather than blocking +// the action, which is what lets restart act on a closed task. func resolvePolicyStepID(ctx context.Context, c *client.Client, taskID, explicit string, required bool) (string, error) { if explicit != "" { return explicit, nil diff --git a/cmd/tasks_action.go b/cmd/tasks_action.go new file mode 100644 index 0000000..2ea0877 --- /dev/null +++ b/cmd/tasks_action.go @@ -0,0 +1,96 @@ +package cmd + +import ( + "fmt" + + "github.com/ConductorOne/c1i/internal/client" + "github.com/spf13/cobra" +) + +// policyStepMode says whether an action's request needs a policy step id. +type policyStepMode int + +const ( + stepUnused policyStepMode = iota // the endpoint takes no policyStepId + stepOptional // send it when it can be resolved, omit otherwise + stepRequired // the server rejects the call without it +) + +// taskAction describes one POST /api/v1/tasks/{id}/action/{verb} command. +// These fields are all the commands differ by, so they share one RunE. +type taskAction struct { + verb string // the path segment, e.g. "restart" + step policyStepMode + // extraBody adds fields beyond comment/policyStepId, and may reject bad + // flag combinations. Runs before any client is built, so it exits 2. + extraBody func(cmd *cobra.Command, body map[string]any) error + // confirm formats the success line. State is passed but is the task's + // PRE-action state, so most actions must not print it. + confirm func(id, state, stepID string) string +} + +// runTaskAction is the shared RunE. Flags are validated before a client is +// built, so a usage error exits 2 rather than failing on credentials. +func (a taskAction) runTaskAction(cmd *cobra.Command, args []string) error { + var comment string + if cmd.Flags().Lookup("comment") != nil { + comment, _ = cmd.Flags().GetString("comment") + } + + body := map[string]any{} + if a.extraBody != nil { + if err := a.extraBody(cmd, body); err != nil { + return err + } + } + + taskID := args[0] + path := client.Path("/api/v1/tasks/%s/action/%s", taskID, a.verb) + if comment != "" { + body["comment"] = comment + } + + // Resolved even for a preview, so --dry-run still rejects a bad --url and + // still names the tenant it would hit. + baseURL, err := GetBaseURL() + if err != nil { + return err + } + + // Credentials are only needed to send, or to fetch a step. Matches what + // each command did before sharing this runner. + if a.step == stepUnused && dryRunActive() { + return printDryRun(cmd, "POST", path, body) + } + c, err := newClient(cmd, baseURL) + if err != nil { + return fmt.Errorf("authentication failed: %w", err) + } + + var stepID string + if a.step != stepUnused { + explicit, _ := cmd.Flags().GetString("policy-step-id") + stepID, err = resolvePolicyStepID(cmd.Context(), c, taskID, explicit, a.step == stepRequired) + if err != nil { + return err + } + if stepID != "" { + body["policyStepId"] = stepID + } + if dryRunActive() { + return printDryRun(cmd, "POST", path, body) + } + } + + data, err := c.Post(cmd.Context(), path, body) + if err != nil { + return fmt.Errorf("API error: %w", err) + } + id, state, err := parseTaskActionResponse(data) + if err != nil { + return fmt.Errorf("failed to parse response: %w", err) + } + + _, _ = fmt.Fprintf(cmd.OutOrStdout(), "%s", a.confirm(id, state, stepID)) + return nil +} diff --git a/cmd/tasks_action_test.go b/cmd/tasks_action_test.go new file mode 100644 index 0000000..c57ea0e --- /dev/null +++ b/cmd/tasks_action_test.go @@ -0,0 +1,465 @@ +package cmd + +import ( + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "bytes" + "context" + + "errors" + + "github.com/ConductorOne/c1i/internal/client" + "github.com/spf13/cobra" + "github.com/spf13/viper" +) + +const actionTestTaskID = "zz-c1i-test-task-2" + +// taskActionRecorder answers every action POST with a success body and records +// the path and body it received. +type taskActionRecorder struct { + srv *httptest.Server + paths []string + bodies []map[string]any +} + +func newTaskActionRecorder(t *testing.T, state, currentStepID string) *taskActionRecorder { + t.Helper() + r := &taskActionRecorder{} + r.srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + w.Header().Set("Content-Type", "application/json") + // A GET is the policy-step lookup resolvePolicyStepID performs. + if req.Method == http.MethodGet { + _, _ = w.Write([]byte(`{"taskView":{"task":{"id":"` + actionTestTaskID + + `","state":"` + state + `","policy":{"current":{"id":"` + currentStepID + `"}}}}}`)) + return + } + raw, _ := io.ReadAll(req.Body) + var body map[string]any + if len(raw) > 0 { + if err := json.Unmarshal(raw, &body); err != nil { + t.Errorf("decoding body for %s: %v", req.URL.Path, err) + } + } + r.paths = append(r.paths, req.URL.Path) + r.bodies = append(r.bodies, body) + _, _ = w.Write([]byte(taskActionResponse(actionTestTaskID, state))) + })) + t.Cleanup(r.srv.Close) + return r +} + +// taskActionExpectations pins each action's path and step mode. +// TestEveryTaskActionIsPinned requires a row per command, so a new command +// cannot go untested. +var taskActionExpectations = map[string]struct { + verb string + step policyStepMode + // setup supplies flags the command requires before it will run. + setup func(cmd *cobra.Command) +}{ + "approve": {verb: "approve", step: stepRequired}, + "deny": {verb: "deny", step: stepOptional}, + "close": {verb: "close", step: stepUnused}, + "restart": {verb: "restart", step: stepOptional}, + "reset": {verb: "reset", step: stepUnused}, + "skip-step": {verb: "skip-step", step: stepRequired}, + "process": {verb: "process", step: stepUnused}, + "comment": {verb: "comment", step: stepUnused, setup: func(c *cobra.Command) { _ = c.Flags().Set("comment", "zz") }}, + "update-grant-duration": {verb: "update-grant-duration", step: stepUnused, setup: func(c *cobra.Command) { _ = c.Flags().Set("duration", "3600s") }}, + "reassign": {verb: "reassign", step: stepRequired, setup: func(c *cobra.Command) { _ = c.Flags().Set("to-user-id", "zz-user") }}, +} + +// nonActionTaskSubcommands are the tasks subcommands that are not action POSTs. +var nonActionTaskSubcommands = map[string]bool{"list": true} + +// TestEveryTaskActionIsPinned requires a row for every action in the tree. +func TestEveryTaskActionIsPinned(t *testing.T) { + seen := 0 + for _, c := range tasksCmd.Commands() { + name := c.Name() + if nonActionTaskSubcommands[name] { + continue + } + seen++ + if _, ok := taskActionExpectations[name]; !ok { + t.Errorf("tasks %s has no row in taskActionExpectations, so nothing pins its action path or policy-step behaviour", name) + } + } + if seen == 0 { + t.Fatal("found no task action commands — this guard is not looking at what it thinks it is") + } + for name := range taskActionExpectations { + if findTasksSubcommandOrNil(name) == nil { + t.Errorf("taskActionExpectations lists %q, which is no longer a tasks subcommand", name) + } + } +} + +// TestEveryTaskActionPostsItsOwnVerbAndStep checks path and policyStepId on +// the wire. A copied verb performs a different action while printing success. +func TestEveryTaskActionPostsItsOwnVerbAndStep(t *testing.T) { + const step = "zz-step-1111111111111111111" + for name, want := range taskActionExpectations { + t.Run(name, func(t *testing.T) { + cmd := findTasksSubcommand(t, name) + resetCmds(t, cmd) + if want.setup != nil { + want.setup(cmd) + } + r := newTaskActionRecorder(t, "TASK_STATE_OPEN", step) + if _, err := runTaskActionCmd(t, cmd, r.srv, actionTestTaskID); err != nil { + t.Fatalf("%s: %v", name, err) + } + if len(r.paths) != 1 { + t.Fatalf("%s posted %d times, want 1: %v", name, len(r.paths), r.paths) + } + if got, wantPath := r.paths[0], "/api/v1/tasks/"+actionTestTaskID+"/action/"+want.verb; got != wantPath { + t.Errorf("%s posted %q, want %q", name, got, wantPath) + } + got, ok := r.bodies[0]["policyStepId"] + if want.step != stepUnused { + if !ok || got != step { + t.Errorf("%s body policyStepId = %v (present=%v), want %q", name, got, ok, step) + } + return + } + if ok { + t.Errorf("%s sent policyStepId=%v to an endpoint that takes none", name, got) + } + }) + } +} + +// TestTasksCommentAlwaysSendsTheCommentKey: an omitted key records nothing +// while the command still prints success. +func TestTasksCommentAlwaysSendsTheCommentKey(t *testing.T) { + cmd := findTasksSubcommand(t, "comment") + resetCmds(t, cmd) + _ = cmd.Flags().Set("comment", "") + r := newTaskActionRecorder(t, "TASK_STATE_OPEN", "zz-step-1111111111111111111") + if _, err := runTaskActionCmd(t, cmd, r.srv, actionTestTaskID); err != nil { + t.Fatalf("comment: %v", err) + } + got, ok := r.bodies[0]["comment"] + if !ok || got != "" { + t.Errorf("comment body = %v (present=%v), want an empty string present", got, ok) + } +} + +// TestTasksDenyOmitsAnUnresolvableStep: the field must be absent, not empty, +// and the denial must still go through. +func TestTasksDenyOmitsAnUnresolvableStep(t *testing.T) { + cmd := findTasksSubcommand(t, "deny") + resetCmds(t, cmd) + r := newTaskActionRecorder(t, "TASK_STATE_OPEN", "") // no current step + if _, err := runTaskActionCmd(t, cmd, r.srv, actionTestTaskID); err != nil { + t.Fatalf("deny: %v", err) + } + if got, ok := r.bodies[0]["policyStepId"]; ok { + t.Errorf("deny sent policyStepId=%v when no step could be resolved; the field must be omitted", got) + } + if len(r.paths) != 1 { + t.Errorf("deny posted %d times, want 1", len(r.paths)) + } +} + +// findTasksSubcommandOrNil is findTasksSubcommand without the fatal, for the +// reverse direction of the pinning check. +func findTasksSubcommandOrNil(name string) *cobra.Command { + for _, c := range tasksCmd.Commands() { + if c.Name() == name { + return c + } + } + return nil +} + +// TestTaskActionsNeverEchoResponseState extends the guarantee close already +// had to every action that does not intend to print state: these endpoints +// return the task as it was BEFORE the action, so echoing it reports the old +// state as though the action had not happened. +func TestTaskActionsNeverEchoResponseState(t *testing.T) { + for _, name := range []string{"restart", "reset", "skip-step", "process", "close", "comment", "update-grant-duration"} { + t.Run(name, func(t *testing.T) { + cmd := findTasksSubcommand(t, name) + resetCmds(t, cmd) + if name == "comment" { + _ = cmd.Flags().Set("comment", "zz") + } + if name == "update-grant-duration" { + _ = cmd.Flags().Set("duration", "3600s") + } + r := newTaskActionRecorder(t, "TASK_STATE_OPEN", "zz-step-1111111111111111111") + out, err := runTaskActionCmd(t, cmd, r.srv, actionTestTaskID) + if err != nil { + t.Fatalf("%s: %v", name, err) + } + if strings.Contains(out, "TASK_STATE_OPEN") { + t.Errorf("%s echoed the pre-action state: %q", name, out) + } + if !strings.Contains(out, actionTestTaskID) { + t.Errorf("%s did not report the task id: %q", name, out) + } + }) + } +} + +// TestTasksRestartOmitsEmptyPolicyStepField pins that a closed task, which has +// no current step, does not produce "policy_step_id=" with nothing after it. +func TestTasksRestartOmitsEmptyPolicyStepField(t *testing.T) { + cmd := findTasksSubcommand(t, "restart") + resetCmds(t, cmd) + r := newTaskActionRecorder(t, "TASK_STATE_CLOSED", "") + out, err := runTaskActionCmd(t, cmd, r.srv, actionTestTaskID) + if err != nil { + t.Fatalf("restart: %v", err) + } + if strings.Contains(out, "policy_step_id=\n") || strings.HasSuffix(strings.TrimRight(out, "\n"), "policy_step_id=") { + t.Errorf("restart printed an empty policy_step_id field: %q", out) + } +} + +// TestTasksUpdateGrantDurationRequiresDuration pins the usage error rather than +// letting the server answer "value is required" after a round trip. +func TestTasksUpdateGrantDurationRequiresDuration(t *testing.T) { + cmd := findTasksSubcommand(t, "update-grant-duration") + resetCmds(t, cmd) + _ = cmd.Flags().Set("duration", "") + r := newTaskActionRecorder(t, "TASK_STATE_OPEN", "zz-step-1111111111111111111") + _, err := runTaskActionCmd(t, cmd, r.srv, actionTestTaskID) + if err == nil { + t.Fatal("expected a usage error for an empty --duration") + } + if got := exitCode(err); got != exitUsage { + t.Errorf("exitCode = %d, want %d (exitUsage); err = %v", got, exitUsage, err) + } + if len(r.paths) != 0 { + t.Errorf("a request was sent despite the usage error: %v", r.paths) + } +} + +// findTasksSubcommand looks the command up in the real tree, so a command that +// stops being registered fails here instead of silently going untested. +func findTasksSubcommand(t *testing.T, name string) *cobra.Command { + t.Helper() + for _, c := range tasksCmd.Commands() { + if c.Name() == name { + return c + } + } + t.Fatalf("tasks has no %q subcommand", name) + return nil +} + +// TestEveryTaskActionModeBehavesOnAnUnresolvableStep separates stepRequired +// from stepOptional: with no derivable step, required errors before sending, +// optional sends without the field. +func TestEveryTaskActionModeBehavesOnAnUnresolvableStep(t *testing.T) { + for name, want := range taskActionExpectations { + if want.step == stepUnused { + continue + } + t.Run(name, func(t *testing.T) { + cmd := findTasksSubcommand(t, name) + resetCmds(t, cmd) + if want.setup != nil { + want.setup(cmd) + } + r := newTaskActionRecorder(t, "TASK_STATE_OPEN", "") // no current step + _, err := runTaskActionCmd(t, cmd, r.srv, actionTestTaskID) + if want.step == stepRequired { + if err == nil { + t.Fatalf("%s is stepRequired but succeeded with no derivable step", name) + } + if got := exitCode(err); got != exitUsage { + t.Errorf("%s exitCode = %d, want %d (exitUsage); err = %v", name, got, exitUsage, err) + } + if len(r.paths) != 0 { + t.Errorf("%s sent a request despite requiring a step: %v", name, r.paths) + } + return + } + // stepOptional: proceed, with the field omitted. + if err != nil { + t.Fatalf("%s is stepOptional but failed with no derivable step: %v", name, err) + } + if _, ok := r.bodies[0]["policyStepId"]; ok { + t.Errorf("%s sent policyStepId when none could be resolved", name) + } + }) + } +} + +// TestTaskActionsDryRunNeverSends: --dry-run must never reach the wire. +func TestTaskActionsDryRunNeverSends(t *testing.T) { + for name, want := range taskActionExpectations { + t.Run(name, func(t *testing.T) { + cmd := findTasksSubcommand(t, name) + resetCmds(t, cmd) + if want.setup != nil { + want.setup(cmd) + } + var posted []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + w.Header().Set("Content-Type", "application/json") + if req.Method == http.MethodPost { + posted = append(posted, req.URL.Path) + t.Errorf("--dry-run sent a real POST to %s", req.URL.Path) + } + // The GET is the policy-step lookup, which a preview may make. + _, _ = w.Write([]byte(`{"taskView":{"task":{"id":"` + actionTestTaskID + + `","state":"TASK_STATE_OPEN","policy":{"current":{"id":"zz-step-1111111111111111111"}}}}}`)) + })) + t.Cleanup(srv.Close) + + stubNewClient(t, srv) + t.Setenv("C1I_URL", "https://example.invalid") + withDryRun(t) + + var out bytes.Buffer + cmd.SetOut(&out) + // Package-level singleton: a left-attached buffer swallows this + // command's output in every later test. + t.Cleanup(func() { cmd.SetOut(nil) }) + cmd.SetContext(context.Background()) + if err := cmd.RunE(cmd, []string{actionTestTaskID}); err != nil { + t.Fatalf("%s --dry-run: %v", name, err) + } + if len(posted) != 0 { + t.Errorf("%s posted during a dry run: %v", name, posted) + } + if !strings.Contains(out.String(), "[dry-run]") { + t.Errorf("%s printed no preview: %q", name, out.String()) + } + if !strings.Contains(out.String(), "/action/"+want.verb) { + t.Errorf("%s previewed the wrong path: %q", name, out.String()) + } + }) + } +} + +// TestTasksUpdateGrantDurationSendsDurationKey. "grantDuration" is the +// plausible wrong name: it is what the response carries. +func TestTasksUpdateGrantDurationSendsDurationKey(t *testing.T) { + cmd := findTasksSubcommand(t, "update-grant-duration") + resetCmds(t, cmd) + _ = cmd.Flags().Set("duration", "3600s") + r := newTaskActionRecorder(t, "TASK_STATE_OPEN", "zz-step-1111111111111111111") + if _, err := runTaskActionCmd(t, cmd, r.srv, actionTestTaskID); err != nil { + t.Fatalf("update-grant-duration: %v", err) + } + if got, ok := r.bodies[0]["duration"]; !ok || got != "3600s" { + t.Errorf(`body["duration"] = %v (present=%v), want "3600s"`, got, ok) + } + if _, ok := r.bodies[0]["grantDuration"]; ok { + t.Error(`body carries "grantDuration"; that is the response field, not the request's`) + } +} + +// TestTaskActionsDryRunStillResolvesTheURL: a bad --url must fail even under +// --dry-run. The other dry-run tests pass a valid URL, so only this sees it. +func TestTaskActionsDryRunStillResolvesTheURL(t *testing.T) { + // Both modes: the runner resolves the URL once, before either path. + for _, name := range []string{"close", "restart"} { + t.Run(name, func(t *testing.T) { + resetRootURLFlag(t) + resetRootDryRunFlag(t) + t.Setenv("C1I_URL", "") + withDryRun(t) + + var out bytes.Buffer + rootCmd.SetOut(&out) + rootCmd.SetErr(&out) + rootCmd.SetArgs([]string{"tasks", name, actionTestTaskID, "--dry-run", "--url", "not a url"}) + err := rootCmd.ExecuteContext(t.Context()) + if err == nil { + t.Fatalf("tasks %s --dry-run accepted a malformed --url; a typo'd tenant previews as though it were real", name) + } + if got := exitCode(err); got != exitUsage { + t.Errorf("exitCode = %d, want %d (exitUsage); err = %v", got, exitUsage, err) + } + if strings.Contains(out.String(), "[dry-run]") { + t.Errorf("tasks %s printed a preview for an unresolvable URL: %q", name, out.String()) + } + }) + } +} + +// withDryRun turns dry-run on and restores the previous value, matching +// withRealDryRun. Hardcoding false would outrank a leaked pflag. +func withDryRun(t *testing.T) { + t.Helper() + orig := viper.GetBool("dry_run") + viper.Set("dry_run", true) + t.Cleanup(func() { viper.Set("dry_run", orig) }) +} + +// resetRootDryRunFlag clears the persistent flag and its Changed bit, so a +// test passing it through rootCmd cannot leak it. +func resetRootDryRunFlag(t *testing.T) { + t.Helper() + f := rootCmd.PersistentFlags().Lookup("dry-run") + if f == nil { + t.Fatal("rootCmd has no --dry-run flag; this reset is not doing what it thinks") + } + orig, changed := f.Value.String(), f.Changed + t.Cleanup(func() { + _ = f.Value.Set(orig) + f.Changed = changed + }) + _ = f.Value.Set("false") + f.Changed = false +} + +// TestStepUnusedActionsPreviewWithoutCredentials: an action needing no step +// previews without authenticating; one needing a step must authenticate. +func TestStepUnusedActionsPreviewWithoutCredentials(t *testing.T) { + for name, want := range taskActionExpectations { + t.Run(name, func(t *testing.T) { + cmd := findTasksSubcommand(t, name) + resetCmds(t, cmd) + if want.setup != nil { + want.setup(cmd) + } + // A client that always fails, standing in for absent credentials. + orig := newClient + newClient = func(_ *cobra.Command, _ string) (*client.Client, error) { + return nil, errNoCredentialsForTest + } + t.Cleanup(func() { newClient = orig }) + + t.Setenv("C1I_URL", "https://example.invalid") + withDryRun(t) + + var out bytes.Buffer + cmd.SetOut(&out) + t.Cleanup(func() { cmd.SetOut(nil) }) + cmd.SetContext(context.Background()) + err := cmd.RunE(cmd, []string{actionTestTaskID}) + + if want.step == stepUnused { + if err != nil { + t.Fatalf("%s --dry-run needs credentials it should not: %v", name, err) + } + if !strings.Contains(out.String(), "[dry-run]") { + t.Errorf("%s printed no preview: %q", name, out.String()) + } + return + } + // Step-using actions must fetch the step, so they authenticate + // first even for a preview — as they did before sharing a runner. + if err == nil { + t.Fatalf("%s --dry-run should have failed without credentials; it resolves a policy step", name) + } + }) + } +} + +// errNoCredentialsForTest stands in for a credential-loading failure. +var errNoCredentialsForTest = errors.New("authentication failed: no credentials found") diff --git a/cmd/tasks_approve.go b/cmd/tasks_approve.go index ccbddb1..619fed9 100644 --- a/cmd/tasks_approve.go +++ b/cmd/tasks_approve.go @@ -3,10 +3,17 @@ package cmd import ( "fmt" - "github.com/ConductorOne/c1i/internal/client" "github.com/spf13/cobra" ) +var tasksApproveAction = taskAction{ + verb: "approve", + step: stepRequired, + confirm: func(id, state, _ string) string { + return fmt.Sprintf("Approved task: task_id=%s state=%s\n", id, state) + }, +} + var tasksApproveCmd = &cobra.Command{ Use: "approve ", Short: "Approve an access request task", @@ -17,50 +24,7 @@ If omitted, the task's currently executing step is fetched and used automatically; if it cannot be determined the command errors and asks you to pass --policy-step-id explicitly (approve requires a step).`, Args: cobra.ExactArgs(1), - RunE: func(cmd *cobra.Command, args []string) error { - baseURL, err := GetBaseURL() - if err != nil { - return err - } - - c, err := newClient(cmd, baseURL) - if err != nil { - return fmt.Errorf("authentication failed: %w", err) - } - - taskID := args[0] - comment, _ := cmd.Flags().GetString("comment") - policyStepID, _ := cmd.Flags().GetString("policy-step-id") - - stepID, err := resolvePolicyStepID(cmd.Context(), c, taskID, policyStepID, true) - if err != nil { - return err - } - - body := map[string]any{ - "policyStepId": stepID, - } - if comment != "" { - body["comment"] = comment - } - - path := client.Path("/api/v1/tasks/%s/action/approve", taskID) - if dryRunActive() { - return printDryRun(cmd, "POST", path, body) - } - data, err := c.Post(cmd.Context(), path, body) - if err != nil { - return fmt.Errorf("API error: %w", err) - } - - id, state, err := parseTaskActionResponse(data) - if err != nil { - return fmt.Errorf("failed to parse response: %w", err) - } - - _, _ = fmt.Fprintf(cmd.OutOrStdout(), "Approved task: task_id=%s state=%s\n", id, state) - return nil - }, + RunE: tasksApproveAction.runTaskAction, } func init() { diff --git a/cmd/tasks_close.go b/cmd/tasks_close.go index 7626a66..e39195a 100644 --- a/cmd/tasks_close.go +++ b/cmd/tasks_close.go @@ -3,10 +3,17 @@ package cmd import ( "fmt" - "github.com/ConductorOne/c1i/internal/client" "github.com/spf13/cobra" ) +var tasksCloseAction = taskAction{ + verb: "close", + step: stepUnused, + confirm: func(id, _, _ string) string { + return fmt.Sprintf("Closed task: task_id=%s\n", id) + }, +} + var tasksCloseCmd = &cobra.Command{ Use: "close ", Short: "Close a task without approving or denying it", @@ -16,45 +23,7 @@ Closing cancels the task and records no approval decision; use approve/deny to record an outcome. The confirmation reports only the task id — the action endpoints echo the task's pre-close state, so printing it would be wrong.`, Args: cobra.ExactArgs(1), - RunE: func(cmd *cobra.Command, args []string) error { - baseURL, err := GetBaseURL() - if err != nil { - return err - } - - taskID := args[0] - comment, _ := cmd.Flags().GetString("comment") - - body := map[string]any{} - if comment != "" { - body["comment"] = comment - } - - path := client.Path("/api/v1/tasks/%s/action/close", taskID) - if dryRunActive() { - return printDryRun(cmd, "POST", path, body) - } - - c, err := newClient(cmd, baseURL) - if err != nil { - return fmt.Errorf("authentication failed: %w", err) - } - data, err := c.Post(cmd.Context(), path, body) - if err != nil { - return fmt.Errorf("API error: %w", err) - } - - // State is deliberately not echoed: the action endpoints return the - // task as it was *before* the action, so a live close prints - // TASK_STATE_OPEN. Parsing still guards the response shape. - id, _, err := parseTaskActionResponse(data) - if err != nil { - return fmt.Errorf("failed to parse response: %w", err) - } - - _, _ = fmt.Fprintf(cmd.OutOrStdout(), "Closed task: task_id=%s\n", id) - return nil - }, + RunE: tasksCloseAction.runTaskAction, } func init() { diff --git a/cmd/tasks_comment.go b/cmd/tasks_comment.go index 8e59207..2906dc8 100644 --- a/cmd/tasks_comment.go +++ b/cmd/tasks_comment.go @@ -3,49 +3,29 @@ package cmd import ( "fmt" - "github.com/ConductorOne/c1i/internal/client" "github.com/spf13/cobra" ) +var tasksCommentAction = taskAction{ + verb: "comment", + step: stepUnused, + // Sent unconditionally: the comment is the payload, so --comment "" must + // reach the server rather than be omitted as an absent option. + extraBody: func(cmd *cobra.Command, body map[string]any) error { + comment, _ := cmd.Flags().GetString("comment") + body["comment"] = comment + return nil + }, + confirm: func(id, _, _ string) string { + return fmt.Sprintf("Comment added: task_id=%s\n", id) + }, +} + var tasksCommentCmd = &cobra.Command{ Use: "comment ", Short: "Add a comment to a task", Args: cobra.ExactArgs(1), - RunE: func(cmd *cobra.Command, args []string) error { - baseURL, err := GetBaseURL() - if err != nil { - return err - } - - taskID := args[0] - comment, _ := cmd.Flags().GetString("comment") - - body := map[string]any{ - "comment": comment, - } - - path := client.Path("/api/v1/tasks/%s/action/comment", taskID) - if dryRunActive() { - return printDryRun(cmd, "POST", path, body) - } - - c, err := newClient(cmd, baseURL) - if err != nil { - return fmt.Errorf("authentication failed: %w", err) - } - data, err := c.Post(cmd.Context(), path, body) - if err != nil { - return fmt.Errorf("API error: %w", err) - } - - id, _, err := parseTaskActionResponse(data) - if err != nil { - return fmt.Errorf("failed to parse response: %w", err) - } - - _, _ = fmt.Fprintf(cmd.OutOrStdout(), "Comment added: task_id=%s\n", id) - return nil - }, + RunE: tasksCommentAction.runTaskAction, } func init() { diff --git a/cmd/tasks_deny.go b/cmd/tasks_deny.go index 0428357..c520c2a 100644 --- a/cmd/tasks_deny.go +++ b/cmd/tasks_deny.go @@ -3,10 +3,19 @@ package cmd import ( "fmt" - "github.com/ConductorOne/c1i/internal/client" "github.com/spf13/cobra" ) +var tasksDenyAction = taskAction{ + verb: "deny", + // Optional, not required: when the current step cannot be determined the + // field is omitted rather than blocking the denial. + step: stepOptional, + confirm: func(id, state, _ string) string { + return fmt.Sprintf("Denied task: task_id=%s state=%s\n", id, state) + }, +} + var tasksDenyCmd = &cobra.Command{ Use: "deny ", Short: "Deny an access request task", @@ -16,53 +25,7 @@ var tasksDenyCmd = &cobra.Command{ currently executing step is used when it can be derived, and simply left off otherwise — deny does not require a step, so it proceeds either way.`, Args: cobra.ExactArgs(1), - RunE: func(cmd *cobra.Command, args []string) error { - baseURL, err := GetBaseURL() - if err != nil { - return err - } - - c, err := newClient(cmd, baseURL) - if err != nil { - return fmt.Errorf("authentication failed: %w", err) - } - - taskID := args[0] - comment, _ := cmd.Flags().GetString("comment") - policyStepID, _ := cmd.Flags().GetString("policy-step-id") - - // policyStepId is optional for deny; include it when we can target a - // specific step (needed on multi-step policies) but don't require one. - stepID, err := resolvePolicyStepID(cmd.Context(), c, taskID, policyStepID, false) - if err != nil { - return err - } - - body := map[string]any{} - if stepID != "" { - body["policyStepId"] = stepID - } - if comment != "" { - body["comment"] = comment - } - - path := client.Path("/api/v1/tasks/%s/action/deny", taskID) - if dryRunActive() { - return printDryRun(cmd, "POST", path, body) - } - data, err := c.Post(cmd.Context(), path, body) - if err != nil { - return fmt.Errorf("API error: %w", err) - } - - id, state, err := parseTaskActionResponse(data) - if err != nil { - return fmt.Errorf("failed to parse response: %w", err) - } - - _, _ = fmt.Fprintf(cmd.OutOrStdout(), "Denied task: task_id=%s state=%s\n", id, state) - return nil - }, + RunE: tasksDenyAction.runTaskAction, } func init() { diff --git a/cmd/tasks_process.go b/cmd/tasks_process.go new file mode 100644 index 0000000..3edf87d --- /dev/null +++ b/cmd/tasks_process.go @@ -0,0 +1,40 @@ +package cmd + +import ( + "fmt" + + "github.com/spf13/cobra" +) + +var tasksProcessAction = taskAction{ + verb: "process", + step: stepUnused, + confirm: func(id, _, _ string) string { + return fmt.Sprintf("Queued task for processing: task_id=%s\n", id) + }, +} + +var tasksProcessCmd = &cobra.Command{ + Use: "process ", + Short: "Process a task now rather than waiting for the next cycle", + Long: `Ask C1 to process a task immediately instead of on its normal schedule. + +Intended for a task that looks stuck: it asks C1 to re-run the policy +evaluation without changing the task's approval state. The request body is empty — this action +takes neither a comment nor a policy step. + +On a healthy task nothing observable changes: state, current policy step and +history are all identical afterwards. The stalled case was not reproduced, so +treat any effect there as unverified. + +The confirmation reports only the task id: the action endpoints echo the task's +state from before the action, and processing is asynchronous, so re-read the +task with "c1i requests get " to see the result.`, + Args: cobra.ExactArgs(1), + RunE: tasksProcessAction.runTaskAction, +} + +func init() { + // No --comment: the ProcessNow request body carries only expandMask. + tasksCmd.AddCommand(tasksProcessCmd) +} diff --git a/cmd/tasks_reassign.go b/cmd/tasks_reassign.go index 51ce5b9..f484ee6 100644 --- a/cmd/tasks_reassign.go +++ b/cmd/tasks_reassign.go @@ -3,10 +3,33 @@ package cmd import ( "fmt" - "github.com/ConductorOne/c1i/internal/client" "github.com/spf13/cobra" ) +var tasksReassignAction = taskAction{ + verb: "reassign", + // The API does not require policyStepId here (approve does). We require a + // resolvable step anyway: a reassign with no step is ambiguous, and failing + // loudly beats sending it. + step: stepRequired, + extraBody: func(cmd *cobra.Command, body map[string]any) error { + // Cobra's required check only proves the flag was set; the accessor is + // what rejects an empty occurrence that would post a blank approver id. + toUserIDs, err := repeatableStringFlag(cmd, "to-user-id") + if err != nil { + return err + } + if len(toUserIDs) == 0 { + return &usageError{fmt.Errorf("flag --to-user-id requires at least one value")} + } + body["newStepUserIds"] = toUserIDs + return nil + }, + confirm: func(id, _, stepID string) string { + return fmt.Sprintf("Reassigned task: task_id=%s policy_step_id=%s\n", id, stepID) + }, +} + var tasksReassignCmd = &cobra.Command{ Use: "reassign ", Short: "Reassign a task's approval step to other users", @@ -22,64 +45,7 @@ the command errors and asks you to pass --policy-step-id explicitly. The confirmation reports the task id and the policy step acted on, never a state: the action endpoints echo the task's state from before the action.`, Args: cobra.ExactArgs(1), - RunE: func(cmd *cobra.Command, args []string) error { - // Cobra's required check only proves the flag was set; the accessor is - // what rejects an empty occurrence that would post a blank approver id. - toUserIDs, err := repeatableStringFlag(cmd, "to-user-id") - if err != nil { - return err - } - if len(toUserIDs) == 0 { - return &usageError{fmt.Errorf("flag --to-user-id requires at least one value")} - } - - baseURL, err := GetBaseURL() - if err != nil { - return err - } - - c, err := newClient(cmd, baseURL) - if err != nil { - return fmt.Errorf("authentication failed: %w", err) - } - - taskID := args[0] - comment, _ := cmd.Flags().GetString("comment") - policyStepID, _ := cmd.Flags().GetString("policy-step-id") - - // The API does not require policyStepId here (approve does). We require a - // resolvable step anyway: a reassign with no step is ambiguous, and failing - // loudly beats sending it. - stepID, err := resolvePolicyStepID(cmd.Context(), c, taskID, policyStepID, true) - if err != nil { - return err - } - - body := map[string]any{ - "newStepUserIds": toUserIDs, - "policyStepId": stepID, - } - if comment != "" { - body["comment"] = comment - } - - path := client.Path("/api/v1/tasks/%s/action/reassign", taskID) - if dryRunActive() { - return printDryRun(cmd, "POST", path, body) - } - data, err := c.Post(cmd.Context(), path, body) - if err != nil { - return fmt.Errorf("API error: %w", err) - } - - id, _, err := parseTaskActionResponse(data) - if err != nil { - return fmt.Errorf("failed to parse response: %w", err) - } - - _, _ = fmt.Fprintf(cmd.OutOrStdout(), "Reassigned task: task_id=%s policy_step_id=%s\n", id, stepID) - return nil - }, + RunE: tasksReassignAction.runTaskAction, } func init() { diff --git a/cmd/tasks_reset.go b/cmd/tasks_reset.go new file mode 100644 index 0000000..1f9c6b9 --- /dev/null +++ b/cmd/tasks_reset.go @@ -0,0 +1,35 @@ +package cmd + +import ( + "fmt" + + "github.com/spf13/cobra" +) + +var tasksResetAction = taskAction{ + verb: "reset", + step: stepUnused, + confirm: func(id, _, _ string) string { + return fmt.Sprintf("Reset task: task_id=%s\n", id) + }, +} + +var tasksResetCmd = &cobra.Command{ + Use: "reset ", + Short: "Hard-reset a task to the start of its policy", + Long: `Hard-reset a task, returning it to the beginning of its policy. + +Unlike "restart", which re-runs the current step, this discards the task's +approval progress and starts the policy over. The endpoint is +/action/reset and takes no policy step. + +The confirmation reports only the task id: the action endpoints echo the task's +state from before the action.`, + Args: cobra.ExactArgs(1), + RunE: tasksResetAction.runTaskAction, +} + +func init() { + tasksResetCmd.Flags().String("comment", "", "Optional comment") + tasksCmd.AddCommand(tasksResetCmd) +} diff --git a/cmd/tasks_restart.go b/cmd/tasks_restart.go new file mode 100644 index 0000000..a65ce60 --- /dev/null +++ b/cmd/tasks_restart.go @@ -0,0 +1,53 @@ +package cmd + +import ( + "fmt" + + "github.com/spf13/cobra" +) + +var tasksRestartAction = taskAction{ + verb: "restart", + step: stepOptional, + confirm: func(id, _, stepID string) string { + // A closed task has no current step. + if stepID == "" { + return fmt.Sprintf("Restarted task: task_id=%s\n", id) + } + return fmt.Sprintf("Restarted task: task_id=%s policy_step_id=%s\n", id, stepID) + }, +} + +var tasksRestartCmd = &cobra.Command{ + Use: "restart ", + Short: "Restart a task's approval step", + Long: `Restart a task's approval step, sending it back for a fresh decision. + +On an open task this rotates the current policy step and records the restart in +the task's policy history. It does NOT reopen a closed task: the API offers +restart on some closed tasks, but the call leaves the state CLOSED and only +appends history, so use it to re-run a live approval, not to undo a close. + +Whether restart is available at all depends on the task; the server refuses +with "action not permitted" otherwise. Check the task's own action list: + c1i api --path /api/v1/tasks/ --fields actions + +restart, reset and skip-step each rotate the current policy step, so a +--policy-step-id captured before one of them goes stale and the server answers: + this action is no longer available: the request has advanced to a new approval step +Omit the flag to act on whatever step is current. + +Because the step is fetched, --dry-run authenticates and issues a read against +the tenant before printing its preview. + +The confirmation reports the task id and the step acted on, never a state: the +action endpoints echo the task's state from before the action.`, + Args: cobra.ExactArgs(1), + RunE: tasksRestartAction.runTaskAction, +} + +func init() { + tasksRestartCmd.Flags().String("comment", "", "Optional comment") + tasksRestartCmd.Flags().String("policy-step-id", "", "Policy step to restart (defaults to the task's current step)") + tasksCmd.AddCommand(tasksRestartCmd) +} diff --git a/cmd/tasks_skip_step.go b/cmd/tasks_skip_step.go new file mode 100644 index 0000000..a02f029 --- /dev/null +++ b/cmd/tasks_skip_step.go @@ -0,0 +1,40 @@ +package cmd + +import ( + "fmt" + + "github.com/spf13/cobra" +) + +var tasksSkipStepAction = taskAction{ + verb: "skip-step", + step: stepRequired, + confirm: func(id, _, stepID string) string { + return fmt.Sprintf("Skipped policy step: task_id=%s policy_step_id=%s\n", id, stepID) + }, +} + +var tasksSkipStepCmd = &cobra.Command{ + Use: "skip-step ", + Short: "Skip a task's current policy step", + Long: `Skip a task's current approval step, advancing the policy without a decision. + +The step id is required by the server, which rejects a missing one with: + invalid TaskActionsServiceSkipStepRequest.PolicyStepId: value does not match regex pattern "^[a-zA-Z0-9]{27}$" +It defaults to the task's currently executing step, so pass --policy-step-id +only to target a different one. Because that step is fetched, --dry-run +authenticates and issues a read against the tenant before previewing. A step +id captured before another action is stale, and the server answers +"this action is no longer available". + +The confirmation reports the task id and the step skipped, never a state: the +action endpoints echo the task's state from before the action.`, + Args: cobra.ExactArgs(1), + RunE: tasksSkipStepAction.runTaskAction, +} + +func init() { + tasksSkipStepCmd.Flags().String("comment", "", "Optional comment") + tasksSkipStepCmd.Flags().String("policy-step-id", "", "Policy step to skip (defaults to the task's current step)") + tasksCmd.AddCommand(tasksSkipStepCmd) +} diff --git a/cmd/tasks_update_grant_duration.go b/cmd/tasks_update_grant_duration.go new file mode 100644 index 0000000..8ea6ee9 --- /dev/null +++ b/cmd/tasks_update_grant_duration.go @@ -0,0 +1,54 @@ +package cmd + +import ( + "fmt" + + "github.com/spf13/cobra" +) + +var tasksUpdateGrantDurationAction = taskAction{ + verb: "update-grant-duration", + step: stepUnused, + extraBody: func(cmd *cobra.Command, body map[string]any) error { + duration, err := requireNonEmptyIfSet(cmd, "duration") + if err != nil { + return err + } + body["duration"] = duration + return nil + }, + confirm: func(id, _, _ string) string { + return fmt.Sprintf("Updated grant duration: task_id=%s\n", id) + }, +} + +var tasksUpdateGrantDurationCmd = &cobra.Command{ + Use: "update-grant-duration ", + Short: "Change the grant duration a task will provision", + Long: `Change how long the access a grant task provisions will last. + +--duration 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" + +The task must still be at an approval step. Once it reaches provisioning the +server refuses with: + cannot update grant duration for a ticket in a provision step + +The new value lands on the task as "grantDuration"; read it back with +"c1i requests get ". + +The confirmation reports only the task id: the action endpoints echo the task's +state from before the action.`, + Args: cobra.ExactArgs(1), + RunE: tasksUpdateGrantDurationAction.runTaskAction, +} + +func init() { + // No --comment: the UpdateGrantDuration request body carries only duration + // and expandMask. + tasksUpdateGrantDurationCmd.Flags().String("duration", "", + `Grant duration as a protobuf duration, e.g. 3600s; "1h" is refused`) + markRequired(tasksUpdateGrantDurationCmd, "duration") + tasksCmd.AddCommand(tasksUpdateGrantDurationCmd) +} diff --git a/cmd/usage_exit_codes_test.go b/cmd/usage_exit_codes_test.go index f852b0b..bdae882 100644 --- a/cmd/usage_exit_codes_test.go +++ b/cmd/usage_exit_codes_test.go @@ -90,6 +90,15 @@ func TestValidationGuardsExitUsage(t *testing.T) { wantMsg: "--classification requires a non-empty value", cmds: []*cobra.Command{mcpToolsSearchCmd}, }, + { + // markRequired is the only thing stopping {"duration": ""} on the + // wire; runTaskActionCmd calls RunE directly and never sees cobra's + // required check, so this row is what pins it. + name: "tasks update-grant-duration: --duration missing", + args: []string{"tasks", "update-grant-duration", "zz-task-1"}, + wantMsg: `required flag(s) "duration" not set`, + cmds: []*cobra.Command{tasksUpdateGrantDurationCmd}, + }, { // The only repeatable flag whose READ was unpinned end to end: the // existing --config-field row passes a non-empty bad pair, which