[SIEM][Detection Engine] Fixes return codes where some were rule_id i…

…nstead of id ## Summary Fixes some return error codes where they were `rule_id` when they should have been `id` - [x] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios
elastic · Feb 20, 2020 · 8097bd8 · 8097bd8
1 parent 1464706
commit 8097bd8
Show file tree

Hide file tree

Showing 8 changed files with 72 additions and 26 deletions.
diff --git a/...k/legacy/plugins/siem/server/lib/detection_engine/routes/rules/import_rules_route.test.ts b/...k/legacy/plugins/siem/server/lib/detection_engine/routes/rules/import_rules_route.test.ts
@@ -210,7 +210,7 @@ describe('import_rules_route', () => {
               message: 'Unexpected token : in JSON at position 8',
               status_code: 400,
             },
-            rule_id: '(unknown)',
+            rule_id: '(unknown id)',
           },
         ],
         success: false,
@@ -329,7 +329,7 @@ describe('import_rules_route', () => {
               message: 'Unexpected token : in JSON at position 8',
               status_code: 400,
             },
-            rule_id: '(unknown)',
+            rule_id: '(unknown id)',
           },
         ],
         success: false,

diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/import_rules_route.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/import_rules_route.ts
@@ -95,7 +95,6 @@ export const createImportRulesRoute = (
                   // early with the error and an (unknown) for the ruleId
                   resolve(
                     createBulkErrorObject({
-                      ruleId: '(unknown)',
                       statusCode: 400,
                       message: parsedRule.message,
                     })

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
@@ -794,10 +794,20 @@ describe('utils', () => {
   });
 
   describe('getIdBulkError', () => {
+    test('outputs message about id and rule_id not being found if both are not null', () => {
+      const error = getIdBulkError({ id: '123', ruleId: '456' });
+      const expected: BulkError = {
+        id: '123',
+        rule_id: '456',
+        error: { message: 'id: "123" and rule_id: "456" not found', status_code: 404 },
+      };
+      expect(error).toEqual(expected);
+    });
+
     test('outputs message about id not being found if only id is defined and ruleId is undefined', () => {
       const error = getIdBulkError({ id: '123', ruleId: undefined });
       const expected: BulkError = {
-        rule_id: '123',
+        id: '123',
         error: { message: 'id: "123" not found', status_code: 404 },
       };
       expect(error).toEqual(expected);
@@ -806,7 +816,7 @@ describe('utils', () => {
     test('outputs message about id not being found if only id is defined and ruleId is null', () => {
       const error = getIdBulkError({ id: '123', ruleId: null });
       const expected: BulkError = {
-        rule_id: '123',
+        id: '123',
         error: { message: 'id: "123" not found', status_code: 404 },
       };
       expect(error).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
@@ -62,9 +62,16 @@ export const getIdBulkError = ({
   id: string | undefined | null;
   ruleId: string | undefined | null;
 }): BulkError => {
-  if (id != null) {
+  if (id != null && ruleId != null) {
+    return createBulkErrorObject({
+      id,
+      ruleId,
+      statusCode: 404,
+      message: `id: "${id}" and rule_id: "${ruleId}" not found`,
+    });
+  } else if (id != null) {
     return createBulkErrorObject({
-      ruleId: id,
+      id,
       statusCode: 404,
       message: `id: "${id}" not found`,
     });
@@ -76,7 +83,6 @@ export const getIdBulkError = ({
     });
   } else {
     return createBulkErrorObject({
-      ruleId: '(unknown id)',
       statusCode: 404,
       message: `id or rule_id should have been defined`,
     });

diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/utils.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/utils.ts
@@ -44,7 +44,8 @@ export const transformError = (err: Error & { statusCode?: number }): OutputErro
 };
 
 export interface BulkError {
-  rule_id: string;
+  id?: string;
+  rule_id?: string;
   error: {
     status_code: number;
     message: string;
@@ -53,24 +54,54 @@ export interface BulkError {
 
 export const createBulkErrorObject = ({
   ruleId,
+  id,
   statusCode,
   message,
 }: {
-  ruleId: string;
+  ruleId?: string;
+  id?: string;
   statusCode: number;
   message: string;
 }): BulkError => {
-  return {
-    rule_id: ruleId,
-    error: {
-      status_code: statusCode,
-      message,
-    },
-  };
+  if (id != null && ruleId != null) {
+    return {
+      id,
+      rule_id: ruleId,
+      error: {
+        status_code: statusCode,
+        message,
+      },
+    };
+  } else if (id != null) {
+    return {
+      id,
+      error: {
+        status_code: statusCode,
+        message,
+      },
+    };
+  } else if (ruleId != null) {
+    return {
+      rule_id: ruleId,
+      error: {
+        status_code: statusCode,
+        message,
+      },
+    };
+  } else {
+    return {
+      rule_id: '(unknown id)',
+      error: {
+        status_code: statusCode,
+        message,
+      },
+    };
+  }
 };
 
 export interface ImportRuleResponse {
-  rule_id: string;
+  rule_id?: string;
+  id?: string;
   status_code?: number;
   message?: string;
   error?: {

diff --git a/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/delete_rules_bulk.ts b/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/delete_rules_bulk.ts
@@ -129,7 +129,7 @@ export default ({ getService }: FtrProviderContext): void => {
               message: 'id: "fake_id" not found',
               status_code: 404,
             },
-            rule_id: 'fake_id', // TODO This is a known issue where it should be id and not rule_id
+            id: 'fake_id',
           },
         ]);
       });
@@ -152,7 +152,7 @@ export default ({ getService }: FtrProviderContext): void => {
         const bodyToCompare = removeServerGeneratedPropertiesIncludingRuleId(body[0]);
         expect([bodyToCompare, body[1]]).to.eql([
           getSimpleRuleOutputWithoutRuleId(),
-          { rule_id: 'fake_id', error: { status_code: 404, message: 'id: "fake_id" not found' } },
+          { id: 'fake_id', error: { status_code: 404, message: 'id: "fake_id" not found' } },
         ]);
       });
     });
@@ -262,7 +262,7 @@ export default ({ getService }: FtrProviderContext): void => {
               message: 'id: "fake_id" not found',
               status_code: 404,
             },
-            rule_id: 'fake_id', // TODO This is a known issue where it should be id and not rule_id
+            id: 'fake_id',
           },
         ]);
       });
@@ -285,7 +285,7 @@ export default ({ getService }: FtrProviderContext): void => {
         const bodyToCompare = removeServerGeneratedPropertiesIncludingRuleId(body[0]);
         expect([bodyToCompare, body[1]]).to.eql([
           getSimpleRuleOutputWithoutRuleId(),
-          { rule_id: 'fake_id', error: { status_code: 404, message: 'id: "fake_id" not found' } },
+          { id: 'fake_id', error: { status_code: 404, message: 'id: "fake_id" not found' } },
         ]);
       });
     });

diff --git a/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/patch_rules_bulk.ts b/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/patch_rules_bulk.ts
@@ -263,7 +263,7 @@ export default ({ getService }: FtrProviderContext) => {
           .expect(200);
 
         expect(body).to.eql([
-          { rule_id: 'fake_id', error: { status_code: 404, message: 'id: "fake_id" not found' } },
+          { id: 'fake_id', error: { status_code: 404, message: 'id: "fake_id" not found' } },
         ]);
       });
 
@@ -347,7 +347,7 @@ export default ({ getService }: FtrProviderContext) => {
               message: 'id: "fake_id" not found',
               status_code: 404,
             },
-            rule_id: 'fake_id', // TODO: This should be id and not rule_id in the codebase
+            id: 'fake_id',
           },
         ]);
       });

diff --git a/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/update_rules_bulk.ts b/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/update_rules_bulk.ts
@@ -277,7 +277,7 @@ export default ({ getService }: FtrProviderContext) => {
           .expect(200);
 
         expect(body).to.eql([
-          { rule_id: 'fake_id', error: { status_code: 404, message: 'id: "fake_id" not found' } },
+          { id: 'fake_id', error: { status_code: 404, message: 'id: "fake_id" not found' } },
         ]);
       });
 
@@ -377,7 +377,7 @@ export default ({ getService }: FtrProviderContext) => {
               message: 'id: "fake_id" not found',
               status_code: 404,
             },
-            rule_id: 'fake_id', // TODO: This should be id and not rule_id in the codebase
+            id: 'fake_id',
           },
         ]);
       });