Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 80 additions & 6 deletions core/station/impl/src/mappers/request_policy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
}
Expand Down Expand Up @@ -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)));
}
}
22 changes: 21 additions & 1 deletion core/station/impl/src/migration.rs
Original file line number Diff line number Diff line change
@@ -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.
///
Expand Down Expand Up @@ -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);
}
}
Comment on lines +65 to +73

/// The migration to apply to the station canister stable memory.
///
Expand Down
34 changes: 32 additions & 2 deletions core/station/impl/src/repositories/request_policy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand All @@ -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();
Expand Down
Loading