From 0c3901abf506e5ffc9a87c4543ddff048c48e8ce Mon Sep 17 00:00:00 2001 From: Philipinho <16838612+Philipinho@users.noreply.github.com> Date: Mon, 29 Dec 2025 22:13:58 +0000 Subject: [PATCH] WIP 4 --- .../extensions/authentication.extension.ts | 51 ++-- .../core/attachment/attachment.controller.ts | 8 +- .../src/core/attachment/attachment.module.ts | 3 +- .../src/core/comment/comment.controller.ts | 14 +- .../server/src/core/comment/comment.module.ts | 3 +- apps/server/src/core/core.module.ts | 2 + .../core/page-access/page-access.module.ts | 9 + .../core/page-access/page-access.service.ts | 71 +++++ apps/server/src/core/page/page.controller.ts | 89 ++++--- .../page/services/page-permission.service.ts | 39 +-- .../src/core/page/services/page.service.ts | 217 ++++++++++++--- apps/server/src/core/search/search.service.ts | 22 +- .../database/repos/group/group-user.repo.ts | 10 + .../repos/page/page-permission.repo.ts | 246 ++++++++++++++++-- .../src/database/repos/page/page.repo.ts | 56 ---- 15 files changed, 612 insertions(+), 228 deletions(-) create mode 100644 apps/server/src/core/page-access/page-access.module.ts create mode 100644 apps/server/src/core/page-access/page-access.service.ts diff --git a/apps/server/src/collaboration/extensions/authentication.extension.ts b/apps/server/src/collaboration/extensions/authentication.extension.ts index 1309c5f8..92d30461 100644 --- a/apps/server/src/collaboration/extensions/authentication.extension.ts +++ b/apps/server/src/collaboration/extensions/authentication.extension.ts @@ -70,36 +70,31 @@ export class AuthenticationExtension implements Extension { throw new UnauthorizedException(); } - if (userSpaceRole === SpaceRole.READER) { - data.connection.readOnly = true; - this.logger.debug(`User granted readonly access to page: ${pageId}`); - } + // Check page-level permissions + const { hasRestriction, canAccess, canEdit } = + await this.pagePermissionRepo.getUserPageAccessLevel(user.id, page.id); - // Check page-level permissions (in addition to space permissions) - const canAccessPage = await this.pagePermissionRepo.canUserAccessPage( - user.id, - page.id, - ); + if (hasRestriction) { + // Page has restrictions - use page-level permissions + if (!canAccess) { + this.logger.warn( + `User ${user.id} denied page-level access to page: ${pageId}`, + ); + throw new UnauthorizedException(); + } - if (!canAccessPage) { - this.logger.warn( - `User ${user.id} denied page-level access to page: ${pageId}`, - ); - throw new UnauthorizedException(); - } - - // Check if user can edit (has writer role on all restricted ancestors) - const canEditPage = await this.pagePermissionRepo.canUserEditPage( - user.id, - page.id, - ); - - // If user has space edit permission but lacks page-level write permission, force readonly - if (!canEditPage && !data.connection.readOnly) { - data.connection.readOnly = true; - this.logger.debug( - `User ${user.id} granted readonly access to restricted page: ${pageId}`, - ); + if (!canEdit) { + data.connection.readOnly = true; + this.logger.debug( + `User ${user.id} granted readonly access to restricted page: ${pageId}`, + ); + } + } else { + // No restrictions - use space-level permissions + if (userSpaceRole === SpaceRole.READER) { + data.connection.readOnly = true; + this.logger.debug(`User granted readonly access to page: ${pageId}`); + } } this.logger.debug(`Authenticated user ${user.id} on page ${pageId}`); diff --git a/apps/server/src/core/attachment/attachment.controller.ts b/apps/server/src/core/attachment/attachment.controller.ts index b3ba841c..4ca17713 100644 --- a/apps/server/src/core/attachment/attachment.controller.ts +++ b/apps/server/src/core/attachment/attachment.controller.ts @@ -53,7 +53,7 @@ import { TokenService } from '../auth/services/token.service'; import { JwtAttachmentPayload, JwtType } from '../auth/dto/jwt-payload'; import * as path from 'path'; import { RemoveIconDto } from './dto/attachment.dto'; -import { PagePermissionService } from '../page/services/page-permission.service'; +import { PageAccessService } from '../page-access/page-access.service'; @Controller() export class AttachmentController { @@ -68,7 +68,7 @@ export class AttachmentController { private readonly attachmentRepo: AttachmentRepo, private readonly environmentService: EnvironmentService, private readonly tokenService: TokenService, - private readonly pagePermissionService: PagePermissionService, + private readonly pageAccessService: PageAccessService, ) {} @UseGuards(JwtAuthGuard) @@ -114,7 +114,7 @@ export class AttachmentController { } // Checks both space-level and page-level edit permissions - await this.pagePermissionService.validateCanEdit(page, user); + await this.pageAccessService.validateCanEdit(page, user); const spaceId = page.spaceId; @@ -174,7 +174,7 @@ export class AttachmentController { } // Checks both space-level and page-level view permissions - await this.pagePermissionService.validateCanView(page, user); + await this.pageAccessService.validateCanView(page, user); try { const fileStream = await this.storageService.read(attachment.filePath); diff --git a/apps/server/src/core/attachment/attachment.module.ts b/apps/server/src/core/attachment/attachment.module.ts index b068b7dc..f80a2eb7 100644 --- a/apps/server/src/core/attachment/attachment.module.ts +++ b/apps/server/src/core/attachment/attachment.module.ts @@ -6,10 +6,9 @@ import { UserModule } from '../user/user.module'; import { WorkspaceModule } from '../workspace/workspace.module'; import { AttachmentProcessor } from './processors/attachment.processor'; import { TokenModule } from '../auth/token.module'; -import { PageModule } from '../page/page.module'; @Module({ - imports: [StorageModule, UserModule, WorkspaceModule, TokenModule, PageModule], + imports: [StorageModule, UserModule, WorkspaceModule, TokenModule], controllers: [AttachmentController], providers: [AttachmentService, AttachmentProcessor], }) diff --git a/apps/server/src/core/comment/comment.controller.ts b/apps/server/src/core/comment/comment.controller.ts index 99d6aa46..e43051ec 100644 --- a/apps/server/src/core/comment/comment.controller.ts +++ b/apps/server/src/core/comment/comment.controller.ts @@ -24,7 +24,7 @@ import { SpaceCaslSubject, } from '../casl/interfaces/space-ability.type'; import { CommentRepo } from '@docmost/db/repos/comment/comment.repo'; -import { PagePermissionService } from '../page/services/page-permission.service'; +import { PageAccessService } from '../page-access/page-access.service'; @UseGuards(JwtAuthGuard) @Controller('comments') @@ -34,7 +34,7 @@ export class CommentController { private readonly commentRepo: CommentRepo, private readonly pageRepo: PageRepo, private readonly spaceAbility: SpaceAbilityFactory, - private readonly pagePermissionService: PagePermissionService, + private readonly pageAccessService: PageAccessService, ) {} @HttpCode(HttpStatus.OK) @@ -55,7 +55,7 @@ export class CommentController { } // Check page-level edit permission (comments require edit access) - await this.pagePermissionService.validateCanEdit(page, user); + await this.pageAccessService.validateCanEdit(page, user); return this.commentService.create( { @@ -83,7 +83,7 @@ export class CommentController { // // Checks both space-level and page-level permissions - await this.pagePermissionService.validateCanView(page, user); + await this.pageAccessService.validateCanView(page, user); return this.commentService.findByPageId(page.id, pagination); } @@ -102,7 +102,7 @@ export class CommentController { } // Checks both space-level and page-level permissions - await this.pagePermissionService.validateCanView(page, user); + await this.pageAccessService.validateCanView(page, user); return comment; } @@ -121,7 +121,7 @@ export class CommentController { } // Checks both space-level and page-level edit permissions - await this.pagePermissionService.validateCanEdit(page, user); + await this.pageAccessService.validateCanEdit(page, user); return this.commentService.update(comment, dto, user); } @@ -140,7 +140,7 @@ export class CommentController { } // Check page-level edit permission first - await this.pagePermissionService.validateCanEdit(page, user); + await this.pageAccessService.validateCanEdit(page, user); const ability = await this.spaceAbility.createForUser( user, diff --git a/apps/server/src/core/comment/comment.module.ts b/apps/server/src/core/comment/comment.module.ts index a32b2fde..60a577e8 100644 --- a/apps/server/src/core/comment/comment.module.ts +++ b/apps/server/src/core/comment/comment.module.ts @@ -1,10 +1,9 @@ import { Module } from '@nestjs/common'; import { CommentService } from './comment.service'; import { CommentController } from './comment.controller'; -import { PageModule } from '../page/page.module'; @Module({ - imports: [PageModule], + imports: [], controllers: [CommentController], providers: [CommentService], exports: [CommentService], diff --git a/apps/server/src/core/core.module.ts b/apps/server/src/core/core.module.ts index f7f4f785..08f35aea 100644 --- a/apps/server/src/core/core.module.ts +++ b/apps/server/src/core/core.module.ts @@ -14,6 +14,7 @@ import { SearchModule } from './search/search.module'; import { SpaceModule } from './space/space.module'; import { GroupModule } from './group/group.module'; import { CaslModule } from './casl/casl.module'; +import { PageAccessModule } from './page-access/page-access.module'; import { DomainMiddleware } from '../common/middlewares/domain.middleware'; import { ShareModule } from './share/share.module'; @@ -29,6 +30,7 @@ import { ShareModule } from './share/share.module'; SpaceModule, GroupModule, CaslModule, + PageAccessModule, ShareModule, ], }) diff --git a/apps/server/src/core/page-access/page-access.module.ts b/apps/server/src/core/page-access/page-access.module.ts new file mode 100644 index 00000000..ac64ea49 --- /dev/null +++ b/apps/server/src/core/page-access/page-access.module.ts @@ -0,0 +1,9 @@ +import { Global, Module } from '@nestjs/common'; +import { PageAccessService } from './page-access.service'; + +@Global() +@Module({ + providers: [PageAccessService], + exports: [PageAccessService], +}) +export class PageAccessModule {} diff --git a/apps/server/src/core/page-access/page-access.service.ts b/apps/server/src/core/page-access/page-access.service.ts new file mode 100644 index 00000000..19bea99c --- /dev/null +++ b/apps/server/src/core/page-access/page-access.service.ts @@ -0,0 +1,71 @@ +import { ForbiddenException, Injectable } from '@nestjs/common'; +import { Page, User } from '@docmost/db/types/entity.types'; +import { PagePermissionRepo } from '@docmost/db/repos/page/page-permission.repo'; +import SpaceAbilityFactory from '../casl/abilities/space-ability.factory'; +import { + SpaceCaslAction, + SpaceCaslSubject, +} from '../casl/interfaces/space-ability.type'; + +@Injectable() +export class PageAccessService { + constructor( + private readonly pagePermissionRepo: PagePermissionRepo, + private readonly spaceAbility: SpaceAbilityFactory, + ) {} + + /** + * Validate user can view page, throws ForbiddenException if not. + * If page has restrictions: page-level permission determines access. + * If no restrictions: space-level permission determines access. + */ + async validateCanView(page: Page, user: User): Promise { + // TODO: cache by pageId and userId. + const ability = await this.spaceAbility.createForUser(user, page.spaceId); + + // User must be at least a space member + if (ability.cannot(SpaceCaslAction.Read, SpaceCaslSubject.Page)) { + throw new ForbiddenException(); + } + + const { hasRestriction, canAccess } = + await this.pagePermissionRepo.getUserPageAccessLevel(user.id, page.id); + + if (hasRestriction) { + // Page has restrictions - use page-level permission + if (!canAccess) { + throw new ForbiddenException(); + } + } + // No restriction - space membership (checked above) is sufficient for view + } + + /** + * Validate user can edit page, throws ForbiddenException if not. + * If page has restrictions: page-level writer permission determines access. + * If no restrictions: space-level edit permission determines access. + */ + async validateCanEdit(page: Page, user: User): Promise { + const ability = await this.spaceAbility.createForUser(user, page.spaceId); + + // User must be at least a space member + if (ability.cannot(SpaceCaslAction.Read, SpaceCaslSubject.Page)) { + throw new ForbiddenException(); + } + + const { hasRestriction, canEdit } = + await this.pagePermissionRepo.getUserPageAccessLevel(user.id, page.id); + + if (hasRestriction) { + // Page has restrictions - use page-level permission + if (!canEdit) { + throw new ForbiddenException(); + } + } else { + // No restrictions - use space-level permission + if (ability.cannot(SpaceCaslAction.Edit, SpaceCaslSubject.Page)) { + throw new ForbiddenException(); + } + } + } +} diff --git a/apps/server/src/core/page/page.controller.ts b/apps/server/src/core/page/page.controller.ts index 5bb5d871..7c3df4ae 100644 --- a/apps/server/src/core/page/page.controller.ts +++ b/apps/server/src/core/page/page.controller.ts @@ -10,7 +10,7 @@ import { UseGuards, } from '@nestjs/common'; import { PageService } from './services/page.service'; -import { PagePermissionService } from './services/page-permission.service'; +import { PageAccessService } from '../page-access/page-access.service'; import { CreatePageDto } from './dto/create-page.dto'; import { UpdatePageDto } from './dto/update-page.dto'; import { MovePageDto, MovePageToSpaceDto } from './dto/move-page.dto'; @@ -45,7 +45,7 @@ export class PageController { private readonly pageRepo: PageRepo, private readonly pageHistoryService: PageHistoryService, private readonly spaceAbility: SpaceAbilityFactory, - private readonly pagePermissionService: PagePermissionService, + private readonly pageAccessService: PageAccessService, ) {} @HttpCode(HttpStatus.OK) @@ -63,8 +63,7 @@ export class PageController { throw new NotFoundException('Page not found'); } - // Checks both space-level and page-level permissions - await this.pagePermissionService.validateCanView(page, user); + await this.pageAccessService.validateCanView(page, user); return page; } @@ -76,19 +75,23 @@ export class PageController { @AuthUser() user: User, @AuthWorkspace() workspace: Workspace, ) { - const ability = await this.spaceAbility.createForUser( - user, - createPageDto.spaceId, - ); - if (ability.cannot(SpaceCaslAction.Create, SpaceCaslSubject.Page)) { - throw new ForbiddenException(); - } - - // If creating under a parent page, check page-level edit permission if (createPageDto.parentPageId) { - const parentPage = await this.pageRepo.findById(createPageDto.parentPageId); - if (parentPage) { - await this.pagePermissionService.validateCanEdit(parentPage, user); + // Creating under a parent page - check edit permission on parent + const parentPage = await this.pageRepo.findById( + createPageDto.parentPageId, + ); + if (!parentPage || parentPage.spaceId !== createPageDto.spaceId) { + throw new NotFoundException('Parent page not found'); + } + await this.pageAccessService.validateCanEdit(parentPage, user); + } else { + // Creating at root level - require space-level permission + const ability = await this.spaceAbility.createForUser( + user, + createPageDto.spaceId, + ); + if (ability.cannot(SpaceCaslAction.Create, SpaceCaslSubject.Page)) { + throw new ForbiddenException(); } } @@ -104,8 +107,7 @@ export class PageController { throw new NotFoundException('Page not found'); } - // Checks both space-level and page-level permissions - await this.pagePermissionService.validateCanEdit(page, user); + await this.pageAccessService.validateCanEdit(page, user); return this.pageService.update(page, updatePageDto, user.id); } @@ -134,12 +136,8 @@ export class PageController { } await this.pageService.forceDelete(deletePageDto.pageId, workspace.id); } else { - // Soft delete requires page manage permissions at space level - if (ability.cannot(SpaceCaslAction.Manage, SpaceCaslSubject.Page)) { - throw new ForbiddenException(); - } - // Also check page-level edit permission - await this.pagePermissionService.validateCanEdit(page, user); + // User with edit permission can delete + await this.pageAccessService.validateCanEdit(page, user); await this.pageService.removePage( deletePageDto.pageId, @@ -162,13 +160,17 @@ export class PageController { throw new NotFoundException('Page not found'); } + //Todo: currently, this means if they are not admins, they need to add a space admin to the page, which is not possible as it was soft-deleted + // so page is virtually lost. Fix. const ability = await this.spaceAbility.createForUser(user, page.spaceId); if (ability.cannot(SpaceCaslAction.Manage, SpaceCaslSubject.Page)) { throw new ForbiddenException(); } + //TODO: can users with page level edit, but no space level edit restore pages they can edit? + // Check page-level edit permission (if restoring to a restricted ancestor) - await this.pagePermissionService.validateCanEdit(page, user); + await this.pageAccessService.validateCanEdit(page, user); await this.pageRepo.restorePage(pageIdDto.pageId, workspace.id); @@ -196,6 +198,7 @@ export class PageController { return this.pageService.getRecentSpacePages( recentPageDto.spaceId, + user.id, pagination, ); } @@ -210,6 +213,7 @@ export class PageController { @Body() pagination: PaginationOptions, @AuthUser() user: User, ) { + //TODO: should space admin see deleted pages they dont have access to? if (deletedPageDto.spaceId) { const ability = await this.spaceAbility.createForUser( user, @@ -227,7 +231,6 @@ export class PageController { } } - // TODO: scope to workspaces @HttpCode(HttpStatus.OK) @Post('/history') async getPageHistory( @@ -240,8 +243,7 @@ export class PageController { throw new NotFoundException('Page not found'); } - // Checks both space-level and page-level permissions - await this.pagePermissionService.validateCanView(page, user); + await this.pageAccessService.validateCanView(page, user); return this.pageHistoryService.findHistoryByPageId(page.id, pagination); } @@ -263,8 +265,7 @@ export class PageController { throw new NotFoundException('Page not found'); } - // Checks both space-level and page-level permissions - await this.pagePermissionService.validateCanView(page, user); + await this.pageAccessService.validateCanView(page, user); return history; } @@ -297,7 +298,12 @@ export class PageController { throw new ForbiddenException(); } - return this.pageService.getSidebarPages(spaceId, pagination, dto.pageId, user.id); + return this.pageService.getSidebarPages( + spaceId, + pagination, + dto.pageId, + user.id, + ); } @HttpCode(HttpStatus.OK) @@ -328,9 +334,10 @@ export class PageController { } // Check page-level edit permission on the source page - await this.pagePermissionService.validateCanEdit(movedPage, user); + await this.pageAccessService.validateCanEdit(movedPage, user); - return this.pageService.movePageToSpace(movedPage, dto.spaceId); + // Moves only accessible pages; inaccessible child pages become root pages in original space + return this.pageService.movePageToSpace(movedPage, dto.spaceId, user.id); } @HttpCode(HttpStatus.OK) @@ -342,7 +349,8 @@ export class PageController { } // Check page-level view permission on the source page (need to read to copy) - await this.pagePermissionService.validateCanView(copiedPage, user); + // Inaccessible child branches are automatically skipped during duplication + await this.pageAccessService.validateCanView(copiedPage, user); // If spaceId is provided, it's a copy to different space if (dto.spaceId) { @@ -382,22 +390,28 @@ export class PageController { throw new NotFoundException('Moved page not found'); } + //TODO: CAN USERS MOVE PAGES IN PORTIONS WHERE THEY HAVE BEEN GRANTED ACCESS TO? + // WHAT HAPPENS IF A PAGE WHICH MODES THE PERMISSION IS MOVED TO A DIFFERENT ROOT? + // ALSO THE EDIT CHECK BELOW WILL NOT WORK FOR USERS GRANTED EDIT WHO INITIALLY HOLD SPACE LEVEL VIEW + // ALSO, SHOULD REALLY PUT IN MIND WHAT SUCH USERS CAN DO IN TERMS OF WHERE THEY MOVE THE PAGE TO + const ability = await this.spaceAbility.createForUser( user, movedPage.spaceId, ); + if (ability.cannot(SpaceCaslAction.Edit, SpaceCaslSubject.Page)) { throw new ForbiddenException(); } // Check page-level edit permission - await this.pagePermissionService.validateCanEdit(movedPage, user); + await this.pageAccessService.validateCanEdit(movedPage, user); // If moving to a new parent, check permission on the target parent if (dto.parentPageId && dto.parentPageId !== movedPage.parentPageId) { const targetParent = await this.pageRepo.findById(dto.parentPageId); if (targetParent) { - await this.pagePermissionService.validateCanEdit(targetParent, user); + await this.pageAccessService.validateCanEdit(targetParent, user); } } @@ -412,8 +426,7 @@ export class PageController { throw new NotFoundException('Page not found'); } - // Checks both space-level and page-level permissions - await this.pagePermissionService.validateCanView(page, user); + await this.pageAccessService.validateCanView(page, user); return this.pageService.getPageBreadCrumbs(page.id); } 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 56f1bba8..48e5bf45 100644 --- a/apps/server/src/core/page/services/page-permission.service.ts +++ b/apps/server/src/core/page/services/page-permission.service.ts @@ -428,38 +428,11 @@ export class PagePermissionService { pageIds: string[], userId: string, ): Promise { - return this.pagePermissionRepo.filterAccessiblePageIds(pageIds, userId); - } - - /** - * Validate user can view page, throws ForbiddenException if not. - * Checks both space-level and page-level permissions. - */ - async validateCanView(page: Page, user: User): Promise { - const ability = await this.spaceAbility.createForUser(user, page.spaceId); - if (ability.cannot(SpaceCaslAction.Read, SpaceCaslSubject.Page)) { - throw new ForbiddenException(); - } - - const canView = await this.canViewPage(user.id, page.id); - if (!canView) { - throw new ForbiddenException(); - } - } - - /** - * Validate user can edit page, throws ForbiddenException if not. - * Checks both space-level and page-level permissions. - */ - async validateCanEdit(page: Page, user: User): Promise { - const ability = await this.spaceAbility.createForUser(user, page.spaceId); - if (ability.cannot(SpaceCaslAction.Edit, SpaceCaslSubject.Page)) { - throw new ForbiddenException(); - } - - const canEdit = await this.canEditPage(user.id, page.id); - if (!canEdit) { - throw new ForbiddenException(); - } + const results = + await this.pagePermissionRepo.filterAccessiblePageIdsWithPermissions( + pageIds, + userId, + ); + return results.map((r) => r.id); } } diff --git a/apps/server/src/core/page/services/page.service.ts b/apps/server/src/core/page/services/page.service.ts index b1b01eab..0d70b1e6 100644 --- a/apps/server/src/core/page/services/page.service.ts +++ b/apps/server/src/core/page/services/page.service.ts @@ -57,6 +57,61 @@ export class PageService { private eventEmitter: EventEmitter2, ) {} + /** + * Filters a list of pages to only those accessible to the user while maintaining tree integrity. + * A page is included only if: + * 1. The user has access to it + * 2. Its parent is also included (or it's the root page) + * This ensures that if a middle page is inaccessible, its entire subtree is excluded. + */ + private async filterAccessibleTreePages( + pages: T[], + rootPageId: string, + userId: string, + ): Promise { + if (pages.length === 0) return []; + + const pageIds = pages.map((p) => p.id); + const accessiblePages = + await this.pagePermissionRepo.filterAccessiblePageIdsWithPermissions( + pageIds, + userId, + ); + const accessibleSet = new Set(accessiblePages.map((p) => p.id)); + + // Build a map for quick lookup + const pageMap = new Map(pages.map((p) => [p.id, p])); + + // Prune: include a page only if it's accessible AND its parent chain to root is included + const includedIds = new Set(); + + // Process pages in a way that ensures parents are processed before children + // We do this by iterating until no more pages can be added + let changed = true; + while (changed) { + changed = false; + for (const page of pages) { + if (includedIds.has(page.id)) continue; + if (!accessibleSet.has(page.id)) continue; + + // Root page: include if accessible + if (page.id === rootPageId) { + includedIds.add(page.id); + changed = true; + continue; + } + + // Non-root: include if parent is already included + if (page.parentPageId && includedIds.has(page.parentPageId)) { + includedIds.add(page.id); + changed = true; + } + } + } + + return pages.filter((p) => includedIds.has(p.id)); + } + async findById( pageId: string, includeContent?: boolean, @@ -169,7 +224,7 @@ export class PageService { page.id, ); - return await this.pageRepo.findById(page.id, { + return this.pageRepo.findById(page.id, { includeSpace: true, includeContent: true, includeCreator: true, @@ -197,12 +252,7 @@ export class PageService { 'creatorId', 'deletedAt', ]) - .$if(Boolean(userId), (qb) => - qb.select((eb) => this.pageRepo.withHasChildrenV2(eb, userId)), - ) - //.$if(!userId, (qb) => - // qb.select((eb) => this.pageRepo.withHasChildren(eb)), - // ) + .select((eb) => this.pageRepo.withHasChildren(eb)) .orderBy('position', (ob) => ob.collate('C').asc()) .where('deletedAt', 'is', null) .where('spaceId', '=', spaceId); @@ -218,22 +268,78 @@ export class PageService { perPage: 250, }); - // Filter by page-level permissions if (userId && result.items.length > 0) { const pageIds = result.items.map((p: any) => p.id); - const accessiblePageIds = await this.pagePermissionRepo.filterAccessiblePageIds( - pageIds, - userId, + + // Single query to get accessible pages with their edit permissions + const accessiblePages = + await this.pagePermissionRepo.filterAccessiblePageIdsWithPermissions( + pageIds, + userId, + ); + + const permissionMap = new Map( + accessiblePages.map((p) => [p.id, p.canEdit]), ); - const accessibleSet = new Set(accessiblePageIds); - result.items = result.items.filter((p: any) => accessibleSet.has(p.id)); + + // Filter and add canEdit flag in one pass + result.items = result.items + .filter((p: any) => permissionMap.has(p.id)) + .map((p: any) => ({ + ...p, + canEdit: permissionMap.get(p.id), + })); + + // For pages with hasChildren: true, verify they have accessible children + const pagesWithChildren = result.items.filter((p: any) => p.hasChildren); + if (pagesWithChildren.length > 0) { + const parentIds = pagesWithChildren.map((p: any) => p.id); + const parentsWithAccessibleChildren = + await this.pagePermissionRepo.getParentIdsWithAccessibleChildren( + parentIds, + userId, + ); + const hasAccessibleChildrenSet = new Set(parentsWithAccessibleChildren); + + result.items = result.items.map((p: any) => ({ + ...p, + hasChildren: p.hasChildren && hasAccessibleChildrenSet.has(p.id), + })); + } } return result; } - async movePageToSpace(rootPage: Page, spaceId: string) { + async movePageToSpace(rootPage: Page, spaceId: string, userId: string) { + const allPages = await this.pageRepo.getPageAndDescendants(rootPage.id, { + includeContent: false, + }); + + // Filter to only accessible pages while maintaining tree integrity + const accessiblePages = await this.filterAccessibleTreePages( + allPages, + rootPage.id, + userId, + ); + const accessibleIds = new Set(accessiblePages.map((p) => p.id)); + + // Find inaccessible pages whose parent is being moved - these need to be orphaned + const pagesToOrphan = allPages.filter( + (p) => !accessibleIds.has(p.id) && p.parentPageId && accessibleIds.has(p.parentPageId), + ); + await executeTx(this.db, async (trx) => { + // Orphan inaccessible child pages (make them root pages in original space) + for (const page of pagesToOrphan) { + const orphanPosition = await this.nextPagePosition(rootPage.spaceId, null); + await this.pageRepo.updatePage( + { parentPageId: null, position: orphanPosition }, + page.id, + trx, + ); + } + // Update root page const nextPosition = await this.nextPagePosition(spaceId); await this.pageRepo.updatePage( @@ -241,44 +347,50 @@ export class PageService { rootPage.id, trx, ); - const pageIds = await this.pageRepo - .getPageAndDescendants(rootPage.id, { includeContent: false }) - .then((pages) => pages.map((page) => page.id)); - // The first id is the root page id - if (pageIds.length > 1) { - // Update sub pages + + const pageIdsToMove = accessiblePages.map((p) => p.id); + + if (pageIdsToMove.length > 1) { + // Update sub pages (all accessible pages except root) await this.pageRepo.updatePages( { spaceId }, - pageIds.filter((id) => id !== rootPage.id), + pageIdsToMove.filter((id) => id !== rootPage.id), trx, ); } - if (pageIds.length > 0) { + if (pageIdsToMove.length > 0) { + // Clear page-level permissions - moved pages inherit destination space permissions + // (page_permissions cascade deletes via foreign key) + await trx + .deleteFrom('pageAccess') + .where('pageId', 'in', pageIdsToMove) + .execute(); + // update spaceId in shares await trx .updateTable('shares') .set({ spaceId: spaceId }) - .where('pageId', 'in', pageIds) + .where('pageId', 'in', pageIdsToMove) .execute(); // Update comments await trx .updateTable('comments') .set({ spaceId: spaceId }) - .where('pageId', 'in', pageIds) + .where('pageId', 'in', pageIdsToMove) .execute(); // Update attachments await this.attachmentRepo.updateAttachmentsByPageId( { spaceId }, - pageIds, + pageIdsToMove, trx, ); await this.aiQueue.add(QueueJob.PAGE_MOVED_TO_SPACE, { - pageId: pageIds, - workspaceId: rootPage.workspaceId + pageId: pageIdsToMove, + workspaceId: rootPage.workspaceId, }); } }); @@ -303,10 +415,17 @@ export class PageService { nextPosition = await this.nextPagePosition(spaceId); } - const pages = await this.pageRepo.getPageAndDescendants(rootPage.id, { + const allPages = await this.pageRepo.getPageAndDescendants(rootPage.id, { includeContent: true, }); + // Filter to only accessible pages while maintaining tree integrity + const pages = await this.filterAccessibleTreePages( + allPages, + rootPage.id, + authUser.id, + ); + const pageMap = new Map(); pages.forEach((page) => { pageMap.set(page.id, { @@ -406,9 +525,14 @@ export class PageService { workspaceId: page.workspaceId, creatorId: authUser.id, lastUpdatedById: authUser.id, - parentPageId: page.id === rootPage.id - ? (isDuplicateInSameSpace ? rootPage.parentPageId : null) - : (page.parentPageId ? pageMap.get(page.parentPageId)?.newPageId : null), + parentPageId: + page.id === rootPage.id + ? isDuplicateInSameSpace + ? rootPage.parentPageId + : null + : page.parentPageId + ? pageMap.get(page.parentPageId)?.newPageId + : null, }; }), ); @@ -587,16 +711,43 @@ export class PageService { async getRecentSpacePages( spaceId: string, + userId: string, pagination: PaginationOptions, ): Promise> { - return await this.pageRepo.getRecentPagesInSpace(spaceId, pagination); + const result = await this.pageRepo.getRecentPagesInSpace(spaceId, pagination); + + if (result.items.length > 0) { + const pageIds = result.items.map((p) => p.id); + const accessiblePages = + await this.pagePermissionRepo.filterAccessiblePageIdsWithPermissions( + pageIds, + userId, + ); + const accessibleSet = new Set(accessiblePages.map((p) => p.id)); + result.items = result.items.filter((p) => accessibleSet.has(p.id)); + } + + return result; } async getRecentPages( userId: string, pagination: PaginationOptions, ): Promise> { - return await this.pageRepo.getRecentPages(userId, pagination); + const result = await this.pageRepo.getRecentPages(userId, pagination); + + if (result.items.length > 0) { + const pageIds = result.items.map((p) => p.id); + const accessiblePages = + await this.pagePermissionRepo.filterAccessiblePageIdsWithPermissions( + pageIds, + userId, + ); + const accessibleSet = new Set(accessiblePages.map((p) => p.id)); + result.items = result.items.filter((p) => accessibleSet.has(p.id)); + } + + return result; } async getDeletedSpacePages( diff --git a/apps/server/src/core/search/search.service.ts b/apps/server/src/core/search/search.service.ts index 26879c51..515580bf 100644 --- a/apps/server/src/core/search/search.service.ts +++ b/apps/server/src/core/search/search.service.ts @@ -125,11 +125,12 @@ export class SearchService { // Filter results by page-level permissions (if user is authenticated) if (opts.userId && results.length > 0) { const pageIds = results.map((r: any) => r.id); - const accessiblePageIds = await this.pagePermissionRepo.filterAccessiblePageIds( - pageIds, - opts.userId, - ); - const accessibleSet = new Set(accessiblePageIds); + const accessiblePages = + await this.pagePermissionRepo.filterAccessiblePageIdsWithPermissions( + pageIds, + opts.userId, + ); + const accessibleSet = new Set(accessiblePages.map((p) => p.id)); results = results.filter((r: any) => accessibleSet.has(r.id)); } @@ -227,11 +228,12 @@ export class SearchService { // Filter by page-level permissions if (pages.length > 0) { const pageIds = pages.map((p) => p.id); - const accessiblePageIds = await this.pagePermissionRepo.filterAccessiblePageIds( - pageIds, - userId, - ); - const accessibleSet = new Set(accessiblePageIds); + const accessiblePages = + await this.pagePermissionRepo.filterAccessiblePageIdsWithPermissions( + pageIds, + userId, + ); + const accessibleSet = new Set(accessiblePages.map((p) => p.id)); pages = pages.filter((p) => accessibleSet.has(p.id)); } } diff --git a/apps/server/src/database/repos/group/group-user.repo.ts b/apps/server/src/database/repos/group/group-user.repo.ts index 5c144ec4..bf3710ee 100644 --- a/apps/server/src/database/repos/group/group-user.repo.ts +++ b/apps/server/src/database/repos/group/group-user.repo.ts @@ -152,4 +152,14 @@ export class GroupUserRepo { .where('groupId', '=', groupId) .execute(); } + + async getUserGroupIds(userId: string): Promise { + const results = await this.db + .selectFrom('groupUsers') + .select('groupId') + .where('userId', '=', userId) + .execute(); + + return results.map((r) => r.groupId); + } } 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 e9b5bd27..07c5c724 100644 --- a/apps/server/src/database/repos/page/page-permission.repo.ts +++ b/apps/server/src/database/repos/page/page-permission.repo.ts @@ -12,12 +12,14 @@ import { PaginationOptions } from '@docmost/db/pagination/pagination-options'; import { executeWithPagination } from '@docmost/db/pagination/pagination'; import { sql } from 'kysely'; import { GroupRepo } from '@docmost/db/repos/group/group.repo'; +import { GroupUserRepo } from '@docmost/db/repos/group/group-user.repo'; @Injectable() export class PagePermissionRepo { constructor( @InjectKysely() private readonly db: KyselyDB, private readonly groupRepo: GroupRepo, + private readonly groupUserRepo: GroupUserRepo, ) {} async findPageAccessByPageId( @@ -270,12 +272,8 @@ export class PagePermissionRepo { /** * Check if user can access a page by verifying they have permission on ALL restricted ancestors. - * Returns true if: - * - No ancestors are restricted, OR - * - User has permission (reader or writer) on every restricted ancestor */ async canUserAccessPage(userId: string, pageId: string): Promise { - // Find any restricted ancestor where user lacks permission const deniedAncestor = await this.db .selectFrom('pageHierarchy') .innerJoin('pageAccess', 'pageAccess.pageId', 'pageHierarchy.ancestorId') @@ -306,12 +304,8 @@ export class PagePermissionRepo { /** * Check if user can edit a page by verifying they have WRITER permission on ALL restricted ancestors. - * Returns true if: - * - No ancestors are restricted, OR - * - User has writer permission on every restricted ancestor */ async canUserEditPage(userId: string, pageId: string): Promise { - // Find any restricted ancestor where user lacks writer permission const deniedAncestor = await this.db .selectFrom('pageHierarchy') .innerJoin('pageAccess', 'pageAccess.pageId', 'pageHierarchy.ancestorId') @@ -342,21 +336,190 @@ export class PagePermissionRepo { } /** - * Filter a list of page IDs to only those the user can access. - * Efficient single-query implementation for bulk filtering. + * Get user's access level for a page, checking ALL restricted ancestors. + * Returns: + * - hasRestriction: whether page or any ancestor has restrictions + * - 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) */ - async filterAccessiblePageIds( + async getUserPageAccessLevel( + userId: string, + pageId: string, + ): Promise<{ hasRestriction: boolean; canAccess: boolean; canEdit: boolean }> { + const result = await this.db + .selectFrom('pages') + .select((eb) => [ + // hasRestriction: any ancestor has page_access entry + eb + .case() + .when( + eb.exists( + eb + .selectFrom('pageHierarchy') + .innerJoin( + 'pageAccess', + 'pageAccess.pageId', + 'pageHierarchy.ancestorId', + ) + .select('pageAccess.id') + .whereRef('pageHierarchy.descendantId', '=', 'pages.id'), + ), + ) + .then(true) + .else(false) + .end() + .as('hasRestriction'), + // canAccess: no restricted ancestor without ANY permission + eb + .case() + .when( + eb.not( + eb.exists( + eb + .selectFrom('pageHierarchy') + .innerJoin( + 'pageAccess', + 'pageAccess.pageId', + 'pageHierarchy.ancestorId', + ) + .leftJoin('pagePermissions', (join) => + join + .onRef('pagePermissions.pageAccessId', '=', 'pageAccess.id') + .on((eb2) => + eb2.or([ + eb2('pagePermissions.userId', '=', userId), + eb2( + 'pagePermissions.groupId', + 'in', + eb2 + .selectFrom('groupUsers') + .select('groupUsers.groupId') + .where('groupUsers.userId', '=', userId), + ), + ]), + ), + ) + .select('pageAccess.pageId') + .whereRef('pageHierarchy.descendantId', '=', 'pages.id') + .where('pagePermissions.id', 'is', null), + ), + ), + ) + .then(true) + .else(false) + .end() + .as('canAccess'), + // canEdit: no restricted ancestor without WRITER permission + eb + .case() + .when( + eb.not( + eb.exists( + eb + .selectFrom('pageHierarchy') + .innerJoin( + 'pageAccess', + 'pageAccess.pageId', + 'pageHierarchy.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', + eb2 + .selectFrom('groupUsers') + .select('groupUsers.groupId') + .where('groupUsers.userId', '=', userId), + ), + ]), + ), + ) + .select('pageAccess.pageId') + .whereRef('pageHierarchy.descendantId', '=', 'pages.id') + .where('pagePermissions.id', 'is', null), + ), + ), + ) + .then(true) + .else(false) + .end() + .as('canEdit'), + ]) + .where('pages.id', '=', pageId) + .executeTakeFirst(); + + return { + hasRestriction: Boolean(result?.hasRestriction), + canAccess: Boolean(result?.canAccess), + canEdit: Boolean(result?.canEdit), + }; + } + + /** + * Filter a list of page IDs to only those the user can access. + * Returns page IDs with their permission level (canEdit). + * Single query implementation for efficiency. + */ + async filterAccessiblePageIdsWithPermissions( pageIds: string[], userId: string, - ): Promise { + ): Promise> { if (pageIds.length === 0) return []; - // For each page, count restricted ancestors vs permitted ancestors - // A page is accessible if restrictedCount == permittedCount const results = await this.db .selectFrom('pages') .select('pages.id') + // Check if user lacks writer permission on any restricted ancestor + .select((eb) => + eb + .case() + .when( + eb.not( + eb.exists( + eb + .selectFrom('pageHierarchy') + .innerJoin( + 'pageAccess', + 'pageAccess.pageId', + 'pageHierarchy.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', + eb2 + .selectFrom('groupUsers') + .select('groupUsers.groupId') + .where('groupUsers.userId', '=', userId), + ), + ]), + ), + ) + .select('pageAccess.pageId') + .whereRef('pageHierarchy.descendantId', '=', 'pages.id') + .where('pagePermissions.id', 'is', null), + ), + ), + ) + .then(true) + .else(false) + .end() + .as('canEdit'), + ) .where('pages.id', 'in', pageIds) + // Filter: user must have access (any permission on all restricted ancestors) .where(({ not, exists, selectFrom }) => not( exists( @@ -391,7 +554,7 @@ export class PagePermissionRepo { ) .execute(); - return results.map((r) => r.id); + return results.map((r) => ({ id: r.id, canEdit: Boolean(r.canEdit) })); } /** @@ -408,4 +571,57 @@ export class PagePermissionRepo { return !!result; } + + /** + * Given a list of parent page IDs, return which ones have at least one accessible child. + * Efficient batch query for sidebar hasChildren calculation. + */ + async getParentIdsWithAccessibleChildren( + parentIds: string[], + userId: string, + ): Promise { + if (parentIds.length === 0) return []; + + const results = await this.db + .selectFrom('pages as child') + .select('child.parentPageId') + .distinct() + .where('child.parentPageId', 'in', parentIds) + .where('child.deletedAt', 'is', null) + .where(({ not, exists, selectFrom }) => + not( + exists( + selectFrom('pageHierarchy') + .innerJoin( + 'pageAccess', + 'pageAccess.pageId', + 'pageHierarchy.ancestorId', + ) + .leftJoin('pagePermissions', (join) => + join + .onRef('pagePermissions.pageAccessId', '=', 'pageAccess.id') + .on((eb) => + eb.or([ + eb('pagePermissions.userId', '=', userId), + eb( + 'pagePermissions.groupId', + 'in', + eb + .selectFrom('groupUsers') + .select('groupUsers.groupId') + .where('groupUsers.userId', '=', userId), + ), + ]), + ), + ) + .select('pageAccess.pageId') + .whereRef('pageHierarchy.descendantId', '=', 'child.id') + .where('pagePermissions.id', 'is', null), + ), + ), + ) + .execute(); + + return results.map((r) => r.parentPageId); + } } diff --git a/apps/server/src/database/repos/page/page.repo.ts b/apps/server/src/database/repos/page/page.repo.ts index cc42ba7d..3b948a48 100644 --- a/apps/server/src/database/repos/page/page.repo.ts +++ b/apps/server/src/database/repos/page/page.repo.ts @@ -411,62 +411,6 @@ export class PageRepo { .as('hasChildren'); } - /** - * Permission-aware version of withHasChildren. - * Returns true only if there are children the user can access. - * Uses page_hierarchy closure table to check all restricted ancestors. - */ - withHasChildrenV2(eb: ExpressionBuilder, userId: string) { - return eb - .selectFrom('pages as child') - .select((eb) => - eb - .case() - .when(eb.fn.countAll(), '>', 0) - .then(true) - .else(false) - .end() - .as('count'), - ) - .whereRef('child.parentPageId', '=', 'pages.id') - .where('child.deletedAt', 'is', null) - // Only count children that the user can access - .where(({ not, exists, selectFrom }) => - not( - exists( - selectFrom('pageHierarchy') - .innerJoin( - 'pageAccess', - 'pageAccess.pageId', - 'pageHierarchy.ancestorId', - ) - .leftJoin('pagePermissions', (join) => - join - .onRef('pagePermissions.pageAccessId', '=', 'pageAccess.id') - .on((eb) => - eb.or([ - eb('pagePermissions.userId', '=', userId), - eb( - 'pagePermissions.groupId', - 'in', - eb - .selectFrom('groupUsers') - .select('groupUsers.groupId') - .where('groupUsers.userId', '=', userId), - ), - ]), - ), - ) - .select('pageAccess.pageId') - .whereRef('pageHierarchy.descendantId', '=', 'child.id') - .where('pagePermissions.id', 'is', null), - ), - ), - ) - .limit(1) - .as('hasChildren'); - } - async getPageAndDescendants( parentPageId: string, opts: { includeContent: boolean },