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

Limit Dapp permissions to primary account #8653

Merged
merged 5 commits into from
May 28, 2020
Merged

Limit Dapp permissions to primary account #8653

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d20449b
Limit Dapp permissions to primary account
brad-decker May 26, 2020
38e322a
update tests for permissisons controller
brad-decker May 27, 2020
4eedf8a
add caveat types enum
brad-decker May 27, 2020
002c8d4
update legacyExposeAccounts notification
brad-decker May 27, 2020
3e868be
remove commented out pieces
brad-decker May 27, 2020
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
6 changes: 6 additions & 0 deletions app/scripts/controllers/permissions/enums.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,12 @@ export const METADATA_STORE_KEY = 'domainMetadata'

export const CAVEAT_NAMES = {
exposedAccounts: 'exposedAccounts',
primaryAccountOnly: 'primaryAccountOnly',
}

export const CAVEAT_TYPES = {
limitResponseLength: 'limitResponseLength',
filterResponse: 'filterResponse',
}

export const NOTIFICATION_NAMES = {
Expand Down
25 changes: 16 additions & 9 deletions app/scripts/controllers/permissions/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
HISTORY_STORE_KEY,
CAVEAT_NAMES,
NOTIFICATION_NAMES,
CAVEAT_TYPES,
} from './enums'

export class PermissionsController {
Expand Down Expand Up @@ -288,9 +289,11 @@ export class PermissionsController {
}
})

const newPermittedAccounts = await this.getAccounts(origin)

this.notifyDomain(origin, {
method: NOTIFICATION_NAMES.accountsChanged,
result: accounts,
result: newPermittedAccounts,
})
this.permissionsLog.logAccountExposure(origin, accounts)

Expand Down Expand Up @@ -424,16 +427,20 @@ export class PermissionsController {

// caveat names are unique, and we will only construct this caveat here
ethAccounts.caveats = ethAccounts.caveats.filter((c) => (
c.name !== CAVEAT_NAMES.exposedAccounts
c.name !== CAVEAT_NAMES.exposedAccounts && c.name !== CAVEAT_NAMES.primaryAccountOnly
))

ethAccounts.caveats.push(
{
type: 'filterResponse',
value: finalizedAccounts,
name: CAVEAT_NAMES.exposedAccounts,
},
)
ethAccounts.caveats.push({
type: CAVEAT_TYPES.limitResponseLength,
value: 1,
name: CAVEAT_NAMES.primaryAccountOnly,
})

rekmarks marked this conversation as resolved.
Show resolved Hide resolved
ethAccounts.caveats.push({
type: CAVEAT_TYPES.filterResponse,
value: finalizedAccounts,
name: CAVEAT_NAMES.exposedAccounts,
})
Comment on lines +433 to +443
Copy link
Member

@rekmarks rekmarks May 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I want to note for posterity that this order is unfortunately important. Fortunately, it is preserved when we call addPermittedAccount, which calls CapabilitiesController.updateCaveatFor on the exposedAccounts caveat, which adds the updated caveat to the end of the caveat array.

There's currently no way to ensure that rpc-cap enforces a particular order of caveat application, except to add/update them such that they're in the order you desire.

So, happily not a problem here, but I'll create an issue on rpc-cap for a feature request.

Copy link
Member

Choose a reason for hiding this comment

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

Ref: MetaMask/rpc-cap#100

}

return finalizedPermissions
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@
"redux": "^4.0.5",
"redux-thunk": "^2.3.0",
"reselect": "^3.0.1",
"rpc-cap": "^2.0.0",
"rpc-cap": "^2.1.0",
"safe-event-emitter": "^1.0.1",
"safe-json-stringify": "^1.2.0",
"single-call-balance-checker-abi": "^1.0.0",
Expand Down
56 changes: 35 additions & 21 deletions test/unit/app/controllers/permissions/mocks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import _getRestrictedMethods

import {
CAVEAT_NAMES,
CAVEAT_TYPES,
NOTIFICATION_NAMES,
} from '../../../../../app/scripts/controllers/permissions/enums'

Expand Down Expand Up @@ -162,10 +163,19 @@ const PERM_NAMES = {
does_not_exist: 'does_not_exist',
}

const ACCOUNT_ARRAYS = {
a: [ ...keyringAccounts.slice(0, 3) ],
b: [keyringAccounts[0]],
c: [keyringAccounts[1]],
const ACCOUNTS = {
a: {
permitted: keyringAccounts.slice(0, 3),
primary: keyringAccounts[0],
},
b: {
permitted: [keyringAccounts[0]],
primary: keyringAccounts[0],
},
c: {
permitted: [keyringAccounts[1]],
primary: keyringAccounts[1],
},
}

/**
Expand All @@ -180,11 +190,15 @@ const CAVEATS = {
* @returns {Object} An eth_accounts exposedAccounts caveats
*/
eth_accounts: (accounts) => {
return {
type: 'filterResponse',
return [{
type: CAVEAT_TYPES.limitResponseLength,
value: 1,
name: CAVEAT_NAMES.primaryAccountOnly,
}, {
type: CAVEAT_TYPES.filterResponse,
value: accounts,
name: CAVEAT_NAMES.exposedAccounts,
}
}]
},
}

Expand Down Expand Up @@ -245,7 +259,7 @@ const PERMS = {
eth_accounts: (accounts) => {
return {
eth_accounts: {
caveats: [CAVEATS.eth_accounts(accounts)],
caveats: CAVEATS.eth_accounts(accounts),
} }
},

Expand Down Expand Up @@ -273,7 +287,7 @@ const PERMS = {
eth_accounts: (accounts) => {
return {
parentCapability: PERM_NAMES.eth_accounts,
caveats: [CAVEATS.eth_accounts(accounts)],
caveats: CAVEATS.eth_accounts(accounts),
}
},

Expand Down Expand Up @@ -639,7 +653,7 @@ export const constants = deepFreeze({

ORIGINS: { ...ORIGINS },

ACCOUNT_ARRAYS: { ...ACCOUNT_ARRAYS },
ACCOUNTS: { ...ACCOUNTS },

PERM_NAMES: { ...PERM_NAMES },

Expand All @@ -659,9 +673,9 @@ export const constants = deepFreeze({
[PERM_NAMES.eth_accounts]: {
lastApproved: 1,
accounts: {
[ACCOUNT_ARRAYS.a[0]]: 1,
[ACCOUNT_ARRAYS.a[1]]: 1,
[ACCOUNT_ARRAYS.a[2]]: 1,
[ACCOUNTS.a.permitted[0]]: 1,
[ACCOUNTS.a.permitted[1]]: 1,
[ACCOUNTS.a.permitted[2]]: 1,
},
},
},
Expand All @@ -671,9 +685,9 @@ export const constants = deepFreeze({
[PERM_NAMES.eth_accounts]: {
lastApproved: 2,
accounts: {
[ACCOUNT_ARRAYS.a[0]]: 2,
[ACCOUNT_ARRAYS.a[1]]: 1,
[ACCOUNT_ARRAYS.a[2]]: 1,
[ACCOUNTS.a.permitted[0]]: 2,
[ACCOUNTS.a.permitted[1]]: 1,
[ACCOUNTS.a.permitted[2]]: 1,
},
},
},
Expand All @@ -700,7 +714,7 @@ export const constants = deepFreeze({
[PERM_NAMES.eth_accounts]: {
lastApproved: 1,
accounts: {
[ACCOUNT_ARRAYS.b[0]]: 1,
[ACCOUNTS.b.permitted[0]]: 1,
},
},
},
Expand All @@ -709,7 +723,7 @@ export const constants = deepFreeze({
[PERM_NAMES.eth_accounts]: {
lastApproved: 1,
accounts: {
[ACCOUNT_ARRAYS.c[0]]: 1,
[ACCOUNTS.c.permitted[0]]: 1,
},
},
},
Expand All @@ -722,7 +736,7 @@ export const constants = deepFreeze({
[PERM_NAMES.eth_accounts]: {
lastApproved: 1,
accounts: {
[ACCOUNT_ARRAYS.b[0]]: 1,
[ACCOUNTS.b.permitted[0]]: 1,
},
},
},
Expand All @@ -731,8 +745,8 @@ export const constants = deepFreeze({
[PERM_NAMES.eth_accounts]: {
lastApproved: 2,
accounts: {
[ACCOUNT_ARRAYS.c[0]]: 1,
[ACCOUNT_ARRAYS.b[0]]: 2,
[ACCOUNTS.c.permitted[0]]: 1,
[ACCOUNTS.b.permitted[0]]: 2,
},
},
},
Expand Down
Loading