[SIEM] Add rule notifications by patrykkopycinski · Pull Request #59004 · elastic/kibana

patrykkopycinski · 2020-03-02T09:15:51Z

Summary

Allow defining notifications that will trigger whenever the rule created new signals.

Requires:

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

…e-notifications

elasticmachine · 2020-03-02T09:16:29Z

Pinging @elastic/siem (Team:SIEM)

…e-notifications # Conflicts: # x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/helpers.tsx # x-pack/legacy/plugins/siem/public/shared_imports.ts

…e-notifications

…patrykkopycinski/kibana into feat/siem-rule-notifications-alert-type

…patrykkopycinski/kibana into feat/siem-rule-notifications # Conflicts: # x-pack/legacy/plugins/siem/common/constants.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/notifications/create_notifications.test.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/notifications/get_signals_count.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/notifications/read_notifications.test.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/notifications/rules_notification_alert_type.test.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/notifications/rules_notification_alert_type.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/notifications/schedule_notification_actions.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/notifications/types.test.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/notifications/update_notifications.test.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/__mocks__/request_responses.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/signals/signal_rule_alert_type.ts # x-pack/legacy/plugins/siem/server/plugin.ts

…riables, snakecase for rule params

…e-notifications # Conflicts: # x-pack/legacy/plugins/siem/common/constants.ts # x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/components/step_define_rule/index.tsx # x-pack/legacy/plugins/siem/server/lib/detection_engine/notifications/build_signals_query.test.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/notifications/build_signals_query.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/notifications/get_signals_count.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/notifications/rules_notification_alert_type.test.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/notifications/rules_notification_alert_type.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/notifications/schedule_notification_actions.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/notifications/utils.test.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/notifications/utils.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/signals/signal_rule_alert_type.ts

…e-notifications

spong · 2020-03-24T17:39:24Z

x-pack/legacy/plugins/siem/public/containers/detection_engine/rules/types.ts

+    group: t.string,
+    id: t.string,
+    action_type_id: t.string,
+    params: t.record(t.string, t.any),


t.any is deprecated it looks like and t.unknown is recommended instead. However I think we may be able to just use UnknownRecord ala params: t.UnknownRecord, -- thoughts?

PS: Thanks for the comment above pointing to the action templates 👍

spong · 2020-03-24T17:46:20Z

...acy/plugins/siem/public/pages/detection_engine/rules/components/rule_actions_field/index.tsx

+import React, { useCallback, useEffect, useState } from 'react';
+import deepMerge from 'deepmerge';
+
+// eslint-disable-next-line @kbn/eslint/no-restricted-paths


Any context as to why it's okay to import these even though they're coming from a restricted path? Should they not be restricted?

loadActionTypes are not exposed on top-level, but we need that to fetch current action types and then to filter only supported ones. It's a temporary solution, will talk to the alerting team to make sure loadActionTypes is exposed on the top-level

spong · 2020-03-24T18:04:17Z

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

  ruleId: string;
  index: string;
-  kibanaUrl: string | undefined;
+  kibanaSiemAppUrl: string | {} | undefined;


This can't actually be an object, but rather is a byproduct of the typing of meta correct?

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

Line 55 in ca3c5ab

meta: Record<string, {} | string> | undefined | null;

If this indeed can only ever be a string, might be best to keep it as such and narrow the type coming from meta in these two files? Thoughts?

kibana/x-pack/legacy/plugins/siem/server/lib/detection_engine/notifications/rules_notification_alert_type.ts

Line 50 in 7e82730

kibanaSiemAppUrl: ruleAlertParams.meta?.kibanaSiemAppUrl,

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

Line 223 in b9c8291

kibanaSiemAppUrl: meta.kibanaSiemAppUrl,

spong · 2020-03-24T18:21:49Z

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

            range: {
              '@timestamp': {
-                gte: from,
+                gt: from,


Just commenting to verify the switch from gte to gt as the timestamp supplied to this query:

kibana/x-pack/legacy/plugins/siem/server/lib/detection_engine/notifications/rules_notification_alert_type.ts

Line 46 in 7e82730

from: previousStartedAt ?? `now-${ruleParams.interval}`,

is the previousStartedAt (or previous interval if n/a), which is the start of the last rule run, correct?

spong · 2020-03-24T18:32:55Z

x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/create/helpers.test.ts

+      const mockAction = {
+        group: 'default',
+        id: '99403909-ca9b-49ba-9d7a-7e5320e68d05',
+        params: { message: 'ML Rule generated {{state.signalsCount}} signals' },


I believe this is the old count field as I was not seeing count come through when testing with the default message.

Suggested change

params: { message: 'ML Rule generated {{state.signalsCount}} signals' },

params: { message: 'ML Rule generated {{state.signals_count}} signals' },

spong · 2020-03-24T19:40:16Z

...y/plugins/siem/public/pages/detection_engine/rules/components/step_about_rule/index.test.tsx

 const theme = () => ({ eui: euiDarkVars, darkMode: true });

+/* eslint-disable no-console */
+// Silence until enzyme fixed to use ReactTestUtils.act()


Thank you for commenting -- sad to see this come back, but so it goes.

spong · 2020-03-24T19:43:51Z

...gacy/plugins/siem/public/pages/detection_engine/rules/components/step_rule_actions/index.tsx

+  const kibanaAbsoluteUrl = useMemo(() => application.getUrlForApp('siem', { absolute: true }), [
+    application,
+  ]);


Thanks for this fix -- I believe this should be fine for cloud, and shouldn't have any effect with CCS since this functionality is specific to rules/saved objects.

spong

Checked out, tested locally and was receiving notifications for signals as expected, and also performed code review. Left a few comments but overall this is fantastic @patrykkopycinski! Thanks for the clean implementation, additional tests, and great feature here! LGMT! 👍 🚀 🙂

kibanamachine · 2020-03-24T23:48:28Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 5f7cf4d

History

💚 Build #35977 succeeded 7cce784
💚 Build #35856 succeeded 7e82730
💔 Build #35745 failed ca3c5ab
💚 Build #35483 succeeded 953df6d
💚 Build #35352 succeeded c2f2934

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

## Summary Allow defining notifications that will trigger whenever the rule created new signals. Requires: - elastic#58395 - elastic#58964 - elastic#60832 ![Screenshot 2020-03-02 at 10 19 18](https://user-images.githubusercontent.com/5188868/75662390-4fe8bf00-5c6f-11ea-943f-591367348b91.png) ![Screenshot 2020-03-02 at 10 13 00](https://user-images.githubusercontent.com/5188868/75662421-5e36db00-5c6f-11ea-9317-d158cddf4344.png) ### Checklist Delete any items that are not applicable to this PR. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/README.md) - [ ] [Documentation](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#writing-documentation) was added for features that require explanation or tutorials - [ ] [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 - [ ] This was checked for [keyboard-only and screenreader accessibility](https://developer.mozilla.org/en-US/docs/Learn/Tools_and_testing/Cross_browser_testing/Accessibility#Accessibility_testing_checklist) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server) - [ ] This was checked for cross-browser compatibility, [including a check against IE11](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#release-notes-process)

alexfrancoeur · 2020-03-25T00:42:14Z

🎉 🎉🎉🎉

## Summary Allow defining notifications that will trigger whenever the rule created new signals. Requires: - #58395 - #58964 - #60832 ![Screenshot 2020-03-02 at 10 19 18](https://user-images.githubusercontent.com/5188868/75662390-4fe8bf00-5c6f-11ea-943f-591367348b91.png) ![Screenshot 2020-03-02 at 10 13 00](https://user-images.githubusercontent.com/5188868/75662421-5e36db00-5c6f-11ea-9317-d158cddf4344.png) ### Checklist Delete any items that are not applicable to this PR. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/README.md) - [ ] [Documentation](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#writing-documentation) was added for features that require explanation or tutorials - [ ] [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 - [ ] This was checked for [keyboard-only and screenreader accessibility](https://developer.mozilla.org/en-US/docs/Learn/Tools_and_testing/Cross_browser_testing/Accessibility#Accessibility_testing_checklist) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server) - [ ] This was checked for cross-browser compatibility, [including a check against IE11](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#release-notes-process) Co-authored-by: patrykkopycinski <patryk.kopycinski@elastic.co>

patrykkopycinski added 9 commits February 25, 2020 15:05

WIP

9b51432

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

4cbd25f

…e-notifications

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

c61f43f

…e-notifications

WIP

e282807

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

2c135cf

…e-notifications

WIP

469f066

WIP

c85c796

WIP

e2eb5b5

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

d75b549

…e-notifications

patrykkopycinski requested a review from a team as a code owner March 2, 2020 09:15

patrykkopycinski added release_note:enhancement v7.7.0 v8.0.0 labels Mar 2, 2020

patrykkopycinski self-assigned this Mar 2, 2020

patrykkopycinski added the Team:SIEM label Mar 2, 2020

patrykkopycinski changed the title ~~[SIEM] Add rule notifications~~ [WIP][SIEM] Add rule notifications Mar 2, 2020

patrykkopycinski added 13 commits March 2, 2020 10:31

cleanup

75dca43

Add throttle

659509f

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

defa879

…e-notifications # Conflicts: # x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/helpers.tsx # x-pack/legacy/plugins/siem/public/shared_imports.ts

cleanup

05c1352

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

3fb3569

…e-notifications

cleanup

ce75881

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

1245d0a

…e-notifications

cleanup

5d6282b

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

d57c6fd

…e-notifications

update unit tests

5b42fae

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

f0b8930

…e-notifications

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

3bce37b

…e-notifications

wip

caefbb2

patrykkopycinski changed the title ~~[WIP][SIEM] Add rule notifications~~ [SIEM] Add rule notifications Mar 23, 2020

patrykkopycinski added 11 commits March 23, 2020 22:01

cleanup

d69ec43

fetch action types

aac7501

PR comments

8771446

Merge branch 'feat/siem-rule-notifications-alert-type' of github.com:…

947911c

…patrykkopycinski/kibana into feat/siem-rule-notifications-alert-type

Attach kibanaSiemAppUrl to the rule meta, fix notification message va…

b9c8291

…riables, snakecase for rule params

cleanup

099b4f5

cleanup types

4cb0be2

fix key

7e82730

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

3ea2960

…e-notifications

spong reviewed Mar 24, 2020

View reviewed changes

spong approved these changes Mar 24, 2020

View reviewed changes

patrykkopycinski and others added 2 commits March 24, 2020 21:17

PR comments and pass ruleAlertId to the notification params

7cce784

Merge branch 'master' into feat/siem-rule-notifications

5f7cf4d

spong merged commit f9d37b3 into elastic:master Mar 24, 2020

spong mentioned this pull request Mar 24, 2020

[7.x] [SIEM] Add rule notifications (#59004) #61190

Merged

patrykkopycinski deleted the feat/siem-rule-notifications branch March 26, 2020 11:28

spong mentioned this pull request May 8, 2020

[Meta] Embeddable types #43879

Closed

1 task

	params: { message: 'ML Rule generated {{state.signalsCount}} signals' },
	params: { message: 'ML Rule generated {{state.signals_count}} signals' },

Conversation

patrykkopycinski commented Mar 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

elasticmachine commented Mar 2, 2020

Uh oh!

spong Mar 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spong left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Mar 24, 2020

💚 Build Succeeded

History

Uh oh!

alexfrancoeur commented Mar 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

patrykkopycinski commented Mar 2, 2020 •

edited

Loading

spong Mar 24, 2020 •

edited

Loading