From fce198cd97917338b88563e2da36ba46ce2b10a9 Mon Sep 17 00:00:00 2001 From: Devin Hurley Date: Thu, 6 Feb 2020 17:25:41 -0500 Subject: [PATCH 1/8] prevents creation of rules when duplicate rule_id is present --- .../routes/rules/create_rules_bulk_route.ts | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts index 0ffa61e2e2bed..8cc896cb3aca0 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts +++ b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts @@ -48,6 +48,35 @@ export const createCreateRulesBulkRoute = (server: ServerFacade): Hapi.ServerRou return headers.response().code(404); } + // Determine if duplicate rule_ids exist in the request + // before attempting to create them. + const mapDuplicates = request.payload.reduce>((acc, rule) => { + if (rule.rule_id != null && acc.has(rule.rule_id)) { + const totalView = acc.get(rule.rule_id) ?? 1; + acc.set(rule.rule_id, totalView + 1); + } else if (rule.rule_id != null) { + acc.set(rule.rule_id, 1); + } + return acc; + }, new Map()); + + const hasDuplicates = Array.from(mapDuplicates.values()).some(i => i > 1); + if (hasDuplicates) { + const duplicates = Array.from(mapDuplicates.entries()) + .reduce((acc, [key, val]) => { + if (val > 1) { + return [...acc, key]; + } + return acc; + }, []) + .join(', '); + return createBulkErrorObject({ + ruleId: duplicates, + statusCode: 409, + message: `rule_id: "${duplicates}" already exists`, + }); + } + const rules = await Promise.all( request.payload.map(async payloadRule => { const { From 7000f8bce4d1ccf9d9384b49c54bb8e779b1a355 Mon Sep 17 00:00:00 2001 From: Devin Hurley Date: Thu, 6 Feb 2020 17:47:09 -0500 Subject: [PATCH 2/8] adds unit test to reflect change --- .../routes/rules/create_rules_bulk_route.test.ts | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.test.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.test.ts index 5cf6d8955d8b2..c1db42d5e66fe 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.test.ts +++ b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.test.ts @@ -124,4 +124,18 @@ describe('create_rules_bulk', () => { expect(statusCode).toBe(400); }); }); + + test('returns 409 if duplicate rule_ids found in request payload', async () => { + alertsClient.find.mockResolvedValue(getFindResult()); + alertsClient.get.mockResolvedValue(getResult()); + actionsClient.create.mockResolvedValue(createActionResult()); + alertsClient.create.mockResolvedValue(getResult()); + const request: ServerInjectOptions = { + method: 'POST', + url: `${DETECTION_ENGINE_RULES_URL}/_bulk_create`, + payload: [typicalPayload(), typicalPayload()], + }; + const { payload } = await server.inject(request); + expect(JSON.parse(payload).error.status_code).toBe(409); + }); }); From 59df66d502bef074a113816e043dcc9be7ee886b Mon Sep 17 00:00:00 2001 From: Devin Hurley Date: Fri, 7 Feb 2020 11:30:00 -0500 Subject: [PATCH 3/8] genericizes duplicate discovery functions, allows creation of non-duplicated rules even when duplicates are discovered, keeps same return type signature, updates relevant test for duplicates in request payload --- .../rules/create_rules_bulk_route.test.ts | 5 ++- .../routes/rules/create_rules_bulk_route.ts | 39 +++++-------------- .../detection_engine/routes/rules/utils.ts | 29 ++++++++++++++ 3 files changed, 43 insertions(+), 30 deletions(-) diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.test.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.test.ts index c1db42d5e66fe..392a74e2da431 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.test.ts +++ b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.test.ts @@ -20,6 +20,8 @@ import { } from '../__mocks__/request_responses'; import { DETECTION_ENGINE_RULES_URL } from '../../../../../common/constants'; import { createRulesBulkRoute } from './create_rules_bulk_route'; +import { BulkError } from '../utils'; +import { OutputRuleAlertRest } from '../../types'; describe('create_rules_bulk', () => { let { server, alertsClient, actionsClient, elasticsearch } = createMockServer(); @@ -136,6 +138,7 @@ describe('create_rules_bulk', () => { payload: [typicalPayload(), typicalPayload()], }; const { payload } = await server.inject(request); - expect(JSON.parse(payload).error.status_code).toBe(409); + const output: Array> = JSON.parse(payload); + expect(output.some(item => item.error?.status_code === 409)).toBeTruthy(); }); }); diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts index 8cc896cb3aca0..801b52dfb1055 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts +++ b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts @@ -12,7 +12,7 @@ import { createRules } from '../../rules/create_rules'; import { BulkRulesRequest } from '../../rules/types'; import { ServerFacade } from '../../../../types'; import { readRules } from '../../rules/read_rules'; -import { transformOrBulkError } from './utils'; +import { transformOrBulkError, getMapDuplicates, getDuplicates } from './utils'; import { getIndexExists } from '../../index/get_index_exists'; import { callWithRequestFactory, @@ -48,34 +48,8 @@ export const createCreateRulesBulkRoute = (server: ServerFacade): Hapi.ServerRou return headers.response().code(404); } - // Determine if duplicate rule_ids exist in the request - // before attempting to create them. - const mapDuplicates = request.payload.reduce>((acc, rule) => { - if (rule.rule_id != null && acc.has(rule.rule_id)) { - const totalView = acc.get(rule.rule_id) ?? 1; - acc.set(rule.rule_id, totalView + 1); - } else if (rule.rule_id != null) { - acc.set(rule.rule_id, 1); - } - return acc; - }, new Map()); - - const hasDuplicates = Array.from(mapDuplicates.values()).some(i => i > 1); - if (hasDuplicates) { - const duplicates = Array.from(mapDuplicates.entries()) - .reduce((acc, [key, val]) => { - if (val > 1) { - return [...acc, key]; - } - return acc; - }, []) - .join(', '); - return createBulkErrorObject({ - ruleId: duplicates, - statusCode: 409, - message: `rule_id: "${duplicates}" already exists`, - }); - } + const mappedDuplicates = getMapDuplicates(request.payload, 'rule_id'); + const dupes = getDuplicates(mappedDuplicates); const rules = await Promise.all( request.payload.map(async payloadRule => { @@ -106,6 +80,13 @@ export const createCreateRulesBulkRoute = (server: ServerFacade): Hapi.ServerRou timeline_title: timelineTitle, version, } = payloadRule; + if (ruleId != null && dupes?.includes(ruleId)) { + return createBulkErrorObject({ + ruleId, + statusCode: 409, + message: `rule_id: "${ruleId}" already exists`, + }); + } const ruleIdOrUuid = ruleId ?? uuid.v4(); try { const finalIndex = outputIndex != null ? outputIndex : getIndex(request, server); diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.ts index b45db53c13d88..c29072976a073 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.ts +++ b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.ts @@ -206,3 +206,32 @@ export const transformOrImportError = ( }); } }; + +export const getMapDuplicates = ( + arr: Array<{ [key: string]: string }>, + prop: string +): Map => + arr.reduce>((acc, item) => { + if (item[prop] != null && acc.has(item[prop])) { + const totalView = acc.get(item[prop]) ?? 1; + acc.set(item[prop], totalView + 1); + } else if (item[prop] != null) { + acc.set(item[prop], 1); + } + return acc; + }, new Map()); + +export const getDuplicates = (someMap: Map): string | null => { + const hasDuplicates = Array.from(someMap.values()).some(i => i > 1); + if (hasDuplicates) { + return Array.from(someMap.entries()) + .reduce((acc, [key, val]) => { + if (val > 1) { + return [...acc, key]; + } + return acc; + }, []) + .join(', '); + } + return null; +}; From 64390df203ef00ba9d82f9c8adb924517638c8fd Mon Sep 17 00:00:00 2001 From: Devin Hurley Date: Mon, 10 Feb 2020 19:50:51 -0500 Subject: [PATCH 4/8] utilizes countBy and removes reduce in favor of a filter on getDuplicates function --- .../routes/rules/create_rules_bulk_route.ts | 6 ++-- .../routes/rules/utils.test.ts | 23 ++++++++++++++ .../detection_engine/routes/rules/utils.ts | 30 ++++--------------- 3 files changed, 32 insertions(+), 27 deletions(-) diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts index 801b52dfb1055..b45fc7d04efdc 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts +++ b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts @@ -5,14 +5,14 @@ */ import Hapi from 'hapi'; -import { isFunction } from 'lodash/fp'; +import { isFunction, countBy } from 'lodash/fp'; import uuid from 'uuid'; import { DETECTION_ENGINE_RULES_URL } from '../../../../../common/constants'; import { createRules } from '../../rules/create_rules'; import { BulkRulesRequest } from '../../rules/types'; import { ServerFacade } from '../../../../types'; import { readRules } from '../../rules/read_rules'; -import { transformOrBulkError, getMapDuplicates, getDuplicates } from './utils'; +import { transformOrBulkError, getDuplicates } from './utils'; import { getIndexExists } from '../../index/get_index_exists'; import { callWithRequestFactory, @@ -48,7 +48,7 @@ export const createCreateRulesBulkRoute = (server: ServerFacade): Hapi.ServerRou return headers.response().code(404); } - const mappedDuplicates = getMapDuplicates(request.payload, 'rule_id'); + const mappedDuplicates = countBy('rule_id', request.payload); const dupes = getDuplicates(mappedDuplicates); const rules = await Promise.all( diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.test.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.test.ts index ec11a8fb2da39..74ec35250b569 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.test.ts +++ b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.test.ts @@ -5,6 +5,7 @@ */ import Boom from 'boom'; +import { Dictionary } from 'lodash'; import { transformAlertToRule, @@ -17,6 +18,7 @@ import { transformRulesToNdjson, transformAlertsToRules, transformOrImportError, + getDuplicates, } from './utils'; import { getResult } from '../__mocks__/request_responses'; import { INTERNAL_IDENTIFIER } from '../../../../../common/constants'; @@ -1172,4 +1174,25 @@ describe('utils', () => { expect(output).toEqual(expected); }); }); + + describe('getDuplicates', () => { + test("returns a string showing the duplicate keys of 'value2' and 'value3'", () => { + const output = getDuplicates({ + value1: 1, + value2: 2, + value3: 2, + }); + const expected = 'value2, value3'; + expect(output).toEqual(expected); + }); + test('returns null when given a map of no duplicates', () => { + const output = getDuplicates({ + value1: 1, + value2: 1, + value3: 1, + }); + const expected = null; + expect(output).toEqual(expected); + }); + }); }); diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.ts index c29072976a073..929dab3f2878b 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.ts +++ b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.ts @@ -5,7 +5,8 @@ */ import Boom from 'boom'; -import { pickBy } from 'lodash/fp'; +import { pickBy, countBy } from 'lodash/fp'; +import { Dictionary } from 'lodash'; import { SavedObject } from 'kibana/server'; import { INTERNAL_IDENTIFIER } from '../../../../../common/constants'; import { @@ -207,30 +208,11 @@ export const transformOrImportError = ( } }; -export const getMapDuplicates = ( - arr: Array<{ [key: string]: string }>, - prop: string -): Map => - arr.reduce>((acc, item) => { - if (item[prop] != null && acc.has(item[prop])) { - const totalView = acc.get(item[prop]) ?? 1; - acc.set(item[prop], totalView + 1); - } else if (item[prop] != null) { - acc.set(item[prop], 1); - } - return acc; - }, new Map()); - -export const getDuplicates = (someMap: Map): string | null => { - const hasDuplicates = Array.from(someMap.values()).some(i => i > 1); +export const getDuplicates = (lodashDict: Dictionary): string | null => { + const hasDuplicates = Object.values(lodashDict).some(i => i > 1); if (hasDuplicates) { - return Array.from(someMap.entries()) - .reduce((acc, [key, val]) => { - if (val > 1) { - return [...acc, key]; - } - return acc; - }, []) + return Object.keys(lodashDict) + .filter(key => lodashDict[key] > 1) .join(', '); } return null; From ee378823f94f194cdd10ea91a66ae8cdfa5d4bc3 Mon Sep 17 00:00:00 2001 From: Devin Hurley Date: Mon, 10 Feb 2020 22:32:22 -0500 Subject: [PATCH 5/8] fix type --- .../siem/server/lib/detection_engine/routes/rules/utils.test.ts | 1 - .../siem/server/lib/detection_engine/routes/rules/utils.ts | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.test.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.test.ts index 74ec35250b569..463c31d030c0e 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.test.ts +++ b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.test.ts @@ -5,7 +5,6 @@ */ import Boom from 'boom'; -import { Dictionary } from 'lodash'; import { transformAlertToRule, diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.ts index 929dab3f2878b..a304a3180efab 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.ts +++ b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.ts @@ -5,7 +5,7 @@ */ import Boom from 'boom'; -import { pickBy, countBy } from 'lodash/fp'; +import { pickBy } from 'lodash/fp'; import { Dictionary } from 'lodash'; import { SavedObject } from 'kibana/server'; import { INTERNAL_IDENTIFIER } from '../../../../../common/constants'; From e2609c1129276d1970edacfe003523e865761f9a Mon Sep 17 00:00:00 2001 From: Devin Hurley Date: Wed, 12 Feb 2020 16:23:19 -0500 Subject: [PATCH 6/8] removes skip from e2e test for duplicates on bulk create, updates expected response in e2e test, fixes bug where duplicated error messages appeared for each instance of a duplicated rule_id (found this one through the e2e tests)! Adds unit test to catch this case. --- .../rules/create_rules_bulk_route.test.ts | 15 ++ .../routes/rules/create_rules_bulk_route.ts | 164 +++++++++--------- .../routes/rules/utils.test.ts | 4 +- .../detection_engine/routes/rules/utils.ts | 6 +- .../tests/create_rules_bulk.ts | 10 +- 5 files changed, 111 insertions(+), 88 deletions(-) diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.test.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.test.ts index 392a74e2da431..f1169442484c6 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.test.ts +++ b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.test.ts @@ -141,4 +141,19 @@ describe('create_rules_bulk', () => { const output: Array> = JSON.parse(payload); expect(output.some(item => item.error?.status_code === 409)).toBeTruthy(); }); + + test('returns one error object in response when duplicate rule_ids found in request payload', async () => { + alertsClient.find.mockResolvedValue(getFindResult()); + alertsClient.get.mockResolvedValue(getResult()); + actionsClient.create.mockResolvedValue(createActionResult()); + alertsClient.create.mockResolvedValue(getResult()); + const request: ServerInjectOptions = { + method: 'POST', + url: `${DETECTION_ENGINE_RULES_URL}/_bulk_create`, + payload: [typicalPayload(), typicalPayload()], + }; + const { payload } = await server.inject(request); + const output: Array> = JSON.parse(payload); + expect(output.length).toBe(1); + }); }); diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts index b45fc7d04efdc..202a9d515750b 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts +++ b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts @@ -48,104 +48,112 @@ export const createCreateRulesBulkRoute = (server: ServerFacade): Hapi.ServerRou return headers.response().code(404); } - const mappedDuplicates = countBy('rule_id', request.payload); + const ruleDefinitions = request.payload; + const mappedDuplicates = countBy('rule_id', ruleDefinitions); const dupes = getDuplicates(mappedDuplicates); const rules = await Promise.all( - request.payload.map(async payloadRule => { - const { - description, - enabled, - false_positives: falsePositives, - from, - query, - language, - output_index: outputIndex, - saved_id: savedId, - meta, - filters, - rule_id: ruleId, - index, - interval, - max_signals: maxSignals, - risk_score: riskScore, - name, - severity, - tags, - threat, - to, - type, - references, - timeline_id: timelineId, - timeline_title: timelineTitle, - version, - } = payloadRule; - if (ruleId != null && dupes?.includes(ruleId)) { - return createBulkErrorObject({ - ruleId, - statusCode: 409, - message: `rule_id: "${ruleId}" already exists`, - }); - } - const ruleIdOrUuid = ruleId ?? uuid.v4(); - try { - const finalIndex = outputIndex != null ? outputIndex : getIndex(request, server); - const callWithRequest = callWithRequestFactory(request, server); - const indexExists = await getIndexExists(callWithRequest, finalIndex); - if (!indexExists) { - return createBulkErrorObject({ - ruleId: ruleIdOrUuid, - statusCode: 400, - message: `To create a rule, the index must exist first. Index ${finalIndex} does not exist`, - }); - } - if (ruleId != null) { - const rule = await readRules({ alertsClient, ruleId }); - if (rule != null) { - return createBulkErrorObject({ - ruleId, - statusCode: 409, - message: `rule_id: "${ruleId}" already exists`, - }); - } - } - const createdRule = await createRules({ - alertsClient, - actionsClient, + ruleDefinitions + .filter(rule => !dupes?.includes(rule.rule_id ?? '')) + .map(async payloadRule => { + const { description, enabled, - falsePositives, + false_positives: falsePositives, from, - immutable: false, query, language, - outputIndex: finalIndex, - savedId, - timelineId, - timelineTitle, + output_index: outputIndex, + saved_id: savedId, meta, filters, - ruleId: ruleIdOrUuid, + rule_id: ruleId, index, interval, - maxSignals, - riskScore, + max_signals: maxSignals, + risk_score: riskScore, name, severity, tags, + threat, to, type, - threat, references, + timeline_id: timelineId, + timeline_title: timelineTitle, version, - }); - return transformOrBulkError(ruleIdOrUuid, createdRule); - } catch (err) { - return transformBulkError(ruleIdOrUuid, err); - } - }) + } = payloadRule; + const ruleIdOrUuid = ruleId ?? uuid.v4(); + try { + const finalIndex = outputIndex != null ? outputIndex : getIndex(request, server); + const callWithRequest = callWithRequestFactory(request, server); + const indexExists = await getIndexExists(callWithRequest, finalIndex); + if (!indexExists) { + return createBulkErrorObject({ + ruleId: ruleIdOrUuid, + statusCode: 400, + message: `To create a rule, the index must exist first. Index ${finalIndex} does not exist`, + }); + } + if (ruleId != null) { + const rule = await readRules({ alertsClient, ruleId }); + if (rule != null) { + return createBulkErrorObject({ + ruleId, + statusCode: 409, + message: `rule_id: "${ruleId}" already exists`, + }); + } + } + const createdRule = await createRules({ + alertsClient, + actionsClient, + description, + enabled, + falsePositives, + from, + immutable: false, + query, + language, + outputIndex: finalIndex, + savedId, + timelineId, + timelineTitle, + meta, + filters, + ruleId: ruleIdOrUuid, + index, + interval, + maxSignals, + riskScore, + name, + severity, + tags, + to, + type, + threat, + references, + version, + }); + return transformOrBulkError(ruleIdOrUuid, createdRule); + } catch (err) { + return transformBulkError(ruleIdOrUuid, err); + } + }) ); - return rules; + if (dupes) { + return [ + ...rules, + ...dupes.map(ruleId => + createBulkErrorObject({ + ruleId, + statusCode: 409, + message: `rule_id: "${ruleId}" already exists`, + }) + ), + ]; + } + return [...rules]; }, }; }; diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.test.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.test.ts index 03ce79aaf0aef..eb3e774c0e039 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.test.ts +++ b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.test.ts @@ -1205,13 +1205,13 @@ describe('utils', () => { }); describe('getDuplicates', () => { - test("returns a string showing the duplicate keys of 'value2' and 'value3'", () => { + test("returns array of ruleIds showing the duplicate keys of 'value2' and 'value3'", () => { const output = getDuplicates({ value1: 1, value2: 2, value3: 2, }); - const expected = 'value2, value3'; + const expected = ['value2', 'value3']; expect(output).toEqual(expected); }); test('returns null when given a map of no duplicates', () => { diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.ts index 3681a05de60e1..85e46ae1ad9ea 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.ts +++ b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.ts @@ -217,12 +217,10 @@ export const transformOrImportError = ( } }; -export const getDuplicates = (lodashDict: Dictionary): string | null => { +export const getDuplicates = (lodashDict: Dictionary): string[] | null => { const hasDuplicates = Object.values(lodashDict).some(i => i > 1); if (hasDuplicates) { - return Object.keys(lodashDict) - .filter(key => lodashDict[key] > 1) - .join(', '); + return Object.keys(lodashDict).filter(key => lodashDict[key] > 1); } return null; }; diff --git a/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/create_rules_bulk.ts b/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/create_rules_bulk.ts index dfa297c85dfb8..be008a34343c4 100644 --- a/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/create_rules_bulk.ts +++ b/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/create_rules_bulk.ts @@ -80,7 +80,7 @@ export default ({ getService }: FtrProviderContext): void => { }); // TODO: This is a valid issue and will be fixed in an upcoming PR and then enabled once that PR is merged - it.skip('should return a 200 ok but have a 409 conflict if we attempt to create the same rule_id twice', async () => { + it('should return a 200 ok but have a 409 conflict if we attempt to create the same rule_id twice', async () => { const { body } = await supertest .post(`${DETECTION_ENGINE_RULES_URL}/_bulk_create`) .set('kbn-xsrf', 'true') @@ -89,9 +89,11 @@ export default ({ getService }: FtrProviderContext): void => { expect(body).to.eql([ { - error: 'Conflict', - message: 'rule_id: "rule-1" already exists', - statusCode: 409, + error: { + message: 'rule_id: "rule-1" already exists', + status_code: 409, + }, + rule_id: 'rule-1', }, ]); }); From ad568922a1136135e4ed43826ac33a28523e8e60 Mon Sep 17 00:00:00 2001 From: Devin Hurley Date: Wed, 12 Feb 2020 17:02:33 -0500 Subject: [PATCH 7/8] getDuplicate returns empty array instead of null, removes unnecessary return logic --- .../routes/rules/create_rules_bulk_route.ts | 25 ++++++++----------- .../routes/rules/utils.test.ts | 2 +- .../detection_engine/routes/rules/utils.ts | 4 +-- 3 files changed, 14 insertions(+), 17 deletions(-) diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts index 202a9d515750b..a74c1658d7216 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts +++ b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts @@ -54,7 +54,7 @@ export const createCreateRulesBulkRoute = (server: ServerFacade): Hapi.ServerRou const rules = await Promise.all( ruleDefinitions - .filter(rule => !dupes?.includes(rule.rule_id ?? '')) + .filter(rule => !dupes.includes(rule.rule_id ?? '')) .map(async payloadRule => { const { description, @@ -141,19 +141,16 @@ export const createCreateRulesBulkRoute = (server: ServerFacade): Hapi.ServerRou } }) ); - if (dupes) { - return [ - ...rules, - ...dupes.map(ruleId => - createBulkErrorObject({ - ruleId, - statusCode: 409, - message: `rule_id: "${ruleId}" already exists`, - }) - ), - ]; - } - return [...rules]; + return [ + ...rules, + ...dupes.map(ruleId => + createBulkErrorObject({ + ruleId, + statusCode: 409, + message: `rule_id: "${ruleId}" already exists`, + }) + ), + ]; }, }; }; diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.test.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.test.ts index eb3e774c0e039..fb3262c476b40 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.test.ts +++ b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.test.ts @@ -1220,7 +1220,7 @@ describe('utils', () => { value2: 1, value3: 1, }); - const expected = null; + const expected: string[] = []; expect(output).toEqual(expected); }); }); diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.ts index 85e46ae1ad9ea..df9e3021e400f 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.ts +++ b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.ts @@ -217,10 +217,10 @@ export const transformOrImportError = ( } }; -export const getDuplicates = (lodashDict: Dictionary): string[] | null => { +export const getDuplicates = (lodashDict: Dictionary): string[] => { const hasDuplicates = Object.values(lodashDict).some(i => i > 1); if (hasDuplicates) { return Object.keys(lodashDict).filter(key => lodashDict[key] > 1); } - return null; + return []; }; From 23c459e26b3a931a969c6c4d0db2864e291da1c2 Mon Sep 17 00:00:00 2001 From: Devin Hurley Date: Wed, 12 Feb 2020 17:36:44 -0500 Subject: [PATCH 8/8] removes null coalescing from includes in filter --- .../detection_engine/routes/rules/create_rules_bulk_route.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts index a74c1658d7216..e7145d2a6f055 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts +++ b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts @@ -54,7 +54,7 @@ export const createCreateRulesBulkRoute = (server: ServerFacade): Hapi.ServerRou const rules = await Promise.all( ruleDefinitions - .filter(rule => !dupes.includes(rule.rule_id ?? '')) + .filter(rule => rule.rule_id == null || !dupes.includes(rule.rule_id)) .map(async payloadRule => { const { description,