From 804b1edf2b7be402dceee2fbac699e83afc19617 Mon Sep 17 00:00:00 2001 From: Kevin Qualters <56408403+kqualters-elastic@users.noreply.github.com> Date: Mon, 25 Nov 2024 06:12:43 -0500 Subject: [PATCH] [Cases] Fix an issue with the reopen case permission, add integration tests for failing case (#201517) ## Summary Currently, the partitionPatchRequest in x-pack/plugins/cases/server/client/cases/bulk_update.ts will not check a case properly if a case is being re-opened, instead of an else if in the loop comparing cases to cases in the request, the status logic should be outside of this set of if statements. If a case is being re-opened, the final else is never run, and casesToAuthorize is 0. This results in ensureAuthorized being called with an empty array of entities, and so the check always succeeds. To fix this, reopenedCases is still populated in the same loop, just not when casesToAuthorize logic is running as well. Also adds some missing tests for this and the create comment permission as requested in https://github.com/elastic/kibana/pull/194898. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios (cherry picked from commit 1c3bceacc06f5f8a01a2ffde2ec24f114717b396) --- .../server/client/cases/bulk_update.test.ts | 91 ++++++---- .../cases/server/client/cases/bulk_update.ts | 1 + .../apis/cases/common/roles.ts | 156 ++++++++++++++++++ .../apis/cases/common/users.ts | 48 ++++++ .../api_integration/apis/cases/privileges.ts | 65 ++++++++ 5 files changed, 328 insertions(+), 33 deletions(-) diff --git a/x-pack/plugins/cases/server/client/cases/bulk_update.test.ts b/x-pack/plugins/cases/server/client/cases/bulk_update.test.ts index 755084d624b9f..5cdd4c943b944 100644 --- a/x-pack/plugins/cases/server/client/cases/bulk_update.test.ts +++ b/x-pack/plugins/cases/server/client/cases/bulk_update.test.ts @@ -1512,6 +1512,59 @@ describe('update', () => { ); }); + it('throws an error if the case is not found', async () => { + clientArgsMock.services.caseService.getCases.mockResolvedValue({ saved_objects: [] }); + + await expect( + bulkUpdate( + { + cases: [ + { + id: mockCases[0].id, + version: mockCases[0].version ?? '', + status: CaseStatuses.open, + }, + ], + }, + clientArgsMock, + casesClientMock + ) + ).rejects.toThrow( + 'Failed to update case, ids: [{"id":"mock-id-1","version":"WzAsMV0="}]: Error: These cases mock-id-1 do not exist. Please check you have the correct ids.' + ); + }); + + it('throws an error if the case is not found and the SO clients returns an SO object', async () => { + clientArgsMock.services.caseService.getCases.mockResolvedValue({ + saved_objects: [ + { + type: 'cases', + id: 'mock-id-1', + references: [], + error: { error: 'Non found', message: 'Non found', statusCode: 404 }, + }, + ], + }); + + await expect( + bulkUpdate( + { + cases: [ + { + id: mockCases[0].id, + version: mockCases[0].version ?? '', + status: CaseStatuses.open, + }, + ], + }, + clientArgsMock, + casesClientMock + ) + ).rejects.toThrow( + 'Failed to update case, ids: [{"id":"mock-id-1","version":"WzAsMV0="}]: Error: These cases mock-id-1 do not exist. Please check you have the correct ids.' + ); + }); + describe('Validate max user actions per page', () => { beforeEach(() => { jest.clearAllMocks(); @@ -1681,6 +1734,7 @@ describe('update', () => { status: CaseStatuses.closed, }, }; + clientArgs.services.caseService.getCases.mockResolvedValue({ saved_objects: [closedCase] }); clientArgs.services.caseService.patchCases.mockResolvedValue({ @@ -1701,7 +1755,10 @@ describe('update', () => { casesClientMock ); - expect(clientArgs.authorization.ensureAuthorized).not.toThrow(); + expect(clientArgs.authorization.ensureAuthorized).toHaveBeenCalledWith({ + entities: [{ id: closedCase.id, owner: closedCase.attributes.owner }], + operation: [Operations.reopenCase, Operations.updateCase], + }); }); it('throws when user is not authorized to update case', async () => { @@ -1726,38 +1783,6 @@ describe('update', () => { `"Failed to update case, ids: [{\\"id\\":\\"mock-id-1\\",\\"version\\":\\"WzAsMV0=\\"}]: Error: Unauthorized"` ); }); - - it('throws when user is not authorized to reopen case', async () => { - const closedCase = { - ...mockCases[0], - attributes: { - ...mockCases[0].attributes, - status: CaseStatuses.closed, - }, - }; - clientArgs.services.caseService.getCases.mockResolvedValue({ saved_objects: [closedCase] }); - - const error = new Error('Unauthorized to reopen case'); - clientArgs.authorization.ensureAuthorized.mockRejectedValueOnce(error); // Reject reopenCase - - await expect( - bulkUpdate( - { - cases: [ - { - id: closedCase.id, - version: closedCase.version ?? '', - status: CaseStatuses.open, - }, - ], - }, - clientArgs, - casesClientMock - ) - ).rejects.toThrowErrorMatchingInlineSnapshot( - `"Failed to update case, ids: [{\\"id\\":\\"mock-id-1\\",\\"version\\":\\"WzAsMV0=\\"}]: Error: Unauthorized to reopen case"` - ); - }); }); }); }); diff --git a/x-pack/plugins/cases/server/client/cases/bulk_update.ts b/x-pack/plugins/cases/server/client/cases/bulk_update.ts index 9a90168b858de..1f9dd5a7cb28c 100644 --- a/x-pack/plugins/cases/server/client/cases/bulk_update.ts +++ b/x-pack/plugins/cases/server/client/cases/bulk_update.ts @@ -295,6 +295,7 @@ function partitionPatchRequest( ) { // Track cases that are closed and a user is attempting to reopen reopenedCases.push(reqCase); + casesToAuthorize.set(foundCase.id, { id: foundCase.id, owner: foundCase.attributes.owner }); } else { casesToAuthorize.set(foundCase.id, { id: foundCase.id, owner: foundCase.attributes.owner }); } diff --git a/x-pack/test/api_integration/apis/cases/common/roles.ts b/x-pack/test/api_integration/apis/cases/common/roles.ts index 21ad6943ba0df..c50076b301f7c 100644 --- a/x-pack/test/api_integration/apis/cases/common/roles.ts +++ b/x-pack/test/api_integration/apis/cases/common/roles.ts @@ -136,6 +136,56 @@ export const secCasesV2All: Role = { }, }; +export const secCasesV2NoReopenWithCreateComment: Role = { + name: 'sec_cases_v2_no_reopen_role_api_int', + privileges: { + elasticsearch: { + indices: [ + { + names: ['*'], + privileges: ['all'], + }, + ], + }, + kibana: [ + { + feature: { + siem: ['all'], + securitySolutionCasesV2: ['read', 'update', 'create', 'delete', 'create_comment'], + actions: ['all'], + actionsSimulators: ['all'], + }, + spaces: ['*'], + }, + ], + }, +}; + +export const secCasesV2NoCreateCommentWithReopen: Role = { + name: 'sec_cases_v2_create_comment_no_reopen_role_api_int', + privileges: { + elasticsearch: { + indices: [ + { + names: ['*'], + privileges: ['all'], + }, + ], + }, + kibana: [ + { + feature: { + siem: ['all'], + securitySolutionCasesV2: ['read', 'update', 'create', 'delete', 'case_reopen'], + actions: ['all'], + actionsSimulators: ['all'], + }, + spaces: ['*'], + }, + ], + }, +}; + export const secAllSpace1: Role = { name: 'sec_all_role_space1_api_int', privileges: { @@ -434,6 +484,56 @@ export const casesV2All: Role = { }, }; +export const casesV2NoReopenWithCreateComment: Role = { + name: 'cases_v2_no_reopen_role_api_int', + privileges: { + elasticsearch: { + indices: [ + { + names: ['*'], + privileges: ['all'], + }, + ], + }, + kibana: [ + { + spaces: ['*'], + base: [], + feature: { + generalCasesV2: ['read', 'update', 'create', 'delete', 'create_comment'], + actions: ['all'], + actionsSimulators: ['all'], + }, + }, + ], + }, +}; + +export const casesV2NoCreateCommentWithReopen: Role = { + name: 'cases_v2_no_create_comment_role_api_int', + privileges: { + elasticsearch: { + indices: [ + { + names: ['*'], + privileges: ['all'], + }, + ], + }, + kibana: [ + { + spaces: ['*'], + base: [], + feature: { + generalCasesV2: ['read', 'update', 'create', 'delete', 'case_reopen'], + actions: ['all'], + actionsSimulators: ['all'], + }, + }, + ], + }, +}; + export const casesRead: Role = { name: 'cases_read_role_api_int', privileges: { @@ -583,6 +683,56 @@ export const obsCasesV2All: Role = { }, }; +export const obsCasesV2NoReopenWithCreateComment: Role = { + name: 'obs_cases_v2_no_reopen_role_api_int', + privileges: { + elasticsearch: { + indices: [ + { + names: ['*'], + privileges: ['all'], + }, + ], + }, + kibana: [ + { + spaces: ['*'], + base: [], + feature: { + observabilityCasesV2: ['read', 'update', 'create', 'delete', 'create_comment'], + actions: ['all'], + actionsSimulators: ['all'], + }, + }, + ], + }, +}; + +export const obsCasesV2NoCreateCommentWithReopen: Role = { + name: 'obs_cases_v2_no_create_comment_role_api_int', + privileges: { + elasticsearch: { + indices: [ + { + names: ['*'], + privileges: ['all'], + }, + ], + }, + kibana: [ + { + spaces: ['*'], + base: [], + feature: { + observabilityCasesV2: ['read', 'update', 'create', 'delete', 'case_reopen'], + actions: ['all'], + actionsSimulators: ['all'], + }, + }, + ], + }, +}; + export const obsCasesRead: Role = { name: 'obs_cases_read_role_api_int', privileges: { @@ -613,6 +763,8 @@ export const roles = [ secAllCasesNoDelete, secAll, secCasesV2All, + secCasesV2NoReopenWithCreateComment, + secCasesV2NoCreateCommentWithReopen, secAllSpace1, secAllCasesRead, secAllCasesNone, @@ -625,11 +777,15 @@ export const roles = [ casesNoDelete, casesAll, casesV2All, + casesV2NoReopenWithCreateComment, + casesV2NoCreateCommentWithReopen, casesRead, obsCasesOnlyDelete, obsCasesOnlyReadDelete, obsCasesNoDelete, obsCasesAll, obsCasesV2All, + obsCasesV2NoReopenWithCreateComment, + obsCasesV2NoCreateCommentWithReopen, obsCasesRead, ]; diff --git a/x-pack/test/api_integration/apis/cases/common/users.ts b/x-pack/test/api_integration/apis/cases/common/users.ts index a64b9767498fb..d3b05c5d3ddf6 100644 --- a/x-pack/test/api_integration/apis/cases/common/users.ts +++ b/x-pack/test/api_integration/apis/cases/common/users.ts @@ -31,6 +31,12 @@ import { secReadCasesAll, secReadCasesNone, secReadCasesRead, + casesV2NoReopenWithCreateComment, + obsCasesV2NoReopenWithCreateComment, + secCasesV2NoReopenWithCreateComment, + secCasesV2NoCreateCommentWithReopen, + casesV2NoCreateCommentWithReopen, + obsCasesV2NoCreateCommentWithReopen, } from './roles'; /** @@ -67,6 +73,18 @@ export const secCasesV2AllUser: User = { roles: [secCasesV2All.name], }; +export const secCasesV2NoReopenWithCreateCommentUser: User = { + username: 'sec_cases_v2_no_reopen_with_create_comment_user_api_int', + password: 'password', + roles: [secCasesV2NoReopenWithCreateComment.name], +}; + +export const secCasesV2NoCreateCommentWithReopenUser: User = { + username: 'sec_cases_v2_no_create_comment_with_reopen_user_api_int', + password: 'password', + roles: [secCasesV2NoCreateCommentWithReopen.name], +}; + export const secAllSpace1User: User = { username: 'sec_all_space1_user_api_int', password: 'password', @@ -143,6 +161,18 @@ export const casesV2AllUser: User = { roles: [casesV2All.name], }; +export const casesV2NoReopenWithCreateCommentUser: User = { + username: 'cases_v2_no_reopen_with_create_comment_user_api_int', + password: 'password', + roles: [casesV2NoReopenWithCreateComment.name], +}; + +export const casesV2NoCreateCommentWithReopenUser: User = { + username: 'cases_v2_no_create_comment_with_reopen_user_api_int', + password: 'password', + roles: [casesV2NoCreateCommentWithReopen.name], +}; + export const casesReadUser: User = { username: 'cases_read_user_api_int', password: 'password', @@ -183,6 +213,18 @@ export const obsCasesV2AllUser: User = { roles: [obsCasesV2All.name], }; +export const obsCasesV2NoReopenWithCreateCommentUser: User = { + username: 'obs_cases_v2_no_reopen_with_create_comment_user_api_int', + password: 'password', + roles: [obsCasesV2NoReopenWithCreateComment.name], +}; + +export const obsCasesV2NoCreateCommentWithReopenUser: User = { + username: 'obs_cases_v2_no_create_comment_with_reopen_user_api_int', + password: 'password', + roles: [obsCasesV2NoCreateCommentWithReopen.name], +}; + export const obsCasesReadUser: User = { username: 'obs_cases_read_user_api_int', password: 'password', @@ -211,6 +253,8 @@ export const users = [ secAllCasesNoDeleteUser, secAllUser, secCasesV2AllUser, + secCasesV2NoReopenWithCreateCommentUser, + secCasesV2NoCreateCommentWithReopenUser, secAllSpace1User, secAllCasesReadUser, secAllCasesNoneUser, @@ -223,12 +267,16 @@ export const users = [ casesNoDeleteUser, casesAllUser, casesV2AllUser, + casesV2NoReopenWithCreateCommentUser, + casesV2NoCreateCommentWithReopenUser, casesReadUser, obsCasesOnlyDeleteUser, obsCasesOnlyReadDeleteUser, obsCasesNoDeleteUser, obsCasesAllUser, obsCasesV2AllUser, + obsCasesV2NoReopenWithCreateCommentUser, + obsCasesV2NoCreateCommentWithReopenUser, obsCasesReadUser, obsSecCasesAllUser, obsSecCasesReadUser, diff --git a/x-pack/test/api_integration/apis/cases/privileges.ts b/x-pack/test/api_integration/apis/cases/privileges.ts index 53a1767f5c1a7..4e2baeeffa515 100644 --- a/x-pack/test/api_integration/apis/cases/privileges.ts +++ b/x-pack/test/api_integration/apis/cases/privileges.ts @@ -40,6 +40,12 @@ import { secReadCasesNoneUser, secReadCasesReadUser, secReadUser, + casesV2NoReopenWithCreateCommentUser, + casesV2NoCreateCommentWithReopenUser, + obsCasesV2NoReopenWithCreateCommentUser, + obsCasesV2NoCreateCommentWithReopenUser, + secCasesV2NoReopenWithCreateCommentUser, + secCasesV2NoCreateCommentWithReopenUser, } from './common/users'; import { getPostCaseRequest } from '../../../cases_api_integration/common/lib/mock'; @@ -183,6 +189,9 @@ export default ({ getService }: FtrProviderContext): void => { { user: obsCasesV2AllUser, owner: OBSERVABILITY_APP_ID }, { user: casesAllUser, owner: CASES_APP_ID }, { user: casesV2AllUser, owner: CASES_APP_ID }, + { user: casesV2NoCreateCommentWithReopenUser, owner: CASES_APP_ID }, + { user: obsCasesV2NoCreateCommentWithReopenUser, owner: OBSERVABILITY_APP_ID }, + { user: secCasesV2NoCreateCommentWithReopenUser, owner: SECURITY_SOLUTION_APP_ID }, ]) { it(`User ${user.username} with role(s) ${user.roles.join()} can reopen a case`, async () => { const caseInfo = await createCase(supertest, getPostCaseRequest({ owner })); @@ -206,6 +215,35 @@ export default ({ getService }: FtrProviderContext): void => { }); } + for (const { user, owner } of [ + { user: casesV2NoReopenWithCreateCommentUser, owner: CASES_APP_ID }, + { user: obsCasesV2NoReopenWithCreateCommentUser, owner: OBSERVABILITY_APP_ID }, + { user: secCasesV2NoReopenWithCreateCommentUser, owner: SECURITY_SOLUTION_APP_ID }, + ]) { + it(`User ${ + user.username + } with role(s) ${user.roles.join()} CANNOT reopen a case`, async () => { + const caseInfo = await createCase(supertest, getPostCaseRequest({ owner })); + await updateCaseStatus({ + supertest: supertestWithoutAuth, + caseId: caseInfo.id, + status: 'closed' as CaseStatuses, + version: '2', + expectedHttpCode: 200, + auth: { user, space: null }, + }); + + await updateCaseStatus({ + supertest: supertestWithoutAuth, + caseId: caseInfo.id, + status: 'open' as CaseStatuses, + version: '3', + expectedHttpCode: 403, + auth: { user, space: null }, + }); + }); + } + for (const { user, owner } of [ { user: secAllUser, owner: SECURITY_SOLUTION_APP_ID }, { user: secCasesV2AllUser, owner: SECURITY_SOLUTION_APP_ID }, @@ -213,6 +251,9 @@ export default ({ getService }: FtrProviderContext): void => { { user: obsCasesV2AllUser, owner: OBSERVABILITY_APP_ID }, { user: casesAllUser, owner: CASES_APP_ID }, { user: casesV2AllUser, owner: CASES_APP_ID }, + { user: casesV2NoReopenWithCreateCommentUser, owner: CASES_APP_ID }, + { user: obsCasesV2NoReopenWithCreateCommentUser, owner: OBSERVABILITY_APP_ID }, + { user: secCasesV2NoReopenWithCreateCommentUser, owner: SECURITY_SOLUTION_APP_ID }, ]) { it(`User ${user.username} with role(s) ${user.roles.join()} can add comments`, async () => { const caseInfo = await createCase(supertest, getPostCaseRequest({ owner })); @@ -230,5 +271,29 @@ export default ({ getService }: FtrProviderContext): void => { }); }); } + + for (const { user, owner } of [ + { user: casesV2NoCreateCommentWithReopenUser, owner: CASES_APP_ID }, + { user: obsCasesV2NoCreateCommentWithReopenUser, owner: OBSERVABILITY_APP_ID }, + { user: secCasesV2NoCreateCommentWithReopenUser, owner: SECURITY_SOLUTION_APP_ID }, + ]) { + it(`User ${ + user.username + } with role(s) ${user.roles.join()} CANNOT add comments`, async () => { + const caseInfo = await createCase(supertest, getPostCaseRequest({ owner })); + const comment: UserCommentAttachmentPayload = { + comment: 'test', + owner, + type: AttachmentType.user, + }; + await createComment({ + params: comment, + supertest: supertestWithoutAuth, + caseId: caseInfo.id, + expectedHttpCode: 403, + auth: { user, space: null }, + }); + }); + } }); };