Skip to content

fix: (PBAC) Add organization fallback on resources #22733

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
emrysal merged 9 commits into from
Jul 26, 2025
Merged

fix: (PBAC) Add organization fallback on resources #22733

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7b95b8c
add fallback to service
sean-brydon Jul 25, 2025
e747c14
fix: org memebership fallback
sean-brydon Jul 25, 2025
ed92c3b
remove log
sean-brydon Jul 25, 2025
6570756
remove log
sean-brydon Jul 25, 2025
0e89b63
Merge remote-tracking branch 'refs/remotes/origin/fix/getResoruces-pb…
sean-brydon Jul 25, 2025
e54fbd6
add merge test case
sean-brydon Jul 25, 2025
a34ea15
add test for no parentId on team for getResources
sean-brydon Jul 25, 2025
c1037af
Merge branch 'main' into fix/getResoruces-pbac-fallback-to-org
sean-brydon Jul 25, 2025
f337bb6
Merge branch 'main' into fix/getResoruces-pbac-fallback-to-org
sean-brydon Jul 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions packages/features/pbac/domain/repositories/IPermissionRepository.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ export interface IPermissionRepository {
teamId: number;
userId: number;
customRoleId: string | null;
team_parentId?: number;
team: {
parentId: number | null;
};
} | null>;

getMembershipByUserAndTeam(
Expand All @@ -20,7 +22,9 @@ export interface IPermissionRepository {
teamId: number;
userId: number;
customRoleId: string | null;
team_parentId?: number;
Copy link
Member Author

@sean-brydon sean-brydon Jul 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Outdated type from when this used to use ksyely

team: {
parentId: number | null;
};
} | null>;

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

/**
* Gets all permissions for a specific resource by role ID
*/
getResourcePermissionsByRoleId(roleId: string, resource: Resource): Promise<(CrudAction | CustomAction)[]>;

/**
* Gets all team IDs where the user has a specific permission
*/
Expand Down
17 changes: 17 additions & 0 deletions packages/features/pbac/infrastructure/repositories/PermissionRepository.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,23 @@ export class PermissionRepository implements IPermissionRepository {
return teamPermissions.map((p) => p.action as CrudAction | CustomAction);
}

async getResourcePermissionsByRoleId(
roleId: string,
resource: Resource
): Promise<(CrudAction | CustomAction)[]> {
const permissions = await this.client.rolePermission.findMany({
where: {
roleId,
OR: [{ resource }, { resource: Resource.All }],
},
select: {
action: true,
resource: true,
},
});
return permissions.map((p) => p.action as CrudAction | CustomAction);
}

async getTeamIdsWithPermission(userId: number, permission: PermissionString): Promise<number[]> {
return this.getTeamIdsWithPermissions(userId, [permission]);
}
Expand Down
254 changes: 254 additions & 0 deletions packages/features/pbac/services/__tests__/permission-check.service.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import type { MembershipRole } from "@calcom/prisma/enums";

import type { IPermissionRepository } from "../../domain/repositories/IPermissionRepository";
import type { PermissionString } from "../../domain/types/permission-registry";
import { Resource } from "../../domain/types/permission-registry";
import { PermissionCheckService } from "../permission-check.service";
import type { PermissionService } from "../permission.service";

Expand Down Expand Up @@ -34,6 +35,7 @@ describe("PermissionCheckService", () => {
checkRolePermission: vi.fn(),
checkRolePermissions: vi.fn(),
getResourcePermissions: vi.fn(),
getResourcePermissionsByRoleId: vi.fn(),
getTeamIdsWithPermission: vi.fn(),
getTeamIdsWithPermissions: vi.fn(),
} as MockRepository;
Expand Down Expand Up @@ -78,6 +80,7 @@ describe("PermissionCheckService", () => {
teamId: membership.teamId,
userId: membership.userId,
customRoleId: membership.customRoleId,
team: { parentId: null },
});
mockRepository.getOrgMembership.mockResolvedValueOnce(null);
mockRepository.checkRolePermission.mockResolvedValueOnce(true);
Expand Down Expand Up @@ -192,6 +195,7 @@ describe("PermissionCheckService", () => {
teamId: membership.teamId,
userId: membership.userId,
customRoleId: membership.customRoleId,
team: { parentId: null },
});
mockRepository.getOrgMembership.mockResolvedValueOnce(null);
mockRepository.checkRolePermissions.mockResolvedValueOnce(true);
Expand Down Expand Up @@ -268,6 +272,7 @@ describe("PermissionCheckService", () => {
teamId: membership.teamId,
userId: membership.userId,
customRoleId: membership.customRoleId,
team: { parentId: null },
});
mockRepository.getOrgMembership.mockResolvedValueOnce(null);
mockRepository.checkRolePermissions.mockResolvedValueOnce(false);
Expand Down Expand Up @@ -379,4 +384,253 @@ describe("PermissionCheckService", () => {
expect(mockRepository.getTeamIdsWithPermissions).toHaveBeenCalledWith(1, permissions);
});
});

describe("getResourcePermissions", () => {
it("should return empty array when PBAC is disabled", async () => {
mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(false);

const result = await service.getResourcePermissions({
userId: 1,
teamId: 1,
resource: Resource.EventType,
});

expect(result).toEqual([]);
expect(mockFeaturesRepository.checkIfTeamHasFeature).toHaveBeenCalledWith(1, "pbac");
expect(mockRepository.getMembershipByUserAndTeam).not.toHaveBeenCalled();
});

it("should return team-level permissions when PBAC is enabled and no org membership", async () => {
mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(true);
mockRepository.getMembershipByUserAndTeam.mockResolvedValueOnce({
id: 1,
teamId: 1,
userId: 1,
customRoleId: "team_role",
team: { parentId: null },
});
mockRepository.getOrgMembership.mockResolvedValueOnce(null);
mockRepository.getResourcePermissionsByRoleId.mockResolvedValueOnce(["create", "read"]);

const result = await service.getResourcePermissions({
userId: 1,
teamId: 1,
resource: Resource.EventType,
});

expect(result).toEqual(["eventType.create", "eventType.read"]);
expect(mockRepository.getMembershipByUserAndTeam).toHaveBeenCalledWith(1, 1);
expect(mockRepository.getResourcePermissionsByRoleId).toHaveBeenCalledWith(
"team_role",
Resource.EventType
);
expect(mockRepository.getOrgMembership).not.toHaveBeenCalled();
});

it("should return only team permissions when team has no parentId", async () => {
mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(true);
mockRepository.getMembershipByUserAndTeam.mockResolvedValueOnce({
id: 1,
teamId: 1,
userId: 1,
customRoleId: "team_role",
team: { parentId: null },
});
mockRepository.getResourcePermissionsByRoleId.mockResolvedValueOnce(["create", "read", "update"]);

const result = await service.getResourcePermissions({
userId: 1,
teamId: 1,
resource: Resource.EventType,
});

expect(result).toEqual(["eventType.create", "eventType.read", "eventType.update"]);
expect(mockRepository.getMembershipByUserAndTeam).toHaveBeenCalledWith(1, 1);
expect(mockRepository.getResourcePermissionsByRoleId).toHaveBeenCalledTimes(1);
expect(mockRepository.getResourcePermissionsByRoleId).toHaveBeenCalledWith(
"team_role",
Resource.EventType
);
expect(mockRepository.getOrgMembership).not.toHaveBeenCalled();
});

it("should return combined team and org permissions when both exist", async () => {
mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(true);
mockRepository.getMembershipByUserAndTeam.mockResolvedValueOnce({
id: 1,
teamId: 1,
userId: 1,
customRoleId: "team_role",
team: { parentId: 2 },
});
mockRepository.getOrgMembership.mockResolvedValueOnce({
id: 2,
teamId: 2,
userId: 1,
customRoleId: "org_role",
});

// Mock team permissions - create implies read
mockRepository.getResourcePermissionsByRoleId
.mockResolvedValueOnce(["create", "read"]) // team permissions
.mockResolvedValueOnce(["update", "delete", "read"]); // org permissions - update/delete imply read

const result = await service.getResourcePermissions({
userId: 1,
teamId: 1,
resource: Resource.EventType,
});

expect(result).toEqual(["eventType.create", "eventType.read", "eventType.update", "eventType.delete"]);
expect(mockRepository.getMembershipByUserAndTeam).toHaveBeenCalledWith(1, 1);
expect(mockRepository.getOrgMembership).toHaveBeenCalledWith(1, 2);
expect(mockRepository.getResourcePermissionsByRoleId).toHaveBeenCalledTimes(2);
expect(mockRepository.getResourcePermissionsByRoleId).toHaveBeenNthCalledWith(
1,
"team_role",
Resource.EventType
);
expect(mockRepository.getResourcePermissionsByRoleId).toHaveBeenNthCalledWith(
2,
"org_role",
Resource.EventType
);
});

it("should deduplicate permissions when team and org have overlapping permissions", async () => {
mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(true);
mockRepository.getMembershipByUserAndTeam.mockResolvedValueOnce({
id: 1,
teamId: 1,
userId: 1,
customRoleId: "team_role",
team: { parentId: 2 },
});
mockRepository.getOrgMembership.mockResolvedValueOnce({
id: 2,
teamId: 2,
userId: 1,
customRoleId: "org_role",
});

// Mock overlapping permissions - both include read, update implies read
mockRepository.getResourcePermissionsByRoleId
.mockResolvedValueOnce(["create", "read"]) // team permissions - create implies read
.mockResolvedValueOnce(["read", "update"]); // org permissions - update implies read, explicit read

const result = await service.getResourcePermissions({
userId: 1,
teamId: 1,
resource: Resource.EventType,
});

expect(result).toEqual(["eventType.create", "eventType.read", "eventType.update"]);
expect(result).toHaveLength(3); // Should not have duplicate "read"
});

it("should return only org permissions when team has no custom role", async () => {
mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(true);
mockRepository.getMembershipByUserAndTeam.mockResolvedValueOnce({
id: 1,
teamId: 1,
userId: 1,
customRoleId: null,
team: { parentId: 2 },
});
mockRepository.getOrgMembership.mockResolvedValueOnce({
id: 2,
teamId: 2,
userId: 1,
customRoleId: "org_role",
});

// Update and delete imply read access
mockRepository.getResourcePermissionsByRoleId.mockResolvedValueOnce(["update", "delete", "read"]);

const result = await service.getResourcePermissions({
userId: 1,
teamId: 1,
resource: Resource.EventType,
});

expect(result).toEqual(["eventType.update", "eventType.delete", "eventType.read"]);
expect(mockRepository.getResourcePermissionsByRoleId).toHaveBeenCalledTimes(1);
expect(mockRepository.getResourcePermissionsByRoleId).toHaveBeenCalledWith(
"org_role",
Resource.EventType
);
});

it("should return empty array when no membership found", async () => {
mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(true);
mockRepository.getMembershipByUserAndTeam.mockResolvedValueOnce(null);

const result = await service.getResourcePermissions({
userId: 1,
teamId: 1,
resource: Resource.EventType,
});

expect(result).toEqual([]);
expect(mockRepository.getResourcePermissionsByRoleId).not.toHaveBeenCalled();
});

it("should return empty array and log error when repository throws", async () => {
mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(true);
mockRepository.getMembershipByUserAndTeam.mockRejectedValueOnce(new Error("Database error"));

const result = await service.getResourcePermissions({
userId: 1,
teamId: 1,
resource: Resource.EventType,
});

expect(result).toEqual([]);
});

it("should enforce correct hierarchy - org permissions should take precedence over team permissions", async () => {
mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(true);
mockRepository.getMembershipByUserAndTeam.mockResolvedValueOnce({
id: 1,
teamId: 1,
userId: 1,
customRoleId: "admin_team_role",
team: { parentId: 2 },
});
mockRepository.getOrgMembership.mockResolvedValueOnce({
id: 2,
teamId: 2,
userId: 1,
customRoleId: "restricted_org_role",
});

// Team role has broad permissions - CUD actions include read
// Org role has restricted permissions (only read)
mockRepository.getResourcePermissionsByRoleId
.mockResolvedValueOnce(["create", "read", "update"]) // team permissions - CRU
.mockResolvedValueOnce(["delete"]); // org permissions - delete only

const result = await service.getResourcePermissions({
userId: 1,
teamId: 1,
resource: Resource.EventType,
});

// User gets eventType.delete because they have this in the org
expect(result).toEqual(["eventType.create", "eventType.read", "eventType.update", "eventType.delete"]);

// Verify both team and org permissions were fetched
expect(mockRepository.getResourcePermissionsByRoleId).toHaveBeenCalledTimes(2);
expect(mockRepository.getResourcePermissionsByRoleId).toHaveBeenNthCalledWith(
1,
"admin_team_role",
Resource.EventType
);
expect(mockRepository.getResourcePermissionsByRoleId).toHaveBeenNthCalledWith(
2,
"restricted_org_role",
Resource.EventType
);
});
});
});
Loading
Loading