[SIEM] [Detection Engine] Reject if duplicate rule_id in request payload

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<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);
+  });
 });

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];
     },
   };
 };

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', () => {

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<number>): string | null => {
+export const getDuplicates = (lodashDict: Dictionary<number>): 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;
 };

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',
           },
         ]);
       });