Skip to content

Commit 6dc536e

Browse files
authored
fix: link existing push token when notification accounts are added (#8449)
## Explanation **Summary** When notification accounts are added after push notifications have already been enabled, we were updating on-chain triggers, but not re-linking the existing Firebase token to the newly added addresses. As a result, the backend never received the additive `POST /api/v2/token` call for those new (address, token) pairs. This change adds a dedicated additive push-link path that reuses the current `fcmToken` and links only the newly added addresses, without rotating the token or sending oldToken. Because notification account discovery already includes all keyrings, this now covers: - adding accounts on the primary SRP - adding accounts on imported/subsequent SRPs **Code Changes** - Add NotificationServicesPushController:addPushNotificationLinks - Use the existing fcmToken to POST newly added addresses to the push registration endpoint - Call that new method from NotificationServicesController.enableAccounts(...) after on-chain notifications are enabled - Add focused tests for both the controller flow and the push-controller API call - Update the package changelog <!-- Thanks for your contribution! Take a moment to answer these questions so that reviewers have the information they need to properly understand your changes: * What is the current state of things and why does it need to change? * What is the solution your changes offer and how does it work? * Are there any changes whose purpose might not obvious to those unfamiliar with the domain? * If your primary goal was to update one package but you found you had to update another one along the way, why did you do so? * If you had to upgrade a dependency, why did you do so? --> ## References <!-- Are there any issues that this pull request is tied to? Are there other links that reviewers should consult to understand these changes better? Are there client or consumer pull requests to adopt any breaking changes? For example: * Fixes #12345 * Related to #67890 --> ## 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 communicated my changes to consumers by [updating changelogs for packages I've changed](https://github.com/MetaMask/core/tree/main/docs/processes/updating-changelogs.md) - [ ] I've introduced [breaking changes](https://github.com/MetaMask/core/tree/main/docs/processes/breaking-changes.md) in this PR and have prepared draft pull requests for clients and consumer packages to resolve them <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Updates push-notification registration flow to add backend token links when accounts are enabled, affecting server-side delivery for newly added addresses. Risk is moderate due to changes in notification subscription side effects and new messenger action surface, but behavior is guarded and tested. > > **Overview** > Fixes a gap where enabling notifications for newly added accounts updated on-chain triggers but **did not register the existing device push token** for those addresses. > > Adds `NotificationServicesPushController:addPushNotificationLinks` (with exported action type) to POST additive `(address, token)` links using the current `fcmToken`, and wires `NotificationServicesController.enableAccounts()` to invoke it after enabling on-chain notifications (silent-fail like other push calls). Tests are updated to cover the new controller call and the push-controller API call/guard conditions, and the changelog documents the fix. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 7a8eae8. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent 2311c16 commit 6dc536e

File tree

7 files changed

+157
-1
lines changed

7 files changed

+157
-1
lines changed

packages/notification-services-controller/CHANGELOG.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
1414
- Bump `@metamask/controller-utils` from `^11.19.0` to `^11.20.0` ([#8344](https://github.com/MetaMask/core/pull/8344))
1515
- Bump `@metamask/messenger` from `^1.0.0` to `^1.1.1` ([#8364](https://github.com/MetaMask/core/pull/8364), [#8373](https://github.com/MetaMask/core/pull/8373))
1616

17+
### Fixed
18+
19+
- Add backend push token links for newly added notification accounts using the existing device token, so account additions trigger additive `POST /api/v2/token` registration for both primary and imported SRPs ([#8449](https://github.com/MetaMask/core/pull/8449))
20+
- Add the `NotificationServicesPushController:addPushNotificationLinks` messenger action and export the corresponding `NotificationServicesPushControllerAddPushNotificationLinksAction` type so clients can allow and type the new additive push-link flow.
21+
1722
## [23.0.1]
1823

1924
### Changed

packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.test.ts

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import log from 'loglevel';
1616
import type nock from 'nock';
1717

1818
import type {
19+
NotificationServicesPushControllerAddPushNotificationLinksAction,
1920
NotificationServicesPushControllerDisablePushNotificationsAction,
2021
NotificationServicesPushControllerDeletePushNotificationLinksAction,
2122
NotificationServicesPushControllerEnablePushNotificationsAction,
@@ -819,7 +820,11 @@ describe('NotificationServicesController', () => {
819820
};
820821

821822
it('enables notifications for given accounts', async () => {
822-
const { messenger, mockUpdateNotifications } = arrangeMocks();
823+
const {
824+
messenger,
825+
mockAddPushNotificationLinks,
826+
mockUpdateNotifications,
827+
} = arrangeMocks();
823828
const controller = new NotificationServicesController({
824829
messenger,
825830
env: { featureAnnouncements: featureAnnouncementsEnv },
@@ -828,6 +833,7 @@ describe('NotificationServicesController', () => {
828833
await controller.enableAccounts([ADDRESS_1]);
829834

830835
expect(mockUpdateNotifications.isDone()).toBe(true);
836+
expect(mockAddPushNotificationLinks).toHaveBeenCalledWith([ADDRESS_1]);
831837
});
832838

833839
it('throws errors when invalid auth', async () => {
@@ -1712,6 +1718,7 @@ function mockNotificationMessenger(): {
17121718
mockGetBearerToken: jest.Mock;
17131719
mockIsSignedIn: jest.Mock;
17141720
mockAuthPerformSignIn: jest.Mock;
1721+
mockAddPushNotificationLinks: jest.Mock;
17151722
mockDisablePushNotifications: jest.Mock;
17161723
mockDeletePushNotificationLinks: jest.Mock;
17171724
mockEnablePushNotifications: jest.Mock;
@@ -1737,6 +1744,7 @@ function mockNotificationMessenger(): {
17371744
'AuthenticationController:getBearerToken',
17381745
'AuthenticationController:isSignedIn',
17391746
'AuthenticationController:performSignIn',
1747+
'NotificationServicesPushController:addPushNotificationLinks',
17401748
'NotificationServicesPushController:disablePushNotifications',
17411749
'NotificationServicesPushController:deletePushNotificationLinks',
17421750
'NotificationServicesPushController:enablePushNotifications',
@@ -1766,6 +1774,11 @@ function mockNotificationMessenger(): {
17661774
['New Access Token'],
17671775
);
17681776

1777+
const mockAddPushNotificationLinks =
1778+
typedMockAction<NotificationServicesPushControllerAddPushNotificationLinksAction>().mockResolvedValue(
1779+
true,
1780+
);
1781+
17691782
const mockDisablePushNotifications =
17701783
typedMockAction<NotificationServicesPushControllerDisablePushNotificationsAction>();
17711784

@@ -1815,6 +1828,13 @@ function mockNotificationMessenger(): {
18151828
return mockAuthPerformSignIn();
18161829
}
18171830

1831+
if (
1832+
actionType ===
1833+
'NotificationServicesPushController:addPushNotificationLinks'
1834+
) {
1835+
return mockAddPushNotificationLinks(params[0]);
1836+
}
1837+
18181838
if (
18191839
actionType ===
18201840
'NotificationServicesPushController:disablePushNotifications'
@@ -1854,6 +1874,7 @@ function mockNotificationMessenger(): {
18541874
mockGetBearerToken,
18551875
mockIsSignedIn,
18561876
mockAuthPerformSignIn,
1877+
mockAddPushNotificationLinks,
18571878
mockDisablePushNotifications,
18581879
mockDeletePushNotificationLinks,
18591880
mockEnablePushNotifications,

packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -342,6 +342,16 @@ export class NotificationServicesController extends BaseController<
342342
// Do nothing, failing silently.
343343
}
344344
},
345+
addPushNotificationLinks: async (addresses: string[]): Promise<void> => {
346+
try {
347+
await this.messenger.call(
348+
'NotificationServicesPushController:addPushNotificationLinks',
349+
addresses,
350+
);
351+
} catch {
352+
// Do nothing, failing silently.
353+
}
354+
},
345355
disablePushNotifications: async (): Promise<void> => {
346356
try {
347357
await this.messenger.call(
@@ -983,6 +993,8 @@ export class NotificationServicesController extends BaseController<
983993
accounts.map((address) => ({ address, enabled: true })),
984994
this.#env,
985995
);
996+
997+
await this.#pushNotifications.addPushNotificationLinks(accounts);
986998
} catch (error) {
987999
log.error('Failed to update OnChain triggers', error);
9881000
throw new Error('Failed to update OnChain triggers');

packages/notification-services-controller/src/NotificationServicesPushController/NotificationServicesPushController-method-action-types.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,19 @@ export type NotificationServicesPushControllerDisablePushNotificationsAction = {
3535
handler: NotificationServicesPushController['disablePushNotifications'];
3636
};
3737

38+
/**
39+
* Adds backend push notification links for the given addresses using the current FCM token.
40+
* This is used when accounts are added after push notifications have already been enabled,
41+
* so backend can link the existing device token to the newly added addresses.
42+
*
43+
* @param addresses - Addresses that should be linked to push notifications.
44+
* @returns Whether the add request succeeded.
45+
*/
46+
export type NotificationServicesPushControllerAddPushNotificationLinksAction = {
47+
type: `NotificationServicesPushController:addPushNotificationLinks`;
48+
handler: NotificationServicesPushController['addPushNotificationLinks'];
49+
};
50+
3851
/**
3952
* Deletes backend push notification links for the given addresses on the current platform.
4053
* This is used when accounts are removed (for example SRP removal), so backend can remove
@@ -70,5 +83,6 @@ export type NotificationServicesPushControllerMethodActions =
7083
| NotificationServicesPushControllerSubscribeToPushNotificationsAction
7184
| NotificationServicesPushControllerEnablePushNotificationsAction
7285
| NotificationServicesPushControllerDisablePushNotificationsAction
86+
| NotificationServicesPushControllerAddPushNotificationLinksAction
7387
| NotificationServicesPushControllerDeletePushNotificationLinksAction
7488
| NotificationServicesPushControllerUpdateTriggerPushNotificationsAction;

packages/notification-services-controller/src/NotificationServicesPushController/NotificationServicesPushController.test.ts

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ describe('NotificationServicesPushController', () => {
2525
): {
2626
activatePushNotificationsMock: jest.SpyInstance;
2727
deactivatePushNotificationsMock: jest.SpyInstance;
28+
updateLinksAPIMock: jest.SpyInstance;
2829
deleteLinksAPIMock: jest.SpyInstance;
2930
} => {
3031
const activatePushNotificationsMock = jest
@@ -35,13 +36,18 @@ describe('NotificationServicesPushController', () => {
3536
.spyOn(services, 'deactivatePushNotifications')
3637
.mockResolvedValue(true);
3738

39+
const updateLinksAPIMock = jest
40+
.spyOn(services, 'updateLinksAPI')
41+
.mockResolvedValue(true);
42+
3843
const deleteLinksAPIMock = jest
3944
.spyOn(services, 'deleteLinksAPI')
4045
.mockResolvedValue(true);
4146

4247
return {
4348
activatePushNotificationsMock,
4449
deactivatePushNotificationsMock,
50+
updateLinksAPIMock,
4551
deleteLinksAPIMock,
4652
};
4753
};
@@ -347,6 +353,67 @@ describe('NotificationServicesPushController', () => {
347353
});
348354
});
349355

356+
describe('addPushNotificationLinks', () => {
357+
afterEach(() => {
358+
jest.clearAllMocks();
359+
});
360+
361+
it('should call updateLinksAPI with addresses and the existing token', async () => {
362+
const mocks = arrangeServicesMocks();
363+
const { controller, messenger } = arrangeMockMessenger({
364+
state: {
365+
fcmToken: MOCK_FCM_TOKEN,
366+
isPushEnabled: true,
367+
isUpdatingFCMToken: false,
368+
},
369+
});
370+
mockAuthBearerTokenCall(messenger);
371+
372+
const result = await controller.addPushNotificationLinks(MOCK_ADDRESSES);
373+
374+
expect(mocks.updateLinksAPIMock).toHaveBeenCalledWith({
375+
bearerToken: MOCK_JWT,
376+
addresses: MOCK_ADDRESSES,
377+
regToken: {
378+
token: MOCK_FCM_TOKEN,
379+
platform: 'extension',
380+
locale: 'en',
381+
},
382+
env: 'prd',
383+
});
384+
expect(result).toBe(true);
385+
});
386+
387+
it('should return false when push feature is disabled', async () => {
388+
const mocks = arrangeServicesMocks();
389+
const { controller } = arrangeMockMessenger({
390+
isPushFeatureEnabled: false,
391+
});
392+
393+
const result = await controller.addPushNotificationLinks(MOCK_ADDRESSES);
394+
395+
expect(mocks.updateLinksAPIMock).not.toHaveBeenCalled();
396+
expect(result).toBe(false);
397+
});
398+
399+
it('should return false when there is no token to add', async () => {
400+
const mocks = arrangeServicesMocks();
401+
const { controller, messenger } = arrangeMockMessenger({
402+
state: {
403+
fcmToken: '',
404+
isPushEnabled: true,
405+
isUpdatingFCMToken: false,
406+
},
407+
});
408+
mockAuthBearerTokenCall(messenger);
409+
410+
const result = await controller.addPushNotificationLinks(MOCK_ADDRESSES);
411+
412+
expect(mocks.updateLinksAPIMock).not.toHaveBeenCalled();
413+
expect(result).toBe(false);
414+
});
415+
});
416+
350417
describe('metadata', () => {
351418
it('includes expected state in debug snapshots', () => {
352419
const { controller } = arrangeMockMessenger();

packages/notification-services-controller/src/NotificationServicesPushController/NotificationServicesPushController.ts

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import {
1515
activatePushNotifications,
1616
deleteLinksAPI,
1717
deactivatePushNotifications,
18+
updateLinksAPI,
1819
} from './services/services';
1920
import type { PushNotificationEnv } from './types';
2021
import type { PushService } from './types/push-service-interface';
@@ -30,6 +31,7 @@ export type NotificationServicesPushControllerState = {
3031
const MESSENGER_EXPOSED_METHODS = [
3132
'subscribeToPushNotifications',
3233
'enablePushNotifications',
34+
'addPushNotificationLinks',
3335
'disablePushNotifications',
3436
'updateTriggerPushNotifications',
3537
'deletePushNotificationLinks',
@@ -357,6 +359,40 @@ export class NotificationServicesPushController extends BaseController<
357359
this.#updatePushState({ type: 'disable' });
358360
}
359361

362+
/**
363+
* Adds backend push notification links for the given addresses using the current FCM token.
364+
* This is used when accounts are added after push notifications have already been enabled,
365+
* so backend can link the existing device token to the newly added addresses.
366+
*
367+
* @param addresses - Addresses that should be linked to push notifications.
368+
* @returns Whether the add request succeeded.
369+
*/
370+
public async addPushNotificationLinks(addresses: string[]): Promise<boolean> {
371+
if (
372+
!this.#config.isPushFeatureEnabled ||
373+
addresses.length === 0 ||
374+
!this.state.fcmToken
375+
) {
376+
return false;
377+
}
378+
379+
try {
380+
const bearerToken = await this.#getAndAssertBearerToken();
381+
return await updateLinksAPI({
382+
bearerToken,
383+
addresses,
384+
regToken: {
385+
token: this.state.fcmToken,
386+
platform: this.#config.platform,
387+
locale: this.#config.getLocale?.() ?? 'en',
388+
},
389+
env: this.#config.env ?? 'prd',
390+
});
391+
} catch {
392+
return false;
393+
}
394+
}
395+
360396
/**
361397
* Deletes backend push notification links for the given addresses on the current platform.
362398
* This is used when accounts are removed (for example SRP removal), so backend can remove

packages/notification-services-controller/src/NotificationServicesPushController/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ export * as Mocks from './mocks';
1212
export type {
1313
NotificationServicesPushControllerSubscribeToPushNotificationsAction,
1414
NotificationServicesPushControllerEnablePushNotificationsAction,
15+
NotificationServicesPushControllerAddPushNotificationLinksAction,
1516
NotificationServicesPushControllerDisablePushNotificationsAction,
1617
NotificationServicesPushControllerUpdateTriggerPushNotificationsAction,
1718
NotificationServicesPushControllerDeletePushNotificationLinksAction,

0 commit comments

Comments
 (0)