Add panel permissions and guards, update authorization logic, and enhance tests#1713
Merged
Add panel permissions and guards, update authorization logic, and enhance tests#1713
Conversation
…ance tests - Introduced new panel-related actions in CedarAction and CedarResourceType enums. - Updated CedarValidationRequest to include panelId for validation. - Enhanced CedarAuthorizationService to handle panel permissions. - Modified buildCedarEntities to include panel entities. - Updated policy generation and parsing to support panel permissions. - Created PanelEditGuard and PanelReadGuard for panel access control. - Updated panel controller to use new guards for CRUD operations. - Enhanced FindAllPanels use case to check permissions before returning results. - Added comprehensive end-to-end tests for panel permissions, ensuring correct access control.
📝 WalkthroughWalkthroughAdds Cedar-based authorization support for panel (saved query) resources by introducing panel-specific actions, guards, and authorization validation throughout the backend authorization flow, enabling granular permission control at the panel level alongside existing connection and dashboard scoping. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends the Cedar-based authorization model to cover Panels (Saved Queries), wiring new panel-specific guards into the panel endpoints and updating policy/schema tooling so panel permissions can be generated, parsed, and enforced. It also tightens dashboard widget authorization by using dashboard-level guards and adds E2E coverage for the new panel permission behaviors.
Changes:
- Add Cedar schema/actions/entities support for
panel:*permissions and extend policy generator/parser accordingly. - Introduce
PanelReadGuard/PanelEditGuardand apply them to saved-query endpoints; filter panel listings by per-panel permissions. - Expand SaaS E2E tests to verify list/read/create behaviors under panel permission policies.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| backend/test/ava-tests/saas-tests/saas-dashboard-permissions-e2e.test.ts | Adds E2E coverage for panel (saved query) permissions (list/read/create cases). |
| backend/src/guards/panel-read.guard.ts | New guard enforcing panel:read for single-panel reads (and connection membership for list). |
| backend/src/guards/panel-edit.guard.ts | New guard enforcing panel:create/panel:edit/panel:delete based on HTTP method and presence of panel id. |
| backend/src/entities/visualizations/panel/use-cases/find-all-panels.use.case.ts | Filters returned panels by Cedar panel:read authorization. |
| backend/src/entities/visualizations/panel/panel.controller.ts | Switches saved-query endpoints to panel-specific guards and passes userId into list use case. |
| backend/src/entities/visualizations/panel/data-structures/find-all-panels.ds.ts | Adds userId to support per-user authorization filtering in list use case. |
| backend/src/entities/visualizations/panel-position/panel-position.controller.ts | Uses DashboardEditGuard for widget create/update/delete to enforce dashboard-level permissions. |
| backend/src/entities/permission/permission.interface.ts | Adds panel permissions shape to the classical permissions model. |
| backend/src/entities/cedar-authorization/cedar-schema.ts | Extends Cedar schema with Panel entity type and panel:* actions. |
| backend/src/entities/cedar-authorization/cedar-policy-parser.ts | Parses panel:* permits into classical panels permission data. |
| backend/src/entities/cedar-authorization/cedar-policy-generator.ts | Generates Cedar permits for panel:* permissions, including __new__ sentinel behavior. |
| backend/src/entities/cedar-authorization/cedar-entity-builder.ts | Adds Cedar entity construction for Panel resources. |
| backend/src/entities/cedar-authorization/cedar-authorization.service.ts | Adds panel-aware validation routing and foreign-connection reference checks for panel resources. |
| backend/src/entities/cedar-authorization/cedar-action-map.ts | Adds Panel* actions/resource type and validation request support for panelId. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+40
to
+44
| const accessChecks = await Promise.all( | ||
| foundQueries.map((query) => | ||
| this.cedarAuthService.validate({ | ||
| userId, | ||
| action: CedarAction.PanelRead, |
There was a problem hiding this comment.
Promise.all(foundQueries.map(... cedarAuthService.validate ...)) triggers one full Cedar validation per panel. Each validate() call hits the DB (user suspension + group lookup) and evaluates policies, so listing panels becomes O(N) DB work and can also fan out many concurrent queries at once. Consider adding a CedarAuthorizationService helper that loads user groups/policies once per request and evaluates multiple panelIds using the same precomputed data (or at least limit concurrency), then filter based on those results.
![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
🧹 Nitpick comments (1)
backend/src/entities/cedar-authorization/cedar-policy-parser.ts (1)
286-330: Preferconstarrow helpers for the new panel parsing utilities.These helpers were introduced as function declarations, while the repo rule for new TS/JS helpers prefers arrow functions. As per coding guidelines, "Prefer arrow functions over function declarations".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/entities/cedar-authorization/cedar-policy-parser.ts` around lines 286 - 330, Replace the three function declarations with const arrow helpers: convert function extractPanelId(resourceId: string | null, connectionId: string): string | null { ... } into const extractPanelId = (resourceId: string | null, connectionId: string): string | null => { ... }, convert getOrCreatePanelEntry(map: Map<string, IPanelPermissionData>, panelId: string): IPanelPermissionData { ... } into const getOrCreatePanelEntry = (map: Map<string, IPanelPermissionData>, panelId: string): IPanelPermissionData => { ... }, and convert applyPanelAction(entry: IPanelPermissionData, action: string): void { ... } into const applyPanelAction = (entry: IPanelPermissionData, action: string): void => { ... }; keep all parameter and return types, body logic, and existing Map/get/set behavior unchanged and ensure you use const for each helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/entities/cedar-authorization/cedar-policy-generator.ts`:
- Around line 117-121: Remove the code that emits a permit for
RocketAdmin::Action::"panel:read" against the __new__ sentinel (newPanelRef); do
not grant panel:read for `RocketAdmin::Panel::"${connectionId}/__new__"`.
Instead, if you need to allow operations on the sentinel, emit only a permit for
RocketAdmin::Action::"panel:create" against newPanelRef (or rely on the existing
create-permission variable/logic), and keep panel:read permits scoped only to
concrete panel IDs (leave the hasPanelReadPermission handling for real panel
resources untouched).
In
`@backend/src/entities/visualizations/panel/use-cases/find-all-panels.use.case.ts`:
- Around line 40-51: The current code calls this.cedarAuthService.validate for
each item in foundQueries causing N DB/policy loads; instead, perform a single
per-request authorization step and then filter locally: either add/use a batch
authorization method on cedarAuthService (e.g.,
validateBatch/authorizePanelsForUser) that accepts userId, connectionId, action
= CedarAction.PanelRead and an array of panelIds and returns a Set/Map of
allowed panelIds, or create a single request auth context (e.g.,
cedarAuthService.createAuthContext(userId, connectionId)) that preloads
assertUserNotSuspended, user groups, and policies and exposes an
isAuthorized(panelId) check; then replace the Promise.all loop over foundQueries
with one batch call and filter foundQueries by the returned allowed IDs before
mapping to buildFoundPanelDto(query).
In `@backend/test/ava-tests/saas-tests/saas-dashboard-permissions-e2e.test.ts`:
- Around line 284-287: The test descriptions use the shared variable currentTest
which is still set to "Dashboard permissions", so update the currentTest
assignment to a panel-specific label (e.g., "Panel permissions" or "Panel (Saved
Query) permissions") before the PANEL (SAVED QUERY) PERMISSIONS block so
test.serial `${currentTest} should allow listing panels...` reflects the correct
context; locate and change the currentTest variable used by the panel tests (the
currentTest identifier referenced in the PANEL (SAVED QUERY) PERMISSIONS section
and surrounding tests) to the new string.
---
Nitpick comments:
In `@backend/src/entities/cedar-authorization/cedar-policy-parser.ts`:
- Around line 286-330: Replace the three function declarations with const arrow
helpers: convert function extractPanelId(resourceId: string | null,
connectionId: string): string | null { ... } into const extractPanelId =
(resourceId: string | null, connectionId: string): string | null => { ... },
convert getOrCreatePanelEntry(map: Map<string, IPanelPermissionData>, panelId:
string): IPanelPermissionData { ... } into const getOrCreatePanelEntry = (map:
Map<string, IPanelPermissionData>, panelId: string): IPanelPermissionData => {
... }, and convert applyPanelAction(entry: IPanelPermissionData, action:
string): void { ... } into const applyPanelAction = (entry:
IPanelPermissionData, action: string): void => { ... }; keep all parameter and
return types, body logic, and existing Map/get/set behavior unchanged and ensure
you use const for each helper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b88ee545-6679-4d31-be5e-c1c483b8f377
📒 Files selected for processing (14)
backend/src/entities/cedar-authorization/cedar-action-map.tsbackend/src/entities/cedar-authorization/cedar-authorization.service.tsbackend/src/entities/cedar-authorization/cedar-entity-builder.tsbackend/src/entities/cedar-authorization/cedar-policy-generator.tsbackend/src/entities/cedar-authorization/cedar-policy-parser.tsbackend/src/entities/cedar-authorization/cedar-schema.tsbackend/src/entities/permission/permission.interface.tsbackend/src/entities/visualizations/panel-position/panel-position.controller.tsbackend/src/entities/visualizations/panel/data-structures/find-all-panels.ds.tsbackend/src/entities/visualizations/panel/panel.controller.tsbackend/src/entities/visualizations/panel/use-cases/find-all-panels.use.case.tsbackend/src/guards/panel-edit.guard.tsbackend/src/guards/panel-read.guard.tsbackend/test/ava-tests/saas-tests/saas-dashboard-permissions-e2e.test.ts
Comment on lines
+117
to
+121
| const newPanelRef = `RocketAdmin::Panel::"${connectionId}/__new__"`; | ||
| if (hasPanelReadPermission) { | ||
| policies.push( | ||
| `permit(\n principal,\n action == RocketAdmin::Action::"panel:read",\n resource == ${newPanelRef}\n);`, | ||
| ); |
There was a problem hiding this comment.
Do not grant panel:read on the __new__ sentinel.
cedar-authorization.service.ts falls back to __new__ when a panel read is evaluated without a panelId. Emitting a read permit for that sentinel means a missing panelId can succeed against __new__ instead of the real panel. __new__ only needs panel:create; keep panel:read scoped to concrete panel IDs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/entities/cedar-authorization/cedar-policy-generator.ts` around
lines 117 - 121, Remove the code that emits a permit for
RocketAdmin::Action::"panel:read" against the __new__ sentinel (newPanelRef); do
not grant panel:read for `RocketAdmin::Panel::"${connectionId}/__new__"`.
Instead, if you need to allow operations on the sentinel, emit only a permit for
RocketAdmin::Action::"panel:create" against newPanelRef (or rely on the existing
create-permission variable/logic), and keep panel:read permits scoped only to
concrete panel IDs (leave the hasPanelReadPermission handling for real panel
resources untouched).
Comment on lines
+40
to
+51
| const accessChecks = await Promise.all( | ||
| foundQueries.map((query) => | ||
| this.cedarAuthService.validate({ | ||
| userId, | ||
| action: CedarAction.PanelRead, | ||
| connectionId, | ||
| panelId: query.id, | ||
| }), | ||
| ), | ||
| ); | ||
|
|
||
| return foundQueries.filter((_, index) => accessChecks[index]).map((query) => buildFoundPanelDto(query)); |
There was a problem hiding this comment.
Avoid per-panel validate() calls in the list path.
This loop turns one listing request into N authorization evaluations, and validate() is doing DB work (assertUserNotSuspended, findAllUserGroupsInConnection, policy loading) on each iteration. With a few hundred panels this endpoint will degrade sharply. Please batch this or reuse a single per-request authorization context before filtering.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@backend/src/entities/visualizations/panel/use-cases/find-all-panels.use.case.ts`
around lines 40 - 51, The current code calls this.cedarAuthService.validate for
each item in foundQueries causing N DB/policy loads; instead, perform a single
per-request authorization step and then filter locally: either add/use a batch
authorization method on cedarAuthService (e.g.,
validateBatch/authorizePanelsForUser) that accepts userId, connectionId, action
= CedarAction.PanelRead and an array of panelIds and returns a Set/Map of
allowed panelIds, or create a single request auth context (e.g.,
cedarAuthService.createAuthContext(userId, connectionId)) that preloads
assertUserNotSuspended, user groups, and policies and exposes an
isAuthorized(panelId) check; then replace the Promise.all loop over foundQueries
with one batch call and filter foundQueries by the returned allowed IDs before
mapping to buildFoundPanelDto(query).
Comment on lines
+284
to
+287
| //****************************** PANEL (SAVED QUERY) PERMISSIONS ****************************** | ||
|
|
||
| test.serial( | ||
| `${currentTest} should allow listing panels when user has read access to a specific panel via cedar policy`, |
There was a problem hiding this comment.
Update currentTest variable for panel tests.
The currentTest variable is still set to 'Dashboard permissions' from line 60, causing all panel test descriptions to incorrectly display as "Dashboard permissions should allow listing panels..." instead of the expected panel-specific context.
🐛 Proposed fix
//****************************** PANEL (SAVED QUERY) PERMISSIONS ******************************
+currentTest = 'Panel permissions';
+
test.serial(
`${currentTest} should allow listing panels when user has read access to a specific panel via cedar policy`,📝 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.
Suggested change
| //****************************** PANEL (SAVED QUERY) PERMISSIONS ****************************** | |
| test.serial( | |
| `${currentTest} should allow listing panels when user has read access to a specific panel via cedar policy`, | |
| //****************************** PANEL (SAVED QUERY) PERMISSIONS ****************************** | |
| currentTest = 'Panel permissions'; | |
| test.serial( | |
| `${currentTest} should allow listing panels when user has read access to a specific panel via cedar policy`, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/test/ava-tests/saas-tests/saas-dashboard-permissions-e2e.test.ts`
around lines 284 - 287, The test descriptions use the shared variable
currentTest which is still set to "Dashboard permissions", so update the
currentTest assignment to a panel-specific label (e.g., "Panel permissions" or
"Panel (Saved Query) permissions") before the PANEL (SAVED QUERY) PERMISSIONS
block so test.serial `${currentTest} should allow listing panels...` reflects
the correct context; locate and change the currentTest variable used by the
panel tests (the currentTest identifier referenced in the PANEL (SAVED QUERY)
PERMISSIONS section and surrounding tests) to the new string.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Security