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.

Sign up for GitHub

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

Jump to bottom

fix: ensure non scope caip25 permission property values are preserved #5183

Merged
merged 2 commits into from
Jan 22, 2025
Merged

fix: ensure non scope caip25 permission property values are preserved #5183

merged 2 commits into from
Jan 22, 2025

Conversation

jiexi
Copy link
Contributor

@jiexi jiexi commented Jan 22, 2025

Explanation

References

Fixes a bug in the @metamask/multichain package where the removeScope mutator was dropping necessary properties from the original caveat value which was making the permission fail in the validator in Extension.

Changelog

@metamask/multichain

  • FIXED: Fix removeScope to only modify requiredScopes and optionalScopes values on the caveat where previously it was also dropping sessionProperties and isMultichainOrigin from the final result.

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

@jiexi jiexi requested a review from a team as a code owner January 22, 2025 18:46
@jiexi jiexi mentioned this pull request Jan 22, 2025
Merged
7 tasks
@jiexi jiexi enabled auto-merge (squash) January 22, 2025 19:40
@jiexi jiexi merged commit 61c459c into main Jan 22, 2025
123 checks passed
@jiexi jiexi deleted the jl/fix-caip25-remove-scope-mutator branch January 22, 2025 19:45
@jiexi jiexi mentioned this pull request Jan 22, 2025
Merged
adonesky1 added a commit that referenced this pull request Jan 22, 2025
@adonesky1 @jiexi
Release/288.0.0 (#5184)
83eae03 
## @metamask/multichain
## [3.0.0]

### Add

- Add key Multichain API methods
([#4813](#4813))
- Adds `getInternalScopesObject` and `getSessionScopes` helpers for
transforming between `NormalizedScopesObject` and
`InternalScopesObject`.
- Adds `walletGetSession`, `walletInvokeMethod`, and
`walletRevokeSession` handlers.
- Adds `multichainMethodCallValidatorMiddleware` for validating
Multichain API method params as defined in @metamask/api-specs.
- Adds `MultichainMiddlewareManager` to multiplex a request to other
middleware based on requested scope.
- Adds `MultichainSubscriptionManager` to handle concurrent
subscriptions across multiple scopes.
- Adds `bucketScopes` which groups the scopes in a
`NormalizedScopesObject` based on if the scopes are already supported,
could be supported, or are not supportable.
- Adds `getSupportedScopeObjects` helper for getting only the supported
methods and notifications from each `NormalizedScopeObject` in a
`NormalizedScopesObject`.
- Fixes `removeScope` mutator incorrectly returning malformed CAIP-25
caveat values ([#5183](#5183)).

### Changed
- Bump `@metamask/controller-utils` from `^11.4.4` to `^11.4.5`
([#5012](#5135))
- Bump `@metamask/permission-controller` from `^11.0.4` to `^11.0.5`
([#5012](#5135))
- Bump `@metamask/utils` to `^11.0.1` and `@metamask/rpc-errors` to
`^7.0.2` ([#5080](#5080))

---------

Co-authored-by: jiexi <[email protected]>
@adonesky1 adonesky1 mentioned this pull request Jan 22, 2025
Merged
adonesky1 added a commit that referenced this pull request Jan 22, 2025
@adonesky1
Release/288.0.0 (#5186)
77f5051 
## `@metamask/multichain`

## [2.1.0]

### Added

- Add key Multichain API methods
([#4813](#4813))
- Adds `getInternalScopesObject` and `getSessionScopes` helpers for
transforming between `NormalizedScopesObject` and
`InternalScopesObject`.
- Adds handlers for `wallet_getSession`, `wallet_invokeMethod`, and
`wallet_revokeSession` methods.
- Adds `multichainMethodCallValidatorMiddleware` for validating
Multichain API method params as defined in @metamask/api-specs.
- Adds `MultichainMiddlewareManager` to multiplex a request to other
middleware based on requested scope.
- Adds `MultichainSubscriptionManager` to handle concurrent
subscriptions across multiple scopes.
- Adds `bucketScopes` which groups the scopes in a
`NormalizedScopesObject` based on if the scopes are already supported,
could be supported, or are not supportable.
- Adds `getSupportedScopeObjects` helper for getting only the supported
methods and notifications from each `NormalizedScopeObject` in a
`NormalizedScopesObject`.

### Changed

- Bump `@metamask/controller-utils` from `^11.4.4` to `^11.4.5`
([#5012](#5135))
- Bump `@metamask/permission-controller` from `^11.0.4` to `^11.0.5`
([#5012](#5135))
- Bump `@metamask/utils` to `^11.0.1` and `@metamask/rpc-errors` to
`^7.0.2` ([#5080](#5080))

### Fixed

- Fixes `removeScope` mutator incorrectly returning malformed CAIP-25
caveat values ([#5183](#5183)).
github-merge-queue bot pushed a commit to MetaMask/metamask-extension that referenced this pull request Jan 23, 2025
@jiexi @adonesky1
fix: fix permissions not correctly being updated when all network cli…
98bc7c7 
…ents are removed for a chainId (#29855)

<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

Fixes a bug where permissions are not being updated when all network
clients for a chainId are removed. This results in permittedChains
permissions referencing chainIds that are no longer supported and makes
it so that the existing permittedChains permissions cannot be modified
in any way until either the permission is revoked entirely or the
missing network is readded.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/29855?quickstart=1)

## **Related issues**

Fixes: MetaMask/MetaMask-planning#3735

**Requires:** MetaMask/core#5183

## **Manual testing steps**

1. Add a new RPC endpoint for a chainId you do not have network clients
for already
2. Add another different RPC endpoint for that same chainId above
3. Permit a dapp to access that chainId
4. Check that you can modify the permitted chains vs the wallet UI
5. Make sure that calling `wallet_getPermissions` shows the chainId in
the `endowment:permitted-chains` permission
6. Remove one RPC endpoint for that chainId
7. Check that you can modify the permitted chains vs the wallet UI
8. Make sure that calling `wallet_getPermissions` shows the chainId in
the `endowment:permitted-chains` permission
9. Remove the chainId entirely
7. Check that you can modify the permitted chains vs the wallet UI
8. Make sure that calling `wallet_getPermissions` shows that the chainId
is no longer in the `endowment:permitted-chains` permission

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.

---------

Co-authored-by: Alex Donesky <[email protected]>
github-merge-queue bot pushed a commit to MetaMask/metamask-extension that referenced this pull request Jan 23, 2025
@jiexi @adonesky1
fix: fix permissions not correctly being updated when all network cli…
305ea7e 
…ents are removed for a chainId (#29855)

<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

Fixes a bug where permissions are not being updated when all network
clients for a chainId are removed. This results in permittedChains
permissions referencing chainIds that are no longer supported and makes
it so that the existing permittedChains permissions cannot be modified
in any way until either the permission is revoked entirely or the
missing network is readded.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/29855?quickstart=1)

## **Related issues**

Fixes: MetaMask/MetaMask-planning#3735

**Requires:** MetaMask/core#5183

## **Manual testing steps**

1. Add a new RPC endpoint for a chainId you do not have network clients
for already
2. Add another different RPC endpoint for that same chainId above
3. Permit a dapp to access that chainId
4. Check that you can modify the permitted chains vs the wallet UI
5. Make sure that calling `wallet_getPermissions` shows the chainId in
the `endowment:permitted-chains` permission
6. Remove one RPC endpoint for that chainId
7. Check that you can modify the permitted chains vs the wallet UI
8. Make sure that calling `wallet_getPermissions` shows the chainId in
the `endowment:permitted-chains` permission
9. Remove the chainId entirely
7. Check that you can modify the permitted chains vs the wallet UI
8. Make sure that calling `wallet_getPermissions` shows that the chainId
is no longer in the `endowment:permitted-chains` permission

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.

---------

Co-authored-by: Alex Donesky <[email protected]>
zone-live pushed a commit that referenced this pull request Jan 27, 2025
@jiexi @zone-live
fix: ensure non scope caip25 permission property values are preserved (…
1291bb2 
…#5183)

## Explanation

<!--
Thanks for your contribution! Take a moment to answer these questions so
that reviewers have the information they need to properly understand
your changes:

* What is the current state of things and why does it need to change?
* What is the solution your changes offer and how does it work?
* Are there any changes whose purpose might not obvious to those
unfamiliar with the domain?
* If your primary goal was to update one package but you found you had
to update another one along the way, why did you do so?
* If you had to upgrade a dependency, why did you do so?
-->

## References

Fixes a bug in the `@metamask/multichain` package where the
`removeScope` mutator was dropping necessary properties from the
original caveat value which was making the permission fail in the
validator in Extension.

## Changelog

<!--
If you're making any consumer-facing changes, list those changes here as
if you were updating a changelog, using the template below as a guide.

(CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or
FIXED. For security-related issues, follow the Security Advisory
process.)

Please take care to name the exact pieces of the API you've added or
changed (e.g. types, interfaces, functions, or methods).

If there are any breaking changes, make sure to offer a solution for
consumers to follow once they upgrade to the changes.

Finally, if you're only making changes to development scripts or tests,
you may replace the template below with "None".
-->

### `@metamask/multichain`

- **FIXED**: Fix `removeScope` to only modify `requiredScopes` and
`optionalScopes` values on the caveat where previously it was also
dropping `sessionProperties` and `isMultichainOrigin` from the final
result.

## 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
zone-live pushed a commit that referenced this pull request Jan 27, 2025
@adonesky1 @jiexi @zone-live
Release/288.0.0 (#5184)
7c22464 
## @metamask/multichain
## [3.0.0]

### Add

- Add key Multichain API methods
([#4813](#4813))
- Adds `getInternalScopesObject` and `getSessionScopes` helpers for
transforming between `NormalizedScopesObject` and
`InternalScopesObject`.
- Adds `walletGetSession`, `walletInvokeMethod`, and
`walletRevokeSession` handlers.
- Adds `multichainMethodCallValidatorMiddleware` for validating
Multichain API method params as defined in @metamask/api-specs.
- Adds `MultichainMiddlewareManager` to multiplex a request to other
middleware based on requested scope.
- Adds `MultichainSubscriptionManager` to handle concurrent
subscriptions across multiple scopes.
- Adds `bucketScopes` which groups the scopes in a
`NormalizedScopesObject` based on if the scopes are already supported,
could be supported, or are not supportable.
- Adds `getSupportedScopeObjects` helper for getting only the supported
methods and notifications from each `NormalizedScopeObject` in a
`NormalizedScopesObject`.
- Fixes `removeScope` mutator incorrectly returning malformed CAIP-25
caveat values ([#5183](#5183)).

### Changed
- Bump `@metamask/controller-utils` from `^11.4.4` to `^11.4.5`
([#5012](#5135))
- Bump `@metamask/permission-controller` from `^11.0.4` to `^11.0.5`
([#5012](#5135))
- Bump `@metamask/utils` to `^11.0.1` and `@metamask/rpc-errors` to
`^7.0.2` ([#5080](#5080))

---------

Co-authored-by: jiexi <[email protected]>
zone-live pushed a commit that referenced this pull request Jan 27, 2025
@adonesky1 @zone-live
Release/288.0.0 (#5186)
cba7be5 
## `@metamask/multichain`

## [2.1.0]

### Added

- Add key Multichain API methods
([#4813](#4813))
- Adds `getInternalScopesObject` and `getSessionScopes` helpers for
transforming between `NormalizedScopesObject` and
`InternalScopesObject`.
- Adds handlers for `wallet_getSession`, `wallet_invokeMethod`, and
`wallet_revokeSession` methods.
- Adds `multichainMethodCallValidatorMiddleware` for validating
Multichain API method params as defined in @metamask/api-specs.
- Adds `MultichainMiddlewareManager` to multiplex a request to other
middleware based on requested scope.
- Adds `MultichainSubscriptionManager` to handle concurrent
subscriptions across multiple scopes.
- Adds `bucketScopes` which groups the scopes in a
`NormalizedScopesObject` based on if the scopes are already supported,
could be supported, or are not supportable.
- Adds `getSupportedScopeObjects` helper for getting only the supported
methods and notifications from each `NormalizedScopeObject` in a
`NormalizedScopesObject`.

### Changed

- Bump `@metamask/controller-utils` from `^11.4.4` to `^11.4.5`
([#5012](#5135))
- Bump `@metamask/permission-controller` from `^11.0.4` to `^11.0.5`
([#5012](#5135))
- Bump `@metamask/utils` to `^11.0.1` and `@metamask/rpc-errors` to
`^7.0.2` ([#5080](#5080))

### Fixed

- Fixes `removeScope` mutator incorrectly returning malformed CAIP-25
caveat values ([#5183](#5183)).
matteoscurati pushed a commit to MetaMask/metamask-extension that referenced this pull request Jan 27, 2025
@jiexi @adonesky1 @matteoscurati
fix: fix permissions not correctly being updated when all network cli…
dba0612 
…ents are removed for a chainId (#29855)

<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

Fixes a bug where permissions are not being updated when all network
clients for a chainId are removed. This results in permittedChains
permissions referencing chainIds that are no longer supported and makes
it so that the existing permittedChains permissions cannot be modified
in any way until either the permission is revoked entirely or the
missing network is readded.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/29855?quickstart=1)

## **Related issues**

Fixes: MetaMask/MetaMask-planning#3735

**Requires:** MetaMask/core#5183

## **Manual testing steps**

1. Add a new RPC endpoint for a chainId you do not have network clients
for already
2. Add another different RPC endpoint for that same chainId above
3. Permit a dapp to access that chainId
4. Check that you can modify the permitted chains vs the wallet UI
5. Make sure that calling `wallet_getPermissions` shows the chainId in
the `endowment:permitted-chains` permission
6. Remove one RPC endpoint for that chainId
7. Check that you can modify the permitted chains vs the wallet UI
8. Make sure that calling `wallet_getPermissions` shows the chainId in
the `endowment:permitted-chains` permission
9. Remove the chainId entirely
7. Check that you can modify the permitted chains vs the wallet UI
8. Make sure that calling `wallet_getPermissions` shows that the chainId
is no longer in the `endowment:permitted-chains` permission

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.

---------

Co-authored-by: Alex Donesky <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adonesky1 adonesky1 adonesky1 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jiexi @adonesky1