From 788fbdbe5e77bf51742a4ba2d71b7801adb88cd8 Mon Sep 17 00:00:00 2001 From: Eunjae Lee Date: Wed, 24 Sep 2025 10:56:03 +0200 Subject: [PATCH] refactor: implement PBAC for team member listing in listSimpleMembers handler (#24005) * refactor: implement PBAC for team member listing - Replace membership-based team filtering with getTeamIdsWithPermission - Use team.listMembers permission for access control - Maintain fallback to original logic when PBAC fails - Add comprehensive PBAC refactoring guide for future use Fixes team fetching logic to use Permission-Based Access Control while preserving existing functionality and privacy checks. Co-Authored-By: eunjae@cal.com * docs: move PBAC refactoring guide to packages/features/pbac/ Move the PBAC refactoring guide to the appropriate location within the PBAC feature package for better organization and discoverability. Co-Authored-By: eunjae@cal.com * refactor: remove unnecessary try-catch wrapper The getTeamIdsWithPermission method already handles all errors internally and returns an empty array instead of throwing exceptions, making the try-catch wrapper redundant. Simplified to use direct fallback logic based on empty array return. Co-Authored-By: eunjae@cal.com * refactor: simplify PBAC implementation - Remove flawed fallback logic that assumed empty array meant PBAC failure - Use direct string 'team.listMembers' instead of PermissionMapper - Remove unused imports (PermissionMapper, Resource, CustomAction) - Empty array from getTeamIdsWithPermission is legitimate (no permissions) Co-Authored-By: eunjae@cal.com * docs: simplify PBAC refactoring guide - Reduce from 260 to 87 lines by removing bloated content - Focus on core pattern: direct permission strings, no fallback logic - Align with actual PR implementation - Remove verbose theoretical sections and complex patterns Co-Authored-By: eunjae@cal.com --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- .../features/pbac/PBAC_REFACTORING_GUIDE.md | 86 +++++++++++++++++++ .../viewer/teams/listSimpleMembers.handler.ts | 22 +---- 2 files changed, 89 insertions(+), 19 deletions(-) create mode 100644 packages/features/pbac/PBAC_REFACTORING_GUIDE.md diff --git a/packages/features/pbac/PBAC_REFACTORING_GUIDE.md b/packages/features/pbac/PBAC_REFACTORING_GUIDE.md new file mode 100644 index 0000000000..dba73fc0c1 --- /dev/null +++ b/packages/features/pbac/PBAC_REFACTORING_GUIDE.md @@ -0,0 +1,86 @@ +# PBAC Refactoring Guide + +Quick guide for refactoring Cal.com handlers to use Permission-Based Access Control (PBAC) instead of membership-based queries. + +## Core Pattern: Team Filtering with PBAC + +**Before (Membership-based)**: +```typescript +const teamsToQuery = ( + await prisma.membership.findMany({ + where: { + userId: ctx.user.id, + accepted: true, + NOT: [ + { + role: MembershipRole.MEMBER, + team: { isPrivate: true }, + }, + ], + }, + select: { teamId: true }, + }) +).map((membership) => membership.teamId); +``` + +**After (PBAC-based)**: +```typescript +import { PermissionCheckService } from "@calcom/features/pbac/services/permission-check.service"; + +const permissionCheckService = new PermissionCheckService(); +const teamsToQuery = await permissionCheckService.getTeamIdsWithPermission( + ctx.user.id, + "team.listMembers" +); +``` + +## 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 +- `"team.listMembers"` - List team members +- `"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"; + ``` + +2. **Replace membership query**: + ```typescript + const permissionCheckService = new PermissionCheckService(); + const teamsToQuery = await permissionCheckService.getTeamIdsWithPermission( + ctx.user.id, + "team.listMembers" + ); + ``` + +3. **Keep existing privacy checks** - PBAC doesn't replace organization-level privacy logic + +## Example: 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. + +## Verification + +- Run `yarn type-check:ci --force` +- Ensure existing privacy checks remain intact +- Test with users who have different permission levels diff --git a/packages/trpc/server/routers/viewer/teams/listSimpleMembers.handler.ts b/packages/trpc/server/routers/viewer/teams/listSimpleMembers.handler.ts index b4d0cf0cbb..6c22550984 100644 --- a/packages/trpc/server/routers/viewer/teams/listSimpleMembers.handler.ts +++ b/packages/trpc/server/routers/viewer/teams/listSimpleMembers.handler.ts @@ -2,8 +2,8 @@ * Simplified version of legacyListMembers.handler.ts that returns basic member info. * Used for filtering people on /bookings. */ +import { PermissionCheckService } from "@calcom/features/pbac/services/permission-check.service"; import type { PrismaClient } from "@calcom/prisma"; -import { MembershipRole } from "@calcom/prisma/enums"; import type { TrpcSessionUser } from "@calcom/trpc/server/types"; type ListSimpleMembersOptions = { @@ -22,24 +22,8 @@ export const listSimpleMembers = async ({ ctx }: ListSimpleMembersOptions) => { return []; } - // query all teams the user is a member of and the team is not private - const teamsToQuery = ( - await prisma.membership.findMany({ - where: { - userId: ctx.user.id, - accepted: true, - NOT: [ - { - role: MembershipRole.MEMBER, - team: { - isPrivate: true, - }, - }, - ], - }, - select: { teamId: true }, - }) - ).map((membership) => membership.teamId); + const permissionCheckService = new PermissionCheckService(); + const teamsToQuery = await permissionCheckService.getTeamIdsWithPermission(ctx.user.id, "team.listMembers"); if (!teamsToQuery.length) { return [];