[server/uiSettings+shortUrl] surface errors from es

airow · Nov 23, 2016 · 9c9b551 · 9c9b551
1 parent 8d003a1
commit 9c9b551
Show file tree

Hide file tree

Showing 17 changed files with 345 additions and 252 deletions.
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
diff --git a/src/core_plugins/elasticsearch/lib/call_with_request.js b/src/core_plugins/elasticsearch/lib/call_with_request.js
@@ -1,31 +1,31 @@
 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));
     if (_.isEmpty(apiContext)) {
       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
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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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) {