From d9cf5fdef8c8c11b34595d6087e232a023152c21 Mon Sep 17 00:00:00 2001 From: Christos Nasikas Date: Sat, 1 Apr 2023 13:55:09 +0300 Subject: [PATCH 1/2] Delete alerts when deleting all comments --- .../server/client/attachments/delete.test.ts | 97 +++- .../cases/server/client/attachments/delete.ts | 26 +- x-pack/plugins/cases/server/mocks.ts | 4 +- .../tests/common/comments/delete_comment.ts | 58 --- .../tests/common/comments/delete_comments.ts | 480 ++++++++++++++++++ .../security_and_spaces/tests/common/index.ts | 1 + 6 files changed, 577 insertions(+), 89 deletions(-) create mode 100644 x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/delete_comments.ts diff --git a/x-pack/plugins/cases/server/client/attachments/delete.test.ts b/x-pack/plugins/cases/server/client/attachments/delete.test.ts index c74f621346728..d484515f4eeaf 100644 --- a/x-pack/plugins/cases/server/client/attachments/delete.test.ts +++ b/x-pack/plugins/cases/server/client/attachments/delete.test.ts @@ -7,40 +7,93 @@ import { mockCaseComments } from '../../mocks'; import { createCasesClientMockArgs } from '../mocks'; -import { deleteComment } from './delete'; +import { deleteComment, deleteAll } from './delete'; -describe('deleteComment', () => { - const clientArgs = createCasesClientMockArgs(); +describe('delete', () => { + describe('deleteComment', () => { + const clientArgs = createCasesClientMockArgs(); - beforeEach(() => { - jest.clearAllMocks(); - }); + beforeEach(() => { + jest.clearAllMocks(); + }); - describe('Alerts', () => { - const commentSO = mockCaseComments[0]; - const alertsSO = mockCaseComments[3]; - clientArgs.services.attachmentService.getter.get.mockResolvedValue(alertsSO); + describe('Alerts', () => { + const commentSO = mockCaseComments[0]; + const alertsSO = mockCaseComments[3]; + clientArgs.services.attachmentService.getter.get.mockResolvedValue(alertsSO); - it('delete alerts correctly', async () => { - await deleteComment({ caseID: 'mock-id-4', attachmentID: 'mock-comment-4' }, clientArgs); + it('delete alerts correctly', async () => { + await deleteComment({ caseID: 'mock-id-4', attachmentID: 'mock-comment-4' }, clientArgs); - expect(clientArgs.services.alertsService.ensureAlertsAuthorized).toHaveBeenCalledWith({ - alerts: [{ id: 'test-id', index: 'test-index' }], + expect(clientArgs.services.alertsService.ensureAlertsAuthorized).toHaveBeenCalledWith({ + alerts: [{ id: 'test-id', index: 'test-index' }], + }); + + expect(clientArgs.services.alertsService.removeCaseIdFromAlerts).toHaveBeenCalledWith({ + alerts: [{ id: 'test-id', index: 'test-index' }], + caseId: 'mock-id-4', + }); }); - expect(clientArgs.services.alertsService.removeCaseIdFromAlerts).toHaveBeenCalledWith({ - alerts: [{ id: 'test-id', index: 'test-index' }], - caseId: 'mock-id-4', + it('does not call the alert service when the attachment is not an alert', async () => { + clientArgs.services.attachmentService.getter.get.mockResolvedValue(commentSO); + await deleteComment({ caseID: 'mock-id-1', attachmentID: 'mock-comment-1' }, clientArgs); + + expect(clientArgs.services.alertsService.ensureAlertsAuthorized).not.toHaveBeenCalledWith(); + expect(clientArgs.services.alertsService.removeCaseIdFromAlerts).not.toHaveBeenCalledWith(); }); }); + }); + + describe('deleteAll', () => { + const clientArgs = createCasesClientMockArgs(); + const getAllCaseCommentsResponse = { + saved_objects: mockCaseComments.map((so) => ({ ...so, score: 0 })), + total: mockCaseComments.length, + page: 1, + per_page: mockCaseComments.length, + }; - it('does not call the alert service when the attachment is not an alert', async () => { - clientArgs.services.attachmentService.getter.get.mockResolvedValue(commentSO); - await deleteComment({ caseID: 'mock-id-1', attachmentID: 'mock-comment-1' }, clientArgs); + beforeEach(() => { + jest.clearAllMocks(); + }); - expect(clientArgs.services.alertsService.ensureAlertsAuthorized).not.toHaveBeenCalledWith(); + describe('Alerts', () => { + clientArgs.services.caseService.getAllCaseComments.mockResolvedValue( + getAllCaseCommentsResponse + ); - expect(clientArgs.services.alertsService.removeCaseIdFromAlerts).not.toHaveBeenCalledWith(); + it('delete alerts correctly', async () => { + await deleteAll({ caseID: 'mock-id-4' }, clientArgs); + + expect(clientArgs.services.alertsService.ensureAlertsAuthorized).toHaveBeenCalledWith({ + alerts: [ + { id: 'test-id', index: 'test-index' }, + { id: 'test-id-2', index: 'test-index-2' }, + { id: 'test-id-3', index: 'test-index-3' }, + ], + }); + + expect(clientArgs.services.alertsService.removeCaseIdFromAlerts).toHaveBeenCalledWith({ + alerts: [ + { id: 'test-id', index: 'test-index' }, + { id: 'test-id-2', index: 'test-index-2' }, + { id: 'test-id-3', index: 'test-index-3' }, + ], + caseId: 'mock-id-4', + }); + }); + + it('does not call the alert service when the attachment is not an alert', async () => { + clientArgs.services.caseService.getAllCaseComments.mockResolvedValue({ + ...getAllCaseCommentsResponse, + saved_objects: [{ ...mockCaseComments[0], score: 0 }], + }); + await deleteAll({ caseID: 'mock-id-1' }, clientArgs); + + expect(clientArgs.services.alertsService.ensureAlertsAuthorized).not.toHaveBeenCalledWith(); + expect(clientArgs.services.alertsService.removeCaseIdFromAlerts).not.toHaveBeenCalledWith(); + }); }); }); }); diff --git a/x-pack/plugins/cases/server/client/attachments/delete.ts b/x-pack/plugins/cases/server/client/attachments/delete.ts index 1a11620d9998e..57bac88da8275 100644 --- a/x-pack/plugins/cases/server/client/attachments/delete.ts +++ b/x-pack/plugins/cases/server/client/attachments/delete.ts @@ -9,7 +9,11 @@ import Boom from '@hapi/boom'; import pMap from 'p-map'; import type { SavedObject } from '@kbn/core/server'; -import type { CommentAttributes } from '../../../common/api'; +import type { + CommentAttributes, + CommentRequest, + CommentRequestAlertType, +} from '../../../common/api'; import { Actions, ActionTypes } from '../../../common/api'; import { getAlertInfoFromComments, isCommentRequestTypeAlert } from '../../common/utils'; import { CASE_SAVED_OBJECT, MAX_CONCURRENT_SEARCHES } from '../../../common/constants'; @@ -29,7 +33,7 @@ export async function deleteAll( ): Promise { const { user, - services: { caseService, attachmentService, userActionService }, + services: { caseService, attachmentService, userActionService, alertsService }, logger, authorization, } = clientArgs; @@ -71,6 +75,10 @@ export async function deleteAll( })), user, }); + + const attachments = comments.saved_objects.map((comment) => comment.attributes); + + await handleAlerts({ alertsService, attachments, caseId: caseID }); } catch (error) { throw createCaseError({ message: `Failed to delete all comments case id: ${caseID}: ${error}`, @@ -133,7 +141,7 @@ export async function deleteComment( owner: attachment.attributes.owner, }); - await handleAlerts({ alertsService, attachment: attachment.attributes, caseId: id }); + await handleAlerts({ alertsService, attachments: [attachment.attributes], caseId: id }); } catch (error) { throw createCaseError({ message: `Failed to delete comment: ${caseID} comment id: ${attachmentID}: ${error}`, @@ -145,16 +153,20 @@ export async function deleteComment( interface HandleAlertsArgs { alertsService: CasesClientArgs['services']['alertsService']; - attachment: CommentAttributes; + attachments: CommentRequest[]; caseId: string; } -const handleAlerts = async ({ alertsService, attachment, caseId }: HandleAlertsArgs) => { - if (!isCommentRequestTypeAlert(attachment)) { +const handleAlerts = async ({ alertsService, attachments, caseId }: HandleAlertsArgs) => { + const alertAttachments = attachments.filter((attachment): attachment is CommentRequestAlertType => + isCommentRequestTypeAlert(attachment) + ); + + if (alertAttachments.length === 0) { return; } - const alerts = getAlertInfoFromComments([attachment]); + const alerts = getAlertInfoFromComments(alertAttachments); await alertsService.ensureAlertsAuthorized({ alerts }); await alertsService.removeCaseIdFromAlerts({ alerts, caseId }); }; diff --git a/x-pack/plugins/cases/server/mocks.ts b/x-pack/plugins/cases/server/mocks.ts index 6a4d76cf035c8..823acef076fe3 100644 --- a/x-pack/plugins/cases/server/mocks.ts +++ b/x-pack/plugins/cases/server/mocks.ts @@ -370,8 +370,8 @@ export const mockCaseComments: Array> = [ id: 'mock-comment-6', attributes: { type: CommentType.alert, - index: 'test-index', - alertId: 'test-id', + index: 'test-index-3', + alertId: 'test-id-3', created_at: '2019-11-25T22:32:30.608Z', created_by: { full_name: 'elastic', diff --git a/x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/delete_comment.ts b/x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/delete_comment.ts index b0c4d670855a3..e61b26a3a0053 100644 --- a/x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/delete_comment.ts +++ b/x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/delete_comment.ts @@ -30,7 +30,6 @@ import { createCase, createComment, deleteComment, - deleteAllComments, superUserSpace1Auth, } from '../../../../common/lib/api'; import { @@ -387,35 +386,6 @@ export default ({ getService }: FtrProviderContext): void => { }); }); - it('should delete multiple comments from the appropriate owner', async () => { - const secCase = await createCase( - supertestWithoutAuth, - getPostCaseRequest({ owner: 'securitySolutionFixture' }), - 200, - { user: secOnly, space: 'space1' } - ); - - await createComment({ - supertest: supertestWithoutAuth, - caseId: secCase.id, - params: postCommentUserReq, - auth: { user: secOnly, space: 'space1' }, - }); - - await createComment({ - supertest: supertestWithoutAuth, - caseId: secCase.id, - params: postCommentUserReq, - auth: { user: secOnly, space: 'space1' }, - }); - - await deleteAllComments({ - supertest: supertestWithoutAuth, - caseId: secCase.id, - auth: { user: secOnly, space: 'space1' }, - }); - }); - it('should not delete a comment from a different owner', async () => { const secCase = await createCase( supertestWithoutAuth, @@ -438,13 +408,6 @@ export default ({ getService }: FtrProviderContext): void => { auth: { user: obsOnly, space: 'space1' }, expectedHttpCode: 403, }); - - await deleteAllComments({ - supertest: supertestWithoutAuth, - caseId: secCase.id, - auth: { user: obsOnly, space: 'space1' }, - expectedHttpCode: 403, - }); }); for (const user of [globalRead, secOnlyRead, obsOnlyRead, obsSecRead, noKibanaPrivileges]) { @@ -472,13 +435,6 @@ export default ({ getService }: FtrProviderContext): void => { auth: { user, space: 'space1' }, expectedHttpCode: 403, }); - - await deleteAllComments({ - supertest: supertestWithoutAuth, - caseId: postedCase.id, - auth: { user, space: 'space1' }, - expectedHttpCode: 403, - }); }); } @@ -504,13 +460,6 @@ export default ({ getService }: FtrProviderContext): void => { auth: { user: secOnly, space: 'space2' }, expectedHttpCode: 403, }); - - await deleteAllComments({ - supertest: supertestWithoutAuth, - caseId: postedCase.id, - auth: { user: secOnly, space: 'space2' }, - expectedHttpCode: 403, - }); }); it('should NOT delete a comment created in space2 by making a request to space1', async () => { @@ -535,13 +484,6 @@ export default ({ getService }: FtrProviderContext): void => { auth: { user: secOnly, space: 'space1' }, expectedHttpCode: 404, }); - - await deleteAllComments({ - supertest: supertestWithoutAuth, - caseId: postedCase.id, - auth: { user: secOnly, space: 'space1' }, - expectedHttpCode: 404, - }); }); }); }); diff --git a/x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/delete_comments.ts b/x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/delete_comments.ts new file mode 100644 index 0000000000000..e7bf9cc9f3e9d --- /dev/null +++ b/x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/delete_comments.ts @@ -0,0 +1,480 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +import expect from '@kbn/expect'; +import { CommentType } from '@kbn/cases-plugin/common'; +import { ALERT_CASE_IDS } from '@kbn/rule-data-utils'; +import { + createSecuritySolutionAlerts, + getAlertById, + getSecuritySolutionAlerts, +} from '../../../../common/lib/alerts'; +import { + createSignalsIndex, + deleteSignalsIndex, + deleteAllRules, +} from '../../../../../detection_engine_api_integration/utils'; +import { FtrProviderContext } from '../../../../common/ftr_provider_context'; +import { User } from '../../../../common/lib/authentication/types'; + +import { getPostCaseRequest, postCaseReq, postCommentUserReq } from '../../../../common/lib/mock'; +import { + deleteAllCaseItems, + deleteCasesByESQuery, + deleteCasesUserActions, + deleteComments, + createCase, + createComment, + deleteAllComments, + superUserSpace1Auth, +} from '../../../../common/lib/api'; +import { + globalRead, + noKibanaPrivileges, + obsOnly, + obsOnlyRead, + obsOnlyReadAlerts, + obsSec, + obsSecRead, + secOnly, + secOnlyRead, + secOnlyReadAlerts, + secSolutionOnlyReadNoIndexAlerts, + superUser, +} from '../../../../common/lib/authentication/users'; + +// eslint-disable-next-line import/no-default-export +export default ({ getService }: FtrProviderContext): void => { + const supertest = getService('supertest'); + const es = getService('es'); + const esArchiver = getService('esArchiver'); + const log = getService('log'); + const supertestWithoutAuth = getService('supertestWithoutAuth'); + + describe('delete_comments', () => { + afterEach(async () => { + await deleteCasesByESQuery(es); + await deleteComments(es); + await deleteCasesUserActions(es); + }); + + describe('happy path', () => { + it('should delete all comments', async () => { + const postedCase = await createCase(supertest, postCaseReq); + + await createComment({ + supertest, + caseId: postedCase.id, + params: postCommentUserReq, + }); + + const comment = await deleteAllComments({ + supertest, + caseId: postedCase.id, + }); + + expect(comment).to.eql({}); + }); + }); + + describe('unhappy path', () => { + it('404s when comment belongs to different case', async () => { + const postedCase = await createCase(supertest, postCaseReq); + await createComment({ + supertest, + caseId: postedCase.id, + params: postCommentUserReq, + }); + + const error = (await deleteAllComments({ + supertest, + caseId: 'fake-id', + expectedHttpCode: 404, + })) as Error; + + expect(error.message).to.be('No comments found for fake-id.'); + }); + }); + + describe('alerts', () => { + type Alerts = Array<{ _id: string; _index: string }>; + + const createCaseAttachAlertAndDeleteAlert = async ({ + totalCases, + indexOfCaseToDelete, + owner, + expectedHttpCode = 204, + deleteCommentAuth = { user: superUser, space: 'space1' }, + alerts, + getAlerts, + }: { + totalCases: number; + indexOfCaseToDelete: number; + owner: string; + expectedHttpCode?: number; + deleteCommentAuth?: { user: User; space: string | null }; + alerts: Alerts; + getAlerts: (alerts: Alerts) => Promise>>; + }) => { + const cases = await Promise.all( + [...Array(totalCases).keys()].map((index) => + createCase( + supertestWithoutAuth, + { + ...postCaseReq, + owner, + settings: { syncAlerts: false }, + }, + 200, + { user: superUser, space: 'space1' } + ) + ) + ); + + const updatedCases = []; + + for (const theCase of cases) { + const updatedCase = await createComment({ + supertest: supertestWithoutAuth, + caseId: theCase.id, + params: { + alertId: alerts.map((alert) => alert._id), + index: alerts.map((alert) => alert._index), + rule: { + id: 'id', + name: 'name', + }, + owner, + type: CommentType.alert, + }, + auth: { user: superUser, space: 'space1' }, + }); + + updatedCases.push(updatedCase); + } + + const caseIds = updatedCases.map((theCase) => theCase.id); + + const updatedAlerts = await getAlerts(alerts); + + for (const alert of updatedAlerts) { + expect(alert[ALERT_CASE_IDS]).eql(caseIds); + } + + const caseToDelete = updatedCases[indexOfCaseToDelete]; + + await deleteAllComments({ + supertest: supertestWithoutAuth, + caseId: caseToDelete.id, + expectedHttpCode, + auth: deleteCommentAuth, + }); + + const alertAfterDeletion = await getAlerts(alerts); + + const caseIdsWithoutRemovedCase = + expectedHttpCode === 204 + ? updatedCases + .filter((theCase) => theCase.id !== caseToDelete.id) + .map((theCase) => theCase.id) + : updatedCases.map((theCase) => theCase.id); + + for (const alert of alertAfterDeletion) { + expect(alert[ALERT_CASE_IDS]).eql(caseIdsWithoutRemovedCase); + } + }; + + describe('security_solution', () => { + let alerts: Alerts = []; + + const getAlerts = async (_alerts: Alerts) => { + await es.indices.refresh({ index: _alerts.map((alert) => alert._index) }); + const updatedAlerts = await getSecuritySolutionAlerts( + supertest, + alerts.map((alert) => alert._id) + ); + + return updatedAlerts.hits.hits.map((alert) => ({ ...alert._source })); + }; + + beforeEach(async () => { + await esArchiver.load('x-pack/test/functional/es_archives/auditbeat/hosts'); + await createSignalsIndex(supertest, log); + const signals = await createSecuritySolutionAlerts(supertest, log); + alerts = [signals.hits.hits[0], signals.hits.hits[1]]; + }); + + afterEach(async () => { + await deleteSignalsIndex(supertest, log); + await deleteAllRules(supertest, log); + await esArchiver.unload('x-pack/test/functional/es_archives/auditbeat/hosts'); + }); + + it('removes a case from the alert schema when deleting all alert attachments', async () => { + await createCaseAttachAlertAndDeleteAlert({ + totalCases: 1, + indexOfCaseToDelete: 0, + owner: 'securitySolutionFixture', + alerts, + getAlerts, + }); + }); + + it('should remove only one case', async () => { + await createCaseAttachAlertAndDeleteAlert({ + totalCases: 3, + indexOfCaseToDelete: 1, + owner: 'securitySolutionFixture', + alerts, + getAlerts, + }); + }); + + it('should delete case ID from the alert schema when the user has write access to the indices and only read access to the siem solution', async () => { + await createCaseAttachAlertAndDeleteAlert({ + totalCases: 1, + indexOfCaseToDelete: 0, + owner: 'securitySolutionFixture', + alerts, + getAlerts, + expectedHttpCode: 204, + deleteCommentAuth: { user: secOnlyReadAlerts, space: 'space1' }, + }); + }); + + it('should NOT delete case ID from the alert schema when the user does NOT have access to the alert', async () => { + await createCaseAttachAlertAndDeleteAlert({ + totalCases: 1, + indexOfCaseToDelete: 0, + owner: 'securitySolutionFixture', + alerts, + getAlerts, + expectedHttpCode: 403, + deleteCommentAuth: { user: obsSec, space: 'space1' }, + }); + }); + + it('should delete the case ID from the alert schema when the user has read access to the kibana feature but no read access to the ES index', async () => { + await createCaseAttachAlertAndDeleteAlert({ + totalCases: 1, + indexOfCaseToDelete: 0, + owner: 'securitySolutionFixture', + alerts, + getAlerts, + expectedHttpCode: 204, + deleteCommentAuth: { user: secSolutionOnlyReadNoIndexAlerts, space: 'space1' }, + }); + }); + }); + + describe('observability', () => { + const alerts = [ + { _id: 'NoxgpHkBqbdrfX07MqXV', _index: '.alerts-observability.apm.alerts' }, + { _id: 'space1alert', _index: '.alerts-observability.apm.alerts' }, + ]; + + const getAlerts = async (_alerts: Alerts) => { + const updatedAlerts = await Promise.all( + _alerts.map((alert) => + getAlertById({ + supertest: supertestWithoutAuth, + id: alert._id, + index: alert._index, + auth: { user: superUser, space: 'space1' }, + }) + ) + ); + + return updatedAlerts as Array>; + }; + + beforeEach(async () => { + await esArchiver.load('x-pack/test/functional/es_archives/rule_registry/alerts'); + }); + + afterEach(async () => { + await esArchiver.unload('x-pack/test/functional/es_archives/rule_registry/alerts'); + }); + + it('removes a case from the alert schema when deleting all alert attachments', async () => { + await createCaseAttachAlertAndDeleteAlert({ + totalCases: 1, + indexOfCaseToDelete: 0, + owner: 'observabilityFixture', + alerts, + getAlerts, + }); + }); + + it('should remove only one case', async () => { + await createCaseAttachAlertAndDeleteAlert({ + totalCases: 3, + indexOfCaseToDelete: 1, + owner: 'observabilityFixture', + alerts, + getAlerts, + }); + }); + + it('should delete case ID from the alert schema when the user has read access only', async () => { + await createCaseAttachAlertAndDeleteAlert({ + totalCases: 1, + indexOfCaseToDelete: 0, + expectedHttpCode: 204, + owner: 'observabilityFixture', + alerts, + getAlerts, + deleteCommentAuth: { user: obsOnlyReadAlerts, space: 'space1' }, + }); + }); + + it('should NOT delete case ID from the alert schema when the user does NOT have access to the alert', async () => { + await createCaseAttachAlertAndDeleteAlert({ + totalCases: 1, + indexOfCaseToDelete: 0, + expectedHttpCode: 403, + owner: 'observabilityFixture', + alerts, + getAlerts, + deleteCommentAuth: { user: obsSec, space: 'space1' }, + }); + }); + }); + }); + + describe('rbac', () => { + afterEach(async () => { + await deleteAllCaseItems(es); + }); + + it('should delete multiple comments from the appropriate owner', async () => { + const secCase = await createCase( + supertestWithoutAuth, + getPostCaseRequest({ owner: 'securitySolutionFixture' }), + 200, + { user: secOnly, space: 'space1' } + ); + + await createComment({ + supertest: supertestWithoutAuth, + caseId: secCase.id, + params: postCommentUserReq, + auth: { user: secOnly, space: 'space1' }, + }); + + await createComment({ + supertest: supertestWithoutAuth, + caseId: secCase.id, + params: postCommentUserReq, + auth: { user: secOnly, space: 'space1' }, + }); + + await deleteAllComments({ + supertest: supertestWithoutAuth, + caseId: secCase.id, + auth: { user: secOnly, space: 'space1' }, + }); + }); + + it('should not delete a comment from a different owner', async () => { + const secCase = await createCase( + supertestWithoutAuth, + getPostCaseRequest({ owner: 'securitySolutionFixture' }), + 200, + { user: secOnly, space: 'space1' } + ); + + await createComment({ + supertest: supertestWithoutAuth, + caseId: secCase.id, + params: postCommentUserReq, + auth: { user: secOnly, space: 'space1' }, + }); + + await deleteAllComments({ + supertest: supertestWithoutAuth, + caseId: secCase.id, + auth: { user: obsOnly, space: 'space1' }, + expectedHttpCode: 403, + }); + }); + + for (const user of [globalRead, secOnlyRead, obsOnlyRead, obsSecRead, noKibanaPrivileges]) { + it(`User ${ + user.username + } with role(s) ${user.roles.join()} - should NOT delete all comments`, async () => { + const postedCase = await createCase( + supertestWithoutAuth, + getPostCaseRequest({ owner: 'securitySolutionFixture' }), + 200, + superUserSpace1Auth + ); + + await createComment({ + supertest: supertestWithoutAuth, + caseId: postedCase.id, + params: postCommentUserReq, + auth: superUserSpace1Auth, + }); + + await deleteAllComments({ + supertest: supertestWithoutAuth, + caseId: postedCase.id, + auth: { user, space: 'space1' }, + expectedHttpCode: 403, + }); + }); + } + + it('should NOT delete a comment in a space with where the user does not have permissions', async () => { + const postedCase = await createCase( + supertestWithoutAuth, + getPostCaseRequest({ owner: 'securitySolutionFixture' }), + 200, + { user: superUser, space: 'space2' } + ); + + await createComment({ + supertest: supertestWithoutAuth, + caseId: postedCase.id, + params: postCommentUserReq, + auth: { user: superUser, space: 'space2' }, + }); + + await deleteAllComments({ + supertest: supertestWithoutAuth, + caseId: postedCase.id, + auth: { user: secOnly, space: 'space2' }, + expectedHttpCode: 403, + }); + }); + + it('should NOT delete a comment created in space2 by making a request to space1', async () => { + const postedCase = await createCase( + supertestWithoutAuth, + getPostCaseRequest({ owner: 'securitySolutionFixture' }), + 200, + { user: superUser, space: 'space2' } + ); + + await createComment({ + supertest: supertestWithoutAuth, + caseId: postedCase.id, + params: postCommentUserReq, + auth: { user: superUser, space: 'space2' }, + }); + + await deleteAllComments({ + supertest: supertestWithoutAuth, + caseId: postedCase.id, + auth: { user: secOnly, space: 'space1' }, + expectedHttpCode: 404, + }); + }); + }); + }); +}; diff --git a/x-pack/test/cases_api_integration/security_and_spaces/tests/common/index.ts b/x-pack/test/cases_api_integration/security_and_spaces/tests/common/index.ts index 6914c23af656f..c9982219d27fe 100644 --- a/x-pack/test/cases_api_integration/security_and_spaces/tests/common/index.ts +++ b/x-pack/test/cases_api_integration/security_and_spaces/tests/common/index.ts @@ -12,6 +12,7 @@ export default ({ loadTestFile }: FtrProviderContext): void => { describe('Common', function () { loadTestFile(require.resolve('./client/update_alert_status')); loadTestFile(require.resolve('./comments/delete_comment')); + loadTestFile(require.resolve('./comments/delete_comments')); loadTestFile(require.resolve('./comments/find_comments')); loadTestFile(require.resolve('./comments/get_comment')); loadTestFile(require.resolve('./comments/get_all_comments')); From 06d859b8efcc6b7b58f62ef8a024881d1ce828e5 Mon Sep 17 00:00:00 2001 From: Christos Nasikas Date: Tue, 4 Apr 2023 13:08:42 +0300 Subject: [PATCH 2/2] Address PR feedback --- .../common/lib/alerts.ts | 95 +++++++++ .../tests/common/comments/delete_comment.ts | 100 +-------- .../tests/common/comments/delete_comments.ts | 197 ++++++++++-------- 3 files changed, 211 insertions(+), 181 deletions(-) diff --git a/x-pack/test/cases_api_integration/common/lib/alerts.ts b/x-pack/test/cases_api_integration/common/lib/alerts.ts index ff29579c91607..08da41280f9be 100644 --- a/x-pack/test/cases_api_integration/common/lib/alerts.ts +++ b/x-pack/test/cases_api_integration/common/lib/alerts.ts @@ -5,12 +5,15 @@ * 2.0. */ +import expect from '@kbn/expect'; import type SuperTest from 'supertest'; import type * as estypes from '@elastic/elasticsearch/lib/api/typesWithBodyKey'; import { ToolingLog } from '@kbn/tooling-log'; import { DETECTION_ENGINE_QUERY_SIGNALS_URL } from '@kbn/security-solution-plugin/common/constants'; import { DetectionAlert } from '@kbn/security-solution-plugin/common/detection_engine/schemas/alerts'; import { RiskEnrichmentFields } from '@kbn/security-solution-plugin/server/lib/detection_engine/rule_types/utils/enrichments/types'; +import { CommentType } from '@kbn/cases-plugin/common'; +import { ALERT_CASE_IDS } from '@kbn/rule-data-utils'; import { getRuleForSignalTesting, createRule, @@ -22,6 +25,9 @@ import { import { superUser } from './authentication/users'; import { User } from './authentication/types'; import { getSpaceUrlPrefix } from './api/helpers'; +import { createCase } from './api/case'; +import { createComment, deleteAllComments } from './api'; +import { postCaseReq } from './mock'; export const createSecuritySolutionAlerts = async ( supertest: SuperTest.SuperTest, @@ -74,3 +80,92 @@ export const getAlertById = async ({ return alert; }; + +export type Alerts = Array<{ _id: string; _index: string }>; + +export const createCaseAttachAlertAndDeleteAlert = async ({ + supertest, + totalCases, + indexOfCaseToDelete, + owner, + expectedHttpCode = 204, + deleteCommentAuth = { user: superUser, space: 'space1' }, + alerts, + getAlerts, +}: { + supertest: SuperTest.SuperTest; + totalCases: number; + indexOfCaseToDelete: number; + owner: string; + expectedHttpCode?: number; + deleteCommentAuth?: { user: User; space: string | null }; + alerts: Alerts; + getAlerts: (alerts: Alerts) => Promise>>; +}) => { + const cases = await Promise.all( + [...Array(totalCases).keys()].map((index) => + createCase( + supertest, + { + ...postCaseReq, + owner, + settings: { syncAlerts: false }, + }, + 200, + { user: superUser, space: 'space1' } + ) + ) + ); + + const updatedCases = []; + + for (const theCase of cases) { + const updatedCase = await createComment({ + supertest, + caseId: theCase.id, + params: { + alertId: alerts.map((alert) => alert._id), + index: alerts.map((alert) => alert._index), + rule: { + id: 'id', + name: 'name', + }, + owner, + type: CommentType.alert, + }, + auth: { user: superUser, space: 'space1' }, + }); + + updatedCases.push(updatedCase); + } + + const caseIds = updatedCases.map((theCase) => theCase.id); + + const updatedAlerts = await getAlerts(alerts); + + for (const alert of updatedAlerts) { + expect(alert[ALERT_CASE_IDS]).eql(caseIds); + } + + const caseToDelete = updatedCases[indexOfCaseToDelete]; + + await deleteAllComments({ + supertest, + caseId: caseToDelete.id, + expectedHttpCode, + auth: deleteCommentAuth, + }); + + const alertAfterDeletion = await getAlerts(alerts); + + const caseIdsWithoutRemovedCase = + expectedHttpCode === 204 + ? updatedCases + .filter((theCase) => theCase.id !== caseToDelete.id) + .map((theCase) => theCase.id) + : updatedCases.map((theCase) => theCase.id); + + for (const alert of alertAfterDeletion) { + expect(alert[ALERT_CASE_IDS]).eql(caseIdsWithoutRemovedCase); + } +}; diff --git a/x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/delete_comment.ts b/x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/delete_comment.ts index e61b26a3a0053..317bd2797245b 100644 --- a/x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/delete_comment.ts +++ b/x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/delete_comment.ts @@ -6,9 +6,8 @@ */ import expect from '@kbn/expect'; -import { CommentType } from '@kbn/cases-plugin/common'; -import { ALERT_CASE_IDS } from '@kbn/rule-data-utils'; import { + createCaseAttachAlertAndDeleteAlert, createSecuritySolutionAlerts, getAlertById, getSecuritySolutionAlerts, @@ -19,7 +18,6 @@ import { deleteAllRules, } from '../../../../../detection_engine_api_integration/utils'; import { FtrProviderContext } from '../../../../common/ftr_provider_context'; -import { User } from '../../../../common/lib/authentication/types'; import { getPostCaseRequest, postCaseReq, postCommentUserReq } from '../../../../common/lib/mock'; import { @@ -113,93 +111,6 @@ export default ({ getService }: FtrProviderContext): void => { describe('alerts', () => { type Alerts = Array<{ _id: string; _index: string }>; - const createCaseAttachAlertAndDeleteAlert = async ({ - totalCases, - indexOfCaseToDelete, - owner, - expectedHttpCode = 204, - deleteCommentAuth = { user: superUser, space: 'space1' }, - alerts, - getAlerts, - }: { - totalCases: number; - indexOfCaseToDelete: number; - owner: string; - expectedHttpCode?: number; - deleteCommentAuth?: { user: User; space: string | null }; - alerts: Alerts; - getAlerts: (alerts: Alerts) => Promise>>; - }) => { - const cases = await Promise.all( - [...Array(totalCases).keys()].map((index) => - createCase( - supertestWithoutAuth, - { - ...postCaseReq, - owner, - settings: { syncAlerts: false }, - }, - 200, - { user: superUser, space: 'space1' } - ) - ) - ); - - const updatedCases = []; - - for (const theCase of cases) { - const updatedCase = await createComment({ - supertest: supertestWithoutAuth, - caseId: theCase.id, - params: { - alertId: alerts.map((alert) => alert._id), - index: alerts.map((alert) => alert._index), - rule: { - id: 'id', - name: 'name', - }, - owner, - type: CommentType.alert, - }, - auth: { user: superUser, space: 'space1' }, - }); - - updatedCases.push(updatedCase); - } - - const caseIds = updatedCases.map((theCase) => theCase.id); - - const updatedAlerts = await getAlerts(alerts); - - for (const alert of updatedAlerts) { - expect(alert[ALERT_CASE_IDS]).eql(caseIds); - } - - const caseToDelete = updatedCases[indexOfCaseToDelete]; - const commentId = caseToDelete.comments![0].id; - - await deleteComment({ - supertest: supertestWithoutAuth, - caseId: caseToDelete.id, - commentId, - expectedHttpCode, - auth: deleteCommentAuth, - }); - - const alertAfterDeletion = await getAlerts(alerts); - - const caseIdsWithoutRemovedCase = - expectedHttpCode === 204 - ? updatedCases - .filter((theCase) => theCase.id !== caseToDelete.id) - .map((theCase) => theCase.id) - : updatedCases.map((theCase) => theCase.id); - - for (const alert of alertAfterDeletion) { - expect(alert[ALERT_CASE_IDS]).eql(caseIdsWithoutRemovedCase); - } - }; - describe('security_solution', () => { let alerts: Alerts = []; @@ -228,6 +139,7 @@ export default ({ getService }: FtrProviderContext): void => { it('removes a case from the alert schema when deleting an alert attachment', async () => { await createCaseAttachAlertAndDeleteAlert({ + supertest: supertestWithoutAuth, totalCases: 1, indexOfCaseToDelete: 0, owner: 'securitySolutionFixture', @@ -238,6 +150,7 @@ export default ({ getService }: FtrProviderContext): void => { it('should remove only one case', async () => { await createCaseAttachAlertAndDeleteAlert({ + supertest: supertestWithoutAuth, totalCases: 3, indexOfCaseToDelete: 1, owner: 'securitySolutionFixture', @@ -248,6 +161,7 @@ export default ({ getService }: FtrProviderContext): void => { it('should delete case ID from the alert schema when the user has write access to the indices and only read access to the siem solution', async () => { await createCaseAttachAlertAndDeleteAlert({ + supertest: supertestWithoutAuth, totalCases: 1, indexOfCaseToDelete: 0, owner: 'securitySolutionFixture', @@ -260,6 +174,7 @@ export default ({ getService }: FtrProviderContext): void => { it('should NOT delete case ID from the alert schema when the user does NOT have access to the alert', async () => { await createCaseAttachAlertAndDeleteAlert({ + supertest: supertestWithoutAuth, totalCases: 1, indexOfCaseToDelete: 0, owner: 'securitySolutionFixture', @@ -272,6 +187,7 @@ export default ({ getService }: FtrProviderContext): void => { it('should delete the case ID from the alert schema when the user has read access to the kibana feature but no read access to the ES index', async () => { await createCaseAttachAlertAndDeleteAlert({ + supertest: supertestWithoutAuth, totalCases: 1, indexOfCaseToDelete: 0, owner: 'securitySolutionFixture', @@ -314,6 +230,7 @@ export default ({ getService }: FtrProviderContext): void => { it('removes a case from the alert schema when deleting an alert attachment', async () => { await createCaseAttachAlertAndDeleteAlert({ + supertest: supertestWithoutAuth, totalCases: 1, indexOfCaseToDelete: 0, owner: 'observabilityFixture', @@ -324,6 +241,7 @@ export default ({ getService }: FtrProviderContext): void => { it('should remove only one case', async () => { await createCaseAttachAlertAndDeleteAlert({ + supertest: supertestWithoutAuth, totalCases: 3, indexOfCaseToDelete: 1, owner: 'observabilityFixture', @@ -334,6 +252,7 @@ export default ({ getService }: FtrProviderContext): void => { it('should delete case ID from the alert schema when the user has read access only', async () => { await createCaseAttachAlertAndDeleteAlert({ + supertest: supertestWithoutAuth, totalCases: 1, indexOfCaseToDelete: 0, expectedHttpCode: 204, @@ -346,6 +265,7 @@ export default ({ getService }: FtrProviderContext): void => { it('should NOT delete case ID from the alert schema when the user does NOT have access to the alert', async () => { await createCaseAttachAlertAndDeleteAlert({ + supertest: supertestWithoutAuth, totalCases: 1, indexOfCaseToDelete: 0, expectedHttpCode: 403, diff --git a/x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/delete_comments.ts b/x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/delete_comments.ts index e7bf9cc9f3e9d..7ac1cc4f9de77 100644 --- a/x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/delete_comments.ts +++ b/x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/delete_comments.ts @@ -6,9 +6,9 @@ */ import expect from '@kbn/expect'; -import { CommentType } from '@kbn/cases-plugin/common'; -import { ALERT_CASE_IDS } from '@kbn/rule-data-utils'; import { + Alerts, + createCaseAttachAlertAndDeleteAlert, createSecuritySolutionAlerts, getAlertById, getSecuritySolutionAlerts, @@ -19,9 +19,18 @@ import { deleteAllRules, } from '../../../../../detection_engine_api_integration/utils'; import { FtrProviderContext } from '../../../../common/ftr_provider_context'; -import { User } from '../../../../common/lib/authentication/types'; -import { getPostCaseRequest, postCaseReq, postCommentUserReq } from '../../../../common/lib/mock'; +import { + getPostCaseRequest, + persistableStateAttachment, + postCaseReq, + postCommentActionsReleaseReq, + postCommentActionsReq, + postCommentAlertReq, + postCommentUserReq, + postExternalReferenceESReq, + postExternalReferenceSOReq, +} from '../../../../common/lib/mock'; import { deleteAllCaseItems, deleteCasesByESQuery, @@ -31,6 +40,8 @@ import { createComment, deleteAllComments, superUserSpace1Auth, + bulkCreateAttachments, + getAllComments, } from '../../../../common/lib/api'; import { globalRead, @@ -101,93 +112,6 @@ export default ({ getService }: FtrProviderContext): void => { }); describe('alerts', () => { - type Alerts = Array<{ _id: string; _index: string }>; - - const createCaseAttachAlertAndDeleteAlert = async ({ - totalCases, - indexOfCaseToDelete, - owner, - expectedHttpCode = 204, - deleteCommentAuth = { user: superUser, space: 'space1' }, - alerts, - getAlerts, - }: { - totalCases: number; - indexOfCaseToDelete: number; - owner: string; - expectedHttpCode?: number; - deleteCommentAuth?: { user: User; space: string | null }; - alerts: Alerts; - getAlerts: (alerts: Alerts) => Promise>>; - }) => { - const cases = await Promise.all( - [...Array(totalCases).keys()].map((index) => - createCase( - supertestWithoutAuth, - { - ...postCaseReq, - owner, - settings: { syncAlerts: false }, - }, - 200, - { user: superUser, space: 'space1' } - ) - ) - ); - - const updatedCases = []; - - for (const theCase of cases) { - const updatedCase = await createComment({ - supertest: supertestWithoutAuth, - caseId: theCase.id, - params: { - alertId: alerts.map((alert) => alert._id), - index: alerts.map((alert) => alert._index), - rule: { - id: 'id', - name: 'name', - }, - owner, - type: CommentType.alert, - }, - auth: { user: superUser, space: 'space1' }, - }); - - updatedCases.push(updatedCase); - } - - const caseIds = updatedCases.map((theCase) => theCase.id); - - const updatedAlerts = await getAlerts(alerts); - - for (const alert of updatedAlerts) { - expect(alert[ALERT_CASE_IDS]).eql(caseIds); - } - - const caseToDelete = updatedCases[indexOfCaseToDelete]; - - await deleteAllComments({ - supertest: supertestWithoutAuth, - caseId: caseToDelete.id, - expectedHttpCode, - auth: deleteCommentAuth, - }); - - const alertAfterDeletion = await getAlerts(alerts); - - const caseIdsWithoutRemovedCase = - expectedHttpCode === 204 - ? updatedCases - .filter((theCase) => theCase.id !== caseToDelete.id) - .map((theCase) => theCase.id) - : updatedCases.map((theCase) => theCase.id); - - for (const alert of alertAfterDeletion) { - expect(alert[ALERT_CASE_IDS]).eql(caseIdsWithoutRemovedCase); - } - }; - describe('security_solution', () => { let alerts: Alerts = []; @@ -214,8 +138,50 @@ export default ({ getService }: FtrProviderContext): void => { await esArchiver.unload('x-pack/test/functional/es_archives/auditbeat/hosts'); }); + it('deletes alerts and comments', async () => { + const postedCase = await createCase(supertest, postCaseReq); + + await createComment({ + supertest, + caseId: postedCase.id, + params: postCommentUserReq, + }); + + await bulkCreateAttachments({ + supertest, + caseId: postedCase.id, + params: [ + { + ...postCommentAlertReq, + alertId: alerts[0]._id, + index: alerts[0]._index, + }, + { + ...postCommentAlertReq, + alertId: alerts[1]._id, + index: alerts[1]._index, + }, + postCommentUserReq, + postCommentActionsReq, + postCommentActionsReleaseReq, + postExternalReferenceESReq, + postExternalReferenceSOReq, + persistableStateAttachment, + ], + }); + + await deleteAllComments({ + supertest, + caseId: postedCase.id, + }); + + const comments = await getAllComments({ supertest, caseId: postedCase.id }); + expect(comments.length).to.eql(0); + }); + it('removes a case from the alert schema when deleting all alert attachments', async () => { await createCaseAttachAlertAndDeleteAlert({ + supertest: supertestWithoutAuth, totalCases: 1, indexOfCaseToDelete: 0, owner: 'securitySolutionFixture', @@ -226,6 +192,7 @@ export default ({ getService }: FtrProviderContext): void => { it('should remove only one case', async () => { await createCaseAttachAlertAndDeleteAlert({ + supertest: supertestWithoutAuth, totalCases: 3, indexOfCaseToDelete: 1, owner: 'securitySolutionFixture', @@ -236,6 +203,7 @@ export default ({ getService }: FtrProviderContext): void => { it('should delete case ID from the alert schema when the user has write access to the indices and only read access to the siem solution', async () => { await createCaseAttachAlertAndDeleteAlert({ + supertest: supertestWithoutAuth, totalCases: 1, indexOfCaseToDelete: 0, owner: 'securitySolutionFixture', @@ -248,6 +216,7 @@ export default ({ getService }: FtrProviderContext): void => { it('should NOT delete case ID from the alert schema when the user does NOT have access to the alert', async () => { await createCaseAttachAlertAndDeleteAlert({ + supertest: supertestWithoutAuth, totalCases: 1, indexOfCaseToDelete: 0, owner: 'securitySolutionFixture', @@ -260,6 +229,7 @@ export default ({ getService }: FtrProviderContext): void => { it('should delete the case ID from the alert schema when the user has read access to the kibana feature but no read access to the ES index', async () => { await createCaseAttachAlertAndDeleteAlert({ + supertest: supertestWithoutAuth, totalCases: 1, indexOfCaseToDelete: 0, owner: 'securitySolutionFixture', @@ -300,8 +270,50 @@ export default ({ getService }: FtrProviderContext): void => { await esArchiver.unload('x-pack/test/functional/es_archives/rule_registry/alerts'); }); + it('deletes alerts and comments', async () => { + const postedCase = await createCase(supertest, postCaseReq); + + await createComment({ + supertest, + caseId: postedCase.id, + params: postCommentUserReq, + }); + + await bulkCreateAttachments({ + supertest, + caseId: postedCase.id, + params: [ + { + ...postCommentAlertReq, + alertId: alerts[0]._id, + index: alerts[0]._index, + }, + { + ...postCommentAlertReq, + alertId: alerts[1]._id, + index: alerts[1]._index, + }, + postCommentUserReq, + postCommentActionsReq, + postCommentActionsReleaseReq, + postExternalReferenceESReq, + postExternalReferenceSOReq, + persistableStateAttachment, + ], + }); + + await deleteAllComments({ + supertest, + caseId: postedCase.id, + }); + + const comments = await getAllComments({ supertest, caseId: postedCase.id }); + expect(comments.length).to.eql(0); + }); + it('removes a case from the alert schema when deleting all alert attachments', async () => { await createCaseAttachAlertAndDeleteAlert({ + supertest: supertestWithoutAuth, totalCases: 1, indexOfCaseToDelete: 0, owner: 'observabilityFixture', @@ -312,6 +324,7 @@ export default ({ getService }: FtrProviderContext): void => { it('should remove only one case', async () => { await createCaseAttachAlertAndDeleteAlert({ + supertest: supertestWithoutAuth, totalCases: 3, indexOfCaseToDelete: 1, owner: 'observabilityFixture', @@ -322,6 +335,7 @@ export default ({ getService }: FtrProviderContext): void => { it('should delete case ID from the alert schema when the user has read access only', async () => { await createCaseAttachAlertAndDeleteAlert({ + supertest: supertestWithoutAuth, totalCases: 1, indexOfCaseToDelete: 0, expectedHttpCode: 204, @@ -334,6 +348,7 @@ export default ({ getService }: FtrProviderContext): void => { it('should NOT delete case ID from the alert schema when the user does NOT have access to the alert', async () => { await createCaseAttachAlertAndDeleteAlert({ + supertest: supertestWithoutAuth, totalCases: 1, indexOfCaseToDelete: 0, expectedHttpCode: 403,