diff --git a/CHANGELOG.md b/CHANGELOG.md index 50f3428fe..3d8c704ac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,29 @@ and this project adheres to ### Added +- ✨(frontend) build the version history from the collaboration server. The + history panel now lists the document's own editing activity instead of the + snapshots the backend used to write to S3 — a list that has not gained an + entry since the document was migrated, because nothing writes those snapshots + any more. A version is a minute of editing: changes less than a minute apart + become one, no version spans more than a minute, and changes are merged + whoever made them, because a version is a moment in the document rather than a + moment in one person's editing. Selecting one previews the document exactly as + it stood then. Note that a document whose history has not yet been replayed + into the collaboration server (`manage.py migrate_documents`) lists only the + edits made since it moved there + +- ✨(frontend) make restoring a version work again. It has done nothing since + the migration, while still promising that the document would be replaced. The + collaboration server now performs the restore where the document lives, so + everyone with it open sees it arrive over their own connection rather than one + tab rewriting the document under the others. Nothing is destroyed — the + restore is itself a change, so the state it replaced stays in the history and + can be restored again. **Any user who may edit a document may restore it**; + readers may not, and nobody may undo work that predates their own access — + where reads are silently trimmed to what a user may see, a restore reaching + further back is refused + - ✨(collaboration) let a user read the document's editing history from the moment they were given access to it. The collaboration server's `activity` and `changeset` routes are opened to the browser, bounded per user to the earliest diff --git a/documentation/collaboration.md b/documentation/collaboration.md index 35e24de71..0e40a2bed 100644 --- a/documentation/collaboration.md +++ b/documentation/collaboration.md @@ -22,7 +22,7 @@ The Django backend reads and writes document content there too, so point it at t YHUB_API_BASE_URL: http://{yhub-service}:443 ``` -Prefer the internal service url: the routes the backend calls are not meant to be reachable from the outside. Route `/collaboration/ws/` to the service publicly — that is the one the browsers open — plus `/collaboration/ydoc/` for the http fallback, `/collaboration/activity/` and `/collaboration/changeset/` for the editing history, and `/collaboration/jwks/`, which carries public keys and nothing else. Keep everything else in-cluster: `rollback`, `prune`, `reset-connections`, `migrate`, `restore-ydoc`, `reset-ydoc` and `create-ydoc` are refused to a browser by the permission tables anyway, and an endpoint that cannot be reached cannot be probed. +Prefer the internal service url: the routes the backend calls are not meant to be reachable from the outside. Route `/collaboration/ws/` to the service publicly — that is the one the browsers open — plus `/collaboration/ydoc/` for the http fallback, `/collaboration/activity/` and `/collaboration/changeset/` for the editing history, `/collaboration/rollback/` for restoring a document to a point in that history, and `/collaboration/jwks/`, which carries public keys and nothing else. Keep everything else in-cluster: `prune`, `reset-connections`, `migrate`, `restore-ydoc`, `reset-ydoc` and `create-ydoc` are refused to a browser by the permission tables anyway, and an endpoint that cannot be reached cannot be probed. Both directions are authenticated with short-lived RS256 JWTs rather than a shared secret, and each side verifies the other against the JWKS it publishes — so both need a signing key of their own, and neither needs a copy of the other's: @@ -131,3 +131,29 @@ A reader who reaches a document through its link alone — a public or authentic they hold no access on — gets **no history at all**, not a bounded one. There is no access record and therefore no date to bound it with, which is the same reason the version endpoints have always refused them. + +## What a version is + +The version history lists the document's editing activity at a granularity of **one minute**: +changes less than a minute apart become one version, and no version spans more than a minute. That +bound is deliberate — the collaboration server records activity at the granularity of a keystroke, +and a list of every few keystrokes is not a history anyone can read. + +Changes are merged **regardless of who made them**. A version is a moment in the document, not a +moment in one person's editing, so two people typing in the same minute produce one version and not +two interleaved ones. The collaboration server only ever groups changes by the same author, so this +last step happens in the browser, on top of its grouping. + +## Restoring a previous state + +Selecting a version and restoring it asks the collaboration server to undo everything that happened +after it. **Any user who may edit a document may restore it**; a reader may not. + +The restore is applied where the document lives, not in the tab that asked for it, so everyone with +the document open sees it arrive over their own connection like any other change. Nothing is +destroyed: the restore is itself a change, so the state it replaced stays in the history and can be +restored again. + +It is bounded by the same date as everything else, and more strictly. Reads are trimmed silently to +what a user may see; a restore is *refused* if it reaches further back than that — so nobody can +undo work that predates their access, even by asking for it directly. diff --git a/src/backend/core/api/viewsets.py b/src/backend/core/api/viewsets.py index b00a4510b..a1314c1c6 100644 --- a/src/backend/core/api/viewsets.py +++ b/src/backend/core/api/viewsets.py @@ -1799,6 +1799,31 @@ class DocumentViewSet( """ Return the document's versions but only those created after the user got access to the document + + DEPRECATED — nothing calls this any more, and it can be removed once the + migration to the collaboration server is finished. + + The collaboration server is the source of truth for document history and + keeps it itself; the version history in the frontend is built from its + `activity` and `changeset` routes, bounded by the same date this method + applies (`user_access_since`, which the collaboration server is handed to + bound what it serves). What this endpoint lists is S3 object versions of + the legacy `{pk}/file` key, and nothing writes that key any more — the + content endpoint that used to went away with the migration — so the list + is frozen at each document's migration date and gains no further entries. + + Removing it is safe once every document has had its real history replayed + into the collaboration server by `manage.py migrate_documents`; until + then these versions are the only record of what a soft-migrated document + looked like before it moved. `versions_detail` and + `Document.get_versions_slice` are kept for the same reason and go at the + same time. + + The `versions_list` ability still gates the history menu item in the + frontend, and correctly: it is `has_access_role`, which is exactly the + condition under which `user_access_since` is not None and the + collaboration server grants a history — so the gate and the grant cannot + disagree. """ user = request.user if not user.is_authenticated: diff --git a/src/frontend/apps/e2e/__tests__/app-impress/doc-version.spec.ts b/src/frontend/apps/e2e/__tests__/app-impress/doc-version.spec.ts index 4812d6d6d..db266cc17 100644 --- a/src/frontend/apps/e2e/__tests__/app-impress/doc-version.spec.ts +++ b/src/frontend/apps/e2e/__tests__/app-impress/doc-version.spec.ts @@ -4,16 +4,48 @@ import { createDoc, goToGridDoc, mockedDocument, - saveContent, + reopenDoc, verifyDocName, } from './utils-common'; import { openSuggestionMenu, writeInEditor } from './utils-editor'; +/** + * A version is at least a minute of editing: the collaboration server groups + * changes that are less than a minute apart, and the panel merges what is left + * across authors. That bound is the feature — a history of every few keystrokes + * is not a history — but it does mean a test cannot produce two versions + * without a minute of real time passing. The timestamps come from the server, + * so no clock can be faked to shorten it. + * + * The tests below therefore assert one version wherever one is enough, and only + * the restore test pays for a second one. + */ +const VERSION_GRANULARITY_MS = 60_000; + test.beforeEach(async ({ page }) => { await page.goto('/'); }); test.describe('Doc Version', () => { + test('it displays an empty history', async ({ page, browserName }) => { + // Stubbed, because a document with no history at all cannot be reached + // through the interface: opening one writes to it, and that write is a + // change like any other. Only a document nobody has ever opened has an + // empty timeline. + await page.route('**/collaboration/activity/**', (route) => + route.fulfill({ json: { activity: [] } }), + ); + + await createDoc(page, 'doc-version-empty', browserName, 1); + + await page.getByLabel('Open the document options').click(); + await page.getByRole('menuitem', { name: 'Version history' }).click(); + + const modal = page.getByRole('dialog', { name: 'Version history' }); + await expect(modal.getByLabel('Version list')).toBeVisible(); + await expect(modal.getByText('No versions')).toBeVisible(); + }); + test('it displays the doc versions', async ({ page, browserName }) => { const [randomDoc] = await createDoc(page, 'doc-version', browserName, 1); @@ -24,20 +56,9 @@ test.describe('Doc Version', () => { const modal = page.getByRole('dialog', { name: 'Version history' }); const panel = modal.getByLabel('Version list'); - await expect(panel).toBeVisible(); - await expect(modal.getByText('No versions')).toBeVisible(); - - await modal.getByRole('button', { name: 'close' }).click(); await writeInEditor({ page, text: 'Hello World' }); - await saveContent(page, randomDoc); - - await expect(page.getByText('Hello World')).toBeVisible(); - - // Write more - await writeInEditor({ page, text: 'It will create a version' }); - const { suggestionMenu } = await openSuggestionMenu({ page }); await suggestionMenu.getByText('Add a callout block').click(); @@ -47,21 +68,9 @@ test.describe('Doc Version', () => { await expect(calloutBlock).toBeVisible(); - await saveContent(page, randomDoc); + await reopenDoc(page, randomDoc); - await expect(page.getByText('Hello World')).toBeHidden(); - await expect(page.getByText('It will create a version')).toBeVisible(); - - await expect(calloutBlock).toBeVisible(); - - // Write more - await writeInEditor({ page, text: 'It will create a second version' }); - - await saveContent(page, randomDoc); - - await expect( - page.getByText('It will create a second version'), - ).toBeVisible(); + await expect(page.getByText('Hello World')).toBeVisible(); await page.getByLabel('Open the document options').click(); await page.getByRole('menuitem', { name: 'History' }).click(); @@ -69,34 +78,19 @@ test.describe('Doc Version', () => { await expect(panel).toBeVisible(); await expect(page.getByText('History', { exact: true })).toBeVisible(); await expect(page.getByRole('status')).toBeHidden(); - const items = panel.locator('.version-item'); - await expect(items).toHaveCount(2); - await items.nth(1).click(); - await expect(modal.getByText('Hello World')).toBeVisible(); - await expect(modal.getByText('It will create a version')).toBeHidden(); - await expect( - modal.locator('div[data-content-type="callout"]').first(), - ).toBeHidden(); + // One entry: opening the document, naming it and typing into it all + // happened inside one minute, and a version is a minute of editing. + const items = panel.locator('.version-item'); + await expect(items).toHaveCount(1); await items.nth(0).click(); + // the preview renders the document as it stood at the end of that version await expect(modal.getByText('Hello World')).toBeVisible(); - await expect(modal.getByText('It will create a version')).toBeVisible(); await expect( modal.locator('div[data-content-type="callout"]').first(), ).toBeVisible(); - await expect( - modal.getByText('It will create a second version'), - ).toBeHidden(); - - await items.nth(1).click(); - - await expect(modal.getByText('Hello World')).toBeVisible(); - await expect(modal.getByText('It will create a version')).toBeHidden(); - await expect( - modal.locator('div[data-content-type="callout"]').first(), - ).toBeHidden(); }); test('it does not display the doc versions if not allowed', async ({ @@ -118,6 +112,11 @@ test.describe('Doc Version', () => { }); test('it restores the doc version', async ({ page, browserName }) => { + // The wait below is the whole reason for this budget, and it is not + // padding: two versions cannot exist any closer together. See the note at + // the top of the file before trying to make this faster. + test.setTimeout(VERSION_GRANULARITY_MS + 120_000); + const [randomDoc] = await createDoc(page, 'doc-version', browserName, 1); await verifyDocName(page, randomDoc); @@ -132,32 +131,35 @@ test.describe('Doc Version', () => { await thread.locator('[data-test="save"]').click(); await expect(thread).toBeHidden(); - await saveContent(page, randomDoc); - + await reopenDoc(page, randomDoc); await expect(editor.getByText('Hello')).toBeVisible(); + + // Let the first version close before writing the text that has to end up + // in a second one — a minute of silence is what separates them. + await page.waitForTimeout(VERSION_GRANULARITY_MS + 1_000); + await page.locator('.bn-block-outer').last().click(); await page.keyboard.press('Enter'); await page.locator('.bn-block-outer').last().fill('World'); - await saveContent(page, randomDoc); + await reopenDoc(page, randomDoc); await expect(page.getByText('World')).toBeVisible(); - await editor.getByText('Hello').click(); - await thread.getByText('This is a comment').first().hover(); - await thread.locator('[data-test="resolve"]').click(); - await expect(thread).toBeHidden(); - await page.getByLabel('Open the document options').click(); await page.getByRole('menuitem', { name: 'History' }).click(); const modal = page.getByRole('dialog', { name: 'Version history' }); const panel = modal.getByLabel('Version list'); await expect(panel).toBeVisible(); - await expect(page.getByText('History', { exact: true })).toBeVisible(); - await panel.locator('.version-item').first().click(); + // newest first: the second item is the version that predates 'World' + const items = panel.locator('.version-item'); + await expect(items).toHaveCount(2); + await items.nth(1).click(); + + await expect(modal.getByText('Hello')).toBeVisible(); await expect(modal.getByText('World')).toBeHidden(); await page.getByRole('button', { name: 'Restore', exact: true }).click(); @@ -169,23 +171,21 @@ test.describe('Doc Version', () => { await page.getByLabel('Restore', { exact: true }).click(); + // The collaboration server applies the rollback and pushes it back over the + // connection this editor is already holding — nothing is reloaded here. const mainEditor = page.getByLabel('Document editor'); await expect(mainEditor.getByText('Hello')).toBeVisible(); await expect(mainEditor.getByText('World')).toBeHidden(); - // The old comment is not restored - await expect(mainEditor.getByText('Hello')).toHaveCSS( - 'background-color', - 'rgba(0, 0, 0, 0)', - ); - - // We can add a new comment - await mainEditor.getByText('Hello').selectText(); - await page.getByRole('button', { name: 'Add comment' }).click(); - - await thread.getByRole('paragraph').first().fill('This is a comment'); - await thread.locator('[data-test="save"]').click(); + // The comment survives, and that is the point: a restore undoes the changes + // made after the chosen version rather than replacing the document with an + // old copy of it. This comment belongs to the version being restored, so + // nothing about it was undone. await expect(mainEditor.getByText('Hello')).toHaveClass('bn-thread-mark'); + + // and the document is still live afterwards + await mainEditor.getByText('Hello').click(); + await expect(thread.getByText('This is a comment').first()).toBeVisible(); }); }); diff --git a/src/frontend/apps/e2e/__tests__/app-impress/presenter-mode.spec.ts b/src/frontend/apps/e2e/__tests__/app-impress/presenter-mode.spec.ts index e7e21f52b..f8bd12787 100644 --- a/src/frontend/apps/e2e/__tests__/app-impress/presenter-mode.spec.ts +++ b/src/frontend/apps/e2e/__tests__/app-impress/presenter-mode.spec.ts @@ -6,7 +6,7 @@ import { createDoc, goToGridDoc, mockedDocument, - saveContent, + reopenDoc, } from './utils-common'; import { openSuggestionMenu, @@ -416,9 +416,10 @@ test.describe('Presenter Mode', () => { await writeMultiSlideDoc(page); const docId = getDocIdFromUrl(page); - // Ensure the typed content is persisted (awaits the PATCH /content/) before - // reloading the page through the deep-link, instead of using a fixed sleep. - await saveContent(page, docTitle); + // Leave and come back before reloading through the deep-link, so what the + // deep-link opens is what the collaboration server kept, not what this tab + // still had in memory. + await reopenDoc(page, docTitle); await page.goto(`/docs/${docId}/?view=present&slide=3`); const overlay = page.getByRole('dialog', { name: 'Presenter mode' }); diff --git a/src/frontend/apps/e2e/__tests__/app-impress/utils-common.ts b/src/frontend/apps/e2e/__tests__/app-impress/utils-common.ts index 59ba264cb..cfa3e9b61 100644 --- a/src/frontend/apps/e2e/__tests__/app-impress/utils-common.ts +++ b/src/frontend/apps/e2e/__tests__/app-impress/utils-common.ts @@ -279,27 +279,21 @@ export const waitForResponseCreateDoc = (page: Page) => { }; /** - * Navigates back to the homepage, waits for the PATCH /content/ request - * triggered by the route change to complete, then navigates back to the doc. + * Leaves the doc and comes back to it, so that what follows reads the document + * from the collaboration server rather than from the editor that just wrote it. * - * Use this instead of goToGridDoc when the test must assert on content that - * was just written in the editor, to avoid a race condition where the GET - * request fired on doc mount returns stale data because the server has not - * yet processed the PATCH. + * There is nothing to save: the collaboration server receives every change as + * it is typed, and the backend holds no copy of the content to be pushed to. + * (This used to wait for a `PATCH /content/`, which stopped existing with the + * migration — and so waited forever.) What makes the round trip meaningful is + * therefore the assertion that follows it: content still on the page after + * leaving and returning is content the server kept. */ -export const saveContent = async (page: Page, title: string) => { - const savePromise = page.waitForResponse( - (response) => - response.url().includes('/content/') && - response.request().method() === 'PATCH', - ); - +export const reopenDoc = async (page: Page, title: string) => { await page.getByRole('button', { name: 'Back to homepage' }).click(); await expect(page.getByTestId('docs-grid')).toBeVisible(); await expect(page.getByTestId('grid-loader')).toBeHidden(); - await savePromise; - await goToGridDoc(page, { title }); }; diff --git a/src/frontend/apps/impress/src/api/fetchCollaborationApi.ts b/src/frontend/apps/impress/src/api/fetchCollaborationApi.ts new file mode 100644 index 000000000..2e84b4312 --- /dev/null +++ b/src/frontend/apps/impress/src/api/fetchCollaborationApi.ts @@ -0,0 +1,103 @@ +import { APIError } from './APIError'; + +/** + * Where the collaboration server lives and which org its rooms are under, as + * `collaborationHttpTarget` derives it from the websocket url. + */ +export interface CollaborationTarget { + serverUrl: string; + org: string; +} + +export type CollaborationQuery = Record< + string, + string | number | boolean | undefined +>; + +interface FetchCollaborationInit extends Omit { + query?: CollaborationQuery; + body?: unknown; +} + +/** + * The collaboration server's error bodies are `{ error, code, ... }`, which is + * not the shape `errorCauses` reads (it flattens DRF's `{ field: [...] }` and + * would hand back the letters of a string). `code` is the part worth keeping: + * `doc-deleted` is the only way to tell a deleted document from one that was + * never written, since a docid nobody has ever saved answers 200 with an empty + * document. + */ +interface CollaborationErrorBody { + error?: string; + code?: string; +} + +const collaborationError = async (response: Response, message: string) => { + let body: CollaborationErrorBody = {}; + + try { + body = (await response.json()) as CollaborationErrorBody; + } catch { + // an error with no json body — a proxy's 502 page, an aborted response + } + + return new APIError(message, { + status: response.status, + cause: [body.error ?? response.statusText], + data: { code: body.code }, + }); +}; + +/** + * Call one of the collaboration server's REST endpoints for a document. + * + * Deliberately not `fetchAPI`: that one prefixes the Django api url and attaches + * the CSRF token, neither of which applies here. What this shares with it is the + * credential — the session cookie, exactly as on the websocket upgrade and on + * the http fallback's polling (see `useProviderStore`). + * + * `Accept: application/json` is not optional. The collaboration server + * negotiates on that header alone and otherwise answers `application/x-lib0any`, + * which would need a decoder; in the json path it base64-encodes binary fields + * such as `ydoc`. + * + * The url is `{serverUrl}/{endpoint}/v1/{org}/{docId}`, the same shape the http + * fallback polls for `ydoc`. + */ +export const fetchCollaborationAPI = async ( + target: CollaborationTarget, + endpoint: string, + docId: string, + { query, body, headers, ...init }: FetchCollaborationInit = {}, +): Promise => { + const params = new URLSearchParams(); + Object.entries(query ?? {}).forEach(([key, value]) => { + if (value !== undefined) { + params.set(key, String(value)); + } + }); + const search = params.toString(); + + const response = await fetch( + `${target.serverUrl}/${endpoint}/v1/${target.org}/${docId}${search ? `?${search}` : ''}`, + { + ...init, + credentials: 'include', + headers: { + Accept: 'application/json', + ...(body !== undefined && { 'Content-Type': 'application/json' }), + ...headers, + }, + ...(body !== undefined && { body: JSON.stringify(body) }), + }, + ); + + if (!response.ok) { + throw await collaborationError( + response, + `Failed to reach the collaboration server (${endpoint})`, + ); + } + + return response.json() as Promise; +}; diff --git a/src/frontend/apps/impress/src/api/index.ts b/src/frontend/apps/impress/src/api/index.ts index 1d742adb8..c8d43a5f9 100644 --- a/src/frontend/apps/impress/src/api/index.ts +++ b/src/frontend/apps/impress/src/api/index.ts @@ -1,6 +1,7 @@ export * from './APIError'; export * from './config'; export * from './fetchApi'; +export * from './fetchCollaborationApi'; export * from './helpers'; export * from './types'; export * from './utils'; diff --git a/src/frontend/apps/impress/src/core/config/hooks/useCollaborationUrl.tsx b/src/frontend/apps/impress/src/core/config/hooks/useCollaborationUrl.tsx index a2d7f503b..5922cefe5 100644 --- a/src/frontend/apps/impress/src/core/config/hooks/useCollaborationUrl.tsx +++ b/src/frontend/apps/impress/src/core/config/hooks/useCollaborationUrl.tsx @@ -1,13 +1,14 @@ +import { CollaborationTarget } from '@/api'; + import { useConfig } from '../api'; -export const useCollaborationUrl = (room?: string) => { +/** + * Where the collaboration server's rooms live, independent of which document is + * being opened. Kept apart so the two hooks below cannot answer differently. + */ +const useCollaborationBaseUrl = () => { const { data: conf } = useConfig(); - if (!room) { - return; - } - - // The room is appended to the base URL by the provider (y-websocket) return ( conf?.COLLABORATION_WS_URL || (typeof window !== 'undefined' @@ -17,6 +18,17 @@ export const useCollaborationUrl = (room?: string) => { ); }; +export const useCollaborationUrl = (room?: string) => { + const baseUrl = useCollaborationBaseUrl(); + + if (!room) { + return; + } + + // The room is appended to the base URL by the provider (y-websocket) + return baseUrl; +}; + /** * y/hub serves the same rooms over two transports, mounted side by side under one prefix: * `{prefix}/ws/v1/{org}/{docid}` for the websocket and `{prefix}/ydoc/v1/{org}/{docid}` over @@ -42,3 +54,19 @@ export const collaborationHttpTarget = (wsUrl: string) => { org, }; }; + +/** + * The collaboration server's http address, for the routes that are plain REST + * rather than a transport: the editing history (`activity`, `changeset`) and + * the restore it feeds (`rollback`). + * + * `undefined` while the configuration is still loading, and for an instance + * whose collaboration url is not shaped like a room url — the same answer, and + * the same reason, as the http fallback's: no address is better than one nobody + * serves. + */ +export const useCollaborationTarget = (): CollaborationTarget | undefined => { + const baseUrl = useCollaborationBaseUrl(); + + return baseUrl ? collaborationHttpTarget(baseUrl) : undefined; +}; diff --git a/src/frontend/apps/impress/src/features/docs/doc-versioning/__tests__/utils.test.ts b/src/frontend/apps/impress/src/features/docs/doc-versioning/__tests__/utils.test.ts new file mode 100644 index 000000000..99402b5a9 --- /dev/null +++ b/src/frontend/apps/impress/src/features/docs/doc-versioning/__tests__/utils.test.ts @@ -0,0 +1,94 @@ +import { describe, expect, it } from 'vitest'; + +import { ActivityEntry } from '../types'; +import { VERSION_GRANULARITY_MS, mergeActivityEntries } from '../utils'; + +/** + * `mergeActivityEntries` is the only part of the history policy that lives in + * the client, and it decides what a "version" is. The cases below are the ones + * it can plausibly get wrong: the two bounds, each of which is a strict + * comparison, and the author test that this deliberately does *not* apply. + */ +const entry = (from: number, to: number, by: string | null): ActivityEntry => ({ + from, + to, + by, +}); + +const G = VERSION_GRANULARITY_MS; + +describe('mergeActivityEntries', () => { + it('has nothing to say about an empty timeline', () => { + expect(mergeActivityEntries([])).toEqual([]); + }); + + it('merges changes closer together than the granularity', () => { + const versions = mergeActivityEntries([ + entry(0, 0, 'alice'), + entry(1_000, 1_000, 'alice'), + ]); + + expect(versions).toEqual([ + { id: '1000', from: 0, to: 1_000, by: ['alice'] }, + ]); + }); + + it('merges across authors, which the server will not do', () => { + // the reason this function exists: the collaboration server breaks a run + // wherever the author changes, and a version is a moment in the document + // rather than a moment in one person's editing + const versions = mergeActivityEntries([ + entry(0, 0, 'alice'), + entry(1_000, 1_000, 'bob'), + entry(2_000, 2_000, 'alice'), + ]); + + expect(versions).toHaveLength(1); + expect(versions[0]).toMatchObject({ from: 0, to: 2_000 }); + expect(versions[0].by.sort()).toEqual(['alice', 'bob']); + }); + + it('starts a new version after a gap of exactly the granularity', () => { + // `<`, not `<=` — the same comparison the server makes, so the two halves + // of the grouping cannot disagree at the boundary + expect( + mergeActivityEntries([entry(0, 0, 'a'), entry(G, G, 'a')]), + ).toHaveLength(2); + expect( + mergeActivityEntries([entry(0, 0, 'a'), entry(G - 1, G - 1, 'a')]), + ).toHaveLength(1); + }); + + it('never lets one version span more than the granularity', () => { + // unbroken typing: every change is a millisecond after the last, so no gap + // ever ends a version and only the span bound can + const timeline = Array.from({ length: 5 }, (_, i) => + entry(i * (G / 2), i * (G / 2), 'alice'), + ); + + const versions = mergeActivityEntries(timeline); + + expect(versions.length).toBeGreaterThan(1); + versions.forEach((version) => { + expect(version.to - version.from).toBeLessThan(G); + }); + }); + + it('identifies a version by the moment it ends', () => { + // that id is what the preview and the restore are asked for, so it has to + // survive merging rather than being the first entry's timestamp + const [version] = mergeActivityEntries([ + entry(0, 0, 'alice'), + entry(500, 900, 'alice'), + ]); + + expect(version.id).toBe('900'); + expect(version.to).toBe(900); + }); + + it('keeps an unattributed change without inventing an author', () => { + const [version] = mergeActivityEntries([entry(0, 0, null)]); + + expect(version.by).toEqual([]); + }); +}); diff --git a/src/frontend/apps/impress/src/features/docs/doc-versioning/api/index.ts b/src/frontend/apps/impress/src/features/docs/doc-versioning/api/index.ts index d300be2c2..43b8f9990 100644 --- a/src/frontend/apps/impress/src/features/docs/doc-versioning/api/index.ts +++ b/src/frontend/apps/impress/src/features/docs/doc-versioning/api/index.ts @@ -1,2 +1,3 @@ -export * from './useDocVersions'; +export * from './useDocActivity'; export * from './useDocVersion'; +export * from './useRestoreDocVersion'; diff --git a/src/frontend/apps/impress/src/features/docs/doc-versioning/api/useDocActivity.tsx b/src/frontend/apps/impress/src/features/docs/doc-versioning/api/useDocActivity.tsx new file mode 100644 index 000000000..d784171fc --- /dev/null +++ b/src/frontend/apps/impress/src/features/docs/doc-versioning/api/useDocActivity.tsx @@ -0,0 +1,80 @@ +import { useQuery } from '@tanstack/react-query'; + +import { + APIError, + CollaborationTarget, + UseQueryOptionsAPI, + fetchCollaborationAPI, +} from '@/api'; +import { useCollaborationTarget } from '@/core/config/hooks/useCollaborationUrl'; + +import { APIActivity, DocVersion } from '../types'; +import { VERSION_GRANULARITY_MS, mergeActivityEntries } from '../utils'; + +export type DocActivityParam = { + docId: string; +}; + +/** + * The whole timeline in one request, deliberately. + * + * `activity` takes a `limit` but no cursor, and the server groups the entire + * filtered history before applying it — so paging costs a full regrouping per + * page and saves only the response body, which is three numbers per version. + * Responses are cached briefly and keyed on every parameter, so asking the same + * question every time is worth more than asking a smaller one. + * + * What comes back is already bounded to what this user may see: the history + * starts at the moment they were given access to the document, and the server + * applies that silently rather than refusing the request. + */ +const getDocActivity = async ( + target: CollaborationTarget, + { docId }: DocActivityParam, +): Promise => { + const { activity } = await fetchCollaborationAPI( + target, + 'activity', + docId, + { + query: { + group: true, + groupMaxGap: VERSION_GRANULARITY_MS, + groupMaxDuration: VERSION_GRANULARITY_MS, + }, + }, + ); + + // ascending from the server, newest first for the panel + return mergeActivityEntries(activity).reverse(); +}; + +export const KEY_DOC_ACTIVITY = 'doc-activity'; + +export function useDocActivity( + params: DocActivityParam, + queryConfig?: Omit, 'queryKey' | 'queryFn'>, +) { + const target = useCollaborationTarget(); + + return useQuery({ + // `target` belongs in the key: it arrives with the configuration, so a + // query started before it resolved must not be reused after + queryKey: [KEY_DOC_ACTIVITY, params, target], + queryFn: () => getDocActivity(target as CollaborationTarget, params), + enabled: !!target, + /** + * Against the application's three-minute default, which is wrong for this + * one: the timeline grows while the panel is closed, and opening it is a + * deliberate request to see the history *now*. Left at the default, a user + * who edits and reopens the panel is shown the list as it was when they + * last looked at it. + * + * There is little to save by holding it: the collaboration server caches + * its own answer for a few seconds, so a reopen inside that window costs a + * round trip and no computation. + */ + staleTime: 0, + ...queryConfig, + }); +} diff --git a/src/frontend/apps/impress/src/features/docs/doc-versioning/api/useDocVersion.tsx b/src/frontend/apps/impress/src/features/docs/doc-versioning/api/useDocVersion.tsx index 35c148ac0..f9af5f66c 100644 --- a/src/frontend/apps/impress/src/features/docs/doc-versioning/api/useDocVersion.tsx +++ b/src/frontend/apps/impress/src/features/docs/doc-versioning/api/useDocVersion.tsx @@ -1,39 +1,53 @@ import { useQuery } from '@tanstack/react-query'; -import { APIError, UseQueryOptionsAPI, errorCauses, fetchAPI } from '@/api'; +import { + APIError, + CollaborationTarget, + UseQueryOptionsAPI, + fetchCollaborationAPI, +} from '@/api'; +import { useCollaborationTarget } from '@/core/config/hooks/useCollaborationUrl'; -import { Version } from '../types'; +import { APIChangeset, DocVersion } from '../types'; export type DocVersionParam = { docId: string; - versionId: string; + versionId: DocVersion['id']; }; -const getDocVersion = async ({ - versionId, - docId, -}: DocVersionParam): Promise => { - const response = await fetchAPI(`documents/${docId}/versions/${versionId}/`); - - if (!response.ok) { - throw new APIError( - 'Failed to get the doc version', - await errorCauses(response), - ); - } - - return response.json() as Promise; -}; +/** + * The document as it stood at the end of a version. + * + * `changeset` renders it from a time-zero baseline, so this is the whole + * document at that moment and not a diff — which is what the preview needs, and + * what makes it readable with the same base64 decoding as any other snapshot. + * + * `activity?ydoc=true` would answer with one document at the newest entry plus a + * projection per entry, which is the right shape for annotating a timeline and + * the wrong one for showing a single point in it. + */ +const getDocVersion = async ( + target: CollaborationTarget, + { docId, versionId }: DocVersionParam, +): Promise => + fetchCollaborationAPI(target, 'changeset', docId, { + query: { to: versionId, ydoc: true }, + }); export const KEY_DOC_VERSION = 'doc-version'; export function useDocVersion( params: DocVersionParam, - queryConfig?: UseQueryOptionsAPI, + queryConfig?: Omit, 'queryKey' | 'queryFn'>, ) { - return useQuery({ - queryKey: [KEY_DOC_VERSION, params], - queryFn: () => getDocVersion(params), + const target = useCollaborationTarget(); + + return useQuery({ + // `target` belongs in the key: it arrives with the configuration, so a + // query started before it resolved must not be reused after + queryKey: [KEY_DOC_VERSION, params, target], + queryFn: () => getDocVersion(target as CollaborationTarget, params), + enabled: !!target, ...queryConfig, }); } diff --git a/src/frontend/apps/impress/src/features/docs/doc-versioning/api/useDocVersions.tsx b/src/frontend/apps/impress/src/features/docs/doc-versioning/api/useDocVersions.tsx deleted file mode 100644 index 80cb20490..000000000 --- a/src/frontend/apps/impress/src/features/docs/doc-versioning/api/useDocVersions.tsx +++ /dev/null @@ -1,82 +0,0 @@ -import { - InfiniteData, - QueryKey, - useInfiniteQuery, - useQuery, -} from '@tanstack/react-query'; - -import { - APIError, - DefinedInitialDataInfiniteOptionsAPI, - UseQueryOptionsAPI, - errorCauses, - fetchAPI, -} from '@/api'; - -import { APIListVersions } from '../types'; - -export type DocVersionsParam = { - docId: string; -}; - -export type DocVersionsAPIParams = DocVersionsParam & { - versionId: string; -}; - -type VersionsResponse = APIListVersions; - -const getDocVersions = async ({ - versionId, - docId, -}: DocVersionsAPIParams): Promise => { - const response = await fetchAPI( - `documents/${docId}/versions/?version_id=${versionId}`, - ); - - if (!response.ok) { - throw new APIError( - 'Failed to get the doc versions', - await errorCauses(response), - ); - } - - return response.json() as Promise; -}; - -export const KEY_LIST_DOC_VERSIONS = 'doc-versions'; - -export function useDocVersions( - params: DocVersionsAPIParams, - queryConfig?: UseQueryOptionsAPI, -) { - return useQuery({ - queryKey: [KEY_LIST_DOC_VERSIONS, params], - queryFn: () => getDocVersions(params), - ...queryConfig, - }); -} - -export function useDocVersionsInfiniteQuery( - param: DocVersionsParam, - queryConfig?: DefinedInitialDataInfiniteOptionsAPI, -) { - return useInfiniteQuery< - VersionsResponse, - APIError, - InfiniteData, - QueryKey, - string - >({ - initialPageParam: '', - queryKey: [KEY_LIST_DOC_VERSIONS, param], - queryFn: ({ pageParam }) => - getDocVersions({ - ...param, - versionId: pageParam, - }), - getNextPageParam(lastPage) { - return lastPage.next_version_id_marker || undefined; - }, - ...queryConfig, - }); -} diff --git a/src/frontend/apps/impress/src/features/docs/doc-versioning/api/useRestoreDocVersion.tsx b/src/frontend/apps/impress/src/features/docs/doc-versioning/api/useRestoreDocVersion.tsx new file mode 100644 index 000000000..6d2f07d22 --- /dev/null +++ b/src/frontend/apps/impress/src/features/docs/doc-versioning/api/useRestoreDocVersion.tsx @@ -0,0 +1,58 @@ +import { useMutation, useQueryClient } from '@tanstack/react-query'; + +import { APIError, CollaborationTarget, fetchCollaborationAPI } from '@/api'; +import { useCollaborationTarget } from '@/core/config/hooks/useCollaborationUrl'; + +import { DocVersion } from '../types'; + +import { KEY_DOC_ACTIVITY } from './useDocActivity'; + +export type RestoreDocVersionParam = { + docId: string; + versionId: DocVersion['id']; +}; + +/** + * Put the document back as it was at the end of a version, by asking the + * collaboration server to undo everything that happened after it. + * + * The rollback is applied where the document lives rather than in this tab: the + * server appends the undoing change to the room, so every open editor receives + * it over its connection the way it receives any other change. A client-side + * undo would have had to be applied here and pushed, which is the same thing + * with a race in it. + * + * `from` is one millisecond past the end of the chosen version, so the version + * itself survives and everything after it is undone. It is also what keeps this + * inside what the user may touch: unlike a read, a rollback is refused rather + * than trimmed if it reaches further back than the history they were granted — + * and every moment they can name here is one the server showed them. + * + * Nothing is lost. The undo is a change like any other, so the state it replaced + * stays in the history and can be restored again from the same panel. + */ +const restoreDocVersion = async ( + target: CollaborationTarget, + { docId, versionId }: RestoreDocVersionParam, +) => + fetchCollaborationAPI(target, 'rollback', docId, { + method: 'POST', + body: { from: Number(versionId) + 1 }, + }); + +export function useRestoreDocVersion({ + onSuccess, +}: { onSuccess?: () => void } = {}) { + const queryClient = useQueryClient(); + const target = useCollaborationTarget(); + + return useMutation({ + mutationFn: async (params) => { + await restoreDocVersion(target as CollaborationTarget, params); + }, + onSuccess: () => { + void queryClient.invalidateQueries({ queryKey: [KEY_DOC_ACTIVITY] }); + onSuccess?.(); + }, + }); +} diff --git a/src/frontend/apps/impress/src/features/docs/doc-versioning/components/DocVersionEditor.tsx b/src/frontend/apps/impress/src/features/docs/doc-versioning/components/DocVersionEditor.tsx index 11d862a81..c29cd6a49 100644 --- a/src/frontend/apps/impress/src/features/docs/doc-versioning/components/DocVersionEditor.tsx +++ b/src/frontend/apps/impress/src/features/docs/doc-versioning/components/DocVersionEditor.tsx @@ -9,13 +9,13 @@ import { DocEditorContainer } from '@/docs/doc-editor/components/DocEditor'; import { Doc, base64ToBlocknoteXmlFragment } from '@/docs/doc-management'; import { useDocVersion } from '../api/useDocVersion'; -import { Versions } from '../types'; +import { DocVersion } from '../types'; import { DocVersionHeader } from './DocVersionHeader'; interface DocVersionEditorProps { docId: Doc['id']; - versionId: Versions['version_id']; + versionId: DocVersion['id']; } export const DocVersionEditor = ({ @@ -41,12 +41,12 @@ export const DocVersionEditor = ({ }, [versionId]); useEffect(() => { - if (!version?.content || isLoading || initialContent) { + if (!version?.ydoc || isLoading || initialContent) { return; } - setInitialContent(base64ToBlocknoteXmlFragment(version.content)); - }, [versionId, version?.content, isLoading, initialContent]); + setInitialContent(base64ToBlocknoteXmlFragment(version.ydoc)); + }, [versionId, version?.ydoc, isLoading, initialContent]); if (isError && error) { if (error.status === 404) { @@ -90,7 +90,12 @@ export const DocVersionEditor = ({ > diff --git a/src/frontend/apps/impress/src/features/docs/doc-versioning/components/ModalConfirmationVersion.tsx b/src/frontend/apps/impress/src/features/docs/doc-versioning/components/ModalConfirmationVersion.tsx index dfcf317e3..5b5ac8497 100644 --- a/src/frontend/apps/impress/src/features/docs/doc-versioning/components/ModalConfirmationVersion.tsx +++ b/src/frontend/apps/impress/src/features/docs/doc-versioning/components/ModalConfirmationVersion.tsx @@ -1,12 +1,18 @@ -import { Button, Modal, ModalSize } from '@gouvfr-lasuite/ui-components'; +import { + Button, + Modal, + ModalSize, + VariantType, + useToastProvider, +} from '@gouvfr-lasuite/ui-components'; import { useTranslation } from 'react-i18next'; import { createGlobalStyle } from 'styled-components'; import { Box, Text } from '@/components'; import { Doc } from '@/docs/doc-management/'; -import { useDocVersion } from '../api'; -import { Versions } from '../types'; +import { useRestoreDocVersion } from '../api'; +import { DocVersion } from '../types'; const ModalStyle = createGlobalStyle` .c__modal__title { @@ -18,46 +24,29 @@ interface ModalConfirmationVersionProps { docId: Doc['id']; onClose: () => void; onSuccess: () => void; - versionId: Versions['version_id']; + versionId: DocVersion['id']; } export const ModalConfirmationVersion = ({ onClose, - onSuccess: __onSuccess, + onSuccess, docId, versionId, }: ModalConfirmationVersionProps) => { - const { data: version } = useDocVersion({ - docId, - versionId, - }); const { t } = useTranslation(); + const { toast } = useToastProvider(); - // TODO(yhub) : Revert the doc to a previous state using Y.js / Yhub - // const { mutate: updateDocContent } = useDocContentUpdate({ - // listInvalidQueries: [KEY_LIST_DOC_VERSIONS], - // onSuccess: () => { - // const onDisplaySuccess = () => { - // toast(t('Version restored successfully'), VariantType.SUCCESS); - // onSuccess(); - // }; - - // if (!provider || !version?.content) { - // onDisplaySuccess(); - // return; - // } - - // revertUpdate(provider.doc, provider.doc, base64ToYDoc(version.content)); - - // threadStore?.refreshThreads(); - - // onDisplaySuccess(); - // }, - // }); - - if (!version) { - return null; - } + /** + * The collaboration server undoes everything after this version and hands the + * result to every open editor, this one included — so there is nothing to + * apply here and nothing to reload. + */ + const { mutate: restoreVersion, isPending } = useRestoreDocVersion({ + onSuccess: () => { + toast(t('Version restored successfully'), VariantType.SUCCESS); + onSuccess(); + }, + }); return ( { - if (!version?.content) { - return; - } - - onClose(); - }} + disabled={isPending} + onClick={() => restoreVersion({ docId, versionId })} > {t('Restore')} diff --git a/src/frontend/apps/impress/src/features/docs/doc-versioning/components/ModalSelectVersion.tsx b/src/frontend/apps/impress/src/features/docs/doc-versioning/components/ModalSelectVersion.tsx index 77850f0d0..6f91ed287 100644 --- a/src/frontend/apps/impress/src/features/docs/doc-versioning/components/ModalSelectVersion.tsx +++ b/src/frontend/apps/impress/src/features/docs/doc-versioning/components/ModalSelectVersion.tsx @@ -12,7 +12,7 @@ import { createGlobalStyle, css } from 'styled-components'; import { Box, ButtonCloseModal, Text } from '@/components'; import { Doc } from '@/docs/doc-management'; -import { Versions } from '../types'; +import { DocVersion } from '../types'; import { DocVersionEditor } from './DocVersionEditor'; import { VersionList } from './VersionList'; @@ -48,7 +48,7 @@ export const ModalSelectVersion = ({ }: ModalSelectVersionProps) => { const { t } = useTranslation(); const [selectedVersionId, setSelectedVersionId] = - useState(); + useState(); const canRestore = doc.abilities.partial_update; const restoreModal = useModal(); diff --git a/src/frontend/apps/impress/src/features/docs/doc-versioning/components/VersionList.tsx b/src/frontend/apps/impress/src/features/docs/doc-versioning/components/VersionList.tsx index ee79ed0d8..efb02f619 100644 --- a/src/frontend/apps/impress/src/features/docs/doc-versioning/components/VersionList.tsx +++ b/src/frontend/apps/impress/src/features/docs/doc-versioning/components/VersionList.tsx @@ -3,21 +3,27 @@ import { DateTime } from 'luxon'; import { useTranslation } from 'react-i18next'; import { APIError } from '@/api'; -import { Box, Icon, InfiniteScroll, Text, TextErrors } from '@/components'; +import { Box, Icon, Text, TextErrors } from '@/components'; import { Doc } from '@/docs/doc-management'; import { useDate } from '@/hooks'; -import { useDocVersionsInfiniteQuery } from '../api/useDocVersions'; -import { Versions } from '../types'; +import { useDocActivity } from '../api/useDocActivity'; +import { DocVersion } from '../types'; import { VersionItem } from './VersionItem'; +/** + * The timeline's timestamps are unix milliseconds; every date helper here reads + * ISO strings. + */ +const toISO = (timestamp: number) => new Date(timestamp).toISOString(); + interface VersionListStateProps { isLoading: boolean; error: APIError | null; - versions?: Versions[]; - selectedVersionId?: Versions['version_id']; - onSelectVersion?: (versionId: Versions['version_id']) => void; + versions?: DocVersion[]; + selectedVersionId?: DocVersion['id']; + onSelectVersion?: (versionId: DocVersion['id']) => void; } const VersionListState = ({ @@ -41,16 +47,16 @@ const VersionListState = ({ {versions?.map((version) => { const formattedDate = formatDateSpecial( - version.last_modified, + toISO(version.to), 'dd MMMM · HH:mm', ); - const isSelected = version.version_id === selectedVersionId; + const isSelected = version.id === selectedVersionId; return ( - + onSelectVersion?.(version.version_id)} + onSelect={() => onSelectVersion?.(version.id)} /> ); @@ -76,8 +82,8 @@ const VersionListState = ({ interface VersionListProps { doc: Doc; - onSelectVersion?: (versionId: Versions['version_id']) => void; - selectedVersionId?: Versions['version_id']; + onSelectVersion?: (versionId: DocVersion['id']) => void; + selectedVersionId?: DocVersion['id']; } export const VersionList = ({ @@ -88,25 +94,22 @@ export const VersionList = ({ const { t } = useTranslation(); const { formatDate } = useDate(); + /** + * The whole list arrives at once — the collaboration server bounds it to the + * history this user may see, and a version is at least a minute of editing — + * so there is nothing to page through. + */ const { - data, + data: versions, error, isLoading, - fetchNextPage, - hasNextPage, - isFetchingNextPage, - } = useDocVersionsInfiniteQuery({ - docId: doc.id, - }); + } = useDocActivity({ docId: doc.id }); - const versions = data?.pages.reduce((acc, page) => { - return acc.concat(page.versions); - }, [] as Versions[]); const selectedVersion = versions?.find( - (version) => version.version_id === selectedVersionId, + (version) => version.id === selectedVersionId, ); const selectedVersionDate = selectedVersion - ? formatDate(selectedVersion.last_modified, DateTime.DATETIME_MED) + ? formatDate(toISO(selectedVersion.to), DateTime.DATETIME_MED) : null; return ( @@ -114,17 +117,7 @@ export const VersionList = ({ $css="overflow-y: auto; overflow-x: hidden;" className="--docs--version-list" > - { - void fetchNextPage(); - }} - as="ul" - $padding="none" - $margin={{ top: 'none' }} - role="list" - > + {versions?.length === 0 && ( @@ -139,7 +132,7 @@ export const VersionList = ({ versions={versions} selectedVersionId={selectedVersionId} /> - + {selectedVersionDate ? t('Selected version {{date}}', { date: selectedVersionDate }) diff --git a/src/frontend/apps/impress/src/features/docs/doc-versioning/types.ts b/src/frontend/apps/impress/src/features/docs/doc-versioning/types.ts index 87359070b..ac75e8cc9 100644 --- a/src/frontend/apps/impress/src/features/docs/doc-versioning/types.ts +++ b/src/frontend/apps/impress/src/features/docs/doc-versioning/types.ts @@ -1,19 +1,43 @@ -export interface APIListVersions { - count: number; - is_truncated: boolean; - next_version_id_marker: string | null; - versions: Versions[]; +/** + * One entry of the collaboration server's `activity` timeline: a stretch of + * editing, bounded by the first and last change in it, credited to whoever made + * them. `from` and `to` are unix milliseconds. + * + * `by` is a user id — a Docs user's uuid, or the literal `anonymous` for + * visitors editing a public document, or `system` for content the migration + * imported. It is not displayed today; resolving ids to names is its own + * feature. + */ +export interface ActivityEntry { + from: number; + to: number; + by: string | null; } -export interface Versions { - etag: string; - is_latest: boolean; - last_modified: string; - version_id: string; +export interface APIActivity { + activity: ActivityEntry[]; } -export interface Version { - content: string; // Base64 encoded content - last_modified: string; +/** + * A version, as the history panel shows it: one entry of the timeline after + * neighbouring entries have been merged (see `mergeActivityEntries`), with the + * authors that contributed to it. + * + * `id` is `to` as a string — the moment the version ends, which is both what + * identifies it in the list and what the changeset and the rollback are asked + * for. + */ +export interface DocVersion { id: string; + from: number; + to: number; + by: string[]; +} + +/** + * `GET changeset?ydoc=true`: the document as it stood at `to`, as a base64 + * y.js update. + */ +export interface APIChangeset { + ydoc: string | null; } diff --git a/src/frontend/apps/impress/src/features/docs/doc-versioning/utils.ts b/src/frontend/apps/impress/src/features/docs/doc-versioning/utils.ts index e79a1d47f..202f6476a 100644 --- a/src/frontend/apps/impress/src/features/docs/doc-versioning/utils.ts +++ b/src/frontend/apps/impress/src/features/docs/doc-versioning/utils.ts @@ -1,55 +1,68 @@ -import * as Y from 'yjs'; +import { ActivityEntry, DocVersion } from './types'; /** - * Revert the doc to a previous state. + * How coarse the version history is: neighbouring changes closer together than + * this become one version, and no version spans more than this. * - * We cannot simply replace a doc with another previous doc, - * because Y.js will act as if the previous doc is a new doc and so - * merge it with the current doc, so we need to revert the doc (undo). - * - * To do so we simulate a history of the doc by saving snapshots of the doc - * and then revert the doc to a previous snapshot. - * - * @param doc - * @param snapshotOrigin - * @param snapshotUpdate + * A minute is a deliberate choice about what a "version" means here. The + * collaboration server records an activity entry per stretch of editing, which + * at typing speed is far finer than anything worth listing — a history of every + * few keystrokes is not a history. */ -export function revertUpdate( - doc: Y.Doc, - snapshotOrigin: Y.Doc, - snapshotUpdate: Y.Doc, -) { - try { - const snapshotDoc = new Y.Doc(); - Y.applyUpdate( - snapshotDoc, - Y.encodeStateAsUpdate(snapshotUpdate), - snapshotOrigin, - ); +export const VERSION_GRANULARITY_MS = 60_000; - const currentStateVector = Y.encodeStateVector(doc); - const snapshotStateVector = Y.encodeStateVector(snapshotDoc); +/** + * Merge an ascending activity timeline into the versions the panel lists. + * + * The collaboration server already groups with the same two bounds (a gap and a + * maximum span, both a minute), but it will only ever merge changes by the + * *same* author: it breaks a run whenever the author changes. Two people typing + * in the same paragraph at the same time would otherwise produce two interleaved + * columns of entries, which is not what a version is — a version is a moment in + * the document, not a moment in someone's editing. + * + * So this applies the server's own rule again, minus the author test, and keeps + * the authors instead of discarding them. Because the server sorts by `from` and + * breaks a run only where the author changes, re-merging those adjacent runs + * yields exactly what one pass over the whole timeline would have produced. + * + * Both comparisons are `<`, matching the server's, so entries exactly a minute + * apart start a new version rather than joining the old one. + */ +export const mergeActivityEntries = ( + activity: ActivityEntry[], + granularityMs: number = VERSION_GRANULARITY_MS, +): DocVersion[] => { + const versions: DocVersion[] = []; + const authors: Set[] = []; - const changesSinceSnapshotUpdate = Y.encodeStateAsUpdate( - doc, - snapshotStateVector, - ); + activity.forEach((entry) => { + const last = versions[versions.length - 1]; - const undoManager = new Y.UndoManager( - [snapshotDoc.getMap('document-store')], - { - trackedOrigins: new Set([snapshotOrigin]), - }, - ); + if ( + last && + entry.from - last.to < granularityMs && + entry.to - last.from < granularityMs + ) { + last.to = entry.to; + last.id = String(entry.to); + } else { + versions.push({ + id: String(entry.to), + from: entry.from, + to: entry.to, + by: [], + }); + authors.push(new Set()); + } - Y.applyUpdate(snapshotDoc, changesSinceSnapshotUpdate, snapshotOrigin); - undoManager.undo(); - const revertChangesSinceSnapshotUpdate = Y.encodeStateAsUpdate( - snapshotDoc, - currentStateVector, - ); - Y.applyUpdate(doc, revertChangesSinceSnapshotUpdate, snapshotOrigin); - } catch (e) { - console.error('Failed to revert the doc to a previous state', e); - } -} + if (entry.by) { + authors[authors.length - 1].add(entry.by); + } + }); + + return versions.map((version, index) => ({ + ...version, + by: Array.from(authors[index]), + })); +}; diff --git a/src/helm/impress/values.yaml b/src/helm/impress/values.yaml index e51c69c16..ee9a4983e 100644 --- a/src/helm/impress/values.yaml +++ b/src/helm/impress/values.yaml @@ -120,10 +120,10 @@ ingressCollaborationApi: ## ## The routes yhub serves to browsers, guarded by the same document ## authorization as the websocket. Everything it serves that is not listed - ## here stays in-cluster — `create-ydoc`, `reset-connections`, `migrate`, - ## `restore-ydoc` and `reset-ydoc` are called by the backend only, and - ## publishing them would put document deletion and the legacy migration one - ## request away from the internet. + ## here stays in-cluster — `prune`, `create-ydoc`, `reset-connections`, + ## `migrate`, `restore-ydoc` and `reset-ydoc` are called by the backend only + ## (or by nobody), and publishing them would put document deletion, content + ## erasure and the legacy migration one request away from the internet. ## ## `jwks` is public on purpose: it carries the public halves of the keys ## yhub signs with, and nothing else. @@ -131,10 +131,17 @@ ingressCollaborationApi: ## `activity` and `changeset` carry the editing history, bounded per user to ## the moment they were given access to the document — a user who holds no ## access on it, only its link, is refused both. + ## + ## `rollback` restores a document to an earlier state, and is the one route + ## here through which a browser changes the past rather than reading it. It + ## is published because the version history's restore button calls it: any + ## user who may edit a document may roll it back, bounded by the same date as + ## the history they can see. Readers are refused it. paths: - /collaboration/ydoc/ - /collaboration/activity/ - /collaboration/changeset/ + - /collaboration/rollback/ - /collaboration/jwks/ ## @param ingressCollaborationApi.hosts Additional host to configure for the Ingress hosts: [] diff --git a/src/yhub-server/README.md b/src/yhub-server/README.md index f5ab27427..f194a4f45 100644 --- a/src/yhub-server/README.md +++ b/src/yhub-server/README.md @@ -82,14 +82,14 @@ It is not a fork of yhub — it is a thin wrapper: Public exposure: the browser needs the websocket `/collaboration/ws/`, `/collaboration/ydoc/` for the http fallback, `/collaboration/activity/` and -`/collaboration/changeset/` for the editing history, plus -`/collaboration/jwks/v1`, which carries public keys and nothing else. Every -other route this server serves — `rollback`, `prune`, `reset-connections`, -`migrate`, `create-ydoc`, `restore-ydoc`, `reset-ydoc` — is refused to a browser -by the permission tables themselves (see "Access control" below), so publishing -one is no longer the security boundary it was under yhub 0.7. Keep them off the -public ingress all the same: an endpoint that cannot be reached cannot be -probed. The two probes are not worth publishing either — kubelet calls them from +`/collaboration/changeset/` for the editing history, `/collaboration/rollback/` +for restoring a document to a point in it, plus `/collaboration/jwks/v1`, which +carries public keys and nothing else. Every other route this server serves — +`prune`, `reset-connections`, `migrate`, `create-ydoc`, `restore-ydoc`, +`reset-ydoc` — is refused to a browser by the permission tables themselves (see +"Access control" below), so publishing one is no longer the security boundary it +was under yhub 0.7. Keep them off the public ingress all the same: an endpoint +that cannot be reached cannot be probed. The two probes are not worth publishing either — kubelet calls them from inside — and the helm chart's ingress lists what it routes rather than what it hides, so they stay in-cluster on their own. @@ -109,12 +109,13 @@ Masks are positional `crud` strings where `-` denies, so `'-r--'` is read-only. |---|---|---|---|---| | `ydoc` | `-r--` | `-ru-` | as reader/editor | `cru-` | | `awareness` | `-r--` | `-ru-` | as reader/editor | `-ru-` | -| `history` | `from: ` | `from: ` | — | `from: 0` | +| `history` | `from: ` | `from: `, `rollback` | — | `from: 0` | | `delete` | — | — | — | `['soft']` | | `endpoint.ws` | `-r--` | `-ru-` | as reader/editor | `crud` (`'*'`) | | `endpoint.ydoc` | `-r--` | `-ru-` | as reader/editor | `crud` (`'*'`) | | `endpoint.activity` | `-r--` | `-r--` | — | `crud` (`'*'`) | | `endpoint.changeset` | `-r--` | `-r--` | — | `crud` (`'*'`) | +| `endpoint.rollback` | — | `c---` | — | `crud` (`'*'`) | | every other endpoint | — | — | — | `crud` (`'*'`) | All three browser columns are the same document permission, @@ -123,7 +124,7 @@ All three browser columns are the same document permission, there is a history to read. `abilities.retrieve` decided whether there is any access at all before either. -Five of those cells are decisions rather than transcriptions: +Six of those cells are decisions rather than transcriptions: - **`awareness: '-r--'` for a reader.** A reader receives presence and never publishes it — [suitenumerique/docs#2544](https://github.com/suitenumerique/docs/pull/2544), @@ -135,7 +136,7 @@ Five of those cells are decisions rather than transcriptions: a feature. The frontend has to know it too: the http fallback provider has no receive-only setting, so a reader's `HttpProvider` is built with no awareness instance at all, or its first `PATCH` would take a 403 and close it for good. -- **No `'*'` endpoint fallback for the browser.** Only the four routes above are +- **No `'*'` endpoint fallback for the browser.** Only the routes above are named, so everything else is denied — including any endpoint a future yhub release adds. Under 0.7 this fence was a `purpose != null` check, which `create-ydoc` slipped through by declaring no purpose. @@ -159,8 +160,24 @@ Five of those cells are decisions rather than transcriptions: version history for exactly that reason ("we wouldn't know from which date to allow them anyway"). `activity` and `changeset` are withheld together with the ray rather than granted alone, which would open a route that answers 403 by - itself. `rollback` and `prune` are withheld from everyone: they are - destructive and are granted by name. + itself — and `rollback` with them. +- **`history.rollback` for an editor.** `POST /rollback` undoes every change in + a window, and it is what the version history's restore button calls: **any + user who may edit a document may restore it to an earlier state.** Four things + bound that. + - A reader cannot, twice over. yhub normalizes `rollback` to `false` unless + `ydoc` carries `u` — it is a dead grant without the write it rides on — and + the requirement side mirrors it. Docs additionally withholds the endpoint, + so a reader is refused at the door rather than inside the handler. + - Nobody can undo what happened before they arrived. Mutations *refuse* where + reads clamp: a rollback demands a ray reaching back to its own `from` + instead of having it moved forward silently. Every moment a user can name is + one the timeline showed them, and that timeline starts at their access date + — so the bound holds without trusting the client, and a rollback with no + `from` at all, which asks to undo all of history, is refused outright. + - Nothing is destroyed. A rollback appends an update that undoes another; what + it undid stays in the history and can be restored again from the same panel. + - `prune`, which does erase, stays withheld from everyone. - **`delete: ['soft']` and not `'hard'` for the admin.** yhub 0.8 made `DELETE /ydoc?hard=true` reachable over REST for the first time. Docs keeps irreversible erasure programmatic, behind `reset-ydoc` (see "Deletion"). @@ -601,10 +618,16 @@ Operational notes: ## Full migration (`POST /collaboration/migrate/v1/{org}/{docid}`) The media bucket is versioned, so `{docid}/file` keeps every snapshot Django -ever wrote — that is the version history the backend exposes at -`/documents/{id}/versions/`. The lazy seed above replays only the newest one, so -a soft-migrated document lands in yhub as a single `system` change stamped with -the migration time and its past is gone. +ever wrote — that is the version history the backend used to expose at +`/documents/{id}/versions/`, and which nothing writes to any more. The lazy seed +above replays only the newest one, so a soft-migrated document lands in yhub as a +single `system` change stamped with the migration time and its past is gone. + +This is what makes the backfill user-visible rather than housekeeping. The +frontend's version history is built from `activity`, so until a document has been +migrated in full its history begins at the moment it reached yhub: the snapshots +are still in S3, but nothing reads them. Running the backfill is what gives those +documents their past back. `migrate` replays the whole history instead. It lists the object's versions and applies them, oldest first, to a single `Y.Doc({ gc: false })`; after each one @@ -612,9 +635,12 @@ it credits the ids that version introduced (and the ones it deleted) with **that version's own S3 timestamp**. `GET /collaboration/activity/v1/{org}/{docid}?group=false` then reports one entry per S3 version, at the same timestamps the backend's version listing reports as -`last_modified` — which is what lines the two up. (Pass `group=false`: the -default grouping merges changes by the same author less than a second apart, -which would fold versions saved in quick succession into one entry.) +`last_modified` — which is what lines the two up, and what makes the two lists +comparable when checking a backfill. (Pass `group=false`: the default grouping +merges changes by the same author less than a second apart, which would fold +versions saved in quick succession into one entry. The frontend asks for the +opposite — a minute of grouping — because it wants a readable history rather +than a faithful one; use `group=false` to compare, not what the browser sends.) `gc: false` is what preserves content that later versions deleted — most of what makes a history worth keeping. diff --git a/src/yhub-server/permissions.js b/src/yhub-server/permissions.js index 0f556b3a0..995c980fc 100644 --- a/src/yhub-server/permissions.js +++ b/src/yhub-server/permissions.js @@ -52,9 +52,30 @@ * connection, which requires `from === 0` exactly — see the guard in server.js. * * No `delete` facet: deleting a document is Django's, through the admin token. - * Deliberately absent too: `rollback` and `prune`, which are destructive and are - * granted by name — restoring a version is not something a reader, or an editor, - * does through this grant today. + * + * `history.rollback` is granted to an editor, and it is the one thing here that + * lets a browser change the past rather than read it: `POST /rollback` undoes + * every change in a window, which is what the version history's "restore" button + * is. Four things bound it. + * + * A reader never gets it, twice over. yhub normalizes `rollback` to `false` + * unless `ydoc` carries `u` — it is a dead grant without the write it rides on — + * and the requirement side mirrors that, so a reader would be refused even if + * this table said otherwise. `canEdit` is belt to those braces, and withholds + * the endpoint with it so a reader is refused once, at the door, instead of + * halfway through the handler. + * + * Nobody can undo what happened before they arrived. Mutations refuse where + * reads clamp: `POST /rollback` demands a ray reaching back to its own `from`, + * rather than quietly moving it forward the way `activity` does. Every moment a + * user can name is one they were shown, and everything they were shown is inside + * their ray — so the bound holds without the client being trusted to respect it, + * and a rollback with no `from` at all, which would ask to undo all of history, + * is refused outright. + * + * `prune` stays absent. Rollback is additive — it appends an update that undoes + * another, and what it undid is still in the history, still restorable by the + * same route. Prune erases, and no browser needs that. * * No `'*'` endpoint fallback, so everything not named here is denied — including * any endpoint a future yhub release adds. Under 0.7 this fence was a @@ -65,15 +86,24 @@ export const browserDocumentPermissions = (canEdit, historyFrom = null) => ({ type: 'permissions:document:v1', ydoc: canEdit ? '-ru-' : '-r--', awareness: canEdit ? '-ru-' : '-r--', - ...(historyFrom ? { history: { from: historyFrom } } : null), + ...(historyFrom + ? { history: { from: historyFrom, ...(canEdit && { rollback: true }) } } + : null), endpoint: { // `r` opens the socket, `u` admits document updates over it ws: canEdit ? '-ru-' : '-r--', // GET is `r` and PATCH is `u`; DELETE (`d`) stays out — see `delete` above ydoc: canEdit ? '-ru-' : '-r--', // the editing timeline, and one point in it — both GET-only, both clamped to - // the ray above - ...(historyFrom ? { activity: '-r--', changeset: '-r--' } : null), + // the ray above — and, for an editor, the route that undoes a window of it. + // `c---` because rollback is a POST; there is no other verb on it + ...(historyFrom + ? { + activity: '-r--', + changeset: '-r--', + ...(canEdit && { rollback: 'c---' }), + } + : null), }, }); diff --git a/src/yhub-server/permissions.test.js b/src/yhub-server/permissions.test.js index 9a4e796c7..1a505ceac 100644 --- a/src/yhub-server/permissions.test.js +++ b/src/yhub-server/permissions.test.js @@ -144,24 +144,82 @@ describe('the history a user may read', () => { } }); - it('never grants rollback or prune', () => { - // destructive, granted by name, and restoring a version is not something - // this grant does + it('never grants prune', () => { + // erasure, granted by name, and nothing a browser does needs it for (const who of [reader, editor]) { - assert.equal( - grants(who, { history: { from: ACCESS_SINCE, rollback: true } }), - false, - ); assert.equal( grants(who, { history: { from: ACCESS_SINCE, prune: true } }), false, ); - assert.equal(grants(who, { endpoint: { rollback: 'c---' } }), false); assert.equal(grants(who, { endpoint: { prune: 'c---' } }), false); } }); }); +describe('undoing a stretch of history', () => { + /** + * `POST /rollback` appends an update that undoes every change in a window: it + * is what the version history's restore button does. An editor may, a reader + * may not, and neither may reach back past the moment they arrived. + */ + it('lets an editor undo a window of its own history', () => { + assert.equal( + grants(editor, { history: { from: ACCESS_SINCE, rollback: true } }), + true, + ); + assert.equal(grants(editor, { endpoint: { rollback: 'c---' } }), true); + }); + + it('refuses an editor a window wider than its ray', () => { + // the difference between a read and a mutation: `activity` would clamp this + // silently, `rollback` refuses it. `from: 0` is what a rollback with no + // bound at all asks for, so this is also what stops "undo everything" + assert.equal( + grants(editor, { history: { from: 0, rollback: true } }), + false, + ); + assert.equal( + grants(editor, { history: { from: ACCESS_SINCE - 1, rollback: true } }), + false, + ); + }); + + it('never lets a reader undo anything', () => { + assert.equal( + grants(reader, { history: { from: ACCESS_SINCE, rollback: true } }), + false, + ); + assert.equal(grants(reader, { endpoint: { rollback: 'c---' } }), false); + }); + + it('is a dead grant without the write it rides on', () => { + // yhub's own rule, asserted here because it is what makes the reader case + // safe even if this policy ever spelled it wrong: `rollback` normalizes to + // false unless `ydoc` carries `u` + const readerWithRollback = normalizePermissions({ + ...browserDocumentPermissions(false, ACCESS_SINCE), + history: { from: ACCESS_SINCE, rollback: true }, + }); + assert.equal(readerWithRollback.history.rollback, false); + }); + + it('is withheld from a browser that holds no access', () => { + for (const who of [linkReader, linkEditor]) { + assert.equal( + grants(who, { history: { from: ACCESS_SINCE, rollback: true } }), + false, + ); + assert.equal(grants(who, { endpoint: { rollback: 'c---' } }), false); + } + }); + + it('is not granted to the admin token', () => { + // 0.8 stopped implying it from write access, and the backend does not + // restore versions — the browser does + assert.equal(grants(admin, { history: { from: 0, rollback: true } }), false); + }); +}); + describe('a reader who holds no access, only the link', () => { /** * There is no access row and so no date. The backend has always refused these @@ -199,7 +257,6 @@ describe('everything the browser must not reach', () => { // a future release is denied until it is named — this is the property that // replaced 0.7's `purpose != null` check for (const name of [ - 'rollback', 'prune', 'create-ydoc', 'migrate', @@ -235,9 +292,8 @@ describe('the admin token', () => { assert.equal(grants(admin, { delete: ['hard'] }), false); }); - it('is not granted rollback or prune', () => { - // 0.8 stopped implying them from write access; Docs does not use them - assert.equal(grants(admin, { history: { from: 0, rollback: true } }), false); + it('is not granted prune', () => { + // 0.8 stopped implying it from write access; Docs does not use it assert.equal(grants(admin, { history: { from: 0, prune: true } }), false); }); });