diff --git a/android/app/src/main/java/com/anonymous/bulwarkmobile/sync/PushRouting.kt b/android/app/src/main/java/com/anonymous/bulwarkmobile/sync/PushRouting.kt index 70c0b97d..fd5d938a 100644 --- a/android/app/src/main/java/com/anonymous/bulwarkmobile/sync/PushRouting.kt +++ b/android/app/src/main/java/com/anonymous/bulwarkmobile/sync/PushRouting.kt @@ -13,9 +13,11 @@ import org.json.JSONObject * The relay forwards the JMAP StateChange's `changed` map verbatim (FCM data * key `changed`: JMAP account id → type → state; only its first account is * repeated as `accountId`), so every account key is looked at. The mail task - * runs only for mail: when some account's map has a mail type, for an - * EmailPush, or when `changed` is missing or unreadable (older relays), so a - * contact change never ends in a mail notification. + * runs only for mail: when some account's map has a mail type, for anything + * but a StateChange (an EmailPush), when `changed` is missing, unreadable or + * names no type (older relays), or when an account's entry is unreadable, so + * a contact change never ends in a mail notification. The push task's guard + * (`carriesNoMail` in src/lib/push-background-task.ts) applies the same rules. */ object PushRouting { /** An Android account and the authorities it syncs, fed by one JMAP account. */ @@ -27,7 +29,7 @@ object PushRouting { val startMailTask: Boolean, ) - private const val EMAIL_PUSH_KIND = "jmap-email-push" + private const val STATE_CHANGE_KIND = "jmap-state-change" /** * Types that may mean new mail: `EmailDelivery`, and `Email`/`Mailbox`, @@ -42,7 +44,8 @@ object PushRouting { fun decide(data: Map, routes: Map>): Decision { val changed = data["changed"]?.let(::parseChanged) - var mail = data["kind"] == EMAIL_PUSH_KIND || changed == null + // Only a StateChange can say it carries no mail. + var mail = data["kind"] != STATE_CHANGE_KIND || changed == null val syncs = LinkedHashSet>() for ((jmapAccountId, types) in changed.orEmpty()) { if (types == null) { diff --git a/android/app/src/test/java/com/anonymous/bulwarkmobile/sync/PushRoutingTest.kt b/android/app/src/test/java/com/anonymous/bulwarkmobile/sync/PushRoutingTest.kt index 8e608022..db2d5b53 100644 --- a/android/app/src/test/java/com/anonymous/bulwarkmobile/sync/PushRoutingTest.kt +++ b/android/app/src/test/java/com/anonymous/bulwarkmobile/sync/PushRoutingTest.kt @@ -108,6 +108,56 @@ class PushRoutingTest { assertEquals(setOf("alice@example.org" to contacts), decision.syncs) } + @Test + fun `reads a push the same way as the push task`() { + // kind (null: none), changed (null: none) → whether the mail task starts. The + // same table as MAIL_CASES in src/lib/__tests__/push-background-task.test.ts, + // whose carriesNoMail must agree. + val state = "jmap-state-change" + val emailPush = "jmap-email-push" + val cases: List> = listOf( + // No readable map: as before device sync. + Triple(state, null, true), + Triple(state, "", true), + Triple(state, "not json", true), + Triple(state, "[1]", true), + Triple(state, "{}", true), + // A map that names no type is read like a missing one. + Triple(state, """{"c":{}}""", true), + Triple(state, """{"c":{},"team":{}}""", true), + // An account entry that can't be read may be mail. + Triple(state, """{"c":null}""", true), + Triple(state, """{"c":"garbled","team":{"ContactCard":"s"}}""", true), + Triple(state, """{"c":[],"team":{"ContactCard":"s"}}""", true), + Triple(state, """{"c":5}""", true), + // Mail types anywhere in the map. + Triple(state, """{"c":{"EmailDelivery":"s"}}""", true), + Triple(state, """{"c":{"Email":"s"}}""", true), + Triple(state, """{"c":{"Mailbox":"s"}}""", true), + Triple(state, """{"c":{"ContactCard":"s1"},"team":{"EmailDelivery":"s2"}}""", true), + // Only a StateChange can say it carries no mail. + Triple(null, """{"c":{"ContactCard":"s"}}""", true), + Triple(emailPush, """{"c":{"EmailDelivery":"s"}}""", true), + Triple(emailPush, null, true), + // Contact and calendar changes, and types that say nothing about mail. + Triple(state, """{"c":{"ContactCard":"s1","AddressBook":"s2"}}""", false), + Triple(state, """{"c":{"CalendarEvent":"e1"},"team":{"Calendar":"c1"}}""", false), + Triple(state, """{"c":{"ContactCard":"s"},"team":{}}""", false), + Triple(state, """{"elsewhere":{"ContactCard":"s"}}""", false), + Triple(state, """{"c":{"Thread":"t1","ContactCard":"s1"}}""", false), + ) + for ((kind, changed, mail) in cases) { + val data = buildMap { + if (kind != null) put("kind", kind) + put("accountLabel", "alice") + put("accountId", "c") + put("emailIds", "[]") + if (changed != null) put("changed", changed) + } + assertEquals("kind=$kind changed=$changed", mail, PushRouting.decide(data, routes).startMailTask) + } + } + @Test fun `accounts without routes are ignored`() { val decision = PushRouting.decide(stateChange("""{"elsewhere":{"ContactCard":"s"}}"""), routes) diff --git a/src/lib/__tests__/push-background-task.test.ts b/src/lib/__tests__/push-background-task.test.ts index 8f1f0dae..b8298866 100644 --- a/src/lib/__tests__/push-background-task.test.ts +++ b/src/lib/__tests__/push-background-task.test.ts @@ -238,6 +238,63 @@ describe('pushes for device sync (#34)', () => { expect(jmapClient.getStoredCredentials).toHaveBeenCalledWith(LOCAL); }); + // kind (undefined: none), changed (undefined: none) → whether the push may + // carry mail. The same table as "reads a push the same way as the push task" + // in PushRoutingTest.kt, whose startMailTask must agree. + const STATE = 'jmap-state-change'; + const EMAIL_PUSH = 'jmap-email-push'; + const MAIL_CASES: Array<[string | undefined, string | undefined, boolean]> = [ + // No readable map: as before device sync. + [STATE, undefined, true], + [STATE, '', true], + [STATE, 'not json', true], + [STATE, '[1]', true], + [STATE, '{}', true], + // A map that names no type is read like a missing one. + [STATE, '{"c":{}}', true], + [STATE, '{"c":{},"team":{}}', true], + // An account entry that can't be read may be mail. + [STATE, '{"c":null}', true], + [STATE, '{"c":"garbled","team":{"ContactCard":"s"}}', true], + [STATE, '{"c":[],"team":{"ContactCard":"s"}}', true], + [STATE, '{"c":5}', true], + // Mail types anywhere in the map. + [STATE, '{"c":{"EmailDelivery":"s"}}', true], + [STATE, '{"c":{"Email":"s"}}', true], + [STATE, '{"c":{"Mailbox":"s"}}', true], + [STATE, '{"c":{"ContactCard":"s1"},"team":{"EmailDelivery":"s2"}}', true], + // Only a StateChange can say it carries no mail. + [undefined, '{"c":{"ContactCard":"s"}}', true], + [EMAIL_PUSH, '{"c":{"EmailDelivery":"s"}}', true], + [EMAIL_PUSH, undefined, true], + // Contact and calendar changes, and types that say nothing about mail. + [STATE, '{"c":{"ContactCard":"s1","AddressBook":"s2"}}', false], + [STATE, '{"c":{"CalendarEvent":"e1"},"team":{"Calendar":"c1"}}', false], + [STATE, '{"c":{"ContactCard":"s"},"team":{}}', false], + [STATE, '{"elsewhere":{"ContactCard":"s"}}', false], + [STATE, '{"c":{"Thread":"t1","ContactCard":"s1"}}', false], + ]; + + it('reads a push the same way as the native router', () => { + for (const [kind, changed, mail] of MAIL_CASES) { + const data: Record = { accountLabel: 'alice', accountId: 'c', emailIds: '[]' }; + if (kind !== undefined) data.kind = kind; + if (changed !== undefined) data.changed = changed; + expect(carriesNoMail(parseRelayPushData(data)), `kind=${kind} changed=${changed}`).toBe(!mail); + } + // Native code hands `changed` over as a string; a map that is not an + // object is unreadable in any form. + expect(carriesNoMail(parseRelayPushData({ kind: STATE, changed: [{ ContactCard: 's' }] }))).toBe(false); + }); + + it("looks for mail when it can't read an account's entry, as the native router does", async () => { + for (const changed of ['{"jmap-primary":null}', '{"jmap-primary":{}}']) { + vi.clearAllMocks(); + await pushBackgroundTask({ kind: STATE, accountLabel: 'alice', accountId: 'jmap-primary', emailIds: '[]', changed }); + expect(jmapClient.getStoredCredentials, changed).toHaveBeenCalledWith(LOCAL); + } + }); + it('takes an Email or Mailbox change for possible mail, as a subscription from an older build sends it', () => { expect(carriesNoMail(parseRelayPushData(stateChange({ a: { Email: 's1', Mailbox: 's2' } })))).toBe(false); expect(carriesNoMail(parseRelayPushData(stateChange({ a: { Email: 's1' } })))).toBe(false); diff --git a/src/lib/__tests__/push-notifications.test.ts b/src/lib/__tests__/push-notifications.test.ts index 11db452c..44f1c2b2 100644 --- a/src/lib/__tests__/push-notifications.test.ts +++ b/src/lib/__tests__/push-notifications.test.ts @@ -409,7 +409,7 @@ describe('push types for device sync (#34)', () => { } // A server that does not know a type refuses the whole write. - const refusedTypes = (types: string[] | undefined) => { + const refuseUnknownTypes = (types: string[] | undefined) => { const unknown = (types ?? []).filter((type) => type !== 'EmailDelivery'); if (unknown.length > 0) throw new JMAPMethodError('invalidProperties', `Unknown types: ${unknown.join(', ')}`); }; @@ -458,7 +458,7 @@ describe('push types for device sync (#34)', () => { capabilities: { 'urn:ietf:params:jmap:core': {} }, }; createMock.mockImplementation(async (params: { types: string[] }) => { - refusedTypes(params.types); + refuseUnknownTypes(params.types); return CREATED; }); const result = await setupPushNotifications({ relayBaseUrl: RELAY }); @@ -483,10 +483,10 @@ describe('push types for device sync (#34)', () => { it('keeps mail push on a server that refuses the device sync types', async () => { await syncing({ [CONTACTS]: true }); createMock.mockImplementation(async (params: { types: string[] }) => { - refusedTypes(params.types); + refuseUnknownTypes(params.types); return CREATED; }); - updateMock.mockImplementation(async (_id: string, patch: { types?: string[] }) => refusedTypes(patch.types)); + updateMock.mockImplementation(async (_id: string, patch: { types?: string[] }) => refuseUnknownTypes(patch.types)); // A new subscription: mail-only rather than none. const created = await setupPushNotifications({ relayBaseUrl: RELAY }); diff --git a/src/lib/push-background-task.ts b/src/lib/push-background-task.ts index 2c703f8e..1f513bc9 100644 --- a/src/lib/push-background-task.ts +++ b/src/lib/push-background-task.ts @@ -154,7 +154,7 @@ export function parseRelayPushData(data: unknown): RelayPushData { } catch { // ignore } - } else if (d.changed && typeof d.changed === 'object') { + } else if (d.changed && typeof d.changed === 'object' && !Array.isArray(d.changed)) { out.changed = d.changed as Record>; } // Older relays only sent `changed`; the account id is its first key. @@ -389,14 +389,17 @@ const MAIL_TYPES = ['EmailDelivery', 'Email', 'Mailbox']; * calendar changes, which the native push router already turned into device * syncs (#34, docs/device-sync.md "Triggers"). Without a mail type there is * nothing to notify, and the legacy path (the newest unread mail) must not - * run for it. An empty or missing `changed` map keeps the old behaviour. + * run for it. A missing map, or one that names no type, keeps the old + * behaviour, and an account entry it can't read may be mail. The same rules + * as PushRouting.decide, so the two never disagree about a push. */ export function carriesNoMail(payload: RelayPushData): boolean { if (payload.kind !== 'jmap-state-change' || !payload.changed) return false; - const accounts = Object.values(payload.changed); - if (accounts.length === 0) return false; - return !accounts.some((types) => !!types && typeof types === 'object' - && MAIL_TYPES.some((type) => type in types)); + const entries: unknown[] = Object.values(payload.changed); + if (entries.some((types) => !types || typeof types !== 'object' || Array.isArray(types))) return false; + const types = entries.flatMap((entry) => Object.keys(entry as Record)); + if (types.length === 0) return false; + return !types.some((type) => MAIL_TYPES.includes(type)); } // Fired by BulwarkPushTaskService when a data FCM message arrives. Runs in a