From 43be0a1bb38b133fb2b15366a85184f0bf9606f1 Mon Sep 17 00:00:00 2001 From: VasilisNtovolos Date: Wed, 21 Oct 2026 14:14:35 +0300 Subject: [PATCH] fix: route bulk delete and trash by authenticated client --- .../__tests__/email-batch-namesakes.test.ts | 15 +- .../email-store-bulk-delete-routing.test.ts | 204 ++++++++++++++++++ stores/email-store.ts | 87 ++++---- 3 files changed, 261 insertions(+), 45 deletions(-) create mode 100644 stores/__tests__/email-store-bulk-delete-routing.test.ts diff --git a/stores/__tests__/email-batch-namesakes.test.ts b/stores/__tests__/email-batch-namesakes.test.ts index 4ffae0142..b1eb51be1 100644 --- a/stores/__tests__/email-batch-namesakes.test.ts +++ b/stores/__tests__/email-batch-namesakes.test.ts @@ -31,11 +31,11 @@ const mailbox = (overrides: Partial): Mailbox => ({ ...overrides, } as Mailbox); -const makeClient = () => ({ +const makeClient = (mailboxes: Mailbox[]) => ({ batchDeleteEmails: vi.fn().mockResolvedValue(undefined), batchMoveEmails: vi.fn().mockResolvedValue(undefined), batchMarkAsRead: vi.fn().mockResolvedValue(undefined), - getMailboxes: vi.fn().mockResolvedValue([]), + getMailboxes: vi.fn().mockResolvedValue(mailboxes), }) as unknown as IJMAPClient & Record>; const a = twin('account-a'); @@ -44,8 +44,10 @@ let clientA: ReturnType; let clientB: ReturnType; beforeEach(() => { - clientA = makeClient(); - clientB = makeClient(); + const trashA = mailbox({ id: 'trash-a', name: 'Trash', role: 'trash', accountId: 'account-a' }); + const trashB = mailbox({ id: 'trash-b', name: 'Trash', role: 'trash', accountId: 'account-b' }); + clientA = makeClient([trashA]); + clientB = makeClient([trashB]); useAuthStore.setState({ activeAccountId: 'account-a', client: clientA, @@ -62,8 +64,8 @@ beforeEach(() => { lastSelectedEmailKey: null, mailboxes: [], accountMailboxes: { - 'account-a': [mailbox({ id: 'trash-a', name: 'Trash', role: 'trash', accountId: 'account-a' })], - 'account-b': [mailbox({ id: 'trash-b', name: 'Trash', role: 'trash', accountId: 'account-b' })], + 'account-a': [trashA], + 'account-b': [trashB], }, error: null, }); @@ -89,6 +91,7 @@ describe('batch actions with a namesake in another account', () => { }); it('keeps the namesake whose account has no trash when the other one moved', async () => { + vi.mocked(clientA.getMailboxes).mockResolvedValue([]); useEmailStore.setState({ accountMailboxes: { 'account-a': [], diff --git a/stores/__tests__/email-store-bulk-delete-routing.test.ts b/stores/__tests__/email-store-bulk-delete-routing.test.ts new file mode 100644 index 000000000..bbab0bb39 --- /dev/null +++ b/stores/__tests__/email-store-bulk-delete-routing.test.ts @@ -0,0 +1,204 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { useEmailStore } from '../email-store'; +import { useAuthStore } from '../auth-store'; +import { useSettingsStore } from '../settings-store'; +import { JMAPClient } from '@/lib/jmap/client'; +import { clearAllHooks } from '@/lib/plugin-hooks'; +import { emailKeyFor, threadKeyFor } from '@/lib/thread-utils'; +import type { Email, Mailbox } from '@/lib/jmap/types'; + +type Login = 'a' | 'b'; +type Args = { accountId: string; destroy?: string[]; update?: Record }; +type Request = { login: Login; authorization: string | null; method: string; args: Args }; +const server = (login: Login) => `https://${login}.example.invalid`; +const email = (login: Login, id: string, owner = 'c'): Email => ({ + id, threadId: 'thread', sourceClientAccountId: login, sourceAccountId: owner, + receivedAt: '2026-10-01T00:00:00Z', mailboxIds: { inbox: true }, keywords: {}, size: 100, hasAttachment: false, +} as Email); +const mailbox = (id: string, owner = 'c', shared = false): Mailbox => ({ + id, accountId: owner, name: 'Trash', role: 'trash', isShared: shared, + totalEmails: 0, unreadEmails: 0, totalThreads: 0, unreadThreads: 0, + myRights: { mayReadItems: true, mayAddItems: true, mayRemoveItems: true }, +} as Mailbox); +const unstamped = (id: string): Email => ({ + ...email('a', id), sourceClientAccountId: undefined, sourceAccountId: undefined, +}); + +// Exercise the real JMAP serialization and authentication. No connection/session +// request or keepalive is needed, and fetch is intercepted for every request. +function client(login: Login): JMAPClient { + const result = JMAPClient.withBearer(server(login), `synthetic-${login}`, `${login}@example.invalid`); + Object.assign(result, { accountId: 'c', apiUrl: `${server(login)}/jmap` }); + return result; +} + +describe('bulk Delete/Trash authenticated-client routing', () => { + let a: JMAPClient; + let b: JMAPClient; + let requests: Request[]; + let folders: Record>; + + function select(list: Email[], selected = list) { + useEmailStore.setState({ emails: list, selectedEmailKeys: new Set(selected.map(emailKeyFor)) }); + } + function writes() { + return requests.filter(r => r.method === 'Email/set'); + } + function expectWrite(login: Login, args: Args) { + expect(writes().filter(r => r.login === login)).toEqual([{ + login, authorization: `Bearer synthetic-${login}`, method: 'Email/set', args, + }]); + } + + beforeEach(() => { + clearAllHooks(); + a = client('a'); b = client('b'); requests = []; + folders = { a: { c: [mailbox('trash-a')] }, b: { c: [mailbox('trash-b')] } }; + vi.stubGlobal('fetch', vi.fn(async (url: string, init: RequestInit) => { + const login = (['a', 'b'] as const).find(id => url === `${server(id)}/jmap`); + if (!login) throw new Error(`Unexpected synthetic request: ${url}`); + const body = JSON.parse(String(init.body)) as { methodCalls: [string, Args, string][] }; + const methodResponses = body.methodCalls.map(([method, args, tag]) => { + requests.push({ login, authorization: new Headers(init.headers).get('Authorization'), method, args }); + if (method === 'Mailbox/get') return [method, { accountId: args.accountId, list: folders[login][args.accountId] ?? [] }, tag]; + if (method === 'Email/set') return [method, { + accountId: args.accountId, destroyed: args.destroy, + updated: args.update && Object.fromEntries(Object.keys(args.update).map(id => [id, null])), + }, tag]; + throw new Error(`Unexpected synthetic method: ${method}`); + }); + return new Response(JSON.stringify({ methodResponses }), { status: 200 }); + })); + useAuthStore.setState({ + activeAccountId: 'a', client: a, + getClientForAccount: (id: string) => id === 'a' ? a : id === 'b' ? b : undefined, + } as never); + useSettingsStore.setState({ deleteAction: 'trash', permanentlyDeleteJunk: false }); + useEmailStore.setState({ + isUnifiedView: true, unifiedRole: 'inbox', selectedMailbox: '__unified_inbox__', viewingAccountId: null, + selectedKeyword: null, searchQuery: '', emails: [], selectedEmail: null, selectedEmailKeys: new Set(), + mailboxes: [mailbox('wrong-active-trash')], + // This cache cannot distinguish the two servers' owner c. Never use it + // as a stamped message's authoritative Trash destination. + accountMailboxes: { c: [mailbox('wrong-cached-trash')] }, + threadEmailsCache: new Map(), unifiedScope: [], error: null, isLoading: false, + }); + }); + afterEach(() => { clearAllHooks(); vi.restoreAllMocks(); vi.unstubAllGlobals(); }); + + it('destroys selected A/c:p and B/c:q through their own clients, leaving unselected A/c:q alone', async () => { + const p = email('a', 'p'), unselected = email('a', 'q'), q = email('b', 'q'); + select([p, unselected, q], [p, q]); + expect(await useEmailStore.getState().batchDelete(a, true)).toBe(true); + expectWrite('a', { accountId: 'c', destroy: ['p'] }); + expectWrite('b', { accountId: 'c', destroy: ['q'] }); + expect(useEmailStore.getState().emails).toEqual([unselected]); + expect(useEmailStore.getState().selectedEmailKeys.size).toBe(0); + expect(requests.filter(r => r.method === 'Mailbox/get')).toEqual([]); + }); + + it('resolves each Trash through the same authenticated client and owner, despite a colliding folder cache', async () => { + const p = email('a', 'p'), unselected = email('a', 'q'), q = email('b', 'q'); + select([p, unselected, q], [p, q]); + expect(await useEmailStore.getState().batchDelete(a)).toBe(true); + for (const login of ['a', 'b'] as const) { + expect(requests.filter(r => r.login === login && r.method === 'Mailbox/get')).toEqual([{ + login, authorization: `Bearer synthetic-${login}`, method: 'Mailbox/get', args: { accountId: 'c' }, + }]); + expectWrite(login, { accountId: 'c', update: { [login === 'a' ? 'p' : 'q']: { mailboxIds: { [`trash-${login}`]: true } } } }); + } + expect(useEmailStore.getState().emails).toEqual([unselected]); + }); + + it.each([false, true])('keeps equal raw email and mailbox IDs on distinct clients (permanent=%s)', async permanent => { + folders.a.c = [mailbox('trash')]; folders.b.c = [mailbox('trash')]; + select([email('a', 'same'), email('b', 'same')]); + expect(await useEmailStore.getState().batchDelete(a, permanent)).toBe(true); + for (const login of ['a', 'b'] as const) expectWrite(login, permanent + ? { accountId: 'c', destroy: ['same'] } + : { accountId: 'c', update: { same: { mailboxIds: { trash: true } } } }); + expect(useEmailStore.getState().emails).toEqual([]); + }); + + it('keeps owners reached through one client separate and uses the delegated owner Trash', async () => { + folders.a.d = [mailbox('delegated-trash', 'd', true)]; + select([email('a', 'p'), email('a', 'q', 'd')]); + expect(await useEmailStore.getState().batchDelete(a)).toBe(true); + expect(writes().map(r => [r.login, r.authorization, r.args])).toEqual([ + ['a', 'Bearer synthetic-a', { accountId: 'c', update: { p: { mailboxIds: { 'trash-a': true } } } }], + ['a', 'Bearer synthetic-a', { accountId: 'd', update: { q: { mailboxIds: { 'delegated-trash': true } } } }], + ]); + }); + + it.each([false, true])('preserves unstamped primary-folder requests (permanent=%s)', async permanent => { + useEmailStore.setState({ isUnifiedView: false, selectedMailbox: 'inbox', mailboxes: [mailbox('primary-trash')] }); + select([unstamped('p')]); + expect(await useEmailStore.getState().batchDelete(a, permanent)).toBe(true); + expectWrite('a', permanent ? { accountId: 'c', destroy: ['p'] } + : { accountId: 'c', update: { p: { mailboxIds: { 'primary-trash': true } } } }); + expect(requests).toHaveLength(1); + }); + + it.each([false, true])('preserves direct shared-folder routing through the viewing client (permanent=%s)', async permanent => { + const trash = { ...mailbox('d:trash', 'd', true), originalId: 'trash' }; + useEmailStore.setState({ isUnifiedView: false, selectedMailbox: 'd:inbox', viewingAccountId: 'b', + accountMailboxes: { b: [{ ...trash, id: 'd:inbox', role: 'inbox' }, trash], d: [mailbox('wrong-owner-cache', 'd', true)] }, + }); + select([unstamped('p')]); + expect(await useEmailStore.getState().batchDelete(a, permanent)).toBe(true); + expectWrite('b', permanent ? { accountId: 'd', destroy: ['p'] } + : { accountId: 'd', update: { p: { mailboxIds: { trash: true } } } }); + expect(requests).toHaveLength(1); + }); + + it.each([false, true])('routes selected expanded-thread members with colliding IDs (permanent=%s)', async permanent => { + const row = email('a', 'row'), memberA = email('a', 'same'), memberB = email('b', 'same'); + select([row], [memberA, memberB]); + useEmailStore.setState({ threadEmailsCache: new Map([[threadKeyFor(memberA), [row, memberA]], [threadKeyFor(memberB), [memberB]]]) }); + expect(await useEmailStore.getState().batchDelete(a, permanent)).toBe(true); + expect(writes()).toHaveLength(2); + for (const login of ['a', 'b'] as const) expectWrite(login, permanent + ? { accountId: 'c', destroy: ['same'] } + : { accountId: 'c', update: { same: { mailboxIds: { [`trash-${login}`]: true } } } }); + expect(useEmailStore.getState().emails).toEqual([row]); + }); + + it.each([false, true])('does not redirect an unavailable explicit source, even without its owner stamp (permanent=%s)', async permanent => { + const unavailable = { ...email('b', 'q'), sourceAccountId: undefined }; + useAuthStore.setState({ getClientForAccount: (id: string) => id === 'a' ? a : undefined } as never); + select([unavailable]); + await useEmailStore.getState().batchDelete(a, permanent); + expect(requests).toEqual([]); + }); + + it.each([false, true])('does not merge an unavailable stamped source into a connected source with the same owner (permanent=%s)', async permanent => { + useAuthStore.setState({ getClientForAccount: (id: string) => id === 'a' ? a : undefined } as never); + select([email('a', 'p'), email('b', 'q')]); + await useEmailStore.getState().batchDelete(a, permanent); + expectWrite('a', permanent ? { accountId: 'c', destroy: ['p'] } + : { accountId: 'c', update: { p: { mailboxIds: { 'trash-a': true } } } }); + expect(requests.every(r => r.login === 'a')).toBe(true); + }); + + it.each([false, true])('does not redirect unstamped direct-folder messages when the viewing client disconnects (permanent=%s)', async permanent => { + useAuthStore.setState({ getClientForAccount: (id: string) => id === 'a' ? a : undefined } as never); + useEmailStore.setState({ isUnifiedView: false, selectedMailbox: 'd:inbox', viewingAccountId: 'b', + accountMailboxes: { b: [ + { ...mailbox('d:inbox', 'd', true), role: 'inbox' }, + { ...mailbox('d:trash', 'd', true), originalId: 'trash' }, + ] }, + }); + select([unstamped('q')]); + await useEmailStore.getState().batchDelete(a, permanent); + expect(requests).toEqual([]); + }); + + it('does not use another client’s cached Trash when the source owner has no Trash', async () => { + folders.b.c = []; + const q = email('b', 'q'); select([q]); + expect(await useEmailStore.getState().batchDelete(a)).toBe(false); + expect(writes()).toEqual([]); + expect(useEmailStore.getState().emails).toEqual([q]); + expect([...useEmailStore.getState().selectedEmailKeys]).toEqual([emailKeyFor(q)]); + }); +}); diff --git a/stores/email-store.ts b/stores/email-store.ts index cd4a71c5e..1d41dcd3c 100644 --- a/stores/email-store.ts +++ b/stores/email-store.ts @@ -4676,44 +4676,51 @@ export const useEmailStore = create((set, get) => ({ const forceDestroy = permanent || isInTrash || (isInJunk && permanentlyDeleteJunk); const alsoMarkRead = useSettingsStore.getState().deleteAction === 'trash-and-read'; - // Group emails by owning JMAP account (handles unified view and search results - // spanning accounts). Each group resolves the reaching login client via - // `sourceClientAccountId` and routes JMAP via `sourceAccountId`. Undecorated - // emails (normal single-mailbox view) fall into '__default__' = active client. - const accountMailboxes = get().accountMailboxes; - // Group the selected emails themselves: looking an id up in the list - // would find the first account's namesake and act on it instead. - const bySource = new Map(); + // Account ids are server-local. Match the authenticated client AND owner, + // as batchArchive does, so another login's namesake never joins a write. + // Unstamped folder rows keep the viewing client's primary/shared context. + const unstamped = resolveEmailActionContext({}, client); + const viewingLoginId = get().viewingAccountId; + const unstampedClient = viewingLoginId + ? useAuthStore.getState().getClientForAccount(viewingLoginId) + : unstamped.client; + type Group = { + client: IJMAPClient | undefined; + accountId: string | undefined; + stamped: boolean; + ids: string[]; + keys: string[]; + }; + const groups: Group[] = []; + const sourceClients = new Map(); for (const email of selectedEmails) { - const key = email.sourceAccountId || '__default__'; - if (!bySource.has(key)) bySource.set(key, { clientAccountId: email.sourceClientAccountId, ids: [], keys: [] }); - bySource.get(key)!.ids.push(email.id); - bySource.get(key)!.keys.push(emailKeyFor(email)); + const stamped = !!(email.sourceClientAccountId || email.sourceAccountId); + const loginId = email.sourceClientAccountId; + if (loginId && !sourceClients.has(loginId)) { + sourceClients.set(loginId, useAuthStore.getState().getClientForAccount(loginId)); + } + // An explicit source must never fall back to the active client, even + // when only its login stamp survives or its connection is unavailable. + const actionClient = stamped + ? (loginId ? sourceClients.get(loginId) : undefined) + : unstampedClient; + const accountId = stamped ? email.sourceAccountId : unstamped.accountId; + let group = groups.find(g => g.client === actionClient && g.accountId === accountId); + if (!group) { + group = { client: actionClient, accountId, stamped, ids: [], keys: [] }; + groups.push(group); + } + group.stamped ||= stamped; + group.ids.push(email.id); + group.keys.push(emailKeyFor(email)); } - // Undecorated emails viewed in a shared/group folder directly (non-unified) - // are reached through the active client but must carry the owner accountId, - // or the move/destroy silently no-ops on the user's own account (see - // resolveViewAccountId). undefined for a normal own-account view. - const viewAccountId = currentMailbox?.isShared ? currentMailbox.accountId : undefined; - const getClient = (sourceAccountId: string, clientAccountId?: string) => - sourceAccountId === '__default__' - ? resolveActionClient(client) - : (clientAccountId ? useAuthStore.getState().getClientForAccount(clientAccountId) : undefined); - const mailboxesFor = (sourceAccountId: string) => - sourceAccountId === '__default__' - ? (viewAccountId ? (accountMailboxes[viewAccountId] ?? mailboxes) : mailboxes) - : (accountMailboxes[sourceAccountId] ?? mailboxes); - const jmapIdFor = (sourceAccountId: string) => - sourceAccountId === '__default__' ? viewAccountId : sourceAccountId; - // Ids the server confirmed, for onAfterEmailDelete. const deletedIds = new Set(); if (forceDestroy) { - const promises = Array.from(bySource.entries()).map(async ([sourceAccountId, { clientAccountId, ids }]) => { - const acctClient = getClient(sourceAccountId, clientAccountId); + const promises = groups.map(async ({ client: acctClient, accountId, ids }) => { if (!acctClient) return; - await acctClient.batchDeleteEmails(ids, jmapIdFor(sourceAccountId)); + await acctClient.batchDeleteEmails(ids, accountId); ids.forEach(id => deletedIds.add(id)); }); await Promise.allSettled(promises); @@ -4722,23 +4729,25 @@ export const useEmailStore = create((set, get) => ({ const failedAccounts: string[] = []; // Keys, not ids: a namesake in an account that failed must stay. const movedKeys = new Set(); - const promises = Array.from(bySource.entries()).map(async ([sourceAccountId, { clientAccountId, ids, keys }]) => { - const acctClient = getClient(sourceAccountId, clientAccountId); + const promises = groups.map(async ({ client: acctClient, accountId, stamped, ids, keys }) => { if (!acctClient) { - failedAccounts.push(sourceAccountId); + failedAccounts.push(accountId ?? '__default__'); return; } - const trashMailbox = findTrashMailbox(mailboxesFor(sourceAccountId), { - accountId: jmapIdFor(sourceAccountId), - }); + // The owner-only cache is ambiguous across clients/servers. Resolve + // stamped sources through the same client/account pair as the write. + const sourceMailboxes = stamped + ? await acctClient.getMailboxes(accountId) + : unstamped.mailboxes; + const trashMailbox = findTrashMailbox(sourceMailboxes, { accountId }); if (!trashMailbox) { // No trash for this account: skip rather than silently destroying. // The user asked to move to trash, not permanently delete. - failedAccounts.push(sourceAccountId); + failedAccounts.push(accountId ?? '__default__'); return; } const trashId = trashMailbox.originalId || trashMailbox.id; - await acctClient.batchMoveEmails(ids, trashId, jmapIdFor(sourceAccountId), alsoMarkRead); + await acctClient.batchMoveEmails(ids, trashId, accountId, alsoMarkRead); keys.forEach(key => movedKeys.add(key)); ids.forEach(id => deletedIds.add(id)); });