fix: Add PBAC permission checks for insights access #25381
Conversation
- Add checkInsightsPermission() helper that properly checks insights.read permission with PBAC support - Update userBelongsToTeamProcedure to use PBAC-aware permission check for org-level access - Update teamListForUser query to filter teams based on insights.read permission instead of only checking base ADMIN/OWNER roles - Maintain backward compatibility with fallback to traditional role checks (ADMIN/OWNER) when PBAC is disabled - Org admins (base role ADMIN/OWNER) continue to have automatic insights access as a privileged position - Team-level admins with custom roles now properly checked for insights.read permission Fixes issue where users with custom PBAC roles couldn't access insights even if they had insights.read permission. Related: CAL-XXXX
|
@dhairyashiil is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
keithwillcode
added
the
community-interns
| // Filter teams to only include those where the user has insights.read permission | ||
| // This properly handles both PBAC permissions and traditional role-based access | ||
| const teamsWithAccess = await Promise.all( | ||
| belongsToTeams.map(async (membership) => { | ||
| const hasAccess = await checkInsightsPermission(user.id, membership.team.id); | ||
| return hasAccess ? membership.team : null; | ||
| }) | ||
| ); | ||
|
|
||
| const result: IResultTeamList[] = teamsWithAccess | ||
| .filter((team): team is NonNullable<typeof team> => team !== null) |
There was a problem hiding this comment.
I think we can use getTeamIdsWithPermission method in permission-service to avoid N+1 queries here
There was a problem hiding this comment.
I have implemented the suggested changes and tested it from the UI again. it’s working. Thank you for the review 🙏🏼
Replace individual permission checks per team with bulk query using getTeamIdsWithPermission(). This reduces database queries from N (one per team) to a single optimized query. - Use PermissionCheckService.getTeamIdsWithPermission() for bulk permission checking - Filter teams based on returned team IDs instead of individual checks - Maintains same functionality with significantly better performance for users with many teams
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file (reviewed changes from recent commits).
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="packages/features/insights/server/trpc-router.ts">
<violation number="1" location="packages/features/insights/server/trpc-router.ts:687">
Bulk permission filtering no longer honors org-level fallback roles, so org admins who are members on child teams lose insights access.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| // Get all team IDs where user has insights.read permission in a single optimized query | ||
| // This avoids N+1 queries by fetching all permissions at once | ||
| const permissionCheckService = new PermissionCheckService(); | ||
| const teamIdsWithAccess = await permissionCheckService.getTeamIdsWithPermission({ |
There was a problem hiding this comment.
Bulk permission filtering no longer honors org-level fallback roles, so org admins who are members on child teams lose insights access.
Prompt for AI agents
Address the following comment on packages/features/insights/server/trpc-router.ts at line 687:
<comment>Bulk permission filtering no longer honors org-level fallback roles, so org admins who are members on child teams lose insights access.</comment>
<file context>
@@ -681,18 +681,19 @@ export const insightsRouter = router({
+ // Get all team IDs where user has insights.read permission in a single optimized query
+ // This avoids N+1 queries by fetching all permissions at once
+ const permissionCheckService = new PermissionCheckService();
+ const teamIdsWithAccess = await permissionCheckService.getTeamIdsWithPermission({
+ userId: user.id,
+ permission: "insights.read",
</file context>
✅ Addressed in 624d1ef
There was a problem hiding this comment.
I don't believe so - will double check the code
There was a problem hiding this comment.
Actually believe this to be the case - working on a fix. SQL will be pretty complex here so wll push a new PR
| // Filter to only teams where user has access | ||
| const result: IResultTeamList[] = belongsToTeams | ||
| .filter((membership) => teamIdsWithAccess.includes(membership.team.id)) | ||
| .map((membership) => ({ ...membership.team })); |
There was a problem hiding this comment.
Also when we get teamIdsWithAccess. We can simply fetch these teams only instead of all the teams and then filtering
There was a problem hiding this comment.
Yeah we should deffo just change the query to check teamIds IN this teamIdsWithAccess
There was a problem hiding this comment.
Implemented the suggested changes. Thank you both 🙏🏼
Move permission check before team query to filter at database level.
Previously fetched all teams then filtered in JavaScript.
Now only fetches teams the user has insights access to.
- Check permissions first using getTeamIdsWithPermission()
- Add teamId filter to membership query (teamId: { in: teamIdsWithAccess })
- Remove JavaScript filter step (done at DB level)
- Reduces data transfer and improves query efficiency
E2E results are ready! |
4 participants
Before:
Screen.Recording.2025-11-25.at.6.04.01.AM.mov
After:
Screen.Recording.2025-11-25.at.6.04.44.AM.mov
Problem
admin_role(which hasinsights.readpermission) were blockedrolefield (ADMIN/OWNER/MEMBER)What This Fixes
PermissionCheckServiceinsights.readpermission now workChanges Made
Created
checkInsightsPermission()helper functioninsights.readpermissionUpdated organization-level access check
Updated team list query
insights.readpermissionTesting
✅ PBAC enabled +
admin_role→ Can access insights✅ PBAC enabled + custom role with
insights.read→ Can access insights✅ PBAC enabled + custom role without
insights.read→ Cannot access insights✅ PBAC disabled + traditional ADMIN → Can access insights
✅ PBAC disabled + traditional MEMBER → Cannot access insights