From 8097bd8dc1ca012bfdc516e1e44e47c3c943c7fc Mon Sep 17 00:00:00 2001 From: Frank Hassanabad Date: Wed, 19 Feb 2020 19:44:11 -0700 Subject: [PATCH] [SIEM][Detection Engine] Fixes return codes where some were rule_id instead 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 --- .../routes/rules/import_rules_route.test.ts | 4 +- .../routes/rules/import_rules_route.ts | 1 - .../routes/rules/utils.test.ts | 14 ++++- .../detection_engine/routes/rules/utils.ts | 12 +++-- .../lib/detection_engine/routes/utils.ts | 51 +++++++++++++++---- .../tests/delete_rules_bulk.ts | 8 +-- .../tests/patch_rules_bulk.ts | 4 +- .../tests/update_rules_bulk.ts | 4 +- 8 files changed, 72 insertions(+), 26 deletions(-) diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/import_rules_route.test.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/import_rules_route.test.ts index e3283a750869c..b1dd08f8ca371 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/import_rules_route.test.ts +++ b/x-pack/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 index cdb09ff7b04ed..f438e0120f96a 100644 --- 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 index 2b0da8251b387..593c55bcae9f2 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 @@ -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 index 21fc5a12db536..198cdbfb9771d 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 @@ -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 index 4a586e21c9e7f..55832ab67dc6b 100644 --- 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 index 5a1c178f6b211..6b87c94029189 100644 --- 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 index 3d14bc2db47b4..c13e8909dacf9 100644 --- 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 index 4894cac2b2608..220a4af4c5c5e 100644 --- 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', }, ]); });