From 7ff2eaf293d3f1ba7b4cc64fdcd6c1f18652b9f4 Mon Sep 17 00:00:00 2001 From: Eunjae Lee Date: Wed, 11 Mar 2026 18:46:57 +0100 Subject: [PATCH] fix: consolidate booking access checks into doesUserIdHaveAccessToBooking (#28071) * fix: align checkBookingAccessWithPBAC with listing logic for personal event types Co-Authored-By: eunjae@cal.com * fix: align permission strings with doesUserIdHaveAccessToBooking granular permissions Co-Authored-By: eunjae@cal.com * change implementation * remove unused code * test: add getBookingDetails tests for personal event type booking access Co-Authored-By: eunjae@cal.com * fix: remove unused MembershipRole import Co-Authored-By: eunjae@cal.com * test: convert BookingDetailsService tests to integration tests Replace unit tests with mocks by integration tests using real database. Tests verify org/team admin access to personal event type bookings. - Org admin viewing team member's personal event booking (fails on main, passes on PR) - Team admin viewing team member's personal event booking (fails on main, passes on PR) - Non-admin denied access (passes on both) - Owner viewing own booking (passes on both) - Non-existent booking returns Forbidden (passes on both) Co-Authored-By: eunjae@cal.com * test: refactor integration tests to use repositories instead of direct prisma calls Co-Authored-By: eunjae@cal.com * test: replace direct prisma usage with TestXXXRepository pattern in integration tests Co-Authored-By: eunjae@cal.com * test: use production repositories (OrganizationRepository, BookingRepository, UserRepository) instead of test repos for setup Co-Authored-By: eunjae@cal.com * test: inline test repository logic into integration test file The TestXXXRepository classes were thin wrappers around single prisma calls with barely any logic, so the indirection wasn't justified. Co-Authored-By: Claude Opus 4.6 --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 --- .../repositories/BookingRepository.ts | 15 +- .../services/BookingAccessService.test.ts | 5 +- .../bookings/services/BookingAccessService.ts | 59 ---- .../BookingDetailsService.integration-test.ts | 280 ++++++++++++++++++ .../services/BookingDetailsService.ts | 3 +- packages/prisma/selects/booking.ts | 30 -- 6 files changed, 284 insertions(+), 108 deletions(-) create mode 100644 packages/features/bookings/services/BookingDetailsService.integration-test.ts diff --git a/packages/features/bookings/repositories/BookingRepository.ts b/packages/features/bookings/repositories/BookingRepository.ts index 311ecd1ed5..908dd4e542 100644 --- a/packages/features/bookings/repositories/BookingRepository.ts +++ b/packages/features/bookings/repositories/BookingRepository.ts @@ -2,11 +2,7 @@ import { withReporting } from "@calcom/lib/sentryWrapper"; import type { PrismaClient } from "@calcom/prisma"; import type { Booking, Prisma } from "@calcom/prisma/client"; import { BookingStatus, RRTimestampBasis } from "@calcom/prisma/enums"; -import { - bookingAuthorizationCheckSelect, - bookingDetailsSelect, - bookingMinimalSelect, -} from "@calcom/prisma/selects/booking"; +import { bookingDetailsSelect, bookingMinimalSelect } from "@calcom/prisma/selects/booking"; import { credentialForCalendarServiceSelect } from "@calcom/prisma/selects/credential"; import { workflowSelect } from "../../ee/workflows/lib/getAllWorkflows"; import type { @@ -674,15 +670,6 @@ export class BookingRepository implements IBookingRepository { }); } - async findByUidForAuthorizationCheck({ bookingUid }: { bookingUid: string }) { - return await this.prismaClient.booking.findUnique({ - where: { - uid: bookingUid, - }, - select: bookingAuthorizationCheckSelect, - }); - } - async findByUidForDetails({ bookingUid }: { bookingUid: string }) { return await this.prismaClient.booking.findUnique({ where: { diff --git a/packages/features/bookings/services/BookingAccessService.test.ts b/packages/features/bookings/services/BookingAccessService.test.ts index c417177f5c..ffa8358d14 100644 --- a/packages/features/bookings/services/BookingAccessService.test.ts +++ b/packages/features/bookings/services/BookingAccessService.test.ts @@ -1,9 +1,7 @@ -import { describe, it, expect, beforeEach, vi } from "vitest"; - import { UserRepository } from "@calcom/features/users/repositories/UserRepository"; import type { PrismaClient } from "@calcom/prisma"; import { MembershipRole } from "@calcom/prisma/enums"; - +import { beforeEach, describe, expect, it, vi } from "vitest"; import { BookingRepository } from "../repositories/BookingRepository"; import { BookingAccessService } from "./BookingAccessService"; @@ -341,4 +339,5 @@ describe("BookingAccessService", () => { }); }); }); + }); diff --git a/packages/features/bookings/services/BookingAccessService.ts b/packages/features/bookings/services/BookingAccessService.ts index be035b9f7c..0ff10a6ccf 100644 --- a/packages/features/bookings/services/BookingAccessService.ts +++ b/packages/features/bookings/services/BookingAccessService.ts @@ -2,7 +2,6 @@ import { PermissionCheckService } from "@calcom/features/pbac/services/permissio import { UserRepository } from "@calcom/features/users/repositories/UserRepository"; import type { PrismaClient } from "@calcom/prisma"; import { MembershipRole } from "@calcom/prisma/enums"; - import { BookingRepository } from "../repositories/BookingRepository"; type BookingForAccessCheck = NonNullable>>; @@ -131,62 +130,4 @@ export class BookingAccessService { return false; } - - /** - * Checks if a user has access to view booking details using: - * 1. Owner/host check - * 2. PBAC permission check for team bookings - */ - async checkBookingAccessWithPBAC({ - userId, - bookingUid, - }: { - userId: number; - bookingUid: string; - }): Promise { - const bookingRepo = new BookingRepository(this.prismaClient); - - const booking = await bookingRepo.findByUidForAuthorizationCheck({ bookingUid }); - - if (!booking) { - return false; - } - - // Check 1: User is the owner of the booking - const isOwner = booking.userId === userId; - if (isOwner) { - return true; - } - - // Check 2: User is a host (checking eventType.users and eventType.hosts) - const attendeeEmails = new Set(booking.attendees?.map((attendee) => attendee.email) || []); - - const isHostViaEventTypeUsers = booking.eventType?.users?.some( - (user) => user.id === userId && attendeeEmails.has(user.email) - ); - - const isHostViaEventTypeHosts = booking.eventType?.hosts?.some( - (host) => host.user?.id === userId && attendeeEmails.has(host.user.email) - ); - - if (isHostViaEventTypeUsers || isHostViaEventTypeHosts) { - return true; - } - - // Check 3: PBAC permission check for team bookings - if (!booking.eventType?.teamId) { - // No team associated with booking and user is not owner/host - return false; - } - - const permissionCheckService = new PermissionCheckService(); - const hasPermission = await permissionCheckService.checkPermission({ - userId, - teamId: booking.eventType.teamId, - permission: "booking.read", - fallbackRoles: [MembershipRole.ADMIN, MembershipRole.OWNER], - }); - - return hasPermission; - } } diff --git a/packages/features/bookings/services/BookingDetailsService.integration-test.ts b/packages/features/bookings/services/BookingDetailsService.integration-test.ts new file mode 100644 index 0000000000..bf7191ca8a --- /dev/null +++ b/packages/features/bookings/services/BookingDetailsService.integration-test.ts @@ -0,0 +1,280 @@ +import { describe, it, expect, beforeAll, afterAll } from "vitest"; + +import { BookingRepository } from "@calcom/features/bookings/repositories/BookingRepository"; +import { OrganizationRepository } from "@calcom/features/ee/organizations/repositories/OrganizationRepository"; +import { TeamRepository } from "@calcom/features/ee/teams/repositories/TeamRepository"; +import { EventTypeRepository } from "@calcom/features/eventtypes/repositories/eventTypeRepository"; +import { MembershipRepository } from "@calcom/features/membership/repositories/MembershipRepository"; +import { UserRepository } from "@calcom/features/users/repositories/UserRepository"; +import { prisma } from "@calcom/prisma"; +import { BookingStatus, CreationSource, MembershipRole } from "@calcom/prisma/enums"; + +import { BookingDetailsService } from "./BookingDetailsService"; + +/** + * Integration tests for BookingDetailsService.getBookingDetails + * + * These tests verify that org/team admins can view booking details + * for bookings made by team members using personal event types (no teamId). + * + * On main (before the fix), the access check (checkBookingAccessWithPBAC) returns false + * for personal event type bookings when the viewer is not the owner/host — even if + * they are an org/team admin. These tests fail on main, proving the bug. + * + * On the PR branch, getBookingDetails uses doesUserIdHaveAccessToBooking which + * correctly checks org/team admin access for personal event type bookings. + */ + +// Track resource IDs for cleanup +const createdBookingIds: number[] = []; +const createdTeamIds: number[] = []; +const createdUserIds: number[] = []; + +describe("BookingDetailsService (Integration Tests)", () => { + const timestamp = Date.now(); + + const orgRepo = new OrganizationRepository({ prismaClient: prisma }); + const bookingRepo = new BookingRepository(prisma); + const userRepo = new UserRepository(prisma); + const eventTypeRepo = new EventTypeRepository(prisma); + const teamRepo = new TeamRepository(prisma); + + // Users + let bookingOwnerId: number; + let orgAdminId: number; + let teamAdminId: number; + let regularUserId: number; + + // Org and team + let orgId: number; + let teamId: number; + + // Booking + let personalBookingUid: string; + + beforeAll(async () => { + // 1. Create an organization via OrganizationRepository + const org = await orgRepo.create({ + name: `Test Org ${timestamp}`, + slug: `test-org-${timestamp}`, + isOrganizationConfigured: true, + isOrganizationAdminReviewed: true, + autoAcceptEmail: `test-${timestamp}.com`, + seats: null, + pricePerSeat: null, + isPlatform: false, + logoUrl: null, + bio: null, + brandColor: null, + bannerUrl: null, + }); + orgId = org.id; + createdTeamIds.push(org.id); + + // 2. Create a team within the org (no production TeamRepository.create() exists) + const team = await prisma.team.create({ + data: { + name: `Test Team ${timestamp}`, + slug: `test-team-${timestamp}`, + parentId: org.id, + }, + select: { id: true }, + }); + teamId = team.id; + createdTeamIds.push(team.id); + + // 3. Create booking owner WITH organizationId via UserRepository + // (eliminates need for separate updateOrganizationId call) + const bookingOwner = await userRepo.create({ + email: `booking-owner-${timestamp}@test.com`, + username: `booking-owner-${timestamp}`, + organizationId: orgId, + creationSource: CreationSource.WEBAPP, + locked: false, + }); + bookingOwnerId = bookingOwner.id; + createdUserIds.push(bookingOwner.id); + + // 4. Create other users via UserRepository + const orgAdmin = await userRepo.create({ + email: `org-admin-${timestamp}@test.com`, + username: `org-admin-${timestamp}`, + organizationId: null, + creationSource: CreationSource.WEBAPP, + locked: false, + }); + orgAdminId = orgAdmin.id; + createdUserIds.push(orgAdmin.id); + + const teamAdmin = await userRepo.create({ + email: `team-admin-${timestamp}@test.com`, + username: `team-admin-${timestamp}`, + organizationId: null, + creationSource: CreationSource.WEBAPP, + locked: false, + }); + teamAdminId = teamAdmin.id; + createdUserIds.push(teamAdmin.id); + + const regularUser = await userRepo.create({ + email: `regular-user-${timestamp}@test.com`, + username: `regular-user-${timestamp}`, + organizationId: null, + creationSource: CreationSource.WEBAPP, + locked: false, + }); + regularUserId = regularUser.id; + createdUserIds.push(regularUser.id); + + // 5. Create memberships via MembershipRepository (static method) + await MembershipRepository.create({ + userId: bookingOwnerId, + teamId: orgId, + role: MembershipRole.MEMBER, + accepted: true, + }); + + await MembershipRepository.create({ + userId: bookingOwnerId, + teamId: teamId, + role: MembershipRole.MEMBER, + accepted: true, + }); + + await MembershipRepository.create({ + userId: orgAdminId, + teamId: orgId, + role: MembershipRole.ADMIN, + accepted: true, + }); + + await MembershipRepository.create({ + userId: teamAdminId, + teamId: teamId, + role: MembershipRole.ADMIN, + accepted: true, + }); + + // 6. Create a personal event type (no teamId) via EventTypeRepository + const personalEventType = await eventTypeRepo.create({ + title: `Personal Event ${timestamp}`, + slug: `personal-event-${timestamp}`, + length: 30, + userId: bookingOwnerId, + }); + + // 7. Create a booking on the personal event type via BookingRepository + personalBookingUid = `personal-booking-${timestamp}`; + const booking = await bookingRepo.createBookingForManagedEventReassignment({ + uid: personalBookingUid, + userId: bookingOwnerId, + userPrimaryEmail: `booking-owner-${timestamp}@test.com`, + eventTypeId: personalEventType.id, + title: `Personal Booking ${timestamp}`, + description: null, + startTime: new Date("2026-03-01T10:00:00.000Z"), + endTime: new Date("2026-03-01T10:30:00.000Z"), + status: BookingStatus.ACCEPTED, + location: null, + smsReminderNumber: null, + idempotencyKey: `idempotency-${timestamp}`, + iCalUID: `ical-${timestamp}@test.com`, + iCalSequence: 0, + attendees: [{ email: "attendee@test.com", name: "Test Attendee", timeZone: "UTC", locale: "en" }], + }); + createdBookingIds.push(booking.id); + }); + + afterAll(async () => { + // Clean up bookings and attendees + if (createdBookingIds.length > 0) { + await prisma.attendee.deleteMany({ where: { bookingId: { in: createdBookingIds } } }); + await prisma.booking.deleteMany({ where: { id: { in: createdBookingIds } } }); + } + + // Delete personal event types not associated with a team + if (createdUserIds.length > 0) { + await prisma.eventType.deleteMany({ where: { userId: { in: createdUserIds } } }); + } + + // Delete users (memberships, profiles, schedules, availability) + // Must happen before team deletion so user.organizationId FK is removed + if (createdUserIds.length > 0) { + await prisma.membership.deleteMany({ where: { userId: { in: createdUserIds } } }); + await prisma.profile.deleteMany({ where: { userId: { in: createdUserIds } } }); + for (const userId of createdUserIds) { + await prisma.availability.deleteMany({ where: { Schedule: { userId } } }); + await prisma.schedule.deleteMany({ where: { userId } }); + await prisma.user.delete({ where: { id: userId } }); + } + } + + // TeamRepository.deleteById handles remaining memberships + managed event types + for (const id of [...createdTeamIds].reverse()) { + await teamRepo.deleteById({ id }); + } + }); + + describe("getBookingDetails - personal event type bookings", () => { + it("should allow the booking owner to view their own booking details", async () => { + const service = new BookingDetailsService(prisma); + + const result = await service.getBookingDetails({ + userId: bookingOwnerId, + bookingUid: personalBookingUid, + }); + + expect(result).toBeDefined(); + expect(result.rescheduledToBooking).toBeNull(); + expect(result.previousBooking).toBeNull(); + }); + + it("should allow an org admin to view booking details for a team member's personal event type booking", async () => { + const service = new BookingDetailsService(prisma); + + const result = await service.getBookingDetails({ + userId: orgAdminId, + bookingUid: personalBookingUid, + }); + + expect(result).toBeDefined(); + expect(result.rescheduledToBooking).toBeNull(); + expect(result.previousBooking).toBeNull(); + }); + + it("should allow a team admin to view booking details for a team member's personal event type booking", async () => { + const service = new BookingDetailsService(prisma); + + const result = await service.getBookingDetails({ + userId: teamAdminId, + bookingUid: personalBookingUid, + }); + + expect(result).toBeDefined(); + expect(result.rescheduledToBooking).toBeNull(); + expect(result.previousBooking).toBeNull(); + }); + + it("should deny access to a non-admin user viewing another user's personal event type booking", async () => { + const service = new BookingDetailsService(prisma); + + await expect( + service.getBookingDetails({ + userId: regularUserId, + bookingUid: personalBookingUid, + }) + ).rejects.toThrow("You do not have permission to view this booking"); + }); + + it("should throw when booking does not exist", async () => { + const service = new BookingDetailsService(prisma); + + await expect( + service.getBookingDetails({ + userId: bookingOwnerId, + bookingUid: "non-existent-booking-uid", + }) + ).rejects.toThrow("You do not have permission to view this booking"); + }); + }); +}); diff --git a/packages/features/bookings/services/BookingDetailsService.ts b/packages/features/bookings/services/BookingDetailsService.ts index 00c6652c1a..a6798954bf 100644 --- a/packages/features/bookings/services/BookingDetailsService.ts +++ b/packages/features/bookings/services/BookingDetailsService.ts @@ -1,6 +1,5 @@ import { ErrorWithCode } from "@calcom/lib/errors"; import type { PrismaClient } from "@calcom/prisma"; - import { BookingRepository } from "../repositories/BookingRepository"; import { BookingAccessService } from "./BookingAccessService"; @@ -14,7 +13,7 @@ export class BookingDetailsService { } async getBookingDetails({ userId, bookingUid }: { userId: number; bookingUid: string }) { - const hasAccess = await this.bookingAccessService.checkBookingAccessWithPBAC({ + const hasAccess = await this.bookingAccessService.doesUserIdHaveAccessToBooking({ userId, bookingUid, }); diff --git a/packages/prisma/selects/booking.ts b/packages/prisma/selects/booking.ts index 1941b881ac..1b7d58e563 100644 --- a/packages/prisma/selects/booking.ts +++ b/packages/prisma/selects/booking.ts @@ -13,36 +13,6 @@ export const bookingMinimalSelect = { createdAt: true, } satisfies Prisma.BookingSelect; -export const bookingAuthorizationCheckSelect = { - userId: true, - eventType: { - select: { - teamId: true, - users: { - select: { - id: true, - email: true, - }, - }, - hosts: { - select: { - user: { - select: { - id: true, - email: true, - }, - }, - }, - }, - }, - }, - attendees: { - select: { - email: true, - }, - }, -} satisfies Prisma.BookingSelect; - export const bookingDetailsSelect = { uid: true, rescheduled: true,