Skip to content

Commit 97a5966

Browse files
ffmcgee725ffmcgee725
authored
feat: add wallet_revokePermissions rpc call (#14091)
<!-- 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** `wallet_revokePermissions` has been live on extension for ~1 year. We did not add it to Mobile at the time because of controllers versions being behind. We are adding it now. <!-- Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions: 1. What is the reason for the change? 2. What is the improvement/solution? --> ## **Related issues** Fixes: MetaMask/MetaMask-planning#4426 ## **Manual testing steps** 1. Go to a dapp (example used in this case was https://app.uniswap.org) 2. Connect wallet to dapp via dapp UI 3. Disconnect wallet from dapp via dapp UI 4. Make sure permissions have been revoked (either through mobile wallet UI or console sending a [wallet_getPermissions](https://docs.metamask.io/wallet/reference/json-rpc-methods/wallet_getpermissions/) request) ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** https://github.com/user-attachments/assets/a27be969-2b40-4ab1-8557-ffeea55f2df0 ### **After** https://github.com/user-attachments/assets/cff4d39e-3868-41db-864a-8ff60ec9791b ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [x] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [x] 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.
1 parent 6603207 commit 97a5966

10 files changed

+216
-20
lines changed

Diff for: ‎app/core/Permissions/specifications.js

+1
Original file line numberDiff line numberDiff line change
@@ -383,6 +383,7 @@ export const unrestrictedMethods = Object.freeze([
383383
// Define unrestricted methods below to bypass PermissionController. These are eventually handled by RPCMethodMiddleware (User facing RPC methods)
384384
'wallet_getPermissions',
385385
'wallet_requestPermissions',
386+
'wallet_revokePermissions',
386387
'eth_getTransactionByHash',
387388
'eth_getTransactionByBlockHashAndIndex',
388389
'eth_getTransactionByBlockNumberAndIndex',

Diff for: ‎app/core/RPCMethods/RPCMethodMiddleware.test.ts

+133-4
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@ jest.mock('../Engine', () => ({
5757
PermissionController: {
5858
requestPermissions: jest.fn(),
5959
getPermissions: jest.fn(),
60+
revokePermissions: jest.fn(),
6061
},
6162
NetworkController: {
6263
getNetworkClientById: () => ({
@@ -578,6 +579,133 @@ describe('getRpcMethodMiddleware', () => {
578579
});
579580
}
580581

582+
describe('wallet_revokePermissions', () => {
583+
it('revokes eth_accounts and endowment:permitted-chains permissions if eth_accounts permission key is passed', async () => {
584+
const hostname = 'example.metamask.io';
585+
const middleware = getRpcMethodMiddleware({
586+
...getMinimalOptions(),
587+
hostname,
588+
});
589+
const request = {
590+
jsonrpc,
591+
id: 1,
592+
method: 'wallet_revokePermissions',
593+
params: [{ eth_accounts: {} }],
594+
};
595+
await callMiddleware({ middleware, request });
596+
expect(
597+
MockEngine.context.PermissionController.revokePermissions,
598+
).toHaveBeenCalledWith({
599+
[hostname]: ['eth_accounts', 'endowment:permitted-chains'],
600+
});
601+
});
602+
603+
it('revokes eth_accounts and endowment:permitted-chains permissions if endowment:permitted-chains permission key is passed', async () => {
604+
const hostname = 'example.metamask.io';
605+
const middleware = getRpcMethodMiddleware({
606+
...getMinimalOptions(),
607+
hostname,
608+
});
609+
const request = {
610+
jsonrpc,
611+
id: 1,
612+
method: 'wallet_revokePermissions',
613+
params: [{ 'endowment:permitted-chains': {} }],
614+
};
615+
await callMiddleware({ middleware, request });
616+
expect(
617+
MockEngine.context.PermissionController.revokePermissions,
618+
).toHaveBeenCalledWith({
619+
[hostname]: ['eth_accounts', 'endowment:permitted-chains'],
620+
});
621+
});
622+
623+
it('revokes eth_accounts and endowment:permitted-chains permissions if both permission keys are passed', async () => {
624+
const hostname = 'example.metamask.io';
625+
const middleware = getRpcMethodMiddleware({
626+
...getMinimalOptions(),
627+
hostname,
628+
});
629+
const request = {
630+
jsonrpc,
631+
id: 1,
632+
method: 'wallet_revokePermissions',
633+
params: [{ eth_accounts: {}, 'endowment:permitted-chains': {} }],
634+
};
635+
await callMiddleware({ middleware, request });
636+
expect(
637+
MockEngine.context.PermissionController.revokePermissions,
638+
).toHaveBeenCalledWith({
639+
[hostname]: ['eth_accounts', 'endowment:permitted-chains'],
640+
});
641+
});
642+
643+
it('revokes eth_accounts, endowment:permitted-chains, and other permissions, either is passed alongside other permissions', async () => {
644+
const hostname = 'example.metamask.io';
645+
const middleware = getRpcMethodMiddleware({
646+
...getMinimalOptions(),
647+
hostname,
648+
});
649+
const request = {
650+
jsonrpc,
651+
id: 1,
652+
method: 'wallet_revokePermissions',
653+
params: [{ 'endowment:permitted-chains': {}, a: {}, b: {} }],
654+
};
655+
await callMiddleware({ middleware, request });
656+
expect(
657+
MockEngine.context.PermissionController.revokePermissions,
658+
).toHaveBeenCalledWith({
659+
[hostname]: ['eth_accounts', 'endowment:permitted-chains', 'a', 'b'],
660+
});
661+
});
662+
663+
it('will revoke other permissions if neither eth_accounts or endowment:permitted-chains keys are passed', async () => {
664+
const hostname = 'example.metamask.io';
665+
const middleware = getRpcMethodMiddleware({
666+
...getMinimalOptions(),
667+
hostname,
668+
});
669+
const request = {
670+
jsonrpc,
671+
id: 1,
672+
method: 'wallet_revokePermissions',
673+
params: [{ a: {}, b: {} }],
674+
};
675+
await callMiddleware({ middleware, request });
676+
expect(
677+
MockEngine.context.PermissionController.revokePermissions,
678+
).toHaveBeenCalledWith({
679+
[hostname]: ['a', 'b'],
680+
});
681+
});
682+
683+
it('returns null result if PermissionController throws an error', async () => {
684+
MockEngine.context.PermissionController.revokePermissions.mockImplementation(
685+
() => {
686+
throw new Error('permission error');
687+
},
688+
);
689+
const hostname = 'example.metamask.io';
690+
const middleware = getRpcMethodMiddleware({
691+
...getMinimalOptions(),
692+
hostname,
693+
});
694+
const request = {
695+
jsonrpc,
696+
id: 1,
697+
method: 'wallet_revokePermissions',
698+
params: [{ eth_accounts: {} }],
699+
};
700+
701+
expect(await callMiddleware({ middleware, request })).toEqual({
702+
result: null,
703+
jsonrpc,
704+
id: 1,
705+
});
706+
});
707+
});
708+
581709
describe('wallet_requestPermissions', () => {
582710
it('can requestPermissions for eth_accounts', async () => {
583711
const mockOrigin = 'example.metamask.io';
@@ -1124,10 +1252,11 @@ describe('getRpcMethodMiddleware', () => {
11241252
expect((response as JsonRpcFailure).error.message).toBe(
11251253
expectedError.message,
11261254
);
1127-
// eslint-disable-next-line @typescript-eslint/no-explicit-any
1128-
expect(((response as JsonRpcFailure).error as JsonRpcError<any>).data.cause.message).toBe(
1129-
expectedError.message,
1130-
);
1255+
expect(
1256+
//@ts-expect-error {JsonRpcError} will be fixed by a future bump. [Reference](https://github.com/MetaMask/metamask-mobile/pull/14091/files#r2009831015)
1257+
((response as JsonRpcFailure).error as JsonRpcError<unknown>).data.cause
1258+
.message,
1259+
).toBe(expectedError.message);
11311260
});
11321261

11331262
it('returns a JSON-RPC error if an error is thrown after approval', async () => {

Diff for: ‎app/core/RPCMethods/RPCMethodMiddleware.ts

+52-2
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ import {
4848
MessageParamsTyped,
4949
SignatureController,
5050
} from '@metamask/signature-controller';
51+
import { PermissionKeys } from '../Permissions/specifications.js';
5152

5253
// TODO: Replace "any" with type
5354
// eslint-disable-next-line @typescript-eslint/no-explicit-any
@@ -397,12 +398,61 @@ export const getRpcMethodMiddleware = ({
397398
return responseData;
398399
};
399400

400-
const [requestPermissionsHandler, getPermissionsHandler] =
401-
permissionRpcMethods.handlers;
401+
const [
402+
requestPermissionsHandler,
403+
getPermissionsHandler,
404+
revokePermissionsHandler,
405+
] = permissionRpcMethods.handlers;
402406

403407
// TODO: Replace "any" with type
404408
// eslint-disable-next-line @typescript-eslint/no-explicit-any
405409
const rpcMethods: any = {
410+
wallet_revokePermissions: async () =>
411+
await revokePermissionsHandler.implementation(
412+
req,
413+
res,
414+
next,
415+
(err) => {
416+
if (err) {
417+
throw err;
418+
}
419+
},
420+
{
421+
revokePermissionsForOrigin: (permissionKeys) => {
422+
try {
423+
/**
424+
* For now, we check if either eth_accounts or endowment:permitted-chains are sent. If either of those is sent, we revoke both.
425+
* This manual filtering will be handled / refactored once we implement [CAIP-25 permissions](https://github.com/MetaMask/MetaMask-planning/issues/4129)
426+
*/
427+
const caip25EquivalentPermissions: string[] = [
428+
PermissionKeys.eth_accounts,
429+
PermissionKeys.permittedChains,
430+
];
431+
432+
const keysToRevoke = permissionKeys.some((key) =>
433+
caip25EquivalentPermissions.includes(key),
434+
)
435+
? Array.from(
436+
new Set([
437+
...caip25EquivalentPermissions,
438+
...permissionKeys,
439+
]),
440+
)
441+
: permissionKeys;
442+
443+
Engine.context.PermissionController.revokePermissions({
444+
[origin]: keysToRevoke,
445+
});
446+
} catch (e) {
447+
// we dont want to handle errors here because
448+
// the revokePermissions api method should just
449+
// return `null` if the permissions were not
450+
// successfully revoked or if the permissions
451+
// for the origin do not exist
452+
}
453+
},
454+
},
455+
),
406456
wallet_getPermissions: async () =>
407457
// TODO: Replace "any" with type
408458
// eslint-disable-next-line @typescript-eslint/no-explicit-any

Diff for: ‎app/core/SDKConnect/SDKConnectConstants.ts

+2
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ export const RPC_METHODS = {
2323
WALLET_SWITCHETHEREUMCHAIN: 'wallet_switchEthereumChain',
2424
WALLET_REQUESTPERMISSIONS: 'wallet_requestPermissions',
2525
WALLET_GETPERMISSIONS: 'wallet_getPermissions',
26+
WALLET_REVOKEPERMISSIONS: 'wallet_revokePermissions',
2627
ETH_ACCOUNTS: 'eth_accounts',
2728
ETH_CHAINID: 'eth_chainId',
2829
};
@@ -41,6 +42,7 @@ export const METHODS_TO_REDIRECT: { [method: string]: boolean } = {
4142
[RPC_METHODS.WALLET_SWITCHETHEREUMCHAIN]: true,
4243
[RPC_METHODS.WALLET_REQUESTPERMISSIONS]: true,
4344
[RPC_METHODS.WALLET_GETPERMISSIONS]: true,
45+
[RPC_METHODS.WALLET_REVOKEPERMISSIONS]: true,
4446
[RPC_METHODS.METAMASK_CONNECTSIGN]: true,
4547
[RPC_METHODS.METAMASK_BATCH]: true,
4648
};

Diff for: ‎bitrise.yml

+8-8
Original file line numberDiff line numberDiff line change
@@ -444,16 +444,16 @@ workflows:
444444
#!/usr/bin/env bash
445445
# Define label data
446446
LABELS_JSON='{"labels":["bitrise-result-ready"]}'
447-
447+
448448
# API URL to add labels to a PR
449449
API_URL="https://api.github.com/repos/$BITRISEIO_GIT_REPOSITORY_OWNER/$BITRISEIO_GIT_REPOSITORY_SLUG/issues/$GITHUB_PR_NUMBER/labels"
450450
451451
# Perform the curl request and capture the HTTP status code
452452
HTTP_RESPONSE=$(curl -s -o response.txt -w "%{http_code}" -X POST -H "Authorization: token $GITHUB_ACCESS_TOKEN" -H "Accept: application/vnd.github.v3+json" -d "$LABELS_JSON" "$API_URL")
453-
453+
454454
# Output the HTTP status code
455-
echo "HTTP Response Code: $HTTP_RESPONSE"
456-
455+
echo "HTTP Response Code: $HTTP_RESPONSE"
456+
457457
# Optionally check the response
458458
echo "HTTP Response Code: $HTTP_RESPONSE"
459459
@@ -466,7 +466,7 @@ workflows:
466466
467467
# Clean up the response file
468468
rm response.txt
469-
469+
470470
471471
# Send a Slack message when workflow fails
472472
notify_failure:
@@ -1570,9 +1570,9 @@ workflows:
15701570
- pipeline_intermediate_files: sourcemaps/ios/index.js.map:BITRISE_APP_STORE_SOURCEMAP_PATH
15711571
- deploy_path: sourcemaps/ios/index.js.map
15721572
title: Deploy Source Map
1573-
meta:
1574-
bitrise.io:
1575-
stack: osx-xcode-15.0.x
1573+
meta:
1574+
bitrise.io:
1575+
stack: osx-xcode-15.0.x
15761576
machine_type_id: g2.mac.large
15771577
build_ios_release_and_upload_sourcemaps:
15781578
envs:

Diff for: ‎e2e/api-specs/ConfirmationsRejectionRule.js

+7
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@ import Gestures from '../utils/Gestures';
77
import ConnectBottomSheet from '../pages/Browser/ConnectBottomSheet';
88
import AssetWatchBottomSheet from '../pages/Transactions/AssetWatchBottomSheet';
99
import SpamFilterModal from '../pages/Browser/SpamFilterModal';
10+
import BrowserView from '../pages/Browser/BrowserView';
11+
import ConnectedAccountsModal from '../pages/Browser/ConnectedAccountsModal';
1012

1113
// eslint-disable-next-line import/no-nodejs-modules
1214
import fs from 'fs';
@@ -24,10 +26,12 @@ export default class ConfirmationsRejectRule {
2426
this.driver = options.driver; // Pass element for detox instead of all the driver
2527
this.only = options.only;
2628
this.allCapsCancel = ['wallet_watchAsset'];
29+
this.permissionConnectionSheet = ['wallet_revokePermissions'];
2730
this.requiresEthAccountsPermission = [
2831
'personal_sign',
2932
'eth_signTypedData_v4',
3033
'eth_getEncryptionPublicKey',
34+
'wallet_revokePermissions',
3135
];
3236
}
3337

@@ -153,6 +157,9 @@ export default class ConfirmationsRejectRule {
153157
await TestHelpers.delay(3000);
154158
if (this.allCapsCancel.includes(call.methodName)) {
155159
await AssetWatchBottomSheet.tapCancelButton();
160+
} else if (call.methodName === 'wallet_revokePermissions') {
161+
await BrowserView.tapLocalHostDefaultAvatar();
162+
await Assertions.checkIfNotVisible(ConnectedAccountsModal.title);
156163
} else {
157164
cancelButton = await Matchers.getElementByText('Cancel');
158165
await Gestures.waitAndTap(cancelButton);

Diff for: ‎e2e/api-specs/json-rpc-coverage.js

+1-2
Original file line numberDiff line numberDiff line change
@@ -181,7 +181,7 @@ const main = async () => {
181181

182182
const methodsWithConfirmations = [
183183
'wallet_requestPermissions',
184-
// 'eth_requestAccounts', // mobile is missing revokePermissions to reset this to prompt and cancel
184+
'eth_requestAccounts',
185185
'wallet_watchAsset',
186186
'personal_sign', // requires permissions for eth_accounts
187187
'wallet_addEthereumChain',
@@ -211,7 +211,6 @@ const main = async () => {
211211
.map((m) => m.name);
212212

213213
const skip = [
214-
'wallet_revokePermissions', // mobile is missing revokePermissions
215214
'eth_coinbase',
216215
'wallet_registerOnboarding',
217216
'eth_getEncryptionPublicKey',

Diff for: ‎e2e/pages/Browser/BrowserView.js

+8
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,10 @@ class Browser {
8787
return Matchers.getElementByID(BrowserViewSelectorsIDs.ADD_NEW_TAB);
8888
}
8989

90+
get DefaultAvatarImageForLocalHost() {
91+
return Matchers.getElementByLabel('L');
92+
}
93+
9094
get networkAvatarButton() {
9195
return device.getPlatform() === 'ios'
9296
? Matchers.getElementByID(BrowserViewSelectorsIDs.AVATAR_IMAGE)
@@ -125,6 +129,10 @@ class Browser {
125129
await Gestures.waitAndTap(this.addressBar);
126130
}
127131

132+
async tapLocalHostDefaultAvatar() {
133+
await Gestures.waitAndTap(this.DefaultAvatarImageForLocalHost);
134+
}
135+
128136
async tapBottomSearchBar() {
129137
await Gestures.waitAndTap(this.searchButton);
130138
}

Diff for: ‎e2e/pages/Browser/ConnectedAccountsModal.js

+4
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,10 @@ import Gestures from '../../utils/Gestures';
77
import TestHelpers from '../../helpers';
88

99
class ConnectedAccountsModal {
10+
get container() {
11+
return Matchers.getElementByID(ConnectedAccountsSelectorsIDs.CONTAINER);
12+
}
13+
1014
get permissionsButton() {
1115
return Matchers.getElementByText(
1216
ConnectedAccountModalSelectorsText.PERMISSION_LINK,

Diff for: ‎e2e/pages/Browser/TestDApp.js

-4
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,6 @@ const CONFIRM_BUTTON_TEXT = enContent.confirmation_modal.confirm_cta;
1515
const APPROVE_BUTTON_TEXT = enContent.transactions.tx_review_approve;
1616

1717
class TestDApp {
18-
get androidContainer() {
19-
return BrowserViewSelectorsIDs.ANDROID_CONTAINER;
20-
}
21-
2218
get confirmButtonText() {
2319
return Matchers.getElementByText(CONFIRM_BUTTON_TEXT);
2420
}

0 commit comments

Comments
 (0)