[Cases] Delete file API #153604

jonathan-buttner · 2023-03-23T20:56:45Z

This PR adds a new API for deleting a file within a case given the file id.

This API will retrieve the file saved object provided in the query and perform an authorization check using each file's file kind. It will also retrieve all the attachments associated with the files and perform an authorization check for each attachment. This api supports calling it with ids that only have the file saved objects and not the corresponding attachments. For the deletion sub privilege to work correctly, it must have access to updating the file saved objects. Therefore we also had to give the delete sub privilege all access to the file saved objects types.

This PR does not contain the logic for deleting all files when a case is deleted. That'll be completed in a separate PR.

Example request

POST /internal/cases/a58847c0-cccc-11ed-b071-4f11aa24310c/attachments/files/_bulk_delete
{
  "ids": ["clfr5sdky0001n811gjot7tv5", "clfr5sgru0002n8112t54bave"]
}

Example response

Notable changes

Refactored the delete all comments to leverage the bulk delete API from the saved object client
Updated the names of the api_integration users and roles to avoid clashing with the ones in cases_api_integration

jonathan-buttner · 2023-03-27T16:00:15Z

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

 export const MAX_FILE_SIZE = 100 * 1024 * 1024; // 100 MiB
-
-export const constructFilesHttpOperationTag = (owner: Owner, operation: HttpApiTagOperation) => {


Moved to common/files/index.ts since they aren't really constants

jonathan-buttner · 2023-03-27T16:03:40Z

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

+
+export const CaseFileMetadataRt = rt.type({
+  // TODO: do we want this as an array?
+  caseId: rt.string,


I think we'd need this as an array if we wanted to support having a single file be associated with multiple cases (aka the file picker). I don't know if we'll want to do that, but just in case should we format this as an array for now?

jonathan-buttner · 2023-03-27T16:03:56Z

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

+  // TODO: do we want this as an array?
+  caseId: rt.string,
+  // TODO: do we want this as an array?
+  owner: rt.array(rt.string),


Just checking that we're in agreement on this being an array.

The owner also? In case we are deleting multiple files with different owners?

Yeah I don't think we have a use case currently for an array but we discussed it here: https://github.com/elastic/kibana/pull/152941/files#r1145115748

jonathan-buttner · 2023-03-27T16:13:26Z

x-pack/test/api_integration/apis/cases/common/roles.ts

@@ -12,7 +12,7 @@ import { Role } from '../../../../cases_api_integration/common/lib/authenticatio
 */

 export const secAllCasesOnlyDelete: Role = {
-  name: 'sec_all_cases_only_delete',
+  name: 'sec_all_cases_only_delete_api_int',


I'm appending api_int to these because I need to use these users/roles in the case_api_integration tests and I want to avoid collisions with the users/roles defined in cases_api_integration. I need to use these tests because the fixture users/roles aren't registered with the file kind.

jonathan-buttner · 2023-03-27T18:21:08Z

x-pack/test/cases_api_integration/common/lib/api/files.ts

+  }
+};
+
+export const deleteAllFiles = async ({


This is mostly used for clean up to remove the files once the tests are completed.

jonathan-buttner · 2023-03-27T18:21:53Z

...st/cases_api_integration/security_and_spaces/tests/trial/internal/delete_file_attachments.ts

@@ -0,0 +1,136 @@
+/*


The deletion sub privilege is only available under a trial license so these tests are specifically for a user with the deletion sub privilege.

elasticmachine · 2023-03-27T20:05:41Z

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

elasticmachine · 2023-03-27T20:05:42Z

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

jonathan-buttner · 2023-03-27T20:21:24Z

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

+  });
+
+  const [validFiles, invalidFiles] = partition(files, (file) => {
+    return CaseFileMetadataRt.is(file.data.meta) && file.data.meta.caseId === caseId;


@cnasikas should we be more lenient here? For example lets say a user manually created a file and formatted the metadata incorrectly. If we are going to prevent deletion through the file management page, the user wouldn't have anyway to delete the file.

Maybe instead we just check that the metadata contains the caseId field and ignores the format of the owner since this function doesn't actually need it.

We can still enforce that the caseId has to be present and match the passed in caseId from the request.

jonathan-buttner · 2023-03-27T20:52:26Z

@elasticmachine merge upstream

…a into cases-file-delete

cnasikas

I have not checked the code yet but I think we should change the API to be POST /internal/cases/<case_ids>/attachments/files/_bulk_delete or POST /internal/cases/<case_ids>/files/_bulk_deletewith body { ids: [] }. The reason is that if the ids are a lot some proxies or gateway may trim the query params and lose some of the ids or even break an id in the middle making it invalid. Also, the _<whatever> is usually used in ES & Kibana to denote an operation and not a resource. Finally, we should put a limit to the number of files someone can delete with the API (you may have it already. I did not check 🙂 ).

jonathan-buttner · 2023-03-28T12:12:27Z

I have not checked the code yet but I think we should change the API to be POST /internal/cases/<case_ids>/attachments/files/_bulk_delete

Great point. Will do!

Finally, we should put a limit to the number of files someone can delete with the API (you may have it already. I did not check 🙂 ).

Yep, it's currently set at 500 (I just did 5 times the max number of files on a case), happy to lower it though.

semd

SecuritySolution changes LGTM

kdelemme

actionable-observability code change looks good to me !

adcoelho · 2023-03-30T13:22:21Z

x-pack/plugins/cases/server/client/attachments/bulk_delete.ts

+    const fileEntities = await getFileEntities(caseId, request.ids, fileService);
+
+    // It's possible for this to return an empty array if there was an error creating file attachments in which case the
+    // file would be present but the case attachment would not


// It's possible for this to return an empty array if there was an error creating file attachments in which case the // file would be present but the case attachment would not

I'd like to think this won't happen based on the AddFile implementation.

haha yeah let's hope not!

adcoelho · 2023-03-30T13:38:30Z

x-pack/plugins/cases/server/services/attachments/operations/get.ts

+       * to retrieve them all.
+       */
+      const finder =
+        this.context.unsecuredSavedObjectsClient.createPointInTimeFinder<AttachmentAttributesWithoutRefs>(


I didn't know this createPointInTimeFinder API so I was reading the saved_objects_client docs.

Considering we have the file ids references already are we really anticipating:

page through large sets of saved objects. We strongly recommend using this API for
any find queries that might return more than 1000 saved objects, [...]

?

Yeah this is more of a safety measure. It's probably unlikely that we'd get more than 100 (or 50 if we lower the max number of files on a case). We could use the find api to do the same thing but it's limited to 10k results where as this will get all of them.

adcoelho · 2023-03-30T13:44:49Z

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

+  rt.identity
+);
+
+export const limitedArraySchema = <T extends rt.Mixed>(codec: T, min: number, max: number) =>


this looks like an incredibly complicated way of checking an array size 😆

Seems like magic.

haha, yep this is exactly why Christos and I wanted to switch from io-ts to zod or anything else that isn't this convoluted. I did this as an example of what we're going to need to do for the road to versioned HTTP apis.

cnasikas · 2023-03-30T13:49:06Z

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

-
-export const constructFileKindIdByOwner = (owner: Owner) => `${owner}FilesCases`;
+export const MAX_FILES_PER_CASE = 100;
+export const MAX_DELETE_FILES = MAX_FILES_PER_CASE * 5;


Given that the file deletion is slow what do you think about reducing the number to 50 (the maximum number of files shown on one page in the UI table)?

Sounds reasonable to me!

cnasikas · 2023-03-30T13:51:28Z

x-pack/plugins/cases/common/files/index.test.ts

+
+import { constructOwnerFromFileKind } from '.';
+
+describe('files index', () => {


Do you mind if you add one test for constructFilesHttpOperationTag and one for constructFileKindIdByOwner?

cnasikas · 2023-03-30T13:52:08Z

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

+export type CaseFileMetadata = rt.TypeOf<typeof CaseFileMetadataForDeletionRt>;
+
+export const constructFilesHttpOperationTag = (owner: Owner, operation: HttpApiTagOperation) => {
+  return `${owner}FilesCases${operation}`;


Let's also put the FILE_KIND_DELIMITER here.

adcoelho · 2023-03-30T13:54:32Z

x-pack/plugins/cases/server/client/attachments/bulk_delete.ts

+          id: attachment.id,
+          owner: attachment.attributes.owner,
+        })),
+        ...fileEntities,


In addition to checking if the user can delete the attachments is this checking if he can also delete the files in the fileService?

And if so, is operation: Operations.deleteComment the correct one for that, isn't this Cases specific?

is this checking if he can also delete the files in the fileService?

Sort of. The actual check for whether a user can delete a file service saved object is done within the saved object client. The way we give a user access to deleting the file service saved objects is when we implement the features within each plugin for example: https://github.com/elastic/kibana/blob/main/x-pack/plugins/cases/server/features.ts#L54

So if a user has a role that grants the All feature privilege within cases they will be authorized to delete file service saved objects. Technically that user could use the exposed saved object API to delete those file saved objects: https://www.elastic.co/guide/en/kibana/master/saved-objects-api-delete.html

The check we're doing here is one extra safety measure that ensures that the user does have access to the owner specified for that file saved object based on the file kind. It's unlikely that they wouldn't have access though because they'd have to manually create a file that has a reference to a case plugin outside of their access.

operation: Operations.deleteComment the correct one for that, isn't this Cases specific?

For now I think it's ok to leverage the deleteComment operation because we're defining the whole operation as a deleteComment. For example when you delete a case we specify the operation as delete case but in reality it's a combination of operations (deleting the case, comments, user actions, etc). If in the future we want to allow a user to delete a comment/attachment but not a file then we'll need to rethink this model and would likely need another operation here.

cnasikas · 2023-03-30T14:01:53Z

x-pack/plugins/cases/server/common/utils.ts

@@ -470,3 +477,16 @@ export const getCaseViewPath = (params: {

  return `${basePath}${normalizePath(CASE_VIEW_PATH.replace(':detailName', caseId))}`;
 };
+
+export const partitionByCaseAssociation = (caseId: string, attachments: AttachmentSavedObject[]) =>


Can you add some unit tests for the new functions? (I know there was no test for them. My fault.)

cnasikas · 2023-03-30T14:13:50Z

x-pack/plugins/cases/server/client/attachments/bulk_delete.ts

+) => {
+  // 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 }), {


@elastic/appex-sharedux Is it possible to expose a bulk get operation to improve performance?

cnasikas · 2023-03-30T14:14:03Z

x-pack/plugins/cases/server/client/attachments/bulk_delete.ts

+    });
+
+    await Promise.all([
+      pMap(request.ids, async (fileId: string) => fileService.delete({ id: fileId }), {


@elastic/appex-sharedux Is it possible to expose a bulk delete operation to improve performance?

cnasikas

Great job!

cnasikas · 2023-03-31T14:13:47Z

...es_api_integration/security_and_spaces/tests/common/internal/bulk_delete_file_attachments.ts

+          supertest,
+          caseId: postedCase.id,
+          fileIds: ['abc'],
+          expectedHttpCode: 500,


Should not this be 404?

Great point. The issue here was that the file service isn't returning the saved object error (which would have been a boom) it was wrapping it in its own FileNotFound error class. Our cases code only looks for booms to grab the status from. So since our code was encountering a non-boom it defaults to 500. I added a simple fix to do an error instanceof FileNotFound so we can get the 400. The file service does a similar check before it returns from their route.

cnasikas · 2023-03-31T14:14:14Z

...es_api_integration/security_and_spaces/tests/common/internal/bulk_delete_file_attachments.ts

+      });
+
+      it('returns a 400 when there are 501 ids being deleted', async () => {
+        const ids = Array.from(Array(501).keys()).map((item) => item.toString());


We need to change this to 51, right?

cnasikas · 2023-03-31T14:39:29Z

...es_api_integration/security_and_spaces/tests/common/internal/bulk_delete_file_attachments.ts

+      });
+    });
+
+    describe('rbac', () => {


Can you please add a test where we upload a file with a different file kind, not owned by cases (I think we can register a dummy one in our cases fixture plugin), with the exact same metadata (valid case ID) as expected by cases, attach the file to the case (only create an attachment), and see if we get a 403 when we try to delete it?

Great idea I'll add the test, we'll actually get a 400 bad request because we can't determine the owner. Does a 400 seem appropriate to you?

400 is also fine 🙂.

kibana-ci · 2023-04-03T16:37:11Z

💚 Build Succeeded

Buildkite Build
Commit: f5ecd3d

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`cases`	565	567	+2

Adoption-tracked APIs that are not used anywhere

APIs whose adoption is being tracked and are still not being used. This number should tend to zero.

id	before	after	diff
`files`	1	0	-1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`cases`	364.3KB	364.3KB	+4.0B

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.3KB	134.1KB	+769.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`securitySolution`	432	435	+3

Total ESLint disabled count

id	before	after	diff
`securitySolution`	512	515	+3

History

💔 Build #117376 failed eaf06f6
💔 Build #117363 failed 1b06eea
💔 Build #117208 failed 4e5efcf
💚 Build #116882 succeeded e9cb718
💚 Build #116569 succeeded 8792545
💔 Build #116546 failed 0d3e4be

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

jonathan-buttner added 3 commits March 21, 2023 17:27

Adding delete client function

5bea85a

Making progress

24bc49e

Adding integration tests

e90b81b

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 23, 2023

jonathan-buttner added 4 commits March 24, 2023 18:07

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

jonathan-buttner commented Mar 27, 2023

View reviewed changes

jonathan-buttner and others added 2 commits March 27, 2023 14:34

Fixing test error

08197fb

Merge branch 'main' into cases-file-delete

26eb6c6

jonathan-buttner marked this pull request as ready for review March 27, 2023 20:05

jonathan-buttner requested review from a team as code owners March 27, 2023 20:05

jonathan-buttner commented Mar 27, 2023

View reviewed changes

kibanamachine and others added 4 commits March 27, 2023 16:52

Merge branch 'main' into cases-file-delete

5a2bcc5

Merge branch 'main' into cases-file-delete

514488d

Removing type

bc54bf6

Merge branch 'cases-file-delete' of github.com:jonathan-buttner/kiban…

7a198c4

…a into cases-file-delete

cnasikas mentioned this pull request Mar 28, 2023

[Cases] Implement files backend changes #151933

Closed

13 tasks

cnasikas reviewed Mar 28, 2023

View reviewed changes

Merge branch 'main' into cases-file-delete

204168a

Adding space test

0d3e4be

jonathan-buttner mentioned this pull request Mar 29, 2023

Cases delete files case deletion #153979

Merged

jonathan-buttner added 2 commits March 29, 2023 16:44

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

cb007ae

Fixing space test

8792545

jonathan-buttner mentioned this pull request Mar 29, 2023

[Cases] File improvements and other clean up #154011

Merged

semd approved these changes Mar 30, 2023

View reviewed changes

kdelemme approved these changes Mar 30, 2023

View reviewed changes

adcoelho reviewed Mar 30, 2023

View reviewed changes

cnasikas reviewed Mar 30, 2023

View reviewed changes

adcoelho reviewed Mar 30, 2023

View reviewed changes

cnasikas reviewed Mar 30, 2023

View reviewed changes

jonathan-buttner added 2 commits March 30, 2023 14:08

Addressing feedback

5688ce4

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

e9cb718

cnasikas approved these changes Mar 31, 2023

View reviewed changes

jonathan-buttner added 2 commits March 31, 2023 15:38

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

69ec214

Adding test and fixing 404 issue

4e5efcf

cnasikas mentioned this pull request Apr 3, 2023

[Cases][Meta]: File attachments #151595

Closed

5 tasks

jonathan-buttner and others added 3 commits April 3, 2023 08:52

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

1b06eea

Fixing type

eaf06f6

Merge branch 'main' into cases-file-delete

f5ecd3d

jonathan-buttner merged commit 1e63515 into elastic:main Apr 3, 2023

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

jonathan-buttner deleted the cases-file-delete branch April 3, 2023 16:39

		export const MAX_FILE_SIZE = 100 * 1024 * 1024; // 100 MiB

		export const constructFilesHttpOperationTag = (owner: Owner, operation: HttpApiTagOperation) => {


		import { constructOwnerFromFileKind } from '.';

		describe('files index', () => {

[Cases] Delete file API #153604

[Cases] Delete file API #153604

Conversation

jonathan-buttner commented Mar 23, 2023 • edited Loading

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 Mar 27, 2023

elasticmachine commented Mar 27, 2023

Choose a reason for hiding this comment

jonathan-buttner commented Mar 27, 2023

cnasikas left a comment • edited Loading

Choose a reason for hiding this comment

jonathan-buttner commented Mar 28, 2023

semd left a comment

Choose a reason for hiding this comment

kdelemme left a comment

Choose a reason for hiding this comment

adcoelho Mar 30, 2023 • edited Loading

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

cnasikas Mar 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathan-buttner Mar 30, 2023 • edited Loading

Choose a reason for hiding this comment

cnasikas Mar 30, 2023 • edited Loading

Choose a reason for hiding this comment

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

cnasikas Mar 31, 2023 • edited Loading

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 3, 2023

💚 Build Succeeded

Metrics [docs]

Module Count

Adoption-tracked APIs that are not used anywhere

Async chunks

Page load bundle

ESLint disabled line counts

Total ESLint disabled count

History

jonathan-buttner commented Mar 23, 2023 •

edited

Loading

cnasikas left a comment •

edited

Loading

adcoelho Mar 30, 2023 •

edited

Loading

cnasikas Mar 30, 2023 •

edited

Loading

jonathan-buttner Mar 30, 2023 •

edited

Loading

cnasikas Mar 30, 2023 •

edited

Loading

cnasikas Mar 31, 2023 •

edited

Loading