Refactor vault access handling in event command #1478
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1478 +/- ##
===========================================
+ Coverage 73.63% 73.65% +0.01%
===========================================
Files 45 45
Lines 3414 3416 +2
Branches 986 986
===========================================
+ Hits 2514 2516 +2
Misses 701 701
Partials 199 199
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Browser tests are failing consistently across other merged PR's too : https://github.com/postmanlabs/postman-runtime/actions/runs/10701761076/job/29668418235?pr=1452 |
| if (typeof isVaultAccessInScriptsAllowed === 'function') { | ||
| isVaultAccessAllowed = await isVaultAccessInScriptsAllowed(item.id); | ||
| } | ||
| else { |
There was a problem hiding this comment.
We do not need to support this.
| } | ||
| } | ||
| catch (error) { | ||
| console.error(error.message); |
| }); | ||
| }); | ||
|
|
||
| describe('should handle _allowScriptAccess as undefined for backward compatibility', function () { |
| else { | ||
| isVaultAccessAllowed = isVaultAccessInScriptsAllowed; | ||
| } | ||
| if (isVaultAccessAllowed) { |
There was a problem hiding this comment.
why is this part of try catch?
| isVaultAccessInScriptsAllowed = _.get(vaultSecrets, '_.allowScriptAccess'), | ||
| packageResolver = _.get(this, 'options.script.packageResolver'), | ||
| events, | ||
| isVaultAccessAllowed; | ||
|
|
||
| // Explicitly enable tracking for vault secrets here as this will | ||
| // not be sent to sandbox who otherwise takes care of mutation tracking |
There was a problem hiding this comment.
the comment also needs to be moved along with the code
| this.host.on(EXECUTION_VAULT_BASE + executionId, async function (id, cmd, ...args) { | ||
| try { | ||
| if (typeof isVaultAccessInScriptsAllowed === 'function') { | ||
| isVaultAccessAllowed = await isVaultAccessInScriptsAllowed(item.id); |
There was a problem hiding this comment.
| isVaultAccessAllowed = await isVaultAccessInScriptsAllowed(item.id); | |
| isVaultAccessAllowed = Boolean(await _.get(vaultSecrets, '_.allowScriptAccess', _.noop)(item.id)); |
There was a problem hiding this comment.
You can clean up the code above (remove if condition and unused vars) accrodingly.
There was a problem hiding this comment.
Also, we don't need to run the consent check everytime there is get/set/unset on vault secrets. It should just be done for the first time in that execution, unless we are allowing the conset to be changed mid-execution (IMO that would be just confusing for users)?
This PR refactors how vault access is handled in the event command extension:
Renames allowVaultAccess to isVaultAccessInScriptsAllowed for clarity
Moves vault access check and tracking enablement into the vault execution handler
Adds support for isVaultAccessInScriptsAllowed to be either a boolean or an async function
Updates vault access check to use the new isVaultAccessInScriptsAllowed approach