From e1cc0ccd3a55c5a9a0f9a2816ccf55265729221b Mon Sep 17 00:00:00 2001 From: gharden Date: Wed, 20 May 2026 16:01:43 -0400 Subject: [PATCH 1/2] fix(cost-management): register dynamic RBAC permissions for cluster/project tiers (FLPATH-4207) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cluster-specific permissions (ros/, ros//) were created at runtime but never registered with createPermissionIntegrationRouter. The RHDH RBAC backend only evaluates registered permissions — unregistered ones get DENY by default, breaking the entire 3-tier RBAC model. At router init, fetch cluster/project data from upstream APIs and register all dynamic permissions. Also improve secureProxy.ts error messages (include path and error details instead of opaque "Internal proxy error"). Co-authored-by: Cursor --- .../src/routes/secureProxy.ts | 11 ++- .../src/service/router.ts | 98 +++++++++++++++++++ 2 files changed, 107 insertions(+), 2 deletions(-) diff --git a/workspaces/cost-management/plugins/cost-management-backend/src/routes/secureProxy.ts b/workspaces/cost-management/plugins/cost-management-backend/src/routes/secureProxy.ts index 66ffaa2ad9..9ec75c5627 100644 --- a/workspaces/cost-management/plugins/cost-management-backend/src/routes/secureProxy.ts +++ b/workspaces/cost-management/plugins/cost-management-backend/src/routes/secureProxy.ts @@ -409,7 +409,12 @@ export const secureProxy: (options: RouterOptions) => RequestHandler = res.set('Content-Type', contentType); return res.send(await upstreamResponse.text()); } catch (error) { - options.logger.error('Secure proxy error', error); + const errorMessage = + error instanceof Error ? error.message : String(error); + options.logger.error( + `Secure proxy error for path "${proxyPath}": ${errorMessage}`, + error instanceof Error ? { stack: error.stack } : {}, + ); if (error instanceof SsoAuthenticationError) { return res.status(502).json({ @@ -419,6 +424,8 @@ export const secureProxy: (options: RouterOptions) => RequestHandler = }); } - return res.status(500).json({ error: 'Internal proxy error' }); + return res + .status(500) + .json({ error: `Internal proxy error: ${errorMessage}` }); } }; diff --git a/workspaces/cost-management/plugins/cost-management-backend/src/service/router.ts b/workspaces/cost-management/plugins/cost-management-backend/src/service/router.ts index e30e631d51..8b4da018af 100644 --- a/workspaces/cost-management/plugins/cost-management-backend/src/service/router.ts +++ b/workspaces/cost-management/plugins/cost-management-backend/src/service/router.ts @@ -18,26 +18,124 @@ import express from 'express'; import Router from 'express-promise-router'; import type { RouterOptions } from '../models/RouterOptions'; import { createPermissionIntegrationRouter } from '@backstage/plugin-permission-node'; +import type { BasicPermission } from '@backstage/plugin-permission-common'; import { rosPluginPermissions, rosApplyPermissions, costPluginPermissions, + rosClusterSpecificPermission, + rosClusterProjectPermission, + costClusterSpecificPermission, + costClusterProjectPermission, } from '@red-hat-developer-hub/plugin-cost-management-common/permissions'; import { getAccess } from '../routes/access'; import { getCostManagementAccess } from '../routes/costManagementAccess'; import { secureProxy } from '../routes/secureProxy'; import { applyRecommendation } from '../routes/applyRecommendation'; +import { getTokenFromApi } from '../util/tokenUtil'; + +/** + * Fetches cluster and project data from the upstream APIs and builds + * permission objects for every ros/{cluster}, ros/{cluster}/{project}, + * cost/{cluster}, and cost/{cluster}/{project} combination. + * + * These permissions must be registered with the permission integration + * router so that the RBAC backend recognises them as valid and evaluates + * the corresponding policies instead of returning DENY by default. + */ +async function fetchDynamicPermissions( + options: RouterOptions, +): Promise { + const { logger } = options; + const permissions: BasicPermission[] = []; + + try { + const token = await getTokenFromApi(options); + + const [rosData, costClusters, costProjects] = await Promise.allSettled([ + options.optimizationApi + .getRecommendationList( + { query: { limit: -1, orderHow: 'desc', orderBy: 'last_reported' } }, + { token }, + ) + .then(r => r.json()), + options.costManagementApi + .searchOpenShiftClusters('', { token, limit: 1000 }) + .then(r => r.json()), + options.costManagementApi + .searchOpenShiftProjects('', { token, limit: 1000 }) + .then(r => r.json()), + ]); + + const rosClusterNames = new Set(); + const rosProjects = new Set(); + + if (rosData.status === 'fulfilled' && rosData.value?.data) { + for (const rec of rosData.value.data) { + if (rec.clusterAlias) rosClusterNames.add(rec.clusterAlias); + if (rec.project) rosProjects.add(rec.project); + } + } + + for (const cluster of rosClusterNames) { + permissions.push(rosClusterSpecificPermission(cluster)); + for (const project of rosProjects) { + permissions.push(rosClusterProjectPermission(cluster, project)); + } + } + + const costClusterNames = new Set(); + const costProjectNames = new Set(); + + if (costClusters.status === 'fulfilled' && costClusters.value?.data) { + for (const c of costClusters.value.data as { + cluster_alias: string; + }[]) { + if (c.cluster_alias) costClusterNames.add(c.cluster_alias); + } + } + if (costProjects.status === 'fulfilled' && costProjects.value?.data) { + for (const p of costProjects.value.data as { value: string }[]) { + if (p.value) costProjectNames.add(p.value); + } + } + + for (const cluster of costClusterNames) { + permissions.push(costClusterSpecificPermission(cluster)); + for (const project of costProjectNames) { + permissions.push(costClusterProjectPermission(cluster, project)); + } + } + + logger.info( + `Registered ${permissions.length} dynamic RBAC permissions ` + + `(${rosClusterNames.size} ROS clusters, ${costClusterNames.size} cost clusters)`, + ); + } catch (error) { + logger.warn( + 'Could not fetch cluster/project data for dynamic permission registration. ' + + 'Cluster-specific RBAC permissions will not be evaluated until the next refresh.', + error instanceof Error ? { error: error.message } : {}, + ); + } + + return permissions; +} /** @public */ export async function createRouter( options: RouterOptions, ): Promise { const router = Router(); + + const dynamicPermissions = await fetchDynamicPermissions(options); + const permissionsIntegrationRouter = createPermissionIntegrationRouter({ permissions: [ ...rosPluginPermissions, ...rosApplyPermissions, ...costPluginPermissions, + ...dynamicPermissions, ], }); From e0b16271f4fb7cbdd52f5d940b6e821c9846b8fe Mon Sep 17 00:00:00 2001 From: gharden Date: Wed, 20 May 2026 16:16:18 -0400 Subject: [PATCH 2/2] refactor: reduce cognitive complexity and add unit tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Extract extractStrings() and buildClusterProjectPermissions() helpers to flatten nested if/for blocks (SonarCloud cognitive complexity 33→8) - Add 8 unit tests covering both helpers (fulfilled/rejected/empty/dedup) - Add changeset for cost-management-backend patch release Co-authored-by: Cursor --- .../fix-rbac-dynamic-permissions.md | 12 ++ .../src/service/router.test.ts | 106 +++++++++++++++++- .../src/service/router.ts | 101 ++++++++++------- 3 files changed, 177 insertions(+), 42 deletions(-) create mode 100644 workspaces/cost-management/.changeset/fix-rbac-dynamic-permissions.md diff --git a/workspaces/cost-management/.changeset/fix-rbac-dynamic-permissions.md b/workspaces/cost-management/.changeset/fix-rbac-dynamic-permissions.md new file mode 100644 index 0000000000..d9031a6cf4 --- /dev/null +++ b/workspaces/cost-management/.changeset/fix-rbac-dynamic-permissions.md @@ -0,0 +1,12 @@ +--- +'@red-hat-developer-hub/plugin-cost-management-backend': patch +--- + +fix: register dynamic RBAC permissions for cluster/project tiers (FLPATH-4207) + +Cluster-specific permissions (ros/, ros//) were created +at runtime but never registered with createPermissionIntegrationRouter. The RHDH +RBAC backend only evaluates registered permissions — unregistered ones get DENY by +default, breaking the 3-tier RBAC model. Now fetches cluster/project data at router +init and registers all dynamic permissions. Also improves secureProxy.ts error +messages to include request path and error details. diff --git a/workspaces/cost-management/plugins/cost-management-backend/src/service/router.test.ts b/workspaces/cost-management/plugins/cost-management-backend/src/service/router.test.ts index 08104462e1..d8474a8a29 100644 --- a/workspaces/cost-management/plugins/cost-management-backend/src/service/router.test.ts +++ b/workspaces/cost-management/plugins/cost-management-backend/src/service/router.test.ts @@ -18,7 +18,111 @@ import express from 'express'; import request from 'supertest'; import { mockServices } from '@backstage/backend-test-utils'; -import { createRouter } from './router'; +import { + createRouter, + extractStrings, + buildClusterProjectPermissions, +} from './router'; +import type { BasicPermission } from '@backstage/plugin-permission-common'; + +describe('extractStrings', () => { + it('returns values from a fulfilled result', () => { + const result: PromiseSettledResult<{ + data?: { name: string; alias?: string }[]; + }> = { + status: 'fulfilled', + value: { + data: [ + { name: 'a', alias: 'x' }, + { name: 'b' }, + { name: 'c', alias: 'y' }, + ], + }, + }; + const out = extractStrings(result, item => item.alias); + expect(out).toEqual(new Set(['x', 'y'])); + }); + + it('returns empty set for rejected result', () => { + const result: PromiseSettledResult<{ data?: unknown[] }> = { + status: 'rejected', + reason: new Error('fail'), + }; + expect(extractStrings(result, () => 'x')).toEqual(new Set()); + }); + + it('returns empty set when data is undefined', () => { + const result: PromiseSettledResult<{ data?: unknown[] }> = { + status: 'fulfilled', + value: {}, + }; + expect(extractStrings(result, () => 'x')).toEqual(new Set()); + }); + + it('deduplicates values', () => { + const result: PromiseSettledResult<{ data?: { v: string }[] }> = { + status: 'fulfilled', + value: { data: [{ v: 'a' }, { v: 'a' }, { v: 'b' }] }, + }; + expect(extractStrings(result, i => i.v)).toEqual(new Set(['a', 'b'])); + }); + + it('skips falsy values from accessor', () => { + const result: PromiseSettledResult<{ + data?: { v: string | undefined }[]; + }> = { + status: 'fulfilled', + value: { data: [{ v: undefined }, { v: '' }, { v: 'ok' }] }, + }; + expect(extractStrings(result, i => i.v)).toEqual(new Set(['ok'])); + }); +}); + +describe('buildClusterProjectPermissions', () => { + const mockClusterFn = (c: string) => + ({ name: `ros/${c}`, type: 'basic', attributes: {} } as BasicPermission); + const mockProjectFn = (c: string, p: string) => + ({ + name: `ros/${c}/${p}`, + type: 'basic', + attributes: {}, + } as BasicPermission); + + it('builds cluster + cluster/project combinations', () => { + const perms = buildClusterProjectPermissions( + new Set(['c1', 'c2']), + new Set(['p1']), + mockClusterFn, + mockProjectFn, + ); + expect(perms.map(p => p.name)).toEqual([ + 'ros/c1', + 'ros/c1/p1', + 'ros/c2', + 'ros/c2/p1', + ]); + }); + + it('returns only cluster perms when projects is empty', () => { + const perms = buildClusterProjectPermissions( + new Set(['c1']), + new Set(), + mockClusterFn, + mockProjectFn, + ); + expect(perms.map(p => p.name)).toEqual(['ros/c1']); + }); + + it('returns empty array when clusters is empty', () => { + const perms = buildClusterProjectPermissions( + new Set(), + new Set(['p1']), + mockClusterFn, + mockProjectFn, + ); + expect(perms).toEqual([]); + }); +}); describe('createRouter', () => { let app: express.Express; diff --git a/workspaces/cost-management/plugins/cost-management-backend/src/service/router.ts b/workspaces/cost-management/plugins/cost-management-backend/src/service/router.ts index 8b4da018af..15c4148b94 100644 --- a/workspaces/cost-management/plugins/cost-management-backend/src/service/router.ts +++ b/workspaces/cost-management/plugins/cost-management-backend/src/service/router.ts @@ -34,6 +34,39 @@ import { secureProxy } from '../routes/secureProxy'; import { applyRecommendation } from '../routes/applyRecommendation'; import { getTokenFromApi } from '../util/tokenUtil'; +/** @internal Visible for testing */ +export function extractStrings( + result: PromiseSettledResult<{ data?: T[] }>, + accessor: (item: T) => string | undefined, +): Set { + const values = new Set(); + if (result.status !== 'fulfilled' || !result.value?.data) { + return values; + } + for (const item of result.value.data) { + const v = accessor(item); + if (v) values.add(v); + } + return values; +} + +/** @internal Visible for testing */ +export function buildClusterProjectPermissions( + clusters: Set, + projects: Set, + clusterFn: (cluster: string) => BasicPermission, + projectFn: (cluster: string, project: string) => BasicPermission, +): BasicPermission[] { + const perms: BasicPermission[] = []; + for (const cluster of clusters) { + perms.push(clusterFn(cluster)); + for (const project of projects) { + perms.push(projectFn(cluster, project)); + } + } + return perms; +} + /** * Fetches cluster and project data from the upstream APIs and builds * permission objects for every ros/{cluster}, ros/{cluster}/{project}, @@ -47,7 +80,6 @@ async function fetchDynamicPermissions( options: RouterOptions, ): Promise { const { logger } = options; - const permissions: BasicPermission[] = []; try { const token = await getTokenFromApi(options); @@ -67,59 +99,46 @@ async function fetchDynamicPermissions( .then(r => r.json()), ]); - const rosClusterNames = new Set(); - const rosProjects = new Set(); - - if (rosData.status === 'fulfilled' && rosData.value?.data) { - for (const rec of rosData.value.data) { - if (rec.clusterAlias) rosClusterNames.add(rec.clusterAlias); - if (rec.project) rosProjects.add(rec.project); - } - } - - for (const cluster of rosClusterNames) { - permissions.push(rosClusterSpecificPermission(cluster)); - for (const project of rosProjects) { - permissions.push(rosClusterProjectPermission(cluster, project)); - } - } - - const costClusterNames = new Set(); - const costProjectNames = new Set(); - - if (costClusters.status === 'fulfilled' && costClusters.value?.data) { - for (const c of costClusters.value.data as { - cluster_alias: string; - }[]) { - if (c.cluster_alias) costClusterNames.add(c.cluster_alias); - } - } - if (costProjects.status === 'fulfilled' && costProjects.value?.data) { - for (const p of costProjects.value.data as { value: string }[]) { - if (p.value) costProjectNames.add(p.value); - } - } + const rosClusterNames = extractStrings(rosData, r => r.clusterAlias); + const rosProjectNames = extractStrings(rosData, r => r.project); + const costClusterNames = extractStrings( + costClusters, + (c: { cluster_alias: string }) => c.cluster_alias, + ); + const costProjectNames = extractStrings( + costProjects, + (p: { value: string }) => p.value, + ); - for (const cluster of costClusterNames) { - permissions.push(costClusterSpecificPermission(cluster)); - for (const project of costProjectNames) { - permissions.push(costClusterProjectPermission(cluster, project)); - } - } + const permissions = [ + ...buildClusterProjectPermissions( + rosClusterNames, + rosProjectNames, + rosClusterSpecificPermission, + rosClusterProjectPermission, + ), + ...buildClusterProjectPermissions( + costClusterNames, + costProjectNames, + costClusterSpecificPermission, + costClusterProjectPermission, + ), + ]; logger.info( `Registered ${permissions.length} dynamic RBAC permissions ` + `(${rosClusterNames.size} ROS clusters, ${costClusterNames.size} cost clusters)`, ); + + return permissions; } catch (error) { logger.warn( 'Could not fetch cluster/project data for dynamic permission registration. ' + 'Cluster-specific RBAC permissions will not be evaluated until the next refresh.', error instanceof Error ? { error: error.message } : {}, ); + return []; } - - return permissions; } /** @public */