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

Hide management sections based on cluster/index privileges #67791

Merged
merged 31 commits into from
Sep 14, 2020

Conversation

legrego
Copy link
Member

@legrego legrego commented May 29, 2020

Summary

Updates the Stack Management application to respond to the user's cluster and index privileges.

Previously, a vast majority of the Stack Management features were visible to all end-users, even if they weren't authorized to take advantage of these features. We were able to hide features within the Kibana section, as they were designed to respond to Kibana's privilege model (aka "feature controls"), but we were not able to hide anything else, as the rest of the stack management features relied on cluster and index privileges which were not under Kibana's control.

This PR allows Kibana to respond to the user's cluster and index privileges in order to toggle visibility for what I'm calling "Elasticsearch Features". As of now, this is limited to the functionality within the Stack Management application.

This does not add any additional security controls. It merely allows Kibana to know when it's safe to show or hide these features in the UI. The responsibility for authorizing access is still governed by Elasticsearch.

Motivation

The primary motivation is to reach feature-parity with the deprecated (and soon to be removed) "Dashboard-only mode". Dashboard-only mode allowed administrators to configure users who could only access the Dashboard application, and nothing else. Feature controls in its current form gets us very close to this behavior, but Stack Management has been a holdout due to its diverging privilege model.

This PR will allow the Stack Management application to be completely hidden if the end-user is not authorized to access any of the underlying features. Additionally, users will only see the management features that they're authorized to use.

Notable changes

Introduces "Elasticsearch Features"

This PR introduces the concept of an "Elasticsearch Feature", which is maintained by the existing features plugin. Elasticsearch features are siblings to the existing Kibana features. A key distinction is that Elasticsearch features are not visible within the Spaces or Role management screens. At this point, they are considered an implementation detail in order for Kibana to respond to the current user's privileges.

We may decide to surface this in the UI at some point in the future.

Renames Feature to KibanaFeature

Now that there are two types of features, we had to rename the existing Feature and FeatureConfig to KibanaFeature and KibanaFeatureConfig in order to distinguish this from ElasticsearchFeature and ElasticsearchFeatureConfig.

Updates authorization checks

Now that Kibana needs to respond to more than Kibana's own privileges, we've had to expand the security plugin's authorization checks to also account for cluster and index privileges. The checkPrivileges* suite of authorization functions have been updated to accept an object of the format:

// Old format
checkPrivileges(['array', 'of', 'kibana', 'privileges']);

// New format
checkPrivileges({
  kibana: ['array', 'of', 'kibana', 'privileges'],
  elasticsearch: {
    cluster: ['array', 'of', 'cluster', 'privileges'],
    index: {
       [indexName: string]: ['array', 'of', 'index', 'privileges']
    }
  }
});

👋 Attention code owners!

Please pay close attention to your review of this PR. This is one of those PRs where the review is as important as the implementation.
If I added a call to registerElasticsearchFeature within your code, then I humbly ask for the following as part of your review:

  1. Make sure your management feature still works
  2. Pay extra-special attention to any requiredClusterPrivileges, requiredIndexPrivileges, and requiredRoles that I defined as part of the call to registerElasticsearchFeature. I TOOK A GUESS when defining these privileges, as the documentation is sparse (or I can't search very well). If your application requires a different set of privileges to run, then please let me know as part of your review! For this PR, I'm only interested in defining the minimal set of privileges to make your feature function without authorization errors.
  3. Create a user with the privileges defined in step 2, and make sure your management feature works as expected. I did this as part of my work on this PR, but this is an invasive change that would benefit from your expertise.
  4. I'm being a pain. Please leave a comment in your PR review that indicates that you followed these instructions.

I'm aware that some features offer read-only and read-write access. You are welcome to enhance your feature in a followup PR (and I'm more than happy to help!). My goal for this PR is to know when it's safe to hide or show your feature. Nothing more, nothing less.

Preview of doc changes: https://kibana_67791.docs-preview.app.elstc.co/diff

Resolves #35040
Resolves #35965

@botelastic botelastic bot added Team:APM All issues that need APM UI Team support Team:Fleet Team label for Observability Data Collection Fleet team Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability labels May 29, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/ingest-management (Team:Ingest Management)

@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@legrego
Copy link
Member Author

legrego commented May 29, 2020

bad bot. You all will want to unsubscribe, this will be noisy for quite some time.

@legrego legrego force-pushed the fc/hide-management branch 2 times, most recently from 7175841 to 460d24e Compare June 1, 2020 13:44
@ph ph self-assigned this Jun 1, 2020
@legrego legrego force-pushed the fc/hide-management branch 2 times, most recently from 771f931 to 9aeb2ea Compare June 1, 2020 18:18
@ph ph removed their assignment Jun 1, 2020
@ph
Copy link
Contributor

ph commented Jun 1, 2020

@jen-huang Can you keep on eye on this issue?

@legrego
Copy link
Member Author

legrego commented Sep 10, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
features 11 +2 9

async chunks size

id value diff baseline
management 176.8KB +2.0B 176.8KB
security 1.0MB +18.0B 1.0MB
total +20.0B

page load bundle size

id value diff baseline
features 15.8KB +2.9KB 12.9KB
management 30.1KB +573.0B 29.5KB
ml 845.5KB +686.0B 844.8KB
security 305.7KB +225.0B 305.4KB
total +4.3KB

distributable file count

id value diff baseline
default 45522 +4 45518

History

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

@legrego legrego merged commit 2e34eb2 into elastic:master Sep 14, 2020
@legrego legrego deleted the fc/hide-management branch September 14, 2020 13:30
gmmorris added a commit to gmmorris/kibana that referenced this pull request Sep 14, 2020
* master: (26 commits)
  updating datatable type (elastic#77320)
  [ML] Fix custom URLs processing for security app (elastic#76957)
  [telemetry] add schema guideline + schema_check new check for --path config (elastic#75747)
  [ML] Transforms: API schemas and integration tests (elastic#75164)
  [Mappings editor] Add support for wildcard field type (elastic#76574)
  [Ingest Manager] Fix flyout instruction selection (elastic#77071)
  [Telemetry Tools] update lodash to 4.17 (elastic#77317)
  [APM] Service inventory redesign (elastic#76744)
  Hide management sections based on cluster/index privileges (elastic#67791)
  [Snapshot Restore] Disable steps when form is invalid (elastic#76540)
  [Mappings editor] Add support for positive_score_impact to rank_feature (elastic#76824)
  Update apm.ts (elastic#77310)
  [OBS] Remove beta badge, change news feed size and add external icon to news feed link (elastic#77164)
  [Discover] Convert legacy sort to be compatible with multi sort (elastic#76986)
  [APM] API Snapshot Testing (elastic#77229)
  [ML] Functional tests - increase wait time for DFA start (elastic#77307)
  [UiActions][Drilldowns] Fix actions sorting in context menu (elastic#77162)
  [Drilldowns] Wire up new links to new docs (elastic#77154)
  Fix APM issue template
  [Ingest Pipelines] Drop into an empty tree (elastic#76885)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Sep 14, 2020
* master: (65 commits)
  [Security Solution][Resolver] Analyzed event styling (elastic#77115)
  filter invalid SOs from the searc hresults in Task Manager (elastic#76891)
  [RUM Dashboard] Visitors by region map (elastic#77135)
  [Security Solution][Endpoint][Admin] Task/endpoint list actions (elastic#76555)
  [Ingest pipelines] Forms for processors T-U (elastic#76710)
  updating datatable type (elastic#77320)
  [ML] Fix custom URLs processing for security app (elastic#76957)
  [telemetry] add schema guideline + schema_check new check for --path config (elastic#75747)
  [ML] Transforms: API schemas and integration tests (elastic#75164)
  [Mappings editor] Add support for wildcard field type (elastic#76574)
  [Ingest Manager] Fix flyout instruction selection (elastic#77071)
  [Telemetry Tools] update lodash to 4.17 (elastic#77317)
  [APM] Service inventory redesign (elastic#76744)
  Hide management sections based on cluster/index privileges (elastic#67791)
  [Snapshot Restore] Disable steps when form is invalid (elastic#76540)
  [Mappings editor] Add support for positive_score_impact to rank_feature (elastic#76824)
  Update apm.ts (elastic#77310)
  [OBS] Remove beta badge, change news feed size and add external icon to news feed link (elastic#77164)
  [Discover] Convert legacy sort to be compatible with multi sort (elastic#76986)
  [APM] API Snapshot Testing (elastic#77229)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Sep 14, 2020
* master: (65 commits)
  [Security Solution][Resolver] Analyzed event styling (elastic#77115)
  filter invalid SOs from the searc hresults in Task Manager (elastic#76891)
  [RUM Dashboard] Visitors by region map (elastic#77135)
  [Security Solution][Endpoint][Admin] Task/endpoint list actions (elastic#76555)
  [Ingest pipelines] Forms for processors T-U (elastic#76710)
  updating datatable type (elastic#77320)
  [ML] Fix custom URLs processing for security app (elastic#76957)
  [telemetry] add schema guideline + schema_check new check for --path config (elastic#75747)
  [ML] Transforms: API schemas and integration tests (elastic#75164)
  [Mappings editor] Add support for wildcard field type (elastic#76574)
  [Ingest Manager] Fix flyout instruction selection (elastic#77071)
  [Telemetry Tools] update lodash to 4.17 (elastic#77317)
  [APM] Service inventory redesign (elastic#76744)
  Hide management sections based on cluster/index privileges (elastic#67791)
  [Snapshot Restore] Disable steps when form is invalid (elastic#76540)
  [Mappings editor] Add support for positive_score_impact to rank_feature (elastic#76824)
  Update apm.ts (elastic#77310)
  [OBS] Remove beta badge, change news feed size and add external icon to news feed link (elastic#77164)
  [Discover] Convert legacy sort to be compatible with multi sort (elastic#76986)
  [APM] API Snapshot Testing (elastic#77229)
  ...
legrego added a commit that referenced this pull request Sep 14, 2020
) (#77345)

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
smith added a commit to smith/kibana that referenced this pull request Sep 14, 2020
Looks like elastic#67791 introduced a find/replace change that broke APM's e2e tests. This reverts that change.
@smith smith mentioned this pull request Sep 14, 2020
smith added a commit that referenced this pull request Sep 15, 2020
* Move remaining uses of serviceName away from urlParams

There were a few of these that were either missed or lost in merge conflict resolution.

I went through everything that's used as a path parameter and made sure it wasn't being used anywhere with `urlParams`.

Previously none of the charts were working, now they all are.

Looks like #67791 introduced a find/replace change that broke APM's e2e tests. This reverts that change.
smith added a commit to smith/kibana that referenced this pull request Sep 15, 2020
* Move remaining uses of serviceName away from urlParams

There were a few of these that were either missed or lost in merge conflict resolution.

I went through everything that's used as a path parameter and made sure it wasn't being used anywhere with `urlParams`.

Previously none of the charts were working, now they all are.

Looks like elastic#67791 introduced a find/replace change that broke APM's e2e tests. This reverts that change.
smith added a commit that referenced this pull request Sep 15, 2020
* Move remaining uses of serviceName away from urlParams

There were a few of these that were either missed or lost in merge conflict resolution.

I went through everything that's used as a path parameter and made sure it wasn't being used anywhere with `urlParams`.

Previously none of the charts were working, now they all are.

Looks like #67791 introduced a find/replace change that broke APM's e2e tests. This reverts that change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Security/Feature Controls Platform Security - Spaces & Role Mgmt feature controls release_note:enhancement Team:APM All issues that need APM UI Team support Team:Fleet Team label for Observability Data Collection Fleet team Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.10.0 v8.0.0
Projects
None yet