feat(access-manager): special case internal calls (#5222)

Author: benluelo

cosmwasm/access-manager/src/contract.rs

@@ -344,7 +344,7 @@ pub(crate) fn set_target_function_role<'a>(
     only_authorized(ctx)?;
 
     for selector in selectors {
-        _set_target_function_role(ctx, target, selector, role_id);
+        _set_target_function_role(ctx, target, selector, role_id)?;
     }
 
     Ok(())
@@ -358,14 +358,23 @@ fn _set_target_function_role(
     target: &Addr,
     selector: &Selector,
     role_id: RoleId,
-) {
+) -> Result<(), ContractError> {
+    if selector.is_internal() {
+        return Err(ContractError::AccessManager(
+            AccessManagerError::InternalSelector(selector.to_owned()),
+        ));
+    }
+
     ctx.storage()
         .write::<TargetAllowedRoles>(&(target.clone(), selector.to_owned()), &role_id);
+
     ctx.emit(TargetFunctionRoleUpdated {
         target,
         selector,
         role_id,
     });
+
+    Ok(())
 }
 
 /// See [`ExecuteMsg::SetTargetAdminDelay`].
@@ -986,7 +995,12 @@ pub(crate) fn can_call(
     target: &Addr,
     selector: &Selector,
 ) -> Result<CanCall, ContractError> {
-    if is_target_closed(ctx, target)? {
+    if selector.is_internal() {
+        Ok(CanCall {
+            allowed: true,
+            delay: 0,
+        })
+    } else if is_target_closed(ctx, target)? {
         Ok(CanCall {
             allowed: false,
             delay: 0,
@@ -1046,6 +1060,12 @@ pub(crate) fn get_target_function_role(
     target: &Addr,
     selector: &Selector,
 ) -> Result<RoleId, ContractError> {
+    if selector.is_internal() {
+        return Err(ContractError::AccessManager(
+            AccessManagerError::InternalSelector(selector.to_owned()),
+        ));
+    }
+
     Ok(ctx
         .storage()
         .maybe_read::<TargetAllowedRoles>(&(target.clone(), selector.to_owned()))?

cosmwasm/access-manager/src/tests.rs

@@ -15,7 +15,7 @@ use unionlabs_primitives::H256;
 
 use crate::{
     error::ContractError,
-    execute, min_setback,
+    execute, min_setback, query,
     tests::utils::{assert_query_result, setup, ACCOUNT_1, ACCOUNT_2, ADMIN, TARGET_1, TARGET_2},
 };
 
@@ -1503,3 +1503,54 @@ fn schedule_reentrant_works() {
         }),
     );
 }
+
+#[test]
+fn target_role_internal_selector_fails() {
+    let (mut deps, env) = setup();
+
+    assert_eq!(
+        execute(
+            deps.as_mut(),
+            env.clone(),
+            message_info(&ADMIN, &[]).clone(),
+            ExecuteMsg::SetTargetFunctionRole {
+                selectors: vec![Selector::new("$$internal_method").to_owned()],
+                role_id: RoleId::new(1),
+                target: TARGET_1.clone(),
+            },
+        )
+        .unwrap_err(),
+        ContractError::AccessManager(AccessManagerError::InternalSelector(
+            Selector::new("$$internal_method").to_owned()
+        ))
+    );
+
+    assert_eq!(
+        query(
+            deps.as_ref(),
+            env.clone(),
+            QueryMsg::GetTargetFunctionRole {
+                target: TARGET_1.clone(),
+                selector: Selector::new("$$also_internal").to_owned(),
+            },
+        )
+        .unwrap_err(),
+        ContractError::AccessManager(AccessManagerError::InternalSelector(
+            Selector::new("$$also_internal").to_owned()
+        ))
+    );
+
+    assert_query_result(
+        deps.as_ref(),
+        &env,
+        QueryMsg::CanCall {
+            selector: Selector::new("$$internal_yet_again").to_owned(),
+            target: TARGET_1.clone(),
+            caller: ACCOUNT_1.clone(),
+        },
+        &CanCall {
+            allowed: true,
+            delay: 0,
+        },
+    );
+}

cosmwasm/access-manager/src/tests/utils.rs

@@ -14,9 +14,10 @@ use cosmwasm_std::{
     Addr, Deps, Env, OwnedDeps, Response,
 };
 use depolama::StorageExt;
+use frissitheto::UpgradeMsg;
 use serde::de::DeserializeOwned;
 
-use crate::{init, query, state::RoleMembers};
+use crate::{migrate, query, state::RoleMembers};
 
 pub static ADMIN: LazyLock<Addr> = LazyLock::new(|| Addr::unchecked("admin"));
 
@@ -30,12 +31,12 @@ pub fn setup() -> (OwnedDeps<MockStorage, MockApi, MockQuerier>, Env) {
     let mut deps = mock_dependencies();
     let env = mock_env();
 
-    let res = init(
+    let res = migrate(
         deps.as_mut(),
-        &env,
-        &InitMsg {
+        env.clone(),
+        UpgradeMsg::Init(InitMsg {
             initial_admin: ADMIN.clone(),
-        },
+        }),
     )
     .unwrap();
 

e2e/access-managed-example/src/msg.rs

@@ -14,6 +14,7 @@ pub enum ExecuteMsg {
         by: u32,
         in_sub_msg: bool,
     },
+    #[serde(rename = "$$decrement_in_sub_msg")]
     DecrementInSubMsg {
         by: u32,
     },

e2e/access-manager-tests/src/main.rs

@@ -251,7 +251,6 @@ async fn main() -> Result<()> {
     info!("set INCREMENT role guardian");
     info!("set DECREMENT role guardian");
     info!("set INCREMENT_IN_REPLY role guardian");
-    info!("set DECREMENT_IN_SUB_MSG role guardian");
     info!("grant INCREMENT to bob with an execution delay");
 
     execute(
@@ -273,12 +272,6 @@ async fn main() -> Result<()> {
                 selectors: vec![Selector::new("increment_in_reply").to_owned()],
                 role_id: INCREMENT_IN_REPLY,
             },
-            // reentrant call, access is checked by the contract itself
-            &manager::msg::ExecuteMsg::SetTargetFunctionRole {
-                target: Addr::unchecked(managed.to_string()),
-                selectors: vec![Selector::new("decrement_in_sub_msg").to_owned()],
-                role_id: RoleId::PUBLIC_ROLE,
-            },
             &manager::msg::ExecuteMsg::SetRoleGuardian {
                 role_id: INCREMENT,
                 guardian: INCREMENT_GUARDIAN,

lib/access-manager-types/src/lib.rs

@@ -199,6 +199,8 @@ impl<Context> bincode::Decode<Context> for Box<Selector> {
 }
 
 impl Selector {
+    pub const INTERNAL_PREFIX: &str = "$$";
+
     pub fn new(selector: &str) -> &Self {
         unsafe { &*(ptr::from_ref::<str>(selector) as *const Self) }
     }
@@ -505,6 +507,10 @@ impl Selector {
 
         t.serialize(ExtractSelector(None)).unwrap()
     }
+
+    pub fn is_internal(&self) -> bool {
+        self.0.starts_with(Self::INTERNAL_PREFIX)
+    }
 }
 
 impl fmt::Display for Selector {

lib/access-manager-types/src/manager/error.rs

@@ -49,4 +49,7 @@ pub enum AccessManagerError {
         target: Addr,
         selector: Box<Selector>,
     },
+
+    #[error("cannot set permissions for internal selector {0}")]
+    InternalSelector(Box<Selector>),
 }