From ff342156e448d8b1e3cf1403cf730a275b2264dc Mon Sep 17 00:00:00 2001 From: sea-snake Date: Fri, 23 Oct 2026 14:40:36 +0200 Subject: [PATCH 1/3] fix(station): re-check the requester after preparing a request Preparing a request can await an external validation call, so the requester's status and permission are checked again before the request is stored. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../station/impl/src/models/request_policy.rs | 15 +++- core/station/impl/src/services/request.rs | 70 +++++++++++++++++-- 2 files changed, 79 insertions(+), 6 deletions(-) diff --git a/core/station/impl/src/models/request_policy.rs b/core/station/impl/src/models/request_policy.rs index 651cb1011..92546d28a 100644 --- a/core/station/impl/src/models/request_policy.rs +++ b/core/station/impl/src/models/request_policy.rs @@ -166,14 +166,19 @@ pub mod request_policy_test_utils { core::CallContext, errors::RequestPolicyError, models::{ - request_policy_rule::RequestPolicyRule, request_specifier::RequestSpecifier, + permission::{Allow, Permission}, + request_policy_rule::RequestPolicyRule, + request_specifier::RequestSpecifier, + resource::{Resource, UserResourceAction}, AddNamedRuleOperationInput, AddRequestPolicyOperationInput, AddUserOperationInput, MetadataItem, ADMIN_GROUP_ID, }, + repositories::permission::PERMISSION_REPOSITORY, services::{NAMED_RULE_SERVICE, REQUEST_POLICY_SERVICE, REQUEST_SERVICE, USER_SERVICE}, }; use candid::Principal; - use orbit_essentials::model::ModelValidator; + use orbit_essentials::model::{ModelKey, ModelValidator}; + use orbit_essentials::repository::Repository; use station_api::CreateRequestInput; use uuid::Uuid; @@ -222,6 +227,12 @@ pub mod request_policy_test_utils { }) .expect("Failed to add user"); + let permission = Permission::new( + Allow::user_groups(vec![*ADMIN_GROUP_ID]), + Resource::User(UserResourceAction::Create), + ); + PERMISSION_REPOSITORY.insert(permission.key(), permission); + let ctx = CallContext::new(user.identities[0]); let request = REQUEST_SERVICE diff --git a/core/station/impl/src/services/request.rs b/core/station/impl/src/services/request.rs index bac89216c..fe12a888d 100644 --- a/core/station/impl/src/services/request.rs +++ b/core/station/impl/src/services/request.rs @@ -327,8 +327,18 @@ impl RequestService { ctx: &CallContext, ) -> ServiceResult { let requester = self.user_service.get_user_by_identity(&ctx.caller())?; + let resource = Resource::from(&input); let mut request = RequestFactory::create_request(requester.id, input).await?; + // Preparing the request can await an external validation call, so the requester is + // re-checked against the current state before the request is stored. + let current_ctx = CallContext::new(ctx.caller()); + if current_ctx.user().map(|user| user.id) != Some(requester.id) + || !Authorization::is_allowed(¤t_ctx, &resource) + { + return Err(RequestError::Unauthorized.into()); + } + // Different request types may have different validation rules. request.validate()?; @@ -571,12 +581,12 @@ mod tests { models::{ account_test_utils::mock_account, asset_test_utils::mock_asset, - permission::Allow, + permission::{Allow, Permission}, request_policy_rule::RequestPolicyRule, request_policy_test_utils::mock_request_policy, request_specifier::{RequestSpecifier, UserSpecifier}, request_test_utils::mock_request, - resource::ResourceIds, + resource::{AccountResourceAction, ResourceAction, ResourceIds}, user_test_utils::mock_user, AddAccountOperationInput, AddAddressBookEntryOperation, AddAddressBookEntryOperationInput, AddAssetOperationInput, AddUserOperation, @@ -585,8 +595,9 @@ mod tests { TransferOperation, TransferOperationInput, User, UserGroup, UserStatus, ADMIN_GROUP_ID, }, repositories::{ - request_policy::REQUEST_POLICY_REPOSITORY, AccountRepository, AssetRepository, - NOTIFICATION_REPOSITORY, USER_GROUP_REPOSITORY, USER_REPOSITORY, + permission::PERMISSION_REPOSITORY, request_policy::REQUEST_POLICY_REPOSITORY, + AccountRepository, AssetRepository, NOTIFICATION_REPOSITORY, USER_GROUP_REPOSITORY, + USER_REPOSITORY, }, services::{AccountService, ASSET_SERVICE}, }; @@ -625,6 +636,14 @@ mod tests { USER_REPOSITORY.insert(user.to_key(), user.clone()); + for resource in [ + Resource::AddressBook(ResourceAction::Create), + Resource::Account(AccountResourceAction::Transfer(ResourceId::Any)), + ] { + let permission = Permission::new(Allow::user_groups(vec![*ADMIN_GROUP_ID]), resource); + PERMISSION_REPOSITORY.insert(permission.key(), permission); + } + let call_context = CallContext::new(caller_principal); TestContext { @@ -847,6 +866,49 @@ mod tests { assert!(!request.approvals.is_empty()); } + #[tokio::test] + async fn create_request_refuses_requester_deactivated_after_authorization() { + let ctx = setup(); + + let input = CreateRequestInput { + operation: station_api::RequestOperationInput::AddAddressBookEntry( + station_api::AddAddressBookEntryOperationInput { + address_owner: "".to_owned(), + address: "abc".to_owned(), + blockchain: "icp".to_owned(), + metadata: vec![], + labels: vec![], + address_format: AddressFormat::ICPAccountIdentifier.to_string(), + }, + ), + title: None, + summary: None, + execution_plan: None, + expiration_dt: None, + deduplication_key: None, + tags: None, + }; + + ctx.service + .create_request(input.clone(), &ctx.call_context) + .await + .expect("an active requester can create the request"); + assert_eq!(ctx.repository.list().len(), 1); + + let mut deactivated_user = ctx.caller_user.clone(); + deactivated_user.status = UserStatus::Inactive; + USER_REPOSITORY.insert(deactivated_user.to_key(), deactivated_user); + + let error = ctx + .service + .create_request(input, &ctx.call_context) + .await + .expect_err("a deactivated requester cannot create the request"); + + assert_eq!(error.code, "UNAUTHORIZED"); + assert_eq!(ctx.repository.list().len(), 1); + } + #[tokio::test] async fn user_can_cancel_their_own_pending_request() { let ctx = setup(); From 045d9ee0c11cb928d4abf8b0ace9515fd31f6456 Mon Sep 17 00:00:00 2001 From: sea-snake Date: Fri, 23 Oct 2026 14:56:36 +0200 Subject: [PATCH 2/3] test(station): deactivate the requester while request validation is pending Adds an integration test that keeps a call external canister request's validation call pending, deactivates the requester meanwhile, and checks that the request is refused once validation replies. Co-Authored-By: Claude Opus 5.5 (1M context) --- tests/canister/api/spec.did | 2 + tests/canister/impl/src/lib.rs | 16 +++ .../src/external_canister_tests.rs | 118 ++++++++++++++++-- 3 files changed, 129 insertions(+), 7 deletions(-) diff --git a/tests/canister/api/spec.did b/tests/canister/api/spec.did index 3dfc291be..52b8f26a9 100644 --- a/tests/canister/api/spec.did +++ b/tests/canister/api/spec.did @@ -10,6 +10,8 @@ type StoreNumberInput = record { service : { call : (canister_id : principal, method_name : text, arg : blob, total_calls : nat64) -> (); validate_number : (input : StoreNumberInput) -> (ValidationResponse); + validate_when_released : () -> (ValidationResponse); + release_validation : () -> (); store_number : (input : StoreNumberInput) -> (); get_number : () -> (nat64) query; noop : () -> (); diff --git a/tests/canister/impl/src/lib.rs b/tests/canister/impl/src/lib.rs index 408d99e1b..a5954254b 100644 --- a/tests/canister/impl/src/lib.rs +++ b/tests/canister/impl/src/lib.rs @@ -6,6 +6,7 @@ use ic_cdk::{id, query, update}; thread_local! { static NUMBER: std::cell::RefCell = const { std::cell::RefCell::new(0) }; + static VALIDATION_RELEASED: std::cell::RefCell = const { std::cell::RefCell::new(false) }; } #[update] @@ -44,6 +45,21 @@ async fn validate_number(input: StoreNumberInput) -> ValidationResponse { } } +#[update] +async fn validate_when_released() -> ValidationResponse { + while !VALIDATION_RELEASED.with(|released| *released.borrow()) { + let _ = ic_cdk::call::<_, ()>(id(), "noop", ((),)).await; + } + ValidationResponse::Ok("Released".to_string()) +} + +#[update] +async fn release_validation() { + VALIDATION_RELEASED.with(|released| { + *released.borrow_mut() = true; + }); +} + #[update] async fn store_number(input: StoreNumberInput) { NUMBER.with(|n| { diff --git a/tests/integration/src/external_canister_tests.rs b/tests/integration/src/external_canister_tests.rs index 41d8b7b81..ea25b12f6 100644 --- a/tests/integration/src/external_canister_tests.rs +++ b/tests/integration/src/external_canister_tests.rs @@ -1,9 +1,10 @@ use crate::setup::{create_canister, setup_new_env, WALLET_ADMIN_USER}; use crate::utils::{ add_user, bump_time_to_avoid_ratelimit, canister_status, deploy_test_canister, execute_request, - get_core_canister_health_status, get_request, get_system_info, hash, submit_request, - submit_request_approval, submit_request_raw, submit_request_with_expected_trap, update_raw, - upload_canister_chunks_to_asset_canister, user_test_id, wait_for_request, COUNTER_WAT, + expect_await_call_result, get_core_canister_health_status, get_request, get_system_info, hash, + submit_request, submit_request_approval, submit_request_raw, submit_request_with_expected_trap, + update_raw, upload_canister_chunks_to_asset_canister, user_test_id, wait_for_request, + COUNTER_WAT, }; use crate::TestEnv; use candid::{Encode, Principal}; @@ -19,13 +20,15 @@ use station_api::{ CallExternalCanisterResourceTargetDTO, CanisterInstallMode, CanisterMethodDTO, ChangeExternalCanisterOperationInput, CreateExternalCanisterOperationInput, CreateExternalCanisterOperationKindCreateNewDTO, CreateExternalCanisterOperationKindDTO, - EditPermissionOperationInput, ExecutionMethodResourceTargetDTO, ExternalCanisterIdDTO, + CreateRequestInput, CreateRequestResponse, EditPermissionOperationInput, + EditUserOperationInput, ExecutionMethodResourceTargetDTO, ExternalCanisterIdDTO, ExternalCanisterPermissionsCreateInput, ExternalCanisterRequestPoliciesCreateInput, HealthStatus, ListRequestsInput, ListRequestsOperationTypeDTO, ListRequestsResponse, PruneExternalCanisterOperationInput, PruneExternalCanisterResourceDTO, QuorumDTO, - RequestApprovalStatusDTO, RequestOperationDTO, RequestOperationInput, RequestPolicyRuleDTO, - RequestSpecifierDTO, RequestStatusDTO, RestoreExternalCanisterOperationInput, Snapshot, - SnapshotExternalCanisterOperationInput, UserSpecifierDTO, ValidationMethodResourceTargetDTO, + RequestApprovalStatusDTO, RequestExecutionScheduleDTO, RequestOperationDTO, + RequestOperationInput, RequestPolicyRuleDTO, RequestSpecifierDTO, RequestStatusDTO, + RestoreExternalCanisterOperationInput, Snapshot, SnapshotExternalCanisterOperationInput, + UserSpecifierDTO, UserStatusDTO, ValidationMethodResourceTargetDTO, }; use std::str::FromStr; use std::time::Duration; @@ -1064,6 +1067,107 @@ fn call_external_canister_test() { ); } +#[test] +fn create_call_external_canister_request_refuses_requester_deactivated_during_validation() { + let TestEnv { + env, canister_ids, .. + } = setup_new_env(); + + let test_canister_id = deploy_test_canister(&env, canister_ids.station); + + let requester = user_test_id(0); + let requester_user = add_user(&env, requester, vec![], canister_ids.station); + + let validation_method = CanisterMethodDTO { + canister_id: test_canister_id, + method_name: "validate_when_released".to_string(), + }; + let execution_method = CanisterMethodDTO { + canister_id: test_canister_id, + method_name: "noop".to_string(), + }; + execute_request( + &env, + WALLET_ADMIN_USER, + canister_ids.station, + RequestOperationInput::EditPermission(EditPermissionOperationInput { + resource: station_api::ResourceDTO::ExternalCanister( + station_api::ExternalCanisterResourceActionDTO::Call( + CallExternalCanisterResourceTargetDTO { + validation_method: ValidationMethodResourceTargetDTO::ValidationMethod( + validation_method.clone(), + ), + execution_method: ExecutionMethodResourceTargetDTO::ExecutionMethod( + execution_method.clone(), + ), + }, + ), + ), + auth_scope: Some(station_api::AuthScopeDTO::Authenticated), + user_groups: None, + users: None, + }), + ) + .unwrap(); + + // the validation method does not reply until it is released, which keeps the request creation pending + let create_request_input = CreateRequestInput { + operation: RequestOperationInput::CallExternalCanister( + CallExternalCanisterOperationInput { + validation_method: Some(validation_method), + execution_method, + arg: None, + execution_method_cycles: None, + }, + ), + title: None, + summary: None, + execution_plan: Some(RequestExecutionScheduleDTO::Immediate), + expiration_dt: None, + deduplication_key: None, + tags: None, + }; + let message_id = env + .submit_call( + canister_ids.station, + requester, + "create_request", + Encode!(&create_request_input).unwrap(), + ) + .unwrap(); + env.tick(); + env.tick(); + + execute_request( + &env, + WALLET_ADMIN_USER, + canister_ids.station, + RequestOperationInput::EditUser(EditUserOperationInput { + id: requester_user.id, + name: None, + identities: None, + groups: None, + status: Some(UserStatusDTO::Inactive), + cancel_pending_requests: None, + }), + ) + .unwrap(); + + update_candid_as::<_, ((),)>( + &env, + test_canister_id, + Principal::anonymous(), + "release_validation", + ((),), + ) + .unwrap(); + + let (result,) = expect_await_call_result::<(ApiResult,)>( + env.await_call(message_id).unwrap(), + ); + assert_eq!(result.unwrap_err().code, "UNAUTHORIZED"); +} + #[test] fn create_external_canister_with_too_many_cycles() { let TestEnv { From 771fb6cb3d9539c7201a93bae7f6395124b6cf51 Mon Sep 17 00:00:00 2001 From: sea-snake Date: Fri, 23 Oct 2026 14:59:53 +0200 Subject: [PATCH 3/3] fix(station): require an active requester when storing a request The re-check after preparing a request now requires the requester to be active independently of the resource permission, so a public permission does not let a deactivated requester's request through. Co-Authored-By: Claude Opus 5.5 (1M context) --- core/station/impl/src/services/request.rs | 50 +++++++++++++++++++++-- 1 file changed, 47 insertions(+), 3 deletions(-) diff --git a/core/station/impl/src/services/request.rs b/core/station/impl/src/services/request.rs index fe12a888d..4c6a8c1ac 100644 --- a/core/station/impl/src/services/request.rs +++ b/core/station/impl/src/services/request.rs @@ -333,9 +333,10 @@ impl RequestService { // Preparing the request can await an external validation call, so the requester is // re-checked against the current state before the request is stored. let current_ctx = CallContext::new(ctx.caller()); - if current_ctx.user().map(|user| user.id) != Some(requester.id) - || !Authorization::is_allowed(¤t_ctx, &resource) - { + let requester_is_current_and_active = current_ctx + .user() + .is_some_and(|user| user.id == requester.id && user.is_active()); + if !requester_is_current_and_active || !Authorization::is_allowed(¤t_ctx, &resource) { return Err(RequestError::Unauthorized.into()); } @@ -909,6 +910,49 @@ mod tests { assert_eq!(ctx.repository.list().len(), 1); } + #[tokio::test] + async fn create_request_refuses_deactivated_requester_with_public_permission() { + let ctx = setup(); + + let permission = Permission::new( + Allow::public(), + Resource::AddressBook(ResourceAction::Create), + ); + PERMISSION_REPOSITORY.insert(permission.key(), permission); + + let mut deactivated_user = ctx.caller_user.clone(); + deactivated_user.status = UserStatus::Inactive; + USER_REPOSITORY.insert(deactivated_user.to_key(), deactivated_user); + + let input = CreateRequestInput { + operation: station_api::RequestOperationInput::AddAddressBookEntry( + station_api::AddAddressBookEntryOperationInput { + address_owner: "".to_owned(), + address: "abc".to_owned(), + blockchain: "icp".to_owned(), + metadata: vec![], + labels: vec![], + address_format: AddressFormat::ICPAccountIdentifier.to_string(), + }, + ), + title: None, + summary: None, + execution_plan: None, + expiration_dt: None, + deduplication_key: None, + tags: None, + }; + + let error = ctx + .service + .create_request(input, &ctx.call_context) + .await + .expect_err("a deactivated requester cannot create the request"); + + assert_eq!(error.code, "UNAUTHORIZED"); + assert!(ctx.repository.list().is_empty()); + } + #[tokio::test] async fn user_can_cancel_their_own_pending_request() { let ctx = setup();