From e3742dbc660b79d61ebdcf77158c8b8ac7193ddf Mon Sep 17 00:00:00 2001 From: sean-brydon <55134778+sean-brydon@users.noreply.github.com> Date: Wed, 1 Oct 2025 11:06:18 +0100 Subject: [PATCH] fix: Always enforce one owner with pbac (#24144) * Always enforce one owner * updates from merge * remove redudant eslint rules * Update packages/features/pbac/services/pbac-role-manager.service.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --- .../__tests__/role-management.factory.test.ts | 197 ++++++++++++++++++ .../services/pbac-role-manager.service.ts | 68 +++++- .../pbac/services/role-management.factory.ts | 2 +- 3 files changed, 260 insertions(+), 7 deletions(-) diff --git a/packages/features/pbac/services/__tests__/role-management.factory.test.ts b/packages/features/pbac/services/__tests__/role-management.factory.test.ts index 4468a303af..669ad39345 100644 --- a/packages/features/pbac/services/__tests__/role-management.factory.test.ts +++ b/packages/features/pbac/services/__tests__/role-management.factory.test.ts @@ -19,6 +19,8 @@ vi.mock("@calcom/prisma", () => ({ prisma: { membership: { update: vi.fn(), + findUnique: vi.fn(), + count: vi.fn(), }, }, })); @@ -129,6 +131,19 @@ describe("RoleManagementFactory", () => { describe("assignRole", () => { it("should assign default role correctly", async () => { + // Mock non-owner membership to bypass owner validation + vi.mocked(prisma.membership.findUnique).mockResolvedValue({ + id: membershipId, + userId, + teamId: organizationId, + role: MembershipRole.MEMBER, + customRoleId: null, + accepted: true, + disableImpersonation: false, + createdAt: new Date(), + updatedAt: new Date(), + }); + mockRoleService.assignRoleToMember.mockResolvedValue(undefined); const manager = await factory.createRoleManager(organizationId); await manager.assignRole(userId, organizationId, MembershipRole.ADMIN, membershipId); @@ -140,6 +155,20 @@ describe("RoleManagementFactory", () => { it("should assign custom role after validation", async () => { const customRoleId = "custom-role-123"; + + // Mock non-owner membership to bypass owner validation + vi.mocked(prisma.membership.findUnique).mockResolvedValue({ + id: membershipId, + userId, + teamId: organizationId, + role: MembershipRole.MEMBER, + customRoleId: null, + accepted: true, + disableImpersonation: false, + createdAt: new Date(), + updatedAt: new Date(), + }); + mockRoleService.roleBelongsToTeam.mockResolvedValue(true); mockRoleService.assignRoleToMember.mockResolvedValue(undefined); @@ -154,6 +183,19 @@ describe("RoleManagementFactory", () => { const customRoleId = "invalid-role"; mockRoleService.roleBelongsToTeam.mockResolvedValue(false); + // Mock non-owner membership to bypass owner validation + vi.mocked(prisma.membership.findUnique).mockResolvedValue({ + id: membershipId, + userId, + teamId: organizationId, + role: MembershipRole.MEMBER, + customRoleId: null, + accepted: true, + disableImpersonation: false, + createdAt: new Date(), + updatedAt: new Date(), + }); + const manager = await factory.createRoleManager(organizationId); await expect( manager.assignRole(userId, organizationId, customRoleId as MembershipRole, membershipId) @@ -161,6 +203,161 @@ describe("RoleManagementFactory", () => { new RoleManagementError("You do not have access to this role", RoleManagementErrorCode.INVALID_ROLE) ); }); + + it("should prevent changing the last owner to non-owner role", async () => { + // Mock current membership as owner with customRoleId + vi.mocked(prisma.membership.findUnique).mockResolvedValue({ + id: membershipId, + userId, + teamId: organizationId, + role: MembershipRole.MEMBER, + customRoleId: DEFAULT_ROLE_IDS[MembershipRole.OWNER], + accepted: true, + disableImpersonation: false, + createdAt: new Date(), + updatedAt: new Date(), + }); + + // Mock count showing only 1 owner + vi.mocked(prisma.membership.count).mockResolvedValue(1); + + const manager = await factory.createRoleManager(organizationId); + await expect( + manager.assignRole(userId, organizationId, MembershipRole.ADMIN, membershipId) + ).rejects.toThrow( + new RoleManagementError( + "Cannot change the role of the last owner in the organization", + RoleManagementErrorCode.UNAUTHORIZED + ) + ); + }); + + it("should prevent changing the last legacy owner to non-owner role", async () => { + // Mock current membership as legacy owner (role field) + vi.mocked(prisma.membership.findUnique).mockResolvedValue({ + id: membershipId, + userId, + teamId: organizationId, + role: MembershipRole.OWNER, + customRoleId: null, + accepted: true, + disableImpersonation: false, + createdAt: new Date(), + updatedAt: new Date(), + }); + + // Mock count showing only 1 owner + vi.mocked(prisma.membership.count).mockResolvedValue(1); + + const manager = await factory.createRoleManager(organizationId); + await expect( + manager.assignRole(userId, organizationId, MembershipRole.ADMIN, membershipId) + ).rejects.toThrow( + new RoleManagementError( + "Cannot change the role of the last owner in the organization", + RoleManagementErrorCode.UNAUTHORIZED + ) + ); + }); + + it("should allow changing owner role when multiple owners exist", async () => { + // Mock current membership as owner + vi.mocked(prisma.membership.findUnique).mockResolvedValue({ + id: membershipId, + userId, + teamId: organizationId, + role: MembershipRole.MEMBER, + customRoleId: DEFAULT_ROLE_IDS[MembershipRole.OWNER], + accepted: true, + disableImpersonation: false, + createdAt: new Date(), + updatedAt: new Date(), + }); + + // Mock count showing multiple owners + vi.mocked(prisma.membership.count).mockResolvedValue(2); + mockRoleService.assignRoleToMember.mockResolvedValue(undefined); + + const manager = await factory.createRoleManager(organizationId); + await expect( + manager.assignRole(userId, organizationId, MembershipRole.ADMIN, membershipId) + ).resolves.not.toThrow(); + + expect(mockRoleService.assignRoleToMember).toHaveBeenCalledWith( + DEFAULT_ROLE_IDS[MembershipRole.ADMIN], + membershipId + ); + }); + + it("should allow changing owner to owner (same role)", async () => { + // Mock current membership as owner + vi.mocked(prisma.membership.findUnique).mockResolvedValue({ + id: membershipId, + userId, + teamId: organizationId, + role: MembershipRole.MEMBER, + customRoleId: DEFAULT_ROLE_IDS[MembershipRole.OWNER], + accepted: true, + disableImpersonation: false, + createdAt: new Date(), + updatedAt: new Date(), + }); + + // Mock count showing only 1 owner (should be bypassed) + vi.mocked(prisma.membership.count).mockResolvedValue(1); + mockRoleService.assignRoleToMember.mockResolvedValue(undefined); + + const manager = await factory.createRoleManager(organizationId); + await expect( + manager.assignRole(userId, organizationId, MembershipRole.OWNER, membershipId) + ).resolves.not.toThrow(); + + expect(mockRoleService.assignRoleToMember).toHaveBeenCalledWith( + DEFAULT_ROLE_IDS[MembershipRole.OWNER], + membershipId + ); + }); + + it("should allow changing non-owner role without validation", async () => { + // Mock current membership as non-owner + vi.mocked(prisma.membership.findUnique).mockResolvedValue({ + id: membershipId, + userId, + teamId: organizationId, + role: MembershipRole.MEMBER, + customRoleId: null, + accepted: true, + disableImpersonation: false, + createdAt: new Date(), + updatedAt: new Date(), + }); + + mockRoleService.assignRoleToMember.mockResolvedValue(undefined); + + const manager = await factory.createRoleManager(organizationId); + await expect( + manager.assignRole(userId, organizationId, MembershipRole.ADMIN, membershipId) + ).resolves.not.toThrow(); + + expect(mockRoleService.assignRoleToMember).toHaveBeenCalledWith( + DEFAULT_ROLE_IDS[MembershipRole.ADMIN], + membershipId + ); + // Should not call count since it's not an owner + expect(prisma.membership.count).not.toHaveBeenCalled(); + }); + + it("should throw error when membership is not found", async () => { + // Mock membership not found + vi.mocked(prisma.membership.findUnique).mockResolvedValue(null); + + const manager = await factory.createRoleManager(organizationId); + await expect( + manager.assignRole(userId, organizationId, MembershipRole.ADMIN, membershipId) + ).rejects.toThrow( + new RoleManagementError("Membership not found", RoleManagementErrorCode.UNAUTHORIZED) + ); + }); }); }); diff --git a/packages/features/pbac/services/pbac-role-manager.service.ts b/packages/features/pbac/services/pbac-role-manager.service.ts index f2c871fc0c..58a9cb8196 100644 --- a/packages/features/pbac/services/pbac-role-manager.service.ts +++ b/packages/features/pbac/services/pbac-role-manager.service.ts @@ -1,9 +1,10 @@ +import { prisma } from "@calcom/prisma"; import { MembershipRole } from "@calcom/prisma/enums"; import { RoleManagementError, RoleManagementErrorCode } from "../domain/errors/role-management.error"; import { DEFAULT_ROLE_IDS } from "../lib/constants"; -import type { IRoleManager } from "./role-manager.interface"; import { PermissionCheckService } from "./permission-check.service"; +import type { IRoleManager } from "./role-manager.interface"; import { RoleService } from "./role.service"; export class PBACRoleManager implements IRoleManager { @@ -18,11 +19,7 @@ export class PBACRoleManager implements IRoleManager { userId: number, targetId: number, scope: "org" | "team", - // Not required for this instance - // eslint-disable-next-line @typescript-eslint/no-unused-vars _memberId?: number, - // Not required for this instance - // eslint-disable-next-line @typescript-eslint/no-unused-vars _newRole?: MembershipRole | string ): Promise { const hasPermission = await this.permissionCheckService.checkPermission({ @@ -40,16 +37,75 @@ export class PBACRoleManager implements IRoleManager { } } + private async validateNotLastOwner( + organizationId: number, + membershipId: number, + newRole: MembershipRole | string + ): Promise { + // Get current membership details + const currentMembership = await prisma.membership.findUnique({ + where: { id: membershipId }, + select: { + id: true, + customRoleId: true, + role: true, + teamId: true, + }, + }); + + if (!currentMembership) { + throw new RoleManagementError("Membership not found", RoleManagementErrorCode.UNAUTHORIZED); + } + + // Check if the current membership has owner role (either through customRoleId or role field) + const hasOwnerRole = + currentMembership.customRoleId === DEFAULT_ROLE_IDS[MembershipRole.OWNER] || + currentMembership.role === MembershipRole.OWNER; + + // If current membership is not an owner, no validation needed + if (!hasOwnerRole) { + return; + } + + // Check if new role is still owner - if so, no validation needed + const newRoleIsOwner = + newRole === MembershipRole.OWNER || newRole === DEFAULT_ROLE_IDS[MembershipRole.OWNER]; + + if (newRoleIsOwner) { + return; + } + + // Count total owners in the organization/team + const ownerCount = await prisma.membership.count({ + where: { + teamId: organizationId, + accepted: true, + OR: [{ customRoleId: DEFAULT_ROLE_IDS[MembershipRole.OWNER] }, { role: MembershipRole.OWNER }], + }, + }); + + // If this is the last owner, prevent the role change + if (ownerCount <= 1) { + throw new RoleManagementError( + "Cannot change the role of the last owner in the organization", + RoleManagementErrorCode.UNAUTHORIZED + ); + } + } + async assignRole( _userId: number, organizationId: number, role: MembershipRole | string, membershipId: number ): Promise { + await this.validateNotLastOwner(organizationId, membershipId, role); + // Check if role is one of the default MembershipRole enum values const isDefaultRole = role in DEFAULT_ROLE_IDS; // Also check if the role is a default role ID value + // eslint-disable-next-line @typescript-eslint/no-explicit-any const isDefaultRoleId = Object.values(DEFAULT_ROLE_IDS).includes(role as any); if (isDefaultRole) { @@ -86,4 +142,4 @@ export class PBACRoleManager implements IRoleManager { name: role.name, })); } -} \ No newline at end of file +} diff --git a/packages/features/pbac/services/role-management.factory.ts b/packages/features/pbac/services/role-management.factory.ts index 351044efc5..bf929554d4 100644 --- a/packages/features/pbac/services/role-management.factory.ts +++ b/packages/features/pbac/services/role-management.factory.ts @@ -4,7 +4,7 @@ import { prisma } from "@calcom/prisma"; import { LegacyRoleManager } from "./legacy-role-manager.service"; import { PBACRoleManager } from "./pbac-role-manager.service"; import { PermissionCheckService } from "./permission-check.service"; -import type { IRoleManager } from "./role-manager.interface"; +import { IRoleManager } from "./role-manager.interface"; import { RoleService } from "./role.service"; export class RoleManagementFactory {