From 945349b5fa783969d08468a7017398ee7179cb87 Mon Sep 17 00:00:00 2001 From: Prithpal Sooriya Date: Thu, 19 Dec 2024 15:25:45 +0000 Subject: [PATCH] fix: add notification call prevention logic (#5081) ## Explanation We've added logic on platforms to make sure that the global notifications switch is enabled before we fetch. But ideally this should be internal logic. So we've updated the logic to ensure that Feature Announcements and Wallet Notifications will bail early if the toggle is off. Snap notifications however will not obey this flag (to keep compatibility with the original snap notifications feature). ## References https://github.com/MetaMask/metamask-extension/issues/28173 ## Changelog ### `@metamask/notification-services-controller` - **CHANGED**: add `isNotificationServicesEnabled` check inside `fetchAndUpdateMetamaskNotifications` method. - this will prevent fetching feature announcements and wallet notifications if set to `false`. ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate - [x] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes --- .../NotificationServicesController.test.ts | 112 +++++++++++------- .../NotificationServicesController.ts | 66 ++++++----- ...NotificationServicesPushController.test.ts | 6 + .../services/services.test.ts | 13 ++ 4 files changed, 129 insertions(+), 68 deletions(-) diff --git a/packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.test.ts b/packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.test.ts index 13745c925b7..77979ae8ce6 100644 --- a/packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.test.ts +++ b/packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.test.ts @@ -6,6 +6,7 @@ import type { } from '@metamask/keyring-controller'; import type { UserStorageController } from '@metamask/profile-sync-controller'; import { AuthenticationController } from '@metamask/profile-sync-controller'; +import log from 'loglevel'; import { createMockSnapNotification } from './__fixtures__'; import { @@ -36,6 +37,8 @@ import type { NotificationServicesPushControllerEnablePushNotifications, NotificationServicesPushControllerDisablePushNotifications, NotificationServicesPushControllerUpdateTriggerPushNotifications, + NotificationServicesControllerMessenger, + NotificationServicesControllerState, } from './NotificationServicesController'; import { processFeatureAnnouncement } from './processors'; import { processNotification } from './processors/process-notifications'; @@ -55,6 +58,11 @@ const featureAnnouncementsEnv = { platform: 'extension' as const, }; +// Testing util to clean up verbose logs when testing errors +const mockErrorLog = () => + jest.spyOn(log, 'error').mockImplementation(jest.fn()); +const mockWarnLog = () => jest.spyOn(log, 'warn').mockImplementation(jest.fn()); + describe('metamask-notifications - constructor()', () => { const arrangeMocks = () => { const messengerMocks = mockNotificationMessenger(); @@ -294,6 +302,7 @@ describe('metamask-notifications - createOnChainTriggers()', () => { it('throws if not given a valid auth & storage key', async () => { const mocks = arrangeMocks(); + mockErrorLog(); const controller = new NotificationServicesController({ messenger: mocks.messenger, env: { featureAnnouncements: featureAnnouncementsEnv }, @@ -377,6 +386,7 @@ describe('metamask-notifications - deleteOnChainTriggersByAccount', () => { it('throws errors when invalid auth and storage', async () => { const mocks = arrangeMocks(); + mockErrorLog(); const controller = new NotificationServicesController({ messenger: mocks.messenger, env: { featureAnnouncements: featureAnnouncementsEnv }, @@ -431,6 +441,7 @@ describe('metamask-notifications - updateOnChainTriggersByAccount()', () => { it('throws errors when invalid auth and storage', async () => { const mocks = arrangeMocks(); + mockErrorLog(); const controller = new NotificationServicesController({ messenger: mocks.messenger, env: { featureAnnouncements: featureAnnouncementsEnv }, @@ -477,68 +488,87 @@ describe('metamask-notifications - fetchAndUpdateMetamaskNotifications()', () => }; }; - it('processes and shows feature announcements, wallet and snap notifications', async () => { - const { - messenger, - mockFeatureAnnouncementAPIResult, - mockListNotificationsAPIResult, - } = arrangeMocks(); - - const snapNotification = createMockSnapNotification(); - const processedSnapNotification = processSnapNotification(snapNotification); - + const arrangeController = ( + messenger: NotificationServicesControllerMessenger, + overrideState?: Partial, + ) => { const controller = new NotificationServicesController({ messenger, env: { featureAnnouncements: featureAnnouncementsEnv }, state: { ...defaultState, + isNotificationServicesEnabled: true, isFeatureAnnouncementsEnabled: true, - metamaskNotificationsList: [processedSnapNotification], + ...overrideState, }, }); + return controller; + }; + + it('processes and shows all notifications (announcements, wallet, and snap notifications)', async () => { + const { messenger } = arrangeMocks(); + const controller = arrangeController(messenger, { + metamaskNotificationsList: [ + processSnapNotification(createMockSnapNotification()), + ], + }); + const result = await controller.fetchAndUpdateMetamaskNotifications(); - // Should have 1 feature announcement and 1 wallet notification - expect(result).toHaveLength(3); + // Should have 1 feature announcement expect( - result.find( - (n) => n.id === mockFeatureAnnouncementAPIResult.items?.[0].fields.id, - ), - ).toBeDefined(); + result.filter((n) => n.type === TRIGGER_TYPES.FEATURES_ANNOUNCEMENT), + ).toHaveLength(1); + + // Should have 1 Wallet Notification expect( - result.find((n) => n.id === mockListNotificationsAPIResult[0].id), - ).toBeDefined(); - expect(result.find((n) => n.type === TRIGGER_TYPES.SNAP)).toBeDefined(); + result.filter((n) => n.type === TRIGGER_TYPES.ETH_SENT), + ).toHaveLength(1); - // State is also updated - expect(controller.state.metamaskNotificationsList).toHaveLength(3); - }); + // Should have 1 Snap Notification + expect(result.filter((n) => n.type === TRIGGER_TYPES.SNAP)).toHaveLength(1); - it('only fetches and processes feature announcements if not authenticated', async () => { - const { messenger, mockGetBearerToken, mockFeatureAnnouncementAPIResult } = - arrangeMocks(); - mockGetBearerToken.mockRejectedValue( - new Error('MOCK - failed to get access token'), - ); + // Total notification length = 3 + expect(result).toHaveLength(3); + }); - const controller = new NotificationServicesController({ - messenger, - env: { featureAnnouncements: featureAnnouncementsEnv }, - state: { ...defaultState, isFeatureAnnouncementsEnabled: true }, + it('does not fetch feature announcements or wallet notifications if notifications are disabled globally', async () => { + const { messenger, ...mocks } = arrangeMocks(); + const controller = arrangeController(messenger, { + isNotificationServicesEnabled: false, + metamaskNotificationsList: [ + processSnapNotification(createMockSnapNotification()), + ], }); - // Should only have feature announcement const result = await controller.fetchAndUpdateMetamaskNotifications(); + + // Should only contain snap notification + // As this is not controlled by the global notification switch expect(result).toHaveLength(1); + expect(result.filter((n) => n.type === TRIGGER_TYPES.SNAP)).toHaveLength(1); + + // APIs should not have been called + expect(mocks.mockFeatureAnnouncementsAPI.isDone()).toBe(false); + expect(mocks.mockListNotificationsAPI.isDone()).toBe(false); + }); + + it('should not fetch feature announcements if disabled', async () => { + const { messenger, ...mocks } = arrangeMocks(); + const controller = arrangeController(messenger, { + isFeatureAnnouncementsEnabled: false, + }); + + const result = await controller.fetchAndUpdateMetamaskNotifications(); + + // Should not have any feature announcements expect( - result.find( - (n) => n.id === mockFeatureAnnouncementAPIResult.items?.[0].fields.id, - ), - ).toBeDefined(); + result.filter((n) => n.type === TRIGGER_TYPES.FEATURES_ANNOUNCEMENT), + ).toHaveLength(0); - // State is also updated - expect(controller.state.metamaskNotificationsList).toHaveLength(1); + // Should not have called feature announcement API + expect(mocks.mockFeatureAnnouncementsAPI.isDone()).toBe(false); }); }); @@ -720,6 +750,8 @@ describe('metamask-notifications - markMetamaskNotificationsAsRead()', () => { messenger, env: { featureAnnouncements: featureAnnouncementsEnv }, }); + mockErrorLog(); + mockWarnLog(); await controller.markMetamaskNotificationsAsRead([ processNotification(createMockFeatureAnnouncementRaw()), diff --git a/packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.ts b/packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.ts index 1421b6cf690..f349dfdf5e3 100644 --- a/packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.ts +++ b/packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.ts @@ -577,7 +577,7 @@ export default class NotificationServicesController extends BaseController< #registerMessageHandlers(): void { this.messagingSystem.registerActionHandler( `${controllerName}:updateMetamaskNotificationsList`, - async (...args) => this.updateMetamaskNotificationsList(...args), + this.updateMetamaskNotificationsList.bind(this), ); this.messagingSystem.registerActionHandler( @@ -1083,7 +1083,7 @@ export default class NotificationServicesController extends BaseController< /** * Fetches the list of metamask notifications. - * This includes OnChain notifications and Feature Announcements. + * This includes OnChain notifications; Feature Announcements; and Snap Notifications. * * **Action** - When a user views the notification list page/dropdown * @@ -1096,35 +1096,46 @@ export default class NotificationServicesController extends BaseController< try { this.#setIsFetchingMetamaskNotifications(true); + // This is used by Feature Announcement & On Chain + // Not used by Snaps + const isGlobalNotifsEnabled = this.state.isNotificationServicesEnabled; + // Raw Feature Notifications - const rawFeatureAnnouncementNotifications = this.state - .isFeatureAnnouncementsEnabled - ? await FeatureNotifications.getFeatureAnnouncementNotifications( - this.#featureAnnouncementEnv, - previewToken, - ).catch(() => []) - : []; + const rawFeatureAnnouncementNotifications = + isGlobalNotifsEnabled && this.state.isFeatureAnnouncementsEnabled + ? await FeatureNotifications.getFeatureAnnouncementNotifications( + this.#featureAnnouncementEnv, + previewToken, + ).catch(() => []) + : []; // Raw On Chain Notifications const rawOnChainNotifications: OnChainRawNotification[] = []; - const userStorage = await this.#storage - .getNotificationStorage() - .then((s) => s && (JSON.parse(s) as UserStorage)) - .catch(() => null); - const bearerToken = await this.#auth.getBearerToken().catch(() => null); - if (userStorage && bearerToken) { - const notifications = - await OnChainNotifications.getOnChainNotifications( - userStorage, - bearerToken, - ).catch(() => []); - - rawOnChainNotifications.push(...notifications); + if (isGlobalNotifsEnabled) { + const userStorage = await this.#storage + .getNotificationStorage() + .then((s) => s && (JSON.parse(s) as UserStorage)) + .catch(() => null); + const bearerToken = await this.#auth.getBearerToken().catch(() => null); + if (userStorage && bearerToken) { + const notifications = + await OnChainNotifications.getOnChainNotifications( + userStorage, + bearerToken, + ).catch(() => []); + + rawOnChainNotifications.push(...notifications); + } } - const readIds = this.state.metamaskNotificationsReadList; + // Snap Notifications (original) + // We do not want to remove them + const snapNotifications = this.state.metamaskNotificationsList.filter( + (notification) => notification.type === TRIGGER_TYPES.SNAP, + ); - // Combined Notifications + // Process Notifications + const readIds = this.state.metamaskNotificationsReadList; const isNotUndefined = (t?: Item): t is Item => Boolean(t); const processAndFilter = (ns: RawNotificationUnion[]) => ns @@ -1136,15 +1147,14 @@ export default class NotificationServicesController extends BaseController< ); const onChainNotifications = processAndFilter(rawOnChainNotifications); - const snapNotifications = this.state.metamaskNotificationsList.filter( - (notification) => notification.type === TRIGGER_TYPES.SNAP, - ); - + // Combine Notifications const metamaskNotifications: INotification[] = [ ...featureAnnouncementNotifications, ...onChainNotifications, ...snapNotifications, ]; + + // Sort Notifications metamaskNotifications.sort( (a, b) => new Date(b.createdAt).getTime() - new Date(a.createdAt).getTime(), diff --git a/packages/notification-services-controller/src/NotificationServicesPushController/NotificationServicesPushController.test.ts b/packages/notification-services-controller/src/NotificationServicesPushController/NotificationServicesPushController.test.ts index ef1505ca392..2a01d18b7b5 100644 --- a/packages/notification-services-controller/src/NotificationServicesPushController/NotificationServicesPushController.test.ts +++ b/packages/notification-services-controller/src/NotificationServicesPushController/NotificationServicesPushController.test.ts @@ -1,5 +1,6 @@ import { ControllerMessenger } from '@metamask/base-controller'; import type { AuthenticationController } from '@metamask/profile-sync-controller'; +import log from 'loglevel'; import NotificationServicesPushController from './NotificationServicesPushController'; import type { @@ -10,6 +11,10 @@ import type { import * as services from './services/services'; import type { PushNotificationEnv } from './types'; +// Testing util to clean up verbose logs when testing errors +const mockErrorLog = () => + jest.spyOn(log, 'error').mockImplementation(jest.fn()); + const MOCK_JWT = 'mockJwt'; const MOCK_FCM_TOKEN = 'mockFcmToken'; const MOCK_MOBILE_FCM_TOKEN = 'mockMobileFcmToken'; @@ -90,6 +95,7 @@ describe('NotificationServicesPushController', () => { it('should fail if a jwt token is not provided', async () => { arrangeServicesMocks(); + mockErrorLog(); const { controller, messenger } = arrangeMockMessenger(); mockAuthBearerTokenCall(messenger).mockResolvedValue( null as unknown as string, diff --git a/packages/notification-services-controller/src/NotificationServicesPushController/services/services.test.ts b/packages/notification-services-controller/src/NotificationServicesPushController/services/services.test.ts index 694d907f234..447321e1de1 100644 --- a/packages/notification-services-controller/src/NotificationServicesPushController/services/services.test.ts +++ b/packages/notification-services-controller/src/NotificationServicesPushController/services/services.test.ts @@ -1,3 +1,5 @@ +import log from 'loglevel'; + import { mockEndpointGetPushNotificationLinks, mockEndpointUpdatePushNotificationLinks, @@ -13,6 +15,10 @@ import { updateTriggerPushNotifications, } from './services'; +// Testing util to clean up verbose logs when testing errors +const mockErrorLog = () => + jest.spyOn(log, 'error').mockImplementation(jest.fn()); + const MOCK_REG_TOKEN = 'REG_TOKEN'; const MOCK_NEW_REG_TOKEN = 'NEW_REG_TOKEN'; const MOCK_MOBILE_FCM_TOKEN = 'mockMobileFcmToken'; @@ -31,6 +37,7 @@ describe('NotificationServicesPushController Services', () => { it('should return null if given a bad response', async () => { const mockAPI = mockEndpointGetPushNotificationLinks({ status: 500 }); + mockErrorLog(); const result = await getPushNotificationLinks(MOCK_JWT); expect(mockAPI.isDone()).toBe(true); expect(result).toBeNull(); @@ -108,6 +115,7 @@ describe('NotificationServicesPushController Services', () => { it('should successfully call APIs and add provided mobile fcmToken', async () => { const { mobileParams, apis } = arrangeMocks(); + mockErrorLog(); const result = await activatePushNotifications(mobileParams); expect(apis.mockGet.isDone()).toBe(true); @@ -119,6 +127,7 @@ describe('NotificationServicesPushController Services', () => { it('should return null if unable to get links from API', async () => { const { params, apis } = arrangeMocks({ mockGet: { status: 500 } }); + mockErrorLog(); const result = await activatePushNotifications(params); expect(apis.mockGet.isDone()).toBe(true); @@ -177,6 +186,7 @@ describe('NotificationServicesPushController Services', () => { it('should return early when there is no registration token to delete', async () => { const { params, apis } = arrangeMocks(); + mockErrorLog(); const result = await deactivatePushNotifications({ ...params, regToken: '', @@ -191,6 +201,7 @@ describe('NotificationServicesPushController Services', () => { it('should return false when unable to get links api', async () => { const { params, apis } = arrangeMocks({ mockGet: { status: 500 } }); + mockErrorLog(); const result = await deactivatePushNotifications(params); expect(apis.mockGet.isDone()).toBe(true); @@ -250,6 +261,7 @@ describe('NotificationServicesPushController Services', () => { it('should update trigger links and replace existing reg token', async () => { const { params, apis } = arrangeMocks(); + mockErrorLog(); const result = await updateTriggerPushNotifications(params); expect(apis.mockGet.isDone()).toBe(true); @@ -263,6 +275,7 @@ describe('NotificationServicesPushController Services', () => { it('should return early if fails to get links api', async () => { const { params, apis } = arrangeMocks({ mockGet: { status: 500 } }); + mockErrorLog(); const result = await updateTriggerPushNotifications(params); expect(apis.mockGet.isDone()).toBe(true);