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..4c6a8c1ac 100644 --- a/core/station/impl/src/services/request.rs +++ b/core/station/impl/src/services/request.rs @@ -327,8 +327,19 @@ 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()); + 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()); + } + // Different request types may have different validation rules. request.validate()?; @@ -571,12 +582,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 +596,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 +637,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 +867,92 @@ 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 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(); diff --git a/tests/canister/api/spec.did b/tests/canister/api/spec.did index 1ab6c0b9c..eaa0f807d 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 40519c8ed..045c36d1e 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) }; static RELEASED: std::cell::RefCell = const { std::cell::RefCell::new(false) }; } @@ -45,6 +46,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 1e10b19d6..ebbb568ce 100644 --- a/tests/integration/src/external_canister_tests.rs +++ b/tests/integration/src/external_canister_tests.rs @@ -1,10 +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, ADMIN_GROUP_ID, - 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, + ADMIN_GROUP_ID, COUNTER_WAT, }; use crate::TestEnv; use candid::{Encode, Principal}; @@ -21,15 +21,16 @@ use station_api::{ ChangeExternalCanisterOperationInput, CreateExternalCanisterOperationInput, CreateExternalCanisterOperationKindAddExistingDTO, CreateExternalCanisterOperationKindCreateNewDTO, CreateExternalCanisterOperationKindDTO, - EditPermissionOperationInput, ExecutionMethodResourceTargetDTO, - ExternalCanisterCallPermissionDTO, ExternalCanisterCallRequestPolicyRuleInput, - ExternalCanisterIdDTO, ExternalCanisterPermissionsCreateInput, - ExternalCanisterRequestPoliciesCreateInput, HealthStatus, ListRequestsInput, - ListRequestsOperationTypeDTO, ListRequestsResponse, PruneExternalCanisterOperationInput, - PruneExternalCanisterResourceDTO, QuorumDTO, RequestApprovalStatusDTO, RequestOperationDTO, + CreateRequestInput, CreateRequestResponse, EditPermissionOperationInput, + EditUserOperationInput, ExecutionMethodResourceTargetDTO, ExternalCanisterCallPermissionDTO, + ExternalCanisterCallRequestPolicyRuleInput, ExternalCanisterIdDTO, + ExternalCanisterPermissionsCreateInput, ExternalCanisterRequestPoliciesCreateInput, + HealthStatus, ListRequestsInput, ListRequestsOperationTypeDTO, ListRequestsResponse, + PruneExternalCanisterOperationInput, PruneExternalCanisterResourceDTO, QuorumDTO, + RequestApprovalStatusDTO, RequestExecutionScheduleDTO, RequestOperationDTO, RequestOperationInput, RequestPolicyRuleDTO, RequestSpecifierDTO, RequestStatusDTO, RestoreExternalCanisterOperationInput, Snapshot, SnapshotExternalCanisterOperationInput, - UserSpecifierDTO, ValidationMethodResourceTargetDTO, + UserSpecifierDTO, UserStatusDTO, ValidationMethodResourceTargetDTO, }; use std::str::FromStr; use std::time::Duration; @@ -1068,6 +1069,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 {