From 7d2b97b1dfd6802401da20135e42ea8d7e03e1ef Mon Sep 17 00:00:00 2001 From: Justin Baur <19896123+justindbaur@users.noreply.github.com> Date: Wed, 25 Jun 2025 21:42:06 -0400 Subject: [PATCH] [PM-22573] Don't call server on existing subscription (#15154) * Don't put subscription to our server when it's existing * Only update server when subscription-user associations change --------- Co-authored-by: Matt Gibson --- .../browser/src/background/main.background.ts | 1 + .../worker-webpush-connection.service.spec.ts | 387 ++++++++++++++++++ .../worker-webpush-connection.service.ts | 70 +++- .../src/platform/state/state-definitions.ts | 3 + 4 files changed, 445 insertions(+), 16 deletions(-) create mode 100644 libs/common/src/platform/notifications/internal/worker-webpush-connection.service.spec.ts diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index 8acbd4373a0..18fa463f7ad 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -1087,6 +1087,7 @@ export default class MainBackground { this.configService, new WebPushNotificationsApiService(this.apiService, this.appIdService), registration, + this.stateProvider, ); } else { this.webPushConnectionService = new UnsupportedWebPushConnectionService(); diff --git a/libs/common/src/platform/notifications/internal/worker-webpush-connection.service.spec.ts b/libs/common/src/platform/notifications/internal/worker-webpush-connection.service.spec.ts new file mode 100644 index 00000000000..6d9457389ca --- /dev/null +++ b/libs/common/src/platform/notifications/internal/worker-webpush-connection.service.spec.ts @@ -0,0 +1,387 @@ +import { mock, MockProxy } from "jest-mock-extended"; +import { firstValueFrom, of } from "rxjs"; + +import { + awaitAsync, + FakeGlobalState, + FakeStateProvider, + mockAccountServiceWith, +} from "../../../../spec"; +import { PushTechnology } from "../../../enums/push-technology.enum"; +import { UserId } from "../../../types/guid"; +import { ConfigService } from "../../abstractions/config/config.service"; +import { ServerConfig } from "../../abstractions/config/server-config"; +import { Supported } from "../../misc/support-status"; +import { Utils } from "../../misc/utils"; +import { ServerConfigData } from "../../models/data/server-config.data"; +import { PushSettingsConfigResponse } from "../../models/response/server-config.response"; +import { KeyDefinition } from "../../state"; + +import { WebPushNotificationsApiService } from "./web-push-notifications-api.service"; +import { WebPushConnector } from "./webpush-connection.service"; +import { + WEB_PUSH_SUBSCRIPTION_USERS, + WorkerWebPushConnectionService, +} from "./worker-webpush-connection.service"; + +const mockUser1 = "testUser1" as UserId; + +const createSub = (key: string) => { + return { + options: { applicationServerKey: Utils.fromUrlB64ToArray(key), userVisibleOnly: true }, + endpoint: `web.push.endpoint/?${Utils.newGuid()}`, + expirationTime: 5, + getKey: () => null, + toJSON: () => ({ endpoint: "something", keys: {}, expirationTime: 5 }), + unsubscribe: () => Promise.resolve(true), + } satisfies PushSubscription; +}; + +describe("WorkerWebpushConnectionService", () => { + let configService: MockProxy; + let webPushApiService: MockProxy; + let stateProvider: FakeStateProvider; + let pushManager: MockProxy; + const userId = "testUser1" as UserId; + + let sut: WorkerWebPushConnectionService; + + beforeEach(() => { + configService = mock(); + webPushApiService = mock(); + stateProvider = new FakeStateProvider(mockAccountServiceWith(userId)); + pushManager = mock(); + + sut = new WorkerWebPushConnectionService( + configService, + webPushApiService, + mock({ pushManager: pushManager }), + stateProvider, + ); + }); + + afterEach(() => { + jest.resetAllMocks(); + }); + type ExtractKeyDefinitionType = T extends KeyDefinition ? U : never; + describe("supportStatus$", () => { + let fakeGlobalState: FakeGlobalState< + ExtractKeyDefinitionType + >; + + beforeEach(() => { + fakeGlobalState = stateProvider.getGlobal(WEB_PUSH_SUBSCRIPTION_USERS) as FakeGlobalState< + ExtractKeyDefinitionType + >; + }); + + test("when web push is supported, have an existing subscription, and we've already registered the user, should not call API", async () => { + configService.serverConfig$ = of( + new ServerConfig( + new ServerConfigData({ + push: new PushSettingsConfigResponse({ + pushTechnology: PushTechnology.WebPush, + vapidPublicKey: "dGVzdA", + }), + }), + ), + ); + const existingSubscription = createSub("dGVzdA"); + await fakeGlobalState.nextState({ [existingSubscription.endpoint]: [userId] }); + + pushManager.getSubscription.mockResolvedValue(existingSubscription); + + const supportStatus = await firstValueFrom(sut.supportStatus$(mockUser1)); + expect(supportStatus.type).toBe("supported"); + const service = (supportStatus as Supported).service; + expect(service).not.toBeFalsy(); + + const notificationsSub = service.notifications$.subscribe(); + + await awaitAsync(2); + + expect(pushManager.getSubscription).toHaveBeenCalledTimes(1); + expect(webPushApiService.putSubscription).toHaveBeenCalledTimes(0); + + expect(fakeGlobalState.nextMock).toHaveBeenCalledTimes(0); + + notificationsSub.unsubscribe(); + }); + + test("when web push is supported, have an existing subscription, and we haven't registered the user, should call API", async () => { + configService.serverConfig$ = of( + new ServerConfig( + new ServerConfigData({ + push: new PushSettingsConfigResponse({ + pushTechnology: PushTechnology.WebPush, + vapidPublicKey: "dGVzdA", + }), + }), + ), + ); + const existingSubscription = createSub("dGVzdA"); + await fakeGlobalState.nextState({ + [existingSubscription.endpoint]: ["otherUserId" as UserId], + }); + + pushManager.getSubscription.mockResolvedValue(existingSubscription); + + const supportStatus = await firstValueFrom(sut.supportStatus$(mockUser1)); + expect(supportStatus.type).toBe("supported"); + const service = (supportStatus as Supported).service; + expect(service).not.toBeFalsy(); + + const notificationsSub = service.notifications$.subscribe(); + + await awaitAsync(2); + + expect(pushManager.getSubscription).toHaveBeenCalledTimes(1); + expect(webPushApiService.putSubscription).toHaveBeenCalledTimes(1); + + expect(fakeGlobalState.nextMock).toHaveBeenCalledTimes(1); + expect(fakeGlobalState.nextMock).toHaveBeenCalledWith({ + [existingSubscription.endpoint]: ["otherUserId", mockUser1], + }); + + notificationsSub.unsubscribe(); + }); + + test("when web push is supported, have an existing subscription, but it isn't in state, should call API and add to state", async () => { + configService.serverConfig$ = of( + new ServerConfig( + new ServerConfigData({ + push: new PushSettingsConfigResponse({ + pushTechnology: PushTechnology.WebPush, + vapidPublicKey: "dGVzdA", + }), + }), + ), + ); + const existingSubscription = createSub("dGVzdA"); + await fakeGlobalState.nextState({ + [existingSubscription.endpoint]: null!, + }); + + pushManager.getSubscription.mockResolvedValue(existingSubscription); + + const supportStatus = await firstValueFrom(sut.supportStatus$(mockUser1)); + expect(supportStatus.type).toBe("supported"); + const service = (supportStatus as Supported).service; + expect(service).not.toBeFalsy(); + + const notificationsSub = service.notifications$.subscribe(); + + await awaitAsync(2); + + expect(pushManager.getSubscription).toHaveBeenCalledTimes(1); + expect(webPushApiService.putSubscription).toHaveBeenCalledTimes(1); + + expect(fakeGlobalState.nextMock).toHaveBeenCalledTimes(1); + expect(fakeGlobalState.nextMock).toHaveBeenCalledWith({ + [existingSubscription.endpoint]: [mockUser1], + }); + + notificationsSub.unsubscribe(); + }); + + test("when web push is supported, have an existing subscription, but state array is null, should call API and add to state", async () => { + configService.serverConfig$ = of( + new ServerConfig( + new ServerConfigData({ + push: new PushSettingsConfigResponse({ + pushTechnology: PushTechnology.WebPush, + vapidPublicKey: "dGVzdA", + }), + }), + ), + ); + const existingSubscription = createSub("dGVzdA"); + await fakeGlobalState.nextState({}); + + pushManager.getSubscription.mockResolvedValue(existingSubscription); + + const supportStatus = await firstValueFrom(sut.supportStatus$(mockUser1)); + expect(supportStatus.type).toBe("supported"); + const service = (supportStatus as Supported).service; + expect(service).not.toBeFalsy(); + + const notificationsSub = service.notifications$.subscribe(); + + await awaitAsync(2); + + expect(pushManager.getSubscription).toHaveBeenCalledTimes(1); + expect(webPushApiService.putSubscription).toHaveBeenCalledTimes(1); + + expect(fakeGlobalState.nextMock).toHaveBeenCalledTimes(1); + expect(fakeGlobalState.nextMock).toHaveBeenCalledWith({ + [existingSubscription.endpoint]: [mockUser1], + }); + + notificationsSub.unsubscribe(); + }); + + test("when web push is supported, but we don't have an existing subscription, should call the api and wipe out existing state", async () => { + configService.serverConfig$ = of( + new ServerConfig( + new ServerConfigData({ + push: new PushSettingsConfigResponse({ + pushTechnology: PushTechnology.WebPush, + vapidPublicKey: "dGVzdA", + }), + }), + ), + ); + const existingState = createSub("dGVzdA"); + await fakeGlobalState.nextState({ [existingState.endpoint]: [userId] }); + + pushManager.getSubscription.mockResolvedValue(null); + const newSubscription = createSub("dGVzdA"); + pushManager.subscribe.mockResolvedValue(newSubscription); + + const supportStatus = await firstValueFrom(sut.supportStatus$(mockUser1)); + expect(supportStatus.type).toBe("supported"); + const service = (supportStatus as Supported).service; + expect(service).not.toBeFalsy(); + + const notificationsSub = service.notifications$.subscribe(); + + await awaitAsync(2); + + expect(pushManager.getSubscription).toHaveBeenCalledTimes(1); + expect(webPushApiService.putSubscription).toHaveBeenCalledTimes(1); + + expect(fakeGlobalState.nextMock).toHaveBeenCalledTimes(1); + expect(fakeGlobalState.nextMock).toHaveBeenCalledWith({ + [newSubscription.endpoint]: [mockUser1], + }); + + notificationsSub.unsubscribe(); + }); + + test("when web push is supported and no existing subscription, should call API", async () => { + configService.serverConfig$ = of( + new ServerConfig( + new ServerConfigData({ + push: new PushSettingsConfigResponse({ + pushTechnology: PushTechnology.WebPush, + vapidPublicKey: "dGVzdA", + }), + }), + ), + ); + + pushManager.getSubscription.mockResolvedValue(null); + pushManager.subscribe.mockResolvedValue(createSub("dGVzdA")); + + const supportStatus = await firstValueFrom(sut.supportStatus$(mockUser1)); + expect(supportStatus.type).toBe("supported"); + const service = (supportStatus as Supported).service; + expect(service).not.toBeFalsy(); + + const notificationsSub = service.notifications$.subscribe(); + + await awaitAsync(2); + + expect(pushManager.getSubscription).toHaveBeenCalledTimes(1); + expect(pushManager.subscribe).toHaveBeenCalledTimes(1); + expect(webPushApiService.putSubscription).toHaveBeenCalledTimes(1); + + notificationsSub.unsubscribe(); + }); + + test("when web push is supported and existing subscription with different key, should call API", async () => { + configService.serverConfig$ = of( + new ServerConfig( + new ServerConfigData({ + push: new PushSettingsConfigResponse({ + pushTechnology: PushTechnology.WebPush, + vapidPublicKey: "dGVzdA", + }), + }), + ), + ); + + pushManager.getSubscription.mockResolvedValue(createSub("dGVzdF9hbHQ")); + + pushManager.subscribe.mockResolvedValue(createSub("dGVzdA")); + + const supportStatus = await firstValueFrom(sut.supportStatus$(mockUser1)); + expect(supportStatus.type).toBe("supported"); + const service = (supportStatus as Supported).service; + expect(service).not.toBeFalsy(); + + const notificationsSub = service.notifications$.subscribe(); + + await awaitAsync(2); + + expect(pushManager.getSubscription).toHaveBeenCalledTimes(1); + expect(pushManager.subscribe).toHaveBeenCalledTimes(1); + expect(webPushApiService.putSubscription).toHaveBeenCalledTimes(1); + + notificationsSub.unsubscribe(); + }); + + test("when server config emits multiple times quickly while api call takes a long time will only call API once", async () => { + configService.serverConfig$ = of( + new ServerConfig( + new ServerConfigData({ + push: new PushSettingsConfigResponse({ + pushTechnology: PushTechnology.WebPush, + vapidPublicKey: "dGVzdA", + }), + }), + ), + ); + + pushManager.getSubscription.mockResolvedValue(createSub("dGVzdF9hbHQ")); + + pushManager.subscribe.mockResolvedValue(createSub("dGVzdA")); + + const supportStatus = await firstValueFrom(sut.supportStatus$(mockUser1)); + expect(supportStatus.type).toBe("supported"); + const service = (supportStatus as Supported).service; + expect(service).not.toBeFalsy(); + + const notificationsSub = service.notifications$.subscribe(); + + await awaitAsync(2); + + expect(pushManager.getSubscription).toHaveBeenCalledTimes(1); + expect(pushManager.subscribe).toHaveBeenCalledTimes(1); + expect(webPushApiService.putSubscription).toHaveBeenCalledTimes(1); + + notificationsSub.unsubscribe(); + }); + + it("server config shows SignalR support should return not-supported", async () => { + configService.serverConfig$ = of( + new ServerConfig( + new ServerConfigData({ + push: new PushSettingsConfigResponse({ + pushTechnology: PushTechnology.SignalR, + }), + }), + ), + ); + + const supportStatus = await firstValueFrom(sut.supportStatus$(mockUser1)); + expect(supportStatus.type).toBe("not-supported"); + }); + + it("server config shows web push but no public key support should return not-supported", async () => { + configService.serverConfig$ = of( + new ServerConfig( + new ServerConfigData({ + push: new PushSettingsConfigResponse({ + pushTechnology: PushTechnology.WebPush, + vapidPublicKey: null, + }), + }), + ), + ); + + const supportStatus = await firstValueFrom(sut.supportStatus$(mockUser1)); + expect(supportStatus.type).toBe("not-supported"); + }); + }); +}); diff --git a/libs/common/src/platform/notifications/internal/worker-webpush-connection.service.ts b/libs/common/src/platform/notifications/internal/worker-webpush-connection.service.ts index a1143d14d1d..528ad90ed61 100644 --- a/libs/common/src/platform/notifications/internal/worker-webpush-connection.service.ts +++ b/libs/common/src/platform/notifications/internal/worker-webpush-connection.service.ts @@ -9,6 +9,7 @@ import { Subject, Subscription, switchMap, + withLatestFrom, } from "rxjs"; import { PushTechnology } from "../../../enums/push-technology.enum"; @@ -17,6 +18,7 @@ import { UserId } from "../../../types/guid"; import { ConfigService } from "../../abstractions/config/config.service"; import { SupportStatus } from "../../misc/support-status"; import { Utils } from "../../misc/utils"; +import { KeyDefinition, StateProvider, WEB_PUSH_SUBSCRIPTION } from "../../state"; import { WebPushNotificationsApiService } from "./web-push-notifications-api.service"; import { WebPushConnectionService, WebPushConnector } from "./webpush-connection.service"; @@ -48,6 +50,7 @@ export class WorkerWebPushConnectionService implements WebPushConnectionService private readonly configService: ConfigService, private readonly webPushApiService: WebPushNotificationsApiService, private readonly serviceWorkerRegistration: ServiceWorkerRegistration, + private readonly stateProvider: StateProvider, ) {} start(): Subscription { @@ -97,6 +100,7 @@ export class WorkerWebPushConnectionService implements WebPushConnectionService this.serviceWorkerRegistration, this.pushEvent, this.pushChangeEvent, + this.stateProvider, ), } satisfies SupportStatus; }), @@ -114,20 +118,36 @@ class MyWebPushConnector implements WebPushConnector { private readonly serviceWorkerRegistration: ServiceWorkerRegistration, private readonly pushEvent$: Observable, private readonly pushChangeEvent$: Observable, + private readonly stateProvider: StateProvider, ) { + const subscriptionUsersState = this.stateProvider.getGlobal(WEB_PUSH_SUBSCRIPTION_USERS); this.notifications$ = this.getOrCreateSubscription$(this.vapidPublicKey).pipe( - concatMap((subscription) => { - return defer(() => { - if (subscription == null) { - throw new Error("Expected a non-null subscription."); - } - return this.webPushApiService.putSubscription(subscription.toJSON()); - }).pipe( - switchMap(() => this.pushEvent$), - map((e) => { - return new NotificationResponse(e.data.json().data); - }), - ); + withLatestFrom(subscriptionUsersState.state$.pipe(map((x) => x ?? {}))), + concatMap(async ([[isExistingSubscription, subscription], subscriptionUsers]) => { + if (subscription == null) { + throw new Error("Expected a non-null subscription."); + } + + // If this is a new subscription, we can clear state and start over + if (!isExistingSubscription) { + subscriptionUsers = {}; + } + + // If the user is already subscribed, we don't need to do anything + if (subscriptionUsers[subscription.endpoint]?.includes(this.userId)) { + return; + } + subscriptionUsers[subscription.endpoint] ??= []; + subscriptionUsers[subscription.endpoint].push(this.userId); + // Update the state with the new subscription-user association + await subscriptionUsersState.update(() => subscriptionUsers); + + // Inform the server about the new subscription-user association + await this.webPushApiService.putSubscription(subscription.toJSON()); + }), + switchMap(() => this.pushEvent$), + map((e) => { + return new NotificationResponse(e.data.json().data); }), ); } @@ -146,7 +166,7 @@ class MyWebPushConnector implements WebPushConnector { await this.serviceWorkerRegistration.pushManager.getSubscription(); if (existingSubscription == null) { - return await this.pushManagerSubscribe(key); + return [false, await this.pushManagerSubscribe(key)] as const; } const subscriptionKey = Utils.fromBufferToUrlB64( @@ -159,12 +179,30 @@ class MyWebPushConnector implements WebPushConnector { if (subscriptionKey !== key) { // There is a subscription, but it's not for the current server, unsubscribe and then make a new one await existingSubscription.unsubscribe(); - return await this.pushManagerSubscribe(key); + return [false, await this.pushManagerSubscribe(key)] as const; } - return existingSubscription; + return [true, existingSubscription] as const; }), - this.pushChangeEvent$.pipe(map((event) => event.newSubscription)), + this.pushChangeEvent$.pipe(map((event) => [false, event.newSubscription] as const)), ); } } + +export const WEB_PUSH_SUBSCRIPTION_USERS = new KeyDefinition>( + WEB_PUSH_SUBSCRIPTION, + "subUsers", + { + deserializer: (obj) => { + if (obj == null) { + return {}; + } + + const result: Record = {}; + for (const [key, value] of Object.entries(obj)) { + result[key] = Array.isArray(value) ? value : []; + } + return result; + }, + }, +); diff --git a/libs/common/src/platform/state/state-definitions.ts b/libs/common/src/platform/state/state-definitions.ts index 563f8404931..f9e6a5007c7 100644 --- a/libs/common/src/platform/state/state-definitions.ts +++ b/libs/common/src/platform/state/state-definitions.ts @@ -128,6 +128,9 @@ export const EXTENSION_INITIAL_INSTALL_DISK = new StateDefinition( "extensionInitialInstall", "disk", ); +export const WEB_PUSH_SUBSCRIPTION = new StateDefinition("webPushSubscription", "disk", { + web: "disk-local", +}); // Design System