fix: keep a push for another login's group account out of the login whose own id it carries

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/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);
 }
 
 /**
@@ -1943,7 +1950,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);
 }
 
 /**
@@ -2216,11 +2223,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;
   }