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

Cleanup alerting / actions feature controls #52286

Merged
merged 11 commits into from
Dec 13, 2019

Conversation

mikecote
Copy link
Contributor

@mikecote mikecote commented Dec 5, 2019

Resolves #51121.

This PR removes the alerting and action features from feature controls. Solution teams who want to use alerting will have to grant alerting privileges within their features as this PR does for SIEM.

Note: Granting alerting privileges at this time will allow full control of alerts created by any solution team. This will be solved with #43994.

Note: The alerting UI within the management section will have to look at multiple places to know whether to show or not. This will be solved with #52300 and if needed, we can automate this beforehand.

Example:

const canShowAlerting = capabilities.get().siem.showAlerting || capabilities.get().monitoring.showAlerting

@mikecote mikecote self-assigned this Dec 5, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-stack-services (Team:Stack Services)

Copy link
Contributor Author

@mikecote mikecote left a comment

Choose a reason for hiding this comment

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

Had a few questions about feature controls that I'm hoping someone on @elastic/kibana-security team can help me with 🙏

x-pack/legacy/plugins/siem/server/kibana.index.ts Outdated Show resolved Hide resolved
x-pack/legacy/plugins/monitoring/server/plugin.js Outdated Show resolved Hide resolved
x-pack/legacy/plugins/monitoring/server/plugin.js Outdated Show resolved Hide resolved
@elasticmachine
Copy link
Contributor

💔 Build Failed

@mikecote mikecote marked this pull request as ready for review December 11, 2019 13:29
@mikecote mikecote requested a review from a team as a code owner December 11, 2019 13:29
@mikecote mikecote requested review from a team December 11, 2019 13:29
@mikecote
Copy link
Contributor Author

@elastic/stack-monitoring I'm not sure if you wanted alerting privileges granted on master with this PR or if you wanted to do it yourself? Let me know, the code necessary is here and easy to remove.

@mikecote
Copy link
Contributor Author

@elasticmachine merge upstream

@mikecote
Copy link
Contributor Author

cc @peterschretlen so you're aware of the changes for 7.6. Let me know if this doesn't align.

Copy link
Contributor

@FrankHassanabad FrankHassanabad left a comment

Choose a reason for hiding this comment

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

Checked it out, ran a few test runs and everything still seems functional from the API standpoint, so I think we are good.

LGTM

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.

code LGTM

@chrisronline
Copy link
Contributor

@mikecote From a user perspective, what changes with this PR for stack monitoring? I pulled it down but don't see anything different in the spaces configuration.

@mikecote
Copy link
Contributor Author

@mikecote From a user perspective, what changes with this PR for stack monitoring? I pulled it down but don't see anything different in the spaces configuration.

From a user perspective, nothing will change when it comes to managing roles / users. Though they will see the alert / actions management screens once the PR merges (#48959) if they have monitoring_user role granted.

@mikecote
Copy link
Contributor Author

mikecote commented Dec 11, 2019

Also, you will see from a technical perspective the alerting APIs get disabled if you don't have monitoring or SIEM access.

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

@pmuellr
Copy link
Member

pmuellr commented Dec 12, 2019

Also, you will see from a technical perspective the alerting APIs get disabled if you don't have monitoring or SIEM access.

Ya, that's slightly weird, but understandable.

Two things:

  • will this cause an issue in the future; will we have to change some of this stuff in a future release that would end up getting messy in terms of migration? I guess we don't know exactly what the future holds in terms of sub-/super-feature access stuff, and it also doesn't (from my n00b perspective) seem like anything we're doing here is future-problematic, but ... still a bit of a security n00b.

  • if we find a case where a customer wants to "play" with alerts/actions, but can't get access to siem or monitoring in their organization, is there an "out" so they can? Like, a new role could be created with the appropriate privileges to work with alerts/actions without siem or monitoring access? Kinda thing.

@mikecote
Copy link
Contributor Author

will this cause an issue in the future; will we have to change some of this stuff in a future release that would end up getting messy in terms of migration? I guess we don't know exactly what the future holds in terms of sub-/super-feature access stuff, and it also doesn't (from my n00b perspective) seem like anything we're doing here is future-problematic, but ... still a bit of a security n00b.

I believe all features / privileges are calculated at run time so any change we do would automatically apply to the users on existing systems. This would mean once stop granting full alert object type access to certain features, users will stop having full control of alerts. cc @elastic/kibana-security just to confirm this is the case.

if we find a case where a customer wants to "play" with alerts/actions, but can't get access to siem or monitoring in their organization, is there an "out" so they can? Like, a new role could be created with the appropriate privileges to work with alerts/actions without siem or monitoring access? Kinda thing.

I think the future direction would be granting a default application to user defined alerts (ex: visualization). For that scenario, as long as users have access to visualizations, they would be able to use alerts.

@chrisronline
Copy link
Contributor

@mikecote I don't think that'll be a problem. Should be fine

@mikecote mikecote added Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) and removed DONOTUSETeam:Stack Services labels Dec 12, 2019
@mikecote
Copy link
Contributor Author

@chrisronline last question I had was did you want this PR to enable alerting for Stack Monitoring or did you have it in a feature branch somewhere and want to enable it there instead?

@chrisronline
Copy link
Contributor

to enable alerting for Stack Monitoring

What does that look like exactly? I guess I'm not sure what "enabling" means in this context? Make it a dependency on the plugin?

@mikecote
Copy link
Contributor Author

What does that look like exactly? I guess I'm not sure what "enabling" means in this context? Make it a dependency on the plugin?

Me and @chrisronline discussed offline that I will remove monitoring related changes in this PR and it will be added within their feature branch of alerting.

@mikecote mikecote removed the request for review from a team December 12, 2019 20:20
@kobelb
Copy link
Contributor

kobelb commented Dec 12, 2019

Did we release any versions with the alerting/actions features/privileges? Even if we did, we wouldn't break anything by removing them, but we might surprise users with them disappearing and then reappearing and users having privileges to them...

@mikecote
Copy link
Contributor Author

@kobelb the 7.4 release introduced features (#41389) while also disabling the plugins by default in the same release (#44083). Only as of 7.6 we've enabled them back (#51254). Though 7.3 did have alerting saved objects / some APIs but nothing officially supported or with features.

@kobelb
Copy link
Contributor

kobelb commented Dec 12, 2019

@kobelb the 7.4 release introduced features (#41389) while also disabling the plugins by default in the same release (#44083). Only as of 7.6 we've enabled them back (#51254). Though 7.3 did have alerting saved objects / some APIs but nothing officially supported or with features.

Thanks for the explanation, seems reasonable enough for us to just remove these features in this situation :)

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

History

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

@mikecote mikecote merged commit 88ae8d0 into elastic:master Dec 13, 2019
mikecote added a commit to mikecote/kibana that referenced this pull request Dec 13, 2019
* Initial work

* Apply changes based on feedback

* Fix a broken test

* Fix failing test

* Revert monitoring changes

* Remove UI features for now
mikecote added a commit that referenced this pull request Dec 13, 2019
* Initial work

* Apply changes based on feedback

* Fix a broken test

* Fix failing test

* Revert monitoring changes

* Remove UI features for now
timductive pushed a commit to timductive/kibana that referenced this pull request Dec 16, 2019
* Initial work

* Apply changes based on feedback

* Fix a broken test

* Fix failing test

* Revert monitoring changes

* Remove UI features for now
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.6.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove alerting / actions from features UI in security
7 participants