mirror of
https://github.com/bulwarkmail/webmail.git
synced 2026-11-03 16:16:59 +00:00
Merge pull request #1307 from VasilisNtovolos/fix/bulk-delete-client-routing
fix: route bulk delete and trash by authenticated client
This commit is contained in:
Linus Rath
@@ -31,11 +31,11 @@ const mailbox = (overrides: Partial<Mailbox>): 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<string, ReturnType<typeof vi.fn>>;
|
||||
|
||||
const a = twin('account-a');
|
||||
@@ -44,8 +44,10 @@ let clientA: ReturnType<typeof makeClient>;
|
||||
let clientB: ReturnType<typeof makeClient>;
|
||||
|
||||
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': [],
|
||||
|
||||
@@ -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<string, unknown> };
|
||||
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<Login, Record<string, Mailbox[]>>;
|
||||
|
||||
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)]);
|
||||
});
|
||||
});
|
||||
+48
-39
@@ -4677,44 +4677,51 @@ export const useEmailStore = create<EmailStore>((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<string, { clientAccountId?: string; ids: string[]; keys: string[] }>();
|
||||
// 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<string, IJMAPClient | undefined>();
|
||||
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<string>();
|
||||
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);
|
||||
@@ -4723,23 +4730,25 @@ export const useEmailStore = create<EmailStore>((set, get) => ({
|
||||
const failedAccounts: string[] = [];
|
||||
// Keys, not ids: a namesake in an account that failed must stay.
|
||||
const movedKeys = new Set<string>();
|
||||
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));
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user