From d8bd392fcfc1fb139048ddb138b5c133af161650 Mon Sep 17 00:00:00 2001 From: Dmytro Kozhevin Date: Tue, 22 Aug 2023 13:53:18 -0400 Subject: [PATCH] Provide a way to allow non-root auth in recording mode. (#1059) ### What Provide a way to allow non-root auth in recording mode. This is intentionally done via a separate helper with lengthy name and not a `mock_all_auths` argument - this is intended to be an option for a few 'exotic' contracts that do non-atomic bundling and majority of the users shouldn't ever need that. Resolves https://github.com/stellar/rs-soroban-sdk/issues/1058 ### Why Recording auth now doesn't allow non-root `require_auth` by default in order to prevent the potential user footguns. This PR brings back the old behavior where non-root auth is allowed. ### Known limitations N/A Co-authored-by: Siddharth Suresh --- soroban-sdk-macros/src/derive_client.rs | 38 ++++++++++++++++- soroban-sdk/src/env.rs | 55 +++++++++++++++++++++++++ soroban-sdk/src/tests/token_client.rs | 12 +++++- 3 files changed, 101 insertions(+), 4 deletions(-) diff --git a/soroban-sdk-macros/src/derive_client.rs b/soroban-sdk-macros/src/derive_client.rs index 05443eca3..7ecc7b252 100644 --- a/soroban-sdk-macros/src/derive_client.rs +++ b/soroban-sdk-macros/src/derive_client.rs @@ -26,6 +26,9 @@ pub fn derive_client_type(crate_path: &Path, ty: &str, name: &str) -> TokenStrea #[doc(hidden)] #[cfg(any(test, feature = "testutils"))] mock_all_auths: bool, + #[doc(hidden)] + #[cfg(any(test, feature = "testutils"))] + allow_non_root_auth: bool, } impl<'a> #client_ident<'a> { @@ -41,6 +44,8 @@ pub fn derive_client_type(crate_path: &Path, ty: &str, name: &str) -> TokenStrea mock_auths: None, #[cfg(any(test, feature = "testutils"))] mock_all_auths: false, + #[cfg(any(test, feature = "testutils"))] + allow_non_root_auth: false, } } @@ -60,6 +65,7 @@ pub fn derive_client_type(crate_path: &Path, ty: &str, name: &str) -> TokenStrea set_auths: Some(auths), mock_auths: self.mock_auths.clone(), mock_all_auths: false, + allow_non_root_auth: false, } } @@ -76,6 +82,7 @@ pub fn derive_client_type(crate_path: &Path, ty: &str, name: &str) -> TokenStrea set_auths: self.set_auths.clone(), mock_auths: Some(mock_auths), mock_all_auths: false, + allow_non_root_auth: false, } } @@ -83,7 +90,8 @@ pub fn derive_client_type(crate_path: &Path, ty: &str, name: &str) -> TokenStrea /// `Address::require_auth_for_args` functions in invoked contracts, /// having them succeed as if authorization was provided. /// - /// See `soroban_sdk::Env::set_auths` for more details and examples. + /// See `soroban_sdk::Env::mock_all_auths` for more details and + /// examples. #[cfg(any(test, feature = "testutils"))] pub fn mock_all_auths(&self) -> Self { Self { @@ -92,6 +100,28 @@ pub fn derive_client_type(crate_path: &Path, ty: &str, name: &str) -> TokenStrea set_auths: None, mock_auths: None, mock_all_auths: true, + allow_non_root_auth: false, + } + } + + /// A version of `mock_all_auths` that allows authorizations that + /// are not present in the root invocation. + /// + /// Refer to `mock_all_auths` documentation for details and + /// prefer using `mock_all_auths` unless non-root authorization is + /// required. + /// + /// See `soroban_sdk::Env::mock_all_auths_allowing_non_root_auth` + /// for more details and examples. + #[cfg(any(test, feature = "testutils"))] + pub fn mock_all_auths_allowing_non_root_auth(&self) -> Self { + Self { + env: self.env.clone(), + address: self.address.clone(), + set_auths: None, + mock_auths: None, + mock_all_auths: true, + allow_non_root_auth: true, } } } @@ -163,7 +193,11 @@ pub fn derive_client_impl(crate_path: &Path, name: &str, fns: &[syn_ext::Fn]) -> self.env.mock_auths(mock_auths); } if self.mock_all_auths { - self.env.mock_all_auths(); + if self.allow_non_root_auth { + self.env.mock_all_auths_allowing_non_root_auth(); + } else { + self.env.mock_all_auths(); + } } } use #crate_path::{IntoVal,FromVal}; diff --git a/soroban-sdk/src/env.rs b/soroban-sdk/src/env.rs index b15e38f09..db09c3fc2 100644 --- a/soroban-sdk/src/env.rs +++ b/soroban-sdk/src/env.rs @@ -887,6 +887,61 @@ impl Env { self.env_impl.switch_to_recording_auth(true).unwrap(); } + /// A version of `mock_all_auths` that allows authorizations that are not + /// present in the root invocation. + /// + /// Refer to `mock_all_auths` documentation for general information and + /// prefer using `mock_all_auths` unless non-root authorization is required. + /// + /// The only difference from `mock_all_auths` is that this won't return an + /// error when `require_auth` hasn't been called in the root invocation for + /// any given address. This is useful to test contracts that bundle calls to + /// another contract without atomicity requirements (i.e. any contract call + /// can be frontrun). + /// + /// ### Examples + /// ``` + /// use soroban_sdk::{contract, contractimpl, Env, Address, testutils::Address as _}; + /// + /// #[contract] + /// pub struct ContractA; + /// + /// #[contractimpl] + /// impl ContractA { + /// pub fn do_auth(env: Env, addr: Address) { + /// addr.require_auth(); + /// } + /// } + /// #[contract] + /// pub struct ContractB; + /// + /// #[contractimpl] + /// impl ContractB { + /// pub fn call_a(env: Env, contract_a: Address, addr: Address) { + /// // Notice there is no `require_auth` call here. + /// ContractAClient::new(&env, &contract_a).do_auth(&addr); + /// } + /// } + /// #[test] + /// fn test() { + /// # } + /// # fn main() { + /// let env = Env::default(); + /// let contract_a = env.register_contract(None, ContractA); + /// let contract_b = env.register_contract(None, ContractB); + /// // The regular `env.mock_all_auths()` would result in the call + /// // failure. + /// env.mock_all_auths_allowing_non_root_auth(); + /// + /// let client = ContractBClient::new(&env, &contract_b); + /// let addr = Address::random(&env); + /// client.call_a(&contract_a, &addr); + /// } + /// ``` + pub fn mock_all_auths_allowing_non_root_auth(&self) { + self.env_impl.switch_to_recording_auth(false).unwrap(); + } + /// Returns a list of authorization trees that were seen during the last /// contract or authorized host function invocation. /// diff --git a/soroban-sdk/src/tests/token_client.rs b/soroban-sdk/src/tests/token_client.rs index 6c02dfd7a..0a5caba22 100644 --- a/soroban-sdk/src/tests/token_client.rs +++ b/soroban-sdk/src/tests/token_client.rs @@ -42,7 +42,6 @@ impl TestContract { } #[test] -#[should_panic] // FIXME: this is not supposed to panic; it does due to auth failure, dmkoz to fix. fn test_mock_all_auth() { extern crate std; @@ -60,7 +59,16 @@ fn test_mock_all_auth() { let from = Address::random(&env); let spender = Address::random(&env); - client.mock_all_auths().approve(&from, &spender, &20, &200); + // `TestContract` doesn't call `require_auth` before calling into token, + // thus we need to allow non-root auth and regular `mock_all_auths` will + // fail. + assert!(client + .mock_all_auths() + .try_approve(&from, &spender, &20, &200) + .is_err()); + client + .mock_all_auths_allowing_non_root_auth() + .approve(&from, &spender, &20, &200); assert_eq!( env.auths(),