From 85315741c43fa31eafeac77872ad8c12d5cc2b07 Mon Sep 17 00:00:00 2001 From: David Dal Busco Date: Tue, 14 Nov 2023 20:57:49 +0100 Subject: [PATCH 1/2] feat: disallow controller being set with anonymous principal Signed-off-by: David Dal Busco --- src/satellite/src/rules/assert_stores.rs | 12 +++--------- src/shared/src/controllers.rs | 22 ++++++++++++---------- 2 files changed, 15 insertions(+), 19 deletions(-) diff --git a/src/satellite/src/rules/assert_stores.rs b/src/satellite/src/rules/assert_stores.rs index f942b30c2..66f78d790 100644 --- a/src/satellite/src/rules/assert_stores.rs +++ b/src/satellite/src/rules/assert_stores.rs @@ -15,10 +15,8 @@ pub fn assert_permission( match permission { Permission::Public => true, Permission::Private => assert_caller(caller, owner), - Permission::Managed => { - assert_caller(caller, owner) || assert_controller(caller, controllers) - } - Permission::Controllers => assert_controller(caller, controllers), + Permission::Managed => assert_caller(caller, owner) || is_controller(caller, controllers), + Permission::Controllers => is_controller(caller, controllers), } } @@ -33,7 +31,7 @@ pub fn assert_create_permission( Permission::Public => true, Permission::Private => assert_not_anonymous(caller), Permission::Managed => assert_not_anonymous(caller), - Permission::Controllers => assert_controller(caller, controllers), + Permission::Controllers => is_controller(caller, controllers), } } @@ -41,10 +39,6 @@ fn assert_caller(caller: Principal, owner: Principal) -> bool { assert_not_anonymous(caller) && principal_equal(owner, caller) } -fn assert_controller(caller: Principal, controllers: &Controllers) -> bool { - assert_not_anonymous(caller) && is_controller(caller, controllers) -} - fn assert_not_anonymous(caller: Principal) -> bool { principal_not_anonymous(caller) } diff --git a/src/shared/src/controllers.rs b/src/shared/src/controllers.rs index e54782328..8e6c187cd 100644 --- a/src/shared/src/controllers.rs +++ b/src/shared/src/controllers.rs @@ -1,7 +1,7 @@ use crate::env::{CONSOLE, OBSERVATORY}; use crate::types::interface::SetController; use crate::types::state::{Controller, ControllerId, ControllerScope, Controllers, UserId}; -use crate::utils::principal_equal; +use crate::utils::{principal_equal, principal_not_anonymous}; use candid::Principal; use ic_cdk::api::time; use std::collections::HashMap; @@ -56,18 +56,20 @@ pub fn delete_controllers(remove_controllers: &[UserId], controllers: &mut Contr } pub fn is_controller(caller: UserId, controllers: &Controllers) -> bool { - controllers - .iter() - .any(|(&controller_id, _)| principal_equal(controller_id, caller)) + principal_not_anonymous(caller) + && controllers + .iter() + .any(|(&controller_id, _)| principal_equal(controller_id, caller)) } pub fn is_admin_controller(caller: UserId, controllers: &Controllers) -> bool { - controllers - .iter() - .any(|(&controller_id, controller)| match controller.scope { - ControllerScope::Write => false, - ControllerScope::Admin => principal_equal(controller_id, caller), - }) + principal_not_anonymous(caller) + && controllers + .iter() + .any(|(&controller_id, controller)| match controller.scope { + ControllerScope::Write => false, + ControllerScope::Admin => principal_equal(controller_id, caller), + }) } pub fn into_controller_ids(controllers: &Controllers) -> Vec { From 03d41eb61f79f3083aae80e2cd2aa75387e9f99b Mon Sep 17 00:00:00 2001 From: David Dal Busco Date: Tue, 14 Nov 2023 21:09:17 +0100 Subject: [PATCH 2/2] feat: do not allow set anonnymous for controllers Signed-off-by: David Dal Busco --- .../src/controllers/mission_control.rs | 6 +++++- src/orbiter/src/lib.rs | 6 +++++- src/satellite/src/lib.rs | 6 +++++- src/shared/src/controllers.rs | 13 ++++++++++++- src/shared/src/utils.rs | 4 ++++ 5 files changed, 31 insertions(+), 4 deletions(-) diff --git a/src/mission_control/src/controllers/mission_control.rs b/src/mission_control/src/controllers/mission_control.rs index 7123b5036..eed113810 100644 --- a/src/mission_control/src/controllers/mission_control.rs +++ b/src/mission_control/src/controllers/mission_control.rs @@ -2,7 +2,9 @@ use crate::controllers::store::{delete_controllers, get_admin_controllers, set_c use crate::store::get_user; use ic_cdk::id; use shared::constants::MAX_NUMBER_OF_MISSION_CONTROL_CONTROLLERS; -use shared::controllers::{assert_max_number_of_controllers, into_controller_ids}; +use shared::controllers::{ + assert_max_number_of_controllers, assert_no_anonymous_controller, into_controller_ids, +}; use shared::ic::update_canister_controllers; use shared::types::interface::SetController; use shared::types::state::{ControllerId, ControllerScope, Controllers}; @@ -22,6 +24,8 @@ pub async fn set_mission_control_controllers( } } + assert_no_anonymous_controller(controllers)?; + set_controllers(controllers, controller); // We update the IC controllers because it is possible that an existing controller was updated. diff --git a/src/orbiter/src/lib.rs b/src/orbiter/src/lib.rs index 889d6bb67..80ef22db1 100644 --- a/src/orbiter/src/lib.rs +++ b/src/orbiter/src/lib.rs @@ -41,7 +41,9 @@ use ic_stable_structures::writer::Writer; #[allow(unused)] use ic_stable_structures::Memory as _; use shared::constants::MAX_NUMBER_OF_SATELLITE_CONTROLLERS; -use shared::controllers::{assert_max_number_of_controllers, init_controllers}; +use shared::controllers::{ + assert_max_number_of_controllers, assert_no_anonymous_controller, init_controllers, +}; use shared::types::interface::{DeleteControllersArgs, SegmentArgs, SetControllersArgs}; use shared::types::state::{ControllerScope, Controllers, SatelliteId}; use std::mem; @@ -219,6 +221,8 @@ fn set_controllers( } } + assert_no_anonymous_controller(&controllers).unwrap_or_else(|e| trap(&e)); + set_controllers_store(&controllers, &controller); get_controllers() } diff --git a/src/satellite/src/lib.rs b/src/satellite/src/lib.rs index a319b8272..ef7dc8605 100644 --- a/src/satellite/src/lib.rs +++ b/src/satellite/src/lib.rs @@ -58,7 +58,9 @@ use ic_stable_structures::writer::Writer; #[allow(unused)] use ic_stable_structures::Memory as _; use shared::constants::MAX_NUMBER_OF_SATELLITE_CONTROLLERS; -use shared::controllers::{assert_max_number_of_controllers, init_controllers}; +use shared::controllers::{ + assert_max_number_of_controllers, assert_no_anonymous_controller, init_controllers, +}; use shared::types::interface::{DeleteControllersArgs, SegmentArgs, SetControllersArgs}; use shared::types::state::{ControllerScope, Controllers}; use std::mem; @@ -233,6 +235,8 @@ fn set_controllers( } } + assert_no_anonymous_controller(&controllers).unwrap_or_else(|e| trap(&e)); + set_controllers_store(&controllers, &controller); get_controllers() } diff --git a/src/shared/src/controllers.rs b/src/shared/src/controllers.rs index 8e6c187cd..212dd1868 100644 --- a/src/shared/src/controllers.rs +++ b/src/shared/src/controllers.rs @@ -1,7 +1,7 @@ use crate::env::{CONSOLE, OBSERVATORY}; use crate::types::interface::SetController; use crate::types::state::{Controller, ControllerId, ControllerScope, Controllers, UserId}; -use crate::utils::{principal_equal, principal_not_anonymous}; +use crate::utils::{principal_anonymous, principal_equal, principal_not_anonymous}; use candid::Principal; use ic_cdk::api::time; use std::collections::HashMap; @@ -102,6 +102,17 @@ pub fn assert_max_number_of_controllers( Ok(()) } +pub fn assert_no_anonymous_controller(controllers_ids: &[ControllerId]) -> Result<(), String> { + let has_anonymous = controllers_ids + .iter() + .any(|controller_id| principal_anonymous(controller_id.clone())); + + match has_anonymous { + true => Err("Anonymous controller not allowed.".to_string()), + false => Ok(()), + } +} + pub fn caller_is_console(caller: UserId) -> bool { let console = Principal::from_text(CONSOLE).unwrap(); diff --git a/src/shared/src/utils.rs b/src/shared/src/utils.rs index 21379356d..816b49d71 100644 --- a/src/shared/src/utils.rs +++ b/src/shared/src/utils.rs @@ -13,6 +13,10 @@ pub fn principal_not_anonymous(p: Principal) -> bool { principal_not_equal(p, Principal::anonymous()) } +pub fn principal_anonymous(p: Principal) -> bool { + principal_equal(p, Principal::anonymous()) +} + pub fn account_identifier_equal(x: AccountIdentifier, y: AccountIdentifier) -> bool { x == y }