From 69a76f7fb773d72935bf72bf86689ddf14eb32c4 Mon Sep 17 00:00:00 2001 From: sean-brydon <55134778+sean-brydon@users.noreply.github.com> Date: Thu, 18 Sep 2025 09:30:56 +0100 Subject: [PATCH] fix: reassign PBAC role before deleting (#23889) --- .../domain/repositories/IRoleRepository.ts | 1 + .../repositories/RoleRepository.ts | 7 ++++ packages/features/pbac/lib/constants.ts | 15 +++++-- .../services/__tests__/role.service.test.ts | 39 ++++++++++++++++++- .../features/pbac/services/role.service.ts | 6 ++- 5 files changed, 63 insertions(+), 5 deletions(-) diff --git a/packages/features/pbac/domain/repositories/IRoleRepository.ts b/packages/features/pbac/domain/repositories/IRoleRepository.ts index a52e5f0831..4a82e3eb90 100644 --- a/packages/features/pbac/domain/repositories/IRoleRepository.ts +++ b/packages/features/pbac/domain/repositories/IRoleRepository.ts @@ -20,4 +20,5 @@ export interface IRoleRepository { } ): Promise; getPermissions(roleId: string): Promise; + reassignUsersToRole(fromRoleId: string, toRoleId: string): Promise; } diff --git a/packages/features/pbac/infrastructure/repositories/RoleRepository.ts b/packages/features/pbac/infrastructure/repositories/RoleRepository.ts index 280fd4014a..1e35d3e541 100644 --- a/packages/features/pbac/infrastructure/repositories/RoleRepository.ts +++ b/packages/features/pbac/infrastructure/repositories/RoleRepository.ts @@ -82,6 +82,13 @@ export class RoleRepository { ]); } + async reassignUsersToRole(fromRoleId: string, toRoleId: string): Promise { + await this.client.membership.updateMany({ + where: { customRoleId: fromRoleId }, + data: { customRoleId: toRoleId }, + }); + } + async update( roleId: string, permissionChanges: { diff --git a/packages/features/pbac/lib/constants.ts b/packages/features/pbac/lib/constants.ts index 9d6ca73c12..f84a44ec35 100644 --- a/packages/features/pbac/lib/constants.ts +++ b/packages/features/pbac/lib/constants.ts @@ -1,13 +1,22 @@ import { MembershipRole } from "@calcom/prisma/enums"; +/** + * Enum for default role IDs + */ +export enum DefaultPBACRole { + OWNER_ROLE = "owner_role", + ADMIN_ROLE = "admin_role", + MEMBER_ROLE = "member_role", +} + /** * Default role IDs used in the PBAC system * These IDs match the ones created in the migration */ export const DEFAULT_ROLE_IDS = { - [MembershipRole.OWNER]: "owner_role", - [MembershipRole.ADMIN]: "admin_role", - [MembershipRole.MEMBER]: "member_role", + [MembershipRole.OWNER]: DefaultPBACRole.OWNER_ROLE, + [MembershipRole.ADMIN]: DefaultPBACRole.ADMIN_ROLE, + [MembershipRole.MEMBER]: DefaultPBACRole.MEMBER_ROLE, } as const; /** diff --git a/packages/features/pbac/services/__tests__/role.service.test.ts b/packages/features/pbac/services/__tests__/role.service.test.ts index 712b3bf276..29ead3b38c 100644 --- a/packages/features/pbac/services/__tests__/role.service.test.ts +++ b/packages/features/pbac/services/__tests__/role.service.test.ts @@ -39,6 +39,7 @@ describe("RoleService", () => { update: vi.fn(), roleBelongsToTeam: vi.fn(), getPermissions: vi.fn(), + reassignUsersToRole: vi.fn(), }; mockPermissionDiffService = { @@ -300,7 +301,7 @@ describe("RoleService", () => { }); describe("deleteRole", () => { - it("should delete a custom role", async () => { + it("should delete a custom role and reassign users to member_role", async () => { const roleId = "role-id"; const role: Role = { id: roleId, @@ -314,9 +315,43 @@ describe("RoleService", () => { }; mockRepository.findById.mockResolvedValueOnce(role); + mockRepository.reassignUsersToRole.mockResolvedValueOnce(void 0); mockRepository.delete.mockResolvedValueOnce(void 0); await service.deleteRole(roleId); + expect(mockRepository.reassignUsersToRole).toHaveBeenCalledWith(roleId, "member_role"); + expect(mockRepository.delete).toHaveBeenCalledWith(roleId); + }); + + it("should reassign users before deleting the role", async () => { + const roleId = "role-id"; + const role: Role = { + id: roleId, + name: "Test Role", + teamId: 1, + color: "#000000", + type: RoleType.CUSTOM, + permissions: [], + createdAt: new Date(), + updatedAt: new Date(), + }; + + const callOrder: string[] = []; + mockRepository.findById.mockResolvedValueOnce(role); + mockRepository.reassignUsersToRole.mockImplementation(() => { + callOrder.push("reassignUsersToRole"); + return Promise.resolve(); + }); + mockRepository.delete.mockImplementation(() => { + callOrder.push("delete"); + return Promise.resolve(); + }); + + await service.deleteRole(roleId); + + // Verify that reassignment happens before deletion + expect(callOrder).toEqual(["reassignUsersToRole", "delete"]); + expect(mockRepository.reassignUsersToRole).toHaveBeenCalledWith(roleId, "member_role"); expect(mockRepository.delete).toHaveBeenCalledWith(roleId); }); @@ -334,6 +369,7 @@ describe("RoleService", () => { }); await expect(service.deleteRole(roleId)).rejects.toThrow("Cannot delete default roles"); + expect(mockRepository.reassignUsersToRole).not.toHaveBeenCalled(); expect(mockRepository.delete).not.toHaveBeenCalled(); }); @@ -342,6 +378,7 @@ describe("RoleService", () => { mockRepository.findById.mockResolvedValueOnce(null); await expect(service.deleteRole(roleId)).rejects.toThrow("Role not found"); + expect(mockRepository.reassignUsersToRole).not.toHaveBeenCalled(); expect(mockRepository.delete).not.toHaveBeenCalled(); }); }); diff --git a/packages/features/pbac/services/role.service.ts b/packages/features/pbac/services/role.service.ts index dd7313c306..2eecce7ab3 100644 --- a/packages/features/pbac/services/role.service.ts +++ b/packages/features/pbac/services/role.service.ts @@ -5,7 +5,7 @@ import { RoleType as DomainRoleType } from "../domain/models/Role"; import type { CreateRoleData, UpdateRolePermissionsData } from "../domain/models/Role"; import type { IRoleRepository } from "../domain/repositories/IRoleRepository"; import { RoleRepository } from "../infrastructure/repositories/RoleRepository"; -import { DEFAULT_ROLE_IDS } from "../lib/constants"; +import { DEFAULT_ROLE_IDS, DefaultPBACRole } from "../lib/constants"; import { PermissionDiffService } from "./permission-diff.service"; import { PermissionService } from "./permission.service"; @@ -75,6 +75,10 @@ export class RoleService { if (role.type === DomainRoleType.SYSTEM) { throw new Error("Cannot delete default roles"); } + + // Reassign all users with this role to the members_role + await this.repository.reassignUsersToRole(roleId, DefaultPBACRole.MEMBER_ROLE); + await this.repository.delete(roleId); }