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 <hey@eunjae.dev> * 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 <hey@eunjae.dev> * 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 <hey@eunjae.dev> * 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 <hey@eunjae.dev> * 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 <hey@eunjae.dev> --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
This commit is contained in:
@@ -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
|
||||
@@ -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 [];
|
||||
|
||||
Reference in New Issue
Block a user