From a1922efab579a4330199b4e997d8ed952d1ddcbb Mon Sep 17 00:00:00 2001 From: Eunjae Lee Date: Fri, 19 Sep 2025 16:13:21 +0200 Subject: [PATCH] feat: make orgId optional for user and team scopes in /insights and /insights/routing (#23912) * feat: make orgId optional for user and team scopes in InsightsRoutingBaseService - User scope authorization only checks formUserId and formTeamId IS NULL - Team scope now supports standalone teams without organizations - Add validation logic to return NOTHING_CONDITION if team belongs to org but no orgId provided - Add comprehensive test coverage for null/undefined orgId scenarios in both scopes - Aligns schema with actual usage patterns and supports teams without organizations Co-Authored-By: eunjae@cal.com * feat: extend orgId optional support to InsightsBookingBaseService - Make orgId optional for user and team scopes in InsightsBookingBaseService - Update InsightsBookingServicePublicOptions type to allow orgId: number | null - Add validation logic for team scope to handle missing orgId - Add comprehensive test coverage for null/undefined orgId scenarios - Fix type casting issues in test file - Maintains backward compatibility while supporting teams without organizations Co-Authored-By: eunjae@cal.com * fix: correct authorization logic for optional orgId in team scope - Skip isOwnerOrAdmin check for team scope when orgId is null (standalone teams) - Maintain security for org scope and team scope with orgId - Fixes integration test failures for null orgId test cases Co-Authored-By: eunjae@cal.com * fix: use != null instead of !== undefined for orgId checks - Properly handle both null and undefined orgId values in authorization logic - Fix integration test failures where null orgId was incorrectly triggering isOwnerOrAdmin check - Ensure team scope with null orgId skips ownership validation for standalone teams Co-Authored-By: eunjae@cal.com * fix: always validate team membership for team scope - Remove orgId condition from isOwnerOrAdmin check for team scope - Ensure both standalone teams and org-based teams require ownership validation - Maintain orgId validation logic in buildTeamAuthorizationCondition methods Co-Authored-By: eunjae@cal.com * fix: use nullish() for orgId schema validation - Change from .optional() to .nullish() to allow both null and undefined - Fixes schema validation when tests pass orgId: null - Resolves authorization logic returning NOTHING_CONDITION for valid cases Co-Authored-By: eunjae@cal.com * feat: replace isOwnerOrAdmin with PBAC checkPermission in insights services - Replace isOwnerOrAdmin method in InsightsBookingBaseService with checkPermission from PermissionCheckService - Replace isOwnerOrAdmin method in InsightsRoutingBaseService with checkPermission from PermissionCheckService - Use permission 'insights.read' with fallback roles MembershipRole.OWNER and MembershipRole.ADMIN - Maintain same method signature and behavior while leveraging PBAC system Co-Authored-By: eunjae@cal.com * clean up types * add comment --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- .../features/insights/server/trpc-router.ts | 2 +- .../service/InsightsBookingBaseService.ts | 50 +++-- .../service/InsightsRoutingBaseService.ts | 53 ++++-- ...InsightsBookingService.integration-test.ts | 171 ++++++++++++++++- ...InsightsRoutingService.integration-test.ts | 174 +++++++++++++++++- 5 files changed, 407 insertions(+), 43 deletions(-) diff --git a/packages/features/insights/server/trpc-router.ts b/packages/features/insights/server/trpc-router.ts index c8d55f681e..060313b181 100644 --- a/packages/features/insights/server/trpc-router.ts +++ b/packages/features/insights/server/trpc-router.ts @@ -324,7 +324,7 @@ function createInsightsBookingService( options: { scope, userId: ctx.user.id, - orgId: ctx.user.organizationId ?? 0, + orgId: ctx.user.organizationId, ...(selectedTeamId && { teamId: selectedTeamId }), }, filters: { diff --git a/packages/lib/server/service/InsightsBookingBaseService.ts b/packages/lib/server/service/InsightsBookingBaseService.ts index 064ecb3d10..3046bd5913 100644 --- a/packages/lib/server/service/InsightsBookingBaseService.ts +++ b/packages/lib/server/service/InsightsBookingBaseService.ts @@ -18,6 +18,7 @@ import { replaceDateRangeColumnFilter, } from "@calcom/features/insights/lib/bookingUtils"; import type { DateRange } from "@calcom/features/insights/server/insightsDateUtils"; +import { PermissionCheckService } from "@calcom/features/pbac/services/permission-check.service"; import type { PrismaClient } from "@calcom/prisma"; import { Prisma } from "@calcom/prisma/client"; import { MembershipRole } from "@calcom/prisma/enums"; @@ -106,7 +107,7 @@ export const insightsBookingServiceOptionsSchema = z.discriminatedUnion("scope", z.object({ scope: z.literal("user"), userId: z.number(), - orgId: z.number(), + orgId: z.number().nullish().optional(), }), z.object({ scope: z.literal("org"), @@ -116,7 +117,7 @@ export const insightsBookingServiceOptionsSchema = z.discriminatedUnion("scope", z.object({ scope: z.literal("team"), userId: z.number(), - orgId: z.number(), + orgId: z.number().nullish().optional(), teamId: z.number(), }), ]); @@ -124,7 +125,7 @@ export const insightsBookingServiceOptionsSchema = z.discriminatedUnion("scope", export type InsightsBookingServicePublicOptions = { scope: "user" | "org" | "team"; userId: number; - orgId: number; + orgId: number | null | undefined; teamId?: number; }; @@ -435,13 +436,27 @@ export class InsightsBookingBaseService { options: Extract ): Promise { const teamRepo = new TeamRepository(this.prisma); - const childTeamOfOrg = await teamRepo.findByIdAndParentId({ - id: options.teamId, - parentId: options.orgId, - select: { id: true }, - }); - if (options.orgId && !childTeamOfOrg) { - return NOTHING_CONDITION; + + if (options.orgId) { + // team under org + const childTeamOfOrg = await teamRepo.findByIdAndParentId({ + id: options.teamId, + parentId: options.orgId, + select: { id: true }, + }); + if (!childTeamOfOrg) { + // teamId and its orgId does not match + return NOTHING_CONDITION; + } + } else { + // standalone team + const team = await teamRepo.findById({ + id: options.teamId, + }); + if (team?.parentId) { + // a team without orgId is not supposed to have parentId + return NOTHING_CONDITION; + } } const usersFromTeam = await MembershipRepository.findAllByTeamIds({ @@ -1256,13 +1271,12 @@ export class InsightsBookingBaseService { } private async isOwnerOrAdmin(userId: number, targetId: number): Promise { - // Check if the user is an owner or admin of the organization or team - const membership = await MembershipRepository.findUniqueByUserIdAndTeamId({ userId, teamId: targetId }); - return Boolean( - membership && - membership.accepted && - membership.role && - (membership.role === MembershipRole.OWNER || membership.role === MembershipRole.ADMIN) - ); + const permissionCheckService = new PermissionCheckService(); + return await permissionCheckService.checkPermission({ + userId, + teamId: targetId, + permission: "insights.read", + fallbackRoles: [MembershipRole.OWNER, MembershipRole.ADMIN], + }); } } diff --git a/packages/lib/server/service/InsightsRoutingBaseService.ts b/packages/lib/server/service/InsightsRoutingBaseService.ts index d410a1c2fe..fa46d0b588 100644 --- a/packages/lib/server/service/InsightsRoutingBaseService.ts +++ b/packages/lib/server/service/InsightsRoutingBaseService.ts @@ -11,19 +11,19 @@ import { isSingleSelectFilterValue, } from "@calcom/features/data-table/lib/utils"; import type { DateRange } from "@calcom/features/insights/server/insightsDateUtils"; +import { PermissionCheckService } from "@calcom/features/pbac/services/permission-check.service"; import type { PrismaClient } from "@calcom/prisma"; import { Prisma } from "@calcom/prisma/client"; import type { BookingStatus } from "@calcom/prisma/enums"; import { MembershipRole } from "@calcom/prisma/enums"; -import { MembershipRepository } from "../repository/membership"; import { TeamRepository } from "../repository/team"; export const insightsRoutingServiceOptionsSchema = z.discriminatedUnion("scope", [ z.object({ scope: z.literal("user"), userId: z.number(), - orgId: z.number(), + orgId: z.number().nullish().optional(), }), z.object({ scope: z.literal("org"), @@ -33,7 +33,7 @@ export const insightsRoutingServiceOptionsSchema = z.discriminatedUnion("scope", z.object({ scope: z.literal("team"), userId: z.number(), - orgId: z.number(), + orgId: z.number().nullish().optional(), teamId: z.number(), }), ]); @@ -41,8 +41,8 @@ export const insightsRoutingServiceOptionsSchema = z.discriminatedUnion("scope", export type InsightsRoutingServicePublicOptions = { scope: "user" | "org" | "team"; userId: number; - orgId: number | null; - teamId: number | undefined; + orgId: number | null | undefined; + teamId?: number; }; export type InsightsRoutingServiceOptions = z.infer; @@ -852,27 +852,40 @@ export class InsightsRoutingBaseService { options: Extract ): Promise { const teamRepo = new TeamRepository(this.prisma); - const childTeamOfOrg = await teamRepo.findByIdAndParentId({ - id: options.teamId, - parentId: options.orgId, - select: { id: true }, - }); - if (options.orgId && !childTeamOfOrg) { - return NOTHING_CONDITION; + + if (options.orgId) { + // team under org + const childTeamOfOrg = await teamRepo.findByIdAndParentId({ + id: options.teamId, + parentId: options.orgId, + select: { id: true }, + }); + if (!childTeamOfOrg) { + // teamId and its orgId does not match + return NOTHING_CONDITION; + } + } else { + // standalone team + const team = await teamRepo.findById({ + id: options.teamId, + }); + if (team?.parentId) { + // a team without orgId is not supposed to have parentId + return NOTHING_CONDITION; + } } return Prisma.sql`rfrd."formTeamId" = ${options.teamId}`; } private async isOwnerOrAdmin(userId: number, targetId: number): Promise { - // Check if the user is an owner or admin of the organization or team - const membership = await MembershipRepository.findUniqueByUserIdAndTeamId({ userId, teamId: targetId }); - return Boolean( - membership && - membership.accepted && - membership.role && - (membership.role === MembershipRole.OWNER || membership.role === MembershipRole.ADMIN) - ); + const permissionCheckService = new PermissionCheckService(); + return await permissionCheckService.checkPermission({ + userId, + teamId: targetId, + permission: "insights.read", + fallbackRoles: [MembershipRole.OWNER, MembershipRole.ADMIN], + }); } private buildFormFieldSqlCondition(fieldId: string, filterValue: FilterValue): Prisma.Sql | null { diff --git a/packages/lib/server/service/__tests__/InsightsBookingService.integration-test.ts b/packages/lib/server/service/__tests__/InsightsBookingService.integration-test.ts index 6c08f7ac83..e1059a47e1 100644 --- a/packages/lib/server/service/__tests__/InsightsBookingService.integration-test.ts +++ b/packages/lib/server/service/__tests__/InsightsBookingService.integration-test.ts @@ -6,7 +6,10 @@ import type { Team, User, Membership } from "@calcom/prisma/client"; import { Prisma } from "@calcom/prisma/client"; import { BookingStatus, MembershipRole } from "@calcom/prisma/enums"; -import { InsightsBookingBaseService as InsightsBookingService } from "../InsightsBookingBaseService"; +import { + InsightsBookingBaseService as InsightsBookingService, + type InsightsBookingServicePublicOptions, +} from "../InsightsBookingBaseService"; const NOTHING_CONDITION = Prisma.sql`1=0`; @@ -204,7 +207,7 @@ describe("InsightsBookingService Integration Tests", () => { it("should return NOTHING for invalid options", async () => { const service = new InsightsBookingService({ prisma, - options: null as any, + options: null as unknown as InsightsBookingServicePublicOptions, }); const conditions = await service.getAuthorizationConditions(); @@ -345,6 +348,170 @@ describe("InsightsBookingService Integration Tests", () => { await testData.cleanup(); }); + + it("should build user scope conditions with null orgId", async () => { + const testData = await createTestData({ + teamRole: MembershipRole.OWNER, + orgRole: MembershipRole.OWNER, + }); + + const service = new InsightsBookingService({ + prisma, + options: { + scope: "user", + userId: testData.user.id, + orgId: null, + }, + }); + + const conditions = await service.getAuthorizationConditions(); + expect(conditions).toEqual(Prisma.sql`("userId" = ${testData.user.id}) AND ("teamId" IS NULL)`); + + await testData.cleanup(); + }); + + it("should build user scope conditions with undefined orgId", async () => { + const testData = await createTestData({ + teamRole: MembershipRole.OWNER, + orgRole: MembershipRole.OWNER, + }); + + const service = new InsightsBookingService({ + prisma, + options: { + scope: "user", + userId: testData.user.id, + orgId: null, + }, + }); + + const conditions = await service.getAuthorizationConditions(); + expect(conditions).toEqual(Prisma.sql`("userId" = ${testData.user.id}) AND ("teamId" IS NULL)`); + + await testData.cleanup(); + }); + + it("should build team scope conditions with null orgId for standalone team", async () => { + const testData = await createTestData({ + teamRole: MembershipRole.OWNER, + orgRole: MembershipRole.OWNER, + }); + + const standaloneTeam = await prisma.team.create({ + data: { + name: "Standalone Team", + slug: `standalone-team-${Date.now()}-${Math.random().toString(36).substring(7)}`, + isOrganization: false, + parentId: null, + }, + }); + + await prisma.membership.create({ + data: { + userId: testData.user.id, + teamId: standaloneTeam.id, + role: MembershipRole.OWNER, + accepted: true, + }, + }); + + const service = new InsightsBookingService({ + prisma, + options: { + scope: "team", + userId: testData.user.id, + orgId: null, + teamId: standaloneTeam.id, + }, + }); + + const conditions = await service.getAuthorizationConditions(); + expect(conditions).toEqual( + Prisma.sql`(("teamId" = ${standaloneTeam.id}) AND ("isTeamBooking" = true)) OR (("userId" = ANY(${[ + testData.user.id, + ]})) AND ("isTeamBooking" = false))` + ); + + await prisma.membership.deleteMany({ + where: { teamId: standaloneTeam.id }, + }); + await prisma.team.delete({ + where: { id: standaloneTeam.id }, + }); + await testData.cleanup(); + }); + + it("should return NOTHING_CONDITION for team scope when team belongs to org but no orgId provided", async () => { + const testData = await createTestData({ + teamRole: MembershipRole.OWNER, + orgRole: MembershipRole.OWNER, + }); + + const service = new InsightsBookingService({ + prisma, + options: { + scope: "team", + userId: testData.user.id, + orgId: null, + teamId: testData.team.id, + }, + }); + + const conditions = await service.getAuthorizationConditions(); + expect(conditions).toEqual(NOTHING_CONDITION); + + await testData.cleanup(); + }); + + it("should build team scope conditions with undefined orgId for standalone team", async () => { + const testData = await createTestData({ + teamRole: MembershipRole.OWNER, + orgRole: MembershipRole.OWNER, + }); + + const standaloneTeam = await prisma.team.create({ + data: { + name: "Standalone Team 2", + slug: `standalone-team-2-${Date.now()}-${Math.random().toString(36).substring(7)}`, + isOrganization: false, + parentId: null, + }, + }); + + await prisma.membership.create({ + data: { + userId: testData.user.id, + teamId: standaloneTeam.id, + role: MembershipRole.OWNER, + accepted: true, + }, + }); + + const service = new InsightsBookingService({ + prisma, + options: { + scope: "team", + userId: testData.user.id, + orgId: null, + teamId: standaloneTeam.id, + }, + }); + + const conditions = await service.getAuthorizationConditions(); + expect(conditions).toEqual( + Prisma.sql`(("teamId" = ${standaloneTeam.id}) AND ("isTeamBooking" = true)) OR (("userId" = ANY(${[ + testData.user.id, + ]})) AND ("isTeamBooking" = false))` + ); + + await prisma.membership.deleteMany({ + where: { teamId: standaloneTeam.id }, + }); + await prisma.team.delete({ + where: { id: standaloneTeam.id }, + }); + await testData.cleanup(); + }); }); describe("Filter Conditions", () => { diff --git a/packages/lib/server/service/__tests__/InsightsRoutingService.integration-test.ts b/packages/lib/server/service/__tests__/InsightsRoutingService.integration-test.ts index 25b222b628..a1f320307d 100644 --- a/packages/lib/server/service/__tests__/InsightsRoutingService.integration-test.ts +++ b/packages/lib/server/service/__tests__/InsightsRoutingService.integration-test.ts @@ -8,7 +8,10 @@ import type { Team, User, Membership } from "@calcom/prisma/client"; import { Prisma } from "@calcom/prisma/client"; import { BookingStatus, MembershipRole } from "@calcom/prisma/enums"; -import { InsightsRoutingBaseService as InsightsRoutingService } from "../../service/InsightsRoutingBaseService"; +import { + InsightsRoutingBaseService as InsightsRoutingService, + type InsightsRoutingServicePublicOptions, +} from "../../service/InsightsRoutingBaseService"; // SQL condition constants for testing const NOTHING_CONDITION = Prisma.sql`1=0`; @@ -251,7 +254,7 @@ describe("InsightsRoutingService Integration Tests", () => { it("should return NOTHING for invalid options", async () => { const service = new InsightsRoutingService({ prisma, - options: null as any, + options: null as unknown as InsightsRoutingServicePublicOptions, filters: createDefaultFilters(), }); @@ -330,6 +333,56 @@ describe("InsightsRoutingService Integration Tests", () => { await testData.cleanup(); }); + it("should build user scope conditions with null orgId", async () => { + const testData = await createTestData({ + teamRole: MembershipRole.OWNER, + orgRole: MembershipRole.OWNER, + }); + + const service = new InsightsRoutingService({ + prisma, + options: { + scope: "user", + userId: testData.user.id, + orgId: null, + teamId: undefined, + }, + filters: createDefaultFilters(), + }); + + const conditions = await service.getAuthorizationConditions(); + expect(conditions).toEqual( + Prisma.sql`rfrd."formUserId" = ${testData.user.id} AND rfrd."formTeamId" IS NULL` + ); + + await testData.cleanup(); + }); + + it("should build user scope conditions with undefined orgId", async () => { + const testData = await createTestData({ + teamRole: MembershipRole.OWNER, + orgRole: MembershipRole.OWNER, + }); + + const service = new InsightsRoutingService({ + prisma, + options: { + scope: "user", + userId: testData.user.id, + orgId: null, + teamId: undefined, + }, + filters: createDefaultFilters(), + }); + + const conditions = await service.getAuthorizationConditions(); + expect(conditions).toEqual( + Prisma.sql`rfrd."formUserId" = ${testData.user.id} AND rfrd."formTeamId" IS NULL` + ); + + await testData.cleanup(); + }); + it("should build team scope conditions", async () => { const testData = await createTestData({ teamRole: MembershipRole.OWNER, @@ -426,6 +479,123 @@ describe("InsightsRoutingService Integration Tests", () => { }); await testData.cleanup(); }); + + it("should build team scope conditions with null orgId for standalone team", async () => { + const testData = await createTestData({ + teamRole: MembershipRole.OWNER, + orgRole: MembershipRole.OWNER, + }); + + const standaloneTeam = await prisma.team.create({ + data: { + name: "Standalone Team", + slug: `standalone-team-${randomUUID()}`, + isOrganization: false, + parentId: null, + }, + }); + + await prisma.membership.create({ + data: { + userId: testData.user.id, + teamId: standaloneTeam.id, + role: MembershipRole.OWNER, + accepted: true, + }, + }); + + const service = new InsightsRoutingService({ + prisma, + options: { + scope: "team", + userId: testData.user.id, + orgId: null, + teamId: standaloneTeam.id, + }, + filters: createDefaultFilters(), + }); + + const conditions = await service.getAuthorizationConditions(); + expect(conditions).toEqual(Prisma.sql`rfrd."formTeamId" = ${standaloneTeam.id}`); + + await prisma.membership.deleteMany({ + where: { teamId: standaloneTeam.id }, + }); + await prisma.team.delete({ + where: { id: standaloneTeam.id }, + }); + await testData.cleanup(); + }); + + it("should return NOTHING_CONDITION for team scope when team belongs to org but no orgId provided", async () => { + const testData = await createTestData({ + teamRole: MembershipRole.OWNER, + orgRole: MembershipRole.OWNER, + }); + + const service = new InsightsRoutingService({ + prisma, + options: { + scope: "team", + userId: testData.user.id, + orgId: null, + teamId: testData.team.id, + }, + filters: createDefaultFilters(), + }); + + const conditions = await service.getAuthorizationConditions(); + expect(conditions).toEqual(NOTHING_CONDITION); + + await testData.cleanup(); + }); + + it("should build team scope conditions with undefined orgId for standalone team", async () => { + const testData = await createTestData({ + teamRole: MembershipRole.OWNER, + orgRole: MembershipRole.OWNER, + }); + + const standaloneTeam = await prisma.team.create({ + data: { + name: "Standalone Team 2", + slug: `standalone-team-2-${randomUUID()}`, + isOrganization: false, + parentId: null, + }, + }); + + await prisma.membership.create({ + data: { + userId: testData.user.id, + teamId: standaloneTeam.id, + role: MembershipRole.OWNER, + accepted: true, + }, + }); + + const service = new InsightsRoutingService({ + prisma, + options: { + scope: "team", + userId: testData.user.id, + orgId: null, + teamId: standaloneTeam.id, + }, + filters: createDefaultFilters(), + }); + + const conditions = await service.getAuthorizationConditions(); + expect(conditions).toEqual(Prisma.sql`rfrd."formTeamId" = ${standaloneTeam.id}`); + + await prisma.membership.deleteMany({ + where: { teamId: standaloneTeam.id }, + }); + await prisma.team.delete({ + where: { id: standaloneTeam.id }, + }); + await testData.cleanup(); + }); }); describe("Filter Conditions", () => {