From c37e3f173d0a06aa0e0cff46d4b546569019d32f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?=
 <danielgarciaaubert@gmail.com>
Date: Wed, 3 Jun 2020 15:39:02 +0200
Subject: [PATCH] Handle error properly in user middleware, it will logged in
 error middleware

---
 lib/api/middlewares/user.js   | 11 +++++++----
 lib/models/cdb-request.js     | 11 +++--------
 test/unit/cdb-request-test.js | 33 +++++++++++++--------------------
 3 files changed, 23 insertions(+), 32 deletions(-)

diff --git a/lib/api/middlewares/user.js b/lib/api/middlewares/user.js
index 31e7f909..814b62e6 100644
--- a/lib/api/middlewares/user.js
+++ b/lib/api/middlewares/user.js
@@ -3,11 +3,14 @@
 const CdbRequest = require('../../models/cdb-request');
 
 module.exports = function user (metadataBackend) {
-    return function userMiddleware (req, res, next) {
-        const { logger } = res.locals;
-        const cdbRequest = new CdbRequest({ logger });
+    const cdbRequest = new CdbRequest();
 
-        res.locals.user = getUserNameFromRequest(req, cdbRequest);
+    return function userMiddleware (req, res, next) {
+        try {
+            res.locals.user = getUserNameFromRequest(req, cdbRequest);
+        } catch (err) {
+            return next(err);
+        }
 
         metadataBackend.getUserId(res.locals.user, (err, userId) => {
             if (err || !userId) {
diff --git a/lib/models/cdb-request.js b/lib/models/cdb-request.js
index a962b558..ca160649 100644
--- a/lib/models/cdb-request.js
+++ b/lib/models/cdb-request.js
@@ -1,8 +1,7 @@
 'use strict';
 
 module.exports = class CdbRequest {
-    constructor ({ logger }) {
-        this.logger = logger;
+    constructor () {
         // would extract "strk" from "strk.cartodb.com"
         this.RE_USER_FROM_HOST = new RegExp(global.environment.user_from_host || '^([^\\.]+)\\.');
     }
@@ -16,12 +15,8 @@ module.exports = class CdbRequest {
 
         const mat = host.match(this.RE_USER_FROM_HOST);
 
-        if (!mat) {
-            return this.logger.error(new Error(`Pattern '${this.RE_USER_FROM_HOST}' does not match hostname '${host}'`));
-        }
-
-        if (mat.length !== 2) {
-            return this.logger.error(new Error(`Pattern '${this.RE_USER_FROM_HOST}' gave unexpected matches against '${host}': ${mat}`));
+        if (!mat || mat.length !== 2) {
+            throw new Error(`No username found in hostname '${host}'`);
         }
 
         return mat[1];
diff --git a/test/unit/cdb-request-test.js b/test/unit/cdb-request-test.js
index 9db38488..93d1894a 100644
--- a/test/unit/cdb-request-test.js
+++ b/test/unit/cdb-request-test.js
@@ -4,9 +4,8 @@ require('../support/test-helper');
 var assert = require('assert');
 
 var CdbRequest = require('../../lib/models/cdb-request');
-const { logger } = require('../../lib/server-options');
 
-describe('req2params', function () {
+describe('username in host header (CdbRequest)', function () {
     function createRequest (host, userParam) {
         var req = {
             params: {},
@@ -23,7 +22,7 @@ describe('req2params', function () {
     }
 
     it('extracts name from host header', function () {
-        var cdbRequest = new CdbRequest({ logger });
+        var cdbRequest = new CdbRequest();
         var user = cdbRequest.userByReq(createRequest('localhost'));
 
         assert.strictEqual(user, 'localhost');
@@ -33,7 +32,7 @@ describe('req2params', function () {
         var userFromHostConfig = global.environment.user_from_host;
         global.environment.user_from_host = null;
 
-        var cdbRequest = new CdbRequest({ logger });
+        var cdbRequest = new CdbRequest();
         var user = cdbRequest.userByReq(createRequest('development.localhost.lan'));
 
         global.environment.user_from_host = userFromHostConfig;
@@ -42,45 +41,39 @@ describe('req2params', function () {
     });
 
     it('considers user param before headers', function () {
-        var cdbRequest = new CdbRequest({ logger });
+        var cdbRequest = new CdbRequest();
         var user = cdbRequest.userByReq(createRequest('localhost', 'development'));
 
         assert.strictEqual(user, 'development');
     });
 
-    it('returns undefined when it cannot extract username', function () {
+    it('returns throw when it cannot extract username', function () {
         var userFromHostConfig = global.environment.user_from_host;
         global.environment.user_from_host = null;
 
-        var cdbRequest = new CdbRequest({ logger });
-        var user = cdbRequest.userByReq(createRequest('localhost'));
+        var cdbRequest = new CdbRequest();
+        assert.throws(() => cdbRequest.userByReq(createRequest('localhost')));
 
         global.environment.user_from_host = userFromHostConfig;
-
-        assert.strictEqual(user, undefined);
     });
 
-    it('should not fail for undefined host header', function () {
+    it('should throw for undefined host header', function () {
         var userFromHostConfig = global.environment.user_from_host;
         global.environment.user_from_host = null;
 
-        var cdbRequest = new CdbRequest({ logger });
-        var user = cdbRequest.userByReq(createRequest(undefined));
+        var cdbRequest = new CdbRequest();
+        assert.throws(() => cdbRequest.userByReq(createRequest(undefined)));
 
         global.environment.user_from_host = userFromHostConfig;
-
-        assert.strictEqual(user, undefined);
     });
 
-    it('should not fail for null host header', function () {
+    it('should throw for null host header', function () {
         var userFromHostConfig = global.environment.user_from_host;
         global.environment.user_from_host = null;
 
-        var cdbRequest = new CdbRequest({ logger });
-        var user = cdbRequest.userByReq(createRequest(null));
+        var cdbRequest = new CdbRequest();
+        assert.throws(() => cdbRequest.userByReq(createRequest(null)));
 
         global.environment.user_from_host = userFromHostConfig;
-
-        assert.strictEqual(user, undefined);
     });
 });