Skip to content

Refactor Cedar policy tests to remove group principal references #1660

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Artuomka merged 2 commits into from
Mar 12, 2026
Merged

Refactor Cedar policy tests to remove group principal references #1660

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7a41f62
Refactor Cedar policy tests to remove group principal references
Artuomka Mar 12, 2026
9b30f7d
Refactor Cedar policy generation to remove group ID references across…
Artuomka Mar 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 11 additions & 21 deletions backend/src/entities/cedar-authorization/cedar-authorization.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ export class CedarAuthorizationService implements ICedarAuthorizationService, On
throw new HttpException({ message: Messages.CANNOT_CHANGE_ADMIN_GROUP }, HttpStatus.BAD_REQUEST);
}

await this.validatePolicyReferences(cedarPolicy, connectionId, groupId);
await this.validatePolicyReferences(cedarPolicy, connectionId);

const classicalPermissions = parseCedarPolicyToClassicalPermissions(cedarPolicy, connectionId, groupId);

Expand Down Expand Up @@ -186,7 +186,8 @@ export class CedarAuthorizationService implements ICedarAuthorizationService, On
const userGroups = await this.globalDbContext.groupRepository.findAllUserGroupsInConnection(connectionId, userId);
if (userGroups.length === 0) return false;

const policies = await this.loadPoliciesForConnection(connectionId);
const userGroupIds = userGroups.map((g) => g.id);
const policies = await this.loadPoliciesForUser(connectionId, userId, userGroupIds);
if (!policies) return false;

const entities = buildCedarEntities(userId, userGroups, connectionId, tableName, dashboardId);
Expand All @@ -210,17 +211,21 @@ export class CedarAuthorizationService implements ICedarAuthorizationService, On
return false;
}

private async loadPoliciesForConnection(connectionId: string): Promise<string | null> {
const cached = Cacher.getCedarPolicyCache(connectionId);
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;
Comment on lines +214 to 229
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
}

Expand Down Expand Up @@ -268,22 +273,7 @@ export class CedarAuthorizationService implements ICedarAuthorizationService, On
private async validatePolicyReferences(
cedarPolicy: string,
connectionId: string,
groupId: string,
): Promise<void> {

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,
);
}
}

const connectionIds = [
...cedarPolicy.matchAll(/resource\s*==\s*RocketAdmin::Connection::"([^"]+)"/g),
].map((m) => m[1]);
Expand Down
2 changes: 1 addition & 1 deletion backend/src/entities/cedar-authorization/cedar-entity-builder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ export function buildCedarEntities(
entities.push({
uid: { type: 'RocketAdmin::User', id: userId },
attrs: { suspended: false },
parents: userGroups.map((g) => ({ type: 'RocketAdmin::Group', id: g.id })),
parents: [],
});

// Group entities
Expand Down
34 changes: 16 additions & 18 deletions backend/src/entities/cedar-authorization/cedar-policy-generator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,16 @@ import { AccessLevelEnum } from '../../enums/index.js';
import { IComplexPermission } from '../permission/permission.interface.js';

export function generateCedarPolicyForGroup(
groupId: string,
connectionId: string,
isMain: boolean,
permissions: IComplexPermission,
): string {
const policies: Array<string> = [];
const groupRef = `RocketAdmin::Group::"${groupId}"`;
const connectionRef = `RocketAdmin::Connection::"${connectionId}"`;
Comment on lines 9 to 10
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

if (isMain) {
policies.push(
`permit(\n principal in ${groupRef},\n action,\n resource\n);`,
`permit(\n principal,\n action,\n resource\n);`,
);
return policies.join('\n\n');
}
Expand All @@ -22,14 +20,14 @@ export function generateCedarPolicyForGroup(
const connAccess = permissions.connection.accessLevel;
if (connAccess === AccessLevelEnum.edit) {
policies.push(
`permit(\n principal in ${groupRef},\n action == RocketAdmin::Action::"connection:read",\n resource == ${connectionRef}\n);`,
`permit(\n principal,\n action == RocketAdmin::Action::"connection:read",\n resource == ${connectionRef}\n);`,
);
policies.push(
`permit(\n principal in ${groupRef},\n action == RocketAdmin::Action::"connection:edit",\n resource == ${connectionRef}\n);`,
`permit(\n principal,\n action == RocketAdmin::Action::"connection:edit",\n resource == ${connectionRef}\n);`,
);
} else if (connAccess === AccessLevelEnum.readonly) {
policies.push(
`permit(\n principal in ${groupRef},\n action == RocketAdmin::Action::"connection:read",\n resource == ${connectionRef}\n);`,
`permit(\n principal,\n action == RocketAdmin::Action::"connection:read",\n resource == ${connectionRef}\n);`,
);
}

Expand All @@ -38,14 +36,14 @@ export function generateCedarPolicyForGroup(
const groupResourceRef = `RocketAdmin::Group::"${permissions.group.groupId}"`;
if (groupAccess === AccessLevelEnum.edit) {
policies.push(
`permit(\n principal in ${groupRef},\n action == RocketAdmin::Action::"group:read",\n resource == ${groupResourceRef}\n);`,
`permit(\n principal,\n action == RocketAdmin::Action::"group:read",\n resource == ${groupResourceRef}\n);`,
);
policies.push(
`permit(\n principal in ${groupRef},\n action == RocketAdmin::Action::"group:edit",\n resource == ${groupResourceRef}\n);`,
`permit(\n principal,\n action == RocketAdmin::Action::"group:edit",\n resource == ${groupResourceRef}\n);`,
);
} else if (groupAccess === AccessLevelEnum.readonly) {
policies.push(
`permit(\n principal in ${groupRef},\n action == RocketAdmin::Action::"group:read",\n resource == ${groupResourceRef}\n);`,
`permit(\n principal,\n action == RocketAdmin::Action::"group:read",\n resource == ${groupResourceRef}\n);`,
);
}

Expand All @@ -59,32 +57,32 @@ export function generateCedarPolicyForGroup(
if (access.read) {
hasReadPermission = true;
policies.push(
`permit(\n principal in ${groupRef},\n action == RocketAdmin::Action::"dashboard:read",\n resource == ${dashboardRef}\n);`,
`permit(\n principal,\n action == RocketAdmin::Action::"dashboard:read",\n resource == ${dashboardRef}\n);`,
);
}
if (access.create) {
hasCreatePermission = true;
}
if (access.edit) {
policies.push(
`permit(\n principal in ${groupRef},\n action == RocketAdmin::Action::"dashboard:edit",\n resource == ${dashboardRef}\n);`,
`permit(\n principal,\n action == RocketAdmin::Action::"dashboard:edit",\n resource == ${dashboardRef}\n);`,
);
}
if (access.delete) {
policies.push(
`permit(\n principal in ${groupRef},\n action == RocketAdmin::Action::"dashboard:delete",\n resource == ${dashboardRef}\n);`,
`permit(\n principal,\n action == RocketAdmin::Action::"dashboard:delete",\n resource == ${dashboardRef}\n);`,
);
}
}
const newDashboardRef = `RocketAdmin::Dashboard::"${connectionId}/__new__"`;
if (hasReadPermission) {
policies.push(
`permit(\n principal in ${groupRef},\n action == RocketAdmin::Action::"dashboard:read",\n resource == ${newDashboardRef}\n);`,
`permit(\n principal,\n action == RocketAdmin::Action::"dashboard:read",\n resource == ${newDashboardRef}\n);`,
);
}
if (hasCreatePermission) {
policies.push(
`permit(\n principal in ${groupRef},\n action == RocketAdmin::Action::"dashboard:create",\n resource == ${newDashboardRef}\n);`,
`permit(\n principal,\n action == RocketAdmin::Action::"dashboard:create",\n resource == ${newDashboardRef}\n);`,
);
}
}
Expand All @@ -96,22 +94,22 @@ export function generateCedarPolicyForGroup(
const hasAnyAccess = access.visibility || access.add || access.delete || access.edit;
if (hasAnyAccess) {
policies.push(
`permit(\n principal in ${groupRef},\n action == RocketAdmin::Action::"table:read",\n resource == ${tableRef}\n);`,
`permit(\n principal,\n action == RocketAdmin::Action::"table:read",\n resource == ${tableRef}\n);`,
);
}
if (access.add) {
policies.push(
`permit(\n principal in ${groupRef},\n action == RocketAdmin::Action::"table:add",\n resource == ${tableRef}\n);`,
`permit(\n principal,\n action == RocketAdmin::Action::"table:add",\n resource == ${tableRef}\n);`,
);
}
if (access.edit) {
policies.push(
`permit(\n principal in ${groupRef},\n action == RocketAdmin::Action::"table:edit",\n resource == ${tableRef}\n);`,
`permit(\n principal,\n action == RocketAdmin::Action::"table:edit",\n resource == ${tableRef}\n);`,
);
}
if (access.delete) {
policies.push(
`permit(\n principal in ${groupRef},\n action == RocketAdmin::Action::"table:delete",\n resource == ${tableRef}\n);`,
`permit(\n principal,\n action == RocketAdmin::Action::"table:delete",\n resource == ${tableRef}\n);`,
);
}
}
Expand Down
7 changes: 0 additions & 7 deletions backend/src/entities/cedar-authorization/cedar-policy-parser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import {
} from '../permission/permission.interface.js';

interface ParsedPermitStatement {
groupId: string | null;
action: string | null;
resourceType: string | null;
resourceId: string | null;
Expand Down Expand Up @@ -140,18 +139,12 @@ function extractPermitStatements(policyText: string): ParsedPermitStatement[] {

function parsePermitBody(body: string): ParsedPermitStatement {
const result: ParsedPermitStatement = {
groupId: null,
action: null,
resourceType: null,
resourceId: null,
isWildcard: false,
};

const principalMatch = body.match(/principal\s+in\s+RocketAdmin::Group::"([^"]+)"/);
if (principalMatch) {
result.groupId = principalMatch[1];
}

const actionMatch = body.match(/action\s*==\s*RocketAdmin::Action::"([^"]+)"/);
if (actionMatch) {
result.action = actionMatch[1];
Expand Down
2 changes: 1 addition & 1 deletion backend/src/entities/cedar-authorization/cedar-schema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ export const CEDAR_SCHEMA = {
RocketAdmin: {
entityTypes: {
User: {
memberOfTypes: ['Group'],
memberOfTypes: [],
shape: {
type: 'Record',
attributes: {
Expand Down
2 changes: 1 addition & 1 deletion backend/src/entities/cedar-authorization/scripts/migrate-permissions-to-cedar.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ export async function migratePermissionsToCedar(dataSource: DataSource): Promise
tables: Array.from(tableMap.values()),
};

const cedarPolicy = generateCedarPolicyForGroup(group.id, connection.id, group.isMain, complexPermission);
const cedarPolicy = generateCedarPolicyForGroup(connection.id, group.isMain, complexPermission);
group.cedarPolicy = cedarPolicy;
await groupRepository.save(group);
migratedCount++;
Expand Down
1 change: 0 additions & 1 deletion backend/src/entities/connection/use-cases/create-connection.use.case.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,6 @@ export class CreateConnectionUseCase
);
await this._dbContext.permissionRepository.createdDefaultAdminPermissionsInGroup(createdAdminGroup);
createdAdminGroup.cedarPolicy = generateCedarPolicyForGroup(
createdAdminGroup.id,
savedConnection.id,
true,
{
Expand Down
1 change: 0 additions & 1 deletion backend/src/entities/connection/use-cases/create-group-in-connection.use.case.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ export class CreateGroupInConnectionUseCase
const newGroupEntity = buildNewGroupEntityForConnectionWithUser(connectionToUpdate, foundUser, title);
const savedGroup = await this._dbContext.groupRepository.saveNewOrUpdatedGroup(newGroupEntity);
savedGroup.cedarPolicy = generateCedarPolicyForGroup(
savedGroup.id,
connectionId,
false,
{
Expand Down
2 changes: 1 addition & 1 deletion backend/src/entities/permission/use-cases/create-or-update-permissions.use.case.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ export class CreateOrUpdatePermissionsUseCase
);

// Generate and save Cedar policy for this group
const cedarPolicy = generateCedarPolicyForGroup(groupId, connectionId, groupToUpdate.isMain, resultPermissions);
const cedarPolicy = generateCedarPolicyForGroup(connectionId, groupToUpdate.isMain, resultPermissions);
groupToUpdate.cedarPolicy = cedarPolicy;
await this._dbContext.groupRepository.saveNewOrUpdatedGroup(groupToUpdate);
Cacher.invalidateCedarPolicyCache(connectionId);
Expand Down
16 changes: 11 additions & 5 deletions backend/src/helpers/cache/cacher.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,17 +66,23 @@ export class Cacher {
return userInvitations <= 10 && groupInvitations <= 10;
}

public static getCedarPolicyCache(connectionId: string): string | null {
const cached = cedarPolicyCache.get(connectionId);
public static getCedarPolicyCache(connectionId: string, userId: string): string | null {
const cacheKey = `${connectionId}:${userId}`;
const cached = cedarPolicyCache.get(cacheKey);
return cached !== undefined ? cached : null;
}

public static setCedarPolicyCache(connectionId: string, policies: string): void {
cedarPolicyCache.set(connectionId, policies);
public static setCedarPolicyCache(connectionId: string, userId: string, policies: string): void {
const cacheKey = `${connectionId}:${userId}`;
cedarPolicyCache.set(cacheKey, policies);
}

public static invalidateCedarPolicyCache(connectionId: string): void {
cedarPolicyCache.delete(connectionId);
for (const key of cedarPolicyCache.keys()) {
if (key.startsWith(`${connectionId}:`)) {
cedarPolicyCache.delete(key);
}
}
}

public static async clearAllCache(): Promise<void> {
Expand Down
6 changes: 2 additions & 4 deletions backend/test/ava-tests/non-saas-tests/non-saas-cedar-entity-builder.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,14 @@ function makeGroup(id: string, isMain: boolean): GroupEntity {
return { id, isMain } as unknown as GroupEntity;
}

test('user entity has correct type, id, suspended=false, and group parents', (t) => {
test('user entity has correct type, id, suspended=false, and empty parents', (t) => {
const groups = [makeGroup('g1', false), makeGroup('g2', true)];
const entities = buildCedarEntities(userId, groups, connectionId);
const userEntity = entities.find((e) => e.uid.type === 'RocketAdmin::User');
t.truthy(userEntity);
t.is(userEntity.uid.id, userId);
t.is(userEntity.attrs.suspended, false);
t.is(userEntity.parents.length, 2);
t.deepEqual(userEntity.parents[0], { type: 'RocketAdmin::Group', id: 'g1' });
t.deepEqual(userEntity.parents[1], { type: 'RocketAdmin::Group', id: 'g2' });
t.deepEqual(userEntity.parents, []);
});

test('group entities have correct type, isMain attribute, connectionId attribute, empty parents', (t) => {
Expand Down
Loading
Loading