fix: align checkBookingAccessWithPBAC with listing logic for personal event types#28071
fix: align checkBookingAccessWithPBAC with listing logic for personal event types#28071eunjae-lee wants to merge 1 commit intomainfrom
Conversation
… event types 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:
|
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/features/bookings/services/BookingAccessService.ts">
<violation number="1" location="packages/features/bookings/services/BookingAccessService.ts:180">
P1: Permission inconsistency: this method uses `"booking.read"` for the personal-event-type org/team checks, but the sibling method `doesUserIdHaveAccessToBooking` in the same class uses the more specific `"booking.readOrgBookings"` and `"booking.readTeamBookings"` for the identical logical scenario (Cases 4 & 5). Similarly, `get-booking.ts` uses `"booking.readOrgBookings"`. This divergence means the two code paths may grant/deny access differently for the same user and booking. Please verify this is the intended permission and consider aligning with the granular permissions used elsewhere, or document why the generic permission is correct here.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const hasPermission = await this.permissionCheckService.checkPermission({ | ||
| userId, | ||
| teamId: booking.eventType.teamId, | ||
| permission: "booking.read", |
There was a problem hiding this comment.
P1: Permission inconsistency: this method uses "booking.read" for the personal-event-type org/team checks, but the sibling method doesUserIdHaveAccessToBooking in the same class uses the more specific "booking.readOrgBookings" and "booking.readTeamBookings" for the identical logical scenario (Cases 4 & 5). Similarly, get-booking.ts uses "booking.readOrgBookings". This divergence means the two code paths may grant/deny access differently for the same user and booking. Please verify this is the intended permission and consider aligning with the granular permissions used elsewhere, or document why the generic permission is correct here.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/features/bookings/services/BookingAccessService.ts, line 180:
<comment>Permission inconsistency: this method uses `"booking.read"` for the personal-event-type org/team checks, but the sibling method `doesUserIdHaveAccessToBooking` in the same class uses the more specific `"booking.readOrgBookings"` and `"booking.readTeamBookings"` for the identical logical scenario (Cases 4 & 5). Similarly, `get-booking.ts` uses `"booking.readOrgBookings"`. This divergence means the two code paths may grant/deny access differently for the same user and booking. Please verify this is the intended permission and consider aligning with the granular permissions used elsewhere, or document why the generic permission is correct here.</comment>
<file context>
@@ -174,19 +173,44 @@ export class BookingAccessService {
+ const hasPermission = await this.permissionCheckService.checkPermission({
+ userId,
+ teamId: booking.eventType.teamId,
+ permission: "booking.read",
+ fallbackRoles: [MembershipRole.ADMIN, MembershipRole.OWNER],
+ });
</file context>
Devin AI is addressing Cubic AI's review feedbackNew feedback has been sent to the existing Devin session. ✅ No changes pushed |
What does this PR do?
Fixes a policy misalignment between the booking listing endpoint and the booking details access check for personal event types (event types with no
teamId).The listing query (
get.handler.ts, query #7) includes bookings created by any member of a team where the current user hasbooking.readpermission — including personal event type bookings. But when clicking to view details,checkBookingAccessWithPBACearly-returnedfalseat line 176-180 because there was noteamIdon the event type. Users with proper team/org permissions could see the booking in the list but got a "forbidden" error viewing details.Changes:
return falsefor!booking.eventType?.teamIdwith logic that looks up the booking organizer's teams/org and checksbooking.readpermission — mirroring the listing querythis.permissionCheckService(class-level instance) instead of creating a new local instance, consistent withdoesUserIdHaveAccessToBookingcheckBookingAccessWithPBACcode pathsMandatory Tasks (DO NOT REMOVE)
How should this be tested?
booking.readpermissionImportant for reviewers
checkBookingAccessWithPBACnow uses"booking.read"permission for the new personal-event-type checks (matching the listing inget.handler.ts). Note thatdoesUserIdHaveAccessToBookingin the same class uses"booking.readOrgBookings"/"booking.readTeamBookings"for its equivalent Cases 4 & 5. This difference is intentional per the spec to match the listing behavior, but worth confirming this is the desired alignment.bookingAuthorizationCheckSelectalready includesuserId.Link to Devin run: https://app.devin.ai/sessions/093e52a39f484789887ff4d70f1c0229
Requested by: @eunjae-lee