From ca612dd02a5753575133c2655958d3f3fdbef122 Mon Sep 17 00:00:00 2001 From: Simon Date: Thu, 28 Sep 2017 11:43:12 +0200 Subject: [PATCH 01/25] res.locals in context middlewares --- lib/cartodb/middleware/context/authorize.js | 7 +------ .../context/clean-up-query-params.js | 4 ++-- .../middleware/context/db-conn-setup.js | 15 ++++----------- lib/cartodb/middleware/context/index.js | 2 ++ .../middleware/context/layergroup-token.js | 18 +++++++++--------- lib/cartodb/middleware/context/locals.js | 7 +++++++ 6 files changed, 25 insertions(+), 28 deletions(-) create mode 100644 lib/cartodb/middleware/context/locals.js diff --git a/lib/cartodb/middleware/context/authorize.js b/lib/cartodb/middleware/context/authorize.js index bf95a78a..bab100b0 100644 --- a/lib/cartodb/middleware/context/authorize.js +++ b/lib/cartodb/middleware/context/authorize.js @@ -2,14 +2,9 @@ const _ = require('underscore'); module.exports = function authorizeMiddleware (authApi) { return function (req, res, next) { - // FIXME: Temporary hack to share data between middlewares. Express overrides req.params to - // parse url params to an object and it's performed after matching path and controller. - res.locals = {}; - _.extend(res.locals, req.params); - req.profiler.done('req2params.setup'); - authApi.authorize(req, (err, authorized) => { + authApi.authorize(req, res, (err, authorized) => { req.profiler.done('authorize'); if (err) { return next(err); diff --git a/lib/cartodb/middleware/context/clean-up-query-params.js b/lib/cartodb/middleware/context/clean-up-query-params.js index 7b0b56eb..4cc1ceb6 100644 --- a/lib/cartodb/middleware/context/clean-up-query-params.js +++ b/lib/cartodb/middleware/context/clean-up-query-params.js @@ -24,8 +24,8 @@ module.exports = function cleanUpQueryParamsMiddleware () { req.query = _.pick(req.query, allowedQueryParams); - // bring all query values onto req.params object - _.extend(req.params, req.query); + // bring all query values onto res.locals object + _.extend(res.locals, req.query); next(); }; diff --git a/lib/cartodb/middleware/context/db-conn-setup.js b/lib/cartodb/middleware/context/db-conn-setup.js index 97efb77d..cc7df0d5 100644 --- a/lib/cartodb/middleware/context/db-conn-setup.js +++ b/lib/cartodb/middleware/context/db-conn-setup.js @@ -4,9 +4,8 @@ module.exports = function dbConnSetupMiddleware(pgConnection) { return function (req, res, next) { const user = req.context.user; - // FIXME: this function shouldn't be able to change `req.params`. It should return an - // object with the user's conf and it should be merge with default here. - pgConnection.setDBConn(user, req.params, (err) => { + res.locals.db = {} + pgConnection.setDBConn(user, res.locals.db, (err) => { if (err) { if (err.message && -1 !== err.message.indexOf('name not found')) { err.http_status = 404; @@ -17,20 +16,14 @@ module.exports = function dbConnSetupMiddleware(pgConnection) { // Add default database connection parameters // if none given - _.defaults(req.params, { + _.defaults(res.locals.db, { dbuser: global.environment.postgres.user, dbpassword: global.environment.postgres.password, dbhost: global.environment.postgres.host, dbport: global.environment.postgres.port }); - // FIXME: Temporary hack to share data between middlewares. Express overrides req.params to - // parse url params to an object and it's performed after matching path and controller. - if (!res.locals) { - res.locals = {}; - } - _.defaults(res.locals, req.params); - + req.profiler.done('req2params'); next(null, req); diff --git a/lib/cartodb/middleware/context/index.js b/lib/cartodb/middleware/context/index.js index 411b6f93..d660dcaf 100644 --- a/lib/cartodb/middleware/context/index.js +++ b/lib/cartodb/middleware/context/index.js @@ -1,3 +1,4 @@ +const locals = require('./locals') const cleanUpQueryParams = require('./clean-up-query-params'); const layergroupToken = require('./layergroup-token'); const authorize = require('./authorize'); @@ -5,6 +6,7 @@ const dbConnSetup = require('./db-conn-setup'); module.exports = function prepareContextMiddleware(authApi, pgConnection) { return [ + locals, cleanUpQueryParams(), layergroupToken, authorize(authApi), diff --git a/lib/cartodb/middleware/context/layergroup-token.js b/lib/cartodb/middleware/context/layergroup-token.js index d1ccb3be..b90d5e13 100644 --- a/lib/cartodb/middleware/context/layergroup-token.js +++ b/lib/cartodb/middleware/context/layergroup-token.js @@ -1,22 +1,22 @@ var LayergroupToken = require('../../models/layergroup-token'); module.exports = function layergroupTokenMiddleware(req, res, next) { - if (!req.params.hasOwnProperty('token')) { + if (!res.locals.hasOwnProperty('token')) { return next(); } var user = req.context.user; - var layergroupToken = LayergroupToken.parse(req.params.token); - req.params.token = layergroupToken.token; - req.params.cache_buster = layergroupToken.cacheBuster; + var layergroupToken = LayergroupToken.parse(res.locals.token); + res.locals.token = layergroupToken.token; + res.locals.cache_buster = layergroupToken.cacheBuster; if (layergroupToken.signer) { - req.params.signer = layergroupToken.signer; - if (!req.params.signer) { - req.params.signer = user; - } else if (req.params.signer !== user) { - var err = new Error(`Cannot use map signature of user "${req.params.signer}" on db of user "${user}"`); + res.locals.signer = layergroupToken.signer; + if (!res.locals.signer) { + res.locals.signer = user; + } else if (res.locals.signer !== user) { + var err = new Error(`Cannot use map signature of user "${res.locals.signer}" on db of user "${user}"`); err.type = 'auth'; err.http_status = 403; if (req.query && req.query.callback) { diff --git a/lib/cartodb/middleware/context/locals.js b/lib/cartodb/middleware/context/locals.js new file mode 100644 index 00000000..ec847788 --- /dev/null +++ b/lib/cartodb/middleware/context/locals.js @@ -0,0 +1,7 @@ +module.exports = function layergroupTokenMiddleware(req, res, next) { + // FIXME: Temporary hack to share data between middlewares. Express overrides req.params to + // parse url params to an object and it's performed after matching path and controller. + res.locals = {}; + _.extend(res.locals, req.params); +} + From 4a2cc6a5f8a7adca7df0b7f9487eeb71d013ca35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Mart=C3=ADn?= Date: Thu, 28 Sep 2017 11:55:36 +0200 Subject: [PATCH 02/25] res.locals in auth_api --- lib/cartodb/api/auth_api.js | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/lib/cartodb/api/auth_api.js b/lib/cartodb/api/auth_api.js index 484d66b1..f93d5a17 100644 --- a/lib/cartodb/api/auth_api.js +++ b/lib/cartodb/api/auth_api.js @@ -26,15 +26,15 @@ module.exports = AuthApi; // null if the request is not signed by anyone // or will be a string cartodb username otherwise. // -AuthApi.prototype.authorizedBySigner = function(req, callback) { - if ( ! req.params.token || ! req.params.signer ) { +AuthApi.prototype.authorizedBySigner = function(locals, callback) { + if ( ! locals.token || ! locals.signer ) { return callback(null, false); // no signer requested } var self = this; - var layergroup_id = req.params.token; - var auth_token = req.params.auth_token; + var layergroup_id = locals.token; + var auth_token = locals.auth_token; this.mapStore.load(layergroup_id, function(err, mapConfig) { if (err) { @@ -86,7 +86,7 @@ AuthApi.prototype.authorizedByAPIKey = function(user, req, callback) { * @param req - standard req object. Importantly contains table and host information * @param callback function(err, allowed) is access allowed not? */ -AuthApi.prototype.authorize = function(req, callback) { +AuthApi.prototype.authorize = function(req, res, callback) { var self = this; var user = req.context.user; @@ -101,11 +101,11 @@ AuthApi.prototype.authorize = function(req, callback) { // if not authorized by api_key, continue if (!authorized) { // not authorized by api_key, check if authorized by signer - return self.authorizedBySigner(req, this); + return self.authorizedBySigner(res.locals, this); } // authorized by api key, login as the given username and stop - self.pgConnection.setDBAuth(user, req.params, function(err) { + self.pgConnection.setDBAuth(user, res.locals.db, function(err) { callback(err, true); // authorized (or error) }); }, @@ -120,7 +120,7 @@ AuthApi.prototype.authorize = function(req, callback) { // if no signer name was given, let dbparams and // PostgreSQL do the rest. // - if ( ! req.params.signer ) { + if ( ! res.locals.signer ) { return callback(null, true); // authorized so far } @@ -128,7 +128,7 @@ AuthApi.prototype.authorize = function(req, callback) { return callback(null, false); } - self.pgConnection.setDBAuth(user, req.params, function(err) { + self.pgConnection.setDBAuth(user, res.locals.db, function(err) { req.profiler.done('setDBAuth'); callback(err, true); // authorized (or error) }); From f824fc52435a94c8591a4ea27371a67a4594157f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Mart=C3=ADn?= Date: Thu, 28 Sep 2017 12:02:34 +0200 Subject: [PATCH 03/25] base and analyses controller --- lib/cartodb/controllers/analyses.js | 2 +- lib/cartodb/controllers/base.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/cartodb/controllers/analyses.js b/lib/cartodb/controllers/analyses.js index ebe9c007..e230f350 100644 --- a/lib/cartodb/controllers/analyses.js +++ b/lib/cartodb/controllers/analyses.js @@ -44,7 +44,7 @@ AnalysesController.prototype.catalog = function (req, res, next) { step( function catalogQuery() { - var pg = new PSQL(dbParamsFromReqParams(req.params)); + var pg = new PSQL(dbParamsFromReqParams(res.locals.db)); getMetadata(username, pg, this); }, function prepareResponse(err, results) { diff --git a/lib/cartodb/controllers/base.js b/lib/cartodb/controllers/base.js index 7462f6ba..769a9228 100644 --- a/lib/cartodb/controllers/base.js +++ b/lib/cartodb/controllers/base.js @@ -7,8 +7,8 @@ module.exports = BaseController; // jshint maxcomplexity:9 BaseController.prototype.send = function(req, res, body, status, headers) { - if (req.params.dbhost) { - res.set('X-Served-By-DB-Host', req.params.dbhost); + if (res.locals.db.dbhost) { + res.set('X-Served-By-DB-Host', res.locals.db.dbhost); } res.set('X-Tiler-Profiler', req.profiler.toJSONString()); From b4d03c074a0d79bec9e91215544025c2599a6ee5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Mart=C3=ADn?= Date: Fri, 29 Sep 2017 11:07:11 +0200 Subject: [PATCH 04/25] not move db params to res.locals.db --- lib/cartodb/api/auth_api.js | 4 ++-- lib/cartodb/controllers/analyses.js | 2 +- lib/cartodb/controllers/base.js | 4 ++-- lib/cartodb/middleware/context/db-conn-setup.js | 5 ++--- 4 files changed, 7 insertions(+), 8 deletions(-) diff --git a/lib/cartodb/api/auth_api.js b/lib/cartodb/api/auth_api.js index f93d5a17..5b62ff44 100644 --- a/lib/cartodb/api/auth_api.js +++ b/lib/cartodb/api/auth_api.js @@ -105,7 +105,7 @@ AuthApi.prototype.authorize = function(req, res, callback) { } // authorized by api key, login as the given username and stop - self.pgConnection.setDBAuth(user, res.locals.db, function(err) { + self.pgConnection.setDBAuth(user, res.locals, function(err) { callback(err, true); // authorized (or error) }); }, @@ -128,7 +128,7 @@ AuthApi.prototype.authorize = function(req, res, callback) { return callback(null, false); } - self.pgConnection.setDBAuth(user, res.locals.db, function(err) { + self.pgConnection.setDBAuth(user, res.locals, function(err) { req.profiler.done('setDBAuth'); callback(err, true); // authorized (or error) }); diff --git a/lib/cartodb/controllers/analyses.js b/lib/cartodb/controllers/analyses.js index e230f350..22db4014 100644 --- a/lib/cartodb/controllers/analyses.js +++ b/lib/cartodb/controllers/analyses.js @@ -44,7 +44,7 @@ AnalysesController.prototype.catalog = function (req, res, next) { step( function catalogQuery() { - var pg = new PSQL(dbParamsFromReqParams(res.locals.db)); + var pg = new PSQL(dbParamsFromReqParams(res.locals)); getMetadata(username, pg, this); }, function prepareResponse(err, results) { diff --git a/lib/cartodb/controllers/base.js b/lib/cartodb/controllers/base.js index 769a9228..9309de11 100644 --- a/lib/cartodb/controllers/base.js +++ b/lib/cartodb/controllers/base.js @@ -7,8 +7,8 @@ module.exports = BaseController; // jshint maxcomplexity:9 BaseController.prototype.send = function(req, res, body, status, headers) { - if (res.locals.db.dbhost) { - res.set('X-Served-By-DB-Host', res.locals.db.dbhost); + if (res.locals.dbhost) { + res.set('X-Served-By-DB-Host', res.locals.dbhost); } res.set('X-Tiler-Profiler', req.profiler.toJSONString()); diff --git a/lib/cartodb/middleware/context/db-conn-setup.js b/lib/cartodb/middleware/context/db-conn-setup.js index cc7df0d5..634711f7 100644 --- a/lib/cartodb/middleware/context/db-conn-setup.js +++ b/lib/cartodb/middleware/context/db-conn-setup.js @@ -4,8 +4,7 @@ module.exports = function dbConnSetupMiddleware(pgConnection) { return function (req, res, next) { const user = req.context.user; - res.locals.db = {} - pgConnection.setDBConn(user, res.locals.db, (err) => { + pgConnection.setDBConn(user, res.locals, (err) => { if (err) { if (err.message && -1 !== err.message.indexOf('name not found')) { err.http_status = 404; @@ -16,7 +15,7 @@ module.exports = function dbConnSetupMiddleware(pgConnection) { // Add default database connection parameters // if none given - _.defaults(res.locals.db, { + _.defaults(res.locals, { dbuser: global.environment.postgres.user, dbpassword: global.environment.postgres.password, dbhost: global.environment.postgres.host, From a21648ab4ae15bd21daed0989fad115c4d866838 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Mart=C3=ADn?= Date: Fri, 29 Sep 2017 12:32:46 +0200 Subject: [PATCH 05/25] res.locals in layergroup controller --- lib/cartodb/backends/analysis-status.js | 4 +--- lib/cartodb/backends/dataview.js | 8 ++----- lib/cartodb/controllers/layergroup.js | 28 ++++++++++++------------- 3 files changed, 17 insertions(+), 23 deletions(-) diff --git a/lib/cartodb/backends/analysis-status.js b/lib/cartodb/backends/analysis-status.js index 97f851d2..71ee6fc8 100644 --- a/lib/cartodb/backends/analysis-status.js +++ b/lib/cartodb/backends/analysis-status.js @@ -6,9 +6,7 @@ function AnalysisStatusBackend() { module.exports = AnalysisStatusBackend; -AnalysisStatusBackend.prototype.getNodeStatus = function (params, callback) { - var nodeId = params.nodeId; - +AnalysisStatusBackend.prototype.getNodeStatus = function (nodeId, params, callback) { var statusQuery = [ 'SELECT node_id, status, updated_at, last_error_message as error_message', 'FROM cdb_analysis_catalog where node_id = \'' + nodeId + '\'' diff --git a/lib/cartodb/backends/dataview.js b/lib/cartodb/backends/dataview.js index 29dcd903..8a9c0a85 100644 --- a/lib/cartodb/backends/dataview.js +++ b/lib/cartodb/backends/dataview.js @@ -22,9 +22,7 @@ function DataviewBackend(analysisBackend) { module.exports = DataviewBackend; -DataviewBackend.prototype.getDataview = function (mapConfigProvider, user, params, callback) { - - var dataviewName = params.dataviewName; +DataviewBackend.prototype.getDataview = function (mapConfigProvider, user, dataviewName, params, callback) { step( function getMapConfig() { mapConfigProvider.getMapConfig(this); @@ -113,9 +111,7 @@ function getOverrideParams(params, ownFilter) { return overrideParams; } -DataviewBackend.prototype.search = function (mapConfigProvider, user, params, callback) { - var dataviewName = params.dataviewName; - +DataviewBackend.prototype.search = function (mapConfigProvider, user, dataviewName, params, callback) { step( function getMapConfig() { mapConfigProvider.getMapConfig(this); diff --git a/lib/cartodb/controllers/layergroup.js b/lib/cartodb/controllers/layergroup.js index 2172f6c3..700564fc 100644 --- a/lib/cartodb/controllers/layergroup.js +++ b/lib/cartodb/controllers/layergroup.js @@ -151,7 +151,7 @@ LayergroupController.prototype.analysisNodeStatus = function(req, res, next) { step( function retrieveNodeStatus() { - self.analysisStatusBackend.getNodeStatus(req.params, this); + self.analysisStatusBackend.getNodeStatus(req.params.nodeId, res.locals, this); }, function finish(err, nodeStatus, stats) { req.profiler.add(stats || {}); @@ -175,9 +175,9 @@ LayergroupController.prototype.dataview = function(req, res, next) { step( function retrieveDataview() { var mapConfigProvider = new MapStoreMapConfigProvider( - self.mapStore, req.context.user, self.userLimitsApi, req.params + self.mapStore, req.context.user, self.userLimitsApi, res.locals ); - self.dataviewBackend.getDataview(mapConfigProvider, req.context.user, req.params, this); + self.dataviewBackend.getDataview(mapConfigProvider, req.context.user, req.params.dataviewName, res.locals, this); }, function finish(err, dataview, stats) { req.profiler.add(stats || {}); @@ -198,9 +198,9 @@ LayergroupController.prototype.dataviewSearch = function(req, res, next) { step( function searchDataview() { var mapConfigProvider = new MapStoreMapConfigProvider( - self.mapStore, req.context.user, self.userLimitsApi, req.params + self.mapStore, req.context.user, self.userLimitsApi, res.locals ); - self.dataviewBackend.search(mapConfigProvider, req.context.user, req.params, this); + self.dataviewBackend.search(mapConfigProvider, req.context.user, req.params.dataviewName, res.locals, this); }, function finish(err, searchResult, stats) { req.profiler.add(stats || {}); @@ -224,9 +224,9 @@ LayergroupController.prototype.attributes = function(req, res, next) { step( function retrieveFeatureAttributes() { var mapConfigProvider = new MapStoreMapConfigProvider( - self.mapStore, req.context.user, self.userLimitsApi, req.params + self.mapStore, req.context.user, self.userLimitsApi, res.locals ); - self.attributesBackend.getFeatureAttributes(mapConfigProvider, req.params, false, this); + self.attributesBackend.getFeatureAttributes(mapConfigProvider, res.locals, false, this); }, function finish(err, tile, stats) { req.profiler.add(stats || {}); @@ -260,7 +260,7 @@ LayergroupController.prototype.tileOrLayer = function (req, res, next) { step( function mapController$getTileOrGrid() { self.tileBackend.getTile( - new MapStoreMapConfigProvider(self.mapStore, req.context.user, self.userLimitsApi, req.params), + new MapStoreMapConfigProvider(self.mapStore, req.context.user, self.userLimitsApi, res.locals), req.params, this ); }, @@ -341,11 +341,11 @@ LayergroupController.prototype.staticMap = function(req, res, width, height, zoo function getImage() { if (center) { self.previewBackend.getImage( - new MapStoreMapConfigProvider(self.mapStore, req.context.user, self.userLimitsApi, req.params), + new MapStoreMapConfigProvider(self.mapStore, req.context.user, self.userLimitsApi, res.locals), format, width, height, zoom, center, this); } else { self.previewBackend.getImage( - new MapStoreMapConfigProvider(self.mapStore, req.context.user, self.userLimitsApi, req.params), + new MapStoreMapConfigProvider(self.mapStore, req.context.user, self.userLimitsApi, res.locals), format, width, height, zoom /* bounds */, this); } }, @@ -373,18 +373,18 @@ LayergroupController.prototype.sendResponse = function(req, res, body, status, h // Set Last-Modified header var lastUpdated; - if (req.params.cache_buster) { + if (res.locals.cache_buster) { // Assuming cache_buster is a timestamp - lastUpdated = new Date(parseInt(req.params.cache_buster)); + lastUpdated = new Date(parseInt(res.locals.cache_buster)); } else { lastUpdated = new Date(); } res.set('Last-Modified', lastUpdated.toUTCString()); - var dbName = req.params.dbname; + var dbName = res.locals.dbname; step( function getAffectedTables() { - self.getAffectedTables(req.context.user, dbName, req.params.token, this); + self.getAffectedTables(req.context.user, dbName, res.locals.token, this); }, function sendResponse(err, affectedTables) { req.profiler.done('affectedTables'); From 0a753400e0fe23c6e01c4572eb8697a84f4982d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Mart=C3=ADn?= Date: Fri, 29 Sep 2017 12:54:21 +0200 Subject: [PATCH 06/25] res.locals in map controller --- lib/cartodb/controllers/map.js | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/lib/cartodb/controllers/map.js b/lib/cartodb/controllers/map.js index 661cdb39..64ad1bb7 100644 --- a/lib/cartodb/controllers/map.js +++ b/lib/cartodb/controllers/map.js @@ -94,11 +94,11 @@ MapController.prototype.register = function(app) { MapController.prototype.createGet = function(req, res, next){ req.profiler.start('windshaft.createmap_get'); - this.create(req, res, function createGet$prepareConfig(req) { - if ( ! req.params.config ) { + this.create(req, res, function createGet$prepareConfig(req, config) { + if ( ! config ) { throw new Error('layergroup GET needs a "config" parameter'); } - return JSON.parse(req.params.config); + return JSON.parse(config); }, next); }; @@ -155,7 +155,7 @@ MapController.prototype.create = function(req, res, prepareConfigFn, next) { step( function prepareConfig () { - const requestMapConfig = prepareConfigFn(req); + const requestMapConfig = prepareConfigFn(req, res.locals.config); return requestMapConfig; }, function prepareAdapterMapConfig(err, requestMapConfig) { @@ -163,18 +163,18 @@ MapController.prototype.create = function(req, res, prepareConfigFn, next) { context.analysisConfiguration = { user: req.context.user, db: { - host: req.params.dbhost, - port: req.params.dbport, - dbname: req.params.dbname, - user: req.params.dbuser, - pass: req.params.dbpassword + host: res.locals.dbhost, + port: res.locals.dbport, + dbname: res.locals.dbname, + user: res.locals.dbuser, + pass: res.locals.dbpassword }, batch: { username: req.context.user, - apiKey: req.params.api_key + apiKey: res.locals.api_key } }; - self.mapConfigAdapter.getMapConfig(req.context.user, requestMapConfig, req.params, context, this); + self.mapConfigAdapter.getMapConfig(req.context.user, requestMapConfig, res.locals, context, this); }, function createLayergroup(err, requestMapConfig) { assert.ifError(err); @@ -182,7 +182,7 @@ MapController.prototype.create = function(req, res, prepareConfigFn, next) { mapConfig = new MapConfig(requestMapConfig, datasource); self.mapBackend.createLayergroup( mapConfig, req.params, - new CreateLayergroupMapConfigProvider(mapConfig, req.context.user, self.userLimitsApi, req.params), + new CreateLayergroupMapConfigProvider(mapConfig, req.context.user, self.userLimitsApi, res.locals), this ); }, @@ -257,8 +257,8 @@ MapController.prototype.instantiateTemplate = function(req, res, prepareParamsFn cdbuser, req.params.template_id, templateParams, - req.query.auth_token, - req.params + res.locals.auth_token, + res.locals ); mapConfigProvider.getMapConfig(this); }, @@ -344,7 +344,7 @@ function(req, res, mapconfig, layergroup, analysesResults, callback) { } }); - var dbName = req.params.dbname; + var dbName = res.locals.dbname; var layergroupId = layergroup.layergroupid; var dbConnection; From 482feabce286ed0a2fd7df0f8bc14bd38c5c1979 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Mart=C3=ADn?= Date: Fri, 29 Sep 2017 14:37:55 +0200 Subject: [PATCH 07/25] res.locals in named maps controller --- lib/cartodb/controllers/named_maps.js | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/lib/cartodb/controllers/named_maps.js b/lib/cartodb/controllers/named_maps.js index 3898b0c7..8b3765fe 100644 --- a/lib/cartodb/controllers/named_maps.js +++ b/lib/cartodb/controllers/named_maps.js @@ -97,7 +97,7 @@ NamedMapsController.prototype.tile = function(req, res, next) { req.params.template_id, req.query.config, req.query.auth_token, - req.params, + res.locals, this ); }, @@ -135,7 +135,7 @@ NamedMapsController.prototype.staticMap = function(req, res, next) { req.params.template_id, req.query.config, req.query.auth_token, - req.params, + res.locals, this ); }, @@ -144,7 +144,7 @@ NamedMapsController.prototype.staticMap = function(req, res, next) { namedMapProvider = _namedMapProvider; - self.prepareLayerFilterFromPreviewLayers(cdbUser, req, namedMapProvider, this); + self.prepareLayerFilterFromPreviewLayers(cdbUser, req, res, namedMapProvider, this); }, function prepareImageOptions(err) { assert.ifError(err); @@ -191,7 +191,13 @@ NamedMapsController.prototype.staticMap = function(req, res, next) { ); }; -NamedMapsController.prototype.prepareLayerFilterFromPreviewLayers = function (user, req, namedMapProvider, callback) { +NamedMapsController.prototype.prepareLayerFilterFromPreviewLayers = function ( + user, + req, + res, + namedMapProvider, + callback +) { var self = this; namedMapProvider.getTemplate(function (err, template) { if (err) { @@ -224,7 +230,7 @@ NamedMapsController.prototype.prepareLayerFilterFromPreviewLayers = function (us req.params.template_id, req.query.config, req.query.auth_token, - req.params, + res.locals, callback ); }); From c22a35489dd2cbcab37fc4de422de4aaecf82a55 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Mart=C3=ADn?= Date: Fri, 29 Sep 2017 14:38:28 +0200 Subject: [PATCH 08/25] res.locals forgotten things and make jshint happy --- lib/cartodb/controllers/layergroup.js | 8 +++++++- lib/cartodb/middleware/context/authorize.js | 2 -- lib/cartodb/middleware/context/index.js | 2 +- lib/cartodb/middleware/context/locals.js | 4 ++++ 4 files changed, 12 insertions(+), 4 deletions(-) diff --git a/lib/cartodb/controllers/layergroup.js b/lib/cartodb/controllers/layergroup.js index 700564fc..a68f22a7 100644 --- a/lib/cartodb/controllers/layergroup.js +++ b/lib/cartodb/controllers/layergroup.js @@ -177,7 +177,13 @@ LayergroupController.prototype.dataview = function(req, res, next) { var mapConfigProvider = new MapStoreMapConfigProvider( self.mapStore, req.context.user, self.userLimitsApi, res.locals ); - self.dataviewBackend.getDataview(mapConfigProvider, req.context.user, req.params.dataviewName, res.locals, this); + self.dataviewBackend.getDataview( + mapConfigProvider, + req.context.user, + req.params.dataviewName, + res.locals, + this + ); }, function finish(err, dataview, stats) { req.profiler.add(stats || {}); diff --git a/lib/cartodb/middleware/context/authorize.js b/lib/cartodb/middleware/context/authorize.js index bab100b0..a42b5407 100644 --- a/lib/cartodb/middleware/context/authorize.js +++ b/lib/cartodb/middleware/context/authorize.js @@ -1,5 +1,3 @@ -const _ = require('underscore'); - module.exports = function authorizeMiddleware (authApi) { return function (req, res, next) { req.profiler.done('req2params.setup'); diff --git a/lib/cartodb/middleware/context/index.js b/lib/cartodb/middleware/context/index.js index d660dcaf..640aafd6 100644 --- a/lib/cartodb/middleware/context/index.js +++ b/lib/cartodb/middleware/context/index.js @@ -1,4 +1,4 @@ -const locals = require('./locals') +const locals = require('./locals'); const cleanUpQueryParams = require('./clean-up-query-params'); const layergroupToken = require('./layergroup-token'); const authorize = require('./authorize'); diff --git a/lib/cartodb/middleware/context/locals.js b/lib/cartodb/middleware/context/locals.js index ec847788..a4df17ca 100644 --- a/lib/cartodb/middleware/context/locals.js +++ b/lib/cartodb/middleware/context/locals.js @@ -1,7 +1,11 @@ +const _ = require('underscore'); + module.exports = function layergroupTokenMiddleware(req, res, next) { // FIXME: Temporary hack to share data between middlewares. Express overrides req.params to // parse url params to an object and it's performed after matching path and controller. res.locals = {}; _.extend(res.locals, req.params); + + next(); } From 783eb0eec715107fd1b89631138e297cf0353df7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Mart=C3=ADn?= Date: Fri, 29 Sep 2017 17:03:57 +0200 Subject: [PATCH 09/25] res.locals format and layer in namep maps --- lib/cartodb/controllers/named_maps.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/cartodb/controllers/named_maps.js b/lib/cartodb/controllers/named_maps.js index 8b3765fe..e224fc7f 100644 --- a/lib/cartodb/controllers/named_maps.js +++ b/lib/cartodb/controllers/named_maps.js @@ -124,8 +124,8 @@ NamedMapsController.prototype.staticMap = function(req, res, next) { var cdbUser = req.context.user; var format = req.params.format === 'jpg' ? 'jpeg' : 'png'; - req.params.format = req.params.format || 'png'; - req.params.layer = req.params.layer || 'all'; + res.locals.format = req.params.format || 'png'; + res.locals.layer = req.params.layer || 'all'; var namedMapProvider; step( @@ -143,7 +143,7 @@ NamedMapsController.prototype.staticMap = function(req, res, next) { assert.ifError(err); namedMapProvider = _namedMapProvider; - + self.prepareLayerFilterFromPreviewLayers(cdbUser, req, res, namedMapProvider, this); }, function prepareImageOptions(err) { From f9d87bc40fb002599174748867ab28d4b7a44ff1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Mart=C3=ADn?= Date: Mon, 2 Oct 2017 12:07:35 +0200 Subject: [PATCH 10/25] res.locals fixing controllers --- lib/cartodb/controllers/layergroup.js | 38 +++++++++++++------ lib/cartodb/controllers/map.js | 3 +- lib/cartodb/controllers/named_maps.js | 6 +-- .../ported/support/ported_server_options.js | 3 ++ 4 files changed, 35 insertions(+), 15 deletions(-) diff --git a/lib/cartodb/controllers/layergroup.js b/lib/cartodb/controllers/layergroup.js index a68f22a7..9e93ea0f 100644 --- a/lib/cartodb/controllers/layergroup.js +++ b/lib/cartodb/controllers/layergroup.js @@ -321,25 +321,41 @@ LayergroupController.prototype.finalizeGetTileOrGrid = function(err, req, res, t }; LayergroupController.prototype.bbox = function(req, res, next) { - this.staticMap(req, res, +req.params.width, +req.params.height, { - west: +req.params.west, - north: +req.params.north, - east: +req.params.east, - south: +req.params.south - }, next); + this.staticMap( + req, + res, + +req.params.width, + +req.params.height, + null, + { + west: +req.params.west, + north: +req.params.north, + east: +req.params.east, + south: +req.params.south + }, + next + ); }; LayergroupController.prototype.center = function(req, res, next) { - this.staticMap(req, res, +req.params.width, +req.params.height, +req.params.z, { - lng: +req.params.lng, - lat: +req.params.lat - }, next); + this.staticMap( + req, + res, + +req.params.width, + +req.params.height, + +req.params.z, + { + lng: +req.params.lng, + lat: +req.params.lat + }, + next + ); }; LayergroupController.prototype.staticMap = function(req, res, width, height, zoom /* bounds */, center, next) { var format = req.params.format === 'jpg' ? 'jpeg' : 'png'; - req.params.layer = req.params.layer || 'all'; req.params.format = req.params.format || 'png'; + res.locals.layer = res.locals.layer || 'all'; var self = this; diff --git a/lib/cartodb/controllers/map.js b/lib/cartodb/controllers/map.js index 64ad1bb7..044a2b79 100644 --- a/lib/cartodb/controllers/map.js +++ b/lib/cartodb/controllers/map.js @@ -181,7 +181,8 @@ MapController.prototype.create = function(req, res, prepareConfigFn, next) { var datasource = context.datasource || Datasource.EmptyDatasource(); mapConfig = new MapConfig(requestMapConfig, datasource); self.mapBackend.createLayergroup( - mapConfig, req.params, + mapConfig, + res.locals, new CreateLayergroupMapConfigProvider(mapConfig, req.context.user, self.userLimitsApi, res.locals), this ); diff --git a/lib/cartodb/controllers/named_maps.js b/lib/cartodb/controllers/named_maps.js index e224fc7f..3f1c3437 100644 --- a/lib/cartodb/controllers/named_maps.js +++ b/lib/cartodb/controllers/named_maps.js @@ -125,7 +125,7 @@ NamedMapsController.prototype.staticMap = function(req, res, next) { var format = req.params.format === 'jpg' ? 'jpeg' : 'png'; res.locals.format = req.params.format || 'png'; - res.locals.layer = req.params.layer || 'all'; + res.locals.layer = res.locals.layer || 'all'; var namedMapProvider; step( @@ -148,7 +148,7 @@ NamedMapsController.prototype.staticMap = function(req, res, next) { }, function prepareImageOptions(err) { assert.ifError(err); - self.getStaticImageOptions(cdbUser, req.params, namedMapProvider, this); + self.getStaticImageOptions(cdbUser, res.locals, namedMapProvider, this); }, function getImage(err, imageOpts) { assert.ifError(err); @@ -222,7 +222,7 @@ NamedMapsController.prototype.prepareLayerFilterFromPreviewLayers = function ( } // overwrites 'all' default filter - req.params.layer = layerVisibilityFilter.join(','); + res.locals.layer = layerVisibilityFilter.join(','); // recreates the provider self.namedMapProviderCache.get( diff --git a/test/acceptance/ported/support/ported_server_options.js b/test/acceptance/ported/support/ported_server_options.js index 36684f77..25a25fc7 100644 --- a/test/acceptance/ported/support/ported_server_options.js +++ b/test/acceptance/ported/support/ported_server_options.js @@ -74,6 +74,9 @@ module.exports = _.extend({}, serverOptions, { } req.params.dbname = 'test_windshaft_cartodb_user_1_db'; + // add all params to res.locals + res.locals = _.extend({}, req.params); + // increment number of calls counter global.req2params_calls = global.req2params_calls ? global.req2params_calls + 1 : 1; From 55f593eae6d057e27e26e592a158dc82a3b46768 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Mart=C3=ADn?= Date: Mon, 2 Oct 2017 12:08:10 +0200 Subject: [PATCH 11/25] adding forgotten semicolon --- lib/cartodb/middleware/context/locals.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/cartodb/middleware/context/locals.js b/lib/cartodb/middleware/context/locals.js index a4df17ca..93f28c54 100644 --- a/lib/cartodb/middleware/context/locals.js +++ b/lib/cartodb/middleware/context/locals.js @@ -7,5 +7,5 @@ module.exports = function layergroupTokenMiddleware(req, res, next) { _.extend(res.locals, req.params); next(); -} +}; From aa625290415391d4010c1071ee911f2ae2510672 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Mart=C3=ADn?= Date: Mon, 2 Oct 2017 12:09:19 +0200 Subject: [PATCH 12/25] updating preprare-context test to allow the new res.locals usage --- test/unit/cartodb/prepare-context.test.js | 100 ++++++++++++++-------- 1 file changed, 62 insertions(+), 38 deletions(-) diff --git a/test/unit/cartodb/prepare-context.test.js b/test/unit/cartodb/prepare-context.test.js index a9119c13..0df4da24 100644 --- a/test/unit/cartodb/prepare-context.test.js +++ b/test/unit/cartodb/prepare-context.test.js @@ -10,6 +10,7 @@ var TemplateMaps = require('../../../lib/cartodb/backends/template_maps'); const cleanUpQueryParamsMiddleware = require('../../../lib/cartodb/middleware/context/clean-up-query-params'); const authorizeMiddleware = require('../../../lib/cartodb/middleware/context/authorize'); const dbConnSetupMiddleware = require('../../../lib/cartodb/middleware/context/db-conn-setup'); +const localsMiddleware = require('../../../lib/cartodb/middleware/context/locals'); var windshaft = require('windshaft'); @@ -43,60 +44,80 @@ describe('prepare-context', function() { assert.ok(_.isFunction(cleanUpQueryParams)); }); - function prepareRequest(req) { + function prepareRequest(req, res) { req.profiler = { done: function() {} }; req.context = { user: 'localhost' }; - req.params = {}; - return req; + res.locals = {}; + + return {req, res}; } - it('cleans up request', function(done){ - var req = {headers: { host:'localhost' }, query: {dbuser:'hacker',dbname:'secret'}}; - var res = {}; + it('res.locals are created', function(done) { + let req = {}; + let res = {}; - cleanUpQueryParams(prepareRequest(req), res, function(err) { - if ( err ) { done(err); return; } - assert.ok(_.isObject(req.query), 'request has query'); - assert.ok(!req.query.hasOwnProperty('dbuser'), 'dbuser was removed from query'); - assert.ok(req.hasOwnProperty('params'), 'request has params'); - assert.ok(!req.params.hasOwnProperty('interactivity'), 'request params do not have interactivity'); - done(); - }); + ({req, res} = prepareRequest(req, res)); + + localsMiddleware(req, res, function(err) { + if ( err ) { done(err); return; } + assert.ok(res.hasOwnProperty('locals'), 'response has locals'); + done(); + }); + }); + + it('cleans up request', function(done){ + var req = {headers: { host:'localhost' }, query: {dbuser:'hacker',dbname:'secret'}}; + var res = {}; + + ({req, res} = prepareRequest(req, res)); + + cleanUpQueryParams(req, res, function(err) { + if ( err ) { done(err); return; } + assert.ok(_.isObject(req.query), 'request has query'); + assert.ok(!req.query.hasOwnProperty('dbuser'), 'dbuser was removed from query'); + assert.ok(res.hasOwnProperty('locals'), 'response has locals'); + assert.ok(!res.locals.hasOwnProperty('interactivity'), 'response locals do not have interactivity'); + done(); + }); }); it('sets dbname from redis metadata', function(done){ - var req = {headers: { host:'localhost' }, query: {}, locals: {} }; - var res = {}; + var req = {headers: { host:'localhost' }, query: {} }; + var res = {}; - dbConnSetup(prepareRequest(req), res, function(err) { - if ( err ) { done(err); return; } - assert.ok(_.isObject(req.query), 'request has query'); - assert.ok(!req.query.hasOwnProperty('dbuser'), 'dbuser was removed from query'); - assert.ok(req.hasOwnProperty('params'), 'request has params'); - assert.ok(!req.params.hasOwnProperty('interactivity'), 'request params do not have interactivity'); - assert.equal(req.params.dbname, test_database); - assert.ok(req.params.dbuser === test_pubuser, 'could inject dbuser ('+req.params.dbuser+')'); - done(); - }); + ({req, res} = prepareRequest(req, res)); + + dbConnSetup(req, res, function(err) { + if ( err ) { done(err); return; } + assert.ok(_.isObject(req.query), 'request has query'); + assert.ok(!req.query.hasOwnProperty('dbuser'), 'dbuser was removed from query'); + assert.ok(res.hasOwnProperty('locals'), 'response has locals'); + assert.ok(!res.locals.hasOwnProperty('interactivity'), 'response locals do not have interactivity'); + assert.equal(res.locals.dbname, test_database); + assert.ok(res.locals.dbuser === test_pubuser, 'could inject dbuser ('+res.locals.dbuser+')'); + done(); + }); }); it('sets also dbuser for authenticated requests', function(done){ - var req = { headers: { host: 'localhost' }, query: { map_key: '1234' }, locals: {} }; + var req = { headers: { host: 'localhost' }, query: { map_key: '1234' }}; var res = {}; + ({req, res} = prepareRequest(req, res)); + // FIXME: review authorize-pgconnsetup workflow, It might we are doing authorization twice. - authorize(prepareRequest(req), res, function (err) { + authorize(req, res, function (err) { if (err) { done(err); return; } dbConnSetup(req, res, function(err) { if ( err ) { done(err); return; } assert.ok(_.isObject(req.query), 'request has query'); assert.ok(!req.query.hasOwnProperty('dbuser'), 'dbuser was removed from query'); - assert.ok(req.hasOwnProperty('params'), 'request has params'); - assert.ok(!req.params.hasOwnProperty('interactivity'), 'request params do not have interactivity'); - assert.equal(req.params.dbname, test_database); - assert.equal(req.params.dbuser, test_user); + assert.ok(res.hasOwnProperty('locals'), 'response has locals'); + assert.ok(!res.locals.hasOwnProperty('interactivity'), 'request params do not have interactivity'); + assert.equal(res.locals.dbname, test_database); + assert.equal(res.locals.dbuser, test_user); req = { headers: { @@ -108,9 +129,11 @@ describe('prepare-context', function() { locals: {} }; - dbConnSetup(prepareRequest(req), res, function(err, req) { + ({req, res} = prepareRequest(req, res)); + + dbConnSetup(req, res, function(err, req) { // wrong key resets params to no user - assert.ok(req.params.dbuser === test_pubuser, 'could inject dbuser ('+req.params.dbuser+')'); + assert.ok(res.locals.dbuser === test_pubuser, 'could inject dbuser ('+res.locals.dbuser+')'); done(); }); }); @@ -130,17 +153,18 @@ describe('prepare-context', function() { api_key: 'test', style: 'override', config: config - }, - locals: {} + } }; var res = {}; - cleanUpQueryParams(prepareRequest(req), res, function (err) { + ({req, res} = prepareRequest(req, res)); + + cleanUpQueryParams(req, res, function (err) { if ( err ) { return done(err); } - var query = req.params; + var query = res.locals; assert.deepEqual(config, query.config); assert.equal('test', query.api_key); assert.equal(undefined, query.non_included); From c8944141925206ad6b809c4aeae82dd083ddba27 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Mart=C3=ADn?= Date: Mon, 2 Oct 2017 12:28:29 +0200 Subject: [PATCH 13/25] going green --- test/unit/cartodb/prepare-context.test.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/unit/cartodb/prepare-context.test.js b/test/unit/cartodb/prepare-context.test.js index 0df4da24..d4e36378 100644 --- a/test/unit/cartodb/prepare-context.test.js +++ b/test/unit/cartodb/prepare-context.test.js @@ -125,13 +125,13 @@ describe('prepare-context', function() { }, query: { map_key: '1235' - }, - locals: {} + } }; ({req, res} = prepareRequest(req, res)); - dbConnSetup(req, res, function(err, req) { + dbConnSetup(req, res, function(err) { + if ( err ) { done(err); return; } // wrong key resets params to no user assert.ok(res.locals.dbuser === test_pubuser, 'could inject dbuser ('+res.locals.dbuser+')'); done(); From 430e1513d83f387002a16718653123a26289b3bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Mart=C3=ADn?= Date: Tue, 3 Oct 2017 13:00:52 +0200 Subject: [PATCH 14/25] fix incorrect function parameter --- lib/cartodb/controllers/layergroup.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/cartodb/controllers/layergroup.js b/lib/cartodb/controllers/layergroup.js index 9e93ea0f..3660948b 100644 --- a/lib/cartodb/controllers/layergroup.js +++ b/lib/cartodb/controllers/layergroup.js @@ -326,7 +326,6 @@ LayergroupController.prototype.bbox = function(req, res, next) { res, +req.params.width, +req.params.height, - null, { west: +req.params.west, north: +req.params.north, From 6bfc5d8891ce2c67f87c18c55c4caa660d2c3a55 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Mart=C3=ADn?= Date: Tue, 3 Oct 2017 13:03:02 +0200 Subject: [PATCH 15/25] fix function name and removing comments of localsMiddleware --- lib/cartodb/middleware/context/locals.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/cartodb/middleware/context/locals.js b/lib/cartodb/middleware/context/locals.js index 93f28c54..ce34f442 100644 --- a/lib/cartodb/middleware/context/locals.js +++ b/lib/cartodb/middleware/context/locals.js @@ -1,8 +1,6 @@ const _ = require('underscore'); -module.exports = function layergroupTokenMiddleware(req, res, next) { - // FIXME: Temporary hack to share data between middlewares. Express overrides req.params to - // parse url params to an object and it's performed after matching path and controller. +module.exports = function localsMiddleware(req, res, next) { res.locals = {}; _.extend(res.locals, req.params); From 3ce10690d60ce2220ef6f00efba0008b40afc2fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Mart=C3=ADn?= Date: Tue, 3 Oct 2017 13:06:12 +0200 Subject: [PATCH 16/25] send res.locals instead of res when possible --- lib/cartodb/api/auth_api.js | 10 +++++----- lib/cartodb/controllers/named_maps.js | 8 ++++---- lib/cartodb/middleware/context/authorize.js | 2 +- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/lib/cartodb/api/auth_api.js b/lib/cartodb/api/auth_api.js index 5b62ff44..709667f8 100644 --- a/lib/cartodb/api/auth_api.js +++ b/lib/cartodb/api/auth_api.js @@ -86,7 +86,7 @@ AuthApi.prototype.authorizedByAPIKey = function(user, req, callback) { * @param req - standard req object. Importantly contains table and host information * @param callback function(err, allowed) is access allowed not? */ -AuthApi.prototype.authorize = function(req, res, callback) { +AuthApi.prototype.authorize = function(req, params, callback) { var self = this; var user = req.context.user; @@ -101,11 +101,11 @@ AuthApi.prototype.authorize = function(req, res, callback) { // if not authorized by api_key, continue if (!authorized) { // not authorized by api_key, check if authorized by signer - return self.authorizedBySigner(res.locals, this); + return self.authorizedBySigner(params, this); } // authorized by api key, login as the given username and stop - self.pgConnection.setDBAuth(user, res.locals, function(err) { + self.pgConnection.setDBAuth(user, params, function(err) { callback(err, true); // authorized (or error) }); }, @@ -120,7 +120,7 @@ AuthApi.prototype.authorize = function(req, res, callback) { // if no signer name was given, let dbparams and // PostgreSQL do the rest. // - if ( ! res.locals.signer ) { + if ( ! params.signer ) { return callback(null, true); // authorized so far } @@ -128,7 +128,7 @@ AuthApi.prototype.authorize = function(req, res, callback) { return callback(null, false); } - self.pgConnection.setDBAuth(user, res.locals, function(err) { + self.pgConnection.setDBAuth(user, params, function(err) { req.profiler.done('setDBAuth'); callback(err, true); // authorized (or error) }); diff --git a/lib/cartodb/controllers/named_maps.js b/lib/cartodb/controllers/named_maps.js index 3f1c3437..5748cba7 100644 --- a/lib/cartodb/controllers/named_maps.js +++ b/lib/cartodb/controllers/named_maps.js @@ -144,7 +144,7 @@ NamedMapsController.prototype.staticMap = function(req, res, next) { namedMapProvider = _namedMapProvider; - self.prepareLayerFilterFromPreviewLayers(cdbUser, req, res, namedMapProvider, this); + self.prepareLayerFilterFromPreviewLayers(cdbUser, req, res.locals, namedMapProvider, this); }, function prepareImageOptions(err) { assert.ifError(err); @@ -194,7 +194,7 @@ NamedMapsController.prototype.staticMap = function(req, res, next) { NamedMapsController.prototype.prepareLayerFilterFromPreviewLayers = function ( user, req, - res, + params, namedMapProvider, callback ) { @@ -222,7 +222,7 @@ NamedMapsController.prototype.prepareLayerFilterFromPreviewLayers = function ( } // overwrites 'all' default filter - res.locals.layer = layerVisibilityFilter.join(','); + params.layer = layerVisibilityFilter.join(','); // recreates the provider self.namedMapProviderCache.get( @@ -230,7 +230,7 @@ NamedMapsController.prototype.prepareLayerFilterFromPreviewLayers = function ( req.params.template_id, req.query.config, req.query.auth_token, - res.locals, + params, callback ); }); diff --git a/lib/cartodb/middleware/context/authorize.js b/lib/cartodb/middleware/context/authorize.js index a42b5407..dd29f502 100644 --- a/lib/cartodb/middleware/context/authorize.js +++ b/lib/cartodb/middleware/context/authorize.js @@ -2,7 +2,7 @@ module.exports = function authorizeMiddleware (authApi) { return function (req, res, next) { req.profiler.done('req2params.setup'); - authApi.authorize(req, res, (err, authorized) => { + authApi.authorize(req, res.locals, (err, authorized) => { req.profiler.done('authorize'); if (err) { return next(err); From 21720267cfd32d944392a62b5173b39c811ed05b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Mart=C3=ADn?= Date: Tue, 3 Oct 2017 17:47:57 +0200 Subject: [PATCH 17/25] from req.context to res.locals --- lib/cartodb/api/auth_api.js | 2 +- lib/cartodb/controllers/analyses.js | 2 +- lib/cartodb/controllers/layergroup.js | 18 +++++++++--------- lib/cartodb/controllers/map.js | 16 ++++++++-------- lib/cartodb/controllers/named_maps.js | 6 +++--- lib/cartodb/controllers/named_maps_admin.js | 10 +++++----- lib/cartodb/middleware/allow-query-params.js | 2 +- .../context/clean-up-query-params.js | 6 +++--- .../middleware/context/db-conn-setup.js | 2 +- .../middleware/context/layergroup-token.js | 2 +- lib/cartodb/middleware/context/locals.js | 3 +-- lib/cartodb/middleware/user.js | 9 ++++++++- lib/cartodb/server.js | 3 ++- .../ported/support/ported_server_options.js | 2 +- test/unit/cartodb/prepare-context.test.js | 11 ++++++++--- 15 files changed, 53 insertions(+), 41 deletions(-) diff --git a/lib/cartodb/api/auth_api.js b/lib/cartodb/api/auth_api.js index 709667f8..e99d78b6 100644 --- a/lib/cartodb/api/auth_api.js +++ b/lib/cartodb/api/auth_api.js @@ -88,7 +88,7 @@ AuthApi.prototype.authorizedByAPIKey = function(user, req, callback) { */ AuthApi.prototype.authorize = function(req, params, callback) { var self = this; - var user = req.context.user; + var user = params.user; step( function () { diff --git a/lib/cartodb/controllers/analyses.js b/lib/cartodb/controllers/analyses.js index 22db4014..7b140e8f 100644 --- a/lib/cartodb/controllers/analyses.js +++ b/lib/cartodb/controllers/analyses.js @@ -40,7 +40,7 @@ AnalysesController.prototype.sendResponse = function(req, res, resource) { AnalysesController.prototype.catalog = function (req, res, next) { var self = this; - var username = req.context.user; + var username = res.locals.user; step( function catalogQuery() { diff --git a/lib/cartodb/controllers/layergroup.js b/lib/cartodb/controllers/layergroup.js index 3660948b..41a37b31 100644 --- a/lib/cartodb/controllers/layergroup.js +++ b/lib/cartodb/controllers/layergroup.js @@ -175,11 +175,11 @@ LayergroupController.prototype.dataview = function(req, res, next) { step( function retrieveDataview() { var mapConfigProvider = new MapStoreMapConfigProvider( - self.mapStore, req.context.user, self.userLimitsApi, res.locals + self.mapStore, res.locals.user, self.userLimitsApi, res.locals ); self.dataviewBackend.getDataview( mapConfigProvider, - req.context.user, + res.locals.user, req.params.dataviewName, res.locals, this @@ -204,9 +204,9 @@ LayergroupController.prototype.dataviewSearch = function(req, res, next) { step( function searchDataview() { var mapConfigProvider = new MapStoreMapConfigProvider( - self.mapStore, req.context.user, self.userLimitsApi, res.locals + self.mapStore, res.locals.user, self.userLimitsApi, res.locals ); - self.dataviewBackend.search(mapConfigProvider, req.context.user, req.params.dataviewName, res.locals, this); + self.dataviewBackend.search(mapConfigProvider, res.locals.user, req.params.dataviewName, res.locals, this); }, function finish(err, searchResult, stats) { req.profiler.add(stats || {}); @@ -230,7 +230,7 @@ LayergroupController.prototype.attributes = function(req, res, next) { step( function retrieveFeatureAttributes() { var mapConfigProvider = new MapStoreMapConfigProvider( - self.mapStore, req.context.user, self.userLimitsApi, res.locals + self.mapStore, res.locals.user, self.userLimitsApi, res.locals ); self.attributesBackend.getFeatureAttributes(mapConfigProvider, res.locals, false, this); }, @@ -266,7 +266,7 @@ LayergroupController.prototype.tileOrLayer = function (req, res, next) { step( function mapController$getTileOrGrid() { self.tileBackend.getTile( - new MapStoreMapConfigProvider(self.mapStore, req.context.user, self.userLimitsApi, res.locals), + new MapStoreMapConfigProvider(self.mapStore, res.locals.user, self.userLimitsApi, res.locals), req.params, this ); }, @@ -362,11 +362,11 @@ LayergroupController.prototype.staticMap = function(req, res, width, height, zoo function getImage() { if (center) { self.previewBackend.getImage( - new MapStoreMapConfigProvider(self.mapStore, req.context.user, self.userLimitsApi, res.locals), + new MapStoreMapConfigProvider(self.mapStore, res.locals.user, self.userLimitsApi, res.locals), format, width, height, zoom, center, this); } else { self.previewBackend.getImage( - new MapStoreMapConfigProvider(self.mapStore, req.context.user, self.userLimitsApi, res.locals), + new MapStoreMapConfigProvider(self.mapStore, res.locals.user, self.userLimitsApi, res.locals), format, width, height, zoom /* bounds */, this); } }, @@ -405,7 +405,7 @@ LayergroupController.prototype.sendResponse = function(req, res, body, status, h var dbName = res.locals.dbname; step( function getAffectedTables() { - self.getAffectedTables(req.context.user, dbName, res.locals.token, this); + self.getAffectedTables(res.locals.user, dbName, res.locals.token, this); }, function sendResponse(err, affectedTables) { req.profiler.done('affectedTables'); diff --git a/lib/cartodb/controllers/map.js b/lib/cartodb/controllers/map.js index 044a2b79..f2e86d13 100644 --- a/lib/cartodb/controllers/map.js +++ b/lib/cartodb/controllers/map.js @@ -161,7 +161,7 @@ MapController.prototype.create = function(req, res, prepareConfigFn, next) { function prepareAdapterMapConfig(err, requestMapConfig) { assert.ifError(err); context.analysisConfiguration = { - user: req.context.user, + user: res.locals.user, db: { host: res.locals.dbhost, port: res.locals.dbport, @@ -170,11 +170,11 @@ MapController.prototype.create = function(req, res, prepareConfigFn, next) { pass: res.locals.dbpassword }, batch: { - username: req.context.user, + username: res.locals.user, apiKey: res.locals.api_key } }; - self.mapConfigAdapter.getMapConfig(req.context.user, requestMapConfig, res.locals, context, this); + self.mapConfigAdapter.getMapConfig(res.locals.user, requestMapConfig, res.locals, context, this); }, function createLayergroup(err, requestMapConfig) { assert.ifError(err); @@ -183,7 +183,7 @@ MapController.prototype.create = function(req, res, prepareConfigFn, next) { self.mapBackend.createLayergroup( mapConfig, res.locals, - new CreateLayergroupMapConfigProvider(mapConfig, req.context.user, self.userLimitsApi, res.locals), + new CreateLayergroupMapConfigProvider(mapConfig, res.locals.user, self.userLimitsApi, res.locals), this ); }, @@ -215,8 +215,8 @@ MapController.prototype.create = function(req, res, prepareConfigFn, next) { next(err); } else { var analysesResults = context.analysesResults || []; - self.addDataviewsAndWidgetsUrls(req.context.user, layergroup, mapConfig.obj()); - self.addAnalysesMetadata(req.context.user, layergroup, analysesResults, true); + self.addDataviewsAndWidgetsUrls(res.locals.user, layergroup, mapConfig.obj()); + self.addAnalysesMetadata(res.locals.user, layergroup, analysesResults, true); addContextMetadata(layergroup, mapConfig.obj(), context); res.set('X-Layergroup-Id', layergroup.layergroupid); self.send(req, res, layergroup, 200); @@ -239,7 +239,7 @@ function addContextMetadata(layergroup, mapConfig, context) { MapController.prototype.instantiateTemplate = function(req, res, prepareParamsFn, next) { var self = this; - var cdbuser = req.context.user; + var cdbuser = res.locals.user; var mapConfigProvider; var mapConfig; @@ -304,7 +304,7 @@ MapController.prototype.afterLayergroupCreate = function(req, res, mapconfig, layergroup, analysesResults, callback) { var self = this; - var username = req.context.user; + var username = res.locals.user; var tasksleft = 2; // redis key and affectedTables var errors = []; diff --git a/lib/cartodb/controllers/named_maps.js b/lib/cartodb/controllers/named_maps.js index 5748cba7..759ae4b6 100644 --- a/lib/cartodb/controllers/named_maps.js +++ b/lib/cartodb/controllers/named_maps.js @@ -47,7 +47,7 @@ NamedMapsController.prototype.register = function(app) { }; NamedMapsController.prototype.sendResponse = function(req, res, resource, headers, namedMapProvider) { - this.surrogateKeysCache.tag(res, new NamedMapsCacheEntry(req.context.user, namedMapProvider.getTemplateName())); + this.surrogateKeysCache.tag(res, new NamedMapsCacheEntry(res.locals.user, namedMapProvider.getTemplateName())); res.set('Content-Type', headers['content-type'] || headers['Content-Type'] || 'image/png'); res.set('Cache-Control', 'public,max-age=7200,must-revalidate'); @@ -87,7 +87,7 @@ NamedMapsController.prototype.sendResponse = function(req, res, resource, header NamedMapsController.prototype.tile = function(req, res, next) { var self = this; - var cdbUser = req.context.user; + var cdbUser = res.locals.user; var namedMapProvider; step( @@ -121,7 +121,7 @@ NamedMapsController.prototype.tile = function(req, res, next) { NamedMapsController.prototype.staticMap = function(req, res, next) { var self = this; - var cdbUser = req.context.user; + var cdbUser = res.locals.user; var format = req.params.format === 'jpg' ? 'jpeg' : 'png'; res.locals.format = req.params.format || 'png'; diff --git a/lib/cartodb/controllers/named_maps_admin.js b/lib/cartodb/controllers/named_maps_admin.js index a4a126d5..f9e68184 100644 --- a/lib/cartodb/controllers/named_maps_admin.js +++ b/lib/cartodb/controllers/named_maps_admin.js @@ -44,7 +44,7 @@ NamedMapsAdminController.prototype.register = function (router) { NamedMapsAdminController.prototype.create = function(req, res, next) { var self = this; - var cdbuser = req.context.user; + var cdbuser = res.locals.user; step( function checkPerms(){ @@ -68,7 +68,7 @@ NamedMapsAdminController.prototype.create = function(req, res, next) { NamedMapsAdminController.prototype.update = function(req, res, next) { var self = this; - var cdbuser = req.context.user; + var cdbuser = res.locals.user; var template; var tpl_id; @@ -99,7 +99,7 @@ NamedMapsAdminController.prototype.retrieve = function(req, res, next) { req.profiler.start('windshaft-cartodb.get_template'); - var cdbuser = req.context.user; + var cdbuser = res.locals.user; var tpl_id; step( function checkPerms(){ @@ -133,7 +133,7 @@ NamedMapsAdminController.prototype.destroy = function(req, res, next) { req.profiler.start('windshaft-cartodb.delete_template'); - var cdbuser = req.context.user; + var cdbuser = res.locals.user; var tpl_id; step( function checkPerms(){ @@ -158,7 +158,7 @@ NamedMapsAdminController.prototype.list = function(req, res, next) { var self = this; req.profiler.start('windshaft-cartodb.get_template_list'); - var cdbuser = req.context.user; + var cdbuser = res.locals.user; step( function checkPerms(){ diff --git a/lib/cartodb/middleware/allow-query-params.js b/lib/cartodb/middleware/allow-query-params.js index 04a27033..7ec31d74 100644 --- a/lib/cartodb/middleware/allow-query-params.js +++ b/lib/cartodb/middleware/allow-query-params.js @@ -3,7 +3,7 @@ module.exports = function allowQueryParams(params) { throw new Error('allowQueryParams must receive an Array of params'); } return function allowQueryParamsMiddleware(req, res, next) { - req.context.allowedQueryParams = params; + res.locals.allowedQueryParams = params; next(); }; }; diff --git a/lib/cartodb/middleware/context/clean-up-query-params.js b/lib/cartodb/middleware/context/clean-up-query-params.js index 4cc1ceb6..280fe986 100644 --- a/lib/cartodb/middleware/context/clean-up-query-params.js +++ b/lib/cartodb/middleware/context/clean-up-query-params.js @@ -18,15 +18,15 @@ module.exports = function cleanUpQueryParamsMiddleware () { return function cleanUpQueryParams (req, res, next) { var allowedQueryParams = REQUEST_QUERY_PARAMS_WHITELIST; - if (Array.isArray(req.context.allowedQueryParams)) { - allowedQueryParams = allowedQueryParams.concat(req.context.allowedQueryParams); + if (Array.isArray(res.locals.allowedQueryParams)) { + allowedQueryParams = allowedQueryParams.concat(res.locals.allowedQueryParams); } req.query = _.pick(req.query, allowedQueryParams); // bring all query values onto res.locals object _.extend(res.locals, req.query); - + next(); }; }; diff --git a/lib/cartodb/middleware/context/db-conn-setup.js b/lib/cartodb/middleware/context/db-conn-setup.js index 634711f7..a39466fe 100644 --- a/lib/cartodb/middleware/context/db-conn-setup.js +++ b/lib/cartodb/middleware/context/db-conn-setup.js @@ -2,7 +2,7 @@ const _ = require('underscore'); module.exports = function dbConnSetupMiddleware(pgConnection) { return function (req, res, next) { - const user = req.context.user; + const user = res.locals.user; pgConnection.setDBConn(user, res.locals, (err) => { if (err) { diff --git a/lib/cartodb/middleware/context/layergroup-token.js b/lib/cartodb/middleware/context/layergroup-token.js index b90d5e13..63524942 100644 --- a/lib/cartodb/middleware/context/layergroup-token.js +++ b/lib/cartodb/middleware/context/layergroup-token.js @@ -5,7 +5,7 @@ module.exports = function layergroupTokenMiddleware(req, res, next) { return next(); } - var user = req.context.user; + var user = res.locals.user; var layergroupToken = LayergroupToken.parse(res.locals.token); res.locals.token = layergroupToken.token; diff --git a/lib/cartodb/middleware/context/locals.js b/lib/cartodb/middleware/context/locals.js index ce34f442..bb7d5ee2 100644 --- a/lib/cartodb/middleware/context/locals.js +++ b/lib/cartodb/middleware/context/locals.js @@ -1,9 +1,8 @@ const _ = require('underscore'); module.exports = function localsMiddleware(req, res, next) { - res.locals = {}; _.extend(res.locals, req.params); - + next(); }; diff --git a/lib/cartodb/middleware/user.js b/lib/cartodb/middleware/user.js index 40934849..ce3fdd29 100644 --- a/lib/cartodb/middleware/user.js +++ b/lib/cartodb/middleware/user.js @@ -2,6 +2,13 @@ var CdbRequest = require('../models/cdb_request'); var cdbRequest = new CdbRequest(); module.exports = function userMiddleware(req, res, next) { - req.context.user = cdbRequest.userByReq(req); + res.locals.user = cdbRequest.userByReq(req); + + // avoid a req.params.user equals to undefined + // overwrites res.locals.user + if(!req.params.user) { + delete req.params.user; + } + next(); }; diff --git a/lib/cartodb/server.js b/lib/cartodb/server.js index 51841dca..44be69a1 100644 --- a/lib/cartodb/server.js +++ b/lib/cartodb/server.js @@ -369,7 +369,8 @@ function bootstrap(opts) { app.use(bodyParser.json()); app.use(function bootstrap$prepareRequestResponse(req, res, next) { - req.context = req.context || {}; + res.locals = {}; + req.profiler = new Profiler({ statsd_client: global.statsClient, profile: opts.useProfiler diff --git a/test/acceptance/ported/support/ported_server_options.js b/test/acceptance/ported/support/ported_server_options.js index 25a25fc7..8604e68c 100644 --- a/test/acceptance/ported/support/ported_server_options.js +++ b/test/acceptance/ported/support/ported_server_options.js @@ -63,7 +63,7 @@ module.exports = _.extend({}, serverOptions, { _.extend(req.params, req.query); req.params.user = 'localhost'; - req.context = {user: 'localhost'}; + res.locals.user = 'localhost'; req.params.dbhost = global.environment.postgres.host; req.params.dbport = req.params.dbport || global.environment.postgres.port; diff --git a/test/unit/cartodb/prepare-context.test.js b/test/unit/cartodb/prepare-context.test.js index d4e36378..0cbc6b85 100644 --- a/test/unit/cartodb/prepare-context.test.js +++ b/test/unit/cartodb/prepare-context.test.js @@ -48,8 +48,11 @@ describe('prepare-context', function() { req.profiler = { done: function() {} }; - req.context = { user: 'localhost' }; - res.locals = {}; + + if(!res.locals) { + res.locals = {}; + } + res.locals.user = 'localhost'; return {req, res}; } @@ -128,8 +131,10 @@ describe('prepare-context', function() { } }; + res = {}; + ({req, res} = prepareRequest(req, res)); - + dbConnSetup(req, res, function(err) { if ( err ) { done(err); return; } // wrong key resets params to no user From 1c3f2b93e3ea8a71a2f43586849b372edff5e8f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Mart=C3=ADn?= Date: Tue, 3 Oct 2017 17:58:16 +0200 Subject: [PATCH 18/25] prepareRequest and prepareResponse in prepare-context.test --- test/unit/cartodb/prepare-context.test.js | 32 +++++++++-------------- 1 file changed, 12 insertions(+), 20 deletions(-) diff --git a/test/unit/cartodb/prepare-context.test.js b/test/unit/cartodb/prepare-context.test.js index 0cbc6b85..66d03bf5 100644 --- a/test/unit/cartodb/prepare-context.test.js +++ b/test/unit/cartodb/prepare-context.test.js @@ -44,26 +44,28 @@ describe('prepare-context', function() { assert.ok(_.isFunction(cleanUpQueryParams)); }); - function prepareRequest(req, res) { + function prepareRequest(req) { req.profiler = { done: function() {} }; + return req; + } + + function prepareResponse(res) { if(!res.locals) { res.locals = {}; } res.locals.user = 'localhost'; - return {req, res}; + return res; } it('res.locals are created', function(done) { let req = {}; let res = {}; - ({req, res} = prepareRequest(req, res)); - - localsMiddleware(req, res, function(err) { + localsMiddleware(prepareRequest(req), prepareResponse(res), function(err) { if ( err ) { done(err); return; } assert.ok(res.hasOwnProperty('locals'), 'response has locals'); done(); @@ -74,9 +76,7 @@ describe('prepare-context', function() { var req = {headers: { host:'localhost' }, query: {dbuser:'hacker',dbname:'secret'}}; var res = {}; - ({req, res} = prepareRequest(req, res)); - - cleanUpQueryParams(req, res, function(err) { + cleanUpQueryParams(prepareRequest(req), prepareResponse(res), function(err) { if ( err ) { done(err); return; } assert.ok(_.isObject(req.query), 'request has query'); assert.ok(!req.query.hasOwnProperty('dbuser'), 'dbuser was removed from query'); @@ -90,9 +90,7 @@ describe('prepare-context', function() { var req = {headers: { host:'localhost' }, query: {} }; var res = {}; - ({req, res} = prepareRequest(req, res)); - - dbConnSetup(req, res, function(err) { + dbConnSetup(prepareRequest(req), prepareResponse(res), function(err) { if ( err ) { done(err); return; } assert.ok(_.isObject(req.query), 'request has query'); assert.ok(!req.query.hasOwnProperty('dbuser'), 'dbuser was removed from query'); @@ -108,10 +106,8 @@ describe('prepare-context', function() { var req = { headers: { host: 'localhost' }, query: { map_key: '1234' }}; var res = {}; - ({req, res} = prepareRequest(req, res)); - // FIXME: review authorize-pgconnsetup workflow, It might we are doing authorization twice. - authorize(req, res, function (err) { + authorize(prepareRequest(req), prepareResponse(res), function (err) { if (err) { done(err); return; } dbConnSetup(req, res, function(err) { if ( err ) { done(err); return; } @@ -133,9 +129,7 @@ describe('prepare-context', function() { res = {}; - ({req, res} = prepareRequest(req, res)); - - dbConnSetup(req, res, function(err) { + dbConnSetup(prepareRequest(req), prepareResponse(res), function(err) { if ( err ) { done(err); return; } // wrong key resets params to no user assert.ok(res.locals.dbuser === test_pubuser, 'could inject dbuser ('+res.locals.dbuser+')'); @@ -161,10 +155,8 @@ describe('prepare-context', function() { } }; var res = {}; - - ({req, res} = prepareRequest(req, res)); - cleanUpQueryParams(req, res, function (err) { + cleanUpQueryParams(prepareRequest(req), prepareResponse(res), function (err) { if ( err ) { return done(err); } From ec8fcc7302993000f96ae02f11d53571f526d5f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Mart=C3=ADn?= Date: Wed, 4 Oct 2017 12:50:27 +0200 Subject: [PATCH 19/25] change param name and comments updated --- lib/cartodb/api/auth_api.js | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/lib/cartodb/api/auth_api.js b/lib/cartodb/api/auth_api.js index e99d78b6..6a639aae 100644 --- a/lib/cartodb/api/auth_api.js +++ b/lib/cartodb/api/auth_api.js @@ -19,22 +19,22 @@ function AuthApi(pgConnection, metadataBackend, mapStore, templateMaps) { module.exports = AuthApi; -// Check if a request is authorized by a signer +// Check if the user is authorized by a signer // -// @param req express request object +// @param res express response object // @param callback function(err, signed_by) signed_by will be // null if the request is not signed by anyone // or will be a string cartodb username otherwise. // -AuthApi.prototype.authorizedBySigner = function(locals, callback) { - if ( ! locals.token || ! locals.signer ) { +AuthApi.prototype.authorizedBySigner = function(params, callback) { + if ( ! params.token || ! params.signer ) { return callback(null, false); // no signer requested } var self = this; - var layergroup_id = locals.token; - var auth_token = locals.auth_token; + var layergroup_id = params.token; + var auth_token = params.auth_token; this.mapStore.load(layergroup_id, function(err, mapConfig) { if (err) { @@ -84,6 +84,7 @@ AuthApi.prototype.authorizedByAPIKey = function(user, req, callback) { * Check access authorization * * @param req - standard req object. Importantly contains table and host information + * @param params res.locals parameters. Contains the auth parameters * @param callback function(err, allowed) is access allowed not? */ AuthApi.prototype.authorize = function(req, params, callback) { From 1f03a6b181aff9b9f5e781189827ec114f68f1ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Mart=C3=ADn?= Date: Thu, 5 Oct 2017 11:28:41 +0200 Subject: [PATCH 20/25] using res.locals instead of params in AuthApi --- lib/cartodb/api/auth_api.js | 20 ++++++++++---------- lib/cartodb/middleware/context/authorize.js | 2 +- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/lib/cartodb/api/auth_api.js b/lib/cartodb/api/auth_api.js index 6a639aae..4bc3f050 100644 --- a/lib/cartodb/api/auth_api.js +++ b/lib/cartodb/api/auth_api.js @@ -26,15 +26,15 @@ module.exports = AuthApi; // null if the request is not signed by anyone // or will be a string cartodb username otherwise. // -AuthApi.prototype.authorizedBySigner = function(params, callback) { - if ( ! params.token || ! params.signer ) { +AuthApi.prototype.authorizedBySigner = function(res, callback) { + if ( ! res.locals.token || ! res.locals.signer ) { return callback(null, false); // no signer requested } var self = this; - var layergroup_id = params.token; - var auth_token = params.auth_token; + var layergroup_id = res.locals.token; + var auth_token = res.locals.auth_token; this.mapStore.load(layergroup_id, function(err, mapConfig) { if (err) { @@ -87,9 +87,9 @@ AuthApi.prototype.authorizedByAPIKey = function(user, req, callback) { * @param params res.locals parameters. Contains the auth parameters * @param callback function(err, allowed) is access allowed not? */ -AuthApi.prototype.authorize = function(req, params, callback) { +AuthApi.prototype.authorize = function(req, res, callback) { var self = this; - var user = params.user; + var user = res.locals.user; step( function () { @@ -102,11 +102,11 @@ AuthApi.prototype.authorize = function(req, params, callback) { // if not authorized by api_key, continue if (!authorized) { // not authorized by api_key, check if authorized by signer - return self.authorizedBySigner(params, this); + return self.authorizedBySigner(res, this); } // authorized by api key, login as the given username and stop - self.pgConnection.setDBAuth(user, params, function(err) { + self.pgConnection.setDBAuth(user, res.locals, function(err) { callback(err, true); // authorized (or error) }); }, @@ -121,7 +121,7 @@ AuthApi.prototype.authorize = function(req, params, callback) { // if no signer name was given, let dbparams and // PostgreSQL do the rest. // - if ( ! params.signer ) { + if ( ! res.locals.signer ) { return callback(null, true); // authorized so far } @@ -129,7 +129,7 @@ AuthApi.prototype.authorize = function(req, params, callback) { return callback(null, false); } - self.pgConnection.setDBAuth(user, params, function(err) { + self.pgConnection.setDBAuth(user, res.locals, function(err) { req.profiler.done('setDBAuth'); callback(err, true); // authorized (or error) }); diff --git a/lib/cartodb/middleware/context/authorize.js b/lib/cartodb/middleware/context/authorize.js index dd29f502..a42b5407 100644 --- a/lib/cartodb/middleware/context/authorize.js +++ b/lib/cartodb/middleware/context/authorize.js @@ -2,7 +2,7 @@ module.exports = function authorizeMiddleware (authApi) { return function (req, res, next) { req.profiler.done('req2params.setup'); - authApi.authorize(req, res.locals, (err, authorized) => { + authApi.authorize(req, res, (err, authorized) => { req.profiler.done('authorize'); if (err) { return next(err); From 5abe25c316d77564a5778f93727f302a5eaa90a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Mart=C3=ADn?= Date: Thu, 5 Oct 2017 11:35:49 +0200 Subject: [PATCH 21/25] undo style/format changes --- lib/cartodb/controllers/layergroup.js | 35 ++++++------------ test/unit/cartodb/prepare-context.test.js | 44 +++++++++++------------ 2 files changed, 32 insertions(+), 47 deletions(-) diff --git a/lib/cartodb/controllers/layergroup.js b/lib/cartodb/controllers/layergroup.js index 41a37b31..9b0c1b51 100644 --- a/lib/cartodb/controllers/layergroup.js +++ b/lib/cartodb/controllers/layergroup.js @@ -321,34 +321,19 @@ LayergroupController.prototype.finalizeGetTileOrGrid = function(err, req, res, t }; LayergroupController.prototype.bbox = function(req, res, next) { - this.staticMap( - req, - res, - +req.params.width, - +req.params.height, - { - west: +req.params.west, - north: +req.params.north, - east: +req.params.east, - south: +req.params.south - }, - next - ); + this.staticMap(req, res, +req.params.width, +req.params.height, { + west: +req.params.west, + north: +req.params.north, + east: +req.params.east, + south: +req.params.south + }, next); }; LayergroupController.prototype.center = function(req, res, next) { - this.staticMap( - req, - res, - +req.params.width, - +req.params.height, - +req.params.z, - { - lng: +req.params.lng, - lat: +req.params.lat - }, - next - ); + this.staticMap(req, res, +req.params.width, +req.params.height, +req.params.z, { + lng: +req.params.lng, + lat: +req.params.lat + }, next); }; LayergroupController.prototype.staticMap = function(req, res, width, height, zoom /* bounds */, center, next) { diff --git a/test/unit/cartodb/prepare-context.test.js b/test/unit/cartodb/prepare-context.test.js index 66d03bf5..a30f7245 100644 --- a/test/unit/cartodb/prepare-context.test.js +++ b/test/unit/cartodb/prepare-context.test.js @@ -73,33 +73,33 @@ describe('prepare-context', function() { }); it('cleans up request', function(done){ - var req = {headers: { host:'localhost' }, query: {dbuser:'hacker',dbname:'secret'}}; - var res = {}; + var req = {headers: { host:'localhost' }, query: {dbuser:'hacker',dbname:'secret'}}; + var res = {}; - cleanUpQueryParams(prepareRequest(req), prepareResponse(res), function(err) { - if ( err ) { done(err); return; } - assert.ok(_.isObject(req.query), 'request has query'); - assert.ok(!req.query.hasOwnProperty('dbuser'), 'dbuser was removed from query'); - assert.ok(res.hasOwnProperty('locals'), 'response has locals'); - assert.ok(!res.locals.hasOwnProperty('interactivity'), 'response locals do not have interactivity'); - done(); - }); + cleanUpQueryParams(prepareRequest(req), prepareResponse(res), function(err) { + if ( err ) { done(err); return; } + assert.ok(_.isObject(req.query), 'request has query'); + assert.ok(!req.query.hasOwnProperty('dbuser'), 'dbuser was removed from query'); + assert.ok(res.hasOwnProperty('locals'), 'response has locals'); + assert.ok(!res.locals.hasOwnProperty('interactivity'), 'response locals do not have interactivity'); + done(); + }); }); it('sets dbname from redis metadata', function(done){ - var req = {headers: { host:'localhost' }, query: {} }; - var res = {}; + var req = {headers: { host:'localhost' }, query: {} }; + var res = {}; - dbConnSetup(prepareRequest(req), prepareResponse(res), function(err) { - if ( err ) { done(err); return; } - assert.ok(_.isObject(req.query), 'request has query'); - assert.ok(!req.query.hasOwnProperty('dbuser'), 'dbuser was removed from query'); - assert.ok(res.hasOwnProperty('locals'), 'response has locals'); - assert.ok(!res.locals.hasOwnProperty('interactivity'), 'response locals do not have interactivity'); - assert.equal(res.locals.dbname, test_database); - assert.ok(res.locals.dbuser === test_pubuser, 'could inject dbuser ('+res.locals.dbuser+')'); - done(); - }); + dbConnSetup(prepareRequest(req), prepareResponse(res), function(err) { + if ( err ) { done(err); return; } + assert.ok(_.isObject(req.query), 'request has query'); + assert.ok(!req.query.hasOwnProperty('dbuser'), 'dbuser was removed from query'); + assert.ok(res.hasOwnProperty('locals'), 'response has locals'); + assert.ok(!res.locals.hasOwnProperty('interactivity'), 'response locals do not have interactivity'); + assert.equal(res.locals.dbname, test_database); + assert.ok(res.locals.dbuser === test_pubuser, 'could inject dbuser ('+res.locals.dbuser+')'); + done(); + }); }); it('sets also dbuser for authenticated requests', function(done){ From 9083fc2e20ff5e5dd911c06774e93546e139845e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Mart=C3=ADn?= Date: Thu, 5 Oct 2017 12:44:03 +0200 Subject: [PATCH 22/25] fix forgotten comment --- lib/cartodb/api/auth_api.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/cartodb/api/auth_api.js b/lib/cartodb/api/auth_api.js index 4bc3f050..562614c3 100644 --- a/lib/cartodb/api/auth_api.js +++ b/lib/cartodb/api/auth_api.js @@ -84,7 +84,7 @@ AuthApi.prototype.authorizedByAPIKey = function(user, req, callback) { * Check access authorization * * @param req - standard req object. Importantly contains table and host information - * @param params res.locals parameters. Contains the auth parameters + * @param res - standard res object. Contains the auth parameters in locals * @param callback function(err, allowed) is access allowed not? */ AuthApi.prototype.authorize = function(req, res, callback) { From 2f310a15bdd26bfa947aa497c165976a12c37f63 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Mart=C3=ADn?= Date: Thu, 5 Oct 2017 17:23:07 +0200 Subject: [PATCH 23/25] do not overwrite creation of res.locals --- lib/cartodb/middleware/context/layergroup-token.js | 2 +- lib/cartodb/server.js | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/cartodb/middleware/context/layergroup-token.js b/lib/cartodb/middleware/context/layergroup-token.js index 63524942..026d0806 100644 --- a/lib/cartodb/middleware/context/layergroup-token.js +++ b/lib/cartodb/middleware/context/layergroup-token.js @@ -1,7 +1,7 @@ var LayergroupToken = require('../../models/layergroup-token'); module.exports = function layergroupTokenMiddleware(req, res, next) { - if (!res.locals.hasOwnProperty('token')) { + if (!res.locals.token) { return next(); } diff --git a/lib/cartodb/server.js b/lib/cartodb/server.js index 44be69a1..ffd7efda 100644 --- a/lib/cartodb/server.js +++ b/lib/cartodb/server.js @@ -369,8 +369,6 @@ function bootstrap(opts) { app.use(bodyParser.json()); app.use(function bootstrap$prepareRequestResponse(req, res, next) { - res.locals = {}; - req.profiler = new Profiler({ statsd_client: global.statsClient, profile: opts.useProfiler From e3405ea2fc3ab6e43c0e40f423e2357bbd69dc55 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Mart=C3=ADn?= Date: Mon, 9 Oct 2017 12:27:58 +0200 Subject: [PATCH 24/25] doing changes after merge with middlewarify --- lib/cartodb/middleware/context/db-conn-setup.js | 7 +++---- test/unit/cartodb/prepare-context.test.js | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/lib/cartodb/middleware/context/db-conn-setup.js b/lib/cartodb/middleware/context/db-conn-setup.js index 90b76818..dec48f6d 100644 --- a/lib/cartodb/middleware/context/db-conn-setup.js +++ b/lib/cartodb/middleware/context/db-conn-setup.js @@ -3,7 +3,6 @@ const _ = require('underscore'); module.exports = function dbConnSetupMiddleware(pgConnection) { return function dbConnSetup(req, res, next) { const user = res.locals.user; - pgConnection.setDBConn(user, res.locals, (err) => { if (err) { if (err.message && -1 !== err.message.indexOf('name not found')) { @@ -21,11 +20,11 @@ module.exports = function dbConnSetupMiddleware(pgConnection) { dbhost: global.environment.postgres.host, dbport: global.environment.postgres.port }); - - res.set('X-Served-By-DB-Host', req.params.dbhost); + + res.set('X-Served-By-DB-Host', res.locals.dbhost); req.profiler.done('req2params'); - + next(null); }); }; diff --git a/test/unit/cartodb/prepare-context.test.js b/test/unit/cartodb/prepare-context.test.js index fc8c9bd0..dbbfb8bf 100644 --- a/test/unit/cartodb/prepare-context.test.js +++ b/test/unit/cartodb/prepare-context.test.js @@ -127,7 +127,7 @@ describe('prepare-context', function() { } }; - res = {}; + res = { set: function () {} }; dbConnSetup(prepareRequest(req), prepareResponse(res), function() { // wrong key resets params to no user From 484e0fda2f28562334a4b806b6f27cb506436089 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Mart=C3=ADn?= Date: Mon, 9 Oct 2017 16:29:35 +0200 Subject: [PATCH 25/25] undo changing services params --- lib/cartodb/backends/analysis-status.js | 4 +++- lib/cartodb/backends/dataview.js | 4 +++- lib/cartodb/controllers/layergroup.js | 3 +-- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/lib/cartodb/backends/analysis-status.js b/lib/cartodb/backends/analysis-status.js index 71ee6fc8..97f851d2 100644 --- a/lib/cartodb/backends/analysis-status.js +++ b/lib/cartodb/backends/analysis-status.js @@ -6,7 +6,9 @@ function AnalysisStatusBackend() { module.exports = AnalysisStatusBackend; -AnalysisStatusBackend.prototype.getNodeStatus = function (nodeId, params, callback) { +AnalysisStatusBackend.prototype.getNodeStatus = function (params, callback) { + var nodeId = params.nodeId; + var statusQuery = [ 'SELECT node_id, status, updated_at, last_error_message as error_message', 'FROM cdb_analysis_catalog where node_id = \'' + nodeId + '\'' diff --git a/lib/cartodb/backends/dataview.js b/lib/cartodb/backends/dataview.js index 8a9c0a85..b6037ae6 100644 --- a/lib/cartodb/backends/dataview.js +++ b/lib/cartodb/backends/dataview.js @@ -22,7 +22,9 @@ function DataviewBackend(analysisBackend) { module.exports = DataviewBackend; -DataviewBackend.prototype.getDataview = function (mapConfigProvider, user, dataviewName, params, callback) { +DataviewBackend.prototype.getDataview = function (mapConfigProvider, user, params, callback) { + + var dataviewName = params.dataviewName; step( function getMapConfig() { mapConfigProvider.getMapConfig(this); diff --git a/lib/cartodb/controllers/layergroup.js b/lib/cartodb/controllers/layergroup.js index 3a63b0c9..f83ffb82 100644 --- a/lib/cartodb/controllers/layergroup.js +++ b/lib/cartodb/controllers/layergroup.js @@ -169,7 +169,7 @@ LayergroupController.prototype.analysisNodeStatus = function(req, res, next) { step( function retrieveNodeStatus() { - self.analysisStatusBackend.getNodeStatus(req.params.nodeId, res.locals, this); + self.analysisStatusBackend.getNodeStatus(res.locals, this); }, function finish(err, nodeStatus, stats) { req.profiler.add(stats || {}); @@ -198,7 +198,6 @@ LayergroupController.prototype.dataview = function(req, res, next) { self.dataviewBackend.getDataview( mapConfigProvider, res.locals.user, - req.params.dataviewName, res.locals, this );