From 803d31440c83fd4a4cb45709c35e536107e6e2f2 Mon Sep 17 00:00:00 2001 From: Hassan Malik <41640681+hmalik88@users.noreply.github.com> Date: Tue, 24 Sep 2024 11:57:31 -0400 Subject: [PATCH] Update `snap_notify` to allow JSX content (#2706) Added the `title`, `detailedView` and `footerLink` properties to `inApp` notifications. This allows snaps to provide expanded views with their notifications. The `detailedView` property lets snaps return JSX content with a limited subset of components. The `footerLink` property is optional when providing an expanded view. This content is initially being exposed to populate a modal, but in the future may populate an entire page. --- .../browserify-plugin/snap.manifest.json | 2 +- .../packages/browserify/snap.manifest.json | 2 +- packages/snaps-rpc-methods/jest.config.js | 6 +- .../{notify.test.ts => notify.test.tsx} | 264 ++++++++++++++---- .../src/restricted/notify.ts | 126 +++++++-- packages/snaps-sdk/src/jsx/index.ts | 1 + .../snaps-sdk/src/jsx/validation.test.tsx | 82 ++++++ packages/snaps-sdk/src/jsx/validation.ts | 37 +++ .../snaps-sdk/src/types/methods/notify.ts | 58 +++- 9 files changed, 498 insertions(+), 80 deletions(-) rename packages/snaps-rpc-methods/src/restricted/{notify.test.ts => notify.test.tsx} (56%) diff --git a/packages/examples/packages/browserify-plugin/snap.manifest.json b/packages/examples/packages/browserify-plugin/snap.manifest.json index e6bfac87e3..2214c8ea2e 100644 --- a/packages/examples/packages/browserify-plugin/snap.manifest.json +++ b/packages/examples/packages/browserify-plugin/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/snaps.git" }, "source": { - "shasum": "oeprRlfJxxGH+tvOyO7i4hy3l0SjzR3rA73tZS7vlZk=", + "shasum": "Rvk4nDRt8bofy5oUtavVGwVB0pnZ7BdsmO9V315Qs+w=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/examples/packages/browserify/snap.manifest.json b/packages/examples/packages/browserify/snap.manifest.json index bf36e9f6a9..e8a3a4c059 100644 --- a/packages/examples/packages/browserify/snap.manifest.json +++ b/packages/examples/packages/browserify/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/snaps.git" }, "source": { - "shasum": "PL+Bg6eUiAl4uBSuGbX4gdcrgxFZIedy/N7FjVMzKIY=", + "shasum": "3LsbiHzT2I8gHJPQCu6JbTiKKCy5XA2VQQdW9Qkn3XU=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/snaps-rpc-methods/jest.config.js b/packages/snaps-rpc-methods/jest.config.js index 964b05f888..e48443e6ec 100644 --- a/packages/snaps-rpc-methods/jest.config.js +++ b/packages/snaps-rpc-methods/jest.config.js @@ -10,10 +10,10 @@ module.exports = deepmerge(baseConfig, { ], coverageThreshold: { global: { - branches: 92.62, + branches: 92.68, functions: 97.17, - lines: 97.67, - statements: 97.16, + lines: 97.71, + statements: 97.21, }, }, }); diff --git a/packages/snaps-rpc-methods/src/restricted/notify.test.ts b/packages/snaps-rpc-methods/src/restricted/notify.test.tsx similarity index 56% rename from packages/snaps-rpc-methods/src/restricted/notify.test.ts rename to packages/snaps-rpc-methods/src/restricted/notify.test.tsx index fc62b2854a..cb1482ee8b 100644 --- a/packages/snaps-rpc-methods/src/restricted/notify.test.ts +++ b/packages/snaps-rpc-methods/src/restricted/notify.test.tsx @@ -1,5 +1,6 @@ import { PermissionType, SubjectType } from '@metamask/permission-controller'; import { NotificationType } from '@metamask/snaps-sdk'; +import { Box, Text } from '@metamask/snaps-sdk/jsx'; import { getImplementation, @@ -20,6 +21,7 @@ describe('snap_notify', () => { showInAppNotification: jest.fn(), isOnPhishingList: jest.fn(), maybeUpdatePhishingList: jest.fn(), + createInterface: jest.fn(), getSnap: jest.fn(), }; @@ -44,12 +46,14 @@ describe('snap_notify', () => { const isOnPhishingList = jest.fn().mockResolvedValueOnce(false); const getSnap = jest.fn(); const maybeUpdatePhishingList = jest.fn(); + const createInterface = jest.fn(); const notificationImplementation = getImplementation({ showNativeNotification, showInAppNotification, isOnPhishingList, maybeUpdatePhishingList, + createInterface, getSnap, }); @@ -70,11 +74,55 @@ describe('snap_notify', () => { }); }); + it('shows inApp notifications with a detailed view', async () => { + const showNativeNotification = jest.fn().mockResolvedValueOnce(true); + const showInAppNotification = jest.fn().mockResolvedValueOnce(true); + const isOnPhishingList = jest.fn().mockResolvedValueOnce(false); + const maybeUpdatePhishingList = jest.fn(); + const createInterface = jest.fn().mockResolvedValueOnce(1); + const getSnap = jest.fn(); + + const notificationImplementation = getImplementation({ + showNativeNotification, + showInAppNotification, + isOnPhishingList, + maybeUpdatePhishingList, + createInterface, + getSnap, + }); + + await notificationImplementation({ + context: { + origin: 'extension', + }, + method: 'snap_notify', + params: { + type: NotificationType.InApp, + message: 'Some message', + title: 'Detailed view title', + content: Hello, + }, + }); + + expect(showInAppNotification).toHaveBeenCalledWith('extension', { + type: NotificationType.InApp, + message: 'Some message', + title: 'Detailed view title', + content: 1, + }); + + expect(createInterface).toHaveBeenCalledWith( + 'extension', + Hello, + ); + }); + it('shows native notification', async () => { const showNativeNotification = jest.fn().mockResolvedValueOnce(true); const showInAppNotification = jest.fn().mockResolvedValueOnce(true); const isOnPhishingList = jest.fn().mockResolvedValueOnce(false); const maybeUpdatePhishingList = jest.fn(); + const createInterface = jest.fn(); const getSnap = jest.fn(); const notificationImplementation = getImplementation({ @@ -82,6 +130,7 @@ describe('snap_notify', () => { showInAppNotification, isOnPhishingList, maybeUpdatePhishingList, + createInterface, getSnap, }); @@ -107,6 +156,7 @@ describe('snap_notify', () => { const showInAppNotification = jest.fn().mockResolvedValueOnce(true); const isOnPhishingList = jest.fn().mockResolvedValueOnce(false); const maybeUpdatePhishingList = jest.fn(); + const createInterface = jest.fn(); const getSnap = jest.fn(); const notificationImplementation = getImplementation({ @@ -114,6 +164,7 @@ describe('snap_notify', () => { showInAppNotification, isOnPhishingList, maybeUpdatePhishingList, + createInterface, getSnap, }); @@ -139,6 +190,7 @@ describe('snap_notify', () => { const showInAppNotification = jest.fn().mockResolvedValueOnce(true); const isOnPhishingList = jest.fn().mockResolvedValueOnce(false); const maybeUpdatePhishingList = jest.fn(); + const createInterface = jest.fn(); const getSnap = jest.fn(); const notificationImplementation = getImplementation({ @@ -146,6 +198,7 @@ describe('snap_notify', () => { showInAppNotification, isOnPhishingList, maybeUpdatePhishingList, + createInterface, getSnap, }); @@ -163,12 +216,91 @@ describe('snap_notify', () => { }), ).rejects.toThrow('Must specify a valid notification "type".'); }); + }); + + describe('getValidatedParams', () => { + it('throws an error if the params is not an object', () => { + const isOnPhishingList = jest.fn().mockResolvedValue(true); + expect(() => getValidatedParams([], isOnPhishingList, jest.fn())).toThrow( + 'Expected params to be a single object.', + ); + }); + + it('throws an error if the type is missing from params object', () => { + const isOnPhishingList = jest.fn().mockResolvedValue(true); + expect(() => + getValidatedParams( + { type: undefined, message: 'Something happened.' }, + isOnPhishingList, + jest.fn(), + ), + ).toThrow('Must specify a valid notification "type".'); + }); + + it('throws an error if the message is empty', () => { + const isOnPhishingList = jest.fn().mockResolvedValue(true); + expect(() => + getValidatedParams( + { type: NotificationType.InApp, message: '' }, + isOnPhishingList, + jest.fn(), + ), + ).toThrow( + 'Must specify a non-empty string "message" less than 500 characters long.', + ); + }); + + it('throws an error if the message is not a string', () => { + const isOnPhishingList = jest.fn().mockResolvedValue(true); + expect(() => + getValidatedParams( + { type: NotificationType.InApp, message: 123 }, + isOnPhishingList, + jest.fn(), + ), + ).toThrow( + 'Must specify a non-empty string "message" less than 500 characters long.', + ); + }); + + it('throws an error if the message is larger than or equal to 50 characters for native notifications', () => { + const isOnPhishingList = jest.fn().mockResolvedValue(true); + expect(() => + getValidatedParams( + { + type: NotificationType.Native, + message: 'test'.repeat(20), + }, + isOnPhishingList, + jest.fn(), + ), + ).toThrow( + 'Must specify a non-empty string "message" less than 50 characters long.', + ); + }); - it('throws an error if a link is on the phishing list', async () => { + it('throws an error if the message is larger than or equal to 500 characters for in app notifications', () => { + const isOnPhishingList = jest.fn().mockResolvedValue(true); + expect(() => + getValidatedParams( + { + type: NotificationType.InApp, + message: 'test'.repeat(150), + }, + isOnPhishingList, + jest.fn(), + ), + ).toThrow( + 'Must specify a non-empty string "message" less than 500 characters long.', + ); + }); + + it('throws an error if a link in the `message` property is on the phishing list', async () => { const showNativeNotification = jest.fn().mockResolvedValueOnce(true); const showInAppNotification = jest.fn().mockResolvedValueOnce(true); - const isOnPhishingList = jest.fn().mockResolvedValueOnce(true); + const isOnPhishingList = jest.fn().mockResolvedValue(true); const maybeUpdatePhishingList = jest.fn(); + const createInterface = jest.fn(); const getSnap = jest.fn(); const notificationImplementation = getImplementation({ @@ -176,6 +308,7 @@ describe('snap_notify', () => { showInAppNotification, isOnPhishingList, maybeUpdatePhishingList, + createInterface, getSnap, }); @@ -186,18 +319,19 @@ describe('snap_notify', () => { }, method: 'snap_notify', params: { - type: 'native', + type: 'inApp', message: '[test link](https://foo.bar)', }, }), ).rejects.toThrow('Invalid URL: The specified URL is not allowed.'); }); - it('throws an error if a link is invalid', async () => { + it('throws an error if a link in the `message` property is invalid', async () => { const showNativeNotification = jest.fn().mockResolvedValueOnce(true); const showInAppNotification = jest.fn().mockResolvedValueOnce(true); - const isOnPhishingList = jest.fn().mockResolvedValueOnce(true); + const isOnPhishingList = jest.fn().mockResolvedValue(true); const maybeUpdatePhishingList = jest.fn(); + const createInterface = jest.fn(); const getSnap = jest.fn(); const notificationImplementation = getImplementation({ @@ -205,6 +339,7 @@ describe('snap_notify', () => { showInAppNotification, isOnPhishingList, maybeUpdatePhishingList, + createInterface, getSnap, }); @@ -215,7 +350,7 @@ describe('snap_notify', () => { }, method: 'snap_notify', params: { - type: 'native', + type: 'inApp', message: '[test](http://foo.bar)', }, }), @@ -223,63 +358,94 @@ describe('snap_notify', () => { 'Invalid URL: Protocol must be one of: https:, mailto:, metamask:.', ); }); - }); - describe('getValidatedParams', () => { - it('throws an error if the params is not an object', () => { - expect(() => getValidatedParams([])).toThrow( - 'Expected params to be a single object.', - ); - }); + it('throws an error if a link in the `footerLink` property is on the phishing list', async () => { + const showNativeNotification = jest.fn().mockResolvedValueOnce(true); + const showInAppNotification = jest.fn().mockResolvedValueOnce(true); + const isOnPhishingList = jest.fn().mockResolvedValue(true); + const maybeUpdatePhishingList = jest.fn(); + const createInterface = jest.fn(); + const getSnap = jest.fn(); - it('throws an error if the type is missing from params object', () => { - expect(() => - getValidatedParams({ type: undefined, message: 'Something happened.' }), - ).toThrow('Must specify a valid notification "type".'); - }); + const notificationImplementation = getImplementation({ + showNativeNotification, + showInAppNotification, + isOnPhishingList, + maybeUpdatePhishingList, + createInterface, + getSnap, + }); - it('throws an error if the message is empty', () => { - expect(() => - getValidatedParams({ type: NotificationType.InApp, message: '' }), - ).toThrow( - 'Must specify a non-empty string "message" less than 500 characters long.', + const content = ( + + Hello, world! + ); - }); - it('throws an error if the message is not a string', () => { - expect(() => - getValidatedParams({ type: NotificationType.InApp, message: 123 }), - ).toThrow( - 'Must specify a non-empty string "message" less than 500 characters long.', - ); + await expect( + notificationImplementation({ + context: { + origin: 'extension', + }, + method: 'snap_notify', + params: { + type: 'inApp', + message: 'message', + footerLink: { href: 'https://www.metamask.io', text: 'test' }, + title: 'A title', + content, + }, + }), + ).rejects.toThrow('Invalid URL: The specified URL is not allowed.'); }); - it('throws an error if the message is larger than or equal to 50 characters for native notifications', () => { - expect(() => - getValidatedParams({ - type: NotificationType.Native, - message: - 'test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg', - }), - ).toThrow( - 'Must specify a non-empty string "message" less than 50 characters long.', + it('throws an error if a link in the `footerLink` property is invalid', async () => { + const showNativeNotification = jest.fn().mockResolvedValueOnce(true); + const showInAppNotification = jest.fn().mockResolvedValueOnce(true); + const isOnPhishingList = jest.fn().mockResolvedValue(true); + const maybeUpdatePhishingList = jest.fn(); + const createInterface = jest.fn(); + const getSnap = jest.fn(); + + const notificationImplementation = getImplementation({ + showNativeNotification, + showInAppNotification, + isOnPhishingList, + maybeUpdatePhishingList, + createInterface, + getSnap, + }); + + const content = ( + + Hello, world! + ); - }); - it('throws an error if the message is larger than or equal to 500 characters for in app notifications', () => { - expect(() => - getValidatedParams({ - type: NotificationType.InApp, - message: - 'test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg_test_msg', + await expect( + notificationImplementation({ + context: { + origin: 'extension', + }, + method: 'snap_notify', + params: { + type: 'inApp', + message: 'message', + footerLink: { href: 'http://foo.bar', text: 'test' }, + title: 'A title', + content, + }, }), - ).toThrow( - 'Must specify a non-empty string "message" less than 500 characters long.', + ).rejects.toThrow( + 'Invalid params: Invalid URL: Protocol must be one of: https:, mailto:, metamask:.', ); }); it('returns valid parameters', () => { - expect(getValidatedParams(validParams)).toStrictEqual(validParams); + const isNotOnPhishingList = jest.fn().mockResolvedValueOnce(false); + expect( + getValidatedParams(validParams, isNotOnPhishingList, jest.fn()), + ).toStrictEqual(validParams); }); }); }); diff --git a/packages/snaps-rpc-methods/src/restricted/notify.ts b/packages/snaps-rpc-methods/src/restricted/notify.ts index 86d551951a..5f5feb36c8 100644 --- a/packages/snaps-rpc-methods/src/restricted/notify.ts +++ b/packages/snaps-rpc-methods/src/restricted/notify.ts @@ -5,31 +5,67 @@ import type { } from '@metamask/permission-controller'; import { PermissionType, SubjectType } from '@metamask/permission-controller'; import { rpcErrors } from '@metamask/rpc-errors'; -import { NotificationType } from '@metamask/snaps-sdk'; +import { enumValue, NotificationType, union } from '@metamask/snaps-sdk'; import type { NotifyParams, NotifyResult, - EnumToUnion, + NotificationComponent, } from '@metamask/snaps-sdk'; -import { type Snap, validateTextLinks } from '@metamask/snaps-utils'; +import { NotificationComponentsStruct } from '@metamask/snaps-sdk/jsx'; +import { + createUnion, + validateLink, + validateTextLinks, + type Snap, +} from '@metamask/snaps-utils'; +import type { InferMatching } from '@metamask/snaps-utils'; +import { object, string } from '@metamask/superstruct'; import type { NonEmptyArray } from '@metamask/utils'; -import { isObject } from '@metamask/utils'; +import { hasProperty, isObject } from '@metamask/utils'; import { type MethodHooksObject } from '../utils'; const methodName = 'snap_notify'; -export type NotificationArgs = { - /** - * Enum type to determine notification type. - */ - type: EnumToUnion; +const NativeNotificationStruct = object({ + type: enumValue(NotificationType.Native), + message: string(), +}); - /** - * A message to show on the notification. - */ - message: string; -}; +const InAppNotificationStruct = object({ + type: enumValue(NotificationType.InApp), + message: string(), +}); + +const InAppNotificationWithDetailsStruct = object({ + type: enumValue(NotificationType.InApp), + message: string(), + content: NotificationComponentsStruct, + title: string(), +}); + +const InAppNotificationWithDetailsAndFooterStruct = object({ + type: enumValue(NotificationType.InApp), + message: string(), + content: NotificationComponentsStruct, + title: string(), + footerLink: object({ + href: string(), + text: string(), + }), +}); + +const NotificationParametersStruct = union([ + InAppNotificationStruct, + InAppNotificationWithDetailsStruct, + InAppNotificationWithDetailsAndFooterStruct, + NativeNotificationStruct, +]); + +export type NotificationParameters = InferMatching< + typeof NotificationParametersStruct, + NotifyParams +>; export type NotifyMethodHooks = { /** @@ -38,7 +74,7 @@ export type NotifyMethodHooks = { */ showNativeNotification: ( snapId: string, - args: NotificationArgs, + args: NotificationParameters, ) => Promise; /** @@ -47,13 +83,17 @@ export type NotifyMethodHooks = { */ showInAppNotification: ( snapId: string, - args: NotificationArgs, + args: NotificationParameters, ) => Promise; isOnPhishingList: (url: string) => boolean; maybeUpdatePhishingList: () => Promise; + createInterface: ( + origin: string, + content: NotificationComponent, + ) => Promise; getSnap: (snapId: string) => Snap | undefined; }; @@ -97,6 +137,7 @@ const methodHooks: MethodHooksObject = { showInAppNotification: true, isOnPhishingList: true, maybeUpdatePhishingList: true, + createInterface: true, getSnap: true, }; @@ -114,6 +155,7 @@ export const notifyBuilder = Object.freeze({ * @param hooks.showInAppNotification - A function that shows a notification in the MetaMask UI. * @param hooks.isOnPhishingList - A function that checks for links against the phishing list. * @param hooks.maybeUpdatePhishingList - A function that updates the phishing list if needed. + * @param hooks.createInterface - A function that creates the interface in SnapInterfaceController. * @param hooks.getSnap - A function that checks if a snap is installed. * @returns The method implementation which returns `null` on success. * @throws If the params are invalid. @@ -123,6 +165,7 @@ export function getImplementation({ showInAppNotification, isOnPhishingList, maybeUpdatePhishingList, + createInterface, getSnap, }: NotifyMethodHooks) { return async function implementation( @@ -133,11 +176,22 @@ export function getImplementation({ context: { origin }, } = args; - const validatedParams = getValidatedParams(params); - await maybeUpdatePhishingList(); - validateTextLinks(validatedParams.message, isOnPhishingList, getSnap); + const validatedParams = getValidatedParams( + params, + isOnPhishingList, + getSnap, + ); + + let id; + if (hasProperty(validatedParams, 'content')) { + id = await createInterface( + origin, + validatedParams.content as NotificationComponent, + ); + validatedParams.content = id; + } switch (validatedParams.type) { case NotificationType.Native: @@ -157,9 +211,16 @@ export function getImplementation({ * type. Throws if validation fails. * * @param params - The unvalidated params object from the method request. + * @param isOnPhishingList - The function that checks for links against the phishing list. + * @param getSnap - A function that checks if a snap is installed. * @returns The validated method parameter object. + * @throws If the params are invalid. */ -export function getValidatedParams(params: unknown): NotifyParams { +export function getValidatedParams( + params: unknown, + isOnPhishingList: NotifyMethodHooks['isOnPhishingList'], + getSnap: NotifyMethodHooks['getSnap'], +): NotifyParams { if (!isObject(params)) { throw rpcErrors.invalidParams({ message: 'Expected params to be a single object.', @@ -200,5 +261,28 @@ export function getValidatedParams(params: unknown): NotifyParams { }); } - return params as NotificationArgs; + try { + const validatedParams = createUnion( + params, + NotificationParametersStruct, + 'type', + ); + + validateTextLinks(validatedParams.message, isOnPhishingList, getSnap); + + if (hasProperty(validatedParams, 'footerLink')) { + validateTextLinks( + validatedParams.footerLink.text, + isOnPhishingList, + getSnap, + ); + validateLink(validatedParams.footerLink.href, isOnPhishingList, getSnap); + } + + return validatedParams; + } catch (error) { + throw rpcErrors.invalidParams({ + message: `Invalid params: ${error.message}`, + }); + } } diff --git a/packages/snaps-sdk/src/jsx/index.ts b/packages/snaps-sdk/src/jsx/index.ts index b5cb2e6a51..651d253e37 100644 --- a/packages/snaps-sdk/src/jsx/index.ts +++ b/packages/snaps-sdk/src/jsx/index.ts @@ -11,4 +11,5 @@ export { BoxChildStruct, FormChildStruct, FieldChildUnionStruct, + NotificationComponentsStruct, } from './validation'; diff --git a/packages/snaps-sdk/src/jsx/validation.test.tsx b/packages/snaps-sdk/src/jsx/validation.test.tsx index 428d2e8764..c0ec8df9a6 100644 --- a/packages/snaps-sdk/src/jsx/validation.test.tsx +++ b/packages/snaps-sdk/src/jsx/validation.test.tsx @@ -68,6 +68,7 @@ import { IconStruct, SelectorStruct, SectionStruct, + NotificationComponentsStruct, } from './validation'; describe('KeyStruct', () => { @@ -543,6 +544,87 @@ describe('BoxStruct', () => { }); }); +describe('NotificationComponentsStruct', () => { + it.each([ + + foo + , + + foo + bar + , + + foo + + alt + + , + + foo + + alt + + , + + foo + + alt + + , + + Foo + {[1, 2, 3, 4, 5].map((value) => ( + {value.toString()} + ))} + , + foo, + + alt + , + ])( + "validates content returned for a notification's detailed view", + (value) => { + expect(is(value, NotificationComponentsStruct)).toBe(true); + }, + ); + + it.each([ + 'foo', + 42, + null, + undefined, + {}, + [], + // @ts-expect-error - Invalid props. + + foo + + alt + + , + // @ts-expect-error - Invalid props. + + foo + + alt + + , + +
+ + + + +
+
, + + + , + ])('does not validate "%p"', (value) => { + expect(is(value, NotificationComponentsStruct)).toBe(false); + }); +}); + describe('FooterStruct', () => { it.each([