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] Create and update case API guardrails for title, description, category, tags #160844

Merged
merged 15 commits into from
Jun 30, 2023
Merged
Show file tree
Hide file tree
Changes from 14 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
70 changes: 64 additions & 6 deletions x-pack/plugins/cases/common/api/cases/case.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,14 @@ import { CommentRt } from './comment';
import { CasesStatusResponseRt, CaseStatusRt } from './status';
import { CaseConnectorRt } from '../connectors/connector';
import { CaseAssigneesRt } from './assignee';
import { limitedArraySchema, NonEmptyString } from '../../schema';
import { limitedArraySchema, limitedStringSchema, NonEmptyString } from '../../schema';
import {
MAX_DELETE_IDS_LENGTH,
MAX_DESCRIPTION_LENGTH,
MAX_TITLE_LENGTH,
MAX_LENGTH_PER_TAG,
MAX_CATEGORY_LENGTH,
MAX_TAGS_PER_CASE,
MAX_ASSIGNEES_FILTER_LENGTH,
MAX_REPORTERS_FILTER_LENGTH,
MAX_TAGS_FILTER_LENGTH,
Expand Down Expand Up @@ -139,15 +144,20 @@ export const CasePostRequestRt = rt.intersection([
/**
* Description of the case
*/
description: rt.string,
description: limitedStringSchema('description', 1, MAX_DESCRIPTION_LENGTH),
/**
* Identifiers for the case.
*/
tags: rt.array(rt.string),
tags: limitedArraySchema(
limitedStringSchema('tag', 1, MAX_LENGTH_PER_TAG),
0,
MAX_TAGS_PER_CASE,
'tags'
),
/**
* Title of the case
*/
title: rt.string,
title: limitedStringSchema('title', 1, MAX_TITLE_LENGTH),
/**
* The external configuration for the case
*/
Expand Down Expand Up @@ -176,7 +186,7 @@ export const CasePostRequestRt = rt.intersection([
/**
* The category of the case.
*/
category: rt.union([rt.string, rt.null]),
category: rt.union([limitedStringSchema('category', 1, MAX_CATEGORY_LENGTH), rt.null]),
})
),
]);
Expand Down Expand Up @@ -355,7 +365,55 @@ export const CasesFindResponseRt = rt.intersection([
]);

export const CasePatchRequestRt = rt.intersection([
rt.exact(rt.partial(CaseBasicRt.type.props)),
rt.exact(
rt.partial({
/**
* The description of the case
*/
description: limitedStringSchema('description', 1, MAX_DESCRIPTION_LENGTH),
/**
* The current status of the case (open, closed, in-progress)
*/
status: CaseStatusRt,
/**
* The identifying strings for filter a case
*/
tags: limitedArraySchema(
limitedStringSchema('tag', 1, MAX_LENGTH_PER_TAG),
0,
MAX_TAGS_PER_CASE,
'tags'
),
/**
* The title of a case
*/
title: limitedStringSchema('title', 1, MAX_TITLE_LENGTH),
/**
* The external system that the case can be synced with
*/
connector: CaseConnectorRt,
/**
* The alert sync settings
*/
settings: SettingsRt,
/**
* The plugin owner of the case
*/
owner: rt.string,
/**
* The severity of the case
*/
severity: CaseSeverityRt,
/**
* The users assigned to this case
*/
assignees: CaseAssigneesRt,
/**
* The category of the case.
*/
category: rt.union([limitedStringSchema('category', 1, MAX_CATEGORY_LENGTH), rt.null]),
})
),
/**
* The saved object ID and version
*/
Expand Down
3 changes: 3 additions & 0 deletions x-pack/plugins/cases/common/constants/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,9 @@ export const MAX_REPORTERS_FILTER_LENGTH = 100 as const;

export const MAX_TITLE_LENGTH = 160 as const;
export const MAX_CATEGORY_LENGTH = 50 as const;
export const MAX_DESCRIPTION_LENGTH = 30000 as const;
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;

/**
Expand Down
149 changes: 110 additions & 39 deletions x-pack/plugins/cases/common/schema/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,70 +7,141 @@

import { PathReporter } from 'io-ts/lib/PathReporter';

import { limitedArraySchema, NonEmptyString } from '.';
import { limitedArraySchema, limitedStringSchema, NonEmptyString } from '.';

describe('schema', () => {
it('fails when given an empty string', () => {
expect(PathReporter.report(limitedArraySchema(NonEmptyString, 1, 1).decode([''])))
.toMatchInlineSnapshot(`
Array [
"string must have length >= 1",
]
`);
});
describe('limitedArraySchema', () => {
const fieldName = 'foobar';

it('fails when given an empty array', () => {
expect(PathReporter.report(limitedArraySchema(NonEmptyString, 1, 1).decode([])))
.toMatchInlineSnapshot(`
it('fails when given an empty string', () => {
expect(PathReporter.report(limitedArraySchema(NonEmptyString, 1, 1, fieldName).decode([''])))
.toMatchInlineSnapshot(`
Array [
"Array must be of length >= 1.",
]
`);
});

it('fails when given an array larger than the limit of one item', () => {
expect(PathReporter.report(limitedArraySchema(NonEmptyString, 1, 1).decode(['a', 'b'])))
.toMatchInlineSnapshot(`
Array [
"Array must be of length <= 1.",
"string must have length >= 1",
]
`);
});
});

it('displays field name error message when lower boundary fails', () => {
expect(PathReporter.report(limitedArraySchema(NonEmptyString, 1, 1, 'foobar').decode([])))
.toMatchInlineSnapshot(`
it('fails when given an empty array', () => {
expect(PathReporter.report(limitedArraySchema(NonEmptyString, 1, 1, fieldName).decode([])))
.toMatchInlineSnapshot(`
Array [
"The length of the field foobar is too short. Array must be of length >= 1.",
]
`);
});
});

it('displays field name error message when upper boundary fails', () => {
expect(
PathReporter.report(limitedArraySchema(NonEmptyString, 1, 1, 'foobar').decode(['a', 'b']))
).toMatchInlineSnapshot(`
it('fails when given an array larger than the limit of one item', () => {
expect(
PathReporter.report(limitedArraySchema(NonEmptyString, 1, 1, fieldName).decode(['a', 'b']))
).toMatchInlineSnapshot(`
Array [
"The length of the field foobar is too long. Array must be of length <= 1.",
]
`);
});
});

it('succeeds when given an array of 1 item with a non-empty string', () => {
expect(PathReporter.report(limitedArraySchema(NonEmptyString, 1, 1).decode(['a'])))
.toMatchInlineSnapshot(`
it('succeeds when given an array of 1 item with a non-empty string', () => {
expect(PathReporter.report(limitedArraySchema(NonEmptyString, 1, 1, fieldName).decode(['a'])))
.toMatchInlineSnapshot(`
Array [
"No errors!",
]
`);
});
});

it('succeeds when given an array of 0 item with a non-empty string when the min is 0', () => {
expect(PathReporter.report(limitedArraySchema(NonEmptyString, 0, 2).decode([])))
.toMatchInlineSnapshot(`
it('succeeds when given an array of 0 item with a non-empty string when the min is 0', () => {
expect(PathReporter.report(limitedArraySchema(NonEmptyString, 0, 2, fieldName).decode([])))
.toMatchInlineSnapshot(`
Array [
"No errors!",
]
`);
});
});

describe('limitedStringSchema', () => {
js-jankisalvi marked this conversation as resolved.
Show resolved Hide resolved
const fieldName = 'foo';

it('fails when given string is shorter than minimum', () => {
expect(PathReporter.report(limitedStringSchema(fieldName, 2, 1).decode('a')))
.toMatchInlineSnapshot(`
Array [
"The length of the ${fieldName} is too short. The minimum length is 2.",
]
`);
});

it('fails when given string is empty and minimum is not 0', () => {
expect(PathReporter.report(limitedStringSchema(fieldName, 1, 1).decode('')))
.toMatchInlineSnapshot(`
Array [
"The ${fieldName} field cannot be an empty string.",
]
`);
});

it('fails when given string consists only empty characters and minimum is not 0', () => {
expect(PathReporter.report(limitedStringSchema(fieldName, 1, 1).decode(' ')))
.toMatchInlineSnapshot(`
Array [
"The ${fieldName} field cannot be an empty string.",
]
`);
});

it('fails when given string is larger than maximum', () => {
expect(PathReporter.report(limitedStringSchema(fieldName, 1, 5).decode('Hello there!!')))
.toMatchInlineSnapshot(`
Array [
"The length of the ${fieldName} is too long. The maximum length is 5.",
]
`);
});

it('succeeds when given string within limit', () => {
expect(PathReporter.report(limitedStringSchema(fieldName, 1, 50).decode('Hello!!')))
.toMatchInlineSnapshot(`
Array [
"No errors!",
]
`);
});

it('succeeds when given string is empty and minimum is 0', () => {
expect(PathReporter.report(limitedStringSchema(fieldName, 0, 5).decode('')))
.toMatchInlineSnapshot(`
Array [
"No errors!",
]
`);
});

it('succeeds when given string consists only empty characters and minimum is 0', () => {
expect(PathReporter.report(limitedStringSchema(fieldName, 0, 5).decode(' ')))
.toMatchInlineSnapshot(`
Array [
"No errors!",
]
`);
});

it('succeeds when given string is same as maximum', () => {
expect(PathReporter.report(limitedStringSchema(fieldName, 0, 5).decode('Hello')))
.toMatchInlineSnapshot(`
Array [
"No errors!",
]
`);
});

it('succeeds when given string is larger than maximum but same as maximum after trim', () => {
expect(PathReporter.report(limitedStringSchema(fieldName, 0, 5).decode('Hello ')))
.toMatchInlineSnapshot(`
Array [
"No errors!",
]
`);
});
});
});
42 changes: 34 additions & 8 deletions x-pack/plugins/cases/common/schema/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,35 +22,61 @@ export const NonEmptyString = new rt.Type<string, string, unknown>(
rt.identity
);

export const limitedStringSchema = (fieldName: string, min: number, max: number) =>
cnasikas marked this conversation as resolved.
Show resolved Hide resolved
new rt.Type<string, string, unknown>(
'LimitedString',
rt.string.is,
(input, context) =>
either.chain(rt.string.validate(input, context), (s) => {
if (s.trim().length === 0 && s.trim().length < min) {
Copy link
Member

Choose a reason for hiding this comment

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

As we trim a lot in this function I think it is better if we put it on a variable. For example, const trimedString = s.trim()

return rt.failure(input, context, `The ${fieldName} field cannot be an empty string.`);
}

if (s.trim().length < min) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm do we want spaces to count as valid characters towards the min and max?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am assuming here that start and end of string spaces are unnecessary and should be removed before checking the length. But I am open if we want to keep it.

Copy link
Member

@cnasikas cnasikas Jun 30, 2023

Choose a reason for hiding this comment

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

I think it all depends on what we persist in ES. If we persist the spaces (which I think we are), then we should not trim. Or the other way around, if we think that we should trim (probably yes because we should not allow a title of only spaces), then we should trim before persisting it to ES. I lean towards trimming it to not allow titles with only empty spaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It makes sense to me to trim for validation as well as before persisting to ES.

Copy link
Member

@cnasikas cnasikas Jun 30, 2023

Choose a reason for hiding this comment

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

Ok, let's leave the trimming but let's be consistent (not on this PR). In the UI for example no error is being shown in the case view page when you add an empty string to the title. In the backend, we do not trim before saving.

return rt.failure(
input,
context,
`The length of the ${fieldName} is too short. The minimum length is ${min}.`
);
}

if (s.trim().length > max) {
return rt.failure(
input,
context,
`The length of the ${fieldName} is too long. The maximum length is ${max}.`
);
}

return rt.success(s);
}),
rt.identity
);

export const limitedArraySchema = <T extends rt.Mixed>(
codec: T,
min: number,
max: number,
fieldName?: string
fieldName: string
) =>
new rt.Type<Array<rt.TypeOf<typeof codec>>, Array<rt.TypeOf<typeof codec>>, unknown>(
'LimitedArray',
(input): input is T[] => rt.array(codec).is(input),
(input, context) =>
either.chain(rt.array(codec).validate(input, context), (s) => {
if (s.length < min) {
const fieldNameErrorMessage =
fieldName != null ? `The length of the field ${fieldName} is too short. ` : '';

return rt.failure(
input,
context,
`${fieldNameErrorMessage}Array must be of length >= ${min}.`
`The length of the field ${fieldName} is too short. Array must be of length >= ${min}.`
);
}

if (s.length > max) {
const fieldNameErrorMessage =
fieldName != null ? `The length of the field ${fieldName} is too long. ` : '';
return rt.failure(
input,
context,
`${fieldNameErrorMessage}Array must be of length <= ${max}.`
`The length of the field ${fieldName} is too long. Array must be of length <= ${max}.`
);
}

Expand Down
Loading