[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 <mgibson@bitwarden.com>

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

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<ConfigService>;
+  let webPushApiService: MockProxy<WebPushNotificationsApiService>;
+  let stateProvider: FakeStateProvider;
+  let pushManager: MockProxy<PushManager>;
+  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<ServiceWorkerRegistration>({ pushManager: pushManager }),
+      stateProvider,
+    );
+  });
+
+  afterEach(() => {
+    jest.resetAllMocks();
+  });
+  type ExtractKeyDefinitionType<T> = T extends KeyDefinition<infer U> ? U : never;
+  describe("supportStatus$", () => {
+    let fakeGlobalState: FakeGlobalState<
+      ExtractKeyDefinitionType<typeof WEB_PUSH_SUBSCRIPTION_USERS>
+    >;
+
+    beforeEach(() => {
+      fakeGlobalState = stateProvider.getGlobal(WEB_PUSH_SUBSCRIPTION_USERS) as FakeGlobalState<
+        ExtractKeyDefinitionType<typeof WEB_PUSH_SUBSCRIPTION_USERS>
+      >;
+    });
+
+    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<WebPushConnector>).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<WebPushConnector>).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<WebPushConnector>).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<WebPushConnector>).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<WebPushConnector>).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<WebPushConnector>).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<WebPushConnector>).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<WebPushConnector>).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");
+    });
+  });
+});

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<WebPushConnector>;
       }),
@@ -114,20 +118,36 @@ class MyWebPushConnector implements WebPushConnector {
     private readonly serviceWorkerRegistration: ServiceWorkerRegistration,
     private readonly pushEvent$: Observable<PushEvent>,
     private readonly pushChangeEvent$: Observable<PushSubscriptionChangeEvent>,
+    private readonly stateProvider: StateProvider,
   ) {
+    const subscriptionUsersState = this.stateProvider.getGlobal(WEB_PUSH_SUBSCRIPTION_USERS);
     this.notifications$ = this.getOrCreateSubscription$(this.vapidPublicKey).pipe(
-      concatMap((subscription) => {
+      withLatestFrom(subscriptionUsersState.state$.pipe(map((x) => x ?? {}))),
-        return defer(() => {
+      concatMap(async ([[isExistingSubscription, subscription], subscriptionUsers]) => {
-          if (subscription == null) {
+        if (subscription == null) {
-            throw new Error("Expected a non-null subscription.");
+          throw new Error("Expected a non-null subscription.");
-          }
+        }
-          return this.webPushApiService.putSubscription(subscription.toJSON());
+
-        }).pipe(
+        // If this is a new subscription, we can clear state and start over
-          switchMap(() => this.pushEvent$),
+        if (!isExistingSubscription) {
-          map((e) => {
+          subscriptionUsers = {};
-            return new NotificationResponse(e.data.json().data);
+        }
-          }),
+
-        );
+        // 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<Record<string, UserId[]>>(
+  WEB_PUSH_SUBSCRIPTION,
+  "subUsers",
+  {
+    deserializer: (obj) => {
+      if (obj == null) {
+        return {};
+      }
+
+      const result: Record<string, UserId[]> = {};
+      for (const [key, value] of Object.entries(obj)) {
+        result[key] = Array.isArray(value) ? value : [];
+      }
+      return result;
+    },
+  },
+);

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