From 18b8aa096fb7e7feecbe4a93859d713953e43a2c Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Thu, 29 Oct 2026 02:01:47 +0100 Subject: [PATCH] fix: send writes in shared folders to the account that holds them and offer the ones its rights allow --- src/api/__tests__/files.test.ts | 81 ++++++++++++++++ src/api/files.ts | 125 +++++++++++++++++++------ src/lib/__tests__/files-access.test.ts | 59 ++++++++++++ src/lib/files-access.ts | 55 +++++++++++ src/screens/FilesScreen.tsx | 35 +++---- 5 files changed, 308 insertions(+), 47 deletions(-) create mode 100644 src/lib/__tests__/files-access.test.ts create mode 100644 src/lib/files-access.ts diff --git a/src/api/__tests__/files.test.ts b/src/api/__tests__/files.test.ts index d3f7f995..d49bac33 100644 --- a/src/api/__tests__/files.test.ts +++ b/src/api/__tests__/files.test.ts @@ -45,6 +45,7 @@ import { listPrincipals, moveFileNode, renameFileNode, + resolveFileNodeId, setFileNodeShare, supportsSharing, uploadFileBatch, @@ -1014,6 +1015,86 @@ describe('moveFileNode', () => { }); }); +describe('writes in a shared account', () => { + const CAP_100 = { forbiddenNameChars: '/<>:"\\|?*', supportedNodeTypes: ['Directory', 'File', 'SymLink'] }; + + beforeEach(() => { + // A Stalwart 1.0.0 session where the shared drive publishes no filenode + // capability of its own (non-personal accounts don't always). + setSession({ + primaryAccounts: { [CAPABILITIES.FILES]: 'acc-1' }, + accounts: { + 'acc-1': { name: 'me@example.com', isPersonal: true, accountCapabilities: { [CAPABILITIES.FILES]: CAP_100 } }, + 'acc-2': { name: 'Team', isPersonal: false, accountCapabilities: {} }, + }, + }); + }); + + const sent = () => mockRequest.mock.calls.map(([calls]) => calls[0][1] as Record); + + it('creates folders and uploads in the account that holds the folder, with the id it knows', async () => { + (uploadBlob as ReturnType).mockResolvedValueOnce({ blobId: 'blob-up', type: 'text/plain', size: 3 }); + mockRequest + .mockResolvedValueOnce({ methodResponses: [['FileNode/set', { created: { 'new-dir': { id: 'n1' } } }, '0']] }) + .mockResolvedValueOnce({ methodResponses: [['FileNode/set', { created: { 'new-file': { id: 'n2' } } }, '0']] }); + + const dir = await createFolder('New', 'acc-2:s1'); + const { uploaded } = await uploadFileBatch([{ uri: 'file:///a', name: 'a.txt', mimeType: 'text/plain' }], 'acc-2:s1', []); + + expect(sent().map((a) => [a.accountId, Object.values(a.create)[0]])).toEqual([ + ['acc-2', { '@type': 'Directory', name: 'New', parentId: 's1' }], + ['acc-2', { '@type': 'File', name: 'a.txt', mediaType: 'text/plain', blobId: 'blob-up', parentId: 's1' }], + ]); + // Named like the listing names that account's nodes. + expect(dir).toMatchObject({ id: 'acc-2:n1', parentId: 'acc-2:s1', accountId: 'acc-2', isShared: true }); + expect(uploaded[0].id).toBe('acc-2:n2'); + }); + + it('renames, moves, duplicates and shares in the node\'s account', async () => { + mockRequest.mockImplementation(async (calls: MethodCall[]) => { + const args = calls[0][1] as Record; + if (args.create) return { methodResponses: [['FileNode/set', { created: { 'new-file': { id: 'c1' } } }, '0']] }; + const id = Object.keys(args.update)[0]; + return { methodResponses: [['FileNode/set', { updated: { [id]: null } }, '0']] }; + }); + + await renameFileNode('acc-2:f1', 'b.txt'); + await moveFileNode('acc-2:f1', 'acc-2:s1'); + await copyFileNode({ id: 'acc-2:f1', name: 'b.txt', type: 'text/plain', blobId: 'b1', accountId: 'acc-2' }, 'acc-2:s1', 'b (1).txt'); + await setFileNodeShare('acc-2:f1', 'p1', null); + + expect(sent()).toEqual([ + { accountId: 'acc-2', update: { f1: { name: 'b.txt' } } }, + { accountId: 'acc-2', update: { f1: { parentId: 's1' } } }, + { accountId: 'acc-2', create: { 'new-file': { '@type': 'File', name: 'b (1).txt', mediaType: 'text/plain', blobId: 'b1', parentId: 's1' } } }, + { accountId: 'acc-2', update: { f1: { 'shareWith/p1': null } } }, + ]); + }); + + it('refuses a move into another account before sending anything', async () => { + await expect(moveFileNode('acc-2:f1', 'own-dir')).rejects.toThrow('Files cannot be moved between accounts'); + await expect(moveFileNode('own-file', 'acc-2:s1')).rejects.toThrow('Files cannot be moved between accounts'); + expect(mockRequest).not.toHaveBeenCalled(); + }); + + it('deletes each account\'s nodes in that account', async () => { + mockRequest.mockResolvedValue({ methodResponses: [['FileNode/set', { destroyed: [] }, '0']] }); + + await deleteFileNodes(['own-1', 'acc-2:f1', 'acc-2:f2']); + + expect(sent().map((a) => [a.accountId, a.destroy])).toEqual([ + ['acc-1', ['own-1']], + ['acc-2', ['f1', 'f2']], + ]); + }); + + it('leaves an id alone whose prefix is no account of the session', () => { + expect(resolveFileNodeId('nope:f1')).toEqual({ accountId: 'acc-1', id: 'nope:f1' }); + expect(resolveFileNodeId('acc-1:f1')).toEqual({ accountId: 'acc-1', id: 'acc-1:f1' }); + expect(resolveFileNodeId(null)).toEqual({ accountId: 'acc-1', id: null }); + }); +}); + describe('FileNode/set refused by the server', () => { const methodError = { methodResponses: [['error', { type: 'forbidden', description: 'No write access' }, '0']], diff --git a/src/api/files.ts b/src/api/files.ts index 1a30dc28..86193963 100644 --- a/src/api/files.ts +++ b/src/api/files.ts @@ -167,6 +167,50 @@ export function isCrossAccountId(id: string | null | undefined): boolean { return id != null && id.includes(':'); } +/** + * The account and bare id behind a FileNode id as the listings name it. A + * write must go to the account that holds the node, with the id that + * account knows: sent to the user's own account, "acc:node" is an unknown + * id at best and, stripped, could name an unrelated node of the user's own. + * `null` is the user's own drive root. + */ +export function resolveFileNodeId(id: string | null): { accountId: string; id: string | null } { + const primary = filesAccountId(); + if (id === null) return { accountId: primary, id: null }; + const sep = id.indexOf(':'); + if (sep > 0) { + const accountId = id.slice(0, sep); + if (accountId !== primary && filesCapableAccountIds().includes(accountId)) { + return { accountId, id: id.slice(sep + 1) }; + } + } + return { accountId: primary, id }; +} + +/** A node created or listed in `accountId`, with ids namespaced as the listings do. */ +function namespaceFileNode(node: FileNode, accountId: string): FileNode { + if (accountId === filesAccountId()) return node; + return { + ...node, + id: `${accountId}:${node.id}`, + parentId: node.parentId == null ? null : `${accountId}:${node.parentId}`, + accountId, + isShared: true, + }; +} + +/** + * The bare id of `parentId` in `accountId`. Folders hold nodes of their own + * account only, so a parent elsewhere is refused before anything is sent. + */ +function parentInAccount(parentId: string | null, accountId: string): string | null { + const parent = resolveFileNodeId(parentId); + if (parent.accountId !== accountId) { + throw new JMAPSetError('invalidProperties', 'Files cannot be moved between accounts', ['parentId']); + } + return parent.id; +} + /** Ids asked for per FileNode/query page; Stalwart clamps it to queryMaxResults (5000 by default). */ const FILE_NODE_QUERY_PAGE = 5000; /** Safety bound on how many FileNode ids one listing pages through. */ @@ -404,28 +448,33 @@ async function createFileNodeIn( return fromDraftFileNode({ ...wire, ...created } as FileNode); } +/** A new folder in `parentId`, which may be a folder of a shared account. */ export async function createFolder( name: string, parentId: string | null, ): Promise { - const accountId = filesAccountId(); + const parent = resolveFileNodeId(parentId); const props: Record = { name: acceptedFileName(name, getFileNameRules()) }; - if (parentId !== null) props.parentId = parentId; - return createFileNodeIn(accountId, props, 'new-dir', 'Create folder failed'); + if (parent.id !== null) props.parentId = parent.id; + const created = await createFileNodeIn(parent.accountId, props, 'new-dir', 'Create folder failed'); + return namespaceFileNode(created, parent.accountId); } export async function updateFileNode( id: string, updates: Partial>, ): Promise { - const accountId = filesAccountId(); + const node = resolveFileNodeId(id); + const nodeId = node.id as string; + const patch: Record = { ...updates }; + if ('parentId' in updates) patch.parentId = parentInAccount(updates.parentId ?? null, node.accountId); const res = await jmapClient.request( - [['FileNode/set', { accountId, update: { [id]: updates } }, '0']], + [['FileNode/set', { accountId: node.accountId, update: { [nodeId]: patch } }, '0']], fileUsing(), ); // A method-level error (e.g. forbidden) has no notUpdated entry, so without // this check a refused rename or move reported success. - const notUpdated = requireMethodResult(res, '0', 'FileNode/set').notUpdated?.[id]; + const notUpdated = requireMethodResult(res, '0', 'FileNode/set').notUpdated?.[nodeId]; if (notUpdated) throw fileSetError(notUpdated, 'Update failed'); } @@ -435,32 +484,42 @@ export async function renameFileNode(id: string, newName: string): Promise // Re-parent a node. `null` moves it to the drive root (the property must be // sent explicitly as null, omitting it would leave the node where it is). +// The new parent must be in the node's own account. export async function moveFileNode(id: string, parentId: string | null): Promise { await updateFileNode(id, { parentId }); } export async function deleteFileNodes(ids: string[]): Promise { if (ids.length === 0) return; - const accountId = filesAccountId(); + // Each account gets its own FileNode/set with the ids it knows. + const byAccount = new Map(); + for (const id of ids) { + const node = resolveFileNodeId(id); + const list = byAccount.get(node.accountId) ?? []; + list.push(node.id as string); + byAccount.set(node.accountId, list); + } // One FileNode/set per `maxObjectsInSet` ids: a request over the limit // fails whole (RFC 8620 §3.6.1), so a large selection deleted nothing. // Every batch runs; refusals are reported once all of them are done. const refused: SetError[] = []; - for (const slice of batched(ids, jmapClient.getMaxObjectsInSet())) { - const res = await jmapClient.request( - [['FileNode/set', { - accountId, - destroy: slice, - // The server removes descendant nodes of destroyed folders. - onDestroyRemoveChildren: true, - }, '0']], - fileUsing(), - ); - const notDestroyed = requireMethodResult(res, '0', 'FileNode/set').notDestroyed as - | Record - | undefined; - // A node inside a folder an earlier batch removed is already gone. - refused.push(...Object.values(notDestroyed ?? {}).filter((err) => err?.type !== 'notFound')); + for (const [accountId, accountIds] of byAccount) { + for (const slice of batched(accountIds, jmapClient.getMaxObjectsInSet())) { + const res = await jmapClient.request( + [['FileNode/set', { + accountId, + destroy: slice, + // The server removes descendant nodes of destroyed folders. + onDestroyRemoveChildren: true, + }, '0']], + fileUsing(), + ); + const notDestroyed = requireMethodResult(res, '0', 'FileNode/set').notDestroyed as + | Record + | undefined; + // A node inside a folder an earlier batch removed is already gone. + refused.push(...Object.values(notDestroyed ?? {}).filter((err) => err?.type !== 'notFound')); + } } if (refused.length > 0) { throw fileSetError(refused[0], `Failed to delete ${refused.length} item(s)`); @@ -483,14 +542,17 @@ async function createFileNodeFromBlob( type: string | undefined, parentId: string | null, ): Promise { - const accountId = filesAccountId(); + // The blob may come from the user's own account (an upload) or another + // one (a copy): Stalwart takes any blob the user may read. + const parent = resolveFileNodeId(parentId); const props: Record = { name: acceptedFileName(name, getFileNameRules()), type: mediaTypeEssence(type), blobId, }; - if (parentId !== null) props.parentId = parentId; - return createFileNodeIn(accountId, props, 'new-file', 'Upload failed'); + if (parent.id !== null) props.parentId = parent.id; + const created = await createFileNodeIn(parent.accountId, props, 'new-file', 'Upload failed'); + return namespaceFileNode(created, parent.accountId); } export async function uploadFileNode( @@ -588,19 +650,20 @@ export async function setFileNodeShare( principalId: string, rights: FileNodeRights | null, ): Promise { - const accountId = filesAccountId(); + const node = resolveFileNodeId(fileNodeId); + const nodeId = node.id as string; const res = await jmapClient.request( [['FileNode/set', { - accountId, - update: { [fileNodeId]: { [`shareWith/${principalId}`]: rights } }, + accountId: node.accountId, + update: { [nodeId]: { [`shareWith/${principalId}`]: rights } }, }, '0']], fileUsing(), ); const result = requireMethodResult(res, '0', 'FileNode/set'); - if (result.notUpdated?.[fileNodeId]) { - throw fileSetError(result.notUpdated[fileNodeId], 'Failed to update file share'); + if (result.notUpdated?.[nodeId]) { + throw fileSetError(result.notUpdated[nodeId], 'Failed to update file share'); } - if (!result.updated || !(fileNodeId in result.updated)) { + if (!result.updated || !(nodeId in result.updated)) { throw new Error('Server did not confirm the share update'); } } diff --git a/src/lib/__tests__/files-access.test.ts b/src/lib/__tests__/files-access.test.ts new file mode 100644 index 00000000..024b8e16 --- /dev/null +++ b/src/lib/__tests__/files-access.test.ts @@ -0,0 +1,59 @@ +import { describe, it, expect } from 'vitest'; +import type { FileNode, FileNodeRights } from '../../api/types'; +import { fileNodeAccess, mayCreateIn } from '../files-access'; + +// What the Files screen offers inside folders other accounts shared with the +// user. It used to offer no write there at all, since every write went to +// the user's own account. + +const NONE: FileNodeRights = { + mayRead: true, mayAddChildren: false, mayRename: false, mayDelete: false, mayModifyContent: false, mayShare: false, +}; +const WRITE: FileNodeRights = { ...NONE, mayAddChildren: true, mayRename: true, mayDelete: true, mayModifyContent: true }; + +const node = (id: string, parentId: string | null, extra: Partial = {}): FileNode => + ({ id, parentId, name: id, type: '', ...extra }); + +const own = [node('d1', null, { blobId: null }), node('f1', 'd1', { blobId: 'b1' })]; +const shared = (rootRights: FileNodeRights, childRights: FileNodeRights) => [ + // Stalwart 1.0's placeholder above the shared folder. + node('acc-2:top', null, { blobId: null, isShared: true, discoverableOnly: true, myRights: { ...NONE, mayRead: false } }), + node('acc-2:s1', 'acc-2:top', { blobId: null, isShared: true, myRights: rootRights }), + node('acc-2:f2', 'acc-2:s1', { blobId: 'b2', isShared: true, myRights: childRights }), +]; + +describe('fileNodeAccess', () => { + it('lets the user do everything with their own nodes', () => { + expect(fileNodeAccess(own[1], own)).toEqual({ rename: true, delete: true, move: true, duplicate: true }); + }); + + it('follows the rights inside a folder shared with write access', () => { + const nodes = [...own, ...shared(WRITE, WRITE)]; + expect(fileNodeAccess(nodes.at(-1)!, nodes)).toEqual({ rename: true, delete: true, move: false, duplicate: true }); + }); + + it('offers nothing inside a folder shared read-only', () => { + const nodes = shared(NONE, NONE); + expect(fileNodeAccess(nodes.at(-1)!, nodes)).toEqual({ rename: false, delete: false, move: false, duplicate: false }); + }); + + it('never renames or deletes the shared root, even with the rights', () => { + const nodes = shared(WRITE, WRITE); + expect(fileNodeAccess(nodes[1], nodes)).toEqual({ rename: false, delete: false, move: false, duplicate: false }); + }); +}); + +describe('mayCreateIn', () => { + it('creates in the own drive and its folders', () => { + expect(mayCreateIn(null, own)).toBe(true); + expect(mayCreateIn('d1', own)).toBe(true); + }); + + it('creates in a shared folder only with mayAddChildren', () => { + expect(mayCreateIn('acc-2:s1', shared(WRITE, NONE))).toBe(true); + expect(mayCreateIn('acc-2:s1', shared(NONE, NONE))).toBe(false); + // A placeholder folder is never a place to write. + expect(mayCreateIn('acc-2:top', shared(WRITE, NONE))).toBe(false); + expect(mayCreateIn('gone', own)).toBe(false); + }); +}); diff --git a/src/lib/files-access.ts b/src/lib/files-access.ts new file mode 100644 index 00000000..641ae2f2 --- /dev/null +++ b/src/lib/files-access.ts @@ -0,0 +1,55 @@ +import type { FileNode } from '../api/types'; + +/** + * What the Files screen offers for a node another account shared with the + * user (or a node of a shared drive). The writes go to that account + * (api/files resolveFileNodeId); what the server allows there is the node's + * `myRights`. Own nodes may do everything, as before. + * + * A shared root (the node the user was given, at the top of "Shared with + * me") is never renamed, deleted or moved from here: it sits in the owner's + * tree, under folders the user can't see. Moving any shared node is left + * out, like in the webmail: a node can only move within its own account, + * and the picker lists the user's own folders. + */ +export interface FileNodeAccess { + rename: boolean; + delete: boolean; + move: boolean; + /** A copy next to the original: needs the right to add to its folder. */ + duplicate: boolean; +} + +const ALL: FileNodeAccess = { rename: true, delete: true, move: true, duplicate: true }; + +function sharedParent(node: FileNode, nodes: FileNode[]): FileNode | undefined { + if (node.parentId == null) return undefined; + return nodes.find((n) => n.id === node.parentId && !n.discoverableOnly); +} + +export function fileNodeAccess(node: FileNode, nodes: FileNode[]): FileNodeAccess { + if (!node.isShared) return ALL; + const parent = sharedParent(node, nodes); + if (!parent) return { rename: false, delete: false, move: false, duplicate: false }; + const rights = node.myRights; + return { + rename: !!rights?.mayRename, + delete: !!rights?.mayDelete, + move: false, + duplicate: !!parent.myRights?.mayAddChildren, + }; +} + +/** + * Whether new folders and uploads may go into the folder `folderId` (null: + * the user's own drive root). Inside a shared folder only with its + * `mayAddChildren`; Stalwart 0.16 reports a child of a shared folder with + * every right false, so there it is offered in the shared folder itself. + */ +export function mayCreateIn(folderId: string | null, nodes: FileNode[]): boolean { + if (folderId === null) return true; + const folder = nodes.find((n) => n.id === folderId); + if (!folder) return false; + if (!folder.isShared) return true; + return !folder.discoverableOnly && !!folder.myRights?.mayAddChildren; +} diff --git a/src/screens/FilesScreen.tsx b/src/screens/FilesScreen.tsx index ebd6b36b..fb31a433 100644 --- a/src/screens/FilesScreen.tsx +++ b/src/screens/FilesScreen.tsx @@ -46,7 +46,7 @@ function loadDocumentPicker(): DocumentPickerModule | null { import { copyFileNode, createFolder, deleteFileNodes, getAllFileNodesAcrossAccounts, - getFileNameRules, getFileNodeDownloadUrl, getMaxSizeUpload, isCrossAccountId, isFolder, + getFileNameRules, getFileNodeDownloadUrl, getMaxSizeUpload, isFolder, isSymlink, listFolder, moveFileNode, renameFileNode, supportsSharing, uploadFileBatch, } from '../api/files'; import { jmapClient } from '../api/jmap-client'; @@ -62,6 +62,7 @@ import { useLayout, useSheetInsets } from '../lib/use-layout'; import { useBackWhileFocused } from '../lib/use-back-while-focused'; import { filesBackStep } from '../lib/files-back'; import { driveChange, type DriveScope } from '../lib/files-drive'; +import { fileNodeAccess, mayCreateIn, type FileNodeAccess } from '../lib/files-access'; import type { FileNode } from '../api/types'; import { spacing, radius, typography, componentSizes, type ThemePalette } from '../theme/tokens'; import { useColors } from '../theme/colors'; @@ -248,10 +249,9 @@ export default function FilesScreen() { const uploading = uploadProgress != null; const selectionMode = selection.size > 0; const currentParentId = path.length === 0 ? null : path[path.length - 1].id; - // True while browsing inside a folder another principal shared with us. - // Creates/uploads would have to route to the owner's account, which this - // screen doesn't do (matches the webmail's Files app); writes are hidden. - const inSharedSubtree = isCrossAccountId(currentParentId); + // Inside a folder another account shared with us, new folders and uploads + // go to that account, and only where its rights allow them. + const canCreateHere = mayCreateIn(currentParentId, allNodes); const rootLabel = t('files.title', 'Files'); const headerName = path.length === 0 ? rootLabel : path[path.length - 1].name; const sharingEnabled = supportsSharing(); @@ -561,7 +561,7 @@ export default function FilesScreen() { }, [renameTarget, renameValue, loadFiles, checkName, t]); const duplicateFile = useCallback(async (row: FileRow) => { - if (isFolder(row) || isSymlink(row) || row.isShared) return; + if (isFolder(row) || isSymlink(row) || !fileNodeAccess(row, allNodes).duplicate) return; setBusyId(row.id); try { const siblings = new Set( @@ -765,7 +765,7 @@ export default function FilesScreen() { > - {!inSharedSubtree ? ( + {canCreateHere ? ( setNewFolderOpen(true)} hitSlop={8} @@ -775,7 +775,7 @@ export default function FilesScreen() { ) : null} - {!inSharedSubtree ? ( + {canCreateHere ? ( void startUpload()} hitSlop={8} @@ -1096,6 +1096,7 @@ export default function FilesScreen() { setActionsTarget(null)} @@ -1238,6 +1239,8 @@ function PromptModal(props: PromptModalProps) { interface ActionsSheetProps { target: FileRow | null; + /** The writes the target allows (lib/files-access). */ + access: FileNodeAccess | null; sharingEnabled: boolean; t: Translate; onClose: () => void; @@ -1252,22 +1255,22 @@ interface ActionsSheetProps { } function ActionsSheet({ - target, sharingEnabled, t, onClose, onPreview, onOpenWith, onDownload, onShare, onMove, onDuplicate, onRename, onDelete, + target, access, sharingEnabled, t, onClose, onPreview, onOpenWith, onDownload, onShare, onMove, onDuplicate, onRename, onDelete, }: ActionsSheetProps) { // Centred on a wide window, clear of side insets in landscape. const sheetInsets = useSheetInsets(); const formatModified = useFormatModified(); const c = useColors(); const styles = React.useMemo(() => makeSheetStyles(c), [c]); - if (!target) return null; + if (!target || !access) return null; const isDir = isFolder(target); // A symlink (Stalwart 1.0) has no content: it can be renamed, moved and // deleted, but not previewed, opened, downloaded, duplicated or shared. const link = isSymlink(target); const isFile = !isDir && !link; // Owned nodes report full rights (or no myRights at all); shared-with-me - // nodes are managed by their owner, and write ops would have to route to - // the owner's account — so they only get preview/download here. + // nodes are shared on by their owner, and get the writes their rights + // allow (`access`). const canShare = sharingEnabled && !link && !target.isShared && (target.myRights?.mayShare ?? true); const modified = modifiedOf(target); return ( @@ -1312,25 +1315,25 @@ function ActionsSheet({ {t('files.sharing_access', 'Sharing & access')} ) : null} - {!target.isShared ? ( + {access.move ? ( onMove(target)}> {t('files.move_to', 'Move to…')} ) : null} - {!target.isShared && isFile ? ( + {access.duplicate && isFile ? ( onDuplicate(target)}> {t('files.duplicate', 'Duplicate')} ) : null} - {!target.isShared ? ( + {access.rename ? ( onRename(target)}> {t('files.rename', 'Rename')} ) : null} - {!target.isShared ? ( + {access.delete ? ( onDelete(target)}> {t('files.delete', 'Delete')}