diff --git a/rs/execution_environment/src/canister_logs.rs b/rs/execution_environment/src/canister_logs.rs index 4bd74b18f7bc..fc2aae6f557e 100644 --- a/rs/execution_environment/src/canister_logs.rs +++ b/rs/execution_environment/src/canister_logs.rs @@ -1,3 +1,4 @@ +use crate::canister_settings::VisibilitySettings; use ic_config::flag_status::FlagStatus; use ic_error_types::{ErrorCode, UserError}; use ic_management_canister_types_private::{ @@ -42,22 +43,13 @@ pub(crate) fn check_log_visibility_permission( log_visibility: &LogVisibilityV2, controllers: &std::collections::BTreeSet, ) -> Result<(), UserError> { - let has_access = match log_visibility { - LogVisibilityV2::Public => true, - LogVisibilityV2::Controllers => controllers.contains(caller), - LogVisibilityV2::AllowedViewers(principals) => { - principals.get().contains(caller) || controllers.contains(caller) - } - }; - - if has_access { - Ok(()) - } else { - Err(UserError::new( + if !VisibilitySettings::from(log_visibility).has_access(caller, controllers) { + return Err(UserError::new( ErrorCode::CanisterRejectedMessage, format!("Caller {caller} is not allowed to access canister logs"), - )) + )); } + Ok(()) } fn filter_records( diff --git a/rs/execution_environment/src/canister_settings.rs b/rs/execution_environment/src/canister_settings.rs index 877a94916f84..457ec22e2d8f 100644 --- a/rs/execution_environment/src/canister_settings.rs +++ b/rs/execution_environment/src/canister_settings.rs @@ -1,11 +1,13 @@ use ic_base_types::{EnvironmentVariables, NumBytes, NumSeconds}; use ic_error_types::{ErrorCode, UserError}; -use ic_management_canister_types_private::{CanisterSettingsArgs, LogVisibilityV2}; +use ic_management_canister_types_private::{ + BoundedAllowedViewers, CanisterSettingsArgs, LogVisibilityV2, +}; use ic_types::{ ComputeAllocation, Cycles, InvalidComputeAllocationError, MemoryAllocation, PrincipalId, }; use num_traits::cast::ToPrimitive; -use std::collections::BTreeMap; +use std::collections::{BTreeMap, BTreeSet}; use std::convert::TryFrom; #[cfg(test)] @@ -483,3 +485,36 @@ impl ValidatedCanisterSettings { self.environment_variables.as_ref() } } + +#[derive(Clone, Eq, PartialEq, Debug)] +pub(crate) enum VisibilitySettings<'a> { + Public, + Controllers, + AllowedViewers(&'a BoundedAllowedViewers), +} + +impl<'a> From<&'a LogVisibilityV2> for VisibilitySettings<'a> { + fn from(v: &'a LogVisibilityV2) -> Self { + match v { + LogVisibilityV2::Public => Self::Public, + LogVisibilityV2::Controllers => Self::Controllers, + LogVisibilityV2::AllowedViewers(principals) => Self::AllowedViewers(principals), + } + } +} + +impl VisibilitySettings<'_> { + pub(crate) fn has_access( + &self, + caller: &PrincipalId, + controllers: &BTreeSet, + ) -> bool { + match self { + Self::Public => true, + Self::Controllers => controllers.contains(caller), + Self::AllowedViewers(allowed) => { + allowed.get().contains(caller) || controllers.contains(caller) + } + } + } +} diff --git a/rs/execution_environment/src/canister_settings/tests.rs b/rs/execution_environment/src/canister_settings/tests.rs index 3dde4fbf7590..69f8da923e1f 100644 --- a/rs/execution_environment/src/canister_settings/tests.rs +++ b/rs/execution_environment/src/canister_settings/tests.rs @@ -86,3 +86,56 @@ fn test_environment_variables_hash_output() { let actual = EnvironmentVariables::new(env_vars).hash(); assert_eq!(actual, expected); } + +mod visibility_settings { + use crate::canister_settings::VisibilitySettings; + use ic_management_canister_types_private::BoundedAllowedViewers; + use ic_types::PrincipalId; + use proptest::prelude::*; + use std::collections::BTreeSet; + + proptest! { + #[test] + fn public_always_grants_access( + caller in arb_principal_id(), + controllers in arb_controllers(), + ) { + prop_assert!(VisibilitySettings::Public.has_access(&caller, &controllers)); + } + + #[test] + fn controllers_grants_access_only_to_controllers( + caller in arb_principal_id(), + controllers in arb_controllers(), + ) { + prop_assert_eq!( + VisibilitySettings::Controllers.has_access(&caller, &controllers), + controllers.contains(&caller), + ); + } + + #[test] + fn allowed_viewers_grants_access_to_viewers_and_controllers( + caller in arb_principal_id(), + controllers in arb_controllers(), + viewers in proptest::collection::vec(arb_principal_id(), 0..=10), + ) { + let allowed = BoundedAllowedViewers::new(viewers.clone()); + let settings = VisibilitySettings::AllowedViewers(&allowed); + prop_assert_eq!( + settings.has_access(&caller, &controllers), + viewers.contains(&caller) || controllers.contains(&caller), + ); + } + } + + fn arb_principal_id() -> BoxedStrategy { + (0..u64::MAX) + .prop_map(PrincipalId::new_user_test_id) + .boxed() + } + + fn arb_controllers() -> BoxedStrategy> { + proptest::collection::btree_set(arb_principal_id(), 0..=10).boxed() + } +}