fix: remove page permission check for /insights#23945
Conversation
WalkthroughRemoved the per-user Insights permission check and its module. The page-level guard now enforces only the global "insights" feature flag (redirect to "/") and authentication (redirect to "/auth/login"); it returns the session otherwise. The deleted helper was packages/features/insights/server/hasInsightsPermission.ts. In the TRPC router, all endpoints once using the PBAC-based Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/features/insights/server/trpc-router.ts (2)
29-33: Critical: input shape mismatch breaks authorization (team/org scope bypass).UserBelongsToTeamInput only accepts teamId/isAll, while most endpoints pass selectedTeamId/scope. As a result, userBelongsToTeamProcedure does not validate team membership nor org-scope, enabling unauthorized access when clients pass selectedTeamId or scope="org". Normalize both shapes.
Apply this diff to accept both forms and derive normalized fields:
-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(), + scope: z.union([z.literal("user"), z.literal("team"), z.literal("org")]).optional(), + isAll: z.boolean().optional(), + }) + .transform((data) => ({ + teamId: data.teamId ?? data.selectedTeamId ?? null, + isAll: data.isAll ?? data.scope === "org", + }));
176-240: Critical: close auth bypass in userBelongsToTeamProcedureThe middleware currently permits requests when a team id is supplied and the user is neither a member nor part of the org — and "isAll" (org-scope) is not enforced to require OWNER/ADMIN. Require membership OR org OWNER/ADMIN for team scope, and require org OWNER/ADMIN for isAll.
Apply this diff (also handle selectedTeamId callers):
@@ - const membershipWhereConditional: Prisma.MembershipWhereInput = { + const membershipWhereConditional: Prisma.MembershipWhereInput = { userId: ctx.user.id, accepted: true, }; - - if (parse.data.teamId) { - membershipWhereConditional["teamId"] = parse.data.teamId; - } + const effectiveTeamId = parse.data.teamId ?? parse.data.selectedTeamId ?? undefined; + if (effectiveTeamId) { + membershipWhereConditional.teamId = effectiveTeamId; + } @@ - // 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)) { - //Look for membership type in organizationId - if (!membership && ctx.user.organizationId && parse.data.teamId) { - const isChildTeamOfOrg = await ctx.insightsDb.team.findFirst({ - where: { - id: parse.data.teamId, - parentId: ctx.user.organizationId, - }, - }); - if (!isChildTeamOfOrg) { - throw new TRPCError({ code: "UNAUTHORIZED" }); - } - } - - const membershipOrg = await ctx.insightsDb.membership.findFirst({ - where: { - userId: ctx.user.id, - teamId: ctx.user.organizationId, - accepted: true, - role: { - in: ["OWNER", "ADMIN"], - }, - }, - }); - if (!membershipOrg) { - throw new TRPCError({ code: "UNAUTHORIZED" }); - } - isOwnerAdminOfParentTeam = true; - } + // Org-scope (isAll) always requires OWNER/ADMIN on parent org. + if (parse.data.isAll) { + if (!ctx.user.organizationId) throw new TRPCError({ code: "UNAUTHORIZED" }); + const membershipOrg = await ctx.insightsDb.membership.findFirst({ + where: { + userId: ctx.user.id, + teamId: ctx.user.organizationId, + accepted: true, + role: { in: ["OWNER", "ADMIN"] }, + }, + }); + if (!membershipOrg) throw new TRPCError({ code: "UNAUTHORIZED" }); + isOwnerAdminOfParentTeam = true; + } + + // Team-scope with a specific team requires membership OR org OWNER/ADMIN over that child team. + if (effectiveTeamId && !membership) { + if (!ctx.user.organizationId) throw new TRPCError({ code: "UNAUTHORIZED" }); + const isChildTeamOfOrg = await ctx.insightsDb.team.findFirst({ + where: { id: effectiveTeamId, parentId: ctx.user.organizationId }, + }); + if (!isChildTeamOfOrg) throw new TRPCError({ code: "UNAUTHORIZED" }); + const membershipOrg = await ctx.insightsDb.membership.findFirst({ + where: { + userId: ctx.user.id, + teamId: ctx.user.organizationId, + accepted: true, + role: { in: ["OWNER", "ADMIN"] }, + }, + }); + if (!membershipOrg) throw new TRPCError({ code: "UNAUTHORIZED" }); + isOwnerAdminOfParentTeam = true; + }Precise pointers: middleware in packages/features/insights/server/trpc-router.ts (~lines 176–240). Callsites pass selectedTeamId at ~lines 306, 312, 327, 871 — ensure selectedTeamId -> teamId mapping is respected.
🧹 Nitpick comments (3)
packages/features/insights/server/trpc-router.ts (3)
445-470: Handle missing/invalid date range as BAD_REQUEST.extractDateRangeFromColumnFilters throws if filters are absent; this happens before try/catch and returns a generic 500. Validate earlier and return a clear TRPC BAD_REQUEST.
- const { startDate, endDate } = extractDateRangeFromColumnFilters(columnFilters); + try { + const { startDate, endDate } = extractDateRangeFromColumnFilters(columnFilters); + // ... keep existing logic below + } catch { + throw new TRPCError({ code: "BAD_REQUEST", message: "Missing or invalid date range filters" }); + }
482-542: Push average computation to DB to avoid N+dayjs on large sets.Looping through allBookings and formatting with dayjs per row may be heavy. Prefer an aggregate grouped by period at the DB layer.
Would you like a follow-up patch using Prisma raw SQL/groupBy to compute averages per period?
817-837: Endpoint depends on fixed guard; add a hard cap server-side.rawData sets default limit 100, but clients can request up to 100; consider enforcing a strict upper bound server-side irrespective of input to protect memory.
- return await insightsBookingService.getCsvData({ - limit: limit ?? 100, + return await insightsBookingService.getCsvData({ + limit: Math.min(limit ?? 100, 100), offset: offset ?? 0, });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/web/app/(use-page-wrapper)/insights/checkInsightsPagePermission.ts(0 hunks)packages/features/insights/server/hasInsightsPermission.ts(0 hunks)packages/features/insights/server/trpc-router.ts(22 hunks)
💤 Files with no reviewable changes (2)
- apps/web/app/(use-page-wrapper)/insights/checkInsightsPagePermission.ts
- packages/features/insights/server/hasInsightsPermission.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/trpc-router.ts
**/*.{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:
packages/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:
packages/features/insights/server/trpc-router.ts
🧠 Learnings (4)
📓 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.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
🧬 Code graph analysis (1)
packages/features/insights/server/trpc-router.ts (3)
packages/features/insights/server/raw-data.schema.ts (2)
bookingRepositoryBaseInputSchema(103-108)insightsRoutingServiceInputSchema(69-75)packages/features/insights/lib/bookingUtils.ts (1)
extractDateRangeFromColumnFilters(5-25)packages/features/insights/server/insightsDateUtils.ts (2)
getTimeView(20-31)getDateRanges(33-117)
⏰ 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: Install dependencies / Yarn install & cache
🔇 Additional comments (24)
packages/features/insights/server/trpc-router.ts (24)
340-444: Endpoint depends on fixed guard; otherwise OK.bookingKPIStats is fine, but it passes selectedTeamId via bookingRepositoryBaseInputSchema; ensure the guard normalization above is in place.
471-481: Endpoint depends on fixed guard; otherwise OK.popularEvents uses bookingRepositoryBaseInputSchema; relies on guard to validate selectedTeamId/scope.
543-556: Endpoint depends on fixed guard; otherwise OK.membersWithMostCancelledBookings relies on guard to scope to user/team/org.
557-571: Endpoint depends on fixed guard; otherwise OK.membersWithMostCompletedBookings is fine post-guard fix.
572-586: Endpoint depends on fixed guard; otherwise OK.membersWithLeastCompletedBookings is fine post-guard fix.
587-597: Endpoint depends on fixed guard; otherwise OK.membersWithMostBookings is fine post-guard fix.
598-608: Endpoint depends on fixed guard; otherwise OK.membersWithLeastBookings is fine post-guard fix.
762-788: Good: ignores input userId for personal scope, uses authenticated user.Passing ctx.user into getEventTypeList enforces the learned pattern to prevent userId spoofing. LGTM.
788-793: Endpoint depends on fixed guard; otherwise OK.recentRatings is fine post-guard fix.
794-804: Endpoint depends on fixed guard; otherwise OK.membersWithMostNoShow is fine post-guard fix.
805-810: Endpoint depends on fixed guard; otherwise OK.membersWithHighestRatings is fine post-guard fix.
811-816: Endpoint depends on fixed guard; otherwise OK.membersWithLowestRatings is fine post-guard fix.
839-849: LGTM (with guard fix).getRoutingFormsForFilters correctly overrides userId with ctx.user.id. Relies on guard fix; otherwise fine.
850-856: Endpoint depends on fixed guard; otherwise OK.routingFormsByStatus is fine post-guard fix.
857-865: Endpoint depends on fixed guard; otherwise OK.routingFormResponses is fine post-guard fix.
866-891: Endpoint depends on fixed guard; CSV generation is OK.Ensure guard normalization first; CSV escaping looks correct.
891-907: LGTM (with guard fix).getRoutingFormFieldOptions properly forces userId from context.
908-917: Endpoint depends on fixed guard; otherwise OK.failedBookingsByField is fine post-guard fix.
918-937: LGTM (with guard fix).routingFormResponsesHeaders uses ctx.user.id/orgId; good.
938-953: Endpoint depends on fixed guard; otherwise OK.routedToPerPeriod is fine post-guard fix.
954-975: Endpoint depends on fixed guard; otherwise OK.routedToPerPeriodCsv is fine post-guard fix.
987-1004: Endpoint depends on fixed guard; otherwise OK.getRoutingFunnelData is fine post-guard fix.
1005-1018: Endpoint depends on fixed guard; otherwise OK.bookingsByHourStats is fine post-guard fix.
1019-1029: Endpoint depends on fixed guard; otherwise OK.recentNoShowGuests is fine post-guard fix.
- Add helper functions for PBAC setup in e2e fixtures - Add test verifying users with custom roles lacking insights.read can access /insights - Test validates that PR #23945 correctly removes page-level permission checks - Fix existing lint violations in users.ts fixture file Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/web/playwright/lib/test-helpers/pbac.ts (1)
5-18: Skips the internal_resourcemetadata field.The function correctly filters out the "*" resource and processes all other resources. However, it also iterates over the internal
_resourcemetadata field present in each resource config, which should be skipped.Apply this diff to skip the
_resourcemetadata field:export const createAllPermissionsArray = () => { const allPermissions: { resource: string; action: string }[] = []; Object.entries(PERMISSION_REGISTRY).forEach(([resource, resourceConfig]) => { if (resource === "*") { return; } Object.entries(resourceConfig).forEach(([action, _details]) => { + if (action === "_resource") { + return; + } allPermissions.push({ resource, action }); }); }); return allPermissions; };apps/web/playwright/insights.e2e.ts (1)
300-302: Consider using a more explicit filter condition for clarity.While the current filter correctly excludes the
insights.readpermission, consider using a more explicit condition for better readability.Apply this diff for improved clarity:
const permissions = createAllPermissionsArray().filter( - ({ resource, action }) => !(resource === "insights" && action === "read") + (permission) => permission.resource !== "insights" || permission.action !== "read" );
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/web/playwright/insights.e2e.ts(2 hunks)apps/web/playwright/lib/test-helpers/pbac.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/playwright/lib/test-helpers/pbac.tsapps/web/playwright/insights.e2e.ts
**/*.{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/playwright/lib/test-helpers/pbac.tsapps/web/playwright/insights.e2e.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/playwright/lib/test-helpers/pbac.tsapps/web/playwright/insights.e2e.ts
🧠 Learnings (1)
📓 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.
🧬 Code graph analysis (2)
apps/web/playwright/lib/test-helpers/pbac.ts (1)
packages/features/pbac/domain/types/permission-registry.ts (1)
PERMISSION_REGISTRY(154-552)
apps/web/playwright/insights.e2e.ts (2)
apps/web/playwright/lib/test-helpers/pbac.ts (2)
enablePBACForTeam(20-29)createAllPermissionsArray(5-18)packages/features/pbac/services/permission-check.service.ts (2)
PermissionCheckService(19-290)hasPermission(191-209)
⏰ 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). (3)
- GitHub Check: Tests / Unit
- GitHub Check: Type check / check-types
- GitHub Check: Linters / lint
🔇 Additional comments (2)
apps/web/playwright/lib/test-helpers/pbac.ts (1)
20-29: LGTM!The function correctly creates a PBAC feature record for the specified team.
apps/web/playwright/insights.e2e.ts (1)
279-349: Test verifies the intended behavior of allowing access without insights.read permission.The test successfully validates that users without the
insights.readpermission can still access the insights page when viewing their personal data, which aligns with the PR's objective of reverting the permission check. The test creates a robust scenario with PBAC enabled and confirms the lack of permission before verifying page access.
E2E results are ready! |
eunjae-lee
deleted the
fix/remove-page-permission-check-for-insights
branch
3 participants
What does this PR do?
We've recently added a permission check for /insights/xxx pages. And this PR reverts some of them.
It's because a normal member without access to
insights.readshould still be able to access insights page for their own personal scoped data like this↓
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Impersonate
pbac-analyticsand visit /insights. It should open.(on current
main, it gets redirected to /)