Skip to content
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] Alerting authorization should always exempt alerts consumer #108220

Merged

Conversation

ymao1
Copy link
Contributor

@ymao1 ymao1 commented Aug 11, 2021

Resolves #108206

Summary

Reverts some the changes from this PR, specifically the changes to the alerting authorization client.

Previously the alerting authorization client always special-cased the alerts consumer when checking authorization, so that rules created in the Stack Rules Management UI would only be subject to producer based RBAC. I assumed this exemption was something only needed by the alerting framework, which is true for the way rules are used now, but not true for alerts derived from rules, so during the refactor, the rules client would specify this exemption when using the authorization client but any external users of the client would not be able to specify this exemption. With this revert, this exemption is applied to all users of the authorization client so rules created in Stack Rules Management and their derived alerts are only subject to producer based RBAC.

Checklist

Delete any items that are not applicable to this PR.

@ymao1 ymao1 changed the title Reverting changes to genericize exempt consumer id [Alerting] Alerting authorization should always exempt alerts consumer Aug 11, 2021
@ymao1 ymao1 self-assigned this Aug 11, 2021
@ymao1 ymao1 added Feature:Alerting Feature:Alerting/RulesFramework Issues related to the Alerting Rules Framework Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) labels Aug 11, 2021
@ymao1 ymao1 marked this pull request as ready for review August 11, 2021 17:31
@ymao1 ymao1 requested a review from a team as a code owner August 11, 2021 17:31
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

@ymao1 ymao1 added v7.15.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Aug 11, 2021
Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes LGTM

Only thing I'm wondering about is if we would want a FT for this, but not quite sure how it would work. Wouldn't need to be part of this PR, and we're getting some implicit testing with the rule registry anyway.

@ymao1
Copy link
Contributor Author

ymao1 commented Aug 11, 2021

Only thing I'm wondering about is if we would want a FT for this, but not quite sure how it would work. Wouldn't need to be part of this PR, and we're getting some implicit testing with the rule registry anyway.

I agree! We have this issue open for adding tests under user with limited role and I think functional tests for this stuff in that test suite would make a lot of sense!

Copy link
Contributor

@YulNaumenko YulNaumenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Is it documented for plugin devs somewhere how to use our authorization client?

@ymao1
Copy link
Contributor Author

ymao1 commented Aug 11, 2021

LGTM! Is it documented for plugin devs somewhere how to use our authorization client?

I don't think so but it definitely should be! I'll make an issue for it, thanks!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

cc @ymao1

@ymao1 ymao1 added the auto-backport Deprecated - use backport:version if exact versions are needed label Aug 11, 2021
@ymao1 ymao1 merged commit d07f0e6 into elastic:master Aug 11, 2021
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Aug 11, 2021
…mer (elastic#108220)

* Reverting changes to genericize exempt consumer id

* Adding unit test for find auth filter when user has no privileges
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Aug 11, 2021
…mer (#108220) (#108258)

* Reverting changes to genericize exempt consumer id

* Adding unit test for find auth filter when user has no privileges

Co-authored-by: ymao1 <ying.mao@elastic.co>
@ymao1 ymao1 deleted the alerting/auth-client-exempt-alerts-consumer branch August 17, 2021 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Alerting/RulesFramework Issues related to the Alerting Rules Framework Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.15.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Alerting] Alerting authorization should always exempt alerts consumer
5 participants