From 018d96b0fadc04b4406f1d645b925e272446da2c Mon Sep 17 00:00:00 2001 From: Anik Dhabal Babu Date: Mon, 18 Aug 2025 12:32:52 +0000 Subject: [PATCH 1/4] fix: team admin/owner can't change member role --- .../pbac/services/role-management.factory.ts | 22 ++++++++++++++----- .../viewer/teams/changeMemberRole.handler.ts | 2 +- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/packages/features/pbac/services/role-management.factory.ts b/packages/features/pbac/services/role-management.factory.ts index a2bb073c418412..27be70864e9119 100644 --- a/packages/features/pbac/services/role-management.factory.ts +++ b/packages/features/pbac/services/role-management.factory.ts @@ -10,7 +10,7 @@ import { RoleService } from "./role.service"; interface IRoleManager { isPBACEnabled: boolean; - checkPermissionToChangeRole(userId: number, organizationId: number): Promise; + checkPermissionToChangeRole(userId: number, targetId: number, isOrganization?: boolean): Promise; assignRole( userId: number, organizationId: number, @@ -29,11 +29,15 @@ class PBACRoleManager implements IRoleManager { private readonly permissionCheckService: PermissionCheckService ) {} - async checkPermissionToChangeRole(userId: number, organizationId: number): Promise { + async checkPermissionToChangeRole( + userId: number, + targetId: number, + isOrganization?: boolean + ): Promise { const hasPermission = await this.permissionCheckService.checkPermission({ userId, - teamId: organizationId, - permission: "organization.changeMemberRole", + teamId: targetId, + permission: isOrganization ? "organization.changeMemberRole" : "team.changeMemberRole", fallbackRoles: [MembershipRole.OWNER, MembershipRole.ADMIN], }); @@ -95,8 +99,14 @@ class PBACRoleManager implements IRoleManager { class LegacyRoleManager implements IRoleManager { public isPBACEnabled = false; - async checkPermissionToChangeRole(userId: number, organizationId: number): Promise { - const membership = await isOrganisationAdmin(userId, organizationId); + async checkPermissionToChangeRole( + userId: number, + targetId: number, + isOrganization?: boolean + ): Promise { + const membership = isOrganization + ? !!(await isOrganisationAdmin(userId, targetId)) + : !!(await isTeamAdmin(userId, targetId)); // Only OWNER/ADMIN can update role if (!membership) { diff --git a/packages/trpc/server/routers/viewer/teams/changeMemberRole.handler.ts b/packages/trpc/server/routers/viewer/teams/changeMemberRole.handler.ts index 297896548d6dab..23b857dc5ccf09 100644 --- a/packages/trpc/server/routers/viewer/teams/changeMemberRole.handler.ts +++ b/packages/trpc/server/routers/viewer/teams/changeMemberRole.handler.ts @@ -32,7 +32,7 @@ export const changeMemberRoleHandler = async ({ ctx, input }: ChangeMemberRoleOp // Check permission to change roles try { - await roleManager.checkPermissionToChangeRole(ctx.user.id, organizationId); + await roleManager.checkPermissionToChangeRole(ctx.user.id, input.teamId, false); } catch (error) { throw new TRPCError({ code: "UNAUTHORIZED", From 5f02637d77e5ea0dc55482b0619b7744f1dd91fc Mon Sep 17 00:00:00 2001 From: Anik Dhabal Babu Date: Mon, 18 Aug 2025 12:43:43 +0000 Subject: [PATCH 2/4] update --- .../pbac/services/role-management.factory.ts | 15 +++++++-------- .../viewer/teams/changeMemberRole.handler.ts | 6 ++---- 2 files changed, 9 insertions(+), 12 deletions(-) diff --git a/packages/features/pbac/services/role-management.factory.ts b/packages/features/pbac/services/role-management.factory.ts index 27be70864e9119..a9b348a3ea9ab3 100644 --- a/packages/features/pbac/services/role-management.factory.ts +++ b/packages/features/pbac/services/role-management.factory.ts @@ -10,7 +10,7 @@ import { RoleService } from "./role.service"; interface IRoleManager { isPBACEnabled: boolean; - checkPermissionToChangeRole(userId: number, targetId: number, isOrganization?: boolean): Promise; + checkPermissionToChangeRole(userId: number, targetId: number, checkAtTeamLevel?: boolean): Promise; assignRole( userId: number, organizationId: number, @@ -32,12 +32,12 @@ class PBACRoleManager implements IRoleManager { async checkPermissionToChangeRole( userId: number, targetId: number, - isOrganization?: boolean + checkAtTeamLevel?: boolean ): Promise { const hasPermission = await this.permissionCheckService.checkPermission({ userId, teamId: targetId, - permission: isOrganization ? "organization.changeMemberRole" : "team.changeMemberRole", + permission: checkAtTeamLevel ? "team.changeMemberRole" : "organization.changeMemberRole", fallbackRoles: [MembershipRole.OWNER, MembershipRole.ADMIN], }); @@ -102,12 +102,11 @@ class LegacyRoleManager implements IRoleManager { async checkPermissionToChangeRole( userId: number, targetId: number, - isOrganization?: boolean + checkAtTeamLevel?: boolean ): Promise { - const membership = isOrganization - ? !!(await isOrganisationAdmin(userId, targetId)) - : !!(await isTeamAdmin(userId, targetId)); - + const membership = checkAtTeamLevel + ? !!(await isTeamAdmin(userId, targetId)) + : !!(await isOrganisationAdmin(userId, targetId)); // Only OWNER/ADMIN can update role if (!membership) { throw new RoleManagementError( diff --git a/packages/trpc/server/routers/viewer/teams/changeMemberRole.handler.ts b/packages/trpc/server/routers/viewer/teams/changeMemberRole.handler.ts index 23b857dc5ccf09..8cb30422a68a1f 100644 --- a/packages/trpc/server/routers/viewer/teams/changeMemberRole.handler.ts +++ b/packages/trpc/server/routers/viewer/teams/changeMemberRole.handler.ts @@ -1,5 +1,5 @@ import { RoleManagementFactory } from "@calcom/features/pbac/services/role-management.factory"; -import { isTeamAdmin, isTeamOwner } from "@calcom/lib/server/queries/teams"; +import { isTeamOwner } from "@calcom/lib/server/queries/teams"; import { TeamRepository } from "@calcom/lib/server/repository/team"; import { prisma } from "@calcom/prisma"; import { MembershipRole } from "@calcom/prisma/enums"; @@ -32,7 +32,7 @@ export const changeMemberRoleHandler = async ({ ctx, input }: ChangeMemberRoleOp // Check permission to change roles try { - await roleManager.checkPermissionToChangeRole(ctx.user.id, input.teamId, false); + await roleManager.checkPermissionToChangeRole(ctx.user.id, input.teamId, true); } catch (error) { throw new TRPCError({ code: "UNAUTHORIZED", @@ -45,8 +45,6 @@ export const changeMemberRoleHandler = async ({ ctx, input }: ChangeMemberRoleOp typeof input.role === "string" && Object.values(MembershipRole).includes(input.role as MembershipRole) ) { - // Traditional role assignment logic - if (!(await isTeamAdmin(ctx.user?.id, input.teamId))) throw new TRPCError({ code: "UNAUTHORIZED" }); // Only owners can award owner role. if (input.role === MembershipRole.OWNER && !(await isTeamOwner(ctx.user?.id, input.teamId))) throw new TRPCError({ code: "UNAUTHORIZED" }); From 16582651918a3b81bab7b0df5af4b6e51dd181a0 Mon Sep 17 00:00:00 2001 From: Anik Dhabal Babu <81948346+anikdhabal@users.noreply.github.com> Date: Mon, 18 Aug 2025 18:21:52 +0530 Subject: [PATCH 3/4] Update role-management.factory.ts --- packages/features/pbac/services/role-management.factory.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/features/pbac/services/role-management.factory.ts b/packages/features/pbac/services/role-management.factory.ts index a9b348a3ea9ab3..92e90ec876d832 100644 --- a/packages/features/pbac/services/role-management.factory.ts +++ b/packages/features/pbac/services/role-management.factory.ts @@ -1,5 +1,6 @@ import { FeaturesRepository } from "@calcom/features/flags/features.repository"; import { isOrganisationAdmin } from "@calcom/lib/server/queries/organisations"; +import { isTeamAdmin } from "@calcom/lib/server/queries/teams"; import { prisma } from "@calcom/prisma"; import { MembershipRole } from "@calcom/prisma/enums"; From 95143bb43b317fc8f845f6feae47c825e1f2a5fc Mon Sep 17 00:00:00 2001 From: Anik Dhabal Babu Date: Mon, 18 Aug 2025 14:20:13 +0000 Subject: [PATCH 4/4] address review --- .../__tests__/role-management.factory.test.ts | 12 ++++++---- .../pbac/services/role-management.factory.ts | 23 +++++++------------ .../organizations/updateUser.handler.ts | 2 +- .../viewer/teams/changeMemberRole.handler.ts | 2 +- 4 files changed, 18 insertions(+), 21 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 c0fd3a22742763..4468a303af7033 100644 --- a/packages/features/pbac/services/__tests__/role-management.factory.test.ts +++ b/packages/features/pbac/services/__tests__/role-management.factory.test.ts @@ -110,13 +110,15 @@ describe("RoleManagementFactory", () => { it("should allow role change when user has permission", async () => { mockPermissionCheckService.checkPermission.mockResolvedValue(true); const manager = await factory.createRoleManager(organizationId); - await expect(manager.checkPermissionToChangeRole(userId, organizationId)).resolves.not.toThrow(); + await expect( + manager.checkPermissionToChangeRole(userId, organizationId, "org") + ).resolves.not.toThrow(); }); it("should throw UNAUTHORIZED when user lacks permission", async () => { mockPermissionCheckService.checkPermission.mockResolvedValue(false); const manager = await factory.createRoleManager(organizationId); - await expect(manager.checkPermissionToChangeRole(userId, organizationId)).rejects.toThrow( + await expect(manager.checkPermissionToChangeRole(userId, organizationId, "org")).rejects.toThrow( new RoleManagementError( "You do not have permission to change roles", RoleManagementErrorCode.UNAUTHORIZED @@ -193,13 +195,15 @@ describe("RoleManagementFactory", () => { customRoleId: null, }); const manager = await factory.createRoleManager(organizationId); - await expect(manager.checkPermissionToChangeRole(userId, organizationId)).resolves.not.toThrow(); + await expect( + manager.checkPermissionToChangeRole(userId, organizationId, "org") + ).resolves.not.toThrow(); }); it("should throw UNAUTHORIZED when user is not owner", async () => { vi.mocked(isOrganisationAdmin).mockResolvedValue(false); const manager = await factory.createRoleManager(organizationId); - await expect(manager.checkPermissionToChangeRole(userId, organizationId)).rejects.toThrow( + await expect(manager.checkPermissionToChangeRole(userId, organizationId, "org")).rejects.toThrow( new RoleManagementError( "Only owners or admin can update roles", RoleManagementErrorCode.UNAUTHORIZED diff --git a/packages/features/pbac/services/role-management.factory.ts b/packages/features/pbac/services/role-management.factory.ts index 92e90ec876d832..66a88f18c957c5 100644 --- a/packages/features/pbac/services/role-management.factory.ts +++ b/packages/features/pbac/services/role-management.factory.ts @@ -11,7 +11,7 @@ import { RoleService } from "./role.service"; interface IRoleManager { isPBACEnabled: boolean; - checkPermissionToChangeRole(userId: number, targetId: number, checkAtTeamLevel?: boolean): Promise; + checkPermissionToChangeRole(userId: number, targetId: number, scope: "org" | "team"): Promise; assignRole( userId: number, organizationId: number, @@ -30,15 +30,11 @@ class PBACRoleManager implements IRoleManager { private readonly permissionCheckService: PermissionCheckService ) {} - async checkPermissionToChangeRole( - userId: number, - targetId: number, - checkAtTeamLevel?: boolean - ): Promise { + async checkPermissionToChangeRole(userId: number, targetId: number, scope: "org" | "team"): Promise { const hasPermission = await this.permissionCheckService.checkPermission({ userId, teamId: targetId, - permission: checkAtTeamLevel ? "team.changeMemberRole" : "organization.changeMemberRole", + permission: scope === "team" ? "team.changeMemberRole" : "organization.changeMemberRole", fallbackRoles: [MembershipRole.OWNER, MembershipRole.ADMIN], }); @@ -100,14 +96,11 @@ class PBACRoleManager implements IRoleManager { class LegacyRoleManager implements IRoleManager { public isPBACEnabled = false; - async checkPermissionToChangeRole( - userId: number, - targetId: number, - checkAtTeamLevel?: boolean - ): Promise { - const membership = checkAtTeamLevel - ? !!(await isTeamAdmin(userId, targetId)) - : !!(await isOrganisationAdmin(userId, targetId)); + async checkPermissionToChangeRole(userId: number, targetId: number, scope: "org" | "team"): Promise { + const membership = + scope === "team" + ? !!(await isTeamAdmin(userId, targetId)) + : !!(await isOrganisationAdmin(userId, targetId)); // Only OWNER/ADMIN can update role if (!membership) { throw new RoleManagementError( diff --git a/packages/trpc/server/routers/viewer/organizations/updateUser.handler.ts b/packages/trpc/server/routers/viewer/organizations/updateUser.handler.ts index 5eeec3f8a5719d..901abbe1eb7d28 100644 --- a/packages/trpc/server/routers/viewer/organizations/updateUser.handler.ts +++ b/packages/trpc/server/routers/viewer/organizations/updateUser.handler.ts @@ -47,7 +47,7 @@ export const updateUserHandler = async ({ ctx, input }: UpdateUserOptions) => { const roleManager = await RoleManagementFactory.getInstance().createRoleManager(organizationId); try { - await roleManager.checkPermissionToChangeRole(userId, organizationId); + await roleManager.checkPermissionToChangeRole(userId, organizationId, "org"); } catch (error) { if (error instanceof RoleManagementError) { throw new TRPCError({ code: "UNAUTHORIZED", message: error.message }); diff --git a/packages/trpc/server/routers/viewer/teams/changeMemberRole.handler.ts b/packages/trpc/server/routers/viewer/teams/changeMemberRole.handler.ts index 8cb30422a68a1f..5fdc873aa961cd 100644 --- a/packages/trpc/server/routers/viewer/teams/changeMemberRole.handler.ts +++ b/packages/trpc/server/routers/viewer/teams/changeMemberRole.handler.ts @@ -32,7 +32,7 @@ export const changeMemberRoleHandler = async ({ ctx, input }: ChangeMemberRoleOp // Check permission to change roles try { - await roleManager.checkPermissionToChangeRole(ctx.user.id, input.teamId, true); + await roleManager.checkPermissionToChangeRole(ctx.user.id, input.teamId, "team"); } catch (error) { throw new TRPCError({ code: "UNAUTHORIZED",