Skip to content
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

Merged
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
5bea85a
Adding delete client function
jonathan-buttner Mar 21, 2023
24bc49e
Making progress
jonathan-buttner Mar 22, 2023
e90b81b
Adding integration tests
jonathan-buttner Mar 23, 2023
94ff3e1
Adding more rbac tests
jonathan-buttner Mar 24, 2023
f7f1b22
Adding more tests and using correct users for tests
jonathan-buttner Mar 27, 2023
662caef
Merge branch 'main' of github.com:elastic/kibana into cases-file-delete
jonathan-buttner Mar 27, 2023
e522345
Refactoring and fixing tests
jonathan-buttner Mar 27, 2023
08197fb
Fixing test error
jonathan-buttner Mar 27, 2023
26eb6c6
Merge branch 'main' into cases-file-delete
jonathan-buttner Mar 27, 2023
3c1d04d
Implementing deletion
jonathan-buttner Mar 28, 2023
b41a6ff
Merge branch 'main' of github.com:elastic/kibana into cases-delete-fi…
jonathan-buttner Mar 29, 2023
9067a19
Adding some tests
jonathan-buttner Mar 29, 2023
4b7cea7
Merge branch 'main' of github.com:elastic/kibana into cases-delete-fi…
jonathan-buttner Apr 3, 2023
bf7bff5
Fixing merge and tests
jonathan-buttner Apr 3, 2023
3b514f1
Adding failure test
jonathan-buttner Apr 3, 2023
e33c041
Merge branch 'main' of github.com:elastic/kibana into cases-delete-fi…
jonathan-buttner Apr 3, 2023
acf1ad5
Merge branch 'main' of github.com:elastic/kibana into cases-delete-fi…
jonathan-buttner Apr 4, 2023
6f72f9e
Fixing await issue
jonathan-buttner Apr 4, 2023
770c786
Merge branch 'main' of github.com:elastic/kibana into cases-delete-fi…
jonathan-buttner Apr 10, 2023
ea37ca6
Addressing feedback
jonathan-buttner Apr 10, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -105,5 +105,5 @@ export interface FindFileArgs extends Pagination {
/**
* File metadata values. These values are governed by the consumer.
*/
meta?: Record<string, string>;
meta?: Record<string, string | string[]>;
Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it seems so 👍

}
5 changes: 5 additions & 0 deletions x-pack/plugins/cases/common/api/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 => {
Expand All @@ -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 => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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);
};
Expand Down
6 changes: 6 additions & 0 deletions x-pack/plugins/cases/common/constants/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind if you change the path attribute here x-pack/plugins/cases/server/routes/api/comments/find_comments.ts to use the new variable (CASE_FIND_ATTACHMENTS_URL)?

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;
Expand Down Expand Up @@ -94,6 +95,11 @@ export const CONNECTORS_URL = `${ACTION_URL}/connectors` as const;
*/
export const MAX_ALERTS_PER_CASE = 1000 as const;

/**
* Requests
*/
export const MAX_DELETE_CASE_IDS = 100 as const;

/**
* Searching
*/
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/cases/common/files/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ export const CaseFileMetadataForDeletionRt = rt.type({
caseIds: rt.array(rt.string),
});

export type CaseFileMetadata = rt.TypeOf<typeof CaseFileMetadataForDeletionRt>;
export type CaseFileMetadataForDeletion = rt.TypeOf<typeof CaseFileMetadataForDeletionRt>;

const FILE_KIND_DELIMITER = 'FilesCases';

Expand Down
34 changes: 6 additions & 28 deletions x-pack/plugins/cases/server/client/attachments/bulk_delete.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,18 +12,18 @@ import { identity } from 'fp-ts/lib/function';

import pMap from 'p-map';
import { partition } from 'lodash';
import type { File } from '@kbn/files-plugin/common';
import type { File, FileJSON } from '@kbn/files-plugin/common';
import type { FileServiceStart } from '@kbn/files-plugin/server';
import { FileNotFoundError } from '@kbn/files-plugin/server/file_service/errors';
import { BulkDeleteFileAttachmentsRequestRt, excess, throwErrors } from '../../../common/api';
import { MAX_CONCURRENT_SEARCHES } from '../../../common/constants';
import type { CasesClientArgs } from '../types';
import { createCaseError } from '../../common/error';
import type { OwnerEntity } from '../../authorization';
import { Operations } from '../../authorization';
import type { BulkDeleteFileArgs } from './types';
import { constructOwnerFromFileKind, CaseFileMetadataForDeletionRt } from '../../../common/files';
import { CaseFileMetadataForDeletionRt } from '../../../common/files';
import type { CasesClient } from '../client';
import { createFileEntities, deleteFiles } from '../files';

export const bulkDeleteFileAttachments = async (
{ caseId, fileIds }: BulkDeleteFileArgs,
Expand Down Expand Up @@ -67,9 +67,7 @@ export const bulkDeleteFileAttachments = async (
});

await Promise.all([
pMap(request.ids, async (fileId: string) => fileService.delete({ id: fileId }), {
concurrency: MAX_CONCURRENT_SEARCHES,
}),
deleteFiles(request.ids, fileService),
attachmentService.bulkDelete({
attachmentIds: fileAttachments.map((so) => so.id),
refresh: false,
Expand Down Expand Up @@ -117,7 +115,7 @@ const getFiles = async (
caseId: BulkDeleteFileArgs['caseId'],
fileIds: BulkDeleteFileArgs['fileIds'],
fileService: FileServiceStart
) => {
): Promise<FileJSON[]> => {
// it's possible that we're trying to delete a file when an attachment wasn't created (for example if the create
// attachment request failed)
const files = await pMap(fileIds, async (fileId: string) => fileService.getById({ id: fileId }), {
Expand All @@ -143,25 +141,5 @@ const getFiles = async (
throw Boom.badRequest('Failed to find files to delete');
}

return validFiles;
};

const createFileEntities = (files: File[]) => {
const fileEntities: OwnerEntity[] = [];

// It's possible that the owner array could have invalid information in it so we'll use the file kind for determining if the user
// has the correct authorization for deleting these files
for (const fileInfo of files) {
const ownerFromFileKind = constructOwnerFromFileKind(fileInfo.data.fileKind);

if (ownerFromFileKind == null) {
throw Boom.badRequest(
`File id ${fileInfo.id} has invalid file kind ${fileInfo.data.fileKind}`
);
}

fileEntities.push({ id: fileInfo.id, owner: ownerFromFileKind });
}

return fileEntities;
return validFiles.map((fileInfo) => fileInfo.data);
};
71 changes: 71 additions & 0 deletions x-pack/plugins/cases/server/client/cases/delete.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
/*
* 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 entities = await getFileEntities(caseIds, mockFileService);

expect(entities.length).toEqual(numCaseIds * MAX_FILES_PER_CASE);
});
});
});

const createMockFileJSON = (): FileJSON => {
return {
id: '123',
fileKind: constructFileKindIdByOwner('securitySolution'),
meta: {
owner: ['securitySolution'],
},
} as unknown as FileJSON;
};
51 changes: 44 additions & 7 deletions x-pack/plugins/cases/server/client/cases/delete.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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_DELETE_CASE_IDS,
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 });
Expand All @@ -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({
Expand All @@ -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)
Expand All @@ -77,3 +88,29 @@ export async function deleteCases(ids: string[], clientArgs: CasesClientArgs): P
});
}
}

export const getFileEntities = async (
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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_DELETE_CASE_IDS / 2;
const chunkedIds = chunk(caseIds, chunkSize);

const fileResults = await pMap(chunkedIds, async (ids: string[]) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the difference between this and createPointInTimeFinder?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We talked about this offline, unfortunately we can't use a createPointInTimeFinder here since we're leveraging the file service.

const files = await fileService.find({
perPage: MAX_DOCS_PER_PAGE,
meta: {
caseIds: ids,
},
});

return files;
});

const files = fileResults.flatMap((res) => res.files);
const fileEntities = createFileEntities(files);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to do it inside pMap so we don't have to iterate a large list of files?


return fileEntities;
};
38 changes: 38 additions & 0 deletions x-pack/plugins/cases/server/client/files/index.ts
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add some unit tests for the two functions?

Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
/*
* 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 Boom from '@hapi/boom';
import type { FileJSON } from '@kbn/files-plugin/common';
import type { FileServiceStart } from '@kbn/files-plugin/server';
import pMap from 'p-map';
import { constructOwnerFromFileKind } from '../../../common/files';
import { MAX_CONCURRENT_SEARCHES } from '../../../common/constants';
import type { OwnerEntity } from '../../authorization';

export const createFileEntities = (files: FileJSON[]): OwnerEntity[] => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just moved these to be shared between the bulk delete files API within cases and the delete case code.

const fileEntities: OwnerEntity[] = [];

// It's possible that the owner array could have invalid information in it so we'll use the file kind for determining if the user
// has the correct authorization for deleting these files
for (const fileInfo of files) {
const ownerFromFileKind = constructOwnerFromFileKind(fileInfo.fileKind);

if (ownerFromFileKind == null) {
throw Boom.badRequest(`File id ${fileInfo.id} has invalid file kind ${fileInfo.fileKind}`);
}

fileEntities.push({ id: fileInfo.id, owner: ownerFromFileKind });
}

return fileEntities;
};

export const deleteFiles = async (fileIds: string[], fileService: FileServiceStart) => {
pMap(fileIds, async (fileId: string) => fileService.delete({ id: fileId }), {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elastic/appex-sharedux how feasible would it be to have a bulk delete files API?

Issue: #154286

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for creating an issue, we will triage.
I am not sure if there were any technical or API design limitations to not adding this api.

concurrency: MAX_CONCURRENT_SEARCHES,
});
};
7 changes: 5 additions & 2 deletions x-pack/plugins/cases/server/routes/api/cases/delete_cases.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

import { schema } from '@kbn/config-schema';

import { CASES_URL } from '../../../../common/constants';
import { CASES_URL, MAX_DELETE_CASE_IDS } from '../../../../common/constants';
import { createCaseError } from '../../../common/error';
import { createCasesRoute } from '../create_cases_route';

Expand All @@ -16,7 +16,10 @@ export const deleteCaseRoute = createCasesRoute({
path: CASES_URL,
params: {
query: schema.object({
ids: schema.arrayOf(schema.string()),
ids: schema.arrayOf(schema.string({ minLength: 1 }), {
minSize: 1,
maxSize: MAX_DELETE_CASE_IDS,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Limiting the deletion API based on the issue here: #146945

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, we cannot do that as it will be a breaking change.

}),
}),
},
handler: async ({ context, request, response }) => {
Expand Down
25 changes: 25 additions & 0 deletions x-pack/test/cases_api_integration/common/lib/api/attachments.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@ import {
CommentPatchRequest,
CommentRequest,
CommentResponse,
CommentsResponse,
CommentType,
getCaseFindAttachmentsUrl,
getCasesDeleteFileAttachmentsUrl,
} from '@kbn/cases-plugin/common/api';
import { User } from '../authentication/types';
Expand Down Expand Up @@ -280,3 +282,26 @@ export const bulkDeleteFileAttachments = async ({
.auth(auth.user.username, auth.user.password)
.expect(expectedHttpCode);
};

export const findAttachments = async ({
supertest,
caseId,
query = {},
expectedHttpCode = 200,
auth = { user: superUser, space: null },
}: {
supertest: SuperTest.SuperTest<SuperTest.Test>;
caseId: string;
query?: Record<string, unknown>;
expectedHttpCode?: number;
auth?: { user: User; space: string | null };
}): Promise<CommentsResponse> => {
const { body } = await supertest
.get(`${getSpaceUrlPrefix(auth.space)}${getCaseFindAttachmentsUrl(caseId)}`)
.set('kbn-xsrf', 'true')
.query(query)
.auth(auth.user.username, auth.user.password)
.expect(expectedHttpCode);

return body;
};
Loading