feat: add PBAC permission checks to insights#23592
Conversation
- Add layout-level protection for insights pages with session.user.org.id check - Add API-level protection for insights tRPC endpoints using insightsPbacProcedure - Enhance routing form insights with PBAC team filtering - Use insights.read permission with OWNER/ADMIN fallback roles - Replace all userBelongsToTeamProcedure with insightsPbacProcedure in insights router - Fix lint issues: remove unused variables and functions, fix non-null assertion Implements Permission-Based Access Control (PBAC) for insights functionality following the same patterns as event type PBAC implementation from PR #22618. Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
- Remove unused isFormCreateEditAllowed import - Remove unused TRPCError import - Change MembershipRole to type import for better tree-shaking 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:
|
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
WalkthroughAdds server-side checkInsightsPagePermission() to gate Insights pages using feature flag, authentication, and hasInsightsPermission; redirects on failure and returns session on success. Updates insights/page.tsx and routing subpages to call this helper. Removes feature-flag gating from insights/layout.tsx. Introduces hasInsightsPermission for PBAC checks across teams or a specific organization. Converts a Prisma import in routing-events.ts to type-only. Centralizes access control in trpc-router.ts via insightsPbacProcedure, migrating many endpoints to it, removing some helpers/imports, and making minor refactors (time-range precompute, defensive map access, error wrapping). Possibly related PRs
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
dosubot
bot
added
insights
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
apps/web/app/(use-page-wrapper)/insights/layout.tsx (1)
1-63: Update recentNoShowGuests to use insightsPbacProcedure
insights.read is correctly registered in PBAC (packages/features/pbac/README.md), but recentNoShowGuests in packages/features/insights/server/trpc-router.ts (around line 1025) still uses userBelongsToTeamProcedure—switch it to insightsPbacProcedure to enforce the proper PBAC check.packages/features/insights/server/trpc-router.ts (6)
1025-1035: recentNoShowGuests isn’t PBAC-protected.Wrap with insightsPbacProcedure to keep parity with other endpoints.
- recentNoShowGuests: userBelongsToTeamProcedure + recentNoShowGuests: insightsPbacProcedure .input(bookingRepositoryBaseInputSchema) .query(async ({ ctx, input }) => {
325-347: Avoid passing orgId: 0; use undefined/null.0 can be misinterpreted downstream and complicates scope checks. Align with createInsightsRoutingService.
- orgId: ctx.user.organizationId ?? 0, + orgId: ctx.user.organizationId ?? undefined,
503-559: Average duration: push aggregation to DB; avoid iterating all bookings in Node.findAll + per-row dayjs formatting will not scale. Prefer a grouped AVG(eventLength) per bucket in the service (SQL GROUP BY truncated date) and return already-aggregated rows.
If immediate change isn’t feasible, at least pre-compute bucket start timestamps and use numeric keys to reduce dayjs churn.
661-688: Replace Prisma include with select per guidelines.Minimize payload and match repo standards.
- const belongsToTeams = await ctx.insightsDb.membership.findMany({ + const belongsToTeams = await ctx.insightsDb.membership.findMany({ where: { team: { slug: { not: null }, }, accepted: true, userId: user.id, OR: [ { role: "ADMIN" }, { role: "OWNER" }, ], }, - include: { - team: { - select: { - id: true, - name: true, - logoUrl: true, - slug: true, - metadata: true, - }, - }, - }, + select: { + team: { + select: { + id: true, + name: true, + logoUrl: true, + slug: true, + metadata: true, + }, + }, + }, });
732-744: Replace include with select in user lookups.Follow select-only rule and fetch only required fields.
- const membership = await ctx.insightsDb.membership.findFirst({ + const membership = await ctx.insightsDb.membership.findFirst({ where: { userId: user.id, teamId, accepted: true, }, - include: { - user: { - select: userSelect, - }, - }, + select: { + role: true, + user: { select: userSelect }, + }, }); @@ - const usersInTeam = await ctx.insightsDb.membership.findMany({ + const usersInTeam = await ctx.insightsDb.membership.findMany({ where: { teamId, accepted: true, }, - include: { - user: { - select: userSelect, - }, - }, + select: { + user: { select: userSelect }, + }, distinct: ["userId"], });Also applies to: 753-764
47-172: Enforce PBAC in buildBaseWhereCondition org-scope
buildBaseWhereCondition’s org-wide branch (packages/features/insights/server/trpc-router.ts) currently includes all teams and users under the organization without filtering by per-team “insights.read” permission. Before querying, call PermissionCheckService.getTeamIdsWithPermission(ctx.user.id, "insights.read") and intersect the result with the computedteamIds(and deriveduserIds) to enforce PBAC.
🧹 Nitpick comments (4)
packages/app-store/routing-forms/trpc/permissions.ts (2)
31-37: Nit: fix error string grammar.Prefer “permission for this …” over “permission to this …”.
- message: "You don't have permission to this personal-scoped routing form", + message: "You don't have permission for this personal-scoped routing form",
39-48: Optional: reuse PermissionCheckService instance.Constructing a new service per call is fine but unnecessary. Consider injecting/reusing a singleton to reduce overhead and ease testing.
apps/web/app/(use-page-wrapper)/insights/layout.tsx (1)
25-26: Nit: headers() and cookies() are synchronous.Remove unnecessary awaits.
- const session = await getServerSession({ req: buildLegacyRequest(await headers(), await cookies()) }); + const session = await getServerSession({ req: buildLegacyRequest(headers(), cookies()) });packages/features/insights/server/trpc-router.ts (1)
174-238: Tighten membership checks: enum roles and null-safe teamId guard.Use MembershipRole enum to avoid typos and treat 0/NaN distinctly.
- if (parse.data.teamId) { + if (parse.data.teamId != null) { membershipWhereConditional["teamId"] = parse.data.teamId; } @@ - role: { - in: ["OWNER", "ADMIN"], - }, + role: { in: [MembershipRole.OWNER, MembershipRole.ADMIN] },
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
apps/web/app/(use-page-wrapper)/insights/layout.tsx(2 hunks)packages/app-store/routing-forms/trpc/formMutation.handler.ts(0 hunks)packages/app-store/routing-forms/trpc/formQuery.handler.ts(0 hunks)packages/app-store/routing-forms/trpc/permissions.ts(1 hunks)packages/features/insights/server/routing-events.ts(3 hunks)packages/features/insights/server/trpc-router.ts(23 hunks)
💤 Files with no reviewable changes (2)
- packages/app-store/routing-forms/trpc/formMutation.handler.ts
- packages/app-store/routing-forms/trpc/formQuery.handler.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
apps/web/app/(use-page-wrapper)/insights/layout.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
apps/web/app/(use-page-wrapper)/insights/layout.tsxpackages/app-store/routing-forms/trpc/permissions.tspackages/features/insights/server/routing-events.tspackages/features/insights/server/trpc-router.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
apps/web/app/(use-page-wrapper)/insights/layout.tsxpackages/app-store/routing-forms/trpc/permissions.tspackages/features/insights/server/routing-events.tspackages/features/insights/server/trpc-router.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/app-store/routing-forms/trpc/permissions.tspackages/features/insights/server/routing-events.tspackages/features/insights/server/trpc-router.ts
🧠 Learnings (6)
📚 Learning: 2025-08-26T08:08:23.395Z
Learnt from: SinghaAnirban005
PR: calcom/cal.com#23343
File: packages/features/insights/server/trpc-router.ts:1080-1101
Timestamp: 2025-08-26T08:08:23.395Z
Learning: In packages/features/insights/server/trpc-router.ts, when filtering personal event types (userId provided, no teamId, not isAll), the query correctly uses user.id (authenticated user) instead of the input userId parameter for security reasons. This prevents users from accessing other users' personal event types by passing arbitrary user IDs.
Applied to files:
packages/features/insights/server/routing-events.tspackages/features/insights/server/trpc-router.ts
📚 Learning: 2025-07-15T12:59:34.389Z
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.389Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
Applied to files:
packages/features/insights/server/routing-events.tspackages/features/insights/server/trpc-router.ts
📚 Learning: 2025-07-15T12:58:40.539Z
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/RoutingFunnel.tsx:15-17
Timestamp: 2025-07-15T12:58:40.539Z
Learning: In the insights routing funnel component (packages/features/insights/components/RoutingFunnel.tsx), the useColumnFilters exclusions are intentionally different from the general useInsightsParameters exclusions. RoutingFunnel specifically excludes only ["createdAt"] while useInsightsParameters excludes ["bookingUserId", "formId", "createdAt", "eventTypeId"]. This difference is by design.
Applied to files:
packages/features/insights/server/routing-events.ts
📚 Learning: 2025-07-28T11:50:23.946Z
Learnt from: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-28T11:50:23.946Z
Learning: Applies to **/*.ts : For Prisma queries, only select data you need; never use `include`, always use `select`
Applied to files:
packages/features/insights/server/routing-events.ts
📚 Learning: 2025-09-03T09:52:51.182Z
Learnt from: hariombalhara
PR: calcom/cal.com#23541
File: packages/features/bookings/lib/di/modules/RegularBookingServiceModule.ts:22-28
Timestamp: 2025-09-03T09:52:51.182Z
Learning: The IBookingServiceDependencies interface in packages/features/bookings/lib/handleNewBooking.ts contains 6 properties: cacheService, checkBookingAndDurationLimitsService, prismaClient, bookingRepository, featuresRepository, and checkBookingLimitsService. This interface is used by RegularBookingService and matches the depsMap structure in RegularBookingServiceModule.
Applied to files:
packages/features/insights/server/trpc-router.ts
📚 Learning: 2025-08-21T13:59:04.230Z
Learnt from: supalarry
PR: calcom/cal.com#23217
File: docs/api-reference/v2/openapi.json:17226-17229
Timestamp: 2025-08-21T13:59:04.230Z
Learning: In the cal.com codebase, permission logic is handled by business logic in specific endpoints/services rather than being enforced at the API schema level. The schema fields like `bookingRequiresAuthentication` serve as indicators, but the actual permission decisions are made by the individual services.
Applied to files:
packages/features/insights/server/trpc-router.ts
🧬 Code graph analysis (3)
apps/web/app/(use-page-wrapper)/insights/layout.tsx (3)
packages/features/auth/lib/getServerSession.ts (1)
getServerSession(32-136)apps/web/lib/buildLegacyCtx.ts (1)
buildLegacyRequest(47-49)packages/features/pbac/services/permission-check.service.ts (2)
PermissionCheckService(21-366)hasPermission(221-239)
packages/features/insights/server/routing-events.ts (1)
packages/features/pbac/services/permission-check.service.ts (1)
PermissionCheckService(21-366)
packages/features/insights/server/trpc-router.ts (3)
packages/features/pbac/services/permission-check.service.ts (2)
PermissionCheckService(21-366)hasPermission(221-239)packages/features/insights/server/raw-data.schema.ts (1)
bookingRepositoryBaseInputSchema(103-110)packages/features/insights/server/insightsDateUtils.ts (2)
getTimeView(20-31)getDateRanges(33-117)
🔇 Additional comments (6)
packages/app-store/routing-forms/trpc/permissions.ts (1)
4-4: Type-only enum import is correct.Good switch to a type-only import; avoids bundling the enum at runtime here.
packages/features/insights/server/routing-events.ts (2)
114-121: Prisma selects look good.Only required fields are selected; no include usage. Keep this pattern.
Also applies to: 126-135, 144-153, 164-174
248-299: All calls to getRoutingFormFieldOptions and getRoutingFormHeaders are already PBAC-protected
Every invocation lives inside an insightsPbacProcedure-wrapped tRPC handler, so no additional PBAC logic is needed here.packages/features/insights/server/trpc-router.ts (3)
845-855: Good: server-authoritative userId for routing forms.Using ctx.user.id (ignoring client-provided userId) matches prior security learning and prevents cross-user access.
768-793: Event type list under PBAC wrapper looks correct.Input is respected; personal queries use authenticated user.id, and org/owner-admin flows are preserved.
240-263: Enforce insights.read for team and org scopes
Verified that “insights.read” is registered in PBAC permissions (packages/features/pbac/README.md:181,385), and the proposed diff correctly applies permission checks for orgId, teamId, and selectedTeamId.
| if (session.user.org?.id) { | ||
| const pbacFeatureEnabled = await featuresRepository.checkIfTeamHasFeature(session.user.org.id, "pbac"); | ||
|
|
||
| if (pbacFeatureEnabled) { | ||
| const permissionCheckService = new PermissionCheckService(); | ||
| const hasPermission = await permissionCheckService.checkPermission({ | ||
| userId: session.user.id, | ||
| teamId: session.user.org.id, | ||
| permission: "insights.read", | ||
| fallbackRoles: [MembershipRole.OWNER, MembershipRole.ADMIN], | ||
| }); | ||
|
|
||
| if (!hasPermission) { | ||
| redirect("/"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: fallback roles aren’t enforced when PBAC is disabled.
You only call checkPermission when PBAC is enabled; this bypasses OWNER/ADMIN fallback in PBAC-off orgs. Call checkPermission unconditionally and let the service handle PBAC vs fallback.
- if (session.user.org?.id) {
- const pbacFeatureEnabled = await featuresRepository.checkIfTeamHasFeature(session.user.org.id, "pbac");
-
- if (pbacFeatureEnabled) {
- const permissionCheckService = new PermissionCheckService();
- const hasPermission = await permissionCheckService.checkPermission({
- userId: session.user.id,
- teamId: session.user.org.id,
- permission: "insights.read",
- fallbackRoles: [MembershipRole.OWNER, MembershipRole.ADMIN],
- });
-
- if (!hasPermission) {
- redirect("/");
- }
- }
- }
+ if (session.user.org?.id) {
+ const permissionCheckService = new PermissionCheckService();
+ const hasPermission = await permissionCheckService.checkPermission({
+ userId: session.user.id,
+ teamId: session.user.org.id,
+ permission: "insights.read",
+ fallbackRoles: [MembershipRole.OWNER, MembershipRole.ADMIN],
+ });
+ if (!hasPermission) {
+ redirect("/");
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (session.user.org?.id) { | |
| const pbacFeatureEnabled = await featuresRepository.checkIfTeamHasFeature(session.user.org.id, "pbac"); | |
| if (pbacFeatureEnabled) { | |
| const permissionCheckService = new PermissionCheckService(); | |
| const hasPermission = await permissionCheckService.checkPermission({ | |
| userId: session.user.id, | |
| teamId: session.user.org.id, | |
| permission: "insights.read", | |
| fallbackRoles: [MembershipRole.OWNER, MembershipRole.ADMIN], | |
| }); | |
| if (!hasPermission) { | |
| redirect("/"); | |
| } | |
| } | |
| } | |
| if (session.user.org?.id) { | |
| const permissionCheckService = new PermissionCheckService(); | |
| const hasPermission = await permissionCheckService.checkPermission({ | |
| userId: session.user.id, | |
| teamId: session.user.org.id, | |
| permission: "insights.read", | |
| fallbackRoles: [MembershipRole.OWNER, MembershipRole.ADMIN], | |
| }); | |
| if (!hasPermission) { | |
| redirect("/"); | |
| } | |
| } |
🤖 Prompt for AI Agents
In apps/web/app/(use-page-wrapper)/insights/layout.tsx around lines 30 to 46,
the current code only calls PermissionCheckService.checkPermission when the PBAC
feature flag is enabled, which skips OWNER/ADMIN fallback for orgs with PBAC
disabled; remove the PBAC gating and call permissionCheckService.checkPermission
unconditionally (letting the service itself decide PBAC vs fallback), then
perform the redirect if hasPermission is false; you can also remove or repurpose
the featuresRepository.checkIfTeamHasFeature call if it is no longer needed.
| const featuresRepository = new FeaturesRepository(prisma); | ||
| const pbacFeatureEnabled = await featuresRepository.checkIfTeamHasFeature(organizationId || -1, "pbac"); | ||
|
|
||
| if (pbacFeatureEnabled) { | ||
| const permissionCheckService = new PermissionCheckService(); | ||
| const teamIds = await permissionCheckService.getTeamIdsWithPermission(userId, "insights.read"); | ||
|
|
||
| if (isAll && organizationId) { | ||
| const forms = await prisma.app_RoutingForms_Form.findMany({ | ||
| where: { | ||
| teamId: { in: teamIds }, | ||
| }, | ||
| select: { | ||
| responses: true, | ||
| id: true, | ||
| name: true, | ||
| _count: { | ||
| select: { | ||
| responses: true, | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
| return forms; | ||
| } else if (teamId && teamIds.includes(teamId)) { | ||
| const forms = await prisma.app_RoutingForms_Form.findMany({ | ||
| where: { teamId }, | ||
| select: { | ||
| id: true, | ||
| name: true, | ||
| _count: { | ||
| select: { | ||
| responses: true, | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
| return forms; | ||
| } else { | ||
| const forms = await prisma.app_RoutingForms_Form.findMany({ | ||
| where: { | ||
| userId, | ||
| teamId: null, | ||
| }, | ||
| select: { | ||
| id: true, | ||
| name: true, | ||
| _count: { | ||
| select: { | ||
| responses: true, | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
| return forms; | ||
| } | ||
| } else { | ||
| const formsWhereCondition = await this.getWhereForTeamOrAllTeams({ | ||
| userId, | ||
| teamId, | ||
| isAll, | ||
| organizationId, | ||
| }); | ||
| return await prisma.app_RoutingForms_Form.findMany({ | ||
| where: formsWhereCondition, | ||
| select: { | ||
| id: true, | ||
| name: true, | ||
| _count: { | ||
| select: { | ||
| responses: true, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Critical: cross-org data leak risk; intersect permitted teamIds with org teams.
When isAll && organizationId, you use all teamIds with permission, not just those in the org. Intersect with the org’s teams; also treat unauthorized teamId explicitly.
const featuresRepository = new FeaturesRepository(prisma);
const pbacFeatureEnabled = await featuresRepository.checkIfTeamHasFeature(organizationId || -1, "pbac");
if (pbacFeatureEnabled) {
const permissionCheckService = new PermissionCheckService();
- const teamIds = await permissionCheckService.getTeamIdsWithPermission(userId, "insights.read");
+ const teamIds = await permissionCheckService.getTeamIdsWithPermission(userId, "insights.read");
+ // Limit to teams in the provided organization (parent + children)
+ const orgTeamIds = organizationId
+ ? (
+ await prisma.team.findMany({
+ where: { OR: [{ id: organizationId }, { parentId: organizationId }] },
+ select: { id: true },
+ })
+ ).map((t) => t.id)
+ : [];
+ const allowedTeamIds = orgTeamIds.length ? teamIds.filter((id) => orgTeamIds.includes(id)) : teamIds;
- if (isAll && organizationId) {
+ if (isAll && organizationId) {
const forms = await prisma.app_RoutingForms_Form.findMany({
where: {
- teamId: { in: teamIds },
+ teamId: { in: allowedTeamIds },
},
select: {
id: true,
name: true,
_count: {
select: {
responses: true,
},
},
},
});
return forms;
- } else if (teamId && teamIds.includes(teamId)) {
+ } else if (teamId && allowedTeamIds.includes(teamId)) {
const forms = await prisma.app_RoutingForms_Form.findMany({
where: { teamId },
select: {
id: true,
name: true,
_count: {
select: {
responses: true,
},
},
},
});
return forms;
- } else {
+ } else if (!teamId) {
const forms = await prisma.app_RoutingForms_Form.findMany({
where: {
userId,
teamId: null,
},
select: {
id: true,
name: true,
_count: {
select: {
responses: true,
},
},
},
});
return forms;
+ } else {
+ // teamId provided but not authorized within this org
+ return [];
}
} else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const featuresRepository = new FeaturesRepository(prisma); | |
| const pbacFeatureEnabled = await featuresRepository.checkIfTeamHasFeature(organizationId || -1, "pbac"); | |
| if (pbacFeatureEnabled) { | |
| const permissionCheckService = new PermissionCheckService(); | |
| const teamIds = await permissionCheckService.getTeamIdsWithPermission(userId, "insights.read"); | |
| if (isAll && organizationId) { | |
| const forms = await prisma.app_RoutingForms_Form.findMany({ | |
| where: { | |
| teamId: { in: teamIds }, | |
| }, | |
| select: { | |
| responses: true, | |
| id: true, | |
| name: true, | |
| _count: { | |
| select: { | |
| responses: true, | |
| }, | |
| }, | |
| }, | |
| }); | |
| return forms; | |
| } else if (teamId && teamIds.includes(teamId)) { | |
| const forms = await prisma.app_RoutingForms_Form.findMany({ | |
| where: { teamId }, | |
| select: { | |
| id: true, | |
| name: true, | |
| _count: { | |
| select: { | |
| responses: true, | |
| }, | |
| }, | |
| }, | |
| }); | |
| return forms; | |
| } else { | |
| const forms = await prisma.app_RoutingForms_Form.findMany({ | |
| where: { | |
| userId, | |
| teamId: null, | |
| }, | |
| select: { | |
| id: true, | |
| name: true, | |
| _count: { | |
| select: { | |
| responses: true, | |
| }, | |
| }, | |
| }, | |
| }); | |
| return forms; | |
| } | |
| } else { | |
| const formsWhereCondition = await this.getWhereForTeamOrAllTeams({ | |
| userId, | |
| teamId, | |
| isAll, | |
| organizationId, | |
| }); | |
| return await prisma.app_RoutingForms_Form.findMany({ | |
| where: formsWhereCondition, | |
| select: { | |
| id: true, | |
| name: true, | |
| _count: { | |
| select: { | |
| responses: true, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }); | |
| }); | |
| } | |
| const featuresRepository = new FeaturesRepository(prisma); | |
| const pbacFeatureEnabled = await featuresRepository.checkIfTeamHasFeature(organizationId || -1, "pbac"); | |
| if (pbacFeatureEnabled) { | |
| const permissionCheckService = new PermissionCheckService(); | |
| const teamIds = await permissionCheckService.getTeamIdsWithPermission(userId, "insights.read"); | |
| // Limit to teams in the provided organization (parent + children) | |
| const orgTeamIds = organizationId | |
| ? ( | |
| await prisma.team.findMany({ | |
| where: { OR: [{ id: organizationId }, { parentId: organizationId }] }, | |
| select: { id: true }, | |
| }) | |
| ).map((t) => t.id) | |
| : []; | |
| const allowedTeamIds = orgTeamIds.length | |
| ? teamIds.filter((id) => orgTeamIds.includes(id)) | |
| : teamIds; | |
| if (isAll && organizationId) { | |
| const forms = await prisma.app_RoutingForms_Form.findMany({ | |
| where: { | |
| teamId: { in: allowedTeamIds }, | |
| }, | |
| select: { | |
| id: true, | |
| name: true, | |
| _count: { | |
| select: { | |
| responses: true, | |
| }, | |
| }, | |
| }, | |
| }); | |
| return forms; | |
| } else if (teamId && allowedTeamIds.includes(teamId)) { | |
| const forms = await prisma.app_RoutingForms_Form.findMany({ | |
| where: { teamId }, | |
| select: { | |
| id: true, | |
| name: true, | |
| _count: { | |
| select: { | |
| responses: true, | |
| }, | |
| }, | |
| }, | |
| }); | |
| return forms; | |
| } else if (!teamId) { | |
| const forms = await prisma.app_RoutingForms_Form.findMany({ | |
| where: { | |
| userId, | |
| teamId: null, | |
| }, | |
| select: { | |
| id: true, | |
| name: true, | |
| _count: { | |
| select: { | |
| responses: true, | |
| }, | |
| }, | |
| }, | |
| }); | |
| return forms; | |
| } else { | |
| // teamId provided but not authorized within this org | |
| return []; | |
| } | |
| } else { | |
| const formsWhereCondition = await this.getWhereForTeamOrAllTeams({ | |
| userId, | |
| teamId, | |
| isAll, | |
| organizationId, | |
| }); | |
| return await prisma.app_RoutingForms_Form.findMany({ | |
| where: formsWhereCondition, | |
| select: { | |
| id: true, | |
| name: true, | |
| _count: { | |
| select: { | |
| responses: true, | |
| }, | |
| }, | |
| }, | |
| }); | |
| } |
🤖 Prompt for AI Agents
In packages/features/insights/server/routing-events.ts around lines 101 to 175,
the branch handling pbac when isAll && organizationId currently uses all teamIds
the user has permission for regardless of organization, risking cross-org data
exposure; change it to fetch the organization’s team IDs (via prisma) and
compute the intersection with the permissioned teamIds, then use that
intersected list in the findMany where clause; additionally, when a specific
teamId is passed, explicitly check that teamId is in the
intersected/permissioned list and return an empty array or throw a permission
error if not authorized instead of falling back to user-scoped forms, and ensure
the final else/queries use the intersected team set so only forms belonging to
teams within the org and the user’s permission are returned.
| const forms = await prisma.app_RoutingForms_Form.findMany({ | ||
| where: { | ||
| teamId: { in: teamIds }, | ||
| }, | ||
| select: { | ||
| responses: true, | ||
| id: true, | ||
| name: true, | ||
| _count: { | ||
| select: { | ||
| responses: true, | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
| return forms; | ||
| } else if (teamId && teamIds.includes(teamId)) { | ||
| const forms = await prisma.app_RoutingForms_Form.findMany({ | ||
| where: { teamId }, | ||
| select: { | ||
| id: true, | ||
| name: true, | ||
| _count: { | ||
| select: { | ||
| responses: true, | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
| return forms; | ||
| } else { | ||
| const forms = await prisma.app_RoutingForms_Form.findMany({ | ||
| where: { | ||
| userId, | ||
| teamId: null, | ||
| }, | ||
| select: { | ||
| id: true, | ||
| name: true, | ||
| _count: { | ||
| select: { | ||
| responses: true, | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
| return forms; | ||
| } | ||
| } else { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Behavior clarity: avoid silent fallback to personal when unauthorized teamId is supplied.
Returning personal forms when an unauthorized teamId is requested can confuse clients. The diff above returns [] instead; alternatively, raise a typed error at the tRPC layer.
| const pbacFeatureEnabled = await featuresRepository.checkIfTeamHasFeature(session.user.org.id, "pbac"); | ||
|
|
||
| if (pbacFeatureEnabled) { |
There was a problem hiding this comment.
We dont need to do this here - the permissionCheckService does this automatically with fallbackRoles
One thing i noticed - when i have zero perms we still show navigation in the shell - then when i click them its just an instant redirect. No indication that i dont have permission or anything. 🤔 i wonder what the best approach is? Do we just remove from shell?
|
yeah I actually had the same thought. we currently redirect to / if no permission, but the unusable menu item doesn't make much sense. either
or
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/features/insights/server/trpc-router.ts (3)
173-237: Critical: unauthorized teamId can slip through when user has no org.
If a user specifies a teamId they don’t belong to and has no organization, userBelongsToTeamProcedure doesn’t throw; insightsPbacProcedure then checks any-team permission (not the specific team), so a user in some other team may pass and query another team’s data.Apply this minimal fix to enforce team membership when no org context exists:
const membership = await ctx.insightsDb.membership.findFirst({ where: membershipWhereConditional, }); let isOwnerAdminOfParentTeam = false; + // If a specific team was requested and user doesn't belong to it, and there's no org context, block early. + if (parse.data.teamId && !membership && !ctx.user.organizationId) { + throw new TRPCError({ code: "UNAUTHORIZED" }); + } + // Probably we couldn't find a membership because the user is not a direct member of the team // So that would mean ctx.user.organization is present if ((parse.data.isAll && ctx.user.organizationId) || (!membership && ctx.user.organizationId)) {Follow-up (optional, stronger PBAC): also validate the specific teamId in insightsPbacProcedure using PermissionCheckService for that team when present.
Do you want me to wire the parsed input (teamId/isAll) into ctx so insightsPbacProcedure can PBAC-check the exact team?
Also applies to: 239-253
701-714: User list (org scope): filter only accepted memberships.
This branch may include pending/invited users. Add accepted: true.- const usersInTeam = await ctx.insightsDb.membership.findMany({ + const usersInTeam = await ctx.insightsDb.membership.findMany({ where: { team: { parentId: user.organizationId, }, + accepted: true, },Also consider using select instead of include in related queries per guideline.
255-261: Remove sensitivecredential.keyfrom tRPC responses
– Inpackages/trpc/server/routers/viewer/apps/updateAppCredentials.handler.ts:66, you’re returningcredential.keyin the response payload; strip or redact this field.
♻️ Duplicate comments (2)
packages/features/insights/server/routing-events.ts (1)
34-66: Avoid silent fallback; return no data when teamId/isAll unauthorized.
If membership filtering removes all teamIds, the code currently falls back to personal forms, which can confuse clients when they explicitly requested a team/org scope.Apply this diff to return an empty result instead of personal forms when a specific team or org scope was requested but not authorized:
- // Filter teamIds to only include teams the user has access to - if (teamIds.length > 0) { + // Filter teamIds to only include teams the user has access to + if (teamIds.length > 0) { const accessibleTeams = await prisma.membership.findMany({ where: { userId: userId ?? -1, teamId: { in: teamIds, }, accepted: true, }, select: { teamId: true, }, }); teamIds = accessibleTeams.map((membership) => membership.teamId); } // Base where condition for forms - const formsWhereCondition: WhereForTeamOrAllTeams = { - ...(teamIds.length > 0 - ? { - teamId: { - in: teamIds, - }, - } - : { - userId: userId ?? -1, - teamId: null, - }), - ...(routingFormId && { - id: routingFormId, - }), - }; + const formsWhereCondition: WhereForTeamOrAllTeams = + teamIds.length > 0 + ? { + teamId: { in: teamIds }, + ...(routingFormId && { id: routingFormId }), + } + : // If a team or org scope was requested but user isn't authorized, return no rows + (teamId || isAll) + ? ({ + // empty IN yields no results + id: { in: [] }, + } as WhereForTeamOrAllTeams) + : { + // personal forms + userId: userId ?? -1, + teamId: null, + ...(routingFormId && { id: routingFormId }), + };Also applies to: 67-82
apps/web/app/(use-page-wrapper)/insights/router-position/page.tsx (1)
17-18: Same UX note as routing page.Apply the nav-hiding or “no access” page approach consistently to avoid redirect flicker.
🧹 Nitpick comments (17)
packages/features/insights/server/routing-events.ts (2)
34-66: Org-scope under-authorization for org admins.
When isAll && organizationId, org admins who aren’t direct members of child teams lose access due to the membership filter. Consider skipping the membership filter for org scope (and rely on the upstream check that only OWNER/ADMIN may use org scope), or pass an explicit isOwnerAdminOfParentTeam flag into this method to decide filtering.Do you want me to wire ctx.user.isOwnerAdminOfParentTeam through the TRPC call sites and add it to RoutingFormInsightsTeamFilter?
278-298: Duplicate CSV utility across modules.
objectToCsv is duplicated here and in trpc-router.ts. Extract to a shared utility to avoid drift.apps/web/app/(use-page-wrapper)/insights/layout.tsx (1)
12-20: Hide or “lock” the Insights nav for users without access.
Given the review thread, consider gating the shell/nav items with a server-side hasInsightsPermission check (or exposing a boolean from checkInsightsPagePermission) to remove the menu or show a “no access” tooltip/badge instead of redirecting after click.packages/features/insights/server/hasInsightsPermission.ts (1)
21-36: Reduce latency by reusing the service and checking teams in parallel.
Current sequential awaits can add noticeable latency for users in many teams.Apply this diff:
-export async function hasInsightsPermission({ +export async function hasInsightsPermission({ userId, organizationId, }: { userId: number; organizationId: number | null | undefined; }) { - if (organizationId) { - const permissionCheckService = new PermissionCheckService(); + const permissionCheckService = new PermissionCheckService(); + if (organizationId) { return await permissionCheckService.checkPermission({ userId, teamId: organizationId, permission: "insights.read", fallbackRoles: [MembershipRole.OWNER, MembershipRole.ADMIN, MembershipRole.MEMBER], // even members can see their own data }); } else { - const permissionCheckService = new PermissionCheckService(); const teamIds = await MembershipRepository.findUserTeamIds({ userId }); - for (const teamId of teamIds) { - const hasPermission = await permissionCheckService.checkPermission({ - userId, - teamId, - permission: "insights.read", - fallbackRoles: [MembershipRole.OWNER, MembershipRole.ADMIN, MembershipRole.MEMBER], // even members can see their own data - }); - - if (hasPermission) { - return true; - } - } + const checks = await Promise.all( + teamIds.map((teamId) => + permissionCheckService.checkPermission({ + userId, + teamId, + permission: "insights.read", + fallbackRoles: [MembershipRole.OWNER, MembershipRole.ADMIN, MembershipRole.MEMBER], + }) + ) + ); + if (checks.some(Boolean)) return true; } return false; }packages/features/insights/server/trpc-router.ts (6)
532-536: Null-guard on Map read: LGTM; consider reducing dayjs in the hot loop.
You call dayjs per booking. If volumes are high, pre-group with integer timestamps (e.g., epoch bucket keys) or cache startOf(period) computations.
666-676: Prefer select over include to limit payloads.
Switch include to select to follow our Prisma guideline and avoid fetching unused membership fields.Apply:
- include: { - team: { - select: { - id: true, - name: true, - logoUrl: true, - slug: true, - metadata: true, - }, - }, - }, + select: { + team: { + select: { + id: true, + name: true, + logoUrl: true, + slug: true, + metadata: true, + }, + }, + },
721-733: Replace include with select and explicitly select role.
membership.role is used later; with select you must ask for it.- const membership = await ctx.insightsDb.membership.findFirst({ - where: { - userId: user.id, - teamId, - accepted: true, - }, - include: { - user: { - select: userSelect, - }, - }, - }); + const membership = await ctx.insightsDb.membership.findFirst({ + where: { userId: user.id, teamId, accepted: true }, + select: { + role: true, + user: { select: userSelect }, + }, + });
742-753: Use select instead of include to fetch users.
Aligns with “select only what you need.”- const usersInTeam = await ctx.insightsDb.membership.findMany({ + const usersInTeam = await ctx.insightsDb.membership.findMany({ where: { teamId, accepted: true, }, - include: { - user: { - select: userSelect, - }, - }, + select: { + user: { select: userSelect }, + }, distinct: ["userId"], });
834-844: Routing forms filter call: confirm org-scope behavior.
This delegates to RoutingEventsInsights which currently filters by direct memberships; org admins won’t see child-team forms unless they’re members. Either pass an orgScopeAllowed flag and broaden selection there, or document the limitation.
1170-1191: CSV helper duplication with routing-events.ts.
Extract to a shared utility to avoid duplication.apps/web/app/(use-page-wrapper)/insights/routing/page.tsx (1)
17-18: Avoid bounce UX for unauthorized Insights access
All Insights pages correctly callcheckInsightsPagePermission, but redirecting after rendering the nav causes a jarring “bounce.” Instead, either remove the Insights item from the shell nav when a user lacksinsights.reador render a lightweight server-side “no access” component with localized guidance.apps/web/app/(use-page-wrapper)/insights/page.tsx (2)
19-26: Remove the -1 fallback; session is guaranteed post-gate.After the gate,
session.user.idexists. Using?? -1can trigger an unintended throw if types drift. Rely on the invariant.- const session = await checkInsightsPagePermission(); - - const { timeZone } = await prisma.user.findUniqueOrThrow({ - where: { id: session?.user.id ?? -1 }, + const session = await checkInsightsPagePermission(); + const { timeZone } = await prisma.user.findUniqueOrThrow({ + where: { id: session.user.id }, select: { timeZone: true, }, });
3-3: Prisma import style is inconsistent with the permission gate file.This file uses a default import (
import prisma from "@calcom/prisma"), whilecheckInsightsPagePermission.tsuses a named import. Prefer one style repo-wide to avoid type/emit drift.apps/web/app/(use-page-wrapper)/insights/checkInsightsPagePermission.ts (4)
7-8: Unify Prisma import style to match other pages.Align with
apps/web/app/(use-page-wrapper)/insights/page.tsxwhich uses a default import.-import { prisma } from "@calcom/prisma"; +import prisma from "@calcom/prisma";
19-22: Drop unnecessary awaits on headers()/cookies().They’re synchronous in the App Router; awaiting them is misleading.
- const session = await getServerSession({ req: buildLegacyRequest(await headers(), await cookies()) }); + const session = await getServerSession({ req: buildLegacyRequest(headers(), cookies()) });
24-31: Confirm org scoping: passing organizationId may wrongly block users who have insights.read in non-org teams.If the product intent is “grant access when the user has insights.read in any team,” omit
organizationIdhere and let the helper fall back to cross-team checks. If the intent is “respect current org context,” keep it but ensure this matches page semantics.- const hasPermission = await hasInsightsPermission({ - userId: session.user.id, - organizationId: session.user.org?.id, - }); + const hasPermission = await hasInsightsPermission({ + userId: session.user.id, + // Intentionally omit organizationId to allow any-team permission; add back only if org-scoped gating is desired. + });
12-18: Optional: cache/short-circuit feature-flag reads.If
checkIfFeatureIsEnabledGlobally("insights")hits the DB each request, consider a short TTL cache to cut latency on hot routes.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
apps/web/app/(use-page-wrapper)/insights/checkInsightsPagePermission.ts(1 hunks)apps/web/app/(use-page-wrapper)/insights/layout.tsx(1 hunks)apps/web/app/(use-page-wrapper)/insights/page.tsx(2 hunks)apps/web/app/(use-page-wrapper)/insights/router-position/page.tsx(2 hunks)apps/web/app/(use-page-wrapper)/insights/routing/page.tsx(2 hunks)packages/features/insights/server/hasInsightsPermission.ts(1 hunks)packages/features/insights/server/routing-events.ts(1 hunks)packages/features/insights/server/trpc-router.ts(24 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
apps/web/app/(use-page-wrapper)/insights/routing/page.tsxapps/web/app/(use-page-wrapper)/insights/router-position/page.tsxapps/web/app/(use-page-wrapper)/insights/layout.tsxapps/web/app/(use-page-wrapper)/insights/page.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
apps/web/app/(use-page-wrapper)/insights/routing/page.tsxpackages/features/insights/server/hasInsightsPermission.tsapps/web/app/(use-page-wrapper)/insights/router-position/page.tsxapps/web/app/(use-page-wrapper)/insights/layout.tsxpackages/features/insights/server/routing-events.tsapps/web/app/(use-page-wrapper)/insights/checkInsightsPagePermission.tsapps/web/app/(use-page-wrapper)/insights/page.tsxpackages/features/insights/server/trpc-router.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
apps/web/app/(use-page-wrapper)/insights/routing/page.tsxpackages/features/insights/server/hasInsightsPermission.tsapps/web/app/(use-page-wrapper)/insights/router-position/page.tsxapps/web/app/(use-page-wrapper)/insights/layout.tsxpackages/features/insights/server/routing-events.tsapps/web/app/(use-page-wrapper)/insights/checkInsightsPagePermission.tsapps/web/app/(use-page-wrapper)/insights/page.tsxpackages/features/insights/server/trpc-router.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/features/insights/server/hasInsightsPermission.tspackages/features/insights/server/routing-events.tsapps/web/app/(use-page-wrapper)/insights/checkInsightsPagePermission.tspackages/features/insights/server/trpc-router.ts
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Check for E2E label
🔇 Additional comments (7)
packages/features/insights/server/routing-events.ts (1)
1-1: Type-only Prisma import: LGTM.
This removes an unnecessary runtime dependency and keeps typings intact.apps/web/app/(use-page-wrapper)/insights/layout.tsx (1)
12-20: Layout changes: LGTM.
Uses t() for strings and keeps the shell structure simple.packages/features/insights/server/hasInsightsPermission.ts (1)
12-20: Org-scope permission includes MEMBER—verify product intent.
Allowing MEMBER at org scope returns true here; org-level queries are later limited by data filtering, but please confirm this aligns with the requirement that members should not see org-wide aggregates unless explicitly permitted.Would you like me to adjust this to OWNER/ADMIN at org scope and keep MEMBER for team scope only?
packages/features/insights/server/trpc-router.ts (2)
458-480: eventTrends date range setup: LGTM.
TimeView and dateRanges derived from input plus weekStart; guarded error handling.
862-885: Endpoint guards and inputs: LGTM.
Applies insightsPbacProcedure consistently; inputs trimmed; selections delegated to services.Also applies to: 886-903, 914-933, 982-999, 1000-1013, 1014-1024
apps/web/app/(use-page-wrapper)/insights/routing/page.tsx (1)
5-5: PBAC page gate added — LGTM.Importing and using the centralized gate here keeps page-level access consistent.
apps/web/app/(use-page-wrapper)/insights/router-position/page.tsx (1)
5-5: PBAC page gate imported — LGTM.Matches the pattern used across Insights pages.
To match the theme of the rest of the app - i think removing the navigation item is probably better. We do this when a user doesnt have access to routing forms, billing etc |
E2E results are ready! |
sounds good. i'll do it in a separate PR |
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/insights/server/trpc-router.ts (1)
26-31: Membership gate ignores selectedTeamId; users can pass with membership in any team
userBelongsToTeamProcedureparses onlyteamId. Many routes useselectedTeamId; in those cases, this middleware will only check “any accepted membership,” not membership to the specificselectedTeamId. Together with the PBAC issue above, this enables cross-team access.Fix: accept
selectedTeamIdand normalize to a singlerequestedTeamIdfor membership checks and org-child validation.Apply this diff:
-const UserBelongsToTeamInput = z.object({ - teamId: z.coerce.number().optional().nullable(), - isAll: z.boolean().optional(), -}); +const UserBelongsToTeamInput = z.object({ + teamId: z.coerce.number().optional().nullable(), + selectedTeamId: z.coerce.number().optional().nullable(), + isAll: z.boolean().optional(), +}); @@ - if (parse.data.teamId) { - membershipWhereConditional["teamId"] = parse.data.teamId; - } + const requestedTeamId = parse.data.teamId ?? parse.data.selectedTeamId ?? null; + if (requestedTeamId) { + membershipWhereConditional["teamId"] = requestedTeamId; + } @@ - if ((parse.data.isAll && ctx.user.organizationId) || (!membership && ctx.user.organizationId)) { + if ((parse.data.isAll && ctx.user.organizationId) || (!membership && ctx.user.organizationId)) { @@ - if (!membership && ctx.user.organizationId && parse.data.teamId) { + if (!membership && ctx.user.organizationId && requestedTeamId) { const isChildTeamOfOrg = await ctx.insightsDb.team.findFirst({ where: { - id: parse.data.teamId, + id: requestedTeamId, parentId: ctx.user.organizationId, }, });Also applies to: 182-190, 199-211
🧹 Nitpick comments (4)
apps/web/app/(use-page-wrapper)/insights/page.tsx (1)
3-3: Prefer readonly Prisma for reads.For read-only queries, use
readonlyPrismato align with replica usage across the codebase.Apply:
-import prisma from "@calcom/prisma"; +import { readonlyPrisma as prisma } from "@calcom/prisma";packages/features/insights/server/hasInsightsPermission.ts (2)
21-31: Optimize: avoid N permission checks per user.You can use the PBAC repository’s batch helper to cut DB round-trips.
Example change:
- const teamIds = await MembershipRepository.findUserTeamIds({ userId }); - for (const teamId of teamIds) { - const hasPermission = await permissionCheckService.checkPermission({ userId, teamId, permission: "insights.read", fallbackRoles: [MembershipRole.OWNER, MembershipRole.ADMIN, MembershipRole.MEMBER] }); - if (hasPermission) return true; - } + const permitted = await permissionCheckService.getTeamIdsWithPermission(userId, "insights.read"); + return permitted.length > 0;
9-11: Type consistency: unify organizationId type across callers.This file accepts
number | null | undefinedwhile some callers usenumber | undefined. Unify to a single union to reduce unnecessary narrowing.packages/features/insights/server/trpc-router.ts (1)
458-480: Type-safety and error diagnostics: cast weekStart and keep error causeMinor tightening:
- Cast
ctx.user.weekStartto the expected union to avoid TS friction.- Preserve the original error as
causewhen throwingTRPCErrorto aid server-side diagnostics without leaking to clients.- weekStart: ctx.user.weekStart, + weekStart: ctx.user.weekStart as GetDateRangesParams["weekStart"], @@ - } catch (e) { - throw new TRPCError({ code: "INTERNAL_SERVER_ERROR" }); + } catch (e) { + throw new TRPCError({ code: "INTERNAL_SERVER_ERROR", cause: e as Error }); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
apps/web/app/(use-page-wrapper)/insights/checkInsightsPagePermission.ts(1 hunks)apps/web/app/(use-page-wrapper)/insights/layout.tsx(1 hunks)apps/web/app/(use-page-wrapper)/insights/page.tsx(2 hunks)apps/web/app/(use-page-wrapper)/insights/router-position/page.tsx(2 hunks)apps/web/app/(use-page-wrapper)/insights/routing/page.tsx(2 hunks)packages/features/insights/server/hasInsightsPermission.ts(1 hunks)packages/features/insights/server/routing-events.ts(1 hunks)packages/features/insights/server/trpc-router.ts(24 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
apps/web/app/(use-page-wrapper)/insights/routing/page.tsxapps/web/app/(use-page-wrapper)/insights/router-position/page.tsxapps/web/app/(use-page-wrapper)/insights/layout.tsxapps/web/app/(use-page-wrapper)/insights/page.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
apps/web/app/(use-page-wrapper)/insights/routing/page.tsxapps/web/app/(use-page-wrapper)/insights/checkInsightsPagePermission.tsapps/web/app/(use-page-wrapper)/insights/router-position/page.tsxpackages/features/insights/server/hasInsightsPermission.tsapps/web/app/(use-page-wrapper)/insights/layout.tsxpackages/features/insights/server/trpc-router.tsapps/web/app/(use-page-wrapper)/insights/page.tsxpackages/features/insights/server/routing-events.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
apps/web/app/(use-page-wrapper)/insights/routing/page.tsxapps/web/app/(use-page-wrapper)/insights/checkInsightsPagePermission.tsapps/web/app/(use-page-wrapper)/insights/router-position/page.tsxpackages/features/insights/server/hasInsightsPermission.tsapps/web/app/(use-page-wrapper)/insights/layout.tsxpackages/features/insights/server/trpc-router.tsapps/web/app/(use-page-wrapper)/insights/page.tsxpackages/features/insights/server/routing-events.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
apps/web/app/(use-page-wrapper)/insights/checkInsightsPagePermission.tspackages/features/insights/server/hasInsightsPermission.tspackages/features/insights/server/trpc-router.tspackages/features/insights/server/routing-events.ts
🧠 Learnings (7)
📓 Common learnings
Learnt from: SinghaAnirban005
PR: calcom/cal.com#23343
File: packages/features/insights/server/trpc-router.ts:1080-1101
Timestamp: 2025-08-26T08:08:23.395Z
Learning: In packages/features/insights/server/trpc-router.ts, when filtering personal event types (userId provided, no teamId, not isAll), the query correctly uses user.id (authenticated user) instead of the input userId parameter for security reasons. This prevents users from accessing other users' personal event types by passing arbitrary user IDs.
📚 Learning: 2025-08-26T08:08:23.395Z
Learnt from: SinghaAnirban005
PR: calcom/cal.com#23343
File: packages/features/insights/server/trpc-router.ts:1080-1101
Timestamp: 2025-08-26T08:08:23.395Z
Learning: In packages/features/insights/server/trpc-router.ts, when filtering personal event types (userId provided, no teamId, not isAll), the query correctly uses user.id (authenticated user) instead of the input userId parameter for security reasons. This prevents users from accessing other users' personal event types by passing arbitrary user IDs.
Applied to files:
packages/features/insights/server/trpc-router.tspackages/features/insights/server/routing-events.ts
📚 Learning: 2025-08-17T22:00:16.329Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22995
File: packages/trpc/server/routers/viewer/aiVoiceAgent/_router.ts:117-126
Timestamp: 2025-08-17T22:00:16.329Z
Learning: In calcom/cal.com PR #22995, packages/trpc/server/routers/viewer/aiVoiceAgent/_router.ts, the enabled input parameter in the update endpoint is intentionally not forwarded to aiService.updateAgentConfiguration() as the enabled/disabled agent functionality is not required at the moment (per maintainer Udit-takkar). Future reviews should not flag this as missing functionality unless requirements change.
Applied to files:
packages/features/insights/server/trpc-router.ts
📚 Learning: 2025-07-15T12:59:34.389Z
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.389Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
Applied to files:
packages/features/insights/server/trpc-router.ts
📚 Learning: 2025-09-09T03:29:42.984Z
Learnt from: emrysal
PR: calcom/cal.com#23692
File: packages/lib/server/service/InsightsBookingBaseService.ts:16-16
Timestamp: 2025-09-09T03:29:42.984Z
Learning: In the Cal.com codebase, readonlyPrisma is still an instance of PrismaClient, making type changes from `typeof readonlyPrisma` to `PrismaClient` less critical since they are fundamentally compatible types.
Applied to files:
packages/features/insights/server/routing-events.ts
📚 Learning: 2025-08-27T12:15:43.830Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22995
File: packages/trpc/server/routers/viewer/aiVoiceAgent/testCall.handler.ts:41-44
Timestamp: 2025-08-27T12:15:43.830Z
Learning: In calcom/cal.com, the AgentService.getAgent() method in packages/features/calAIPhone/providers/retellAI/services/AgentService.ts does NOT include authorization checks - it only validates the agentId parameter and directly calls the repository without verifying user/team access. This contrasts with other methods like getAgentWithDetails() which properly use findByIdWithUserAccessAndDetails() for authorization. When reviewing updateToolsFromAgentId() calls, always verify both agent ownership and eventType ownership are checked.
Applied to files:
packages/features/insights/server/routing-events.ts
📚 Learning: 2025-07-28T11:50:23.946Z
Learnt from: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-28T11:50:23.946Z
Learning: Applies to **/*.ts : For Prisma queries, only select data you need; never use `include`, always use `select`
Applied to files:
packages/features/insights/server/routing-events.ts
🧬 Code graph analysis (7)
apps/web/app/(use-page-wrapper)/insights/routing/page.tsx (1)
apps/web/app/(use-page-wrapper)/insights/checkInsightsPagePermission.ts (1)
checkInsightsPagePermission(11-34)
apps/web/app/(use-page-wrapper)/insights/checkInsightsPagePermission.ts (3)
packages/features/auth/lib/getServerSession.ts (1)
getServerSession(32-136)apps/web/lib/buildLegacyCtx.ts (1)
buildLegacyRequest(47-49)packages/features/insights/server/hasInsightsPermission.ts (1)
hasInsightsPermission(5-39)
apps/web/app/(use-page-wrapper)/insights/router-position/page.tsx (1)
apps/web/app/(use-page-wrapper)/insights/checkInsightsPagePermission.ts (1)
checkInsightsPagePermission(11-34)
packages/features/insights/server/hasInsightsPermission.ts (2)
packages/features/pbac/services/permission-check.service.ts (2)
PermissionCheckService(21-366)hasPermission(221-239)packages/platform/libraries/index.ts (1)
MembershipRole(34-34)
apps/web/app/(use-page-wrapper)/insights/layout.tsx (2)
packages/features/shell/Shell.tsx (1)
Shell(108-121)packages/features/data-table/lib/utils.ts (1)
CTA_CONTAINER_CLASS_NAME(168-168)
packages/features/insights/server/trpc-router.ts (4)
packages/features/pbac/services/permission-check.service.ts (1)
hasPermission(221-239)packages/features/insights/server/hasInsightsPermission.ts (1)
hasInsightsPermission(5-39)packages/features/insights/server/raw-data.schema.ts (1)
bookingRepositoryBaseInputSchema(103-110)packages/features/insights/server/insightsDateUtils.ts (2)
getTimeView(20-31)getDateRanges(33-117)
apps/web/app/(use-page-wrapper)/insights/page.tsx (2)
packages/features/auth/lib/next-auth-options.ts (1)
session(716-741)apps/web/app/(use-page-wrapper)/insights/checkInsightsPagePermission.ts (1)
checkInsightsPagePermission(11-34)
🔇 Additional comments (14)
packages/features/insights/server/routing-events.ts (1)
1-1: Type-only Prisma import — good cleanup.Using
import typeavoids unnecessary runtime bundle weight. No concerns.apps/web/app/(use-page-wrapper)/insights/layout.tsx (1)
12-20: All Insight pages include permission guard
Verified that everyapps/web/app/(use-page-wrapper)/insights/**/page.tsximports and callscheckInsightsPagePermission().apps/web/app/(use-page-wrapper)/insights/router-position/page.tsx (1)
5-6: Page-level guard correctly applied.Import + await of
checkInsightsPagePermission()before rendering looks good.Also applies to: 17-18
apps/web/app/(use-page-wrapper)/insights/routing/page.tsx (1)
5-6: Page-level guard correctly applied.Same pattern as other Insights pages; approved.
Also applies to: 17-18
apps/web/app/(use-page-wrapper)/insights/page.tsx (1)
7-8: Good: centralized permission check; minimal Prisma select.Using the helper to get the session and selecting only
timeZonealigns with data-minimization guidelines.Also applies to: 19-19
packages/features/insights/server/hasInsightsPermission.ts (2)
12-19: Org-scoped check with MEMBER fallback — aligns with PBAC goals.Including
MEMBERinfallbackRolesmatches the PR intent to broaden access appropriately.
21-36: Align insights permission with active org context
hasInsightsPermission(packages/features/insights/server/hasInsightsPermission.ts:21-36) currently checksinsights.readacross all teams returned byfindUserTeamIds({ userId }), ignoring theorganizationIdpassed in fromcheckInsightsPagePermission.ts. If access should be scoped to the active org, updatefindUserTeamIdsto filter by thatorganizationId; otherwise confirm that the current “any-team grants access” behavior is intended.apps/web/app/(use-page-wrapper)/insights/checkInsightsPagePermission.ts (3)
12-18: Global feature flag check — OK.Redirecting when “insights” is globally disabled is fine; PBAC enablement is handled inside
PermissionCheckService.
19-23: Auth redirect path is correct for unauthenticated users.Using
redirect("/auth/login")before permission checks prevents unnecessary work.
24-34: All Insights tRPC endpoints use insightsPbacProcedure—no API-level PBAC gaps detected.packages/features/insights/server/trpc-router.ts (4)
532-534: Good defensive guard when reading from MapAvoids undefined access before mutation. Looks good.
834-843: Good: userId is taken from session, not from inputPrevents user-id spoofing for routing filters/options. Matches prior security learning.
Also applies to: 895-901
321-336: Confirm intended orgId semantics when user has no org
createInsightsBookingServicepassesorgId: ctx.user.organizationId ?? 0. If downstream treats 0 as a real org/team id, scoping could be off. Prefernull | undefinedover sentinel 0 unless required by the service.If safe, change to:
- orgId: ctx.user.organizationId ?? 0, + orgId: ctx.user.organizationId ?? null,Please confirm
getInsightsBookingServiceoption typing.
757-782: Verify PBAC wrapping for remaining authedProcedure endpoints
Three insights routes still useauthedProcedure:teamListForUser,userList, andgetUserRelevantTeamRoutingForms. Confirm whether they should be converted toinsightsPbacProcedure(enforcing theinsights.readpermission) or are intentionally left unguarded for pre-filter data population.
| const insightsPbacProcedure = userBelongsToTeamProcedure.use(async ({ ctx, next }) => { | ||
| const hasPermission = await hasInsightsPermission({ | ||
| userId: ctx.user.id, | ||
| organizationId: ctx.user.organizationId, | ||
| }); | ||
|
|
||
| if (!hasPermission) { | ||
| throw new TRPCError({ code: "UNAUTHORIZED" }); | ||
| } | ||
|
|
||
| return next({ | ||
| ctx, | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Scope permission checks to the requested org/team; current PBAC gate allows cross-team escalation
insightsPbacProcedure calls hasInsightsPermission without scoping to the requested team/org from the input. Combined with the preceding middleware, a user who:
- belongs to Team A (has insights.read there), and
- also belongs to Team B (no insights.read),
can pass this PBAC gate while queryingselectedTeamId = Team B, sincehasInsightsPermissionreturns true if the user has permission in any team. This breaks cross-team isolation.
Fix: infer the target scope from raw input (scope/isAll, teamId, selectedTeamId) and check permission on that specific org/team. Fall back to “any team” only for personal scope.
Apply this diff to scope the check (requires the companion change to hasInsightsPermission shown below):
-const insightsPbacProcedure = userBelongsToTeamProcedure.use(async ({ ctx, next }) => {
- const hasPermission = await hasInsightsPermission({
- userId: ctx.user.id,
- organizationId: ctx.user.organizationId,
- });
+const insightsPbacProcedure = userBelongsToTeamProcedure.use(async ({ ctx, next, getRawInput }) => {
+ const raw = (await getRawInput()) as Record<string, unknown> | undefined;
+ const scope = (raw?.scope as string | undefined) ?? (raw?.isAll ? "org" : undefined);
+ const selectedTeamId =
+ (raw?.selectedTeamId as number | undefined) ?? (raw?.teamId as number | undefined) ?? null;
+
+ const hasPermission = await hasInsightsPermission({
+ userId: ctx.user.id,
+ // org-level (isAll or explicit org scope)
+ organizationId: scope === "org" ? ctx.user.organizationId : null,
+ // team-level (explicit team selection)
+ teamId: scope !== "org" ? selectedTeamId ?? null : null,
+ });Companion change needed in packages/features/insights/server/hasInsightsPermission.ts:
-export async function hasInsightsPermission({
- userId,
- organizationId,
-}: {
- userId: number;
- organizationId: number | null | undefined;
-}) {
+export async function hasInsightsPermission({
+ userId,
+ organizationId,
+ teamId,
+}: {
+ userId: number;
+ organizationId?: number | null;
+ teamId?: number | null;
+}) {
+ const permissionCheckService = new PermissionCheckService();
+
+ if (teamId) {
+ return permissionCheckService.checkPermission({
+ userId,
+ teamId,
+ permission: "insights.read",
+ fallbackRoles: [MembershipRole.OWNER, MembershipRole.ADMIN, MembershipRole.MEMBER],
+ });
+ }
+
if (organizationId) {
- const permissionCheckService = new PermissionCheckService();
return await permissionCheckService.checkPermission({
userId,
teamId: organizationId,
permission: "insights.read",
fallbackRoles: [MembershipRole.OWNER, MembershipRole.ADMIN, MembershipRole.MEMBER], // even members can see their own data
});
} else {
- const permissionCheckService = new PermissionCheckService();
const teamIds = await MembershipRepository.findUserTeamIds({ userId });
for (const teamId of teamIds) {
const hasPermission = await permissionCheckService.checkPermission({
userId,
teamId,
permission: "insights.read",
fallbackRoles: [MembershipRole.OWNER, MembershipRole.ADMIN, MembershipRole.MEMBER], // even members can see their own data
});
if (hasPermission) {
return true;
}
}
}
return false;
}
sean-brydon
left a comment
sean-brydon
left a comment
There was a problem hiding this comment.
Woah nice! Happy with this - tested lots of flows seems to work perfectly! Lets address the navigation item in a follow up PR as it does make things here a little bit more complex!
3 participants
What does this PR do?
This PR implements comprehensive Permission-Based Access Control (PBAC) for the insights feature, replacing the previous simple role-based checks with a more granular permission system using the
insights.readpermission.Key Changes:
checkInsightsPagePermission.tsutility used across all insights pageshasInsightsPermission.tssupports both organization-based and team-based permission checkinginsightsPbacProcedureArchitecture:
/insights,/insights/routing,/insights/router-position) now callcheckInsightsPagePermission()insightsPbacProcedurewithhasInsightsPermission()validationHow should this be tested?
Environment Setup:
Test Scenarios:
insights.readpermission can access insightsExpected Behavior:
Mandatory Tasks
Security & Permissions:
hasInsightsPermission.ts- Verify the loop for non-org teams doesn't allow unauthorized accessorganizationIdtype mismatch (number | null vs number | undefined) in trpc-router.tsImplementation:
insightsPbacProcedure(no missed endpoints)Link to Devin run: https://app.devin.ai/sessions/a126a44cf2dc4bd08cbba117b2f2bc7f
Requested by: @eunjae-lee
This PR recreates the functionality from stale PR #22324 using the latest codebase and follows the PBAC patterns established in PR #22618 (Event Type PBAC).