[Cases] Update cases ids in the alerts schema when attaching an alert to a case #147985

cnasikas · 2022-12-22T10:25:34Z

Summary

PR #147013 extended the alert's schema to be able to store the case id an alert is attached to. This PR adds the ability to update the case_ids field of the alert when an alert is attached to a case. It also limits the number of cases an alert can be attached to ten.

Permissions

A user to attach an alert to a case needs a) to have read access to the solution via the kibana feature privileges and b) to have write access to the case. The user that did the request should not need to have written access to the alert for Cases to add the case ID to the alert's data. For that reason, the alert data client is extended to cover this particular need: update an alert even if the user has read access.

Alerts client aside

For future reference, the alerts client uses the request to check the kibana feature authorization but uses the internal system user to perform any write and get operations on the alert indices themselves. For security solution this means that a user with only read access to the security solution kibana feature, write access to cases, and no read or write access to the alert indices could attach an alert to a case and have the case id stored in the alert.

For observability, users intentionally do not have access to the alert indices so we want to bypass the authorization check on the indices which is why the current alerts client uses an es client that is an internal system user.

Related issue: #146864

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

Release notes

Attaching an alert to a case requires read permissions on the alert.

cnasikas · 2023-01-12T09:29:40Z

x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts

+    indexName: string;
+    operation: ReadOperations.Find | ReadOperations.Get | WriteOperations.Update;
+  }) {
+    return this.mgetAlertsAuditOperate({


I make mgetAlertsAuditOperate generic to accept any field and I created the mgetAlertsAuditOperateStatus for the status

cnasikas · 2023-01-12T09:30:46Z

x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts

+      ids,
+      indexName,
+      operation,
+      fieldToUpdate: (source) => this.getAlertStatusFieldUpdate(source, status),


Same logic as before. I moved it out from mgetAlertsAuditOperate. The consumer of mgetAlertsAuditOperate should define how to get the field based on the source.

cnasikas · 2023-01-12T09:35:27Z

x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts

+      indexName: index,
+      operation: ReadOperations.Get,
+      fieldToUpdate: (source) => this.getAlertCaseIdsFieldUpdate(source, caseIds),
+      validate: (source) => this.validateTotalCasesPerAlert(source, caseIds),


The mgetAlertsAuditOperate is extended to do validation per alert.

elasticmachine · 2023-01-12T11:27:49Z

Pinging @elastic/response-ops (Team:ResponseOps)

elasticmachine · 2023-01-12T11:27:50Z

Pinging @elastic/response-ops-cases (Feature:Cases)

ymao1

Code review of the alerts client in the rule registry changes only, which LGTM. Would be nice to have unit tests for the new function but I see there are no existing unit tests for the alerts client :( and that functionality is covered by the integration test. Will let the cases team review the overall PR :)

x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts

jonathan-buttner

Left a few optimization suggestions

x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts

x-pack/plugins/cases/server/common/models/case_with_comments.ts

CoenWarmer

LGTM!

jonathan-buttner · 2023-02-08T16:39:22Z

x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts

+
+      return mgetRes;
+    } catch (exc) {
+      this.logger.error(`error in ensureAlertsAuthorized ${exc}`);


nit: error in ensureAllAlertsAuthorized ${exc}

jonathan-buttner · 2023-02-08T17:17:18Z

x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/post_comment.ts

+              user: superUser,
+              space: 'space1',
+            },
+            attachmentAuth: { user: secOnlyReadAlerts, space: 'space1' },


I think the reason that this user fails to when they don't have index privileges is because we're trying to update the status of the alert in this test:

Failed to update alert status ids: [{"id":"155bf8908ab2dee36b72f6e304fcd7de3de53104b3f8fb351f4e0463a704e63d","index":".internal.alerts-security.alerts-default-000001","status":"in-progress"}]: ResponseError: security_exception

jonathan-buttner · 2023-02-08T17:18:40Z

x-pack/test/cases_api_integration/common/lib/authentication/roles.ts

@@ -190,6 +190,29 @@ export const securitySolutionOnlyRead: Role = {
  },
 };

+export const securitySolutionOnlyReadAlerts: Role = {


How about we create another user that does not have access to the elasticsearch indices and ensure that the user cannot update the status of an alert. We may already have a test that does that I can't remember.

Either way I think it'd be good to test adding the case ID to an alert when the security solution user does not have access to the elasticsearch index.

x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/post_comment.ts

jonathan-buttner · 2023-02-08T17:24:41Z

...t/cases_api_integration/security_and_spaces/tests/common/internal/bulk_create_attachments.ts

+          });
+        });
+
+        it('should change the status of the alert when the user has read access only', async () => {


I think this test should fail if the user only has read access to the elasticsearch index. Or we can keep it but change the test name to say something like: when the user has write access to the indices and only read access to the siem solution

marshallmain

Detections and response changes LGTM

jonathan-buttner

Thanks for the changes!

kibana-ci · 2023-02-11T10:11:31Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 5e11b24

Failed CI Steps

Test Failures

[job] [logs] FTR Configs #24 / cases security and spaces enabled: trial push_case memoryless server alerts should change the status of all alerts attached to a case to closed when closure_type: close-by-pushing and syncAlerts: true
[job] [logs] FTR Configs #51 / console app console app with comments with single line comments should allow in request body, using #

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`apm`	1386	1387	+1
`cases`	595	596	+1
`infra`	1250	1251	+1
`observability`	514	515	+1
`securitySolution`	3720	3721	+1
`synthetics`	1354	1355	+1
`triggersActionsUi`	510	511	+1
total			+7

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/rule-data-utils`	97	98	+1
`ruleRegistry`	224	228	+4
total			+5

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`ruleRegistry`	12	13	+1

Unknown metric groups

API count

id	before	after	diff
`@kbn/rule-data-utils`	100	101	+1
`ruleRegistry`	252	257	+5
total			+6

History

💚 Build #107301 succeeded ae05f46
💔 Build #106404 failed 6e1f3cf
💔 Build #106381 failed aafdf84
💚 Build #100619 succeeded bd6c48d
💔 Build #100609 failed 065558faae89fc2a4dd25363cade2ba43c655acb
💔 Build #100523 failed 5f48d8d

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

cc @cnasikas

cnasikas added release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Cases Cases feature v8.7.0 labels Dec 22, 2022

cnasikas self-assigned this Dec 22, 2022

cnasikas mentioned this pull request Dec 22, 2022

[Cases] Link cases to alerts #146864

Closed

8 tasks

cnasikas force-pushed the link_alerts_to_case branch 2 times, most recently from 1746c00 to 227d858 Compare December 23, 2022 13:19

cnasikas added 7 commits January 11, 2023 11:58

Extend alerts client to update case ids

5f5ef30

Pass the alertsClient to the cases client

799d296

Update alerts schema when attaching alerts to a case

235c5e5

Check total cases limit per alert

c466e46

Add integration checks

9796e97

Fix types

340b86f

Add o11y tests

f9205c0

cnasikas force-pushed the link_alerts_to_case branch from 227d858 to f9205c0 Compare January 11, 2023 16:10

Add rbac tests

3ffe519

cnasikas commented Jan 12, 2023

View reviewed changes

cnasikas added 2 commits January 12, 2023 11:37

Address TODOs

f99aad0

Integration tests: check for uniqueness

994be50

cnasikas marked this pull request as ready for review January 12, 2023 11:27

cnasikas requested review from a team as code owners January 12, 2023 11:27

ymao1 approved these changes Jan 12, 2023

View reviewed changes

x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts Show resolved Hide resolved

x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts Show resolved Hide resolved

jonathan-buttner reviewed Jan 12, 2023

View reviewed changes

cnasikas added 4 commits January 16, 2023 17:09

Change schema to do one mget call

00f880c

Move updating case ids to the alerts service

82c69bc

Filter out empty alets

5f48d8d

Do not call the alerts client if there are no alerts to update

bd6c48d

cnasikas force-pushed the link_alerts_to_case branch from 065558f to bd6c48d Compare January 17, 2023 08:51

CoenWarmer approved these changes Jan 17, 2023

View reviewed changes

cnasikas added v8.8.0 and removed v8.7.0 labels Feb 7, 2023

cnasikas added 5 commits February 7, 2023 16:38

Merge branch 'main' into link_alerts_to_case

acd4186

Add ensureAllAlertsAuthorizedRead to the alerts client

8419345

Authorize alerts when attached to a case

aafdf84

More tests

a9e5562

Merge branch 'main' into link_alerts_to_case

6e1f3cf

cnasikas requested a review from jonathan-buttner February 8, 2023 12:49

jonathan-buttner reviewed Feb 8, 2023

View reviewed changes

cnasikas added 2 commits February 9, 2023 16:27

Merge branch 'main' into link_alerts_to_case

b41bd88

Fix conflicts

7e5e8c0

marshallmain approved these changes Feb 10, 2023

View reviewed changes

PR feedback

ae05f46

jonathan-buttner approved these changes Feb 10, 2023

View reviewed changes

Merge branch 'main' into link_alerts_to_case

5e11b24

cnasikas enabled auto-merge (squash) February 11, 2023 08:32

cnasikas merged commit 1f19c9e into elastic:main Feb 11, 2023

cnasikas deleted the link_alerts_to_case branch February 11, 2023 10:11

kibanamachine added the backport:skip This commit does not require backporting label Feb 11, 2023

cnasikas added release_note:breaking and removed release_note:skip Skip the PR/issue when compiling release notes labels Apr 3, 2023

nastasha-solomon mentioned this pull request Apr 30, 2023

Changes to Security cases in 8.8 elastic/security-docs#3171

Closed

19 tasks

nastasha-solomon mentioned this pull request May 9, 2023

8.8.0 Release notes elastic/security-docs#3139

Closed

18 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Cases] Update cases ids in the alerts schema when attaching an alert to a case #147985

[Cases] Update cases ids in the alerts schema when attaching an alert to a case #147985

cnasikas commented Dec 22, 2022 •

edited

Loading

cnasikas Jan 12, 2023

cnasikas Jan 12, 2023

cnasikas Jan 12, 2023

elasticmachine commented Jan 12, 2023

elasticmachine commented Jan 12, 2023

ymao1 left a comment

jonathan-buttner left a comment

CoenWarmer left a comment

jonathan-buttner Feb 8, 2023

jonathan-buttner Feb 8, 2023

jonathan-buttner Feb 8, 2023

jonathan-buttner Feb 8, 2023

marshallmain left a comment

jonathan-buttner left a comment

kibana-ci commented Feb 11, 2023

API count

[Cases] Update cases ids in the alerts schema when attaching an alert to a case #147985

[Cases] Update cases ids in the alerts schema when attaching an alert to a case #147985

Conversation

cnasikas commented Dec 22, 2022 • edited Loading

Summary

Permissions

Alerts client aside

Checklist

For maintainers

Release notes

cnasikas Jan 12, 2023

Choose a reason for hiding this comment

cnasikas Jan 12, 2023

Choose a reason for hiding this comment

cnasikas Jan 12, 2023

Choose a reason for hiding this comment

elasticmachine commented Jan 12, 2023

elasticmachine commented Jan 12, 2023

ymao1 left a comment

Choose a reason for hiding this comment

jonathan-buttner left a comment

Choose a reason for hiding this comment

CoenWarmer left a comment

Choose a reason for hiding this comment

jonathan-buttner Feb 8, 2023

Choose a reason for hiding this comment

jonathan-buttner Feb 8, 2023

Choose a reason for hiding this comment

jonathan-buttner Feb 8, 2023

Choose a reason for hiding this comment

jonathan-buttner Feb 8, 2023

Choose a reason for hiding this comment

marshallmain left a comment

Choose a reason for hiding this comment

jonathan-buttner left a comment

Choose a reason for hiding this comment

kibana-ci commented Feb 11, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Public APIs missing comments

Public APIs missing exports

API count

History

cnasikas commented Dec 22, 2022 •

edited

Loading