diff --git a/plugins/ios-push/NotificationService.swift b/plugins/ios-push/NotificationService.swift index beed8009..0557455f 100644 --- a/plugins/ios-push/NotificationService.swift +++ b/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__ {credentialsKey, apiUrl} +/// bulwark_push_account__ {credentialsKey, apiUrl, label} /// bulwark_push_settings {badge, activeAccountId, strings{...}} /// 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] ?? [:] diff --git a/src/lib/__tests__/ios-push-tap.test.ts b/src/lib/__tests__/ios-push-tap.test.ts index 6de9cd69..a9c85d90 100644 --- a/src/lib/__tests__/ios-push-tap.test.ts +++ b/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(); }); diff --git a/src/lib/__tests__/push-background-task.test.ts b/src/lib/__tests__/push-background-task.test.ts index 24ae517e..65c7f27a 100644 --- a/src/lib/__tests__/push-background-task.test.ts +++ b/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)', () => { diff --git a/src/lib/ios-push-shared.ts b/src/lib/ios-push-shared.ts index 7e476e4d..2f28e57e 100644 --- a/src/lib/ios-push-shared.ts +++ b/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 { diff --git a/src/lib/push-background-task.ts b/src/lib/push-background-task.ts index a9dbc814..f13dd680 100644 --- a/src/lib/push-background-task.ts +++ b/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 { diff --git a/src/lib/push-notifications.ts b/src/lib/push-notifications.ts index d08e38c5..a4d4c733 100644 --- a/src/lib/push-notifications.ts +++ b/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 { 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 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; }