[Alerting] Refactor alerts authorization client by ymao1 · Pull Request #99078 · elastic/kibana

ymao1 · 2021-05-03T17:19:22Z

Resolves #98818

Summary

Refactoring alerts authorization client to be more generic. This uses the existing alerting feature privilege model to provide all and read privileges to rules and alerts. There will be a followup issue to handle subfeature privilege specification. There should be no change to how the rules RBAC works functionally.

Updated alerting feature privilege builder to include entity in the feature privilege string. Allowed entities are rule and alert
Added ability to specify exemptConsumers in authorization client to handle the alerting use case where alerts was a special consumer id with special handling
Updated terminology within AlertsAuthorization class to reflect updated rule terminology
Updated authorization filter builder to return EsQuery or KQL
Updated authorization filter to allow field names to be specified
Exposing getAlertingAuthorizationWithRequest through alerting plugin start to be used by other plugins
Renamed AlertsAuthorization to AlertingAuthorization
Changes formatting of recorded audit events
- Audit event types have changed from using alerts_ prefix to alerting_ prefix:
  - alerts_authorization_failure is now alerting_authorization_failure
  - alerts_unscoped_authorization_failure is now alerting_unscoped_authorization_failure
  - alerts_authorization_success is now alerting_authorization_success
- Audit events have been updated to reflect new alerting terminology
  - alerts have been renamed to rules, for example the audit event user Authorized to create a ".index-threshold" alert by "app" will now read user Authorized to create a ".index-threshold" rule by "app"
  - alert instances have been renamed to alert, for example audit events for muteInstance and unmuteInstance operations will now reference muteAlert and unmuteAlert

To Verify

Verify that existing roles that give different levels of access to rules for different features still work with this PR
Verify that new roles created in this PR branch work as expected in terms of giving different levels of access to rules for different features.

Checklist

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

…horization client on plugin start contract

…ing types

…g builder

…ing/refactor-alerts-authorization

…gger

…sl option

x-pack/plugins/security/server/authorization/privileges/feature_privilege_builder/alerting.ts

x-pack/plugins/alerting/server/alerts_client/alerts_client.ts

x-pack/plugins/alerting/server/alerts_client_factory.ts

…ing/refactor-alerts-authorization

gmmorris · 2021-05-05T10:24:59Z

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

+    authorizationType: string
  ): string {
-    return `${authorizationResult} to ${operation} a "${alertTypeId}" alert ${
+    return `${authorizationResult} to ${operation} a "${alertTypeId}" ${authorizationType} ${


One thought that crosses my mind - once customers upgrade to 7.14 their audit logs will say rule where they used to say alert, and alert on the new Alert entities.
This means that if a user looks at their audit logs across versions they might have two audit log rows that say Alert, but in one its a Rule and in the other an Alert.

I'm not sure what we can do about that, but perhaps it's worth making it easier to distinguish between them by changing the format in 7.14 so that the sentences at least appear different in some manner? 🤔

x-pack/plugins/security/server/authorization/actions/alerting.ts

gmmorris · 2021-05-05T11:11:42Z

This looks like the right direction to me 👍
I haven't done a full review and leaving that to the team, but I think this approach fits in with the ideal endstate of having the Rules and Alerts share the core semantics, while leaving room for change later where they diverge on specific operations.

This sparks joy 🎉 😉

…ing/refactor-alerts-authorization

yctercero

LGTM! I integrated this PR into the work Devin and I have been doing for RAC. There are some open questions about possible changes needed to the feature privilege builder, but I think those can be addressed as follow up to keep things flowing 🌊

Thanks so much for all these changes and the quick turnaround!

ymao1 · 2021-05-13T15:29:36Z

@elasticmachine merge upstream

YulNaumenko

Changes LGTM!

legrego · 2021-05-13T15:23:32Z

x-pack/plugins/security/server/authorization/privileges/feature_privilege_builder/alerting.ts

+}
+
+const readOperations: Record<AlertingType, string[]> = {
+  rule: ['get', 'getAlertState', 'getAlertInstanceSummary', 'find'],


This is probably a lack of understanding on my part -- why does the rule alerting type have operations called getAlertState and getAlertInstanceSummary? Are these names holdovers from the old terminology, or are they actually operations against the alert alerting type?

Yes, these are holdovers from the old terminology and you are right, they are confusing. I will open an issue to update these separately.

I guess we can change these as part of this PR if we want, as it's under the hood and shouldn't break anything if we change it in 7.14, right? 🤔

Updated in this commit: ce70ff2

x-pack/plugins/security/server/authorization/actions/alerting.ts

legrego · 2021-05-13T15:27:58Z

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

  Authorized = 'Authorized',
 }

-export class AlertsAuthorizationAuditLogger {


Since we are changing the recorded audit events, we should document this as a breaking change for the release notes

@legrego I have added the release_note:breaking label and added a line in the PR description about the change to audit events.

Thanks, @ymao1! I think it would be beneficial to our users if we also gave some examples of the changes made to the audit log. For example, mentioning that the audit event types have changed (alerts_authorization_failure -> alerting_authorization_failure, etc.)

@legrego PR description updated!

…ing/refactor-alerts-authorization

chrisronline

LGTM! I did some basic testing around creating rules on master with various permission sets and ensuring they still work properly on the PR and it looks great to me!

ymao1 · 2021-05-18T20:38:40Z

@elasticmachine merge upstream

kibanamachine · 2021-05-18T22:25:15Z

💚 Build Succeeded

Metrics [docs]

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
`alerting`	201	203	+2

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
`alerting`	9	10	+1

Unknown metric groups

API count

id	before	after	diff
`alerting`	201	203	+2

History

💚 Build #126256 succeeded 918bc00
💔 Build #126208 failed ce70ff2
💛 Build #125807 was flaky 3bafd6d
💚 Build #125648 succeeded a04dc1e
💔 Build #125619 failed 334030b

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

cc @ymao1

* WIP - creating alerting authorization client factory and exposing authorization client on plugin start contract * Updating alerting feature privilege builder to handle different alerting types * Passing in alerting authorization type to AlertingActions class string builder * Passing in authorization type in each function call * Passing in exempt consumer ids. Adding authorization type to audit logger * Changing alertType to ruleType * Changing alertType to ruleType * Updating unit tests * Updating unit tests * Passing field names into authorization query builder. Adding kql/es dsl option * Converting to es query if requested * Fixing functional tests * Removing ability to specify feature privilege name in constructor * Fixing some types and tests * Consolidating alerting authorization kuery filter options * Cleanup and tests * Cleanup and tests * Throwing error when AlertingAuthorizationClientFactory is not defined * Renaming authorizationType to entity * Renaming AlertsAuthorization to AlertingAuthorization * Fixing unit tests * Updating privilege string terminology * Updating privilege string terminology Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine · 2021-05-18T22:36:23Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

* WIP - creating alerting authorization client factory and exposing authorization client on plugin start contract * Updating alerting feature privilege builder to handle different alerting types * Passing in alerting authorization type to AlertingActions class string builder * Passing in authorization type in each function call * Passing in exempt consumer ids. Adding authorization type to audit logger * Changing alertType to ruleType * Changing alertType to ruleType * Updating unit tests * Updating unit tests * Passing field names into authorization query builder. Adding kql/es dsl option * Converting to es query if requested * Fixing functional tests * Removing ability to specify feature privilege name in constructor * Fixing some types and tests * Consolidating alerting authorization kuery filter options * Cleanup and tests * Cleanup and tests * Throwing error when AlertingAuthorizationClientFactory is not defined * Renaming authorizationType to entity * Renaming AlertsAuthorization to AlertingAuthorization * Fixing unit tests * Updating privilege string terminology * Updating privilege string terminology Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: ymao1 <ying.mao@elastic.co>

* WIP - creating alerting authorization client factory and exposing authorization client on plugin start contract * Updating alerting feature privilege builder to handle different alerting types * Passing in alerting authorization type to AlertingActions class string builder * Passing in authorization type in each function call * Passing in exempt consumer ids. Adding authorization type to audit logger * Changing alertType to ruleType * Changing alertType to ruleType * Updating unit tests * Updating unit tests * Passing field names into authorization query builder. Adding kql/es dsl option * Converting to es query if requested * Fixing functional tests * Removing ability to specify feature privilege name in constructor * Fixing some types and tests * Consolidating alerting authorization kuery filter options * Cleanup and tests * Cleanup and tests * Throwing error when AlertingAuthorizationClientFactory is not defined * Renaming authorizationType to entity * Renaming AlertsAuthorization to AlertingAuthorization * Fixing unit tests * Updating privilege string terminology * Updating privilege string terminology Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

ymao1 added 12 commits April 30, 2021 09:21

WIP - creating alerting authorization client factory and exposing aut…

1170c6e

…horization client on plugin start contract

Updating alerting feature privilege builder to handle different alert…

5a1c2c8

…ing types

Passing in alerting authorization type to AlertingActions class strin…

cbde0cc

…g builder

Merge branch 'master' of https://github.com/elastic/kibana into alert…

a2ffb8d

…ing/refactor-alerts-authorization

Passing in authorization type in each function call

fd1a28b

Passing in exempt consumer ids. Adding authorization type to audit lo…

a97e064

…gger

Changing alertType to ruleType

b9ee8ac

Changing alertType to ruleType

cd6185f

Updating unit tests

6daa470

Updating unit tests

be00344

Passing field names into authorization query builder. Adding kql/es d…

721576a

…sl option

Converting to es query if requested

e1cd872

ymao1 commented May 3, 2021

View reviewed changes

x-pack/plugins/security/server/authorization/privileges/feature_privilege_builder/alerting.ts Show resolved Hide resolved

yctercero reviewed May 3, 2021

View reviewed changes

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

yctercero reviewed May 3, 2021

View reviewed changes

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

Fixing functional tests

2007a56

ymao1 requested a review from gmmorris May 3, 2021 19:56

ymao1 added 6 commits May 3, 2021 16:10

Removing ability to specify feature privilege name in constructor

857b0de

Merge branch 'master' of https://github.com/elastic/kibana into alert…

56e876c

…ing/refactor-alerts-authorization

Fixing some types and tests

7ef5bcd

Consolidating alerting authorization kuery filter options

8359289

Cleanup and tests

b5a49cd

Cleanup and tests

0fbaf23

gmmorris reviewed May 5, 2021

View reviewed changes

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

Merge branch 'master' of https://github.com/elastic/kibana into alert…

1792895

…ing/refactor-alerts-authorization

yctercero approved these changes May 11, 2021

View reviewed changes

ymao1 added 2 commits May 12, 2021 10:27

Throwing error when AlertingAuthorizationClientFactory is not defined

ee3f226

Renaming authorizationType to entity

bfbfae4

ymao1 requested a review from legrego May 13, 2021 14:08

Merge branch 'master' into alerting/refactor-alerts-authorization

3bafd6d

YulNaumenko approved these changes May 14, 2021

View reviewed changes

legrego reviewed May 14, 2021

View reviewed changes

ymao1 added release_note:breaking and removed release_note:skip Skip the PR/issue when compiling release notes labels May 14, 2021

ymao1 requested a review from legrego May 14, 2021 20:31

ymao1 added 3 commits May 17, 2021 07:48

Merge branch 'master' of https://github.com/elastic/kibana into alert…

91a98ea

…ing/refactor-alerts-authorization

Updating privilege string terminology

ce70ff2

Updating privilege string terminology

918bc00

legrego approved these changes May 18, 2021

View reviewed changes

chrisronline approved these changes May 18, 2021

View reviewed changes

Merge branch 'master' into alerting/refactor-alerts-authorization

ad4d1ef

ymao1 added the auto-backport Deprecated - use backport:version if exact versions are needed label May 18, 2021

ymao1 merged commit 0f0cee2 into elastic:master May 18, 2021

kibanamachine mentioned this pull request May 18, 2021

[7.x] [Alerting] Refactor alerts authorization client (#99078) #100307

Merged

ymao1 mentioned this pull request May 24, 2021

[Alerting] Update README #100478

Merged

yctercero mentioned this pull request May 25, 2021

[RAC][RBAC] - Alerts as data RBAC MVP #100527

Closed

4 tasks

yctercero mentioned this pull request May 27, 2021

[RAC] [RBAC] MVP RBAC for alerts as data #100705

Merged

10 tasks

peluja1012 mentioned this pull request Jul 20, 2021

[RAC] [Alerts] [RBAC] #95721

Closed

13 tasks

mikecote mentioned this pull request Jul 29, 2021

[DOCS] Adds the 7.14.0 release notes #107058

Merged

This was referenced Aug 11, 2021

[Alerting] Alerting authorization should always exempt alerts consumer #108206

Closed

[Alerting] Alerting authorization should always exempt alerts consumer #108220

Merged

Conversation

ymao1 commented May 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

To Verify

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gmmorris commented May 5, 2021

Uh oh!

yctercero left a comment

Choose a reason for hiding this comment

Uh oh!

ymao1 commented May 13, 2021

Uh oh!

YulNaumenko left a comment

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!

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!

chrisronline left a comment

Choose a reason for hiding this comment

Uh oh!

ymao1 commented May 18, 2021

Uh oh!

kibanamachine commented May 18, 2021

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Public APIs missing exports

API count

History

Uh oh!

kibanamachine commented May 18, 2021

💚 Backport successful

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

ymao1 commented May 3, 2021 •

edited

Loading