fix: tear down a removed account's push through its own client, not the open account's

src/lib/__tests__/push-notifications.test.ts

@@ -66,6 +66,10 @@ const { DETACHED, SYNC_SESSION } = vi.hoisted(() => {
       // The subscription's expiry, and what an update of it settles on.
       expires: null as string | null,
       settles: null as string | null,
+      // Every subscription of the account, for a listing with ids: null, and
+      // whether that listing fails.
+      all: [] as Array<{ id: string; deviceClientId?: string }>,
+      listFails: false,
       log: [] as unknown[],
       session: SYNC_SESSION,
     },
@@ -96,6 +100,11 @@ vi.mock('../../api/jmap-client', () => ({
     async request(calls: Array<[string, Record<string, any>, string]>) {
       const methodResponses = calls.map(([name, args, id]) => {
         DETACHED.log.push([name, args]);
+        if (name === 'PushSubscription/get' && args.ids === null) {
+          if (DETACHED.listFails) throw new Error('network down');
+          return [name, { list: DETACHED.all }, id];
+        }
+        if (args.destroy) return [name, { destroyed: args.destroy }, id];
         if (name === 'PushSubscription/get') {
           const listed = {
             id: args.ids[0],
@@ -1854,6 +1863,63 @@ describe('teardownPushNotificationsForAccount', () => {
     expect(destroyMock.mock.calls.map((c) => c[0])).toEqual(['recorded']);
     expect(await AsyncStorage.getItem(SUB_KEY)).toBeNull();
   });
+
+  describe('for a signed-in account the client does not serve', () => {
+    const OTHER = 'bob@other.example.net';
+    const OTHER_DCID = 'bobsphonebobsphonebobsphonebobsp';
+    const destroyedThroughOwnClient = () => DETACHED.log
+      .filter((entry) => (entry as unknown[])[0] === 'PushSubscription/set')
+      .map((entry) => ((entry as [string, { destroy: string[] }])[1]).destroy);
+
+    beforeEach(async () => {
+      await AsyncStorage.setItem('push:relayBaseUrl:v1', RELAY);
+      await AsyncStorage.setItem(`push:subscriptionId:v2:${OTHER}`, 'b');
+      await AsyncStorage.setItem(deviceClientIdKey(OTHER), OTHER_DCID);
+      // The open account's own push, with the same short id.
+      listMock.mockResolvedValue([{ id: 'b', types: ['EmailDelivery'] }]);
+      Object.assign(DETACHED, {
+        loaded: true,
+        all: [{ id: 'b', deviceClientId: OTHER_DCID }, { id: 'c', deviceClientId: 'ffffffffffffffffffffffffffffffff' }],
+        listFails: false,
+        log: [],
+      });
+    });
+
+    afterEach(() => {
+      listMock.mockImplementation(async () => []);
+      Object.assign(DETACHED, { loaded: true, all: [], listFails: false });
+    });
+
+    it('destroys its subscriptions through a client of its own', async () => {
+      await teardownPushNotificationsForAccount(OTHER);
+
+      expect(DETACHED.log[0]).toEqual(['load', OTHER]);
+      expect(destroyedThroughOwnClient()).toEqual([['b']]);
+      expect(listMock).not.toHaveBeenCalled();
+      expect(destroyMock).not.toHaveBeenCalled();
+      expect(await AsyncStorage.getItem(`push:subscriptionId:v2:${OTHER}`)).toBeNull();
+    });
+
+    it("never destroys the recorded id on the open account when its own server can't list", async () => {
+      listMock.mockRejectedValue(new Error('network down'));
+      DETACHED.listFails = true;
+
+      await teardownPushNotificationsForAccount(OTHER);
+
+      expect(destroyMock).not.toHaveBeenCalled();
+      expect(destroyedThroughOwnClient()).toEqual([['b']]);
+
+      // An account that can't be loaded leaves every server alone.
+      DETACHED.log = [];
+      DETACHED.loaded = false;
+      await AsyncStorage.setItem(`push:subscriptionId:v2:${OTHER}`, 'b');
+      await AsyncStorage.setItem(deviceClientIdKey(OTHER), OTHER_DCID);
+      await teardownPushNotificationsForAccount(OTHER);
+      expect(destroyMock).not.toHaveBeenCalled();
+      expect(destroyedThroughOwnClient()).toEqual([]);
+      expect(await AsyncStorage.getItem(`push:subscriptionId:v2:${OTHER}`)).toBeNull();
+    });
+  });
 });
 
 describe('notificationTapJmapAccountId', () => {

src/lib/push-notifications.ts

@@ -1682,10 +1682,13 @@ export async function recordPushSubscriptionExpires(accountId: string, expires:
 
 /**
  * Tear down push for a single account. Destroys every JMAP subscription the
- * server holds for this device (assumes the jmapClient is currently
+ * server holds for this device and tells the relay to drop its mapping.
- * authenticated to that account; the active-account logout flow guarantees
+ * Other accounts' push setups are untouched. The account the jmapClient
- * this) and tells the relay to drop its mapping. Other accounts' push setups
+ * serves goes through it; another signed-in account (removed from the
- * are untouched.
+ * account list while a different one is open) through a client of its own,
+ * never the singleton: subscription ids repeat across accounts ("b" in every
+ * Stalwart account), so a destroy by id there could end the open account's
+ * own push.
  *
  * The FCM token is deliberately left alive: the relay mapping is gone so
  * nothing gets forwarded, and deleting the token makes the next getToken()
@@ -1706,8 +1709,9 @@ export async function teardownPushNotificationsForAccount(
   // failed enable can leave a registration this client no longer tracks (#841).
   // The recorded id alone is trusted only when the server can't be asked: on
   // a server reinstalled at the same address it can name another device's.
+  const server = await pushSubscriptionsOf(accountId);
   const idsToDestroy = new Set<string>();
-  const existing = storedDcid ? await listPushSubscriptions().catch(() => null) : null;
+  const existing = server && storedDcid ? await server.list().catch(() => null) : null;
   if (existing) {
     for (const s of existing) {
       const recordedWithoutDevice = s.id === storedSubId && !s.deviceClientId;
@@ -1717,7 +1721,7 @@ export async function teardownPushNotificationsForAccount(
     idsToDestroy.add(storedSubId);
   }
   for (const id of idsToDestroy) {
-    await destroyPushSubscription(id).catch(() => undefined);
+    await server?.destroy(id).catch(() => undefined);
   }
   if (relayBaseUrl && storedDcid) {
     await deregisterFromRelay(relayBaseUrl, storedDcid);
@@ -1729,6 +1733,38 @@ export async function teardownPushNotificationsForAccount(
   await writePushAccountIds(remaining);
 }
 
+/**
+ * Listing and destroying the account's push subscriptions, through the
+ * jmapClient when it serves the account, else through a client of its own.
+ * Null when the account can't be loaded: its server is left alone then.
+ */
+async function pushSubscriptionsOf(accountId: string): Promise<{
+  list: () => Promise<ListedSubscription[]>;
+  destroy: (id: string) => Promise<void>;
+} | null> {
+  const username = jmapClient.username;
+  const serverUrl = jmapClient.serverUrl;
+  if (username && serverUrl && generateAccountId(username, serverUrl) === accountId) {
+    return { list: listPushSubscriptions, destroy: destroyPushSubscription };
+  }
+  const client = new JMAPClient();
+  if (!(await client.loadAccount(accountId).catch(() => false))) return null;
+  const using = [CAPABILITIES.CORE];
+  return {
+    list: async () => {
+      const res = await client.request(
+        [['PushSubscription/get', { ids: null, properties: ['id', 'deviceClientId'] }, '0']],
+        using,
+      );
+      return (requireMethodResult(res, '0', 'PushSubscription/get').list as ListedSubscription[] | undefined) ?? [];
+    },
+    destroy: async (id) => {
+      const res = await client.request([['PushSubscription/set', { destroy: [id] }, '0']], using);
+      requireMethodResult(res, '0', 'PushSubscription/set');
+    },
+  };
+}
+
 /**
  * Turn push off for one account because the user asked to: the settings
  * toggle, or revoking this device (here, or from another device - see