[Cases] Limit perPage param in findComments API #160042

js-jankisalvi · 2023-06-20T15:19:29Z

Summary

This PR limits perPage param to 100 in findComments API.

Checklist

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2023-06-20T15:19:32Z

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

elasticmachine · 2023-06-20T15:19:32Z

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

adcoelho · 2023-06-20T16:02:11Z

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

@@ -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 (Math.max(perPageAsNumber, 0) > MAX_COMMENTS_PER_PAGE) {


why the Math.max?

just in case params.perPage is negative

hmm if params.perPage was negative and we didn't have the Math.max it'd still return false right? Do we need the Math.max? If so could you add a test case for a negative value.

removed Math.max as in both case it will return false

adcoelho · 2023-06-20T16:07:19Z

x-pack/plugins/cases/server/client/attachments/validators.test.ts


 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 = `Too many comments perPage provided, The maximum allowed perPage is ${MAX_COMMENTS_PER_PAGE}.`;
+
 describe('validators', () => {
  describe('validateFindCommentsPagination', () => {
    it('does not throw if only page is undefined', () => {


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(); });

lcawl

API docs LGTM, thanks!

jonathan-buttner

Left some minor comments.

jonathan-buttner · 2023-06-20T19:18:44Z

x-pack/plugins/cases/server/client/attachments/validators.test.ts


 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 = `Too many comments perPage provided, The maximum allowed perPage is ${MAX_COMMENTS_PER_PAGE}.`;


How about: The provided perPage value was too high. The maximum allowed perPage value is ${...}.

jonathan-buttner · 2023-06-20T19:20:43Z

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

-  if (Math.max(pageAsNumber, perPageAsNumber, pageAsNumber * perPageAsNumber) > MAX_DOCS_PER_PAGE) {
+  if (Math.max(perPageAsNumber, 0) > MAX_COMMENTS_PER_PAGE) {
+    throw Boom.badRequest(
+      `Too many comments perPage provided, The maximum allowed perPage is ${MAX_COMMENTS_PER_PAGE}.`


Same suggestions as before about the wording.

jonathan-buttner · 2023-06-20T19:27:44Z

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

@@ -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 (Math.max(perPageAsNumber, 0) > MAX_COMMENTS_PER_PAGE) {


hmm if params.perPage was negative and we didn't have the Math.max it'd still return false right? Do we need the Math.max? If so could you add a test case for a negative value.

…ankisalvi/kibana into limit-perpage-find-comments

…-fix'

…ankisalvi/kibana into limit-perpage-find-comments

…comments

kibana-ci · 2023-06-21T11:34:10Z

💚 Build Succeeded

Buildkite Build
Commit: bb3ce13

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	13	15	+2
`securitySolution`	411	415	+4
total			+6

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	14	16	+2
`securitySolution`	492	496	+4
total			+6

History

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

cc @js-jankisalvi

add validation for perPage param of findComments API

be995c2

js-jankisalvi 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.9.0 labels Jun 20, 2023

js-jankisalvi requested a review from a team as a code owner June 20, 2023 15:19

js-jankisalvi self-assigned this Jun 20, 2023

js-jankisalvi requested a review from a team as a code owner June 20, 2023 15:19

js-jankisalvi changed the title ~~[Cases] limit perPage param in findComments API~~ [Cases] Limit perPage param in findComments API Jun 20, 2023

remove only from test

eaff89d

adcoelho reviewed Jun 20, 2023

View reviewed changes

adcoelho approved these changes Jun 20, 2023

View reviewed changes

js-jankisalvi and others added 2 commits June 20, 2023 18:14

PR feedback

5db6207

[DOCS] Add maximum perPage property; generate API preview

5344ccb

lcawl approved these changes Jun 20, 2023

View reviewed changes

jonathan-buttner reviewed Jun 20, 2023

View reviewed changes

js-jankisalvi and others added 8 commits June 21, 2023 09:39

removed max, updated message

2420145

Merge branch 'limit-perpage-find-comments' of https://github.com/js-j…

8a0d148

…ankisalvi/kibana into limit-perpage-find-comments

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

1b37862

…-fix'

fix integration test

1753e08

Merge branch 'limit-perpage-find-comments' of https://github.com/js-j…

3b4df47

…ankisalvi/kibana into limit-perpage-find-comments

Merge remote-tracking branch 'upstream/main' into limit-perpage-find-…

38ad526

…comments

fix jest test message

133416b

Merge branch 'main' into limit-perpage-find-comments

bb3ce13

js-jankisalvi merged commit de3f8fc into elastic:main Jun 21, 2023

kibanamachine added the backport:skip This commit does not require backporting label Jun 21, 2023

cnasikas mentioned this pull request Jul 13, 2023

[Cases] Guardrails in Cases APIs #146945

Closed

js-jankisalvi deleted the limit-perpage-find-comments branch October 6, 2023 07:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Cases] Limit perPage param in findComments API #160042

[Cases] Limit perPage param in findComments API #160042

js-jankisalvi commented Jun 20, 2023

elasticmachine commented Jun 20, 2023

elasticmachine commented Jun 20, 2023

adcoelho Jun 20, 2023

js-jankisalvi Jun 20, 2023

jonathan-buttner Jun 20, 2023

js-jankisalvi Jun 21, 2023

adcoelho Jun 20, 2023

lcawl left a comment

jonathan-buttner left a comment

jonathan-buttner Jun 20, 2023

jonathan-buttner Jun 20, 2023

jonathan-buttner Jun 20, 2023

kibana-ci commented Jun 21, 2023

ESLint disabled line counts

Total ESLint disabled count

[Cases] Limit perPage param in findComments API #160042

[Cases] Limit perPage param in findComments API #160042

Conversation

js-jankisalvi commented Jun 20, 2023

Summary

Checklist

elasticmachine commented Jun 20, 2023

elasticmachine commented Jun 20, 2023

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

lcawl left a comment

Choose a reason for hiding this comment

jonathan-buttner 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

kibana-ci commented Jun 21, 2023

💚 Build Succeeded

Metrics [docs]

ESLint disabled line counts

Total ESLint disabled count

History