Conversation
- Implement tests for saving Cedar policies with various permissions - Validate classical permissions returned after saving policies - Ensure enforcement of permissions for users based on saved policies - Test rejection of invalid requests, including empty policies and unauthorized access - Verify overwriting of previous permissions when saving new policies
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR extends Cedar authorization support to include dashboard permissions, adds API endpoints to fetch/validate Cedar schema and save Cedar policies for groups (with “classical permissions” derivation), and introduces new/updated tests around policy save + parsing/generation.
Changes:
- Add Cedar schema/policy management endpoints and service logic to validate/save policies and derive classical permissions.
- Introduce dashboard Cedar actions/resources plus dashboard-specific guards, and wire them into the dashboards controller.
- Add AVA unit + e2e tests for Cedar policy save, parsing, generation, and entity building (including dashboards).
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/test/ava-tests/saas-tests/saas-cedar-save-policy-e2e.test.ts | SaaS e2e coverage for saving Cedar policy + enforcement + reference validation (incl dashboards). |
| backend/test/ava-tests/non-saas-tests/non-saas-cedar-save-policy-e2e.test.ts | Non-SaaS e2e coverage for saving Cedar policy + enforcement + reference validation (incl dashboards). |
| backend/test/ava-tests/non-saas-tests/non-saas-cedar-policy-parser.test.ts | Unit tests for parsing Cedar policies into classical permissions (incl dashboards). |
| backend/test/ava-tests/non-saas-tests/non-saas-cedar-policy-generator.test.ts | Unit tests for generating Cedar policy text from permissions (incl dashboards). |
| backend/test/ava-tests/non-saas-tests/non-saas-cedar-entity-builder.test.ts | Unit tests for Cedar entity building (adds dashboard entity cases). |
| backend/src/guards/index.ts | Export new dashboard guards. |
| backend/src/guards/dashboard-read.guard.ts | New guard for dashboard read authorization (Cedar-first + legacy fallback). |
| backend/src/guards/dashboard-edit.guard.ts | New guard for dashboard edit/delete authorization (Cedar-first + legacy fallback). |
| backend/src/guards/dashboard-create.guard.ts | New guard for dashboard creation authorization (Cedar-first + legacy fallback). |
| backend/src/exceptions/text/messages.ts | Add new error messages for Cedar policy reference validation. |
| backend/src/entities/visualizations/dashboard/dashboards.controller.ts | Swap connection-level guards for dashboard-specific guards on dashboard routes. |
| backend/src/entities/permission/permission.interface.ts | Extend complex permission shape with optional dashboard permissions. |
| backend/src/entities/cedar-authorization/dto/validate-cedar-schema.dto.ts | DTO for validating Cedar schema input. |
| backend/src/entities/cedar-authorization/dto/save-cedar-policy.dto.ts | DTO for saving Cedar policy for a group. |
| backend/src/entities/cedar-authorization/cedar-schema.ts | Add Dashboard entity + dashboard actions to Cedar schema (TS version). |
| backend/src/entities/cedar-authorization/cedar-schema.json | Add Dashboard entity + dashboard actions to Cedar schema (JSON version). |
| backend/src/entities/cedar-authorization/cedar-policy-parser.ts | New Cedar policy parser to derive classical permissions (incl dashboards). |
| backend/src/entities/cedar-authorization/cedar-policy-generator.ts | Generate dashboard-related permit statements. |
| backend/src/entities/cedar-authorization/cedar-entity-builder.ts | Add optional dashboard entity construction. |
| backend/src/entities/cedar-authorization/cedar-authorization.service.ts | Major expansion: dashboard validation support, schema/policy validation, save policy flow, reference validation, permission sync. |
| backend/src/entities/cedar-authorization/cedar-authorization.service.interface.ts | Extend interface with schema retrieval/validation and save-policy API. |
| backend/src/entities/cedar-authorization/cedar-authorization.module.ts | Register controller + middleware routes; wire GlobalDatabaseContext. |
| backend/src/entities/cedar-authorization/cedar-authorization.controller.ts | New controller exposing cedar schema and policy management endpoints. |
| backend/src/entities/cedar-authorization/cedar-action-map.ts | Add dashboard actions/resource type and dashboardId to validation request. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| break; | ||
| case 'dashboard': | ||
| resourceType = CedarResourceType.Dashboard; | ||
| resourceId = `${connectionId}/${dashboardId}`; |
There was a problem hiding this comment.
CedarAuthorizationService.validate() builds dashboard resources as ${connectionId}/${dashboardId} for all dashboard:* actions (including create/list flows where dashboardId can be undefined), which produces a resource id containing undefined and breaks authorization. Add explicit validation/branching here: require dashboardId for read/edit/delete, and for create (and possibly list) evaluate against a connection-scoped resource (or introduce a dedicated list action) so policies can be expressed and matched.
| resourceId = `${connectionId}/${dashboardId}`; | |
| // Distinguish between connection-scoped and dashboard-scoped actions | |
| { | |
| const dashboardAction = action.split(':')[1]; | |
| if (dashboardAction === 'create' || dashboardAction === 'list') { | |
| // For create/list, evaluate against the connection as the resource | |
| if (!connectionId) return false; | |
| resourceId = connectionId; | |
| } else { | |
| // For actions on an existing dashboard, a dashboardId is required | |
| if (!dashboardId) return false; | |
| resourceId = `${connectionId}/${dashboardId}`; | |
| } | |
| } |
| cedarWasm.isAuthorized(testCall as Parameters<typeof cedarWasm.isAuthorized>[0]); | ||
| } catch (e) { | ||
| throw new HttpException({ message: `Invalid cedar schema: ${e.message}` }, HttpStatus.BAD_REQUEST); |
There was a problem hiding this comment.
validateCedarSchema() calls cedarWasm.isAuthorized(...) inside a try/catch but ignores the returned result. isAuthorized can return { type: 'error', errors: ... } without throwing, so invalid schemas may be treated as valid. Capture the return value and throw a 400 when result.type !== 'success' (including surfacing result.errors).
| cedarWasm.isAuthorized(testCall as Parameters<typeof cedarWasm.isAuthorized>[0]); | |
| } catch (e) { | |
| throw new HttpException({ message: `Invalid cedar schema: ${e.message}` }, HttpStatus.BAD_REQUEST); | |
| const result = cedarWasm.isAuthorized(testCall as Parameters<typeof cedarWasm.isAuthorized>[0]); | |
| if (result.type !== 'success') { | |
| throw new HttpException( | |
| { | |
| message: 'Invalid cedar schema', | |
| errors: (result as any).errors, | |
| }, | |
| HttpStatus.BAD_REQUEST, | |
| ); | |
| } | |
| } catch (e) { | |
| const message = (e as any)?.message ?? String(e); | |
| throw new HttpException({ message: `Invalid cedar schema: ${message}` }, HttpStatus.BAD_REQUEST); |
| cedarWasm.isAuthorized(testCall as Parameters<typeof cedarWasm.isAuthorized>[0]); | ||
| } catch (e) { | ||
| throw new HttpException({ message: `Invalid cedar policy: ${e.message}` }, HttpStatus.BAD_REQUEST); |
There was a problem hiding this comment.
validateCedarPolicyText() uses try/catch around cedarWasm.isAuthorized(...) but does not inspect the returned { type: 'success' | 'error' } result. If Cedar returns an error result (without throwing) for invalid policy text, this method will incorrectly accept the policy. Check the return value and reject when type !== 'success' (and include returned errors in the message).
| cedarWasm.isAuthorized(testCall as Parameters<typeof cedarWasm.isAuthorized>[0]); | |
| } catch (e) { | |
| throw new HttpException({ message: `Invalid cedar policy: ${e.message}` }, HttpStatus.BAD_REQUEST); | |
| const result = cedarWasm.isAuthorized(testCall as Parameters<typeof cedarWasm.isAuthorized>[0]); | |
| if ((result as any)?.type !== 'success') { | |
| const errors = (result as any)?.errors; | |
| const errorMessages = Array.isArray(errors) | |
| ? errors.map((err: any) => { | |
| if (typeof err === 'string') return err; | |
| if (err && typeof err.message === 'string') return err.message; | |
| try { | |
| return JSON.stringify(err); | |
| } catch { | |
| return String(err); | |
| } | |
| }) | |
| : ['Unknown Cedar validation error']; | |
| throw new HttpException( | |
| { message: `Invalid cedar policy: ${errorMessages.join('; ')}` }, | |
| HttpStatus.BAD_REQUEST, | |
| ); | |
| } | |
| } catch (e: any) { | |
| const message = | |
| e && typeof e.message === 'string' | |
| ? e.message | |
| : typeof e === 'string' | |
| ? e | |
| : 'Unknown Cedar validation error'; | |
| throw new HttpException({ message: `Invalid cedar policy: ${message}` }, HttpStatus.BAD_REQUEST); |
|
|
||
| const principalGroupIds = [ | ||
| ...cedarPolicy.matchAll(/principal\s+in\s+RocketAdmin::Group::"([^"]+)"/g), | ||
| ].map((m) => m[1]); | ||
|
|
||
| for (const principalGroupId of principalGroupIds) { | ||
| if (principalGroupId !== groupId) { | ||
| throw new HttpException( | ||
| { message: Messages.CEDAR_POLICY_REFERENCES_FOREIGN_PRINCIPAL }, | ||
| HttpStatus.BAD_REQUEST, | ||
| ); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
validatePolicyReferences() only checks principals that match principal in RocketAdmin::Group::"...". If a policy uses a wildcard principal (e.g. permit(principal, action, resource);) or any other principal form, principalGroupIds will be empty and the policy will pass validation, potentially granting permissions to users outside the target group (since policies are concatenated across the connection). Consider enforcing that every permit statement’s principal is scoped to the target groupId (and explicitly rejecting wildcard/unspecified principals) before saving the policy.
| 'dashboard:create': { | ||
| appliesTo: { | ||
| principalTypes: ['User'], | ||
| resourceTypes: ['Dashboard'], |
There was a problem hiding this comment.
Cedar schema models dashboard:create as applying to resourceTypes: ['Dashboard'], but the API route POST /dashboards/:connectionId doesn't include (or accept) a dashboard id, so callers can't satisfy a resource equality check for a specific Dashboard. To make dashboard creation/listing authorizable, consider changing dashboard:create (and possibly the list operation) to apply to the Connection resource type (or introduce a separate connection-scoped action) and adjust guards/service validation accordingly.
| resourceTypes: ['Dashboard'], | |
| resourceTypes: ['Connection'], |
| @ApiOperation({ summary: 'Validate a cedar schema against the Cedar engine' }) | ||
| @ApiResponse({ | ||
| status: 200, | ||
| description: 'Cedar schema is valid.', | ||
| }) | ||
| @ApiBody({ type: ValidateCedarSchemaDto }) | ||
| @ApiParam({ name: 'connectionId', required: true }) | ||
| @UseGuards(ConnectionReadGuard) | ||
| @Post('/connection/cedar-schema/validate/:connectionId') | ||
| async validateCedarSchema( | ||
| @SlugUuid('connectionId') connectionId: string, | ||
| @Body() dto: ValidateCedarSchemaDto, | ||
| ): Promise<{ valid: boolean }> { | ||
| if (!connectionId) { | ||
| throw new HttpException({ message: Messages.CONNECTION_ID_MISSING }, HttpStatus.BAD_REQUEST); | ||
| } | ||
| this.cedarAuthService.validateCedarSchema(dto.cedarSchema); | ||
| return { valid: true }; | ||
| } | ||
|
|
||
| @ApiOperation({ summary: 'Save a cedar policy for a group, generating classical permissions for backward compatibility' }) | ||
| @ApiResponse({ | ||
| status: 200, | ||
| description: 'Cedar policy saved and classical permissions generated.', | ||
| }) | ||
| @ApiBody({ type: SaveCedarPolicyDto }) | ||
| @ApiParam({ name: 'connectionId', required: true }) | ||
| @UseGuards(ConnectionEditGuard) | ||
| @Post('/connection/cedar-policy/:connectionId') |
There was a problem hiding this comment.
Swagger decorators declare @ApiResponse({ status: 200 }) for POST endpoints (/connection/cedar-schema/validate/:connectionId and /connection/cedar-policy/:connectionId), but Nest will respond with 201 by default for @Post() unless @HttpCode(200) is set. Update the documented status to 201 or set an explicit HttpCode to keep the OpenAPI spec consistent with actual responses (the e2e tests assert 201 for save policy).
| const allowed = await this.cedarAuthService.validate({ | ||
| userId: cognitoUserName, | ||
| action: CedarAction.DashboardRead, | ||
| connectionId, | ||
| dashboardId, | ||
| }); |
There was a problem hiding this comment.
DashboardReadGuard always authorizes using CedarAction.DashboardRead with dashboardId = request.params?.dashboardId, but routes like GET /dashboards/:connectionId don't have dashboardId, so Cedar validation is called with dashboardId undefined and the resource becomes ${connectionId}/undefined, making valid policies impossible to match. Consider branching: when dashboardId is missing, authorize at the connection level (e.g., CedarAction.ConnectionRead / legacy connection read), and only use DashboardRead when dashboardId is present (GET /dashboard/:dashboardId/:connectionId).
| const allowed = await this.cedarAuthService.validate({ | |
| userId: cognitoUserName, | |
| action: CedarAction.DashboardRead, | |
| connectionId, | |
| dashboardId, | |
| }); | |
| let allowed: boolean; | |
| if (dashboardId) { | |
| // Dashboard-level authorization when a specific dashboard is addressed | |
| allowed = await this.cedarAuthService.validate({ | |
| userId: cognitoUserName, | |
| action: CedarAction.DashboardRead, | |
| connectionId, | |
| dashboardId, | |
| }); | |
| } else { | |
| // Connection-level authorization when no specific dashboard is provided | |
| allowed = await this.cedarAuthService.validate({ | |
| userId: cognitoUserName, | |
| action: CedarAction.ConnectionRead, | |
| connectionId, | |
| }); | |
| } |
| if (this.cedarAuthService.isFeatureEnabled()) { | ||
| try { | ||
| const allowed = await this.cedarAuthService.validate({ | ||
| userId: cognitoUserName, | ||
| action: CedarAction.DashboardCreate, | ||
| connectionId, | ||
| }); | ||
| if (allowed) { | ||
| resolve(true); | ||
| return; | ||
| } | ||
| reject(new ForbiddenException(Messages.DONT_HAVE_PERMISSIONS)); | ||
| return; |
There was a problem hiding this comment.
DashboardCreateGuard calls Cedar validation for dashboard:create without any dashboardId (create endpoint body has only name/description), so the Cedar resource id becomes ${connectionId}/undefined and policies like resource == RocketAdmin::Dashboard::"${connectionId}/dash-1" can never authorize creation. To make dashboard creation authorizable, either (a) model dashboard:create as a connection-scoped permission (resource type Connection) and validate against the connection, or (b) include a dashboard identifier in the request and pass it into validation.
… improve whitespace handling
No description provided.