Skip to content

Commit 528d3f0

Browse files
hmalik88hmalik88
committed
address PR comments
1 parent 757b5a2 commit 528d3f0

File tree

14 files changed

+193
-56
lines changed

14 files changed

+193
-56
lines changed

‎packages/examples/packages/cronjobs/src/index.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ export const onCronjob: OnCronjobHandler = async ({ request }) => {
4848

4949
/**
5050
* Handle incoming JSON-RPC requests from the dapp, sent through the
51-
* `wallet_invokeSnap` method. This handler handles two methods:
51+
* `wallet_invokeSnap` method. This handler handles three methods:
5252
*
5353
* - `scheduleNotification`: Schedule a notification in the future.
5454
* - `cancelNotification`: Cancel a notification.

‎packages/snaps-controllers/src/cronjob/CronjobController.test.ts‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -288,6 +288,33 @@ describe('CronjobController', () => {
288288
cronjobController.destroy();
289289
});
290290

291+
it('fails to schedule a background event if the date is in the past', () => {
292+
const rootMessenger = getRootCronjobControllerMessenger();
293+
const controllerMessenger =
294+
getRestrictedCronjobControllerMessenger(rootMessenger);
295+
296+
const cronjobController = new CronjobController({
297+
messenger: controllerMessenger,
298+
});
299+
300+
const backgroundEvent = {
301+
snapId: MOCK_SNAP_ID,
302+
date: '2021-01-01T01:00Z',
303+
request: {
304+
method: 'handleEvent',
305+
params: ['p1'],
306+
},
307+
};
308+
309+
expect(() =>
310+
cronjobController.scheduleBackgroundEvent(backgroundEvent),
311+
).toThrow('Cannot schedule an event in the past.');
312+
313+
expect(cronjobController.state.events).toStrictEqual({});
314+
315+
cronjobController.destroy();
316+
});
317+
291318
it('cancels a background event', () => {
292319
const rootMessenger = getRootCronjobControllerMessenger();
293320
const controllerMessenger =

‎packages/snaps-controllers/src/cronjob/CronjobController.ts‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -340,10 +340,10 @@ export class CronjobController extends BaseController<
340340
scheduleBackgroundEvent(
341341
backgroundEventWithoutId: Omit<BackgroundEvent, 'id' | 'scheduledAt'>,
342342
) {
343-
// removing minute precision and converting to UTC.
343+
// removing milliseond precision and converting to UTC.
344344
const scheduledAt = DateTime.fromJSDate(new Date())
345345
.toUTC()
346-
.toFormat("yyyy-MM-dd'T'HH:mm'Z'");
346+
.toFormat("yyyy-MM-dd'T'HH:mm:ss'Z'");
347347
const event = {
348348
...backgroundEventWithoutId,
349349
id: nanoid(),
@@ -394,6 +394,10 @@ export class CronjobController extends BaseController<
394394
const now = new Date();
395395
const ms = date.getTime() - now.getTime();
396396

397+
if (ms < 0) {
398+
throw new Error('Cannot schedule an event in the past.');
399+
}
400+
397401
const timer = new Timer(ms);
398402
timer.start(() => {
399403
this.#messenger

‎packages/snaps-rpc-methods/jest.config.js‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,10 +10,10 @@ module.exports = deepmerge(baseConfig, {
1010
],
1111
coverageThreshold: {
1212
global: {
13-
branches: 92.83,
14-
functions: 97.36,
15-
lines: 97.92,
16-
statements: 97.47,
13+
branches: 93.23,
14+
functions: 97.89,
15+
lines: 98.48,
16+
statements: 98.01,
1717
},
1818
},
1919
});

‎packages/snaps-rpc-methods/src/endowments/cronjob.test.ts‎

Lines changed: 122 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,7 @@
1-
import type { Caveat } from '@metamask/permission-controller';
1+
import type {
2+
Caveat,
3+
PermissionConstraint,
4+
} from '@metamask/permission-controller';
25
import { PermissionType, SubjectType } from '@metamask/permission-controller';
36
import { SnapCaveatType } from '@metamask/snaps-utils';
47

@@ -7,6 +10,7 @@ import {
710
cronjobEndowmentBuilder,
811
validateCronjobCaveat,
912
cronjobCaveatSpecifications,
13+
getCronjobCaveatJobs,
1014
} from './cronjob';
1115
import { SnapEndowments } from './enum';
1216

@@ -62,6 +66,123 @@ describe('endowment:cronjob', () => {
6266
});
6367
});
6468

69+
describe('getCronjobCaveatJobs', () => {
70+
it('returns the jobs from a cronjob caveat', () => {
71+
const permission: PermissionConstraint = {
72+
date: 0,
73+
parentCapability: 'foo',
74+
invoker: 'bar',
75+
id: 'baz',
76+
caveats: [
77+
{
78+
type: SnapCaveatType.SnapCronjob,
79+
value: {
80+
jobs: [
81+
{
82+
expression: '* * * * *',
83+
request: {
84+
method: 'exampleMethodOne',
85+
params: ['p1'],
86+
},
87+
},
88+
],
89+
},
90+
},
91+
],
92+
};
93+
94+
expect(getCronjobCaveatJobs(permission)).toStrictEqual([
95+
{
96+
expression: '* * * * *',
97+
request: {
98+
method: 'exampleMethodOne',
99+
params: ['p1'],
100+
},
101+
},
102+
]);
103+
});
104+
105+
it('returns null if there are no caveats', () => {
106+
const permission: PermissionConstraint = {
107+
date: 0,
108+
parentCapability: 'foo',
109+
invoker: 'bar',
110+
id: 'baz',
111+
caveats: null,
112+
};
113+
114+
expect(getCronjobCaveatJobs(permission)).toBeNull();
115+
});
116+
117+
it('will throw if there is more than one caveat', () => {
118+
const permission: PermissionConstraint = {
119+
date: 0,
120+
parentCapability: 'foo',
121+
invoker: 'bar',
122+
id: 'baz',
123+
caveats: [
124+
{
125+
type: SnapCaveatType.SnapCronjob,
126+
value: {
127+
jobs: [
128+
{
129+
expression: '* * * * *',
130+
request: {
131+
method: 'exampleMethodOne',
132+
params: ['p1'],
133+
},
134+
},
135+
],
136+
},
137+
},
138+
{
139+
type: SnapCaveatType.SnapCronjob,
140+
value: {
141+
jobs: [
142+
{
143+
expression: '* * * * *',
144+
request: {
145+
method: 'exampleMethodOne',
146+
params: ['p1'],
147+
},
148+
},
149+
],
150+
},
151+
},
152+
],
153+
};
154+
155+
expect(() => getCronjobCaveatJobs(permission)).toThrow('Assertion failed.');
156+
});
157+
158+
it('will throw if the caveat type is wrong', () => {
159+
const permission: PermissionConstraint = {
160+
date: 0,
161+
parentCapability: 'foo',
162+
invoker: 'bar',
163+
id: 'baz',
164+
caveats: [
165+
{
166+
type: SnapCaveatType.ChainIds,
167+
value: {
168+
jobs: [
169+
{
170+
expression: '* * * * *',
171+
request: {
172+
method: 'exampleMethodOne',
173+
params: ['p1'],
174+
},
175+
},
176+
],
177+
},
178+
},
179+
],
180+
};
181+
182+
expect(() => getCronjobCaveatJobs(permission)).toThrow('Assertion failed.');
183+
});
184+
});
185+
65186
describe('validateCronjobCaveat', () => {
66187
it('should not throw an error when provided specification is valid', () => {
67188
const caveat: Caveat<string, any> = {

‎packages/snaps-rpc-methods/src/endowments/cronjob.ts‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,10 @@ export const cronjobEndowmentBuilder = Object.freeze({
6262
*/
6363
export function getCronjobCaveatMapper(
6464
value: Json,
65-
): Pick<PermissionConstraint, 'caveats'> {
65+
): Pick<PermissionConstraint, 'caveats'> | Record<string, never> {
66+
if (Object.keys(value as Record<string, Json>).length === 0) {
67+
return {};
68+
}
6669
return {
6770
caveats: [
6871
{
@@ -96,7 +99,7 @@ export function getCronjobCaveatJobs(
9699

97100
const caveat = permission.caveats[0] as Caveat<string, { jobs: Json[] }>;
98101

99-
return (caveat.value?.jobs as CronjobSpecification[]) ?? [];
102+
return (caveat.value?.jobs as CronjobSpecification[]) ?? null;
100103
}
101104

102105
/**
@@ -116,17 +119,13 @@ export function validateCronjobCaveat(caveat: Caveat<string, any>) {
116119

117120
const { value } = caveat;
118121

119-
if (!isPlainObject(value)) {
122+
if (!hasProperty(value, 'jobs') || !isPlainObject(value)) {
120123
throw rpcErrors.invalidParams({
121124
message: 'Expected a plain object.',
122125
});
123126
}
124127

125-
const valueKeys = Object.keys(value);
126-
127-
// If it's an empty object, ok to skip validation as this indicates
128-
// intention to use the background event rpc methods
129-
if (valueKeys.length !== 0 && !isCronjobSpecificationArray(value.jobs)) {
128+
if (!isCronjobSpecificationArray(value.jobs)) {
130129
throw rpcErrors.invalidParams({
131130
message: 'Expected a valid cronjob specification array.',
132131
});

‎packages/snaps-rpc-methods/src/permitted/cancelBackgroundEvent.test.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ import type {
66
import { MOCK_SNAP_ID } from '@metamask/snaps-utils/test-utils';
77
import type { JsonRpcRequest, PendingJsonRpcResponse } from '@metamask/utils';
88

9-
import { SnapEndowments } from '../endowments';
109
import { cancelBackgroundEventHandler } from './cancelBackgroundEvent';
1110

1211
describe('snap_cancelBackgroundEvent', () => {
@@ -142,8 +141,9 @@ describe('snap_cancelBackgroundEvent', () => {
142141

143142
expect(response).toStrictEqual({
144143
error: {
145-
code: -32600,
146-
message: `The snap "${MOCK_SNAP_ID}" does not have the "${SnapEndowments.Cronjob}" permission.`,
144+
code: 4100,
145+
message:
146+
'The requested account and/or method has not been authorized by the user.',
147147
stack: expect.any(String),
148148
},
149149
id: 1,

‎packages/snaps-rpc-methods/src/permitted/cancelBackgroundEvent.ts‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import type { JsonRpcEngineEndCallback } from '@metamask/json-rpc-engine';
22
import type { PermittedHandlerExport } from '@metamask/permission-controller';
3-
import { rpcErrors } from '@metamask/rpc-errors';
3+
import { providerErrors, rpcErrors } from '@metamask/rpc-errors';
44
import type {
55
JsonRpcRequest,
66
CancelBackgroundEventParams,
@@ -64,14 +64,10 @@ async function getCancelBackgroundEventImplementation(
6464
end: JsonRpcEngineEndCallback,
6565
{ cancelBackgroundEvent, hasPermission }: CancelBackgroundEventMethodHooks,
6666
): Promise<void> {
67-
const { params, origin } = req as JsonRpcRequest & { origin: string };
67+
const { params } = req;
6868

6969
if (!hasPermission(SnapEndowments.Cronjob)) {
70-
return end(
71-
rpcErrors.invalidRequest({
72-
message: `The snap "${origin}" does not have the "${SnapEndowments.Cronjob}" permission.`,
73-
}),
74-
);
70+
return end(providerErrors.unauthorized());
7571
}
7672

7773
try {

‎packages/snaps-rpc-methods/src/permitted/getBackgroundEvents.test.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ import type {
66
import { MOCK_SNAP_ID } from '@metamask/snaps-utils/test-utils';
77
import type { JsonRpcRequest, PendingJsonRpcResponse } from '@metamask/utils';
88

9-
import { SnapEndowments } from '../endowments';
109
import { getBackgroundEventsHandler } from './getBackgroundEvents';
1110

1211
describe('snap_getBackgroundEvents', () => {
@@ -207,8 +206,9 @@ describe('snap_getBackgroundEvents', () => {
207206

208207
expect(response).toStrictEqual({
209208
error: {
210-
code: -32600,
211-
message: `The snap "${MOCK_SNAP_ID}" does not have the "${SnapEndowments.Cronjob}" permission.`,
209+
code: 4100,
210+
message:
211+
'The requested account and/or method has not been authorized by the user.',
212212
stack: expect.any(String),
213213
},
214214
id: 1,

‎packages/snaps-rpc-methods/src/permitted/getBackgroundEvents.ts‎

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import type { JsonRpcEngineEndCallback } from '@metamask/json-rpc-engine';
22
import type { PermittedHandlerExport } from '@metamask/permission-controller';
3-
import { rpcErrors } from '@metamask/rpc-errors';
3+
import { providerErrors } from '@metamask/rpc-errors';
44
import type {
55
BackgroundEvent,
66
GetBackgroundEventsParams,
@@ -37,7 +37,7 @@ export const getBackgroundEventsHandler: PermittedHandlerExport<
3737
/**
3838
* The `snap_getBackgroundEvents` method implementation.
3939
*
40-
* @param req - The JSON-RPC request object.
40+
* @param _req - The JSON-RPC request object. Not used by this function.
4141
* @param res - The JSON-RPC response object.
4242
* @param _next - The `json-rpc-engine` "next" callback.
4343
* Not used by this function.
@@ -48,20 +48,14 @@ export const getBackgroundEventsHandler: PermittedHandlerExport<
4848
* @returns An array of background events.
4949
*/
5050
async function getGetBackgroundEventsImplementation(
51-
req: JsonRpcRequest<GetBackgroundEventsParams>,
51+
_req: JsonRpcRequest<GetBackgroundEventsParams>,
5252
res: PendingJsonRpcResponse<GetBackgroundEventsResult>,
5353
_next: unknown,
5454
end: JsonRpcEngineEndCallback,
5555
{ getBackgroundEvents, hasPermission }: GetBackgroundEventsMethodHooks,
5656
): Promise<void> {
57-
const { origin } = req as JsonRpcRequest & { origin: string };
58-
5957
if (!hasPermission(SnapEndowments.Cronjob)) {
60-
return end(
61-
rpcErrors.invalidRequest({
62-
message: `The snap "${origin}" does not have the "${SnapEndowments.Cronjob}" permission.`,
63-
}),
64-
);
58+
return end(providerErrors.unauthorized());
6559
}
6660
try {
6761
const events = getBackgroundEvents();

0 commit comments

Comments
 (0)