From a40f68f5046a19d7395d4bf7554fbd963b9ffb83 Mon Sep 17 00:00:00 2001 From: "konrad.szwarc" Date: Thu, 13 Feb 2025 12:58:31 +0100 Subject: [PATCH] Allow only action.type to be changed with read permission --- .../workflow_insights/update_insight.test.ts | 110 +++++++++++------- .../workflow_insights/update_insight.ts | 12 +- 2 files changed, 78 insertions(+), 44 deletions(-) diff --git a/x-pack/solutions/security/plugins/security_solution/server/endpoint/routes/workflow_insights/update_insight.test.ts b/x-pack/solutions/security/plugins/security_solution/server/endpoint/routes/workflow_insights/update_insight.test.ts index f3146eed8242b..969cc71bce3f3 100644 --- a/x-pack/solutions/security/plugins/security_solution/server/endpoint/routes/workflow_insights/update_insight.test.ts +++ b/x-pack/solutions/security/plugins/security_solution/server/endpoint/routes/workflow_insights/update_insight.test.ts @@ -9,6 +9,7 @@ import { httpServerMock, httpServiceMock } from '@kbn/core/server/mocks'; import { registerUpdateInsightsRoute } from './update_insight'; import { createMockEndpointAppContext, getRegisteredVersionedRouteMock } from '../../mocks'; import { WORKFLOW_INSIGHTS_UPDATE_ROUTE } from '../../../../common/endpoint/constants'; +import type { EndpointAppContext } from '../../types'; jest.mock('../../services', () => ({ securityWorkflowInsightsService: { @@ -21,56 +22,55 @@ const updateMock = jest.requireMock('../../services').securityWorkflowInsightsSe describe('Update Insights Route Handler', () => { let mockResponse: ReturnType; - let callRoute: ( - params: Record, - body: Record, - authz?: Record - ) => Promise; - beforeEach(() => { + const callRoute = async ( + params: Record, + body: Record, + authz: Record = { + canWriteWorkflowInsights: true, + canReadWorkflowInsights: true, + } + ) => { mockResponse = httpServerMock.createResponseFactory(); const mockEndpointContext = createMockEndpointAppContext(); const router = httpServiceMock.createRouter(); - registerUpdateInsightsRoute(router, mockEndpointContext); - - callRoute = async ( - params, - body, - authz = { canWriteWorkflowInsights: true, canReadWorkflowInsights: true } - ) => { - const mockContext = { - core: { - security: { - authc: { - getCurrentUser: jest - .fn() - .mockReturnValue({ username: 'test-user', roles: ['admin'] }), - }, + const mockContext = { + ...mockEndpointContext, + service: { + ...mockEndpointContext.service, + getEndpointAuthz: jest.fn().mockResolvedValue(authz), + }, + securitySolution: { + getEndpointAuthz: jest.fn().mockResolvedValue(authz), + }, + core: { + security: { + authc: { + getCurrentUser: jest.fn().mockReturnValue({ username: 'test-user', roles: ['admin'] }), }, }, - securitySolution: { - getEndpointAuthz: jest.fn().mockResolvedValue(authz), - }, - }; + }, + }; - const request = httpServerMock.createKibanaRequest({ - method: 'put', - path: WORKFLOW_INSIGHTS_UPDATE_ROUTE, - params, - body, - }); + registerUpdateInsightsRoute(router, mockContext as unknown as EndpointAppContext); - const { routeHandler } = getRegisteredVersionedRouteMock( - router, - 'put', - WORKFLOW_INSIGHTS_UPDATE_ROUTE, - '1' - )!; - await routeHandler(mockContext, request, mockResponse); - }; - }); + const request = httpServerMock.createKibanaRequest({ + method: 'put', + path: WORKFLOW_INSIGHTS_UPDATE_ROUTE, + params, + body, + }); + + const { routeHandler } = getRegisteredVersionedRouteMock( + router, + 'put', + WORKFLOW_INSIGHTS_UPDATE_ROUTE, + '1' + )!; + await routeHandler(mockContext, request, mockResponse); + }; describe('with valid privileges', () => { it('should update insight and return the updated data', async () => { @@ -102,14 +102,38 @@ describe('Update Insights Route Handler', () => { }); describe('with invalid privileges', () => { - it('should return forbidden if user lacks read privileges', async () => { + it('should return forbidden if user lacks write and read privileges', async () => { await callRoute( { insightId: '1' }, - { name: 'Updated Insight' }, - { canWriteWorkflowInsights: false } + { action: { type: 'remediated' } }, + { canWriteWorkflowInsights: false, canReadWorkflowInsights: false } ); expect(mockResponse.forbidden).toHaveBeenCalled(); }); + + it('should allow update if user has no write privileges but read and the body only contains action.type', async () => { + const updateBody = { action: { type: 'remediated' } }; // Only action.type in the body + updateMock.mockResolvedValue({ id: 1, ...updateBody }); + await callRoute({ insightId: '1' }, updateBody, { + canWriteWorkflowInsights: false, + canReadWorkflowInsights: true, + }); + + expect(updateMock).toHaveBeenCalledWith('1', updateBody); + expect(mockResponse.ok).toHaveBeenCalledWith({ + body: { id: 1, action: { type: 'remediated' } }, + }); + }); + + it('should return forbidden if user has no write privileges but read and the body contains more than action.type', async () => { + const updateBody = { action: { type: 'remediated' }, value: 'changeme' }; + await callRoute({ insightId: '1' }, updateBody, { + canWriteWorkflowInsights: false, + canReadWorkflowInsights: true, + }); + + expect(mockResponse.forbidden).toHaveBeenCalled(); + }); }); }); diff --git a/x-pack/solutions/security/plugins/security_solution/server/endpoint/routes/workflow_insights/update_insight.ts b/x-pack/solutions/security/plugins/security_solution/server/endpoint/routes/workflow_insights/update_insight.ts index a6fb3c6cf5d20..d1584cf083b9b 100644 --- a/x-pack/solutions/security/plugins/security_solution/server/endpoint/routes/workflow_insights/update_insight.ts +++ b/x-pack/solutions/security/plugins/security_solution/server/endpoint/routes/workflow_insights/update_insight.ts @@ -63,9 +63,19 @@ const updateInsightsRouteHandler = ( > => { const logger = endpointContext.logFactory.get('workflowInsights'); + const isOnlyActionTypeUpdate = (body: Partial): boolean => { + // Type guard is done by schema validation + if (!body?.action?.type) return false; + // Make sure the body only contains the action.type field + return Object.keys(body).length === 1 && Object.keys(body.action).length === 1; + }; + return async (_, request, response) => { const { insightId } = request.params; - + const { canWriteWorkflowInsights } = await endpointContext.service.getEndpointAuthz(request); + if (!canWriteWorkflowInsights && !isOnlyActionTypeUpdate(request.body)) { + return response.forbidden({ body: 'Unauthorized to update workflow insights' }); + } logger.debug(`Updating insight ${insightId}`); try { const body = await securityWorkflowInsightsService.update(