From ab053f24370b3308b257a9e24ae4accff444071d Mon Sep 17 00:00:00 2001 From: Mario Ruci Date: Thu, 6 Aug 2026 15:49:13 +0200 Subject: [PATCH] fix(station): index EditPermission policies under the permission resource RequestSpecifier::EditPermission(Resource(R)) indexed the policy under R itself rather than under Resource::Permission(Update). The EditPermission operation only ever emits Permission(Update), so the granular form never governed permission changes, and because the resource index is the sole policy selector it was instead returned for any operation whose resource is R. With OR-combined evaluation that turned a rule about who may change a permission into an additional, weaker approval path for the operations on that resource. Index both scopes under Permission(Update), and rebuild the request policy resource index on upgrade so policies already stored under the old keys are refiled. Correcting the mapper alone would leave existing entries pointing at the wrong resources. Co-Authored-By: Claude Opus 5 --- .../impl/src/mappers/request_policy.rs | 86 +++++++++++++++++-- core/station/impl/src/migration.rs | 22 ++++- .../impl/src/repositories/request_policy.rs | 34 +++++++- 3 files changed, 133 insertions(+), 9 deletions(-) diff --git a/core/station/impl/src/mappers/request_policy.rs b/core/station/impl/src/mappers/request_policy.rs index f84783c8d..fd754ab7a 100644 --- a/core/station/impl/src/mappers/request_policy.rs +++ b/core/station/impl/src/mappers/request_policy.rs @@ -485,12 +485,13 @@ impl RequestSpecifier { ExternalCanisterResourceAction::Call(target.clone()), )] } - RequestSpecifier::EditPermission(resource_specifier) => match resource_specifier { - ResourceSpecifier::Any => { - vec![Resource::Permission(PermissionResourceAction::Update)] - } - ResourceSpecifier::Resource(resource) => vec![resource.clone()], - }, + // Both scopes index under what the EditPermission *operation* produces. Filing the + // granular form under its target resource put it in the same bucket as that + // resource's own operations, where OR-combined evaluation turned a + // permission-administration rule into an extra approval path for those operations. + RequestSpecifier::EditPermission(_) => { + vec![Resource::Permission(PermissionResourceAction::Update)] + } RequestSpecifier::AddRequestPolicy => { vec![Resource::RequestPolicy(ResourceAction::Create)] } @@ -571,3 +572,76 @@ impl RequestSpecifier { } } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::models::{ + asset_test_utils::mock_asset, + resource::{AccountResourceAction, ResourceId}, + EditPermissionOperation, EditPermissionOperationInput, Metadata, RequestOperation, + TokenStandard, TransferOperation, TransferOperationInput, + }; + + fn treasury_transfer_resource(account_id: [u8; 16]) -> Resource { + Resource::Account(AccountResourceAction::Transfer(ResourceId::Id(account_id))) + } + + fn transfer_operation(account_id: [u8; 16]) -> RequestOperation { + RequestOperation::Transfer(TransferOperation { + fee: None, + transfer_id: None, + asset: mock_asset(), + input: TransferOperationInput { + from_account_id: account_id, + from_asset_id: [0; 16], + with_standard: TokenStandard::InternetComputerNative, + to: "0x1234567890abcdef".to_string(), + amount: 100u64.into(), + metadata: Metadata::default(), + network: "mainnet".to_string(), + fee: None, + }, + }) + } + + #[test] + fn edit_permission_specifier_indexes_under_the_operation_resource() { + let expected = vec![Resource::Permission(PermissionResourceAction::Update)]; + + let scoped = RequestSpecifier::EditPermission(ResourceSpecifier::Resource( + treasury_transfer_resource([1; 16]), + )); + let any = RequestSpecifier::EditPermission(ResourceSpecifier::Any); + + assert_eq!(scoped.to_resources(), expected); + assert_eq!(any.to_resources(), expected); + } + + /// Round-trip invariant: a specifier must only index under keys its own operation is + /// evaluated against, and must never index under a key produced by a different operation. + #[test] + fn edit_permission_specifier_never_matches_an_unrelated_operation() { + let account_id = [7; 16]; + let specifier = RequestSpecifier::EditPermission(ResourceSpecifier::Resource( + treasury_transfer_resource(account_id), + )); + + let own_operation = RequestOperation::EditPermission(EditPermissionOperation { + input: EditPermissionOperationInput { + resource: treasury_transfer_resource(account_id), + auth_scope: None, + users: None, + user_groups: None, + }, + }); + + let indexed = specifier.to_resources(); + let own_keys = own_operation.to_resources(); + let unrelated_keys = transfer_operation(account_id).to_resources(); + + assert!(!indexed.is_empty()); + assert!(indexed.iter().all(|key| own_keys.contains(key))); + assert!(indexed.iter().all(|key| !unrelated_keys.contains(key))); + } +} diff --git a/core/station/impl/src/migration.rs b/core/station/impl/src/migration.rs index c2d6107f3..b9f28d606 100644 --- a/core/station/impl/src/migration.rs +++ b/core/station/impl/src/migration.rs @@ -1,6 +1,8 @@ use crate::core::ic_cdk::api::trap; use crate::core::{read_system_info, write_system_info}; +use crate::repositories::REQUEST_POLICY_REPOSITORY; use crate::STABLE_MEMORY_VERSION; +use orbit_essentials::repository::{IndexedRepository, Repository}; /// Handles stable memory schema migrations for the station canister. /// @@ -50,7 +52,25 @@ impl MigrationHandler { /// If there is a check that needs to be run on every upgrade, regardless if the memory version has changed, /// it should be added here. -fn post_run() {} +fn post_run() { + rebuild_request_policy_resource_index(); +} + +/// Rebuilds the request policy resource index from the policies themselves. +/// +/// The index is derived from `RequestSpecifier::to_resources()` and is written once, when a policy +/// is inserted. Entries written before that mapping is corrected keep pointing at the old keys, so +/// correcting the mapping alone would leave existing policies matching the wrong operations. This +/// is idempotent and cheap relative to the number of stored policies. +fn rebuild_request_policy_resource_index() { + let policies = REQUEST_POLICY_REPOSITORY.list(); + + REQUEST_POLICY_REPOSITORY.clear_indexes(); + + for policy in &policies { + REQUEST_POLICY_REPOSITORY.add_entry_indexes(policy); + } +} /// The migration to apply to the station canister stable memory. /// diff --git a/core/station/impl/src/repositories/request_policy.rs b/core/station/impl/src/repositories/request_policy.rs index 8be6cdf17..8e9160f32 100644 --- a/core/station/impl/src/repositories/request_policy.rs +++ b/core/station/impl/src/repositories/request_policy.rs @@ -161,8 +161,10 @@ mod tests { indexes::request_policy_resource_index::RequestPolicyResourceIndex, request_policy_rule::RequestPolicyRule, request_policy_test_utils::mock_request_policy, - request_specifier::RequestSpecifier, - resource::{AccountResourceAction, Resource, ResourceId, ResourceIds}, + request_specifier::{RequestSpecifier, ResourceSpecifier}, + resource::{ + AccountResourceAction, PermissionResourceAction, Resource, ResourceId, ResourceIds, + }, }; #[test] @@ -179,6 +181,34 @@ mod tests { assert!(repository.get(&policy.id).is_none()); } + /// A rule about who may change permissions on a resource must not become an approval path for + /// operations on that resource. + #[test] + fn edit_permission_policy_is_not_returned_for_the_target_resource() { + let repository = RequestPolicyRepository::default(); + let treasury = [42; 16]; + let treasury_transfer = + Resource::Account(AccountResourceAction::Transfer(ResourceId::Id(treasury))); + + let policy = RequestPolicy { + id: [9; 16], + specifier: RequestSpecifier::EditPermission(ResourceSpecifier::Resource( + treasury_transfer.clone(), + )), + rule: RequestPolicyRule::AutoApproved, + }; + + repository.insert(policy.id, policy.clone()); + + assert!(repository.find_by_resource(treasury_transfer).is_empty()); + assert_eq!( + repository + .find_by_resource(Resource::Permission(PermissionResourceAction::Update)) + .len(), + 1 + ); + } + #[test] fn update_policy_resource_index_on_policy_mutation() { let repository = RequestPolicyRepository::default();