Skip to content

Commit b155d73

Browse files
sean-brydonsean-brydon
authored
fix: (PBAC) Add organization fallback on resources (calcom#22733)
* add fallback to service * fix: org memebership fallback * remove log * remove log * add merge test case * add test for no parentId on team for getResources
1 parent e42013b commit b155d73

4 files changed

Lines changed: 312 additions & 7 deletions

File tree

‎packages/features/pbac/domain/repositories/IPermissionRepository.ts‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,9 @@ export interface IPermissionRepository {
99
teamId: number;
1010
userId: number;
1111
customRoleId: string | null;
12-
team_parentId?: number;
12+
team: {
13+
parentId: number | null;
14+
};
1315
} | null>;
1416

1517
getMembershipByUserAndTeam(
@@ -20,7 +22,9 @@ export interface IPermissionRepository {
2022
teamId: number;
2123
userId: number;
2224
customRoleId: string | null;
23-
team_parentId?: number;
25+
team: {
26+
parentId: number | null;
27+
};
2428
} | null>;
2529

2630
getOrgMembership(
@@ -45,6 +49,11 @@ export interface IPermissionRepository {
4549
resource: Resource
4650
): Promise<(CrudAction | CustomAction)[]>;
4751

52+
/**
53+
* Gets all permissions for a specific resource by role ID
54+
*/
55+
getResourcePermissionsByRoleId(roleId: string, resource: Resource): Promise<(CrudAction | CustomAction)[]>;
56+
4857
/**
4958
* Gets all team IDs where the user has a specific permission
5059
*/

‎packages/features/pbac/infrastructure/repositories/PermissionRepository.ts‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -179,6 +179,23 @@ export class PermissionRepository implements IPermissionRepository {
179179
return teamPermissions.map((p) => p.action as CrudAction | CustomAction);
180180
}
181181

182+
async getResourcePermissionsByRoleId(
183+
roleId: string,
184+
resource: Resource
185+
): Promise<(CrudAction | CustomAction)[]> {
186+
const permissions = await this.client.rolePermission.findMany({
187+
where: {
188+
roleId,
189+
OR: [{ resource }, { resource: Resource.All }],
190+
},
191+
select: {
192+
action: true,
193+
resource: true,
194+
},
195+
});
196+
return permissions.map((p) => p.action as CrudAction | CustomAction);
197+
}
198+
182199
async getTeamIdsWithPermission(userId: number, permission: PermissionString): Promise<number[]> {
183200
return this.getTeamIdsWithPermissions(userId, [permission]);
184201
}

‎packages/features/pbac/services/__tests__/permission-check.service.test.ts‎

Lines changed: 254 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import type { MembershipRole } from "@calcom/prisma/enums";
66

77
import type { IPermissionRepository } from "../../domain/repositories/IPermissionRepository";
88
import type { PermissionString } from "../../domain/types/permission-registry";
9+
import { Resource } from "../../domain/types/permission-registry";
910
import { PermissionCheckService } from "../permission-check.service";
1011
import type { PermissionService } from "../permission.service";
1112

@@ -34,6 +35,7 @@ describe("PermissionCheckService", () => {
3435
checkRolePermission: vi.fn(),
3536
checkRolePermissions: vi.fn(),
3637
getResourcePermissions: vi.fn(),
38+
getResourcePermissionsByRoleId: vi.fn(),
3739
getTeamIdsWithPermission: vi.fn(),
3840
getTeamIdsWithPermissions: vi.fn(),
3941
} as MockRepository;
@@ -78,6 +80,7 @@ describe("PermissionCheckService", () => {
7880
teamId: membership.teamId,
7981
userId: membership.userId,
8082
customRoleId: membership.customRoleId,
83+
team: { parentId: null },
8184
});
8285
mockRepository.getOrgMembership.mockResolvedValueOnce(null);
8386
mockRepository.checkRolePermission.mockResolvedValueOnce(true);
@@ -192,6 +195,7 @@ describe("PermissionCheckService", () => {
192195
teamId: membership.teamId,
193196
userId: membership.userId,
194197
customRoleId: membership.customRoleId,
198+
team: { parentId: null },
195199
});
196200
mockRepository.getOrgMembership.mockResolvedValueOnce(null);
197201
mockRepository.checkRolePermissions.mockResolvedValueOnce(true);
@@ -268,6 +272,7 @@ describe("PermissionCheckService", () => {
268272
teamId: membership.teamId,
269273
userId: membership.userId,
270274
customRoleId: membership.customRoleId,
275+
team: { parentId: null },
271276
});
272277
mockRepository.getOrgMembership.mockResolvedValueOnce(null);
273278
mockRepository.checkRolePermissions.mockResolvedValueOnce(false);
@@ -379,4 +384,253 @@ describe("PermissionCheckService", () => {
379384
expect(mockRepository.getTeamIdsWithPermissions).toHaveBeenCalledWith(1, permissions);
380385
});
381386
});
387+
388+
describe("getResourcePermissions", () => {
389+
it("should return empty array when PBAC is disabled", async () => {
390+
mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(false);
391+
392+
const result = await service.getResourcePermissions({
393+
userId: 1,
394+
teamId: 1,
395+
resource: Resource.EventType,
396+
});
397+
398+
expect(result).toEqual([]);
399+
expect(mockFeaturesRepository.checkIfTeamHasFeature).toHaveBeenCalledWith(1, "pbac");
400+
expect(mockRepository.getMembershipByUserAndTeam).not.toHaveBeenCalled();
401+
});
402+
403+
it("should return team-level permissions when PBAC is enabled and no org membership", async () => {
404+
mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(true);
405+
mockRepository.getMembershipByUserAndTeam.mockResolvedValueOnce({
406+
id: 1,
407+
teamId: 1,
408+
userId: 1,
409+
customRoleId: "team_role",
410+
team: { parentId: null },
411+
});
412+
mockRepository.getOrgMembership.mockResolvedValueOnce(null);
413+
mockRepository.getResourcePermissionsByRoleId.mockResolvedValueOnce(["create", "read"]);
414+
415+
const result = await service.getResourcePermissions({
416+
userId: 1,
417+
teamId: 1,
418+
resource: Resource.EventType,
419+
});
420+
421+
expect(result).toEqual(["eventType.create", "eventType.read"]);
422+
expect(mockRepository.getMembershipByUserAndTeam).toHaveBeenCalledWith(1, 1);
423+
expect(mockRepository.getResourcePermissionsByRoleId).toHaveBeenCalledWith(
424+
"team_role",
425+
Resource.EventType
426+
);
427+
expect(mockRepository.getOrgMembership).not.toHaveBeenCalled();
428+
});
429+
430+
it("should return only team permissions when team has no parentId", async () => {
431+
mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(true);
432+
mockRepository.getMembershipByUserAndTeam.mockResolvedValueOnce({
433+
id: 1,
434+
teamId: 1,
435+
userId: 1,
436+
customRoleId: "team_role",
437+
team: { parentId: null },
438+
});
439+
mockRepository.getResourcePermissionsByRoleId.mockResolvedValueOnce(["create", "read", "update"]);
440+
441+
const result = await service.getResourcePermissions({
442+
userId: 1,
443+
teamId: 1,
444+
resource: Resource.EventType,
445+
});
446+
447+
expect(result).toEqual(["eventType.create", "eventType.read", "eventType.update"]);
448+
expect(mockRepository.getMembershipByUserAndTeam).toHaveBeenCalledWith(1, 1);
449+
expect(mockRepository.getResourcePermissionsByRoleId).toHaveBeenCalledTimes(1);
450+
expect(mockRepository.getResourcePermissionsByRoleId).toHaveBeenCalledWith(
451+
"team_role",
452+
Resource.EventType
453+
);
454+
expect(mockRepository.getOrgMembership).not.toHaveBeenCalled();
455+
});
456+
457+
it("should return combined team and org permissions when both exist", async () => {
458+
mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(true);
459+
mockRepository.getMembershipByUserAndTeam.mockResolvedValueOnce({
460+
id: 1,
461+
teamId: 1,
462+
userId: 1,
463+
customRoleId: "team_role",
464+
team: { parentId: 2 },
465+
});
466+
mockRepository.getOrgMembership.mockResolvedValueOnce({
467+
id: 2,
468+
teamId: 2,
469+
userId: 1,
470+
customRoleId: "org_role",
471+
});
472+
473+
// Mock team permissions - create implies read
474+
mockRepository.getResourcePermissionsByRoleId
475+
.mockResolvedValueOnce(["create", "read"]) // team permissions
476+
.mockResolvedValueOnce(["update", "delete", "read"]); // org permissions - update/delete imply read
477+
478+
const result = await service.getResourcePermissions({
479+
userId: 1,
480+
teamId: 1,
481+
resource: Resource.EventType,
482+
});
483+
484+
expect(result).toEqual(["eventType.create", "eventType.read", "eventType.update", "eventType.delete"]);
485+
expect(mockRepository.getMembershipByUserAndTeam).toHaveBeenCalledWith(1, 1);
486+
expect(mockRepository.getOrgMembership).toHaveBeenCalledWith(1, 2);
487+
expect(mockRepository.getResourcePermissionsByRoleId).toHaveBeenCalledTimes(2);
488+
expect(mockRepository.getResourcePermissionsByRoleId).toHaveBeenNthCalledWith(
489+
1,
490+
"team_role",
491+
Resource.EventType
492+
);
493+
expect(mockRepository.getResourcePermissionsByRoleId).toHaveBeenNthCalledWith(
494+
2,
495+
"org_role",
496+
Resource.EventType
497+
);
498+
});
499+
500+
it("should deduplicate permissions when team and org have overlapping permissions", async () => {
501+
mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(true);
502+
mockRepository.getMembershipByUserAndTeam.mockResolvedValueOnce({
503+
id: 1,
504+
teamId: 1,
505+
userId: 1,
506+
customRoleId: "team_role",
507+
team: { parentId: 2 },
508+
});
509+
mockRepository.getOrgMembership.mockResolvedValueOnce({
510+
id: 2,
511+
teamId: 2,
512+
userId: 1,
513+
customRoleId: "org_role",
514+
});
515+
516+
// Mock overlapping permissions - both include read, update implies read
517+
mockRepository.getResourcePermissionsByRoleId
518+
.mockResolvedValueOnce(["create", "read"]) // team permissions - create implies read
519+
.mockResolvedValueOnce(["read", "update"]); // org permissions - update implies read, explicit read
520+
521+
const result = await service.getResourcePermissions({
522+
userId: 1,
523+
teamId: 1,
524+
resource: Resource.EventType,
525+
});
526+
527+
expect(result).toEqual(["eventType.create", "eventType.read", "eventType.update"]);
528+
expect(result).toHaveLength(3); // Should not have duplicate "read"
529+
});
530+
531+
it("should return only org permissions when team has no custom role", async () => {
532+
mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(true);
533+
mockRepository.getMembershipByUserAndTeam.mockResolvedValueOnce({
534+
id: 1,
535+
teamId: 1,
536+
userId: 1,
537+
customRoleId: null,
538+
team: { parentId: 2 },
539+
});
540+
mockRepository.getOrgMembership.mockResolvedValueOnce({
541+
id: 2,
542+
teamId: 2,
543+
userId: 1,
544+
customRoleId: "org_role",
545+
});
546+
547+
// Update and delete imply read access
548+
mockRepository.getResourcePermissionsByRoleId.mockResolvedValueOnce(["update", "delete", "read"]);
549+
550+
const result = await service.getResourcePermissions({
551+
userId: 1,
552+
teamId: 1,
553+
resource: Resource.EventType,
554+
});
555+
556+
expect(result).toEqual(["eventType.update", "eventType.delete", "eventType.read"]);
557+
expect(mockRepository.getResourcePermissionsByRoleId).toHaveBeenCalledTimes(1);
558+
expect(mockRepository.getResourcePermissionsByRoleId).toHaveBeenCalledWith(
559+
"org_role",
560+
Resource.EventType
561+
);
562+
});
563+
564+
it("should return empty array when no membership found", async () => {
565+
mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(true);
566+
mockRepository.getMembershipByUserAndTeam.mockResolvedValueOnce(null);
567+
568+
const result = await service.getResourcePermissions({
569+
userId: 1,
570+
teamId: 1,
571+
resource: Resource.EventType,
572+
});
573+
574+
expect(result).toEqual([]);
575+
expect(mockRepository.getResourcePermissionsByRoleId).not.toHaveBeenCalled();
576+
});
577+
578+
it("should return empty array and log error when repository throws", async () => {
579+
mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(true);
580+
mockRepository.getMembershipByUserAndTeam.mockRejectedValueOnce(new Error("Database error"));
581+
582+
const result = await service.getResourcePermissions({
583+
userId: 1,
584+
teamId: 1,
585+
resource: Resource.EventType,
586+
});
587+
588+
expect(result).toEqual([]);
589+
});
590+
591+
it("should enforce correct hierarchy - org permissions should take precedence over team permissions", async () => {
592+
mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(true);
593+
mockRepository.getMembershipByUserAndTeam.mockResolvedValueOnce({
594+
id: 1,
595+
teamId: 1,
596+
userId: 1,
597+
customRoleId: "admin_team_role",
598+
team: { parentId: 2 },
599+
});
600+
mockRepository.getOrgMembership.mockResolvedValueOnce({
601+
id: 2,
602+
teamId: 2,
603+
userId: 1,
604+
customRoleId: "restricted_org_role",
605+
});
606+
607+
// Team role has broad permissions - CUD actions include read
608+
// Org role has restricted permissions (only read)
609+
mockRepository.getResourcePermissionsByRoleId
610+
.mockResolvedValueOnce(["create", "read", "update"]) // team permissions - CRU
611+
.mockResolvedValueOnce(["delete"]); // org permissions - delete only
612+
613+
const result = await service.getResourcePermissions({
614+
userId: 1,
615+
teamId: 1,
616+
resource: Resource.EventType,
617+
});
618+
619+
// User gets eventType.delete because they have this in the org
620+
expect(result).toEqual(["eventType.create", "eventType.read", "eventType.update", "eventType.delete"]);
621+
622+
// Verify both team and org permissions were fetched
623+
expect(mockRepository.getResourcePermissionsByRoleId).toHaveBeenCalledTimes(2);
624+
expect(mockRepository.getResourcePermissionsByRoleId).toHaveBeenNthCalledWith(
625+
1,
626+
"admin_team_role",
627+
Resource.EventType
628+
);
629+
expect(mockRepository.getResourcePermissionsByRoleId).toHaveBeenNthCalledWith(
630+
2,
631+
"restricted_org_role",
632+
Resource.EventType
633+
);
634+
});
635+
});
382636
});

0 commit comments

Comments
 (0)