fix: isolate cycles in bulk permission traversal

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<string[]>`ARRAY[pages.id]::uuid[]`.as('traversalPath'),
+            sql<boolean>`false`.as('isCycle'),
           ])
           .where(sql<SqlBool>`pages.id = ANY(${pageIds}::uuid[])`)
           .unionAll((eb) =>
@@ -754,12 +756,29 @@ export class PagePermissionRepo {
                 'allAncestors.pageId',
                 'pages.id as ancestorId',
                 'pages.parentPageId',
-              ]),
+                sql<string[]>`all_ancestors.traversal_path || pages.id`.as(
+                  'traversalPath',
+                ),
+                sql<boolean>`pages.id = ANY(all_ancestors.traversal_path)`.as(
+                  'isCycle',
+                ),
+              ])
+              .where('allAncestors.isCycle', '=', false),
           ),
       )
       .selectFrom('pages')
       .select('pages.id')
       .where(sql<SqlBool>`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<number>`0`.as('depth'),
+            sql<string[]>`ARRAY[pages.id]::uuid[]`.as('traversalPath'),
+            sql<boolean>`false`.as('isCycle'),
           ])
           .where(sql<SqlBool>`pages.id = ANY(${pageIds}::uuid[])`)
           .unionAll((eb) =>
@@ -824,7 +845,14 @@ export class PagePermissionRepo {
                 'pages.id as ancestorId',
                 'pages.parentPageId',
                 sql<number>`all_ancestors.depth + 1`.as('depth'),
-              ]),
+                sql<string[]>`all_ancestors.traversal_path || pages.id`.as(
+                  'traversalPath',
+                ),
+                sql<boolean>`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<SqlBool>`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<string[]>`ARRAY[child.id]::uuid[]`.as('traversalPath'),
+            sql<boolean>`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<string[]>`child_ancestors.traversal_path || pages.id`.as(
+                  'traversalPath',
+                ),
+                sql<boolean>`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(

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(
+        shareService.getShareForPage(grandchild.id, storedShare.workspaceId),
-          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,
+          })),
+        );
+      });
+    });
+  });
 });