Spaces - Hiding management link #38472

kobelb · 2019-06-07T21:57:03Z

Prior to this change, the Spaces management link was always being displayed, even if the user didn't have privileges. Now that we have uiCapabilities, we can hide this when appropriate!

"Release Note: Hiding the spaces management link when the user doesn't have privileges to manage spaces"

controlled sections

elasticmachine · 2019-06-07T21:57:13Z

Pinging @elastic/kibana-security

kobelb · 2019-06-07T21:58:46Z

src/legacy/server/capabilities/merge_capabilities.ts

-      catalogue: {},
-    } as Capabilities
-  );
+  merge({}, ...sources);


We were doing a "shallow merge" before, so the management section defined in the spaces plugin was clobbering the one in the kibana plugin.

kobelb · 2019-06-07T21:59:13Z

x-pack/plugins/spaces/public/views/management/index.tsx

@@ -18,7 +18,7 @@ import routes from 'ui/routes';
 import { AdvancedSettingsSubtitle } from './components/advanced_settings_subtitle';
 import { AdvancedSettingsTitle } from './components/advanced_settings_title';

-const MANAGE_SPACES_KEY = 'manage_spaces';
+const MANAGE_SPACES_KEY = 'spaces';


It felt weird to have the ui capability management.kibana.manage_spaces so I changed this. It's now management.kibana.spaces.

elasticmachine · 2019-06-07T23:03:06Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-10T17:45:48Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…gement-link

elasticmachine · 2019-06-10T20:50:04Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

legrego · 2019-06-10T20:52:05Z

ACK: reviewing today/tomorrow

legrego · 2019-06-11T11:47:03Z

src/legacy/server/capabilities/merge_capabilities.ts

-
-      return capabilities;
-    },
+  merge(


We're changing from a shallow merge to a deep merge to support the new management.kibana.spaces flag. Originally, this was intentionally designed as a shallow merge so that plugins would not clobber capabilities which didn't belong to them. While it's true that merge won't allow a plugin to delete capabilities defined by other plugins, the addition of capabilities to other plugin's "buckets" is likely not something we want to encourage/support.

I know we've discussed letting plugins enhance the capabilities of other plugins, but I don't know if this is the way we want to do so. Since management is a defined key of the Capabilities interface, how do you feel about allowing the merge for this key, but not for the rest?

The previous implementation wasn't preventing duplicate definitions of the same first-level ui capability: https://codepen.io/kobelb/pen/MMWROw?editors=0012. We take advantage of this for building the catalogue ui capabilities: some of them are hard-coded here and we add to this list within the xpack_main plugin here.

While it's true that merge won't allow a plugin to delete capabilities defined by other plugins, the addition of capabilities to other plugin's "buckets" is likely not something we want to encourage/support.

I agree, but I don't believe those restrictions where previously enforced, at least not when using injectDefaultVars (when we first implemented FC) or the new uiCapabilities. We have some enforcement on this within xpack_main here, but this is only for merging the ui capabilities that we derive from the feature registration.

I can look at changing spaces to not use uiCapabilities to define the management section, and instead augment uiCapabilitiesForFeatures, but we'll still need the ability to merge all of these "deeply".

Since management is a defined key of the Capabilities interface, how do you feel about allowing the merge for this key, but not for the rest?

I think the proper solution to this problem is to follow through on #36221 and make it so that each of these "ui capability sections" is able to define it's own rules and so we can enforce these restrictions throughout the system, I was trying to prevent making this a dependency for fixing this specific issue though.

I added tests with 4d21c34 to ensure we aren't clobbering anything, and added additional safeguards. Does this assuage any of your previous concerns?

I can look at changing spaces to not use uiCapabilities to define the management section, and instead augment uiCapabilitiesForFeatures, but we'll still need the ability to merge all of these "deeply".

So... we can't do this at the moment because we don't register a "Spaces" feature, and these actions are hard-coded here: https://github.com/elastic/kibana/pull/38472/files#diff-f5ed4149300a16c9d74494d7b470b450R68. I don't know what I was thinking previously.

I think the proper solution to this problem is to follow through on #36221 and make it so that each of these "ui capability sections" is able to define it's own rules and so we can enforce these restrictions throughout the system, I was trying to prevent making this a dependency for fixing this specific issue though.

Agreed, thanks for the thoughtful followup!

I added tests with 4d21c34 to ensure we aren't clobbering anything, and added additional safeguards. Does this assuage any of your previous concerns?

❤️

legrego · 2019-06-11T11:49:53Z

x-pack/plugins/spaces/public/views/management/page_routes.tsx

@@ -21,6 +21,7 @@ const reactRootNodeId = 'manageSpacesReactRoot';
 routes.when('/management/spaces/list', {
  template,
  k7Breadcrumbs: getListBreadcrumbs,
+  requireUICapability: 'spaces.manage',


The other management routes use their management.${section}.${feature} UI Capability to control access. Do we want to follow suite here and use management.kibana.spaces?

Sure, I'll change this to be consistent.

elasticmachine · 2019-06-11T16:57:43Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-11T17:59:03Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

legrego

LGTM!

* Changing the Spaces management section to behave like the other FC controlled sections * Adding those glorious tests and fixing a bug * Fixing some test descriptions * Making the mergeCapabilities operation emulate the old behavior * Fixing privileges test with the addition of the new action * Updating jest snapshot * Adding tests, preventing additional clobbering * Changing requireUICapability to use management.kibana.spaces

elasticmachine · 2019-07-08T18:46:30Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

kobelb added 2 commits June 7, 2019 14:14

Changing the Spaces management section to behave like the other FC

2f3e7fc

controlled sections

Adding those glorious tests and fixing a bug

131cfc2

kobelb requested a review from a team as a code owner June 7, 2019 21:57

kobelb added the Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! label Jun 7, 2019

kobelb added enhancement New value added to drive a business result v8.0.0 v7.3.0 labels Jun 7, 2019

kobelb changed the title ~~Spaces - Management link and authorization~~ Spaces - Hiding management link Jun 7, 2019

kobelb commented Jun 7, 2019

View reviewed changes

Fixing some test descriptions

6250324

kobelb added 2 commits June 10, 2019 07:39

Making the mergeCapabilities operation emulate the old behavior

00fb4f3

Fixing privileges test with the addition of the new action

84f8acf

kobelb added 2 commits June 10, 2019 12:49

Updating jest snapshot

49e078f

Merge remote-tracking branch 'upstream/master' into authz/spaces-mana…

f7a48ce

…gement-link

kobelb requested a review from legrego June 10, 2019 20:51

kobelb added the review label Jun 10, 2019

legrego reviewed Jun 11, 2019

View reviewed changes

Adding tests, preventing additional clobbering

4d21c34

Changing requireUICapability to use management.kibana.spaces

84644c8

legrego approved these changes Jun 11, 2019

View reviewed changes

kobelb merged commit 3e06e6b into elastic:master Jun 13, 2019

kobelb deleted the authz/spaces-management-link branch June 13, 2019 16:32

kobelb mentioned this pull request Jun 13, 2019

[7.x] Spaces - Hiding management link (#38472) #38903

Merged

kobelb added release_note:enhancement and removed enhancement New value added to drive a business result review labels Jul 8, 2019

cuff-links mentioned this pull request Mar 17, 2022

[Meta] FTR configurable test users for xpack #60815

Closed

68 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spaces - Hiding management link #38472

Spaces - Hiding management link #38472

kobelb commented Jun 7, 2019 •

edited

Loading

elasticmachine commented Jun 7, 2019

kobelb Jun 7, 2019

kobelb Jun 7, 2019 •

edited

Loading

elasticmachine commented Jun 7, 2019

elasticmachine commented Jun 10, 2019

elasticmachine commented Jun 10, 2019

legrego commented Jun 10, 2019

legrego Jun 11, 2019

kobelb Jun 11, 2019

kobelb Jun 11, 2019

legrego Jun 11, 2019

legrego Jun 11, 2019

kobelb Jun 11, 2019

elasticmachine commented Jun 11, 2019

elasticmachine commented Jun 11, 2019

legrego left a comment

elasticmachine commented Jul 8, 2019

Spaces - Hiding management link #38472

Spaces - Hiding management link #38472

Conversation

kobelb commented Jun 7, 2019 • edited Loading

elasticmachine commented Jun 7, 2019

kobelb Jun 7, 2019

Choose a reason for hiding this comment

kobelb Jun 7, 2019 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Jun 7, 2019

💔 Build Failed

elasticmachine commented Jun 10, 2019

💔 Build Failed

elasticmachine commented Jun 10, 2019

💚 Build Succeeded

legrego commented Jun 10, 2019

legrego Jun 11, 2019

Choose a reason for hiding this comment

kobelb Jun 11, 2019

Choose a reason for hiding this comment

kobelb Jun 11, 2019

Choose a reason for hiding this comment

legrego Jun 11, 2019

Choose a reason for hiding this comment

legrego Jun 11, 2019

Choose a reason for hiding this comment

kobelb Jun 11, 2019

Choose a reason for hiding this comment

elasticmachine commented Jun 11, 2019

💔 Build Failed

elasticmachine commented Jun 11, 2019

💚 Build Succeeded

legrego left a comment

Choose a reason for hiding this comment

elasticmachine commented Jul 8, 2019

💔 Build Failed

kobelb commented Jun 7, 2019 •

edited

Loading

kobelb Jun 7, 2019 •

edited

Loading