[RAC] [RBAC] Adds bulk update route to rule registry and bulk update function to alerts client #106297

dhurley14 · 2021-07-20T17:26:47Z

Summary

Adds a bulk update route (POST /internal/rac/alerts/bulk_update) to the rule registry and bulkUpdate function to the alerts as data client.

To be used here: #107141

To test...

Download apm server https://www.elastic.co/downloads/apm
Create a sample application to send data to APM

// index.js
const apm = require('elastic-apm-node').start({
serviceName: 'demoapp',
  serverUrl: 'http://localhost:8200',
  // Set the service environment
  environment: 'production'
});

const express = require('express');
const app = express();
app.get('/', (req, res) => {
  throw Error('HELLO WORLD!!');
});

app.listen(3000, () => console.log('server running on 3000'));

start elasticsearch, start kibana, start apm server (./apm-server -e) then start simple express server above
create an apm error count rule (should generate alerts in .alerts-observability-apm
execute x-pack/plugins/rule_registry/server/scripts/bulk_update_observability_alert_by_query.sh
Should resolve with a 200

Checklist

Delete any items that are not applicable to this PR.

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space.	Low	High	Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks.	High	Low	Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled.	Medium	High	Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2021-07-29T20:53:53Z

Pinging @elastic/apm-ui (Team:apm)

dhurley14 · 2021-07-29T22:55:24Z

@elasticmachine merge upstream

cauemarcondes

LGTM

packages/kbn-rule-data-utils/src/alerts_as_data_rbac.ts

x-pack/plugins/alerting/server/authorization/alerting_authorization.ts

x-pack/plugins/apm/public/components/shared/charts/helper/get_alert_annotations.test.tsx

dhurley14 · 2021-08-02T15:34:06Z

@elasticmachine merge upstream

ymao1

Alerting changes LGTM with a small suggestion.

ymao1 · 2021-08-02T17:11:49Z

x-pack/plugins/alerting/server/authorization/alerting_authorization.ts

@@ -285,7 +286,7 @@ export class AlertingAuthorization {
    if (this.authorization && this.shouldCheckAuthorization()) {
      const { username, authorizedRuleTypes } = await this.augmentRuleTypesWithAuthorization(
        this.ruleTypeRegistry.list(),
-        [ReadOperations.Find],
+        [operation == null ? ReadOperations.Find : operation],


If you are changing the name to getAuthorizationFilter (which makes sense to me), I would make the parameter operation non-optional and not default to ReadOperations.Find. Maybe maintain backwards compatibility by creating a getFindAuthorizationFilter fn that calls this fn and passes in ReadOperations.Find?

Awesome will do! Thanks for the review @ymao1

yctercero · 2021-08-02T19:06:10Z

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

@@ -52,11 +55,26 @@ export interface UpdateOptions<Params extends AlertTypeParams> {
  index: string;
 }

+export interface BulkUpdateOptions<Params extends AlertTypeParams> {
+  ids: string[] | undefined | null;
+  status: 'open' | 'closed';


I know there's been a lot of discussion around what statuses can be. Haven't followed, but might want to double check that these are indeed the terms we're going with and maybe create the type for it to be shared?

The alerts as data spreadsheet defines it as Must be one of {open, acknowledged, closed}. so I'll create an exported common type (maybe in the rule registry package where the technical terms are defined) and update that to include acknowledged

good catch!

yctercero

LGTM! Left some nits, but looks great. Thanks for all the work here!

yctercero · 2021-08-02T19:11:23Z

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

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


Is this supposed to be:

Suggested change

this.logger.error(`error in fetchAlertAuthzAlertOperateAlert ${exc}`);

this.logger.error(`error in fetchAlertAuditOperate ${exc}`);

yctercero · 2021-08-02T19:13:26Z

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

+      return;
+    }
+
+    const config: EsQueryConfig = {


Do we want to pull this out? Think we're using it in a couple places.

yctercero · 2021-08-02T19:14:35Z

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


-      return alert;
+      return alert.hits.hits[0]._source;


Should we add a to do to remind us that we have to follow up to remove us off of _source?

yctercero · 2021-08-02T19:16:59Z

x-pack/plugins/rule_registry/server/utils/create_lifecycle_executor.test.ts

@@ -128,12 +130,16 @@ describe('createLifecycleExecutor', () => {
          {
            fields: {
              [ALERT_ID]: 'TEST_ALERT_0',
+              [OWNER]: 'CONSUMER',


Should we update these to match so we no longer use OWNER and instead CONSUMER?

renaming this field should happen in another PR, but yes we do want to change OWNER to CONSUMER.

fyi: #107857

jportner

For audit logging, we have two basic types of operations, write (update, delete, create) and read (get, find).

For the saved objects client, today we have these four paths:

Read operations success path:

Check authorization
Fetch object
Add audit event success
Return result

Read operations failure path:

Check authorization
Add audit event failure
Return 403 error

Write operation success path:

Check authorization
Add audit event w/ outcome "unknown"
Create/update/delete object
Return result

Write operation failure path:

Check authorization
Add audit event w/ outcome "unknown"
Create/update/delete object
Return result

For consistency, I'd like for the alerts client to hew to these paths as much as possible.

It appears this PR is drafty/unfinished, which is fine, but it's very hard to review without unit tests in place. I went ahead and made a best effort at a first pass, but I'd like to ask that you add unit tests for the different positive/negative audit event scenarios. I would be happy to re-review at that point.

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

more WIP I think this is working now getting there.. working integration tests fixes type check errors fix jest somehow this was deleted after rebasing... fix early rejection error ids array XOR query string in bulk update route if ids, use elasticsearch bulk update method otherwise use update by query fix tests refresh true fix tests general cleanup fixes apm jest test fixes jest tests code cleanup and fixed jest tests WIP - trying to fix integration tests after refactor fix tests, throw appropriate HTTP errors in alerts client fix issues after rebasing with master updates alerts client docs general cleanup fix trial integration tests possibly fixes loader issue in package updates getSafeSortIds param type to include 'null' in param type, renames getFindAuthorizationFilter -> getAuthorizationFilter and renames usage elsewhere, utilizes const for test in apm make 'operation' parameter non-optional in getAuthorizationFilter and retain backwards compatibility by pointing the getFindAuthorizationFilter to getAuthorizationFilter and operation = ReadOperations.Find adds an exported type to be used when setting a status on an alert moves es query config out of client definition and into a function to allow for customized configurations update by query should check for siem signals status field as well return 200 when no alerts are found, not a 401 error, updates integration tests with stricter expects updates alerts clients docs fixing merge with master conflicts adds missing required space ids field to mocked alerts to fix failing apm jest tests implements suggested changes with auditing failures in bulk update by ids function, removes audit logging in catch blocks, cleans up other jest tests execute ensure authorized in the search after loop, adds jest tests to make sure we log an error to the audit log for the case when ensureAuthorized throws an authorization error

…tespace char to inline snapshots

...lib/detection_engine/rule_execution_log/rule_registry_log_client/rule_execution_field_map.ts

jportner · 2021-08-09T14:07:56Z

x-pack/plugins/rule_registry/server/alert_data_client/tests/bulk_update.test.ts

+        expect(auditLogger.log.mock.calls).toHaveLength(2);
+        expect(auditLogger.log.mock.calls[0][0]).toEqual({
+          message: `Failed attempt to update alert [id=${unsuccessfulAuthzHit}]`,
+          event: {
+            action: 'alert_update',
+            category: ['database'],
+            outcome: 'failure',
+            type: ['change'],
+          },
+          error: {
+            code: 'Error',
+            message: 'Unauthorized for fake.rule and apm',
+          },
+        });
+        expect(auditLogger.log.mock.calls[1][0]).toEqual({
+          message: `Failed attempt to update alert [id=${successfulAuthzHit}]`,
+          event: {
+            action: 'alert_update',
+            category: ['database'],
+            outcome: 'failure',
+            type: ['change'],
+          },
+          error: {
+            code: 'Error',
+            message: 'Unauthorized for fake.rule and apm',
+          },
+        });


Optional nit: Jest provides specific functions to deal with these situations, e.g.,

expect(auditLogger.log).toHaveBeenCalledTimes(2); expect(auditLogger.log).toHaveBeenNthCalledWith( 1, {...} ); expect(auditLogger.log).toHaveBeenNthCalledWith( 2, {...} );

Thanks! Fixed here: ba1857e

jportner · 2021-08-09T14:09:37Z

x-pack/plugins/rule_registry/server/alert_data_client/tests/get.test.ts


    expect(auditLogger.log).toHaveBeenCalledWith({
      error: undefined,
-      event: { action: 'alert_get', category: ['database'], outcome: 'success', type: ['access'] },
-      message: 'User has accessed alert [id=1]',
+      event: { action: 'alert_get', category: ['database'], outcome: 'unknown', type: ['access'] },


get should never have outcome: 'unknown'; we know the outcome (we fetched the object and returned it to the user, this is a 'success' outcome).

Ah! Yes thank you. Good catch.

Fixed here: 36838ab

jportner · 2021-08-09T14:12:30Z

x-pack/plugins/rule_registry/server/alert_data_client/tests/get.test.ts

-      event: { action: 'alert_get', category: ['database'], outcome: 'success', type: ['access'] },
-      message: 'User has accessed alert [id=1]',
+      event: { action: 'alert_get', category: ['database'], outcome: 'unknown', type: ['access'] },
+      message: 'User is accessing alert [id=NoxgpHkBqbdrfX07MqXV]',


You have a test case for a get() that results in a successful audit event; what about an audit error when the user is unauthorized?

Yes I think I actually removed the test case because with the get we were relying on a search executed with the authorization filter from the alerting framework to only give us back the data we were authorized to view, so there would be no authorization failure because we never attempted to access it in the first place.

I changed this so we are not solely rely on the authorization filter. We also call into ensureAuthorized on each search result (+ utilizing the authorization filter) so adding in an audit failure test makes sense again.

Fixed here: 36838ab

jportner

Sorry I think I fat-fingered this and didn't submit my entire review at once!
I think a couple minor things need to be changed and hopefully a few more test cases added but otherwise this is looking great. Thanks for all of the changes so far!

jportner · 2021-08-09T14:14:38Z

x-pack/plugins/rule_registry/server/alert_data_client/tests/update.test.ts

@@ -181,33 +187,26 @@ describe('update()', () => {
        outcome: 'unknown',
        type: ['change'],
      },
-      message: 'User is updating alert [id=1]',
+      message: 'User is updating alert [id=NoxgpHkBqbdrfX07MqXV]',


You have a test case for an update() that results in a successful audit event; what about an audit error when the user is unauthorized?

Yup very true. Added an audit failure test here: 36838ab

jportner · 2021-08-09T14:20:30Z

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

+          alertAuditEvent({
+            action: operationAlertAuditActionMap[operation],
+            id: item._id,
+            outcome: 'unknown',


As mentioned in my other comment on the unit test: the outcome should only be 'unknown' if the action is an update.
If outcome is omitted and error is not present, the audit event is written as a a successful event (outcome: 'success')

Suggested change

outcome: 'unknown',

...(operation === WriteOperations.Update ? { outcome: 'unknown' } : {}),

Fixed here: 36838ab

jportner · 2021-08-09T14:21:14Z

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

+          alertAuditEvent({
+            action: operationAlertAuditActionMap[operation],
+            id,
+            ...(operation === WriteOperations.Update ? { outcome: 'unknown' } : { operation }),


You don't need to pass operation in if this is not a write operation:

Suggested change

...(operation === WriteOperations.Update ? { outcome: 'unknown' } : { operation }),

...(operation === WriteOperations.Update ? { outcome: 'unknown' } : {}),

Cleaned this up here: 36838ab

…xepcted values

…on, adds jest tests for when user is unauthorized to access given alert

kibanamachine · 2021-08-09T18:32:28Z

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`observability`	511.2KB	511.8KB	+617.0B
`securitySolution`	6.5MB	6.5MB	+617.0B
`timelines`	273.1KB	273.7KB	+617.0B
total			+1.8KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`infra`	149.5KB	150.1KB	+617.0B
`uptime`	35.1KB	35.7KB	+617.0B
total			+1.2KB

Unknown metric groups

API count

id	before	after	diff
`alerting`	243	247	+4

API count missing comments

id	before	after	diff
`alerting`	235	239	+4

References to deprecated APIs

id	before	after	diff
`alerting`	23	26	+3

History

💔 Build #143661 failed 8dd966f
💚 Build #143363 succeeded 77f9413
💔 Build #143336 failed f5cc9a0
💚 Build #143245 succeeded a5a267d
💔 Build #143239 failed 4dc9a29

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

cc @dhurley14

jportner

Great stuff, thanks again for being so responsive to feedback!

…function to alerts client (elastic#106297) Adds a bulk update route (POST /internal/rac/alerts/bulk_update) to the rule registry and bulkUpdate function to the alerts as data client.

kibanamachine · 2021-08-09T19:42:45Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

…function to alerts client (#106297) (#107965) Adds a bulk update route (POST /internal/rac/alerts/bulk_update) to the rule registry and bulkUpdate function to the alerts as data client. Co-authored-by: Devin W. Hurley <devin.hurley@elastic.co>

dhurley14 force-pushed the bulk_update_rbac_alerts branch from 0a42280 to e08eb76 Compare July 20, 2021 22:02

peluja1012 mentioned this pull request Jul 20, 2021

[RAC] [Alerts] [RBAC] #95721

Closed

13 tasks

dhurley14 force-pushed the bulk_update_rbac_alerts branch 2 times, most recently from 2e993d2 to 8860188 Compare July 29, 2021 19:11

dhurley14 changed the title ~~wip undo me~~ [RAC] [RBAC] Adds bulk update route to rule registry and bulk update function to alerts client Jul 29, 2021

dhurley14 self-assigned this Jul 29, 2021

dhurley14 added 1 Feature:RAC label obsolete release_note:enhancement Theme: rac label obsolete v7.15.0 v8.0.0 and removed 1 labels Jul 29, 2021

dhurley14 marked this pull request as ready for review July 29, 2021 20:48

dhurley14 requested review from a team as code owners July 29, 2021 20:48

botelastic bot added the Team:APM All issues that need APM UI Team support label Jul 29, 2021

dhurley14 force-pushed the bulk_update_rbac_alerts branch from 96d3749 to 02b2f98 Compare July 30, 2021 15:13

cauemarcondes approved these changes Jul 30, 2021

View reviewed changes

yctercero reviewed Jul 30, 2021

View reviewed changes

packages/kbn-rule-data-utils/src/alerts_as_data_rbac.ts Show resolved Hide resolved

yctercero reviewed Jul 30, 2021

View reviewed changes

x-pack/plugins/alerting/server/authorization/alerting_authorization.ts Outdated Show resolved Hide resolved

yctercero reviewed Jul 30, 2021

View reviewed changes

x-pack/plugins/apm/public/components/shared/charts/helper/get_alert_annotations.test.tsx Outdated Show resolved Hide resolved

dhurley14 requested a review from a team as a code owner July 30, 2021 21:30

kqualters-elastic approved these changes Jul 30, 2021

View reviewed changes

ymao1 approved these changes Aug 2, 2021

View reviewed changes

yctercero reviewed Aug 2, 2021

View reviewed changes

yctercero approved these changes Aug 2, 2021

View reviewed changes

dhurley14 enabled auto-merge (squash) August 3, 2021 15:50

dhurley14 disabled auto-merge August 3, 2021 16:07

jportner self-requested a review August 3, 2021 18:56

jportner reviewed Aug 3, 2021

View reviewed changes

dhurley14 force-pushed the bulk_update_rbac_alerts branch from 4036ac2 to 774c3ca Compare August 3, 2021 22:50

dhurley14 requested a review from a team as a code owner August 3, 2021 22:50

dhurley14 force-pushed the bulk_update_rbac_alerts branch 2 times, most recently from db2453b to 4dc9a29 Compare August 5, 2021 13:44

dhurley14 added 6 commits August 9, 2021 08:22

removes duplicate import after rebase with master

60e35bb

adds more jest tests

ce41d56

rename private functions in alerts as data client and add missing whi…

c3ad00f

…tespace char to inline snapshots

fixes typo from renaming

d4e8164

update alerts as data client docs

8dd966f

dhurley14 force-pushed the bulk_update_rbac_alerts branch from 77f9413 to 8dd966f Compare August 9, 2021 12:23

michaelolo24 reviewed Aug 9, 2021

View reviewed changes

...lib/detection_engine/rule_execution_log/rule_registry_log_client/rule_execution_field_map.ts Show resolved Hide resolved

jportner reviewed Aug 9, 2021

View reviewed changes

dhurley14 added 3 commits August 9, 2021 10:52

use jest-specific functions instead of reaching into mock calls for e…

ba1857e

…xepcted values

only writes an outcome of 'success' if operation is an access operati…

36838ab

…on, adds jest tests for when user is unauthorized to access given alert

replaces toHaveBeenLastCalledWith with toHaveBeenNthCalledWith

04044f5

jportner approved these changes Aug 9, 2021

View reviewed changes

dhurley14 merged commit ab43afa into elastic:master Aug 9, 2021

kibanamachine mentioned this pull request Aug 9, 2021

[7.x] [RAC] [RBAC] Adds bulk update route to rule registry and bulk update function to alerts client (#106297) #107965

Merged

banderror mentioned this pull request Aug 13, 2021

[RAC][Rule Registry] Improve RuleDataService API and index bootstrapping implementation #108115

Merged

17 tasks

	this.logger.error(`error in fetchAlertAuthzAlertOperateAlert ${exc}`);
	this.logger.error(`error in fetchAlertAuditOperate ${exc}`);

	outcome: 'unknown',
	...(operation === WriteOperations.Update ? { outcome: 'unknown' } : {}),

[RAC] [RBAC] Adds bulk update route to rule registry and bulk update function to alerts client #106297

[RAC] [RBAC] Adds bulk update route to rule registry and bulk update function to alerts client #106297

Conversation

dhurley14 commented Jul 20, 2021 • edited

Summary

Checklist

Risk Matrix

For maintainers

elasticmachine commented Jul 29, 2021

dhurley14 commented Jul 29, 2021

cauemarcondes left a comment

Choose a reason for hiding this comment

dhurley14 commented Aug 2, 2021

ymao1 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

yctercero 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

jportner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jportner Aug 9, 2021 • edited

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

jportner 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

kibanamachine commented Aug 9, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

API count

API count missing comments

References to deprecated APIs

History

jportner left a comment

Choose a reason for hiding this comment

kibanamachine commented Aug 9, 2021

💚 Backport successful

dhurley14 commented Jul 20, 2021 •

edited

jportner Aug 9, 2021 •

edited