Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(permission-controller): Rationalize permission and caveat validation #5062

Merged
merged 2 commits into from
Dec 12, 2024

Conversation

rekmarks
Copy link
Member

@rekmarks rekmarks commented Dec 12, 2024

Explanation

Via discussion during MetaMask/metamask-extension#27847, it became apparent that there was confusion re: the purpose of permission vs. caveat validators and when to use one vs. the other. This has now been properly documented in multiple places in the permission controller package.

In addition, the permission controller implementation contained some irregularities regarding its own permission validation logic. This surfaced in the form of redundant permission and caveat validator calls, both of which have now been removed. (In addition, one overly complicated "optimization" to avoid redundant caveat validator calls has been removed.)

Reviewers: although I doubt it's a problem, we should take care to ensure that we weren't relying on the old behavior in any of our existing caveat or permission specifications.

References

Changelog

@metamask/permission-controller

  • CHANGED: Remove redundant caveat validator calls
    • In some cases, caveats were being validated multiple times or without the possibility of having changed.
    • The intended purpose of permission and caveat validators has also been documented. See ARCHITECTURE.md.

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate
  • I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

Sorry, something went wrong.

Comment on lines -1782 to -1786

// We do not need to validate caveats in this case, because the plain
// permission constructor function does not modify the caveats, which
// were already validated by `constructCaveats` above.
performCaveatValidation = false;
Copy link
Member Author

@rekmarks rekmarks Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're all trying to find the guy who introduced this futile and brittle "optimization".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Verified

This commit was signed with the committer’s verified signature.
rekmarks Erik Marks
…lidation
@rekmarks rekmarks force-pushed the rekm/clarify-permission-caveat-validation branch from 8d7dbed to 2422ce2 Compare December 12, 2024 01:36
@rekmarks rekmarks marked this pull request as ready for review December 12, 2024 01:45
@rekmarks rekmarks requested review from a team as code owners December 12, 2024 01:45
Copy link
Member

@FrederikBolding FrederikBolding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - have verified that Snaps doesn't use permission validators to validate caveat contents too.

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@rekmarks rekmarks enabled auto-merge (squash) December 12, 2024 17:55

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@rekmarks rekmarks disabled auto-merge December 12, 2024 17:56
@rekmarks rekmarks enabled auto-merge (squash) December 12, 2024 17:58
@rekmarks rekmarks merged commit ed8677e into main Dec 12, 2024
120 checks passed
@rekmarks rekmarks deleted the rekm/clarify-permission-caveat-validation branch December 12, 2024 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants