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.

Sign up for GitHub

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

POC to migrate Code to use Feature Controls #35093

Closed
wants to merge 4 commits into from
Closed

POC to migrate Code to use Feature Controls #35093

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
48af57b
rough POC to migrate Code to use Feature Controls
legrego Apr 15, 2019
12f8fd6
update security api tests
legrego Apr 15, 2019
d1559bc
Merge branch 'feature/merge-code' into fc/code
legrego Apr 15, 2019
40b14b9
fix tests
legrego Apr 15, 2019
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
18 changes: 4 additions & 14 deletions x-pack/plugins/code/public/reducers/user.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

import produce from 'immer';
import { Action, handleActions } from 'redux-actions';
import { uiCapabilities } from 'ui/capabilities';

import { loadUserProfile, loadUserProfileFailed, loadUserProfileSuccess } from '../actions';

Expand All @@ -16,8 +17,8 @@ export interface UserProfileState {
}

const initialState: UserProfileState = {
isCodeAdmin: false,
isCodeUser: false,
isCodeAdmin: Boolean(uiCapabilities.code.admin),
isCodeUser: Boolean(uiCapabilities.code.user),
};

export const userProfile = handleActions(
Copy link
Member Author

Choose a reason for hiding this comment

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

It's likely that you don't need any of these userProfile actions anymore. I left it in place because I didn't feel comfortable refactoring. All you essentially need is the initialState above, however you wish to incorporate that into the application.

Expand All @@ -28,18 +29,7 @@ export const userProfile = handleActions(
}),
[String(loadUserProfileSuccess)]: (state: UserProfileState, action: Action<any>) =>
produce<UserProfileState>(state, draft => {
if (action.payload!.roles) {
// If security is enabled and the roles field is set. Then we should check the
// 'code_admin' and 'code_user' roles.
draft.isCodeAdmin =
action.payload!.roles.includes('code_admin') ||
// 'superuser' should be deemed as code admin user as well.
action.payload!.roles.includes('superuser');
draft.isCodeUser = action.payload!.roles.includes('code_user');
} else {
// If security is not enabled, then every user is code admin.
draft.isCodeAdmin = true;
}
return state; // no-op, this can be removed
}),
[String(loadUserProfileFailed)]: (state: UserProfileState, action: Action<any>) => {
if (action.payload) {
Expand Down
32 changes: 32 additions & 0 deletions x-pack/plugins/code/server/init.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
*/

import { Server } from 'hapi';
import { i18n } from '@kbn/i18n';
import fetch from 'node-fetch';
import { XPackMainPlugin } from '../../xpack_main/xpack_main';
import { checkRepos } from './check_repos';
import { LspIndexerFactory, RepositoryIndexInitializerFactory, tryMigrateIndices } from './indexer';
import { EsClient, Esqueue } from './lib/esqueue';
Expand Down Expand Up @@ -77,6 +79,36 @@ async function getCodeNodeUuid(url: string, log: Logger) {
export function init(server: Server, options: any) {
const log = new Logger(server);
const serverOptions = new ServerOptions(options, server.config());
const xpackMainPlugin: XPackMainPlugin = server.plugins.xpack_main;
xpackMainPlugin.registerFeature({
id: 'code',
name: i18n.translate('xpack.code.featureRegistry.codeFeatureName', {
defaultMessage: 'Code',
}),
icon: 'codeApp',
navLinkId: 'code',
app: ['code', 'kibana'],
catalogue: [],
Copy link
Member Author

Choose a reason for hiding this comment

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

If you add an entry to the Feature Catalogue, then you should add the ID of that entry to this array. This catalogue is what populates the Kibana Home page with the various features that users can enjoy.

privileges: {
all: {
api: ['code_user', 'code_admin'],
Copy link
Member Author

Choose a reason for hiding this comment

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

This grants users with the all privilege the ability to execute any API endpoints that are tagged with either access:code_user or access:code_admin

savedObject: {
all: [],
read: ['config'],
},
ui: ['show', 'user', 'admin'],
},
read: {
api: ['code_user'],
Copy link
Member Author

Choose a reason for hiding this comment

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

This grants users with the all privilege the ability to execute any API endpoints that are tagged with access:code_user.

savedObject: {
all: [],
read: ['config'],
},
ui: ['show', 'user'],
},
},
});

// @ts-ignore
const kbnServer = this.kbnServer;
kbnServer.ready().then(async () => {
Expand Down
6 changes: 1 addition & 5 deletions x-pack/plugins/code/server/security.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
import { Server } from 'hapi';
// @ts-ignore
import HapiAuthCookie from 'hapi-auth-cookie';
import sinon from 'sinon';
import { SecureRoute } from './security';

const createMockServer = async () => {
Expand All @@ -26,9 +25,6 @@ const createMockServer = async () => {

const secureRoute = new SecureRoute(server);
secureRoute.install();
// @ts-ignore
const stub = sinon.stub(secureRoute, 'isSecurityEnabledInEs');
stub.returns(true);
server.securedRoute({
method: 'GET',
path: '/test',
Expand Down Expand Up @@ -66,7 +62,7 @@ async function checkWithRoles(server: any, roles: any) {
return response;
}

it('should response 403 when roles check failed', async () => {
it.skip('should response 403 when roles check failed', async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Skipping this test, but this entire suite should be revisited.

const server = await createMockServer();
const response = await checkWithRoles(server, []);
expect(response.statusCode).toBe(403);
Expand Down
55 changes: 9 additions & 46 deletions x-pack/plugins/code/server/security.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,12 @@
* you may not use this file except in compliance with the Elastic License.
*/

import Boom from 'boom';
import { Lifecycle, Request, ResponseToolkit, Server, ServerRoute } from 'hapi';
import { Server, ServerRoute, RouteOptions } from 'hapi';

export interface SecuredRoute extends ServerRoute {
requireRoles?: string[];
requireAdmin?: boolean;
}

export const ADMIN_ROLE = 'code_admin';

declare module 'hapi' {
interface Server {
securedRoute(route: SecuredRoute): void;
Expand All @@ -26,56 +22,23 @@ export class SecureRoute {
public install() {
const self = this;
function securedRoute(route: SecuredRoute) {
if (route.handler) {
const originHandler = route.handler as Lifecycle.Method;
route.handler = async (req: Request, h: ResponseToolkit, err?: Error) => {
if (self.isSecurityEnabledInEs()) {
let requiredRoles = route.requireRoles || [];
if (route.requireAdmin) {
requiredRoles = requiredRoles.concat([ADMIN_ROLE]);
}
if (requiredRoles.length > 0) {
if (!req.auth.isAuthenticated) {
throw Boom.unauthorized('not login.');
} else {
// @ts-ignore
const userRoles = new Set(req.auth.credentials.roles || []);
const authorized =
userRoles.has('superuser') ||
requiredRoles.every((value: string) => userRoles.has(value));
if (!authorized) {
throw Boom.forbidden('not authorized user.');
}
}
}
}
return await originHandler(req, h, err);
};
}
const routeOptions: RouteOptions = (route.options || {}) as RouteOptions;
routeOptions.tags = [
...(routeOptions.tags || []),
`access:code_${route.requireAdmin ? 'admin' : 'user'}`,
Copy link
Member Author

Choose a reason for hiding this comment

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

Tags the route with either access:code_user or access:code_admin depending on if requireAdmin is set on the route.

];

self.server.route({
handler: route.handler,
method: route.method,
options: route.options,
options: routeOptions,
path: route.path,
});
}

// FIXME: don't attach to the server this way. Use server.decorate or similar.
this.server.securedRoute = securedRoute;
}

private isSecurityEnabledInEs() {
const xpackInfo = this.server.plugins.xpack_main.info;
if (!xpackInfo.isAvailable()) {
throw Boom.serverUnavailable('x-pack info is not available yet.');
}
if (
!xpackInfo.feature('security').isEnabled() ||
!xpackInfo.feature('security').isAvailable()
) {
return false;
}
return true;
}
}

export function enableSecurity(server: Server) {
Expand Down
6 changes: 3 additions & 3 deletions x-pack/plugins/code/server/utils/with_request.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ export class WithRequest {
public readonly callWithRequest: (endpoint: string, clientOptions?: AnyObject) => Promise<any>;

constructor(readonly req: Request) {
this.callWithRequest = req.server.plugins.elasticsearch
.getCluster('data')
.callWithRequest.bind(null, req);
// FIXME: I swapped out `callWithRequest` for `callWithInternalUser` to test and verify functionality, but you'll want to refactor this since
// the names no longer make sense.
this.callWithRequest = req.server.plugins.elasticsearch.getCluster('data').callWithInternalUser;
Copy link
Member Author

Choose a reason for hiding this comment

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

Important

In order to test my changes, I simply changed the call being made here from callWithRequest to callWithInternalUser. I assume that we want this in most places, but I'm not sure if there are legitimate cases for callWithRequest -- Do you need to query any user indices, or are you only querying .code* indices?

Either way, this will need refactoring, since it is no longer "WithRequest".

A couple other notes:

  1. The kibana_system role has access to .code-*. Are there any other indices it needs access to in order for Code to function correctly?
  2. The code_admin and code_user roles that were introduced here will need to be removed. They are no longer needed, and we shouldn't confuse users with those options.

Copy link
Member Author

Choose a reason for hiding this comment

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

Update: We should not blindly use callWithInternalUser. Instead, Code should check if the Kibana security plugin is available, and if it is, use its exposed authorization mode to determine what to do:

const securityPlugin = server.plugins.security;
if (securityPlugin) {
  const useRbac = securityPlugin.authorization.mode.useRbacForRequest(request);
  if (useRbac) {
    // use `callWithInternalUser`
  } else {
    // use `callWithRequest`
  }
}

}
}
58 changes: 58 additions & 0 deletions x-pack/test/api_integration/apis/security/privileges.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -498,6 +498,39 @@ export default function({ getService }: KibanaFunctionalTestDefaultProviders) {
`ui:${version}:apm/show`,
],
},
code: {
all: [
'login:',
`version:${version}`,
`api:${version}:code_user`,
`api:${version}:code_admin`,
`app:${version}:code`,
`app:${version}:kibana`,
`ui:${version}:navLinks/code`,
`saved_object:${version}:config/bulk_get`,
`saved_object:${version}:config/get`,
`saved_object:${version}:config/find`,
`ui:${version}:savedObjectsManagement/config/read`,
`ui:${version}:code/show`,
`ui:${version}:code/user`,
`ui:${version}:code/admin`,
'allHack:',
],
read: [
'login:',
`version:${version}`,
`api:${version}:code_user`,
`app:${version}:code`,
`app:${version}:kibana`,
`ui:${version}:navLinks/code`,
`saved_object:${version}:config/bulk_get`,
`saved_object:${version}:config/get`,
`saved_object:${version}:config/find`,
`ui:${version}:savedObjectsManagement/config/read`,
`ui:${version}:code/show`,
`ui:${version}:code/user`,
],
},
maps: {
all: [
'login:',
Expand Down Expand Up @@ -853,6 +886,13 @@ export default function({ getService }: KibanaFunctionalTestDefaultProviders) {
`ui:${version}:catalogue/apm`,
`ui:${version}:navLinks/apm`,
`ui:${version}:apm/show`,
`api:${version}:code_user`,
`api:${version}:code_admin`,
`app:${version}:code`,
`ui:${version}:navLinks/code`,
`ui:${version}:code/show`,
`ui:${version}:code/user`,
`ui:${version}:code/admin`,
`app:${version}:maps`,
`ui:${version}:catalogue/maps`,
`ui:${version}:navLinks/maps`,
Expand Down Expand Up @@ -973,6 +1013,11 @@ export default function({ getService }: KibanaFunctionalTestDefaultProviders) {
`ui:${version}:catalogue/apm`,
`ui:${version}:navLinks/apm`,
`ui:${version}:apm/show`,
`api:${version}:code_user`,
`app:${version}:code`,
`ui:${version}:navLinks/code`,
`ui:${version}:code/show`,
`ui:${version}:code/user`,
`app:${version}:maps`,
`ui:${version}:catalogue/maps`,
`ui:${version}:navLinks/maps`,
Expand Down Expand Up @@ -1132,6 +1177,13 @@ export default function({ getService }: KibanaFunctionalTestDefaultProviders) {
`ui:${version}:catalogue/apm`,
`ui:${version}:navLinks/apm`,
`ui:${version}:apm/show`,
`api:${version}:code_user`,
`api:${version}:code_admin`,
`app:${version}:code`,
`ui:${version}:navLinks/code`,
`ui:${version}:code/show`,
`ui:${version}:code/user`,
`ui:${version}:code/admin`,
`app:${version}:maps`,
`ui:${version}:catalogue/maps`,
`ui:${version}:navLinks/maps`,
Expand Down Expand Up @@ -1252,6 +1304,11 @@ export default function({ getService }: KibanaFunctionalTestDefaultProviders) {
`ui:${version}:catalogue/apm`,
`ui:${version}:navLinks/apm`,
`ui:${version}:apm/show`,
`api:${version}:code_user`,
`app:${version}:code`,
`ui:${version}:navLinks/code`,
`ui:${version}:code/show`,
`ui:${version}:code/user`,
`app:${version}:maps`,
`ui:${version}:catalogue/maps`,
`ui:${version}:navLinks/maps`,
Expand Down Expand Up @@ -1329,6 +1386,7 @@ export default function({ getService }: KibanaFunctionalTestDefaultProviders) {
logs: ['all', 'read'],
uptime: ['all', 'read'],
apm: ['all', 'read'],
code: ['all', 'read'],
},
global: ['all', 'read'],
space: ['all', 'read'],
Expand Down
1 change: 1 addition & 0 deletions x-pack/test/api_integration/apis/xpack_main/features/features.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ export default function({ getService }: KibanaFunctionalTestDefaultProviders) {
'ml',
'apm',
'canvas',
'code',
'infrastructure',
'logs',
'maps',
Expand Down
55 changes: 37 additions & 18 deletions x-pack/test/functional/apps/code/with_security.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,31 +19,50 @@ export default function testWithSecurity({ getService, getPageObjects }: TestInv
const repositoryListSelector = 'codeRepositoryList codeRepositoryItem';
const manageButtonSelectors = ['indexRepositoryButton', 'deleteRepositoryButton'];
const log = getService('log');
const security = getService('security');

describe('with security enabled:', () => {
before(async () => {
await esArchiver.load('empty_kibana');
await PageObjects.settings.navigateTo();
await PageObjects.security.clickElasticsearchUsers();
await PageObjects.security.addUser({
username: codeAdmin,
await security.role.create('global_code_all_role', {
elasticsearch: {
indices: [],
},
kibana: [
{
feature: {
code: ['all'],
},
spaces: ['*'],
},
],
});

await security.user.create(codeAdmin, {
password: dummyPassword,
confirmPassword: dummyPassword,
fullname: 'Code Admin',
email: 'codeAdmin@elastic.co',
save: true,
roles: ['kibana_user', 'code_admin'],
roles: ['global_code_all_role'],
full_name: 'code admin',
});

await security.role.create('global_code_read_role', {
elasticsearch: {
indices: [],
},
kibana: [
{
feature: {
code: ['read'],
},
spaces: ['*'],
},
],
});
await PageObjects.security.addUser({
username: codeUser,
password: '123321',
confirmPassword: dummyPassword,
fullname: 'Code User',
email: 'codeUser@elastic.co',
save: true,
roles: ['kibana_user', 'code_user'],

await security.user.create(codeUser, {
password: dummyPassword,
roles: ['global_code_read_role'],
full_name: 'code user',
});
// Navigate to the search page of the code app.
});

async function login(user: string) {
Expand Down