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

dhurley14 · 2020-02-06T22:48:51Z

Rejects request with 409 error if duplicate rule_ids are found in the request payload on bulk create.

Checklist

Delete any items that are not applicable to this PR.

~~- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~- [ ] Documentation was added for features that require explanation or tutorials~~

Unit or functional tests were updated or added to match the most common scenarios
~~- [ ] This was checked for keyboard-only and screenreader accessibility~~
~~- [ ] This renders correctly on smaller devices using a responsive layout. (You can test this in your browser~~
~~- [ ] This was checked for cross-browser compatibility, including a check against IE11~~

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2020-02-06T22:49:48Z

Pinging @elastic/siem (Team:SIEM)

elasticmachine · 2020-02-07T00:13:30Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 1a12d92

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts

...legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.test.ts

…licated rules even when duplicates are discovered, keeps same return type signature, updates relevant test for duplicates in request payload

dhurley14 · 2020-02-08T16:18:16Z

@elasticmachine merge upstream

FrankHassanabad

LGTM

x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.ts

…ates function

dhurley14 · 2020-02-12T15:04:10Z

@elasticmachine merge upstream

FrankHassanabad · 2020-02-12T15:31:08Z

There is a a e2e test located here:
https://github.com/elastic/kibana/blob/master/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/create_rules_bulk.ts#L83

It has a .skip, with this PR could you remove the .skip and the comment above it and then to speed up your testing comment out all e2e except for this one here:
https://github.com/elastic/kibana/blob/master/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/index.ts#L16

Then run this command:

node scripts/functional_tests --config x-pack/test/detection_engine_api_integration/security_and_spaces/config.ts

Once that test passes, go ahead and put back all the tests, and do a check-in.

FrankHassanabad · 2020-02-12T15:34:17Z

x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.test.ts

+      const expected = null;
+      expect(output).toEqual(expected);
+    });
+  });


Awesome! 👍 Love the tests! Going to pay off so much in the long run. Hopefully they give you the warm fuzzy feelings that what you're writing is some great quality stuff.

Your code is really starting to shine 🌟 with how you write it. Future maintainers, maybe even future Devin, will be nodding their head in approval if these ever break, like thanks past Devin you got my back!

FrankHassanabad · 2020-02-12T15:38:06Z

x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.ts

@@ -215,3 +216,13 @@ export const transformOrImportError = (
    });
  }
 };
+
+export const getDuplicates = (lodashDict: Dictionary<number>): string | null => {


That's a cool lodash type, haven't used it before.

rylnd

LGTM Once CI Is green (including that skipped e2e test!)

…ected 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.

x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts

… return logic

kibanamachine · 2020-02-13T00:10:41Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 23c459e

History

💚 Build #26355 succeeded ad56892
💚 Build #26339 succeeded e2609c1
💔 Build #26195 failed b58e9d6
💚 Build #25794 succeeded ee37882
💔 Build #25787 failed 64390df

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…oad (elastic#57057) * 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 <[email protected]>

* master: add `absolute` option to `getUrlForApp` (elastic#57193) [Telemetry] Migrate public to NP (elastic#56285) address flaky test where instances might have different start… (elastic#57506) fix(NA): support legacy plugins path in plugins (elastic#57472) build immutable bundles for new platform plugins (elastic#53976) [SIEM] [Detection Engine] Reject if duplicate rule_id in request payload (elastic#57057) Add autocomplete="off" for input type="password" to appease the scanners (elastic#56922) Use default spaces suffix for signals index if spaces disabled (elastic#57244) [Alerting] Create alert design cleanup (elastic#56929)

…oad (#57057) (#57526) * 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 <[email protected]> Co-authored-by: Elastic Machine <[email protected]>

…oad (#57057) (#57527) * 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 <[email protected]> Co-authored-by: Elastic Machine <[email protected]>

* master: (22 commits) Use log4j pattern syntax (elastic#57433) [ML] Categorization field example endpoint tests (elastic#57471) [Lens] Filter out pinned filters from saved object of Lens (elastic#57197) Lens client side shim cleanup (elastic#56976) [Maps] do not show border color for icon in legend when border width is zero (elastic#57501) refactors 'data-providers' tests (elastic#57474) add `absolute` option to `getUrlForApp` (elastic#57193) [Telemetry] Migrate public to NP (elastic#56285) address flaky test where instances might have different start… (elastic#57506) fix(NA): support legacy plugins path in plugins (elastic#57472) build immutable bundles for new platform plugins (elastic#53976) [SIEM] [Detection Engine] Reject if duplicate rule_id in request payload (elastic#57057) Add autocomplete="off" for input type="password" to appease the scanners (elastic#56922) Use default spaces suffix for signals index if spaces disabled (elastic#57244) [Alerting] Create alert design cleanup (elastic#56929) Management Api - add to migration guide (elastic#56892) fixing maps (elastic#56706) [Maps] Autocomplete for custom color palettes and custom icon palettes (elastic#56446) [Alerting] make actionGroup name's i18n-able (elastic#57404) fixed flaky test (elastic#57490) ... # Conflicts: # src/legacy/core_plugins/telemetry/public/components/__snapshots__/telemetry_form.test.js.snap # src/plugins/telemetry/public/components/telemetry_management_section.tsx

dhurley14 self-assigned this Feb 6, 2020

dhurley14 added release_note:skip Skip the PR/issue when compiling release notes Team:SIEM v7.6.1 v7.7.0 v8.0.0 labels Feb 6, 2020

FrankHassanabad reviewed Feb 7, 2020

View reviewed changes

x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts Outdated Show resolved Hide resolved

FrankHassanabad reviewed Feb 7, 2020

View reviewed changes

x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts Outdated Show resolved Hide resolved

FrankHassanabad reviewed Feb 7, 2020

View reviewed changes

...legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.test.ts Outdated Show resolved Hide resolved

dhurley14 force-pushed the check-for-dupes-on-bulk branch from 1a12d92 to 1241ef8 Compare February 7, 2020 16:30

dhurley14 added 3 commits February 7, 2020 14:43

prevents creation of rules when duplicate rule_id is present

fce198c

adds unit test to reflect change

7000f8b

genericizes duplicate discovery functions, allows creation of non-dup…

59df66d

…licated rules even when duplicates are discovered, keeps same return type signature, updates relevant test for duplicates in request payload

dhurley14 force-pushed the check-for-dupes-on-bulk branch from 1241ef8 to 59df66d Compare February 7, 2020 19:43

Merge branch 'master' into check-for-dupes-on-bulk

f00a7c4

FrankHassanabad approved these changes Feb 10, 2020

View reviewed changes

FrankHassanabad reviewed Feb 10, 2020

View reviewed changes

x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.ts Show resolved Hide resolved

rylnd reviewed Feb 10, 2020

View reviewed changes

x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.ts Outdated Show resolved Hide resolved

rylnd reviewed Feb 10, 2020

View reviewed changes

x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.ts Outdated Show resolved Hide resolved

dhurley14 added 2 commits February 10, 2020 19:50

utilizes countBy and removes reduce in favor of a filter on getDuplic…

64390df

…ates function

fix type

ee37882

Merge branch 'master' into check-for-dupes-on-bulk

b58e9d6

FrankHassanabad reviewed Feb 12, 2020

View reviewed changes

rylnd reviewed Feb 12, 2020

View reviewed changes

removes skip from e2e test for duplicates on bulk create, updates exp…

e2609c1

…ected 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.

rylnd reviewed Feb 12, 2020

View reviewed changes

x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts Outdated Show resolved Hide resolved

rylnd reviewed Feb 12, 2020

View reviewed changes

x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts Outdated Show resolved Hide resolved

dhurley14 added 2 commits February 12, 2020 17:02

getDuplicate returns empty array instead of null, removes unnecessary…

ad56892

… return logic

removes null coalescing from includes in filter

23c459e

dhurley14 merged commit ee6386b into elastic:master Feb 13, 2020

dhurley14 mentioned this pull request Feb 13, 2020

[7.x] [SIEM] [Detection Engine] Reject if duplicate rule_id in request payload (#57057) #57526

Merged

dhurley14 mentioned this pull request Feb 13, 2020

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dhurley14 commented Feb 6, 2020 •

edited

Loading

elasticmachine commented Feb 6, 2020

elasticmachine commented Feb 7, 2020

dhurley14 commented Feb 8, 2020

FrankHassanabad left a comment

dhurley14 commented Feb 12, 2020

FrankHassanabad commented Feb 12, 2020 •

edited

Loading

FrankHassanabad Feb 12, 2020

FrankHassanabad Feb 12, 2020

rylnd left a comment

kibanamachine commented Feb 13, 2020

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

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

Conversation

dhurley14 commented Feb 6, 2020 • edited Loading

Checklist

For maintainers

elasticmachine commented Feb 6, 2020

elasticmachine commented Feb 7, 2020

💚 Build Succeeded

dhurley14 commented Feb 8, 2020

FrankHassanabad left a comment

Choose a reason for hiding this comment

dhurley14 commented Feb 12, 2020

FrankHassanabad commented Feb 12, 2020 • edited Loading

FrankHassanabad Feb 12, 2020

Choose a reason for hiding this comment

FrankHassanabad Feb 12, 2020

Choose a reason for hiding this comment

rylnd left a comment

Choose a reason for hiding this comment

kibanamachine commented Feb 13, 2020

💚 Build Succeeded

History

dhurley14 commented Feb 6, 2020 •

edited

Loading

FrankHassanabad commented Feb 12, 2020 •

edited

Loading