Refactor Cedar policy tests to remove group principal references#1660
Refactor Cedar policy tests to remove group principal references#1660
Conversation
Artuomka
commented
Mar 12, 2026
Artuomka
commented
- Updated Cedar policy definitions in non-SaaS and SaaS end-to-end tests to use 'principal' instead of 'principal in RocketAdmin::Group::"{groupId}"'.
- Removed redundant tests that checked for foreign group references in Cedar policies.
- Ensured consistency across all Cedar policy tests by standardizing the policy format.
- Updated Cedar policy definitions in non-SaaS and SaaS end-to-end tests to use 'principal' instead of 'principal in RocketAdmin::Group::"{groupId}"'.
- Removed redundant tests that checked for foreign group references in Cedar policies.
- Ensured consistency across all Cedar policy tests by standardizing the policy format.
There was a problem hiding this comment.
Pull request overview
This PR refactors the Cedar authorization system to remove group principal scoping from policies. Instead of encoding principal in RocketAdmin::Group::"<groupId>" in each policy statement, policies now use bare principal. The authorization model shifts from Cedar-engine-level principal filtering to application-level policy loading, where only policies for groups the user belongs to are loaded and evaluated.
Changes:
- Replaced
principal in RocketAdmin::Group::"..."with bareprincipalin policy generation, parsing, schema, entity building, and all test files. - Changed policy loading from connection-scoped (
loadPoliciesForConnection) to user-scoped (loadPoliciesForUser), with per-user caching keyed byconnectionId:userId. - Removed validation and tests for foreign group principal references, since policies no longer contain principal group scoping.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
backend/src/entities/cedar-authorization/cedar-policy-generator.ts |
Removed groupRef and changed all policy strings to use bare principal |
backend/src/entities/cedar-authorization/cedar-entity-builder.ts |
Removed group parents from user entity |
backend/src/entities/cedar-authorization/cedar-schema.ts |
Cleared memberOfTypes for User entity |
backend/src/entities/cedar-authorization/cedar-policy-parser.ts |
Renamed match variable (old matching logic retained but now dead code) |
backend/src/entities/cedar-authorization/cedar-authorization.service.ts |
Replaced loadPoliciesForConnection with loadPoliciesForUser that filters by user group membership; removed foreign principal validation |
backend/src/helpers/cache/cacher.ts |
Updated cache to key by connectionId:userId and iterate-invalidate by connection prefix |
backend/test/.../non-saas-cedar-policy-parser.test.ts |
Updated test policies to use bare principal |
backend/test/.../non-saas-cedar-policy-generator.test.ts |
Updated assertions to check for bare principal |
backend/test/.../non-saas-cedar-entity-builder.test.ts |
Updated test to expect empty parents |
backend/test/.../non-saas-cedar-save-policy-e2e.test.ts |
Updated policies and removed foreign principal tests |
backend/test/.../saas-cedar-save-policy-e2e.test.ts |
Updated policies and removed foreign principal test |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const policies: Array<string> = []; | ||
| const groupRef = `RocketAdmin::Group::"${groupId}"`; | ||
| const connectionRef = `RocketAdmin::Connection::"${connectionId}"`; |
There was a problem hiding this comment.
The groupId parameter (line 5) is now unused in this function. After removing the groupRef variable, the only reference to a group ID is permissions.group.groupId (line 37), which comes from the permissions object, not from the groupId parameter. Consider removing the groupId parameter and updating all callers (found in create-connection.use.case.ts, create-group-in-connection.use.case.ts, create-or-update-permissions.use.case.ts, migrate-permissions-to-cedar.ts, and tests) to avoid confusion.
| const principalInMatch = body.match(/principal\s+in\s+RocketAdmin::Group::"([^"]+)"/); | ||
| if (principalInMatch) { | ||
| result.groupId = principalInMatch[1]; | ||
| } |
There was a problem hiding this comment.
The parser still attempts to match the old principal in RocketAdmin::Group::"..." pattern on line 150, and the ParsedPermitStatement.groupId field (line 9) is set but never consumed downstream. Since policies now use bare principal, this matching code is dead. Consider removing the principalInMatch logic (lines 150-153) and the groupId field from ParsedPermitStatement to avoid confusion about its purpose.
| private async loadPoliciesForUser(connectionId: string, userId: string, userGroupIds: string[]): Promise<string | null> { | ||
| const cached = Cacher.getCedarPolicyCache(connectionId, userId); | ||
| if (cached !== null) return cached; | ||
|
|
||
| const groups = await this.globalDbContext.groupRepository.findAllGroupsInConnection(connectionId); | ||
| const policyTexts = groups.map((g) => g.cedarPolicy).filter(Boolean); | ||
| const userGroupIdSet = new Set(userGroupIds); | ||
| const policyTexts = groups | ||
| .filter((g) => userGroupIdSet.has(g.id)) | ||
| .map((g) => g.cedarPolicy) | ||
| .filter(Boolean); | ||
|
|
||
| if (policyTexts.length === 0) return null; | ||
|
|
||
| const combined = policyTexts.join('\n\n'); | ||
| Cacher.setCedarPolicyCache(connectionId, combined); | ||
| Cacher.setCedarPolicyCache(connectionId, userId, combined); | ||
| return combined; |
There was a problem hiding this comment.
The loadPoliciesForUser method fetches all groups in the connection via findAllGroupsInConnection and then filters in-memory by user group IDs. Since you already have the userGroupIds, it would be more efficient to query only the user's groups directly (e.g., using the userGroups already fetched in the evaluate method and accessing their cedarPolicy field) instead of fetching all groups and filtering. This avoids loading unnecessary data, especially for connections with many groups.
