Skip to content

Add opt-in confirmations for task mutations #155

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
leet-c1 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add opt-in confirmations for task mutations #155

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,13 @@ $ docker pull public.ecr.aws/conductorone/cone:<version>

To authenticate to Cone, run `cone login <tenant-name or tenant-url>`, passing in the name (such as `example.conductor.one`) or URL (such as `https://example.conductor.one`) of your ConductorOne instance and follow the prompts.

# Confirming task mutations

Add `--confirm` to `task approve`, `task deny`, `task comment`, or `task
escalate` to require an explicit interactive confirmation after Cone has
resolved and validated the task, immediately before it sends the change. It is
rejected with `--non-interactive`; the default scripting behavior is unchanged.

# Getting started with Cone

Run `cone help` to see the full list of available Cone commands.
Expand Down
46 changes: 46 additions & 0 deletions cmd/cone/confirmation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
package main

import (
"fmt"

"github.com/pterm/pterm"
"github.com/spf13/cobra"
)

const (
confirmFlag = "confirm"
confirmAnnotation = "cone.conductorone.com/confirm"
)

func supportsConfirmation(cmd *cobra.Command) *cobra.Command {
if cmd.Annotations == nil {
cmd.Annotations = make(map[string]string)
}
cmd.Annotations[confirmAnnotation] = "true"
return cmd
}

func confirmationSupported(cmd *cobra.Command) bool {
return cmd.Annotations[confirmAnnotation] == "true"
}

func confirmMutation(cmd *cobra.Command, action string) error {
enabled, _ := cmd.Flags().GetBool(confirmFlag)
if !enabled {
return nil
}

nonInteractive, _ := cmd.Flags().GetBool(nonInteractiveFlag)
Comment on lines +28 to +33

@github-actions github-actions Bot Sep 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Bug: Both flags are read straight off the pflag set, bypassing viper. Everywhere else in this repo non-interactive is read as v.GetBool(nonInteractiveFlag) (get_drop_task.go:136,190,449, form_fields.go:37), and getSubViperForProfile binds cmd.Flags() so config-profile and CONE_* env values resolve. Two consequences: CONE_CONFIRM=true / confirm: true in a profile silently does nothing (the safety control fails open, no prompt), and CONE_NON_INTERACTIVE=true combined with --confirm skips the guard and falls through to pterm, which opens /dev/tty directly — so it blocks on a terminal prompt instead of returning the intended error.

Suggest threading the *viper.Viper that all four call sites already have from cmdContext into this helper and using v.GetBool(...) for both reads. Confidence: high.

if nonInteractive {
return fmt.Errorf("--confirm requires interactive mode")
}

confirmed, err := pterm.DefaultInteractiveConfirm.Show("Proceed with " + action + "?")
if err != nil {
return err
}
if !confirmed {
return fmt.Errorf("mutation cancelled")
}
return nil
}
28 changes: 28 additions & 0 deletions cmd/cone/confirmation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package main

import (
"strings"
"testing"

"github.com/spf13/cobra"
)

func TestConfirmMutationRequiresInteractiveMode(t *testing.T) {
cmd := &cobra.Command{}
cmd.Flags().Bool(confirmFlag, false, "")
cmd.Flags().Bool(nonInteractiveFlag, false, "")
if err := cmd.ParseFlags([]string{"--confirm", "--non-interactive"}); err != nil {
t.Fatalf("ParseFlags: %v", err)
}

err := confirmMutation(cmd, "creating an access request")
if err == nil || !strings.Contains(err.Error(), "requires interactive mode") {
t.Fatalf("confirmMutation error = %v, want interactive-mode error", err)
}
}

func TestConfirmMutationIsOptIn(t *testing.T) {
if err := confirmMutation(&cobra.Command{}, "creating an access request"); err != nil {
t.Fatalf("confirmMutation without --confirm: %v", err)
}
}
Comment on lines +24 to +28

@github-actions github-actions Bot Sep 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: This test passes vacuously. &cobra.Command{} has no confirm flag registered, so GetBool returns an error that confirmMutation discards and enabled is false regardless of the opt-in logic — the test would still pass if the default flipped to opt-out. Register confirmFlag (defaulting false) on the command so the assertion exercises the real path. A case covering rejection (confirmed == false → mutation cancelled) and one for confirmationSupported on an un-annotated command would also be worth adding. Confidence: high.

8 changes: 8 additions & 0 deletions cmd/cone/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,14 @@ func runCli(ctx context.Context) int {
Use: "cone",
Short: "Cone interacts with the ConductorOne API to manage access to entitlements.",
Version: version,
RunE: func(cmd *cobra.Command, _ []string) error {
return cmd.Help()
},
PersistentPreRunE: func(cmd *cobra.Command, args []string) error {
confirm, _ := cmd.Flags().GetBool(confirmFlag)
if confirm && !confirmationSupported(cmd) {
return fmt.Errorf("--confirm is not supported by %q", cmd.CommandPath())
}
cmd.SetContext(ctx)
return nil
},
Expand All @@ -58,6 +65,7 @@ func runCli(ctx context.Context) int {
cliCmd.PersistentFlags().StringP("output", "o", "table", "Output format. Valid values: table, json, json-pretty, wide.")
cliCmd.PersistentFlags().Bool("debug", false, "Enable HTTP debug logging")
cliCmd.PersistentFlags().String("log-level", "", "Set log level (debug, info, warn, error)")
cliCmd.PersistentFlags().Bool(confirmFlag, false, "Prompt before supported task mutations")

@github-actions github-actions Bot Sep 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: --confirm is registered as a root persistent flag, so it appears in the help output of every command (cone login --confirm, cone get --confirm, …) while the PersistentPreRunE guard rejects all but the four annotated task commands. The guard also only fires for runnable commands: cone task --confirm returns flag.ErrHelp before PersistentPreRunE runs, so it silently prints help instead of erroring. Registering the flag on the four supported commands directly would make the surface self-describing and remove the need for the annotation plumbing. Confidence: high.


err := initConfig(cliCmd)
if err != nil {
Expand Down
12 changes: 8 additions & 4 deletions cmd/cone/task_approve_deny.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ func approveTasksCmd() *cobra.Command {

addCommentFlag(cmd)
addWaitFlag(cmd)
return cmd
return supportsConfirmation(cmd)
}

func denyTasksCmd() *cobra.Command {
Expand All @@ -32,11 +32,11 @@ func denyTasksCmd() *cobra.Command {

addCommentFlag(cmd)
addWaitFlag(cmd)
return cmd
return supportsConfirmation(cmd)
}

func runApproveTasks(cmd *cobra.Command, args []string) error {
return runApproveDeny(cmd, args, func(c client.C1Client, ctx context.Context, taskId string, comment string, policyId string) (*shared.Task, error) {
return runApproveDeny(cmd, args, "approving", func(c client.C1Client, ctx context.Context, taskId string, comment string, policyId string) (*shared.Task, error) {
approveResp, err := c.ApproveTask(ctx, taskId, comment, policyId)
if err != nil {
return nil, err
Expand All @@ -46,7 +46,7 @@ func runApproveTasks(cmd *cobra.Command, args []string) error {
}

func runDenyTasks(cmd *cobra.Command, args []string) error {
return runApproveDeny(cmd, args, func(c client.C1Client, ctx context.Context, taskId string, comment string, policyId string) (*shared.Task, error) {
return runApproveDeny(cmd, args, "denying", func(c client.C1Client, ctx context.Context, taskId string, comment string, policyId string) (*shared.Task, error) {
approveResp, err := c.DenyTask(ctx, taskId, comment, policyId)
if err != nil {
return nil, err
Expand All @@ -58,6 +58,7 @@ func runDenyTasks(cmd *cobra.Command, args []string) error {
func runApproveDeny(
cmd *cobra.Command,
args []string,
action string,
run func(c client.C1Client, ctx context.Context, taskId string, comment string, policyId string) (*shared.Task, error),
) error {
ctx, c, v, err := cmdContext(cmd)
Expand All @@ -80,6 +81,9 @@ func runApproveDeny(
if taskResp.TaskView.Task.Policy == nil || taskResp.TaskView.Task.Policy.Current == nil {
return errors.New("task does not have a current policy step id and cannot be approved or denied")
}
if err := confirmMutation(cmd, action+" task "+taskId); err != nil {
return err
}

task, err := run(c, ctx, taskId, comment, client.StringFromPtr(taskResp.TaskView.Task.Policy.Current.ID))
if err != nil {
Expand Down
12 changes: 9 additions & 3 deletions cmd/cone/task_comment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ func tasksCommentCmd() *cobra.Command {
RunE: tasksCommentRun,
}

return cmd
return supportsConfirmation(cmd)
}

func tasksCommentRun(cmd *cobra.Command, args []string) error {
Expand All @@ -29,10 +29,16 @@ func tasksCommentRun(cmd *cobra.Command, args []string) error {
return err
}

taskId := args[0]
taskID := args[0]
comment := args[1]
if _, err := c.GetTask(ctx, taskID); err != nil {
return err
}
Comment on lines +34 to +36

@github-actions github-actions Bot Sep 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: This GetTask runs unconditionally, so every cone task comment invocation now pays an extra API round-trip and gains a new failure mode even when --confirm is not passed — contrary to the PR's "default scripting behavior is unchanged". Approve/deny already needed the task for its policy ID, but here the result is discarded. Consider gating it on the confirm flag, and since you are fetching it anyway, using the display name in the prompt text so the resolved task is actually visible to the user. Same pattern at cmd/cone/task_escalate.go:32. Confidence: high on the behavior change, medium on user impact.

if err := confirmMutation(cmd, "commenting on task "+taskID); err != nil {
return err
}

userResp, err := c.CommentOnTask(ctx, taskId, comment)
userResp, err := c.CommentOnTask(ctx, taskID, comment)
if err != nil {
return err
}
Expand Down
12 changes: 9 additions & 3 deletions cmd/cone/task_escalate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ func escalateTasksCmd() *cobra.Command {
Short: "Escalate an access request task to emergency access",
RunE: runEscalateTasks,
}
return cmd
return supportsConfirmation(cmd)
}

func runEscalateTasks(cmd *cobra.Command, args []string) error {
Expand All @@ -28,9 +28,15 @@ func runEscalateTasks(cmd *cobra.Command, args []string) error {
return err
}

taskId := args[0]
taskID := args[0]
if _, err := c.GetTask(ctx, taskID); err != nil {
return err
}
if err := confirmMutation(cmd, "escalating task "+taskID+" to emergency access"); err != nil {
return err
}

userResp, err := c.EscalateTask(ctx, taskId)
userResp, err := c.EscalateTask(ctx, taskID)
if err != nil {
return err
}
Expand Down
Loading