Merge branch 'fx/push2' into feat/stalwart-1.0

plugins/ios-push/NotificationService.swift

@@ -11,7 +11,7 @@ let pushLog = Logger(subsystem: "org.bulwarkmail.mobile.NotificationService", ca
 /// passes through the relay or Apple.
 ///
 /// What the app shares through the keychain group (src/lib/ios-push-shared.ts):
-///   bulwark_push_account__<JMAP account id>  {credentialsKey, apiUrl}
+///   bulwark_push_account__<JMAP account id>  {credentialsKey, apiUrl, label}
 ///   bulwark_push_settings                    {badge, activeAccountId, strings{...}}
 ///   <credentialsKey>                         the account's stored credentials
 /// If anything is missing or the server can't be reached, the relay's
@@ -66,6 +66,13 @@ final class NotificationService: UNNotificationServiceExtension {
       pushLog.error("no shared account entry for \(accountId, privacy: .public)")
       return content
     }
+    // Found by the account id alone, which a group account of another login
+    // can share: the relay's label (the username) has to be this entry's.
+    if let label = push["accountLabel"] as? String, !label.isEmpty,
+       let owner = account["label"] as? String, owner.lowercased() != label.lowercased() {
+      pushLog.error("push label does not fit the shared account for \(accountId, privacy: .public)")
+      return content
+    }
     let emailIds = push["emailIds"] as? [String] ?? []
     let settings = SharedKeychain.readJSON("bulwark_push_settings") ?? [:]
     let strings = settings["strings"] as? [String: String] ?? [:]

src/lib/__tests__/ios-push-tap.test.ts

@@ -32,6 +32,12 @@ describe('shareAccountWithExtension', () => {
     expect(writeIosPushAccount).toHaveBeenCalledWith('c', { credentialsKey: expect.any(String), apiUrl: API });
   });
 
+  it('names the username the relay labels the account\'s pushes with', async () => {
+    await AsyncStorage.setItem('push:jmapAccountIds:v1', JSON.stringify({ 'ada@mail.example.org': 'c' }));
+    await shareAccountWithExtension('ada@mail.example.org', 'c', API, 'ada');
+    expect(writeIosPushAccount).toHaveBeenCalledWith('c', { credentialsKey: expect.any(String), apiUrl: API, label: 'ada' });
+  });
+
   it('shares neither of two logins with the same JMAP account id, so the extension shows its placeholder', async () => {
     await AsyncStorage.setItem('push:jmapAccountIds:v1', JSON.stringify({
       'ada@mail.example.org': 'c',
@@ -88,6 +94,13 @@ describe('iosTapPayload', () => {
     expect(await iosTapPayload(push)).toBeNull();
   });
 
+  it("does not open another login's group push in the login whose own id it carries", async () => {
+    // Bea's group account "c" on her server; Ada's own account is "c" on hers.
+    const push = { kind: 'jmap-email-push', accountId: 'c', emailIds: ['e1'] };
+    expect(await iosTapPayload({ ...push, accountLabel: 'bea' })).toBeNull();
+    expect((await iosTapPayload({ ...push, accountLabel: 'ada' }))?.accountId).toBe('ada@mail.example.org');
+  });
+
   it('does nothing for a push without a message id', async () => {
     expect(await iosTapPayload({ kind: 'jmap-state-change', accountId: 'c', emailIds: [] })).toBeNull();
   });

src/lib/__tests__/push-background-task.test.ts

@@ -68,9 +68,20 @@ describe('matchAccountsForPush', () => {
   ];
 
   it('matches on the recorded JMAP account id first', () => {
-    const payload = parseRelayPushData({ accountId: 'jb', accountLabel: 'alice' });
+    const payload = parseRelayPushData({ accountId: 'jb', accountLabel: 'bob', emailIds: '["m1"]' });
     expect(matchAccountsForPush(payload, accounts, { 'bob@mail.example.com': 'jb' }, registry))
       .toEqual({ accountIds: ['bob@mail.example.com'], ambiguous: false });
+    // A payload without a label (an older relay) goes by the id alone.
+    const unlabelled = parseRelayPushData({ accountId: 'jb', emailIds: '["m1"]' });
+    expect(matchAccountsForPush(unlabelled, accounts, { 'bob@mail.example.com': 'jb' }, registry))
+      .toEqual({ accountIds: ['bob@mail.example.com'], ambiguous: false });
+  });
+
+  it("does not take another login's group push for the one whose own id it carries", () => {
+    // Bob's group account "c" on his server; Alice's own account is "c" on hers.
+    const payload = parseRelayPushData({ accountId: 'c', accountLabel: 'bob', emailIds: '["m1"]' });
+    expect(matchAccountsForPush(payload, accounts, { 'alice@mail.example.com': 'c', 'bob@mail.example.com': 'b' }, registry))
+      .toMatchObject({ ambiguous: true });
   });
 
   it('falls back to the relay accountLabel (username)', () => {

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],
@@ -1244,6 +1253,36 @@ describe('setupPushNotifications at the server\'s subscription limit', () => {
     expect(createMock).toHaveBeenCalledTimes(1);
     expect(destroyMock.mock.calls.map((c) => c[0])).not.toContain('existing');
     expect(await AsyncStorage.getItem(SUB_KEY)).toBe('existing');
+    // The working record still delivers through the old relay, so that one
+    // stays the relay in use, and this device is registered with it again.
+    expect(await AsyncStorage.getItem('push:relayBaseUrl:v1')).toBe(RELAY);
+    const relayCalls = (global.fetch as ReturnType<typeof vi.fn>).mock.calls
+      .filter((c) => String(c[0]).includes('/api/push/register'))
+      .map((c) => `${(c[1] as RequestInit | undefined)?.method ?? 'GET'} ${String(c[0])}`);
+    expect(relayCalls).toEqual([
+      'POST https://relay.home.lan/api/push/register',
+      `DELETE https://relay.home.lan/api/push/register/${OUR_DCID}`,
+      `POST ${RELAY}/api/push/register`,
+    ]);
+  });
+
+  it('keeps the old relay when the new one never verifies the replacement', async () => {
+    verifyMock.mockRejectedValueOnce(new Error('invalid verification code'));
+
+    const err = await setupPushNotifications({ relayBaseUrl: 'https://relay.other.example', forceRecreate: true }).catch((e) => e);
+
+    expect(err).toBeInstanceOf(PushSetupError);
+    expect(err.phase).toBe('verify');
+    expect(await AsyncStorage.getItem('push:relayBaseUrl:v1')).toBe(RELAY);
+    expect(await AsyncStorage.getItem(SUB_KEY)).toBe('existing');
+    expect(destroyMock.mock.calls.map((c) => c[0])).not.toContain('existing');
+    const fetchMock = global.fetch as ReturnType<typeof vi.fn>;
+    expect(fetchMock.mock.calls.some((c) => String(c[0]) === `https://relay.other.example/api/push/register/${OUR_DCID}`
+      && (c[1] as RequestInit | undefined)?.method === 'DELETE')).toBe(true);
+
+    // A move that works keeps the new relay.
+    expect((await setupPushNotifications({ relayBaseUrl: 'https://relay.other.example', forceRecreate: true })).verified).toBe(true);
+    expect(await AsyncStorage.getItem('push:relayBaseUrl:v1')).toBe('https://relay.other.example');
   });
 
   it('a forced re-register frees the old record\'s place and tries once more', async () => {
@@ -1824,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/ios-push-shared.ts

@@ -62,6 +62,13 @@ export interface IosPushAccount {
   credentialsKey: string;
   /** The JMAP API URL as the app uses it (rewritten to the server's host). */
   apiUrl: string;
+  /**
+   * The username the account registered with the relay, which the relay
+   * sends back as the push's accountLabel. The extension leaves a push whose
+   * label is another's as the placeholder: it may be for another login's
+   * group account that has this account's id.
+   */
+  label?: string;
 }
 
 export async function writeIosPushAccount(jmapAccountId: string, account: IosPushAccount): Promise<void> {

src/lib/push-background-task.ts

@@ -224,7 +224,8 @@ export interface PushAccountMatch {
  * same few), and so do message ids. A push naming messages that still fits
  * more than one login is `ambiguous`: each login would fetch those ids from
  * its own server and could show another server's message, so the caller
- * shows a plain notification instead. A push without ids makes each login
+ * shows a plain notification instead. So is one whose label fits none of the
+ * logins with its JMAP account id. A push without ids makes each login
  * look at its own newest mail, which is never another login's.
  */
 export function matchAccountsForPush(
@@ -237,17 +238,22 @@ export function matchAccountsForPush(
     ? ids.filter((id) => accountMatchesPushLabel(id, payload.accountLabel!, registry.find((a) => a.id === id)?.username))
     : []);
   let accountIds = pushAccountIds;
+  let unsure = false;
   const byJmapId = payload.jmapAccountId
     ? pushAccountIds.filter((id) => jmapAccountIds[id] === payload.jmapAccountId)
     : [];
   if (byJmapId.length > 0) {
-    const narrowed = byJmapId.length > 1 ? byLabel(byJmapId) : [];
+    // Also a single match has to fit the label: group accounts are subscribed
+    // too, and their ids come from the same short pool, so a push for another
+    // login's group can carry this login's own id.
+    const narrowed = byLabel(byJmapId);
     accountIds = narrowed.length > 0 ? narrowed : byJmapId;
+    unsure = !!payload.accountLabel && narrowed.length === 0;
   } else {
     const labelled = byLabel(pushAccountIds);
     if (labelled.length > 0) accountIds = labelled;
   }
-  return { accountIds, ambiguous: accountIds.length > 1 && payload.emailIds.length > 0 };
+  return { accountIds, ambiguous: (unsure || accountIds.length > 1) && payload.emailIds.length > 0 };
 }
 
 async function readNotifiedIds(accountId: string): Promise<string[]> {

src/lib/push-notifications.ts

@@ -1010,12 +1010,15 @@ async function registerWithRelayApns(params: {
  * another login with push has the same id, neither gets an entry: the
  * placeholder says there is new mail, where one login's credentials would
  * fetch and show a message of the other's server. Run again for the login
- * that is left once the other is gone (the next resync does).
+ * that is left once the other is gone (the next resync does). The entry also
+ * names the username, which the extension compares with the push's label: a
+ * group account of another login can have this login's id.
  */
 export async function shareAccountWithExtension(
   accountId: string,
   jmapAccountId: string | null,
   apiUrl: string | undefined = jmapClient.currentSession?.apiUrl,
+  username: string | null | undefined = jmapClient.username,
 ): Promise<void> {
   if (Platform.OS !== 'ios' || !jmapAccountId) return;
   const byLocal = await readPushJmapAccountIds();
@@ -1024,7 +1027,11 @@ export async function shareAccountWithExtension(
     return;
   }
   if (!apiUrl) return;
-  await writeIosPushAccount(jmapAccountId, { credentialsKey: credentialsKey(accountId), apiUrl }).catch(() => undefined);
+  await writeIosPushAccount(jmapAccountId, {
+    credentialsKey: credentialsKey(accountId),
+    apiUrl,
+    ...(username ? { label: username } : {}),
+  }).catch(() => undefined);
 }
 
 /**
@@ -1187,9 +1194,15 @@ export function setupPushNotifications(
   const key = `${jmapClient.username ?? ''}@${jmapClient.serverUrl ?? ''}`;
   const existing = inFlightSetups.get(key);
   if (existing) return existing;
-  const run = setupPushNotificationsInner(params).finally(() => {
-    inFlightSetups.delete(key);
-  });
+  const rollback: SetupRollback = {};
+  const run = setupPushNotificationsInner(params, rollback)
+    .catch(async (err: unknown) => {
+      await rollback.relayMove?.().catch(() => undefined);
+      throw err;
+    })
+    .finally(() => {
+      inFlightSetups.delete(key);
+    });
   inFlightSetups.set(key, run);
   return run;
 }
@@ -1198,8 +1211,14 @@ function logPhase(phase: string, detail?: string): void {
   console.log(`[push] ${phase}${detail ? `: ${detail}` : ''}`);
 }
 
+// What a failed setup run has to undo, set by the run as it goes.
+interface SetupRollback {
+  relayMove?: () => Promise<void>;
+}
+
 async function setupPushNotificationsInner(
   params: PushSetupParams,
+  rollback: SetupRollback,
 ): Promise<PushSetupResult> {
   const transport = await getEffectivePushTransport();
   const native = getNative();
@@ -1249,8 +1268,8 @@ async function setupPushNotificationsInner(
   await migrateLegacyPushKeys();
 
   const deviceClientId = await getOrCreateDeviceClientId(accountId);
-  // The relay a working registration goes through, put back should the mail
-  // server refuse the new one's address.
+  // The relay a working registration goes through, put back should a move
+  // to another relay fail.
   const previousRelayBaseUrl = await getStoredRelayBaseUrl();
   await setStoredRelayBaseUrl(relayBaseUrl);
 
@@ -1284,18 +1303,32 @@ async function setupPushNotificationsInner(
       });
     }
   };
-  await registerWith(relayBaseUrl, upEndpoint);
-  // Undo this run's relay steps once the mail server refuses the new relay's
-  // address. The refused URL may be another name of the relay in use, so the
-  // record dropped from it can be the working one; and a UnifiedPush
-  // distributor now holds the refused relay's VAPID key, which may make it
-  // turn away the previous relay's pushes. Registering with the previous relay
-  // again puts both back; the record itself only for an account that had a
-  // working subscription through it.
+  // Undo this run's relay steps once a move to another relay fails. The new
+  // URL may be another name of the relay in use, so the record dropped from
+  // it can be the working one; and a UnifiedPush distributor now holds the
+  // new relay's VAPID key, which may make it turn away the previous relay's
+  // pushes. Registering with the previous relay again puts both back; the
+  // record itself only for an account that had a working subscription
+  // through it.
   const restoreRelay = async (relay: string, hadSubscription: boolean) => {
     const endpoint = upEndpoint ? await getUnifiedPushEndpointOrThrow(relay) : null;
     if (hadSubscription) await registerWith(relay, endpoint);
   };
+  // Until a subscription through the new relay is verified, whatever still
+  // works goes through the relay used before, whichever step failed (the
+  // server refused the address, the subscription limit, a verification that
+  // never came): keep that one stored, since token refreshes, the settings,
+  // the device list and the next launch all follow it, and drop the record
+  // this run left on the new relay.
+  let match: PushSubscription | undefined;
+  if (previousRelayBaseUrl !== relayBaseUrl) {
+    rollback.relayMove = async () => {
+      await setStoredRelayBaseUrl(previousRelayBaseUrl);
+      await deregisterFromRelay(relayBaseUrl, deviceClientId);
+      if (previousRelayBaseUrl) await restoreRelay(previousRelayBaseUrl, !!match).catch(() => undefined);
+    };
+  }
+  await registerWith(relayBaseUrl, upEndpoint);
 
   // Reuse the previous JMAP subscription when the server still has it, but
   // push the expiry forward so it doesn't time out before the next app start.
@@ -1339,7 +1372,7 @@ async function setupPushNotificationsInner(
   // Ours only if it was made for this device: Stalwart's ids are short and
   // sequential, so once the server is reinstalled at the same address the
   // recorded id can name another device's subscription.
-  const match = storedServerId
+  match = storedServerId
     ? existingSubs.find((s) => s.id === storedServerId && (!s.deviceClientId || s.deviceClientId === deviceClientId))
     : undefined;
   if (storedServerId) {
@@ -1447,15 +1480,6 @@ async function setupPushNotificationsInner(
     serverExpires = created.result.expires;
     await rememberRefusedEmailPushAccounts(accountId, refusedBefore, emailPush, created.emailPush);
   } catch (err) {
-    // The server will never take this relay's address. Whatever registration
-    // still works goes through the relay used before: keep that one stored
-    // (token refreshes and the next launch register with it) and drop the
-    // record this run left on the refused relay.
-    if (isRefusedPushUrl(err) && previousRelayBaseUrl !== relayBaseUrl) {
-      await setStoredRelayBaseUrl(previousRelayBaseUrl);
-      await deregisterFromRelay(relayBaseUrl, deviceClientId);
-      if (previousRelayBaseUrl) await restoreRelay(previousRelayBaseUrl, !!match).catch(() => undefined);
-    }
     throw subscriptionError(err, relayBaseUrl);
   }
 
@@ -1470,6 +1494,7 @@ async function setupPushNotificationsInner(
       t('settings.notifications.push.err_verify_rejected', 'The mail server rejected the verification code: {detail}', { detail }),
     );
   }
+  rollback.relayMove = undefined;
 
   await AsyncStorage.setItem(subKey, serverAssignedId);
   if (serverExpires) await AsyncStorage.setItem(subscriptionExpiresKey(accountId), serverExpires);
@@ -1657,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
- * authenticated to that account; the active-account logout flow guarantees
- * this) and tells the relay to drop its mapping. Other accounts' push setups
- * are untouched.
+ * server holds for this device and tells the relay to drop its mapping.
+ * Other accounts' push setups are untouched. The account the jmapClient
+ * serves goes through it; another signed-in account (removed from the
+ * 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()
@@ -1681,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;
@@ -1692,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);
@@ -1704,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
@@ -1786,8 +1847,9 @@ export interface PushEnableOutcome {
 /**
  * The enable and re-register buttons of the notification settings: run the
  * setup and say what it left behind. When the mail server refuses a new
- * relay, the setup goes back to the one in use; the relay field must show
- * that one again rather than the refused address, since the device list asks
+ * relay, or the move to it fails otherwise, the setup goes back to the one in
+ * use; the relay field must show that one again rather than the new address,
+ * since the device list asks
  * the relay in the field about each device and a revoke goes to it. With no
  * relay in use the typed one stays, so the user can correct it.
  */
@@ -1924,7 +1986,7 @@ async function renewPushAccount(accountId: string, relayBaseUrl: string | null):
   } catch {
     jmapAccountId = null;
   }
-  await shareAccountWithExtension(accountId, jmapAccountId, client.currentSession?.apiUrl);
+  await shareAccountWithExtension(accountId, jmapAccountId, client.currentSession?.apiUrl, client.username);
 }
 
 /**
@@ -2197,11 +2259,13 @@ export async function iosTapPayload(data: unknown): Promise<NotificationTapPaylo
   // setup recorded this JMAP account id. Ids repeat across servers: with
   // more than one such login the relay's label (the username) picks one, and
   // when it can't, the app just opens rather than show another login's
-  // message on a guess.
+  // message on a guess. A single login has to fit the label too: the push
+  // may be for another login's group account, whose id comes from the same
+  // short pool.
   const byLocal = await readPushJmapAccountIds();
   let candidates = Object.keys(byLocal).filter((local) => byLocal[local] === jmapAccountId);
-  if (candidates.length > 1) {
-    const label = typeof d.accountLabel === 'string' ? d.accountLabel : '';
+  const label = typeof d.accountLabel === 'string' ? d.accountLabel : '';
+  if (candidates.length > 1 || (label && candidates.length === 1)) {
     candidates = label ? candidates.filter((local) => accountMatchesPushLabel(local, label)) : [];
     if (candidates.length !== 1) return null;
   }