diff --git a/CHANGELOG.md b/CHANGELOG.md index 0983ff5b..46b57a80 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,8 @@ # Unreleased +* feat: Add safety controls for `--set-controller` and `--remove-controller` + * Warn and prompt for confirmation when removing yourself from controllers + * Add `-f/--force` flag to skip confirmation prompts * feat: Show `name` in `canister status` command * feat: `icp canister metadata ` now fetches metadata sections from specified canisters * fix: Validate explicit canister paths and throw an error if `canister.yaml` is not found diff --git a/crates/icp-cli/src/commands/canister/settings/update.rs b/crates/icp-cli/src/commands/canister/settings/update.rs index 05a43d76..ebe1da3b 100644 --- a/crates/icp-cli/src/commands/canister/settings/update.rs +++ b/crates/icp-cli/src/commands/canister/settings/update.rs @@ -2,6 +2,8 @@ use anyhow::bail; use byte_unit::{Byte, Unit}; use clap::{ArgAction, Args}; use console::Term; +use dialoguer::Confirm; +use ic_agent::Identity; use ic_agent::export::Principal; use ic_management_canister_types::{CanisterStatusResult, EnvironmentVariable, LogVisibility}; use icp::ProjectLoadError; @@ -13,12 +15,20 @@ use crate::commands::args; #[derive(Clone, Debug, Default, Args)] pub(crate) struct ControllerOpt { + /// Add one or more principals to the canister's controller list. #[arg(long, action = ArgAction::Append, conflicts_with("set_controller"))] add_controller: Option>, + /// Remove one or more principals from the canister's controller list. + /// + /// Warning: Removing yourself will cause you to lose control of the canister. #[arg(long, action = ArgAction::Append, conflicts_with("set_controller"))] remove_controller: Option>, + /// Replace the canister's controller list with the specified principals. + /// + /// Warning: This removes all existing controllers not in the new list. + /// If you don't include yourself, you will lose control of the canister. #[arg(long, action = ArgAction::Append)] set_controller: Option>, } @@ -76,6 +86,10 @@ pub(crate) struct UpdateArgs { #[command(flatten)] pub(crate) cmd_args: args::CanisterCommandArgs, + /// Force the operation without confirmation prompts + #[arg(short = 'f', long)] + force: bool, + #[command(flatten)] controllers: Option, @@ -106,6 +120,11 @@ pub(crate) struct UpdateArgs { pub(crate) async fn exec(ctx: &Context, args: &UpdateArgs) -> Result<(), anyhow::Error> { let selections = args.cmd_args.selections(); + let identity = ctx.get_identity(&selections.identity).await?; + let caller_principal = identity + .sender() + .map_err(|e| anyhow::anyhow!("failed to get caller principal: {e}"))?; + let agent = ctx .get_agent( &selections.identity, @@ -139,14 +158,34 @@ pub(crate) async fn exec(ctx: &Context, args: &UpdateArgs) -> Result<(), anyhow: current_status = Some(mgmt.canister_status(&cid).await?.0); } - // TODO(VZ): Ask for consent - // - if the freezing threshold is too long or too short. - // - if trying to remove the caller itself from the controllers. + // TODO(VZ): Ask for consent if the freezing threshold is too long or too short. // Handle controllers. let mut controllers: Option> = None; if let Some(controllers_opt) = &args.controllers { controllers = get_controllers(controllers_opt, current_status.as_ref()); + + // Check if the caller is being removed from controllers + if let Some(new_controllers) = &controllers + && !new_controllers.contains(&caller_principal) + && !args.force + { + ctx.term.write_line( + "Warning: You are about to remove yourself from the controllers list.", + )?; + ctx.term.write_line( + "This will cause you to lose control of the canister and cannot be undone.", + )?; + + let confirmed = Confirm::new() + .with_prompt("Do you want to proceed?") + .default(false) + .interact()?; + + if !confirmed { + bail!("Operation cancelled by user"); + } + } } // Handle log visibility. diff --git a/crates/icp-cli/tests/canister_settings_tests.rs b/crates/icp-cli/tests/canister_settings_tests.rs index 9260c882..7e79c506 100644 --- a/crates/icp-cli/tests/canister_settings_tests.rs +++ b/crates/icp-cli/tests/canister_settings_tests.rs @@ -271,7 +271,7 @@ async fn canister_settings_update_controllers() { .and(contains(principal_bob.as_str()).not()), ); - // Set multiple controllers + // Set multiple controllers (uses --force since we're removing ourselves as controller) ctx.icp() .current_dir(&project_dir) .args([ @@ -281,6 +281,7 @@ async fn canister_settings_update_controllers() { "my-canister", "--environment", "random-environment", + "--force", "--set-controller", principal_alice.as_str(), "--set-controller", diff --git a/docs/reference/cli.md b/docs/reference/cli.md index 49a4721b..ef5997df 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -290,9 +290,14 @@ Change a canister's settings to specified values * `-n`, `--network ` — Name of the network to target, conflicts with environment argument * `-e`, `--environment ` — Override the environment to connect to. By default, the local environment is used * `--identity ` — The user identity to run this command as -* `--add-controller ` -* `--remove-controller ` -* `--set-controller ` +* `-f`, `--force` — Force the operation without confirmation prompts +* `--add-controller ` — Add one or more principals to the canister's controller list +* `--remove-controller ` — Remove one or more principals from the canister's controller list. + + Warning: Removing yourself will cause you to lose control of the canister. +* `--set-controller ` — Replace the canister's controller list with the specified principals. + + Warning: This removes all existing controllers not in the new list. If you don't include yourself, you will lose control of the canister. * `--compute-allocation ` * `--memory-allocation ` * `--freezing-threshold `