diff --git a/candid/evm_rpc.did b/candid/evm_rpc.did index 170cbf77..367d1b95 100644 --- a/candid/evm_rpc.did +++ b/candid/evm_rpc.did @@ -226,8 +226,8 @@ type ValidationError = variant { CredentialHeaderNotAllowed; }; service : (InitArgs) -> { - authorize : (principal, Auth) -> (); - deauthorize : (principal, Auth) -> (); + authorize : (principal, Auth) -> (bool); + deauthorize : (principal, Auth) -> (bool); eth_feeHistory : (RpcSource, opt RpcConfig, FeeHistoryArgs) -> (MultiFeeHistoryResult); eth_getBlockByNumber : (RpcSource, opt RpcConfig, BlockTag) -> (MultiGetBlockByNumberResult); eth_getLogs : (RpcSource, opt RpcConfig, GetLogsArgs) -> (MultiGetLogsResult); diff --git a/src/auth.rs b/src/auth.rs index c2c875cb..0632b780 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -33,32 +33,40 @@ pub fn is_rpc_allowed(caller: &Principal) -> bool { METADATA.with(|m| m.borrow().get().open_rpc_access) || is_authorized(caller, Auth::PriorityRpc) } -pub fn do_authorize(principal: Principal, auth: Auth) { - AUTH.with(|a| { - let mut auth_map = a.borrow_mut(); - let principal = PrincipalStorable(principal); - if let Some(mut v) = auth_map.get(&principal) { - v.authorize(auth); - auth_map.insert(principal, v); - } else { - auth_map.insert(principal, AuthSet::new(vec![auth])); - } - }); +pub fn do_authorize(principal: Principal, auth: Auth) -> bool { + if principal == Principal::anonymous() { + false + } else { + AUTH.with(|a| { + let mut auth_map = a.borrow_mut(); + let principal = PrincipalStorable(principal); + let mut auth_set = auth_map.get(&principal).unwrap_or_default(); + if auth_set.authorize(auth) { + auth_map.insert(principal, auth_set); + true + } else { + false + } + }) + } } -pub fn do_deauthorize(principal: Principal, auth: Auth) { +pub fn do_deauthorize(principal: Principal, auth: Auth) -> bool { AUTH.with(|a| { let mut auth_map = a.borrow_mut(); let principal = PrincipalStorable(principal); - if let Some(mut v) = auth_map.get(&principal) { - v.deauthorize(auth); - if v.is_empty() { + if let Some(mut auth_set) = auth_map.get(&principal) { + let changed = auth_set.deauthorize(auth); + if auth_set.is_empty() { auth_map.remove(&principal); } else { - auth_map.insert(principal, v); + auth_map.insert(principal, auth_set); } + changed + } else { + false } - }); + }) } #[test] diff --git a/src/main.rs b/src/main.rs index 115b39da..89338e8d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -10,6 +10,7 @@ use ic_canister_log::log; use ic_canisters_http_types::{ HttpRequest as AssetHttpRequest, HttpResponse as AssetHttpResponse, HttpResponseBuilder, }; +use ic_cdk::api::is_controller; use ic_cdk::api::management_canister::http_request::{HttpResponse, TransformArgs}; use ic_cdk::{query, update}; use ic_nervous_system_common::serve_metrics; @@ -150,13 +151,15 @@ fn register_provider(provider: RegisterProviderArgs) -> u64 { #[update(name = "unregisterProvider")] #[candid_method(rename = "unregisterProvider")] fn unregister_provider(provider_id: u64) -> bool { - do_unregister_provider(ic_cdk::caller(), provider_id) + let caller = ic_cdk::caller(); + do_unregister_provider(caller, is_controller(&caller), provider_id) } #[update(name = "updateProvider")] #[candid_method(rename = "updateProvider")] fn update_provider(provider: UpdateProviderArgs) { - do_update_provider(ic_cdk::caller(), provider) + let caller = ic_cdk::caller(); + do_update_provider(caller, is_controller(&caller), provider) } #[update(name = "manageProvider", guard = "require_admin_or_controller")] @@ -386,7 +389,7 @@ fn stable_read(offset: u64, length: u64) -> Vec { #[update(guard = "require_admin_or_controller")] #[candid_method] -fn authorize(principal: Principal, auth: Auth) { +fn authorize(principal: Principal, auth: Auth) -> bool { log!( INFO, "[{}] Authorizing `{:?}` for principal: {}", @@ -413,7 +416,7 @@ fn get_authorized(auth: Auth) -> Vec { #[update(guard = "require_admin_or_controller")] #[candid_method] -fn deauthorize(principal: Principal, auth: Auth) { +fn deauthorize(principal: Principal, auth: Auth) -> bool { log!( INFO, "[{}] Deauthorizing `{:?}` for principal: {}", diff --git a/src/providers.rs b/src/providers.rs index 17599a1d..1464e9aa 100644 --- a/src/providers.rs +++ b/src/providers.rs @@ -213,12 +213,12 @@ pub fn do_register_provider(caller: Principal, args: RegisterProviderArgs) -> u6 provider_id } -pub fn do_unregister_provider(caller: Principal, provider_id: u64) -> bool { +pub fn do_unregister_provider(caller: Principal, is_controller: bool, provider_id: u64) -> bool { PROVIDERS.with(|providers| { let mut providers = providers.borrow_mut(); if let Some(provider) = providers.get(&provider_id) { - if provider.owner != caller { - ic_cdk::trap("You are not authorized"); + if !(provider.owner == caller || is_controller) { + ic_cdk::trap("You are not authorized: check provider owner"); } else { log!( INFO, @@ -235,13 +235,13 @@ pub fn do_unregister_provider(caller: Principal, provider_id: u64) -> bool { } /// Changes provider details. The caller must be the owner of the provider. -pub fn do_update_provider(caller: Principal, args: UpdateProviderArgs) { +pub fn do_update_provider(caller: Principal, is_controller: bool, args: UpdateProviderArgs) { PROVIDERS.with(|providers| { let mut providers = providers.borrow_mut(); match providers.get(&args.provider_id) { Some(mut provider) => { - if provider.owner != caller { - ic_cdk::trap("Provider owner != caller"); + if !(provider.owner == caller || is_controller) { + ic_cdk::trap("You are not authorized: check provider owner"); } else { log!(INFO, "[{}] Updating provider: {}", caller, args.provider_id); if let Some(hostname) = args.hostname { diff --git a/tests/tests.rs b/tests/tests.rs index 3fa95d6b..23ac08b7 100644 --- a/tests/tests.rs +++ b/tests/tests.rs @@ -159,12 +159,12 @@ impl EvmRpcSetup { } } - pub fn authorize(&self, principal: &PrincipalId, auth: Auth) { + pub fn authorize(&self, principal: &PrincipalId, auth: Auth) -> bool { self.call_update("authorize", Encode!(&principal.0, &auth).unwrap()) .wait() } - pub fn deauthorize(&self, principal: &PrincipalId, auth: Auth) { + pub fn deauthorize(&self, principal: &PrincipalId, auth: Auth) -> bool { self.call_update("deauthorize", Encode!(&principal.0, &auth).unwrap()) .wait() } @@ -655,7 +655,7 @@ fn should_panic_if_anonymous_register_provider() { } #[test] -#[should_panic(expected = "Provider owner != caller")] +#[should_panic(expected = "You are not authorized")] fn should_panic_if_anonymous_update_provider() { let setup = EvmRpcSetup::new().as_anonymous(); setup.update_provider(UpdateProviderArgs { @@ -707,10 +707,10 @@ fn should_panic_if_anonymous_manage_provider() { } #[test] -#[should_panic(expected = "Provider owner != caller")] +#[should_panic(expected = "You are not authorized: check provider owner")] fn should_panic_if_unauthorized_update_provider() { - // Providers can only be updated by the original owner - let setup = EvmRpcSetup::new().authorize_caller(Auth::RegisterProvider); + // Providers can only be updated by the original owner or canister controller + let setup = EvmRpcSetup::new(); setup.update_provider(UpdateProviderArgs { provider_id: 0, hostname: Some("unauthorized.host".to_string()), @@ -722,33 +722,35 @@ fn should_panic_if_unauthorized_update_provider() { } #[test] -#[should_panic(expected = "You are not authorized")] -fn should_panic_if_unauthorized_unregister_provider() { - // Only the `Manage` authorization may unregister a provider - let setup = EvmRpcSetup::new().authorize_caller(Auth::RegisterProvider); - setup.unregister_provider(0); -} - -#[test] -#[should_panic(expected = "You are not authorized")] -fn should_panic_if_manage_auth_unregister_provider() { - let setup = EvmRpcSetup::new().authorize_caller(Auth::Manage); - setup.unregister_provider(3); +fn should_allow_controller_update_provider() { + let setup = EvmRpcSetup::new().as_controller(); + setup.update_provider(UpdateProviderArgs { + provider_id: 0, + hostname: Some("controller.host".to_string()), + credential_path: None, + credential_headers: None, + cycles_per_call: None, + cycles_per_message_byte: None, + }); } #[test] -#[should_panic(expected = "Provider owner != caller")] +#[should_panic(expected = "You are not authorized: check provider owner")] fn should_panic_if_manage_auth_update_non_owned_provider() { - let setup = EvmRpcSetup::new().authorize_caller(Auth::RegisterProvider); - let provider_id = setup.register_provider(RegisterProviderArgs { - chain_id: 123, - hostname: "example.com".to_string(), - credential_path: "".to_string(), - credential_headers: None, - cycles_per_call: 0, - cycles_per_message_byte: 0, - }); - setup.as_controller().update_provider(UpdateProviderArgs { + let setup = EvmRpcSetup::new().authorize_caller(Auth::Manage); + let provider_id = setup + .clone() + .as_controller() + .authorize_caller(Auth::RegisterProvider) + .register_provider(RegisterProviderArgs { + chain_id: 123, + hostname: "example.com".to_string(), + credential_path: "".to_string(), + credential_headers: None, + cycles_per_call: 0, + cycles_per_message_byte: 0, + }); + setup.update_provider(UpdateProviderArgs { provider_id, hostname: Some("unauthorized.host".to_string()), credential_path: None, @@ -758,6 +760,21 @@ fn should_panic_if_manage_auth_update_non_owned_provider() { }); } +#[test] +#[should_panic(expected = "You are not authorized: check provider owner")] +fn should_panic_if_unauthorized_unregister_provider() { + // Only the `Manage` authorization may unregister a provider + let setup = EvmRpcSetup::new().authorize_caller(Auth::RegisterProvider); + setup.unregister_provider(0); +} + +#[test] +#[should_panic(expected = "You are not authorized: check provider owner")] +fn should_panic_if_manage_auth_unregister_provider() { + let setup = EvmRpcSetup::new().authorize_caller(Auth::Manage); + setup.unregister_provider(3); +} + #[test] fn should_change_provider_owner() { let mut setup = EvmRpcSetup::new().authorize_caller(Auth::RegisterProvider);