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

MetaMask/metamask-extension#28173

## Changelog

<!--
If you're making any consumer-facing changes, list those changes here as
if you were updating a changelog, using the template below as a guide.

(CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or
FIXED. For security-related issues, follow the Security Advisory
process.)

Please take care to name the exact pieces of the API you've added or
changed (e.g. types, interfaces, functions, or methods).

If there are any breaking changes, make sure to offer a solution for
consumers to follow once they upgrade to the changes.

Finally, if you're only making changes to development scripts or tests,
you may replace the template below with "None".
-->

### `@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

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<NotificationServicesControllerState>,
+  ) => {
     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()),

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 = <Item>(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(),

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,

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);