From 7e6d94c8e0cd0f109a37d07c86423a9bfdd30233 Mon Sep 17 00:00:00 2001 From: Anik Dhabal Babu <81948346+anikdhabal@users.noreply.github.com> Date: Mon, 18 Aug 2025 20:27:22 +0530 Subject: [PATCH] fix: team admin/owner can't change others member role (#23161) * fix: team admin/owner can't change member role * update * Update role-management.factory.ts * address review --- .../__tests__/role-management.factory.test.ts | 12 ++++++++---- .../pbac/services/role-management.factory.ts | 17 ++++++++++------- .../viewer/organizations/updateUser.handler.ts | 2 +- .../viewer/teams/changeMemberRole.handler.ts | 6 ++---- 4 files changed, 21 insertions(+), 16 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 c0fd3a2274..4468a303af 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 a2bb073c41..66a88f18c9 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"; @@ -10,7 +11,7 @@ import { RoleService } from "./role.service"; interface IRoleManager { isPBACEnabled: boolean; - checkPermissionToChangeRole(userId: number, organizationId: number): Promise; + checkPermissionToChangeRole(userId: number, targetId: number, scope: "org" | "team"): Promise; assignRole( userId: number, organizationId: number, @@ -29,11 +30,11 @@ class PBACRoleManager implements IRoleManager { private readonly permissionCheckService: PermissionCheckService ) {} - async checkPermissionToChangeRole(userId: number, organizationId: number): Promise { + async checkPermissionToChangeRole(userId: number, targetId: number, scope: "org" | "team"): Promise { const hasPermission = await this.permissionCheckService.checkPermission({ userId, - teamId: organizationId, - permission: "organization.changeMemberRole", + teamId: targetId, + permission: scope === "team" ? "team.changeMemberRole" : "organization.changeMemberRole", fallbackRoles: [MembershipRole.OWNER, MembershipRole.ADMIN], }); @@ -95,9 +96,11 @@ 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, 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 5eeec3f8a5..901abbe1eb 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 297896548d..5fdc873aa9 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, organizationId); + await roleManager.checkPermissionToChangeRole(ctx.user.id, input.teamId, "team"); } 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" });