feat: controllers cannot be anonymous (#306)

* feat: disallow controller being set with anonymous principal Signed-off-by: David Dal Busco <[email protected]> * feat: do not allow set anonnymous for controllers Signed-off-by: David Dal Busco <[email protected]> --------- Signed-off-by: David Dal Busco <[email protected]>
junobuild · Nov 14, 2023 · 832c84a · 832c84a
1 parent cb343aa
commit 832c84a
Show file tree

Hide file tree

Showing 6 changed files with 45 additions and 22 deletions.
diff --git 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
@@ -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
@@ -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/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,18 +31,14 @@ 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),
     }
 }
 
 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
@@ -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_anonymous, 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<ControllerId> {
@@ -100,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
@@ -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
 }