[SIEM] [Detection Engine] Reject if duplicate rule_id in request payload (#57057) (#57527)

dhurley14 · elasticmachine · web-flow · commit d51c1481e704 · 2020-02-13T08:48:31.000-05:00
* prevents creation of rules when duplicate rule_id is present

* adds unit test to reflect change

* 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

* utilizes countBy and removes reduce in favor of a filter on getDuplicates function

* fix type

* 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.

* getDuplicate returns empty array instead of null, removes unnecessary return logic

* removes null coalescing from includes in filter

Co-authored-by: Elastic Machine &lt;elasticmachine@users.noreply.github.com&gt;

Co-authored-by: Elastic Machine &lt;elasticmachine@users.noreply.github.com&gt;
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
@@ -22,6 +22,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();
@@ -142,4 +144,34 @@ 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);
+    const output: Array<BulkError | Partial<OutputRuleAlertRest>> = 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<BulkError | Partial<OutputRuleAlertRest>> = 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
@@ -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 } from './utils';
+import { transformOrBulkError, getDuplicates } from './utils';
 import { getIndexExists } from '../../index/get_index_exists';
 import {
   callWithRequestFactory,
@@ -47,94 +47,109 @@ export const createCreateRulesBulkRoute = (server: ServerFacade): Hapi.ServerRou
         return headers.response().code(404);
       }
 
+      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;
-          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 => rule.rule_id == null || !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;
+      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
@@ -15,6 +15,7 @@ import {
   transformRulesToNdjson,
   transformAlertsToRules,
   transformOrImportError,
+  getDuplicates,
 } from './utils';
 import { getResult } from '../__mocks__/request_responses';
 import { INTERNAL_IDENTIFIER } from '../../../../../common/constants';
@@ -1202,4 +1203,25 @@ describe('utils', () => {
       expect(output).toEqual(expected);
     });
   });
+
+  describe('getDuplicates', () => {
+    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'];
+      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: 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
@@ -5,6 +5,7 @@
  */
 
 import { pickBy } from 'lodash/fp';
+import { Dictionary } from 'lodash';
 import { SavedObject } from 'kibana/server';
 import { INTERNAL_IDENTIFIER } from '../../../../../common/constants';
 import {
@@ -215,3 +216,11 @@ export const transformOrImportError = (
     });
   }
 };
+
+export const getDuplicates = (lodashDict: Dictionary<number>): string[] => {
+  const hasDuplicates = Object.values(lodashDict).some(i => i > 1);
+  if (hasDuplicates) {
+    return Object.keys(lodashDict).filter(key => lodashDict[key] > 1);
+  }
+  return [];
+};
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
@@ -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',
           },
         ]);
       });
