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

fix: eth_accounts / accountsChanged behavior when wallet is locked #30067

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 7 additions & 11 deletions app/scripts/metamask-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -2388,10 +2388,8 @@ export default class MetamaskController extends EventEmitter {
const selectedAddress =
this.accountsController.getSelectedAccount().address;
return selectedAddress ? [selectedAddress] : [];
} else if (this.isUnlocked()) {
return this.getPermittedAccounts(innerOrigin);
}
return []; // changing this is a breaking change
return this.getPermittedAccounts(innerOrigin);
Copy link
Contributor

Choose a reason for hiding this comment

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

this change is breaking and will result in unintended behavior. We still need to return empty array for accounts in this hook when the wallet is locked

Copy link
Member Author

Choose a reason for hiding this comment

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

@adonesky1 can you chime in here ?

},
// tx signing
processTransaction: (transactionParams, dappRequest) =>
Expand Down Expand Up @@ -3316,7 +3314,11 @@ export default class MetamaskController extends EventEmitter {
const providerNetworkState = await this.getProviderNetworkState(origin);

return {
isUnlocked: this.isUnlocked(),
/**
* We default `isUnlocked` to `true` because even though we no longer emit events depending on this,
* embedded dapp providers might listen directly to our streams, and therefore depend on it, so we leave it here.
*/
isUnlocked: true,
accounts: this.getPermittedAccounts(origin),
...providerNetworkState,
};
Expand Down Expand Up @@ -5211,12 +5213,10 @@ export default class MetamaskController extends EventEmitter {
* return permissioned accounts to the dapp when the wallet is locked.
*
* @param {string} origin - The origin whose exposed accounts to retrieve.
* @param {object} [options] - The options object
* @param {boolean} [options.ignoreLock] - If accounts should be returned even if the wallet is locked.
* @returns {Promise<string[]>} The origin's permitted accounts, or an empty
* array.
*/
getPermittedAccounts(origin, { ignoreLock } = {}) {
getPermittedAccounts(origin) {
let caveat;
try {
caveat = this.permissionController.getCaveat(
Expand All @@ -5237,10 +5237,6 @@ export default class MetamaskController extends EventEmitter {
return [];
}

if (!this.isUnlocked() && !ignoreLock) {
return [];
}

const ethAccounts = getEthAccounts(caveat.value);
return this.sortAccountsByLastSelected(ethAccounts);
}
Expand Down
45 changes: 37 additions & 8 deletions app/scripts/metamask-controller.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -858,10 +858,42 @@ describe('MetaMaskController', () => {

describe('the wallet is locked', () => {
beforeEach(() => {
jest.spyOn(metamaskController, 'isUnlocked').mockReturnValue(false);
const internalAccounts = [
{
address: '0xbeef',
id: '0bd7348e-bdfe-4f67-875c-de831a583857',
metadata: {
name: 'Test Account',
lastSelected: 2,
keyring: {
type: 'HD Key Tree',
},
},
options: {},
methods: ETH_EOA_METHODS,
type: EthAccountType.Eoa,
},
{
address: '0xdead',
id: 'ff8fda69-d416-4d25-80a2-efb77bc7d4ad',
metadata: {
name: 'Test Account',
lastSelected: 3,
keyring: {
type: 'HD Key Tree',
},
},
options: {},
methods: ETH_EOA_METHODS,
type: EthAccountType.Eoa,
},
];
jest
.spyOn(metamaskController.accountsController, 'listAccounts')
.mockReturnValueOnce(internalAccounts);
});

it('returns empty array if there is a CAIP-25 permission for the origin and ignoreLock is false', async () => {
it('returns `permittedAccounts` if there is a CAIP-25 permission for the origin', async () => {
jest
.spyOn(metamaskController.permissionController, 'getCaveat')
.mockReturnValue({
Expand All @@ -874,15 +906,12 @@ describe('MetaMaskController', () => {
},
},
});

expect(
metamaskController.getPermittedAccounts('test.com', {
ignoreLock: false,
}),
).toStrictEqual([]);
metamaskController.getPermittedAccounts('test.com'),
).toStrictEqual(['0xdead', '0xbeef']);
});

it('returns accounts if there is a CAIP-25 permission for the origin and ignoreLock is true', async () => {
it('returns accounts if there is a CAIP-25 permission for the origin', async () => {
jest
.spyOn(metamaskController.permissionController, 'getCaveat')
.mockReturnValue({
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,7 @@
"@metamask/ppom-validator": "0.36.0",
"@metamask/preinstalled-example-snap": "^0.3.0",
"@metamask/profile-sync-controller": "^4.1.1",
"@metamask/providers": "^19.0.0",
"@metamask/providers": "^20.0.0",
"@metamask/queued-request-controller": "^7.0.1",
"@metamask/rate-limit-controller": "^6.0.0",
"@metamask/remote-feature-flag-controller": "^1.3.0",
Expand Down
10 changes: 5 additions & 5 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -6059,9 +6059,9 @@ __metadata:
languageName: node
linkType: hard

"@metamask/providers@npm:^19.0.0":
version: 19.0.0
resolution: "@metamask/providers@npm:19.0.0"
"@metamask/providers@npm:^20.0.0":
version: 20.0.0
resolution: "@metamask/providers@npm:20.0.0"
dependencies:
"@metamask/json-rpc-engine": "npm:^10.0.2"
"@metamask/json-rpc-middleware-stream": "npm:^8.0.6"
Expand All @@ -6076,7 +6076,7 @@ __metadata:
readable-stream: "npm:^3.6.2"
peerDependencies:
webextension-polyfill: ^0.10.0 || ^0.11.0 || ^0.12.0
checksum: 10/e45b2e90aa45db689bed582e85181d6964daff3c4b651ffc8503edcec2206a1b76677c0ae6fc0b29cef37a1b33775545e73bd1cf62802a4fc5e2841973cdafb6
checksum: 10/b958d03a9380d86e605db239109a3debcc1ffde90371abe5beb82a5bed46c7718303a2bb92ec269eae16eff145b9ebbfcb3445a2b6bad4f297a590ee725a5bad
languageName: node
linkType: hard

Expand Down Expand Up @@ -26655,7 +26655,7 @@ __metadata:
"@metamask/preferences-controller": "npm:^15.0.1"
"@metamask/preinstalled-example-snap": "npm:^0.3.0"
"@metamask/profile-sync-controller": "npm:^4.1.1"
"@metamask/providers": "npm:^19.0.0"
"@metamask/providers": "npm:^20.0.0"
"@metamask/queued-request-controller": "npm:^7.0.1"
"@metamask/rate-limit-controller": "npm:^6.0.0"
"@metamask/remote-feature-flag-controller": "npm:^1.3.0"
Expand Down
Loading