Migrate security chromeless views to Kibana Platform plugin #54021

azasypkin · 2020-01-06T15:29:01Z

In this PR I migrate everything that's currently possible from the legacy to the platform Kibana Security plugin:

Login, Logout, Logged Out, Overwritten Session and Account Management views are migrated
All remaining legacy interceptors are moved into a single hack/legacy.ts file
Platform plugin config is complete and doesn't allow unknown keys anymore

Note to reviewers: it may be a bit easier to review commit by commit since there is a lot of moves that were done in a separate commit.

Note to myself: be careful when backporting config changes to 7.x (xpack.security.authProviders|public are still there 🙈 )

Blocked by: ~~#53880, #54470, #54472~~, #58327, ~~#58627~~

elasticmachine · 2020-01-06T15:29:04Z

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

azasypkin · 2020-02-26T13:36:47Z

retest

x-pack/legacy/plugins/security/public/hacks/legacy.ts

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


x-pack/legacy/plugins/security/public/hacks/legacy.ts

+
+const securityPluginSetup = (npSetup.plugins as any).security as SecurityPluginSetup;
+if (securityPluginSetup) {
+  routes.when('/account', {


x-pack/legacy/plugins/security/public/hacks/legacy.ts

+
+      function interceptorFactory(responseHandler: (response: ng.IHttpResponse<unknown>) => any) {
+        return function interceptor(response: ng.IHttpResponse<unknown>) {
+          // TODO: SHOULD WE CHECK THAT IT'S NOT ERROR RESPONSE (&& response.status !== 401)?


x-pack/legacy/plugins/security/public/hacks/legacy.ts

+            securityPluginSetup.sessionTimeout.extend(response.config.url);
+          }
+
+          if (response.status !== 401 || isAnonymous) {


x-pack/legacy/plugins/security/public/hacks/on_unauthorized_response.js

-import 'plugins/security/services/auto_logout';
-
-function isUnauthorizedResponseAllowed(response) {
-  const API_WHITELIST = ['/internal/security/login', '/internal/security/users/.*/password'];


x-pack/legacy/plugins/security/public/services/auto_logout.js

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


x-pack/legacy/plugins/security/public/hacks/on_session_timeout.js

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


x-pack/legacy/plugins/security/public/hacks/on_unauthorized_response.js

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


azasypkin · 2020-02-26T16:39:15Z

x-pack/plugins/security/public/account_management/account_management_app.ts

+      title: i18n.translate('xpack.security.account.breadcrumb', {
+        defaultMessage: 'Account Management',
+      }),
+      // TODO: switch to proper enum once https://github.com/elastic/kibana/issues/58327 is resolved.


note: just in case this issue isn't resolved before this PR is reviewed - Jest test tests that our magic number 3 corresponds to the AppNavLinkStatus.hidden

x-pack/plugins/security/public/authentication/login/login_page.tsx

+  state = { loginState: null };
+
+  public async componentDidMount() {
+    const loadingCount$ = new BehaviorSubject(1);


Uses a single test for the entire page, and more focused tests for each permutation of BasicLoginForm/DisabledLoginForm. This greatly reduces the snapshot size. Also adds tests for API calls.

jportner

Finished reviewing code (and added updated LoginPage tests as discussed offline).
I'll try out the functionality now.

x-pack/plugins/security/server/routes/views/login.ts

+
+      // Authentication flow isn't triggered automatically for this route, so we should explicitly
+      // check whether user has an active session already.
+      const isUserAlreadyLoggedIn = (await authc.getSessionInfo(request)) != null;


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

  public stop() {
-    this.sessionTimeout.stop();
+    if (this.sessionTimeout) {
+      this.sessionTimeout.stop();
+    }
+
    this.navControlService.stop();


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

+import { Observable } from 'rxjs';
+import { CoreSetup } from 'src/core/public';
+import { DataPublicPluginStart } from '../../../../src/plugins/data/public';
+import { SessionTimeout } from './session';
+import { PluginStartDependencies, SecurityPlugin } from './plugin';
+
+import { coreMock } from '../../../../src/core/public/mocks';
+import { managementPluginMock } from '../../../../src/plugins/management/public/mocks';
+import { licensingMock } from '../../licensing/public/mocks';
+import { ManagementService } from './management';
+
+describe('Security Plugin', () => {
+  describe('#setup', () => {


x-pack/plugins/security/public/authentication/login/login_page.test.tsx

+  beforeAll(() => {
+    Object.defineProperty(window, 'location', {
+      value: { href: 'http://some-host/bar', protocol: 'http' },
+      writable: true,
+    });
+  });


jportner

Tested changed pages locally, all works well.

LGTM on green CI / after comments are addressed!

…ore.

azasypkin · 2020-03-03T12:47:14Z

test/functional/config.js

@@ -92,10 +92,6 @@ export default async function({ readConfigFile }) {
        pathname: '/app/kibana',
        hash: '/dev_tools/console',
      },
-      account: {


note: I haven't found any place that would use this, so just removed instead of updating.

azasypkin · 2020-03-03T12:48:48Z

x-pack/plugins/security/server/authentication/authenticator.ts

@@ -192,7 +192,6 @@ export class Authenticator {
        client: this.options.clusterClient,
        logger: this.options.loggers.get('tokens'),
      }),
-      isProviderEnabled: this.isProviderEnabled.bind(this),


note: leftover from my recent PR, couldn't resist to fix that 🙈

azasypkin · 2020-03-03T12:49:52Z

x-pack/plugins/security/server/authentication/providers/saml.ts

@@ -366,7 +366,7 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider {
        'Login initiated by Identity Provider is for a different user than currently authenticated.'
      );
      return AuthenticationResult.redirectTo(
-        `${this.options.basePath.get(request)}/overwritten_session`,
+        `${this.options.basePath.serverBasePath}/app/security/overwritten_session`,


note: it should really be serverBasePath since view is Space agnostic.

azasypkin · 2020-03-03T12:51:30Z

x-pack/plugins/security/server/config.ts

-      true,
-      schema.maybe(schema.string({ minLength: 32 })),
-      schema.string({ minLength: 32, defaultValue: 'a'.repeat(32) })
+export const ConfigSchema = schema.object({


note: just reformatted to make eslint happy

… prefix for view route (with `appRoute`) that are rendered by dedicated server routes.

jportner · 2020-03-03T16:11:34Z

...plugins/security/public/authentication/overwritten_session/overwritten_session_page.test.tsx

+import { AuthenticationStatePage } from '../components/authentication_state_page';
+
+describe('OverwrittenSessionPage', () => {
+  it('renders as expected', async () => {


Nit: doesn't include test case for username === null

jportner · 2020-03-03T16:33:54Z

Tested again locally and it's all working as expected 👏

… for empty username in OverwrittenSession view.

azasypkin · 2020-03-03T19:06:40Z

@elasticmachine merge upstream

elasticmachine · 2020-03-03T19:06:42Z

merge conflict between base and head

azasypkin · 2020-03-03T21:18:46Z

retest

azasypkin · 2020-03-04T06:44:33Z

@elasticmachine merge upstream

azasypkin · 2020-03-04T12:31:48Z

7.x/7.7.0: 91babc7

kibanamachine · 2020-03-04T14:17:48Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 0822ae4

History

💚 Build #30641 succeeded 0822ae4
💔 Build #30542 failed 3710751
💔 Build #30491 failed 3710751
💛 Build #30434 was flaky f6cc830
💔 Build #30368 failed 321a4cf

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

azasypkin · 2020-03-04T14:19:18Z

Yeah, thank you for the heads up @kibanamachine, it's always better later than never.

azasypkin added chore blocked Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! release_note:skip Skip the PR/issue when compiling release notes Feature:NP Migration v7.7.0 labels Jan 6, 2020

mshustov mentioned this pull request Jan 8, 2020

[New platform] Security plugin integration #33775

Closed

34 tasks

azasypkin force-pushed the issue-xxx-np-views branch from 3c2fbca to 5065eb6 Compare January 21, 2020 17:54

azasypkin removed the blocked label Jan 21, 2020

azasypkin force-pushed the issue-xxx-np-views branch from 5065eb6 to 949db04 Compare February 4, 2020 15:01

azasypkin force-pushed the issue-xxx-np-views branch 2 times, most recently from 06b5fab to eb7f32f Compare February 24, 2020 18:08

azasypkin added 2 commits February 26, 2020 08:50

Move files

0619274

Changes

826b327

azasypkin force-pushed the issue-xxx-np-views branch from 3f7cb00 to 826b327 Compare February 26, 2020 11:46

azasypkin marked this pull request as ready for review February 26, 2020 16:08

azasypkin requested review from a team as code owners February 26, 2020 16:08