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.

Sign up for GitHub

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 Cases API Guardrails #160846

Merged
merged 4 commits into from
Jun 30, 2023
Merged

[Cases] Delete Cases API Guardrails #160846

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
429dd71
Limit number of case ids in delete cases api.
adcoelho Jun 29, 2023
cc5688a
Merge remote-tracking branch 'upstream/main' into limit-max-delete-cases
adcoelho Jun 29, 2023
6d34670
Addressing pr comments.
adcoelho Jun 29, 2023
27cdcf8
Merge remote-tracking branch 'upstream/main' into limit-max-delete-cases
adcoelho Jun 30, 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
10 changes: 10 additions & 0 deletions x-pack/plugins/cases/common/api/cases/case.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import { CommentRt } from './comment';
import { CasesStatusResponseRt, CaseStatusRt } from './status';
import { CaseConnectorRt } from '../connectors/connector';
import { CaseAssigneesRt } from './assignee';
import { limitedArraySchema, NonEmptyString } from '../../schema';
import { MAX_DELETE_IDS_LENGTH } from '../../constants';

export const AttachmentTotalsRt = rt.strict({
alerts: rt.number,
Expand Down Expand Up @@ -283,6 +285,13 @@ export const CasesFindRequestRt = rt.exact(
})
);

export const CasesDeleteRequestRt = limitedArraySchema(
NonEmptyString,
1,
MAX_DELETE_IDS_LENGTH,
'ids'
);

export const CasesByAlertIDRequestRt = rt.exact(
rt.partial({
/**
Expand Down Expand Up @@ -420,6 +429,7 @@ export type CasePostRequest = rt.TypeOf<typeof CasePostRequestRt>;
export type Case = rt.TypeOf<typeof CaseRt>;
export type CaseResolveResponse = rt.TypeOf<typeof CaseResolveResponseRt>;
export type Cases = rt.TypeOf<typeof CasesRt>;
export type CasesDeleteRequest = rt.TypeOf<typeof CasesDeleteRequestRt>;
export type CasesFindRequest = rt.TypeOf<typeof CasesFindRequestRt>;
export type CasesByAlertIDRequest = rt.TypeOf<typeof CasesByAlertIDRequestRt>;
export type CasesFindResponse = rt.TypeOf<typeof CasesFindResponseRt>;
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/cases/common/constants/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ export const MAX_CONCURRENT_SEARCHES = 10 as const;
export const MAX_BULK_GET_CASES = 1000 as const;
export const MAX_COMMENTS_PER_PAGE = 100 as const;
export const MAX_CATEGORY_FILTER_LENGTH = 100 as const;
export const MAX_DELETE_IDS_LENGTH = 100 as const;
Copy link
Member

@cnasikas cnasikas Jun 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

super nit: I think we should move it to Validation section below.


/**
* Validation
Expand Down
7 changes: 6 additions & 1 deletion x-pack/plugins/cases/docs/openapi/bundled.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,12 @@
"in": "query",
"required": true,
"schema": {
"type": "string"
"type": "array",
"items": {
"type": "string",
"minItems": 1,
"maxItems": 100
}
},
"example": "d4e7abb0-b462-11ec-9a8d-698504725a43"
}
Expand Down
6 changes: 5 additions & 1 deletion x-pack/plugins/cases/docs/openapi/bundled.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,11 @@ paths:
in: query
required: true
schema:
type: string
type: array
items:
type: string
minItems: 1
maxItems: 100
example: d4e7abb0-b462-11ec-9a8d-698504725a43
responses:
'204':
Expand Down
14 changes: 9 additions & 5 deletions x-pack/plugins/cases/docs/openapi/paths/s@{spaceid}@api@cases.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ post:
'200':
description: Indicates a successful call.
content:
application/json:
application/json:
schema:
$ref: '../components/schemas/case_response_properties.yaml'
examples:
Expand All @@ -36,7 +36,7 @@ post:
schema:
$ref: '../components/schemas/4xx_response.yaml'
servers:
- url: https://localhost:5601
- url: https://localhost:5601

delete:
summary: Deletes one or more cases.
Expand All @@ -56,7 +56,11 @@ delete:
in: query
required: true
schema:
type: string
type: array
items:
type: string
minItems: 1
maxItems: 100
example: d4e7abb0-b462-11ec-9a8d-698504725a43
responses:
'204':
Expand Down Expand Up @@ -109,7 +113,7 @@ patch:
schema:
$ref: '../components/schemas/4xx_response.yaml'
servers:
- url: https://localhost:5601
- url: https://localhost:5601

servers:
- url: https://localhost:5601
- url: https://localhost:5601
18 changes: 17 additions & 1 deletion x-pack/plugins/cases/server/client/cases/delete.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* 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 { MAX_DELETE_IDS_LENGTH, 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';
Expand Down Expand Up @@ -95,6 +95,22 @@ describe('delete', () => {
});
});
});

describe('errors', () => {
it(`throws 400 when trying to delete more than ${MAX_DELETE_IDS_LENGTH} files at a time`, async () => {
Copy link
Member

Choose a reason for hiding this comment

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

This should be testing the MAX_DELETE_IDS_LENGTH, right?

Copy link
Contributor Author

@adcoelho adcoelho Jun 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yup, do you mean the test name? I left files there by mistake 😅

Copy link
Member

Choose a reason for hiding this comment

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

Yes, for some reason I thought the MAX_DELETE_IDS_LENGTH was only for the files.

const caseIds = new Array(MAX_DELETE_IDS_LENGTH + 1).fill('id');

await expect(deleteCases(caseIds, clientArgs)).rejects.toThrowError(
'Error: The length of the field ids is too long. Array must be of length <= 100.'
);
});

it('throws 400 when no id is passed to delete', async () => {
await expect(deleteCases([], clientArgs)).rejects.toThrowError(
'Error: The length of the field ids is too short. Array must be of length >= 1.'
);
});
});
});
});

Expand Down
10 changes: 8 additions & 2 deletions x-pack/plugins/cases/server/client/cases/delete.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ 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 type { CasesDeleteRequest } from '../../../common/api';
import { CasesDeleteRequestRt, decodeWithExcessOrThrow } from '../../../common/api';
import {
CASE_COMMENT_SAVED_OBJECT,
CASE_SAVED_OBJECT,
Expand All @@ -26,7 +28,10 @@ import { createFileEntities, deleteFiles } from '../files';
/**
* Deletes the specified cases and their attachments.
*/
export async function deleteCases(ids: string[], clientArgs: CasesClientArgs): Promise<void> {
export async function deleteCases(
Copy link
Member

Choose a reason for hiding this comment

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

I noticed we also define a params schema in x-pack/plugins/cases/server/routes/api/cases/delete_cases.ts. Can you also put the validation there? (using the @kbn/schema)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  params: {
    query: schema.object({
      ids: schema.arrayOf(schema.string()),
    }),
  }

What do you mean with

(using the @kbn/schema)

?

I am not familiar with this '@kbn/config-schema';. I see some routes use it for the parameters(not all though) is it used for validation anywhere, how does it work?

I guess I can just replace this with the limitedArraySchema definition I created.

Copy link
Member

Choose a reason for hiding this comment

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

We discuss it offline and we decided that we are going to remove the schema defined in all cases routes and rely only on the validation inside the cases client.

ids: CasesDeleteRequest,
clientArgs: CasesClientArgs
): Promise<void> {
const {
services: { caseService, attachmentService, userActionService, alertsService },
logger,
Expand All @@ -35,7 +40,8 @@ export async function deleteCases(ids: string[], clientArgs: CasesClientArgs): P
} = clientArgs;

try {
const cases = await caseService.getCases({ caseIds: ids });
const caseIds = decodeWithExcessOrThrow(CasesDeleteRequestRt)(ids);
const cases = await caseService.getCases({ caseIds });
const entities = new Map<string, OwnerEntity>();

for (const theCase of cases.saved_objects) {
Expand Down
16 changes: 16 additions & 0 deletions x-pack/test/cases_api_integration/security_and_spaces/tests/common/cases/delete_cases.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,22 @@ export default ({ getService }: FtrProviderContext): void => {
await deleteCases({ supertest, caseIDs: ['fake-id'], expectedHttpCode: 404 });
});

it('unhappy path - 400s when trying to delete more than 100 cases at a time', async () => {
await deleteCases({
supertest: supertestWithoutAuth,
caseIDs: new Array(101).fill('id'),
expectedHttpCode: 400,
});
});

it('unhappy path - 400s when trying to delete 0 cases at a time', async () => {
await deleteCases({
supertest: supertestWithoutAuth,
caseIDs: [],
expectedHttpCode: 400,
});
});

describe('files', () => {
afterEach(async () => {
await deleteAllFiles({
Expand Down