fix: let Undo of a Files delete do nothing once another account is signed in

src/api/__tests__/files-trash.test.ts

@@ -3,6 +3,8 @@ import { describe, it, expect, vi, beforeEach } from 'vitest';
 vi.mock('../jmap-client', () => ({
   jmapClient: {
     accountId: 'acc-1',
+    serverUrl: 'https://mail.example.org',
+    username: 'me@example.org',
     request: vi.fn(),
     hasCapability: vi.fn(() => true),
     hasAccountCapability: vi.fn(() => false),
@@ -19,7 +21,7 @@ vi.mock('../jmap-client', () => ({
 
 import { jmapClient } from '../jmap-client';
 import type { FileNode } from '../types';
-import { emptyTrash, moveToTrash, purgeFromTrash, restoreFromTrash } from '../files-trash';
+import { emptyTrash, moveToTrash, purgeFromTrash, restoreFromTrash, undoMoveToTrash } from '../files-trash';
 import { trashedItems } from '../../lib/files-trash';
 
 // A Stalwart-like FileNode store for the user's own account: destroy without
@@ -83,7 +85,15 @@ function serve(method: string, args: Record<string, any>) {
 const listed = (): FileNode[] => store.map((n) => ({ ...n, type: n.type ?? '' }) as FileNode);
 const byId = (id: string) => store.find((n) => n.id === id);
 
+// Who is signed in: another login, or another files account of it.
+const client = jmapClient as unknown as { username: string; currentSession: { primaryAccounts: Record<string, string> } };
+function signIn(username: string, filesAccount = 'acc-1') {
+  client.username = username;
+  client.currentSession.primaryAccounts['urn:ietf:params:jmap:filenode'] = filesAccount;
+}
+
 beforeEach(() => {
+  signIn('me@example.org');
   nextId = 1;
   calls = [];
   refuseUpdate = new Set();
@@ -258,6 +268,63 @@ describe('restoreFromTrash', () => {
   });
 });
 
+describe('undoMoveToTrash', () => {
+  it('puts the items back while the same drive is signed in', async () => {
+    const before = listed();
+    const moved = await moveToTrash(before, before.filter((n) => n.id === 'a'), 1730112000007);
+    expect((await undoMoveToTrash(moved))?.done).toHaveLength(1);
+    expect(byId('a')).toMatchObject({ parentId: 'docs' });
+  });
+
+  it('sends nothing once another login is signed in, whose ids may be the same', async () => {
+    const before = listed();
+    const moved = await moveToTrash(before, before.filter((n) => n.id === 'a'), 1730112000007);
+    signIn('other@example.org');
+    const sent = calls.length;
+    const requests = (jmapClient.request as ReturnType<typeof vi.fn>).mock.calls.length;
+    expect(await undoMoveToTrash(moved)).toBeNull();
+    expect(calls.length).toBe(sent);
+    expect((jmapClient.request as ReturnType<typeof vi.fn>).mock.calls.length).toBe(requests);
+    expect(byId(byId('a')!.parentId!)!.name).toBe('1730112000007~docs');
+  });
+
+  it('sends nothing once the login reads another files account', async () => {
+    const before = listed();
+    const moved = await moveToTrash(before, before.filter((n) => n.id === 'a'), 1730112000007);
+    signIn('me@example.org', 'acc-3');
+    const sent = calls.length;
+    expect(await undoMoveToTrash(moved)).toBeNull();
+    expect(calls.length).toBe(sent);
+  });
+
+  it('stops before the restore when the account changes while the drive is read', async () => {
+    const before = listed();
+    const moved = await moveToTrash(before, before.filter((n) => n.id === 'a'), 1730112000007);
+    const real = (jmapClient.request as ReturnType<typeof vi.fn>).getMockImplementation() as (c: any[]) => Promise<unknown>;
+    (jmapClient.request as ReturnType<typeof vi.fn>).mockImplementation(async (methodCalls: any[]) => {
+      const out = await real(methodCalls);
+      if (methodCalls[0][0] === 'FileNode/get') signIn('other@example.org');
+      return out;
+    });
+    const sent = calls.length;
+    expect(await undoMoveToTrash(moved)).toBeNull();
+    expect(calls.length).toBe(sent);
+  });
+
+  it('stops a delete whose account changes before its items are moved', async () => {
+    const before = listed();
+    const real = (jmapClient.request as ReturnType<typeof vi.fn>).getMockImplementation() as (c: any[]) => Promise<unknown>;
+    (jmapClient.request as ReturnType<typeof vi.fn>).mockImplementation(async (methodCalls: any[]) => {
+      const out = await real(methodCalls);
+      if (methodCalls[0][1].create) signIn('other@example.org');
+      return out;
+    });
+    await expect(moveToTrash(before, before.filter((n) => n.id === 'a'), 1730112000007)).rejects.toThrow('Another account');
+    expect(calls.some((c) => c.update)).toBe(false);
+    expect(byId('a')!.parentId).toBe('docs');
+  });
+});
+
 describe('purgeFromTrash and emptyTrash', () => {
   it('deletes items for good with what is inside, and their emptied trash folder', async () => {
     let nodes = listed();

src/api/files-trash.ts

@@ -1,6 +1,8 @@
 import type { FileNode } from './types';
+import { jmapClient } from './jmap-client';
 import {
-  createFolder, deleteFileNodes, destroyEmptyFolders, getAllFileNodes, isCrossAccountId, updateFileNodes,
+  createFolder, deleteFileNodes, destroyEmptyFolders, filesAccountId, getAllFileNodes, isCrossAccountId,
+  updateFileNodes,
 } from './files';
 import { JMAPSetError } from './jmap-result';
 import { getUniqueName } from '../lib/filenode-name';
@@ -22,6 +24,44 @@ export interface TrashWriteResult {
   refused: JMAPSetError[];
 }
 
+/**
+ * The drive a write went to: the login (server and user) and its files
+ * account. Every write resolves the account when it is sent, and node ids
+ * repeat across accounts and servers, so a write prepared for one drive
+ * must not go out once another is signed in.
+ */
+export interface TrashDrive {
+  server: string | null;
+  username: string | null;
+  accountId: string;
+}
+
+export function currentTrashDrive(): TrashDrive {
+  return { server: jmapClient.serverUrl, username: jmapClient.username, accountId: filesAccountId() };
+}
+
+function isCurrentDrive(drive: TrashDrive): boolean {
+  const now = currentTrashDrive();
+  return now.server === drive.server && now.username === drive.username && now.accountId === drive.accountId;
+}
+
+/** Thrown before a write would go to another drive than the one it was prepared for. */
+export class TrashDriveChangedError extends Error {
+  constructor() {
+    super('Another account is signed in');
+    this.name = 'TrashDriveChangedError';
+  }
+}
+
+function assertDrive(drive: TrashDrive | undefined): void {
+  if (drive && !isCurrentDrive(drive)) throw new TrashDriveChangedError();
+}
+
+/** A delete into the trash, with the drive it went to (for its undo). */
+export interface TrashMoveResult extends TrashWriteResult {
+  drive: TrashDrive;
+}
+
 /** The trash folder, created on the first delete. */
 async function ensureTrashFolder(nodes: FileNode[]): Promise<FileNode> {
   const known = findTrashFolder(nodes);
@@ -47,14 +87,15 @@ async function removeEmptyBatches(nodes: FileNode[], batchIds: Iterable<string>,
  * Move nodes of the user's own drive into the trash: one trash folder per
  * folder they came from, named after the time and that folder, like the
  * webmail does. A node of a shared drive can't go there; delete it instead.
- * Throws when nothing went.
+ * Stops once another account is signed in. Throws when nothing went.
  */
 export async function moveToTrash(
   nodes: FileNode[],
   targets: FileNode[],
   deletedAt: number = Date.now(),
-): Promise<TrashWriteResult> {
-  const result: TrashWriteResult = { done: [], refused: [] };
+): Promise<TrashMoveResult> {
+  const drive = currentTrashDrive();
+  const result: TrashMoveResult = { done: [], refused: [], drive };
   if (targets.length === 0) return result;
   if (targets.some((n) => n.isShared || isCrossAccountId(n.id))) {
     throw new JMAPSetError('forbidden', 'Files of a shared drive cannot be moved to the trash');
@@ -74,12 +115,15 @@ export async function moveToTrash(
       // The trash on screen may be gone (emptied on another device): read
       // the drive again and try once more.
       trash = await ensureTrashFolder(await getAllFileNodes());
+      assertDrive(drive);
       return createFolder(name, trash.id);
     }
   };
 
   for (const [parentId, group] of groups) {
+    if (!isCurrentDrive(drive)) break;
     const batch = await createBatch(parentId);
+    if (!isCurrentDrive(drive)) break;
     const update = await updateFileNodes(Object.fromEntries(group.map((n) => [n.id, { parentId: batch.id }])));
     const moved = new Set(update.updated);
     result.refused.push(...Object.values(update.notUpdated));
@@ -92,6 +136,7 @@ export async function moveToTrash(
     }
   }
   if (result.done.length === 0) {
+    assertDrive(drive);
     throw result.refused[0] ?? new JMAPSetError('setError', 'Failed to move to the trash');
   }
   return result;
@@ -105,12 +150,16 @@ export async function moveToTrash(
  * The drive is read again first, and that is what counts: the listing on
  * screen (for an undo, the one from before the delete) misses a folder
  * trashed since and a name taken since. Items no longer in the trash are left
- * where they are. Throws when nothing of what is still there went.
+ * where they are. Throws when nothing of what is still there went, and
+ * TrashDriveChangedError, with nothing sent, once `drive` is no longer the
+ * one signed in.
  */
-export async function restoreFromTrash(items: TrashedItem[]): Promise<TrashWriteResult> {
+export async function restoreFromTrash(items: TrashedItem[], drive?: TrashDrive): Promise<TrashWriteResult> {
   const result: TrashWriteResult = { done: [], refused: [] };
   if (items.length === 0) return result;
+  assertDrive(drive);
   const nodes = await getAllFileNodes();
+  assertDrive(drive);
   const trash = findTrashFolder(nodes);
   const hidden = trashSubtreeIds(nodes, trash?.id);
   // The items as the trash holds them now: their name and trash folder may
@@ -146,6 +195,7 @@ export async function restoreFromTrash(items: TrashedItem[]): Promise<TrashWrite
     updates[item.node.id] = name === item.node.name ? { parentId: target } : { parentId: target, name };
     targets.set(item.node.id, target);
   }
+  assertDrive(drive);
   const update = await updateFileNodes(updates);
   result.refused.push(...Object.values(update.notUpdated));
   const restored = new Set(update.updated);
@@ -156,10 +206,26 @@ export async function restoreFromTrash(items: TrashedItem[]): Promise<TrashWrite
     const name = updates[item.node.id].name ?? item.node.name;
     result.done.push({ ...item, node: { ...item.node, name, parentId: targets.get(item.node.id) ?? null } });
   }
-  await removeEmptyBatches(nodes, result.done.flatMap((i) => (i.batchId ? [i.batchId] : [])), restored);
+  if (!drive || isCurrentDrive(drive)) {
+    await removeEmptyBatches(nodes, result.done.flatMap((i) => (i.batchId ? [i.batchId] : [])), restored);
+  }
   return result;
 }
 
+/**
+ * Undo of a delete into the trash (see restoreFromTrash). Null, and nothing
+ * sent, once another account or server is signed in than the one the delete
+ * went to: the same ids name other files there.
+ */
+export async function undoMoveToTrash(moved: Pick<TrashMoveResult, 'done' | 'drive'>): Promise<TrashWriteResult | null> {
+  try {
+    return await restoreFromTrash(moved.done, moved.drive);
+  } catch (err) {
+    if (err instanceof TrashDriveChangedError) return null;
+    throw err;
+  }
+}
+
 /**
  * Delete items of the trash for good, with what is inside a deleted folder.
  * The drive is read again first and only what is still in the trash goes: a

src/screens/FilesScreen.tsx

@@ -51,7 +51,9 @@ import {
   getFileNameRules, getFileNodeDownloadUrl, getMaxSizeUpload, isFolder,
   isSymlink, listFolder, moveFileNode, renameFileNode, supportsSharing, uploadFileBatch,
 } from '../api/files';
-import { emptyTrash, moveToTrash, purgeFromTrash, restoreFromTrash } from '../api/files-trash';
+import {
+  emptyTrash, moveToTrash, purgeFromTrash, restoreFromTrash, undoMoveToTrash, type TrashMoveResult,
+} from '../api/files-trash';
 import { trashOriginPath, trashedItems, withoutTrash, type TrashedItem } from '../lib/files-trash';
 import { toast } from '../stores/toast-store';
 import { jmapClient } from '../api/jmap-client';
@@ -317,12 +319,16 @@ export default function FilesScreen() {
   // (another port, or Stalwart reinstalled there), which the account id does
   // not tell. Every new session reads the drive again.
   const driveRef = useRef<DriveScope | undefined>(undefined);
+  const undoToastRef = useRef<string | null>(null);
   useEffect(() => {
     const { action, scope } = driveChange(driveRef.current, activeAccountId, session);
     driveRef.current = scope;
     if (action === 'keep') return;
     if (action === 'reset') {
       uploadAbortRef.current?.abort();
+      // The last delete's Undo belongs to the previous drive.
+      if (undoToastRef.current) toast.dismiss(undoToastRef.current);
+      undoToastRef.current = null;
       setAllNodes([]);
       setPath([]);
       setInTrash(false);
@@ -745,11 +751,14 @@ export default function FilesScreen() {
   }, [uploading, currentParentId, allNodes, loadFiles, t]);
 
   // Undo of a delete: the items go back from the trash the way Restore puts
-  // them back, judged against the drive as it is by then.
-  const undoTrash = useCallback(async (items: TrashedItem[]) => {
+  // them back, judged against the drive as it is by then. Nothing happens
+  // once another account is signed in (see undoMoveToTrash).
+  const undoTrash = useCallback(async (moved: TrashMoveResult) => {
+    undoToastRef.current = null;
     try {
-      const { done } = await restoreFromTrash(items);
-      if (done.length > 0) toast.success(t('files.undo_success', 'Action undone'));
+      const result = await undoMoveToTrash(moved);
+      if (!result) return;
+      if (result.done.length > 0) toast.success(t('files.undo_success', 'Action undone'));
     } catch (e) {
       toast.error(t('files.undo_error', 'Failed to undo'), fileErrorMessage(e, t));
     }
@@ -763,14 +772,15 @@ export default function FilesScreen() {
     clearSelection();
     const listing = allNodes;
     try {
-      const { done, refused } = await moveToTrash(listing, rows);
+      const moved = await moveToTrash(listing, rows);
+      const { done, refused } = moved;
       await loadFiles('refresh');
       if (refused.length > 0) {
         Alert.alert(t('files.delete_error', 'Failed to delete'), fileErrorMessage(refused[0], t));
       }
-      toast.success(
+      undoToastRef.current = toast.success(
         t('files.moved_to_trash', '{count, plural, one {Moved to trash} other {# items moved to trash}}', { count: done.length }),
-        { action: { label: t('files.undo', 'Undo'), onPress: () => void undoTrash(done) } },
+        { action: { label: t('files.undo', 'Undo'), onPress: () => void undoTrash(moved) } },
       );
     } catch (e) {
       Alert.alert(t('files.delete_error', 'Failed to delete'), fileErrorMessage(e, t));