Cases delete files case deletion #153979

jonathan-buttner · 2023-03-29T20:15:01Z

This PR extends the case deletion functionality to delete any files that are attached to the case when a case is deleted.

To avoid attempting to delete too many files at once I chunk case ids into 50 at a time such that we're at most only deleting 5000 (50 case * 100 files per case) at once. That way we don't exceed the 10k find api limit.

Testing

Run the python script from here: https://github.com/elastic/cases-files-generator to generate some files for a case

Deleting the case should remove all files

…les-case-deletion

jonathan-buttner · 2023-04-03T20:08:18Z

src/plugins/files/server/file_service/file_action_types.ts

@@ -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[]>;


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.

Yes, it seems so 👍

jonathan-buttner · 2023-04-03T20:09:44Z

x-pack/plugins/cases/common/api/helpers.ts

@@ -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 => {


Just adding this to make the integration tests a little easier

jonathan-buttner · 2023-04-03T20:10:24Z

x-pack/plugins/cases/server/client/cases/delete.ts

@@ -77,3 +88,29 @@ export async function deleteCases(ids: string[], clientArgs: CasesClientArgs): P
    });
  }
 }
+
+export const getFileEntities = async (


Chunking logic is here

jonathan-buttner · 2023-04-03T20:17:33Z

x-pack/plugins/cases/server/client/files/index.ts

+import { MAX_CONCURRENT_SEARCHES } from '../../../common/constants';
+import type { OwnerEntity } from '../../authorization';
+
+export const createFileEntities = (files: FileJSON[]): OwnerEntity[] => {


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

jonathan-buttner · 2023-04-03T20:18:00Z

x-pack/plugins/cases/server/client/files/index.ts

+};
+
+export const deleteFiles = async (fileIds: string[], fileService: FileServiceStart) => {
+  pMap(fileIds, async (fileId: string) => fileService.delete({ id: fileId }), {


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

Issue: #154286

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.

jonathan-buttner · 2023-04-03T20:18:39Z

x-pack/plugins/cases/server/routes/api/cases/delete_cases.ts

-      ids: schema.arrayOf(schema.string()),
+      ids: schema.arrayOf(schema.string({ minLength: 1 }), {
+        minSize: 1,
+        maxSize: MAX_DELETE_CASE_IDS,


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

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

…les-case-deletion

elasticmachine · 2023-04-03T20:25:32Z

Pinging @elastic/response-ops (Team:ResponseOps)

elasticmachine · 2023-04-03T20:25:33Z

Pinging @elastic/response-ops-cases (Feature:Cases)

Dosant · 2023-04-04T09:27:52Z

src/plugins/files/server/file_service/file_action_types.ts

@@ -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[]>;


Yes, it seems so 👍

Dosant · 2023-04-04T09:31:48Z

x-pack/plugins/cases/server/client/files/index.ts

+};
+
+export const deleteFiles = async (fileIds: string[], fileService: FileServiceStart) => {
+  pMap(fileIds, async (fileId: string) => fileService.delete({ id: fileId }), {


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.

…les-case-deletion

cnasikas

Nice work! I left some comments.

cnasikas · 2023-04-04T15:31:36Z

x-pack/plugins/cases/common/constants/index.ts

@@ -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;


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)?

cnasikas · 2023-04-04T15:38:20Z

x-pack/plugins/cases/server/routes/api/cases/delete_cases.ts

-      ids: schema.arrayOf(schema.string()),
+      ids: schema.arrayOf(schema.string({ minLength: 1 }), {
+        minSize: 1,
+        maxSize: MAX_DELETE_CASE_IDS,


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

cnasikas · 2023-04-04T15:50:55Z

x-pack/plugins/cases/server/client/files/index.ts

Can you add some unit tests for the two functions?

cnasikas · 2023-04-04T15:57:56Z

x-pack/plugins/cases/server/client/cases/delete.ts

+  const chunkSize = MAX_DELETE_CASE_IDS / 2;
+  const chunkedIds = chunk(caseIds, chunkSize);
+
+  const fileResults = await pMap(chunkedIds, async (ids: string[]) => {


What is the difference between this and createPointInTimeFinder?

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

cnasikas · 2023-04-04T15:59:34Z

x-pack/plugins/cases/server/client/cases/delete.ts

+  const files = fileResults.flatMap((res) => res.files);
+  const fileEntities = createFileEntities(files);


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

…les-case-deletion

kibana-ci · 2023-04-10T15:55:59Z

💚 Build Succeeded

Buildkite Build
Commit: ea37ca6

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`cases`	133.5KB	133.6KB	+54.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`securitySolution`	433	436	+3

Total ESLint disabled count

id	before	after	diff
`securitySolution`	513	516	+3

History

💚 Build #117691 succeeded 6f72f9e
💔 Build #117541 failed e33c041
💔 Build #117500 failed 4b7cea7
💔 Build #116558 failed 9067a19

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

jonathan-buttner and others added 12 commits March 21, 2023 17:27

Adding delete client function

5bea85a

Making progress

24bc49e

Adding integration tests

e90b81b

Adding more rbac tests

94ff3e1

Adding more tests and using correct users for tests

f7f1b22

Merge branch 'main' of github.com:elastic/kibana into cases-file-delete

662caef

Refactoring and fixing tests

e522345

Fixing test error

08197fb

Merge branch 'main' into cases-file-delete

26eb6c6

Implementing deletion

3c1d04d

Merge branch 'main' of github.com:elastic/kibana into cases-delete-fi…

b41a6ff

…les-case-deletion

Adding some tests

9067a19

jonathan-buttner added release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Cases Cases feature v8.8.0 labels Mar 29, 2023

jonathan-buttner mentioned this pull request Mar 29, 2023

[Cases] Implement files backend changes #151933

Closed

13 tasks

cnasikas mentioned this pull request Apr 3, 2023

[Cases][Meta]: File attachments #151595

Closed

5 tasks

jonathan-buttner added 3 commits April 3, 2023 13:34

Merge branch 'main' of github.com:elastic/kibana into cases-delete-fi…

4b7cea7

…les-case-deletion

Fixing merge and tests

bf7bff5

Adding failure test

3b514f1

jonathan-buttner commented Apr 3, 2023

View reviewed changes

Merge branch 'main' of github.com:elastic/kibana into cases-delete-fi…

e33c041

…les-case-deletion

jonathan-buttner marked this pull request as ready for review April 3, 2023 20:25

jonathan-buttner requested review from a team as code owners April 3, 2023 20:25

Dosant approved these changes Apr 4, 2023

View reviewed changes

Merge branch 'main' of github.com:elastic/kibana into cases-delete-fi…

acf1ad5

…les-case-deletion

Fixing await issue

6f72f9e

cnasikas reviewed Apr 4, 2023

View reviewed changes

jonathan-buttner added 2 commits April 10, 2023 08:37

Merge branch 'main' of github.com:elastic/kibana into cases-delete-fi…

770c786

…les-case-deletion

Addressing feedback

ea37ca6

cnasikas approved these changes Apr 10, 2023

View reviewed changes

jonathan-buttner merged commit b6a113c into elastic:main Apr 10, 2023

kibanamachine added the backport:skip This commit does not require backporting label Apr 10, 2023

jonathan-buttner deleted the cases-delete-files-case-deletion branch April 10, 2023 16:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cases delete files case deletion #153979

Cases delete files case deletion #153979

jonathan-buttner commented Mar 29, 2023 •

edited

Loading

jonathan-buttner Apr 3, 2023

Dosant Apr 4, 2023

jonathan-buttner Apr 3, 2023

jonathan-buttner Apr 3, 2023

jonathan-buttner Apr 3, 2023

jonathan-buttner Apr 3, 2023

Dosant Apr 4, 2023

jonathan-buttner Apr 3, 2023

cnasikas Apr 4, 2023

elasticmachine commented Apr 3, 2023

elasticmachine commented Apr 3, 2023

Dosant Apr 4, 2023

Dosant Apr 4, 2023

cnasikas left a comment

cnasikas Apr 4, 2023

cnasikas Apr 4, 2023

cnasikas Apr 4, 2023

cnasikas Apr 4, 2023

jonathan-buttner Apr 10, 2023

cnasikas Apr 4, 2023

kibana-ci commented Apr 10, 2023

ESLint disabled line counts

Total ESLint disabled count

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

Cases delete files case deletion #153979

Cases delete files case deletion #153979

Conversation

jonathan-buttner commented Mar 29, 2023 • edited Loading

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Apr 3, 2023

elasticmachine commented Apr 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cnasikas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented Apr 10, 2023

💚 Build Succeeded

Metrics [docs]

Page load bundle

ESLint disabled line counts

Total ESLint disabled count

History

jonathan-buttner commented Mar 29, 2023 •

edited

Loading