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

[ResponseOps][Alerting] Hiding all features in a space causes rules to stop running #145372

Merged
merged 5 commits into from Nov 17, 2022

Conversation

doakalexi
Copy link
Contributor

@doakalexi doakalexi commented Nov 16, 2022

Resolves #144638

Summary

Removes logic that prevents rules from running when all features in a space are disabled.

Checklist

To verify

  • Create an alerting rule
  • Go to the spaces page, and disable all features in the space
  • Look at your terminal to see the alerting rule still running and no errors

@doakalexi doakalexi changed the title Updating to run rules when features are hidden [ResponseOps][Alerting] Hiding all features in a space causes rules to stop running Nov 16, 2022
@doakalexi doakalexi added Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) release_note:fix labels Nov 16, 2022
@doakalexi doakalexi marked this pull request as ready for review November 16, 2022 21:04
@doakalexi doakalexi requested a review from a team as a code owner November 16, 2022 21:04
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

ESLint disabled in files

id before after diff
osquery 1 2 +1

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
fleet 59 65 +6
osquery 108 113 +5
securitySolution 441 447 +6
total +19

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
fleet 67 73 +6
osquery 109 115 +6
securitySolution 518 524 +6
total +20

History

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

Copy link
Contributor

@ersin-erdal ersin-erdal left a comment

Choose a reason for hiding this comment

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

LGTM.
Tested locally, works as expected.

@doakalexi doakalexi merged commit 6a3f8ad into elastic:main Nov 17, 2022
@kibanamachine kibanamachine added v8.7.0 backport:skip This commit does not require backporting labels Nov 17, 2022
@ymao1
Copy link
Contributor

ymao1 commented Nov 17, 2022

This change also makes it so that a user with access to a space where all features are disabled can still use the alerting API to list all rules from this space that were created before the features were disabled. I'm assuming that's ok? cc @mikecote

@mikecote
Copy link
Contributor

This change also makes it so that a user with access to a space where all features are disabled can still use the alerting API to list all rules from this space that were created before the features were disabled. I'm assuming that's ok? cc @mikecote

If ^^ is true, we can't have the alerting API leak rule information for users who don't have access to the feature.. We might have to revisit the approach and revert / fix the change.

@ymao1
Copy link
Contributor

ymao1 commented Nov 17, 2022

This is what I tested:

  • Create a rule in the default space
  • Turn off all features for that space
  • Create a role for the default space that doesn't have access to any rule types
  • Create a user with this role
  • Log in as this user. Can't navigate anywhere in the UI
  • Navigate to https://localhost:5601/api/alerting/rules/_find and see the rule that I created in step 1, even though I shouldn't have access to any rule types.

Would be great to verify that others see this behavior as well.

@pmuellr
Copy link
Member

pmuellr commented Nov 17, 2022

TBH, I was kinda confused by this issue, because I thought - well, if they don't have access to the features, they probably shouldn't be able to run rules.

Was the thinking here resilience? To make sure the rule would keep running if someone accidentally made a bunch of feature changes?

Especially given the unintended consequences ^^^, I think we should revisit. To me, ideally the rule would error out when run, and surface an actionable message to the user in the UX on what happened.

@mikecote
Copy link
Contributor

The way I see it with my thinking is; when rules are created, they take a snapshot of the user's privileges in time and will keep running using those. So if the privileges or user roles have changed, the rule shouldn't be impacted by them and still run as what the snapshot encapsulated when the rule was created / updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:fix Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hiding all features in a space causes rules to stop running
8 participants