fix: account for caveats in permissions difference calculation #2345
Conversation
FrederikBolding
changed the title
| return Object.entries(permissionsA).reduce< | ||
| Record<string, Pick<PermissionConstraint, 'caveats'>> | ||
| >((acc, [key, value]) => { | ||
| const isIncluded = key in permissionsB; |
There was a problem hiding this comment.
| const isIncluded = key in permissionsB; | |
| const isIncluded = hasProperty(permissionsB, key); |
There was a problem hiding this comment.
I just realized: Ditto for setDiff. Maybe best to ignore in this PR.
| if ( | ||
| !isIncluded || | ||
| (isIncluded && | ||
| !deepEqual(value.caveats ?? [], permissionsB[key].caveats ?? [])) |
There was a problem hiding this comment.
Just a note here, for my own benefit as much as anyone else:
fast-deep-equal is sensitive to array element order:
> const deepEqual = require('fast-deep-equal')
undefined
> deepEqual([1, 2, 3], [1, 2, 3])
true
> deepEqual([1, 2, 3], [3, 2, 1])
false
I don't believe that this is documented or enforced anywhere, but I believe that element order should not be a consideration for caveat identity. In practice, I think it probably is, just like it will be here.
This problem should probably be solved in the PermissionController itself, where we'd want to add some kind of logic that always treats caveat value arrays as sets.
For present purposes, I don't think it matters, because we'll just revoke "identical" permissions only to immediately add them back.
The permission diff calculation was incorrect when considering caveats.
Because of this, changes to caveats were not always correctly applied when updating a Snap.
One specific instance of this was updating a Snap with:
to
This problem was fixed by introducing
permissionsDiffwhich follows the implementation ofsetDiffexcept for the additional condition that when the permission is included in both permission sets, it is subtracted if the caveats differ.