From 8112c3578b192f42da5b2d8f6d15a6682d3521fb Mon Sep 17 00:00:00 2001 From: Philipinho <16838612+Philipinho@users.noreply.github.com> Date: Wed, 7 Jan 2026 17:22:58 +0000 Subject: [PATCH] fix page permissions management --- .../src/core/page/dto/page-permission.dto.ts | 18 ++- .../core/page/page-permission.controller.ts | 46 +++--- .../page/services/page-permission.service.ts | 152 +++++++++--------- .../repos/page/page-permission.repo.ts | 28 ++++ 4 files changed, 142 insertions(+), 102 deletions(-) diff --git a/apps/server/src/core/page/dto/page-permission.dto.ts b/apps/server/src/core/page/dto/page-permission.dto.ts index 17bd75a3..581856ee 100644 --- a/apps/server/src/core/page/dto/page-permission.dto.ts +++ b/apps/server/src/core/page/dto/page-permission.dto.ts @@ -43,12 +43,22 @@ export class AddPagePermissionDto extends PageIdDto { export class RemovePagePermissionDto extends PageIdDto { @IsOptional() - @IsUUID() - userId?: string; + @IsArray() + @ArrayMaxSize(25, { + message: 'userIds must be an array with no more than 25 elements', + }) + @ArrayMinSize(1) + @IsUUID('all', { each: true }) + userIds?: string[]; @IsOptional() - @IsUUID() - groupId?: string; + @IsArray() + @ArrayMaxSize(25, { + message: 'groupIds must be an array with no more than 25 elements', + }) + @ArrayMinSize(1) + @IsUUID('all', { each: true }) + groupIds?: string[]; } export class UpdatePagePermissionRoleDto extends PageIdDto { diff --git a/apps/server/src/core/page/page-permission.controller.ts b/apps/server/src/core/page/page-permission.controller.ts index 987f6ad9..38dddba1 100644 --- a/apps/server/src/core/page/page-permission.controller.ts +++ b/apps/server/src/core/page/page-permission.controller.ts @@ -25,9 +25,7 @@ import { User, Workspace } from '@docmost/db/types/entity.types'; @UseGuards(JwtAuthGuard) @Controller('pages/permissions') export class PagePermissionController { - constructor( - private readonly pagePermissionService: PagePermissionService, - ) {} + constructor(private readonly pagePermissionService: PagePermissionService) {} @HttpCode(HttpStatus.OK) @Post('restrict') @@ -36,41 +34,42 @@ export class PagePermissionController { @AuthUser() user: User, @AuthWorkspace() workspace: Workspace, ) { - await this.pagePermissionService.restrictPage(dto.pageId, user, workspace.id); + await this.pagePermissionService.restrictPage( + dto.pageId, + user, + workspace.id, + ); } @HttpCode(HttpStatus.OK) - @Post('add') + @Post('add-members') async addPagePermission( @Body() dto: AddPagePermissionDto, @AuthUser() user: User, @AuthWorkspace() workspace: Workspace, ) { - if ( - (!dto.userIds || dto.userIds.length === 0) && - (!dto.groupIds || dto.groupIds.length === 0) - ) { - throw new BadRequestException('userIds or groupIds is required'); - } + validateMemberIds(dto); - await this.pagePermissionService.addPagePermissions(dto, user, workspace.id); + await this.pagePermissionService.addPagePermissions( + dto, + user, + workspace.id, + ); } @HttpCode(HttpStatus.OK) - @Post('remove') - async removePagePermission( + @Post('remove-members') + async removePagePermissions( @Body() dto: RemovePagePermissionDto, @AuthUser() user: User, ) { - if (!dto.userId && !dto.groupId) { - throw new BadRequestException('userId or groupId is required'); - } + validateMemberIds(dto); - await this.pagePermissionService.removePagePermission(dto, user); + await this.pagePermissionService.removePagePermissions(dto, user); } @HttpCode(HttpStatus.OK) - @Post('update-role') + @Post('change-role') async updatePagePermissionRole( @Body() dto: UpdatePagePermissionRoleDto, @AuthUser() user: User, @@ -105,3 +104,12 @@ export class PagePermissionController { ); } } + +function validateMemberIds(dto: { userIds?: string[]; groupIds?: string[] }) { + if ( + (!dto.userIds || dto.userIds.length === 0) && + (!dto.groupIds || dto.groupIds.length === 0) + ) { + throw new BadRequestException('userIds or groupIds is required'); + } +} 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 48e5bf45..762cc282 100644 --- a/apps/server/src/core/page/services/page-permission.service.ts +++ b/apps/server/src/core/page/services/page-permission.service.ts @@ -45,13 +45,7 @@ export class PagePermissionService { throw new NotFoundException('Page not found'); } - const ability = await this.spaceAbility.createForUser(authUser, page.spaceId); - if (ability.cannot(SpaceCaslAction.Edit, SpaceCaslSubject.Page)) { - throw new ForbiddenException(); - } - - // TODO: does this check if any of the page's ancestor's is restricted and the user don't have access to it? - // to have access to this page, they must already have access to the page if any of it's ancestor's is restricted + await this.validateWriteAccess(page, authUser); const existingAccess = await this.pagePermissionRepo.findPageAccessByPageId(pageId); @@ -171,7 +165,7 @@ export class PagePermissionService { } } - async removePagePermission( + async removePagePermissions( dto: RemovePagePermissionDto, authUser: User, ): Promise { @@ -189,44 +183,28 @@ export class PagePermissionService { throw new BadRequestException('Page is not restricted'); } - if (!dto.userId && !dto.groupId) { - throw new BadRequestException('Please provide a userId or groupId'); + const userIds = dto.userIds ?? []; + const groupIds = dto.groupIds ?? []; + + if (userIds.length > 0) { + await this.pagePermissionRepo.deletePagePermissionsByUserIds( + pageAccess.id, + userIds, + ); } - if (dto.userId) { - const permission = await this.pagePermissionRepo.findPagePermissionByUserId( + if (groupIds.length > 0) { + await this.pagePermissionRepo.deletePagePermissionsByGroupIds( pageAccess.id, - dto.userId, + groupIds, ); - if (!permission) { - throw new NotFoundException('Permission not found'); - } + } - if (permission.role === PagePermissionRole.WRITER) { - await this.validateLastWriter(pageAccess.id); - } - - await this.pagePermissionRepo.deletePagePermissionByUserId( - pageAccess.id, - dto.userId, - ); - } else if (dto.groupId) { - const permission = - await this.pagePermissionRepo.findPagePermissionByGroupId( - pageAccess.id, - dto.groupId, - ); - if (!permission) { - throw new NotFoundException('Permission not found'); - } - - if (permission.role === PagePermissionRole.WRITER) { - await this.validateLastWriter(pageAccess.id); - } - - await this.pagePermissionRepo.deletePagePermissionByGroupId( - pageAccess.id, - dto.groupId, + 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', ); } } @@ -329,21 +307,29 @@ export class PagePermissionService { throw new NotFoundException('Page not found'); } - const ability = await this.spaceAbility.createForUser(authUser, page.spaceId); + const ability = await this.spaceAbility.createForUser( + authUser, + page.spaceId, + ); + // user must be a space member if (ability.cannot(SpaceCaslAction.Read, SpaceCaslSubject.Page)) { throw new ForbiddenException(); } + // user must not have any restriction to view this page + const canView = await this.canViewPage(authUser.id, pageId); + if (!canView) { + throw new ForbiddenException(); + } + const pageAccess = await this.pagePermissionRepo.findPageAccessByPageId(pageId); if (!pageAccess) { return { items: [], - pagination: { + meta: { + limit: pagination.limit, page: 1, - perPage: pagination.limit, - totalItems: 0, - totalPages: 0, hasNextPage: false, hasPrevPage: false, }, @@ -367,36 +353,38 @@ export class PagePermissionService { } /** - * Check if user has writer permission on ALL restricted ancestors of a page. - * Used for permission management operations. + * Validate if user can manage page permissions (restrict, add/remove members, etc.) + * + * Requirements: + * 1. User must have space-level Edit permission (minimum baseline) + * 2. For restricted pages, user must have one of: + * - Page-level Writer permission on all restricted ancestors + * - Space Admin role + at least page-level Reader permission (admin elevates) */ - async hasWritePermission(userId: string, pageId: string): Promise { - const hasRestriction = - await this.pagePermissionRepo.hasRestrictedAncestor(pageId); + async validateWriteAccess(page: Page, user: User): Promise { + const ability = await this.spaceAbility.createForUser(user, page.spaceId); - if (!hasRestriction) { - return false; // no restrictions, defer to space permissions + if (ability.cannot(SpaceCaslAction.Edit, SpaceCaslSubject.Page)) { + throw new ForbiddenException(); } - return this.pagePermissionRepo.canUserEditPage(userId, pageId); - } - - async hasPageAccess(pageId: string): Promise { - const pageAccess = - await this.pagePermissionRepo.findPageAccessByPageId(pageId); - return !!pageAccess; - } - - async validateWriteAccess(page: Page, user: User): Promise { - const hasWritePermission = await this.hasWritePermission(user.id, page.id); - if (hasWritePermission) { + const canEdit = await this.canEditPage(user.id, page.id); + if (canEdit) { return; } - const ability = await this.spaceAbility.createForUser(user, page.spaceId); - if (ability.cannot(SpaceCaslAction.Manage, SpaceCaslSubject.Page)) { - throw new ForbiddenException(); + const isSpaceAdmin = ability.can( + SpaceCaslAction.Manage, + SpaceCaslSubject.Page, + ); + if (isSpaceAdmin) { + const canView = await this.canViewPage(user.id, page.id); + if (canView) { + return; + } } + + throw new ForbiddenException(); } /** @@ -422,17 +410,23 @@ export class PagePermissionService { } /** - * Filter page IDs to only those the user can access. + * Check if user has writer permission on ALL restricted ancestors of a page. + * Used for permission management operations. */ - async filterAccessiblePages( - pageIds: string[], - userId: string, - ): Promise { - const results = - await this.pagePermissionRepo.filterAccessiblePageIdsWithPermissions( - pageIds, - userId, - ); - return results.map((r) => r.id); + async hasWritePermission(userId: string, pageId: string): Promise { + const hasRestriction = + await this.pagePermissionRepo.hasRestrictedAncestor(pageId); + + if (!hasRestriction) { + return false; // no restrictions, defer to space permissions + } + + return this.pagePermissionRepo.canUserEditPage(userId, pageId); + } + + async hasPageAccess(pageId: string): Promise { + const pageAccess = + await this.pagePermissionRepo.findPageAccessByPageId(pageId); + return !!pageAccess; } } 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 ebb17e51..77039a76 100644 --- a/apps/server/src/database/repos/page/page-permission.repo.ts +++ b/apps/server/src/database/repos/page/page-permission.repo.ts @@ -117,6 +117,34 @@ export class PagePermissionRepo { .execute(); } + async deletePagePermissionsByUserIds( + pageAccessId: string, + userIds: string[], + trx?: KyselyTransaction, + ): Promise { + if (userIds.length === 0) return; + const db = dbOrTx(this.db, trx); + await db + .deleteFrom('pagePermissions') + .where('pageAccessId', '=', pageAccessId) + .where('userId', 'in', userIds) + .execute(); + } + + async deletePagePermissionsByGroupIds( + pageAccessId: string, + groupIds: string[], + trx?: KyselyTransaction, + ): Promise { + if (groupIds.length === 0) return; + const db = dbOrTx(this.db, trx); + await db + .deleteFrom('pagePermissions') + .where('pageAccessId', '=', pageAccessId) + .where('groupId', 'in', groupIds) + .execute(); + } + async updatePagePermissionRole( pageAccessId: string, role: string,