Skip to content

Commit ce8cb07

Browse files
hardenglclaude
andauthored
fix(cost-management): add authorization, validation, and confirmation for Apply Recommendation (#2618)
* fix(cost-management): add authorization, validation, and confirmation for Apply Recommendation - New ros.apply permission required to execute Apply Recommendation workflow - New POST /apply-recommendation backend endpoint validates resourceType against server-side allowlist and checks ros.apply permission before forwarding to Orchestrator - Workflow execution now routes through the cost-management backend instead of directly to the Orchestrator plugin, enabling server-side authorization and audit logging - Confirmation dialog prevents accidental workflow execution - Register costPluginPermissions in permission integration router (was previously missing) Fixes: FLPATH-3488, FLPATH-3492, FLPATH-3491 Made-with: Cursor * fix(cost-management): regenerate API reports to match CI expectations Run yarn build:api-reports:only to update report-clients.api.md and report.api.md with the correct auto-generated format. Made-with: Cursor * fix(cost-management): address Qodo review findings - Wrap decodeURIComponent in try/catch, return 400 on malformed encoding - Safe JSON parse in applyRecommendation (check content-type first) - Change router.all('/proxy/*') to router.get (proxy is read-only) - Delete unused routes/token.ts Made-with: Cursor * fix(cost-management): sanitize inputData before Orchestrator forwarding Construct a clean inputData object with only known fields before forwarding to the Orchestrator, preventing extra injected fields from passing through to the workflow execution. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent ab26a80 commit ce8cb07

15 files changed

Lines changed: 603 additions & 118 deletions

File tree

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
---
2+
'@red-hat-developer-hub/plugin-cost-management-backend': minor
3+
'@red-hat-developer-hub/plugin-cost-management-common': minor
4+
'@red-hat-developer-hub/plugin-cost-management': minor
5+
---
6+
7+
Add authorization, input validation, and confirmation dialog for Apply Recommendation workflow.
8+
9+
- New `ros.apply` permission required to execute the Apply Recommendation workflow
10+
- New backend `POST /apply-recommendation` endpoint validates `resourceType` against server-side allowlist and checks `ros.apply` permission before forwarding to Orchestrator
11+
- Workflow execution now routes through the cost-management backend instead of directly to the Orchestrator plugin, enabling server-side authorization and audit logging
12+
- Confirmation dialog added before workflow execution to prevent accidental clicks

workspaces/cost-management/docs/rbac.md

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,17 @@ When a frontend request arrives at `/api/cost-management/proxy/*`, the backend:
1818

1919
This means granting `ros.demolab` only allows seeing data for the `demolab` cluster — the user cannot modify query parameters to access other clusters.
2020

21+
### Apply Recommendation authorization
22+
23+
When a user clicks "Apply recommendation", the frontend sends the request to the backend's `/api/cost-management/apply-recommendation` endpoint. The backend:
24+
25+
1. Validates the `resourceType` against a server-side allowlist (`deployment`, `replicaset`, `daemonset`, `statefulset`, `deploymentconfig`, `replicationcontroller`)
26+
2. Checks the `ros.apply` permission — the user must be explicitly granted this permission to execute workflows
27+
3. Forwards the validated request to the Orchestrator plugin using service-to-service authentication
28+
4. Audit logs the action (user, cluster, namespace, workload, workflow ID, outcome)
29+
30+
A confirmation dialog on the frontend also prevents accidental clicks.
31+
2132
## 1. Optimizations Section
2233

2334
The Optimizations section allows users to view resource usage trends and optimization recommendations for workloads running on OpenShift clusters.
@@ -29,6 +40,7 @@ The Optimizations section allows users to view resource usage trends and optimiz
2940
| ros.plugin | - | read | Allows the user to access all optimization data in the Cost Management plugin |
3041
| ros.[CLUSTER_NAME] | - | read | Allows the user to access optimization data for a specific Cluster in the Cost Management plugin |
3142
| ros.[CLUSTER_NAME].[PROJECT_NAME] | - | read | Allows the user to access optimization data for a specific Project within a specific Cluster in the Cost Management plugin |
43+
| ros.apply | - | update | Allows the user to apply optimization recommendations via workflow execution |
3244

3345
The user is permitted to do an action if either the generic permission or the specific one allows it. In other words, it is not possible to grant generic ros.plugin and then selectively disable it for a specific cluster via ros.[CLUSTER_NAME] with deny.
3446

@@ -70,6 +82,11 @@ p, role:default/rosUser, ros.demolab, read, allow
7082
p, role:default/rosUser, ros.demolab.thanos, read, allow
7183
p, role:default/rosUser, ros.OpenShift on Azure.mobile, read, allow
7284
85+
####
86+
# Optimizations Section (ros.) - Apply Recommendation permission
87+
####
88+
p, role:default/rosUser, ros.apply, update, allow
89+
7390
####
7491
# OpenShift Section (cost.) - Generic permissions
7592
####
@@ -131,6 +148,20 @@ p, role:default/rosClusterProjectUser, ros.demolab.thanos, read, allow
131148
g, user:default/test_user_3, role:default/rosClusterProjectUser
132149
```
133150

151+
#### ros.apply Permission
152+
153+
Since the `test_user_7` user has the `default/rosApplyUser` role, which has `ros.apply` permission, it can:
154+
155+
- Execute the Apply Recommendation workflow to modify workload resource configurations
156+
157+
```csv
158+
p, role:default/rosApplyUser, ros.apply, update, allow
159+
160+
g, user:default/test_user_7, role:default/rosApplyUser
161+
```
162+
163+
> **Note:** `ros.apply` is separate from the read permissions. A user can have read access to optimization data without being able to apply recommendations, and vice versa. Typically both `ros.plugin` (or cluster-specific read) and `ros.apply` are granted together.
164+
134165
### OpenShift Cost Section
135166

136167
#### cost.plugin Permission

workspaces/cost-management/plugins/cost-management-backend/src/models/RouterOptions.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@ import type {
2424
HttpAuthService,
2525
PermissionsService,
2626
CacheService,
27+
DiscoveryService,
28+
AuthService,
2729
} from '@backstage/backend-plugin-api';
2830

2931
/** @public */
@@ -33,6 +35,8 @@ export interface RouterOptions {
3335
httpAuth: HttpAuthService;
3436
permissions: PermissionsService;
3537
cache: CacheService;
38+
discovery: DiscoveryService;
39+
auth: AuthService;
3640
optimizationApi: OptimizationsApi;
3741
costManagementApi: CostManagementSlimApi;
3842
}

workspaces/cost-management/plugins/cost-management-backend/src/plugin.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,8 @@ export const costManagementPlugin = createBackendPlugin({
3838
httpAuth: coreServices.httpAuth,
3939
permissions: coreServices.permissions,
4040
cache: coreServices.cache,
41+
discovery: coreServices.discovery,
42+
auth: coreServices.auth,
4143
optimizationApi: optimizationServiceRef,
4244
costManagementApi: costManagementServiceRef,
4345
},
@@ -48,6 +50,8 @@ export const costManagementPlugin = createBackendPlugin({
4850
httpAuth,
4951
permissions,
5052
cache,
53+
discovery,
54+
auth,
5155
optimizationApi,
5256
costManagementApi,
5357
}) {
@@ -57,6 +61,8 @@ export const costManagementPlugin = createBackendPlugin({
5761
httpAuth,
5862
permissions,
5963
cache,
64+
discovery,
65+
auth,
6066
optimizationApi,
6167
costManagementApi,
6268
});
@@ -78,6 +84,10 @@ export const costManagementPlugin = createBackendPlugin({
7884
path: '/proxy',
7985
allow: 'user-cookie',
8086
});
87+
httpRouter.addAuthPolicy({
88+
path: '/apply-recommendation',
89+
allow: 'user-cookie',
90+
});
8191
},
8292
});
8393
},
Lines changed: 211 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,211 @@
1+
/*
2+
* Copyright Red Hat, Inc.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
import express from 'express';
18+
import request from 'supertest';
19+
import { mockServices } from '@backstage/backend-test-utils';
20+
import { AuthorizeResult } from '@backstage/plugin-permission-common';
21+
import { applyRecommendation } from './applyRecommendation';
22+
import type { RouterOptions } from '../models/RouterOptions';
23+
24+
const validBody = {
25+
workflowId: 'patch-k8s-resource',
26+
inputData: {
27+
clusterName: 'test-cluster',
28+
resourceType: 'deployment',
29+
resourceNamespace: 'default',
30+
resourceName: 'my-app',
31+
containerName: 'main',
32+
containerResources: {
33+
limits: { cpu: 0.5, memory: 134217728 },
34+
requests: { cpu: 0.25, memory: 67108864 },
35+
},
36+
},
37+
};
38+
39+
describe('applyRecommendation', () => {
40+
let app: express.Express;
41+
let mockPermissions: ReturnType<typeof mockServices.permissions.mock>;
42+
let mockDiscovery: ReturnType<typeof mockServices.discovery>;
43+
44+
beforeEach(() => {
45+
jest.resetAllMocks();
46+
mockPermissions = mockServices.permissions.mock();
47+
mockDiscovery = mockServices.discovery();
48+
49+
const options: RouterOptions = {
50+
logger: mockServices.rootLogger(),
51+
httpAuth: mockServices.httpAuth(),
52+
permissions: mockPermissions,
53+
cache: mockServices.cache.mock(),
54+
discovery: mockDiscovery,
55+
auth: mockServices.auth(),
56+
optimizationApi: {
57+
getRecommendationList: jest.fn(),
58+
getRecommendationById: jest.fn(),
59+
},
60+
costManagementApi: {
61+
getCostManagementReport: jest.fn(),
62+
downloadCostManagementReport: jest.fn(),
63+
searchOpenShiftProjects: jest.fn(),
64+
searchOpenShiftClusters: jest.fn(),
65+
searchOpenShiftNodes: jest.fn(),
66+
getOpenShiftTags: jest.fn(),
67+
getOpenShiftTagValues: jest.fn(),
68+
},
69+
};
70+
71+
app = express();
72+
app.use(express.json());
73+
app.post('/apply-recommendation', applyRecommendation(options));
74+
});
75+
76+
it('returns 400 when workflowId is missing', async () => {
77+
const response = await request(app)
78+
.post('/apply-recommendation')
79+
.send({ inputData: validBody.inputData });
80+
81+
expect(response.status).toBe(400);
82+
expect(response.body.error).toContain('workflowId');
83+
});
84+
85+
it('returns 400 for invalid resourceType', async () => {
86+
mockPermissions.authorize.mockResolvedValueOnce([
87+
{ result: AuthorizeResult.ALLOW },
88+
]);
89+
90+
const response = await request(app)
91+
.post('/apply-recommendation')
92+
.send({
93+
...validBody,
94+
inputData: { ...validBody.inputData, resourceType: 'cronjob' },
95+
});
96+
97+
expect(response.status).toBe(400);
98+
expect(response.body.error).toContain('Invalid resourceType');
99+
expect(response.body.error).toContain('cronjob');
100+
});
101+
102+
it('returns 400 when required fields are missing', async () => {
103+
const response = await request(app)
104+
.post('/apply-recommendation')
105+
.send({
106+
workflowId: 'patch-k8s-resource',
107+
inputData: {
108+
resourceType: 'deployment',
109+
containerResources: {},
110+
},
111+
});
112+
113+
expect(response.status).toBe(400);
114+
expect(response.body.error).toContain('Missing or invalid');
115+
});
116+
117+
it('returns 403 when ros.apply permission is denied', async () => {
118+
mockPermissions.authorize.mockResolvedValueOnce([
119+
{ result: AuthorizeResult.DENY },
120+
]);
121+
122+
const response = await request(app)
123+
.post('/apply-recommendation')
124+
.send(validBody);
125+
126+
expect(response.status).toBe(403);
127+
expect(response.body.error).toContain('ros.apply');
128+
});
129+
130+
it('validates all allowed resourceType values', async () => {
131+
const allowedTypes = [
132+
'deployment',
133+
'replicaset',
134+
'daemonset',
135+
'statefulset',
136+
'deploymentconfig',
137+
'replicationcontroller',
138+
];
139+
140+
for (const resourceType of allowedTypes) {
141+
mockPermissions.authorize.mockResolvedValueOnce([
142+
{ result: AuthorizeResult.ALLOW },
143+
]);
144+
145+
// eslint-disable-next-line no-restricted-syntax
146+
const fetchSpy = jest
147+
.spyOn(globalThis, 'fetch')
148+
.mockResolvedValueOnce(
149+
new Response(JSON.stringify({ id: 'instance-1' }), { status: 200 }),
150+
);
151+
152+
const response = await request(app)
153+
.post('/apply-recommendation')
154+
.send({
155+
...validBody,
156+
inputData: { ...validBody.inputData, resourceType },
157+
});
158+
159+
expect(response.status).toBe(200);
160+
fetchSpy.mockRestore();
161+
}
162+
});
163+
164+
it('forwards to orchestrator and returns instance id on success', async () => {
165+
mockPermissions.authorize.mockResolvedValueOnce([
166+
{ result: AuthorizeResult.ALLOW },
167+
]);
168+
169+
const fetchSpy = jest.spyOn(globalThis, 'fetch').mockResolvedValueOnce(
170+
new Response(JSON.stringify({ id: 'workflow-instance-123' }), {
171+
status: 200,
172+
headers: { 'Content-Type': 'application/json' },
173+
}),
174+
);
175+
176+
const response = await request(app)
177+
.post('/apply-recommendation')
178+
.send(validBody);
179+
180+
expect(response.status).toBe(200);
181+
expect(response.body).toEqual({ id: 'workflow-instance-123' });
182+
expect(fetchSpy).toHaveBeenCalledTimes(1);
183+
184+
const fetchUrl = fetchSpy.mock.calls[0][0] as string;
185+
expect(fetchUrl).toContain('/v2/workflows/patch-k8s-resource/execute');
186+
187+
fetchSpy.mockRestore();
188+
});
189+
190+
it('returns upstream error status on orchestrator failure', async () => {
191+
mockPermissions.authorize.mockResolvedValueOnce([
192+
{ result: AuthorizeResult.ALLOW },
193+
]);
194+
195+
const fetchSpy = jest.spyOn(globalThis, 'fetch').mockResolvedValueOnce(
196+
new Response(JSON.stringify({ error: 'Workflow not found' }), {
197+
status: 404,
198+
headers: { 'Content-Type': 'application/json' },
199+
}),
200+
);
201+
202+
const response = await request(app)
203+
.post('/apply-recommendation')
204+
.send(validBody);
205+
206+
expect(response.status).toBe(404);
207+
expect(response.body).toEqual({ error: 'Workflow not found' });
208+
209+
fetchSpy.mockRestore();
210+
});
211+
});

0 commit comments

Comments
 (0)