From c7d5f38a9ee5d58c7c7a88fe2595e243b831f085 Mon Sep 17 00:00:00 2001 From: Catherine Liu Date: Thu, 25 Oct 2018 22:39:05 -0700 Subject: [PATCH 1/5] Fixes check for security plugin --- .../canvas/server/lib/create_handlers.js | 3 ++- .../plugins/canvas/server/lib/feature_check.js | 17 +++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) create mode 100644 x-pack/plugins/canvas/server/lib/feature_check.js diff --git a/x-pack/plugins/canvas/server/lib/create_handlers.js b/x-pack/plugins/canvas/server/lib/create_handlers.js index 01a9c6adfb745..a679f354cdca6 100644 --- a/x-pack/plugins/canvas/server/lib/create_handlers.js +++ b/x-pack/plugins/canvas/server/lib/create_handlers.js @@ -5,6 +5,7 @@ */ import boom from 'boom'; +import { isSecurityEnabled } from './feature_check'; export const createHandlers = (request, server) => { const { callWithRequest } = server.plugins.elasticsearch.getCluster('data'); @@ -19,7 +20,7 @@ export const createHandlers = (request, server) => { httpHeaders: request.headers, elasticsearchClient: async (...args) => { // check if the session is valid because continuing to use it - if (server.plugins.security) { + if (isSecurityEnabled(server)) { const authenticationResult = await server.plugins.security.authenticate(request); if (!authenticationResult.succeeded()) throw boom.unauthorized(authenticationResult.error); } diff --git a/x-pack/plugins/canvas/server/lib/feature_check.js b/x-pack/plugins/canvas/server/lib/feature_check.js new file mode 100644 index 0000000000000..0783bdf16605b --- /dev/null +++ b/x-pack/plugins/canvas/server/lib/feature_check.js @@ -0,0 +1,17 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +export const isSecurityEnabled = server => { + const securityPlugin = server.plugins.security; + const xpackInfo = server.plugins.xpack_main.info; + + return ( + securityPlugin && + xpackInfo.isAvailable() && + xpackInfo.feature('security').isEnabled() && + !xpackInfo.license.isOneOf('basic') + ); +}; From 86dad6372de154fde69f13da89db42d680019014 Mon Sep 17 00:00:00 2001 From: Catherine Liu Date: Fri, 26 Oct 2018 14:33:23 -0700 Subject: [PATCH 2/5] Cleaned up security check logic. Fixed tests for create_handlers. Added TODOs --- .../canvas/server/lib/__tests__/create_handlers.js | 9 +++++++++ x-pack/plugins/canvas/server/lib/create_handlers.js | 1 + x-pack/plugins/canvas/server/lib/feature_check.js | 11 +++-------- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/x-pack/plugins/canvas/server/lib/__tests__/create_handlers.js b/x-pack/plugins/canvas/server/lib/__tests__/create_handlers.js index f3a92b573680c..ad3c9c57d5ec5 100644 --- a/x-pack/plugins/canvas/server/lib/__tests__/create_handlers.js +++ b/x-pack/plugins/canvas/server/lib/__tests__/create_handlers.js @@ -27,6 +27,15 @@ const mockServer = { callWithRequest: (...args) => Promise.resolve(args), }), }, + // TODO: remove this when we use the method exposed by security https://github.com/elastic/kibana/pull/24616 + xpack_main: { + info: { + feature: () => ({ + isAvailable: () => true, + isEnabled: () => true, + }), + }, + }, }, config: () => ({ has: () => false, diff --git a/x-pack/plugins/canvas/server/lib/create_handlers.js b/x-pack/plugins/canvas/server/lib/create_handlers.js index a679f354cdca6..9297fbd88d2e0 100644 --- a/x-pack/plugins/canvas/server/lib/create_handlers.js +++ b/x-pack/plugins/canvas/server/lib/create_handlers.js @@ -20,6 +20,7 @@ export const createHandlers = (request, server) => { httpHeaders: request.headers, elasticsearchClient: async (...args) => { // check if the session is valid because continuing to use it + // TODO: remove this when we use the method exposed by security https://github.com/elastic/kibana/pull/24616 if (isSecurityEnabled(server)) { const authenticationResult = await server.plugins.security.authenticate(request); if (!authenticationResult.succeeded()) throw boom.unauthorized(authenticationResult.error); diff --git a/x-pack/plugins/canvas/server/lib/feature_check.js b/x-pack/plugins/canvas/server/lib/feature_check.js index 0783bdf16605b..a752f2520e25a 100644 --- a/x-pack/plugins/canvas/server/lib/feature_check.js +++ b/x-pack/plugins/canvas/server/lib/feature_check.js @@ -5,13 +5,8 @@ */ export const isSecurityEnabled = server => { - const securityPlugin = server.plugins.security; - const xpackInfo = server.plugins.xpack_main.info; + const kibanaSecurity = server.plugins.security; + const esSecurity = server.plugins.xpack_main.info.feature('security'); - return ( - securityPlugin && - xpackInfo.isAvailable() && - xpackInfo.feature('security').isEnabled() && - !xpackInfo.license.isOneOf('basic') - ); + return kibanaSecurity && esSecurity.isAvailable() && esSecurity.isEnabled(); }; From dea410cbc0dff99622f6ab341d6b2314bc405082 Mon Sep 17 00:00:00 2001 From: Catherine Liu Date: Fri, 26 Oct 2018 14:41:38 -0700 Subject: [PATCH 3/5] Updated comment --- x-pack/plugins/canvas/server/lib/create_handlers.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugins/canvas/server/lib/create_handlers.js b/x-pack/plugins/canvas/server/lib/create_handlers.js index 9297fbd88d2e0..75e5679dc1e5a 100644 --- a/x-pack/plugins/canvas/server/lib/create_handlers.js +++ b/x-pack/plugins/canvas/server/lib/create_handlers.js @@ -20,7 +20,7 @@ export const createHandlers = (request, server) => { httpHeaders: request.headers, elasticsearchClient: async (...args) => { // check if the session is valid because continuing to use it - // TODO: remove this when we use the method exposed by security https://github.com/elastic/kibana/pull/24616 + // TODO: replace this when we use the method exposed by security https://github.com/elastic/kibana/pull/24616 if (isSecurityEnabled(server)) { const authenticationResult = await server.plugins.security.authenticate(request); if (!authenticationResult.succeeded()) throw boom.unauthorized(authenticationResult.error); From 8efc212670fcf7521edab11261a83e5125777d62 Mon Sep 17 00:00:00 2001 From: Catherine Liu Date: Fri, 26 Oct 2018 15:33:22 -0700 Subject: [PATCH 4/5] Added tests --- .../server/lib/__tests__/create_handlers.js | 46 +++++++++++++++++-- 1 file changed, 43 insertions(+), 3 deletions(-) diff --git a/x-pack/plugins/canvas/server/lib/__tests__/create_handlers.js b/x-pack/plugins/canvas/server/lib/__tests__/create_handlers.js index ad3c9c57d5ec5..9c49e798cb211 100644 --- a/x-pack/plugins/canvas/server/lib/__tests__/create_handlers.js +++ b/x-pack/plugins/canvas/server/lib/__tests__/create_handlers.js @@ -8,6 +8,8 @@ import expect from 'expect.js'; import { createHandlers } from '../create_handlers'; let securityMode = 'pass'; +let license = 'trial'; +let securityEnabled = true; const authError = new Error('auth error'); const mockRequest = { @@ -31,8 +33,8 @@ const mockServer = { xpack_main: { info: { feature: () => ({ - isAvailable: () => true, - isEnabled: () => true, + isAvailable: () => license !== 'basic', + isEnabled: () => securityEnabled, }), }, }, @@ -51,6 +53,8 @@ describe('server createHandlers', () => { beforeEach(() => { securityMode = 'pass'; + securityEnabled = true; + license = 'trial'; handlers = createHandlers(mockRequest, mockServer); }); @@ -84,7 +88,7 @@ describe('server createHandlers', () => { }); }); - it('works without security', async () => { + it('works without security plugin in kibana', async () => { // create server without security plugin const mockServerClone = { ...mockServer, @@ -107,5 +111,41 @@ describe('server createHandlers', () => { expect(endpoint).to.equal('endpoint'); expect(payload).to.equal('payload'); }); + + it('works without security on basic license', async () => { + // create server with a basic license + license = 'basic'; + + // this shouldn't do anything + securityMode = 'fail'; + + // make sure the method still works + handlers = createHandlers(mockRequest, mockServer); + const [request, endpoint, payload] = await handlers.elasticsearchClient( + 'endpoint', + 'payload' + ); + expect(request).to.equal(mockRequest); + expect(endpoint).to.equal('endpoint'); + expect(payload).to.equal('payload'); + }); + + it('works with security disabled in elasticsearch', async () => { + // create server with security disabled + securityEnabled = false; + + // this shouldn't do anything + securityMode = 'fail'; + + // make sure the method still works + handlers = createHandlers(mockRequest, mockServer); + const [request, endpoint, payload] = await handlers.elasticsearchClient( + 'endpoint', + 'payload' + ); + expect(request).to.equal(mockRequest); + expect(endpoint).to.equal('endpoint'); + expect(payload).to.equal('payload'); + }); }); }); From c4017392764294b92c755ab24b8758892b156d29 Mon Sep 17 00:00:00 2001 From: Catherine Liu Date: Mon, 29 Oct 2018 12:05:54 -0700 Subject: [PATCH 5/5] Updated variable names --- .../server/lib/__tests__/create_handlers.js | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/x-pack/plugins/canvas/server/lib/__tests__/create_handlers.js b/x-pack/plugins/canvas/server/lib/__tests__/create_handlers.js index 9c49e798cb211..d55bd49c983cc 100644 --- a/x-pack/plugins/canvas/server/lib/__tests__/create_handlers.js +++ b/x-pack/plugins/canvas/server/lib/__tests__/create_handlers.js @@ -8,8 +8,8 @@ import expect from 'expect.js'; import { createHandlers } from '../create_handlers'; let securityMode = 'pass'; -let license = 'trial'; -let securityEnabled = true; +let isSecurityAvailable = true; +let isSecurityEnabled = true; const authError = new Error('auth error'); const mockRequest = { @@ -33,8 +33,8 @@ const mockServer = { xpack_main: { info: { feature: () => ({ - isAvailable: () => license !== 'basic', - isEnabled: () => securityEnabled, + isAvailable: () => isSecurityAvailable, + isEnabled: () => isSecurityEnabled, }), }, }, @@ -53,8 +53,8 @@ describe('server createHandlers', () => { beforeEach(() => { securityMode = 'pass'; - securityEnabled = true; - license = 'trial'; + isSecurityEnabled = true; + isSecurityAvailable = true; handlers = createHandlers(mockRequest, mockServer); }); @@ -112,9 +112,9 @@ describe('server createHandlers', () => { expect(payload).to.equal('payload'); }); - it('works without security on basic license', async () => { - // create server with a basic license - license = 'basic'; + it('works without security available', async () => { + // create server with security unavailable (i.e. when user is on a basic license) + isSecurityAvailable = false; // this shouldn't do anything securityMode = 'fail'; @@ -132,7 +132,7 @@ describe('server createHandlers', () => { it('works with security disabled in elasticsearch', async () => { // create server with security disabled - securityEnabled = false; + isSecurityEnabled = false; // this shouldn't do anything securityMode = 'fail';