refactor: replace isTeamAdminOrOwner with PBAC team.listMembers permission (#24006)

* refactor: replace isTeamAdminOrOwner with PBAC team.listMembers permission - Add canListMembers prop to BookingsProps interface - Implement server-side permission check using PermissionCheckService - Handle organization vs team context as specified - Use ADMIN/OWNER fallback roles for backward compatibility - Replace user?.isTeamAdminOrOwner check in bookings column filter - Fix React Hook dependency arrays for ESLint compliance Co-Authored-By: eunjae@cal.com <hey@eunjae.dev> * refactor: wrap canListMembers in permissions object and simplify server logic - Wrap canListMembers in permissions object for future extensibility - Simplify server-side logic to only use getTeamIdsWithPermission - Remove unused imports (prisma, MembershipRole) - Address user feedback on PR #24006 Co-Authored-By: eunjae@cal.com <hey@eunjae.dev> * docs: add comment explaining canListMembers UI logic - Clarify that teamIdsWithPermission.length > 0 check is for UI purposes - Actual accurate filtering happens server-side for filter values Co-Authored-By: eunjae@cal.com <hey@eunjae.dev> * feat: add e2e test for member filter visibility - Verify that users with the MEMBER role cannot see the member filter - Test creates team with ADMIN and MEMBER users - Confirms UI correctly reflects PBAC permissions - Address user feedback on PR #24006 Co-Authored-By: eunjae@cal.com <hey@eunjae.dev> * fix: correct e2e test team member creation pattern - Replace invalid teamId property with hasTeam and teammates pattern - Fix TypeScript error in booking-filters.e2e.ts - Resolve CI type check failure Co-Authored-By: eunjae@cal.com <hey@eunjae.dev> * fix e2e test * remove booking.read permission from member * add guide * update * resource scope * fix markdown * update usage * update guide --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: sean-brydon <55134778+sean-brydon@users.noreply.github.com>
2025-09-26 06:26:20 -03:00
1 parent 689150d78a
commit a4dbb1a90b
@@ -6,6 +6,8 @@ import { redirect } from "next/navigation";
 import { z } from "zod";

 import { getServerSession } from "@calcom/features/auth/lib/getServerSession";
+import { PermissionCheckService } from "@calcom/features/pbac/services/permission-check.service";
+import { MembershipRole } from "@calcom/prisma/enums";

 import { buildLegacyRequest } from "@lib/buildLegacyCtx";

@@ -33,9 +35,30 @@ const Page = async ({ params }: PageProps) => {
  const t = await getTranslate();
  const session = await getServerSession({ req: buildLegacyRequest(await headers(), await cookies()) });

+  let canReadOthersBookings = false;
+  if (session?.user?.id) {
+    const permissionService = new PermissionCheckService();
+    const userId = session.user.id;
+
+    const teamIdsWithPermission = await permissionService.getTeamIdsWithPermission({
+      userId,
+      permission: "booking.read",
+      fallbackRoles: [MembershipRole.OWNER, MembershipRole.ADMIN],
+    });
+    // We check if teamIdsWithPermission.length > 0.
+    // While this may not be entirely accurate, it's acceptable
+    // because we perform a thorough validation on the server side for the actual filter values.
+    // This variable is primarily for UI purposes.
+    canReadOthersBookings = teamIdsWithPermission.length > 0;
+  }
+
  return (
    <ShellMainAppDir heading={t("bookings")} subtitle={t("bookings_description")}>
-      <BookingsList status={parsed.data.status} userId={session?.user?.id} />
+      <BookingsList
+        status={parsed.data.status}
+        userId={session?.user?.id}
+        permissions={{ canReadOthersBookings }}
+      />
    </ShellMainAppDir>
  );
 };
@@ -57,6 +57,9 @@ const descriptionByStatus: Record<BookingListingStatus, string> = {
 type BookingsProps = {
  status: (typeof validStatuses)[number];
  userId?: number;
+  permissions: {
+    canReadOthersBookings: boolean;
+  };
 };

 function useSystemSegments(userId?: number) {
@@ -111,7 +114,7 @@ type RowData =
      type: "today" | "next";
    };

-function BookingsContent({ status }: BookingsProps) {
+function BookingsContent({ status, permissions }: BookingsProps) {
  const { t } = useLocale();
  const user = useMeQuery().data;
  const tableContainerRef = useRef<HTMLDivElement>(null);
@@ -154,7 +157,7 @@ function BookingsContent({ status }: BookingsProps) {
      href: queryString ? `${tabConfig.path}?${queryString}` : tabConfig.path,
      "data-testid": tabConfig["data-testid"],
    }));
-  }, [searchParams?.toString()]);
+  }, [searchParams]);

  const eventTypeIds = useFilterValue("eventTypeId", ZMultiSelectFilterValue)?.data as number[] | undefined;
  const teamIds = useFilterValue("teamId", ZMultiSelectFilterValue)?.data as number[] | undefined;
@@ -215,7 +218,7 @@ function BookingsContent({ status }: BookingsProps) {
      columnHelper.accessor((row) => row.type === "data" && row.booking.user?.id, {
        id: "userId",
        header: t("member"),
-        enableColumnFilter: user?.isTeamAdminOrOwner ?? false,
+        enableColumnFilter: permissions.canReadOthersBookings,
        enableSorting: false,
        cell: () => null,
        meta: {
@@ -314,7 +317,7 @@ function BookingsContent({ status }: BookingsProps) {
        },
      }),
    ];
-  }, [user, status, t]);
+  }, [user, status, t, permissions.canReadOthersBookings]);

  const isEmpty = useMemo(() => !query.data?.bookings.length, [query.data]);

@@ -352,7 +355,7 @@ function BookingsContent({ status }: BookingsProps) {
        isToday: false,
      })) || []
    );
-  }, [query.data]);
+  }, [query.data, status, user?.timeZone]);

  const bookingsToday = useMemo<RowData[]>(() => {
    return (
@@ -371,7 +374,7 @@ function BookingsContent({ status }: BookingsProps) {
          isToday: true,
        })) ?? []
    );
-  }, [query.data]);
+  }, [query.data, user?.timeZone]);

  const finalData = useMemo<RowData[]>(() => {
    if (status !== "upcoming") {
@@ -0,0 +1,51 @@
+import { expect } from "@playwright/test";
+
+import { test } from "./lib/fixtures";
+
+test.describe.configure({ mode: "parallel" });
+
+test.afterEach(({ users }) => users.deleteAll());
+
+test.describe("Booking Filters", () => {
+  test("Member role should not see the member filter", async ({ page, users }) => {
+    const teamMateName = "team mate 1";
+    await users.create(undefined, {
+      hasTeam: true,
+      isOrg: true,
+      teammates: [{ name: teamMateName }],
+    });
+
+    const allUsers = await users.get();
+    const memberUser = allUsers.find((user) => user.name === teamMateName);
+
+    // eslint-disable-next-line playwright/no-conditional-in-test
+    if (!memberUser) {
+      throw new Error("user should exist");
+    }
+
+    await memberUser.apiLogin();
+    const bookingsGetResponse = page.waitForResponse((response) =>
+      /\/api\/trpc\/bookings\/get.*/.test(response.url())
+    );
+    await page.goto(`/bookings/upcoming`, { waitUntil: "domcontentloaded" });
+    await bookingsGetResponse;
+    await page.locator('[data-testid="add-filter-button"]').click();
+    await expect(page.locator('[data-testid="add-filter-item-userId"]')).toBeHidden();
+  });
+
+  test("Admin role should see the member filter", async ({ page, users }) => {
+    const owner = await users.create(undefined, {
+      hasTeam: true,
+      isOrg: true,
+    });
+
+    await owner.apiLogin();
+    const bookingsGetResponse = page.waitForResponse((response) =>
+      /\/api\/trpc\/bookings\/get.*/.test(response.url())
+    );
+    await page.goto(`/bookings/upcoming`, { waitUntil: "domcontentloaded" });
+    await bookingsGetResponse;
+    await page.locator('[data-testid="add-filter-button"]').click();
+    await expect(page.locator('[data-testid="add-filter-item-userId"]')).toBeVisible();
+  });
+});
@@ -1,10 +1,13 @@
 # PBAC Refactoring Guide

-Quick guide for refactoring Cal.com handlers to use Permission-Based Access Control (PBAC) instead of membership-based queries.
+Quick guide for refactoring Cal.com to use Permission-Based Access Control (PBAC) instead of role-based checks and membership queries.

-## Core Pattern: Team Filtering with PBAC
+## Core Patterns
+
+### 1. Team Filtering with PBAC (API/tRPC handlers)

 **Before (Membership-based)**:
+
 ```typescript
 const teamsToQuery = (
  await prisma.membership.findMany({
@@ -24,63 +27,272 @@ const teamsToQuery = (
 ```

 **After (PBAC-based)**:
+
 ```typescript
 import { PermissionCheckService } from "@calcom/features/pbac/services/permission-check.service";
+import { MembershipRole } from "@calcom/prisma/enums";

 const permissionCheckService = new PermissionCheckService();
-const teamsToQuery = await permissionCheckService.getTeamIdsWithPermission(
-  ctx.user.id, 
-  "team.listMembers"
-);
+const teamsToQuery = await permissionCheckService.getTeamIdsWithPermission({
+  userId: ctx.user.id,
+  permission: "team.listMembers",
+  fallbackRoles: [MembershipRole.ADMIN, MembershipRole.OWNER],
+});
+```
+
+### 2. UI Permission Checks (React components)
+
+**Before (Role-based)**:
+
+```typescript
+// Client-side role check with role-based variable name
+const isTeamAdminOrOwner = user?.isTeamAdminOrOwner ?? false;
+const canSeeMembers = isTeamAdminOrOwner;
+```
+
+**After (PBAC-based)**:
+
+```typescript
+// Server-side permission check in page/layout
+import { MembershipRole } from "@calcom/prisma/enums";
+
+const permissionCheckService = new PermissionCheckService();
+const teamIdsWithPermission = await permissionCheckService.getTeamIdsWithPermission({
+  userId: session.user.id,
+  permission: "team.listMembers",
+  fallbackRoles: [MembershipRole.ADMIN, MembershipRole.OWNER],
+});
+
+const permissions = {
+  canListMembers: teamIdsWithPermission.length > 0,
+};
+
+// Pass to component
+<MyComponent permissions={permissions} />;
 ```

 ## Key Points

 ### 1. Use Direct Permission Strings
+
 - Use `"team.listMembers"` directly instead of `PermissionMapper.toPermissionString()`
 - Simpler and more readable

 ### 2. No Fallback Logic Needed
+
 - `getTeamIdsWithPermission()` handles errors internally
 - Returns empty array `[]` when user has no permissions (this is legitimate)
 - Don't assume empty array means PBAC failure

-### 3. Common Team Permissions
+### 3. Server-Side Permission Checking
+
+- **Move permission checks from client to server** - crucial for security
+- Check permissions in page/layout components, not in UI components
+- Pass permission results as props to maintain separation of concerns
+
+### 4. Permissions Object Pattern
+
+- Wrap permission booleans in a `permissions` object for extensibility
+- Use descriptive names like `canListMembers` instead of raw permission strings
+- Makes it easy to add more permissions later
+
+### 5. Permission-Based Variable Naming
+
+- **Use permission-specific names** like `canListMembers` instead of role-based names like `isTeamAdminOrOwner`
+- Follow the pattern: `can[Action][Resource]` (e.g., `canListMembers`, `canUpdateTeam`, `canInviteUsers`)
+- Makes code more readable and aligns with the actual permission being checked
+- Easier to understand what specific action is being permitted
+
+### 6. Team vs Personal Resources
+
+**Rule**: Use `teamId` to determine if permission checks are needed:
+
+- `teamId` present → Team resource → Check permissions
+- `teamId` null/undefined → Personal resource → Check ownership only
+
+**Example**: `booking.read` permission
+
+- ✅ Has permission: Can read **all team members' bookings**
+- ❌ No permission: Can **still read own bookings**, just not others'
+
+```typescript
+if (resource.teamId) {
+  // Team resource - check permissions
+  const hasPermission = await permissionService.hasPermission(userId, "resource.read", resource.teamId);
+  if (!hasPermission) throw new ForbiddenError();
+} else {
+  // Personal resource - check ownership only
+  if (resource.userId !== currentUserId) throw new ForbiddenError();
+}
+```
+
+### 7. Common Team Permissions
+
+- `"booking.read"` - Read all team members' bookings (not just own)
 - `"team.listMembers"` - List team members
- `"team.read"` - View team details  
+- `"team.read"` - View team details
 - `"team.update"` - Edit team settings
 - `"team.invite"` - Invite members
 - `"team.remove"` - Remove members

 ## Step-by-Step Refactoring

-1. **Add import**:
-   ```typescript
-   import { PermissionCheckService } from "@calcom/features/pbac/services/permission-check.service";
-   ```
+### For API/tRPC Handlers
+
+1. **Add imports**:
+
+```typescript
+import { PermissionCheckService } from "@calcom/features/pbac/services/permission-check.service";
+import { MembershipRole } from "@calcom/prisma/enums";
+```

 2. **Replace membership query**:
-   ```typescript
-   const permissionCheckService = new PermissionCheckService();
-   const teamsToQuery = await permissionCheckService.getTeamIdsWithPermission(
-     ctx.user.id, 
-     "team.listMembers"
-   );
-   ```
+
+```typescript
+const permissionCheckService = new PermissionCheckService();
+const teamsToQuery = await permissionCheckService.getTeamIdsWithPermission({
+  userId: ctx.user.id,
+  permission: "team.listMembers",
+  fallbackRoles: [MembershipRole.ADMIN, MembershipRole.OWNER],
+});
+```

 3. **Keep existing privacy checks** - PBAC doesn't replace organization-level privacy logic

-## Example: listSimpleMembers.handler.ts
+### For UI Components
+
+1. **Move permission check to server** (page/layout component):
+
+```typescript
+import { PermissionCheckService } from "@calcom/features/pbac/services/permission-check.service";
+import { MembershipRole } from "@calcom/prisma/enums";
+
+// In your page component
+const permissionCheckService = new PermissionCheckService();
+const teamIdsWithPermission = await permissionCheckService.getTeamIdsWithPermission({
+  userId: session.user.id,
+  permission: "team.listMembers",
+  fallbackRoles: [MembershipRole.ADMIN, MembershipRole.OWNER],
+});
+
+const permissions = {
+  canListMembers: teamIdsWithPermission.length > 0, // Permission-specific name
+};
+```
+
+2. **Pass permissions as props**:
+
+```typescript
+<BookingsContent permissions={permissions} />
+```
+
+3. **Update component interface**:
+
+```typescript
+interface BookingsProps {
+  permissions: {
+    canListMembers: boolean;
+  };
+}
+```
+
+4. **Replace role checks in component**:
+
+```typescript
+// Replace: const isTeamAdminOrOwner = user?.isTeamAdminOrOwner ?? false
+// With: const canListMembers = permissions.canListMembers
+
+// Use permission-specific variable names throughout your component
+```
+
+## Examples
+
+### API Handler: listSimpleMembers.handler.ts

 **What changed**:
+
 - Replaced 17 lines of membership query with 2 lines of PBAC
 - Removed unnecessary imports and fallback logic
 - Used direct permission string `"team.listMembers"`

 **Result**: Cleaner, more maintainable code that respects fine-grained permissions.

+### UI Component: Bookings Page
+
+**What changed** (from PR #24006):
+
+- Moved permission check from client (`user?.isTeamAdminOrOwner`) to server
+- Added `permissions` prop with `canListMembers` boolean
+- Used `teamIdsWithPermission.length > 0` pattern for UI control
+- Added E2E test to verify member filter visibility
+
+**Before**:
+
+```typescript
+// In component - role-based variable name
+const isTeamAdminOrOwner = user?.isTeamAdminOrOwner ?? false;
+const canSeeMembers = isTeamAdminOrOwner;
+```
+
+**After**:
+
+```typescript
+// In page component (server-side)
+const permissions = {
+  canListMembers: teamIdsWithPermission.length > 0,
+};
+
+// In UI component - permission-specific variable name
+const canListMembers = permissions.canListMembers;
+const canSeeMembers = canListMembers; // Use permission-specific name
+```
+
+**Result**: Secure server-side permission checking with clean UI separation.
+
 ## Verification

+### For All Refactoring
+
 - Run `yarn type-check:ci --force`
 - Ensure existing privacy checks remain intact
 - Test with users who have different permission levels
+
+### For UI Refactoring
+
+- Add E2E tests to verify UI element visibility based on permissions
+- Test with different user roles (ADMIN, OWNER, MEMBER)
+- Verify that permission checks happen server-side, not client-side
+- Confirm that UI elements are properly hidden/shown based on `permissions` props
+
+### Common Pitfalls
+
+- **Don't check permissions client-side** - always do it server-side for security
+- **Don't assume empty array means failure** - it's valid when user has no permissions
+- **Remember to update TypeScript interfaces** when adding new permission props
+- **Don't use role-based variable names** - use permission-specific names instead
+
+## Variable Naming Best Practices
+
+### ✅ Good: Permission-Specific Names
+
+```typescript
+const canListMembers = permissions.canListMembers;
+const canUpdateTeam = permissions.canUpdateTeam;
+const canInviteUsers = permissions.canInviteUsers;
+const canDeleteBookings = permissions.canDeleteBookings;
+```
+
+### ❌ Bad: Role-Based Names
+
+```typescript
+const isTeamAdminOrOwner = user?.isTeamAdminOrOwner;
+const isAdmin = user?.role === "ADMIN";
+const hasPermission = user?.isTeamAdminOrOwner;
+```
+
+### Naming Convention
+
+- Use the pattern: `can[Action][Resource]`
+- Examples: `canListMembers`, `canUpdateTeam`, `canInviteUsers`
+- Makes code self-documenting and easier to understand
+- Aligns variable names with the actual permissions being checked
@@ -0,0 +1,2 @@
+-- Do not allow members to read others' bookings in the team
+delete from "RolePermission" where "roleId" = 'member_role' and resource = 'booking' and action = 'read';