refactor: replace isTeamAdminOrOwner with PBAC team.listMembers permission#24006
refactor: replace isTeamAdminOrOwner with PBAC team.listMembers permission#24006eunjae-lee merged 17 commits intomainfrom
Conversation
…ssion - 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>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Walkthrough
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (3)**/*.tsx📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Files:
**/*.{ts,tsx,js,jsx}⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (4)📓 Common learnings📚 Learning: 2025-09-24T11:53:40.185ZApplied to files:
📚 Learning: 2025-08-27T12:15:43.830ZApplied to files:
📚 Learning: 2025-07-15T12:59:34.389ZApplied to files:
🧬 Code graph analysis (1)apps/web/app/(use-page-wrapper)/(main-nav)/bookings/[status]/page.tsx (3)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…er 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>
- 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>
- 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>
- 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>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
eunjae-lee
force-pushed
the
devin/pbac-refactor-bookings-1758631859
branch
from
3f7a8a1 to
64694e4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (11)
packages/prisma/migrations/20250924082337_remove_booking_read_permission_from_member/migration.sql (1)
1-1: Comment is clear; consider naming the exact permission.Nit: Reference the exact permission tuple to aid future grep/debug (e.g., booking/read).
apps/web/playwright/booking-filters.e2e.ts (3)
11-16: Use unique teammate names to avoid collisions across parallel runsHardcoding
"team mate 1"can collide under parallel execution or across retries. Generate a unique suffix to ensure reliable lookup.-const teamMateName = "team mate 1"; +const teamMateName = `team mate ${Date.now()}-${Math.random().toString(36).slice(2, 8)}`;
18-24: Avoid globalusers.get()+ name search; prefer direct handles from the fixtureSearching all users by name risks matching the wrong user under parallelism. If the fixture returns created entities, capture and use that reference directly.
If supported by your fixture, prefer:
const { teammates } = await users.create(undefined, { hasTeam: true, isOrg: true, teammates: [{ name: teamMateName }], }); const memberUser = teammates[0];If not supported today, consider extending the fixture to return created teammates.
27-31: Make the network wait more robust
waitForResponsemay resolve on the first matching request; SPA navigations can trigger multiple TRPC calls. Consider waiting for navigation plus the response together or for network idle.-const bookingsGetResponse = page.waitForResponse((response) => - /\/api\/trpc\/bookings\/get.*/.test(response.url()) -); -await page.goto(`/bookings/upcoming`, { waitUntil: "domcontentloaded" }); -await bookingsGetResponse; +await Promise.all([ + page.waitForResponse((response) => /\/api\/trpc\/bookings\/get.*/.test(response.url())), + page.goto(`/bookings/upcoming`, { waitUntil: "networkidle" }), +]);apps/web/modules/bookings/views/bookings-listing-view.tsx (2)
358-359: Reduce dayjs work in hot pathsBoth “upcoming” filtering and “today” grouping recompute formatted dates per booking and per render. Precompute the “today” key and reuse parsed values to cut allocations.
Example refactor (outside the selected lines, for illustration):
const tz = user?.timeZone; const todayKey = dayjs().tz(tz).format("YYYY-MM-DD"); const isSameLocalDay = (d: string | Date) => dayjs(d).tz(tz).format("YYYY-MM-DD") === todayKey; // In filter for upcoming: return dayjs(booking.startTime).tz(tz).format("YYYY-MM-DD") !== todayKey;This avoids multiple
dayjs()calls and repeatedformat()invocations per row.
377-377: Apply the same precompute optimization to bookingsTodayReuse
todayKey/isSameLocalDayto reduce per-row allocations in this map/filter.apps/web/app/(use-page-wrapper)/(main-nav)/bookings/[status]/page.tsx (1)
52-56: Optional: Rename to reflect semanticsConsider renaming
canReadOthersBookingstocanListMembersacross the prop and usage to better match the permission being checked.If you opt to do this, update:
- BookingsProps in
bookings-listing-view.tsx- Column gating at
enableColumnFilter- The prop here in
page.tsx- Any tests referencing the behavior
packages/features/pbac/PBAC_REFACTORING_GUIDE.md (4)
111-119: Make the “booking.read” example consistent.The snippet introduces “booking.read” but uses generic “resource.*” identifiers, which may confuse readers.
Use booking-specific names:
-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(); -} +if (booking.teamId) { + // Team resource - check permissions + const hasPermission = await permissionService.hasPermission(currentUserId, "booking.read", booking.teamId); + if (!hasPermission) throw new ForbiddenError(); +} else { + // Personal resource - check ownership only + if (booking.userId !== currentUserId) throw new ForbiddenError(); +}
73-78: Clarify “No Fallback Logic Needed” to distinguish no-permission vs. errors.As written, it can be read as endorsing error suppression. Recommend making error behavior explicit to avoid masking outages.
- - `getTeamIdsWithPermission()` handles errors internally - - Returns empty array `[]` when user has no permissions (this is legitimate) - - Don't assume empty array means PBAC failure + - `getTeamIdsWithPermission()` returns an empty array `[]` when the user legitimately lacks the permission. + - Do not treat exceptions the same as “no permission.” Surface/track errors (telemetry/logs) or let them propagate, rather than converting them to `[]`. + - An empty array means “no permission,” not “PBAC failure.”
52-61: Add org-vs-team scope example (matches PR behavior).PR logic checks “team.listMembers” at org scope when the user belongs to an organization; otherwise it aggregates team-scoped permissions. Add this snippet to guide implementers.
// Server-side permission check in page/layout const permissionCheckService = new PermissionCheckService(); -const teamIdsWithPermission = await permissionCheckService.getTeamIdsWithPermission( - session.user.id, - "team.listMembers" -); +const orgId = session.user.organizationId; +const teamIdsWithPermission = orgId + // Org context: check org-scoped permission + ? (await permissionCheckService.hasPermission(session.user.id, "team.listMembers", orgId)) ? ["ORG_SCOPE"] : [] + // No org: collect team IDs where the user has permission + : await permissionCheckService.getTeamIdsWithPermission(session.user.id, "team.listMembers"); const permissions = { - canListMembers: teamIdsWithPermission.length > 0, + // In org scope, a boolean is enough; in team scope, >0 team IDs implies permission. + canListMembers: Array.isArray(teamIdsWithPermission) + ? teamIdsWithPermission.length > 0 + : Boolean(teamIdsWithPermission), };Please confirm
hasPermission(userId, permission, scopeId)supports org scope; if not, we should adjust the example to the correct API.
86-96: Reinforce naming guidance with a type-safe pattern.Consider showing a typed permissions bag to prevent typos and ease discovery.
-// Pass to component -<MyComponent permissions={permissions} />; +// Example: typed permissions object helps avoid stringly-typed mistakes +type BookingsPermissions = { + canListMembers: boolean; + // add more as needed +}; +const permissions: BookingsPermissions = { canListMembers }; +<MyComponent permissions={permissions} />;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
apps/web/app/(use-page-wrapper)/(main-nav)/bookings/[status]/page.tsx(2 hunks)apps/web/modules/bookings/views/bookings-listing-view.tsx(7 hunks)apps/web/playwright/booking-filters.e2e.ts(1 hunks)packages/features/pbac/PBAC_REFACTORING_GUIDE.md(2 hunks)packages/prisma/migrations/20250924082337_remove_booking_read_permission_from_member/migration.sql(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
apps/web/app/(use-page-wrapper)/(main-nav)/bookings/[status]/page.tsxapps/web/modules/bookings/views/bookings-listing-view.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
apps/web/app/(use-page-wrapper)/(main-nav)/bookings/[status]/page.tsxapps/web/playwright/booking-filters.e2e.tsapps/web/modules/bookings/views/bookings-listing-view.tsx
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
apps/web/app/(use-page-wrapper)/(main-nav)/bookings/[status]/page.tsxapps/web/playwright/booking-filters.e2e.tsapps/web/modules/bookings/views/bookings-listing-view.tsx
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
apps/web/playwright/booking-filters.e2e.ts
🧠 Learnings (1)
📚 Learning: 2025-08-27T13:32:46.887Z
Learnt from: supalarry
PR: calcom/cal.com#23364
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/transformers/internal-to-api/internal-to-api.spec.ts:295-296
Timestamp: 2025-08-27T13:32:46.887Z
Learning: In calcom/cal.com, when transforming booking fields from internal to API format, tests in organizations-event-types.e2e-spec.ts already expect name field label and placeholder to be empty strings ("") rather than undefined. PR changes that set these to explicit empty strings are typically fixing implementation to match existing test expectations rather than breaking changes.
Applied to files:
apps/web/playwright/booking-filters.e2e.ts
🧬 Code graph analysis (1)
apps/web/app/(use-page-wrapper)/(main-nav)/bookings/[status]/page.tsx (2)
packages/features/auth/lib/next-auth-options.ts (1)
session(746-771)packages/features/pbac/services/permission-check.service.ts (1)
PermissionCheckService(19-290)
🪛 markdownlint-cli2 (0.18.1)
packages/features/pbac/PBAC_REFACTORING_GUIDE.md
171-171: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (8)
packages/prisma/migrations/20250924082337_remove_booking_read_permission_from_member/migration.sql (1)
2-2: Avoid hard-coding roleId; delete by joining on the canonical Role (slug/name).
'member_role'risks being env-specific. If IDs differ across environments, the delete becomes a no-op, leaving the permission in place. Prefer joining via Role’s stable identifier (e.g., slug/name).Proposed safer SQL (adjust column/table names to your schema):
-delete from "RolePermission" where "roleId" = 'member_role' and resource = 'booking' and action = 'read'; +delete from "RolePermission" rp +using "Role" r +where rp."roleId" = r.id + and r.slug = 'member' -- or r.name = 'MEMBER' + and rp.resource = 'booking' + and rp.action = 'read';Verify schema and constants with:
apps/web/playwright/booking-filters.e2e.ts (2)
33-34: LGTM: Member should not see the filterAsserting the member user filter is hidden aligns with the permission‑gated UI.
49-50: LGTM: Admin should see the filterVisibility assertion for admins is correct and matches the intended PBAC behavior.
apps/web/modules/bookings/views/bookings-listing-view.tsx (4)
117-117: Prop threading into BookingsContent is correctAccepting
permissionsat this level keeps the table logic cleanly decoupled from server checks.
155-161: UsingsearchParamsobject as a dep is fineRelying on
searchParamsidentity to recompute tabs is acceptable in Next.js app router. The tab query preservation logic is correct.
218-223: Correctly gates the member filter by permission
enableColumnFilter: permissions.canReadOthersBookingsreplaces role checks and aligns with PBAC. Good.
320-321: Good: included permission flag in memo depsThis prevents stale column config when permissions or i18n change.
apps/web/app/(use-page-wrapper)/(main-nav)/bookings/[status]/page.tsx (1)
52-56: LGTM: Plumbs permission to BookingsListPassing the computed boolean to the table component follows the new contract.
| // 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; |
There was a problem hiding this comment.
We need to keep in mind fallback roles here - getTeamIdsWithpermissions doesnt account for that
There was a problem hiding this comment.
for the future reference: getTeamIdsWithpermissions now supports fallbackRoles since #24042
| @@ -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'; | |||
There was a problem hiding this comment.
LGTM = not sure why this was in the members role
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
packages/features/pbac/PBAC_REFACTORING_GUIDE.md (7)
73-79: Document transitional fallback explicitly (guide currently contradicts PR behavior).This section says no fallback is needed, but the PR uses ADMIN/OWNER fallback for compatibility. Add a brief “Transitional fallback” note to avoid confusion.
Apply:
### 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 + + - `getTeamIdsWithPermission()` handles errors internally + - Returns empty array `[]` when user has no permissions (this is legitimate) + - Don't assume empty array means PBAC failure + - Transitional fallback: During migration, you may optionally gate with legacy role checks (e.g., ADMIN/OWNER) as a temporary fallback. Keep this isolated server-side and plan removal.
110-118: Use concrete permission in example or clarify placeholder.Using "resource.read" is abstract. Either switch to a concrete example (e.g., "booking.read") or add a brief note that the permission string must match the resource/action.
Apply (option A):
- const hasPermission = await permissionService.hasPermission(userId, "resource.read", resource.teamId); + const hasPermission = await permissionService.hasPermission(userId, "booking.read", resource.teamId);Or (option B) add a comment:
- const hasPermission = await permissionService.hasPermission(userId, "resource.read", resource.teamId); + // Use the permission aligned to the resource/action (e.g., "booking.read", "team.update") + const hasPermission = await permissionService.hasPermission(userId, "resource.read", resource.teamId);
142-148: Clarify permission choice in handler example (don’t hardcode listMembers for non-member flows).Add guidance to pick the correct permission per endpoint; avoid implying "team.listMembers" is the default for all team queries.
Apply:
const permissionCheckService = new PermissionCheckService(); const teamsToQuery = await permissionCheckService.getTeamIdsWithPermission( ctx.user.id, - "team.listMembers" + "team.listMembers" // Choose the permission aligned to the endpoint (e.g., "booking.read" for bookings APIs) );
180-185: Align props interface with bookings permission (rename to canReadOthersBookings).This prevents accidental reuse of the wrong permission flag.
Apply:
interface BookingsProps { permissions: { - canListMembers: boolean; + canReadOthersBookings: boolean; }; }
190-194: Update usage example to the renamed permission.Apply:
// Replace: const isTeamAdminOrOwner = user?.isTeamAdminOrOwner ?? false - // With: const canListMembers = permissions.canListMembers + // With: const canReadOthersBookings = permissions.canReadOthersBookings
210-216: Also adjust the permission retrieval in this section to use "booking.read".Make the preceding retrieval consistent with the changed flag.
Apply:
-// Moved permission check from client (`user?.isTeamAdminOrOwner`) to server -// Added `permissions` prop with `canListMembers` boolean -// Used `teamIdsWithPermission.length > 0` pattern for UI control +// Moved permission check from client (`user?.isTeamAdminOrOwner`) to server +// Added `permissions` prop with `canReadOthersBookings` boolean +// Used `teamIdsWithPermission.length > 0` pattern with "booking.read" for UI control
98-106: Add org-vs-team context note.PR logic handles org-level aggregation for permissions. Add a short note here to reflect that pattern so consumers don’t miss org-context checks.
Apply:
### 6. Team vs Personal Resources + +Note: If the user belongs to an organization, evaluate permissions at the org scope when applicable (e.g., aggregate teamIds with permission via org context before enforcing).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/features/pbac/PBAC_REFACTORING_GUIDE.md(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: eunjae-lee
PR: calcom/cal.com#24006
File: apps/web/app/(use-page-wrapper)/(main-nav)/bookings/[status]/page.tsx:37-48
Timestamp: 2025-09-24T11:53:40.162Z
Learning: In bookings listing views, when checking permissions for the member filter UI, use "booking.read" permission rather than "team.listMembers" because the filter's purpose is to read bookings of other team members, not just to list the members themselves. The permission check should align with the actual capability being granted.
📚 Learning: 2025-09-24T11:53:40.162Z
Learnt from: eunjae-lee
PR: calcom/cal.com#24006
File: apps/web/app/(use-page-wrapper)/(main-nav)/bookings/[status]/page.tsx:37-48
Timestamp: 2025-09-24T11:53:40.162Z
Learning: In bookings listing views, when checking permissions for the member filter UI, use "booking.read" permission rather than "team.listMembers" because the filter's purpose is to read bookings of other team members, not just to list the members themselves. The permission check should align with the actual capability being granted.
Applied to files:
packages/features/pbac/PBAC_REFACTORING_GUIDE.md
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (2)
packages/features/pbac/PBAC_REFACTORING_GUIDE.md (2)
171-176: Re-run markdownlint for fenced code blocks; ensure all fences have a language and use triple backticks.A previous review flagged MD040 and 4‑backtick fences around here. It looks corrected, but please re-verify to avoid CI noise.
Run:
#!/bin/bash set -euo pipefail file='packages/features/pbac/PBAC_REFACTORING_GUIDE.md' echo "Unlabeled fences (should be empty):" rg -nP '^\s*```(?!\w)' "$file" || true echo echo "Four-backtick fences (should be empty):" rg -nP '````' "$file" || true echo echo "Fence summary around 160-190:" nl -ba -w1 -s': ' "$file" | sed -n '160,190p'
121-129: Good list of common permissions.We leveraged the retrieved learning that bookings member filter should use "booking.read". Please confirm this aligns with the final PR implementation in apps/web/app/(use-page-wrapper)/(main-nav)/bookings/[status]/page.tsx.
E2E results are ready! |
dosubot
bot
added
bookings
keithwillcode
left a comment
keithwillcode
left a comment
There was a problem hiding this comment.
Migration looks good
3 participants
What does this PR do?
This PR refactors the bookings listing view to replace the role-based
user?.isTeamAdminOrOwnercheck with PBAC (Permission-Based Access Control) using the "team.listMembers" permission. The change moves permission checking from client-side role checks to server-side permission validation.Key Changes:
apps/web/app/(use-page-wrapper)/(main-nav)/bookings/[status]/page.tsxcanListMembersboolean prop down toBookingsContentcomponentuser?.isTeamAdminOrOwner ?? falsewithcanListMembersfor the member column filterPermission Logic:
getTeamIdsWithPermission[MembershipRole.ADMIN, MembershipRole.OWNER]as fallback roles for backward compatibilityVisual Demo (For contributors especially)
N/A - This is a backend permission refactor without visual UI changes.
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Test with different user roles:
Test Steps:
/bookings/upcoming(or any booking status page)[MembershipRole.ADMIN, MembershipRole.OWNER]matches originalisTeamAdminOrOwnerbehavior exactlyChecklist
Link to Devin run: https://app.devin.ai/sessions/6512603a66cf4744a0a71b31f1a75e73
Requested by: @eunjae-lee