Conversation
…or dashboard access
📝 WalkthroughWalkthroughThis pull request extends dashboard operations with Cedar-based authorization enforcement. The find-all-dashboards use case now retrieves dashboards and filters them based per-dashboard Cedar authorization checks requiring Changes
Sequence DiagramssequenceDiagram
participant Client
participant UseCase as Find-All-Dashboards<br/>Use Case
participant DB as Connection<br/>Repository
participant Cedar as Cedar Auth<br/>Service
participant Mapper as Dashboard<br/>Mapper
Client->>UseCase: Request dashboards<br/>(connectionId, userId)
UseCase->>DB: Get all dashboards<br/>for connection
DB-->>UseCase: List of dashboards
loop For each dashboard
UseCase->>Cedar: Validate<br/>(DashboardRead,<br/>connectionId,<br/>dashboardId)
Cedar-->>UseCase: Authorization<br/>result
alt Authorized
UseCase->>UseCase: Include in results
else Not Authorized
UseCase->>UseCase: Exclude from results
end
end
UseCase->>Mapper: Convert to DTOs
Mapper-->>UseCase: FoundDashboardDto[]
UseCase-->>Client: Filtered dashboard list
sequenceDiagram
participant Client
participant Guard as Dashboard-Read<br/>Guard
participant DB as Connection<br/>Repository
participant Cedar as Cedar Auth<br/>Service
Client->>Guard: Request (dashboardId,<br/>connectionId)
alt dashboardId present
Guard->>Cedar: Validate<br/>(DashboardRead)
Cedar-->>Guard: Result
alt Authorized
Guard-->>Client: Allow
else Not Authorized
Guard-->>Client: ForbiddenException
end
else dashboardId missing
Guard->>DB: Is user from<br/>connection?
DB-->>Guard: Result
alt User belongs
Guard-->>Client: Allow
else User excluded
Guard-->>Client: ForbiddenException
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/test/ava-tests/saas-tests/saas-dashboard-permissions-e2e.test.ts (1)
1-1: Clarify or remove the eslint-disable comment.The
@typescript-eslint/no-unused-varsdisable comment appears to be for_testUtilson line 21, which is declared but not used in this test file. Consider either:
- Using
_testUtilsif needed for test utilities, or- Removing both the variable declaration and the disable comment if it's not required.
🤖 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` at line 1, Remove or clarify the unnecessary eslint-disable comment and the unused variable: locate the top-of-file eslint directive "/* eslint-disable `@typescript-eslint/no-unused-vars` */" and the unused symbol "_testUtils" (declared around line 21) in the saas-dashboard-permissions-e2e.test.ts file; either delete the eslint-disable line and the "_testUtils" declaration if the helper is not needed, or instead keep "_testUtils" and actually use it in the test to justify the ignore—ensure no other unused-vars disables remain and run the linter to confirm the warning is resolved.
🤖 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/test/ava-tests/saas-tests/saas-dashboard-permissions-e2e.test.ts`:
- Around line 183-229: The failing CI is due to Biome formatting issues in this
test block; run the project's formatter (e.g., npx biome format --write or the
repo's format script) and reformat the file containing the test.serial block
(the test that uses
createConnectionsAndInviteNewUserInNewGroupWithTableDifferentConnectionGroupReadOnlyPermissions,
cedarPolicy, savePolicyResponse, listDashboards) so indentation, line breaks,
and spacing match the project's style, then stage and commit the formatted file.
---
Nitpick comments:
In `@backend/test/ava-tests/saas-tests/saas-dashboard-permissions-e2e.test.ts`:
- Line 1: Remove or clarify the unnecessary eslint-disable comment and the
unused variable: locate the top-of-file eslint directive "/* eslint-disable
`@typescript-eslint/no-unused-vars` */" and the unused symbol "_testUtils"
(declared around line 21) in the saas-dashboard-permissions-e2e.test.ts file;
either delete the eslint-disable line and the "_testUtils" declaration if the
helper is not needed, or instead keep "_testUtils" and actually use it in the
test to justify the ignore—ensure no other unused-vars disables remain and run
the linter to confirm the warning is resolved.
🪄 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: f3ac0177-342f-467e-8745-50b106d3ba4c
📒 Files selected for processing (3)
backend/src/entities/visualizations/dashboard/use-cases/find-all-dashboards.use.case.tsbackend/src/guards/dashboard-read.guard.tsbackend/test/ava-tests/saas-tests/saas-dashboard-permissions-e2e.test.ts
| test.serial( | ||
| `${currentTest} should return empty array when user has no dashboard read permissions`, | ||
| async (t) => { | ||
| try { | ||
| const testData = | ||
| await createConnectionsAndInviteNewUserInNewGroupWithTableDifferentConnectionGroupReadOnlyPermissions(app); | ||
| const connectionId = testData.connections.firstId; | ||
| const groupId = testData.groups.createdGroupId; | ||
|
|
||
| // Admin creates a dashboard | ||
| const createDashboard = await request(app.getHttpServer()) | ||
| .post(`/dashboards/${connectionId}`) | ||
| .send({ name: 'Hidden Dashboard' }) | ||
| .set('Cookie', testData.users.adminUserToken) | ||
| .set('masterpwd', 'ahalaimahalai') | ||
| .set('Content-Type', 'application/json') | ||
| .set('Accept', 'application/json'); | ||
|
|
||
| t.is(createDashboard.status, 201); | ||
|
|
||
| // Save cedar policy with connection read only, no dashboard permissions | ||
| const cedarPolicy = `permit(\n principal,\n action == RocketAdmin::Action::"connection:read",\n resource == RocketAdmin::Connection::"${connectionId}"\n);`; | ||
|
|
||
| const savePolicyResponse = await request(app.getHttpServer()) | ||
| .post(`/connection/cedar-policy/${connectionId}`) | ||
| .send({ cedarPolicy, groupId }) | ||
| .set('Cookie', testData.users.adminUserToken) | ||
| .set('Content-Type', 'application/json') | ||
| .set('Accept', 'application/json'); | ||
|
|
||
| t.is(savePolicyResponse.status, 201); | ||
|
|
||
| // Simple user lists dashboards — should get empty array | ||
| const listDashboards = await request(app.getHttpServer()) | ||
| .get(`/dashboards/${connectionId}`) | ||
| .set('Cookie', testData.users.simpleUserToken) | ||
| .set('Content-Type', 'application/json') | ||
| .set('Accept', 'application/json'); | ||
|
|
||
| t.is(listDashboards.status, 200); | ||
| t.is(listDashboards.body.length, 0); | ||
| } catch (error) { | ||
| console.error(error); | ||
| throw error; | ||
| } | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Fix formatting to resolve pipeline failure.
The CI pipeline is failing due to Biome formatting issues in lines 181-263. Please run your formatter (e.g., npx biome format --write or equivalent project command) to fix the formatting discrepancies.
🤖 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 183 - 229, The failing CI is due to Biome formatting issues in this
test block; run the project's formatter (e.g., npx biome format --write or the
repo's format script) and reformat the file containing the test.serial block
(the test that uses
createConnectionsAndInviteNewUserInNewGroupWithTableDifferentConnectionGroupReadOnlyPermissions,
cedarPolicy, savePolicyResponse, listDashboards) so indentation, line breaks,
and spacing match the project's style, then stage and commit the formatted file.
There was a problem hiding this comment.
Pull request overview
Adds Cedar-based authorization enforcement for dashboard listing and introduces end-to-end tests to verify that dashboard access is correctly allowed/denied per-dashboard.
Changes:
- Add E2E coverage for listing and reading dashboards under Cedar policies (allowed, filtered, empty, forbidden cases).
- Update
DashboardReadGuardbehavior for routes withoutdashboardIdby validating connection membership. - Filter
GET /dashboards/:connectionIdresults by per-dashboard Cedardashboard:readauthorization.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| backend/test/ava-tests/saas-tests/saas-dashboard-permissions-e2e.test.ts | New E2E tests covering dashboard list/read behavior under Cedar policies. |
| backend/src/guards/dashboard-read.guard.ts | Allow dashboard-list requests to proceed when user belongs to the connection (no dashboardId), while keeping per-dashboard read checks for specific dashboard routes. |
| backend/src/entities/visualizations/dashboard/use-cases/find-all-dashboards.use.case.ts | Add per-dashboard Cedar authorization filtering to ensure only permitted dashboards are returned from list endpoint. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const dashboards = | ||
| await this._dbContext.dashboardRepository.findAllDashboardsWithWidgetsByConnectionId(connectionId); | ||
|
|
There was a problem hiding this comment.
findAllDashboardsWithWidgetsByConnectionId loads widgets for every dashboard before authorization filtering. With per-dashboard Cedar checks added below, this can fetch potentially large widget payloads for dashboards the caller is not allowed to see, increasing DB load and response time. Consider fetching only dashboard metadata/IDs first, filter by permissions, then load widgets only for the allowed dashboards.
| const accessChecks = await Promise.all( | ||
| dashboards.map((dashboard) => | ||
| this.cedarAuthService.validate({ | ||
| userId, | ||
| action: CedarAction.DashboardRead, | ||
| connectionId, | ||
| dashboardId: dashboard.id, | ||
| }), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
This does one cedarAuthService.validate call per dashboard and runs them all concurrently via Promise.all. validate() performs DB lookups (user suspension + group membership) and Cedar evaluation, so this becomes an N-per-dashboard query/CPU pattern and can overwhelm the DB/pool for connections with many dashboards. Consider adding a batch/streamed validation path that reuses the loaded userGroups/policies once per request (or at least limiting concurrency).
Summary by CodeRabbit
New Features
Tests