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] Limit perPage param in findComments API #160042

Merged
merged 12 commits into from
Jun 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion docs/api-generated/cases/case-apis-passthru.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -556,7 +556,7 @@ Any modifications made to this file will be overwritten.

<div class="param-desc"><span class="param-type">Query Parameter</span> &mdash; The page number to return. default: 1 </div><div class="param">perPage (optional)</div>

<div class="param-desc"><span class="param-type">Query Parameter</span> &mdash; The number of items to return. default: 20 </div><div class="param">sortOrder (optional)</div>
<div class="param-desc"><span class="param-type">Query Parameter</span> &mdash; The number of items to return. Limited to 100 items. default: 20 </div><div class="param">sortOrder (optional)</div>

<div class="param-desc"><span class="param-type">Query Parameter</span> &mdash; Determines the sort order. default: desc </div>
</div> <!-- field-items -->
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/cases/common/constants/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ export const MAX_DOCS_PER_PAGE = 10000 as const;
export const MAX_BULK_GET_ATTACHMENTS = MAX_DOCS_PER_PAGE;
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;

/**
Expand Down
10 changes: 9 additions & 1 deletion x-pack/plugins/cases/docs/openapi/bundled.json
Original file line number Diff line number Diff line change
Expand Up @@ -1852,7 +1852,15 @@
"$ref": "#/components/parameters/page_index"
},
{
"$ref": "#/components/parameters/page_size"
"name": "perPage",
"in": "query",
"description": "The number of items to return. Limited to 100 items.",
"required": false,
"schema": {
"type": "integer",
"default": 20,
"maximum": 100
}
},
{
"$ref": "#/components/parameters/sort_order"
Expand Down
9 changes: 8 additions & 1 deletion x-pack/plugins/cases/docs/openapi/bundled.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -1156,7 +1156,14 @@ paths:
parameters:
- $ref: '#/components/parameters/case_id'
- $ref: '#/components/parameters/page_index'
- $ref: '#/components/parameters/page_size'
- name: perPage
in: query
description: The number of items to return. Limited to 100 items.
required: false
schema:
type: integer
default: 20
maximum: 100
- $ref: '#/components/parameters/sort_order'
- $ref: '#/components/parameters/space_id'
responses:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,14 @@ get:
parameters:
- $ref: '../components/parameters/case_id.yaml'
- $ref: '../components/parameters/page_index.yaml'
- $ref: '../components/parameters/page_size.yaml'
- name: perPage
in: query
description: The number of items to return. Limited to 100 items.
required: false
schema:
type: integer
default: 20
maximum: 100
- $ref: '../components/parameters/sort_order.yaml'
- $ref: '../components/parameters/space_id.yaml'
responses:
Expand Down
10 changes: 9 additions & 1 deletion x-pack/plugins/cases/server/client/attachments/get.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,20 @@ describe('get', () => {

it('Invalid total items results in error', async () => {
await expect(() =>
findComment({ caseID: 'mock-id', findQueryParams: { page: 2, perPage: 9001 } }, clientArgs)
findComment({ caseID: 'mock-id', findQueryParams: { page: 209, perPage: 100 } }, clientArgs)
).rejects.toThrowErrorMatchingInlineSnapshot(
`"Failed to find comments case id: mock-id: Error: The number of documents is too high. Paginating through more than 10,000 documents is not possible."`
);
});

it('Invalid perPage items results in error', async () => {
await expect(() =>
findComment({ caseID: 'mock-id', findQueryParams: { page: 2, perPage: 9001 } }, clientArgs)
).rejects.toThrowErrorMatchingInlineSnapshot(
`"Failed to find comments case id: mock-id: Error: The provided perPage value was too high. The maximum allowed perPage value is 100."`
);
});

it('throws with excess fields', async () => {
await expect(
findComment(
Expand Down
19 changes: 16 additions & 3 deletions x-pack/plugins/cases/server/client/attachments/validators.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,13 @@
*/

import { validateFindCommentsPagination } from './validators';
import { MAX_COMMENTS_PER_PAGE } from '../../../common/constants';

const ERROR_MSG =
'The number of documents is too high. Paginating through more than 10,000 documents is not possible.';

const ERROR_MSG_PER_PAGE = `The provided perPage value was too high. The maximum allowed perPage value is ${MAX_COMMENTS_PER_PAGE}.`;

describe('validators', () => {
describe('validateFindCommentsPagination', () => {
it('does not throw if only page is undefined', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: A basic happy path is missing where both are defined and valid.

it('does not throw if page and perPage are defined and valid', () => {
  expect(() => validateFindCommentsPagination({ page: 2, perPage: 100 })).not.toThrowError();
});

Expand All @@ -20,20 +23,30 @@ describe('validators', () => {
expect(() => validateFindCommentsPagination({ page: 100 })).not.toThrowError();
});

it('does not throw if page and perPage are defined and valid', () => {
expect(() => validateFindCommentsPagination({ page: 2, perPage: 100 })).not.toThrowError();
});

it('returns if page and perPage are undefined', () => {
expect(() => validateFindCommentsPagination({})).not.toThrowError();
});

it('returns if perPage < 0', () => {
expect(() => validateFindCommentsPagination({ perPage: -1 })).not.toThrowError();
});

it('throws if page > 10k', () => {
expect(() => validateFindCommentsPagination({ page: 10001 })).toThrow(ERROR_MSG);
});

it('throws if perPage > 10k', () => {
expect(() => validateFindCommentsPagination({ perPage: 10001 })).toThrowError(ERROR_MSG);
it('throws if perPage > 100', () => {
expect(() =>
validateFindCommentsPagination({ perPage: MAX_COMMENTS_PER_PAGE + 1 })
).toThrowError(ERROR_MSG_PER_PAGE);
});

it('throws if page * perPage > 10k', () => {
expect(() => validateFindCommentsPagination({ page: 10, perPage: 1001 })).toThrow(ERROR_MSG);
expect(() => validateFindCommentsPagination({ page: 101, perPage: 100 })).toThrow(ERROR_MSG);
});
});
});
10 changes: 8 additions & 2 deletions x-pack/plugins/cases/server/client/attachments/validators.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
*/

import Boom from '@hapi/boom';
import { MAX_DOCS_PER_PAGE } from '../../../common/constants';
import { MAX_DOCS_PER_PAGE, MAX_COMMENTS_PER_PAGE } from '../../../common/constants';
import {
isCommentRequestTypeExternalReference,
isCommentRequestTypePersistableState,
Expand Down Expand Up @@ -51,7 +51,13 @@ export const validateFindCommentsPagination = (params?: FindCommentsQueryParams)
const pageAsNumber = params.page ?? 0;
const perPageAsNumber = params.perPage ?? 0;

if (Math.max(pageAsNumber, perPageAsNumber, pageAsNumber * perPageAsNumber) > MAX_DOCS_PER_PAGE) {
if (perPageAsNumber > MAX_COMMENTS_PER_PAGE) {
throw Boom.badRequest(
`The provided perPage value was too high. The maximum allowed perPage value is ${MAX_COMMENTS_PER_PAGE}.`
);
}

if (Math.max(pageAsNumber, pageAsNumber * perPageAsNumber) > MAX_DOCS_PER_PAGE) {
throw Boom.badRequest(
'The number of documents is too high. Paginating through more than 10,000 documents is not possible.'
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

import expect from '@kbn/expect';
import type SuperTest from 'supertest';
import { MAX_DOCS_PER_PAGE } from '@kbn/cases-plugin/common/constants';
import { MAX_COMMENTS_PER_PAGE } from '@kbn/cases-plugin/common/constants';
import {
Alerts,
createCaseAttachAlertAndDeleteCase,
Expand Down Expand Up @@ -170,7 +170,7 @@ export default ({ getService }: FtrProviderContext): void => {
supertest: supertestWithoutAuth,
caseId: postedCase.id,
query: {
perPage: MAX_DOCS_PER_PAGE,
perPage: MAX_COMMENTS_PER_PAGE,
},
}),
]);
Expand Down Expand Up @@ -210,14 +210,14 @@ export default ({ getService }: FtrProviderContext): void => {
supertest: supertestWithoutAuth,
caseId: postedCase1.id,
query: {
perPage: MAX_DOCS_PER_PAGE,
perPage: MAX_COMMENTS_PER_PAGE,
},
}),
findAttachments({
supertest: supertestWithoutAuth,
caseId: postedCase2.id,
query: {
perPage: MAX_DOCS_PER_PAGE,
perPage: MAX_COMMENTS_PER_PAGE,
},
}),
]);
Expand Down Expand Up @@ -457,7 +457,7 @@ export default ({ getService }: FtrProviderContext): void => {
supertest: supertestWithoutAuth,
caseId: postedCase.id,
query: {
perPage: MAX_DOCS_PER_PAGE,
perPage: MAX_COMMENTS_PER_PAGE,
},
auth: { user: secAllUser, space: 'space1' },
}),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ export default ({ getService }: FtrProviderContext): void => {
{ name: 'field is wrong type', queryParams: { perPage: true } },
{ name: 'field is unknown', queryParams: { foo: 'bar' } },
{ name: 'page > 10k', queryParams: { page: 10001 } },
{ name: 'perPage > 10k', queryParams: { perPage: 10001 } },
{ name: 'perPage > 100', queryParams: { perPage: 101 } },
{ name: 'page * perPage > 10k', queryParams: { page: 2, perPage: 9001 } },
]) {
it(`400s when ${errorScenario.name}`, async () => {
Expand Down