Skip to content

Commit b6c4670

Browse files
guguclaude
gugu
and
claude
committed
refactor: simplify cedar policy code — remove dead code, fix leaks, deduplicate
- Delete unused cedar-policy-generator.ts - Remove unused policyItemsToPermissions function - Merge duplicate extractTableName/extractDashboardId into extractResourceSuffix - Extract buildResourceRef helper for table/dashboard resource refs - Extract _parseCedarToPolicyItems private method (was duplicated) - Remove pointless forkJoin wrapping single observable - Fix subscription leak: _editorService.loaded now uses take(1) - Add takeUntilDestroyed to fetchTables and saveCedarPolicy subscriptions - Combine duplicate table array iteration into single loop - Remove no-op map((res) => res) from saveCedarPolicy - Fix deprecated positional .subscribe() to object form Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 9e18c17 commit b6c4670

5 files changed

Lines changed: 62 additions & 214 deletions

File tree

‎frontend/src/app/components/users/cedar-policy-editor-dialog/cedar-policy-editor-dialog.component.ts‎

Lines changed: 52 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,15 @@
11
import { NgIf } from '@angular/common';
2-
import { Component, Inject, OnInit } from '@angular/core';
2+
import { Component, DestroyRef, Inject, inject, OnInit } from '@angular/core';
3+
import { takeUntilDestroyed } from '@angular/core/rxjs-interop';
34
import { MatButtonModule } from '@angular/material/button';
45
import { MatButtonToggleModule } from '@angular/material/button-toggle';
56
import { MAT_DIALOG_DATA, MatDialogModule, MatDialogRef } from '@angular/material/dialog';
67
import { CodeEditorModule, CodeEditorService } from '@ngstack/code-editor';
7-
import { forkJoin } from 'rxjs';
8+
import { take } from 'rxjs';
89
import { registerCedarLanguage } from 'src/app/lib/cedar-monaco-language';
910
import { CedarPolicyItem, permissionsToPolicyItems, policyItemsToCedarPolicy } from 'src/app/lib/cedar-policy-items';
1011
import { parseCedarDashboardItems, parseCedarPolicy } from 'src/app/lib/cedar-policy-parser';
1112
import { normalizeTableName } from 'src/app/lib/normalize';
12-
import { Dashboard } from 'src/app/models/dashboard';
1313
import { TablePermission } from 'src/app/models/user';
1414
import { ConnectionsService } from 'src/app/services/connections.service';
1515
import { DashboardsService } from 'src/app/services/dashboards.service';
@@ -56,6 +56,8 @@ export class CedarPolicyEditorDialogComponent implements OnInit {
5656
};
5757
public codeEditorTheme: string;
5858

59+
private _destroyRef = inject(DestroyRef);
60+
5961
constructor(
6062
@Inject(MAT_DIALOG_DATA) public data: CedarPolicyEditorDialogData,
6163
public dialogRef: MatDialogRef<Self>,
@@ -67,7 +69,7 @@ export class CedarPolicyEditorDialogComponent implements OnInit {
6769
private _editorService: CodeEditorService,
6870
) {
6971
this.codeEditorTheme = this._uiSettings.isDarkMode ? 'vs-dark' : 'vs';
70-
this._editorService.loaded.subscribe(({ monaco }) => registerCedarLanguage(monaco));
72+
this._editorService.loaded.pipe(take(1)).subscribe(({ monaco }) => registerCedarLanguage(monaco));
7173
}
7274

7375
ngOnInit(): void {
@@ -81,36 +83,33 @@ export class CedarPolicyEditorDialogComponent implements OnInit {
8183

8284
this._dashboardsService.setActiveConnection(this.connectionID);
8385

84-
forkJoin([this._tablesService.fetchTables(this.connectionID)]).subscribe(([tables]) => {
85-
this.allTables = tables.map((t) => ({
86-
tableName: t.table,
87-
display_name: t.display_name || normalizeTableName(t.table),
88-
accessLevel: {
89-
visibility: false,
90-
readonly: false,
91-
add: false,
92-
delete: false,
93-
edit: false,
94-
},
95-
}));
96-
this.availableTables = tables.map((t) => ({
97-
tableName: t.table,
98-
displayName: t.display_name || normalizeTableName(t.table),
99-
}));
100-
101-
this.availableDashboards = this._dashboardsService.dashboards().map((d: Dashboard) => ({
102-
id: d.id,
103-
name: d.name,
104-
}));
105-
106-
this.loading = false;
107-
108-
if (this.cedarPolicy) {
109-
const parsed = parseCedarPolicy(this.cedarPolicy, this.connectionID, this.data.groupId, this.allTables);
110-
const dashboardItems = parseCedarDashboardItems(this.cedarPolicy, this.connectionID);
111-
this.policyItems = [...permissionsToPolicyItems(parsed), ...dashboardItems];
112-
}
113-
});
86+
this._tablesService
87+
.fetchTables(this.connectionID)
88+
.pipe(takeUntilDestroyed(this._destroyRef))
89+
.subscribe((tables) => {
90+
this.allTables = [];
91+
this.availableTables = [];
92+
for (const t of tables) {
93+
const displayName = t.display_name || normalizeTableName(t.table);
94+
this.allTables.push({
95+
tableName: t.table,
96+
display_name: displayName,
97+
accessLevel: { visibility: false, readonly: false, add: false, delete: false, edit: false },
98+
});
99+
this.availableTables.push({ tableName: t.table, displayName });
100+
}
101+
102+
this.availableDashboards = this._dashboardsService.dashboards().map((d) => ({
103+
id: d.id,
104+
name: d.name,
105+
}));
106+
107+
this.loading = false;
108+
109+
if (this.cedarPolicy) {
110+
this.policyItems = this._parseCedarToPolicyItems();
111+
}
112+
});
114113
}
115114

116115
onCedarPolicyChange(value: string) {
@@ -132,9 +131,7 @@ export class CedarPolicyEditorDialogComponent implements OnInit {
132131
value: this.cedarPolicy,
133132
};
134133
} else {
135-
const parsed = parseCedarPolicy(this.cedarPolicy, this.connectionID, this.data.groupId, this.allTables);
136-
const dashboardItems = parseCedarDashboardItems(this.cedarPolicy, this.connectionID);
137-
this.policyItems = [...permissionsToPolicyItems(parsed), ...dashboardItems];
134+
this.policyItems = this._parseCedarToPolicyItems();
138135
}
139136

140137
this.editorMode = mode;
@@ -153,15 +150,23 @@ export class CedarPolicyEditorDialogComponent implements OnInit {
153150
return;
154151
}
155152

156-
this._usersService.saveCedarPolicy(this.connectionID, this.data.groupId, this.cedarPolicy).subscribe(
157-
() => {
158-
this.submitting = false;
159-
this.dialogRef.close();
160-
},
161-
() => {},
162-
() => {
163-
this.submitting = false;
164-
},
165-
);
153+
this._usersService
154+
.saveCedarPolicy(this.connectionID, this.data.groupId, this.cedarPolicy)
155+
.pipe(takeUntilDestroyed(this._destroyRef))
156+
.subscribe({
157+
next: () => {
158+
this.submitting = false;
159+
this.dialogRef.close();
160+
},
161+
complete: () => {
162+
this.submitting = false;
163+
},
164+
});
165+
}
166+
167+
private _parseCedarToPolicyItems(): CedarPolicyItem[] {
168+
const parsed = parseCedarPolicy(this.cedarPolicy, this.connectionID, this.data.groupId, this.allTables);
169+
const dashboardItems = parseCedarDashboardItems(this.cedarPolicy, this.connectionID);
170+
return [...permissionsToPolicyItems(parsed), ...dashboardItems];
166171
}
167172
}

‎frontend/src/app/lib/cedar-policy-generator.ts‎

Lines changed: 0 additions & 65 deletions
This file was deleted.

‎frontend/src/app/lib/cedar-policy-items.ts‎

Lines changed: 7 additions & 89 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { AccessLevel, Permissions, TablePermission } from '../models/user';
1+
import { AccessLevel, Permissions } from '../models/user';
22

33
export interface CedarPolicyItem {
44
action: string;
@@ -139,17 +139,9 @@ export function policyItemsToCedarPolicy(items: CedarPolicyItem[], connectionId:
139139
let resource: string;
140140

141141
if (item.action.startsWith('table:')) {
142-
if (item.tableName === '*') {
143-
resource = `resource like RocketAdmin::Table::"${connectionId}/*"`;
144-
} else {
145-
resource = `resource == RocketAdmin::Table::"${connectionId}/${item.tableName}"`;
146-
}
142+
resource = buildResourceRef('Table', connectionId, item.tableName);
147143
} else if (item.action.startsWith('dashboard:')) {
148-
if (item.dashboardId === '*') {
149-
resource = `resource like RocketAdmin::Dashboard::"${connectionId}/*"`;
150-
} else {
151-
resource = `resource == RocketAdmin::Dashboard::"${connectionId}/${item.dashboardId}"`;
152-
}
144+
resource = buildResourceRef('Dashboard', connectionId, item.dashboardId);
153145
} else if (item.action.startsWith('group:')) {
154146
resource = `resource == RocketAdmin::Group::"${groupId}"`;
155147
} else {
@@ -162,83 +154,9 @@ export function policyItemsToCedarPolicy(items: CedarPolicyItem[], connectionId:
162154
return policies.join('\n\n');
163155
}
164156

165-
export function policyItemsToPermissions(
166-
items: CedarPolicyItem[],
167-
connectionId: string,
168-
groupId: string,
169-
availableTables: TablePermission[],
170-
): Permissions {
171-
const result: Permissions = {
172-
connection: { connectionId, accessLevel: AccessLevel.None },
173-
group: { groupId, accessLevel: AccessLevel.None },
174-
tables: availableTables.map((t) => ({
175-
...t,
176-
accessLevel: { visibility: false, readonly: false, add: false, delete: false, edit: false },
177-
})),
178-
};
179-
180-
for (const item of items) {
181-
if (item.action === '*') {
182-
result.connection.accessLevel = AccessLevel.Edit;
183-
result.group.accessLevel = AccessLevel.Edit;
184-
result.tables.forEach((t) => {
185-
t.accessLevel = { visibility: true, readonly: false, add: true, delete: true, edit: true };
186-
});
187-
return result;
188-
}
189-
190-
switch (item.action) {
191-
case 'connection:read':
192-
if (result.connection.accessLevel === AccessLevel.None) {
193-
result.connection.accessLevel = AccessLevel.Readonly;
194-
}
195-
break;
196-
case 'connection:edit':
197-
result.connection.accessLevel = AccessLevel.Edit;
198-
break;
199-
case 'group:read':
200-
if (result.group.accessLevel === AccessLevel.None) {
201-
result.group.accessLevel = AccessLevel.Readonly;
202-
}
203-
break;
204-
case 'group:edit':
205-
result.group.accessLevel = AccessLevel.Edit;
206-
break;
207-
case 'table:*':
208-
case 'table:read':
209-
case 'table:add':
210-
case 'table:edit':
211-
case 'table:delete': {
212-
if (!item.tableName) break;
213-
const targets =
214-
item.tableName === '*' ? result.tables : result.tables.filter((t) => t.tableName === item.tableName);
215-
for (const table of targets) {
216-
table.accessLevel.visibility = true;
217-
switch (item.action) {
218-
case 'table:*':
219-
table.accessLevel.readonly = true;
220-
table.accessLevel.add = true;
221-
table.accessLevel.edit = true;
222-
table.accessLevel.delete = true;
223-
break;
224-
case 'table:read':
225-
table.accessLevel.readonly = true;
226-
break;
227-
case 'table:add':
228-
table.accessLevel.add = true;
229-
break;
230-
case 'table:edit':
231-
table.accessLevel.edit = true;
232-
break;
233-
case 'table:delete':
234-
table.accessLevel.delete = true;
235-
break;
236-
}
237-
}
238-
break;
239-
}
240-
}
157+
function buildResourceRef(type: string, connectionId: string, id: string | undefined): string {
158+
if (id === '*') {
159+
return `resource like RocketAdmin::${type}::"${connectionId}/*"`;
241160
}
242-
243-
return result;
161+
return `resource == RocketAdmin::${type}::"${connectionId}/${id}"`;
244162
}

‎frontend/src/app/lib/cedar-policy-parser.ts‎

Lines changed: 3 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ export function parseCedarPolicy(
5757
case 'table:add':
5858
case 'table:edit':
5959
case 'table:delete': {
60-
const tableName = extractTableName(permit.resourceId, connectionId);
60+
const tableName = extractResourceSuffix(permit.resourceId, connectionId);
6161
if (!tableName) break;
6262
const tableEntry = getOrCreateTableEntry(tableMap, tableName);
6363
applyTableAction(tableEntry, permit.action);
@@ -125,7 +125,7 @@ export function parseCedarDashboardItems(policyText: string, connectionId: strin
125125

126126
for (const permit of permits) {
127127
if (!permit.action || !permit.action.startsWith('dashboard:')) continue;
128-
const dashboardId = extractDashboardId(permit.resourceId, connectionId);
128+
const dashboardId = extractResourceSuffix(permit.resourceId, connectionId);
129129
if (dashboardId) {
130130
items.push({ action: permit.action, dashboardId });
131131
}
@@ -134,15 +134,6 @@ export function parseCedarDashboardItems(policyText: string, connectionId: strin
134134
return items;
135135
}
136136

137-
function extractDashboardId(resourceId: string | null, connectionId: string): string | null {
138-
if (!resourceId) return null;
139-
const prefix = `${connectionId}/`;
140-
if (resourceId.startsWith(prefix)) {
141-
return resourceId.slice(prefix.length);
142-
}
143-
return resourceId;
144-
}
145-
146137
function extractPermitStatements(policyText: string): ParsedPermitStatement[] {
147138
const results: ParsedPermitStatement[] = [];
148139
const permitKeyword = 'permit';
@@ -231,7 +222,7 @@ function parsePermitBody(body: string): ParsedPermitStatement {
231222
return result;
232223
}
233224

234-
function extractTableName(resourceId: string | null, connectionId: string): string | null {
225+
function extractResourceSuffix(resourceId: string | null, connectionId: string): string | null {
235226
if (!resourceId) return null;
236227
const prefix = `${connectionId}/`;
237228
if (resourceId.startsWith(prefix)) {

‎frontend/src/app/services/users.service.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,6 @@ export class UsersService {
6767

6868
saveCedarPolicy(connectionID: string, groupId: string, cedarPolicy: string) {
6969
return this._http.post<any>(`/connection/cedar-policy/${connectionID}`, { cedarPolicy, groupId }).pipe(
70-
map((res) => res),
7170
catchError((err) => {
7271
console.log(err);
7372
this._notifications.showErrorSnackbar(err.error?.message || err.message);

0 commit comments

Comments
 (0)