From a962c17c226d153de2179d752b816d26d07ba291 Mon Sep 17 00:00:00 2001 From: Philipinho <16838612+Philipinho@users.noreply.github.com> Date: Tue, 8 Sep 2026 03:21:20 +0100 Subject: [PATCH] fix: lock role count check --- .../space/services/space-member.service.ts | 133 ++++++------ .../workspace/services/workspace.service.ts | 197 +++++++++++------- .../src/database/repos/space/space.repo.ts | 11 +- .../src/database/repos/user/user.repo.ts | 6 +- 4 files changed, 199 insertions(+), 148 deletions(-) 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/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;