diff --git a/packages/features/pbac/domain/types/__tests__/permission-registry.test.ts b/packages/features/pbac/domain/types/__tests__/permission-registry.test.ts new file mode 100644 index 0000000000..785f94d7f6 --- /dev/null +++ b/packages/features/pbac/domain/types/__tests__/permission-registry.test.ts @@ -0,0 +1,118 @@ +import { describe, it, expect } from "vitest"; + +import { CrudAction, CustomAction, Resource } from "../permission-registry"; +import { isValidPermissionString, parsePermissionString } from "../permission-registry"; + +describe("Permission Registry Utilities", () => { + describe("parsePermissionString", () => { + it("should parse simple permission strings correctly", () => { + const result = parsePermissionString("eventType.create"); + expect(result.resource).toBe("eventType"); + expect(result.action).toBe("create"); + }); + + it("should parse nested resource permission strings correctly", () => { + const result = parsePermissionString("organization.attributes.create"); + expect(result.resource).toBe("organization.attributes"); + expect(result.action).toBe("create"); + }); + + it("should handle wildcard permissions", () => { + const result = parsePermissionString("*.create"); + expect(result.resource).toBe("*"); + expect(result.action).toBe("create"); + }); + + it("should handle custom actions", () => { + const result = parsePermissionString("team.invite"); + expect(result.resource).toBe("team"); + expect(result.action).toBe("invite"); + }); + + it("should handle deeply nested resources", () => { + const result = parsePermissionString("organization.attributes.nested.action"); + expect(result.resource).toBe("organization.attributes.nested"); + expect(result.action).toBe("action"); + }); + }); + + describe("isValidPermissionString", () => { + it("should validate basic permission strings", () => { + expect(isValidPermissionString("eventType.create")).toBe(true); + expect(isValidPermissionString("team.read")).toBe(true); + expect(isValidPermissionString("organization.update")).toBe(true); + }); + + it("should validate nested resource permissions", () => { + expect(isValidPermissionString("organization.attributes.create")).toBe(true); + expect(isValidPermissionString("organization.attributes.read")).toBe(true); + expect(isValidPermissionString("organization.attributes.update")).toBe(true); + expect(isValidPermissionString("organization.attributes.delete")).toBe(true); + }); + + it("should validate wildcard permissions", () => { + expect(isValidPermissionString("*.create")).toBe(true); + expect(isValidPermissionString("eventType.*")).toBe(true); + expect(isValidPermissionString("*.*")).toBe(true); + }); + + it("should validate custom actions", () => { + expect(isValidPermissionString("team.invite")).toBe(true); + expect(isValidPermissionString("team.remove")).toBe(true); + expect(isValidPermissionString("team.changeMemberRole")).toBe(true); + expect(isValidPermissionString("organization.manageBilling")).toBe(true); + expect(isValidPermissionString("booking.readTeamBookings")).toBe(true); + }); + + it("should validate _resource special case", () => { + expect(isValidPermissionString("eventType._resource")).toBe(true); + expect(isValidPermissionString("team._resource")).toBe(true); + expect(isValidPermissionString("organization._resource")).toBe(true); + }); + + it("should reject invalid resource names", () => { + expect(isValidPermissionString("invalidResource.create")).toBe(false); + expect(isValidPermissionString("unknown.read")).toBe(false); + }); + + it("should reject invalid action names", () => { + expect(isValidPermissionString("eventType.invalidAction")).toBe(false); + expect(isValidPermissionString("team.unknownAction")).toBe(false); + }); + + it("should reject malformed permission strings", () => { + expect(isValidPermissionString("invalidformat")).toBe(false); + expect(isValidPermissionString("")).toBe(false); + expect(isValidPermissionString("eventType.")).toBe(false); + expect(isValidPermissionString(".create")).toBe(false); + }); + + it("should reject non-string values", () => { + expect(isValidPermissionString(null)).toBe(false); + expect(isValidPermissionString(undefined)).toBe(false); + expect(isValidPermissionString(123)).toBe(false); + expect(isValidPermissionString({})).toBe(false); + expect(isValidPermissionString([])).toBe(false); + }); + + it("should validate all CRUD actions", () => { + Object.values(CrudAction).forEach((action) => { + expect(isValidPermissionString(`eventType.${action}`)).toBe(true); + }); + }); + + it("should validate all custom actions for appropriate resources", () => { + expect(isValidPermissionString(`team.${CustomAction.Invite}`)).toBe(true); + expect(isValidPermissionString(`team.${CustomAction.Remove}`)).toBe(true); + expect(isValidPermissionString(`team.${CustomAction.ChangeMemberRole}`)).toBe(true); + expect(isValidPermissionString(`organization.${CustomAction.ManageBilling}`)).toBe(true); + expect(isValidPermissionString(`booking.${CustomAction.ReadTeamBookings}`)).toBe(true); + }); + + it("should validate all resources", () => { + Object.values(Resource).forEach((resource) => { + expect(isValidPermissionString(`${resource}.read`)).toBe(true); + }); + }); + }); +}); diff --git a/packages/features/pbac/domain/types/permission-registry.ts b/packages/features/pbac/domain/types/permission-registry.ts index 9ecd3039a5..671438fcc1 100644 --- a/packages/features/pbac/domain/types/permission-registry.ts +++ b/packages/features/pbac/domain/types/permission-registry.ts @@ -58,6 +58,54 @@ export type PermissionRegistry = { export type PermissionString = `${Resource}.${CrudAction | CustomAction}`; +/** + * Parsed permission object containing resource and action parts + */ +export interface ParsedPermission { + resource: string; + action: string; +} + +/** + * Parses a permission string into its resource and action components + * @param permission The permission string to parse + * @returns Parsed permission object with resource and action + */ +export const parsePermissionString = (permission: string): ParsedPermission => { + const lastDotIndex = permission.lastIndexOf("."); + const resource = permission.substring(0, lastDotIndex); + const action = permission.substring(lastDotIndex + 1); + return { resource, action }; +}; + +/** + * Validates a permission string format + * @param val The permission string to validate + * @returns True if valid, false otherwise + */ +export const isValidPermissionString = (val: unknown): val is PermissionString => { + if (typeof val !== "string") return false; + + // Handle special case for _resource + if (val.endsWith("._resource")) { + const resourcePart = val.slice(0, -10); // Remove "._resource" + return Object.values(Resource).includes(resourcePart as Resource); + } + + // Split by the last dot to handle nested resources like "organization.attributes.create" + const lastDotIndex = val.lastIndexOf("."); + if (lastDotIndex === -1) return false; + + const { resource, action } = parsePermissionString(val); + + const isValidResource = Object.values(Resource).includes(resource as Resource); + const isValidAction = + Object.values(CrudAction).includes(action as CrudAction) || + Object.values(CustomAction).includes(action as CustomAction); + + return isValidResource && isValidAction; +}; + /** * Helper function to filter out the _resource property from a ResourceConfig * @param config The ResourceConfig to filter diff --git a/packages/features/pbac/infrastructure/repositories/PermissionRepository.ts b/packages/features/pbac/infrastructure/repositories/PermissionRepository.ts index 52858b4b41..5fe5435131 100644 --- a/packages/features/pbac/infrastructure/repositories/PermissionRepository.ts +++ b/packages/features/pbac/infrastructure/repositories/PermissionRepository.ts @@ -5,7 +5,11 @@ import { PermissionMapper } from "../../domain/mappers/PermissionMapper"; import type { TeamPermissions } from "../../domain/models/Permission"; import type { IPermissionRepository } from "../../domain/repositories/IPermissionRepository"; import type { CrudAction, CustomAction } from "../../domain/types/permission-registry"; -import { Resource, type PermissionString } from "../../domain/types/permission-registry"; +import { + Resource, + type PermissionString, + parsePermissionString, +} from "../../domain/types/permission-registry"; export class PermissionRepository implements IPermissionRepository { private client: PrismaClientWithExtensions; @@ -91,7 +95,7 @@ export class PermissionRepository implements IPermissionRepository { } async checkRolePermission(roleId: string, permission: PermissionString): Promise { - const [resource, action] = permission.split("."); + const { resource, action } = parsePermissionString(permission); const hasPermission = await this.client.rolePermission.findFirst({ where: { roleId, @@ -113,7 +117,7 @@ export class PermissionRepository implements IPermissionRepository { } const permissionPairs = permissions.map((p) => { - const [resource, action] = p.split("."); + const { resource, action } = parsePermissionString(p); return { resource, action }; }); const resourceActions = permissionPairs.map((p) => [p.resource, p.action]); @@ -207,7 +211,7 @@ export class PermissionRepository implements IPermissionRepository { } const permissionPairs = permissions.map((p) => { - const [resource, action] = p.split("."); + const { resource, action } = parsePermissionString(p); return { resource, action }; }); diff --git a/packages/features/pbac/infrastructure/repositories/RoleRepository.ts b/packages/features/pbac/infrastructure/repositories/RoleRepository.ts index d54c1de7ae..280fd4014a 100644 --- a/packages/features/pbac/infrastructure/repositories/RoleRepository.ts +++ b/packages/features/pbac/infrastructure/repositories/RoleRepository.ts @@ -5,6 +5,7 @@ import db from "@calcom/prisma"; import type { Role, RolePermission, PermissionChange, CreateRoleData } from "../../domain/models/Role"; import { RoleType } from "../../domain/models/Role"; +import { parsePermissionString } from "../../domain/types/permission-registry"; import { RoleOutputMapper } from "../mappers/RoleOutputMapper"; export class RoleRepository { @@ -54,7 +55,7 @@ export class RoleRepository { if (data.permissions.length > 0) { const permissionData = data.permissions.map((permission) => { - const [resource, action] = permission.split("."); + const { resource, action } = parsePermissionString(permission); return { id: uuidv4(), roleId: role.id, diff --git a/packages/features/pbac/services/__tests__/permission-diff.service.test.ts b/packages/features/pbac/services/__tests__/permission-diff.service.test.ts index 69dd4d3dd8..aaca9d7908 100644 --- a/packages/features/pbac/services/__tests__/permission-diff.service.test.ts +++ b/packages/features/pbac/services/__tests__/permission-diff.service.test.ts @@ -1,6 +1,7 @@ import type { RolePermission } from "@prisma/client"; import { describe, it, expect } from "vitest"; +import type { PermissionString } from "../../domain/types/permission-registry"; import { PermissionDiffService } from "../permission-diff.service"; describe("PermissionDiffService", () => { @@ -13,13 +14,13 @@ describe("PermissionDiffService", () => { { id: "2", roleId: "role1", resource: "booking", action: "read" }, ] as RolePermission[]; - const newPermissions = ["booking.read", "booking.update", "event.create"]; + const newPermissions = ["booking.read", "booking.update", "eventType.create"] as PermissionString[]; const result = service.calculateDiff(newPermissions, existingPermissions); expect(result.toAdd).toEqual([ { resource: "booking", action: "update" }, - { resource: "event", action: "create" }, + { resource: "eventType", action: "create" }, ]); expect(result.toRemove).toEqual([{ id: "1", roleId: "role1", resource: "booking", action: "create" }]); @@ -37,7 +38,7 @@ describe("PermissionDiffService", () => { }); it("should handle empty existing permissions", () => { - const newPermissions = ["booking.create", "booking.read"]; + const newPermissions = ["booking.create", "booking.read"] as PermissionString[]; const result = service.calculateDiff(newPermissions, []); @@ -53,11 +54,15 @@ describe("PermissionDiffService", () => { { id: "1", roleId: "role1", resource: "booking", action: "create" }, ] as RolePermission[]; - const newPermissions = ["booking.create", "booking._resource", "event.create"]; + const newPermissions = [ + "booking.create", + "booking._resource", + "eventType.create", + ] as PermissionString[]; const result = service.calculateDiff(newPermissions, existingPermissions); - expect(result.toAdd).toEqual([{ resource: "event", action: "create" }]); + expect(result.toAdd).toEqual([{ resource: "eventType", action: "create" }]); expect(result.toRemove).toEqual([]); }); @@ -67,7 +72,7 @@ describe("PermissionDiffService", () => { { id: "2", roleId: "role1", resource: "booking", action: "read" }, ] as RolePermission[]; - const newPermissions = ["booking.create", "booking.read"]; + const newPermissions = ["booking.create", "booking.read"] as PermissionString[]; const result = service.calculateDiff(newPermissions, existingPermissions); diff --git a/packages/features/pbac/services/permission-diff.service.ts b/packages/features/pbac/services/permission-diff.service.ts index d7c13b367d..64d7234fd0 100644 --- a/packages/features/pbac/services/permission-diff.service.ts +++ b/packages/features/pbac/services/permission-diff.service.ts @@ -1,17 +1,19 @@ import type { RolePermission } from "../domain/models/Role"; - -export type PermissionString = string; -export type ParsedPermission = { resource: string; action: string }; +import { + parsePermissionString, + isValidPermissionString, + type PermissionString, + type ParsedPermission, +} from "../domain/types/permission-registry"; export class PermissionDiffService { - private parsePermission(permission: PermissionString): ParsedPermission { - const [resource, action] = permission.split("."); - return { resource, action }; - } - private filterInternalPermissions(permissions: PermissionString[]): PermissionString[] { return permissions.filter((permission) => { - const { action } = this.parsePermission(permission); + // Skip invalid permissions entirely + if (!isValidPermissionString(permission)) { + return false; + } + const { action } = parsePermissionString(permission); return action !== "_resource"; }); } @@ -28,14 +30,14 @@ export class PermissionDiffService { const newSet = new Set( filteredPermissions.map((p) => { - const parsed = this.parsePermission(p); + const parsed = parsePermissionString(p); return this.createPermissionKey(parsed); }) ); // Calculate permissions to add and remove const toAdd = filteredPermissions - .map((p) => this.parsePermission(p)) + .map((p) => parsePermissionString(p)) .filter((p) => !existingSet.has(this.createPermissionKey(p))); const toRemove = existingPermissions.filter((p) => !newSet.has(this.createPermissionKey(p))); diff --git a/packages/features/pbac/services/permission.service.ts b/packages/features/pbac/services/permission.service.ts index a875b99367..98b01b0a3d 100644 --- a/packages/features/pbac/services/permission.service.ts +++ b/packages/features/pbac/services/permission.service.ts @@ -18,12 +18,16 @@ export class PermissionService { validatePermission(permission: PermissionString): PermissionValidationResult { try { const permissionObj = PermissionMapper.fromPermissionString(permission); - const isValid = !!PERMISSION_REGISTRY[permissionObj.resource]?.[permissionObj.action]; + const registryEntry = PERMISSION_REGISTRY[permissionObj.resource]; + const actionEntry = registryEntry?.[permissionObj.action]; + const isValid = !!actionEntry; + return { isValid, error: isValid ? null : `Invalid permission: ${permission}`, }; } catch (error) { + console.debug(`[DEBUG] Error validating permission ${permission}:`, error); if (error instanceof Error) { return { isValid: false, diff --git a/packages/trpc/server/routers/viewer/pbac/_router.tsx b/packages/trpc/server/routers/viewer/pbac/_router.tsx index d93a8eeefc..c3c07b8e09 100644 --- a/packages/trpc/server/routers/viewer/pbac/_router.tsx +++ b/packages/trpc/server/routers/viewer/pbac/_router.tsx @@ -1,7 +1,7 @@ import { z } from "zod"; import { FeaturesRepository } from "@calcom/features/flags/features.repository"; -import { Resource, CrudAction, CustomAction } from "@calcom/features/pbac/domain/types/permission-registry"; +import { isValidPermissionString } from "@calcom/features/pbac/domain/types/permission-registry"; import type { PermissionString } from "@calcom/features/pbac/domain/types/permission-registry"; import { PermissionCheckService } from "@calcom/features/pbac/services/permission-check.service"; import { RoleService } from "@calcom/features/pbac/services/role.service"; @@ -13,21 +13,7 @@ import { router } from "../../../trpc"; // Create a Zod schema for PermissionString that validates the format const permissionStringSchema = z.custom((val) => { - if (typeof val !== "string") return false; - - const [resource, action] = val.split("."); - - const isValidResource = Object.values(Resource).includes(resource as Resource); - - if (action === "_resource") { - return true; - } - - const isValidAction = - Object.values(CrudAction).includes(action as CrudAction) || - Object.values(CustomAction).includes(action as CustomAction); - - return isValidResource && isValidAction; + return isValidPermissionString(val); }, "Invalid permission string format. Must be 'resource.action' where resource and action are valid enums"); // Schema for creating/updating roles