diff --git a/apps/server/src/collaboration/extensions/authentication.extension.ts b/apps/server/src/collaboration/extensions/authentication.extension.ts index ab12ed77c..4045601db 100644 --- a/apps/server/src/collaboration/extensions/authentication.extension.ts +++ b/apps/server/src/collaboration/extensions/authentication.extension.ts @@ -72,7 +72,7 @@ export class AuthenticationExtension implements Extension { // Check page-level permissions const { hasAnyRestriction, canAccess, canEdit } = - await this.pagePermissionRepo.getUserPageAccessLevel(user.id, page.id); + await this.pagePermissionRepo.canUserEditPage(user.id, page.id); if (hasAnyRestriction) { if (!canAccess) { diff --git a/apps/server/src/core/page-access/page-access.service.ts b/apps/server/src/core/page-access/page-access.service.ts index ddac98111..893b8486d 100644 --- a/apps/server/src/core/page-access/page-access.service.ts +++ b/apps/server/src/core/page-access/page-access.service.ts @@ -28,16 +28,13 @@ export class PageAccessService { throw new ForbiddenException(); } - const { hasAnyRestriction, canAccess } = - await this.pagePermissionRepo.getUserPageAccessLevel(user.id, page.id); - - if (hasAnyRestriction) { - // Page has restrictions - use page-level permission - if (!canAccess) { - throw new ForbiddenException(); - } + const canAccess = await this.pagePermissionRepo.canUserAccessPage( + user.id, + page.id, + ); + if (!canAccess) { + throw new ForbiddenException(); } - // No restriction - space membership (checked above) is sufficient for view } /** @@ -54,7 +51,7 @@ export class PageAccessService { } const { hasAnyRestriction, canEdit } = - await this.pagePermissionRepo.getUserPageAccessLevel(user.id, page.id); + await this.pagePermissionRepo.canUserEditPage(user.id, page.id); if (hasAnyRestriction) { // Page has restrictions - use page-level permission diff --git a/apps/server/src/core/page/services/page-permission.service.ts b/apps/server/src/core/page/services/page-permission.service.ts index 3b35a2fb8..5edf2563c 100644 --- a/apps/server/src/core/page/services/page-permission.service.ts +++ b/apps/server/src/core/page/services/page-permission.service.ts @@ -229,27 +229,34 @@ export class PagePermissionService { const userIds = dto.userIds ?? []; const groupIds = dto.groupIds ?? []; - if (userIds.length > 0) { - await this.pagePermissionRepo.deletePagePermissionsByUserIds( - pageAccess.id, - userIds, - ); - } + await executeTx(this.db, async (trx) => { + if (userIds.length > 0) { + await this.pagePermissionRepo.deletePagePermissionsByUserIds( + pageAccess.id, + userIds, + trx, + ); + } - if (groupIds.length > 0) { - await this.pagePermissionRepo.deletePagePermissionsByGroupIds( - pageAccess.id, - groupIds, - ); - } + if (groupIds.length > 0) { + await this.pagePermissionRepo.deletePagePermissionsByGroupIds( + pageAccess.id, + groupIds, + trx, + ); + } - const writerCount = - await this.pagePermissionRepo.countWritersByPageAccessId(pageAccess.id); - if (writerCount < 1) { - throw new BadRequestException( - 'There must be at least one user with "Can edit" permission', - ); - } + const writerCount = + await this.pagePermissionRepo.countWritersByPageAccessId( + pageAccess.id, + trx, + ); + if (writerCount < 1) { + throw new BadRequestException( + 'There must be at least one user with "Can edit" permission', + ); + } + }); } async updatePagePermissionRole( @@ -486,7 +493,10 @@ export class PagePermissionService { throw new ForbiddenException(); } - const canEdit = await this.canEditPage(user.id, page.id); + const { canAccess, canEdit } = await this.canEditPage(user.id, page.id); + if (!canAccess) { + throw new ForbiddenException(); + } if (canEdit) { return; } @@ -517,18 +527,22 @@ export class PagePermissionService { } /** - * Check if user can edit a page. - * User must have WRITER permission on EVERY restricted ancestor. - * Returns true if: - * - No ancestors are restricted (defer to space permission) - * - User has writer permission on all restricted ancestors + * Check if user can edit a page based on page-level permissions. + * Returns { hasAnyRestriction, canAccess, canEdit } from the nearest restricted ancestor logic. */ - async canEditPage(userId: string, pageId: string): Promise { + async canEditPage( + userId: string, + pageId: string, + ): Promise<{ + hasAnyRestriction: boolean; + canAccess: boolean; + canEdit: boolean; + }> { return this.pagePermissionRepo.canUserEditPage(userId, pageId); } /** - * Check if user has writer permission on ALL restricted ancestors of a page. + * Check if user has writer permission on the nearest restricted ancestor. * Used for permission management operations. */ async hasWritePermission(userId: string, pageId: string): Promise { @@ -539,7 +553,11 @@ export class PagePermissionService { return false; // no restrictions, defer to space permissions } - return this.pagePermissionRepo.canUserEditPage(userId, pageId); + const { canEdit } = await this.pagePermissionRepo.canUserEditPage( + userId, + pageId, + ); + return canEdit; } async hasPageAccess(pageId: string): Promise { 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 f4fa5ecbf..2a972c42c 100644 --- a/apps/server/src/database/repos/page/page-permission.repo.ts +++ b/apps/server/src/database/repos/page/page-permission.repo.ts @@ -393,54 +393,60 @@ export class PagePermissionRepo { } /** - * Check if user can edit a page by verifying they have WRITER permission on ALL restricted ancestors. + * Check if user can edit a page. + * Single query: builds ancestor chain once, checks both traversal and nearest-restricted writer. + * - bool_and(pp.id IS NOT NULL): false if any restricted ancestor has no permission (traversal denied) + * - array_agg(role ORDER BY depth)[1]: role on the nearest restricted ancestor + * - Zero rows (no restricted ancestors): both NULL → defer to space permissions (true) */ - async canUserEditPage(userId: string, pageId: string): Promise { - const deniedAncestor = await this.db - .withRecursive('ancestors', (qb) => - qb - .selectFrom('pages') - .select(['pages.id as ancestorId', 'pages.parentPageId']) - .where('pages.id', '=', pageId) - .unionAll((eb) => - eb - .selectFrom('pages') - .innerJoin('ancestors', 'ancestors.parentPageId', 'pages.id') - .select(['pages.id as ancestorId', 'pages.parentPageId']), - ), + async canUserEditPage( + userId: string, + pageId: string, + ): Promise<{ hasAnyRestriction: boolean; canAccess: boolean; canEdit: boolean }> { + const result = await sql<{ canAccess: boolean | null; canEdit: boolean | null }>` + WITH RECURSIVE ancestors AS ( + SELECT id AS ancestor_id, parent_page_id, 0 AS depth + FROM pages + WHERE id = ${pageId}::uuid + UNION ALL + SELECT p.id, p.parent_page_id, a.depth + 1 + FROM pages p + JOIN ancestors a ON a.parent_page_id = p.id ) - .selectFrom('ancestors') - .innerJoin('pageAccess', 'pageAccess.pageId', 'ancestors.ancestorId') - .leftJoin('pagePermissions', (join) => - join - .onRef('pagePermissions.pageAccessId', '=', 'pageAccess.id') - .on('pagePermissions.role', '=', 'writer') - .on((eb) => - eb.or([ - eb('pagePermissions.userId', '=', userId), - eb( - 'pagePermissions.groupId', - 'in', - this.userGroupIdsSubquery(eb, userId), - ), - ]), - ), - ) - .select('pageAccess.pageId') - .where('pagePermissions.id', 'is', null) - .executeTakeFirst(); + SELECT + 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" + FROM ancestors a + JOIN page_access pa ON pa.page_id = a.ancestor_id + LEFT JOIN page_permissions pp ON pp.page_access_id = pa.id + AND ( + pp.user_id = ${userId}::uuid + OR pp.group_id IN ( + SELECT gu.group_id FROM group_users gu WHERE gu.user_id = ${userId}::uuid + ) + ) + `.execute(this.db); - return !deniedAncestor; + const row = result.rows[0]; + if (!row || row.canAccess === null) { + return { hasAnyRestriction: false, canAccess: true, canEdit: true }; + } + return { + hasAnyRestriction: true, + canAccess: row.canAccess, + canEdit: row.canAccess && (row.canEdit ?? false), + }; } /** - * Get user's access level for a page, checking ALL restricted ancestors. + * Get user's access level for a page. * Returns: * - hasDirectRestriction: whether this specific page has restrictions * - hasInheritedRestriction: whether any ancestor (not self) has restrictions * - hasAnyRestriction: hasDirectRestriction || hasInheritedRestriction * - canAccess: user has permission on all restricted ancestors (always true if no restrictions) - * - canEdit: user has writer permission on all restricted ancestors (always true if no restrictions) + * - canEdit: user has writer on nearest restricted ancestor (always true if no restrictions) */ async getUserPageAccessLevel( userId: string, @@ -550,9 +556,43 @@ export class PagePermissionRepo { .else(false) .end() .as('canAccess'), - // canEdit: no restricted ancestor without WRITER permission + // canEdit: nearest restricted ancestor determines edit capability eb .case() + // traversal denied: any restricted ancestor without any permission + .when( + eb.exists( + eb + .selectFrom('ancestors') + .innerJoin( + 'pageAccess', + 'pageAccess.pageId', + 'ancestors.ancestorId', + ) + .leftJoin('pagePermissions', (join) => + join + .onRef( + 'pagePermissions.pageAccessId', + '=', + 'pageAccess.id', + ) + .on((eb2) => + eb2.or([ + eb2('pagePermissions.userId', '=', userId), + eb2( + 'pagePermissions.groupId', + 'in', + this.userGroupIdsSubquery(eb2, userId), + ), + ]), + ), + ) + .select('pageAccess.pageId') + .where('pagePermissions.id', 'is', null), + ), + ) + .then(false) + // no restricted ancestors at all → defer to space permissions .when( eb.not( eb.exists( @@ -563,31 +603,41 @@ export class PagePermissionRepo { 'pageAccess.pageId', 'ancestors.ancestorId', ) - .leftJoin('pagePermissions', (join) => - join - .onRef( - 'pagePermissions.pageAccessId', - '=', - 'pageAccess.id', - ) - .on('pagePermissions.role', '=', 'writer') - .on((eb2) => - eb2.or([ - eb2('pagePermissions.userId', '=', userId), - eb2( - 'pagePermissions.groupId', - 'in', - this.userGroupIdsSubquery(eb2, userId), - ), - ]), - ), - ) - .select('pageAccess.pageId') - .where('pagePermissions.id', 'is', null), + .select('pageAccess.id'), ), ), ) .then(true) + // nearest restricted ancestor has writer for this user + .when( + eb.exists( + eb + .selectFrom('pagePermissions') + .select('pagePermissions.id') + .where('pagePermissions.role', '=', 'writer') + .where( + 'pagePermissions.pageAccessId', + '=', + sql`( + SELECT pa.id FROM ancestors a_nr + JOIN page_access pa ON pa.page_id = a_nr.ancestor_id + ORDER BY a_nr.depth ASC + LIMIT 1 + )`, + ) + .where((eb2) => + eb2.or([ + eb2('pagePermissions.userId', '=', userId), + eb2( + 'pagePermissions.groupId', + 'in', + this.userGroupIdsSubquery(eb2, userId), + ), + ]), + ), + ), + ) + .then(true) .else(false) .end() .as('canEdit'), @@ -703,6 +753,7 @@ export class PagePermissionRepo { 'pages.id as pageId', 'pages.id as ancestorId', 'pages.parentPageId', + sql`0`.as('depth'), ]) .where(sql`pages.id = ANY(${pageIds}::uuid[])`) .unionAll((eb) => @@ -717,6 +768,7 @@ export class PagePermissionRepo { 'allAncestors.pageId', 'pages.id as ancestorId', 'pages.parentPageId', + sql`"allAncestors".depth + 1`.as('depth'), ]), ), ) @@ -725,6 +777,7 @@ export class PagePermissionRepo { .select((eb) => eb .case() + // no restricted ancestors for this page → defer to space .when( eb.not( eb.exists( @@ -735,37 +788,49 @@ export class PagePermissionRepo { 'pageAccess.pageId', 'allAncestors.ancestorId', ) - .leftJoin('pagePermissions', (join) => - join - .onRef( - 'pagePermissions.pageAccessId', - '=', - 'pageAccess.id', - ) - .on('pagePermissions.role', '=', 'writer') - .on((eb2) => - eb2.or([ - eb2('pagePermissions.userId', '=', userId), - eb2( - 'pagePermissions.groupId', - 'in', - this.userGroupIdsSubquery(eb2, userId), - ), - ]), - ), - ) - .select('pageAccess.pageId') - .whereRef('allAncestors.pageId', '=', 'pages.id') - .where('pagePermissions.id', 'is', null), + .select('pageAccess.id') + .whereRef('allAncestors.pageId', '=', 'pages.id'), ), ), ) .then(true) + // nearest restricted ancestor has writer for this user + .when( + eb.exists( + eb + .selectFrom('pagePermissions') + .select('pagePermissions.id') + .where('pagePermissions.role', '=', 'writer') + .where( + 'pagePermissions.pageAccessId', + '=', + sql`( + SELECT pa.id FROM "allAncestors" aa + JOIN page_access pa ON pa.page_id = aa.ancestor_id + WHERE aa.page_id = pages.id + ORDER BY aa.depth ASC + LIMIT 1 + )`, + ) + .where((eb2) => + eb2.or([ + eb2('pagePermissions.userId', '=', userId), + eb2( + 'pagePermissions.groupId', + 'in', + this.userGroupIdsSubquery(eb2, userId), + ), + ]), + ), + ), + ) + .then(true) .else(false) .end() .as('canEdit'), ) .where(sql`pages.id = ANY(${pageIds}::uuid[])`) + // view filter: no restricted ancestor without any permission .where(({ not, exists, selectFrom }) => not( exists(