Skip to content

Code Review Bench PR #25771 - feat: update recording, transcript endpoint and add tests #15

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ketkarameya wants to merge 2 commits into from
Closed

Code Review Bench PR #25771 - feat: update recording, transcript endpoint and add tests #15

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
02743f3
Apply PR changes (squashed due to cherry-pick conflicts)
tomerqodo Jan 25, 2026
eb21d26
update pr
tomerqodo Jan 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 18 additions & 24 deletions apps/api/v2/src/ee/bookings/2024-08-13/controllers/bookings.controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,10 +212,9 @@ export class BookingsController_2024_08_13 {
}

@Get("/:bookingUid/recordings")
// @Pbac(["booking.readRecordings"])
@Permissions([BOOKING_READ])
@UseGuards(BookingUidGuard)
// @UseGuards(ApiAuthGuard, BookingUidGuard, BookingPbacGuard)
@Pbac(["booking.readRecordings"])
@Permissions([BOOKING_WRITE])
@UseGuards(ApiAuthGuard, BookingUidGuard, BookingPbacGuard)
@ApiHeader(API_KEY_OR_ACCESS_TOKEN_HEADER)
@ApiOperation({
summary: "Get all the recordings for the booking",
Expand All @@ -225,21 +224,18 @@ export class BookingsController_2024_08_13 {
`,
})
async getBookingRecordings(@Param("bookingUid") bookingUid: string): Promise<GetBookingRecordingsOutput> {
const recordings = await this.calVideoService.getRecordings(bookingUid);
const recordings = this.calVideoService.getRecordings(bookingUid);

@gitar-bot-staging gitar-bot-staging Bot Feb 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 Bug: Missing await on async getRecordings() returns a Promise

CalVideoService.getRecordings() is an async method that returns Promise<RecordingItem[]> (it internally awaits database queries and Promise.all). The await was removed from the call in the controller, so recordings is now a Promise object, not the resolved array.

The return type annotation says Promise<GetBookingRecordingsOutput> with data: RecordingItem[], but the actual data field will contain an unresolved Promise. This will result in the API returning "data": {} (a serialized Promise object) instead of the actual recordings array, effectively breaking the recordings endpoint for all callers.

The test mocks this with mockResolvedValue([]) which returns a Promise — and since the handler is async, the returned object's data field will be a Promise that NestJS may or may not auto-resolve depending on serialization timing, but TypeScript's type system is being circumvented here.

Was this helpful? React with 👍 / 👎

Suggested change
const recordings = this.calVideoService.getRecordings(bookingUid);
const recordings = await this.calVideoService.getRecordings(bookingUid);
  • Apply suggested fix


return {
status: SUCCESS_STATUS,
data: recordings,
message:
"This endpoint will require authentication in a future release. Please update your integration to include valid credentials. See https://cal.com/docs/api-reference/v2/introduction#authentication for details.",
};
}

@Get("/:bookingUid/transcripts")
// @Pbac(["booking.readRecordings"])
@Pbac(["booking.readRecordings"])
@Permissions([BOOKING_READ])
@UseGuards(BookingUidGuard)
// @UseGuards(ApiAuthGuard, BookingUidGuard, BookingPbacGuard)
@UseGuards(BookingPbacGuard, ApiAuthGuard, BookingUidGuard)

@gitar-bot-staging gitar-bot-staging Bot Feb 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Security: Guard order on transcripts puts PBAC before authentication

The transcripts endpoint uses @UseGuards(BookingPbacGuard, ApiAuthGuard, BookingUidGuard), which places BookingPbacGuard before ApiAuthGuard. NestJS executes guards in the order they are listed. This means the PBAC guard — which checks whether the authenticated user has access to the booking — will run before the user is even authenticated.

Every other endpoint in this controller that uses these three guards uses the order ApiAuthGuard, BookingUidGuard, BookingPbacGuard (authenticate → validate UID → check authorization). The recordings endpoint on line 217 uses this correct order. The transcripts endpoint should match.

Depending on the BookingPbacGuard implementation, this could either:

  • Crash/throw when trying to access the user context (which hasn't been set yet)
  • Or accidentally allow unauthenticated access if the guard fails open when no user is present

Was this helpful? React with 👍 / 👎

Suggested change
@UseGuards(BookingPbacGuard, ApiAuthGuard, BookingUidGuard)
@UseGuards(ApiAuthGuard, BookingUidGuard, BookingPbacGuard)
  • Apply suggested fix

@ApiHeader(API_KEY_OR_ACCESS_TOKEN_HEADER)
@ApiOperation({
summary: "Get Cal Video real time transcript download links for the booking",
Expand All @@ -258,8 +254,6 @@ export class BookingsController_2024_08_13 {
return {
status: SUCCESS_STATUS,
data: transcripts ?? [],
message:
"This endpoint will require authentication in a future release. Please update your integration to include valid credentials. See https://cal.com/docs/api-reference/v2/introduction#authentication for details.",
};
}

Expand Down Expand Up @@ -397,18 +391,18 @@ export class BookingsController_2024_08_13 {
<Note>Please make sure to pass in the cal-api-version header value as mentioned in the Headers section. Not passing the correct value will default to an older version of this endpoint.</Note>
`,
})
async markNoShow(
@Param("bookingUid") bookingUid: string,
@Body() body: MarkAbsentBookingInput_2024_08_13,
@GetUser() user: ApiAuthGuardUser
): Promise<MarkAbsentBookingOutput_2024_08_13> {
const booking = await this.bookingsService.markAbsent(bookingUid, user.id, body, user.uuid);

return {
status: SUCCESS_STATUS,
data: booking,
};
}
async markNoShow(
@Param("bookingUid") bookingUid: string,
@Body() body: MarkAbsentBookingInput_2024_08_13,
@GetUser() user: ApiAuthGuardUser
): Promise<MarkAbsentBookingOutput_2024_08_13> {
const booking = await this.bookingsService.markAbsent(bookingUid, user.id, body, user.uuid);

return {
status: SUCCESS_STATUS,
data: booking,
};
}

@Post("/:bookingUid/reassign")
@HttpCode(HttpStatus.OK)
Expand Down
90 changes: 45 additions & 45 deletions apps/api/v2/src/ee/bookings/2024-08-13/controllers/e2e/booking-access-auth.e2e-spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,51 +185,51 @@ describe("Bookings Endpoints 2024-08-13", () => {
});
});

// describe("GET /v2/bookings/:bookingUid/recordings - Authorization", () => {
// it("should allow booking organizer to access recordings", async () => {
// const calVideoService = app.get(CalVideoService);
// jest.spyOn(calVideoService, "getRecordings").mockResolvedValue([]);

// const response = await request(app.getHttpServer())
// .get(`/v2/bookings/${testBooking.uid}/recordings`)
// .set("Authorization", `Bearer ${ownerApiKey}`)
// .set(CAL_API_VERSION_HEADER, VERSION_2024_08_13)
// .expect(200);

// expect(response.body.status).toEqual(SUCCESS_STATUS);
// });

// it("should return 403 when unauthorized user tries to access recordings", async () => {
// await request(app.getHttpServer())
// .get(`/v2/bookings/${testBooking.uid}/recordings`)
// .set("Authorization", `Bearer ${unauthorizedApiKey}`)
// .set(CAL_API_VERSION_HEADER, VERSION_2024_08_13)
// .expect(403);
// });
// });

// describe("GET /v2/bookings/:bookingUid/transcripts - Authorization", () => {
// it("should allow booking organizer to access transcripts", async () => {
// const calVideoService = app.get(CalVideoService);
// jest.spyOn(calVideoService, "getTranscripts").mockResolvedValue([]);

// const response = await request(app.getHttpServer())
// .get(`/v2/bookings/${testBooking.uid}/transcripts`)
// .set("Authorization", `Bearer ${ownerApiKey}`)
// .set(CAL_API_VERSION_HEADER, VERSION_2024_08_13)
// .expect(200);

// expect(response.body.status).toEqual(SUCCESS_STATUS);
// });

// it("should return 403 when unauthorized user tries to access transcripts", async () => {
// await request(app.getHttpServer())
// .get(`/v2/bookings/${testBooking.uid}/transcripts`)
// .set("Authorization", `Bearer ${unauthorizedApiKey}`)
// .set(CAL_API_VERSION_HEADER, VERSION_2024_08_13)
// .expect(403);
// });
// });
describe("GET /v2/bookings/:bookingUid/recordings - Authorization", () => {
it("should allow booking organizer to access recordings", async () => {
const calVideoService = app.get(CalVideoService);
jest.spyOn(calVideoService, "getRecordings").mockResolvedValue([]);

const response = await request(app.getHttpServer())
.get(`/v2/bookings/${testBooking.uid}/recordings`)
.set("Authorization", `Bearer ${ownerApiKey}`)
.set(CAL_API_VERSION_HEADER, VERSION_2024_08_13)
.expect(200);

expect(response.body.status).toEqual(SUCCESS_STATUS);
});

it("should return 403 when unauthorized user tries to access recordings", async () => {
await request(app.getHttpServer())
.get(`/v2/bookings/${testBooking.uid}/recordings`)
.set("Authorization", `Bearer ${unauthorizedApiKey}`)
.set(CAL_API_VERSION_HEADER, VERSION_2024_08_13)
.expect(403);
});
});

describe("GET /v2/bookings/:bookingUid/transcripts - Authorization", () => {
it("should allow booking organizer to access transcripts", async () => {
const calVideoService = app.get(CalVideoService);
jest.spyOn(calVideoService, "getTranscripts").mockResolvedValue([]);

const response = await request(app.getHttpServer())
.get(`/v2/bookings/${testBooking.uid}/transcripts`)
.set("Authorization", `Bearer ${ownerApiKey}`)
.set(CAL_API_VERSION_HEADER, VERSION_2024_08_13)
.expect(200);

expect(response.body.status).toEqual(SUCCESS_STATUS);
});

it("should return 403 when unauthorized user tries to access transcripts", async () => {
await request(app.getHttpServer())
.get(`/v2/bookings/${testBooking.uid}/transcripts`)
.set("Authorization", `Bearer ${unauthorizedApiKey}`)
.set(CAL_API_VERSION_HEADER, VERSION_2024_08_13)
.expect(403);
});
});

afterAll(async () => {
await bookingsRepositoryFixture.deleteById(testBooking.id);
Expand Down
9 changes: 1 addition & 8 deletions packages/platform/types/bookings/2024-08-13/outputs/get-booking-recordings.output.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { Type } from "class-transformer";
import { IsEnum, ValidateNested, IsNumber, IsString, IsOptional, IsUrl } from "class-validator";

import { SUCCESS_STATUS, ERROR_STATUS } from "@calcom/platform-constants";
import type { BookingRepository } from "@calcom/features/bookings/lib/BookingRepository";

export class RecordingItem {
@ApiProperty({ example: "1234567890" })
Expand Down Expand Up @@ -55,12 +56,4 @@ export class GetBookingRecordingsOutput {
@ValidateNested({ each: true })
@Type(() => RecordingItem)
data!: RecordingItem[];

@ApiProperty({
example: "This endpoint will require authentication in a future release.",
required: false,
})
@IsString()
@IsOptional()
message?: string;
}
7 changes: 0 additions & 7 deletions packages/platform/types/bookings/2024-08-13/outputs/get-booking-transcripts.output.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,4 @@ export class GetBookingTranscriptsOutput {
@IsArray()
@IsString({ each: true })
data!: string[];

@ApiProperty({
example: "This endpoint will require authentication in a future release.",
required: false,
})
@IsString()
message?: string;
}