diff --git a/apps/client/src/features/page/tree/model/tree-model.test.ts b/apps/client/src/features/page/tree/model/tree-model.test.ts index 066464d7..8f16c075 100644 --- a/apps/client/src/features/page/tree/model/tree-model.test.ts +++ b/apps/client/src/features/page/tree/model/tree-model.test.ts @@ -280,6 +280,108 @@ describe('handleCreate optimistic-insert idempotency (find-then-skip)', () => { }); }); +// moveTreeNode socket-handler semantics: the receiver must place the moved node +// by `position` (NOT index 0) and apply the `pageData` the payload carries so a +// moved node's title/icon/chevron stay correct. This mirrors the reducer in +// use-tree-socket.ts so the contract is unit-tested without rendering the hook. +describe('moveTreeNode handler (place by position + apply pageData)', () => { + type P = TreeNode<{ + name: string; + position?: string; + icon?: string; + hasChildren?: boolean; + parentPageId?: string | null; + }>; + + const applyMoveTreeNode = ( + tree: P[], + payload: { + id: string; + parentId: string | null; + position: string; + pageData?: { title?: string | null; icon?: string | null; hasChildren?: boolean }; + }, + ): P[] => { + if (!treeModel.find(tree, payload.id)) return tree; + const placed = treeModel.placeByPosition(tree, payload.id, { + parentId: payload.parentId, + position: payload.position, + }); + if (placed === tree) return treeModel.remove(tree, payload.id); + const patch: Partial

= { + position: payload.position, + parentPageId: payload.parentId, + } as Partial

; + const pd = payload.pageData; + if (pd) { + if (pd.title !== undefined) (patch as { name?: string }).name = pd.title ?? ''; + if (pd.icon !== undefined) (patch as { icon?: string }).icon = pd.icon ?? undefined; + if (pd.hasChildren !== undefined) + (patch as { hasChildren?: boolean }).hasChildren = pd.hasChildren; + } + return treeModel.update(placed, payload.id, patch); + }; + + const tree: P[] = [ + { + id: 'dst', + name: 'DST', + position: 'a0', + children: [ + { id: 'c1', name: 'C1', position: 'a1' }, + { id: 'c2', name: 'C2', position: 'a3' }, + { id: 'c3', name: 'C3', position: 'a5' }, + ], + }, + { id: 'src', name: 'SRC', position: 'a9' }, + ]; + + it('lands the moved node in the correct MIDDLE slot, not at index 0', () => { + const t = applyMoveTreeNode(tree, { + id: 'src', + parentId: 'dst', + position: 'a4', + }); + expect(treeModel.find(t, 'dst')?.children?.map((n) => n.id)).toEqual([ + 'c1', 'c2', 'src', 'c3', + ]); + }); + + it('lands the moved node at the END when position sorts last', () => { + const t = applyMoveTreeNode(tree, { + id: 'src', + parentId: 'dst', + position: 'a8', + }); + expect(treeModel.find(t, 'dst')?.children?.map((n) => n.id)).toEqual([ + 'c1', 'c2', 'c3', 'src', + ]); + }); + + it('applies pageData (title/icon/hasChildren) to the moved node', () => { + const t = applyMoveTreeNode(tree, { + id: 'src', + parentId: 'dst', + position: 'a4', + pageData: { title: 'Renamed', icon: '🔥', hasChildren: true }, + }); + const moved = treeModel.find(t, 'src'); + expect(moved?.name).toBe('Renamed'); + expect(moved?.icon).toBe('🔥'); + expect(moved?.hasChildren).toBe(true); + expect(moved?.position).toBe('a4'); + }); + + it('falls back to removing the node when the destination parent is not loaded', () => { + const t = applyMoveTreeNode(tree, { + id: 'src', + parentId: 'not-loaded', + position: 'a4', + }); + expect(treeModel.find(t, 'src')).toBeNull(); + }); +}); + describe('treeModel.remove', () => { it('removes a leaf', () => { const t = treeModel.remove(fixture, 'a2'); @@ -392,6 +494,118 @@ describe('treeModel.place', () => { }); }); +describe('treeModel.placeByPosition', () => { + // Server-authoritative `moveTreeNode` ships the moved node's fractional + // `position`; the receiver must sort it into the correct slot among the new + // siblings — NOT drop it at index 0. + type P = TreeNode<{ name: string; position?: string }>; + + const tree: P[] = [ + { + id: 'dst', + name: 'DST', + position: 'a0', + children: [ + { id: 'c1', name: 'C1', position: 'a1' }, + { id: 'c2', name: 'C2', position: 'a3' }, + { id: 'c3', name: 'C3', position: 'a5' }, + ], + }, + { id: 'src', name: 'SRC', position: 'a9' }, + ]; + + it('places the moved node in the MIDDLE of new siblings by position', () => { + const t = treeModel.placeByPosition(tree, 'src', { + parentId: 'dst', + position: 'a4', + }); + expect(treeModel.find(t, 'dst')?.children?.map((n) => n.id)).toEqual([ + 'c1', 'c2', 'src', 'c3', + ]); + }); + + it('places the moved node at the END when its position sorts last', () => { + const t = treeModel.placeByPosition(tree, 'src', { + parentId: 'dst', + position: 'a8', + }); + expect(treeModel.find(t, 'dst')?.children?.map((n) => n.id)).toEqual([ + 'c1', 'c2', 'c3', 'src', + ]); + }); + + it('places the moved node at the FRONT only when its position sorts first', () => { + const t = treeModel.placeByPosition(tree, 'src', { + parentId: 'dst', + position: 'a0', + }); + expect(treeModel.find(t, 'dst')?.children?.map((n) => n.id)).toEqual([ + 'src', 'c1', 'c2', 'c3', + ]); + }); + + it('stamps the authoritative position onto the moved node', () => { + const t = treeModel.placeByPosition(tree, 'src', { + parentId: 'dst', + position: 'a4', + }); + expect(treeModel.find(t, 'src')?.position).toBe('a4'); + }); + + it('reorders within the same parent by position (not to index 0)', () => { + const same: P[] = [ + { + id: 'p', + name: 'P', + position: 'a0', + children: [ + { id: 'x', name: 'X', position: 'a1' }, + { id: 'y', name: 'Y', position: 'a2' }, + { id: 'z', name: 'Z', position: 'a3' }, + ], + }, + ]; + // Move x to between y and z. + const t = treeModel.placeByPosition(same, 'x', { + parentId: 'p', + position: 'a25', + }); + expect(treeModel.find(t, 'p')?.children?.map((n) => n.id)).toEqual([ + 'y', 'x', 'z', + ]); + }); + + it('returns same array reference for unknown source', () => { + expect( + treeModel.placeByPosition(tree, 'ghost', { parentId: 'dst', position: 'a4' }), + ).toBe(tree); + }); + + it('returns same array reference when destination parent is not loaded', () => { + expect( + treeModel.placeByPosition(tree, 'src', { parentId: 'ghost', position: 'a4' }), + ).toBe(tree); + }); + + it('moves a node to root by position', () => { + const roots: P[] = [ + { id: 'r1', name: 'R1', position: 'a1' }, + { id: 'r2', name: 'R2', position: 'a5' }, + { + id: 'rp', + name: 'RP', + position: 'a7', + children: [{ id: 'child', name: 'CHILD', position: 'a1' }], + }, + ]; + const t = treeModel.placeByPosition(roots, 'child', { + parentId: null, + position: 'a3', + }); + expect(t.map((n) => n.id)).toEqual(['r1', 'child', 'r2', 'rp']); + }); +}); + describe('treeModel.move', () => { it('reorder-before within same parent: moves source to target index', () => { const { tree: t, result } = treeModel.move(fixture, 'a2', { diff --git a/apps/client/src/features/page/tree/model/tree-model.ts b/apps/client/src/features/page/tree/model/tree-model.ts index 0c1b26a4..2dd100aa 100644 --- a/apps/client/src/features/page/tree/model/tree-model.ts +++ b/apps/client/src/features/page/tree/model/tree-model.ts @@ -215,6 +215,30 @@ export const treeModel = { return treeModel.insert(removed, to.parentId, source, to.index); }, + // Position-aware move for server-authoritative `moveTreeNode` broadcasts. Like + // `place`, but instead of an absolute index (which the sender computed against + // its own loaded set), it inserts the moved node among the destination's + // already-loaded siblings ordered by the node's fractional `position`. This + // keeps the visible order correct for every receiver — `place(..., index: 0)` + // would wrongly drop the node at the TOP of its new sibling list. + // Returns the same array reference (like `place`) when the source is missing + // or the destination parent isn't loaded on this client, so callers can detect + // that and fall back to removing the node. + placeByPosition( + tree: TreeNode[], + sourceId: string, + to: { parentId: string | null; position?: string }, + ): TreeNode[] { + const source = treeModel.find(tree, sourceId); + if (!source) return tree; + if (to.parentId !== null && !treeModel.find(tree, to.parentId)) return tree; + const removed = treeModel.remove(tree, sourceId); + // Reuse the same position-ordered insertion as `insertByPosition` by + // stamping the authoritative position onto the moved node first. + const positioned = { ...source, position: to.position } as TreeNode; + return treeModel.insertByPosition(removed, to.parentId, positioned); + }, + move( tree: TreeNode[], sourceId: string, diff --git a/apps/client/src/features/websocket/use-tree-socket.ts b/apps/client/src/features/websocket/use-tree-socket.ts index 8b17caee..89506a16 100644 --- a/apps/client/src/features/websocket/use-tree-socket.ts +++ b/apps/client/src/features/websocket/use-tree-socket.ts @@ -84,22 +84,48 @@ export const useTreeSocket = () => { (sourceBefore as SpaceTreeNode).parentPageId ?? null; const newParentId = event.payload.parentId as string | null; - const placed = treeModel.place(prev, event.payload.id, { + // Place the node by its fractional `position` among the new + // siblings — NOT by the sender's absolute `index` (the sender + // computed that against its own loaded set, which differs from + // this receiver's). Using the position keeps the visible order + // correct on every client; placing at `index: 0` would wrongly + // drop reordered/moved nodes at the top of their new sibling list. + const placed = treeModel.placeByPosition(prev, event.payload.id, { parentId: newParentId, - index: event.payload.index, + position: event.payload.position, }); - // `place` silently returns the same reference if the destination - // parent isn't loaded on this client. Falling back to removing the - // source keeps the UI consistent (the source will reappear when - // the user expands the new parent and lazy-load fetches it). + // `placeByPosition` silently returns the same reference if the + // destination parent isn't loaded on this client. Falling back to + // removing the source keeps the UI consistent (the source will + // reappear when the user expands the new parent and lazy-load + // fetches it). if (placed === prev) { return treeModel.remove(prev, event.payload.id); } - let next = treeModel.update(placed, event.payload.id, { + // Apply the authoritative node fields the move payload carries + // (`pageData`) so receivers don't keep a stale title/icon/chevron + // on the moved node. `placeByPosition` already set `position`. + const pageData = event.payload.pageData as + | { + title?: string | null; + icon?: string | null; + hasChildren?: boolean; + } + | undefined; + const patch: Partial = { position: event.payload.position, - parentPageId: newParentId, - } as Partial); + parentPageId: newParentId as string, + }; + if (pageData) { + // The tree node stores the title as `name`. + if (pageData.title !== undefined) patch.name = pageData.title ?? ""; + if (pageData.icon !== undefined) + patch.icon = pageData.icon ?? undefined; + if (pageData.hasChildren !== undefined) + patch.hasChildren = pageData.hasChildren; + } + let next = treeModel.update(placed, event.payload.id, patch); // Mirror the emitter's hasChildren bookkeeping so both clients // converge to the same chevron state. diff --git a/apps/server/src/ws/ws-tree.service.spec.ts b/apps/server/src/ws/ws-tree.service.spec.ts index 00877d6d..3f212f5d 100644 --- a/apps/server/src/ws/ws-tree.service.spec.ts +++ b/apps/server/src/ws/ws-tree.service.spec.ts @@ -21,16 +21,33 @@ const snapshot: TreeNodeSnapshot = { describe('WsTreeService', () => { let service: WsTreeService; - let wsService: { emitTreeEvent: jest.Mock; emitToSpaceRoom: jest.Mock }; + let wsService: { + emitTreeEvent: jest.Mock; + emitToSpaceRoom: jest.Mock; + emitDeleteToUnauthorized: jest.Mock; + emitToAuthorizedUsers: jest.Mock; + }; + let pagePermissionRepo: { hasRestrictedAncestor: jest.Mock }; beforeEach(async () => { wsService = { emitTreeEvent: jest.fn().mockResolvedValue(undefined), emitToSpaceRoom: jest.fn(), + emitDeleteToUnauthorized: jest.fn().mockResolvedValue(undefined), + emitToAuthorizedUsers: jest.fn().mockResolvedValue(undefined), + }; + pagePermissionRepo = { + // Default: not restricted, so broadcastPageMoved skips the compensating + // delete unless a test opts in. + hasRestrictedAncestor: jest.fn().mockResolvedValue(false), }; const module: TestingModule = await Test.createTestingModule({ - providers: [WsTreeService, { provide: WsService, useValue: wsService }], + providers: [ + WsTreeService, + { provide: WsService, useValue: wsService }, + { provide: PagePermissionRepo, useValue: pagePermissionRepo }, + ], }).compile(); service = module.get(WsTreeService); @@ -118,6 +135,76 @@ describe('WsTreeService', () => { ); }); + it('broadcastPageMoved into an UNrestricted location does NOT emit a compensating delete', async () => { + pagePermissionRepo.hasRestrictedAncestor.mockResolvedValue(false); + + const event: PageMovedEvent = { + workspaceId: 'ws-1', + oldParentId: 'old-parent', + hasChildren: false, + node: { ...snapshot, parentPageId: 'new-parent', position: 'a5' }, + }; + + await service.broadcastPageMoved(event); + + // Normal path: move goes to the whole room via emitTreeEvent, and neither + // the authorized-only move path nor the compensating delete fire. + expect(wsService.emitTreeEvent).toHaveBeenCalledTimes(1); + expect(wsService.emitToAuthorizedUsers).not.toHaveBeenCalled(); + expect(wsService.emitDeleteToUnauthorized).not.toHaveBeenCalled(); + }); + + it('broadcastPageMoved into a RESTRICTED subtree routes the move to authorized users only AND emits a compensating delete to unauthorized — from one fresh decision', async () => { + // Destination is now under a restricted ancestor. + pagePermissionRepo.hasRestrictedAncestor.mockResolvedValue(true); + + const event: PageMovedEvent = { + workspaceId: 'ws-1', + oldParentId: 'old-parent', + hasChildren: false, + node: { ...snapshot, parentPageId: 'restricted-parent', position: 'a5' }, + }; + + await service.broadcastPageMoved(event); + + // The single fresh restriction decision was read exactly once... + expect(pagePermissionRepo.hasRestrictedAncestor).toHaveBeenCalledTimes(1); + expect(pagePermissionRepo.hasRestrictedAncestor).toHaveBeenCalledWith( + 'page-1', + ); + + // ...and it must NOT go through the cache-gated room-wide emitTreeEvent, + // which could leak the move to the whole room during the stale-cache window. + expect(wsService.emitTreeEvent).not.toHaveBeenCalled(); + + // The move is delivered to authorized users only. + expect(wsService.emitToAuthorizedUsers).toHaveBeenCalledTimes(1); + expect(wsService.emitToAuthorizedUsers).toHaveBeenCalledWith( + 'space-1', + 'page-1', + expect.objectContaining({ + operation: 'moveTreeNode', + spaceId: 'space-1', + payload: expect.objectContaining({ id: 'page-1' }), + }), + ); + + // The users who lost access get a deleteTreeNode for the moved node, scoped + // to the same page id (same fresh authorized set → disjoint from the move). + expect(wsService.emitDeleteToUnauthorized).toHaveBeenCalledTimes(1); + expect(wsService.emitDeleteToUnauthorized).toHaveBeenCalledWith( + 'space-1', + 'page-1', + expect.objectContaining({ + operation: 'deleteTreeNode', + spaceId: 'space-1', + payload: { + node: expect.objectContaining({ id: 'page-1', slugId: 'slug-1' }), + }, + }), + ); + }); + it('broadcastRefetchRoot emits refetchRootTreeNodeEvent to the space room', async () => { await service.broadcastRefetchRoot('space-7'); @@ -203,4 +290,30 @@ describe('WsService.emitTreeEvent', () => { expect(okEmit).toHaveBeenCalledWith('message', data); expect(noEmit).not.toHaveBeenCalled(); }); + + it('emitDeleteToUnauthorized sends ONLY to sockets whose user lacks page access', async () => { + pagePermissionRepo.getUserIdsWithPageAccess.mockResolvedValue(['user-ok']); + + const okEmit = jest.fn(); + const noEmit = jest.fn(); + const anonEmit = jest.fn(); + const sockets = [ + { id: 's1', data: { userId: 'user-ok' }, emit: okEmit }, + { id: 's2', data: { userId: 'user-no' }, emit: noEmit }, + // Unauthenticated socket (no userId) — must also receive the delete. + { id: 's3', data: {}, emit: anonEmit }, + ]; + server.in.mockReturnValue({ + fetchSockets: jest.fn().mockResolvedValue(sockets), + }); + + const data = { operation: 'deleteTreeNode' }; + await service.emitDeleteToUnauthorized('space-1', 'page-1', data); + + // Authorized user does NOT get the delete (they got the move instead). + expect(okEmit).not.toHaveBeenCalled(); + // Unauthorized + anonymous sockets DO get the delete. + expect(noEmit).toHaveBeenCalledWith('message', data); + expect(anonEmit).toHaveBeenCalledWith('message', data); + }); }); diff --git a/apps/server/src/ws/ws-tree.service.ts b/apps/server/src/ws/ws-tree.service.ts index dd5bdb71..98614219 100644 --- a/apps/server/src/ws/ws-tree.service.ts +++ b/apps/server/src/ws/ws-tree.service.ts @@ -1,5 +1,6 @@ import { Injectable } from '@nestjs/common'; import { Page } from '@docmost/db/types/entity.types'; +import { PagePermissionRepo } from '@docmost/db/repos/page/page-permission.repo'; import { WsService } from './ws.service'; import { PageMovedEvent, @@ -8,7 +9,10 @@ import { @Injectable() export class WsTreeService { - constructor(private readonly wsService: WsService) {} + constructor( + private readonly wsService: WsService, + private readonly pagePermissionRepo: PagePermissionRepo, + ) {} // Server-origin tree broadcasts. Built from thin node snapshots carried in the // domain events (variant A) so no DB read happens here — this avoids the @@ -56,7 +60,8 @@ export class WsTreeService { async broadcastPageMoved(event: PageMovedEvent): Promise { const { node } = event; - await this.wsService.emitTreeEvent(node.spaceId, node.id, { + + const movePayload = { operation: 'moveTreeNode', spaceId: node.spaceId, payload: { @@ -77,6 +82,57 @@ export class WsTreeService { hasChildren: event.hasChildren, }, }, + }; + + // Decide the node's restricted state ONCE, fresh (uncached), and drive BOTH + // the move broadcast and the compensating delete from this single decision. + // + // Why not just emitTreeEvent for the move? emitTreeEvent gates the move on + // the CACHED spaceHasRestrictions (30s TTL, never invalidated). In the window + // right after a space gets its FIRST restriction, that cache still says + // "no restrictions" → emitTreeEvent would fan the move out to the WHOLE room + // (including unauthorized users) while the delete below (computed from the + // UNCACHED hasRestrictedAncestor) also fires. An unauthorized user then gets + // BOTH, and if the delete lands first it is a no-op and the later move + // renders the restricted node → leak. So when the node is known-restricted we + // must NOT route the move through the cache-gated path. + const isRestricted = await this.pagePermissionRepo.hasRestrictedAncestor( + node.id, + ); + + if (!isRestricted) { + // Normal case: not under a restricted ancestor. One moveTreeNode to the + // whole space room (emitTreeEvent's open-space fast path), no delete. + await this.wsService.emitTreeEvent(node.spaceId, node.id, movePayload); + return; + } + + // Restricted case: a move can push a previously-visible page UNDER a + // restricted ancestor. Route the move to authorized users ONLY (same fresh + // getUserIdsWithPageAccess set the delete uses) and send the compensating + // delete to everyone else. Both sets come from one fresh decision, so they + // are guaranteed disjoint: authorized users get exactly the moveTreeNode, + // unauthorized users get exactly the deleteTreeNode, nobody gets both. + // + // Users who LOSE visibility need the delete because otherwise the node would + // linger in their tree at its old parent with its real title/slugId/icon + // (existence + metadata leak). + await this.wsService.emitToAuthorizedUsers( + node.spaceId, + node.id, + movePayload, + ); + + await this.wsService.emitDeleteToUnauthorized(node.spaceId, node.id, { + operation: 'deleteTreeNode', + spaceId: node.spaceId, + payload: { + node: { + id: node.id, + slugId: node.slugId, + parentPageId: event.oldParentId ?? null, + }, + }, }); } diff --git a/apps/server/src/ws/ws.service.ts b/apps/server/src/ws/ws.service.ts index bfa421c6..0c23bcef 100644 --- a/apps/server/src/ws/ws.service.ts +++ b/apps/server/src/ws/ws.service.ts @@ -127,6 +127,62 @@ export class WsService { this.server.to(getSpaceRoomName(spaceId)).emit('message', data); } + // Broadcast `data` (a deleteTreeNode) to every socket in the space room whose + // user is NOT authorized to see `pageId`. Used to compensate a move that pushes + // a previously-visible page UNDER a restricted ancestor: authorized users get + // the moveTreeNode (via emitTreeEvent), everyone else gets a deleteTreeNode so + // the now-restricted node disappears from their tree instead of lingering with + // its real title/slugId/icon. The two event sets are disjoint by construction + // (a user is either authorized or not), so no socket receives both. + async emitDeleteToUnauthorized( + spaceId: string, + pageId: string, + data: any, + ): Promise { + const room = getSpaceRoomName(spaceId); + const sockets = await this.server.in(room).fetchSockets(); + if (sockets.length === 0) return; + + const userIds = Array.from( + new Set( + sockets + .map((s) => s.data.userId as string) + .filter((id): id is string => !!id), + ), + ); + if (userIds.length === 0) return; + + const authorizedUserIds = + await this.pagePermissionRepo.getUserIdsWithPageAccess(pageId, userIds); + const authorizedSet = new Set(authorizedUserIds); + + for (const socket of sockets) { + const userId = socket.data.userId as string; + // Unauthenticated sockets (no userId) cannot see restricted content; send + // them the delete too so a leaked node can't linger. + if (!userId || !authorizedSet.has(userId)) { + socket.emit('message', data); + } + } + } + + // Server-origin broadcast of `data` to exactly the users in the space room who + // ARE authorized to see `pageId`. This is the counterpart of + // emitDeleteToUnauthorized: both resolve the authorized set from the SAME + // fetchSockets + getUserIdsWithPageAccess call shape, so a caller that drives + // both from one decision gets two disjoint sets (authorized vs. not) with no + // socket in both. Unlike emitTreeEvent, this does NOT consult the cached + // spaceHasRestrictions: the caller already knows the page is restricted, so we + // must not risk a stale cache fanning the move out to the whole room. + async emitToAuthorizedUsers( + spaceId: string, + pageId: string, + data: any, + ): Promise { + const room = getSpaceRoomName(spaceId); + await this.broadcastToAuthorizedUsers(room, null, pageId, data); + } + async emitToUsers(userIds: string[], data: any): Promise { if (userIds.length === 0) return; const rooms = userIds.map((id) => getUserRoomName(id));