From 751745cb5da413a8633f8784eb85a7193bbb6451 Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Thu, 17 May 2018 17:10:33 +0200 Subject: [PATCH 01/30] use correspondent cartodb-redis branch --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 59c7eaf3..16797716 100644 --- a/package.json +++ b/package.json @@ -22,7 +22,7 @@ "bunyan": "1.8.1", "cartodb-psql": "0.10.1", "cartodb-query-tables": "0.2.0", - "cartodb-redis": "1.0.0", + "cartodb-redis": "git://github.com/CartoDB/node-cartodb-redis.git#remove-auth-fallback", "debug": "2.2.0", "express": "~4.13.3", "log4js": "cartodb/log4js-node#cdb", From e85994293b3e97ba52bd8b104dbfa40f34ceeca9 Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Thu, 17 May 2018 17:13:00 +0200 Subject: [PATCH 02/30] remove fallback tests and refactor some http errors 403 -> 401 --- test/acceptance/app.auth.test.js | 9 +-- test/acceptance/auth-api.js | 104 +++--------------------------- test/acceptance/batch/job.test.js | 18 +++--- 3 files changed, 21 insertions(+), 110 deletions(-) diff --git a/test/acceptance/app.auth.test.js b/test/acceptance/app.auth.test.js index dacd8951..38af61aa 100644 --- a/test/acceptance/app.auth.test.js +++ b/test/acceptance/app.auth.test.js @@ -18,13 +18,8 @@ describe('app.auth', function() { }, { desc: 'invalid api key should NOT allow insert in protected tables', - url: "/api/v1/sql?api_key=RAMBO&q=INSERT%20INTO%20private_table%20(name)%20VALUES%20('RAMBO')", - statusCode: 403 - }, - { - desc: 'invalid api key (old redis location) should NOT allow insert in protected tables', - url: "/api/v1/sql?api_key=1235&q=INSERT%20INTO%20private_table%20(name)%20VALUES%20('RAMBO')", - statusCode: 403 + url: "/api/v1/sql?api_key=THIS_API_KEY_NOT_EXIST&q=INSERT%20INTO%20private_table%20(name)%20VALUES%20('R')", + statusCode: 401 }, { desc: 'no api key should NOT allow insert in protected tables', diff --git a/test/acceptance/auth-api.js b/test/acceptance/auth-api.js index 06d34ad8..b4eed5ba 100644 --- a/test/acceptance/auth-api.js +++ b/test/acceptance/auth-api.js @@ -18,33 +18,18 @@ describe('Auth API', function () { }); }); - // TODO: this is obviously a really dangerous sceneario, but in order to not break - // some uses cases (i.e: new carto.js examples) and keep backwards compatiblity we will keep it during some time. - // It should be fixed as soon as possible - it('should get result from query using a wrong API key', function (done) { - this.testClient = new TestClient({ apiKey: 'wrong' }); + it('should fail when using a wrong API key', function (done) { + this.testClient = new TestClient({ apiKey: 'THIS_API_KEY_DOES_NOT_EXIST' }); - this.testClient.getResult(publicSQL, (err, result) => { - assert.ifError(err); - assert.equal(result.length, 6); - done(); - }); - }); - - // TODO: this is obviously a really dangerous sceneario, but in order to not break - // some uses cases (i.e: new carto.js examples) and keep backwards compatiblity we will keep it during some time. - // It should be fixed as soon as possible - it('should fail while fetching data (private dataset) and using a wrong API key', function (done) { - this.testClient = new TestClient({ apiKey: 'wrong' }); const expectedResponse = { response: { - status: 403 + status: 401 } }; - this.testClient.getResult(privateSQL, expectedResponse, (err, result) => { + this.testClient.getResult(publicSQL, expectedResponse, (err, result) => { assert.ifError(err); - assert.equal(result.error, 'permission denied for relation private_table'); + assert.equal(result.error, 'Unauthorized'); done(); }); }); @@ -106,60 +91,6 @@ describe('Auth API', function () { }); }); - describe('Fallback', function () { - it('should get result from query using master apikey (fallback) and a granted dataset', function (done) { - this.testClient = new TestClient({ apiKey: '4321', host: 'cartofante.cartodb.com' }); - this.testClient.getResult(scopedSQL, (err, result) => { - assert.ifError(err); - assert.equal(result.length, 4); - done(); - }); - }); - - it('should fail while getting result from query using metadata and scoped dataset', function (done) { - this.testClient = new TestClient({ host: 'cartofante.cartodb.com' }); - - const expectedResponse = { - response: { - status: 403 - }, - anonymous: true - }; - - this.testClient.getResult(privateSQL, expectedResponse, (err, result) => { - assert.ifError(err); - assert.equal(result.error, 'permission denied for relation private_table'); - done(); - }); - }); - - it('should insert and delete values on scoped datase using the master apikey', function (done) { - this.testClient = new TestClient({ apiKey: 4321, host: 'cartofante.cartodb.com' }); - - const insertSql = "INSERT INTO scoped_table_1(name) VALUES('wadus1')"; - - this.testClient.getResult(insertSql, (err, rows, body) => { - assert.ifError(err); - - assert.ok(body.hasOwnProperty('time')); - assert.equal(body.total_rows, 1); - assert.equal(rows.length, 0); - - const deleteSql = "DELETE FROM scoped_table_1 WHERE name = 'wadus1'"; - - this.testClient.getResult(deleteSql, (err, rows, body) => { - assert.ifError(err); - - assert.ok(body.hasOwnProperty('time')); - assert.equal(body.total_rows, 1); - assert.equal(rows.length, 0); - - done(); - }); - }); - }); - }); - describe('Batch API', function () { it('should create a job with regular api key and get it done', function (done) { this.testClient = new BatchTestClient({ apiKey: 'regular1' }); @@ -267,34 +198,19 @@ describe('Auth API', function () { }); }); - // TODO: this is obviously a really dangerous sceneario, but in order to not break - // some uses cases (i.e: new carto.js examples) and to keep backwards compatiblity - // we will keep it during some time. It should be fixed as soon as possible - it('should get result from query using a wrong API key and quering to public dataset', function (done) { - this.testClient = new TestClient({ authorization: 'vizzuality:wrong' }); + it('should fail when querying using a wrong API key', function (done) { + this.testClient = new TestClient({ authorization: 'vizzuality:THIS_API_KEY_DOES_NOT_EXIST' }); - this.testClient.getResult(publicSQL, { anonymous: true }, (err, result) => { - assert.ifError(err); - assert.equal(result.length, 6); - done(); - }); - }); - - // TODO: this is obviously a really dangerous sceneario, but in order to not break - // some uses cases (i.e: new carto.js examples) and to keep backwards compatiblity - // we will keep it during some time. It should be fixed as soon as possible - it('should fail while fetching data (private dataset) and using a wrong API key', function (done) { - this.testClient = new TestClient({ authorization: 'vizzuality:wrong' }); const expectedResponse = { response: { - status: 403 + status: 401 }, anonymous: true }; - this.testClient.getResult(privateSQL, expectedResponse, (err, result) => { + this.testClient.getResult(publicSQL, expectedResponse, (err, result) => { assert.ifError(err); - assert.equal(result.error, 'permission denied for relation private_table'); + assert.equal(result.error, 'Unauthorized'); done(); }); }); diff --git a/test/acceptance/batch/job.test.js b/test/acceptance/batch/job.test.js index 7c468e50..a7e8512c 100644 --- a/test/acceptance/batch/job.test.js +++ b/test/acceptance/batch/job.test.js @@ -79,7 +79,7 @@ describe('job module', function() { }); }); - it('POST /api/v2/sql/job with wrong api key should respond with 403 permission denied', function (done){ + it('POST /api/v2/sql/job with wrong api key should respond with 401 permission denied', function (done){ assert.response(server, { url: '/api/v2/sql/job?api_key=wrong', headers: { 'host': 'vizzuality.cartodb.com', 'Content-Type': 'application/x-www-form-urlencoded' }, @@ -88,10 +88,10 @@ describe('job module', function() { query: "SELECT * FROM untitle_table_4" }) }, { - status: 403 + status: 401 }, function(err, res) { var error = JSON.parse(res.body); - assert.deepEqual(error, { error: [ 'permission denied' ] }); + assert.deepEqual(error, { error: [ 'Unauthorized' ] }); done(); }); }); @@ -134,16 +134,16 @@ describe('job module', function() { }); }); - it('GET /api/v2/sql/job/:job_id with wrong api key should respond with 403 permission denied', function (done){ + it('GET /api/v2/sql/job/:job_id with wrong api key should respond with 401 permission denied', function (done){ assert.response(server, { url: '/api/v2/sql/job/' + job.job_id + '?api_key=wrong', headers: { 'host': 'vizzuality.cartodb.com', 'Content-Type': 'application/x-www-form-urlencoded' }, method: 'GET' }, { - status: 403 + status: 401 }, function(err, res) { var error = JSON.parse(res.body); - assert.deepEqual(error, { error: [ 'permission denied' ] }); + assert.deepEqual(error, { error: ['Unauthorized'] }); done(); }); }); @@ -182,16 +182,16 @@ describe('job module', function() { }); }); - it('DELETE /api/v2/sql/job/:job_id with wrong api key should respond with 403 permission denied', function (done){ + it('DELETE /api/v2/sql/job/:job_id with wrong api key should respond with 401 permission denied', function (done){ assert.response(server, { url: '/api/v2/sql/job/' + job.job_id + '?api_key=wrong', headers: { 'host': 'vizzuality.cartodb.com', 'Content-Type': 'application/x-www-form-urlencoded' }, method: 'DELETE' }, { - status: 403 + status: 401 }, function(err, res) { var error = JSON.parse(res.body); - assert.deepEqual(error, { error: [ 'permission denied' ] }); + assert.deepEqual(error, { error: ['Unauthorized'] }); done(); }); }); From 1e8c6e198c5c29ffb36ad7a535edac829777e58a Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Thu, 17 May 2018 17:13:59 +0200 Subject: [PATCH 03/30] remove api key fallback --- app/auth/apikey.js | 34 +++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/app/auth/apikey.js b/app/auth/apikey.js index 417e8e3a..a2e8fa7b 100644 --- a/app/auth/apikey.js +++ b/app/auth/apikey.js @@ -14,6 +14,10 @@ function errorUserNotFoundMessageTemplate (user) { return `Sorry, we can't find CARTO user '${user}'. Please check that you have entered the correct domain.`; } +function usernameMatches(basicAuthUsername, requestUsername) { + return !(basicAuthUsername && (basicAuthUsername !== requestUsername)); +} + ApikeyAuth.prototype.verifyCredentials = function (callback) { this.metadataBackend.getApikey(this.username, this.apikey, (err, apikey) => { if (err) { @@ -22,8 +26,17 @@ ApikeyAuth.prototype.verifyCredentials = function (callback) { return callback(err); } - + if (isApiKeyFound(apikey)) { + if (!usernameMatches(apikey.user, this.username)) { + const error = new Error('Forbidden'); + error.type = 'auth'; + error.subtype = 'api-key-username-mismatch'; + error.http_status = 403; + + return callback(error); + } + if (!apikey.grantsSql) { const forbiddenError = new Error('forbidden'); forbiddenError.http_status = 403; @@ -32,19 +45,14 @@ ApikeyAuth.prototype.verifyCredentials = function (callback) { } return callback(null, verifyRequest(this.apikey, this.apikey)); - } + } else { + const error = new Error('Unauthorized'); + error.type = 'auth'; + error.subtype = 'api-key-not-found'; + error.http_status = 401; - // Auth API Fallback - this.metadataBackend.getAllUserDBParams(this.username, (err, dbParams) => { - if (err) { - err.http_status = 404; - err.message = errorUserNotFoundMessageTemplate(this.username); - - return callback(err); - } - - callback(null, verifyRequest(this.apikey, dbParams.apikey)); - }); + return callback(error); + } }); }; From fc7e24670479b22769ce3c5e5a09697eb0971957 Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Fri, 18 May 2018 11:35:54 +0200 Subject: [PATCH 04/30] please jshint --- app/auth/apikey.js | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/app/auth/apikey.js b/app/auth/apikey.js index a2e8fa7b..bd3181b9 100644 --- a/app/auth/apikey.js +++ b/app/auth/apikey.js @@ -26,15 +26,15 @@ ApikeyAuth.prototype.verifyCredentials = function (callback) { return callback(err); } - + if (isApiKeyFound(apikey)) { if (!usernameMatches(apikey.user, this.username)) { - const error = new Error('Forbidden'); - error.type = 'auth'; - error.subtype = 'api-key-username-mismatch'; - error.http_status = 403; + const usernameError = new Error('Forbidden'); + usernameError.type = 'auth'; + usernameError.subtype = 'api-key-username-mismatch'; + usernameError.http_status = 403; - return callback(error); + return callback(usernameError); } if (!apikey.grantsSql) { @@ -46,12 +46,12 @@ ApikeyAuth.prototype.verifyCredentials = function (callback) { return callback(null, verifyRequest(this.apikey, this.apikey)); } else { - const error = new Error('Unauthorized'); - error.type = 'auth'; - error.subtype = 'api-key-not-found'; - error.http_status = 401; + const apiKeyNotFoundError = new Error('Unauthorized'); + apiKeyNotFoundError.type = 'auth'; + apiKeyNotFoundError.subtype = 'api-key-not-found'; + apiKeyNotFoundError.http_status = 401; - return callback(error); + return callback(apiKeyNotFoundError); } }); }; From 6bc90eb9b397eff4952477b77c6c4e9fe3e07319 Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Thu, 24 May 2018 11:04:14 +0200 Subject: [PATCH 05/30] fix typo: Backed -> Backend --- app/auth/auth_api.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/app/auth/auth_api.js b/app/auth/auth_api.js index b720a529..821ff933 100644 --- a/app/auth/auth_api.js +++ b/app/auth/auth_api.js @@ -3,33 +3,33 @@ var ApiKeyAuth = require('./apikey'), function AuthApi(req, requestParams) { this.req = req; - this.authBacked = getAuthBackend(req, requestParams); + this.authBackend = getAuthBackend(req, requestParams); this._hasCredentials = null; } AuthApi.prototype.getType = function () { - if (this.authBacked instanceof ApiKeyAuth) { + if (this.authBackend instanceof ApiKeyAuth) { return 'apiKey'; - } else if (this.authBacked instanceof OAuthAuth) { + } else if (this.authBackend instanceof OAuthAuth) { return 'oAuth'; } }; AuthApi.prototype.hasCredentials = function() { if (this._hasCredentials === null) { - this._hasCredentials = this.authBacked.hasCredentials(); + this._hasCredentials = this.authBackend.hasCredentials(); } return this._hasCredentials; }; AuthApi.prototype.getCredentials = function() { - return this.authBacked.getCredentials(); + return this.authBackend.getCredentials(); }; AuthApi.prototype.verifyCredentials = function(callback) { if (this.hasCredentials()) { - this.authBacked.verifyCredentials(callback); + this.authBackend.verifyCredentials(callback); } else { callback(null, false); } From ec75227aa243742c1556dcbb2311db81bf04adb7 Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Fri, 25 May 2018 17:23:24 +0200 Subject: [PATCH 06/30] use apikeys instead of metadata for the database connections parameters --- app/services/user_database_service.js | 64 +++++++++++++++------------ 1 file changed, 35 insertions(+), 29 deletions(-) diff --git a/app/services/user_database_service.js b/app/services/user_database_service.js index 296966d4..5b25635c 100644 --- a/app/services/user_database_service.js +++ b/app/services/user_database_service.js @@ -36,34 +36,10 @@ UserDatabaseService.prototype.getConnectionParams = function (username, apikeyTo const dbopts = { port: global.settings.db_port, - pass: global.settings.db_pubuser_pass + host: dbParams.dbhost, + dbname: dbParams.dbname, }; - dbopts.host = dbParams.dbhost; - dbopts.dbname = dbParams.dbname; - dbopts.user = (!!dbParams.dbpublicuser) ? dbParams.dbpublicuser : global.settings.db_pubuser; - - const user = _.template(global.settings.db_user, {user_id: dbParams.dbuser}); - let pass = null; - - if (global.settings.hasOwnProperty('db_user_pass')) { - pass = _.template(global.settings.db_user_pass, { - user_id: dbParams.dbuser, - user_password: dbParams.dbpass - }); - } - - if (authenticated) { - dbopts.user = user; - dbopts.pass = pass; - } - - let authDbOpts = _.defaults({ user: user, pass: pass }, dbopts); - - if (!apikeyToken) { - return callback(null, dbopts, authDbOpts); - } - this.metadataBackend.getApikey(username, apikeyToken, (err, apikey) => { if (err) { err.http_status = 404; @@ -73,15 +49,45 @@ UserDatabaseService.prototype.getConnectionParams = function (username, apikeyTo } if (!isApiKeyFound(apikey)) { - return callback(null, dbopts, authDbOpts); + const apiKeyNotFoundError = new Error('Unauthorized'); + apiKeyNotFoundError.type = 'auth'; + apiKeyNotFoundError.subtype = 'api-key-not-found'; + apiKeyNotFoundError.http_status = 401; + + return callback(apiKeyNotFoundError); } dbopts.user = apikey.databaseRole; dbopts.pass = apikey.databasePassword; - authDbOpts = _.defaults({ user: user, pass: pass }, dbopts); + this.metadataBackend.getMasterApikey(username, (err, masterApikey) => { + if (err) { + err.http_status = 404; + err.message = errorUserNotFoundMessageTemplate(username); + + return callback(err); + } + + if (!isApiKeyFound(apikey)) { + const apiKeyNotFoundError = new Error('Unauthorized'); + apiKeyNotFoundError.type = 'auth'; + apiKeyNotFoundError.subtype = 'api-key-not-found'; + apiKeyNotFoundError.http_status = 401; + + return callback(apiKeyNotFoundError); + } + + dbopts.user = apikey.databaseRole; + dbopts.pass = apikey.databasePassword; + + const authDbOpts = _.defaults({ + user: masterApikey.databaseRole, + pass: masterApikey.databasePassword }, + dbopts); + + callback(null, dbopts, authDbOpts); + }); - callback(null, dbopts, authDbOpts); }); }); }; From 681b60c27d8aab796fa5cb9efe638dadde780ed6 Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Fri, 25 May 2018 17:25:38 +0200 Subject: [PATCH 07/30] return error if batch job has no proper DB configuration --- batch/query_runner.js | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/batch/query_runner.js b/batch/query_runner.js index a067dd86..7dd3001b 100644 --- a/batch/query_runner.js +++ b/batch/query_runner.js @@ -18,13 +18,12 @@ QueryRunner.prototype.run = function (job_id, sql, user, timeout, dbparams, call return this._run(dbparams, job_id, sql, timeout, callback); } - this.userDatabaseMetadataService.getUserMetadata(user, (err, userDBParams) => { - if (err) { - return callback(err); - } + const dbConfigurationError = new Error('Batch Job DB misconfiguration'); + dbConfigurationError.type = 'batch-job'; + dbConfigurationError.subtype = 'job-db-conf-error'; + dbConfigurationError.http_status = 400; - this._run(userDBParams, job_id, sql, timeout, callback); - }); + return callback(dbConfigurationError); }; QueryRunner.prototype._run = function (dbparams, job_id, sql, timeout, callback) { From d3b6ebd2605dc6f528388dfc05a8ed0e92a8399e Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Fri, 25 May 2018 17:28:56 +0200 Subject: [PATCH 08/30] Fallback to default api key if no api key is provided What happens with oauth authorization? This way we will always have an api_key and oauth will never trigger.... --- app/middlewares/authorization.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/app/middlewares/authorization.js b/app/middlewares/authorization.js index 0b7245c3..79c6a98d 100644 --- a/app/middlewares/authorization.js +++ b/app/middlewares/authorization.js @@ -58,6 +58,13 @@ function getCredentialsFromRequest (req) { } } + // Fallback to Default Public Api Key + if ( ! apiKeyTokenFound(credentials)) { + credentials = { + apiKeyToken: 'default_public' + }; + } + return credentials; } From ebe04d04ad397727b5ef076be4f3cbbc0347377a Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Mon, 28 May 2018 15:53:51 +0200 Subject: [PATCH 09/30] refactor user database service. Get proper DB config based on auth type: oauth vs apikey Oauth uses only master api key configuration Api key uses master and the provided api key configurations Also move default api key fallback to this service --- app/middlewares/authorization.js | 7 ---- app/services/user_database_service.js | 46 +++++++++++++++++---------- 2 files changed, 29 insertions(+), 24 deletions(-) diff --git a/app/middlewares/authorization.js b/app/middlewares/authorization.js index 79c6a98d..0b7245c3 100644 --- a/app/middlewares/authorization.js +++ b/app/middlewares/authorization.js @@ -58,13 +58,6 @@ function getCredentialsFromRequest (req) { } } - // Fallback to Default Public Api Key - if ( ! apiKeyTokenFound(credentials)) { - credentials = { - apiKeyToken: 'default_public' - }; - } - return credentials; } diff --git a/app/services/user_database_service.js b/app/services/user_database_service.js index 5b25635c..1d73c448 100644 --- a/app/services/user_database_service.js +++ b/app/services/user_database_service.js @@ -15,6 +15,10 @@ function errorUserNotFoundMessageTemplate (user) { return `Sorry, we can't find CARTO user '${user}'. Please check that you have entered the correct domain.`; } +function isOauthAuthorization({ apikeyToken, authenticated }) { + return authenticated && !apikeyToken; +} + /** * Callback is invoked with `dbParams` and `authDbParams`. * `dbParams` depends on AuthApi verification so it might return a public user with just SELECT permission, where @@ -34,13 +38,14 @@ UserDatabaseService.prototype.getConnectionParams = function (username, apikeyTo return callback(err); } - const dbopts = { + const commonDBConfiguration = { port: global.settings.db_port, host: dbParams.dbhost, dbname: dbParams.dbname, }; - this.metadataBackend.getApikey(username, apikeyToken, (err, apikey) => { + this.metadataBackend.getMasterApikey(username, (err, masterApikey) => { + if (err) { err.http_status = 404; err.message = errorUserNotFoundMessageTemplate(username); @@ -48,19 +53,29 @@ UserDatabaseService.prototype.getConnectionParams = function (username, apikeyTo return callback(err); } - if (!isApiKeyFound(apikey)) { + if (!isApiKeyFound(masterApikey)) { const apiKeyNotFoundError = new Error('Unauthorized'); apiKeyNotFoundError.type = 'auth'; apiKeyNotFoundError.subtype = 'api-key-not-found'; apiKeyNotFoundError.http_status = 401; - return callback(apiKeyNotFoundError); + return callback(apiKeyNotFoundError); } - dbopts.user = apikey.databaseRole; - dbopts.pass = apikey.databasePassword; + const masterDBConfiguration = Object.assign({ + user: masterApikey.databaseRole, + pass: masterApikey.databasePassword + }, + commonDBConfiguration); - this.metadataBackend.getMasterApikey(username, (err, masterApikey) => { + if (isOauthAuthorization({apikeyToken,authenticated})) { + callback(null, masterDBConfiguration, masterDBConfiguration); + } + + // Default Api key fallback + apikeyToken = apikeyToken || 'default_public'; + + this.metadataBackend.getApikey(username, apikeyToken, (err, apikey) => { if (err) { err.http_status = 404; err.message = errorUserNotFoundMessageTemplate(username); @@ -74,20 +89,17 @@ UserDatabaseService.prototype.getConnectionParams = function (username, apikeyTo apiKeyNotFoundError.subtype = 'api-key-not-found'; apiKeyNotFoundError.http_status = 401; - return callback(apiKeyNotFoundError); + return callback(apiKeyNotFoundError); } - dbopts.user = apikey.databaseRole; - dbopts.pass = apikey.databasePassword; + const DBConfiguration = Object.assign({ + user: apikey.databaseRole, + pass: apikey.databasePassword + }, + commonDBConfiguration); - const authDbOpts = _.defaults({ - user: masterApikey.databaseRole, - pass: masterApikey.databasePassword }, - dbopts); - - callback(null, dbopts, authDbOpts); + callback(null, DBConfiguration, masterDBConfiguration); }); - }); }); }; From ef9a5aeb20d9453d3ed65c94a58a81dc871b6b1d Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Mon, 28 May 2018 15:54:48 +0200 Subject: [PATCH 10/30] FIX: use proper database public test user in api keys --- test/prepare_db.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/prepare_db.sh b/test/prepare_db.sh index e7d1400b..35b3ccc6 100755 --- a/test/prepare_db.sh +++ b/test/prepare_db.sh @@ -188,7 +188,7 @@ HMSET api_keys:vizzuality:default_public \ user "vizzuality" \ type "default" \ grants_sql "true" \ - database_role "test_windshaft_publicuser" \ + database_role "testpublicuser" \ database_password "public" EOF @@ -230,7 +230,7 @@ HMSET api_keys:cartodb250user:default_public \ user "cartodb250user" \ type "default" \ grants_sql "true" \ - database_role "test_windshaft_publicuser" \ + database_role "testpublicuser" \ database_password "public" EOF From 017dc69c026cbb875530a520decc08eb40de1392 Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Mon, 28 May 2018 17:38:04 +0200 Subject: [PATCH 11/30] add auth params to test. Is this OK? --- test/integration/batch/job_runner.test.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/integration/batch/job_runner.test.js b/test/integration/batch/job_runner.test.js index 6f923e32..b2f84e06 100644 --- a/test/integration/batch/job_runner.test.js +++ b/test/integration/batch/job_runner.test.js @@ -35,7 +35,11 @@ var HOST = 'localhost'; var JOB = { user: USER, query: QUERY, - host: HOST + host: HOST, + dbname: 'cartodb_test_user_1_db', + dbuser: 'test_cartodb_user_1', + port: 5432, + pass: 'test_cartodb_user_1_pass', }; describe('job runner', function() { From fa5a99211c7db544e5ef22cebb74cb9f09db6a4e Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Tue, 29 May 2018 13:23:50 +0200 Subject: [PATCH 12/30] check user exists in user middleware This way, we keep sending a 404 error if the user does not exist. --- app/auth/apikey.js | 8 ++------ app/controllers/job_controller.js | 2 +- app/controllers/query_controller.js | 2 +- app/middlewares/user.js | 32 ++++++++++++++++++++++++++--- 4 files changed, 33 insertions(+), 11 deletions(-) diff --git a/app/auth/apikey.js b/app/auth/apikey.js index bd3181b9..cc1ee0b4 100644 --- a/app/auth/apikey.js +++ b/app/auth/apikey.js @@ -10,10 +10,6 @@ function ApikeyAuth(req, metadataBackend, username, apikey) { module.exports = ApikeyAuth; -function errorUserNotFoundMessageTemplate (user) { - return `Sorry, we can't find CARTO user '${user}'. Please check that you have entered the correct domain.`; -} - function usernameMatches(basicAuthUsername, requestUsername) { return !(basicAuthUsername && (basicAuthUsername !== requestUsername)); } @@ -21,8 +17,8 @@ function usernameMatches(basicAuthUsername, requestUsername) { ApikeyAuth.prototype.verifyCredentials = function (callback) { this.metadataBackend.getApikey(this.username, this.apikey, (err, apikey) => { if (err) { - err.http_status = 404; - err.message = errorUserNotFoundMessageTemplate(this.username); + err.http_status = 500; + err.message = 'Unexpected error fetching from Redis'; return callback(err); } diff --git a/app/controllers/job_controller.js b/app/controllers/job_controller.js index 85a98884..9b2e74bc 100644 --- a/app/controllers/job_controller.js +++ b/app/controllers/job_controller.js @@ -55,7 +55,7 @@ function composeJobMiddlewares (metadataBackend, userDatabaseService, jobService return [ initializeProfilerMiddleware('job'), - userMiddleware(), + userMiddleware(metadataBackend), rateLimitsMiddleware(userLimitsService, endpointGroup), authorizationMiddleware(metadataBackend, forceToBeAuthenticated), connectionParamsMiddleware(userDatabaseService), diff --git a/app/controllers/query_controller.js b/app/controllers/query_controller.js index 6e5ccd61..02a09a6b 100644 --- a/app/controllers/query_controller.js +++ b/app/controllers/query_controller.js @@ -36,7 +36,7 @@ QueryController.prototype.route = function (app) { const queryMiddlewares = endpointGroup => { return [ initializeProfilerMiddleware('query'), - userMiddleware(), + userMiddleware(this.metadataBackend), rateLimitsMiddleware(this.userLimitsService, endpointGroup), authorizationMiddleware(this.metadataBackend), connectionParamsMiddleware(this.userDatabaseService), diff --git a/app/middlewares/user.js b/app/middlewares/user.js index bfcebd0d..2e2ed175 100644 --- a/app/middlewares/user.js +++ b/app/middlewares/user.js @@ -1,10 +1,36 @@ const CdbRequest = require('../models/cartodb_request'); -module.exports = function user () { +module.exports = function user(metadataBackend) { const cdbRequest = new CdbRequest(); return function userMiddleware (req, res, next) { - res.locals.user = cdbRequest.userByReq(req); - next(); + res.locals.user = getUserNameFromRequest(req, cdbRequest); + + checkUserExists(metadataBackend, res.locals.user, function(userExists) { + if (userExists) { + return next(); + } else { + const error = new Error('Unauthorized'); + error.type = 'auth'; + error.subtype = 'user-not-found'; + error.http_status = 404; + error.message = errorUserNotFoundMessageTemplate(res.locals.user); + next(error); + } + }); }; }; + +function getUserNameFromRequest(req, cdbRequest) { + return cdbRequest.userByReq(req); +} + +function checkUserExists(metadataBackend, userName, callback) { + metadataBackend.getUserId(userName, function(err) { + callback(!err); + }); +} + +function errorUserNotFoundMessageTemplate(user) { + return `Sorry, we can't find CARTO user '${user}'. Please check that you have entered the correct domain.`; +} From 6e3eb8ef288bad7b9e7c807e855720380f89a1bb Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Wed, 30 May 2018 11:48:15 +0200 Subject: [PATCH 13/30] Fix test: add auth params to job --- test/acceptance/batch/batch-drain.test.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/test/acceptance/batch/batch-drain.test.js b/test/acceptance/batch/batch-drain.test.js index 9a1cf0d2..cbb83e99 100644 --- a/test/acceptance/batch/batch-drain.test.js +++ b/test/acceptance/batch/batch-drain.test.js @@ -37,7 +37,11 @@ describe('batch module', function() { var data = { user: username, query: sql, - host: dbInstance + host: dbInstance, + dbname: 'cartodb_test_user_1_db', + dbuser: 'test_cartodb_user_1', + port: 5432, + pass: 'test_cartodb_user_1_pass', }; jobService.create(data, function (err, job) { @@ -60,7 +64,6 @@ describe('batch module', function() { if (err) { done(err); } - assert.equal(job.status, 'running'); self.batch.drain(function () { From 7764975c09df65683b631f850df224788efd6a20 Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Wed, 30 May 2018 12:08:04 +0200 Subject: [PATCH 14/30] please jshint: remove unnecessary require (underscore) --- app/services/user_database_service.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/services/user_database_service.js b/app/services/user_database_service.js index 1d73c448..c22353c8 100644 --- a/app/services/user_database_service.js +++ b/app/services/user_database_service.js @@ -1,5 +1,3 @@ -const _ = require('underscore'); - function isApiKeyFound(apikey) { return apikey.type !== null && apikey.user !== null && From 60beea19d75b6d235f6b1c5d2160505e754d8915 Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Wed, 30 May 2018 13:28:50 +0200 Subject: [PATCH 15/30] add default public fallback tests --- test/acceptance/app.auth.test.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/test/acceptance/app.auth.test.js b/test/acceptance/app.auth.test.js index 38af61aa..c63338a0 100644 --- a/test/acceptance/app.auth.test.js +++ b/test/acceptance/app.auth.test.js @@ -6,6 +6,16 @@ var assert = require('../support/assert'); describe('app.auth', function() { var scenarios = [ + { + desc: 'no api key should fallback to default api key', + url: "/api/v1/sql?q=SELECT%20*%20FROM%20untitle_table_4", + statusCode: 200 + }, + { + desc: 'invalid api key should return 401', + url: "/api/v1/sql?api_key=THIS_API_KEY_NOT_EXIST&q=SELECT%20*%20FROM%20untitle_table_4", + statusCode: 401 + }, { desc: 'valid api key should allow insert in protected tables', url: "/api/v1/sql?api_key=1234&q=INSERT%20INTO%20private_table%20(name)%20VALUES%20('app_auth_test1')", From cf8bf6e5e6f5fbe1d2e7585b8f7dab8ab515c463 Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Wed, 30 May 2018 17:36:55 +0200 Subject: [PATCH 16/30] get DB configuration from job instead from metadata in Job Canceller --- app/server.js | 5 +---- batch/index.js | 2 +- batch/job_canceller.js | 16 ++++++++++------ test/acceptance/batch/batch-drain.test.js | 4 +--- test/integration/batch/job-queue.test.js | 4 +--- test/integration/batch/job_canceller.test.js | 8 ++++++-- test/integration/batch/job_runner.test.js | 2 +- test/integration/batch/job_service.test.js | 10 ++++++++-- 8 files changed, 29 insertions(+), 22 deletions(-) diff --git a/app/server.js b/app/server.js index 6223d4eb..ec58b0f8 100644 --- a/app/server.js +++ b/app/server.js @@ -30,8 +30,6 @@ var JobBackend = require('../batch/job_backend'); var JobCanceller = require('../batch/job_canceller'); var JobService = require('../batch/job_service'); -var UserDatabaseMetadataService = require('../batch/user_database_metadata_service'); - var cors = require('./middlewares/cors'); var GenericController = require('./controllers/generic_controller'); @@ -147,8 +145,7 @@ function App(statsClient) { var jobPublisher = new JobPublisher(redisPool); var jobQueue = new JobQueue(metadataBackend, jobPublisher); var jobBackend = new JobBackend(metadataBackend, jobQueue); - var userDatabaseMetadataService = new UserDatabaseMetadataService(metadataBackend); - var jobCanceller = new JobCanceller(userDatabaseMetadataService); + var jobCanceller = new JobCanceller(); var jobService = new JobService(jobBackend, jobCanceller); var genericController = new GenericController(); diff --git a/batch/index.js b/batch/index.js index 2c0e820b..694dfb42 100644 --- a/batch/index.js +++ b/batch/index.js @@ -21,7 +21,7 @@ module.exports = function batchFactory (metadataBackend, redisPool, name, statsd var jobQueue = new JobQueue(metadataBackend, jobPublisher); var jobBackend = new JobBackend(metadataBackend, jobQueue); var queryRunner = new QueryRunner(userDatabaseMetadataService); - var jobCanceller = new JobCanceller(userDatabaseMetadataService); + var jobCanceller = new JobCanceller(); var jobService = new JobService(jobBackend, jobCanceller); var jobRunner = new JobRunner(jobService, jobQueue, queryRunner, metadataBackend, statsdClient); var logger = new BatchLogger(loggerPath); diff --git a/batch/job_canceller.js b/batch/job_canceller.js index e8d51679..e4731a2c 100644 --- a/batch/job_canceller.js +++ b/batch/job_canceller.js @@ -9,13 +9,17 @@ function JobCanceller(userDatabaseMetadataService) { module.exports = JobCanceller; JobCanceller.prototype.cancel = function (job, callback) { - this.userDatabaseMetadataService.getUserMetadata(job.data.user, function (err, userDatabaseMetadata) { - if (err) { - return callback(err); - } - doCancel(job.data.job_id, userDatabaseMetadata, callback); - }); + const dbConfiguration = { + host: job.data.host, + port: job.data.port, + dbname: job.data.dbname, + user: job.data.dbuser, + pass: job.data.pass, + authenticated: true + }; + + doCancel(job.data.job_id, dbConfiguration, callback); }; function doCancel(job_id, userDatabaseMetadata, callback) { diff --git a/test/acceptance/batch/batch-drain.test.js b/test/acceptance/batch/batch-drain.test.js index cbb83e99..570949fa 100644 --- a/test/acceptance/batch/batch-drain.test.js +++ b/test/acceptance/batch/batch-drain.test.js @@ -7,7 +7,6 @@ var JobPublisher = require('../../../batch/pubsub/job-publisher'); var JobQueue = require('../../../batch/job_queue'); var JobBackend = require('../../../batch/job_backend'); var JobService = require('../../../batch/job_service'); -var UserDatabaseMetadataService = require('../../../batch/user_database_metadata_service'); var JobCanceller = require('../../../batch/job_canceller'); var metadataBackend = require('cartodb-redis')({ pool: redisUtils.getPool() }); @@ -18,8 +17,7 @@ describe('batch module', function() { var jobPublisher = new JobPublisher(pool); var jobQueue = new JobQueue(metadataBackend, jobPublisher); var jobBackend = new JobBackend(metadataBackend, jobQueue); - var userDatabaseMetadataService = new UserDatabaseMetadataService(metadataBackend); - var jobCanceller = new JobCanceller(userDatabaseMetadataService); + var jobCanceller = new JobCanceller(); var jobService = new JobService(jobBackend, jobCanceller); before(function (done) { diff --git a/test/integration/batch/job-queue.test.js b/test/integration/batch/job-queue.test.js index e6b074ae..892a54ad 100644 --- a/test/integration/batch/job-queue.test.js +++ b/test/integration/batch/job-queue.test.js @@ -10,7 +10,6 @@ var JobQueue = require('../../../batch/job_queue'); var JobBackend = require('../../../batch/job_backend'); var JobService = require('../../../batch/job_service'); -var UserDatabaseMetadataService = require('../../../batch/user_database_metadata_service'); var JobCanceller = require('../../../batch/job_canceller'); var metadataBackend = require('cartodb-redis')({ pool: redisUtils.getPool() }); @@ -19,8 +18,7 @@ describe('job queue', function () { var jobPublisher = new JobPublisher(pool); var jobQueue = new JobQueue(metadataBackend, jobPublisher); var jobBackend = new JobBackend(metadataBackend, jobQueue); - var userDatabaseMetadataService = new UserDatabaseMetadataService(metadataBackend); - var jobCanceller = new JobCanceller(userDatabaseMetadataService); + var jobCanceller = new JobCanceller(); var jobService = new JobService(jobBackend, jobCanceller); var userA = 'userA'; diff --git a/test/integration/batch/job_canceller.test.js b/test/integration/batch/job_canceller.test.js index d08cbe47..8ead991c 100644 --- a/test/integration/batch/job_canceller.test.js +++ b/test/integration/batch/job_canceller.test.js @@ -65,12 +65,16 @@ function createWadusJob(query) { return JobFactory.create(JSON.parse(JSON.stringify({ user: USER, query: query, - host: HOST + host: HOST, + dbname: 'cartodb_test_user_1_db', + dbuser: 'test_cartodb_user_1', + port: 5432, + pass: 'test_cartodb_user_1_pass', }))); } describe('job canceller', function() { - var jobCanceller = new JobCanceller(userDatabaseMetadataService); + var jobCanceller = new JobCanceller(); after(function (done) { redisUtils.clean('batch:*', done); diff --git a/test/integration/batch/job_runner.test.js b/test/integration/batch/job_runner.test.js index b2f84e06..ef8e3dd9 100644 --- a/test/integration/batch/job_runner.test.js +++ b/test/integration/batch/job_runner.test.js @@ -23,7 +23,7 @@ var jobPublisher = new JobPublisher(redisUtils.getPool()); var jobQueue = new JobQueue(metadataBackend, jobPublisher); var jobBackend = new JobBackend(metadataBackend, jobQueue); var userDatabaseMetadataService = new UserDatabaseMetadataService(metadataBackend); -var jobCanceller = new JobCanceller(userDatabaseMetadataService); +var jobCanceller = new JobCanceller(); var jobService = new JobService(jobBackend, jobCanceller); var queryRunner = new QueryRunner(userDatabaseMetadataService); var StatsD = require('node-statsd').StatsD; diff --git a/test/integration/batch/job_service.test.js b/test/integration/batch/job_service.test.js index 284672e7..01193d44 100644 --- a/test/integration/batch/job_service.test.js +++ b/test/integration/batch/job_service.test.js @@ -21,7 +21,7 @@ var jobPublisher = new JobPublisher(redisUtils.getPool()); var jobQueue = new JobQueue(metadataBackend, jobPublisher); var jobBackend = new JobBackend(metadataBackend, jobQueue); var userDatabaseMetadataService = new UserDatabaseMetadataService(metadataBackend); -var jobCanceller = new JobCanceller(userDatabaseMetadataService); +var jobCanceller = new JobCanceller(); var USER = 'vizzuality'; var QUERY = 'select pg_sleep(0)'; @@ -29,7 +29,12 @@ var HOST = 'localhost'; var JOB = { user: USER, query: QUERY, - host: HOST + host: HOST, + dbname: 'cartodb_test_user_1_db', + dbuser: 'test_cartodb_user_1', + port: 5432, + pass: 'test_cartodb_user_1_pass', + }; function createWadusDataJob() { @@ -50,6 +55,7 @@ function runQueryHelper(job, callback) { return callback(err); } + //TODO use job db conf userDatabaseMetadataService.getUserMetadata(user, function (err, userDatabaseMetadata) { if (err) { return callback(err); From 71de7248ba90e6d3484966fdc513968f7a73851e Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Wed, 30 May 2018 17:38:06 +0200 Subject: [PATCH 17/30] get DB configuration from job instead from metadata in Job Canceller (again) --- batch/job_canceller.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/batch/job_canceller.js b/batch/job_canceller.js index e4731a2c..888a993c 100644 --- a/batch/job_canceller.js +++ b/batch/job_canceller.js @@ -2,8 +2,7 @@ var PSQL = require('cartodb-psql'); -function JobCanceller(userDatabaseMetadataService) { - this.userDatabaseMetadataService = userDatabaseMetadataService; +function JobCanceller() { } module.exports = JobCanceller; From 46e3a87f41619ff726c479dd81c36c8f1da2c56e Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Wed, 30 May 2018 18:15:35 +0200 Subject: [PATCH 18/30] add some auth TODOs --- app/services/pg-entities-access-validator.js | 2 +- batch/user_database_metadata_service.js | 1 + test/integration/batch/job_service.test.js | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/app/services/pg-entities-access-validator.js b/app/services/pg-entities-access-validator.js index e294f8f8..3f213aeb 100644 --- a/app/services/pg-entities-access-validator.js +++ b/app/services/pg-entities-access-validator.js @@ -23,7 +23,7 @@ const Validator = { if (global.settings.validatePGEntitiesAccess) { hardValidationResult = this.hardValidation(affectedTables.tables); } - +//TODO AUTH only default public? if (!authenticated) { softValidationResult = this.softValidation(affectedTables.tables); } diff --git a/batch/user_database_metadata_service.js b/batch/user_database_metadata_service.js index 2cfe79e3..9863670c 100644 --- a/batch/user_database_metadata_service.js +++ b/batch/user_database_metadata_service.js @@ -18,6 +18,7 @@ UserDatabaseMetadataService.prototype.getUserMetadata = function (username, call }); }; +//TODO AUTH is (all of) this necessary? UserDatabaseMetadataService.prototype.parseMetadataToDatabase = function (userDatabaseMetadata) { var dbParams = userDatabaseMetadata; diff --git a/test/integration/batch/job_service.test.js b/test/integration/batch/job_service.test.js index 01193d44..a80f9034 100644 --- a/test/integration/batch/job_service.test.js +++ b/test/integration/batch/job_service.test.js @@ -55,7 +55,7 @@ function runQueryHelper(job, callback) { return callback(err); } - //TODO use job db conf + //TODO AUTH use job db conf userDatabaseMetadataService.getUserMetadata(user, function (err, userDatabaseMetadata) { if (err) { return callback(err); From 8515c2cc310f585544eb079d723bd3c88ed86ea0 Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Mon, 4 Jun 2018 11:28:59 +0200 Subject: [PATCH 19/30] use job configuration instead of user metadata service --- test/integration/batch/job_service.test.js | 32 ++++++++++------------ 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/test/integration/batch/job_service.test.js b/test/integration/batch/job_service.test.js index a80f9034..bafc240e 100644 --- a/test/integration/batch/job_service.test.js +++ b/test/integration/batch/job_service.test.js @@ -11,7 +11,6 @@ var JobQueue = require(BATCH_SOURCE + 'job_queue'); var JobBackend = require(BATCH_SOURCE + 'job_backend'); var JobPublisher = require(BATCH_SOURCE + 'pubsub/job-publisher'); var jobStatus = require(BATCH_SOURCE + 'job_status'); -var UserDatabaseMetadataService = require(BATCH_SOURCE + 'user_database_metadata_service'); var JobCanceller = require(BATCH_SOURCE + 'job_canceller'); var JobService = require(BATCH_SOURCE + 'job_service'); var PSQL = require('cartodb-psql'); @@ -20,7 +19,6 @@ var metadataBackend = require('cartodb-redis')({ pool: redisUtils.getPool() }); var jobPublisher = new JobPublisher(redisUtils.getPool()); var jobQueue = new JobQueue(metadataBackend, jobPublisher); var jobBackend = new JobBackend(metadataBackend, jobQueue); -var userDatabaseMetadataService = new UserDatabaseMetadataService(metadataBackend); var jobCanceller = new JobCanceller(); var USER = 'vizzuality'; @@ -45,7 +43,6 @@ function createWadusDataJob() { // in order to test query cancelation/draining function runQueryHelper(job, callback) { var job_id = job.job_id; - var user = job.user; var sql = job.query; job.status = jobStatus.RUNNING; @@ -55,28 +52,29 @@ function runQueryHelper(job, callback) { return callback(err); } - //TODO AUTH use job db conf - userDatabaseMetadataService.getUserMetadata(user, function (err, userDatabaseMetadata) { + const dbConfiguration = { + host: job.host, + port: job.port, + dbname: job.dbname, + user: job.dbuser, + pass: job.pass, + }; + + var pg = new PSQL(dbConfiguration); + + sql = '/* ' + job_id + ' */ ' + sql; + + pg.eventedQuery(sql, function (err, query) { if (err) { return callback(err); } - var pg = new PSQL(userDatabaseMetadata); - - sql = '/* ' + job_id + ' */ ' + sql; - - pg.eventedQuery(sql, function (err, query) { - if (err) { - return callback(err); - } - - callback(null, query); - }); + callback(null, query); }); }); } -describe('job service', function() { +describe.only('job service', function() { var jobService = new JobService(jobBackend, jobCanceller); after(function (done) { From a4292f08cff1b1187354f8d442e4938de2528028 Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Tue, 5 Jun 2018 13:13:09 +0200 Subject: [PATCH 20/30] refactor apikey to apikeyToken --- app/auth/apikey.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/auth/apikey.js b/app/auth/apikey.js index cc1ee0b4..0c4e0eb1 100644 --- a/app/auth/apikey.js +++ b/app/auth/apikey.js @@ -1,11 +1,11 @@ /** * this module allows to auth user using an pregenerated api key */ -function ApikeyAuth(req, metadataBackend, username, apikey) { +function ApikeyAuth(req, metadataBackend, username, apikeyToken) { this.req = req; this.metadataBackend = metadataBackend; this.username = username; - this.apikey = apikey; + this.apikeyToken = apikeyToken; } module.exports = ApikeyAuth; @@ -15,7 +15,7 @@ function usernameMatches(basicAuthUsername, requestUsername) { } ApikeyAuth.prototype.verifyCredentials = function (callback) { - this.metadataBackend.getApikey(this.username, this.apikey, (err, apikey) => { + this.metadataBackend.getApikey(this.username, this.apikeyToken, (err, apikey) => { if (err) { err.http_status = 500; err.message = 'Unexpected error fetching from Redis'; @@ -53,11 +53,11 @@ ApikeyAuth.prototype.verifyCredentials = function (callback) { }; ApikeyAuth.prototype.hasCredentials = function () { - return !!this.apikey; + return !!this.apikeyToken; }; ApikeyAuth.prototype.getCredentials = function () { - return this.apikey; + return this.apikeyToken; }; function verifyRequest(apikey, requiredApikey) { From 0207b67d5f1d069592ffe7eef0b4f747c90b84d0 Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Tue, 5 Jun 2018 13:14:50 +0200 Subject: [PATCH 21/30] refactor forceToBeAuthenticated to forceToBeMaster --- app/controllers/job_controller.js | 4 ++-- app/controllers/query_controller.js | 4 +++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/app/controllers/job_controller.js b/app/controllers/job_controller.js index 9b2e74bc..ecfca95b 100644 --- a/app/controllers/job_controller.js +++ b/app/controllers/job_controller.js @@ -51,13 +51,13 @@ JobController.prototype.route = function (app) { function composeJobMiddlewares (metadataBackend, userDatabaseService, jobService, statsdClient, userLimitsService) { return function jobMiddlewares (action, jobMiddleware, endpointGroup) { - const forceToBeAuthenticated = true; + const forceToBeMaster = true; return [ initializeProfilerMiddleware('job'), userMiddleware(metadataBackend), rateLimitsMiddleware(userLimitsService, endpointGroup), - authorizationMiddleware(metadataBackend, forceToBeAuthenticated), + authorizationMiddleware(metadataBackend, forceToBeMaster), connectionParamsMiddleware(userDatabaseService), jobMiddleware(jobService), setServedByDBHostHeader(), diff --git a/app/controllers/query_controller.js b/app/controllers/query_controller.js index 02a09a6b..6b40cc4a 100644 --- a/app/controllers/query_controller.js +++ b/app/controllers/query_controller.js @@ -33,12 +33,14 @@ function QueryController(metadataBackend, userDatabaseService, tableCache, stats QueryController.prototype.route = function (app) { const { base_url } = global.settings; + const forceToBeMaster = false; + const queryMiddlewares = endpointGroup => { return [ initializeProfilerMiddleware('query'), userMiddleware(this.metadataBackend), rateLimitsMiddleware(this.userLimitsService, endpointGroup), - authorizationMiddleware(this.metadataBackend), + authorizationMiddleware(this.metadataBackend, forceToBeMaster), connectionParamsMiddleware(this.userDatabaseService), timeoutLimitsMiddleware(this.metadataBackend), this.handleQuery.bind(this), From da08e42921a1380baee5c83d8932a514b33b49b4 Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Tue, 5 Jun 2018 13:16:36 +0200 Subject: [PATCH 22/30] refactor forceToBeAuthenticated to forceToBeMaster --- app/middlewares/authorization.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/middlewares/authorization.js b/app/middlewares/authorization.js index 0b7245c3..85d5e10a 100644 --- a/app/middlewares/authorization.js +++ b/app/middlewares/authorization.js @@ -1,7 +1,7 @@ const AuthApi = require('../auth/auth_api'); const basicAuth = require('basic-auth'); -module.exports = function authorization (metadataBackend, forceToBeAuthenticated = false) { +module.exports = function authorization (metadataBackend, forceToBeMaster = false) { return function authorizationMiddleware (req, res, next) { const { user } = res.locals; const credentials = getCredentialsFromRequest(req); From eab3d289b6005ad477ec03bd622ba546d6d58c8c Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Tue, 5 Jun 2018 13:21:56 +0200 Subject: [PATCH 23/30] refactor authenticated to authorizationLevel --- app/auth/apikey.js | 6 ++-- app/auth/oauth.js | 3 +- app/controllers/query_controller.js | 6 ++-- app/middlewares/authorization.js | 6 ++-- app/middlewares/connection-params.js | 4 +-- app/middlewares/timeout-limits.js | 4 +-- app/services/pg-entities-access-validator.js | 6 ++-- app/services/user_database_service.js | 8 +++--- batch/job_canceller.js | 2 +- batch/user_database_metadata_service.js | 4 +-- test/unit/oauth.test.js | 4 +-- .../unit/pg-entities-access-validator.test.js | 28 +++++++++---------- 12 files changed, 41 insertions(+), 40 deletions(-) diff --git a/app/auth/apikey.js b/app/auth/apikey.js index 0c4e0eb1..94a362f3 100644 --- a/app/auth/apikey.js +++ b/app/auth/apikey.js @@ -40,7 +40,7 @@ ApikeyAuth.prototype.verifyCredentials = function (callback) { return callback(forbiddenError); } - return callback(null, verifyRequest(this.apikey, this.apikey)); + return callback(null, getAuthorizationLevel(apikey)); } else { const apiKeyNotFoundError = new Error('Unauthorized'); apiKeyNotFoundError.type = 'auth'; @@ -60,8 +60,8 @@ ApikeyAuth.prototype.getCredentials = function () { return this.apikeyToken; }; -function verifyRequest(apikey, requiredApikey) { - return (apikey === requiredApikey && apikey !== 'default_public'); +function getAuthorizationLevel(apikey) { + return apikey.type; } function isApiKeyFound(apikey) { diff --git a/app/auth/oauth.js b/app/auth/oauth.js index 5cab4e5c..0cafb627 100644 --- a/app/auth/oauth.js +++ b/app/auth/oauth.js @@ -142,7 +142,8 @@ var oAuth = (function(){ }, false); }, function finishValidation(err, hasValidSignature) { - return callback(err, hasValidSignature || null); + const authorizationLevel = hasValidSignature ? 'master' : null; + return callback(err, authorizationLevel); } ); }; diff --git a/app/controllers/query_controller.js b/app/controllers/query_controller.js index 6b40cc4a..5a642423 100644 --- a/app/controllers/query_controller.js +++ b/app/controllers/query_controller.js @@ -70,7 +70,7 @@ QueryController.prototype.handleQuery = function (req, res, next) { var filename = requestedFilename; var requestedSkipfields = params.skipfields; - const { user: username, userDbParams: dbopts, authDbParams, userLimits, authenticated } = res.locals; + const { user: username, userDbParams: dbopts, authDbParams, userLimits, authorizationLevel } = res.locals; var skipfields; var dp = params.dp; // decimal point digits (defaults to 6) @@ -145,7 +145,7 @@ QueryController.prototype.handleQuery = function (req, res, next) { var pg = new PSQL(authDbParams); - var skipCache = authenticated; + var skipCache = authorizationLevel === 'master'; self.queryTables.getAffectedTablesFromQuery(pg, sql, skipCache, function(err, result) { if (err) { @@ -164,7 +164,7 @@ QueryController.prototype.handleQuery = function (req, res, next) { } checkAborted('setHeaders'); - if(!pgEntitiesAccessValidator.validate(affectedTables, authenticated)) { + if(!pgEntitiesAccessValidator.validate(affectedTables, authorizationLevel)) { const syntaxError = new SyntaxError("system tables are forbidden"); syntaxError.http_status = 403; throw(syntaxError); diff --git a/app/middlewares/authorization.js b/app/middlewares/authorization.js index 85d5e10a..020d6728 100644 --- a/app/middlewares/authorization.js +++ b/app/middlewares/authorization.js @@ -19,7 +19,7 @@ module.exports = function authorization (metadataBackend, forceToBeMaster = fals const params = Object.assign({ metadataBackend }, res.locals, req.query, req.body); const authApi = new AuthApi(req, params); - authApi.verifyCredentials(function (err, authenticated) { + authApi.verifyCredentials(function (err, authorizationLevel) { if (req.profiler) { req.profiler.done('authorization'); } @@ -28,9 +28,9 @@ module.exports = function authorization (metadataBackend, forceToBeMaster = fals return next(err); } - res.locals.authenticated = authenticated; + res.locals.authorizationLevel = authorizationLevel; - if (forceToBeAuthenticated && !authenticated) { + if (forceToBeMaster && authorizationLevel !== 'master') { return next(new Error('permission denied')); } diff --git a/app/middlewares/connection-params.js b/app/middlewares/connection-params.js index dd5d5ea5..e6320a6c 100644 --- a/app/middlewares/connection-params.js +++ b/app/middlewares/connection-params.js @@ -1,8 +1,8 @@ module.exports = function connectionParams (userDatabaseService) { return function connectionParamsMiddleware (req, res, next) { - const { user, api_key: apikeyToken, authenticated } = res.locals; + const { user, api_key: apikeyToken, authorizationLevel } = res.locals; - userDatabaseService.getConnectionParams(user, apikeyToken, authenticated, + userDatabaseService.getConnectionParams(user, apikeyToken, authorizationLevel, function (err, userDbParams, authDbParams) { if (req.profiler) { req.profiler.done('getConnectionParams'); diff --git a/app/middlewares/timeout-limits.js b/app/middlewares/timeout-limits.js index e40f048c..076ba6ad 100644 --- a/app/middlewares/timeout-limits.js +++ b/app/middlewares/timeout-limits.js @@ -1,6 +1,6 @@ module.exports = function timeoutLimits (metadataBackend) { return function timeoutLimitsMiddleware (req, res, next) { - const { user, authenticated } = res.locals; + const { user, authorizationLevel } = res.locals; metadataBackend.getUserTimeoutRenderLimits(user, function (err, timeoutRenderLimit) { if (req.profiler) { @@ -12,7 +12,7 @@ module.exports = function timeoutLimits (metadataBackend) { } const userLimits = { - timeout: authenticated ? timeoutRenderLimit.render : timeoutRenderLimit.renderPublic + timeout: (authorizationLevel === 'master') ? timeoutRenderLimit.render : timeoutRenderLimit.renderPublic }; res.locals.userLimits = userLimits; diff --git a/app/services/pg-entities-access-validator.js b/app/services/pg-entities-access-validator.js index 3f213aeb..e2d90262 100644 --- a/app/services/pg-entities-access-validator.js +++ b/app/services/pg-entities-access-validator.js @@ -15,7 +15,7 @@ const FORBIDDEN_ENTITIES = { }; const Validator = { - validate(affectedTables, authenticated) { + validate(affectedTables, authorizationLevel) { let hardValidationResult = true; let softValidationResult = true; @@ -23,8 +23,8 @@ const Validator = { if (global.settings.validatePGEntitiesAccess) { hardValidationResult = this.hardValidation(affectedTables.tables); } -//TODO AUTH only default public? - if (!authenticated) { + + if (authorizationLevel !== 'master') { softValidationResult = this.softValidation(affectedTables.tables); } } diff --git a/app/services/user_database_service.js b/app/services/user_database_service.js index c22353c8..dbda65b3 100644 --- a/app/services/user_database_service.js +++ b/app/services/user_database_service.js @@ -13,8 +13,8 @@ function errorUserNotFoundMessageTemplate (user) { return `Sorry, we can't find CARTO user '${user}'. Please check that you have entered the correct domain.`; } -function isOauthAuthorization({ apikeyToken, authenticated }) { - return authenticated && !apikeyToken; +function isOauthAuthorization({ apikeyToken, authorizationLevel }) { + return (authorizationLevel === 'master') && !apikeyToken; } /** @@ -27,7 +27,7 @@ function isOauthAuthorization({ apikeyToken, authenticated }) { * @param {String} cdbUsername * @param {Function} callback (err, dbParams, authDbParams) */ -UserDatabaseService.prototype.getConnectionParams = function (username, apikeyToken, authenticated, callback) { +UserDatabaseService.prototype.getConnectionParams = function (username, apikeyToken, authorizationLevel, callback) { this.metadataBackend.getAllUserDBParams(username, (err, dbParams) => { if (err) { err.http_status = 404; @@ -66,7 +66,7 @@ UserDatabaseService.prototype.getConnectionParams = function (username, apikeyTo }, commonDBConfiguration); - if (isOauthAuthorization({apikeyToken,authenticated})) { + if (isOauthAuthorization({ apikeyToken, authorizationLevel})) { callback(null, masterDBConfiguration, masterDBConfiguration); } diff --git a/batch/job_canceller.js b/batch/job_canceller.js index 888a993c..de3f8510 100644 --- a/batch/job_canceller.js +++ b/batch/job_canceller.js @@ -15,7 +15,7 @@ JobCanceller.prototype.cancel = function (job, callback) { dbname: job.data.dbname, user: job.data.dbuser, pass: job.data.pass, - authenticated: true + authorizationLevel: 'master' }; doCancel(job.data.job_id, dbConfiguration, callback); diff --git a/batch/user_database_metadata_service.js b/batch/user_database_metadata_service.js index 9863670c..f121bcf9 100644 --- a/batch/user_database_metadata_service.js +++ b/batch/user_database_metadata_service.js @@ -30,8 +30,8 @@ UserDatabaseMetadataService.prototype.parseMetadataToDatabase = function (userDa dbopts.dbname = dbParams.dbname; dbopts.user = (!!dbParams.dbpublicuser) ? dbParams.dbpublicuser : global.settings.db_pubuser; - // batch is secure so it's going to be authenticated by default - dbopts.authenticated = true; + // batch is secure so it's going to be master by default + dbopts.authorizationLevel = 'master'; dbopts.user = _.template(global.settings.db_user, { user_id: dbParams.dbuser }); dbopts.pass = _.template(global.settings.db_user_pass, { diff --git a/test/unit/oauth.test.js b/test/unit/oauth.test.js index c3795326..b60db866 100644 --- a/test/unit/oauth.test.js +++ b/test/unit/oauth.test.js @@ -99,7 +99,7 @@ it('can return user for verified signature', function(done){ oAuth.verifyRequest(req, metadataBackend, function(err, data){ assert.ok(!err, err); - assert.equal(data, 1); + assert.equal(data, 'master'); done(); }); }); @@ -120,7 +120,7 @@ it('can return user for verified signature (for other allowed domains)', functio oAuth.verifyRequest(req, metadataBackend, function(err, data){ oAuth.getAllowedHosts = oAuthGetAllowedHostsFn; assert.ok(!err, err); - assert.equal(data, 1); + assert.equal(data, 'master'); done(); }); }); diff --git a/test/unit/pg-entities-access-validator.test.js b/test/unit/pg-entities-access-validator.test.js index d27f1aa0..80d15e59 100644 --- a/test/unit/pg-entities-access-validator.test.js +++ b/test/unit/pg-entities-access-validator.test.js @@ -97,56 +97,56 @@ describe('pg entities access validator with validatePGEntitiesAccess enabled', f }); it('validate function: should not be validated', function () { - let authenticated = true; + let authorizationLevel = 'master'; assert.strictEqual( - pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesCarto }, authenticated), + pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesCarto }, authorizationLevel), false ); assert.strictEqual( - pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesCartodbKO }, authenticated), + pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesCartodbKO }, authorizationLevel), false ); assert.strictEqual( - pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesPgcatalog }, authenticated), + pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesPgcatalog }, authorizationLevel), false ); assert.strictEqual( - pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesInfo }, authenticated), + pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesInfo }, authorizationLevel), false ); assert.strictEqual( - pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesPublicKO }, authenticated), + pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesPublicKO }, authorizationLevel), false ); assert.strictEqual( - pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesTopologyKO }, authenticated), + pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesTopologyKO }, authorizationLevel), false ); - authenticated = false; + authorizationLevel = 'regular'; assert.strictEqual( - pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesCarto }, authenticated), + pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesCarto }, authorizationLevel), false ); assert.strictEqual( - pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesCartodbKO }, authenticated), + pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesCartodbKO }, authorizationLevel), false ); assert.strictEqual( - pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesPgcatalog }, authenticated), + pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesPgcatalog }, authorizationLevel), false ); assert.strictEqual( - pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesInfo }, authenticated), + pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesInfo }, authorizationLevel), false ); assert.strictEqual( - pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesPublicKO }, authenticated), + pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesPublicKO }, authorizationLevel), false ); assert.strictEqual( - pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesTopologyKO }, authenticated), + pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesTopologyKO }, authorizationLevel), false ); }); From 06282b61fb8943896dccf0468e6f133834b62a5b Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Tue, 5 Jun 2018 13:22:43 +0200 Subject: [PATCH 24/30] refactor authenticatedRequest to masterRequest --- test/acceptance/query-tables-api-cache.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/acceptance/query-tables-api-cache.js b/test/acceptance/query-tables-api-cache.js index caf89bbc..8a9af9a1 100644 --- a/test/acceptance/query-tables-api-cache.js +++ b/test/acceptance/query-tables-api-cache.js @@ -74,7 +74,7 @@ describe('query-tables-api', function() { }); it('should skip cache to retrieve affected tables', function(done) { - var authenticatedRequest = { + var masterRequest = { url: '/api/v1/sql?' + qs.stringify({ q: 'SELECT * FROM untitle_table_4', api_key: '1234' @@ -84,7 +84,7 @@ describe('query-tables-api', function() { }, method: 'GET' }; - assert.response(server, authenticatedRequest, RESPONSE_OK, function(err) { + assert.response(server, masterRequest, RESPONSE_OK, function(err) { assert.ok(!err, err); getCacheStatus(function(err, cacheStatus) { From 03e484c5da542e4ff0e4fbc9d62c4f829372c9c1 Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Tue, 5 Jun 2018 15:49:15 +0200 Subject: [PATCH 25/30] FIX tests. Create jobs should return 403 if auth fails --- test/acceptance/auth-api.js | 80 ++++++++++++---------- test/integration/batch/job_service.test.js | 2 +- test/support/batch-test-client.js | 7 +- 3 files changed, 51 insertions(+), 38 deletions(-) diff --git a/test/acceptance/auth-api.js b/test/acceptance/auth-api.js index b4eed5ba..a0b8ece7 100644 --- a/test/acceptance/auth-api.js +++ b/test/acceptance/auth-api.js @@ -92,8 +92,8 @@ describe('Auth API', function () { }); describe('Batch API', function () { - it('should create a job with regular api key and get it done', function (done) { - this.testClient = new BatchTestClient({ apiKey: 'regular1' }); + it('should create a job with master api key and get it done', function (done) { + this.testClient = new BatchTestClient({ apiKey: '1234' }); this.testClient.createJob({ query: scopedSQL }, (err, jobResult) => { if (err) { @@ -115,21 +115,42 @@ describe('Auth API', function () { it('should create a job with regular api key and get it failed', function (done) { this.testClient = new BatchTestClient({ apiKey: 'regular1' }); - this.testClient.createJob({ query: privateSQL }, (err, jobResult) => { + this.testClient.createJob({ query: privateSQL }, { response: 403 }, (err, response) => { if (err) { return done(err); } - jobResult.getStatus(function (err, job) { - if (err) { - return done(err); - } + const body = JSON.parse(response.body); + assert.equal(body.error, 'permission denied'); + done(); + }); + }); - assert.equal(job.status, JobStatus.FAILED); - assert.equal(job.failed_reason, 'permission denied for relation private_table'); + it('should create a job with default public api key and get it failed', function (done) { + this.testClient = new BatchTestClient({ apiKey: 'default_public' }); - done(); - }); + this.testClient.createJob({ query: publicSQL }, { response: 403 }, (err, response) => { + if (err) { + return done(err); + } + + const body = JSON.parse(response.body); + assert.equal(body.error, 'permission denied'); + done(); + }); + }); + + it('should create a job with fallback default public api key and get it failed', function (done) { + this.testClient = new BatchTestClient(); + + this.testClient.createJob({ query: publicSQL }, { response: 403, anonymous: true }, (err, response) => { + if (err) { + return done(err); + } + + const body = JSON.parse(response.body); + assert.equal(body.error, 'permission denied'); + done(); }); }); @@ -216,44 +237,31 @@ describe('Auth API', function () { }); describe('Batch API', function () { - it('should create a job with regular api key and get it done', function (done) { - this.testClient = new BatchTestClient({ authorization: 'vizzuality:regular1' }); + it('should create a job with regular api key and get it failed', function (done) { + this.testClient = new BatchTestClient({ authorization: 'vizzuality:regular1', response: 403 }); - this.testClient.createJob({ query: scopedSQL }, { anonymous: true }, (err, jobResult) => { + this.testClient.createJob({ query: scopedSQL }, { anonymous: true }, (err, response) => { if (err) { return done(err); } - jobResult.getStatus(function (err, job) { - if (err) { - return done(err); - } - - assert.equal(job.status, JobStatus.DONE); - - done(); - }); + const body = JSON.parse(response.body); + assert.equal(body.error, 'permission denied'); + done(); }); }); - it('should create a job with regular api key and get it failed', function (done) { - this.testClient = new BatchTestClient({ authorization: 'vizzuality:regular1' }); + it('should create a job with default api key and get it failed', function (done) { + this.testClient = new BatchTestClient({ authorization: 'vizzuality:default_public', response: 403 }); - this.testClient.createJob({ query: privateSQL }, { anonymous: true }, (err, jobResult) => { + this.testClient.createJob({ query: privateSQL }, { anonymous: true }, (err, response) => { if (err) { return done(err); } - jobResult.getStatus(function (err, job) { - if (err) { - return done(err); - } - - assert.equal(job.status, JobStatus.FAILED); - assert.equal(job.failed_reason, 'permission denied for relation private_table'); - - done(); - }); + const body = JSON.parse(response.body); + assert.equal(body.error, 'permission denied'); + done(); }); }); diff --git a/test/integration/batch/job_service.test.js b/test/integration/batch/job_service.test.js index bafc240e..2b9290fe 100644 --- a/test/integration/batch/job_service.test.js +++ b/test/integration/batch/job_service.test.js @@ -74,7 +74,7 @@ function runQueryHelper(job, callback) { }); } -describe.only('job service', function() { +describe('job service', function() { var jobService = new JobService(jobBackend, jobCanceller); after(function (done) { diff --git a/test/support/batch-test-client.js b/test/support/batch-test-client.js index 71617f3b..99559ca2 100644 --- a/test/support/batch-test-client.js +++ b/test/support/batch-test-client.js @@ -80,7 +80,12 @@ BatchTestClient.prototype.createJob = function(job, override, callback) { if (err) { return callback(err); } - return callback(null, new JobResult(JSON.parse(res.body), this, override)); + + if (res.statusCode < 400) { + return callback(null, new JobResult(JSON.parse(res.body), this, override)); + } else { + return callback(null, res); + } }.bind(this) ); }; From 4993f8a956193d9985850b24a8bfb979a8d99acb Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Tue, 5 Jun 2018 17:56:29 +0200 Subject: [PATCH 26/30] use job configuration instead of user metadata service in test --- batch/job_canceller.js | 5 ++-- test/integration/batch/job_canceller.test.js | 28 ++++++++++---------- 2 files changed, 16 insertions(+), 17 deletions(-) diff --git a/batch/job_canceller.js b/batch/job_canceller.js index de3f8510..ca7abd6c 100644 --- a/batch/job_canceller.js +++ b/batch/job_canceller.js @@ -15,14 +15,13 @@ JobCanceller.prototype.cancel = function (job, callback) { dbname: job.data.dbname, user: job.data.dbuser, pass: job.data.pass, - authorizationLevel: 'master' }; doCancel(job.data.job_id, dbConfiguration, callback); }; -function doCancel(job_id, userDatabaseMetadata, callback) { - var pg = new PSQL(userDatabaseMetadata); +function doCancel(job_id, dbConfiguration, callback) { + var pg = new PSQL(dbConfiguration); getQueryPID(pg, job_id, function (err, pid) { if (err) { diff --git a/test/integration/batch/job_canceller.test.js b/test/integration/batch/job_canceller.test.js index 8ead991c..8d5e916e 100644 --- a/test/integration/batch/job_canceller.test.js +++ b/test/integration/batch/job_canceller.test.js @@ -19,7 +19,6 @@ var metadataBackend = require('cartodb-redis')({ pool: redisUtils.getPool() }); var jobPublisher = new JobPublisher(redisUtils.getPool()); var jobQueue = new JobQueue(metadataBackend, jobPublisher); var jobBackend = new JobBackend(metadataBackend, jobQueue); -var userDatabaseMetadataService = new UserDatabaseMetadataService(metadataBackend); var JobFactory = require(BATCH_SOURCE + 'models/job_factory'); var USER = 'vizzuality'; @@ -30,7 +29,6 @@ var HOST = 'localhost'; // in order to test query cancelation/draining function runQueryHelper(job, callback) { var job_id = job.job_id; - var user = job.user; var sql = job.query; job.status = jobStatus.RUNNING; @@ -40,22 +38,24 @@ function runQueryHelper(job, callback) { return callback(err); } - userDatabaseMetadataService.getUserMetadata(user, function (err, userDatabaseMetadata) { + const dbConfiguration = { + host: job.host, + port: job.port, + dbname: job.dbname, + user: job.dbuser, + pass: job.pass, + }; + + const pg = new PSQL(dbConfiguration); + + sql = '/* ' + job_id + ' */ ' + sql; + + pg.eventedQuery(sql, function (err, query) { if (err) { return callback(err); } - var pg = new PSQL(userDatabaseMetadata); - - sql = '/* ' + job_id + ' */ ' + sql; - - pg.eventedQuery(sql, function (err, query) { - if (err) { - return callback(err); - } - - callback(null, query); - }); + callback(null, query); }); }); } From 62df0a387affa50632503966a821e9018135ba62 Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Tue, 5 Jun 2018 17:58:55 +0200 Subject: [PATCH 27/30] remove functionality from parseMetadataToDatabase. DB user and pass not longer needed --- batch/user_database_metadata_service.js | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/batch/user_database_metadata_service.js b/batch/user_database_metadata_service.js index f121bcf9..6a083a5a 100644 --- a/batch/user_database_metadata_service.js +++ b/batch/user_database_metadata_service.js @@ -1,7 +1,5 @@ 'use strict'; -var _ = require('underscore'); - function UserDatabaseMetadataService(metadataBackend) { this.metadataBackend = metadataBackend; } @@ -18,26 +16,14 @@ UserDatabaseMetadataService.prototype.getUserMetadata = function (username, call }); }; -//TODO AUTH is (all of) this necessary? UserDatabaseMetadataService.prototype.parseMetadataToDatabase = function (userDatabaseMetadata) { var dbParams = userDatabaseMetadata; var dbopts = {}; - dbopts.pass = dbParams.dbpass || global.settings.db_pubuser_pass; dbopts.port = dbParams.dbport || global.settings.db_batch_port || global.settings.db_port; dbopts.host = dbParams.dbhost; dbopts.dbname = dbParams.dbname; - dbopts.user = (!!dbParams.dbpublicuser) ? dbParams.dbpublicuser : global.settings.db_pubuser; - - // batch is secure so it's going to be master by default - dbopts.authorizationLevel = 'master'; - dbopts.user = _.template(global.settings.db_user, { user_id: dbParams.dbuser }); - - dbopts.pass = _.template(global.settings.db_user_pass, { - user_id: dbParams.dbuser, - user_password: dbParams.dbpass - }); return dbopts; }; From 3891d93b8d59c4c26ee93ca20a778a9a053a9622 Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Tue, 5 Jun 2018 17:59:46 +0200 Subject: [PATCH 28/30] remove unnecessary require --- test/integration/batch/job_canceller.test.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/integration/batch/job_canceller.test.js b/test/integration/batch/job_canceller.test.js index 8d5e916e..c9ad89fa 100644 --- a/test/integration/batch/job_canceller.test.js +++ b/test/integration/batch/job_canceller.test.js @@ -11,7 +11,6 @@ var JobQueue = require(BATCH_SOURCE + 'job_queue'); var JobBackend = require(BATCH_SOURCE + 'job_backend'); var JobPublisher = require(BATCH_SOURCE + 'pubsub/job-publisher'); var jobStatus = require(BATCH_SOURCE + 'job_status'); -var UserDatabaseMetadataService = require(BATCH_SOURCE + 'user_database_metadata_service'); var JobCanceller = require(BATCH_SOURCE + 'job_canceller'); var PSQL = require('cartodb-psql'); From 340e55ea468e40960edf32915e760324cf7feeb1 Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Wed, 6 Jun 2018 15:23:53 +0200 Subject: [PATCH 29/30] reduce error info --- app/auth/apikey.js | 2 +- batch/query_runner.js | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/app/auth/apikey.js b/app/auth/apikey.js index 94a362f3..a4534432 100644 --- a/app/auth/apikey.js +++ b/app/auth/apikey.js @@ -18,7 +18,7 @@ ApikeyAuth.prototype.verifyCredentials = function (callback) { this.metadataBackend.getApikey(this.username, this.apikeyToken, (err, apikey) => { if (err) { err.http_status = 500; - err.message = 'Unexpected error fetching from Redis'; + err.message = 'Unexpected error'; return callback(err); } diff --git a/batch/query_runner.js b/batch/query_runner.js index 7dd3001b..e82c55d8 100644 --- a/batch/query_runner.js +++ b/batch/query_runner.js @@ -19,9 +19,6 @@ QueryRunner.prototype.run = function (job_id, sql, user, timeout, dbparams, call } const dbConfigurationError = new Error('Batch Job DB misconfiguration'); - dbConfigurationError.type = 'batch-job'; - dbConfigurationError.subtype = 'job-db-conf-error'; - dbConfigurationError.http_status = 400; return callback(dbConfigurationError); }; From 440ba8c840395fda1e15e639735081f97ec00044 Mon Sep 17 00:00:00 2001 From: Eneko Lakasta Date: Wed, 6 Jun 2018 15:48:22 +0200 Subject: [PATCH 30/30] make function checkUserExists node callback pattern compilant --- app/middlewares/user.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/middlewares/user.js b/app/middlewares/user.js index 2e2ed175..a22edb82 100644 --- a/app/middlewares/user.js +++ b/app/middlewares/user.js @@ -6,10 +6,8 @@ module.exports = function user(metadataBackend) { return function userMiddleware (req, res, next) { res.locals.user = getUserNameFromRequest(req, cdbRequest); - checkUserExists(metadataBackend, res.locals.user, function(userExists) { - if (userExists) { - return next(); - } else { + checkUserExists(metadataBackend, res.locals.user, function(err, userExists) { + if (err || !userExists) { const error = new Error('Unauthorized'); error.type = 'auth'; error.subtype = 'user-not-found'; @@ -17,6 +15,8 @@ module.exports = function user(metadataBackend) { error.message = errorUserNotFoundMessageTemplate(res.locals.user); next(error); } + + return next(); }); }; }; @@ -27,7 +27,7 @@ function getUserNameFromRequest(req, cdbRequest) { function checkUserExists(metadataBackend, userName, callback) { metadataBackend.getUserId(userName, function(err) { - callback(!err); + callback(err, !err); }); }