fix: fixes PBAC roles creation and update when adding Attributes (#23443)

* wip

* restore lock

* remove debug

* add tests + extract util

* Update packages/features/pbac/services/permission.service.ts

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

* fix parse permission string

* use correct permission strings in tests

* use eventType instead of event

* fix import

---------

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

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);
+      });
+    });
+  });
+});

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

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<boolean> {
-    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 };
     });
 

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,

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);
 

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)));

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,

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<PermissionString>((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