diff --git a/packages/features/pbac/services/__tests__/permission-check.service.test.ts b/packages/features/pbac/services/__tests__/permission-check.service.test.ts index 79a503f1c4..b16083ce37 100644 --- a/packages/features/pbac/services/__tests__/permission-check.service.test.ts +++ b/packages/features/pbac/services/__tests__/permission-check.service.test.ts @@ -177,6 +177,89 @@ describe("PermissionCheckService", () => { }); }); + it("should use highest role when user is MEMBER of sub team but ADMIN of parent org (PBAC disabled)", async () => { + const teamMembership = { + id: 1, + teamId: 1, + userId: 1, + accepted: true, + role: "MEMBER" as MembershipRole, + customRoleId: null, + disableImpersonation: false, + createdAt: new Date(), + updatedAt: new Date(), + }; + + const orgMembership = { + id: 2, + teamId: 100, + userId: 1, + accepted: true, + role: "ADMIN" as MembershipRole, + customRoleId: null, + disableImpersonation: false, + createdAt: new Date(), + updatedAt: new Date(), + }; + + mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(false); + (MembershipRepository.findUniqueByUserIdAndTeamId as Mock) + .mockResolvedValueOnce(teamMembership) // Has team membership as MEMBER + .mockResolvedValueOnce(orgMembership); // Has org membership as ADMIN + mockRepository.getTeamById.mockResolvedValueOnce({ id: 1, parentId: 100 }); + + const result = await service.checkPermission({ + userId: 1, + teamId: 1, + permission: "eventType.update", + fallbackRoles: ["ADMIN", "OWNER"], + }); + + expect(result).toBe(true); + expect(mockRepository.getTeamById).toHaveBeenCalledWith(1); + expect(MembershipRepository.findUniqueByUserIdAndTeamId).toHaveBeenNthCalledWith(1, { + userId: 1, + teamId: 1, + }); + expect(MembershipRepository.findUniqueByUserIdAndTeamId).toHaveBeenNthCalledWith(2, { + userId: 1, + teamId: 100, + }); + }); + + it("should check org-level permissions when user is MEMBER of sub team but ADMIN of parent org (PBAC enabled)", async () => { + mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(true); + mockRepository.getMembershipByUserAndTeam.mockResolvedValueOnce({ + id: 1, + teamId: 1, + userId: 1, + customRoleId: "team_member_role", + team: { id: 1, parentId: 100 }, + }); + mockRepository.getOrgMembership.mockResolvedValueOnce({ + id: 2, + teamId: 100, + userId: 1, + customRoleId: "org_admin_role", + }); + mockRepository.checkRolePermission + .mockResolvedValueOnce(false) // Team member role doesn't have permission + .mockResolvedValueOnce(true); // Org admin role has permission + + const result = await service.checkPermission({ + userId: 1, + teamId: 1, + permission: "eventType.update", + fallbackRoles: ["ADMIN", "OWNER"], + }); + + expect(result).toBe(true); + expect(mockRepository.getMembershipByUserAndTeam).toHaveBeenCalledWith(1, 1); + expect(mockRepository.getOrgMembership).toHaveBeenCalledWith(1, 100); + expect(mockRepository.checkRolePermission).toHaveBeenNthCalledWith(1, "team_member_role", "eventType.update"); + expect(mockRepository.checkRolePermission).toHaveBeenNthCalledWith(2, "org_admin_role", "eventType.update"); + }); + it("should check org-level permissions when user has no team membership but PBAC is enabled", async () => { mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(true); mockRepository.getMembershipByUserAndTeam.mockResolvedValueOnce(null); diff --git a/packages/features/pbac/services/permission-check.service.ts b/packages/features/pbac/services/permission-check.service.ts index 370449ed35..84bdc89d17 100644 --- a/packages/features/pbac/services/permission-check.service.ts +++ b/packages/features/pbac/services/permission-check.service.ts @@ -118,30 +118,33 @@ export class PermissionCheckService { return this.hasPermission({ userId, teamId }, permission); } - // Fallback to role-based check - check both team and org membership + // Fallback to role-based check - use highest role between team and org membership const membership = await MembershipRepository.findUniqueByUserIdAndTeamId({ userId, teamId, }); - // If user has team membership, check their role - if (membership) { - return this.checkFallbackRoles(membership.role, fallbackRoles); - } + let effectiveRole: MembershipRole | null = membership?.role ?? null; - // No team membership - check if team has parent org and user is org member + // Check if team has parent org and get org membership const team = await this.repository.getTeamById(teamId); if (team?.parentId) { const orgMembership = await MembershipRepository.findUniqueByUserIdAndTeamId({ userId, teamId: team.parentId, }); + + // Use the highest role between team and org if (orgMembership) { - return this.checkFallbackRoles(orgMembership.role, fallbackRoles); + effectiveRole = this.getHighestRole(effectiveRole, orgMembership.role); } } - return false; + if (!effectiveRole) { + return false; + } + + return this.checkFallbackRoles(effectiveRole, fallbackRoles); } catch (error) { this.logger.error(error); return false; @@ -179,30 +182,33 @@ export class PermissionCheckService { return this.hasPermissions({ userId, teamId }, permissions); } - // Fallback to role-based check - check both team and org membership + // Fallback to role-based check - use highest role between team and org membership const membership = await MembershipRepository.findUniqueByUserIdAndTeamId({ userId, teamId, }); - // If user has team membership, check their role - if (membership) { - return this.checkFallbackRoles(membership.role, fallbackRoles); - } + let effectiveRole: MembershipRole | null = membership?.role ?? null; - // No team membership - check if team has parent org and user is org member + // Check if team has parent org and get org membership const team = await this.repository.getTeamById(teamId); if (team?.parentId) { const orgMembership = await MembershipRepository.findUniqueByUserIdAndTeamId({ userId, teamId: team.parentId, }); + + // Use the highest role between team and org if (orgMembership) { - return this.checkFallbackRoles(orgMembership.role, fallbackRoles); + effectiveRole = this.getHighestRole(effectiveRole, orgMembership.role); } } - return false; + if (!effectiveRole) { + return false; + } + + return this.checkFallbackRoles(effectiveRole, fallbackRoles); } catch (error) { this.logger.error(error); return false; @@ -284,6 +290,22 @@ export class PermissionCheckService { return allowedRoles.includes(userRole); } + private getHighestRole( + role1: MembershipRole | null, + role2: MembershipRole | null + ): MembershipRole | null { + if (!role1) return role2; + if (!role2) return role1; + + const roleHierarchy: Record = { + OWNER: 3, + ADMIN: 2, + MEMBER: 1, + }; + + return roleHierarchy[role1] >= roleHierarchy[role2] ? role1 : role2; + } + /** * Gets all team IDs where the user has a specific permission */