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

Add PermittedHandlerExport to permission-controller #1184

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Add PermittedHandlerExport to permission-controller #1184

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c92d254
Use `@metamask/json-rpc-engine`, `@metamask/rpc-errors`, and `@metama…
Mrtenz Apr 14, 2023
ecd9c6a
Update "@metamask/rpc-errors" to version "^5.1.1"
Mrtenz Apr 19, 2023
327803b
Update packages/permission-controller/src/permitted-handler.ts
Mrtenz Apr 19, 2023
f05f318
Add a 'cause' field to error objects in tests
Mrtenz Apr 19, 2023
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: 5 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,11 @@
"web3-provider-engine>ethereumjs-util>keccak": false,
"web3-provider-engine>ethereumjs-util>secp256k1": false,
"web3-provider-engine>ethereumjs-util>ethereum-cryptography>keccak": false,
"web3-provider-engine>ethereumjs-util>ethereum-cryptography>secp256k1": false
"web3-provider-engine>ethereumjs-util>ethereum-cryptography>secp256k1": false,
"web3-provider-engine>eth-sig-util>ethereumjs-abi>ethereumjs-util>keccak": false,
"web3-provider-engine>eth-sig-util>ethereumjs-abi>ethereumjs-util>secp256k1": false,
"web3-provider-engine>ethereumjs-vm>ethereumjs-util>keccak": false,
"web3-provider-engine>ethereumjs-vm>ethereumjs-util>secp256k1": false
}
}
}
6 changes: 2 additions & 4 deletions packages/permission-controller/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,11 @@
"@metamask/approval-controller": "workspace:^",
"@metamask/base-controller": "workspace:^",
"@metamask/controller-utils": "workspace:^",
"@metamask/types": "^1.1.0",
"@metamask/utils": "^5.0.2",
"@metamask/json-rpc-engine": "^7.0.0",
"@metamask/rpc-errors": "^5.1.1",
"@types/deep-freeze-strict": "^1.1.0",
"deep-freeze-strict": "^1.1.1",
"eth-rpc-errors": "^4.0.2",
"immer": "^9.0.6",
"json-rpc-engine": "^6.1.0",
"nanoid": "^3.1.31"
},
"devDependencies": {
Expand Down
3 changes: 1 addition & 2 deletions packages/permission-controller/src/Caveat.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { Json } from '@metamask/types';
import { hasProperty } from '@metamask/utils';
import { Json, hasProperty } from '@metamask/utils';
import {
CaveatSpecificationMismatchError,
UnrecognizedCaveatTypeError,
Expand Down
8 changes: 5 additions & 3 deletions packages/permission-controller/src/Permission.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { Json } from '@metamask/types';
import { Json, NonEmptyArray } from '@metamask/utils';
import { nanoid } from 'nanoid';
import { NonEmptyArray } from '@metamask/controller-utils';
import { ActionConstraint, EventConstraint } from '@metamask/base-controller';
import type { SubjectType } from './SubjectMetadataController';
import { CaveatConstraint } from './Caveat';
Expand Down Expand Up @@ -259,7 +258,10 @@ type RestrictedMethodContext = Readonly<{
[key: string]: any;
}>;

export type RestrictedMethodParameters = Json[] | Record<string, Json> | void;
export type RestrictedMethodParameters =
| Json[]
| Record<string, Json>
| undefined;

/**
* The arguments passed to a restricted method implementation.
Expand Down
78 changes: 62 additions & 16 deletions packages/permission-controller/src/PermissionController.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,20 @@
import assert from 'assert';
import { JsonRpcEngine, PendingJsonRpcResponse } from 'json-rpc-engine';
import { JsonRpcEngine } from '@metamask/json-rpc-engine';
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we will get type issues by just updating json-rpc-engine in this package alone 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Also this will make the PermissionController minimum Node 16. Are we ready for that in this repo?

Copy link
Contributor

@mcmire mcmire Apr 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Good catch @FrederikBolding.

We don't currently supporting bumping the minimum Node version for select packages in this monorepo. The Node version would have to be bumped across the board.

I'm not sure if we are ready to do this Update: It looks like we are getting pretty close to bumping to Node 16 on mobile, so this may not be a problem, but either way, it might be good to bump @metamask/json-rpc-engine in another PR for visibility, if that's possible?

Copy link
Member Author

Choose a reason for hiding this comment

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

Bumping @metamask/json-rpc-engine requires bumping @metamask/utils and @metamask/rpc-errors, due to type conflicts with @metamask/types otherwise. I could cherry-pick the first commit into a separate PR, if that makes it more clear?

Copy link
Member

Choose a reason for hiding this comment

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

As of today bumping this library to 16 should not be a problem as mobile is now running Node 16.

Copy link
Contributor

@mcmire mcmire Apr 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@Mrtenz Cherry-picking the first commit in a separate PR makes sense. We should also make a new PR that bumps the minimum version of Node to 16 across the board so we can codify that in the next release.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried cherry-picking without including the new PermittedHandlerExport, but the types are not compatible between @metamask/types and @metamask/utils, so that doesn't really work.

Copy link
Member

Choose a reason for hiding this comment

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

All packages in this repo now require Node 16, so that part is at least done.

import {
AcceptRequest as AcceptApprovalRequest,
AddApprovalRequest,
HasApprovalRequest,
RejectRequest as RejectApprovalRequest,
} from '@metamask/approval-controller';
import { ControllerMessenger } from '@metamask/base-controller';
import { hasProperty } from '@metamask/utils';
import { Json, isPlainObject } from '@metamask/controller-utils';
import {
hasProperty,
PendingJsonRpcResponse,
isPlainObject,
Json,
} from '@metamask/utils';
import { GetSubjectMetadata, SubjectType } from './SubjectMetadataController';

import * as errors from './errors';
import { EndowmentGetterParams } from './Permission';
import {
Expand Down Expand Up @@ -251,7 +256,7 @@ function getDefaultPermissionSpecifications() {
CaveatTypes.filterArrayResponse,
CaveatTypes.reverseArrayResponse,
],
methodImplementation: (_args: RestrictedMethodOptions<void>) => {
methodImplementation: (_args: RestrictedMethodOptions<undefined>) => {
return ['a', 'b', 'c'];
},
},
Expand All @@ -262,7 +267,7 @@ function getDefaultPermissionSpecifications() {
CaveatTypes.filterObjectResponse,
CaveatTypes.noopCaveat,
],
methodImplementation: (_args: RestrictedMethodOptions<void>) => {
methodImplementation: (_args: RestrictedMethodOptions<undefined>) => {
return { a: 'x', b: 'y', c: 'z' };
},
validator: (permission: PermissionConstraint) => {
Expand All @@ -279,7 +284,7 @@ function getDefaultPermissionSpecifications() {
permissionType: PermissionType.RestrictedMethod,
targetKey: PermissionKeys['wallet_getSecret_*'],
allowedCaveats: [CaveatTypes.noopCaveat],
methodImplementation: (args: RestrictedMethodOptions<void>) => {
methodImplementation: (args: RestrictedMethodOptions<undefined>) => {
return `Hello, secret friend "${args.method.replace(
'wallet_getSecret_',
'',
Expand Down Expand Up @@ -315,15 +320,15 @@ function getDefaultPermissionSpecifications() {
permissionType: PermissionType.RestrictedMethod,
targetKey: PermissionKeys.wallet_noop,
allowedCaveats: null,
methodImplementation: (_args: RestrictedMethodOptions<void>) => {
methodImplementation: (_args: RestrictedMethodOptions<undefined>) => {
return null;
},
},
[PermissionKeys.wallet_noopWithPermittedAndFailureSideEffects]: {
permissionType: PermissionType.RestrictedMethod,
targetKey: PermissionKeys.wallet_noopWithPermittedAndFailureSideEffects,
allowedCaveats: null,
methodImplementation: (_args: RestrictedMethodOptions<void>) => {
methodImplementation: (_args: RestrictedMethodOptions<undefined>) => {
return null;
},
sideEffect: {
Expand All @@ -335,7 +340,7 @@ function getDefaultPermissionSpecifications() {
permissionType: PermissionType.RestrictedMethod,
targetKey: PermissionKeys.wallet_noopWithPermittedAndFailureSideEffects2,
allowedCaveats: null,
methodImplementation: (_args: RestrictedMethodOptions<void>) => {
methodImplementation: (_args: RestrictedMethodOptions<undefined>) => {
return null;
},
sideEffect: {
Expand All @@ -347,7 +352,7 @@ function getDefaultPermissionSpecifications() {
permissionType: PermissionType.RestrictedMethod,
targetKey: PermissionKeys.wallet_noopWithPermittedSideEffects,
allowedCaveats: null,
methodImplementation: (_args: RestrictedMethodOptions<void>) => {
methodImplementation: (_args: RestrictedMethodOptions<undefined>) => {
return null;
},
sideEffect: {
Expand All @@ -358,7 +363,7 @@ function getDefaultPermissionSpecifications() {
[PermissionKeys.wallet_noopWithValidator]: {
permissionType: PermissionType.RestrictedMethod,
targetKey: PermissionKeys.wallet_noopWithValidator,
methodImplementation: (_args: RestrictedMethodOptions<void>) => {
methodImplementation: (_args: RestrictedMethodOptions<undefined>) => {
return null;
},
allowedCaveats: [CaveatTypes.noopCaveat, CaveatTypes.filterArrayResponse],
Expand All @@ -377,7 +382,7 @@ function getDefaultPermissionSpecifications() {
[PermissionKeys.wallet_noopWithFactory]: {
permissionType: PermissionType.RestrictedMethod,
targetKey: PermissionKeys.wallet_noopWithFactory,
methodImplementation: (_args: RestrictedMethodOptions<void>) => {
methodImplementation: (_args: RestrictedMethodOptions<undefined>) => {
return null;
},
allowedCaveats: [CaveatTypes.filterArrayResponse],
Expand All @@ -404,7 +409,7 @@ function getDefaultPermissionSpecifications() {
permissionType: PermissionType.RestrictedMethod,
targetKey: PermissionKeys.snap_foo,
allowedCaveats: null,
methodImplementation: (_args: RestrictedMethodOptions<void>) => {
methodImplementation: (_args: RestrictedMethodOptions<undefined>) => {
return null;
},
subjectTypes: [SubjectType.Snap],
Expand Down Expand Up @@ -3748,6 +3753,40 @@ describe('PermissionController', () => {
);
});

it('throws if requested permissions object is not JSON', async () => {
const options = getPermissionControllerOptions();
const { messenger } = options;
const origin = 'metamask.io';
const controller = getDefaultPermissionController(options);

jest
.spyOn(messenger, 'call')
.mockImplementationOnce(async (...args: any) => {
const [, { requestData }] = args;
return {
metadata: { ...requestData.metadata },
permissions: { ...requestData.permissions },
};
});

await expect(
async () =>
await controller.requestPermissions(
{ origin },
{
[PermissionNames.wallet_getSecretArray]: {
// @ts-expect-error - Invalid JSON value.
foo: () => undefined,
},
},
),
).rejects.toThrow(
errors.invalidParams({
message: `Approved permissions request for subject "metamask.io" is not a valid JSON value.`,
}),
);
});

it('throws if requested permissions object is not a plain object', async () => {
const options = getPermissionControllerOptions();
const { messenger } = options;
Expand Down Expand Up @@ -5413,7 +5452,11 @@ describe('PermissionController', () => {
};

const expectedError = errors.unauthorized({
data: { origin, method: PermissionNames.wallet_getSecretArray },
data: {
origin,
method: PermissionNames.wallet_getSecretArray,
cause: null,
},
});

const { error }: any = await engine.handle(request);
Expand All @@ -5433,7 +5476,10 @@ describe('PermissionController', () => {
method: 'wallet_foo',
};

const expectedError = errors.methodNotFound('wallet_foo', { origin });
const expectedError = errors.methodNotFound('wallet_foo', {
origin,
cause: null,
});

const { error }: any = await engine.handle(request);
expect(error).toMatchObject(expect.objectContaining(expectedError));
Expand Down Expand Up @@ -5473,7 +5519,7 @@ describe('PermissionController', () => {

const expectedError = errors.internalError(
`Request for method "${PermissionNames.wallet_doubleNumber}" returned undefined result.`,
{ request: { ...request } },
{ request: { ...request }, cause: null },
);

const { error }: any = await engine.handle(request);
Expand Down
36 changes: 24 additions & 12 deletions packages/permission-controller/src/PermissionController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
/* eslint-enable @typescript-eslint/no-unused-vars */
import { Mutable } from '@metamask/types';
import deepFreeze from 'deep-freeze-strict';
import { castDraft, Draft, Patch } from 'immer';
import { nanoid } from 'nanoid';
import { EthereumRpcError } from 'eth-rpc-errors';
import { hasProperty } from '@metamask/utils';
import { hasProperty, Mutable } from '@metamask/utils';
import { JsonRpcError, serializeError } from '@metamask/rpc-errors';
import {
AcceptRequest as AcceptApprovalRequest,
AddApprovalRequest,
Expand All @@ -26,6 +25,7 @@ import {
NonEmptyArray,
} from '@metamask/controller-utils';
import { GetSubjectMetadata } from './SubjectMetadataController';

import {
CaveatConstraint,
CaveatSpecificationConstraint,
Expand Down Expand Up @@ -1849,6 +1849,10 @@ export class PermissionController<
origin: OriginString,
target: string,
): void {
if (!isValidJson(caveat)) {
throw new CaveatInvalidJsonError(caveat, origin, target);
}

if (!isPlainObject(caveat)) {
throw new InvalidCaveatError(caveat, origin, target);
}
Expand All @@ -1870,10 +1874,6 @@ export class PermissionController<
throw new CaveatMissingValueError(caveat, origin, target);
}

if (!isValidJson(caveat.value)) {
throw new CaveatInvalidJsonError(caveat, origin, target);
}

// Typecast: TypeScript still believes that the caveat is a PlainObject.
specification.validator?.(caveat as CaveatConstraint, origin, target);
}
Expand Down Expand Up @@ -1987,7 +1987,7 @@ export class PermissionController<
*/
private validateRequestedPermissions(
origin: OriginString,
requestedPermissions: unknown,
requestedPermissions: Json,
): void {
if (!isPlainObject(requestedPermissions)) {
throw invalidParams({
Expand Down Expand Up @@ -2127,7 +2127,9 @@ export class PermissionController<
failureHandlersList.map((failureHandler) => failureHandler(params)),
);
} catch (error) {
throw internalError('Unexpected error in side-effects', { error });
throw internalError('Unexpected error in side-effects', {
cause: error,
});
}
}
const reasons = rejectedHandlers.map((handler) => handler.reason);
Expand All @@ -2139,7 +2141,7 @@ export class PermissionController<
throw reasons.length > 1
? internalError(
'Multiple errors occurred during side-effects execution',
{ errors: reasons },
{ errors: reasons.map((error) => serializeError(error)) },
)
: reasons[0];
}
Expand Down Expand Up @@ -2169,6 +2171,12 @@ export class PermissionController<
) {
const { id, origin } = originalMetadata;

if (!isValidJson(approvedRequest)) {
throw internalError(
`Approved permissions request for subject "${origin}" is not a valid JSON value.`,
);
}

if (
!isPlainObject(approvedRequest) ||
!isPlainObject(approvedRequest.metadata)
Expand Down Expand Up @@ -2201,15 +2209,19 @@ export class PermissionController<
try {
this.validateRequestedPermissions(origin, permissions);
} catch (error) {
if (error instanceof EthereumRpcError) {
if (error instanceof JsonRpcError) {
// Re-throw as an internal error; we should never receive invalid approved
// permissions.
throw internalError(
`Invalid approved permissions request: ${error.message}`,
error.data,
);
}
throw internalError('Unrecognized error type', { error });

/* istanbul ignore next: should never happen */
throw internalError('Unrecognized error type', {
cause: error,
});
}
}

Expand Down
2 changes: 1 addition & 1 deletion packages/permission-controller/src/SubjectMetadataController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type { Patch } from 'immer';
import { Json } from '@metamask/types';
import { Json } from '@metamask/utils';
import {
BaseControllerV2,
RestrictedControllerMessenger,
Expand Down
Loading