From 8618abf1d7d1d77f6331544d152a9145daa3a8ff Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Thu, 24 Sep 2026 18:59:25 +0200 Subject: [PATCH] fix: open a group mailbox notification's message in the group account #839 The notification only carried the local account it was posted for, so tapping one for a message in a group or shared mailbox opened the thread against the user's own JMAP account, where the message doesn't exist. The background task now passes the JMAP account the message was fetched from as `jmapAccountId`. The native module stores it as an intent extra, validated like the other ids, and hands it back with the tap. `App.tsx` routes the open with it, the way the unified inbox does for group messages. Notifications posted before this change have no such extra and open against the account's own mail as before. --- App.tsx | 3 + .../bulwarkmobile/BulwarkFcmModule.kt | 5 +- .../bulwarkmobile/NotificationTapStore.kt | 9 ++- .../__tests__/push-background-task.test.ts | 81 ++++++++++++++++++- src/lib/__tests__/push-notifications.test.ts | 17 ++++ src/lib/push-background-task.ts | 9 ++- src/lib/push-notifications.ts | 20 +++++ 7 files changed, 139 insertions(+), 5 deletions(-) diff --git a/App.tsx b/App.tsx index dee0d125..7d60e871 100644 --- a/App.tsx +++ b/App.tsx @@ -20,6 +20,7 @@ import { addTokenRefreshListener, getInitialNotificationTap, getStoredRelayBaseUrl, + notificationTapJmapAccountId, resyncPushNotifications, teardownPushNotificationsForAccount, type NotificationTapPayload, @@ -87,6 +88,8 @@ async function navigateToNotificationTap(payload: NotificationTapPayload): Promi emailId: payload.emailId, threadId: payload.threadId, subject: payload.subject, + // A group mailbox's message lives under another JMAP account (#839). + jmapAccountId: notificationTapJmapAccountId(payload), }); } diff --git a/android/app/src/main/java/com/anonymous/bulwarkmobile/BulwarkFcmModule.kt b/android/app/src/main/java/com/anonymous/bulwarkmobile/BulwarkFcmModule.kt index e806a980..f55e86af 100644 --- a/android/app/src/main/java/com/anonymous/bulwarkmobile/BulwarkFcmModule.kt +++ b/android/app/src/main/java/com/anonymous/bulwarkmobile/BulwarkFcmModule.kt @@ -66,6 +66,7 @@ class BulwarkFcmModule(reactContext: ReactApplicationContext) val threadId = options.getString("threadId") val subject = options.takeIf { it.hasKey("subject") }?.getString("subject") val accountId = options.takeIf { it.hasKey("accountId") }?.getString("accountId") + val jmapAccountId = options.takeIf { it.hasKey("jmapAccountId") }?.getString("jmapAccountId") val groupKey = options.takeIf { it.hasKey("groupKey") }?.getString("groupKey") ?: accountId?.let { "bulwark-mail:$it" } val groupTitle = options.takeIf { it.hasKey("groupTitle") }?.getString("groupTitle") @@ -78,7 +79,7 @@ class BulwarkFcmModule(reactContext: ReactApplicationContext) ?: makeLetterAvatar(initials, bgColorHex) postNotification( notificationId, title, body, largeIcon, bgColorHex, - emailId, threadId, subject, accountId, groupKey, + emailId, threadId, subject, accountId, jmapAccountId, groupKey, ) if (groupKey != null) postGroupSummary(groupKey, groupTitle, bgColorHex, accountId) promise.resolve(null) @@ -107,6 +108,7 @@ class BulwarkFcmModule(reactContext: ReactApplicationContext) threadId: String?, subject: String?, accountId: String?, + jmapAccountId: String?, groupKey: String?, ) { val ctx = reactApplicationContext @@ -116,6 +118,7 @@ class BulwarkFcmModule(reactContext: ReactApplicationContext) if (threadId != null) putExtra(NotificationTapStore.EXTRA_THREAD_ID, threadId) if (subject != null) putExtra(NotificationTapStore.EXTRA_SUBJECT, subject) if (accountId != null) putExtra(NotificationTapStore.EXTRA_ACCOUNT_ID, accountId) + if (jmapAccountId != null) putExtra(NotificationTapStore.EXTRA_JMAP_ACCOUNT_ID, jmapAccountId) } val pending = PendingIntent.getActivity( ctx, diff --git a/android/app/src/main/java/com/anonymous/bulwarkmobile/NotificationTapStore.kt b/android/app/src/main/java/com/anonymous/bulwarkmobile/NotificationTapStore.kt index 6c1595be..baed3c73 100644 --- a/android/app/src/main/java/com/anonymous/bulwarkmobile/NotificationTapStore.kt +++ b/android/app/src/main/java/com/anonymous/bulwarkmobile/NotificationTapStore.kt @@ -40,13 +40,17 @@ object NotificationTapStore { // the navigation payload. val subject = extras.getString(EXTRA_SUBJECT)?.take(512) val accountId = extras.getString(EXTRA_ACCOUNT_ID)?.takeIf { ACCOUNT_ID_PATTERN.matches(it) } - val payload = TapPayload(emailId, threadId, subject, accountId) + // The JMAP account a group/shared mailbox's message lives in - a JMAP + // id like the email id. Optional: older notifications don't carry it. + val jmapAccountId = extras.getString(EXTRA_JMAP_ACCOUNT_ID)?.takeIf { ID_PATTERN.matches(it) } + val payload = TapPayload(emailId, threadId, subject, accountId, jmapAccountId) pending = payload // Clear so a subsequent activity lifecycle event doesn't replay this. extras.remove(EXTRA_EMAIL_ID) extras.remove(EXTRA_THREAD_ID) extras.remove(EXTRA_SUBJECT) extras.remove(EXTRA_ACCOUNT_ID) + extras.remove(EXTRA_JMAP_ACCOUNT_ID) return payload } @@ -55,12 +59,14 @@ object NotificationTapStore { val threadId: String, val subject: String?, val accountId: String?, + val jmapAccountId: String?, ) { fun toMap(): WritableMap = Arguments.createMap().apply { putString("emailId", emailId) putString("threadId", threadId) if (subject != null) putString("subject", subject) if (accountId != null) putString("accountId", accountId) + if (jmapAccountId != null) putString("jmapAccountId", jmapAccountId) } } @@ -68,4 +74,5 @@ object NotificationTapStore { const val EXTRA_THREAD_ID = "bulwark.notification.threadId" const val EXTRA_SUBJECT = "bulwark.notification.subject" const val EXTRA_ACCOUNT_ID = "bulwark.notification.accountId" + const val EXTRA_JMAP_ACCOUNT_ID = "bulwark.notification.jmapAccountId" } diff --git a/src/lib/__tests__/push-background-task.test.ts b/src/lib/__tests__/push-background-task.test.ts index 571973e0..45d48429 100644 --- a/src/lib/__tests__/push-background-task.test.ts +++ b/src/lib/__tests__/push-background-task.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, vi } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; vi.mock('../../api/jmap-client', () => ({ jmapClient: { @@ -11,9 +11,13 @@ vi.mock('../client-cert', () => ({ secureFetch: vi.fn(async () => ({ ok: false, vi.mock('../oauth', () => ({ refreshOAuthAccessToken: vi.fn(async (t: unknown) => t) })); import AsyncStorage from '@react-native-async-storage/async-storage'; +import { NativeModules } from 'react-native'; +import { jmapClient } from '../../api/jmap-client'; +import { secureFetch } from '../client-cert'; import { matchAccountsForPush, parseRelayPushData, + pushBackgroundTask, selectNotifiableEmails, senderFaviconsAllowed, } from '../push-background-task'; @@ -78,6 +82,81 @@ describe('matchAccountsForPush', () => { }); }); +describe('pushBackgroundTask notifications', () => { + const LOCAL = 'alice@mail.example.com'; + const showNotification = vi.fn(async () => undefined); + let emailGetAccount: string | null = null; + + beforeEach(async () => { + vi.clearAllMocks(); + await AsyncStorage.clear(); + await AsyncStorage.setItem('push:accountIds:v1', JSON.stringify([LOCAL])); + await AsyncStorage.setItem('push:jmapAccountIds:v1', JSON.stringify({ [LOCAL]: 'jmap-primary' })); + (NativeModules as Record).BulwarkFcm = { showNotification }; + (jmapClient.getStoredCredentials as ReturnType).mockResolvedValue({ + serverUrl: 'https://mail.example.com', + username: 'alice', + password: 'secret', + }); + emailGetAccount = null; + (secureFetch as ReturnType).mockImplementation(async (url: string, init?: { body?: string }) => { + if (url.endsWith('/.well-known/jmap')) { + return { + ok: true, + json: async () => ({ + apiUrl: 'https://mail.example.com/jmap/', + primaryAccounts: { 'urn:ietf:params:jmap:mail': 'jmap-primary' }, + accounts: { 'jmap-primary': {}, team: {} }, + }), + }; + } + const [[, args]] = JSON.parse(init?.body ?? '{}').methodCalls; + emailGetAccount = args.accountId; + return { + ok: true, + json: async () => ({ + methodResponses: [[ + 'Email/get', + { list: [{ id: 'm1', threadId: 't1', keywords: {}, subject: 'Hi', from: [{ email: 'bob@example.com' }] }] }, + '0', + ]], + }), + }; + }); + }); + + afterEach(() => { + delete (NativeModules as Record).BulwarkFcm; + }); + + it('tags a group mailbox notification with the JMAP account the message lives in (#839)', async () => { + await pushBackgroundTask({ + kind: 'jmap-email-push', + accountLabel: 'alice', + accountId: 'team', + emailIds: JSON.stringify(['m1']), + }); + + expect(emailGetAccount).toBe('team'); + expect(showNotification).toHaveBeenCalledWith( + expect.objectContaining({ emailId: 'm1', accountId: LOCAL, jmapAccountId: 'team' }), + ); + }); + + it('tags the user\'s own mail with the primary account', async () => { + await pushBackgroundTask({ + kind: 'jmap-email-push', + accountLabel: 'alice', + accountId: 'jmap-primary', + emailIds: JSON.stringify(['m1']), + }); + + expect(showNotification).toHaveBeenCalledWith( + expect.objectContaining({ emailId: 'm1', accountId: LOCAL, jmapAccountId: 'jmap-primary' }), + ); + }); +}); + describe('selectNotifiableEmails', () => { const email = (id: string, keywords: Record = {}): Email => ({ id, threadId: 't', keywords, mailboxIds: {}, size: 0, receivedAt: '', hasAttachment: false } as Email); diff --git a/src/lib/__tests__/push-notifications.test.ts b/src/lib/__tests__/push-notifications.test.ts index df8a5905..a324a717 100644 --- a/src/lib/__tests__/push-notifications.test.ts +++ b/src/lib/__tests__/push-notifications.test.ts @@ -73,6 +73,7 @@ import { deviceClientIdKey, disablePushForAccount, isValidRelayUrl, + notificationTapJmapAccountId, readPushAccountIds, readPushJmapAccountIds, PushSetupError, @@ -793,6 +794,22 @@ describe('teardownPushNotificationsForAccount', () => { }); }); +describe('notificationTapJmapAccountId', () => { + const tap = { emailId: 'm1', threadId: 't1', accountId: ACCOUNT_ID }; + + it('opens a group mailbox message against the group account', () => { + expect(notificationTapJmapAccountId({ ...tap, jmapAccountId: 'team' })).toBe('team'); + }); + + it('leaves the user\'s own mail on the default account', () => { + expect(notificationTapJmapAccountId({ ...tap, jmapAccountId: 'jmap-primary' })).toBeUndefined(); + }); + + it('keeps notifications posted before the field existed working', () => { + expect(notificationTapJmapAccountId(tap)).toBeUndefined(); + }); +}); + describe('isValidRelayUrl', () => { it('requires https except for loopback development hosts', () => { expect(isValidRelayUrl('https://relay.example.com')).toBe(true); diff --git a/src/lib/push-background-task.ts b/src/lib/push-background-task.ts index 8ca29175..65d51adb 100644 --- a/src/lib/push-background-task.ts +++ b/src/lib/push-background-task.ts @@ -81,6 +81,9 @@ interface ShowNotificationOptions { threadId: string; subject?: string; 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; // Android notification group: one per account so the tray bundles // several deliveries under a "+N more" summary instead of stacking them. groupKey: string; @@ -416,6 +419,7 @@ async function processAccountForPush(accountId: string, payload: RelayPushData): const alreadyNotified = await readNotifiedIds(accountId); let candidates: Email[]; + let emailAccountId = session.jmapAccountId; if (payload.emailIds.length > 0) { // EmailPush (or a relay that forwards ids): fetch exactly the delivered @@ -423,8 +427,8 @@ async function processAccountForPush(accountId: string, payload: RelayPushData): // to, in which case the ids live under that JMAP account. const fresh = payload.emailIds.filter((id) => !alreadyNotified.includes(id)); if (fresh.length === 0) return; - const targetAccount = payload.jmapAccountId ?? session.jmapAccountId; - candidates = await detachedGetEmails(session, targetAccount, fresh); + emailAccountId = payload.jmapAccountId ?? session.jmapAccountId; + candidates = await detachedGetEmails(session, emailAccountId, fresh); } else { // Legacy `jmap-state-change` payload without ids: look at the newest // unread inbox messages and notify the ones not shown before. The ring @@ -472,6 +476,7 @@ async function processAccountForPush(accountId: string, payload: RelayPushData): threadId: email.threadId, subject: email.subject ?? undefined, accountId, + jmapAccountId: emailAccountId, groupKey, groupTitle, }); diff --git a/src/lib/push-notifications.ts b/src/lib/push-notifications.ts index 9f60cf06..1a4a695d 100644 --- a/src/lib/push-notifications.ts +++ b/src/lib/push-notifications.ts @@ -1284,6 +1284,26 @@ export interface NotificationTapPayload { // Optional for back-compat: older notifications already on the system tray // won't carry this and will fall back to the active account on tap. accountId?: string; + // The JMAP account the message lives in: a group or shared mailbox's account + // when it isn't the user's own. Absent on older notifications. + jmapAccountId?: string; +} + +/** + * The JMAP account to open a tapped notification's message against, in the + * form the EmailThread route takes: the group or shared account it was + * delivered from, or undefined for the user's own mail - including + * notifications posted before the field existed (#839). + */ +export function notificationTapJmapAccountId(payload: NotificationTapPayload): string | undefined { + if (!payload.jmapAccountId) return undefined; + let primary: string | null = null; + try { + primary = jmapClient.accountId; + } catch { + primary = null; + } + return payload.jmapAccountId === primary ? undefined : payload.jmapAccountId; } // Returns - and clears - any pending "notification tap" that launched the app