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 <me@alexvanandel.com>
This commit is contained in:
sean-brydon
co-authored by
Alex van Andel
Alex van Andel
parent
3661860433
commit
41553f6c01
@@ -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,
|
||||
|
||||
@@ -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
|
||||
*/
|
||||
|
||||
Reference in New Issue
Block a user