From 572f8c59b712367e79d716a187bad6ae50814a2f Mon Sep 17 00:00:00 2001 From: Raul Ochoa Date: Fri, 4 Jul 2014 16:47:59 +0200 Subject: [PATCH] Changes authentication to start using public user if it is defined in redis. --- app/controllers/app.js | 119 ++++++++++++++++------------------ package.json | 2 +- test/acceptance/app.test.js | 27 ++++---- test/acceptance/export/kml.js | 2 +- test/prepare_db.sh | 5 ++ 5 files changed, 78 insertions(+), 77 deletions(-) diff --git a/app/controllers/app.js b/app/controllers/app.js index 1a175462..6ca4a464 100755 --- a/app/controllers/app.js +++ b/app/controllers/app.js @@ -195,7 +195,6 @@ function handleQuery(req, res) { var params = _.extend({}, req.query, body); // clone so don't modify req.params or req.body so oauth is not broken var sql = params.q; var api_key = params.api_key; - var database = params.database; // TODO: Deprecate var limit = parseInt(params.rows_per_page); var offset = parseInt(params.page); var orderBy = params.order_by; @@ -203,7 +202,6 @@ function handleQuery(req, res) { var requestedFormat = params.format; var format = _.isArray(requestedFormat) ? _.last(requestedFormat) : requestedFormat; var requestedFilename = params.filename; - var cache_policy = params.cache_policy; var filename = requestedFilename; var requestedSkipfields = params.skipfields; var skipfields; @@ -240,7 +238,6 @@ function handleQuery(req, res) { format = (format === "" || _.isUndefined(format)) ? 'json' : format.toLowerCase(); filename = (filename === "" || _.isUndefined(filename)) ? 'cartodb-query' : sanitize_filename(filename); sql = (sql === "" || _.isUndefined(sql)) ? null : sql; - database = (database === "" || _.isUndefined(database)) ? null : database; limit = (!_.isNaN(limit)) ? limit : null; offset = (!_.isNaN(offset)) ? offset * limit : null; @@ -258,10 +255,13 @@ function handleQuery(req, res) { } //if ( -1 === supportedFormats.indexOf(format) ) - if ( ! formats.hasOwnProperty(format) ) - throw new Error("Invalid format: " + format); + if ( ! formats.hasOwnProperty(format) ) { + throw new Error("Invalid format: " + format); + } - if (!_.isString(sql)) throw new Error("You must indicate a sql query"); + if (!_.isString(sql)) { + throw new Error("You must indicate a sql query"); + } // initialise MD5 key of sql for cache lookups var sql_md5 = generateMD5(sql); @@ -289,79 +289,70 @@ function handleQuery(req, res) { // 4. Setup headers // 5. Send formatted results back Step( - function getDatabaseName() { - checkAborted('getDatabaseName'); - if (_.isNull(database)) { - Meta.getUserDBName(cdbuser, this); - } else { - // database hardcoded in query string (deprecated??): don't use redis - return database; - } + function getDatabaseConnectionParams() { + checkAborted('getDatabaseConnectionParams'); + Meta.getUserDBConnectionParams(cdbuser, this); }, - function setDBGetUser(err, data) { + function setDBConnectionParams(err, dbParams) { + if (err) { - // If the database could not be found, the user is non-existant - if ( err.message.match('missing') ) { - err.message = "Sorry, we can't find CartoDB user '" + cdbuser - + "'. Please check that you have entered the correct domain."; err.http_status = 404; - } - throw err; + err.message = "Sorry, we can't find CartoDB user '" + cdbuser + + "'. Please check that you have entered the correct domain."; + throw err; } - if ( req.profiler ) req.profiler.done('getDatabaseName'); + dbopts.host = dbParams.dbhost; + dbopts.dbname = dbParams.dbname; + dbopts.user = (!!dbParams.dbuser) ? dbParams.dbuser : global.settings.db_pubuser; - database = (data === "" || _.isNull(data) || _.isUndefined(data)) ? database : data; - dbopts.dbname = database; - - if(api_key) { + return null; + }, + function authenticate(err) { + if (err) { + throw err; + } + if (api_key) { apiKeyAuth.verifyRequest(req, this); } else { oAuth.verifyRequest(req, this, requestProtocol); } }, - function setUserGetDBHost(err, data){ - if (err) throw err; - if ( req.profiler ) req.profiler.done('verifyRequest_' + ( api_key ? 'apikey' : 'oauth' ) ); - user_id = data; - authenticated = ! _.isNull(user_id); - - var dbuser = user_id ? - _.template(global.settings.db_user, {user_id: user_id}) - : - global.settings.db_pubuser; - - dbopts.user = dbuser; - - Meta.getUserDBHost(cdbuser, this); + function setUserGetDBPassword(err, userId) { + if (err) { + throw err; + } + authenticated = userId !== null; + if (authenticated) { + user_id = userId; + dbopts.user = _.template(global.settings.db_user, {user_id: userId}); + Meta.getUserDBPass(cdbuser, this); + } else { + return null + } }, - function setDBHostGetPassword(err, data){ - if (err) throw err; - if ( req.profiler ) req.profiler.done('getUserDBHost'); - - dbopts.host = data || global.settings.db_host; - - // by-pass redis lookup for password if not authenticated - if ( ! authenticated ) return null; - - Meta.getUserDBPass(cdbuser, this); + function setPassword(err, password) { + if (err) { + throw err; + } + if ( authenticated ) { + if ( global.settings.hasOwnProperty('db_user_pass') ) { + dbopts.pass = _.template(global.settings.db_user_pass, { + user_id: user_id, + user_password: password + }); + } else { + delete dbopts.pass; + } + } + return null; }, - function queryExplain(err, data){ + function queryExplain(err){ var self = this; if (err) throw err; if ( req.profiler ) req.profiler.done('getUserDBPass'); checkAborted('queryExplain'); - if ( authenticated ) { - if ( global.settings.hasOwnProperty('db_user_pass') ) { - dbopts.pass = _.template(global.settings.db_user_pass, { - user_id: user_id, - user_password: data - }); - } - else delete dbopts.pass; - } - pg = new PSQL(dbopts); // get all the tables from Cache or SQL tableCacheItem = tableCache.get(sql_md5); @@ -434,19 +425,19 @@ function handleQuery(req, res) { var ttl = 31536000; // 1 year time to live by default var cache_policy = req.query.cache_policy; if ( cache_policy === 'persist' ) { - res.header('Cache-Control', 'public,max-age=' + ttl); + res.header('Cache-Control', 'public,max-age=' + ttl); } else { if ( ! tableCacheItem || tableCacheItem.may_write ) { // Tell clients this response is already expired // TODO: prevent cache_policy from overriding this ? ttl = 0; - } + } res.header('Cache-Control', 'no-cache,max-age='+ttl+',must-revalidate,public'); } // Only set an X-Cache-Channel for responses we want Varnish to cache. if ( tableCacheItem && ! tableCacheItem.may_write ) { - res.header('X-Cache-Channel', generateCacheKey(database, tableCacheItem, authenticated)); + res.header('X-Cache-Channel', generateCacheKey(dbopts.dbname, tableCacheItem, authenticated)); } // Set Last-Modified header diff --git a/package.json b/package.json index 279c4626..3f7d2ff0 100644 --- a/package.json +++ b/package.json @@ -20,7 +20,7 @@ "underscore.string": "~1.1.6", "pg": "git://github.com/CartoDB/node-postgres.git#2.6.2-cdb1", "express": "~2.5.11", - "cartodb-redis": "~0.3.0", + "cartodb-redis": "~0.5.0", "step": "0.0.x", "topojson": "0.0.8", "oauth-client": "0.2.0", diff --git a/test/acceptance/app.test.js b/test/acceptance/app.test.js index a87c5d0c..f1022584 100644 --- a/test/acceptance/app.test.js +++ b/test/acceptance/app.test.js @@ -89,7 +89,7 @@ test('GET /api/whatever/sql', function(done){ }); }); -// Test that OPTIONS does not run queries +// Test that OPTIONS does not run queries test('OPTIONS /api/x/sql', function(done){ assert.response(app, { url: '/api/x/sql?q=syntax%20error', @@ -109,6 +109,7 @@ test('OPTIONS /api/x/sql', function(done){ test('GET /api/v1/sql with SQL parameter on SELECT only. No oAuth included ', function(done){ assert.response(app, { url: '/api/v1/sql?q=SELECT%20*%20FROM%20untitle_table_4&database=cartodb_test_user_1_db', + headers: {host: 'vizzuality.cartodb.com'}, method: 'GET' },{ }, function(res) { assert.equal(res.statusCode, 200, res.body); @@ -122,6 +123,7 @@ test('GET /api/v1/sql with SQL parameter on SELECT only. No oAuth included ', fu test('cache_policy=persist', function(done){ assert.response(app, { url: '/api/v1/sql?q=SELECT%20*%20FROM%20untitle_table_4&database=cartodb_test_user_1_db&cache_policy=persist', + headers: {host: 'vizzuality.cartodb.com'}, method: 'GET' },{ }, function(res) { assert.equal(res.statusCode, 200, res.body); @@ -207,7 +209,7 @@ function(done){ }); // Test for https://github.com/Vizzuality/CartoDB-SQL-API/issues/85 -test("paging doesn't break x-cache-channel", +test("paging doesn't break x-cache-channel", function(done){ assert.response(app, { url: '/api/v1/sql?' + querystring.stringify({ @@ -334,14 +336,15 @@ test('POST /api/v1/sql with SQL parameter on SELECT only. no database param, jus test('GET /api/v1/sql with INSERT. oAuth not used, so public user - should fail', function(done){ assert.response(app, { url: "/api/v1/sql?q=INSERT%20INTO%20untitle_table_4%20(cartodb_id)%20VALUES%20(1e4)&database=cartodb_test_user_1_db", + headers: {host: 'vizzuality.cartodb.com'}, method: 'GET' },{ }, function(res) { assert.equal(res.statusCode, 400, res.statusCode + ': ' + res.body); assert.deepEqual(res.headers['content-type'], 'application/json; charset=utf-8'); assert.deepEqual(res.headers['content-disposition'], 'inline'); - assert.deepEqual(JSON.parse(res.body), - {"error":["permission denied for relation untitle_table_4"]} + assert.deepEqual(JSON.parse(res.body), + {"error":["permission denied for relation untitle_table_4"]} ); done(); }); @@ -350,13 +353,14 @@ test('GET /api/v1/sql with INSERT. oAuth not used, so public user - should fail' test('GET /api/v1/sql with DROP TABLE. oAuth not used, so public user - should fail', function(done){ assert.response(app, { url: "/api/v1/sql?q=DROP%20TABLE%20untitle_table_4&database=cartodb_test_user_1_db", + headers: {host: 'vizzuality.cartodb.com'}, method: 'GET' },{ }, function(res) { assert.equal(res.statusCode, 400, res.statusCode + ': ' + res.body); assert.deepEqual(res.headers['content-type'], 'application/json; charset=utf-8'); assert.deepEqual(res.headers['content-disposition'], 'inline'); - assert.deepEqual(JSON.parse(res.body), + assert.deepEqual(JSON.parse(res.body), {"error":["must be owner of relation untitle_table_4"]} ); done(); @@ -373,7 +377,7 @@ test('GET /api/v1/sql with INSERT. header based db - should fail', function(){ }); }); -// Check results from INSERT +// Check results from INSERT // // See https://github.com/Vizzuality/CartoDB-SQL-API/issues/13 test('INSERT returns affected rows', function(done){ @@ -534,8 +538,8 @@ test('GET /api/v1/sql with SQL parameter on DROP TABLE. should fail', function(d assert.equal(res.statusCode, 400, res.statusCode + ': ' + res.body); assert.deepEqual(res.headers['content-type'], 'application/json; charset=utf-8'); assert.deepEqual(res.headers['content-disposition'], 'inline'); - assert.deepEqual(JSON.parse(res.body), - {"error":["must be owner of relation untitle_table_4"]} + assert.deepEqual(JSON.parse(res.body), + {"error":["must be owner of relation untitle_table_4"]} ); done(); }); @@ -844,7 +848,7 @@ test('GET /api/v1/sql with SQL parameter and no format, ensuring content-disposi test('POST /api/v1/sql with SQL parameter and no format, ensuring content-disposition set to json', function(done){ assert.response(app, { - url: '/api/v1/sql', + url: '/api/v1/sql', data: querystring.stringify({q: "SELECT * FROM untitle_table_4" }), headers: {host: 'vizzuality.cartodb.com', 'Content-Type': 'application/x-www-form-urlencoded' }, method: 'POST' @@ -1097,7 +1101,7 @@ test('field names and types are exposed', function(done){ ", 'POINT(0 0)'::geometry as h" + // See https://github.com/CartoDB/CartoDB-SQL-API/issues/117 ", now()::date as i" + - ", '1'::numeric as j" + + ", '1'::numeric as j" + " LIMIT 0" }), headers: {host: 'vizzuality.cartodb.com'}, @@ -1182,7 +1186,7 @@ test('numeric fields are rendered as numbers in JSON', function(done){ // // NOTE: results of these tests rely on the TZ env variable // being set to 'Europe/Rome'. The env variable cannot -// be set within this test in a reliable way, see +// be set within this test in a reliable way, see // https://github.com/joyent/node/issues/3286 // // FIXME: we'd like to also test UTC outputs of these @@ -1392,6 +1396,7 @@ test('notice and warning info in JSON output', function(done){ test('GET /api/v1/sql with SQL parameter on SELECT only should return CORS headers ', function(done){ assert.response(app, { url: '/api/v1/sql?q=SELECT%20*%20FROM%20untitle_table_4&database=cartodb_test_user_1_db', + headers: {host: 'vizzuality.cartodb.com'}, method: 'GET' },{ }, function(res) { assert.equal(res.statusCode, 200, res.body); diff --git a/test/acceptance/export/kml.js b/test/acceptance/export/kml.js index 2cc7cc22..6abcdf4a 100644 --- a/test/acceptance/export/kml.js +++ b/test/acceptance/export/kml.js @@ -232,7 +232,7 @@ test('KML format, unauthenticated, concurrent requests', function(done){ for (var i=0; i