Migrate Management views to Kibana Platform plugin #53880

azasypkin · 2020-01-02T08:57:26Z

In this PR I'm moving all management screens and related code to Kibana Platform plugin. Legacy plugin is only responsible for the routing part (this API will available in Kibana Platform after #52579).

Additionally I removed some legacy and not used code.

Note to reviewers: It'd be easier to review this PR commit by commit (pure move commit and then changes). Also lots of changes are just import ... from ... changes. The riskiest change is Edit Role page.

Blocked by: ~~#53768~~, ~~#53620~~, ~~#55136~~
Fixes: #51756

elasticmachine · 2020-01-02T08:57:28Z

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

azasypkin · 2020-01-02T08:58:20Z

x-pack/legacy/plugins/security/common/model.ts

@@ -1,23 +0,0 @@
-/*


note: we don't use any common mode in the legacy plugin anymore.

azasypkin · 2020-01-02T09:09:50Z

x-pack/legacy/plugins/security/public/index.scss

@@ -15,3 +15,6 @@ $secFormWidth: 460px;
 // Public views
 @import './views/index';

+// Styles of Kibana Platform plugin
+@import '../../../../plugins/security/public/index';


note: looks like it's the only way to import style from platform plugin right now.

azasypkin · 2020-01-07T08:49:38Z

x-pack/plugins/security/kibana.json

@@ -3,7 +3,7 @@
  "version": "8.0.0",
  "kibanaVersion": "kibana",
  "configPath": ["xpack", "security"],
-  "requiredPlugins": ["features", "licensing"],
+  "requiredPlugins": ["data", "features", "home", "licensing", "management"],


I think home can be listed as an optional dependency. We're treating it that way for the Spaces plugin, so it would at least be consistent between our two main plugins.

++, it makes sense, will do (even though Kibana can't start without home right now since legacy Kibana plugin assumes that home is always present 🙈 , I don't think it's correct @TinaHeiligers?).

Disabling data will likely cause most of Kibana itself to become unusable in the future...

Yeah, and looking at its name it feels like it's a some sort of a fundamental plugin and I can't think of reason why user would want to disable it.

For management -- a lot of what the Security plugin has to offer is done via its Management screens, but I could see certain customers/users wanting to disable management, but still take advantage of our security features. They could, for example, manage their users and roles via our API, while still allowing users to authenticate and use Kibana normally. I think our ideal answer would be to use Feature Controls to disable management features, but that's not a reality for us yet (#35965)

That's an interesting thought, and management is clear "domain" that can potentially be disabled. It costs nothing for us to make it optional - let's try (same story here though - Kibana just fails if management is disabled - that's bad and we should change that all or nothing behaviour and enforce during review).

azasypkin · 2020-01-06T11:20:18Z

x-pack/legacy/plugins/security/public/images/logout.svg

@@ -1,3 +0,0 @@
-<svg xmlns="http://www.w3.org/2000/svg" width="8" height="8" viewBox="0 0 8 8" fill="white">


note: looks like a legacy we don't need anymore

azasypkin · 2020-01-06T11:20:52Z

...gacy/plugins/security/public/views/management/change_password_form/change_password_form.html

@@ -1,141 +0,0 @@
-<ng-form name="changePasswordForm">


note: looks like a legacy we don't need anymore

x-pack/plugins/security/public/management/edit_role/edit_role_page.tsx

@@ -4,6 +4,7 @@
 * you may not use this file except in compliance with the Elastic License.
 */

+import _, { get } from 'lodash';


x-pack/plugins/security/public/management/edit_role/edit_role_page.tsx

+    http.get('/api/features').then(
+      fetchedFeatures => setFeatures(fetchedFeatures),
+      (err: IHttpFetchError) => {
+        // TODO: This check can be removed once all of these `resolve` entries are moved out of Angular and into the React app.


azasypkin · 2020-01-06T13:57:51Z

x-pack/legacy/plugins/security/public/views/management/breadcrumbs.ts

@@ -66,7 +66,7 @@ export function getEditRoleBreadcrumbs($route: Record<string, any>) {

 export function getCreateRoleBreadcrumbs() {
  return [
-    ...getUsersBreadcrumbs(),
+    ...getRolesBreadcrumbs(),


note: looks like it was just a typo....

x-pack/plugins/security/public/management/_index.scss

@@ -0,0 +1,2 @@
+@import './edit_role/index';
+@import './edit_user/index';


legrego

Did a first pass, and this is looking good! I think the generalizations you put in place will suit the role mappings UI as well

x-pack/plugins/security/public/management/edit_role/edit_role_page.tsx

+  return spaces;
+}
+
+function useFeatures(http: HttpStart, fatalErrors: FatalErrorsSetup) {


x-pack/plugins/security/public/management/management_service.tsx

+        return;
+      }
+
+      // `getCurrentUser` will reject if there is no authenticated user, so we prevent them from


x-pack/plugins/security/public/management/api_clients/api_keys_api_client.ts

+const apiKeysUrl = '/internal/security/api_key';
+
+export class APIKeysAPIClient {
+  constructor(private readonly http: HttpStart) {}


x-pack/plugins/security/public/management/management_service.tsx

+    }
+  }
+
+  private subscribeToLicenseChanges(management: LegacyManagementStart) {


x-pack/legacy/plugins/security/public/views/management/api_keys.js

+import routes from 'ui/routes';
+import { getApiKeysBreadcrumbs } from './breadcrumbs';
+
+if (npStart.plugins.security.__legacyCompat.management) {


x-pack/legacy/plugins/security/public/views/login/parse_next.test.ts

@@ -4,51 +4,50 @@
 * you may not use this file except in compliance with the Elastic License.
 */

-import expect from '@kbn/expect';


x-pack/legacy/plugins/security/public/views/account/account.js

 import React from 'react';
 import { render, unmountComponentAtNode } from 'react-dom';
+import { i18n } from '@kbn/i18n';
+import { npStart } from 'ui/new_platform';
+import routes from 'ui/routes';

 routes.when('/account', {


azasypkin · 2020-01-15T14:05:29Z

x-pack/plugins/security/public/account_management/account_management_page.tsx

 import { PersonalInfo } from './personal_info';

 interface Props {
-  securitySetup: SecurityPluginSetup;
+  authc: AuthenticationServiceSetup;
+  apiClient: PublicMethodsOf<UserAPIClient>;


note: we'll get rid of these PublicMethodsOf approximately at the same time this issue is being handled in the core #54906

x-pack/plugins/security/public/management/management_service.ts

+      euiIconType: 'securityApp',
+    });
+
+    securitySection.registerApp(UsersManagementApp.create({ authc, getStartServices }));


legrego · 2020-01-15T19:38:26Z

x-pack/plugins/security/public/management/management_service.ts

+        if (enableStatus) {
+          app.enable();
+        } else {
+          app.disable();


This is probably a bug or design choice in the management plugin, but disabling our apps does not prevent routing to them. It merely hides the link from the management side nav. If you know the link to a disabled app, or if another app links you to a disabled app, it will still render as if it's enabled. @mattkime do you know if this is intentional or not?

@azasypkin If this is intentional, then I think we still need some version of the checkLicense toast message below that you asked about.

Hmm, good catch! Having disable that just hides doesn't feel right to me, it must be a bug or the wrong method name...

I'll help with this. This is a case where the solution during and after the transition to the new api is different.

x-pack/plugins/security/public/management/api_keys/api_keys_management_app.test.tsx

+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+jest.mock('./api_keys_grid', () => ({


x-pack/plugins/security/public/management/role_mappings/role_mappings_management_app.tsx

+  create({ getStartServices }: CreateParams) {
+    return {
+      id: this.id,
+      order: 40,


legrego

Great job on this! I've made it through most of the PR, and my feedback is really minor. I have some more testing I want to do yet, but so far so good!

x-pack/plugins/security/public/management/roles/roles_management_app.tsx

+          setBreadcrumbs([
+            ...rolesBreadcrumbs,
+            action === 'edit' && roleName
+              ? { text: roleName, href: `#${basePath}/edit/${roleName}` }


legrego · 2020-01-15T20:10:06Z

x-pack/plugins/security/public/management/users/edit_user/edit_user_page.tsx

@@ -579,5 +582,3 @@ class EditUserPageUI extends Component<Props, State> {
    );
  }
 }
-
-export const EditUserPage = injectI18n(EditUserPageUI);


x-pack/plugins/security/public/management/users/users_management_app.tsx

+          setBreadcrumbs([
+            ...usersBreadcrumbs,
+            username
+              ? { text: username, href: `#${basePath}/edit/${username}` }


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

+      });
+    }
+
+    // Home is an optional plugin, and if it's disabled we shouldn't try to fill feature catalog.


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

+            'Protect your data and easily manage who has access to what with users and roles.',
+        }),
+        icon: 'securityApp',
+        path: '/app/kibana#/management/security',


legrego · 2020-01-15T20:21:27Z

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

+            'Protect your data and easily manage who has access to what with users and roles.',
+        }),
+        icon: 'securityApp',
+        path: '/app/kibana#/management/security',


This link ultimately navigates to /app/kibana#/management?section=security, which just places the user at the root of the management application.

I'm not sure where else we'd want to put them, but we can probably just do away with the /security portion of this URL, unless we're going to have our own landing page within management for our security offerings.

I also assume at some point that management won't live within the kibana app. I don't think we have a functional test for this catalogue entry, so we'll want to keep track of when this happens so we can update accordingly.

I'm not sure where else we'd want to put them, but we can probably just do away with the /security portion of this URL, unless we're going to have our own landing page within management for our security offerings.

Right, I forgot to mention that we previously could set default app for the section and made users app as default. It seems we cannot do this anymore with the NP API. I'll switch to users URL here for now to keep existing behavior.

@mattkime can you please confirm that it's intentional that new Management API doesn't provide a way to specify default application for the section URL (e.g. previously if user navigated to #/management/security we'd forward them to #/management/security/users with something like this: uiRoutes.when(SECURITY_PATH, { redirectTo: USERS_PATH });)?

I also assume at some point that management won't live within the kibana app. I don't think we have a functional test for this catalogue entry, so we'll want to keep track of when this happens so we can update accordingly.

Yeah, I see we have quite a few places in Kibana where app/kibana#/management/ is hard-coded, so I believe it's safe to assume that whoever will be changing this URL will grep for places like this or setup an automatic forwarder with deprecation warning (and announcement).

can you please confirm that it's intentional that new Management API doesn't provide a way to specify default application for the section URL

Its intentional but I'm happy to have a discussion. You can implement the redirect, I don't see the importance of having it at the api level but I might be unfamiliar with your particular challenge.

legrego · 2020-01-15T20:30:12Z

x-pack/plugins/security/public/management/role_mappings/role_mappings_management_app.tsx

+          setBreadcrumbs([
+            ...roleMappingsBreadcrumbs,
+            name
+              ? { text: name, href: `#${basePath}/edit/${name}` }


Suggested change

? { text: name, href: `#${basePath}/edit/${name}` }

? { text: name, href: `#${basePath}/edit/${encodeURIComponent(name)}` }

legrego · 2020-01-15T20:36:43Z

x-pack/plugins/security/public/management/users/edit_user/edit_user_page.tsx

+    await this.setCurrentUser();
+  }
+
+  public async componentDidUpdate(prevProps: Props) {


question Is this logic needed as a result of the new routing we have to do?

Yeah, when you open Edit User page and manually change username in the URL or just remove it and hit Enter, Router logic will be triggered and for example will update breadcrumbs. But since view is already mounted we need to reload user as well.

That made me realize that we need componentDidUpdate for Edit Role Mappings as well, I'll add it. Edit Role page is based on Hooks so it already should work properly.

azasypkin · 2020-01-16T08:29:12Z

x-pack/plugins/security/public/management/management_service.ts

+      title: i18n.translate('xpack.security.management.securityTitle', {
+        defaultMessage: 'Security',
+      }),
+      order: 100,


note: Just noticed that ML also uses 100 🙈

… appropriate naming format for the management app object names, fix feature catalogue link, properly handle role mapping name change in Edit Role Mapping page, properly encode parameters in breadcrumbs links.

legrego · 2020-01-16T12:45:29Z

x-pack/plugins/security/public/management/roles/edit_role/edit_role_page.tsx

+) {
+  const [indexPatternsTitles, setIndexPatternsTitles] = useState<string[] | null>(null);
+  useEffect(() => {
+    indexPatterns.getTitles().then(setIndexPatternsTitles, err => fatalErrors.add(err));


Rather than using a fatal error here, we could instead show a toast notification. That would allow us to resolve #51756. When we fail, returning an empty array should allow the app to continue functioning.

Do you think we should make a distinction between 403 and the rest?

I'm torn -- checking for the 403 is more intentional, but on the other hand, this request isn't required for the page to function...

Let's check for the 403, and use a fatal error for other failure codes. We'll have a better chance of catching a regression this way, instead of silently failing when we expected a successful response.

legrego

LGTM pending green CI and change to check for the 403 error response on index patterns. Nice work!

…en index patterns are not available.

azasypkin · 2020-01-17T22:21:49Z

src/core/public/saved_objects/saved_objects_client.ts

-      delete kfetchError.response;
-      return Promise.reject(kfetchError);
-    });
+    return this.http.fetch(path, { method, query, body });


note: discussed with @rudolf that's no longer necessary since BWC is maintained within HttpFetchError itself now.

azasypkin · 2020-01-18T06:33:40Z

@elasticmachine merge upstream

azasypkin · 2020-01-18T06:36:20Z

#55136 is merged, now everything should be in place :)

rudolf

👍 for Platform changes

azasypkin · 2020-01-21T07:17:52Z

@elasticmachine merge upstream

kibanamachine · 2020-01-21T08:40:43Z

💚 Build Succeeded

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

History

💚 Build #21245 succeeded 28aff3b
💚 Build #21202 succeeded e9cb504
💚 Build #20787 succeeded 093b223
💚 Build #20490 succeeded c352d13
💔 Build #20369 failed 0a2e2d2

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

legrego

Sorry for the delay, most recent changes LGTM!

azasypkin · 2020-01-21T14:24:22Z

7.x/7.7.0: d57c7b7

azasypkin added chore blocked Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! Feature:NP Migration v7.6.0 labels Jan 2, 2020

azasypkin commented Jan 2, 2020

View reviewed changes

azasypkin force-pushed the issue-xxx-np-management branch 2 times, most recently from 1ff6eda to 792f460 Compare January 6, 2020 10:53

azasypkin removed the blocked label Jan 6, 2020

azasypkin commented Jan 6, 2020

View reviewed changes

azasypkin marked this pull request as ready for review January 6, 2020 13:56

azasypkin requested review from a team as code owners January 6, 2020 13:56

azasypkin added the release_note:skip Skip the PR/issue when compiling release notes label Jan 6, 2020

azasypkin commented Jan 6, 2020

View reviewed changes

cchaos reviewed Jan 6, 2020

View reviewed changes

legrego self-requested a review January 6, 2020 15:26

azasypkin added the blocked label Jan 6, 2020

azasypkin mentioned this pull request Jan 6, 2020

Migrate security chromeless views to Kibana Platform plugin #54021

Merged

This was referenced Jan 6, 2020

Reorganizing spaces client-side plugin #53644

Merged

Security - Refactor edit role screen #54046

Closed

legrego reviewed Jan 7, 2020

View reviewed changes

mshustov mentioned this pull request Jan 8, 2020

[New platform] Security plugin integration #33775

Closed

34 tasks

legrego mentioned this pull request Jan 10, 2020

Spaces -> Client to NP #54298

Merged

azasypkin commented Jan 15, 2020

View reviewed changes

legrego reviewed Jan 15, 2020

View reviewed changes

azasypkin commented Jan 16, 2020

View reviewed changes

Review#2: remove role related methods from UsersAPIClient, use more…

093b223

… appropriate naming format for the management app object names, fix feature catalogue link, properly handle role mapping name change in Edit Role Mapping page, properly encode parameters in breadcrumbs links.

legrego reviewed Jan 16, 2020

View reviewed changes

azasypkin mentioned this pull request Jan 16, 2020

Remove redundant x-pack/typings. #55042

Merged

legrego approved these changes Jan 17, 2020

View reviewed changes

azasypkin added 2 commits January 17, 2020 23:13

Merge branch 'master' into issue-xxx-np-management

e783bb2

Review#3: remove unnecessary ts-ignore directives and show a toast wh…

e9cb504

…en index patterns are not available.

azasypkin requested a review from a team as a code owner January 17, 2020 22:18

azasypkin commented Jan 17, 2020

View reviewed changes

Merge branch 'master' into issue-xxx-np-management

28aff3b

azasypkin added v7.7.0 and removed v7.6.0 labels Jan 18, 2020

rudolf approved these changes Jan 20, 2020

View reviewed changes

Merge branch 'master' into issue-xxx-np-management

c4cba87

azasypkin requested a review from legrego January 21, 2020 08:50

legrego approved these changes Jan 21, 2020

View reviewed changes

azasypkin merged commit 9d3d3cd into elastic:master Jan 21, 2020

azasypkin deleted the issue-xxx-np-management branch January 21, 2020 12:40

azasypkin added the backport pending label Jan 21, 2020

azasypkin mentioned this pull request Jan 21, 2020

[7.x] Migrate Management views to Kibana Platform plugin (#53880) #55382

Merged

azasypkin added backported and removed backport pending labels Jan 21, 2020

		@@ -1,3 +0,0 @@
		<svg xmlns="http://www.w3.org/2000/svg" width="8" height="8" viewBox="0 0 8 8" fill="white">

		@@ -0,0 +1,2 @@
		@import './edit_role/index';
		@import './edit_user/index';

	? { text: name, href: `#${basePath}/edit/${name}` }
	? { text: name, href: `#${basePath}/edit/${encodeURIComponent(name)}` }

Migrate Management views to Kibana Platform plugin #53880

Migrate Management views to Kibana Platform plugin #53880

Conversation

azasypkin commented Jan 2, 2020 • edited Loading

elasticmachine commented Jan 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

legrego left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

legrego left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

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

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 left a comment

Choose a reason for hiding this comment

azasypkin Jan 17, 2020 • edited Loading

Choose a reason for hiding this comment

azasypkin commented Jan 18, 2020

azasypkin commented Jan 18, 2020

rudolf left a comment

Choose a reason for hiding this comment

azasypkin commented Jan 21, 2020

kibanamachine commented Jan 21, 2020

azasypkin commented Jan 2, 2020 •

edited

Loading

azasypkin Jan 17, 2020 •

edited

Loading