-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Alerting] Split alerting feature privilege between rules and alerts and handle subfeature privilege specification #100127
[Alerting] Split alerting feature privilege between rules and alerts and handle subfeature privilege specification #100127
Conversation
…horization client on plugin start contract
…ing/refactor-alerts-authorization
…ing/refactor-alerts-authorization
…ing/refactor-alerts-authorization
…ing/refactor-alerts-authorization
…dd-subfeature-privilege
Pinging @elastic/uptime (Team:uptime) |
Pinging @elastic/apm-ui (Team:apm) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ML changes LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uptime changes LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! Just one question about tests, otherwise LGTM
@@ -46,8 +46,14 @@ describe('featurePrivilegeIterator', () => { | |||
read: ['read-type'], | |||
}, | |||
alerting: { | |||
all: ['alerting-all-type'], | |||
read: ['alerting-read-type'], | |||
rule: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel your pain having to update this test suite 🙏
@@ -414,6 +460,111 @@ features.registerKibanaFeature({ | |||
In the above example, note that instead of denying users with the `read` role any access to the `my-application-id.my-restricted-rule-type` type, we've decided that these users _should_ be granted `read` privileges over the _restricted_ rule type. | |||
As part of that same change, we also decided that not only should they be allowed to `read` the _restricted_ rule type, but actually, despite having `read` privileges to the feature as a whole, we do actually want to allow them to create our basic 'my-application-id.my-rule-type' rule type, as we consider it an extension of _reading_ data in our feature, rather than _writing_ it. | |||
|
|||
### Subfeature privileges |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love these docs!
'test.throw', | ||
'test.longRunning', | ||
], | ||
rule: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to add an integration test that grants access to alerting.alert
? If I'm following along correctly, all of the test fixtures grant alerting.rule
, but they don't exercise any of the alerting.alert
paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@legrego This is a little tricky because we are adding this granularity to support privileges on rules
and alerts
but right now the alerting framework is only CRUD-ing rules. We haven't hooked up any of the stack rules (that the alerting framework is responsible for) to actually write out the alerts that this feature privilege might grant access to. Getting the stack rules to write out alerts using the alert data service is something we do plan to do so maybe the functional tests would make more sense in that context when we have actual data to control access to?
@yctercero Are you adding functional tests in your RBAC PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ymao1 makes sense, I'm happy to defer this until we have actual data to control access to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - thanks for all the changes!
…ing/add-subfeature-privilege
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
infra
plugin changes LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
APM change look good!
@elasticmachine merge upstream |
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Async chunks
Unknown metric groupsReferences to deprecated APIs
History
To update your PR or re-run it, just comment with: cc @ymao1 |
…and handle subfeature privilege specification (elastic#100127) * 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 * Initial commit with changes needed for subfeature privilege * Throwing error when AlertingAuthorizationClientFactory is not defined * Renaming authorizationType to entity * Renaming AlertsAuthorization to AlertingAuthorization * Fixing unit tests * Changing schema of alerting feature privilege * Changing schema of alerting feature privilege * Updating feature privilege iterator * Updating feature privilege builder * Fixing types check * Updating privilege string terminology * Updating privilege string terminology * Wip * Fixing unit tests * Unit tests * Updating README and removing stack subfeature privilege changes * Fixing README Co-authored-by: Kibana Machine <[email protected]>
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
…and handle subfeature privilege specification (#100127) (#100817) * 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 * Initial commit with changes needed for subfeature privilege * Throwing error when AlertingAuthorizationClientFactory is not defined * Renaming authorizationType to entity * Renaming AlertsAuthorization to AlertingAuthorization * Fixing unit tests * Changing schema of alerting feature privilege * Changing schema of alerting feature privilege * Updating feature privilege iterator * Updating feature privilege builder * Fixing types check * Updating privilege string terminology * Updating privilege string terminology * Wip * Fixing unit tests * Unit tests * Updating README and removing stack subfeature privilege changes * Fixing README Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: ymao1 <[email protected]>
Resolves #99940
Summary
rule
andalert
.Note that this PR does not change the behavior of any roles. It will be up to individual producers to update their feature privilege definitions to add subfeature definition if they wish.
Checklist
Delete any items that are not applicable to this PR.