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 f753526cd..d8105ddc5 100644 --- a/apps/server/src/database/repos/page/page-permission.repo.ts +++ b/apps/server/src/database/repos/page/page-permission.repo.ts @@ -24,6 +24,7 @@ import { CacheKey, PERMISSION_CACHE_TTL_MS, } from '../../../common/helpers/cache-keys'; +import { assertAcyclicPageTraversal } from '../../helpers/page-hierarchy-cycle'; export { PagePermissionMember } from './types/page-permission.types'; @@ -332,7 +333,7 @@ export class PagePermissionRepo { } | undefined > { - return this.db + const ancestors = await this.db .withRecursive('ancestors', (qb) => qb .selectFrom('pages') @@ -340,6 +341,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('pages.id', '=', pageId) .unionAll((eb) => @@ -350,19 +353,41 @@ export class PagePermissionRepo { 'pages.id as ancestorId', 'pages.parentPageId', sql`ancestors.depth + 1`.as('depth'), - ]), + sql`ancestors.traversal_path || pages.id`.as( + 'traversalPath', + ), + sql`pages.id = ANY(ancestors.traversal_path)`.as( + 'isCycle', + ), + ]) + .where('ancestors.isCycle', '=', false), ), ) .selectFrom('ancestors') - .innerJoin('pageAccess', 'pageAccess.pageId', 'ancestors.ancestorId') + .leftJoin('pageAccess', 'pageAccess.pageId', 'ancestors.ancestorId') .select([ 'pageAccess.id as pageAccessId', 'pageAccess.pageId', 'pageAccess.accessLevel', 'ancestors.depth', + 'ancestors.isCycle', ]) .orderBy('ancestors.depth', 'asc') - .executeTakeFirst(); + .execute(); + + assertAcyclicPageTraversal(ancestors, pageId); + + const restrictedAncestor = ancestors.find( + (ancestor) => ancestor.pageAccessId !== null, + ); + if (!restrictedAncestor) return undefined; + + return { + pageAccessId: restrictedAncestor.pageAccessId, + pageId: restrictedAncestor.pageId, + accessLevel: restrictedAncestor.accessLevel, + depth: restrictedAncestor.depth, + }; } /** @@ -396,17 +421,30 @@ export class PagePermissionRepo { const result = await sql<{ canAccess: boolean | null; canEdit: boolean | null; + hasHierarchyCycle: boolean | null; }>` WITH RECURSIVE ancestors AS ( - SELECT id AS ancestor_id, parent_page_id, 0 AS depth + SELECT + id AS ancestor_id, + parent_page_id, + 0 AS depth, + ARRAY[id]::uuid[] AS traversal_path, + false AS is_cycle FROM pages WHERE id = ${pageId}::uuid UNION ALL - SELECT p.id, p.parent_page_id, a.depth + 1 + SELECT + p.id, + p.parent_page_id, + a.depth + 1, + a.traversal_path || p.id, + p.id = ANY(a.traversal_path) AS is_cycle FROM pages p JOIN ancestors a ON a.parent_page_id = p.id + WHERE NOT a.is_cycle ) SELECT + (SELECT bool_or(is_cycle) FROM ancestors) AS "hasHierarchyCycle", bool_and(pp.id IS NOT NULL) AS "canAccess", -- nearest restricted ancestor's highest role wins (DESC: 'writer' > 'reader', NULLS LAST: no-permission after real roles) (array_agg(pp.role ORDER BY a.depth ASC, pp.role DESC NULLS LAST))[1] = 'writer' AS "canEdit" @@ -422,6 +460,13 @@ export class PagePermissionRepo { `.execute(this.db); const row = result.rows[0]; + if (row?.hasHierarchyCycle) { + return { + hasAnyRestriction: true, + canAccess: false, + canEdit: false, + }; + } if (!row || row.canAccess === null) { return { hasAnyRestriction: false, canAccess: true, canEdit: true }; } @@ -461,6 +506,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('pages.id', '=', pageId) .unionAll((eb) => @@ -471,7 +518,14 @@ export class PagePermissionRepo { 'pages.id as ancestorId', 'pages.parentPageId', sql`ancestors.depth + 1`.as('depth'), - ]), + sql`ancestors.traversal_path || pages.id`.as( + 'traversalPath', + ), + sql`pages.id = ANY(ancestors.traversal_path)`.as( + 'isCycle', + ), + ]) + .where('ancestors.isCycle', '=', false), ), ) .selectFrom('pages') @@ -511,6 +565,14 @@ export class PagePermissionRepo { .else(false) .end() .as('hasInheritedRestriction'), + eb + .exists( + eb + .selectFrom('ancestors') + .select('ancestors.ancestorId') + .where('ancestors.isCycle', '=', true), + ) + .as('hasHierarchyCycle'), // canAccess: no restricted ancestor without ANY permission eb .case() @@ -638,13 +700,15 @@ export class PagePermissionRepo { const hasDirectRestriction = Boolean(result?.hasDirectRestriction); const hasInheritedRestriction = Boolean(result?.hasInheritedRestriction); + const hasHierarchyCycle = Boolean(result?.hasHierarchyCycle); return { hasDirectRestriction, hasInheritedRestriction, - hasAnyRestriction: hasDirectRestriction || hasInheritedRestriction, - canAccess: Boolean(result?.canAccess), - canEdit: Boolean(result?.canEdit), + hasAnyRestriction: + hasDirectRestriction || hasInheritedRestriction || hasHierarchyCycle, + canAccess: !hasHierarchyCycle && Boolean(result?.canAccess), + canEdit: !hasHierarchyCycle && Boolean(result?.canEdit), }; } @@ -865,21 +929,39 @@ export class PagePermissionRepo { .withRecursive('ancestors', (qb) => qb .selectFrom('pages') - .select(['pages.id as ancestorId', 'pages.parentPageId']) + .select([ + 'pages.id as ancestorId', + 'pages.parentPageId', + sql`ARRAY[pages.id]::uuid[]`.as('traversalPath'), + sql`false`.as('isCycle'), + ]) .where('pages.id', '=', pageId) .unionAll((eb) => eb .selectFrom('pages') .innerJoin('ancestors', 'ancestors.parentPageId', 'pages.id') - .select(['pages.id as ancestorId', 'pages.parentPageId']), + .select([ + 'pages.id as ancestorId', + 'pages.parentPageId', + sql`ancestors.traversal_path || pages.id`.as( + 'traversalPath', + ), + sql`pages.id = ANY(ancestors.traversal_path)`.as( + 'isCycle', + ), + ]) + .where('ancestors.isCycle', '=', false), ), ) .selectFrom('ancestors') - .innerJoin('pageAccess', 'pageAccess.pageId', 'ancestors.ancestorId') - .select('pageAccess.id') + .leftJoin('pageAccess', 'pageAccess.pageId', 'ancestors.ancestorId') + .select([ + sql`bool_or(ancestors.is_cycle)`.as('hasHierarchyCycle'), + sql`bool_or(page_access.id IS NOT NULL)`.as('hasPageAccess'), + ]) .executeTakeFirst(); - return !!result; + return Boolean(result?.hasHierarchyCycle || result?.hasPageAccess); } /** @@ -1052,17 +1134,27 @@ export class PagePermissionRepo { const results = await sql<{ userId: string }>` WITH RECURSIVE ancestors AS ( - SELECT id AS ancestor_id, parent_page_id + SELECT + id AS ancestor_id, + parent_page_id, + ARRAY[id]::uuid[] AS traversal_path, + false AS is_cycle FROM pages WHERE id = ${pageId}::uuid UNION ALL - SELECT p.id, p.parent_page_id + SELECT + p.id, + p.parent_page_id, + a.traversal_path || p.id, + p.id = ANY(a.traversal_path) AS is_cycle FROM pages p JOIN ancestors a ON a.parent_page_id = p.id + WHERE NOT a.is_cycle ) SELECT cu.user_id AS "userId" FROM unnest(${userIds}::uuid[]) AS cu(user_id) - WHERE NOT EXISTS ( + WHERE NOT EXISTS (SELECT 1 FROM ancestors WHERE is_cycle) + AND NOT EXISTS ( SELECT 1 FROM ancestors a JOIN page_access pa ON pa.page_id = a.ancestor_id diff --git a/apps/server/test/page-hierarchy-cycle.integration-spec.ts b/apps/server/test/page-hierarchy-cycle.integration-spec.ts index 10200cbf3..5938f34e8 100644 --- a/apps/server/test/page-hierarchy-cycle.integration-spec.ts +++ b/apps/server/test/page-hierarchy-cycle.integration-spec.ts @@ -1,7 +1,9 @@ import { randomUUID } from 'node:crypto'; +import { createCache } from 'cache-manager'; import { PageService } from '../src/core/page/services/page.service'; import { ShareService } from '../src/core/share/share.service'; import { PageHierarchyCycleError } from '../src/database/helpers/page-hierarchy-cycle'; +import { PagePermissionRepo } from '../src/database/repos/page/page-permission.repo'; import { KyselyDB } from '../src/database/types/kysely.types'; import { db, withStatementTimeout } from './support/database'; import { @@ -38,6 +40,72 @@ function createShareService(connection: KyselyDB): ShareService { ); } +function createPagePermissionRepo(connection: KyselyDB): PagePermissionRepo { + return new PagePermissionRepo(connection, undefined as never, createCache()); +} + +async function insertTestUser(pageId: string): Promise { + const { workspaceId } = await db + .selectFrom('pages') + .select('workspaceId') + .where('id', '=', pageId) + .executeTakeFirstOrThrow(); + + const user = await db + .insertInto('users') + .values({ + email: `cycle-test-${randomUUID()}@example.com`, + name: 'Page hierarchy test user', + workspaceId, + }) + .returning('id') + .executeTakeFirstOrThrow(); + + return user.id; +} + +async function restrictPage( + pageId: string, + permittedUserId?: string, +): Promise<{ + accessLevel: string; + pageAccessId: string; + pageId: string; +}> { + const page = await db + .selectFrom('pages') + .select(['spaceId', 'workspaceId']) + .where('id', '=', pageId) + .executeTakeFirstOrThrow(); + const pageAccess = await db + .insertInto('pageAccess') + .values({ + accessLevel: 'restricted', + pageId, + spaceId: page.spaceId, + workspaceId: page.workspaceId, + }) + .returning(['accessLevel', 'id', 'pageId']) + .executeTakeFirstOrThrow(); + + if (permittedUserId) { + await db + .insertInto('pagePermissions') + .values({ + pageAccessId: pageAccess.id, + role: 'writer', + userId: permittedUserId, + }) + .execute(); + } + + return { + accessLevel: pageAccess.accessLevel, + pageAccessId: pageAccess.id, + pageId: pageAccess.pageId, + }; +} + async function insertShare(pageId: string, includeSubPages: boolean) { const page = await db .selectFrom('pages') @@ -225,4 +293,185 @@ describe('cycle-safe page hierarchy reads', () => { }); }); }); + + describe('single-page permissions', () => { + it('preserves unrestricted acyclic access', async () => { + const { grandchild } = await seedAcyclicPageChain(); + const userId = await insertTestUser(grandchild.id); + const repo = createPagePermissionRepo(db); + + await expect( + repo.canUserEditPage(userId, grandchild.id), + ).resolves.toEqual({ + hasAnyRestriction: false, + canAccess: true, + canEdit: true, + }); + await expect( + repo.getUserPageAccessLevel(userId, grandchild.id), + ).resolves.toEqual({ + hasDirectRestriction: false, + hasInheritedRestriction: false, + hasAnyRestriction: false, + canAccess: true, + canEdit: true, + }); + await expect(repo.hasRestrictedAncestor(grandchild.id)).resolves.toBe( + false, + ); + await expect( + repo.getUserIdsWithPageAccess(grandchild.id, [userId]), + ).resolves.toEqual([userId]); + await expect( + repo.findRestrictedAncestor(grandchild.id), + ).resolves.toBeUndefined(); + }); + + it('preserves permitted acyclic access', async () => { + const { root, grandchild } = await seedAcyclicPageChain(); + const userId = await insertTestUser(grandchild.id); + const restriction = await restrictPage(root.id, userId); + const repo = createPagePermissionRepo(db); + + await expect( + repo.canUserEditPage(userId, grandchild.id), + ).resolves.toEqual({ + hasAnyRestriction: true, + canAccess: true, + canEdit: true, + }); + await expect( + repo.getUserPageAccessLevel(userId, grandchild.id), + ).resolves.toEqual({ + hasDirectRestriction: false, + hasInheritedRestriction: true, + hasAnyRestriction: true, + canAccess: true, + canEdit: true, + }); + await expect(repo.hasRestrictedAncestor(grandchild.id)).resolves.toBe( + true, + ); + await expect( + repo.getUserIdsWithPageAccess(grandchild.id, [userId]), + ).resolves.toEqual([userId]); + await expect(repo.findRestrictedAncestor(grandchild.id)).resolves.toEqual( + { ...restriction, depth: 2 }, + ); + }); + + it('preserves denied acyclic access', async () => { + const { root, grandchild } = await seedAcyclicPageChain(); + const userId = await insertTestUser(grandchild.id); + const restriction = await restrictPage(root.id); + const repo = createPagePermissionRepo(db); + + await expect( + repo.canUserEditPage(userId, grandchild.id), + ).resolves.toEqual({ + hasAnyRestriction: true, + canAccess: false, + canEdit: false, + }); + await expect( + repo.getUserPageAccessLevel(userId, grandchild.id), + ).resolves.toEqual({ + hasDirectRestriction: false, + hasInheritedRestriction: true, + hasAnyRestriction: true, + canAccess: false, + canEdit: false, + }); + await expect(repo.hasRestrictedAncestor(grandchild.id)).resolves.toBe( + true, + ); + await expect( + repo.getUserIdsWithPageAccess(grandchild.id, [userId]), + ).resolves.toEqual([]); + await expect(repo.findRestrictedAncestor(grandchild.id)).resolves.toEqual( + { ...restriction, depth: 2 }, + ); + }); + + describe.each([ + ['a self-cycle', async () => (await seedSelfCycle()).self.id], + ['a two-page cycle', async () => (await seedTwoPageCycle()).a.id], + ])('%s', (_cycleName, seedCycle) => { + it('fails canUserEditPage closed', async () => { + const pageId = await seedCycle(); + const userId = await insertTestUser(pageId); + + await withStatementTimeout(async (connection) => { + const repo = createPagePermissionRepo(connection); + + await expect(repo.canUserEditPage(userId, pageId)).resolves.toEqual({ + hasAnyRestriction: true, + canAccess: false, + canEdit: false, + }); + }); + }); + + it('fails getUserPageAccessLevel closed', async () => { + const pageId = await seedCycle(); + const userId = await insertTestUser(pageId); + + await withStatementTimeout(async (connection) => { + const repo = createPagePermissionRepo(connection); + + await expect( + repo.getUserPageAccessLevel(userId, pageId), + ).resolves.toEqual( + expect.objectContaining({ + hasAnyRestriction: true, + canAccess: false, + canEdit: false, + }), + ); + }); + }); + + it('treats a cycle as a restricted ancestor', async () => { + const pageId = await seedCycle(); + + await withStatementTimeout(async (connection) => { + const repo = createPagePermissionRepo(connection); + + await expect(repo.hasRestrictedAncestor(pageId)).resolves.toBe(true); + }); + }); + + it('filters every candidate user from a cycle', async () => { + const pageId = await seedCycle(); + const userId = await insertTestUser(pageId); + + await withStatementTimeout(async (connection) => { + const repo = createPagePermissionRepo(connection); + + await expect( + repo.getUserIdsWithPageAccess(pageId, [userId]), + ).resolves.toEqual([]); + }); + }); + + it('raises PageHierarchyCycleError from findRestrictedAncestor', async () => { + const pageId = await seedCycle(); + + await withStatementTimeout(async (connection) => { + const repo = createPagePermissionRepo(connection); + const restrictedAncestor = repo.findRestrictedAncestor(pageId); + + await expect(restrictedAncestor).rejects.toBeInstanceOf( + PageHierarchyCycleError, + ); + await expect(restrictedAncestor).rejects.toEqual( + expect.objectContaining({ + code: 'PAGE_HIERARCHY_CYCLE', + rootPageId: pageId, + }) satisfies Partial, + ); + }); + }); + }); + }); });