Preserve the recording auth settings when temporarily switching to recording auth. #1412
Conversation
…cording auth. Previously this didn't matter as we've only switched for the contract instantiation, but now the contract instantiation may run custom logic due to constructors, so we need to preserve the settings (currently only dissallowing non-root auth).
| @@ -965,7 +965,9 @@ impl Env { | |||
| .unwrap(); | |||
|
|
|||
| let prev_auth_manager = self.env_impl.snapshot_auth_manager().unwrap(); | |||
| self.env_impl.switch_to_recording_auth(true).unwrap(); | |||
| self.env_impl | |||
| .switch_to_recording_auth_inherited_from_snapshot(&prev_auth_manager) | |||
There was a problem hiding this comment.
What does it mean that it's inherited? Or, why do we need it to be inherited? In this instance we're intending to invoke the SAC contract without any existing auth setup bleeding into it, because it's its own top-level invocation.
There was a problem hiding this comment.
The settings of the recording mode are inherited (currently only non-root auth setting). Conceptually these probably could have been an env property and not the auth manager property, but that would be a bigger change. While SAC registration currently doesn't care about this setting, I think it's generally more appropriate to just use the same pattern everywhere.
There was a problem hiding this comment.
I think the auth setting being inherited is a bug, but if it's impractical to fix that bug, then this does make the presence of the bug clearer.
There was a problem hiding this comment.
Wait wait, so does the old switch_to_recording_auth(true) cause inheritance too? Are these two functions doing the same thing? Because if switch_to_recording_auth(true) doesn't inherit, that seems like the better thing to happen here.
There was a problem hiding this comment.
I think the auth setting being inherited is a bug,
It's not a bug, it's the intended behavior. Users should be able to generically change how the recording auth operates. Otherwise we just have unpreventable errors due to non-root auth in constructor.
Wait wait, so does the old switch_to_recording_auth(true) cause inheritance too?
No, it overrides any setting the user had on the setting, which is a bug. Again, we can't know what is the user intention here and we so we shouldn't override it.
There was a problem hiding this comment.
https://discord.com/channels/897514728459468821/1315694921054945340 for some context
There was a problem hiding this comment.
Thanks for the context. I saw this comment in the discussion:
basically if you call
register, we accidentally override your setting for allowing non-root auth.
Ref: https://discord.com/channels/897514728459468821/1315694921054945340/1316177239477456969
If I understand correctly:
- 1️⃣ The developer configured non-root auth.
- 2️⃣ They then called register, which:
- 3️⃣ took a snapshot of auth
- 4️⃣ switched to recording auth
- 5️⃣ switched back to the snapshot
Is it correct that this PR is fixing the bug by making 4️⃣ keep the non-root auth so that while auth is snapshotted, cleared, and then restored, some auth config is not cleared then restored, it just persists throughout the process. Is that correct?
Imo we should be fixing the issue in 3️⃣ and 5️⃣. The auth snapshot is supposed to snapshot and allow restoring the full auth state, so that steps in between, i.e. 4️⃣, can work with a completely neutral state that's unaffected by any caller configuration.
Why fix it at 4️⃣? Is it more practical to fix it at 4️⃣?
There was a problem hiding this comment.
Is it correct that this PR is fixing the bug by making 4️⃣ keep the non-root auth so that while auth is snapshotted,
The bug fix is to allow non-root auth specifically during step 4, because step 4 requires non-root auth to be allowed. The only way to allow it (API-wise) as of today is via env.mock_all_auths_allowing_non_root_auth() and it is expected by the user that indeed all the auths will be mocked allowing non-root auth (which isn't the case currently).
work with a completely neutral state that's unaffected by any caller configuration.
But the issue that the state should not be neutral; it must inherit the general recording mode settings that really have nothing to do with any instance of the auth manager (i.e. whether non-root auth is allowed in general). There is really nothing to fix about 3 and 5, these work as expected. Basically, as I've mentioned above, the design issue here is that non-root auth flag should be a separate env-level setting in order to configure calls that are made through internal machinery (like constructor call in this case). This PR is just a shortcut for achieving that behavior.
There was a problem hiding this comment.
I think implementation decisions (e.g. auth manager's scope) are influencing the API.
If a user is using Env without non-root auth enabled, but we have an Env function that needs to temporarily enable it, perform some custom auth, then disable it again, is that possible with the direction of this fix?
What
Preserve the recording auth settings when temporarily switching to recording auth.
Why
Previously this didn't matter as we've only switched for the contract instantiation, but now the contract instantiation may run custom logic due to constructors, so we need to preserve the settings (currently only disallowing non-root auth).
Known limitations
N/A