-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cases delete files case deletion #153979
Cases delete files case deletion #153979
Changes from all commits
5bea85a
24bc49e
e90b81b
94ff3e1
f7f1b22
662caef
e522345
08197fb
26eb6c6
3c1d04d
b41a6ff
9067a19
4b7cea7
bf7bff5
3b514f1
e33c041
acf1ad5
6f72f9e
770c786
ea37ca6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,7 @@ import { | |
INTERNAL_CONNECTORS_URL, | ||
INTERNAL_CASE_USERS_URL, | ||
INTERNAL_DELETE_FILE_ATTACHMENTS_URL, | ||
CASE_FIND_ATTACHMENTS_URL, | ||
} from '../constants'; | ||
|
||
export const getCaseDetailsUrl = (id: string): string => { | ||
|
@@ -39,6 +40,10 @@ export const getCaseCommentDetailsUrl = (caseId: string, commentId: string): str | |
return CASE_COMMENT_DETAILS_URL.replace('{case_id}', caseId).replace('{comment_id}', commentId); | ||
}; | ||
|
||
export const getCaseFindAttachmentsUrl = (caseId: string): string => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just adding this to make the integration tests a little easier |
||
return CASE_FIND_ATTACHMENTS_URL.replace('{case_id}', caseId); | ||
}; | ||
|
||
export const getCaseCommentDeleteUrl = (caseId: string, commentId: string): string => { | ||
return CASE_COMMENT_DELETE_URL.replace('{case_id}', caseId).replace('{comment_id}', commentId); | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,6 +47,7 @@ export const CASE_CONFIGURE_DETAILS_URL = `${CASES_URL}/configure/{configuration | |
export const CASE_CONFIGURE_CONNECTORS_URL = `${CASE_CONFIGURE_URL}/connectors` as const; | ||
|
||
export const CASE_COMMENTS_URL = `${CASE_DETAILS_URL}/comments` as const; | ||
export const CASE_FIND_ATTACHMENTS_URL = `${CASE_COMMENTS_URL}/_find` as const; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mind if you change the |
||
export const CASE_COMMENT_DETAILS_URL = `${CASE_DETAILS_URL}/comments/{comment_id}` as const; | ||
export const CASE_COMMENT_DELETE_URL = `${CASE_DETAILS_URL}/comments/{comment_id}` as const; | ||
export const CASE_PUSH_URL = `${CASE_DETAILS_URL}/connector/{connector_id}/_push` as const; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
/* | ||
* 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 { MAX_FILES_PER_CASE } from '../../../common/constants'; | ||
import type { FindFileArgs } from '@kbn/files-plugin/server'; | ||
import { createFileServiceMock } from '@kbn/files-plugin/server/mocks'; | ||
import type { FileJSON } from '@kbn/shared-ux-file-types'; | ||
import type { CaseFileMetadataForDeletion } from '../../../common/files'; | ||
import { constructFileKindIdByOwner } from '../../../common/files'; | ||
import { getFileEntities } from './delete'; | ||
|
||
const getCaseIds = (numIds: number) => { | ||
return Array.from(Array(numIds).keys()).map((key) => key.toString()); | ||
}; | ||
describe('delete', () => { | ||
describe('getFileEntities', () => { | ||
const numCaseIds = 1000; | ||
const caseIds = getCaseIds(numCaseIds); | ||
const mockFileService = createFileServiceMock(); | ||
mockFileService.find.mockImplementation(async (args: FindFileArgs) => { | ||
const caseMeta = args.meta as unknown as CaseFileMetadataForDeletion; | ||
const numFilesToGen = caseMeta.caseIds.length * MAX_FILES_PER_CASE; | ||
const files = Array.from(Array(numFilesToGen).keys()).map(() => createMockFileJSON()); | ||
|
||
return { | ||
files, | ||
total: files.length, | ||
}; | ||
}); | ||
|
||
beforeEach(() => { | ||
jest.clearAllMocks(); | ||
}); | ||
|
||
it('only provides 50 case ids in a single call to the find api', async () => { | ||
await getFileEntities(caseIds, mockFileService); | ||
|
||
for (const call of mockFileService.find.mock.calls) { | ||
const callMeta = call[0].meta as unknown as CaseFileMetadataForDeletion; | ||
expect(callMeta.caseIds.length).toEqual(50); | ||
} | ||
}); | ||
|
||
it('calls the find function the number of case ids divided by the chunk size', async () => { | ||
await getFileEntities(caseIds, mockFileService); | ||
|
||
const chunkSize = 50; | ||
|
||
expect(mockFileService.find).toHaveBeenCalledTimes(numCaseIds / chunkSize); | ||
}); | ||
|
||
it('returns the number of entities equal to the case ids times the max files per case limit', async () => { | ||
const expectedEntities = Array.from(Array(numCaseIds * MAX_FILES_PER_CASE).keys()).map( | ||
() => ({ | ||
id: '123', | ||
owner: 'securitySolution', | ||
}) | ||
); | ||
|
||
const entities = await getFileEntities(caseIds, mockFileService); | ||
|
||
expect(entities.length).toEqual(numCaseIds * MAX_FILES_PER_CASE); | ||
expect(entities).toEqual(expectedEntities); | ||
}); | ||
}); | ||
}); | ||
|
||
const createMockFileJSON = (): FileJSON => { | ||
return { | ||
id: '123', | ||
fileKind: constructFileKindIdByOwner('securitySolution'), | ||
meta: { | ||
owner: ['securitySolution'], | ||
}, | ||
} as unknown as FileJSON; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,27 +6,32 @@ | |
*/ | ||
|
||
import { Boom } from '@hapi/boom'; | ||
import pMap from 'p-map'; | ||
import { chunk } from 'lodash'; | ||
import type { SavedObjectsBulkDeleteObject } from '@kbn/core/server'; | ||
import type { FileServiceStart } from '@kbn/files-plugin/server'; | ||
import { | ||
CASE_COMMENT_SAVED_OBJECT, | ||
CASE_SAVED_OBJECT, | ||
CASE_USER_ACTION_SAVED_OBJECT, | ||
MAX_FILES_PER_CASE, | ||
MAX_DOCS_PER_PAGE, | ||
} from '../../../common/constants'; | ||
import type { CasesClientArgs } from '..'; | ||
import { createCaseError } from '../../common/error'; | ||
import type { OwnerEntity } from '../../authorization'; | ||
import { Operations } from '../../authorization'; | ||
import { createFileEntities, deleteFiles } from '../files'; | ||
|
||
/** | ||
* Deletes the specified cases and their attachments. | ||
* | ||
* @ignore | ||
*/ | ||
export async function deleteCases(ids: string[], clientArgs: CasesClientArgs): Promise<void> { | ||
const { | ||
services: { caseService, attachmentService, userActionService }, | ||
logger, | ||
authorization, | ||
fileService, | ||
} = clientArgs; | ||
try { | ||
const cases = await caseService.getCases({ caseIds: ids }); | ||
|
@@ -44,9 +49,11 @@ export async function deleteCases(ids: string[], clientArgs: CasesClientArgs): P | |
entities.set(theCase.id, { id: theCase.id, owner: theCase.attributes.owner }); | ||
} | ||
|
||
const fileEntities = await getFileEntities(ids, fileService); | ||
|
||
await authorization.ensureAuthorized({ | ||
operation: Operations.deleteCase, | ||
entities: Array.from(entities.values()), | ||
entities: [...Array.from(entities.values()), ...fileEntities], | ||
}); | ||
|
||
const attachmentIds = await attachmentService.getter.getAttachmentIdsForCases({ | ||
|
@@ -61,10 +68,14 @@ export async function deleteCases(ids: string[], clientArgs: CasesClientArgs): P | |
...userActionIds.map((id) => ({ id, type: CASE_USER_ACTION_SAVED_OBJECT })), | ||
]; | ||
|
||
await caseService.bulkDeleteCaseEntities({ | ||
entities: bulkDeleteEntities, | ||
options: { refresh: 'wait_for' }, | ||
}); | ||
const fileIds = fileEntities.map((entity) => entity.id); | ||
await Promise.all([ | ||
deleteFiles(fileIds, fileService), | ||
caseService.bulkDeleteCaseEntities({ | ||
entities: bulkDeleteEntities, | ||
options: { refresh: 'wait_for' }, | ||
}), | ||
]); | ||
|
||
await userActionService.creator.bulkAuditLogCaseDeletion( | ||
cases.saved_objects.map((caseInfo) => caseInfo.id) | ||
|
@@ -77,3 +88,29 @@ export async function deleteCases(ids: string[], clientArgs: CasesClientArgs): P | |
}); | ||
} | ||
} | ||
|
||
export const getFileEntities = async ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Chunking logic is here |
||
caseIds: string[], | ||
fileService: FileServiceStart | ||
): Promise<OwnerEntity[]> => { | ||
// using 50 just to be safe, each case can have 100 files = 50 * 100 = 5000 which is half the max number of docs that | ||
// the client can request | ||
const chunkSize = MAX_FILES_PER_CASE / 2; | ||
const chunkedIds = chunk(caseIds, chunkSize); | ||
|
||
const entityResults = await pMap(chunkedIds, async (ids: string[]) => { | ||
const findRes = await fileService.find({ | ||
perPage: MAX_DOCS_PER_PAGE, | ||
meta: { | ||
caseIds: ids, | ||
}, | ||
}); | ||
|
||
const fileEntities = createFileEntities(findRes.files); | ||
return fileEntities; | ||
}); | ||
|
||
const entities = entityResults.flatMap((res) => res); | ||
|
||
return entities; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,77 @@ | ||
/* | ||
* 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 { createFileServiceMock } from '@kbn/files-plugin/server/mocks'; | ||
import { OBSERVABILITY_OWNER, SECURITY_SOLUTION_OWNER } from '../../../common'; | ||
import { constructFileKindIdByOwner } from '../../../common/files'; | ||
import { createFileEntities, deleteFiles } from '.'; | ||
|
||
describe('server files', () => { | ||
describe('createFileEntities', () => { | ||
it('returns an empty array when passed no files', () => { | ||
expect(createFileEntities([])).toEqual([]); | ||
}); | ||
|
||
it('throws an error when the file kind is not valid', () => { | ||
expect.assertions(1); | ||
|
||
expect(() => | ||
createFileEntities([{ fileKind: 'abc', id: '1' }]) | ||
).toThrowErrorMatchingInlineSnapshot(`"File id 1 has invalid file kind abc"`); | ||
}); | ||
|
||
it('throws an error when one of the file entities does not have a valid file kind', () => { | ||
expect.assertions(1); | ||
|
||
expect(() => | ||
createFileEntities([ | ||
{ fileKind: constructFileKindIdByOwner(SECURITY_SOLUTION_OWNER), id: '1' }, | ||
{ fileKind: 'abc', id: '2' }, | ||
]) | ||
).toThrowErrorMatchingInlineSnapshot(`"File id 2 has invalid file kind abc"`); | ||
}); | ||
|
||
it('returns an array of entities when the file kind is valid', () => { | ||
expect.assertions(1); | ||
|
||
expect( | ||
createFileEntities([ | ||
{ fileKind: constructFileKindIdByOwner(SECURITY_SOLUTION_OWNER), id: '1' }, | ||
{ fileKind: constructFileKindIdByOwner(OBSERVABILITY_OWNER), id: '2' }, | ||
]) | ||
).toEqual([ | ||
{ id: '1', owner: 'securitySolution' }, | ||
{ id: '2', owner: 'observability' }, | ||
]); | ||
}); | ||
}); | ||
|
||
describe('deleteFiles', () => { | ||
it('calls delete twice with the ids passed in', async () => { | ||
const fileServiceMock = createFileServiceMock(); | ||
|
||
expect.assertions(2); | ||
await deleteFiles(['1', '2'], fileServiceMock); | ||
|
||
expect(fileServiceMock.delete).toBeCalledTimes(2); | ||
expect(fileServiceMock.delete.mock.calls).toMatchInlineSnapshot(` | ||
Array [ | ||
Array [ | ||
Object { | ||
"id": "1", | ||
}, | ||
], | ||
Array [ | ||
Object { | ||
"id": "2", | ||
}, | ||
], | ||
] | ||
`); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid typescript complaining we need this as a string array because we want to find any files that are associated to an array of ids.
When I looked at the way the metadata query is being built here: https://github.com/elastic/kibana/blob/main/src/plugins/files/server/file_client/file_metadata_client/adapters/query_filters.ts it looks like the file service already handles an array of strings. Let me know if that's not correct though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it seems so 👍