From 5ce8c89c51293ee06ec2d27190840ed5d1e30460 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 26 Sep 2026 22:28:34 +0200 Subject: [PATCH] fix: open and save files shared with you in the office editor #1094 --- app/api/wopi/files/[fileId]/contents/route.ts | 8 +- app/api/wopi/launch/route.ts | 19 +- components/files/files-app.tsx | 13 +- lib/__tests__/wopi-shared-file-routes.test.ts | 180 ++++++++++++++++++ lib/wopi/token.ts | 8 + stores/__tests__/file-store.test.ts | 15 +- stores/file-store.ts | 16 ++ 7 files changed, 242 insertions(+), 17 deletions(-) create mode 100644 lib/__tests__/wopi-shared-file-routes.test.ts diff --git a/app/api/wopi/files/[fileId]/contents/route.ts b/app/api/wopi/files/[fileId]/contents/route.ts index db8524005..15a3e511c 100644 --- a/app/api/wopi/files/[fileId]/contents/route.ts +++ b/app/api/wopi/files/[fileId]/contents/route.ts @@ -6,8 +6,8 @@ import { wopiContext } from '@/lib/wopi/request'; /** * WOPI GetFile / PutFile (#425). Content flows purely over JMAP: GetFile * streams the node's blob (or, for a mail attachment, the attachment blob - * itself - #1047), PutFile uploads the editor's bytes as a new blob and - * points the FileNode at it via `FileNode/set { blobId }`. + * itself - #1047), PutFile uploads the editor's bytes as a new blob into the + * user's own account and points the FileNode at it via `FileNode/set { blobId }`. */ /** @@ -115,7 +115,9 @@ export async function POST( return NextResponse.json({ error: 'Empty document rejected' }, { status: 409 }); } - const blob = await uploadFileBlob(auth.ctx, auth.payload.accountId, body, node.type); + const blob = await uploadFileBlob( + auth.ctx, auth.payload.uploadAccountId || auth.payload.accountId, body, node.type, + ); const { modified } = await updateFileNodeBlob( auth.ctx, auth.payload.accountId, fileId, blob.blobId, ); diff --git a/app/api/wopi/launch/route.ts b/app/api/wopi/launch/route.ts index 38db80a84..d28b5c66b 100644 --- a/app/api/wopi/launch/route.ts +++ b/app/api/wopi/launch/route.ts @@ -58,19 +58,22 @@ export async function POST(request: NextRequest) { const ctx = { serverUrl, authHeader: creds.authHeader, trusted }; let accountId = str('accountId'); + const capability = blobId ? 'urn:ietf:params:jmap:mail' : 'urn:ietf:params:jmap:filenode'; + // Files need the session even with an explicit account: PutFile uploads + // into the login's own account, which is not the one holding a node + // shared with the user (#1094). + const session = !accountId || !blobId + ? await fetchJmapSession(serverUrl, creds.authHeader, { trusted }) + : null; + const ownAccountId = session?.primaryAccounts?.[capability] || ''; if (!accountId) { - const session = await fetchJmapSession(serverUrl, creds.authHeader, { trusted }); - const capability = blobId ? 'urn:ietf:params:jmap:mail' : 'urn:ietf:params:jmap:filenode'; - accountId = - session?.primaryAccounts?.[capability] || - Object.keys(session?.accounts ?? {})[0] || - ''; + accountId = ownAccountId || Object.keys(session?.accounts ?? {})[0] || ''; } if (!accountId) { return NextResponse.json({ error: blobId ? 'No mail account' : 'No files account' }, { status: 404 }); } - let document: Pick; + let document: Pick; let name: string; let canWrite: boolean; if (blobId) { @@ -92,7 +95,7 @@ export async function POST(request: NextRequest) { } name = node.name; canWrite = node.myRights ? !!node.myRights.mayModifyContent : true; - document = { kind: 'file', fileId }; + document = { kind: 'file', fileId, uploadAccountId: ownAccountId || accountId }; } const ext = name.split('.').pop()?.toLowerCase() || ''; diff --git a/components/files/files-app.tsx b/components/files/files-app.tsx index 4bdf59040..0a15510ec 100644 --- a/components/files/files-app.tsx +++ b/components/files/files-app.tsx @@ -10,7 +10,7 @@ import { useConfirmDialog } from "@/hooks/use-confirm-dialog"; import { useAuthStore, redirectToLogin, saveRedirectAfterLogin } from '@/stores/auth-store'; import { useAccountStore } from "@/stores/account-store"; import { useEmailStore } from "@/stores/email-store"; -import { useFileStore } from "@/stores/file-store"; +import { useFileStore, resourceServerRef } from "@/stores/file-store"; import { toast } from "@/stores/toast-store"; import { cn, formatFileSize } from "@/lib/utils"; import { NavigationRail } from "@/components/layout/navigation-rail"; @@ -721,17 +721,20 @@ export function FilesApp({ linkSegments: routeSegments }: FilesAppProps = {}) { {/* WOPI document editor overlay (#425) */} {editFile && (() => { const editResource = resources.find(r => r.name === editFile); - return editResource ? ( + if (!editResource) return null; + // A file shared with the user lives in the owner's account (#1094). + const node = resourceServerRef(editResource); + return ( { setEditFile(null); // The editor may have saved new content - pick up size/mtime. refresh(); }} /> - ) : null; + ); })()} {/* Legacy file migration progress (issue #379) */} diff --git a/lib/__tests__/wopi-shared-file-routes.test.ts b/lib/__tests__/wopi-shared-file-routes.test.ts new file mode 100644 index 000000000..f7df0d92c --- /dev/null +++ b/lib/__tests__/wopi-shared-file-routes.test.ts @@ -0,0 +1,180 @@ +// @vitest-environment node +import { mkdtempSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { describe, it, expect, vi, beforeEach, type Mock } from 'vitest'; + +// #1094: a Files node shared with the user lives in the owner's account. +// The editor must address it there, and PutFile must upload the new content +// into the user's own account - Stalwart refuses a blob the user uploaded +// into the owner's account ("You do not have access to blobId"). + +vi.mock('@/lib/auth/session-secret', () => ({ + getSessionSecret: () => 'x'.repeat(32), + hasSessionSecret: () => true, +})); +vi.mock('@/lib/logger', () => ({ + logger: { warn: () => {}, error: () => {}, info: () => {}, debug: () => {} }, +})); +vi.mock('@/lib/stalwart/credentials', () => ({ getStalwartCredentials: vi.fn() })); +vi.mock('next/headers', () => ({ + cookies: async () => ({ get: () => undefined, set: () => {}, delete: () => {} }), +})); +vi.mock('@/lib/admin/config-manager', () => ({ + configManager: { get: (_key: string, fallback: unknown) => fallback, ensureLoaded: async () => {} }, +})); +vi.mock('@/lib/wopi/discovery', async (importActual) => ({ + ...(await importActual()), + getWopiActions: vi.fn(), +})); +vi.mock('@/lib/stalwart/server-fetch', () => ({ + fetchJmapServer: vi.fn(), + isTrustedJmapServerUrl: async () => true, +})); +vi.mock('@/lib/stalwart/jmap-api', () => ({ + fetchJmapSession: vi.fn(), + postJmap: vi.fn(), + rebaseApiUrl: () => null, +})); + +import { POST as launch } from '@/app/api/wopi/launch/route'; +import { GET as checkFileInfo } from '@/app/api/wopi/files/[fileId]/route'; +import { GET as getFile, POST as putFile } from '@/app/api/wopi/files/[fileId]/contents/route'; +import { getStalwartCredentials } from '@/lib/stalwart/credentials'; +import { getWopiActions } from '@/lib/wopi/discovery'; +import { fetchJmapServer } from '@/lib/stalwart/server-fetch'; +import { fetchJmapSession, postJmap } from '@/lib/stalwart/jmap-api'; + +process.env.ADMIN_STATE_DIR = mkdtempSync(path.join(tmpdir(), 'bw-wopi-shared-')); + +const ORIGIN = 'https://webmail.example.com'; +const OFFICE = 'https://office.example.com/browser/abc/cool.html?'; +const DOCX = 'application/vnd.openxmlformats-officedocument.wordprocessingml.document'; +const OWN = 'c'; +const OWNER = 'd'; + +function request(url: string, init: { body?: unknown; bytes?: ArrayBuffer } = {}) { + return { + nextUrl: new URL(url, ORIGIN), + headers: new Headers(), + json: async () => init.body, + arrayBuffer: async () => init.bytes ?? new ArrayBuffer(0), + } as unknown as Parameters[0]; +} + +const params = (id: string) => ({ params: Promise.resolve({ fileId: id }) }); + +function editorCall(data: { url: string; accessToken: string }) { + const wopiSrc = new URL(new URL(data.url).searchParams.get('WOPISrc')!); + const documentId = decodeURIComponent(wopiSrc.pathname.split('/').pop()!); + return { documentId, token: `access_token=${encodeURIComponent(data.accessToken)}` }; +} + +// The shared node as Stalwart 0.16.23 reports it to the user it is shared with. +let node: { id: string; name: string; type: string; blobId: string; size: number; modified: string; myRights: Record }; +let setCalls: Array<{ accountId: string; update: Record }>; + +beforeEach(() => { + vi.clearAllMocks(); + node = { + id: 'b', name: 'new-file.docx', type: DOCX, blobId: 'Gshared', size: 10, modified: '2026-09-26T20:00:00Z', + myRights: { mayRead: true, mayRename: true, mayModifyContent: true }, + }; + setCalls = []; + (getStalwartCredentials as Mock).mockResolvedValue({ + serverUrl: 'https://mail.example.com', + authHeader: 'Basic dXNlcjpwYXNz', + username: 'user@example.com', + trusted: true, + slot: 0, + hasSessionCookie: true, + }); + (getWopiActions as Mock).mockResolvedValue({ edit: { docx: OFFICE }, view: {} }); + (fetchJmapSession as Mock).mockResolvedValue({ + primaryAccounts: { 'urn:ietf:params:jmap:mail': OWN, 'urn:ietf:params:jmap:filenode': OWN }, + accounts: { [OWN]: { isPersonal: true }, [OWNER]: { isPersonal: false } }, + }); + (postJmap as Mock).mockImplementation(async (_url: string, _auth: string, body: string) => { + const { methodCalls } = JSON.parse(body) as { methodCalls: [string, Record, string][] }; + const methodResponses = methodCalls.map(([method, args, callId]) => { + const inOwner = args.accountId === OWNER; + if (method === 'FileNode/get') { + const ids = args.ids as string[]; + const found = inOwner && ids.includes(node.id); + return [method, { accountId: args.accountId, list: found ? [node] : [], notFound: found ? [] : ids }, callId]; + } + // FileNode/set + const update = args.update as Record; + setCalls.push({ accountId: args.accountId as string, update }); + const patch = update[node.id]; + if (!inOwner || !patch) return [method, { notUpdated: { [Object.keys(update)[0]]: { type: 'notFound' } } }, callId]; + if (!patch.blobId.startsWith(`upload-${OWN}-`)) { + return [method, { notUpdated: { [node.id]: { type: 'forbidden', description: `You do not have access to blobId ${patch.blobId}.` } } }, callId]; + } + node = { ...node, blobId: patch.blobId, modified: '2026-09-26T21:00:00Z' }; + return [method, { updated: { [node.id]: null } }, callId]; + }); + return new Response(JSON.stringify({ methodResponses }), { status: 200 }); + }); + let uploads = 0; + (fetchJmapServer as Mock).mockImplementation(async (url: string, init: { method: string }) => { + const upload = /\/jmap\/upload\/([^/]+)\//.exec(url); + if (init.method === 'POST' && upload) { + return Response.json({ accountId: upload[1], blobId: `upload-${upload[1]}-${++uploads}`, type: DOCX, size: 3 }); + } + return new Response('DOCX-BYTES', { status: 200, headers: { 'Content-Length': '10' } }); + }); +}); + +async function openShared() { + const res = await launch(request('/api/wopi/launch', { body: { fileId: 'b', accountId: OWNER } })); + expect(res.status).toBe(200); + const data = await res.json(); + return { data, ...editorCall(data) }; +} + +describe('WOPI on a file shared with the user (#1094)', () => { + it('launches editable from the owner account and the bare node id', async () => { + const { data, documentId, token } = await openShared(); + expect(data.readOnly).toBe(false); + + const info = await checkFileInfo(request(`/api/wopi/files/${documentId}?${token}`), params(documentId)); + expect(info.status).toBe(200); + expect(await info.json()).toMatchObject({ BaseFileName: 'new-file.docx', UserCanWrite: true, Version: 'Gshared' }); + }); + + it('GetFile reads the blob through the owner account', async () => { + const { documentId, token } = await openShared(); + (fetchJmapServer as Mock).mockClear(); + const res = await getFile(request(`/api/wopi/files/${documentId}/contents?${token}`), params(documentId)); + expect(res.status).toBe(200); + expect((fetchJmapServer as Mock).mock.calls[0][0]).toContain(`/jmap/download/${OWNER}/Gshared/`); + }); + + it('PutFile uploads into the own account and updates the node in the owner account', async () => { + const { documentId, token } = await openShared(); + (fetchJmapServer as Mock).mockClear(); + const res = await putFile( + request(`/api/wopi/files/${documentId}/contents?${token}`, { bytes: new TextEncoder().encode('new').buffer as ArrayBuffer }), + params(documentId), + ); + expect(res.status).toBe(200); + expect(await res.json()).toEqual({ LastModifiedTime: '2026-09-26T21:00:00Z' }); + + const [uploadUrl, uploadInit] = (fetchJmapServer as Mock).mock.calls[0]; + expect(uploadInit.method).toBe('POST'); + expect(uploadUrl).toContain(`/jmap/upload/${OWN}/`); + expect(setCalls).toEqual([{ accountId: OWNER, update: { b: { blobId: `upload-${OWN}-1` } } }]); + }); + + it('shares one editor session with the owner opening the same file', async () => { + const { documentId } = await openShared(); + // The owner's own files account is the one the node lives in. + (fetchJmapSession as Mock).mockResolvedValue({ + primaryAccounts: { 'urn:ietf:params:jmap:filenode': OWNER }, + accounts: { [OWNER]: { isPersonal: true } }, + }); + const res = await launch(request('/api/wopi/launch', { body: { fileId: 'b' } })); + expect(editorCall(await res.json()).documentId).toBe(documentId); + }); +}); diff --git a/lib/wopi/token.ts b/lib/wopi/token.ts index 915537bda..67b0593dc 100644 --- a/lib/wopi/token.ts +++ b/lib/wopi/token.ts @@ -32,6 +32,13 @@ export interface WopiTokenPayload { name?: string; type?: string; size?: number; + /** + * Account PutFile uploads new content into: the login's own. For a node + * shared with the user, `accountId` is the owner's, and Stalwart doesn't + * let the user reference a blob they uploaded there (#1094). Absent on + * older tokens - those upload into `accountId`. + */ + uploadAccountId?: string; canWrite: boolean; /** Browser origin that embeds the editor iframe (WOPI PostMessageOrigin). */ origin: string; @@ -104,6 +111,7 @@ export function verifyWopiToken(token: string | null, documentId: string): WopiT if (p.kind !== undefined && p.kind !== 'file' && p.kind !== 'attachment') return null; if (p.bid !== undefined && typeof p.bid !== 'string') return null; if (p.slot !== undefined && typeof p.slot !== 'number') return null; + if (p.uploadAccountId !== undefined && (typeof p.uploadAccountId !== 'string' || !p.uploadAccountId)) return null; if (wopiDocumentId(p) !== documentId) return null; if (Date.now() > p.exp) return null; return p; diff --git a/stores/__tests__/file-store.test.ts b/stores/__tests__/file-store.test.ts index 7bf6151e6..a2a9a4754 100644 --- a/stores/__tests__/file-store.test.ts +++ b/stores/__tests__/file-store.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, beforeEach } from 'vitest'; -import { useFileStore } from '../file-store'; +import { useFileStore, resourceServerRef } from '../file-store'; import type { IJMAPClient } from '@/lib/jmap/client-interface'; import type { FileNode } from '@/lib/jmap/types'; @@ -411,3 +411,16 @@ describe('file-store modification date (issue #700)', () => { expect(useFileStore.getState().resources[0].lastModified).toBe('2025-01-01T00:00:00Z'); }); }); + +describe('resourceServerRef (issue #1094)', () => { + it('splits a node of a shared account into its owner account and bare id', () => { + // The WOPI editor used to send "d:b" with the user's own account, and + // FileNode/get answered notFound. + expect(resourceServerRef({ id: 'd:b', ownerAccountId: 'd' })).toEqual({ accountId: 'd', id: 'b' }); + }); + + it('keeps own nodes as they are', () => { + expect(resourceServerRef({ id: 'b', ownerAccountId: 'c' })).toEqual({ accountId: 'c', id: 'b' }); + expect(resourceServerRef({ id: 'b' })).toEqual({ accountId: undefined, id: 'b' }); + }); +}); diff --git a/stores/file-store.ts b/stores/file-store.ts index 63e205f7a..a9968e50b 100644 --- a/stores/file-store.ts +++ b/stores/file-store.ts @@ -166,6 +166,22 @@ function isCrossAccountId(id: string | null): boolean { return id != null && id.includes(':'); } +/** + * The account and bare server id behind a listed resource, for callers that + * address the node on the server themselves (the WOPI editor, #1094). A node + * of a shared account is listed as "accountId:nodeId"; `accountId` is only + * known for nodes listed across accounts. + */ +export function resourceServerRef( + resource: Pick, +): { accountId?: string; id: string } { + const owner = resource.ownerAccountId; + if (owner && resource.id.startsWith(`${owner}:`)) { + return { accountId: owner, id: resource.id.slice(owner.length + 1) }; + } + return { accountId: owner, id: resource.id }; +} + function nodeToResource(node: FileNode): FileResource { const isDir = isFolder(node); return {