From a4dbb1a90b8c3ddd39ab082d5969d77a4cf3c98b Mon Sep 17 00:00:00 2001 From: Eunjae Lee Date: Fri, 26 Sep 2025 11:26:20 +0200 Subject: [PATCH] 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 * 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 * 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 * 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 * 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 * 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> --- .../(main-nav)/bookings/[status]/page.tsx | 25 +- .../bookings/views/bookings-listing-view.tsx | 15 +- apps/web/playwright/booking-filters.e2e.ts | 51 ++++ .../features/pbac/PBAC_REFACTORING_GUIDE.md | 252 ++++++++++++++++-- .../migration.sql | 2 + 5 files changed, 318 insertions(+), 27 deletions(-) create mode 100644 apps/web/playwright/booking-filters.e2e.ts create mode 100644 packages/prisma/migrations/20250924082337_remove_booking_read_permission_from_member/migration.sql diff --git a/apps/web/app/(use-page-wrapper)/(main-nav)/bookings/[status]/page.tsx b/apps/web/app/(use-page-wrapper)/(main-nav)/bookings/[status]/page.tsx index d50b09a5bf..21379a8a6f 100644 --- a/apps/web/app/(use-page-wrapper)/(main-nav)/bookings/[status]/page.tsx +++ b/apps/web/app/(use-page-wrapper)/(main-nav)/bookings/[status]/page.tsx @@ -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 ( - + ); }; diff --git a/apps/web/modules/bookings/views/bookings-listing-view.tsx b/apps/web/modules/bookings/views/bookings-listing-view.tsx index 33a7b76534..c4ccbab26e 100644 --- a/apps/web/modules/bookings/views/bookings-listing-view.tsx +++ b/apps/web/modules/bookings/views/bookings-listing-view.tsx @@ -57,6 +57,9 @@ const descriptionByStatus: Record = { 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(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(() => { return ( @@ -371,7 +374,7 @@ function BookingsContent({ status }: BookingsProps) { isToday: true, })) ?? [] ); - }, [query.data]); + }, [query.data, user?.timeZone]); const finalData = useMemo(() => { if (status !== "upcoming") { diff --git a/apps/web/playwright/booking-filters.e2e.ts b/apps/web/playwright/booking-filters.e2e.ts new file mode 100644 index 0000000000..b404a22772 --- /dev/null +++ b/apps/web/playwright/booking-filters.e2e.ts @@ -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(); + }); +}); diff --git a/packages/features/pbac/PBAC_REFACTORING_GUIDE.md b/packages/features/pbac/PBAC_REFACTORING_GUIDE.md index dba73fc0c1..77f26c59d8 100644 --- a/packages/features/pbac/PBAC_REFACTORING_GUIDE.md +++ b/packages/features/pbac/PBAC_REFACTORING_GUIDE.md @@ -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 +; ``` ## 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 + +``` + +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 diff --git a/packages/prisma/migrations/20250924082337_remove_booking_read_permission_from_member/migration.sql b/packages/prisma/migrations/20250924082337_remove_booking_read_permission_from_member/migration.sql new file mode 100644 index 0000000000..f890eefe33 --- /dev/null +++ b/packages/prisma/migrations/20250924082337_remove_booking_read_permission_from_member/migration.sql @@ -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';