From caea3d0c603846a9b66e1131f5f795516ad053c9 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Thu, 24 Sep 2026 18:37:11 +0200 Subject: [PATCH] fix: offer sharing files and calendars with other users on Stalwart `supportsSharing()` required `urn:ietf:params:jmap:principals:owner`, which Stalwart advertises nowhere; its session only has `principals` and `principals:availability`. The file menu therefore never showed "Sharing & access", and `getPrincipals()` returned nothing, so the calendar share sheet had no one to offer either. - Gate sharing on `urn:ietf:params:jmap:principals`, like the webmail's `supportsPrincipals()`. - Keep `principals:owner` out of FileNode requests unless the server advertises it, since an unknown capability in `using` fails the whole request (why the gate had been tied to it). - Page the principal lookup by `maxObjectsInGet`, as the webmail does: one unbounded `Principal/query` fed every id to `Principal/get`, which the server refuses once the directory has more than 500 entries. --- src/api/__tests__/files.test.ts | 55 +++++++++++++++++++++++----- src/api/files.ts | 65 +++++++++++++++++++++------------ 2 files changed, 87 insertions(+), 33 deletions(-) diff --git a/src/api/__tests__/files.test.ts b/src/api/__tests__/files.test.ts index 42ee232b..10a5d27b 100644 --- a/src/api/__tests__/files.test.ts +++ b/src/api/__tests__/files.test.ts @@ -426,6 +426,26 @@ describe('getPrincipals', () => { expect(await getPrincipals()).toEqual([]); expect(mockRequest).not.toHaveBeenCalled(); }); + + it('pages through a directory larger than maxObjectsInGet', async () => { + const directory = Array.from({ length: 1203 }, (_, i) => ({ id: `p${i}`, name: `user${i}`, type: 'individual' })); + mockRequest.mockImplementation(async (calls: [string, Record, string][]) => { + const { position, limit } = calls[0][1] as { position: number; limit: number }; + const page = directory.slice(position, position + limit); + return { + methodResponses: [ + ['Principal/query', { ids: page.map((p) => p.id), position }, '0'], + ['Principal/get', { list: page }, '1'], + ], + }; + }); + + const principals = await getPrincipals(); + + expect(principals).toHaveLength(1203); + expect(mockRequest.mock.calls.map(([calls]) => calls[0][1].position)).toEqual([0, 500, 1000]); + for (const [calls] of mockRequest.mock.calls) expect(calls[0][1].limit).toBe(500); + }); }); describe('decodeFileNodeName at the API boundary (#869)', () => { @@ -540,19 +560,34 @@ describe('accountSupportsFiles (#563)', () => { }); describe('supportsSharing', () => { - it('accepts principals:owner from the session or the account capabilities', () => { - const mockHasAccountCapability = jmapClient.hasAccountCapability as ReturnType; + // What Stalwart advertises: principals and principals:availability, never + // principals:owner. + const stalwartCapabilities = (urn: string) => + urn === CAPABILITIES.FILES || + urn === CAPABILITIES.PRINCIPALS || + urn === 'urn:ietf:params:jmap:principals:availability'; + + it('offers sharing when the server advertises principals', () => { + mockHasCapability.mockImplementation(stalwartCapabilities); + expect(supportsSharing()).toBe(true); + mockHasCapability.mockImplementation((urn: string) => urn === CAPABILITIES.FILES); - mockHasAccountCapability.mockReturnValue(false); expect(supportsSharing()).toBe(false); + }); - mockHasAccountCapability.mockImplementation((urn: string) => urn === CAPABILITIES.PRINCIPALS_OWNER); - expect(supportsSharing()).toBe(true); + it('still lists principals and leaves principals:owner out of FileNode requests on Stalwart', async () => { + mockHasCapability.mockImplementation(stalwartCapabilities); + mockRequest.mockResolvedValue({ + methodResponses: [ + ['Principal/query', { ids: ['p1'] }, '0'], + ['Principal/get', { list: [{ id: 'p1', name: 'other', type: 'individual' }] }, '1'], + ], + }); + expect(await getPrincipals()).toHaveLength(1); - mockHasAccountCapability.mockReturnValue(false); - mockHasCapability.mockImplementation( - (urn: string) => urn === CAPABILITIES.FILES || urn === CAPABILITIES.PRINCIPALS_OWNER, - ); - expect(supportsSharing()).toBe(true); + mockRequest.mockResolvedValue({ methodResponses: [['FileNode/set', { updated: { f1: null } }, '0']] }); + await setFileNodeShare('f1', 'p1', null); + const [, using] = mockRequest.mock.calls[1]; + expect(using).toEqual([CAPABILITIES.CORE, CAPABILITIES.FILES]); }); }); diff --git a/src/api/files.ts b/src/api/files.ts index 02876628..bd871ebc 100644 --- a/src/api/files.ts +++ b/src/api/files.ts @@ -25,14 +25,14 @@ const FILE_NODE_PROPERTIES = [ 'shareWith', 'myRights', ]; -// RFC 9670 sharing is only usable when the server advertises the exact -// `principals:owner` capability, either in the session or on the account -// (Stalwart places it in accountCapabilities). +// Sharing with other users (RFC 9670) needs the principal directory to pick +// people from, so it is offered whenever the server advertises +// `urn:ietf:params:jmap:principals`, like the webmail's supportsPrincipals(). +// It used to require `principals:owner`, which Stalwart advertises nowhere, +// so "Sharing & access" and the calendar share picker never had anyone to +// offer. export function supportsSharing(): boolean { - return ( - jmapClient.hasCapability(CAPABILITIES.PRINCIPALS_OWNER) || - jmapClient.hasAccountCapability(CAPABILITIES.PRINCIPALS_OWNER, filesAccountId()) - ); + return jmapClient.hasCapability(CAPABILITIES.PRINCIPALS); } export function filesAccountId(): string { @@ -74,9 +74,13 @@ function fileUsing(): string[] { if (jmapClient.hasCapability(CAPABILITIES.FILES)) { using.push(CAPABILITIES.FILES); } - // Required for shareWith/myRights on FileNode and for cross-account - // (shared-with-me) FileNode/get. - if (supportsSharing()) { + // Only name principals:owner where the server advertises it: an unknown + // capability in `using` fails the whole request. Stalwart returns + // shareWith/myRights without it. + if ( + jmapClient.hasCapability(CAPABILITIES.PRINCIPALS_OWNER) || + jmapClient.hasAccountCapability(CAPABILITIES.PRINCIPALS_OWNER, filesAccountId()) + ) { using.push(CAPABILITIES.PRINCIPALS_OWNER); } return using; @@ -410,23 +414,38 @@ export async function setFileNodeShare( } } +/** Safety bound on how many principals the share picker pages through. */ +const MAX_PRINCIPALS = 20_000; + // List all principals visible to the user. Stalwart returns the full -// directory regardless of `filter`, so callers filter client-side. +// directory regardless of `filter`, so callers filter client-side. Paged by +// maxObjectsInGet: an unbounded query hands every id to the chained +// Principal/get, which the server refuses with `requestTooLarge` once the +// directory holds more than that (500), leaving the picker empty. export async function getPrincipals(): Promise { if (!supportsSharing()) return []; const accountId = jmapClient.accountId; - const res = await jmapClient.request( - [ - ['Principal/query', { accountId }, '0'], - ['Principal/get', { - accountId, - '#ids': { resultOf: '0', name: 'Principal/query', path: '/ids' }, - }, '1'], - ], - [CAPABILITIES.CORE, CAPABILITIES.PRINCIPALS], - ); - const getResp = res.methodResponses.find((r) => r[0] === 'Principal/get'); - return (getResp?.[1].list ?? []) as Principal[]; + const pageSize = jmapClient.getMaxObjectsInGet(); + const all: Principal[] = []; + for (let position = 0; position < MAX_PRINCIPALS;) { + const res = await jmapClient.request( + [ + ['Principal/query', { accountId, position, limit: pageSize }, '0'], + ['Principal/get', { + accountId, + '#ids': { resultOf: '0', name: 'Principal/query', path: '/ids' }, + }, '1'], + ], + [CAPABILITIES.CORE, CAPABILITIES.PRINCIPALS], + ); + const ids = (res.methodResponses.find((r) => r[0] === 'Principal/query')?.[1].ids ?? []) as string[]; + const getResp = res.methodResponses.find((r) => r[0] === 'Principal/get'); + if (!getResp) break; + all.push(...((getResp[1].list ?? []) as Principal[])); + if (ids.length < pageSize) break; + position += ids.length; + } + return all; } // The principal id that represents the logged-in user (excluded from the