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.

Sign up for GitHub

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

Jump to bottom

[Monitoring] Improve permissions required around setup mode #50421

Merged
merged 8 commits into from
Nov 18, 2019
Merged

[Monitoring] Improve permissions required around setup mode #50421

merged 8 commits into from
Nov 18, 2019

Conversation

chrisronline
Copy link
Contributor

Resolves #50327

This PR does three things:

  1. Users that does not meet the minimum permission list for setup mode are not able to enable it
  2. Cloud users and users who do not meet the permission requirement will see a toast in the bottom right telling them they cannot enable setup mode
  3. Instead of using _cat/indices in get_collection_status, we are going to change the logic to make a terms agg query against the _index field (which is how most other areas of Kibana do this)

Testing

  1. Create a user with only kibana_user and monitoring_user roles
  2. Attempt to enable setup mode while logged into that user

@chrisronline
Add error messages when setup mode is not enabled, disable it for use…
b3455a0 
…rs without the necessary permissions, and change one query to relax the privilege requirements
@chrisronline chrisronline self-assigned this Nov 12, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/stack-monitoring (Team:Monitoring)

@elasticmachine
Copy link
Contributor

💔 Build Failed

@chrisronline
Copy link
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@cachedout
Copy link
Contributor

I encountered an error when testing this. I set up the permissions as described and then navigated to the Stack Monitoring application. From there, I attempted to click to launch Metricbeat-based setup. As expected, I received an error. (Yay!) However, when I return to try setup via internal collection, I receive a second set of errors. I have recorded a small demo of the scenario:

bad_perms

cachedout
cachedout requested changes Nov 14, 2019
View reviewed changes
Copy link
Contributor

@cachedout cachedout left a comment

Choose a reason for hiding this comment

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

Changes requested. Details in the a comment in the main thread of this PR.

@chrisronline
Copy link
Contributor Author

@cachedout This should be the same behavior as on master. If the logged in user does not have the necessary permissions to enable monitoring, they will see errors like it. Assuming we confirm this, do you think we should not attempt to handle that differently in this PR? I do agree we should handle that better though.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

igoristic
igoristic approved these changes Nov 14, 2019
View reviewed changes
Copy link
Contributor

@igoristic igoristic left a comment

Choose a reason for hiding this comment

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

Looks good 👍

I also agree this #50421 (comment) should be handled in a different PR

@cachedout
Copy link
Contributor

@cachedout This should be the same behavior as on master.

@chrisronline Yup, sounds like we should handle this separately from this PR. Shall I go ahead and file an issue or did you file one already?

@chrisronline
Copy link
Contributor Author

@elasticmachine merge upstream

@chrisronline
Copy link
Contributor Author

@cachedout I'll defer that to you - I'm not exactly sure what should be done in this scenario, considering the goal to deprecate internal monitoring in 8.0

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@chrisronline chrisronline merged commit 66038f5 into elastic:master Nov 18, 2019
@chrisronline chrisronline deleted the monitoring/setup_mode_permissions branch November 18, 2019 15:24
chrisronline added a commit to chrisronline/kibana that referenced this pull request Nov 18, 2019
@chrisronline
[Monitoring] Improve permissions required around setup mode (elastic#…
241dae9 
…50421)

* Add error messages when setup mode is not enabled, disable it for users without the necessary permissions, and change one query to relax the privilege requirements

* Fix default value

* PR feedback

* Forgot to update this part

* Fix tests
This was referenced Nov 18, 2019
Merged
Merged
chrisronline added a commit to chrisronline/kibana that referenced this pull request Nov 18, 2019
@chrisronline
[Monitoring] Improve permissions required around setup mode (elastic#…
b939d4c 
…50421)

* Add error messages when setup mode is not enabled, disable it for users without the necessary permissions, and change one query to relax the privilege requirements

* Fix default value

* PR feedback

* Forgot to update this part

* Fix tests
@chrisronline chrisronline mentioned this pull request Nov 18, 2019
Merged
chrisronline added a commit that referenced this pull request Nov 18, 2019
@chrisronline
[Monitoring] Improve permissions required around setup mode (#50421) (#…
ca87ab9 
…50918)

* Add error messages when setup mode is not enabled, disable it for users without the necessary permissions, and change one query to relax the privilege requirements

* Fix default value

* PR feedback

* Forgot to update this part

* Fix tests
chrisronline added a commit that referenced this pull request Nov 18, 2019
@chrisronline
[Monitoring] Improve permissions required around setup mode (#50421) (#…
0a27e53 
…50919)

* Add error messages when setup mode is not enabled, disable it for users without the necessary permissions, and change one query to relax the privilege requirements

* Fix default value

* PR feedback

* Forgot to update this part

* Fix tests
chrisronline added a commit that referenced this pull request Nov 18, 2019
@chrisronline
[7.4] [Monitoring] Improve permissions required around setup mode (#5…
6d2191c 
…0421) (#50922)

* Cherry-pick 66038f5

* Remove unnecessary field
@chrisronline
Copy link
Contributor Author

Backport:

7.x: ca87ab9
7.5: 0a27e53
7.4: 6d2191c

jloleysens added a commit to jloleysens/kibana that referenced this pull request Nov 18, 2019
@jloleysens
Merge branch 'master' of github.com:elastic/kibana into console-proxy…
bbee1ef 
…-fallback

* 'master' of github.com:elastic/kibana: (116 commits)
  [Maps] move apply global filter settting from layer to source (elastic#50523)
  [SIEM] Fix: Empty `Source` / `Destination` shown when only ports are populated (elastic#50843)
  [Maps] Delay vector tile layer syncing until spritesheet is loaded (elastic#48955)
  [Maps] prevent users from overflowing URL when filtering by shape (elastic#50747)
  [DOCS] Mark Beats central management as discontinued (elastic#49423)
  [page_objects/common_page] convert to ts (elastic#50771)
  [NP Kibana Migrations ] kibana plugin home (elastic#50444)
  [DOCS] Shareables naming convention (elastic#50497)
  [ML] DF Analytics - auto-populate model_memory_limit (elastic#50714)
  Increase alerting test stability and reduce flakiness (elastic#50246)
  [ML] Remaning new_job_new folder (elastic#50917)
  [Telemetry] Show opt-in changes for OSS users (elastic#50831)
  [ML] Fix lat_long anomalies table links menu and value formatting (elastic#50916)
  [Dev] Fix serialising a really big string (elastic#50915)
  Better explanation about the Prettier recommendation (extension vs. NPM module) (elastic#50629)
  [Monitoring] Use a basic monitoring user for tests (elastic#47865)
  [Monitoring] Gracefully handle issue with filebeat indices (elastic#48929)
  [Monitoring] Improve permissions required around setup mode (elastic#50421)
  Additional validation for elasticsearch username (elastic#48247)
  Revert changes to use_kibana_ui_setting (elastic#50877)
  ...

# Conflicts:
#	src/legacy/core_plugins/console/server/request.test.ts
@chrisronline chrisronline mentioned this pull request Mar 10, 2020
Closed
@chrisronline chrisronline mentioned this pull request Mar 11, 2021
Closed
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@igoristic igoristic igoristic approved these changes

@cachedout cachedout cachedout approved these changes

Assignees

@chrisronline chrisronline

Labels
Feature:Stack Monitoring release_note:fix review Team:Monitoring Stack Monitoring team v7.4.3 v7.5.1 v7.6.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Monitoring] Only elevated permission users should see Setup Mode
4 participants
@chrisronline @elasticmachine @cachedout @igoristic