From 82011fb417a90cf7fa93f7e0e973cdb0ca4edd0e Mon Sep 17 00:00:00 2001 From: Philipinho <16838612+Philipinho@users.noreply.github.com> Date: Wed, 2 Sep 2026 05:35:38 +0100 Subject: [PATCH] fix: isolate cycles in bulk permission traversal --- .../repos/page/page-permission.repo.ts | 63 +++++++++++++- .../page-hierarchy-cycle.integration-spec.ts | 87 ++++++++++++++++++- 2 files changed, 143 insertions(+), 7 deletions(-) diff --git a/apps/server/src/database/repos/page/page-permission.repo.ts b/apps/server/src/database/repos/page/page-permission.repo.ts index d8105ddc5..4fbad3563 100644 --- a/apps/server/src/database/repos/page/page-permission.repo.ts +++ b/apps/server/src/database/repos/page/page-permission.repo.ts @@ -740,6 +740,8 @@ export class PagePermissionRepo { 'pages.id as pageId', 'pages.id as ancestorId', 'pages.parentPageId', + sql`ARRAY[pages.id]::uuid[]`.as('traversalPath'), + sql`false`.as('isCycle'), ]) .where(sql`pages.id = ANY(${pageIds}::uuid[])`) .unionAll((eb) => @@ -754,12 +756,29 @@ export class PagePermissionRepo { 'allAncestors.pageId', 'pages.id as ancestorId', 'pages.parentPageId', - ]), + sql`all_ancestors.traversal_path || pages.id`.as( + 'traversalPath', + ), + sql`pages.id = ANY(all_ancestors.traversal_path)`.as( + 'isCycle', + ), + ]) + .where('allAncestors.isCycle', '=', false), ), ) .selectFrom('pages') .select('pages.id') .where(sql`pages.id = ANY(${pageIds}::uuid[])`) + .where(({ not, exists, selectFrom }) => + not( + exists( + selectFrom('allAncestors') + .select('allAncestors.ancestorId') + .whereRef('allAncestors.pageId', '=', 'pages.id') + .where('allAncestors.isCycle', '=', true), + ), + ), + ) .where(({ not, exists, selectFrom }) => not( exists( @@ -809,6 +828,8 @@ export class PagePermissionRepo { 'pages.id as ancestorId', 'pages.parentPageId', sql`0`.as('depth'), + sql`ARRAY[pages.id]::uuid[]`.as('traversalPath'), + sql`false`.as('isCycle'), ]) .where(sql`pages.id = ANY(${pageIds}::uuid[])`) .unionAll((eb) => @@ -824,7 +845,14 @@ export class PagePermissionRepo { 'pages.id as ancestorId', 'pages.parentPageId', sql`all_ancestors.depth + 1`.as('depth'), - ]), + sql`all_ancestors.traversal_path || pages.id`.as( + 'traversalPath', + ), + sql`pages.id = ANY(all_ancestors.traversal_path)`.as( + 'isCycle', + ), + ]) + .where('allAncestors.isCycle', '=', false), ), ) .selectFrom('pages') @@ -885,6 +913,16 @@ export class PagePermissionRepo { .as('canEdit'), ) .where(sql`pages.id = ANY(${pageIds}::uuid[])`) + .where(({ not, exists, selectFrom }) => + not( + exists( + selectFrom('allAncestors') + .select('allAncestors.ancestorId') + .whereRef('allAncestors.pageId', '=', 'pages.id') + .where('allAncestors.isCycle', '=', true), + ), + ), + ) // view filter: no restricted ancestor without any permission .where(({ not, exists, selectFrom }) => not( @@ -1003,6 +1041,8 @@ export class PagePermissionRepo { 'child.id as childId', 'child.id as ancestorId', 'child.parentPageId as ancestorParentId', + sql`ARRAY[child.id]::uuid[]`.as('traversalPath'), + sql`false`.as('isCycle'), ]) .where('child.parentPageId', 'in', parentIds) .where('child.deletedAt', 'is', null) @@ -1018,7 +1058,14 @@ export class PagePermissionRepo { 'childAncestors.childId', 'pages.id as ancestorId', 'pages.parentPageId as ancestorParentId', - ]), + sql`child_ancestors.traversal_path || pages.id`.as( + 'traversalPath', + ), + sql`pages.id = ANY(child_ancestors.traversal_path)`.as( + 'isCycle', + ), + ]) + .where('childAncestors.isCycle', '=', false), ), ) .selectFrom('pages as child') @@ -1026,6 +1073,16 @@ export class PagePermissionRepo { .distinct() .where('child.parentPageId', 'in', parentIds) .where('child.deletedAt', 'is', null) + .where(({ not, exists, selectFrom }) => + not( + exists( + selectFrom('childAncestors') + .select('childAncestors.ancestorId') + .whereRef('childAncestors.childId', '=', 'child.id') + .where('childAncestors.isCycle', '=', true), + ), + ), + ) .where(({ not, exists, selectFrom }) => not( exists( diff --git a/apps/server/test/page-hierarchy-cycle.integration-spec.ts b/apps/server/test/page-hierarchy-cycle.integration-spec.ts index 5938f34e8..4b192f6b3 100644 --- a/apps/server/test/page-hierarchy-cycle.integration-spec.ts +++ b/apps/server/test/page-hierarchy-cycle.integration-spec.ts @@ -8,6 +8,7 @@ import { KyselyDB } from '../src/database/types/kysely.types'; import { db, withStatementTimeout } from './support/database'; import { seedAcyclicPageChain, + seedBranchingDescendantTree, seedSelfCycle, seedTwoPageCycle, } from './support/page-hierarchy-fixtures'; @@ -269,10 +270,7 @@ describe('cycle-safe page hierarchy reads', () => { const shareService = createShareService(db); await expect( - shareService.getShareForPage( - grandchild.id, - storedShare.workspaceId, - ), + shareService.getShareForPage(grandchild.id, storedShare.workspaceId), ).resolves.toBeUndefined(); }); @@ -474,4 +472,85 @@ describe('cycle-safe page hierarchy reads', () => { }); }); }); + + describe('bulk permissions', () => { + it('keeps an accessible acyclic page while excluding a cyclic page', async () => { + const { grandchild } = await seedAcyclicPageChain(); + const { a } = await seedTwoPageCycle(); + const userId = await insertTestUser(grandchild.id); + + await withStatementTimeout(async (connection) => { + const repo = createPagePermissionRepo(connection); + + await expect( + repo.filterAccessiblePageIds({ + pageIds: [grandchild.id, a.id], + userId, + }), + ).resolves.toEqual([grandchild.id]); + }); + }); + + it('keeps acyclic permission details while excluding a cyclic page', async () => { + const { grandchild } = await seedAcyclicPageChain(); + const { a } = await seedTwoPageCycle(); + const userId = await insertTestUser(grandchild.id); + + await withStatementTimeout(async (connection) => { + const repo = createPagePermissionRepo(connection); + + await expect( + repo.filterAccessiblePageIdsWithPermissions( + [grandchild.id, a.id], + userId, + ), + ).resolves.toEqual([{ id: grandchild.id, canEdit: true }]); + }); + }); + + it('keeps a parent with an accessible child while excluding a cyclic child', async () => { + const { root, child } = await seedAcyclicPageChain(); + const { a } = await seedTwoPageCycle(); + const userId = await insertTestUser(child.id); + + await withStatementTimeout(async (connection) => { + const repo = createPagePermissionRepo(connection); + + await expect( + repo.getParentIdsWithAccessibleChildren([root.id, a.id], userId), + ).resolves.toEqual([root.id]); + }); + }); + + it('keeps independent acyclic seeds that share ancestors accessible', async () => { + const { firstChild, secondChild, grandchild } = + await seedBranchingDescendantTree(); + const userId = await insertTestUser(grandchild.id); + const pageIds = [firstChild.id, secondChild.id, grandchild.id]; + const expectedPageIds = [...pageIds].sort(); + + await withStatementTimeout(async (connection) => { + const repo = createPagePermissionRepo(connection); + + const accessiblePageIds = await repo.filterAccessiblePageIds({ + pageIds, + userId, + }); + const permissionDetails = + await repo.filterAccessiblePageIdsWithPermissions(pageIds, userId); + + expect(accessiblePageIds.sort()).toEqual(expectedPageIds); + expect( + permissionDetails + .map(({ id, canEdit }) => ({ id, canEdit })) + .sort((left, right) => left.id.localeCompare(right.id)), + ).toEqual( + expectedPageIds.map((id) => ({ + id, + canEdit: true, + })), + ); + }); + }); + }); });