Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Security - remove auth scope provider #36998

Merged
merged 14 commits into from
May 29, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion x-pack/plugins/dashboard_mode/common/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,3 @@
*/

export const CONFIG_DASHBOARD_ONLY_MODE_ROLES = 'xpackDashboardMode:roles';
export const AUTH_SCOPE_DASHBORD_ONLY_MODE = 'xpack:dashboardMode';
4 changes: 0 additions & 4 deletions x-pack/plugins/dashboard_mode/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import {
} from './common';

import {
getDashboardModeAuthScope,
createDashboardModeRequestInterceptor,
} from './server';

Expand Down Expand Up @@ -80,9 +79,6 @@ export function dashboardMode(kibana) {
));

if (server.plugins.security) {
// register auth getter with security plugin
server.plugins.security.registerAuthScopeGetter(getDashboardModeAuthScope);

// extend the server to intercept requests
const dashboardViewerApp = server.getHiddenUiAppById('dashboardViewer');
server.ext(createDashboardModeRequestInterceptor(dashboardViewerApp));
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -6,27 +6,24 @@

import expect from '@kbn/expect';
import Hapi from 'hapi';
import Chance from 'chance';

import { createDashboardModeRequestInterceptor } from '../dashboard_mode_request_interceptor';
import * as constantsNS from '../../common/constants';

const chance = new Chance();
const DASHBOARD_ONLY_MODE_ROLE = 'test_dashboard_only_mode_role';

function setup() {
// randomize AUTH_SCOPE_DASHBORD_ONLY_MODE "constant" to ensure it's being used everywhere
const authScope = chance.word({ length: 12 });
Object.defineProperty(constantsNS, 'AUTH_SCOPE_DASHBORD_ONLY_MODE', {
value: authScope,
configurable: true,
});

const dashboardViewerApp = {
name: 'dashboardViewerApp'
};

const server = new Hapi.Server();

server.decorate('request', 'getUiSettingsService', () => {
return {
get: () => Promise.resolve([DASHBOARD_ONLY_MODE_ROLE])
};
});

// attach the extension
server.ext(createDashboardModeRequestInterceptor(dashboardViewerApp));

Expand All @@ -53,7 +50,7 @@ function setup() {
}
});

return { server, authScope };
return { server };
}

describe('DashboardOnlyModeRequestInterceptor', () => {
Expand All @@ -65,14 +62,14 @@ describe('DashboardOnlyModeRequestInterceptor', () => {
});
});

describe('request does not have `xpack:dashboardMode` scope', () => {
describe('request is not for dashboad-only user', () => {
describe('app route', () => {
it('lets the route render as normal', async () => {
const { server } = setup();
const response = await server.inject({
url: '/app/kibana',
credentials: {
scope: ['foo', 'bar']
roles: ['foo', 'bar']
}
});

Expand All @@ -91,7 +88,7 @@ describe('DashboardOnlyModeRequestInterceptor', () => {
const response = await server.inject({
url: '/foo/bar',
credentials: {
scope: ['foo', 'bar']
roles: ['foo', 'bar']
}
});

Expand All @@ -105,14 +102,14 @@ describe('DashboardOnlyModeRequestInterceptor', () => {
});
});

describe('request has correct auth scope scope', () => {
describe('request for dashboard-only user', () => {
describe('non-kibana app route', () => {
it('responds with 404', async () => {
const { server, authScope } = setup();
const { server } = setup();
const response = await server.inject({
url: '/app/foo',
credentials: {
scope: [authScope]
roles: [DASHBOARD_ONLY_MODE_ROLE]
}
});

Expand All @@ -124,11 +121,11 @@ describe('DashboardOnlyModeRequestInterceptor', () => {
function testRendersDashboardViewerApp(url) {
describe(`requests to url:"${url}"`, () => {
it('renders the dashboardViewerApp instead', async () => {
const { server, authScope } = setup();
const { server } = setup();
const response = await server.inject({
url: '/app/kibana',
credentials: {
scope: [authScope]
roles: [DASHBOARD_ONLY_MODE_ROLE]
}
});

Expand Down
39 changes: 0 additions & 39 deletions x-pack/plugins/dashboard_mode/server/dashboard_mode_auth_scope.js

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,14 @@
import Boom from 'boom';

import {
AUTH_SCOPE_DASHBORD_ONLY_MODE
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

CONFIG_DASHBOARD_ONLY_MODE_ROLES,
} from '../common';

const superuserRole = 'superuser';

/**
* Intercept all requests after auth has completed and apply filtering
* logic to enforce `xpack:dashboardMode` scope
* logic to enforce dashboard only mode.
*
* @type {Hapi.RequestExtension}
*/
Expand All @@ -25,14 +27,34 @@ export function createDashboardModeRequestInterceptor(dashboardViewerApp) {
type: 'onPostAuth',
async method(request, h) {
const { auth, url } = request;
const user = auth.credentials;
Copy link
Member Author

@legrego legrego May 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const roles = user ? user.roles : [];

if (!user) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return h.continue;
}

const isAppRequest = url.path.startsWith('/app/');

if (isAppRequest && auth.credentials.scope && auth.credentials.scope.includes(AUTH_SCOPE_DASHBORD_ONLY_MODE)) {
// 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 changing this is non-trivial.
// This will be addressed once dashboard-only-mode is removed altogether.
const uiSettings = request.getUiSettingsService();
const dashboardOnlyModeRoles = await uiSettings.get(CONFIG_DASHBOARD_ONLY_MODE_ROLES);

if (!isAppRequest || !dashboardOnlyModeRoles || !roles || roles.length === 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

return h.continue;
}

const isDashboardOnlyModeUser = user.roles.find(role => dashboardOnlyModeRoles.includes(role));
const isSuperUser = user.roles.find(role => role === superuserRole);

const enforceDashboardOnlyMode = isDashboardOnlyModeUser && !isSuperUser;
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ha, good catch, will remove

// app is the only one ever rendered.
// Read more about Dashboard Only Mode here: https://github.com/elastic/x-pack-kibana/issues/180
const response = await h.renderApp(dashboardViewerApp);
return response.takeover();
}
Expand Down
1 change: 0 additions & 1 deletion x-pack/plugins/dashboard_mode/server/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,4 @@
* you may not use this file except in compliance with the Elastic License.
*/

export { getDashboardModeAuthScope } from './dashboard_mode_auth_scope';
export { createDashboardModeRequestInterceptor } from './dashboard_mode_request_interceptor';
Loading