commonscomputer/cal.diy
4
0
Fork 0
Code Issues Pull Requests Actions 2.0k Packages Projects Releases Wiki Activity

fix: use highest role for org admins accessing sub team event types (#24748)

Browse Source 
* fix: use highest role for org admins accessing sub team event types

When an org admin who is also a member of a sub team (but only as MEMBER role) tries to update a sub team event type, they were being denied access. This was because the permission check logic was using the team MEMBER role instead of checking both team and org roles and using the highest one.

This fix updates PermissionCheckService.checkPermission() and checkPermissions() to use the highest role between team membership and org membership, similar to how getEventTypePermissions() already works.

Changes:
- Added getHighestRole() helper method to determine the highest role between two roles
- Updated checkPermission() to check both team and org membership and use the highest role
- Updated checkPermissions() to use the same logic
- Added test case for org admin who is team member updating sub team event type

Fixes the issue where org admins get 'permission required eventType.update' errors when updating sub team event types.

Co-Authored-By: joe@cal.com <j.auyeung419@gmail.com>

* test: add PBAC-enabled test case for org admin accessing sub team event types

Adds a test case to verify that when PBAC is enabled, org admins who are also members of a sub team (but only with MEMBER role) can still access sub team event types through their org-level ADMIN permissions.

This complements the existing fallback (PBAC disabled) test case and ensures both code paths handle org admin permissions correctly.

Co-Authored-By: joe@cal.com <j.auyeung419@gmail.com>

---------

Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
This commit is contained in:
Joe Au-Yeung
2025-10-30 13:59:48 +00:00
committed by GitHub
co-authored by joe@cal.com Devin AI
parent 6cbe1c1cf1
commit 2dc0e2a6f2
2 changed files with 121 additions and 16 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/features/pbac/services/__tests__/permission-check.service.test.ts
+83
View File
@@ -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);
packages/features/pbac/services/permission-check.service.ts
+38 -16
View File
@@ -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<MembershipRole, number> = {
OWNER: 3,
ADMIN: 2,
MEMBER: 1,
};
return roleHierarchy[role1] >= roleHierarchy[role2] ? role1 : role2;
}
/**
* Gets all team IDs where the user has a specific permission
*/