[Cases] Include rule registry client for updating alert statuses #108588

jonathan-buttner · 2021-08-13T19:38:50Z

This PR switches the cases plugin to leverage the rule registry's AlertsClient to perform updates and retrieval of alerts. This fixes a bug where the alert status would not be updated if the user does not have the appropriate permissions. Prior to this PR cases was interacting directly with the Elasticsearch indices that the alerts were stored in to update their status field and retrieve the alert.

There are a few differences with using the AlertsClient. The client provides a bulk update that accepts multiple ids that would be within the same index. Before we were relying on ES bulk update which accepted id and index pairs. Because of this we essentially do a Promise.all() for each alert that needs its status changed. The client has a get method to retrieve a single alert so we request each alert using a similar Promise.all() method.

I had to update the ES archives to match the latest mapping and data for the alerts for a few of our integration tests.

I exported a few types in the rule_registry so they could be imported by cases for tests and references the AlertsClient type.

…gistry

jonathan-buttner · 2021-08-16T17:29:45Z

x-pack/plugins/cases/kibana.json

@@ -23,6 +23,7 @@
 "features",
 "kibanaReact",
 "kibanaUtils",
+ "ruleRegistry",


We depend on the rule registry to interact with the alerts now.

ruleRegistry should be optional. Cases should still work without rule registry enabled.

jonathan-buttner · 2021-08-16T17:30:55Z

x-pack/plugins/cases/server/client/alerts/types.ts

-interface Alert {
- id: string;
- index: string;
- destination?: {


We weren't referencing these fields. We're using a lodash get so we don't actually need the explicit fields defined in the type.

jonathan-buttner · 2021-08-16T17:31:27Z

x-pack/plugins/cases/server/client/cases/push.ts

 import { CasesClient, CasesClientArgs, CasesClientInternal } from '..';
 import { Operations } from '../../authorization';
 import { casesConnectors } from '../../connectors';
-


Just moved this below the caller of it.

jonathan-buttner · 2021-08-16T17:32:38Z

x-pack/plugins/cases/server/services/alerts/index.ts

 return;
 }

- return scopedClusterClient.bulk({ body });
+ return await pMap(


Using pMap to limit the number of requests running at the same time.

jonathan-buttner · 2021-08-16T17:33:15Z

x-pack/plugins/cases/tsconfig.json

@@ -21,6 +21,7 @@

 // Required from './kibana.json'
 { "path": "../actions/tsconfig.json" },
+ { "path": "../rule_registry/tsconfig.json" },


Without this I get ts errors saying we can't import files from the rule_registry.

jonathan-buttner · 2021-08-16T17:33:54Z

x-pack/plugins/rule_registry/server/index.ts

@@ -29,6 +29,7 @@ export {
 } from './utils/create_lifecycle_executor';
 export { createPersistenceRuleTypeFactory } from './utils/create_persistence_rule_type_factory';
 export * from './utils/persistence_types';
+export type { AlertsClient } from './alert_data_client/alerts_client';


Exposing this type so other plugins can reference it when passing it around.

@dhurley14 @marshallmain

jonathan-buttner · 2021-08-16T17:34:13Z

x-pack/plugins/rule_registry/server/mocks.ts

 import { ruleDataPluginServiceMock } from './rule_data_plugin_service/rule_data_plugin_service.mock';
 import { createLifecycleAlertServicesMock } from './utils/lifecycle_alert_services_mock';

 export const ruleRegistryMocks = {
 createLifecycleAlertServices: createLifecycleAlertServicesMock,
 createRuleDataPluginService: ruleDataPluginServiceMock.create,
+ createAlertsClientMock: alertsClientMock,


Exposing the alerts client mock for cases unit tests.

jonathan-buttner · 2021-08-16T17:34:31Z

x-pack/test/case_api_integration/security_and_spaces/tests/common/cases/patch_cases.ts

@@ -535,8 +535,8 @@ export default ({ getService }: FtrProviderContext): void => {
 });

 it('should update the status of multiple alerts attached to multiple cases', async () => {
- const signalID = '5f2b9ec41f8febb1c06b5d1045aeabb9874733b7617e88a370510f2fb3a41a5d';
- const signalID2 = '4d0f4b1533e46b66b43bdd0330d23f39f2cf42a7253153270e38d30cce9ff0c6';
+ const signalID = '4679431ee0ba3209b6fcd60a255a696886fe0a7d18f5375de510ff5b68fa6b78';


These changed because I had to regenerate the es archives with new alerts.

elasticmachine · 2021-08-16T17:34:54Z

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

dhurley14

Cases should be able to function without rule registry enabled. Not sure if it really needs to be a required dependency for cases plugin.

Once that change is made this LGTM!

dhurley14 · 2021-08-17T13:43:51Z

x-pack/plugins/cases/kibana.json

@@ -23,6 +23,7 @@
 "features",
 "kibanaReact",
 "kibanaUtils",
+ "ruleRegistry",


ruleRegistry should be optional. Cases should still work without rule registry enabled.

jonathan-buttner · 2021-08-17T15:17:06Z

Cases should be able to function without rule registry enabled. Not sure if it really needs to be a required dependency for cases plugin.

Once that change is made this LGTM!

Thanks for the review Devin! Previous to the rule registry cases depended on Elasticsearch to handle updating the status of alerts and retrieving alerts. This was a required dependency. Would a user still be able to create alerts if the rule registry is disabled? If a user can still do this, then it's feasible that they could attach those alerts to a case. We don't currently have the ability to inform the user that the updating/retrieving of alerts functionality within cases is disabled in the situation that the rule registry is disabled. So I'm wondering for now if cases should not function if the rule registry is not available.

dhurley14 · 2021-08-17T15:42:58Z

@jonathan-buttner Maybe I'm misunderstanding something. Is it possible for a user to open a case without attaching / referencing an alert? If not, then I suppose the dependency makes sense.

If a user should be able to open a case without referencing an alert, then cases doesn't really depend on alerts and should be functional without that dependency.

jonathan-buttner · 2021-08-17T15:58:08Z

Is it possible for a user to open a case without attaching / referencing an alert?

Yeah it is possible. The problem I'm getting at is this scenario:

Let's say the rule registry is disabled. A user attaches an alert to a case. They then update the status of the case hoping that the alert's status will change as well. Behind the scenes cases will skip trying to update the status of the alert. The way this PR is currently written, there will be no API error, so updating the status of the case will succeed but the alert's status will not change and the user won't know why without looking in the logs. The other option is we could respond with an API error when the case status is being changed. If we do this, the user won't be able to update their case status because it'll fail. There's no way to remove the alerts via the cases UI right now, so they'd have to do it through direct API requests if they want the ability to change the status of the case.

The best solution would be to have the rule registry be an optional plugin and when it is disabled, disable the alerts functionality in the cases UI and make it clear to the user why that functionality is disabled. I just don't think we'd be able to accomplish that before FF today. That's why in the short term I was hoping to make the rule registry a required plugin of cases.

dhurley14 · 2021-08-17T16:05:06Z

I approved so feel free to ignore and do what's best 👍

I don't think we should use FF as a reason for making optional plugins required but I'll leave that decision up to @XavierM and @asnehalb

…gistry

kibanamachine · 2021-08-18T20:59:48Z

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

API count

id	before	after	diff
`ruleRegistry`	115	134	+19

API count missing comments

id	before	after	diff
`ruleRegistry`	95	113	+18

Non-exported public API item count

id	before	after	diff
`ruleRegistry`	4	6	+2

History

💔 Build #146312 failed 83a3663
💚 Build #145817 succeeded 2f585da
💔 Build #145416 failed 26f1f9e
💔 Build #145367 failed 59da630

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

jonathan-buttner · 2021-08-18T21:15:02Z

x-pack/plugins/cases/server/client/attachments/add.ts

 });

- if (


These checks were moved into the CommentableCase class so they can be done immediately before creating a case but after some validation.

jonathan-buttner · 2021-08-18T21:16:06Z

x-pack/plugins/cases/server/connectors/servicenow/sir_format.test.ts

- source: { ip: '192.168.1.2' },
- file: {
- hash: { sha256: '9f86d081884c7d659a2feaa0c55ad015a3bf4f1b2b0b822cd15d6c15b0f00a08' },
+ source: {


Everything is nested under source now.

jonathan-buttner · 2021-08-18T21:16:51Z

x-pack/plugins/cases/server/connectors/servicenow/sir_format.ts

- return acc;
- }, sirFields);
+ sirFields = alerts
+ .filter((alert) => !alert.error && alert.source != null)


I added this to ignore alerts with errors and ones that don't have source.

jonathan-buttner · 2021-08-18T21:17:09Z

x-pack/plugins/cases/server/connectors/servicenow/sir_format.ts

+ .filter((alert) => !alert.error && alert.source != null)
+ .reduce<Record<SirFieldKey, string | null>>((acc, alert) => {
+ fieldsToAdd.forEach((alertField) => {
+ const field = get(alertFieldMapping[alertField].alertPath, alert.source);


Grab the fields from alert.source now since everything is nested under there.

XavierM · 2021-08-19T13:24:01Z

x-pack/plugins/cases/server/client/alerts/get.ts

- index: alert._index,
- ...alert._source,
- }));
+ const alerts = await alertsService.getAlerts({ alertsInfo, logger });


nice clean up here but are we sure we are not breaking other people stuff by making this change

MadameSheema · 2021-08-19T18:58:42Z

Ey team, I think I found the following corner case, can you please take a look at it to make sure that I didn't mess with my testing?

Preconditions:

To have a case with an attached alert. Everything created with a user with all the privileges. The case should have the sync alerts feature activated.

Steps to reproduce:

Login with a user with all the privileges for cases and just readme permissions for alerts. Note that the user has all the index permissions.
Open the case with the created alert
Open the alert details
Click on the Take action button at the bottom of the event details
Click on Add rule exception
Fill the mandatory fields
Select Close this alert option
Click on Add rule exception

Current behaviour:

The alert has been closed

XavierM · 2021-08-19T20:01:17Z

Ey team, I think I found the following corner case, can you please take a look at it to make sure that I didn't mess with my testing?

Preconditions:

To have a case with an attached alert. Everything created with a user with all the privileges. The case should have the sync alerts feature activated.

Steps to reproduce:

Login with a user with all the privileges for cases and just readme permissions for alerts. Note that the user has all the index permissions.

Open the case with the created alert

Open the alert details

Click on the Take action button at the bottom of the event details

Click on Add rule exception

Fill the mandatory fields

Select Close this alert option

Click on Add rule exception

Current behaviour:

The alert has been closed

@angorayc I do not think that we used the latest alertClientData to update the alert status and we should remove the menu items if user does not have the alerts permissions. Please check if what I say is true

…stic#108588) * Trying to get import to work * Plumbed alerts client through and logging errors * No longer need the ES cluster client * Fixing types * Fixing imports * Fixing integration tests and refactoring * Throwing an error when rule registry is disabled * Reworking alert update and get to catch errors * Adding tests and fixing errors

kibanamachine · 2021-08-19T20:08:32Z

💚 Backport successful

Status	Branch	Result
✅	7.15
✅	7.x

The backport PRs will be merged automatically after passing CI.

…8588) (#109335) * Trying to get import to work * Plumbed alerts client through and logging errors * No longer need the ES cluster client * Fixing types * Fixing imports * Fixing integration tests and refactoring * Throwing an error when rule registry is disabled * Reworking alert update and get to catch errors * Adding tests and fixing errors Co-authored-by: Jonathan Buttner <56361221+jonathan-buttner@users.noreply.github.com>

…8588) (#109334) * Trying to get import to work * Plumbed alerts client through and logging errors * No longer need the ES cluster client * Fixing types * Fixing imports * Fixing integration tests and refactoring * Throwing an error when rule registry is disabled * Reworking alert update and get to catch errors * Adding tests and fixing errors Co-authored-by: Jonathan Buttner <56361221+jonathan-buttner@users.noreply.github.com>

angorayc · 2021-08-20T18:02:12Z

Hi @MadameSheema , regarding to #108588 (comment), could you please retest it, seemed that it's been fixed. Thank you!

…ses (elastic#108588)" This reverts commit 1fd7038. This leaves the rule registry mock changes

* wip to remove rbac * Revert "[Cases] Include rule registry client for updating alert statuses (#108588)" This reverts commit 1fd7038. This leaves the rule registry mock changes * remove rbac on Trend/Count alert * update detection api for status * remove @kbn-alerts packages * fix leftover * Switching cases to leverage update by query for alert status * Adding missed files * fix bad logic * updating tests for use_alerts_privileges * remove index alias/fields * fix types * fix plugin to get the right index names * left over of alis on template * forget to use current user for create/read route index * updated alerts page to not show table when no privileges and updates to tests * fix bug when switching between o11y and security solution * updates tests and move to use privileges page when user tries to access alerts without proper access * updating jest tests * pairing with yara * bring back kbn-alerts after discussion with the team * fix types * fix index field for o11y * fix bug with updating index priv state * fix i18n issue and update api docs * fix refresh on alerts * fix render view on alerts * updating tests and checking for null in alerts page to not show no privileges page before load * fix details rules Co-authored-by: Jonathan Buttner <jonathan.buttner@elastic.co> Co-authored-by: Yara Tercero <yara.tercero@elastic.co>

* wip to remove rbac * Revert "[Cases] Include rule registry client for updating alert statuses (elastic#108588)" This reverts commit 1fd7038. This leaves the rule registry mock changes * remove rbac on Trend/Count alert * update detection api for status * remove @kbn-alerts packages * fix leftover * Switching cases to leverage update by query for alert status * Adding missed files * fix bad logic * updating tests for use_alerts_privileges * remove index alias/fields * fix types * fix plugin to get the right index names * left over of alis on template * forget to use current user for create/read route index * updated alerts page to not show table when no privileges and updates to tests * fix bug when switching between o11y and security solution * updates tests and move to use privileges page when user tries to access alerts without proper access * updating jest tests * pairing with yara * bring back kbn-alerts after discussion with the team * fix types * fix index field for o11y * fix bug with updating index priv state * fix i18n issue and update api docs * fix refresh on alerts * fix render view on alerts * updating tests and checking for null in alerts page to not show no privileges page before load * fix details rules Co-authored-by: Jonathan Buttner <jonathan.buttner@elastic.co> Co-authored-by: Yara Tercero <yara.tercero@elastic.co> # Conflicts: # x-pack/plugins/security_solution/cypress/integration/detection_alerts/alerts_details.spec.ts

* [RAC] Remove rbac on security solution side (#110472) * wip to remove rbac * Revert "[Cases] Include rule registry client for updating alert statuses (#108588)" This reverts commit 1fd7038. This leaves the rule registry mock changes * remove rbac on Trend/Count alert * update detection api for status * remove @kbn-alerts packages * fix leftover * Switching cases to leverage update by query for alert status * Adding missed files * fix bad logic * updating tests for use_alerts_privileges * remove index alias/fields * fix types * fix plugin to get the right index names * left over of alis on template * forget to use current user for create/read route index * updated alerts page to not show table when no privileges and updates to tests * fix bug when switching between o11y and security solution * updates tests and move to use privileges page when user tries to access alerts without proper access * updating jest tests * pairing with yara * bring back kbn-alerts after discussion with the team * fix types * fix index field for o11y * fix bug with updating index priv state * fix i18n issue and update api docs * fix refresh on alerts * fix render view on alerts * updating tests and checking for null in alerts page to not show no privileges page before load * fix details rules Co-authored-by: Jonathan Buttner <jonathan.buttner@elastic.co> Co-authored-by: Yara Tercero <yara.tercero@elastic.co> # Conflicts: # x-pack/plugins/security_solution/cypress/integration/detection_alerts/alerts_details.spec.ts * skip test Co-authored-by: Xavier Mouligneau <189600+XavierM@users.noreply.github.com>

* [Security Solution] Updates loock-back time on Cypress tests (#110609) * updates loock-back time * updates loock-back value for 'expectedExportedRule' * skips tests to unblock 7.15 branch * [RAC] Remove rbac on security solution side (#110472) * wip to remove rbac * Revert "[Cases] Include rule registry client for updating alert statuses (#108588)" This reverts commit 1fd7038. This leaves the rule registry mock changes * remove rbac on Trend/Count alert * update detection api for status * remove @kbn-alerts packages * fix leftover * Switching cases to leverage update by query for alert status * Adding missed files * fix bad logic * updating tests for use_alerts_privileges * remove index alias/fields * fix types * fix plugin to get the right index names * left over of alis on template * forget to use current user for create/read route index * updated alerts page to not show table when no privileges and updates to tests * fix bug when switching between o11y and security solution * updates tests and move to use privileges page when user tries to access alerts without proper access * updating jest tests * pairing with yara * bring back kbn-alerts after discussion with the team * fix types * fix index field for o11y * fix bug with updating index priv state * fix i18n issue and update api docs * fix refresh on alerts * fix render view on alerts * updating tests and checking for null in alerts page to not show no privileges page before load * fix details rules Co-authored-by: Jonathan Buttner <jonathan.buttner@elastic.co> Co-authored-by: Yara Tercero <yara.tercero@elastic.co> # Conflicts: # x-pack/plugins/security_solution/cypress/integration/detection_alerts/alerts_details.spec.ts * skip tests Co-authored-by: Gloria Hornero <snootchie.boochies@gmail.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Xavier Mouligneau <189600+XavierM@users.noreply.github.com>

Trying to get import to work

59da630

jonathan-buttner added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting Security Solution Threat Hunting Team Feature:Cases Cases feature v7.15.0 labels Aug 13, 2021

jonathan-buttner added 4 commits August 13, 2021 17:27

Plumbed alerts client through and logging errors

c8b7028

No longer need the ES cluster client

d9bf686

Fixing types

44768f1

Fixing imports

26f1f9e

jonathan-buttner mentioned this pull request Aug 16, 2021

[Security Solution][RAC] - 7.15 Checklist #108071

Closed

48 tasks

jonathan-buttner added 2 commits August 16, 2021 11:20

Merge branch 'master' of github.com:elastic/kibana into cases-rule-re…

ed6c6c8

…gistry

Fixing integration tests and refactoring

2f585da

jonathan-buttner commented Aug 16, 2021

View reviewed changes

jonathan-buttner marked this pull request as ready for review August 16, 2021 17:34

jonathan-buttner requested a review from a team as a code owner August 16, 2021 17:34

jonathan-buttner requested review from XavierM, michaelolo24, marshallmain and dhurley14 August 16, 2021 17:35

dhurley14 approved these changes Aug 17, 2021

View reviewed changes

jonathan-buttner added 2 commits August 17, 2021 13:32

Merge branch 'master' of github.com:elastic/kibana into cases-rule-re…

f71d00e

…gistry

Throwing an error when rule registry is disabled

83a3663

jonathan-buttner added v7.16.0 auto-backport Deprecated - use backport:version if exact versions are needed labels Aug 18, 2021

Reworking alert update and get to catch errors

72379b7

mistic removed the v7.15.0 label Aug 18, 2021

jonathan-buttner added 2 commits August 18, 2021 13:51

Adding tests and fixing errors

c4a86fc

Merge branch 'master' of github.com:elastic/kibana into cases-rule-re…

a4c9aff

…gistry

jonathan-buttner added the v7.15.0 label Aug 18, 2021

jonathan-buttner commented Aug 18, 2021

View reviewed changes

XavierM reviewed Aug 19, 2021

View reviewed changes

XavierM approved these changes Aug 19, 2021

View reviewed changes

jonathan-buttner merged commit 1fd7038 into elastic:master Aug 19, 2021

kibanamachine mentioned this pull request Aug 19, 2021

[7.15] [Cases] Include rule registry client for updating alert statuses (#108588) #109334

Merged

kibanamachine mentioned this pull request Aug 19, 2021

[7.x] [Cases] Include rule registry client for updating alert statuses (#108588) #109335

Merged

jonathan-buttner deleted the cases-rule-registry branch August 19, 2021 20:10

jonathan-buttner added a commit to XavierM/kibana that referenced this pull request Aug 30, 2021

Revert "[Cases] Include rule registry client for updating alert statu…

3e76999

…ses (elastic#108588)" This reverts commit 1fd7038. This leaves the rule registry mock changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Cases] Include rule registry client for updating alert statuses #108588

[Cases] Include rule registry client for updating alert statuses #108588

jonathan-buttner commented Aug 13, 2021 •

edited

Loading

jonathan-buttner Aug 16, 2021

dhurley14 Aug 17, 2021

jonathan-buttner Aug 16, 2021

jonathan-buttner Aug 16, 2021

jonathan-buttner Aug 16, 2021

jonathan-buttner Aug 16, 2021

jonathan-buttner Aug 16, 2021

jonathan-buttner Aug 16, 2021

jonathan-buttner Aug 16, 2021

jonathan-buttner Aug 16, 2021

elasticmachine commented Aug 16, 2021

dhurley14 left a comment

dhurley14 Aug 17, 2021

jonathan-buttner commented Aug 17, 2021

dhurley14 commented Aug 17, 2021

jonathan-buttner commented Aug 17, 2021

dhurley14 commented Aug 17, 2021

kibanamachine commented Aug 18, 2021

API count

API count missing comments

Non-exported public API item count

jonathan-buttner Aug 18, 2021

jonathan-buttner Aug 18, 2021

jonathan-buttner Aug 18, 2021

jonathan-buttner Aug 18, 2021

XavierM Aug 19, 2021

MadameSheema commented Aug 19, 2021

XavierM commented Aug 19, 2021

kibanamachine commented Aug 19, 2021

angorayc commented Aug 20, 2021 •

edited

Loading

[Cases] Include rule registry client for updating alert statuses #108588

[Cases] Include rule registry client for updating alert statuses #108588

Conversation

jonathan-buttner commented Aug 13, 2021 • edited Loading

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

Choose a reason for hiding this comment

elasticmachine commented Aug 16, 2021

dhurley14 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathan-buttner commented Aug 17, 2021

dhurley14 commented Aug 17, 2021

jonathan-buttner commented Aug 17, 2021

dhurley14 commented Aug 17, 2021

kibanamachine commented Aug 18, 2021

💚 Build Succeeded

Metrics [docs]

API count

API count missing comments

Non-exported public API item count

History

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

MadameSheema commented Aug 19, 2021

XavierM commented Aug 19, 2021

kibanamachine commented Aug 19, 2021

💚 Backport successful

angorayc commented Aug 20, 2021 • edited Loading

jonathan-buttner commented Aug 13, 2021 •

edited

Loading

angorayc commented Aug 20, 2021 •

edited

Loading