fix: reassign PBAC role before deleting (#23889)
This commit is contained in:
@@ -20,4 +20,5 @@ export interface IRoleRepository {
|
||||
}
|
||||
): Promise<Role>;
|
||||
getPermissions(roleId: string): Promise<RolePermission[]>;
|
||||
reassignUsersToRole(fromRoleId: string, toRoleId: string): Promise<void>;
|
||||
}
|
||||
|
||||
@@ -82,6 +82,13 @@ export class RoleRepository {
|
||||
]);
|
||||
}
|
||||
|
||||
async reassignUsersToRole(fromRoleId: string, toRoleId: string): Promise<void> {
|
||||
await this.client.membership.updateMany({
|
||||
where: { customRoleId: fromRoleId },
|
||||
data: { customRoleId: toRoleId },
|
||||
});
|
||||
}
|
||||
|
||||
async update(
|
||||
roleId: string,
|
||||
permissionChanges: {
|
||||
|
||||
@@ -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;
|
||||
|
||||
/**
|
||||
|
||||
@@ -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();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user