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

Merged
Merged
Changes from 2 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
24 changes: 23 additions & 1 deletion x-pack/plugins/cases/common/api/cases/comment/index.test.ts
Original file line number Diff line number Diff line change
@@ -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', () => {
@@ -843,6 +843,28 @@ 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',
foo: 'bar',
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you remove the foo: 'bar' field that is related to a different error? (I did the same mistake 😄 )

};
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', () => {
8 changes: 7 additions & 1 deletion x-pack/plugins/cases/common/api/cases/comment/index.ts
Original file line number Diff line number Diff line change
@@ -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';
@@ -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({
1 change: 1 addition & 0 deletions x-pack/plugins/cases/common/constants/index.ts
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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', () => {
@@ -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)
Original file line number Diff line number Diff line change
@@ -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,
@@ -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,