From b7d028fd2519ccc677e7acba2f8e4353851559df Mon Sep 17 00:00:00 2001 From: Sean Brydon Date: Wed, 3 Sep 2025 08:20:36 +0100 Subject: [PATCH 1/3] dogfood pbac --- .../permission-check.service.test.ts | 83 ++++++++++++++++++- .../pbac/services/permission-check.service.ts | 81 +++++++++++++++++- 2 files changed, 161 insertions(+), 3 deletions(-) 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 c415a8dddd01e5..abd7a77672b5a1 100644 --- a/packages/features/pbac/services/__tests__/permission-check.service.test.ts +++ b/packages/features/pbac/services/__tests__/permission-check.service.test.ts @@ -14,6 +14,13 @@ vi.mock("../../infrastructure/repositories/PermissionRepository"); vi.mock("@calcom/features/flags/features.repository"); vi.mock("@calcom/lib/server/repository/membership"); vi.mock("../permission.service"); +vi.mock("@calcom/lib/constants", async (importOriginal) => { + const actual = (await importOriginal()) as typeof import("@calcom/lib/constants"); + return { + ...actual, + IS_CALCOM: true, + }; +}); type MockRepository = { [K in keyof IPermissionRepository]: Mock; @@ -147,6 +154,43 @@ describe("PermissionCheckService", () => { expect(result).toBe(false); }); + it("should fallback to legacy roles when PBAC permission check fails (dogfood)", async () => { + const membership = { + id: 1, + teamId: 1, + userId: 1, + accepted: true, + role: "ADMIN" as MembershipRole, + customRoleId: "admin_role", + disableImpersonation: false, + createdAt: new Date(), + updatedAt: new Date(), + }; + + (MembershipRepository.findUniqueByUserIdAndTeamId as Mock).mockResolvedValueOnce(membership); + mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(true); + mockRepository.getMembershipByMembershipId.mockResolvedValueOnce({ + id: membership.id, + teamId: membership.teamId, + userId: membership.userId, + customRoleId: membership.customRoleId, + team: { parentId: null }, + }); + mockRepository.getOrgMembership.mockResolvedValueOnce(null); + mockRepository.checkRolePermission.mockResolvedValueOnce(false); // Permission check fails + + const result = await service.checkPermission({ + userId: 1, + teamId: 1, + permission: "eventType.create", + fallbackRoles: ["ADMIN", "OWNER"], + }); + + // Should fallback to role check and pass since user is ADMIN + expect(result).toBe(true); + expect(mockRepository.checkRolePermission).toHaveBeenCalled(); + }); + it("should return false if PBAC enabled but no customRoleId", async () => { const membership = { id: 1, @@ -252,7 +296,7 @@ describe("PermissionCheckService", () => { expect(mockRepository.checkRolePermissions).not.toHaveBeenCalled(); }); - it("should return false when permissions array is empty", async () => { + it("should fallback to legacy roles when PBAC permissions check fails (dogfood)", async () => { const membership = { id: 1, teamId: 1, @@ -265,6 +309,43 @@ describe("PermissionCheckService", () => { updatedAt: new Date(), }; + (MembershipRepository.findUniqueByUserIdAndTeamId as Mock).mockResolvedValueOnce(membership); + mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(true); + mockRepository.getMembershipByMembershipId.mockResolvedValueOnce({ + id: membership.id, + teamId: membership.teamId, + userId: membership.userId, + customRoleId: membership.customRoleId, + team: { parentId: null }, + }); + mockRepository.getOrgMembership.mockResolvedValueOnce(null); + mockRepository.checkRolePermissions.mockResolvedValueOnce(false); // Permissions check fails + + const result = await service.checkPermissions({ + userId: 1, + teamId: 1, + permissions: ["eventType.create", "team.invite"], + fallbackRoles: ["ADMIN", "OWNER"], + }); + + // Should fallback to role check and pass since user is ADMIN + expect(result).toBe(true); + expect(mockRepository.checkRolePermissions).toHaveBeenCalled(); + }); + + it("should return false when permissions array is empty", async () => { + const membership = { + id: 1, + teamId: 1, + userId: 1, + accepted: true, + role: "MEMBER" as MembershipRole, // Change to MEMBER so fallback also fails + customRoleId: "admin_role", + disableImpersonation: false, + createdAt: new Date(), + updatedAt: new Date(), + }; + (MembershipRepository.findUniqueByUserIdAndTeamId as Mock).mockResolvedValueOnce(membership); mockFeaturesRepository.checkIfTeamHasFeature.mockResolvedValueOnce(true); mockRepository.getMembershipByMembershipId.mockResolvedValueOnce({ diff --git a/packages/features/pbac/services/permission-check.service.ts b/packages/features/pbac/services/permission-check.service.ts index 8b9c67d2e54409..930018b3f7ddef 100644 --- a/packages/features/pbac/services/permission-check.service.ts +++ b/packages/features/pbac/services/permission-check.service.ts @@ -1,4 +1,5 @@ import { FeaturesRepository } from "@calcom/features/flags/features.repository"; +import { IS_CALCOM } from "@calcom/lib/constants"; import logger from "@calcom/lib/logger"; import { MembershipRepository } from "@calcom/lib/server/repository/membership"; import prisma from "@calcom/prisma"; @@ -16,6 +17,8 @@ import type { import { PermissionRepository } from "../infrastructure/repositories/PermissionRepository"; import { PermissionService } from "./permission.service"; +const DOGFOOD_PBAC_INTERNALLY = IS_CALCOM; + export class PermissionCheckService { private readonly PBAC_FEATURE_FLAG = "pbac" as const; private readonly logger = logger.getSubLogger({ prefix: ["PermissionCheckService"] }); @@ -126,7 +129,21 @@ export class PermissionCheckService { return false; } - return this.hasPermission({ membershipId: membership.id }, permission); + const hasPbacPermission = await this.hasPermission({ membershipId: membership.id }, permission); + + if (DOGFOOD_PBAC_INTERNALLY && !hasPbacPermission) { + return this.dogfoodFallback({ + userId, + teamId, + membershipId: membership.id, + permissions: [permission], + hasPbacPermission, + fallbackRoles, + membershipRole: membership.role, + }); + } + + return hasPbacPermission; } return this.checkFallbackRoles(membership.role, fallbackRoles); @@ -175,7 +192,21 @@ export class PermissionCheckService { return false; } - return this.hasPermissions({ membershipId: membership.id }, permissions); + const hasPbacPermission = await this.hasPermissions({ membershipId: membership.id }, permissions); + + if (DOGFOOD_PBAC_INTERNALLY && !hasPbacPermission) { + return this.dogfoodFallback({ + userId, + teamId, + membershipId: membership.id, + permissions, + hasPbacPermission, + fallbackRoles, + membershipRole: membership.role, + }); + } + + return hasPbacPermission; } return this.checkFallbackRoles(membership.role, fallbackRoles); @@ -252,6 +283,52 @@ export class PermissionCheckService { return allowedRoles.includes(userRole); } + /** + * Internal dogfooding fallback for PBAC permissions + * + * This method is used internally at Cal.com to help us transition to PBAC. + * When PBAC is enabled but a user doesn't have the necessary permission strings, + * we fall back to checking their legacy fallback roles to avoid breaking existing workflows. + * + * An alert is logged to Axiom when this fallback occurs so we can identify and fix + * missing permissions in our PBAC configuration. + * + * @private + * @param params - Object containing userId, teamId, membershipId, permissions, hasPbacPermission, fallbackRoles, and membershipRole + * @returns boolean - Whether the user has permission via fallback roles + */ + private dogfoodFallback(params: { + userId: number; + teamId: number; + membershipId: number; + permissions: PermissionString[]; + hasPbacPermission: boolean; + fallbackRoles: MembershipRole[]; + membershipRole: MembershipRole; + }): boolean { + const { userId, teamId, membershipId, permissions, hasPbacPermission, fallbackRoles, membershipRole } = + params; + + const debugInfo = { + userId, + teamId, + membershipId, + permissions, + hasPbacPermission, + fallbackRoles, + }; + + this.logger.warn( + `PBAC INTERNAL - Failed but user doesnt have permission string to carry out this action. Falling back to fallback roles. \n JSON${JSON.stringify( + debugInfo, + null, + 2 + )}` + ); + + return this.checkFallbackRoles(membershipRole, fallbackRoles); + } + /** * Gets all team IDs where the user has a specific permission */ From bf19acbcb227a66af7cae59eb5071128c43c6a67 Mon Sep 17 00:00:00 2001 From: Sean Brydon Date: Wed, 3 Sep 2025 08:55:32 +0100 Subject: [PATCH 2/3] fix constants mock --- .../pbac/services/__tests__/permission-check.service.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 abd7a77672b5a1..c41de0da6ebddf 100644 --- a/packages/features/pbac/services/__tests__/permission-check.service.test.ts +++ b/packages/features/pbac/services/__tests__/permission-check.service.test.ts @@ -14,8 +14,8 @@ vi.mock("../../infrastructure/repositories/PermissionRepository"); vi.mock("@calcom/features/flags/features.repository"); vi.mock("@calcom/lib/server/repository/membership"); vi.mock("../permission.service"); -vi.mock("@calcom/lib/constants", async (importOriginal) => { - const actual = (await importOriginal()) as typeof import("@calcom/lib/constants"); +vi.mock("@calcom/lib/constants", async () => { + const actual = await vi.importActual("@calcom/lib/constants"); return { ...actual, IS_CALCOM: true, From 73d84e5c1c8078589014d8a00f689dea8156f529 Mon Sep 17 00:00:00 2001 From: Sean Brydon Date: Wed, 3 Sep 2025 09:48:03 +0100 Subject: [PATCH 3/3] remove is_calcom dependancy - not needed due to ff --- .../services/__tests__/permission-check.service.test.ts | 7 ------- .../features/pbac/services/permission-check.service.ts | 3 +-- 2 files changed, 1 insertion(+), 9 deletions(-) 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 c41de0da6ebddf..577d2221c4f4ad 100644 --- a/packages/features/pbac/services/__tests__/permission-check.service.test.ts +++ b/packages/features/pbac/services/__tests__/permission-check.service.test.ts @@ -14,13 +14,6 @@ vi.mock("../../infrastructure/repositories/PermissionRepository"); vi.mock("@calcom/features/flags/features.repository"); vi.mock("@calcom/lib/server/repository/membership"); vi.mock("../permission.service"); -vi.mock("@calcom/lib/constants", async () => { - const actual = await vi.importActual("@calcom/lib/constants"); - return { - ...actual, - IS_CALCOM: true, - }; -}); type MockRepository = { [K in keyof IPermissionRepository]: Mock; diff --git a/packages/features/pbac/services/permission-check.service.ts b/packages/features/pbac/services/permission-check.service.ts index 930018b3f7ddef..3c9ab0236d03d1 100644 --- a/packages/features/pbac/services/permission-check.service.ts +++ b/packages/features/pbac/services/permission-check.service.ts @@ -1,5 +1,4 @@ import { FeaturesRepository } from "@calcom/features/flags/features.repository"; -import { IS_CALCOM } from "@calcom/lib/constants"; import logger from "@calcom/lib/logger"; import { MembershipRepository } from "@calcom/lib/server/repository/membership"; import prisma from "@calcom/prisma"; @@ -17,7 +16,7 @@ import type { import { PermissionRepository } from "../infrastructure/repositories/PermissionRepository"; import { PermissionService } from "./permission.service"; -const DOGFOOD_PBAC_INTERNALLY = IS_CALCOM; +const DOGFOOD_PBAC_INTERNALLY = true; export class PermissionCheckService { private readonly PBAC_FEATURE_FLAG = "pbac" as const;