From 9c9b55100ea3fadb155b3e582b8ff83a03a30066 Mon Sep 17 00:00:00 2001 From: spalger Date: Tue, 15 Nov 2016 18:56:38 -0700 Subject: [PATCH 1/3] [server/uiSettings+shortUrl] surface errors from es --- .../lib/__tests__/get_basic_auth_realm.js | 27 --- .../elasticsearch/lib/call_with_request.js | 22 +- .../elasticsearch/lib/get_basic_auth_realm.js | 7 - .../routes/api/settings/register_delete.js | 6 +- .../routes/api/settings/register_get.js | 4 +- .../routes/api/settings/register_set.js | 6 +- .../routes/api/settings/register_set_many.js | 6 +- .../timelion/server/routes/run.js | 67 +++--- .../timelion/server/routes/validate_es.js | 2 +- src/server/http/__tests__/short_url_error.js | 32 +++ src/server/http/index.js | 9 +- src/server/http/short_url_error.js | 9 + src/server/http/short_url_lookup.js | 30 +-- src/ui/index.js | 66 +++--- src/ui/public/config/config.js | 7 +- src/ui/settings/__tests__/index.js | 217 ++++++++++++------ src/ui/settings/index.js | 80 ++++--- 17 files changed, 345 insertions(+), 252 deletions(-) delete mode 100644 src/core_plugins/elasticsearch/lib/__tests__/get_basic_auth_realm.js delete mode 100644 src/core_plugins/elasticsearch/lib/get_basic_auth_realm.js create mode 100644 src/server/http/__tests__/short_url_error.js create mode 100644 src/server/http/short_url_error.js diff --git a/src/core_plugins/elasticsearch/lib/__tests__/get_basic_auth_realm.js b/src/core_plugins/elasticsearch/lib/__tests__/get_basic_auth_realm.js deleted file mode 100644 index 7dc84e75730c8..0000000000000 --- a/src/core_plugins/elasticsearch/lib/__tests__/get_basic_auth_realm.js +++ /dev/null @@ -1,27 +0,0 @@ -import getBasicAuthRealm from '../get_basic_auth_realm'; -import expect from 'expect.js'; -const exception = '[security_exception] missing authentication token for REST request [/logstash-*/_search],' + - ' with: {"header":{"WWW-Authenticate":"Basic realm=\\"shield\\""}}'; - - -describe('plugins/elasticsearch', function () { - describe('lib/get_basic_auth_realm', function () { - - it('should return null if passed something other than a string', function () { - expect(getBasicAuthRealm({})).to.be(null); - expect(getBasicAuthRealm(500)).to.be(null); - expect(getBasicAuthRealm([exception])).to.be(null); - }); - - // TODO: This should be updated to match header strings when the client supports that - it('should return the realm when passed an elasticsearch security exception', function () { - expect(getBasicAuthRealm(exception)).to.be('shield'); - }); - - it('should return null when no basic realm information is found', function () { - expect(getBasicAuthRealm('Basically nothing="the universe"')).to.be(null); - }); - - }); -}); - diff --git a/src/core_plugins/elasticsearch/lib/call_with_request.js b/src/core_plugins/elasticsearch/lib/call_with_request.js index 870b856de5fce..ed53ee36ca372 100644 --- a/src/core_plugins/elasticsearch/lib/call_with_request.js +++ b/src/core_plugins/elasticsearch/lib/call_with_request.js @@ -1,14 +1,14 @@ import _ from 'lodash'; import Promise from 'bluebird'; import Boom from 'boom'; -import getBasicAuthRealm from './get_basic_auth_realm'; import toPath from 'lodash/internal/toPath'; import filterHeaders from './filter_headers'; module.exports = (server, client) => { - return (req, endpoint, params = {}) => { + return (req, endpoint, clientParams = {}, options = {}) => { + const wrap401Errors = options.wrap401Errors !== false; const filteredHeaders = filterHeaders(req.headers, server.config().get('elasticsearch.requestHeadersWhitelist')); - _.set(params, 'headers', filteredHeaders); + _.set(clientParams, 'headers', filteredHeaders); const path = toPath(endpoint); const api = _.get(client, path); let apiContext = _.get(client, path.slice(0, -1)); @@ -16,16 +16,16 @@ module.exports = (server, client) => { apiContext = client; } if (!api) throw new Error(`callWithRequest called with an invalid endpoint: ${endpoint}`); - return api.call(apiContext, params) + return api.call(apiContext, clientParams) .catch((err) => { - if (err.status === 401) { - // TODO: The err.message is temporary until we have support for getting headers in the client. - // Once we have that, we should be able to pass the contents of the WWW-Authenticate head to getRealm - const realm = getBasicAuthRealm(err.message) || 'Authorization Required'; - const options = { realm: realm }; - return Promise.reject(Boom.unauthorized('Unauthorized', 'Basic', options)); + if (!wrap401Errors || err.statusCode !== 401) { + return Promise.reject(err); } - return Promise.reject(err); + + const boomError = Boom.wrap(err, err.statusCode); + const wwwAuthHeader = _.get(err, 'body.error.header[WWW-Authenticate]'); + boomError.output.headers['WWW-Authenticate'] = wwwAuthHeader || 'Basic realm="Authorization Required"'; + throw boomError; }); }; }; diff --git a/src/core_plugins/elasticsearch/lib/get_basic_auth_realm.js b/src/core_plugins/elasticsearch/lib/get_basic_auth_realm.js deleted file mode 100644 index 9a9c183def983..0000000000000 --- a/src/core_plugins/elasticsearch/lib/get_basic_auth_realm.js +++ /dev/null @@ -1,7 +0,0 @@ -export default function getBasicAuthRealm(message) { - if (!message || typeof message !== 'string') return null; - - const parts = message.match(/Basic\ realm=\\"(.*)\\"/); - if (parts && parts.length === 2) return parts[1]; - else return null; -}; \ No newline at end of file diff --git a/src/core_plugins/kibana/server/routes/api/settings/register_delete.js b/src/core_plugins/kibana/server/routes/api/settings/register_delete.js index 46c675c9e9650..ea68620298d3d 100644 --- a/src/core_plugins/kibana/server/routes/api/settings/register_delete.js +++ b/src/core_plugins/kibana/server/routes/api/settings/register_delete.js @@ -8,12 +8,12 @@ export default function registerDelete(server) { const { key } = req.params; const uiSettings = server.uiSettings(); uiSettings - .remove(key) + .remove(req, key) .then(() => uiSettings - .getUserProvided() + .getUserProvided(req) .then(settings => reply({ settings }).type('application/json')) ) - .catch(reason => reply(Boom.wrap(reason))); + .catch(err => reply(Boom.wrap(err, err.statusCode))); } }); } diff --git a/src/core_plugins/kibana/server/routes/api/settings/register_get.js b/src/core_plugins/kibana/server/routes/api/settings/register_get.js index 217dc3936430e..85fc8c9ca5f27 100644 --- a/src/core_plugins/kibana/server/routes/api/settings/register_get.js +++ b/src/core_plugins/kibana/server/routes/api/settings/register_get.js @@ -7,9 +7,9 @@ export default function registerGet(server) { handler: function (req, reply) { server .uiSettings() - .getUserProvided() + .getUserProvided(req) .then(settings => reply({ settings }).type('application/json')) - .catch(reason => reply(Boom.wrap(reason))); + .catch(err => reply(Boom.wrap(err, err.statusCode))); } }); } diff --git a/src/core_plugins/kibana/server/routes/api/settings/register_set.js b/src/core_plugins/kibana/server/routes/api/settings/register_set.js index 51dc364c6c2b9..8d572bbd1ab25 100644 --- a/src/core_plugins/kibana/server/routes/api/settings/register_set.js +++ b/src/core_plugins/kibana/server/routes/api/settings/register_set.js @@ -9,12 +9,12 @@ export default function registerSet(server) { const { value } = req.payload; const uiSettings = server.uiSettings(); uiSettings - .set(key, value) + .set(req, key, value) .then(() => uiSettings - .getUserProvided() + .getUserProvided(req) .then(settings => reply({ settings }).type('application/json')) ) - .catch(reason => reply(Boom.wrap(reason))); + .catch(err => reply(Boom.wrap(err, err.statusCode))); } }); } diff --git a/src/core_plugins/kibana/server/routes/api/settings/register_set_many.js b/src/core_plugins/kibana/server/routes/api/settings/register_set_many.js index 7ab2c035de76f..1fe4abe9a9966 100644 --- a/src/core_plugins/kibana/server/routes/api/settings/register_set_many.js +++ b/src/core_plugins/kibana/server/routes/api/settings/register_set_many.js @@ -9,12 +9,12 @@ export default function registerSet(server) { const { changes } = req.payload; const uiSettings = server.uiSettings(); uiSettings - .setMany(changes) + .setMany(req, changes) .then(() => uiSettings - .getUserProvided() + .getUserProvided(req) .then(settings => reply({ settings }).type('application/json')) ) - .catch(reason => reply(Boom.wrap(reason))); + .catch(err => reply(Boom.wrap(err, err.statusCode))); } }); } diff --git a/src/core_plugins/timelion/server/routes/run.js b/src/core_plugins/timelion/server/routes/run.js index 0855b2384959c..d310b20587d08 100644 --- a/src/core_plugins/timelion/server/routes/run.js +++ b/src/core_plugins/timelion/server/routes/run.js @@ -10,56 +10,43 @@ function replyWithError(e, reply) { module.exports = (server) => { - server.route({ method: ['POST', 'GET'], path: '/api/timelion/run', - handler: (request, reply) => { + handler: async (request, reply) => { + try { + const uiSettings = await server.uiSettings().getAll(request); - // I don't really like this, but we need to get all of the settings - // before every request. This just sucks because its going to slow things - // down. Meh. - return server.uiSettings().getAll().then((uiSettings) => { - var sheet; - var tlConfig = require('../handlers/lib/tl_config.js')({ - server: server, - request: request, + const tlConfig = require('../handlers/lib/tl_config.js')({ + server, + request, settings: _.defaults(uiSettings, timelionDefaults) // Just in case they delete some setting. }); - var chainRunner = chainRunnerFn(tlConfig); - - try { - sheet = chainRunner.processRequest(request.payload || { - sheet: [request.query.expression], - time: { - from: request.query.from, - to: request.query.to, - interval: request.query.interval, - timezone: request.query.timezone - } - }); - } catch (e) { - replyWithError(e, reply); - return; - } - return Promise.all(sheet).then((sheet) => { - var response = { - sheet: sheet, - stats: chainRunner.getStats() - }; - reply(response); - }).catch((e) => { - // TODO Maybe we should just replace everywhere we throw with Boom? Probably. - if (e.isBoom) { - reply(e); - } else { - replyWithError(e, reply); + const chainRunner = chainRunnerFn(tlConfig); + const sheet = await Promise.all(chainRunner.processRequest(request.payload || { + sheet: [request.query.expression], + time: { + from: request.query.from, + to: request.query.to, + interval: request.query.interval, + timezone: request.query.timezone } - }); - }); + })); + reply({ + sheet, + stats: chainRunner.getStats() + }); + } catch (err) { + // TODO Maybe we should just replace everywhere we throw with Boom? Probably. + if (err.isBoom) { + reply(err); + } else { + replyWithError(err, reply); + } + } } }); diff --git a/src/core_plugins/timelion/server/routes/validate_es.js b/src/core_plugins/timelion/server/routes/validate_es.js index 482c2fdac4d07..eaa625beca177 100644 --- a/src/core_plugins/timelion/server/routes/validate_es.js +++ b/src/core_plugins/timelion/server/routes/validate_es.js @@ -4,7 +4,7 @@ module.exports = function (server) { path: '/api/timelion/validate/es', handler: function (request, reply) { - return server.uiSettings().getAll().then((uiSettings) => { + return server.uiSettings().getAll(request).then((uiSettings) => { var callWithRequest = server.plugins.elasticsearch.callWithRequest; var timefield = uiSettings['timelion:es.timefield']; diff --git a/src/server/http/__tests__/short_url_error.js b/src/server/http/__tests__/short_url_error.js new file mode 100644 index 0000000000000..cdec714c73622 --- /dev/null +++ b/src/server/http/__tests__/short_url_error.js @@ -0,0 +1,32 @@ +import Boom from 'boom'; +import expect from 'expect.js'; +import _ from 'lodash'; +import { handleShortUrlError } from '../short_url_error'; + +describe('handleShortUrlError()', () => { + const caughtErrors = [{ + status: 401 + }, { + status: 403 + }, { + status: 404 + }]; + + const uncaughtErrors = [{ + status: null + }, { + status: 500 + }]; + + caughtErrors.forEach((err) => { + it(`should handle ${err.status} errors`, function () { + expect(_.get(handleShortUrlError(err), 'output.statusCode')).to.be(err.status); + }); + }); + + uncaughtErrors.forEach((err) => { + it(`should not handle unknown errors`, function () { + expect(_.get(handleShortUrlError(err), 'output.statusCode')).to.be(500); + }); + }); +}); diff --git a/src/server/http/index.js b/src/server/http/index.js index d5e015cc449cc..26358f4d9b30f 100644 --- a/src/server/http/index.js +++ b/src/server/http/index.js @@ -6,6 +6,7 @@ import Boom from 'boom'; import Hapi from 'hapi'; import getDefaultRoute from './get_default_route'; import versionCheckMixin from './version_check'; +import { handleShortUrlError } from './short_url_error'; import { shortUrlAssertValid } from './short_url_assert_valid'; module.exports = async function (kbnServer, server, config) { @@ -114,11 +115,11 @@ module.exports = async function (kbnServer, server, config) { path: '/goto/{urlId}', handler: async function (request, reply) { try { - const url = await shortUrlLookup.getUrl(request.params.urlId); + const url = await shortUrlLookup.getUrl(request.params.urlId, request); shortUrlAssertValid(url); reply().redirect(config.get('server.basePath') + url); } catch (err) { - reply(err); + reply(handleShortUrlError(err)); } } }); @@ -129,10 +130,10 @@ module.exports = async function (kbnServer, server, config) { handler: async function (request, reply) { try { shortUrlAssertValid(request.payload.url); - const urlId = await shortUrlLookup.generateUrlId(request.payload.url); + const urlId = await shortUrlLookup.generateUrlId(request.payload.url, request); reply(urlId); } catch (err) { - reply(err); + reply(handleShortUrlError(err)); } } }); diff --git a/src/server/http/short_url_error.js b/src/server/http/short_url_error.js new file mode 100644 index 0000000000000..8c617a5fbf5fd --- /dev/null +++ b/src/server/http/short_url_error.js @@ -0,0 +1,9 @@ +import Boom from 'boom'; + +export function handleShortUrlError(err) { + if (err.isBoom) return err; + if (err.status === 401) return Boom.unauthorized(); + if (err.status === 403) return Boom.forbidden(); + if (err.status === 404) return Boom.notFound(); + return Boom.badImplementation(); +} diff --git a/src/server/http/short_url_lookup.js b/src/server/http/short_url_lookup.js index 2f0d3b3764486..1341128c5ee99 100644 --- a/src/server/http/short_url_lookup.js +++ b/src/server/http/short_url_lookup.js @@ -1,12 +1,12 @@ import crypto from 'crypto'; export default function (server) { - async function updateMetadata(urlId, urlDoc) { - const client = server.plugins.elasticsearch.client; + async function updateMetadata(urlId, urlDoc, req) { + const callWithRequest = server.plugins.elasticsearch.callWithRequest; const kibanaIndex = server.config().get('kibana.index'); try { - await client.update({ + await callWithRequest(req, 'update', { index: kibanaIndex, type: 'url', id: urlId, @@ -23,12 +23,12 @@ export default function (server) { } } - async function getUrlDoc(urlId) { + async function getUrlDoc(urlId, req) { const urlDoc = await new Promise((resolve, reject) => { - const client = server.plugins.elasticsearch.client; + const callWithRequest = server.plugins.elasticsearch.callWithRequest; const kibanaIndex = server.config().get('kibana.index'); - client.get({ + callWithRequest(req, 'get', { index: kibanaIndex, type: 'url', id: urlId @@ -44,12 +44,12 @@ export default function (server) { return urlDoc; } - async function createUrlDoc(url, urlId) { + async function createUrlDoc(url, urlId, req) { const newUrlId = await new Promise((resolve, reject) => { - const client = server.plugins.elasticsearch.client; + const callWithRequest = server.plugins.elasticsearch.callWithRequest; const kibanaIndex = server.config().get('kibana.index'); - client.index({ + callWithRequest(req, 'index', { index: kibanaIndex, type: 'url', id: urlId, @@ -80,19 +80,19 @@ export default function (server) { } return { - async generateUrlId(url) { + async generateUrlId(url, req) { const urlId = createUrlId(url); - const urlDoc = await getUrlDoc(urlId); + const urlDoc = await getUrlDoc(urlId, req); if (urlDoc) return urlId; - return createUrlDoc(url, urlId); + return createUrlDoc(url, urlId, req); }, - async getUrl(urlId) { + async getUrl(urlId, req) { try { - const urlDoc = await getUrlDoc(urlId); + const urlDoc = await getUrlDoc(urlId, req); if (!urlDoc) throw new Error('Requested shortened url does not exist in kibana index'); - updateMetadata(urlId, urlDoc); + updateMetadata(urlId, urlDoc, req); return urlDoc._source.url; } catch (err) { diff --git a/src/ui/index.js b/src/ui/index.js index c88ec53c82e6f..be84d29effab3 100644 --- a/src/ui/index.js +++ b/src/ui/index.js @@ -1,6 +1,7 @@ import { format as formatUrl } from 'url'; import { readFileSync as readFile } from 'fs'; import { defaults } from 'lodash'; +import { props } from 'bluebird'; import Boom from 'boom'; import { reduce as reduceAsync } from 'bluebird'; import { resolve } from 'path'; @@ -61,7 +62,9 @@ export default async (kbnServer, server, config) => { } }); - async function getPayload(app) { + async function getKibanaPayload({ app, request, includeUserProvidedConfig }) { + const uiSettings = server.uiSettings(); + return { app: app, nav: uiExports.navLinks.inOrder, @@ -71,42 +74,47 @@ export default async (kbnServer, server, config) => { basePath: config.get('server.basePath'), serverName: config.get('server.name'), devMode: config.get('env.dev'), - uiSettings: { - defaults: await server.uiSettings().getDefaults(), - user: {} - }, + uiSettings: await props({ + defaults: uiSettings.getDefaults(), + user: includeUserProvidedConfig && uiSettings.getUserProvided(request) + }), vars: await reduceAsync( uiExports.injectedVarsReplacers, - async (acc, replacer) => await replacer(acc, this.request, server), + async (acc, replacer) => await replacer(acc, request, server), defaults(await app.getInjectedVars() || {}, uiExports.defaultInjectedVars) - ) + ), }; } - function viewAppWithPayload(app, payload) { - return this.view(app.templateName, { - app: app, - kibanaPayload: payload, - bundlePath: `${config.get('server.basePath')}/bundles`, - }); - } - - async function renderApp(app) { - const payload = await getPayload.call(this, app); - - const esStatus = kbnServer.status.getForPluginId('elasticsearch'); - if (esStatus && esStatus.state !== 'red') { - payload.uiSettings.user = await server.uiSettings().getUserProvided(); + async function renderApp({ app, reply, includeUserProvidedConfig = true }) { + try { + return reply.view(app.templateName, { + app, + kibanaPayload: await getKibanaPayload({ + app, + request: reply.request, + includeUserProvidedConfig + }), + bundlePath: `${config.get('server.basePath')}/bundles`, + }); + } catch (err) { + reply(err); } - - return viewAppWithPayload.call(this, app, payload); } - async function renderAppWithDefaultConfig(app) { - const payload = await getPayload.call(this, app); - return viewAppWithPayload.call(this, app, payload); - } + server.decorate('reply', 'renderApp', function (app) { + return renderApp({ + app, + reply: this, + includeUserProvidedConfig: true, + }); + }); - server.decorate('reply', 'renderApp', renderApp); - server.decorate('reply', 'renderAppWithDefaultConfig', renderAppWithDefaultConfig); + server.decorate('reply', 'renderAppWithDefaultConfig', function (app) { + return renderApp({ + app, + reply: this, + includeUserProvidedConfig: false, + }); + }); }; diff --git a/src/ui/public/config/config.js b/src/ui/public/config/config.js index 265e0b5dd6afe..d6cdbc144f8df 100644 --- a/src/ui/public/config/config.js +++ b/src/ui/public/config/config.js @@ -91,7 +91,12 @@ any custom setting configuration watchers for "${key}" may fix this issue.`); if (value === null) { delete settings[key].userValue; } else { - settings[key].userValue = value; + const { type } = settings[key]; + if (type === 'json' && typeof value !== 'string') { + settings[key].userValue = angular.toJson(value); + } else { + settings[key].userValue = value; + } } } diff --git a/src/ui/settings/__tests__/index.js b/src/ui/settings/__tests__/index.js index c3393ec5135ae..b44c918ad7317 100644 --- a/src/ui/settings/__tests__/index.js +++ b/src/ui/settings/__tests__/index.js @@ -3,6 +3,7 @@ import sinon from 'sinon'; import expect from 'expect.js'; import init from '..'; import defaultsProvider from '../defaults'; +import { errors as esErrors } from 'elasticsearch'; describe('ui settings', function () { describe('overview', function () { @@ -22,23 +23,23 @@ describe('ui settings', function () { describe('#setMany()', function () { it('returns a promise', () => { - const { uiSettings } = instantiate(); - const result = uiSettings.setMany({ a: 'b' }); + const { uiSettings, req } = instantiate(); + const result = uiSettings.setMany(req, { a: 'b' }); expect(result).to.be.a(Promise); }); it('updates a single value in one operation', function () { - const { server, uiSettings, configGet } = instantiate(); - const result = uiSettings.setMany({ one: 'value' }); - expectElasticsearchUpdateQuery(server, configGet, { + const { server, uiSettings, configGet, req } = instantiate(); + const result = uiSettings.setMany(req, { one: 'value' }); + expectElasticsearchUpdateQuery(server, req, configGet, { one: 'value' }); }); it('updates several values in one operation', function () { - const { server, uiSettings, configGet } = instantiate(); - const result = uiSettings.setMany({ one: 'value', another: 'val' }); - expectElasticsearchUpdateQuery(server, configGet, { + const { server, uiSettings, configGet, req } = instantiate(); + const result = uiSettings.setMany(req, { one: 'value', another: 'val' }); + expectElasticsearchUpdateQuery(server, req, configGet, { one: 'value', another: 'val' }); }); @@ -46,15 +47,15 @@ describe('ui settings', function () { describe('#set()', function () { it('returns a promise', () => { - const { uiSettings } = instantiate(); - const result = uiSettings.set('a', 'b'); + const { uiSettings, req } = instantiate(); + const result = uiSettings.set(req, 'a', 'b'); expect(result).to.be.a(Promise); }); it('updates single values by (key, value)', function () { - const { server, uiSettings, configGet } = instantiate(); - const result = uiSettings.set('one', 'value'); - expectElasticsearchUpdateQuery(server, configGet, { + const { server, uiSettings, configGet, req } = instantiate(); + const result = uiSettings.set(req, 'one', 'value'); + expectElasticsearchUpdateQuery(server, req, configGet, { one: 'value' }); }); @@ -62,15 +63,15 @@ describe('ui settings', function () { describe('#remove()', function () { it('returns a promise', () => { - const { uiSettings } = instantiate(); - const result = uiSettings.remove('one'); + const { uiSettings, req } = instantiate(); + const result = uiSettings.remove(req, 'one'); expect(result).to.be.a(Promise); }); it('removes single values by key', function () { - const { server, uiSettings, configGet } = instantiate(); - const result = uiSettings.remove('one'); - expectElasticsearchUpdateQuery(server, configGet, { + const { server, uiSettings, configGet, req } = instantiate(); + const result = uiSettings.remove(req, 'one'); + expectElasticsearchUpdateQuery(server, req, configGet, { one: null }); }); @@ -78,23 +79,23 @@ describe('ui settings', function () { describe('#removeMany()', function () { it('returns a promise', () => { - const { uiSettings } = instantiate(); - const result = uiSettings.removeMany(['one']); + const { uiSettings, req } = instantiate(); + const result = uiSettings.removeMany(req, ['one']); expect(result).to.be.a(Promise); }); it('removes a single value', function () { - const { server, uiSettings, configGet } = instantiate(); - const result = uiSettings.removeMany(['one']); - expectElasticsearchUpdateQuery(server, configGet, { + const { server, uiSettings, configGet, req } = instantiate(); + const result = uiSettings.removeMany(req, ['one']); + expectElasticsearchUpdateQuery(server, req, configGet, { one: null }); }); it('updates several values in one operation', function () { - const { server, uiSettings, configGet } = instantiate(); - const result = uiSettings.removeMany(['one', 'two', 'three']); - expectElasticsearchUpdateQuery(server, configGet, { + const { server, uiSettings, configGet, req } = instantiate(); + const result = uiSettings.removeMany(req, ['one', 'two', 'three']); + expectElasticsearchUpdateQuery(server, req, configGet, { one: null, two: null, three: null }); }); @@ -134,15 +135,15 @@ describe('ui settings', function () { describe('#getUserProvided()', function () { it('pulls user configuration from ES', async function () { const getResult = { user: 'customized' }; - const { server, uiSettings, configGet } = instantiate({ getResult }); - const result = await uiSettings.getUserProvided(); - expectElasticsearchGetQuery(server, configGet); + const { server, uiSettings, configGet, req } = instantiate({ getResult }); + const result = await uiSettings.getUserProvided(req); + expectElasticsearchGetQuery(server, req, configGet); }); it('returns user configuration', async function () { const getResult = { user: 'customized' }; - const { server, uiSettings, configGet } = instantiate({ getResult }); - const result = await uiSettings.getUserProvided(); + const { server, uiSettings, configGet, req } = instantiate({ getResult }); + const result = await uiSettings.getUserProvided(req); expect(isEqual(result, { user: { userValue: 'customized' } })).to.equal(true); @@ -150,33 +151,80 @@ describe('ui settings', function () { it('ignores null user configuration (because default values)', async function () { const getResult = { user: 'customized', usingDefault: null, something: 'else' }; - const { server, uiSettings, configGet } = instantiate({ getResult }); - const result = await uiSettings.getUserProvided(); + const { server, uiSettings, configGet, req } = instantiate({ getResult }); + const result = await uiSettings.getUserProvided(req); expect(isEqual(result, { user: { userValue: 'customized' }, something: { userValue: 'else' } })).to.equal(true); }); + + it('returns an empty object on 404 responses', async function () { + const { uiSettings, req } = instantiate({ + async callWithRequest() { + throw new esErrors[404](); + } + }); + + expect(await uiSettings.getUserProvided(req)).to.eql({}); + }); + + it('returns an empty object on 401 responses', async function () { + const { uiSettings, req } = instantiate({ + async callWithRequest() { + throw new esErrors[401](); + } + }); + + expect(await uiSettings.getUserProvided(req)).to.eql({}); + }); + + it('returns an empty object on NoConnections responses', async function () { + const { uiSettings, req } = instantiate({ + async callWithRequest() { + throw new esErrors.NoConnections(); + } + }); + + expect(await uiSettings.getUserProvided(req)).to.eql({}); + }); + + it('throw when callWithRequest fails in some unexpected way', async function () { + const expectedUnexpectedError = new Error('unexpected'); + + const { uiSettings, req } = instantiate({ + async callWithRequest() { + throw expectedUnexpectedError; + } + }); + + try { + await uiSettings.getUserProvided(req); + throw new Error('expect getUserProvided() to throw'); + } catch (err) { + expect(err).to.be(expectedUnexpectedError); + } + }); }); describe('#getRaw()', function () { it('pulls user configuration from ES', async function () { const getResult = {}; - const { server, uiSettings, configGet } = instantiate({ getResult }); - const result = await uiSettings.getRaw(); - expectElasticsearchGetQuery(server, configGet); + const { server, uiSettings, configGet, req } = instantiate({ getResult }); + const result = await uiSettings.getRaw(req); + expectElasticsearchGetQuery(server, req, configGet); }); it(`without user configuration it's equal to the defaults`, async function () { const getResult = {}; - const { server, uiSettings, configGet } = instantiate({ getResult }); - const result = await uiSettings.getRaw(); + const { server, uiSettings, configGet, req } = instantiate({ getResult }); + const result = await uiSettings.getRaw(req); expect(isEqual(result, defaultsProvider())).to.equal(true); }); it(`user configuration gets merged with defaults`, async function () { const getResult = { foo: 'bar' }; - const { server, uiSettings, configGet } = instantiate({ getResult }); - const result = await uiSettings.getRaw(); + const { server, uiSettings, configGet, req } = instantiate({ getResult }); + const result = await uiSettings.getRaw(req); const merged = defaultsProvider(); merged.foo = { userValue: 'bar' }; expect(isEqual(result, merged)).to.equal(true); @@ -184,8 +232,8 @@ describe('ui settings', function () { it(`user configuration gets merged into defaults`, async function () { const getResult = { dateFormat: 'YYYY-MM-DD' }; - const { server, uiSettings, configGet } = instantiate({ getResult }); - const result = await uiSettings.getRaw(); + const { server, uiSettings, configGet, req } = instantiate({ getResult }); + const result = await uiSettings.getRaw(req); const merged = defaultsProvider(); merged.dateFormat.userValue = 'YYYY-MM-DD'; expect(isEqual(result, merged)).to.equal(true); @@ -195,15 +243,15 @@ describe('ui settings', function () { describe('#getAll()', function () { it('pulls user configuration from ES', async function () { const getResult = {}; - const { server, uiSettings, configGet } = instantiate({ getResult }); - const result = await uiSettings.getAll(); - expectElasticsearchGetQuery(server, configGet); + const { server, uiSettings, configGet, req } = instantiate({ getResult }); + const result = await uiSettings.getAll(req); + expectElasticsearchGetQuery(server, req, configGet); }); it(`returns key value pairs`, async function () { const getResult = {}; - const { server, uiSettings, configGet } = instantiate({ getResult }); - const result = await uiSettings.getAll(); + const { server, uiSettings, configGet, req } = instantiate({ getResult }); + const result = await uiSettings.getAll(req); const defaults = defaultsProvider(); const expectation = {}; Object.keys(defaults).forEach(key => { @@ -214,8 +262,8 @@ describe('ui settings', function () { it(`returns key value pairs including user configuration`, async function () { const getResult = { something: 'user-provided' }; - const { server, uiSettings, configGet } = instantiate({ getResult }); - const result = await uiSettings.getAll(); + const { server, uiSettings, configGet, req } = instantiate({ getResult }); + const result = await uiSettings.getAll(req); const defaults = defaultsProvider(); const expectation = {}; Object.keys(defaults).forEach(key => { @@ -227,8 +275,8 @@ describe('ui settings', function () { it(`returns key value pairs including user configuration for existing settings`, async function () { const getResult = { dateFormat: 'YYYY-MM-DD' }; - const { server, uiSettings, configGet } = instantiate({ getResult }); - const result = await uiSettings.getAll(); + const { server, uiSettings, configGet, req } = instantiate({ getResult }); + const result = await uiSettings.getAll(req); const defaults = defaultsProvider(); const expectation = {}; Object.keys(defaults).forEach(key => { @@ -242,55 +290,63 @@ describe('ui settings', function () { describe('#get()', function () { it('pulls user configuration from ES', async function () { const getResult = {}; - const { server, uiSettings, configGet } = instantiate({ getResult }); - const result = await uiSettings.get(); - expectElasticsearchGetQuery(server, configGet); + const { server, uiSettings, configGet, req } = instantiate({ getResult }); + const result = await uiSettings.get(req); + expectElasticsearchGetQuery(server, req, configGet); }); it(`returns the promised value for a key`, async function () { const getResult = {}; - const { server, uiSettings, configGet } = instantiate({ getResult }); - const result = await uiSettings.get('dateFormat'); + const { server, uiSettings, configGet, req } = instantiate({ getResult }); + const result = await uiSettings.get(req, 'dateFormat'); const defaults = defaultsProvider(); expect(result).to.equal(defaults.dateFormat.value); }); it(`returns the user-configured value for a custom key`, async function () { const getResult = { custom: 'value' }; - const { server, uiSettings, configGet } = instantiate({ getResult }); - const result = await uiSettings.get('custom'); + const { server, uiSettings, configGet, req } = instantiate({ getResult }); + const result = await uiSettings.get(req, 'custom'); expect(result).to.equal('value'); }); it(`returns the user-configured value for a modified key`, async function () { const getResult = { dateFormat: 'YYYY-MM-DD' }; - const { server, uiSettings, configGet } = instantiate({ getResult }); - const result = await uiSettings.get('dateFormat'); + const { server, uiSettings, configGet, req } = instantiate({ getResult }); + const result = await uiSettings.get(req, 'dateFormat'); expect(result).to.equal('YYYY-MM-DD'); }); }); }); -function expectElasticsearchGetQuery(server, configGet) { - expect(server.plugins.elasticsearch.client.get.callCount).to.equal(1); - expect(isEqual(server.plugins.elasticsearch.client.get.firstCall.args, [{ +function expectElasticsearchGetQuery(server, req, configGet) { + const { callWithRequest } = server.plugins.elasticsearch; + sinon.assert.calledOnce(callWithRequest); + const [reqPassed, method, params] = callWithRequest.args[0]; + expect(reqPassed).to.be(req); + expect(method).to.be('get'); + expect(params).to.eql({ index: configGet('kibana.index'), id: configGet('pkg.version'), type: 'config' - }])).to.equal(true); + }); } -function expectElasticsearchUpdateQuery(server, configGet, doc) { - expect(server.plugins.elasticsearch.client.update.callCount).to.equal(1); - expect(isEqual(server.plugins.elasticsearch.client.update.firstCall.args, [{ +function expectElasticsearchUpdateQuery(server, req, configGet, doc) { + const { callWithRequest } = server.plugins.elasticsearch; + sinon.assert.calledOnce(callWithRequest); + const [reqPassed, method, params] = callWithRequest.args[0]; + expect(reqPassed).to.be(req); + expect(method).to.be('update'); + expect(params).to.eql({ index: configGet('kibana.index'), id: configGet('pkg.version'), type: 'config', body: { doc } - }])).to.equal(true); + }); } -function instantiate({ getResult } = {}) { +function instantiate({ getResult, callWithRequest } = {}) { const esStatus = { state: 'green', on: sinon.spy() @@ -308,14 +364,27 @@ function instantiate({ getResult } = {}) { }, ready: sinon.stub().returns(Promise.resolve()) }; + const req = { __stubHapiRequest: true, path: '', headers: {} }; const server = { decorate: (_, key, value) => server[key] = value, plugins: { elasticsearch: { - client: { - get: sinon.stub().returns(Promise.resolve({ _source: getResult })), - update: sinon.stub().returns(Promise.resolve()) - } + errors: esErrors, + callWithRequest: sinon.spy((withReq, method, params) => { + if (callWithRequest) { + return callWithRequest(withReq, method, params); + } + + expect(withReq).to.be(req); + switch (method) { + case 'get': + return Promise.resolve({ _source: getResult }); + case 'update': + return Promise.resolve(); + default: + throw new Error(`callWithRequest() is using unexpected method "${method}"`); + } + }) } } }; @@ -328,5 +397,5 @@ function instantiate({ getResult } = {}) { }; const setupSettings = init(kbnServer, server, config); const uiSettings = server.uiSettings(); - return { server, uiSettings, configGet }; + return { server, uiSettings, configGet, req }; } diff --git a/src/ui/settings/index.js b/src/ui/settings/index.js index d18f7491bc7a1..29f5959d67038 100644 --- a/src/ui/settings/index.js +++ b/src/ui/settings/index.js @@ -1,5 +1,6 @@ import { defaultsDeep, partial } from 'lodash'; import defaultsProvider from './defaults'; +import Bluebird from 'bluebird'; export default function setupSettings(kbnServer, server, config) { const status = kbnServer.status.create('ui settings'); @@ -34,12 +35,14 @@ export default function setupSettings(kbnServer, server, config) { server.decorate('server', 'uiSettings', () => uiSettings); kbnServer.ready().then(mirrorEsStatus); - function get(key) { - return getAll().then(all => all[key]); + async function get(req, key) { + assertRequest(req); + return getAll(req).then(all => all[key]); } - function getAll() { - return getRaw() + async function getAll(req) { + assertRequest(req); + return getRaw(req) .then(raw => Object.keys(raw) .reduce((all, key) => { const item = raw[key]; @@ -50,9 +53,10 @@ export default function setupSettings(kbnServer, server, config) { ); } - function getRaw() { + async function getRaw(req) { + assertRequest(req); return Promise - .all([getDefaults(), getUserProvided()]) + .all([getDefaults(), getUserProvided(req)]) .then(([defaults, user]) => defaultsDeep(user, defaults)); } @@ -60,46 +64,48 @@ export default function setupSettings(kbnServer, server, config) { return Promise.resolve(defaultsProvider()); } - function userSettingsNotFound(kibanaVersion) { - status.red(`Could not find user-provided settings for Kibana ${kibanaVersion}`); - return {}; - } + async function getUserProvided(req, { ignore401Errors = false } = {}) { + assertRequest(req); + const { callWithRequest, errors } = server.plugins.elasticsearch; + + const params = getClientSettings(config); + const allowedErrors = [errors[404], errors[403], errors.NoConnections]; + if (ignore401Errors) allowedErrors.push(errors[401]); - function getUserProvided() { - const { client } = server.plugins.elasticsearch; - const clientSettings = getClientSettings(config); - return client - .get({ ...clientSettings }) - .then(res => res._source) - .catch(partial(userSettingsNotFound, clientSettings.id)) - .then(user => hydrateUserSettings(user)); + return Bluebird.resolve(callWithRequest(req, 'get', params, { wrap401Errors: !ignore401Errors })) + .catch(...allowedErrors, err => ({})) + .then(resp => resp._source || {}) + .then(source => hydrateUserSettings(source)); } - function setMany(changes) { - const { client } = server.plugins.elasticsearch; - const clientSettings = getClientSettings(config); - return client - .update({ - ...clientSettings, - body: { doc: changes } - }) + async function setMany(req, changes) { + assertRequest(req); + const { callWithRequest } = server.plugins.elasticsearch; + const clientParams = { + ...getClientSettings(config), + body: { doc: changes } + }; + return callWithRequest(req, 'update', clientParams) .then(() => ({})); } - function set(key, value) { - return setMany({ [key]: value }); + async function set(req, key, value) { + assertRequest(req); + return setMany(req, { [key]: value }); } - function remove(key) { - return set(key, null); + async function remove(req, key) { + assertRequest(req); + return set(req, key, null); } - function removeMany(keys) { + async function removeMany(req, keys) { + assertRequest(req); const changes = {}; keys.forEach(key => { changes[key] = null; }); - return setMany(changes); + return setMany(req, changes); } function mirrorEsStatus() { @@ -138,3 +144,13 @@ function getClientSettings(config) { const type = 'config'; return { index, type, id }; } + +function assertRequest(req) { + if ( + typeof req === 'object' && + typeof req.path === 'string' && + typeof req.headers === 'object' + ) return; + + throw new TypeError('all uiSettings methods must be passed a hapi.Request object'); +} From 65b1e0a50b13dbd92bd649869908999b67a65de9 Mon Sep 17 00:00:00 2001 From: spalger Date: Wed, 23 Nov 2016 16:38:04 -0700 Subject: [PATCH 2/3] [uiExports/replaceInjectedVars] update the uiSettings stub --- src/ui/__tests__/ui_exports_replace_injected_vars.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ui/__tests__/ui_exports_replace_injected_vars.js b/src/ui/__tests__/ui_exports_replace_injected_vars.js index fe578642eae84..9f4f714906b35 100644 --- a/src/ui/__tests__/ui_exports_replace_injected_vars.js +++ b/src/ui/__tests__/ui_exports_replace_injected_vars.js @@ -41,7 +41,7 @@ describe('UiExports', function () { await kbnServer.ready(); kbnServer.status.get('ui settings').state = 'green'; kbnServer.server.decorate('server', 'uiSettings', () => { - return { getDefaults: noop }; + return { getDefaults: noop, getUserProvided: noop }; }); }); From c2c7fdbb8c5e534d896f1dbe6492d041d7e9bdf0 Mon Sep 17 00:00:00 2001 From: spalger Date: Wed, 23 Nov 2016 16:41:08 -0700 Subject: [PATCH 3/3] [uiSettings] correct test cases after moving from 401 -> 403 --- src/ui/settings/__tests__/index.js | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/src/ui/settings/__tests__/index.js b/src/ui/settings/__tests__/index.js index b44c918ad7317..344552abdce81 100644 --- a/src/ui/settings/__tests__/index.js +++ b/src/ui/settings/__tests__/index.js @@ -168,10 +168,10 @@ describe('ui settings', function () { expect(await uiSettings.getUserProvided(req)).to.eql({}); }); - it('returns an empty object on 401 responses', async function () { + it('returns an empty object on 403 responses', async function () { const { uiSettings, req } = instantiate({ async callWithRequest() { - throw new esErrors[401](); + throw new esErrors[403](); } }); @@ -188,6 +188,21 @@ describe('ui settings', function () { expect(await uiSettings.getUserProvided(req)).to.eql({}); }); + it('throws 401 errors', async function () { + const { uiSettings, req } = instantiate({ + async callWithRequest() { + throw new esErrors[401](); + } + }); + + try { + await uiSettings.getUserProvided(req); + throw new Error('expect getUserProvided() to throw'); + } catch (err) { + expect(err).to.be.a(esErrors[401]); + } + }); + it('throw when callWithRequest fails in some unexpected way', async function () { const expectedUnexpectedError = new Error('unexpected');