From 5792fc7ca2396ea3cf82ad65a671bbad200a032c Mon Sep 17 00:00:00 2001 From: Philip Okugbe <16838612+Philipinho@users.noreply.github.com> Date: Tue, 8 Sep 2026 13:16:56 +0100 Subject: [PATCH] fix: db lock operations (#2479) * fix: advisory lock for page move * fix: lock role count check --- .../src/core/page/services/page.service.ts | 156 +++++++++----- .../space/services/space-member.service.ts | 133 ++++++------ .../workspace/services/workspace.service.ts | 197 +++++++++++------- .../src/database/repos/page/page.repo.ts | 50 ++++- .../src/database/repos/space/space.repo.ts | 11 +- .../src/database/repos/user/user.repo.ts | 6 +- 6 files changed, 348 insertions(+), 205 deletions(-) diff --git a/apps/server/src/core/page/services/page.service.ts b/apps/server/src/core/page/services/page.service.ts index cabfb0c74..990d8fae2 100644 --- a/apps/server/src/core/page/services/page.service.ts +++ b/apps/server/src/core/page/services/page.service.ts @@ -1,5 +1,6 @@ import { BadRequestException, + ConflictException, Injectable, Logger, NotFoundException, @@ -20,7 +21,7 @@ import { generateJitteredKeyBetween } from 'fractional-indexing-jittered'; import { MovePageDto } from '../dto/move-page.dto'; import { generateSlugId } from '../../../common/helpers'; import { getPageTitle } from '../../../common/helpers'; -import { executeTx } from '@docmost/db/utils'; +import { dbOrTx, executeTx } from '@docmost/db/utils'; import { AttachmentRepo } from '@docmost/db/repos/attachment/attachment.repo'; import { v7 as uuid7 } from 'uuid'; import { @@ -174,10 +175,14 @@ export class PageService { return page; } - async nextPagePosition(spaceId: string, parentPageId?: string) { + async nextPagePosition( + spaceId: string, + parentPageId?: string, + trx?: KyselyTransaction, + ) { let pagePosition: string; - const lastPageQuery = this.db + const lastPageQuery = dbOrTx(this.db, trx) .selectFrom('pages') .select(['position']) .where('spaceId', '=', spaceId) @@ -391,35 +396,46 @@ export class PageService { } async movePageToSpace(rootPage: Page, spaceId: string, userId: string) { - let childPageIds: string[] = []; + return executeTx(this.db, async (trx) => { + await this.pageRepo.lockPageHierarchySpaces( + [rootPage.spaceId, spaceId], + trx, + ); - const allPages = await this.pageRepo.getPageAndDescendants(rootPage.id, { - includeContent: false, - }); + const currentRootPage = await this.pageRepo.findById(rootPage.id, { + trx, + }); + if (!currentRootPage || currentRootPage.deletedAt) { + throw new NotFoundException('Page to move not found'); + } + if (currentRootPage.spaceId !== rootPage.spaceId) { + throw new ConflictException('Page location changed; retry the move'); + } - // Filter to only accessible pages while maintaining tree integrity - const accessiblePages = await this.filterAccessibleTreePages( - allPages, - rootPage.id, - userId, - rootPage.spaceId, - ); - const accessibleIds = new Set(accessiblePages.map((p) => p.id)); + const allPages = await this.pageRepo.getPageAndDescendants( + currentRootPage.id, + { includeContent: false, trx }, + ); + const accessiblePages = await this.filterAccessibleTreePages( + allPages, + currentRootPage.id, + userId, + currentRootPage.spaceId, + ); + const accessibleIds = new Set(accessiblePages.map((p) => p.id)); + const pagesToOrphan = allPages.filter( + (p) => + !accessibleIds.has(p.id) && + p.parentPageId && + accessibleIds.has(p.parentPageId), + ); - // 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, + currentRootPage.spaceId, null, + trx, ); await this.pageRepo.updatePage( { parentPageId: null, position: orphanPosition }, @@ -429,16 +445,18 @@ export class PageService { } // Update root page - const nextPosition = await this.nextPagePosition(spaceId); + const nextPosition = await this.nextPagePosition(spaceId, null, trx); await this.pageRepo.updatePage( { spaceId, parentPageId: null, position: nextPosition }, - rootPage.id, + currentRootPage.id, trx, ); const pageIdsToMove = accessiblePages.map((p) => p.id); - childPageIds = pageIdsToMove.filter((id) => id !== rootPage.id); + const childPageIds = pageIdsToMove.filter( + (id) => id !== currentRootPage.id, + ); if (pageIdsToMove.length > 1) { // Update sub pages (all accessible pages except root) @@ -501,7 +519,7 @@ export class PageService { { pageIds: pageIdsToMove, spaceId, - workspaceId: rootPage.workspaceId, + workspaceId: currentRootPage.workspaceId, }, { attempts: 2, @@ -512,9 +530,9 @@ export class PageService { }, ); } - }); - return { childPageIds }; + return { childPageIds }; + }); } async duplicatePage( @@ -825,31 +843,59 @@ export class PageService { throw new BadRequestException('A page cannot be its own parent'); } - let parentPageId = null; - if (movedPage.parentPageId === dto.parentPageId) { - parentPageId = undefined; - } else { - // changing the page's parent - if (dto.parentPageId) { - const parentPage = await this.pageRepo.findById(dto.parentPageId); - if ( - !parentPage || - parentPage.deletedAt || - parentPage.spaceId !== movedPage.spaceId - ) { - throw new NotFoundException('Parent page not found'); - } - parentPageId = parentPage.id; - } - } + await executeTx(this.db, async (trx) => { + await this.pageRepo.lockPageHierarchySpaces( + [movedPage.spaceId], + trx, + ); - await this.pageRepo.updatePage( - { - position: dto.position, - parentPageId: parentPageId, - }, - dto.pageId, - ); + const currentPage = await this.pageRepo.findById(dto.pageId, { trx }); + if (!currentPage || currentPage.deletedAt) { + throw new NotFoundException('Moved page not found'); + } + if (currentPage.spaceId !== movedPage.spaceId) { + throw new ConflictException('Page location changed; retry the move'); + } + + let parentPageId = null; + if (currentPage.parentPageId === dto.parentPageId) { + parentPageId = undefined; + } else { + if (dto.parentPageId) { + const parentPage = await this.pageRepo.findById(dto.parentPageId, { + trx, + }); + if ( + !parentPage || + parentPage.deletedAt || + parentPage.spaceId !== currentPage.spaceId + ) { + throw new NotFoundException('Parent page not found'); + } + if ( + await this.pageRepo.isPageDescendant( + dto.pageId, + parentPage.id, + trx, + ) + ) { + throw new BadRequestException( + 'A page cannot be moved under its descendant', + ); + } + parentPageId = parentPage.id; + } + } + + await this.pageRepo.updatePage( + { + position: dto.position, + parentPageId: parentPageId, + }, + dto.pageId, + trx, + ); + }); } async getPageBreadCrumbs(childPageId: string) { diff --git a/apps/server/src/core/space/services/space-member.service.ts b/apps/server/src/core/space/services/space-member.service.ts index e9b836f1f..42cff32a6 100644 --- a/apps/server/src/core/space/services/space-member.service.ts +++ b/apps/server/src/core/space/services/space-member.service.ts @@ -10,7 +10,7 @@ import { SpaceMemberRepo } from '@docmost/db/repos/space/space-member.repo'; import { GroupUserRepo } from '@docmost/db/repos/group/group-user.repo'; import { AddSpaceMembersDto } from '../dto/add-space-members.dto'; import { InjectKysely } from 'nestjs-kysely'; -import { Space, SpaceMember, User } from '@docmost/db/types/entity.types'; +import { Space, User } from '@docmost/db/types/entity.types'; import { SpaceRepo } from '@docmost/db/repos/space/space.repo'; import { RemoveSpaceMemberDto } from '../dto/remove-space-member.dto'; import { UpdateSpaceMemberRoleDto } from '../dto/update-space-member-role.dto'; @@ -218,41 +218,18 @@ export class SpaceMemberService { dto: RemoveSpaceMemberDto, workspaceId: string, ): Promise { - const space = await this.spaceRepo.findById(dto.spaceId, workspaceId); - if (!space) { - throw new NotFoundException('Space not found'); - } + const memberTypeId = dto.userId + ? { userId: dto.userId } + : dto.groupId + ? { groupId: dto.groupId } + : null; - let spaceMember: SpaceMember = null; - - if (dto.userId) { - spaceMember = await this.spaceMemberRepo.getSpaceMemberByTypeId( - dto.spaceId, - { - userId: dto.userId, - }, - ); - } else if (dto.groupId) { - spaceMember = await this.spaceMemberRepo.getSpaceMemberByTypeId( - dto.spaceId, - { - groupId: dto.groupId, - }, - ); - } else { + if (!memberTypeId) { throw new BadRequestException( 'Please provide a valid userId or groupId to remove', ); } - if (!spaceMember) { - throw new NotFoundException('Space membership not found'); - } - - if (spaceMember.role === SpaceRole.ADMIN) { - await this.validateLastAdmin(dto.spaceId); - } - let affectedUserIds: string[] = []; if (dto.userId) { affectedUserIds = [dto.userId]; @@ -262,7 +239,29 @@ export class SpaceMemberService { ); } - await executeTx(this.db, async (trx) => { + const { space, spaceMember } = await executeTx(this.db, async (trx) => { + const space = await this.spaceRepo.findById( + dto.spaceId, + workspaceId, + { withLock: true, trx }, + ); + if (!space) { + throw new NotFoundException('Space not found'); + } + + const spaceMember = await this.spaceMemberRepo.getSpaceMemberByTypeId( + dto.spaceId, + memberTypeId, + trx, + ); + if (!spaceMember) { + throw new NotFoundException('Space membership not found'); + } + + if (spaceMember.role === SpaceRole.ADMIN) { + await this.validateLastAdmin(dto.spaceId, trx); + } + await this.spaceMemberRepo.removeSpaceMemberById( spaceMember.id, dto.spaceId, @@ -280,6 +279,8 @@ export class SpaceMemberService { dto.spaceId, { trx }, ); + + return { space, spaceMember }; }); this.auditService.log({ @@ -304,48 +305,40 @@ export class SpaceMemberService { dto: UpdateSpaceMemberRoleDto, workspaceId: string, ): Promise { - const space = await this.spaceRepo.findById(dto.spaceId, workspaceId); - if (!space) { - throw new NotFoundException('Space not found'); - } + const memberTypeId = dto.userId + ? { userId: dto.userId } + : dto.groupId + ? { groupId: dto.groupId } + : null; - let spaceMember: SpaceMember = null; - - if (dto.userId) { - spaceMember = await this.spaceMemberRepo.getSpaceMemberByTypeId( - dto.spaceId, - { - userId: dto.userId, - }, - ); - } else if (dto.groupId) { - spaceMember = await this.spaceMemberRepo.getSpaceMemberByTypeId( - dto.spaceId, - { - groupId: dto.groupId, - }, - ); - } else { + if (!memberTypeId) { throw new BadRequestException( 'Please provide a valid userId or groupId to remove', ); } - if (!spaceMember) { - throw new NotFoundException('Space membership not found'); - } + const result = await executeTx(this.db, async (trx) => { + const space = await this.spaceRepo.findById( + dto.spaceId, + workspaceId, + { withLock: true, trx }, + ); + if (!space) { + throw new NotFoundException('Space not found'); + } - if (spaceMember.role === dto.role) { - return; - } + const spaceMember = await this.spaceMemberRepo.getSpaceMemberByTypeId( + dto.spaceId, + memberTypeId, + trx, + ); + if (!spaceMember) { + throw new NotFoundException('Space membership not found'); + } - await executeTx(this.db, async (trx) => { - await trx - .selectFrom('spaces') - .select('id') - .where('id', '=', dto.spaceId) - .forUpdate() - .executeTakeFirst(); + if (spaceMember.role === dto.role) { + return { changed: false, space, spaceMember }; + } if (spaceMember.role === SpaceRole.ADMIN) { await this.validateLastAdmin(dto.spaceId, trx); @@ -357,8 +350,16 @@ export class SpaceMemberService { dto.spaceId, trx, ); + + return { changed: true, space, spaceMember }; }); + if (!result.changed) { + return; + } + + const { space, spaceMember } = result; + this.auditService.log({ event: AuditEvent.SPACE_MEMBER_ROLE_CHANGED, resourceType: AuditResource.SPACE_MEMBER, @@ -387,7 +388,7 @@ export class SpaceMemberService { spaceId, trx, ); - if (spaceOwnerCount === 1) { + if (spaceOwnerCount <= 1) { throw new BadRequestException( 'There must be at least one space admin with full access', ); diff --git a/apps/server/src/core/workspace/services/workspace.service.ts b/apps/server/src/core/workspace/services/workspace.service.ts index c110bb8d1..855069ec0 100644 --- a/apps/server/src/core/workspace/services/workspace.service.ts +++ b/apps/server/src/core/workspace/services/workspace.service.ts @@ -747,44 +747,61 @@ export class WorkspaceService { userRoleDto: UpdateWorkspaceUserRoleDto, workspaceId: string, ) { - const user = await this.userRepo.findById(userRoleDto.userId, workspaceId); - const newRole = userRoleDto.role.toLowerCase(); + const result = await executeTx(this.db, async (trx) => { + const workspace = await this.workspaceRepo.findById(workspaceId, { + withLock: true, + trx, + }); + if (!workspace) { + throw new NotFoundException('Workspace not found'); + } - if (!user) { - throw new BadRequestException('Workspace member not found'); - } - - // prevent ADMIN from managing OWNER role - if ( - isAdminActingOnOwner(authUser.role, newRole) || - isAdminActingOnOwner(authUser.role, user.role) - ) { - throw new ForbiddenException(); - } - - if (user.role === newRole) { - return user; - } - - const workspaceOwnerCount = await this.userRepo.roleCountByWorkspaceId( - UserRole.OWNER, - workspaceId, - ); - - if (user.role === UserRole.OWNER && workspaceOwnerCount === 1) { - throw new BadRequestException( - 'There must be at least one workspace owner', + const user = await this.userRepo.findById( + userRoleDto.userId, + workspaceId, + { trx }, ); + if (!user) { + throw new BadRequestException('Workspace member not found'); + } + + if ( + isAdminActingOnOwner(authUser.role, newRole) || + isAdminActingOnOwner(authUser.role, user.role) + ) { + throw new ForbiddenException(); + } + + if (user.role === newRole) { + return { changed: false, user }; + } + + if ( + user.role === UserRole.OWNER && + !user.deletedAt && + !user.deactivatedAt + ) { + await this.validateLastWorkspaceOwner(workspaceId, trx); + } + + await this.userRepo.updateUser( + { + role: newRole, + }, + user.id, + workspaceId, + trx, + ); + + return { changed: true, user }; + }); + + if (!result.changed) { + return result.user; } - await this.userRepo.updateUser( - { - role: newRole, - }, - user.id, - workspaceId, - ); + const { user } = result; this.auditService.log({ event: AuditEvent.USER_ROLE_CHANGED, @@ -848,40 +865,38 @@ export class WorkspaceService { userId: string, workspaceId: string, ): Promise { - const user = await this.userRepo.findById(userId, workspaceId); + const user = await executeTx(this.db, async (trx) => { + const workspace = await this.workspaceRepo.findById(workspaceId, { + withLock: true, + trx, + }); + if (!workspace) { + throw new NotFoundException('Workspace not found'); + } - if (!user || user.deletedAt) { - throw new BadRequestException('Workspace member not found'); - } + const user = await this.userRepo.findById(userId, workspaceId, { trx }); + if (!user || user.deletedAt) { + throw new BadRequestException('Workspace member not found'); + } - if (user.deactivatedAt) { - throw new BadRequestException('User is already deactivated'); - } + if (user.deactivatedAt) { + throw new BadRequestException('User is already deactivated'); + } - if (authUser.id === userId) { - throw new BadRequestException('You cannot deactivate yourself'); - } + if (authUser.id === userId) { + throw new BadRequestException('You cannot deactivate yourself'); + } - if (isAdminActingOnOwner(authUser.role, user.role)) { - throw new BadRequestException( - 'You cannot deactivate a user with owner role', - ); - } - - if (user.role === UserRole.OWNER) { - const workspaceOwnerCount = await this.userRepo.roleCountByWorkspaceId( - UserRole.OWNER, - workspaceId, - ); - - if (workspaceOwnerCount === 1) { + if (isAdminActingOnOwner(authUser.role, user.role)) { throw new BadRequestException( - 'There must be at least one workspace owner', + 'You cannot deactivate a user with owner role', ); } - } - await executeTx(this.db, async (trx) => { + if (user.role === UserRole.OWNER) { + await this.validateLastWorkspaceOwner(workspaceId, trx); + } + await this.userRepo.updateUser( { deactivatedAt: new Date() }, userId, @@ -889,6 +904,8 @@ export class WorkspaceService { trx, ); await this.userSessionRepo.revokeByUserId(userId, workspaceId, trx); + + return user; }); this.auditService.log({ @@ -951,32 +968,34 @@ export class WorkspaceService { userId: string, workspaceId: string, ): Promise { - const user = await this.userRepo.findById(userId, workspaceId); + const user = await executeTx(this.db, async (trx) => { + const workspace = await this.workspaceRepo.findById(workspaceId, { + withLock: true, + trx, + }); + if (!workspace) { + throw new NotFoundException('Workspace not found'); + } - if (!user || user.deletedAt) { - throw new BadRequestException('Workspace member not found'); - } + const user = await this.userRepo.findById(userId, workspaceId, { trx }); + if (!user || user.deletedAt) { + throw new BadRequestException('Workspace member not found'); + } - const workspaceOwnerCount = await this.userRepo.roleCountByWorkspaceId( - UserRole.OWNER, - workspaceId, - ); + if (authUser.id === userId) { + throw new BadRequestException('You cannot delete yourself'); + } - if (user.role === UserRole.OWNER && workspaceOwnerCount === 1) { - throw new BadRequestException( - 'There must be at least one workspace owner', - ); - } + if (isAdminActingOnOwner(authUser.role, user.role)) { + throw new BadRequestException( + 'You cannot delete a user with owner role', + ); + } - if (authUser.id === userId) { - throw new BadRequestException('You cannot delete yourself'); - } + if (user.role === UserRole.OWNER && !user.deactivatedAt) { + await this.validateLastWorkspaceOwner(workspaceId, trx); + } - if (isAdminActingOnOwner(authUser.role, user.role)) { - throw new BadRequestException('You cannot delete a user with owner role'); - } - - await executeTx(this.db, async (trx) => { await this.userRepo.updateUser( { name: 'Deleted user', @@ -1009,6 +1028,8 @@ export class WorkspaceService { }); await this.userSessionRepo.revokeByUserId(userId, workspaceId, trx); + + return user; }); this.auditService.log({ @@ -1030,4 +1051,20 @@ export class WorkspaceService { // empty } } + + private async validateLastWorkspaceOwner( + workspaceId: string, + trx: KyselyTransaction, + ): Promise { + const workspaceOwnerCount = await this.userRepo.roleCountByWorkspaceId( + UserRole.OWNER, + workspaceId, + trx, + ); + if (workspaceOwnerCount <= 1) { + throw new BadRequestException( + 'There must be at least one workspace owner', + ); + } + } } diff --git a/apps/server/src/database/repos/page/page.repo.ts b/apps/server/src/database/repos/page/page.repo.ts index 389314236..bd6f1ee89 100644 --- a/apps/server/src/database/repos/page/page.repo.ts +++ b/apps/server/src/database/repos/page/page.repo.ts @@ -161,6 +161,22 @@ export class PageRepo { return result; } + async lockPageHierarchySpaces( + spaceIds: string[], + trx: KyselyTransaction, + ): Promise { + const sortedSpaceIds = [...new Set(spaceIds)].sort(); + + for (const spaceId of sortedSpaceIds) { + await sql` + SELECT pg_advisory_xact_lock( + hashtext('page-hierarchy'), + hashtext(${spaceId}) + ) + `.execute(trx); + } + } + async insertPage( insertablePage: InsertablePage, trx?: KyselyTransaction, @@ -489,9 +505,9 @@ export class PageRepo { async getPageAndDescendants( parentPageId: string, - opts: { includeContent: boolean }, + opts: { includeContent: boolean; trx?: KyselyTransaction }, ) { - return this.db + return dbOrTx(this.db, opts.trx) .withRecursive('page_hierarchy', (db) => db .selectFrom('pages') @@ -535,6 +551,36 @@ export class PageRepo { .execute(); } + async isPageDescendant( + ancestorPageId: string, + descendantPageId: string, + trx?: KyselyTransaction, + ): Promise { + const result = await dbOrTx(this.db, trx) + .withRecursive('page_ancestors', (db) => + db + .selectFrom('pages') + .select(['id', 'parentPageId']) + .where('id', '=', descendantPageId) + .union((exp) => + exp + .selectFrom('pages as parent') + .select(['parent.id', 'parent.parentPageId']) + .innerJoin( + 'page_ancestors as ancestor', + 'ancestor.parentPageId', + 'parent.id', + ), + ), + ) + .selectFrom('page_ancestors') + .select('id') + .where('id', '=', ancestorPageId) + .executeTakeFirst(); + + return Boolean(result); + } + /** * Get page and all descendants, excluding restricted pages and their subtrees. * More efficient than getPageAndDescendants + filtering because: diff --git a/apps/server/src/database/repos/space/space.repo.ts b/apps/server/src/database/repos/space/space.repo.ts index f91aae55e..6b791e065 100644 --- a/apps/server/src/database/repos/space/space.repo.ts +++ b/apps/server/src/database/repos/space/space.repo.ts @@ -25,7 +25,11 @@ export class SpaceRepo { async findById( spaceId: string, workspaceId: string, - opts?: { includeMemberCount?: boolean; trx?: KyselyTransaction }, + opts?: { + includeMemberCount?: boolean; + withLock?: boolean; + trx?: KyselyTransaction; + }, ): Promise { const db = dbOrTx(this.db, opts?.trx); @@ -41,6 +45,11 @@ export class SpaceRepo { } else { query = query.where(sql`LOWER(slug)`, '=', sql`LOWER(${spaceId})`); } + + if (opts?.withLock && opts?.trx) { + query = query.forUpdate(); + } + return query.executeTakeFirst(); } diff --git a/apps/server/src/database/repos/user/user.repo.ts b/apps/server/src/database/repos/user/user.repo.ts index 4d619e6c1..d285ddc47 100644 --- a/apps/server/src/database/repos/user/user.repo.ts +++ b/apps/server/src/database/repos/user/user.repo.ts @@ -145,12 +145,16 @@ export class UserRepo { async roleCountByWorkspaceId( role: string, workspaceId: string, + trx?: KyselyTransaction, ): Promise { - const { count } = await this.db + const db = dbOrTx(this.db, trx); + const { count } = await db .selectFrom('users') .select((eb) => eb.fn.count('role').as('count')) .where('role', '=', role) .where('workspaceId', '=', workspaceId) + .where('deletedAt', 'is', null) + .where('deactivatedAt', 'is', null) .executeTakeFirst(); return count as number;