From e7d134d70cd191246f4a0f17f33e442140a7d6a1 Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Thu, 23 Apr 2015 11:29:55 +0200 Subject: [PATCH 1/4] No more {username}@{template_name} template id It's still backwards compatible --- lib/cartodb/controllers/template_maps.js | 87 +++++++------------ .../cache/surrogate_keys_invalidation.js | 2 +- test/acceptance/templates.js | 14 ++- 3 files changed, 38 insertions(+), 65 deletions(-) diff --git a/lib/cartodb/controllers/template_maps.js b/lib/cartodb/controllers/template_maps.js index be2022a8..f24336ec 100644 --- a/lib/cartodb/controllers/template_maps.js +++ b/lib/cartodb/controllers/template_maps.js @@ -1,4 +1,5 @@ var step = require('step'); +var assert = require('assert'); var _ = require('underscore'); var CdbRequest = require('../models/cdb_request'); @@ -42,7 +43,7 @@ TemplateMapsController.prototype.create = function(req, res) { self.serverOptions.authorizedByAPIKey(req, this); }, function addTemplate(err, authenticated) { - if ( err ) throw err; + assert.ifError(err); if (authenticated !== 1) { err = new Error("Only authenticated user can create templated maps"); err.http_status = 403; @@ -54,12 +55,11 @@ TemplateMapsController.prototype.create = function(req, res) { self.templateMaps.addTemplate(cdbuser, cfg, this); }, function prepareResponse(err, tpl_id){ - if ( err ) throw err; - // NOTE: might omit "cdbuser" if == dbowner ... - return { template_id: cdbuser + '@' + tpl_id }; + assert.ifError(err); + return { template_id: tpl_id }; }, function finish(err, response){ - if (err){ + if (err) { response = { error: ''+err }; var statusCode = 400; if ( ! _.isUndefined(err.http_status) ) { @@ -87,7 +87,8 @@ TemplateMapsController.prototype.update = function(req, res) { self.serverOptions.authorizedByAPIKey(req, this); }, function updateTemplate(err, authenticated) { - if ( err ) throw err; + assert.ifError(err); + if (authenticated !== 1) { err = new Error("Only authenticated user can list templated maps"); err.http_status = 403; @@ -96,23 +97,16 @@ TemplateMapsController.prototype.update = function(req, res) { if ( ! req.headers['content-type'] || req.headers['content-type'].split(';')[0] != 'application/json' ) throw new Error('template PUT data must be of type application/json'); template = req.body; - tpl_id = req.params.template_id.split('@'); - if ( tpl_id.length > 1 ) { - if ( tpl_id[0] != cdbuser ) { - err = new Error("Invalid template id '" + req.params.template_id + "' for user '" + cdbuser + "'"); - err.http_status = 404; - throw err; - } - tpl_id = tpl_id[1]; - } + tpl_id = templateName(req.params.template_id); self.templateMaps.updTemplate(cdbuser, tpl_id, template, this); }, function prepareResponse(err){ - if ( err ) throw err; - return { template_id: cdbuser + '@' + tpl_id }; + assert.ifError(err); + + return { template_id: tpl_id }; }, function finish(err, response){ - if (err){ + if (err) { var statusCode = 400; response = { error: ''+err }; if ( ! _.isUndefined(err.http_status) ) { @@ -149,19 +143,10 @@ TemplateMapsController.prototype.retrieve = function(req, res) { err.http_status = 403; throw err; } - tpl_id = req.params.template_id.split('@'); - if ( tpl_id.length > 1 ) { - if ( tpl_id[0] != cdbuser ) { - var templateNotFoundErr = new Error("Cannot get template id '" + req.params.template_id + - "' for user '" + cdbuser + "'"); - templateNotFoundErr.http_status = 404; - throw templateNotFoundErr; - } - tpl_id = tpl_id[1]; - } + tpl_id = templateName(req.params.template_id); self.templateMaps.getTemplate(cdbuser, tpl_id, this); }, - function prepareResponse(err, tpl_val){ + function prepareResponse(err, tpl_val) { if ( err ) throw err; if ( ! tpl_val ) { err = new Error("Cannot find template '" + tpl_id + "' of user '" + cdbuser + "'"); @@ -210,16 +195,7 @@ TemplateMapsController.prototype.destroy = function(req, res) { err.http_status = 403; throw err; } - tpl_id = req.params.template_id.split('@'); - if ( tpl_id.length > 1 ) { - if ( tpl_id[0] != cdbuser ) { - var templateNotFoundErr = new Error("Cannot find template id '" + req.params.template_id + - "' for user '" + cdbuser + "'"); - templateNotFoundErr.http_status = 404; - throw templateNotFoundErr; - } - tpl_id = tpl_id[1]; - } + tpl_id = templateName(req.params.template_id); self.templateMaps.delTemplate(cdbuser, tpl_id, this); }, function prepareResponse(err/*, tpl_val*/){ @@ -265,15 +241,13 @@ TemplateMapsController.prototype.list = function(req, res) { self.templateMaps.listTemplates(cdbuser, this); }, function prepareResponse(err, tpl_ids){ - if ( err ) throw err; - // NOTE: might omit "cbduser" if == dbowner ... - var ids = _.map(tpl_ids, function(id) { return cdbuser + '@' + id; }); - return { template_ids: ids }; + assert.ifError(err); + return { template_ids: tpl_ids }; }, function finish(err, response){ var statusCode = 200; - if (err){ - response = { error: ''+err }; + if (err) { + response = { error: '' + err }; if ( ! _.isUndefined(err.http_status) ) { statusCode = err.http_status; } @@ -350,17 +324,7 @@ TemplateMapsController.prototype.instantiateTemplate = function(req, res, templa var fakereq; // used for call to createLayergroup var cdbuser = cdbRequest.userByReq(req); // Format of template_id: []@ - var tpl_id = req.params.template_id.split('@'); - if ( tpl_id.length > 1 ) { - if ( tpl_id[0] && tpl_id[0] != cdbuser ) { - var err = new Error('Cannot instanciate map of user "' + tpl_id[0] + '" on database of user "' + cdbuser + - '"'); - err.http_status = 403; - callback(err); - return; - } - tpl_id = tpl_id[1]; - } + var tpl_id = templateName(req.params.template_id); var auth_token = req.query.auth_token; step( function getTemplate(){ @@ -468,3 +432,14 @@ TemplateMapsController.prototype.setDBParams = function(cdbuser, params, callbac } ); }; + +function templateName(templateId) { + var templateIdTokens = templateId.split('@'); + var name = templateIdTokens[0]; + + if (templateIdTokens.length > 1) { + name = templateIdTokens[1]; + } + + return name; +} diff --git a/test/acceptance/cache/surrogate_keys_invalidation.js b/test/acceptance/cache/surrogate_keys_invalidation.js index 8b626701..b4f4a165 100644 --- a/test/acceptance/cache/surrogate_keys_invalidation.js +++ b/test/acceptance/cache/surrogate_keys_invalidation.js @@ -23,7 +23,7 @@ describe('templates surrogate keys', function() { var templateOwner = 'localhost', templateName = 'acceptance', - expectedTemplateId = templateOwner + '@' + templateName, + expectedTemplateId = templateName, template = { version: '0.0.1', name: templateName, diff --git a/test/acceptance/templates.js b/test/acceptance/templates.js index aaf3e6b0..829abd12 100644 --- a/test/acceptance/templates.js +++ b/test/acceptance/templates.js @@ -69,7 +69,7 @@ describe('template_api', function() { it("can add template, returning id", function(done) { var errors = []; - var expected_tpl_id = "localhost@acceptance1"; + var expected_tpl_id = "acceptance1"; var post_request_1 = { url: '/api/v1/map/named', method: 'POST', @@ -801,7 +801,7 @@ describe('template_api', function() { ); }); - it("can instanciate a template by id", function(done) { + it("can instanciate a template by id wadus", function(done) { // This map fetches data from a private table var template_acceptance2 = { @@ -880,12 +880,10 @@ describe('template_api', function() { function instanciateAuth(err, res) { if ( err ) throw err; - assert.equal(res.statusCode, 403, res.statusCode + ': ' + res.body); + assert.equal(res.statusCode, 404, res.statusCode + ': ' + res.body); var parsed = JSON.parse(res.body); - assert.ok(parsed.hasOwnProperty('error'), - "Missing 'error' from response body: " + res.body); - assert.ok(parsed.error.match(/cannot instanciate/i), - 'Unexpected error for forbidden instance : ' + parsed.error); + assert.ok(parsed.hasOwnProperty('error'), "Missing 'error' from response body: " + res.body); + assert.ok(parsed.error.match(/not found/i), 'Unexpected error for forbidden instance : ' + parsed.error); var post_request = { url: '/api/v1/map/named/' + tpl_id + '?auth_token=valid2', method: 'POST', @@ -1907,7 +1905,7 @@ describe('template_api', function() { var template_params = {}; var errors = []; - var expectedTemplateId = username + '@' + httpTemplateName; + var expectedTemplateId = httpTemplateName; var layergroupid; step( function createTemplate() From fed29b3b503b829f229d9197e3dd8f9ffaf5b62f Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Thu, 23 Apr 2015 11:47:01 +0200 Subject: [PATCH 2/4] Extract finish function --- lib/cartodb/controllers/template_maps.js | 81 ++++++------------------ 1 file changed, 21 insertions(+), 60 deletions(-) diff --git a/lib/cartodb/controllers/template_maps.js b/lib/cartodb/controllers/template_maps.js index f24336ec..8efb722d 100644 --- a/lib/cartodb/controllers/template_maps.js +++ b/lib/cartodb/controllers/template_maps.js @@ -58,18 +58,7 @@ TemplateMapsController.prototype.create = function(req, res) { assert.ifError(err); return { template_id: tpl_id }; }, - function finish(err, response){ - if (err) { - response = { error: ''+err }; - var statusCode = 400; - if ( ! _.isUndefined(err.http_status) ) { - statusCode = err.http_status; - } - self.app.sendError(res, response, statusCode, 'POST TEMPLATE', err); - } else { - self.app.sendResponse(res, [response, 200]); - } - } + finishFn(self.app, res, 'POST TEMPLATE') ); }; @@ -105,18 +94,7 @@ TemplateMapsController.prototype.update = function(req, res) { return { template_id: tpl_id }; }, - function finish(err, response){ - if (err) { - var statusCode = 400; - response = { error: ''+err }; - if ( ! _.isUndefined(err.http_status) ) { - statusCode = err.http_status; - } - self.app.sendError(res, response, statusCode, 'PUT TEMPLATE', err); - } else { - self.app.sendResponse(res, [response, 200]); - } - } + finishFn(self.app, res, 'PUT TEMPLATE') ); }; @@ -158,18 +136,7 @@ TemplateMapsController.prototype.retrieve = function(req, res) { delete tpl_val.auth_id; return { template: tpl_val }; }, - function finish(err, response){ - if (err){ - var statusCode = 400; - response = { error: ''+err }; - if ( ! _.isUndefined(err.http_status) ) { - statusCode = err.http_status; - } - self.app.sendError(res, response, statusCode, 'GET TEMPLATE', err); - } else { - self.app.sendResponse(res, [response, 200]); - } - } + finishFn(self.app, res, 'GET TEMPLATE') ); }; @@ -202,18 +169,7 @@ TemplateMapsController.prototype.destroy = function(req, res) { if ( err ) throw err; return { status: 'ok' }; }, - function finish(err, response){ - if (err){ - var statusCode = 400; - response = { error: ''+err }; - if ( ! _.isUndefined(err.http_status) ) { - statusCode = err.http_status; - } - self.app.sendError(res, response, statusCode, 'DELETE TEMPLATE', err); - } else { - self.app.sendResponse(res, ['', 204]); - } - } + finishFn(self.app, res, 'DELETE TEMPLATE', ['', 204]) ); }; @@ -244,18 +200,7 @@ TemplateMapsController.prototype.list = function(req, res) { assert.ifError(err); return { template_ids: tpl_ids }; }, - function finish(err, response){ - var statusCode = 200; - if (err) { - response = { error: '' + err }; - if ( ! _.isUndefined(err.http_status) ) { - statusCode = err.http_status; - } - self.app.sendError(res, response, statusCode, 'GET TEMPLATE LIST', err); - } else { - self.app.sendResponse(res, [response, statusCode]); - } - } + finishFn(self.app, res, 'GET TEMPLATE LIST') ); }; @@ -433,6 +378,22 @@ TemplateMapsController.prototype.setDBParams = function(cdbuser, params, callbac ); }; +function finishFn(app, res, description, okResponse) { + return function finish(err, response){ + var statusCode = 200; + if (err) { + statusCode = 400; + response = { error: '' + err }; + if ( ! _.isUndefined(err.http_status) ) { + statusCode = err.http_status; + } + app.sendError(res, response, statusCode, description, err); + } else { + app.sendResponse(res, okResponse || [response, statusCode]); + } + }; +} + function templateName(templateId) { var templateIdTokens = templateId.split('@'); var name = templateIdTokens[0]; From c49f3aaba5a6ef327a5262766923390484ce971d Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Thu, 23 Apr 2015 12:01:53 +0200 Subject: [PATCH 3/4] DRY ifUnauthenticated method --- lib/cartodb/controllers/template_maps.js | 45 +++++++++--------------- 1 file changed, 16 insertions(+), 29 deletions(-) diff --git a/lib/cartodb/controllers/template_maps.js b/lib/cartodb/controllers/template_maps.js index 8efb722d..21f19931 100644 --- a/lib/cartodb/controllers/template_maps.js +++ b/lib/cartodb/controllers/template_maps.js @@ -44,11 +44,7 @@ TemplateMapsController.prototype.create = function(req, res) { }, function addTemplate(err, authenticated) { assert.ifError(err); - if (authenticated !== 1) { - err = new Error("Only authenticated user can create templated maps"); - err.http_status = 403; - throw err; - } + ifUnauthenticated(authenticated, 'Only authenticated users can get template maps'); if ( ! req.headers['content-type'] || req.headers['content-type'].split(';')[0] != 'application/json' ) throw new Error('template POST data must be of type application/json'); var cfg = req.body; @@ -77,12 +73,7 @@ TemplateMapsController.prototype.update = function(req, res) { }, function updateTemplate(err, authenticated) { assert.ifError(err); - - if (authenticated !== 1) { - err = new Error("Only authenticated user can list templated maps"); - err.http_status = 403; - throw err; - } + ifUnauthenticated(authenticated, 'Only authenticated user can update templated maps'); if ( ! req.headers['content-type'] || req.headers['content-type'].split(';')[0] != 'application/json' ) throw new Error('template PUT data must be of type application/json'); template = req.body; @@ -115,12 +106,8 @@ TemplateMapsController.prototype.retrieve = function(req, res) { self.serverOptions.authorizedByAPIKey(req, this); }, function updateTemplate(err, authenticated) { - if ( err ) throw err; - if (authenticated !== 1) { - err = new Error("Only authenticated users can get template maps"); - err.http_status = 403; - throw err; - } + assert.ifError(err); + ifUnauthenticated(authenticated, 'Only authenticated users can get template maps'); tpl_id = templateName(req.params.template_id); self.templateMaps.getTemplate(cdbuser, tpl_id, this); }, @@ -156,12 +143,8 @@ TemplateMapsController.prototype.destroy = function(req, res) { self.serverOptions.authorizedByAPIKey(req, this); }, function updateTemplate(err, authenticated) { - if ( err ) throw err; - if (authenticated !== 1) { - err = new Error("Only authenticated users can delete template maps"); - err.http_status = 403; - throw err; - } + assert.ifError(err); + ifUnauthenticated(authenticated, 'Only authenticated users can delete template maps'); tpl_id = templateName(req.params.template_id); self.templateMaps.delTemplate(cdbuser, tpl_id, this); }, @@ -188,12 +171,8 @@ TemplateMapsController.prototype.list = function(req, res) { self.serverOptions.authorizedByAPIKey(req, this); }, function listTemplates(err, authenticated) { - if ( err ) throw err; - if (authenticated !== 1) { - err = new Error("Only authenticated user can list templated maps"); - err.http_status = 403; - throw err; - } + assert.ifError(err); + ifUnauthenticated(authenticated, 'Only authenticated user can list templated maps'); self.templateMaps.listTemplates(cdbuser, this); }, function prepareResponse(err, tpl_ids){ @@ -394,6 +373,14 @@ function finishFn(app, res, description, okResponse) { }; } +function ifUnauthenticated(authenticated, description) { + if (authenticated !== 1) { + var err = new Error(description); + err.http_status = 403; + throw err; + } +} + function templateName(templateId) { var templateIdTokens = templateId.split('@'); var name = templateIdTokens[0]; From 446c432484e5383caec637066ffc5761258b4f15 Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Thu, 23 Apr 2015 12:05:52 +0200 Subject: [PATCH 4/4] dry content type validation --- lib/cartodb/controllers/template_maps.js | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/lib/cartodb/controllers/template_maps.js b/lib/cartodb/controllers/template_maps.js index 21f19931..29026c41 100644 --- a/lib/cartodb/controllers/template_maps.js +++ b/lib/cartodb/controllers/template_maps.js @@ -45,8 +45,7 @@ TemplateMapsController.prototype.create = function(req, res) { function addTemplate(err, authenticated) { assert.ifError(err); ifUnauthenticated(authenticated, 'Only authenticated users can get template maps'); - if ( ! req.headers['content-type'] || req.headers['content-type'].split(';')[0] != 'application/json' ) - throw new Error('template POST data must be of type application/json'); + ifInvalidContentType(req, 'template POST data must be of type application/json'); var cfg = req.body; self.templateMaps.addTemplate(cdbuser, cfg, this); }, @@ -74,8 +73,8 @@ TemplateMapsController.prototype.update = function(req, res) { function updateTemplate(err, authenticated) { assert.ifError(err); ifUnauthenticated(authenticated, 'Only authenticated user can update templated maps'); - if ( ! req.headers['content-type'] || req.headers['content-type'].split(';')[0] != 'application/json' ) - throw new Error('template PUT data must be of type application/json'); + ifInvalidContentType(req, 'template PUT data must be of type application/json'); + template = req.body; tpl_id = templateName(req.params.template_id); self.templateMaps.updTemplate(cdbuser, tpl_id, template, this); @@ -108,6 +107,7 @@ TemplateMapsController.prototype.retrieve = function(req, res) { function updateTemplate(err, authenticated) { assert.ifError(err); ifUnauthenticated(authenticated, 'Only authenticated users can get template maps'); + tpl_id = templateName(req.params.template_id); self.templateMaps.getTemplate(cdbuser, tpl_id, this); }, @@ -145,6 +145,7 @@ TemplateMapsController.prototype.destroy = function(req, res) { function updateTemplate(err, authenticated) { assert.ifError(err); ifUnauthenticated(authenticated, 'Only authenticated users can delete template maps'); + tpl_id = templateName(req.params.template_id); self.templateMaps.delTemplate(cdbuser, tpl_id, this); }, @@ -173,6 +174,7 @@ TemplateMapsController.prototype.list = function(req, res) { function listTemplates(err, authenticated) { assert.ifError(err); ifUnauthenticated(authenticated, 'Only authenticated user can list templated maps'); + self.templateMaps.listTemplates(cdbuser, this); }, function prepareResponse(err, tpl_ids){ @@ -191,9 +193,8 @@ TemplateMapsController.prototype.instantiate = function(req, res) { } step( function() { - if ( ! req.headers['content-type'] || req.headers['content-type'].split(';')[0] != 'application/json') { - throw new Error('template POST data must be of type application/json, it is instead '); - } + ifInvalidContentType(req, 'template POST data must be of type application/json'); + self.instantiateTemplate(req, res, req.body, this); }, function(err, response) { self.finish_instantiation(err, response, res); @@ -381,6 +382,12 @@ function ifUnauthenticated(authenticated, description) { } } +function ifInvalidContentType(req, description) { + if ( ! req.headers['content-type'] || req.headers['content-type'].split(';')[0] != 'application/json' ) { + throw new Error(description); + } +} + function templateName(templateId) { var templateIdTokens = templateId.split('@'); var name = templateIdTokens[0];