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] limit number of attachments that can be created using the bulk create API #161451

Merged
merged 4 commits into from
Jul 7, 2023
Merged

[Cases] limit number of attachments that can be created using the bulk create API #161451

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dae86ec
limit bulk create attachments to 100
js-jankisalvi Jul 7, 2023
b5c54de
[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…
kibanamachine Jul 7, 2023
c527881
removed unnecessary attribute
js-jankisalvi Jul 7, 2023
a331315
Merge branch 'total-attachements-bulk-create' of https://github.com/j…
js-jankisalvi Jul 7, 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
23 changes: 22 additions & 1 deletion x-pack/plugins/cases/common/api/cases/comment/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import {
BulkGetAttachmentsRequestRt,
BulkGetAttachmentsResponseRt,
} from '.';
import { MAX_COMMENT_LENGTH } from '../../../constants';
import { MAX_COMMENT_LENGTH, MAX_BULK_CREATE_ATTACHMENTS } from '../../../constants';

describe('Comments', () => {
describe('CommentAttributesBasicRt', () => {
Expand Down Expand Up @@ -843,6 +843,27 @@ describe('Comments', () => {
right: defaultRequest,
});
});

describe('errors', () => {
it(`throws error when attachments are more than ${MAX_BULK_CREATE_ATTACHMENTS}`, () => {
const comment = {
comment: 'Solve this fast!',
type: CommentType.user,
owner: 'cases',
};
const attachments = Array(MAX_BULK_CREATE_ATTACHMENTS + 1).fill(comment);

expect(PathReporter.report(BulkCreateCommentRequestRt.decode(attachments))).toContain(
`The length of the field attachments is too long. Array must be of length <= ${MAX_BULK_CREATE_ATTACHMENTS}.`
);
});

it(`no errors when empty array of attachments`, () => {
expect(PathReporter.report(BulkCreateCommentRequestRt.decode([]))).toStrictEqual([
'No errors!',
]);
});
});
});

describe('BulkGetAttachmentsRequestRt', () => {
Expand Down
8 changes: 7 additions & 1 deletion x-pack/plugins/cases/common/api/cases/comment/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
MAX_BULK_GET_ATTACHMENTS,
MAX_COMMENTS_PER_PAGE,
MAX_COMMENT_LENGTH,
MAX_BULK_CREATE_ATTACHMENTS,
} from '../../../constants';
import { limitedArraySchema, paginationSchema, limitedStringSchema } from '../../../schema';
import { jsonValueRt } from '../../runtime_types';
Expand Down Expand Up @@ -328,7 +329,12 @@ export const FindCommentsQueryParamsRt = rt.intersection([
paginationSchema({ maxPerPage: MAX_COMMENTS_PER_PAGE }),
]);

export const BulkCreateCommentRequestRt = rt.array(CommentRequestRt);
export const BulkCreateCommentRequestRt = limitedArraySchema({
codec: CommentRequestRt,
min: 0,
max: MAX_BULK_CREATE_ATTACHMENTS,
fieldName: 'attachments',
});

export const BulkGetAttachmentsRequestRt = rt.strict({
ids: limitedArraySchema({
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 @@ -125,6 +125,7 @@ export const MAX_LENGTH_PER_TAG = 256 as const;
export const MAX_TAGS_PER_CASE = 200 as const;
export const MAX_DELETE_IDS_LENGTH = 100 as const;
export const MAX_CASES_TO_UPDATE = 100 as const;
export const MAX_BULK_CREATE_ATTACHMENTS = 100 as const;

/**
* Cases features
Expand Down
10 changes: 9 additions & 1 deletion x-pack/plugins/cases/server/client/attachments/bulk_create.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

import { comment, actionComment } from '../../mocks';
import { createCasesClientMockArgs } from '../mocks';
import { MAX_COMMENT_LENGTH } from '../../../common/constants';
import { MAX_COMMENT_LENGTH, MAX_BULK_CREATE_ATTACHMENTS } from '../../../common/constants';
import { bulkCreate } from './bulk_create';

describe('bulkCreate', () => {
Expand All @@ -24,6 +24,14 @@ describe('bulkCreate', () => {
).rejects.toThrow('invalid keys "foo"');
});

it(`throws error when attachments are more than ${MAX_BULK_CREATE_ATTACHMENTS}`, async () => {
const attachments = Array(MAX_BULK_CREATE_ATTACHMENTS + 1).fill(comment);

await expect(bulkCreate({ attachments, caseId: 'test-case' }, clientArgs)).rejects.toThrow(
`The length of the field attachments is too long. Array must be of length <= ${MAX_BULK_CREATE_ATTACHMENTS}.`
);
});

describe('comments', () => {
it('should throw an error if the comment length is too long', async () => {
const longComment = Array(MAX_COMMENT_LENGTH + 1)
Expand Down
28 changes: 17 additions & 11 deletions ...ases_api_integration/security_and_spaces/tests/common/internal/bulk_create_attachments.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,17 +218,6 @@ export default ({ getService }: FtrProviderContext): void => {
});
});

it('should bulk create 100 file attachments when there is another attachment type in the request', async () => {
const fileRequests = [...Array(100).keys()].map(() => getFilesAttachmentReq());

const postedCase = await createCase(supertest, postCaseReq);
await bulkCreateAttachments({
supertest,
caseId: postedCase.id,
params: [postExternalReferenceSOReq, ...fileRequests],
});
});

it('should bulk create 99 file attachments when the case has a file associated to it', async () => {
const postedCase = await createCase(
supertestWithoutAuth,
Expand Down Expand Up @@ -376,6 +365,23 @@ export default ({ getService }: FtrProviderContext): void => {
});
});

it('400s when attempting to add more than 100 attachments', async () => {
const comment = {
type: CommentType.user,
comment: 'test',
owner: 'securitySolutionFixture',
};

const attachments = Array(101).fill(comment);
Copy link
Contributor

Choose a reason for hiding this comment

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

In these integration tests I never know if I wanna just put the value or import the constant(MAX_BULK_CREATE_ATTACHMENTS)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, but I thought it's good that integration tests fail when constant is changed, reminds us what all places we have put this check 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Keeps us on edge 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Genau 😆

Copy link
Member

Choose a reason for hiding this comment

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

😆 I tend to user the value for the reasons Janki mentioned.


await bulkCreateAttachments({
supertest,
caseId: 'test-case-id',
params: attachments,
expectedHttpCode: 400,
});
});

it('400s when attempting to create a comment with a different owner than the case', async () => {
const postedCase = await createCase(
supertest,
Expand Down