diff --git a/apps/server/src/collaboration/extensions/authentication.extension.ts b/apps/server/src/collaboration/extensions/authentication.extension.ts index 680afde0..fbcf2408 100644 --- a/apps/server/src/collaboration/extensions/authentication.extension.ts +++ b/apps/server/src/collaboration/extensions/authentication.extension.ts @@ -71,10 +71,10 @@ export class AuthenticationExtension implements Extension { } // Check page-level permissions - const { hasRestriction, canAccess, canEdit } = + const { hasAnyRestriction, canAccess, canEdit } = await this.pagePermissionRepo.getUserPageAccessLevel(user.id, page.id); - if (hasRestriction) { + if (hasAnyRestriction) { if (!canAccess) { this.logger.warn( `User ${user.id} denied page-level access to page: ${pageId}`, 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 19bea99c..ddac9811 100644 --- a/apps/server/src/core/page-access/page-access.service.ts +++ b/apps/server/src/core/page-access/page-access.service.ts @@ -28,10 +28,10 @@ export class PageAccessService { throw new ForbiddenException(); } - const { hasRestriction, canAccess } = + const { hasAnyRestriction, canAccess } = await this.pagePermissionRepo.getUserPageAccessLevel(user.id, page.id); - if (hasRestriction) { + if (hasAnyRestriction) { // Page has restrictions - use page-level permission if (!canAccess) { throw new ForbiddenException(); @@ -53,10 +53,10 @@ export class PageAccessService { throw new ForbiddenException(); } - const { hasRestriction, canEdit } = + const { hasAnyRestriction, canEdit } = await this.pagePermissionRepo.getUserPageAccessLevel(user.id, page.id); - if (hasRestriction) { + if (hasAnyRestriction) { // Page has restrictions - use page-level permission if (!canEdit) { throw new ForbiddenException(); 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 4c2f1866..4de2f44c 100644 --- a/apps/server/src/core/page/services/page-permission.service.ts +++ b/apps/server/src/core/page/services/page-permission.service.ts @@ -359,6 +359,8 @@ export class PagePermissionService { * * Security: User must be a space member. Returns 404 for pages the user cannot view * to avoid leaking existence of restricted pages. + * + * Performance: Uses single optimized query to get all restriction/access data. */ async getPageRestrictionInfo( pageId: string, @@ -378,23 +380,22 @@ export class PagePermissionService { throw new ForbiddenException(); } - const [hasDirectRestriction, hasAnyRestriction, canView, canEdit] = - await Promise.all([ - this.pagePermissionRepo.findPageAccessByPageId(pageId).then((r) => !!r), - this.pagePermissionRepo.hasRestrictedAncestor(pageId), - this.canViewPage(authUser.id, pageId), - this.canEditPage(authUser.id, pageId), - ]); + const { + hasDirectRestriction, + hasInheritedRestriction, + canAccess, + canEdit, + } = await this.pagePermissionRepo.getUserPageAccessLevel( + authUser.id, + pageId, + ); // Security: return 404 to avoid leaking existence of restricted pages - if (!canView) { - throw new NotFoundException('Page not found'); + if (!canAccess) { + throw new NotFoundException('Permission not found'); } - const hasInheritedRestriction = hasAnyRestriction && !hasDirectRestriction; - - // Determine if user can manage permissions - const canManage = this.computeCanManage(ability, canEdit, canView); + const canManage = this.computeCanManage(ability, canEdit, canAccess); return { id: page.id, @@ -402,7 +403,7 @@ export class PagePermissionService { hasDirectRestriction, hasInheritedRestriction, userAccess: { - canView, + canView: canAccess, canEdit, canManage, }, 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 77039a76..238cf860 100644 --- a/apps/server/src/database/repos/page/page-permission.repo.ts +++ b/apps/server/src/database/repos/page/page-permission.repo.ts @@ -380,7 +380,9 @@ export class PagePermissionRepo { /** * Get user's access level for a page, checking ALL restricted ancestors. * Returns: - * - hasRestriction: whether page or any ancestor has restrictions + * - 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) */ @@ -388,14 +390,31 @@ export class PagePermissionRepo { userId: string, pageId: string, ): Promise<{ - hasRestriction: boolean; + hasDirectRestriction: boolean; + hasInheritedRestriction: boolean; + hasAnyRestriction: boolean; canAccess: boolean; canEdit: boolean; }> { const result = await this.db .selectFrom('pages') .select((eb) => [ - // hasRestriction: any ancestor has page_access entry + // hasDirectRestriction: this page itself has page_access entry + eb + .case() + .when( + eb.exists( + eb + .selectFrom('pageAccess') + .select('pageAccess.id') + .whereRef('pageAccess.pageId', '=', 'pages.id'), + ), + ) + .then(true) + .else(false) + .end() + .as('hasDirectRestriction'), + // hasInheritedRestriction: any ancestor (depth > 0) has page_access entry eb .case() .when( @@ -408,13 +427,14 @@ export class PagePermissionRepo { 'pageHierarchy.ancestorId', ) .select('pageAccess.id') - .whereRef('pageHierarchy.descendantId', '=', 'pages.id'), + .whereRef('pageHierarchy.descendantId', '=', 'pages.id') + .where('pageHierarchy.depth', '>', 0), ), ) .then(true) .else(false) .end() - .as('hasRestriction'), + .as('hasInheritedRestriction'), // canAccess: no restricted ancestor without ANY permission eb .case() @@ -508,8 +528,13 @@ export class PagePermissionRepo { .where('pages.id', '=', pageId) .executeTakeFirst(); + const hasDirectRestriction = Boolean(result?.hasDirectRestriction); + const hasInheritedRestriction = Boolean(result?.hasInheritedRestriction); + return { - hasRestriction: Boolean(result?.hasRestriction), + hasDirectRestriction, + hasInheritedRestriction, + hasAnyRestriction: hasDirectRestriction || hasInheritedRestriction, canAccess: Boolean(result?.canAccess), canEdit: Boolean(result?.canEdit), };