Security nav control => NP #52386

legrego · 2019-12-06T13:38:29Z

Summary

This resolves #44929, by migrating the security nav control to the new platform. Specifically, this PR:

Moves Nav Control components to the NP Security plugin
Migrates said components to TypeScript
Updates nav control to render even if user fetch (/api/security/v1/me) hasn't resolved yet
Uses the existing NP Security License service to determine when to register nav control (there was equivalent logic in the LP xpackInfo license service). This required moving the license service into the common directory, since it's being used by both client and server plugins. There was no server-specific code here, so it was a straight move without any code changes.

legrego · 2019-12-06T14:09:26Z

x-pack/plugins/security/public/nav_control/nav_control.tsx

+
+      const props = {
+        user,
+        editProfileUrl: core.http.basePath.prepend('/app/kibana/#account'),


I was going to also move the account management page as part of this PR, but it ended up turning into a larger project than I expected, so I'm keeping it in the LP for now to make this easier to review and merge.

Once we move it to the NP, we should decouple it from the /app/kibana route.

legrego · 2019-12-06T14:13:09Z

x-pack/plugins/security/public/plugin.ts

+      const shouldRegisterNavControl =
+        !this.isAnonymousPath(core.http.anonymousPaths) && showSecurityLinks;
+
+      if (shouldRegisterNavControl && !this.navControlRegistered) {


It's not currently possible to unregister the nav control once it is registered, so this will instead register the nav control the first time it should (based on license and current path).
Once registered, it will always be visible, even if the license changes in a way where we would want to hide it.

This is functionally equivalent to what existed in the LP, but since the NP is more reactive to configuration changes w/o full page reloads, this might be something we want to address in the future.

I need to add a test for this scenario yet

Instead of unregistering the nav control, could we always register the nav control and render an empty div when the license changes to no longer enable security?

Instead of unregistering the nav control, could we always register the nav control and render an empty div when the license changes to no longer enable security?

That's not a bad idea. I think I'll keep it as-is for now, so that we don't have a potentially glitchy UI in the case of a networking hiccup or other unexpected issue retrieving license information. I feel like the odds of a connectivity blip are greater than the odds of the license/security settings changing (which would likely require a page reload anyway).

x-pack/plugins/security/public/plugin.ts

legrego · 2019-12-06T14:52:56Z

Unrelated failure: #51952

legrego · 2019-12-06T14:53:08Z

@elasticmachine merge upstream

elasticmachine · 2019-12-06T16:09:37Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 921cbf8

elasticmachine · 2019-12-06T16:49:12Z

Pinging @elastic/kibana-security (Team:Security)

kobelb · 2019-12-09T17:15:41Z

ACK: reviewing

…hrome-nav

kobelb

nit: x-pack/plugins/security/common/licensing/license_service.ts is importing ILicense from ../../../licensing/server. Can we change this to refer tox-pack/plugins/licensing/common/types.ts?

x-pack/legacy/plugins/security/public/register_feature.js

x-pack/plugins/security/public/plugin.ts

kobelb · 2019-12-09T18:48:04Z

x-pack/plugins/security/public/plugin.ts

+      const shouldRegisterNavControl =
+        !this.isAnonymousPath(core.http.anonymousPaths) && showSecurityLinks;
+
+      if (shouldRegisterNavControl && !this.navControlRegistered) {


Instead of unregistering the nav control, could we always register the nav control and render an empty div when the license changes to no longer enable security?

x-pack/plugins/security/public/plugin.ts

…hrome-nav

x-pack/legacy/plugins/security/public/register_feature.js

x-pack/plugins/security/public/plugin.ts

…hrome-nav

…to security/np-chrome-nav

…in the kibana app

x-pack/plugins/security/common/licensing/license_service.ts

x-pack/plugins/security/common/licensing/license_service.test.ts

x-pack/plugins/security/public/nav_control/nav_control_component.test.tsx

kobelb · 2019-12-12T23:13:40Z

x-pack/plugins/security/public/nav_control/nav_control_service.tsx

+  ) {
+    core.chrome.navControls.registerRight({
+      order: 2000,
+      mount: (el: HTMLElement) => {


question: i'm not seeing an easy way to test the mount method, are you aware of one?

I'm not aware of one, but the functional test suites cover this functionality already by virtue of testing the popover and links inside (account management and logout)

Could mock core.chrome.navControls.registerRight to get the calling arguments, and use enzime/jsdom to test the MountPoint

Something like

kibana/src/core/public/overlays/modal/modal_service.test.tsx

Lines 47 to 58 in 01bbbf4

it('renders a modal to the DOM', () => {

expect(mockReactDomRender).not.toHaveBeenCalled();

modals.open(container => {

const content = document.createElement('span');

content.textContent = 'Modal content';

container.append(content);

return () => {};

});

expect(mockReactDomRender.mock.calls).toMatchSnapshot();

const modalContent = mount(mockReactDomRender.mock.calls[0][0]);

expect(modalContent.html()).toMatchSnapshot();

});

azasypkin · 2019-12-13T10:36:52Z

x-pack/plugins/security/common/licensing/license_service.ts

+    }
+  }
+
+  private isSecurityEnabledFromRawLicense(rawLicense: Readonly<ILicense> | undefined) {


note: was trying to use new and shiny optional chaining here, but couldn't come up with anything elegant 😢 The closest I have is:

const securityFeature = rawLicense?.getFeature('security'); return !!(securityFeature?.isAvailable && securityFeature.isEnabled);

x-pack/plugins/security/common/licensing/license_service.ts

Co-Authored-By: Aleh Zasypkin <[email protected]>

…hrome-nav

legrego · 2019-12-17T12:13:55Z

@azasypkin / @kobelb ready for re-review, for whoever is available to take a look.

…hrome-nav

kobelb · 2019-12-17T20:04:52Z

ACK: reviewing

kobelb · 2019-12-17T21:04:14Z

x-pack/plugins/security/public/nav_control/nav_control_service.tsx

+          !isAnonymousPath && showLinks && !this.navControlRegistered;
+
+        if (shouldRegisterNavControl) {
+          const user = core.http.get('/api/security/v1/me', {


Another PR has deprecated this route, and we should now be using /internal/security/me

Good catch, I'll update this

kobelb · 2019-12-17T21:05:09Z

LGTM minus the one nit about the deprecated route above.

…hrome-nav

kobelb

LGTM on green CI

elasticmachine · 2019-12-17T22:42:09Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: ed440a8

History

💚 Build #16145 succeeded f08cafb
💚 Build #15906 succeeded 03a1409
💔 Build #15848 failed 3d9f278
💔 Build #15516 failed c5a20b8
💔 Build #15363 failed 81a80d2

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

* migrating nav control to NP * move licensing service to common * only retrieve user when necessary * don't block rendering on user promise * testing nav control registration * moving logic to nav_control_service * register account management in a hack * update import location * updating license_service to manage its own subscription to the raw license * updating mock * update editProfileUrl to not require full page reload if already within the kibana app * alternate security license proposal * adds popover test. * switchMap -> map * additional test case. * Apply suggestions from code review Co-Authored-By: Aleh Zasypkin <[email protected]> * additional testing * fix merge from master * fixing es availability check * fix merge from master * switch from deprecated route Co-authored-by: Elastic Machine <[email protected]>

legrego added 4 commits December 6, 2019 08:32

migrating nav control to NP

28bec5c

move licensing service to common

8869956

only retrieve user when necessary

d0de6b1

don't block rendering on user promise

3bf75c8

legrego mentioned this pull request Dec 6, 2019

Port security & spaces chrome nav controls to New Platform #44929

Closed

legrego added the Feature:NP Migration label Dec 6, 2019

legrego commented Dec 6, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

Merge branch 'master' into security/np-chrome-nav

921cbf8

legrego marked this pull request as ready for review December 6, 2019 16:48

legrego requested a review from a team as a code owner December 6, 2019 16:48

legrego added release_note:skip Skip the PR/issue when compiling release notes Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v7.6.0 v8.0.0 labels Dec 6, 2019

kobelb self-requested a review December 9, 2019 17:15

Merge branch 'master' of github.com:elastic/kibana into security/np-c…

5ecea19

…hrome-nav

kobelb reviewed Dec 9, 2019

View reviewed changes

legrego added 4 commits December 9, 2019 13:56

testing nav control registration

8feb6be

moving logic to nav_control_service

6c64e4a

Merge branch 'master' of github.com:elastic/kibana into security/np-c…

bc259e7

…hrome-nav

register account management in a hack

b1cbcc6

legrego commented Dec 9, 2019

View reviewed changes

x-pack/legacy/plugins/security/public/register_feature.js Outdated Show resolved Hide resolved

x-pack/plugins/security/public/plugin.ts Outdated Show resolved Hide resolved

x-pack/plugins/security/public/plugin.ts Outdated Show resolved Hide resolved

legrego added 2 commits December 9, 2019 14:51

Merge branch 'master' of github.com:elastic/kibana into security/np-c…

8e2461f

…hrome-nav

Merge branch 'security/np-chrome-nav' of github.com:legrego/kibana in…

9c1b189

…to security/np-chrome-nav

legrego added 2 commits December 11, 2019 16:06

update editProfileUrl to not require full page reload if already with…

61d8976

…in the kibana app

alternate security license proposal

1ac4f1a

kobelb reviewed Dec 12, 2019

View reviewed changes

legrego added 3 commits December 12, 2019 18:39

adds popover test.

6160ad3

switchMap -> map

9f7a1d4

additional test case.

2bba5e1

azasypkin reviewed Dec 13, 2019

View reviewed changes

x-pack/plugins/security/common/licensing/license_service.ts Outdated Show resolved Hide resolved

azasypkin reviewed Dec 13, 2019

View reviewed changes

x-pack/plugins/security/common/licensing/license_service.ts Show resolved Hide resolved

legrego and others added 7 commits December 13, 2019 06:59

Apply suggestions from code review

81a80d2

Co-Authored-By: Aleh Zasypkin <[email protected]>

additional testing

eb9c5c0

Merge branch 'master' of github.com:elastic/kibana into security/np-c…

64f800c

…hrome-nav

Merge branch 'master' of github.com:elastic/kibana into security/np-c…

2a212a2

…hrome-nav

fix merge from master

c5a20b8

Merge branch 'master' of github.com:elastic/kibana into security/np-c…

3d9f278

…hrome-nav

fixing es availability check

03a1409

legrego added 2 commits December 17, 2019 10:53

Merge branch 'master' of github.com:elastic/kibana into security/np-c…

08dd4d9

…hrome-nav

fix merge from master

f08cafb

kobelb reviewed Dec 17, 2019

View reviewed changes

legrego added 2 commits December 17, 2019 16:14

switch from deprecated route

d493ec9

Merge branch 'master' of github.com:elastic/kibana into security/np-c…

ed440a8

…hrome-nav

kobelb approved these changes Dec 17, 2019

View reviewed changes

legrego merged commit 3b647b1 into elastic:master Dec 18, 2019

legrego deleted the security/np-chrome-nav branch December 18, 2019 12:17

legrego mentioned this pull request Dec 18, 2019

[7.x] Security nav control => NP (#52386) #53472

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Security nav control => NP #52386

Security nav control => NP #52386

legrego commented Dec 6, 2019 •

edited

Loading

legrego Dec 6, 2019

legrego Dec 6, 2019

legrego Dec 7, 2019

kobelb Dec 9, 2019

legrego Dec 11, 2019

This comment has been minimized.

legrego commented Dec 6, 2019

legrego commented Dec 6, 2019

elasticmachine commented Dec 6, 2019

elasticmachine commented Dec 6, 2019

kobelb commented Dec 9, 2019

kobelb left a comment

kobelb Dec 9, 2019

kobelb Dec 12, 2019

legrego Dec 12, 2019

pgayvallet Dec 13, 2019

pgayvallet Dec 13, 2019

azasypkin Dec 13, 2019

legrego commented Dec 17, 2019

kobelb commented Dec 17, 2019

kobelb Dec 17, 2019

legrego Dec 17, 2019

kobelb commented Dec 17, 2019

kobelb left a comment

elasticmachine commented Dec 17, 2019

	it('renders a modal to the DOM', () => {
	expect(mockReactDomRender).not.toHaveBeenCalled();
	modals.open(container => {
	const content = document.createElement('span');
	content.textContent = 'Modal content';
	container.append(content);
	return () => {};
	});
	expect(mockReactDomRender.mock.calls).toMatchSnapshot();
	const modalContent = mount(mockReactDomRender.mock.calls[0][0]);
	expect(modalContent.html()).toMatchSnapshot();
	});

Security nav control => NP #52386

Security nav control => NP #52386

Conversation

legrego commented Dec 6, 2019 • edited Loading

Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

legrego commented Dec 6, 2019

legrego commented Dec 6, 2019

elasticmachine commented Dec 6, 2019

💚 Build Succeeded

elasticmachine commented Dec 6, 2019

kobelb commented Dec 9, 2019

kobelb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

legrego commented Dec 17, 2019

kobelb commented Dec 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kobelb commented Dec 17, 2019

kobelb left a comment

Choose a reason for hiding this comment

elasticmachine commented Dec 17, 2019

💚 Build Succeeded

History

legrego commented Dec 6, 2019 •

edited

Loading