Security - remove auth scope provider by legrego · Pull Request #36998 · elastic/kibana

legrego · 2019-05-23T15:26:34Z

Summary

The security plugin provides a way for other plugins to append "scopes" to the authenticated user. Based on a conversation here, this is not something we want to migrate to the new platform.

This PR removes the auth scope provider, and replaces its usage within the Dashboard Only Mode plugin with a slightly more straightforward implementation.

legrego · 2019-05-23T15:27:10Z

x-pack/plugins/dashboard_mode/server/dashboard_mode_request_interceptor.js

+        return h.continue;
+      }
+
+      const user = auth.credentials;


Logic copied from dashboard_mode_auth_scope

elasticmachine · 2019-05-23T23:20:16Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…ve-auth-scopes

elasticmachine · 2019-05-24T13:11:15Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…ve-auth-scopes

elasticmachine · 2019-05-24T15:05:06Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…ve-auth-scopes

elasticmachine · 2019-05-24T16:25:01Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…ve-auth-scopes

elasticmachine · 2019-05-24T20:51:15Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

legrego · 2019-05-24T21:10:20Z

retest

elasticmachine · 2019-05-24T22:09:31Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-05-24T22:59:14Z

Pinging @elastic/kibana-security

legrego · 2019-05-28T13:49:31Z

x-pack/plugins/dashboard_mode/server/dashboard_mode_request_interceptor.js

    type: 'onPostAuth',
    async method(request, h) {
      const { auth, url } = request;
+      const user = auth.credentials;


As you can tell from the commit history in this PR, the ordering of operations in this function is rather fragile.

Previously, this logic was part of dashboard_mode_auth_scope, which ran after a successful authentication. Now, this runs as part of "onPostAuth", which runs after auth, but also runs when security is disabled completely.

The existing (and current) logic uses the UI Settings service to retrieve the list of dashboard-only-mode roles. The act of retrieving this setting ends up creating the config document if it doesn't already exist. Various functional tests have come to indirectly rely on this behavior, so I've opted to maintain that in this PR. I had originally hoped to optimize the existing implementation by not querying the UI Settings Service unless strictly necessary, but I wasn't able to do so without also revisiting all of the existing functional tests. I vote to work in that when we remove dashboard-only-mode altogether.

The act of retrieving this setting ends up creating the config document if it doesn't already exist. Various functional tests have come to indirectly rely on this behavior, so I've opted to maintain that in this PR. I had originally hoped to optimize the existing implementation by not querying the UI Settings Service unless strictly necessary, but I wasn't able to do so without also revisiting all of the existing functional tests. I vote to work in that when we remove dashboard-only-mode altogether.

That's a bummer :/ How many (approximately) of such weird tests we have? I'm fine with leaving it as is, just curious.

How many (approximately) of such weird tests we have? I'm fine with leaving it as is, just curious.

It's hard to quantify this since the FTR doesn't run test suites to completion once it finds a test failure. I know we had failures in infra and canvas, but I'm sure there are a number of other impacted suites.

Okay, thanks, that's what I wanted to know. I was just curious whether tests were failing within one "area" or not.

azasypkin · 2019-05-28T14:55:19Z

ACK: started to review, planning to finish today or tomorrow at the latest.

azasypkin

LGTM, just a couple of minor nits. Thanks for doing this, I always wanted to get rid of auth scopes ^feature^ 🙂

azasypkin · 2019-05-28T14:18:39Z

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

@@ -133,11 +132,7 @@ class Authenticator {
   * @param authScope AuthScopeService instance.


nit: please remove authScope from JSDoc as well.

azasypkin · 2019-05-28T14:26:25Z

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

-          // Complement user returned from the provider with scopes.
-          scope: await this.authScope.getForRequestAndUser(request, authenticationResult.user!),
-        } as any);
+        return AuthenticationResult.succeeded(authenticationResult.user as any);


nit: authenticationResult.user as any ---> authenticationResult.user! would be more appropriate.

azasypkin · 2019-05-29T08:48:32Z

x-pack/plugins/dashboard_mode/server/dashboard_mode_request_interceptor.js

+      const user = auth.credentials;
+      const roles = user ? user.roles : [];
+
+      if (!user) {


suggestion: since we create roles even if user isn't defined, can't we just do something like this instead and use our roles list below instead of user.roles?

// Clarifying comment goes here .... const roles = (auth.credentials && auth.credentials.roles) || []; if (roles.length === 0) { return h.continue; }

Or the way it's done also related to these weird functional tests that depend on config object? If so, feel free to ignore this suggestion.

Or the way it's done also related to these weird functional tests that depend on config object? If so, feel free to ignore this suggestion.

Yep, it was done this way because of the weird functional tests

azasypkin · 2019-05-29T09:26:26Z

x-pack/plugins/dashboard_mode/server/dashboard_mode_request_interceptor.js

+      if (enforceDashboardOnlyMode) {
        if (url.path.startsWith('/app/kibana')) {
          // If the user is in "Dashboard only mode" they should only be allowed to see
          // that app and none others.  Here we are intercepting all other routing and ensuring the viewer


uh oh: we have a link to x-pack-kibana repo :)

ha, good catch, will remove

azasypkin · 2019-05-29T09:29:42Z

x-pack/plugins/dashboard_mode/server/dashboard_mode_request_interceptor.js

 import Boom from 'boom';

 import {
-  AUTH_SCOPE_DASHBORD_ONLY_MODE


nit: can you please also get rid of AUTH_SCOPE_DASHBORD_ONLY_MODE in x-pack/plugins/dashboard_mode/common/constants.js?

I can't believe I missed that! will do!

azasypkin · 2019-05-29T09:31:08Z

x-pack/plugins/dashboard_mode/server/__tests__/dashboard_mode_request_interceptor.js

@@ -108,11 +105,11 @@ describe('DashboardOnlyModeRequestInterceptor', () => {
  describe('request has correct auth scope scope', () => {


nit: test title is outdated

not related: I've noticed we also reference to auth scope from comment in x-pack/plugins/spaces/server/lib/request_inteceptors/on_post_auth_interceptor.ts, probably some left over.

azasypkin · 2019-05-29T09:55:40Z

x-pack/plugins/dashboard_mode/server/dashboard_mode_request_interceptor.js

    type: 'onPostAuth',
    async method(request, h) {
      const { auth, url } = request;
+      const user = auth.credentials;


The act of retrieving this setting ends up creating the config document if it doesn't already exist. Various functional tests have come to indirectly rely on this behavior, so I've opted to maintain that in this PR. I had originally hoped to optimize the existing implementation by not querying the UI Settings Service unless strictly necessary, but I wasn't able to do so without also revisiting all of the existing functional tests. I vote to work in that when we remove dashboard-only-mode altogether.

That's a bummer :/ How many (approximately) of such weird tests we have? I'm fine with leaving it as is, just curious.

azasypkin · 2019-05-29T10:01:30Z

x-pack/plugins/dashboard_mode/server/dashboard_mode_request_interceptor.js

+      const uiSettings = request.getUiSettingsService();
+      const dashboardOnlyModeRoles = await uiSettings.get(CONFIG_DASHBOARD_ONLY_MODE_ROLES);
+
+      if (!isAppRequest || !dashboardOnlyModeRoles || !roles || roles.length === 0) {


optional nit: I'd say the perf impact of find on roles is negligible comparing to async uiSettings.get call, so maybe there is no need to have this additional return path (and assuming we'll do roles check in the if above) and always treat this advanced setting a defined array.

const dashboardOnlyModeRoles = await uiSettings.get(CONFIG_DASHBOARD_ONLY_MODE_ROLES) || [];

nit: also can you please leave a comment in code explaining why we always call uiSettings.get, so that we don't forget that?

…ve-auth-scopes

elasticmachine · 2019-05-29T12:39:45Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-05-29T14:31:16Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* remove auth scope provider * handle missing roles * guard for unauthenticated calls * update functional tests to not expect a scope property * there's always money in the banana stand * revert interceptor optimizations * protect against missing roles * address pr feedback * remove scope as expected property on kerberos auth response

remove auth scope provider

3532524

legrego commented May 23, 2019

View reviewed changes

legrego added chore release_note:skip Skip the PR/issue when compiling release notes v7.3.0 v8.0.0 labels May 23, 2019

This comment has been minimized.

Sign in to view

handle missing roles

f3bb7cc

This comment has been minimized.

Sign in to view

legrego added 2 commits May 23, 2019 16:58

guard for unauthenticated calls

962fdad

update functional tests to not expect a scope property

caa6fdd

This comment has been minimized.

Sign in to view

legrego added 2 commits May 24, 2019 07:54

there's always money in the banana stand

35a5c7c

Merge branch 'master' of github.com:elastic/kibana into security/remo…

d5c1392

…ve-auth-scopes

legrego added 2 commits May 24, 2019 09:39

Merge branch 'master' of github.com:elastic/kibana into security/remo…

42f9c57

…ve-auth-scopes

revert interceptor optimizations

0b429ad

Merge branch 'master' of github.com:elastic/kibana into security/remo…

39d6757

…ve-auth-scopes

legrego added 2 commits May 24, 2019 15:49

protect against missing roles

909bcfe

Merge branch 'master' of github.com:elastic/kibana into security/remo…

d4a51cf

…ve-auth-scopes

legrego marked this pull request as ready for review May 24, 2019 22:58

legrego requested a review from a team as a code owner May 24, 2019 22:58

legrego added the Team:Security Platform Security: Auth, Users, Roles, Spaces, Audit Logging, etc t// label May 24, 2019

legrego commented May 28, 2019

View reviewed changes

legrego requested a review from azasypkin May 28, 2019 13:50

azasypkin approved these changes May 29, 2019

View reviewed changes

legrego added 2 commits May 29, 2019 06:55

Merge branch 'master' of github.com:elastic/kibana into security/remo…

bf408a2

…ve-auth-scopes

address pr feedback

d6fc719

remove scope as expected property on kerberos auth response

f79257a

legrego merged commit 38eb16d into elastic:master May 29, 2019

legrego mentioned this pull request May 29, 2019

[7.x] Security - remove auth scope provider (#36998) #37339

Merged

legrego deleted the security/remove-auth-scopes branch May 29, 2019 15:04

		@@ -133,11 +132,7 @@ class Authenticator {
		* @param authScope AuthScopeService instance.

		@@ -108,11 +105,11 @@ describe('DashboardOnlyModeRequestInterceptor', () => {
		describe('request has correct auth scope scope', () => {

Conversation

legrego commented May 23, 2019

Summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

elasticmachine commented May 23, 2019

💔 Build Failed

Uh oh!

elasticmachine commented May 24, 2019

💔 Build Failed

Uh oh!

elasticmachine commented May 24, 2019

💔 Build Failed

Uh oh!

elasticmachine commented May 24, 2019

💔 Build Failed

Uh oh!

elasticmachine commented May 24, 2019

💔 Build Failed

Uh oh!

legrego commented May 24, 2019

Uh oh!

elasticmachine commented May 24, 2019

💚 Build Succeeded

Uh oh!

elasticmachine commented May 24, 2019

Uh oh!

legrego May 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azasypkin commented May 28, 2019

Uh oh!

azasypkin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented May 29, 2019

💔 Build Failed

Uh oh!

elasticmachine commented May 29, 2019

💚 Build Succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

legrego May 28, 2019 •

edited

Loading