From f91ee9c139e644cccbcf08bdcafc79ceac58e47f Mon Sep 17 00:00:00 2001 From: Jonathan Buttner <56361221+jonathan-buttner@users.noreply.github.com> Date: Mon, 19 Jun 2023 08:51:42 -0400 Subject: [PATCH] [Cases] Removing escapeKuery calls in nodeBuilder (#159815) Fixes: https://github.com/elastic/kibana/issues/159741 This PR removes calls to `escapeKuery` where the result is passed directly to `nodeBuilder`. `nodeBuilder` expects the raw contents that should not be escaped. Escaping is only need when the result is passed to `fromKueryExpression` or `buildNode`. ## Testing Try creating a case with tags that have kql characters like: `\\():<>"*` and try filtering the cases list page for those tags. The case should be displayed in the table. ## Release Notes Cases fixed an issue where the following special characters could not be included in the case tags: `\\():<>"*` because it resulted in a bug where the case would not be displayed in the cases table when filtered for those tags. These characters are now handled correctly and the cases will be shown in the table. --- .../plugins/cases/server/client/utils.test.ts | 259 +++++++----------- x-pack/plugins/cases/server/client/utils.ts | 49 +--- .../tests/common/cases/find_cases.ts | 23 ++ .../tests/trial/cases/assignees.ts | 24 ++ 4 files changed, 150 insertions(+), 205 deletions(-) diff --git a/x-pack/plugins/cases/server/client/utils.test.ts b/x-pack/plugins/cases/server/client/utils.test.ts index 2b63f9c73210..50dbdb8981c9 100644 --- a/x-pack/plugins/cases/server/client/utils.test.ts +++ b/x-pack/plugins/cases/server/client/utils.test.ts @@ -15,7 +15,7 @@ import { CaseSeverity } from '../../common/api'; import { createSavedObjectsSerializerMock } from './mocks'; import { arraysDifference, - buildNestedFilter, + buildFilter, buildRangeFilter, constructQueryOptions, constructSearch, @@ -24,6 +24,106 @@ import { import { CasePersistedSeverity, CasePersistedStatus } from '../common/types/case'; describe('utils', () => { + describe('buildFilter', () => { + it('returns undefined if filters is undefined', () => { + expect(buildFilter({ filters: undefined, field: 'abc', operator: 'or' })).toBeUndefined(); + }); + + it('returns undefined if filters is is an empty array', () => { + expect(buildFilter({ filters: [], field: 'abc', operator: 'or' })).toBeUndefined(); + }); + + it('returns a KueryNode using or operator', () => { + expect(buildFilter({ filters: ['value1'], field: 'abc', operator: 'or' })) + .toMatchInlineSnapshot(` + Object { + "arguments": Array [ + Object { + "isQuoted": false, + "type": "literal", + "value": "cases.attributes.abc", + }, + Object { + "isQuoted": false, + "type": "literal", + "value": "value1", + }, + ], + "function": "is", + "type": "function", + } + `); + }); + + it("returns multiple nodes or'd together", () => { + expect(buildFilter({ filters: ['value1', 'value2'], field: 'abc', operator: 'or' })) + .toMatchInlineSnapshot(` + Object { + "arguments": Array [ + Object { + "arguments": Array [ + Object { + "isQuoted": false, + "type": "literal", + "value": "cases.attributes.abc", + }, + Object { + "isQuoted": false, + "type": "literal", + "value": "value1", + }, + ], + "function": "is", + "type": "function", + }, + Object { + "arguments": Array [ + Object { + "isQuoted": false, + "type": "literal", + "value": "cases.attributes.abc", + }, + Object { + "isQuoted": false, + "type": "literal", + "value": "value2", + }, + ], + "function": "is", + "type": "function", + }, + ], + "function": "or", + "type": "function", + } + `); + }); + + it('does not escape special kql characters in the filter values', () => { + const specialCharacters = 'awesome:()\\<>"*'; + + expect(buildFilter({ filters: [specialCharacters], field: 'abc', operator: 'or' })) + .toMatchInlineSnapshot(` + Object { + "arguments": Array [ + Object { + "isQuoted": false, + "type": "literal", + "value": "cases.attributes.abc", + }, + Object { + "isQuoted": false, + "type": "literal", + "value": "awesome:()\\\\<>\\"*", + }, + ], + "function": "is", + "type": "function", + } + `); + }); + }); + describe('convertSortField', () => { it('transforms status correctly', () => { expect(convertSortField('status')).toBe('status'); @@ -589,163 +689,6 @@ describe('utils', () => { }); }); - describe('buildNestedFilter', () => { - it('returns undefined if filters is undefined', () => { - expect(buildNestedFilter({ field: '', nestedField: '', operator: 'or' })).toBeUndefined(); - }); - - it('returns undefined when the filters array is empty', () => { - expect( - buildNestedFilter({ filters: [], field: '', nestedField: '', operator: 'or' }) - ).toBeUndefined(); - }); - - it('returns a KueryNode for a single filter', () => { - expect( - toElasticsearchQuery( - buildNestedFilter({ - filters: ['hello'], - field: 'uid', - nestedField: 'nestedField', - operator: 'or', - })! - ) - ).toMatchInlineSnapshot(` - Object { - "nested": Object { - "path": "cases.attributes.nestedField", - "query": Object { - "bool": Object { - "minimum_should_match": 1, - "should": Array [ - Object { - "match": Object { - "cases.attributes.nestedField.uid": "hello", - }, - }, - ], - }, - }, - "score_mode": "none", - }, - } - `); - }); - - it("returns a KueryNode for multiple filters or'd together", () => { - expect( - toElasticsearchQuery( - buildNestedFilter({ - filters: ['uid1', 'uid2'], - field: 'uid', - nestedField: 'nestedField', - operator: 'or', - })! - ) - ).toMatchInlineSnapshot(` - Object { - "bool": Object { - "minimum_should_match": 1, - "should": Array [ - Object { - "nested": Object { - "path": "cases.attributes.nestedField", - "query": Object { - "bool": Object { - "minimum_should_match": 1, - "should": Array [ - Object { - "match": Object { - "cases.attributes.nestedField.uid": "uid1", - }, - }, - ], - }, - }, - "score_mode": "none", - }, - }, - Object { - "nested": Object { - "path": "cases.attributes.nestedField", - "query": Object { - "bool": Object { - "minimum_should_match": 1, - "should": Array [ - Object { - "match": Object { - "cases.attributes.nestedField.uid": "uid2", - }, - }, - ], - }, - }, - "score_mode": "none", - }, - }, - ], - }, - } - `); - }); - - it("returns a KueryNode for multiple filters and'ed together", () => { - expect( - toElasticsearchQuery( - buildNestedFilter({ - filters: ['uid1', 'uid2'], - field: 'uid', - nestedField: 'nestedField', - operator: 'and', - })! - ) - ).toMatchInlineSnapshot(` - Object { - "bool": Object { - "filter": Array [ - Object { - "nested": Object { - "path": "cases.attributes.nestedField", - "query": Object { - "bool": Object { - "minimum_should_match": 1, - "should": Array [ - Object { - "match": Object { - "cases.attributes.nestedField.uid": "uid1", - }, - }, - ], - }, - }, - "score_mode": "none", - }, - }, - Object { - "nested": Object { - "path": "cases.attributes.nestedField", - "query": Object { - "bool": Object { - "minimum_should_match": 1, - "should": Array [ - Object { - "match": Object { - "cases.attributes.nestedField.uid": "uid2", - }, - }, - ], - }, - }, - "score_mode": "none", - }, - }, - ], - }, - } - `); - }); - }); - describe('arraysDifference', () => { it('returns null if originalValue is null', () => { expect(arraysDifference(null, [])).toBeNull(); diff --git a/x-pack/plugins/cases/server/client/utils.ts b/x-pack/plugins/cases/server/client/utils.ts index 0f198a6c8ba7..b630d940ac6b 100644 --- a/x-pack/plugins/cases/server/client/utils.ts +++ b/x-pack/plugins/cases/server/client/utils.ts @@ -197,10 +197,6 @@ interface FilterField { type?: string; } -interface NestedFilterField extends FilterField { - nestedField: string; -} - export const buildFilter = ({ filters, field, @@ -218,48 +214,7 @@ export const buildFilter = ({ } return nodeBuilder[operator]( - filtersAsArray.map((filter) => - nodeBuilder.is(`${escapeKuery(type)}.attributes.${escapeKuery(field)}`, escapeKuery(filter)) - ) - ); -}; - -/** - * Creates a KueryNode filter for the Saved Object find API's filter field. This handles constructing a filter for - * a nested field. - * - * @param filters is a string or array of strings that defines the values to search for - * @param field is the location to search for - * @param nestedField is the field in the saved object that has a type of 'nested' - * @param operator whether to 'or'/'and' the created filters together - * @type the type of saved object being searched - * @returns a constructed KueryNode representing the filter or undefined if one could not be built - */ -export const buildNestedFilter = ({ - filters, - field, - nestedField, - operator, - type = CASE_SAVED_OBJECT, -}: NestedFilterField): KueryNode | undefined => { - if (filters === undefined) { - return; - } - - const filtersAsArray = Array.isArray(filters) ? filters : [filters]; - - if (filtersAsArray.length === 0) { - return; - } - - return nodeBuilder[operator]( - filtersAsArray.map((filter) => - fromKueryExpression( - `${escapeKuery(type)}.attributes.${escapeKuery(nestedField)}:{ ${escapeKuery( - field - )}: ${escapeKuery(filter)} }` - ) - ) + filtersAsArray.map((filter) => nodeBuilder.is(`${type}.attributes.${field}`, filter)) ); }; @@ -365,7 +320,7 @@ export const buildAssigneesFilter = ({ ); const assigneesFilter = assigneesWithoutNone.map((filter) => - nodeBuilder.is(`${CASE_SAVED_OBJECT}.attributes.assignees.uid`, escapeKuery(filter)) + nodeBuilder.is(`${CASE_SAVED_OBJECT}.attributes.assignees.uid`, filter) ); if (!hasNoneAssignee) { diff --git a/x-pack/test/cases_api_integration/security_and_spaces/tests/common/cases/find_cases.ts b/x-pack/test/cases_api_integration/security_and_spaces/tests/common/cases/find_cases.ts index e4861323934f..60b1e789ec77 100644 --- a/x-pack/test/cases_api_integration/security_and_spaces/tests/common/cases/find_cases.ts +++ b/x-pack/test/cases_api_integration/security_and_spaces/tests/common/cases/find_cases.ts @@ -92,6 +92,29 @@ export default ({ getService }: FtrProviderContext): void => { }); }); + it('can filter by reserved kql characters in tags', async () => { + const tagsColon = ['super:bad:case']; + const tagsSlashQuote = ['awesome\\"']; + + const postedCase1 = await createCase(supertest, { ...postCaseReq, tags: tagsSlashQuote }); + const postedCase2 = await createCase(supertest, { + ...postCaseReq, + tags: tagsColon, + }); + + const cases = await findCases({ + supertest, + query: { tags: [...tagsColon, ...tagsSlashQuote] }, + }); + + expect(cases).to.eql({ + ...findCasesResp, + total: 2, + cases: [postedCase1, postedCase2], + count_open_cases: 2, + }); + }); + it('filters by status', async () => { await createCase(supertest, postCaseReq); const toCloseCase = await createCase(supertest, postCaseReq); diff --git a/x-pack/test/cases_api_integration/security_and_spaces/tests/trial/cases/assignees.ts b/x-pack/test/cases_api_integration/security_and_spaces/tests/trial/cases/assignees.ts index 35fa1e5acd75..ebf7b6888fbb 100644 --- a/x-pack/test/cases_api_integration/security_and_spaces/tests/trial/cases/assignees.ts +++ b/x-pack/test/cases_api_integration/security_and_spaces/tests/trial/cases/assignees.ts @@ -189,6 +189,30 @@ export default ({ getService }: FtrProviderContext): void => { }); }); + it('filters cases using an assignee uid with a colon in it', async () => { + await createCase(supertest, postCaseReq); + + const assigneeUid = 'abc:uid:123'; + const caseWithColonAssignee = await createCase( + supertest, + getPostCaseRequest({ + assignees: [{ uid: 'abc:uid:123' }], + }) + ); + + const cases = await findCases({ + supertest, + query: { assignees: [assigneeUid] }, + }); + + expect(cases).to.eql({ + ...findCasesResp, + total: 1, + cases: [caseWithColonAssignee], + count_open_cases: 1, + }); + }); + it("filters cases using the assigned users by constructing an or'd filter", async () => { const profileUidsToFilter = await suggestUserProfiles({ supertest: supertestWithoutAuth,