diff --git a/src/api/__tests__/email-keyword-refusals.test.ts b/src/api/__tests__/email-keyword-refusals.test.ts new file mode 100644 index 00000000..31f16342 --- /dev/null +++ b/src/api/__tests__/email-keyword-refusals.test.ts @@ -0,0 +1,131 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +// Stalwart refuses a keyword per message in Email/set `notUpdated`, not as a +// request error. 1.0 refuses keywords over 128 bytes and more than 100 +// keywords on one message; 0.16 accepts both (it cuts long keywords to 128 +// characters); a read-only shared folder refuses any on both. + +vi.mock('../jmap-client', () => ({ + jmapClient: { + accountId: 'f', + request: vi.fn(), + getMaxObjectsInGet: vi.fn(() => 500), + getMaxObjectsInSet: vi.fn(() => 500), + }, +})); + +import { jmapClient } from '../jmap-client'; +import { markMailboxAsRead, patchKeywordsForEmails } from '../email'; +import { isJMAPSetError, JMAPSetError } from '../jmap-result'; + +const mockRequest = jmapClient.request as ReturnType; +const mockMaxInSet = jmapClient.getMaxObjectsInSet as ReturnType; + +// Exactly what Stalwart 1.0 answers (webmail d7004b38 captured these). +const TOO_LONG = { + type: 'invalidProperties', + description: 'Keywords cannot be longer than 128 bytes.', + properties: ['keywords'], +}; +const TOO_MANY = { + type: 'tooManyKeywords', + description: 'An email cannot have more than 100 keywords.', + properties: ['keywords'], +}; + +type Call = [string, Record, string]; + +/** Answers each Email/set with `result(ids)` and each Email/query with `query()`. */ +function serve(result: (ids: string[]) => Record, query?: () => string[]) { + const sent: Call[] = []; + mockRequest.mockImplementation(async (calls: Call[]) => { + sent.push(...calls); + return { + methodResponses: calls.map(([name, args, id]) => { + if (name === 'Email/query') return [name, { accountId: 'f', ids: query?.() ?? [] }, id]; + const ids = Object.keys((args.update ?? {}) as Record); + return [name, { accountId: 'f', oldState: 'sfy', newState: 'sfz', ...result(ids) }, id]; + }), + }; + }); + return sent; +} + +const refuse = (error: Record, which: (id: string) => boolean = () => true) => (ids: string[]) => ({ + updated: Object.fromEntries(ids.filter((id) => !which(id)).map((id) => [id, null])), + notUpdated: Object.fromEntries(ids.filter(which).map((id) => [id, error])), +}); +const accept = (ids: string[]) => ({ updated: Object.fromEntries(ids.map((id) => [id, null])) }); + +beforeEach(() => { + vi.clearAllMocks(); + mockMaxInSet.mockReturnValue(500); +}); + +describe('keyword writes report what the server refused', () => { + it('rejects a tag over 128 bytes with the SetError on Stalwart 1.0', async () => { + serve(refuse(TOO_LONG)); + const err = await patchKeywordsForEmails(['m1'], { [`$label:${'x'.repeat(130)}`]: true }).catch((e) => e); + expect(isJMAPSetError(err)).toBe(true); + expect(err).toMatchObject({ type: 'invalidProperties', properties: ['keywords'] }); + expect((err as Error).message).toContain('Keywords cannot be longer than 128 bytes.'); + }); + + it('rejects one keyword too many on Stalwart 1.0', async () => { + serve(refuse(TOO_MANY)); + await expect(patchKeywordsForEmails(['m1'], { '$label:one-more': true })) + .rejects.toMatchObject({ type: 'tooManyKeywords' }); + }); + + it('resolves when Stalwart 0.16 takes the write', async () => { + serve(accept); + await expect(patchKeywordsForEmails(['m1', 'm2'], { $flagged: true })).resolves.toBeUndefined(); + }); + + it('writes every batch before reporting a refused message', async () => { + mockMaxInSet.mockReturnValue(1); + const sent = serve(refuse(TOO_MANY, (id) => id === 'm1')); + + await expect(patchKeywordsForEmails(['m1', 'm2', 'm3'], { '$label:work': true })) + .rejects.toThrow(JMAPSetError); + + expect(sent.map(([, args]) => Object.keys(args.update as Record))).toEqual([['m1'], ['m2'], ['m3']]); + }); +}); + +describe('markMailboxAsRead', () => { + it('marks what it can and then reports the refused messages (read-only shared folder)', async () => { + // The refused message stays unread and comes back in the next page. + const unread = ['m1', 'm2', 'm3']; + const sent = serve( + (ids) => { + const result = refuse({ type: 'forbidden', description: 'You are not allowed to modify this message.' }, (id) => id === 'm2')(ids); + for (const id of Object.keys(result.updated)) unread.splice(unread.indexOf(id), 1); + return result; + }, + () => [...unread], + ); + mockMaxInSet.mockReturnValue(3); + + const err = await markMailboxAsRead('a').catch((e) => e); + + expect(isJMAPSetError(err)).toBe(true); + expect(err).toMatchObject({ type: 'forbidden' }); + // Two rounds: m1..m3, then the refused m2 alone, which ends the loop. + expect(sent.filter(([name]) => name === 'Email/set')).toHaveLength(2); + expect(unread).toEqual(['m2']); + }); + + it('returns the count when every message is marked (Stalwart 0.16 and 1.0)', async () => { + const unread = ['m1', 'm2']; + serve( + (ids) => { + for (const id of ids) unread.splice(unread.indexOf(id), 1); + return accept(ids); + }, + () => [...unread], + ); + + expect(await markMailboxAsRead('a')).toBe(2); + }); +}); diff --git a/src/api/email.ts b/src/api/email.ts index 3d8ad6a4..79a9bb34 100644 --- a/src/api/email.ts +++ b/src/api/email.ts @@ -104,7 +104,10 @@ function stateWentBack(accountId: string, sinceState: string, newState: unknown) * Run one `Email/set` per `maxObjectsInSet`-sized slice of `update` / * `destroy`, checking every slice for `notUpdated` / `notDestroyed`. Over-limit * requests fail whole (RFC 8620 ยง3.6.1), so a long multi-select or a big - * offline replay must be split before it is sent. + * offline replay must be split before it is sent. Every slice is sent before + * the first refusal is thrown: a refused message (Stalwart 1.0 refuses + * keywords over 128 bytes and more than 100 keywords on one message) must not + * leave the rest of a selection unchanged. */ async function emailSetBatched( accountId: string, @@ -113,21 +116,30 @@ async function emailSetBatched( ): Promise<{ updated: string[]; destroyed: string[] }> { const out = { updated: [] as string[], destroyed: [] as string[] }; const size = maxInSet(); + let refusal: unknown = null; + const check = (body: Record, ids: string[]) => { + try { + assertSetResult(body, ids, what); + } catch (err) { + refusal ??= err; + } + }; const updateEntries = Object.entries(args.update ?? {}); for (const slice of batched(updateEntries, size)) { const res = await jmapClient.request([ ['Email/set', { accountId, update: Object.fromEntries(slice) }, '0'], ]); const body = requireMethodResult(res, '0', 'Email/set'); - assertSetResult(body, slice.map(([id]) => id), what); + check(body, slice.map(([id]) => id)); out.updated.push(...Object.keys(body.updated ?? {})); } for (const slice of batched(args.destroy ?? [], size)) { const res = await jmapClient.request([['Email/set', { accountId, destroy: slice }, '0']]); const body = requireMethodResult(res, '0', 'Email/set'); - assertSetResult(body, slice, what); + check(body, slice); out.destroyed.push(...((body.destroyed as string[] | undefined) ?? [])); } + if (refusal) throw refusal; return out; } @@ -339,11 +351,17 @@ export async function emptyMailbox(mailboxId: string, accountIdOverride?: string return totalDestroyed; } -/** Set `$seen` on every unread message in a folder. Returns the count. */ +/** + * Set `$seen` on every unread message in a folder. Returns the count. Throws + * the first refusal once the rest are marked: a refused message stays unread + * (a shared folder without the right to set it, a message at Stalwart 1.0's + * 100-keyword limit), and the folder would otherwise look done. + */ export async function markMailboxAsRead(mailboxId: string, accountIdOverride?: string): Promise { const accountId = accountIdOverride ?? jmapClient.accountId; const pageSize = Math.min(500, maxInSet()); let total = 0; + let refusal: unknown = null; for (;;) { const res = await jmapClient.request([ ['Email/query', { @@ -359,8 +377,15 @@ export async function markMailboxAsRead(mailboxId: string, accountIdOverride?: s const body = requireMethodResult(setRes, '0', 'Email/set'); const marked = Object.keys(body.updated ?? {}).length; total += marked; + try { + assertSetResult(body, ids, 'email'); + } catch (err) { + refusal ??= err; + } + // Refused messages come back in every page; stop once only they are left. if (marked === 0 || ids.length < pageSize) break; } + if (refusal) throw refusal; return total; } diff --git a/src/lib/__tests__/keyword-discovery.test.ts b/src/lib/__tests__/keyword-discovery.test.ts index 8ab2ade2..00ea5ca4 100644 --- a/src/lib/__tests__/keyword-discovery.test.ts +++ b/src/lib/__tests__/keyword-discovery.test.ts @@ -23,4 +23,14 @@ describe('keyword discovery (#658)', () => { expect(found[0].keyword).toBe('$label:work'); expect(found[0].label).toBe('Work'); }); + + it('never proposes the attachment keywords Stalwart 1.0 puts on every message', () => { + // 1.0 sets `$hasattachment` or `$hasnoattachment` on each message and + // derives hasAttachment from them; 0.16 never does. They are no tags. + const found = findUnrecognizedKeywords( + { $hasattachment: 40, $hasnoattachment: 60, $HasAttachment: 1, '$label:work': 3 }, + [], + ); + expect(found.map((f) => f.id)).toEqual(['work']); + }); }); diff --git a/src/lib/__tests__/keyword-sort-polarity.test.ts b/src/lib/__tests__/keyword-sort-polarity.test.ts index ad412102..b2672e05 100644 --- a/src/lib/__tests__/keyword-sort-polarity.test.ts +++ b/src/lib/__tests__/keyword-sort-polarity.test.ts @@ -1,11 +1,14 @@ -import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; const request = vi.fn(); +const { server } = vi.hoisted(() => ({ server: { session: { accounts: {} } as Record } })); vi.mock('../../api/jmap-client', () => ({ jmapClient: { accountId: 'acc-1', serverUrl: 'https://mail.example.com', - currentSession: { accounts: {} }, + get currentSession() { + return server.session; + }, request: (...args: unknown[]) => request(...args), }, })); @@ -151,4 +154,57 @@ describe('stored verdicts', () => { await next.resolveKeywordSortPolarity('acc-1'); expect(request).toHaveBeenCalledTimes(2); }); + + describe('when the server at the address changes', () => { + const caps = (names: string[]) => Object.fromEntries(names.map((urn) => [urn, {}])); + const session = (version: '0.16.22' | '1.0') => { + const extra = version === '1.0' ? ['urn:ietf:params:jmap:metadata'] : []; + return { + apiUrl: 'https://mail.example.com/jmap/', + primaryAccounts: { 'urn:ietf:params:jmap:mail': 'acc-1' }, + capabilities: caps(['urn:ietf:params:jmap:core', 'urn:ietf:params:jmap:mail', ...extra]), + accounts: { + 'acc-1': { + name: 'user', isPersonal: true, isReadOnly: false, + accountCapabilities: caps([ + 'urn:ietf:params:jmap:mail', 'urn:stalwart:jmap', + ...extra, ...(version === '1.0' ? ['urn:ietf:params:jmap:principals:owner'] : []), + ]), + }, + }, + }; + }; + + afterEach(() => { + server.session = { accounts: {} }; + }); + + it('probes Stalwart 1.0 afresh instead of reusing the inverted verdict of 0.16.22', async () => { + server.session = session('0.16.22'); + request.mockResolvedValue(probeResponse('a', 'b', true)); + expect(await resolveKeywordSortPolarity('acc-1')).toBe('inverted'); + await new Promise((r) => setTimeout(r, 0)); + + // Reinstalled as 1.0, which follows the RFC. + server.session = session('1.0'); + const next = await relaunch(); + request.mockResolvedValue(probeResponse('a', 'b', false)); + expect(await next.resolveKeywordSortPolarity('acc-1')).toBe('rfc'); + expect(request).toHaveBeenCalledTimes(2); + }); + + it('notices within one launch when the session changes', async () => { + server.session = session('0.16.22'); + request.mockResolvedValue(probeResponse('a', 'b', true)); + expect(await resolveKeywordSortPolarity('acc-1')).toBe('inverted'); + + server.session = session('1.0'); + request.mockResolvedValue(probeResponse('a', 'b', false)); + expect(await resolveKeywordSortPolarity('acc-1')).toBe('rfc'); + // And back: the 0.16.22 verdict is still the one for that server. + server.session = session('0.16.22'); + expect(await resolveKeywordSortPolarity('acc-1')).toBe('inverted'); + expect(request).toHaveBeenCalledTimes(2); + }); + }); }); diff --git a/src/lib/__tests__/thread-utils.test.ts b/src/lib/__tests__/thread-utils.test.ts index 13ff3814..c1dd0b8e 100644 --- a/src/lib/__tests__/thread-utils.test.ts +++ b/src/lib/__tests__/thread-utils.test.ts @@ -36,6 +36,12 @@ describe('tag ids', () => { expect(tagIdFromKeyword('$seen')).toBeNull(); expect(tagIdFromKeyword('$label:')).toBeNull(); }); + + it('shows no tag for the attachment keywords of Stalwart 1.0', () => { + expect(getEmailTagIds({ $hasattachment: true, '$label:work': true })).toEqual(['work']); + expect(getEmailTagIds({ $hasnoattachment: true })).toEqual([]); + expect(tagIdFromKeyword('$hasattachment')).toBeNull(); + }); }); describe('collapseThreads', () => { diff --git a/src/lib/keyword-sort-polarity.ts b/src/lib/keyword-sort-polarity.ts index cd629fcc..fa397c8f 100644 --- a/src/lib/keyword-sort-polarity.ts +++ b/src/lib/keyword-sort-polarity.ts @@ -9,6 +9,7 @@ import { type KeywordSortPolarity, type SortLevel, } from './message-list-order'; +import { sessionFingerprint } from './server-fingerprint'; // Port of the webmail's JMAPClient.resolveKeywordSortPolarity / // probeKeywordSortPolarity / buildListSort (lib/jmap/client.ts). Stalwart @@ -19,23 +20,30 @@ import { // (a server upgrade may change it), instead of probing on every start. const KEYWORD_SORT_PROBE_RETRY_MS = 5 * 60 * 1000; -// Bump the version when the probe or what it decides changes. -const STORAGE_KEY = 'keyword-sort-polarity:v1'; +// Bump the version when the probe, what it decides or the key changes. v2 +// keys verdicts by the server fingerprint too. +const STORAGE_KEY = 'keyword-sort-polarity:v2'; +const OLD_STORAGE_KEY = 'keyword-sort-polarity:v1'; const STORED_VERDICT_MAX_AGE_MS = 7 * 24 * 60 * 60 * 1000; type StoredVerdict = { verdict: KeywordSortPolarity | 'unsupported'; at: number }; let stored: Record = {}; let hydration: Promise | null = null; -// Keyed by `|` so two logged-in accounts on the -// same server, or the same id on two servers, never share a verdict. +// Keyed by `||` so two +// logged-in accounts on the same server, or the same id on two servers, never +// share a verdict, and a server replaced or upgraded at the same address +// (Stalwart 1.0 follows the RFC where 0.16.8 to 0.16.22 invert) is probed +// afresh. The session shows no version, so an upgrade that leaves the +// capabilities alone (0.16.22 to 0.16.23) still waits out the week. const polarityCache = new Map(); const probes = new Map>(); // Accounts whose server refused a hasKeyword comparator with unsupportedSort. const unsupported = new Set(); function cacheKey(accountId: string): string { - return `${jmapClient.serverUrl ?? ''}|${accountId}`; + const session = jmapClient.currentSession; + return `${jmapClient.serverUrl ?? ''}|${accountId}|${session ? sessionFingerprint(session) : ''}`; } /** The server's advertised Email/query sort options, or null when unknown. */ @@ -83,6 +91,8 @@ function isStoredVerdict(value: unknown): value is StoredVerdict { // dropped and probed again. function hydrateStoredVerdicts(): Promise { hydration ??= (async () => { + // Verdicts stored without a fingerprint can't be matched to a server. + void AsyncStorage.removeItem(OLD_STORAGE_KEY).catch(() => undefined); let raw: string | null = null; try { raw = await AsyncStorage.getItem(STORAGE_KEY); diff --git a/src/screens/EmailListScreen.tsx b/src/screens/EmailListScreen.tsx index 148af7c5..ad7eedef 100644 --- a/src/screens/EmailListScreen.tsx +++ b/src/screens/EmailListScreen.tsx @@ -35,6 +35,7 @@ import { useLocaleStore } from '../stores/locale-store'; import { useSearchHistoryStore } from '../stores/search-history-store'; import { useContactsStore } from '../stores/contacts-store'; import { useOutboxStore } from '../stores/outbox-store'; +import { toast } from '../stores/toast-store'; import { getContactDisplayName } from '../lib/contact-utils'; import { formatListDate } from '../lib/date-format'; import { singleLine } from '../lib/single-line'; @@ -51,6 +52,14 @@ import { getFullEmail, emptyMailbox as apiEmptyMailbox } from '../api/email'; import type { RootStackParamList } from '../navigation/types'; import type { Attachment, Email } from '../api/types'; +// The server refuses keywords per message (a read-only shared folder; +// Stalwart 1.0: over 128 bytes, or more than 100 on one message), and the +// rows keep what they had. Say so rather than leaving the promise to drop it. +function reportKeywordFailure(title: string, err: unknown): void { + console.warn('[email-list]', title, err); + toast.error(title, err instanceof Error ? err.message : undefined); +} + function getSenderName(email: Email, unknownLabel: string): string { return email.from?.[0]?.name || email.from?.[0]?.email || unknownLabel; } @@ -640,14 +649,16 @@ export default function EmailListScreen({ onEmailPress, onComposePress }: EmailL } break; case 'read': - if (isUnread(email)) void markRead(id); - else void markUnread(id); + (isUnread(email) ? markRead(id) : markUnread(id)) + .catch((err) => reportKeywordFailure(t('notifications.error_updating', 'Failed to update email'), err)); break; case 'star': - void toggleStar(id, !isStarred(email)); + toggleStar(id, !isStarred(email)) + .catch((err) => reportKeywordFailure(t('notifications.error_updating', 'Failed to update email'), err)); break; case 'pin': - void togglePin(id, !isPinned(email)); + togglePin(id, !isPinned(email)) + .catch((err) => reportKeywordFailure(t('notifications.error_updating', 'Failed to update email'), err)); break; case 'move': setPendingMoveId(id); @@ -743,13 +754,21 @@ export default function EmailListScreen({ onEmailPress, onComposePress }: EmailL // One Email/set for the whole selection, not a request per message. const handleBulkMarkReadToggle = async () => { - await setKeywordForEmails(selectedMessageIds, '$seen', !allSelectedAreRead); - clearSelection(); + try { + await setKeywordForEmails(selectedMessageIds, '$seen', !allSelectedAreRead); + clearSelection(); + } catch (err) { + reportKeywordFailure(t('notifications.error_updating', 'Failed to update email'), err); + } }; const handleBulkStar = async () => { - await setKeywordForEmails(selectedMessageIds, '$flagged', !allSelectedAreStarred); - clearSelection(); + try { + await setKeywordForEmails(selectedMessageIds, '$flagged', !allSelectedAreStarred); + clearSelection(); + } catch (err) { + reportKeywordFailure(t('notifications.error_updating', 'Failed to update email'), err); + } }; const handleBulkDelete = async () => { @@ -795,7 +814,8 @@ export default function EmailListScreen({ onEmailPress, onComposePress }: EmailL }; const handleBatchTagToggle = (token: string, on: boolean) => { - void setKeywordForEmails(selectedMessageIds, token, on); + setKeywordForEmails(selectedMessageIds, token, on).catch((err) => + reportKeywordFailure(t('notifications.tag_failed', 'Tagging failed'), err)); }; // Local input state for uninterrupted typing. The search runs on submit, diff --git a/src/screens/EmailThreadScreen.tsx b/src/screens/EmailThreadScreen.tsx index 104f3852..e37b7484 100644 --- a/src/screens/EmailThreadScreen.tsx +++ b/src/screens/EmailThreadScreen.tsx @@ -341,7 +341,13 @@ function EmailViewer({ route, navigation }: Props) { // The keywords as they are now: a star or tag set during the delay stays. const current = peekDetail(target.id, ownerAccountId) ?? target; updateLocalKeywords(target.id, { ...current.keywords, $seen: true }); - void markSeen(target.id); + markSeen(target.id).catch((err) => { + // Refused (a shared folder without the right to set it; Stalwart 1.0 + // with 100 keywords on the message): it stays unread. + console.warn('[viewer] mark as read failed', err); + const { $seen: _seen, ...rest } = (peekDetail(target.id, ownerAccountId) ?? current).keywords ?? {}; + updateLocalKeywords(target.id, rest); + }); }; if (markAsReadDelay > 0) { const timer = setTimeout(apply, markAsReadDelay);