Repository: stellar/rs-soroban-env
Pull request #674 (2023-02-07 19:35:34)

Change the interface for the auth testing utility.
### What

Change the auth testing to return all the top-level recordings instead of polling auth manager for their presence.

Also did a passing-by cleanup of the invoker logic in recording mode to ensure we always record the address and only replace it with none in the preflight payloads.

### Why

Improve testing UX

### Known limitations

I'm not introducing the function to return the full tree for now as it's not critical for the first release and the tree representation would be a bit tricky. We can add this as a followup later.



Patch:
diff --git a/soroban-env-host/src/auth.rs b/soroban-env-host/src/auth.rs
index 2dfa6ddf..32c1e54e 100644
--- a/soroban-env-host/src/auth.rs
+++ b/soroban-env-host/src/auth.rs
@@ -269,7 +269,7 @@ impl AuthorizationManager {
 
     // Creates a new recording `AuthorizationManager`.
     // All the authorization requirements will be recorded and can then be
-    // retrieved using `get_recorded_signature_payloads`.
+    // retrieved using `get_recorded_auth_payloads`.
     pub(crate) fn new_recording(budget: Budget) -> Self {
         Self {
             mode: AuthorizationMode::Recording(RecordingAuthInfo {
@@ -509,29 +509,28 @@ impl AuthorizationManager {
         }
     }
 
-    // Verify that the top-level authorization has happened for the given
-    // address and invocation arguments.
-    // This also keeps track of verifications that already happened.
+    // Returns the top-level authorizations that have been recorded for the last
+    // contract invocation.
     #[cfg(any(test, feature = "testutils"))]
-    pub(crate) fn verify_top_authorization(
-        &mut self,
-        address: &ScAddress,
-        contract_id: &Hash,
-        function_name: &Symbol,
-        args: &ScVec,
-    ) -> bool {
-        match &mut self.mode {
+    pub(crate) fn get_recorded_top_authorizations(&self) -> Vec<(ScAddress, Hash, Symbol, ScVec)> {
+        match self.mode {
             AuthorizationMode::Enforcing => {
-                panic!("verifying the authorization is only available for recording-mode auth")
-            }
-            AuthorizationMode::Recording(_) => {
-                for tracker in &mut self.trackers {
-                    if tracker.verify_top_authorization(address, contract_id, function_name, args) {
-                        return true;
-                    }
-                }
-                return false;
+                panic!("get_top_authorizations is only available for recording-mode auth")
             }
+            AuthorizationMode::Recording(_) => self
+                .trackers
+                .iter()
+                .map(|t| {
+                    (
+                        // It is ok to unwrap here, since in recording
+                        // mode address should be always present.
+                        t.address.clone().unwrap(),
+                        t.root_authorized_invocation.contract_id.clone(),
+                        t.root_authorized_invocation.function_name.clone(),
+                        t.root_authorized_invocation.args.clone(),
+                    )
+                })
+                .collect(),
         }
     }
 }
@@ -580,15 +579,11 @@ impl AuthorizationTracker {
         } else {
             false
         };
-        let mut nonce = None;
-
-        let address = if is_invoker {
-            None
+        let nonce = if !is_invoker {
+            Some(host.read_and_consume_nonce(&contract_id, &address)?)
         } else {
-            nonce = Some(host.read_and_consume_nonce(&contract_id, &address)?);
-            Some(address)
+            None
         };
-        let is_invoker = address.is_none();
         // Create the stack of `None` leading to the current invocation to
         // represent invocations that didn't need authorization on behalf of
         // the tracked address.
@@ -596,7 +591,7 @@ impl AuthorizationTracker {
         // Add the id for the current(root) invocation.
         invocation_id_in_call_stack.push(Some(0));
         Ok(Self {
-            address,
+            address: Some(address),
             root_authorized_invocation: AuthorizedInvocation::new_recording(
                 contract_id,
                 function_name,
@@ -606,7 +601,7 @@ impl AuthorizationTracker {
             signature_args: Default::default(),
             is_valid: true,
             authenticated: true,
-            need_nonce: !is_invoker,
+            need_nonce: false,
             is_invoker,
             nonce,
         })
@@ -694,7 +689,11 @@ impl AuthorizationTracker {
     // tracker.
     fn get_recorded_auth_payload(&self) -> Result<RecordedAuthPayload, HostError> {
         Ok(RecordedAuthPayload {
-            address: self.address.clone(),
+            address: if self.is_invoker {
+                None
+            } else {
+                self.address.clone()
+            },
             invocation: self.root_authorized_invocation.to_xdr_non_metered()?,
             nonce: self.nonce,
         })
@@ -871,35 +870,6 @@ impl AuthorizationTracker {
             Err(host.err_general("unexpected missing address to emulate authentication"))
         }
     }
-
-    // Checks whether the provided top-level authorized invocation happened
-    // for this tracker.
-    // This also makes sure double verification of the same invocation is not
-    // possible.
-    #[cfg(any(test, feature = "testutils"))]
-    fn verify_top_authorization(
-        &mut self,
-        address: &ScAddress,
-        contract_id: &Hash,
-        function_name: &Symbol,
-        args: &ScVec,
-    ) -> bool {
-        // The recording invariant is that every recorded authorization is
-        // immediately exhausted, so during the verification we revert the
-        // 'exhausted' flags back to 'false' values in order to prevent
-        // verifying the same authorization twice.
-        if !self.root_authorized_invocation.is_exhausted {
-            return false;
-        }
-        let is_matching = self.address.as_ref().unwrap() == address
-            && &self.root_authorized_invocation.contract_id == contract_id
-            && &self.root_authorized_invocation.function_name == function_name
-            && &self.root_authorized_invocation.args == args;
-        if is_matching {
-            self.root_authorized_invocation.is_exhausted = false;
-        }
-        is_matching
-    }
 }
 
 impl Host {
diff --git a/soroban-env-host/src/host.rs b/soroban-env-host/src/host.rs
index c253bf02..0fc0f655 100644
--- a/soroban-env-host/src/host.rs
+++ b/soroban-env-host/src/host.rs
@@ -1054,23 +1054,22 @@ impl Host {
         self.with_mut_storage(|storage| storage.put(&key, &val, self.as_budget()))
     }
 
+    // Returns the top-level authorizations that have been recorded for the last
+    // contract invocation.
+    // More technically, 'top-level' means that these invocations were the first
+    // in the call tree to have called `require_auth` (i.e. they're not
+    // necessarily invocations of the top-level contract that has been invoked).
     #[cfg(any(test, feature = "testutils"))]
-    pub fn verify_top_authorization(
+    pub fn get_recorded_top_authorizations(
         &self,
-        address: Object,
-        contract_id: Hash,
-        function_name: Symbol,
-        args: Object,
-    ) -> Result<bool, HostError> {
-        let address = self.visit_obj(address, |addr: &ScAddress| Ok(addr.clone()))?;
-        let args = self.call_args_to_scvec(args)?;
+    ) -> Result<Vec<(ScAddress, Hash, Symbol, ScVec)>, HostError> {
         Ok(self
             .0
             .previous_authorization_manager
             .borrow_mut()
             .as_mut()
-            .ok_or(self.err_general("previous invocation is missing - no auth to verify"))?
-            .verify_top_authorization(&address, &contract_id, &function_name, &args))
+            .ok_or(self.err_general("previous invocation is missing - no auth data to get"))?
+            .get_recorded_top_authorizations())
     }
 
     /// Records a `System` contract event. `topics` is expected to be a `SCVec`


Test patch:
diff --git a/soroban-env-host/src/test/token.rs b/soroban-env-host/src/test/token.rs
index 4bb2d93d..7b04299d 100644
--- a/soroban-env-host/src/test/token.rs
+++ b/soroban-env-host/src/test/token.rs
@@ -20,7 +20,7 @@ use crate::{
 };
 use ed25519_dalek::Keypair;
 use soroban_env_common::{
-    xdr::{self, AccountFlags, ScVal, ScVec, Uint256},
+    xdr::{self, AccountFlags, ScAddress, ScVal, ScVec, Uint256},
     xdr::{
         AccountEntry, AccountEntryExt, AccountEntryExtensionV1, AccountEntryExtensionV1Ext,
         AccountEntryExtensionV2, AccountEntryExtensionV2Ext, AccountId, AlphaNum12, AlphaNum4,
@@ -2544,92 +2544,17 @@ fn test_recording_auth_for_token() {
         }]
     );
 
-    // Incorrect address
-    assert!(!test
-        .host
-        .verify_top_authorization(
-            user.address(&test.host).into(),
-            Hash(token.id.to_array().unwrap()),
-            Symbol::from_str("mint"),
-            args.clone().into()
-        )
-        .unwrap());
-
-    // Incorrect contract
-    assert!(!test
-        .host
-        .verify_top_authorization(
-            user.address(&test.host).into(),
-            Hash([1; 32]),
-            Symbol::from_str("mint"),
-            args.clone().into()
-        )
-        .unwrap());
-
-    // Incorrect function
-    assert!(!test
-        .host
-        .verify_top_authorization(
-            admin.address(&test.host).into(),
-            Hash(token.id.to_array().unwrap()),
-            Symbol::from_str("mint2"),
-            args.clone().into()
-        )
-        .unwrap());
-
-    // Incorrect args
-    assert!(!test
-        .host
-        .verify_top_authorization(
-            admin.address(&test.host).into(),
-            Hash(token.id.to_array().unwrap()),
-            Symbol::from_str("mint2"),
-            host_vec![
-                &test.host,
-                admin.address(&test.host),
-                user.address(&test.host),
-                101_i128
-            ]
-            .into()
-        )
-        .unwrap());
-
-    // Incorrect args order
-    assert!(!test
-        .host
-        .verify_top_authorization(
-            admin.address(&test.host).into(),
-            Hash(token.id.to_array().unwrap()),
-            Symbol::from_str("mint2"),
-            host_vec![
-                &test.host,
-                admin.address(&test.host),
-                100_i128,
-                user.address(&test.host),
-            ]
-            .into()
-        )
-        .unwrap());
-
-    // Correct args
-    assert!(test
-        .host
-        .verify_top_authorization(
-            admin.address(&test.host).into(),
-            Hash(token.id.to_array().unwrap()),
-            Symbol::from_str("mint"),
-            args.clone().into()
-        )
-        .unwrap());
-
-    // Correct args again, but the verification is exhausted now
-    assert!(!test
-        .host
-        .verify_top_authorization(
-            admin.address(&test.host).into(),
+    assert_eq!(
+        test.host.get_recorded_top_authorizations().unwrap(),
+        vec![(
+            test.host
+                .visit_obj(admin.address(&test.host).into(), |addr: &ScAddress| Ok(
+                    addr.clone()
+                ))
+                .unwrap(),
             Hash(token.id.to_array().unwrap()),
             Symbol::from_str("mint"),
-            args.clone().into()
-        )
-        .unwrap());
+            test.host.call_args_to_scvec(args.clone().into()).unwrap()
+        )]
+    );
 }