mirror of
https://github.com/bulwarkmail/native.git
synced 2026-10-31 08:06:36 +00:00
fix: keep mail push on servers without contacts or calendars #34
The push subscription got the contact and calendar types whenever the account had device sync on, without asking whether its server has contacts or calendars. Sync can be on for such an account: the settings list every signed-in account, and a server can drop a capability later. A server that refuses unknown types then refused the update and the replacement, and push setup failed: a new account got no mail push, an existing one lost it when its subscription expired. The types are now added only for a capability the account's session advertises, and a write the server refuses while it carries device sync's types is retried with EmailDelivery alone, so mail push never depends on device sync. Checked with the push setup tests against a fake server that refuses unknown types: a new subscription is created mail-only, an existing one is kept and refreshed, and another account's subscription is left alone when its server lacks the capability.
This commit is contained in:
Linus Rath
@@ -43,10 +43,21 @@ const { CREATED } = vi.hoisted(() => ({
|
||||
}));
|
||||
|
||||
// A client of another signed-in account (device sync patches its push types
|
||||
// without the singleton).
|
||||
const { DETACHED } = vi.hoisted(() => ({
|
||||
DETACHED: { loaded: true, types: ['EmailDelivery'] as string[], log: [] as unknown[] },
|
||||
}));
|
||||
// without the singleton), on a server with contacts and calendars unless a
|
||||
// test says otherwise.
|
||||
const { DETACHED, SYNC_SESSION } = vi.hoisted(() => {
|
||||
const SYNC_SESSION = {
|
||||
capabilities: {
|
||||
'urn:ietf:params:jmap:core': {},
|
||||
'urn:ietf:params:jmap:contacts': {},
|
||||
'urn:ietf:params:jmap:calendars': {},
|
||||
} as Record<string, unknown>,
|
||||
};
|
||||
return {
|
||||
SYNC_SESSION,
|
||||
DETACHED: { loaded: true, types: ['EmailDelivery'] as string[], log: [] as unknown[], session: SYNC_SESSION },
|
||||
};
|
||||
});
|
||||
|
||||
vi.mock('../../api/jmap-client', () => ({
|
||||
jmapClient: {
|
||||
@@ -56,6 +67,9 @@ vi.mock('../../api/jmap-client', () => ({
|
||||
currentSession: { capabilities: { 'urn:ietf:params:jmap:core': {} } },
|
||||
},
|
||||
JMAPClient: class {
|
||||
get currentSession() {
|
||||
return DETACHED.session;
|
||||
}
|
||||
async loadAccount(id: string) {
|
||||
DETACHED.log.push(['load', id]);
|
||||
return DETACHED.loaded;
|
||||
@@ -112,7 +126,7 @@ import {
|
||||
} from '../../api/push';
|
||||
import { getSharedMailboxes } from '../../api/email';
|
||||
import { JMAPMethodError } from '../../api/jmap-result';
|
||||
import type { EmailPushConfig } from '../../api/types';
|
||||
import type { EmailPushConfig, JMAPSession } from '../../api/types';
|
||||
import { jmapClient } from '../../api/jmap-client';
|
||||
import { NativeModules } from 'react-native';
|
||||
import { generateAccountId } from '../account-utils';
|
||||
@@ -394,26 +408,103 @@ 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 unknown = (types ?? []).filter((type) => type !== 'EmailDelivery');
|
||||
if (unknown.length > 0) throw new JMAPMethodError('invalidProperties', `Unknown types: ${unknown.join(', ')}`);
|
||||
};
|
||||
|
||||
beforeEach(async () => {
|
||||
vi.clearAllMocks();
|
||||
await AsyncStorage.clear();
|
||||
await AsyncStorage.setItem(deviceClientIdKey(ACCOUNT_ID), OUR_DCID);
|
||||
(jmapClient as { currentSession: unknown }).currentSession = {
|
||||
capabilities: { 'urn:ietf:params:jmap:core': {} },
|
||||
};
|
||||
(jmapClient as { currentSession: unknown }).currentSession = SYNC_SESSION;
|
||||
DETACHED.session = SYNC_SESSION;
|
||||
listMock.mockResolvedValue([]);
|
||||
updateMock.mockResolvedValue(true);
|
||||
installFetch({});
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
(jmapClient as { currentSession: unknown }).currentSession = {
|
||||
capabilities: { 'urn:ietf:params:jmap:core': {} },
|
||||
};
|
||||
DETACHED.session = SYNC_SESSION;
|
||||
createMock.mockImplementation(async () => CREATED);
|
||||
updateMock.mockImplementation(async () => undefined);
|
||||
});
|
||||
|
||||
it('adds the contact and calendar types of what the account syncs to the device', async () => {
|
||||
expect(await pushTypesFor(ACCOUNT_ID)).toEqual(['EmailDelivery']);
|
||||
expect(await pushTypesFor(ACCOUNT_ID, SYNC_SESSION as unknown as JMAPSession)).toEqual(['EmailDelivery']);
|
||||
await syncing({ [CONTACTS]: true, [CALENDAR]: false });
|
||||
expect(await pushTypesFor(ACCOUNT_ID)).toEqual(['EmailDelivery', 'ContactCard', 'AddressBook']);
|
||||
expect(await pushTypesFor('someone@else.example')).toEqual(['EmailDelivery']);
|
||||
expect(await pushTypesFor(ACCOUNT_ID, SYNC_SESSION as unknown as JMAPSession))
|
||||
.toEqual(['EmailDelivery', 'ContactCard', 'AddressBook']);
|
||||
expect(await pushTypesFor('someone@else.example', SYNC_SESSION as unknown as JMAPSession)).toEqual(['EmailDelivery']);
|
||||
// Removed in Android Settings: nothing to route any more.
|
||||
await syncing({ [CONTACTS]: true }, { removedInAndroidSettings: true });
|
||||
expect(await pushTypesFor(ACCOUNT_ID)).toEqual(['EmailDelivery']);
|
||||
expect(await pushTypesFor(ACCOUNT_ID, SYNC_SESSION as unknown as JMAPSession)).toEqual(['EmailDelivery']);
|
||||
});
|
||||
|
||||
it('adds only the types of what the server offers', async () => {
|
||||
await syncing({ [CONTACTS]: true, [CALENDAR]: true });
|
||||
const calendarsOnly = { capabilities: { 'urn:ietf:params:jmap:core': {}, 'urn:ietf:params:jmap:calendars': {} } };
|
||||
expect(await pushTypesFor(ACCOUNT_ID, calendarsOnly as unknown as JMAPSession))
|
||||
.toEqual(['EmailDelivery', 'CalendarEvent', 'Calendar']);
|
||||
expect(await pushTypesFor(ACCOUNT_ID, null)).toEqual(['EmailDelivery']);
|
||||
|
||||
// Sync was turned on while another account's server was served; this one
|
||||
// has neither: its subscription stays mail-only.
|
||||
(jmapClient as { currentSession: unknown }).currentSession = {
|
||||
capabilities: { 'urn:ietf:params:jmap:core': {} },
|
||||
};
|
||||
createMock.mockImplementation(async (params: { types: string[] }) => {
|
||||
refusedTypes(params.types);
|
||||
return CREATED;
|
||||
});
|
||||
const result = await setupPushNotifications({ relayBaseUrl: RELAY });
|
||||
expect(result.subscriptionId).toBe('new-server-id');
|
||||
expect(createMock.mock.calls.map((c) => c[0].types)).toEqual([['EmailDelivery']]);
|
||||
});
|
||||
|
||||
it("does not add types another account's server does not offer", async () => {
|
||||
const OTHER = 'bob@other.example.net';
|
||||
await AsyncStorage.setItem(`push:subscriptionId:v2:${OTHER}`, 'bob-sub');
|
||||
await AsyncStorage.setItem(DEVICE_SYNC_KEY, JSON.stringify({
|
||||
state: { accounts: { [OTHER]: { enabled: { [CONTACTS]: true } } } },
|
||||
version: 0,
|
||||
}));
|
||||
DETACHED.session = { capabilities: { 'urn:ietf:params:jmap:core': {}, 'urn:ietf:params:jmap:calendars': {} } };
|
||||
DETACHED.log = [];
|
||||
DETACHED.types = ['EmailDelivery'];
|
||||
await refreshPushSubscriptionTypes(OTHER);
|
||||
expect(DETACHED.log.map((entry) => (entry as unknown[])[0])).toEqual(['load', 'PushSubscription/get']);
|
||||
});
|
||||
|
||||
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);
|
||||
return CREATED;
|
||||
});
|
||||
updateMock.mockImplementation(async (_id: string, patch: { types?: string[] }) => refusedTypes(patch.types));
|
||||
|
||||
// A new subscription: mail-only rather than none.
|
||||
const created = await setupPushNotifications({ relayBaseUrl: RELAY });
|
||||
expect(created).toEqual({ subscriptionId: 'new-server-id', verified: true });
|
||||
expect(createMock.mock.calls.map((c) => c[0].types))
|
||||
.toEqual([['EmailDelivery', 'ContactCard', 'AddressBook'], ['EmailDelivery']]);
|
||||
|
||||
// The working subscription is kept and its expiry pushed forward, never
|
||||
// replaced by one the server refuses as well.
|
||||
vi.clearAllMocks();
|
||||
await AsyncStorage.setItem(SUB_KEY, 'existing');
|
||||
listMock.mockResolvedValue([healthy(['EmailDelivery'])]);
|
||||
const refreshed = await setupPushNotifications({ relayBaseUrl: RELAY });
|
||||
expect(refreshed.subscriptionId).toBe('existing');
|
||||
expect(updateMock.mock.calls.map((c) => c[1].types)).toEqual([['EmailDelivery', 'ContactCard', 'AddressBook'], undefined]);
|
||||
expect(updateMock.mock.calls[1][1].expires).toBeDefined();
|
||||
expect(createMock).not.toHaveBeenCalled();
|
||||
expect(destroyMock).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('subscribes with them', async () => {
|
||||
|
||||
@@ -12,10 +12,16 @@ import { loadedMailboxes } from './mailbox-source';
|
||||
import { jmapClient, JMAPClient } from '../api/jmap-client';
|
||||
import { JMAPMethodError, requireMethodResult } from '../api/jmap-result';
|
||||
import { CAPABILITIES } from '../api/types';
|
||||
import type { EmailPushConfig, JMAPAccountInfo, Mailbox } from '../api/types';
|
||||
import type { EmailPushConfig, JMAPAccountInfo, JMAPSession, Mailbox } from '../api/types';
|
||||
import { generateAccountId } from './account-utils';
|
||||
import { t } from '../stores/locale-store';
|
||||
import { DEVICE_SYNC_STORAGE_KEY, deviceSyncPushTypes, persistedAccounts } from '../device-sync/app/prefs';
|
||||
import {
|
||||
authorityOfType,
|
||||
DEVICE_SYNC_STORAGE_KEY,
|
||||
deviceSyncPushTypes,
|
||||
persistedAccounts,
|
||||
} from '../device-sync/app/prefs';
|
||||
import { CALENDAR_AUTHORITY, CONTACTS_AUTHORITY, type Authority } from '../device-sync/types';
|
||||
import {
|
||||
getUnifiedPushDistributors,
|
||||
isUnifiedPushSupported,
|
||||
@@ -207,19 +213,32 @@ export function isValidRelayUrl(value: string): boolean {
|
||||
// In-app sync uses the separate SSE channel and is unaffected.
|
||||
export const PUSH_TYPES = ['EmailDelivery'] as const;
|
||||
|
||||
// The capability a server advertises for the data types of an authority.
|
||||
const DEVICE_SYNC_CAPABILITIES: Record<Authority, string> = {
|
||||
[CONTACTS_AUTHORITY]: CAPABILITIES.CONTACTS,
|
||||
[CALENDAR_AUTHORITY]: CAPABILITIES.CALENDARS,
|
||||
};
|
||||
|
||||
/**
|
||||
* The push types of one account's subscription: `EmailDelivery`, plus the
|
||||
* contact and calendar types while the account syncs them to the device
|
||||
* (#34, docs/device-sync.md "Triggers"): the native push router turns those
|
||||
* into device syncs, and a push carrying only them never notifies.
|
||||
* into device syncs, and a push carrying only them never notifies. Only the
|
||||
* types of a capability the account's session advertises: a server without
|
||||
* contacts or calendars may refuse the whole subscription over them.
|
||||
*
|
||||
* Read from the device sync store's persisted JSON rather than the store, so
|
||||
* code that runs without it (the push task) never creates it.
|
||||
*/
|
||||
export async function pushTypesFor(accountId: string): Promise<string[]> {
|
||||
export async function pushTypesFor(accountId: string, session: JMAPSession | null): Promise<string[]> {
|
||||
let extra: string[] = [];
|
||||
try {
|
||||
extra = deviceSyncPushTypes(persistedAccounts(await AsyncStorage.getItem(DEVICE_SYNC_STORAGE_KEY))[accountId]);
|
||||
const capabilities = session?.capabilities ?? {};
|
||||
extra = deviceSyncPushTypes(persistedAccounts(await AsyncStorage.getItem(DEVICE_SYNC_STORAGE_KEY))[accountId])
|
||||
.filter((type) => {
|
||||
const authority = authorityOfType(type);
|
||||
return !!authority && DEVICE_SYNC_CAPABILITIES[authority] in capabilities;
|
||||
});
|
||||
} catch {
|
||||
extra = [];
|
||||
}
|
||||
@@ -404,6 +423,24 @@ async function writeWithEmailPush<T>(
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Run a subscription write with the account's push types and, when the server
|
||||
* refuses it while they include device sync's, once more with the mail types
|
||||
* alone: mail push never depends on device sync (#34).
|
||||
*/
|
||||
async function writeWithPushTypes<T>(
|
||||
types: readonly string[],
|
||||
write: (types: readonly string[]) => Promise<T>,
|
||||
): Promise<T> {
|
||||
try {
|
||||
return await write(types);
|
||||
} catch (err) {
|
||||
if (sameTypes(types, PUSH_TYPES)) throw err;
|
||||
logPhase('jmap', 'push types refused, retrying with the mail types alone');
|
||||
return write(PUSH_TYPES);
|
||||
}
|
||||
}
|
||||
|
||||
async function readRefusedEmailPushAccounts(accountId: string): Promise<string[]> {
|
||||
try {
|
||||
const parsed = JSON.parse((await AsyncStorage.getItem(emailPushRefusedKey(accountId))) ?? '[]');
|
||||
@@ -947,7 +984,7 @@ async function setupPushNotificationsInner(
|
||||
: null;
|
||||
const subKey = subscriptionIdKey(accountId);
|
||||
const storedServerId = await AsyncStorage.getItem(subKey);
|
||||
const types = await pushTypesFor(accountId);
|
||||
const types = await pushTypesFor(accountId, jmapClient.currentSession);
|
||||
let jmapAccountId: string | null = null;
|
||||
try {
|
||||
jmapAccountId = jmapClient.accountId;
|
||||
@@ -1006,14 +1043,16 @@ async function setupPushNotificationsInner(
|
||||
let serverAssignedId: string;
|
||||
let serverExpires: string | null;
|
||||
try {
|
||||
const created = await writeWithEmailPush(emailPush, (filter) =>
|
||||
createPushSubscription({
|
||||
deviceClientId,
|
||||
url: buildRelayUrl(relayBaseUrl, `/api/push/jmap/${encodeURIComponent(deviceClientId)}`),
|
||||
types: [...types],
|
||||
expires: expiresFromNow(SUBSCRIPTION_EXPIRES_DAYS),
|
||||
...(filter ? { emailPush: filter } : {}),
|
||||
}),
|
||||
const created = await writeWithPushTypes(types, (wanted) =>
|
||||
writeWithEmailPush(emailPush, (filter) =>
|
||||
createPushSubscription({
|
||||
deviceClientId,
|
||||
url: buildRelayUrl(relayBaseUrl, `/api/push/jmap/${encodeURIComponent(deviceClientId)}`),
|
||||
types: [...wanted],
|
||||
expires: expiresFromNow(SUBSCRIPTION_EXPIRES_DAYS),
|
||||
...(filter ? { emailPush: filter } : {}),
|
||||
}),
|
||||
),
|
||||
);
|
||||
serverAssignedId = created.result.id;
|
||||
serverExpires = created.result.expires;
|
||||
@@ -1063,8 +1102,9 @@ async function addPushAccountId(accountId: string): Promise<void> {
|
||||
// from what this client wants (a subscription created by an older build still
|
||||
// listens to `Email`/`Mailbox`; device sync was turned on or off; a Junk
|
||||
// mailbox id can change under us). Returns false if the server rejects the
|
||||
// update, which the caller treats as "replace"; otherwise the emailPush map
|
||||
// the update installed (null when it left the filter alone).
|
||||
// update, also with the mail types alone, which the caller treats as
|
||||
// "replace"; otherwise the emailPush map the update installed (null when it
|
||||
// left the filter alone).
|
||||
async function refreshSubscriptionExpires(
|
||||
sub: {
|
||||
id: string;
|
||||
@@ -1088,15 +1128,17 @@ async function refreshSubscriptionExpires(
|
||||
}
|
||||
}
|
||||
try {
|
||||
const patch: { expires?: string; types?: string[] } = {
|
||||
expires: expiresFromNow(SUBSCRIPTION_EXPIRES_DAYS),
|
||||
};
|
||||
if (typesNeedUpdate) patch.types = [...types];
|
||||
const { emailPush } = await writeWithEmailPush(
|
||||
emailPushNeedsUpdate ? desiredEmailPush : null,
|
||||
(filter) => updatePushSubscription(sub.id, filter ? { ...patch, emailPush: filter } : patch),
|
||||
);
|
||||
return { emailPush };
|
||||
return await writeWithPushTypes(types, async (wanted) => {
|
||||
const patch: { expires?: string; types?: string[] } = {
|
||||
expires: expiresFromNow(SUBSCRIPTION_EXPIRES_DAYS),
|
||||
};
|
||||
if (!sameTypes(sub.types, wanted)) patch.types = [...wanted];
|
||||
const { emailPush } = await writeWithEmailPush(
|
||||
emailPushNeedsUpdate ? desiredEmailPush : null,
|
||||
(filter) => updatePushSubscription(sub.id, filter ? { ...patch, emailPush: filter } : patch),
|
||||
);
|
||||
return { emailPush };
|
||||
});
|
||||
} catch {
|
||||
return false;
|
||||
}
|
||||
@@ -1231,12 +1273,12 @@ export async function refreshPushSubscriptionTypes(accountId: string): Promise<v
|
||||
if (await AsyncStorage.getItem(optedOutKey(accountId))) return;
|
||||
const subscriptionId = await AsyncStorage.getItem(subscriptionIdKey(accountId));
|
||||
if (!subscriptionId) return;
|
||||
const types = await pushTypesFor(accountId);
|
||||
const username = jmapClient.username;
|
||||
const serverUrl = jmapClient.serverUrl;
|
||||
const served = !!username && !!serverUrl && generateAccountId(username, serverUrl) === accountId;
|
||||
try {
|
||||
if (served) {
|
||||
const types = await pushTypesFor(accountId, jmapClient.currentSession);
|
||||
const current = (await listPushSubscriptions()).find((s) => s.id === subscriptionId);
|
||||
if (!current || sameTypes(current.types, types)) return;
|
||||
await updatePushSubscription(subscriptionId, { types: [...types] });
|
||||
@@ -1244,6 +1286,7 @@ export async function refreshPushSubscriptionTypes(accountId: string): Promise<v
|
||||
}
|
||||
const client = new JMAPClient();
|
||||
if (!(await client.loadAccount(accountId))) return;
|
||||
const types = await pushTypesFor(accountId, client.currentSession);
|
||||
const using = [CAPABILITIES.CORE];
|
||||
const res = await client.request(
|
||||
[['PushSubscription/get', { ids: [subscriptionId], properties: ['id', 'types'] }, '0']],
|
||||
|
||||
Reference in New Issue
Block a user