From 9ccb62286efaed4a2bfba3ab69ddd3bc85b586ae Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Thu, 29 Oct 2026 02:51:24 +0100 Subject: [PATCH] fix: show a plain new-mail notification instead of another login's message when two logins share a JMAP account id --- src/lib/__tests__/ios-push-tap.test.ts | 57 +++++++++++- .../__tests__/push-background-task.test.ts | 55 ++++++++++- src/lib/push-background-task.ts | 91 ++++++++++++++----- src/lib/push-notifications.ts | 41 ++++++++- 4 files changed, 212 insertions(+), 32 deletions(-) diff --git a/src/lib/__tests__/ios-push-tap.test.ts b/src/lib/__tests__/ios-push-tap.test.ts index 98aa33b7..6de9cd69 100644 --- a/src/lib/__tests__/ios-push-tap.test.ts +++ b/src/lib/__tests__/ios-push-tap.test.ts @@ -1,6 +1,47 @@ -import { beforeEach, describe, expect, it } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import AsyncStorage from '@react-native-async-storage/async-storage'; -import { iosTapPayload } from '../push-notifications'; +import { Platform } from 'react-native'; + +vi.mock('../ios-push-shared', () => ({ + apnsEnvironment: () => 'production', + writeIosPushAccount: vi.fn(async () => undefined), + removeIosPushAccount: vi.fn(async () => undefined), +})); + +import { iosTapPayload, shareAccountWithExtension } from '../push-notifications'; +import { removeIosPushAccount, writeIosPushAccount } from '../ios-push-shared'; + +// The extension's entry for a pushed account is keyed by the JMAP account id +// alone (NotificationService.swift reads bulwark_push_account__). +describe('shareAccountWithExtension', () => { + const API = 'https://mail.example.org/jmap/'; + + beforeEach(async () => { + vi.clearAllMocks(); + await AsyncStorage.clear(); + (Platform as { OS: string }).OS = 'ios'; + }); + + afterEach(() => { + (Platform as { OS: string }).OS = 'android'; + }); + + it('shares the login whose push setup recorded the JMAP account id', async () => { + await AsyncStorage.setItem('push:jmapAccountIds:v1', JSON.stringify({ 'ada@mail.example.org': 'c' })); + await shareAccountWithExtension('ada@mail.example.org', 'c', API); + expect(writeIosPushAccount).toHaveBeenCalledWith('c', { credentialsKey: expect.any(String), apiUrl: API }); + }); + + 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', + 'bea@other.example.net': 'c', + })); + await shareAccountWithExtension('bea@other.example.net', 'c', 'https://other.example.net/jmap/'); + expect(writeIosPushAccount).not.toHaveBeenCalled(); + expect(removeIosPushAccount).toHaveBeenCalledWith('c'); + }); +}); // A tapped iOS mail notification: the relay's ids arrive as the // notification's data (APNs `body`), the notification extension adds the @@ -35,6 +76,18 @@ describe('iosTapPayload', () => { expect(await iosTapPayload(null)).toBeNull(); }); + it('tells two logins with the same JMAP account id apart by the relay label, or opens nothing', async () => { + await AsyncStorage.setItem('push:jmapAccountIds:v1', JSON.stringify({ + 'ada@mail.example.org': 'c', + 'bea@other.example.net': 'c', + })); + const push = { kind: 'jmap-email-push', accountId: 'c', emailIds: ['e1'] }; + expect((await iosTapPayload({ ...push, accountLabel: 'bea' }))?.accountId).toBe('bea@other.example.net'); + // Opening e1 in either login is a guess: the app opens as it is. + expect(await iosTapPayload({ ...push, accountLabel: 'carl' })).toBeNull(); + expect(await iosTapPayload(push)).toBeNull(); + }); + 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 d2f707a8..24ae517e 100644 --- a/src/lib/__tests__/push-background-task.test.ts +++ b/src/lib/__tests__/push-background-task.test.ts @@ -70,17 +70,45 @@ describe('matchAccountsForPush', () => { it('matches on the recorded JMAP account id first', () => { const payload = parseRelayPushData({ accountId: 'jb', accountLabel: 'alice' }); expect(matchAccountsForPush(payload, accounts, { 'bob@mail.example.com': 'jb' }, registry)) - .toEqual(['bob@mail.example.com']); + .toEqual({ accountIds: ['bob@mail.example.com'], ambiguous: false }); }); it('falls back to the relay accountLabel (username)', () => { const payload = parseRelayPushData({ accountId: 'unknown', accountLabel: 'alice' }); - expect(matchAccountsForPush(payload, accounts, {}, registry)).toEqual(['alice@mail.example.com']); + expect(matchAccountsForPush(payload, accounts, {}, registry).accountIds).toEqual(['alice@mail.example.com']); }); it('checks every account when nothing matches', () => { const payload = parseRelayPushData({ accountId: 'unknown', accountLabel: 'carol' }); - expect(matchAccountsForPush(payload, accounts, {}, registry)).toEqual(accounts); + expect(matchAccountsForPush(payload, accounts, {}, registry)).toEqual({ accountIds: accounts, ambiguous: false }); + }); + + describe('with two logins on different servers that share a JMAP account id', () => { + const logins = ['alice@one.example.com', 'bob@two.example.net']; + const ids = { 'alice@one.example.com': 'b', 'bob@two.example.net': 'b' }; + const users = [ + { id: 'alice@one.example.com', username: 'alice' }, + { id: 'bob@two.example.net', username: 'bob' }, + ]; + + it('picks the login the relay label names', () => { + const payload = parseRelayPushData({ accountId: 'b', accountLabel: 'bob', emailIds: '["m1"]' }); + expect(matchAccountsForPush(payload, logins, ids, users)).toEqual({ accountIds: ['bob@two.example.net'], ambiguous: false }); + }); + + it('calls a push naming messages ambiguous when the label does not tell them apart', () => { + const sameName = [ + { id: 'alice@one.example.com', username: 'alice' }, + { id: 'alice@two.example.net', username: 'alice' }, + ]; + const both = ['alice@one.example.com', 'alice@two.example.net']; + const map = { 'alice@one.example.com': 'b', 'alice@two.example.net': 'b' }; + const payload = parseRelayPushData({ accountId: 'b', accountLabel: 'alice', emailIds: '["m1"]' }); + expect(matchAccountsForPush(payload, both, map, sameName)).toEqual({ accountIds: both, ambiguous: true }); + // Without ids each login only looks at its own newest mail. + const legacy = parseRelayPushData({ accountId: 'b', accountLabel: 'alice', changed: '{"b":{"Email":"s"}}' }); + expect(matchAccountsForPush(legacy, both, map, sameName).ambiguous).toBe(false); + }); }); }); @@ -145,6 +173,27 @@ describe('pushBackgroundTask notifications', () => { ); }); + it("shows a plain notification, not another server's message, when two logins fit the push", async () => { + // A second login named alice, on another server whose account id is the same. + const SECOND = 'alice@other.example.net'; + await AsyncStorage.setItem('push:accountIds:v1', JSON.stringify([LOCAL, SECOND])); + await AsyncStorage.setItem('push:jmapAccountIds:v1', JSON.stringify({ [LOCAL]: 'jmap-primary', [SECOND]: 'jmap-primary' })); + + await pushBackgroundTask({ + kind: 'jmap-email-push', + accountLabel: 'alice', + accountId: 'jmap-primary', + emailIds: JSON.stringify(['m1']), + }); + + expect(emailGetAccount).toBeNull(); + expect(showNotification).toHaveBeenCalledTimes(1); + const shown = (showNotification.mock.calls[0] as unknown[])[0] as Record; + expect(shown).toMatchObject({ title: 'New email', emailId: '', threadId: '' }); + expect(shown.accountId).toBeUndefined(); + expect(shown.subject).toBeUndefined(); + }); + it('tags the user\'s own mail with the primary account', async () => { await pushBackgroundTask({ kind: 'jmap-email-push', diff --git a/src/lib/push-background-task.ts b/src/lib/push-background-task.ts index e5d1bd68..a9dbc814 100644 --- a/src/lib/push-background-task.ts +++ b/src/lib/push-background-task.ts @@ -19,6 +19,7 @@ import { notifiedIdsKey, pushSubscriptionDueForExtension, pushSubscriptionNeedsExtension, + accountMatchesPushLabel, readPushAccountIds, readPushJmapAccountIds, recordPushSubscriptionExpires, @@ -114,10 +115,11 @@ interface ShowNotificationOptions { emailId: string; threadId: string; subject?: string; - accountId: string; + // Left out for a notification that names no login (showPlainNotification). + accountId?: string; // The JMAP account the message lives in - a group or shared mailbox's // account rather than the user's own - so a tap opens it there. - jmapAccountId: string; + jmapAccountId?: string; // Android notification group: one per account so the tray bundles // several deliveries under a "+N more" summary instead of stacking them. groupKey: string; @@ -204,32 +206,48 @@ async function readAccountRegistry(): Promise { } } +export interface PushAccountMatch { + accountIds: string[]; + // The push names messages, and more than one login may be the one they + // were delivered to: see matchAccountsForPush. + ambiguous: boolean; +} + /** * Pick the local account(s) a relay payload belongs to. Primary key is the * JMAP account id recorded at setup; the relay's `accountLabel` (= username) - * is a weaker fallback. When neither matches - a payload from a build that - * predates the id map - every registered account is checked. + * is a weaker fallback, and also tells apart logins that share a JMAP account + * id. When neither matches - a payload from a build that predates the id map - + * every registered account is checked. + * + * JMAP account ids repeat across servers (Stalwart starts every server at the + * 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 + * look at its own newest mail, which is never another login's. */ export function matchAccountsForPush( payload: RelayPushData, pushAccountIds: string[], jmapAccountIds: Record, registry: RegistryAccount[], -): string[] { - if (payload.jmapAccountId) { - const byJmapId = pushAccountIds.filter((id) => jmapAccountIds[id] === payload.jmapAccountId); - if (byJmapId.length > 0) return byJmapId; +): PushAccountMatch { + const byLabel = (ids: string[]) => (payload.accountLabel + ? ids.filter((id) => accountMatchesPushLabel(id, payload.accountLabel!, registry.find((a) => a.id === id)?.username)) + : []); + let accountIds = pushAccountIds; + const byJmapId = payload.jmapAccountId + ? pushAccountIds.filter((id) => jmapAccountIds[id] === payload.jmapAccountId) + : []; + if (byJmapId.length > 0) { + const narrowed = byJmapId.length > 1 ? byLabel(byJmapId) : []; + accountIds = narrowed.length > 0 ? narrowed : byJmapId; + } else { + const labelled = byLabel(pushAccountIds); + if (labelled.length > 0) accountIds = labelled; } - if (payload.accountLabel) { - const label = payload.accountLabel.toLowerCase(); - const byLabel = pushAccountIds.filter((id) => { - const entry = registry.find((a) => a.id === id); - const username = entry?.username?.toLowerCase(); - return username === label || id.toLowerCase().startsWith(`${label}@`); - }); - if (byLabel.length > 0) return byLabel; - } - return pushAccountIds; + return { accountIds, ambiguous: accountIds.length > 1 && payload.emailIds.length > 0 }; } async function readNotifiedIds(accountId: string): Promise { @@ -444,11 +462,12 @@ export async function pushBackgroundTask(data: unknown): Promise { const registry = await readAccountRegistry(); const jmapAccountIds = await readPushJmapAccountIds(); - const accountsToCheck = matchAccountsForPush(payload, accountIds, jmapAccountIds, registry); + const match = matchAccountsForPush(payload, accountIds, jmapAccountIds, registry); + if (match.ambiguous) await showPlainNotification(payload); - for (const accountId of accountsToCheck) { + for (const accountId of match.accountIds) { try { - await processAccountForPush(accountId, payload); + await processAccountForPush(accountId, payload, !match.ambiguous); } catch (err) { console.warn( '[push] background check failed for account', @@ -511,11 +530,13 @@ async function extendPushSubscription(accountId: string, session: DetachedSessio if (extended?.expires) await recordPushSubscriptionExpires(accountId, extended.expires); } -async function processAccountForPush(accountId: string, payload: RelayPushData): Promise { +// `notify` is false for an ambiguous push (see matchAccountsForPush): each +// login it may belong to still keeps its subscription alive. +async function processAccountForPush(accountId: string, payload: RelayPushData, notify: boolean): Promise { const session = await openDetachedSession(accountId); if (!session) return; try { - await notifyForPush(accountId, session, payload); + if (notify) await notifyForPush(accountId, session, payload); } finally { await extendPushSubscription(accountId, session).catch(() => undefined); } @@ -600,6 +621,30 @@ async function notifyForPush(accountId: string, session: DetachedSession, payloa await rememberNotifiedIds(accountId, ordered.map((e) => e.id).reverse()); } +/** + * New mail without saying which message: for a push more than one login may + * belong to. It carries no message and no login, so it has no actions and a + * tap opens the app as it is; empty ids, which the tap handler refuses, keep + * it from opening anything (NotificationTapStore). + */ +async function showPlainNotification(payload: RelayPushData): Promise { + const native = NativeModules.BulwarkFcm as BulwarkFcmNative | undefined; + if (!native?.showNotification) return; + const locale = await notificationLocale(); + const label = payload.accountLabel ?? ''; + await native.showNotification({ + notificationId: `mail-plain:${payload.jmapAccountId ?? ''}:${payload.emailIds[0] ?? ''}`, + title: translate(locale, 'notifications.new_email', 'New email'), + body: label, + initials: getEmailInitials(label, ''), + bgColorHex: hslToHex(generateEmailAvatarColor(label, '')), + emailId: '', + threadId: '', + groupKey: 'bulwark-mail:plain', + groupTitle: label || 'Bulwark', + }); +} + function hslToHex(hsl: string): string { const match = hsl.match(/hsl\(\s*(\d+)\s*,\s*(\d+)%\s*,\s*(\d+)%\s*\)/); if (!match) return '#2563eb'; diff --git a/src/lib/push-notifications.ts b/src/lib/push-notifications.ts index 8e34a513..6fb7ff35 100644 --- a/src/lib/push-notifications.ts +++ b/src/lib/push-notifications.ts @@ -1003,14 +1003,38 @@ async function registerWithRelayApns(params: { * Tell the iOS notification extension how to reach an account: which stored * credentials to use and the JMAP API URL. Without it the extension shows the * relay's placeholder text. + * + * The extension finds the entry by the push's JMAP account id alone + * (NotificationService.swift), and those ids repeat across servers. While + * 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). */ -export async function shareAccountWithExtension(accountId: string, jmapAccountId: string | null): Promise { +export async function shareAccountWithExtension( + accountId: string, + jmapAccountId: string | null, + apiUrl: string | undefined = jmapClient.currentSession?.apiUrl, +): Promise { if (Platform.OS !== 'ios' || !jmapAccountId) return; - const apiUrl = jmapClient.currentSession?.apiUrl; + const byLocal = await readPushJmapAccountIds(); + if (Object.keys(byLocal).some((local) => local !== accountId && byLocal[local] === jmapAccountId)) { + await removeIosPushAccount(jmapAccountId).catch(() => undefined); + return; + } if (!apiUrl) return; await writeIosPushAccount(jmapAccountId, { credentialsKey: credentialsKey(accountId), apiUrl }).catch(() => undefined); } +/** + * Whether a local account (username@host) is the login a relay label names. + * The label is the username the account registered with the relay. + */ +export function accountMatchesPushLabel(localAccountId: string, label: string, username?: string): boolean { + const wanted = label.toLowerCase(); + return username?.toLowerCase() === wanted || localAccountId.toLowerCase().startsWith(`${wanted}@`); +} + async function registerWithRelayUnifiedPush(params: { relayBaseUrl: string; subscriptionId: string; @@ -2054,9 +2078,18 @@ export async function iosTapPayload(data: unknown): Promise byLocal[local] === jmapAccountId); + let candidates = Object.keys(byLocal).filter((local) => byLocal[local] === jmapAccountId); + if (candidates.length > 1) { + const label = typeof d.accountLabel === 'string' ? d.accountLabel : ''; + candidates = label ? candidates.filter((local) => accountMatchesPushLabel(local, label)) : []; + if (candidates.length !== 1) return null; + } + const accountId = candidates[0]; return { emailId, threadId: typeof d.threadId === 'string' ? d.threadId : '',