From 25166f63f5172dc22dd731be31733280a487cf98 Mon Sep 17 00:00:00 2001 From: Oleksandr Andriienko Date: Wed, 16 Oct 2024 17:17:59 +0300 Subject: [PATCH] fix(rbac): make working rbac-backend with newer APIs (#2359) * fix(rbac): make working rbac-backend with newer APIs Signed-off-by: Oleksandr Andriienko * fit(rbac): fix unit tests Signed-off-by: Oleksandr Andriienko * fix(rbac): fix lint Signed-off-by: Oleksandr Andriienko * fix(orchestrator): fix lint Signed-off-by: Oleksandr Andriienko * fix(rbac): update error type, when httpAuth is not availiable Signed-off-by: Oleksandr Andriienko --------- Signed-off-by: Oleksandr Andriienko --- .../src/api/OrchestratorClient.test.ts | 2 +- .../WorkflowEditor/WorkflowEditor.tsx | 5 +- .../channel/WorkflowEditorLanguageService.ts | 2 +- ...flowEditorLanguageServiceChannelApiImpl.ts | 4 +- .../src/service/policies-rest-api.test.ts | 43 ++++++----- .../src/service/policies-rest-api.ts | 71 +++++++++---------- 6 files changed, 61 insertions(+), 66 deletions(-) diff --git a/plugins/orchestrator/src/api/OrchestratorClient.test.ts b/plugins/orchestrator/src/api/OrchestratorClient.test.ts index 789ae26416..eb418434e0 100644 --- a/plugins/orchestrator/src/api/OrchestratorClient.test.ts +++ b/plugins/orchestrator/src/api/OrchestratorClient.test.ts @@ -1,5 +1,5 @@ import { DiscoveryApi, IdentityApi } from '@backstage/core-plugin-api'; -import { JsonObject } from '@backstage/types'; +import type { JsonObject } from '@backstage/types'; import axios, { AxiosRequestConfig, diff --git a/plugins/orchestrator/src/components/WorkflowEditor/WorkflowEditor.tsx b/plugins/orchestrator/src/components/WorkflowEditor/WorkflowEditor.tsx index 86a2cdfbca..dd537fe898 100644 --- a/plugins/orchestrator/src/components/WorkflowEditor/WorkflowEditor.tsx +++ b/plugins/orchestrator/src/components/WorkflowEditor/WorkflowEditor.tsx @@ -34,7 +34,10 @@ import { useCancelableEffect } from '@kie-tools-core/react-hooks/dist/useCancela import { editorDisplayOptions } from '@kie-tools/serverless-workflow-combined-editor/dist/api'; import { SwfCombinedEditorChannelApiImpl } from '@kie-tools/serverless-workflow-combined-editor/dist/channel/SwfCombinedEditorChannelApiImpl'; import { SwfPreviewOptionsChannelApiImpl } from '@kie-tools/serverless-workflow-combined-editor/dist/channel/SwfPreviewOptionsChannelApiImpl'; -import { Diagnostic, DiagnosticSeverity } from 'vscode-languageserver-types'; +import { + DiagnosticSeverity, + type Diagnostic, +} from 'vscode-languageserver-types'; import { extractWorkflowFormat, diff --git a/plugins/orchestrator/src/components/WorkflowEditor/channel/WorkflowEditorLanguageService.ts b/plugins/orchestrator/src/components/WorkflowEditor/channel/WorkflowEditorLanguageService.ts index 54282d4a5a..22255048fb 100644 --- a/plugins/orchestrator/src/components/WorkflowEditor/channel/WorkflowEditorLanguageService.ts +++ b/plugins/orchestrator/src/components/WorkflowEditor/channel/WorkflowEditorLanguageService.ts @@ -4,8 +4,8 @@ import { } from '@kie-tools/serverless-workflow-language-service/dist/api'; import { SwfJsonLanguageService, - SwfLanguageServiceArgs, SwfYamlLanguageService, + type SwfLanguageServiceArgs, } from '@kie-tools/serverless-workflow-language-service/dist/channel'; import { SwfServiceCatalogService } from '@kie-tools/serverless-workflow-service-catalog/dist/api'; diff --git a/plugins/orchestrator/src/components/WorkflowEditor/channel/WorkflowEditorLanguageServiceChannelApiImpl.ts b/plugins/orchestrator/src/components/WorkflowEditor/channel/WorkflowEditorLanguageServiceChannelApiImpl.ts index 517334635e..fb232c0364 100644 --- a/plugins/orchestrator/src/components/WorkflowEditor/channel/WorkflowEditorLanguageServiceChannelApiImpl.ts +++ b/plugins/orchestrator/src/components/WorkflowEditor/channel/WorkflowEditorLanguageServiceChannelApiImpl.ts @@ -1,9 +1,9 @@ import { SwfLanguageServiceChannelApi } from '@kie-tools/serverless-workflow-language-service/dist/api'; -import { +import type { SwfJsonLanguageService, SwfYamlLanguageService, } from '@kie-tools/serverless-workflow-language-service/dist/channel'; -import { +import type { CodeLens, CompletionItem, Position, diff --git a/plugins/rbac-backend/src/service/policies-rest-api.test.ts b/plugins/rbac-backend/src/service/policies-rest-api.test.ts index c665a326b5..2bcfb698dd 100644 --- a/plugins/rbac-backend/src/service/policies-rest-api.test.ts +++ b/plugins/rbac-backend/src/service/policies-rest-api.test.ts @@ -120,9 +120,12 @@ const auditLoggerMock = { auditLog: jest.fn().mockImplementation(() => Promise.resolve()), }; -const mockHttpAuth = mockServices.httpAuth(); +const mockHttpAuth = mockServices.httpAuth({ + pluginId: 'permission', + defaultCredentials: mockCredentials.user('user:default/guest'), +}); const mockAuth = mockServices.auth(); -const credentials = mockCredentials.user(); +const credentials = mockCredentials.user('user:default/guest'); const conditions: RoleConditionalPolicyDecision[] = [ { @@ -184,16 +187,6 @@ describe('REST policies api', () => { authorizeConditional: mockedAuthorizeConditional, }; - const mockIdentityClient = { - getIdentity: jest.fn().mockImplementation(async () => ({ - identity: { - type: 'User', - userEntityRef: 'user:default/guest', - ownershipEntityRefs: ['guest'], - }, - })), - }; - const logger = mockServices.logger.mock(); const mockDiscovery = mockServices.discovery.mock(); @@ -272,11 +265,14 @@ describe('REST policies api', () => { }, ); + mockHttpAuth.credentials = jest.fn().mockImplementation(() => credentials); + const options: RouterOptions = { config: config, logger, discovery: mockDiscovery, - identity: mockIdentityClient, + httpAuth: mockHttpAuth, + auth: mockAuth, policy: await RBACPermissionPolicy.build( logger, auditLoggerMock, @@ -373,14 +369,16 @@ describe('REST policies api', () => { }); }); - it('should return a status of Unauthorized - no user', async () => { - mockIdentityClient.getIdentity.mockImplementationOnce(() => undefined); + it('should return a status of Unauthorized - non user request', async () => { + mockHttpAuth.credentials = jest + .fn() + .mockImplementationOnce(() => mockCredentials.service()); const result = await request(app).post('/policies').send(); expect(result.statusCode).toBe(403); expect(result.body.error).toEqual({ name: 'NotAllowedError', - message: 'User identity not found', + message: `Only creadential principal with type 'user' permitted to modify permissions`, }); }); @@ -3095,7 +3093,7 @@ describe('REST policies api', () => { const result = await request(app).get('/roles/conditions').send(); expect(result.statusCode).toBe(200); expect(result.body).toEqual(expectedConditions); - expect(mockIdentityClient.getIdentity).toHaveBeenCalledTimes(0); + expect(mockHttpAuth.credentials).toHaveBeenCalledTimes(1); }); it('should be returned condition decision by pluginId', async () => { @@ -3204,7 +3202,7 @@ describe('REST policies api', () => { const result = await request(app).delete('/roles/conditions/1').send(); expect(result.statusCode).toEqual(204); - expect(mockIdentityClient.getIdentity).toHaveBeenCalledTimes(1); + expect(mockHttpAuth.credentials).toHaveBeenCalledTimes(1); expect(conditionalStorage.deleteCondition).toHaveBeenCalled(); }); @@ -3274,7 +3272,7 @@ describe('REST policies api', () => { const result = await request(app).get('/roles/conditions/1').send(); expect(result.statusCode).toBe(200); expect(result.body).toEqual(expectedConditions[0]); - expect(mockIdentityClient.getIdentity).toHaveBeenCalledTimes(0); + expect(mockHttpAuth.credentials).toHaveBeenCalledTimes(1); }); it('should return return 404', async () => { @@ -3369,7 +3367,7 @@ describe('REST policies api', () => { expect(result.statusCode).toBe(201); expect(validateRoleConditionMock).toHaveBeenCalledWith(roleCondition); expect(result.body).toEqual({ id: 1 }); - expect(mockIdentityClient.getIdentity).toHaveBeenCalledTimes(1); + expect(mockHttpAuth.credentials).toHaveBeenCalledTimes(1); }); }); @@ -3466,7 +3464,7 @@ describe('REST policies api', () => { params: { claims: ['group:default/team-a'] }, }, }); - expect(mockIdentityClient.getIdentity).toHaveBeenCalledTimes(1); + expect(mockHttpAuth.credentials).toHaveBeenCalledTimes(1); }); }); @@ -3482,7 +3480,8 @@ describe('REST policies api', () => { config: config, logger, discovery: mockDiscovery, - identity: mockIdentityClient, + httpAuth: mockHttpAuth, + auth: mockAuth, policy: await RBACPermissionPolicy.build( logger, auditLoggerMock, diff --git a/plugins/rbac-backend/src/service/policies-rest-api.ts b/plugins/rbac-backend/src/service/policies-rest-api.ts index 681bab856a..98a6ffe6a5 100644 --- a/plugins/rbac-backend/src/service/policies-rest-api.ts +++ b/plugins/rbac-backend/src/service/policies-rest-api.ts @@ -9,8 +9,8 @@ import { InputError, NotAllowedError, NotFoundError, + ServiceUnavailableError, } from '@backstage/errors'; -import type { IdentityApi } from '@backstage/plugin-auth-node'; import { createRouter, RouterOptions, @@ -94,22 +94,26 @@ export class PoliciesServer { private async authorize( request: Request, - identity: IdentityApi, permission: ResourcePermission, ): Promise { - if (permission !== policyEntityReadPermission) { - const userIdentity = await identity.getIdentity({ request }); - if (!userIdentity) { - throw new NotAllowedError('User identity not found'); - } + const credentials = await this.httpAuth.credentials(request, { + allow: ['user', 'service'], + }); + + // allow service to service communication, but only with read permission + if ( + this.auth.isPrincipal(credentials, 'service') && + permission !== policyEntityReadPermission + ) { + throw new NotAllowedError( + `Only creadential principal with type 'user' permitted to modify permissions`, + ); } const decision = ( await this.permissions.authorize( [{ permission: permission, resourceRef: permission.resourceType }], - { - credentials: await this.httpAuth.credentials(request), - }, + { credentials }, ) )[0]; @@ -119,11 +123,11 @@ export class PoliciesServer { async serve(): Promise { const router = await createRouter(this.options); - const { identity } = this.options; + const { httpAuth } = this.options; - if (!identity) { - throw new NotAllowedError( - 'Identity api not found, ensure the correct configuration for the RBAC plugin', + if (!httpAuth) { + throw new ServiceUnavailableError( + 'httpAuth not found, ensure the correct configuration for the RBAC plugin', ); } @@ -142,7 +146,6 @@ export class PoliciesServer { router.get('/', async (request, response) => { const decision = await this.authorize( request, - identity, policyEntityReadPermission, ); @@ -157,7 +160,6 @@ export class PoliciesServer { router.get('/policies', async (request, response) => { const decision = await this.authorize( request, - identity, policyEntityReadPermission, ); @@ -197,7 +199,6 @@ export class PoliciesServer { async (request, response) => { const decision = await this.authorize( request, - identity, policyEntityReadPermission, ); @@ -232,7 +233,6 @@ export class PoliciesServer { async (request, response) => { const decision = await this.authorize( request, - identity, policyEntityDeletePermission, ); @@ -272,7 +272,6 @@ export class PoliciesServer { router.post('/policies', async (request, response) => { const decision = await this.authorize( request, - identity, policyEntityCreatePermission, ); @@ -314,7 +313,6 @@ export class PoliciesServer { async (request, response) => { const decision = await this.authorize( request, - identity, policyEntityUpdatePermission, ); @@ -402,7 +400,6 @@ export class PoliciesServer { router.get('/roles', async (request, response) => { const decision = await this.authorize( request, - identity, policyEntityReadPermission, ); @@ -429,7 +426,6 @@ export class PoliciesServer { router.get('/roles/:kind/:namespace/:name', async (request, response) => { const decision = await this.authorize( request, - identity, policyEntityReadPermission, ); @@ -465,7 +461,6 @@ export class PoliciesServer { const uniqueItems = new Set(); const decision = await this.authorize( request, - identity, policyEntityCreatePermission, ); @@ -506,8 +501,10 @@ export class PoliciesServer { } } - const user = await identity.getIdentity({ request }); - const modifiedBy = user?.identity.userEntityRef!; + const credentials = await httpAuth.credentials(request, { + allow: ['user'], + }); + const modifiedBy = credentials.principal.userEntityRef; const metadata: RoleMetadataDao = { roleEntityRef: roleRaw.name, source: 'rest', @@ -538,7 +535,6 @@ export class PoliciesServer { const uniqueItems = new Set(); const decision = await this.authorize( request, - identity, policyEntityUpdatePermission, ); @@ -575,12 +571,15 @@ export class PoliciesServer { const newRole = this.transformRoleToArray(newRoleRaw); // todo shell we allow newRole with an empty array?... - const user = await identity.getIdentity({ request }); + const credentials = await httpAuth.credentials(request, { + allow: ['user'], + }); + const newMetadata: RoleMetadataDao = { ...newRoleRaw.metadata, source: newRoleRaw.metadata?.source ?? 'rest', roleEntityRef: newRoleRaw.name, - modifiedBy: user?.identity.userEntityRef!, + modifiedBy: credentials.principal.userEntityRef, }; const oldMetadata = @@ -679,7 +678,6 @@ export class PoliciesServer { async (request, response) => { const decision = await this.authorize( request, - identity, policyEntityDeletePermission, ); @@ -726,11 +724,14 @@ export class PoliciesServer { throw new NotAllowedError(`Unable to delete role: ${err.message}`); } - const user = await identity.getIdentity({ request }); + const credentials = await httpAuth.credentials(request, { + allow: ['user'], + }); + const metadata: RoleMetadataDao = { roleEntityRef, source: 'rest', - modifiedBy: user?.identity.userEntityRef!, + modifiedBy: credentials.principal.userEntityRef, }; await this.enforcer.removeGroupingPolicies( @@ -759,7 +760,6 @@ export class PoliciesServer { router.get('/plugins/policies', async (request, response) => { const decision = await this.authorize( request, - identity, policyEntityReadPermission, ); @@ -784,7 +784,6 @@ export class PoliciesServer { router.get('/plugins/condition-rules', async (request, response) => { const decision = await this.authorize( request, - identity, policyEntityReadPermission, ); @@ -811,7 +810,6 @@ export class PoliciesServer { router.get('/roles/conditions', async (request, response) => { const decision = await this.authorize( request, - identity, policyEntityReadPermission, ); @@ -849,7 +847,6 @@ export class PoliciesServer { router.post('/roles/conditions', async (request, response) => { const decision = await this.authorize( request, - identity, policyEntityCreatePermission, ); @@ -888,7 +885,6 @@ export class PoliciesServer { router.get('/roles/conditions/:id', async (request, response) => { const decision = await this.authorize( request, - identity, policyEntityReadPermission, ); @@ -926,7 +922,6 @@ export class PoliciesServer { router.delete('/roles/conditions/:id', async (request, response) => { const decision = await this.authorize( request, - identity, policyEntityDeletePermission, ); @@ -967,7 +962,6 @@ export class PoliciesServer { router.put('/roles/conditions/:id', async (request, response) => { const decision = await this.authorize( request, - identity, policyEntityUpdatePermission, ); @@ -1009,7 +1003,6 @@ export class PoliciesServer { router.post('/refresh/:id', async (request, response) => { const decision = await this.authorize( request, - identity, policyEntityCreatePermission, );