[SIEM] Add support for actions and throttle in Rules #59641

patrykkopycinski · 2020-03-09T11:57:13Z

Summary

It's a part of #59004 effort, but I have decided to split it into couple PRs to make it easier to check

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

yctercero

LGTM! Added minor comment on response type.

yctercero · 2020-03-09T16:33:24Z

x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/schemas/response/schemas.ts

@@ -24,6 +24,7 @@ export const file_name = t.string;
 * become the actual ESFilter as a type.
 */
 export const filters = t.array(t.unknown); // Filters are not easily type-able yet
+export const actions = t.array(t.unknown);


Is this unkown for the same reason as filters? If so, maybe we can add the same comment?

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

x-pack/legacy/plugins/siem/server/lib/detection_engine/types.ts

elasticmachine · 2020-03-09T20:31:50Z

Pinging @elastic/siem (Team:SIEM)

x-pack/legacy/plugins/siem/server/lib/detection_engine/signals/signal_rule_alert_type.ts

…e-actions-throttle

dhurley14

LGTM!

x-pack/legacy/plugins/siem/server/lib/detection_engine/signals/build_signal.ts

FrankHassanabad

I would change back the type in one area and any other suggestions I have here and then I will approve.

x-pack/legacy/plugins/siem/server/lib/detection_engine/types.ts

…e-actions-throttle

patrykkopycinski · 2020-03-18T09:12:02Z

@elasticmachine merge upstream

…e-actions-throttle # Conflicts: # x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_route.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/import_rules_route.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/update_rules_bulk_route.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/update_rules_route.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.test.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/schemas/add_prepackaged_rules_schema.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/schemas/create_rules_schema.test.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/schemas/create_rules_schema.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/schemas/patch_rules_schema.test.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/schemas/patch_rules_schema.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/schemas/response/rules_schema.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/schemas/response/schemas.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/schemas/update_rules_schema.test.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/schemas/update_rules_schema.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/create_rules.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/get_export_all.test.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/get_export_by_object_ids.test.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/install_prepacked_rules.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/patch_rules.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/types.ts

rylnd

Most of these comments are just suggestions, feel free to ignore them. The one concern is the change to calculateVersion in patchRules, so I'm going to request changes until I get verification that that's intentional and not a bug.

rylnd · 2020-03-19T18:14:18Z

x-pack/legacy/plugins/siem/server/lib/detection_engine/types.ts

@@ -4,6 +4,8 @@
 * you may not use this file except in compliance with the Elastic License.
 */

+import { AlertAction } from '../../../../../../plugins/alerting/common';
+export { AlertAction } from '../../../../../../plugins/alerting/common';


It looks like we import from both this file and the original type throughout this branch. Should we consolidate to one location?

rylnd · 2020-03-19T18:22:11Z

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

@@ -82,8 +84,8 @@ export const createRules = ({
      },
      schedule: { interval },
      enabled,
-      actions: [], // TODO: Create and add actions here once we have email, etc...
-      throttle: null,
+      actions: actions ?? [],


The createRulesSchema validation should set this default, right? Do we need the coalescing here?

This might be because of the tech debt with some of the TypeScript "soup" issues that are from me when sharing types between patch and create here:

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

If that's true, and it's messy to correct this at this point without large refactoring you can add a comment like so:

actions ?? [], // FUNFACT: This is really never undefined here but since types are over shared between patch and create it exists here.

And we can clean this up down the road.

rylnd · 2020-03-19T18:24:14Z

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

@@ -76,7 +77,6 @@ export const patchRules = async ({
    type,
    references,
    version,
-    throttle,


This means that changes to throttle (and actions) will not bump the rule version, right? Is that the correct behavior?

After discussion with @rylnd and @FrankHassanabad we have decided to bump the rule's version whenever actions or throttle change

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

rylnd · 2020-03-19T18:39:49Z

x-pack/legacy/plugins/siem/server/lib/detection_engine/signals/build_rule.test.ts

+    expect(rule).toEqual(expected);
+  });
+
+  test('it omits a null value such as if throttle is undefined if is present', () => {


Test description is unclear; what is this testing that the previous test is not?

added " around the keys we're checking, did it help?

rylnd · 2020-03-19T18:41:34Z

x-pack/legacy/plugins/siem/server/lib/detection_engine/types.ts

@@ -59,11 +62,14 @@ export interface RuleAlertParams {
  threat: ThreatParams[] | undefined | null;
  type: RuleType;
  version: number;
-  throttle?: string;
+  throttle: string | null;


x-pack/legacy/plugins/siem/server/lib/detection_engine/types.ts

rylnd

On second thought, that minor bug shouldn't block this getting merged. We can address it in a followup.

FrankHassanabad · 2020-03-19T19:32:37Z

...plugins/siem/server/lib/detection_engine/routes/schemas/add_prepackaged_rules_schema.test.ts

+      }).value.throttle
+    ).toEqual(null);
+  });
+


Awesome tests 👍 !!!

...ck/legacy/plugins/siem/server/lib/detection_engine/routes/schemas/patch_rules_schema.test.ts

FrankHassanabad · 2020-03-19T19:43:39Z

x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/schemas/response/schemas.ts

+  t.type({
+    group: t.string,
+    id: t.string,
+    actionTypeId: t.string,


We might not have a choice but to keep this as camel case 🐫 , but if possible we should try for 🐍 snake case.

Up to you on this one, I know that the alerting team was made aware that they should be using snake case.

I leave this part as optional, but the more snake case we have now, the less migrations or headaches and support later for the API.

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

…e-actions-throttle

FrankHassanabad

I think this is really great improvements. I always appreciate people like you Patryk that can come up to speed quickly in code areas and learn both the detection engine and the alerting team's code base and navigate between different team's coding philosophy's.

You have been really good about fixing all the suggestions as well as adjusting your work schedule to quickly fix things while we are online which is really awesome of you. Thank you.

I know it's tricky to not mix in 🐫 case with 🐍 case for REST interfaces within Kibana and I left those as optional because I don't want to delay something as good as this and I have left a few 🐫 cases as well myself so I don't think it's such a big ordeal if those are in here for a while.

But since you added so many tests and took a lot of extra time to work through comments from myself and Ryland (as well as resolve conflicts from our PR) I think this is good to go and you should merge it before any other conflicts!

So 👍 and double 💪 💪

LGTM!

kibanamachine · 2020-03-20T01:08:35Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 585884e

History

💚 Build #34701 succeeded aaf4df5
💚 Build #34706 succeeded 1b03386
💚 Build #34442 succeeded abd6471
💛 Build #34104 was flaky fff7c85
💚 Build #33988 succeeded 2df113e

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

* master: (52 commits) [SIEM] Fix types in rules tests (elastic#60736) [Alerting] prevent flickering when fields are updated in an alert (elastic#60666) License checks for actions plugin (elastic#59070) Implemented ability to clear and properly validate alert interval (elastic#60571) WebElementWrapper: add findByTestSubject/findAllByTestSubject to search with data-test-subj (elastic#60568) [Maps] Update layer dependencies to NP (elastic#59585) [Discover] Remove StateManagementConfigProvider (elastic#60221) [ML] Listing all categorization wizard checks (elastic#60502) [Upgrade Assistant] First iteration of batch reindex docs (elastic#59887) [SIEM] Export timeline (elastic#58368) [SIEM] Add support for actions and throttle in Rules (elastic#59641) Fix ace a11y listener (elastic#60639) Add addInfo toast to core notifications service (elastic#60574) fix test description (elastic#60638) [SIEM] Cypress screenshots upload to google cloud (elastic#60556) [canvas/shareable_runtime] sync sass loaders with kbn/optimizer (elastic#60653) [SIEM] Fixes Modification of ML Rules (elastic#60662) [SIEM] [Case] Bulk status update, add comment avatar, id => title in breadcrumbs (elastic#60410) [Alerting] add functional tests for index threshold alertType (elastic#60597) [Ingest]EMT-248: add post action request handler and resources (elastic#60581) ...

* master: (55 commits) Update dependency @elastic/charts to v18.1.0 (elastic#60578) Only set timezone when user setting is a valid timezone (elastic#57850) [NP] Remove `ui/agg_types` dependencies and move paginated table to kibana_legacy (elastic#60276) [SIEM] Fix types in rules tests (elastic#60736) [Alerting] prevent flickering when fields are updated in an alert (elastic#60666) License checks for actions plugin (elastic#59070) Implemented ability to clear and properly validate alert interval (elastic#60571) WebElementWrapper: add findByTestSubject/findAllByTestSubject to search with data-test-subj (elastic#60568) [Maps] Update layer dependencies to NP (elastic#59585) [Discover] Remove StateManagementConfigProvider (elastic#60221) [ML] Listing all categorization wizard checks (elastic#60502) [Upgrade Assistant] First iteration of batch reindex docs (elastic#59887) [SIEM] Export timeline (elastic#58368) [SIEM] Add support for actions and throttle in Rules (elastic#59641) Fix ace a11y listener (elastic#60639) Add addInfo toast to core notifications service (elastic#60574) fix test description (elastic#60638) [SIEM] Cypress screenshots upload to google cloud (elastic#60556) [canvas/shareable_runtime] sync sass loaders with kbn/optimizer (elastic#60653) [SIEM] Fixes Modification of ML Rules (elastic#60662) ...

…o alerting/tls-warning * 'alerting/tls-warning' of github.com:gmmorris/kibana: (32 commits) [ML] Listing all categorization wizard checks (elastic#60502) [Upgrade Assistant] First iteration of batch reindex docs (elastic#59887) [SIEM] Export timeline (elastic#58368) [SIEM] Add support for actions and throttle in Rules (elastic#59641) Fix ace a11y listener (elastic#60639) Add addInfo toast to core notifications service (elastic#60574) fix test description (elastic#60638) [SIEM] Cypress screenshots upload to google cloud (elastic#60556) [canvas/shareable_runtime] sync sass loaders with kbn/optimizer (elastic#60653) [SIEM] Fixes Modification of ML Rules (elastic#60662) [SIEM] [Case] Bulk status update, add comment avatar, id => title in breadcrumbs (elastic#60410) [Alerting] add functional tests for index threshold alertType (elastic#60597) [Ingest]EMT-248: add post action request handler and resources (elastic#60581) Return incident's url (elastic#60617) [Endpoint] TEST: GET alert details - boundary test for first alert retrieval (elastic#60320) [ML] Transforms: Fix pivot preview table mapping. (elastic#60609) [Endpoint] Log random seed for sample data CLI to console (elastic#60646) Use common event model for determining if event is v0 or v1 (elastic#60667) Disables PR Project Assigner workflow [Reporting] Allow reports to be deleted in Management > Kibana > Reporting (elastic#60077) ...

…60727) * [SIEM] Add support for actions and throttle in Rules (#59641) * fix types Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

patrykkopycinski force-pushed the feat/siem-rule-actions-throttle branch from 6ebdfc5 to c27eacb Compare March 9, 2020 12:14

WIP

f8892c7

patrykkopycinski force-pushed the feat/siem-rule-actions-throttle branch from c27eacb to f8892c7 Compare March 9, 2020 12:16

cleanup

e9a87aa

patrykkopycinski self-assigned this Mar 9, 2020

patrykkopycinski added release_note:skip Skip the PR/issue when compiling release notes v7.7.0 v8.0.0 labels Mar 9, 2020

patrykkopycinski requested review from FrankHassanabad, dhurley14 and yctercero and removed request for FrankHassanabad March 9, 2020 13:17

patrykkopycinski changed the title ~~Feat/siem rule actions throttle~~ [SIEM] Add support for actions and throttle in Rules Mar 9, 2020

patrykkopycinski marked this pull request as ready for review March 9, 2020 14:41

fix tests

a628ade

yctercero approved these changes Mar 9, 2020

View reviewed changes

PR comments

027ea3a

dhurley14 reviewed Mar 9, 2020

View reviewed changes

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

dhurley14 reviewed Mar 9, 2020

View reviewed changes

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

patrykkopycinski added the Team:SIEM label Mar 9, 2020

dhurley14 reviewed Mar 10, 2020

View reviewed changes

x-pack/legacy/plugins/siem/server/lib/detection_engine/signals/signal_rule_alert_type.ts Outdated Show resolved Hide resolved

patrykkopycinski added 2 commits March 10, 2020 15:49

Merge branch 'master' of github.com:elastic/kibana into feat/siem-rul…

47a0a73

…e-actions-throttle

PR comments

65ec6df

dhurley14 approved these changes Mar 10, 2020

View reviewed changes

FrankHassanabad reviewed Mar 10, 2020

View reviewed changes

x-pack/legacy/plugins/siem/server/lib/detection_engine/signals/build_signal.ts Outdated Show resolved Hide resolved

FrankHassanabad suggested changes Mar 10, 2020

View reviewed changes

fix types

5bf1bb0

FrankHassanabad reviewed Mar 10, 2020

View reviewed changes

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

patrykkopycinski added 2 commits March 17, 2020 18:19

cleanup

50abbc2

add unit tests

1e969a9

patrykkopycinski requested a review from FrankHassanabad March 17, 2020 19:48

Merge branch 'master' of github.com:elastic/kibana into feat/siem-rul…

2df113e

…e-actions-throttle

elasticmachine and others added 3 commits March 18, 2020 05:12

Merge branch 'master' into feat/siem-rule-actions-throttle

fff7c85

fix types

abd6471

rylnd requested changes Mar 19, 2020

View reviewed changes

rylnd approved these changes Mar 19, 2020

View reviewed changes

FrankHassanabad reviewed Mar 19, 2020

View reviewed changes

...ck/legacy/plugins/siem/server/lib/detection_engine/routes/schemas/patch_rules_schema.test.ts Outdated Show resolved Hide resolved

FrankHassanabad reviewed Mar 19, 2020

View reviewed changes

...ck/legacy/plugins/siem/server/lib/detection_engine/routes/schemas/patch_rules_schema.test.ts Outdated Show resolved Hide resolved

FrankHassanabad reviewed Mar 19, 2020

View reviewed changes

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

patrykkopycinski added 3 commits March 19, 2020 21:05

Merge branch 'master' of github.com:elastic/kibana into feat/siem-rul…

fedc474

…e-actions-throttle

PR comments

aaf4df5

update response schema

1b03386

FrankHassanabad approved these changes Mar 19, 2020

View reviewed changes

patrykkopycinski added 2 commits March 20, 2020 00:12

actionTypeId => action_type_id

07aa54f

Add extra check

585884e

patrykkopycinski merged commit 8f1e22f into elastic:master Mar 20, 2020

patrykkopycinski deleted the feat/siem-rule-actions-throttle branch March 20, 2020 09:54

patrykkopycinski mentioned this pull request Mar 20, 2020

[7.x] [SIEM] Add support for actions and throttle in Rules (#59641) #60727

Merged

patrykkopycinski added a commit to patrykkopycinski/kibana that referenced this pull request Mar 20, 2020

[SIEM] Add support for actions and throttle in Rules (elastic#59641)

2c6eae7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SIEM] Add support for actions and throttle in Rules #59641

[SIEM] Add support for actions and throttle in Rules #59641

patrykkopycinski commented Mar 9, 2020 •

edited

Loading

yctercero left a comment

yctercero Mar 9, 2020

elasticmachine commented Mar 9, 2020

dhurley14 left a comment

FrankHassanabad left a comment

patrykkopycinski commented Mar 18, 2020

rylnd left a comment

rylnd Mar 19, 2020

rylnd Mar 19, 2020

FrankHassanabad Mar 19, 2020

rylnd Mar 19, 2020

patrykkopycinski Mar 19, 2020

rylnd Mar 19, 2020

patrykkopycinski Mar 19, 2020

rylnd Mar 19, 2020

rylnd left a comment

FrankHassanabad Mar 19, 2020

FrankHassanabad Mar 19, 2020

FrankHassanabad left a comment

kibanamachine commented Mar 20, 2020

[SIEM] Add support for actions and throttle in Rules #59641

[SIEM] Add support for actions and throttle in Rules #59641

Conversation

patrykkopycinski commented Mar 9, 2020 • edited Loading

Summary

Checklist

For maintainers

yctercero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Mar 9, 2020

dhurley14 left a comment

Choose a reason for hiding this comment

FrankHassanabad left a comment

Choose a reason for hiding this comment

patrykkopycinski commented Mar 18, 2020

rylnd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rylnd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrankHassanabad left a comment

Choose a reason for hiding this comment

kibanamachine commented Mar 20, 2020

💚 Build Succeeded

History

patrykkopycinski commented Mar 9, 2020 •

edited

Loading