From 41553f6c01f6ef90a87b0dc79abeba63e566b534 Mon Sep 17 00:00:00 2001 From: sean-brydon <55134778+sean-brydon@users.noreply.github.com> Date: Mon, 15 Sep 2025 14:57:11 +0100 Subject: [PATCH] feat: PBAC remove dogfood for safe checking (#23846) * Remove dogfood for safe checking * chore: Remove now redundant test cases --------- Co-authored-by: Alex van Andel --- .../permission-check.service.test.ts | 74 ----------------- .../pbac/services/permission-check.service.ts | 80 +------------------ 2 files changed, 2 insertions(+), 152 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 d44ad1b829..8b1dc954ee 100644 --- a/packages/features/pbac/services/__tests__/permission-check.service.test.ts +++ b/packages/features/pbac/services/__tests__/permission-check.service.test.ts @@ -153,43 +153,6 @@ 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, @@ -295,43 +258,6 @@ describe("PermissionCheckService", () => { expect(mockRepository.checkRolePermissions).not.toHaveBeenCalled(); }); - it("should fallback to legacy roles when PBAC permissions 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.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, diff --git a/packages/features/pbac/services/permission-check.service.ts b/packages/features/pbac/services/permission-check.service.ts index 3c9ab0236d..8b9c67d2e5 100644 --- a/packages/features/pbac/services/permission-check.service.ts +++ b/packages/features/pbac/services/permission-check.service.ts @@ -16,8 +16,6 @@ import type { import { PermissionRepository } from "../infrastructure/repositories/PermissionRepository"; import { PermissionService } from "./permission.service"; -const DOGFOOD_PBAC_INTERNALLY = true; - export class PermissionCheckService { private readonly PBAC_FEATURE_FLAG = "pbac" as const; private readonly logger = logger.getSubLogger({ prefix: ["PermissionCheckService"] }); @@ -128,21 +126,7 @@ export class PermissionCheckService { return false; } - 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.hasPermission({ membershipId: membership.id }, permission); } return this.checkFallbackRoles(membership.role, fallbackRoles); @@ -191,21 +175,7 @@ export class PermissionCheckService { return false; } - 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.hasPermissions({ membershipId: membership.id }, permissions); } return this.checkFallbackRoles(membership.role, fallbackRoles); @@ -282,52 +252,6 @@ 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 */