fix: get bookings handler for pbac and fallback roles#28
Conversation
WalkthroughThe changes implement organization-scoped permission checking across the PBAC system and refactor the bookings handler to replace admin/owner membership-based access control with a permission-based system. This includes new optional orgId parameters in repository and service layers, organization hierarchy-aware query logic, comprehensive integration and unit tests, and restructured booking access filtering. Changes
Sequence Diagram(s)sequenceDiagram
participant Handler as Bookings Handler
participant Service as PermissionCheckService
participant Repo as PermissionRepository
participant DB as Database
Handler->>Service: getTeamIdsWithPermission(userId, permission, fallbackRoles, orgId?)
Service->>Repo: getTeamIdsWithPermission(params with orgId)
par Parallel Fetch
Repo->>DB: Query PBAC permissions<br/>JOIN Team WHERE orgId/parentId
Repo->>DB: Query fallback roles<br/>JOIN Team WHERE orgId/parentId
end
DB-->>Repo: PBAC team IDs
DB-->>Repo: Fallback team IDs
Repo->>Repo: Deduplicate & union team IDs
Repo-->>Service: Combined team IDs (org-scoped)
Service-->>Handler: Team IDs with booking.read permission
Handler->>Handler: Filter bookings by accessible team IDs
Handler-->>Client: Filtered booking results
sequenceDiagram
participant Client
participant Handler as get.handler
participant PermissionService as PermissionCheckService
participant Repo as PermissionRepository
Client->>Handler: getHandler(request with userId filter)
Handler->>Handler: Check if filtering by own userId
alt User filtering by own ID
Handler-->>Client: Return user's own bookings
else User filtering by other userIds
Handler->>PermissionService: getTeamIdsWithPermission(userId, booking.read)
PermissionService->>Repo: Query org-scoped teams
Repo-->>PermissionService: Team IDs where user has booking.read
PermissionService-->>Handler: Team IDs
Handler->>Handler: Check if user accessible via permission-scoped teams
alt User lacks required permission
Handler-->>Client: FORBIDDEN error
else User has permission
Handler->>Handler: Filter bookings by allowed team scope
Handler-->>Client: Filtered bookings
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes The changes involve substantial refactoring across multiple layers (repository, service, handler) with new org-scoped query logic, complex SQL filtering with joins and hierarchical constraints, a comprehensive test suite with numerous scenarios, and significant control flow changes replacing membership-based access with permission-based access. The heterogeneous nature of modifications (interface, implementation, logic, and test changes) requires careful verification of correctness across layers. Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/features/pbac/infrastructure/repositories/PermissionRepository.ts (1)
211-223: Critical:orgIdparameter is not forwarded togetTeamIdsWithPermissions.The
getTeamIdsWithPermissionmethod acceptsorgIdbut does not pass it togetTeamIdsWithPermissions, causing org-scoped filtering to be ignored when using this method.🐛 Fix: Forward orgId to getTeamIdsWithPermissions
async getTeamIdsWithPermission({ userId, permission, fallbackRoles, orgId, }: { userId: number; permission: PermissionString; fallbackRoles: MembershipRole[]; orgId?: number; }): Promise<number[]> { - return this.getTeamIdsWithPermissions({ userId, permissions: [permission], fallbackRoles }); + return this.getTeamIdsWithPermissions({ userId, permissions: [permission], fallbackRoles, orgId }); }
🤖 Fix all issues with AI agents
In `@packages/features/pbac/services/permission-check.service.ts`:
- Line 6: Remove the unused TRPCError import from the top-level imports in
permission-check.service.ts: delete the `TRPCError` specifier from the import
statement so only used symbols remain, and run the linter/TS build to confirm
there are no remaining references to `TRPCError`.
🧹 Nitpick comments (2)
packages/features/pbac/infrastructure/repositories/__tests__/PermissionRepository.integration-test.ts (1)
1050-1053: Cleanup may fail due to foreign key constraint order.The cleanup deletes teams before memberships could potentially reference them. While the current order (
membership.deleteManybeforeteam.deleteMany) is correct, the delete statement includes all team IDs in a single operation, which could fail if parent teams are deleted before child teams due to theparentIdforeign key.🔧 Suggested fix to ensure child teams are deleted first
// Cleanup await prisma.membership.deleteMany({ where: { userId: testUserId } }); - await prisma.team.deleteMany({ where: { id: { in: [org1.id, org2.id, team1.id, team2.id] } } }); + // Delete child teams first, then parent orgs to respect foreign key constraints + await prisma.team.deleteMany({ where: { id: { in: [team1.id, team2.id] } } }); + await prisma.team.deleteMany({ where: { id: { in: [org1.id, org2.id] } } });packages/trpc/server/routers/viewer/bookings/get.handler.ts (1)
114-122: Consider injectingPermissionCheckServicerather than instantiating per request.Creating a new
PermissionCheckServiceinstance on every call togetBookingsmay have performance implications if the service constructor performs expensive operations. Consider passing it as a parameter or using dependency injection for better testability and potential caching.♻️ Optional refactor to inject the service
export async function getBookings({ user, prisma, kysely, bookingListingByStatus, sort, filters, take, skip, + permissionCheckService = new PermissionCheckService(), }: { user: { id: number; email: string; orgId?: number | null }; filters: TGetInputSchema["filters"]; prisma: PrismaClient; kysely: Kysely<DB>; bookingListingByStatus: InputByStatus[]; sort?: { ... }; take: number; skip: number; + permissionCheckService?: PermissionCheckService; }) { - const permissionCheckService = new PermissionCheckService(); const fallbackRoles: MembershipRole[] = [MembershipRole.ADMIN, MembershipRole.OWNER];
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/features/pbac/domain/repositories/IPermissionRepository.tspackages/features/pbac/infrastructure/repositories/PermissionRepository.tspackages/features/pbac/infrastructure/repositories/__tests__/PermissionRepository.integration-test.tspackages/features/pbac/services/permission-check.service.tspackages/trpc/server/routers/viewer/bookings/get.handler.test.tspackages/trpc/server/routers/viewer/bookings/get.handler.ts
🧰 Additional context used
🧬 Code graph analysis (6)
packages/features/pbac/infrastructure/repositories/__tests__/PermissionRepository.integration-test.ts (3)
scripts/prepare-local-for-delegation-credentials-testing.js (2)
prisma(7-7)org(45-47)packages/platform/libraries/index.ts (1)
MembershipRole(34-34)apps/web/test/lib/getSchedule/expects.ts (1)
expect(152-152)
packages/features/pbac/infrastructure/repositories/PermissionRepository.ts (2)
packages/features/pbac/domain/types/permission-registry.ts (1)
PermissionString(72-72)packages/platform/libraries/index.ts (1)
MembershipRole(34-34)
packages/trpc/server/routers/viewer/bookings/get.handler.test.ts (2)
packages/trpc/server/routers/viewer/bookings/get.handler.ts (2)
getHandler(37-82)getBookings(90-769)packages/features/pbac/services/permission-check.service.ts (1)
PermissionCheckService(20-377)
packages/features/pbac/domain/repositories/IPermissionRepository.ts (2)
packages/features/pbac/domain/types/permission-registry.ts (1)
PermissionString(72-72)packages/platform/libraries/index.ts (1)
MembershipRole(34-34)
packages/features/pbac/services/permission-check.service.ts (2)
packages/platform/libraries/index.ts (1)
MembershipRole(34-34)packages/features/pbac/domain/types/permission-registry.ts (1)
PermissionString(72-72)
packages/trpc/server/routers/viewer/bookings/get.handler.ts (1)
packages/platform/libraries/index.ts (1)
MembershipRole(34-34)
🔇 Additional comments (15)
packages/features/pbac/domain/repositories/IPermissionRepository.ts (1)
64-84: LGTM!The interface changes are well-designed. The optional
orgIdparameter is properly typed and the JSDoc comments clearly explain the scoping behavior. The changes align with the implementation inPermissionRepository.ts.packages/features/pbac/infrastructure/repositories/__tests__/PermissionRepository.integration-test.ts (2)
972-1053: Good test coverage for org-scoped filtering.The test properly validates:
- Without
orgId, both organizations are returned- With
orgId, only the specified org and its child teams are returned- Teams from other organizations are excluded
1055-1098: LGTM!The test correctly validates that child teams are included when
orgIdis provided, which is essential for the organization hierarchy permission model.packages/features/pbac/services/permission-check.service.ts (2)
314-342: LGTM!The
orgIdparameter is properly added to the method signature and correctly forwarded to the repository call. The JSDoc comment clearly documents the parameter's purpose.
348-376: LGTM!Consistent implementation with
getTeamIdsWithPermission. TheorgIdparameter is properly forwarded and documented.packages/trpc/server/routers/viewer/bookings/get.handler.ts (2)
144-159: LGTM!The permission validation logic correctly:
- Checks if filtered userIds are within accessible teams
- Allows users to always access their own bookings (
isCurrentUsercheck)- Throws a clear
FORBIDDENerror with descriptive message when access is denied
946-978: LGTM!The
getUserIdsAndEmailsFromTeamIdshelper is well-implemented:
- Early return for empty
teamIdsprevents unnecessary database queries- Proper deduplication using
Set- Clear return type
[number[], string[]]packages/features/pbac/infrastructure/repositories/PermissionRepository.ts (3)
248-258: LGTM!The parallel execution with
Promise.alland subsequent deduplication is well-implemented. This approach efficiently combines PBAC permissions and fallback roles while avoiding duplicates.
264-326: LGTM!The PBAC permissions query correctly:
- Uses a CTE for required permissions
- Handles both direct team memberships and child teams via org membership
- Applies org scoping with
(orgId IS NULL OR t.id = orgId OR t.parentId = orgId)- Uses proper wildcard matching for permissions
332-359: LGTM!The fallback roles query correctly:
- Excludes teams where PBAC is enabled (
f.teamId IS NULL)- Handles both direct memberships and child teams
- Applies proper org scoping
packages/trpc/server/routers/viewer/bookings/get.handler.test.ts (5)
10-28: LGTM!The mock setup is comprehensive and properly isolates the unit under test by mocking external dependencies including the logger, kysely client, and permission service.
196-247: LGTM!The test correctly validates that a
FORBIDDENerror is thrown when a user tries to filter by userIds that they don't have permission to access. The test setup properly mocks the scenario where user 4 is not in the accessible teams.
249-278: LGTM!Good test coverage for the edge case where users should always be able to access their own bookings regardless of PBAC permissions.
309-335: LGTM!The test correctly validates that PBAC permissions and fallback ADMIN/OWNER roles are combined, allowing access to bookings from multiple teams.
189-194: Remove this comment - no issue exists.The test assertion is correct. In Prisma, enum values without explicit assignments default to string literals.
MembershipRole.ADMINandMembershipRole.OWNERare equivalent to the strings"ADMIN"and"OWNER", so the test assertionfallbackRoles: ["ADMIN", "OWNER"]accurately reflects what the implementation passes to the mocked function.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| import logger from "@calcom/lib/logger"; | ||
| import prisma from "@calcom/prisma"; | ||
| import type { MembershipRole } from "@calcom/prisma/enums"; | ||
| import { TRPCError } from "@trpc/server"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "TRPCError" packages/features/pbac/services/permission-check.service.tsRepository: qodo-benchmark/cal.com-combined-coderabbit
Length of output: 129
Remove unused import TRPCError.
The TRPCError import at line 6 is not used anywhere in this file.
🤖 Prompt for AI Agents
In `@packages/features/pbac/services/permission-check.service.ts` at line 6,
Remove the unused TRPCError import from the top-level imports in
permission-check.service.ts: delete the `TRPCError` specifier from the import
statement so only used symbols remain, and run the linter/TS build to confirm
there are no remaining references to `TRPCError`.
1 participant
Benchmark PR from agentic-review-benchmarks#1
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.