From 5804a44982d94bf881c3cb86a0b869798ca24c76 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 4 Jul 2018 12:45:21 +0200 Subject: [PATCH 1/9] Avoid singletons --- app/controllers/query_controller.js | 3 +- app/services/pg-entities-access-validator.js | 10 +++---- .../unit/pg-entities-access-validator.test.js | 30 ++++++++++--------- 3 files changed, 22 insertions(+), 21 deletions(-) diff --git a/app/controllers/query_controller.js b/app/controllers/query_controller.js index 3c959264a..e3bc8c692 100644 --- a/app/controllers/query_controller.js +++ b/app/controllers/query_controller.js @@ -5,7 +5,8 @@ var step = require('step'); var assert = require('assert'); var PSQL = require('cartodb-psql'); var CachedQueryTables = require('../services/cached-query-tables'); -const pgEntitiesAccessValidator = require('../services/pg-entities-access-validator'); +const PGEntitiesAccessValidator = require('../services/pg-entities-access-validator'); +const pgEntitiesAccessValidator = new PGEntitiesAccessValidator(); var queryMayWrite = require('../utils/query_may_write'); var formats = require('../models/formats'); diff --git a/app/services/pg-entities-access-validator.js b/app/services/pg-entities-access-validator.js index e2d902623..80291529e 100644 --- a/app/services/pg-entities-access-validator.js +++ b/app/services/pg-entities-access-validator.js @@ -14,7 +14,7 @@ const FORBIDDEN_ENTITIES = { ] }; -const Validator = { +module.exports = class Validator { validate(affectedTables, authorizationLevel) { let hardValidationResult = true; let softValidationResult = true; @@ -30,7 +30,7 @@ const Validator = { } return hardValidationResult && softValidationResult; - }, + } hardValidation(tables) { for (let table of tables) { @@ -45,7 +45,7 @@ const Validator = { } return true; - }, + } softValidation(tables) { for (let table of tables) { @@ -56,6 +56,4 @@ const Validator = { return true; } -}; - -module.exports = Validator; +}; diff --git a/test/unit/pg-entities-access-validator.test.js b/test/unit/pg-entities-access-validator.test.js index 80d15e597..da306c895 100644 --- a/test/unit/pg-entities-access-validator.test.js +++ b/test/unit/pg-entities-access-validator.test.js @@ -1,5 +1,5 @@ const assert = require('assert'); -const pgEntitiesAccessValidator = require('../../app/services/pg-entities-access-validator'); +const PGEntitiesAccessValidator = require('../../app/services/pg-entities-access-validator'); const fakeAffectedTables = [{ schema_name: 'schema', @@ -72,6 +72,8 @@ const fakeAffectedTablesTopologyKO = [ describe('pg entities access validator with validatePGEntitiesAccess enabled', function () { + const pgEntitiesAccessValidator = new PGEntitiesAccessValidator(); + before(function() { global.settings.validatePGEntitiesAccess = true; }); @@ -99,54 +101,54 @@ describe('pg entities access validator with validatePGEntitiesAccess enabled', f it('validate function: should not be validated', function () { let authorizationLevel = 'master'; assert.strictEqual( - pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesCarto }, authorizationLevel), + pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesCarto }, authorizationLevel), false ); assert.strictEqual( - pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesCartodbKO }, authorizationLevel), + pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesCartodbKO }, authorizationLevel), false ); assert.strictEqual( - pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesPgcatalog }, authorizationLevel), + pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesPgcatalog }, authorizationLevel), false ); assert.strictEqual( - pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesInfo }, authorizationLevel), + pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesInfo }, authorizationLevel), false ); assert.strictEqual( - pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesPublicKO }, authorizationLevel), + pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesPublicKO }, authorizationLevel), false ); assert.strictEqual( - pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesTopologyKO }, authorizationLevel), + pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesTopologyKO }, authorizationLevel), false ); - + authorizationLevel = 'regular'; assert.strictEqual( - pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesCarto }, authorizationLevel), + pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesCarto }, authorizationLevel), false ); assert.strictEqual( - pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesCartodbKO }, authorizationLevel), + pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesCartodbKO }, authorizationLevel), false ); assert.strictEqual( - pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesPgcatalog }, authorizationLevel), + pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesPgcatalog }, authorizationLevel), false ); assert.strictEqual( - pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesInfo }, authorizationLevel), + pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesInfo }, authorizationLevel), false ); assert.strictEqual( - pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesPublicKO }, authorizationLevel), + pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesPublicKO }, authorizationLevel), false ); assert.strictEqual( - pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesTopologyKO }, authorizationLevel), + pgEntitiesAccessValidator.validate({ tables: fakeAffectedTablesTopologyKO }, authorizationLevel), false ); }); From 2850c35bd744705c2d1eaea6ffb150902062e3b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 4 Jul 2018 12:46:22 +0200 Subject: [PATCH 2/9] Use private method notation --- app/services/pg-entities-access-validator.js | 8 ++--- .../unit/pg-entities-access-validator.test.js | 30 +++++++++---------- 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/app/services/pg-entities-access-validator.js b/app/services/pg-entities-access-validator.js index 80291529e..67b5a81d0 100644 --- a/app/services/pg-entities-access-validator.js +++ b/app/services/pg-entities-access-validator.js @@ -21,18 +21,18 @@ module.exports = class Validator { if (!!affectedTables && affectedTables.tables) { if (global.settings.validatePGEntitiesAccess) { - hardValidationResult = this.hardValidation(affectedTables.tables); + hardValidationResult = this._hardValidation(affectedTables.tables); } if (authorizationLevel !== 'master') { - softValidationResult = this.softValidation(affectedTables.tables); + softValidationResult = this._softValidation(affectedTables.tables); } } return hardValidationResult && softValidationResult; } - hardValidation(tables) { + _hardValidation(tables) { for (let table of tables) { if (FORBIDDEN_ENTITIES[table.schema_name] && FORBIDDEN_ENTITIES[table.schema_name].length && ( @@ -47,7 +47,7 @@ module.exports = class Validator { return true; } - softValidation(tables) { + _softValidation(tables) { for (let table of tables) { if (table.table_name.match(/\bpg_/)) { return false; diff --git a/test/unit/pg-entities-access-validator.test.js b/test/unit/pg-entities-access-validator.test.js index da306c895..8934d16e6 100644 --- a/test/unit/pg-entities-access-validator.test.js +++ b/test/unit/pg-entities-access-validator.test.js @@ -153,23 +153,23 @@ describe('pg entities access validator with validatePGEntitiesAccess enabled', f ); }); - it('hardValidation function', function () { - assert.strictEqual(pgEntitiesAccessValidator.hardValidation(fakeAffectedTables), true); - assert.strictEqual(pgEntitiesAccessValidator.hardValidation(fakeAffectedTablesCartodbOK), true); - assert.strictEqual(pgEntitiesAccessValidator.hardValidation(fakeAffectedTablesPublicOK), true); - assert.strictEqual(pgEntitiesAccessValidator.hardValidation(fakeAffectedTablesTopologyOK), true); - - assert.strictEqual(pgEntitiesAccessValidator.hardValidation(fakeAffectedTablesCarto), false); - assert.strictEqual(pgEntitiesAccessValidator.hardValidation(fakeAffectedTablesCartodbKO), false); - assert.strictEqual(pgEntitiesAccessValidator.hardValidation(fakeAffectedTablesPgcatalog), false); - assert.strictEqual(pgEntitiesAccessValidator.hardValidation(fakeAffectedTablesInfo), false); - assert.strictEqual(pgEntitiesAccessValidator.hardValidation(fakeAffectedTablesPublicKO), false); - assert.strictEqual(pgEntitiesAccessValidator.hardValidation(fakeAffectedTablesTopologyKO), false); + it('_hardValidation function', function () { + assert.strictEqual(pgEntitiesAccessValidator._hardValidation(fakeAffectedTables), true); + assert.strictEqual(pgEntitiesAccessValidator._hardValidation(fakeAffectedTablesCartodbOK), true); + assert.strictEqual(pgEntitiesAccessValidator._hardValidation(fakeAffectedTablesPublicOK), true); + assert.strictEqual(pgEntitiesAccessValidator._hardValidation(fakeAffectedTablesTopologyOK), true); + + assert.strictEqual(pgEntitiesAccessValidator._hardValidation(fakeAffectedTablesCarto), false); + assert.strictEqual(pgEntitiesAccessValidator._hardValidation(fakeAffectedTablesCartodbKO), false); + assert.strictEqual(pgEntitiesAccessValidator._hardValidation(fakeAffectedTablesPgcatalog), false); + assert.strictEqual(pgEntitiesAccessValidator._hardValidation(fakeAffectedTablesInfo), false); + assert.strictEqual(pgEntitiesAccessValidator._hardValidation(fakeAffectedTablesPublicKO), false); + assert.strictEqual(pgEntitiesAccessValidator._hardValidation(fakeAffectedTablesTopologyKO), false); }); - it('softValidation function', function () { - assert.strictEqual(pgEntitiesAccessValidator.softValidation(fakeAffectedTablesCartodbKO), true); - assert.strictEqual(pgEntitiesAccessValidator.softValidation(fakeAffectedTablesPgcatalog), false); + it('_softValidation function', function () { + assert.strictEqual(pgEntitiesAccessValidator._softValidation(fakeAffectedTablesCartodbKO), true); + assert.strictEqual(pgEntitiesAccessValidator._softValidation(fakeAffectedTablesPgcatalog), false); }); }); From 9fd8fe2b8bfa314d1b30a4f35533a93f51480ba7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 4 Jul 2018 12:57:51 +0200 Subject: [PATCH 3/9] Extract condition --- app/services/pg-entities-access-validator.js | 21 +++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/app/services/pg-entities-access-validator.js b/app/services/pg-entities-access-validator.js index 67b5a81d0..9a33cb50a 100644 --- a/app/services/pg-entities-access-validator.js +++ b/app/services/pg-entities-access-validator.js @@ -14,6 +14,18 @@ const FORBIDDEN_ENTITIES = { ] }; +function isForbiddenEntity (table) { + return FORBIDDEN_ENTITIES[table.schema_name] && + FORBIDDEN_ENTITIES[table.schema_name].length && ( + FORBIDDEN_ENTITIES[table.schema_name][0] === '*' || + FORBIDDEN_ENTITIES[table.schema_name].includes(table.table_name) + ); +} + +function isSystemEntity (table) { + return table.table_name.match(/\bpg_/; +} + module.exports = class Validator { validate(affectedTables, authorizationLevel) { let hardValidationResult = true; @@ -34,12 +46,7 @@ module.exports = class Validator { _hardValidation(tables) { for (let table of tables) { - if (FORBIDDEN_ENTITIES[table.schema_name] && FORBIDDEN_ENTITIES[table.schema_name].length && - ( - FORBIDDEN_ENTITIES[table.schema_name][0] === '*' || - FORBIDDEN_ENTITIES[table.schema_name].includes(table.table_name) - ) - ) { + if (isForbiddenEntity(table)) { return false; } } @@ -49,7 +56,7 @@ module.exports = class Validator { _softValidation(tables) { for (let table of tables) { - if (table.table_name.match(/\bpg_/)) { + if (isSystemEntity(table)) { return false; } } From b01eb2cf648fda31d922deb2264414a0256dda6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 4 Jul 2018 13:16:50 +0200 Subject: [PATCH 4/9] Improve readability --- app/services/pg-entities-access-validator.js | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/app/services/pg-entities-access-validator.js b/app/services/pg-entities-access-validator.js index 9a33cb50a..2940929f1 100644 --- a/app/services/pg-entities-access-validator.js +++ b/app/services/pg-entities-access-validator.js @@ -14,12 +14,11 @@ const FORBIDDEN_ENTITIES = { ] }; -function isForbiddenEntity (table) { - return FORBIDDEN_ENTITIES[table.schema_name] && - FORBIDDEN_ENTITIES[table.schema_name].length && ( - FORBIDDEN_ENTITIES[table.schema_name][0] === '*' || - FORBIDDEN_ENTITIES[table.schema_name].includes(table.table_name) - ); +function isForbiddenEntity (entity) { + const { schema_name: schema, table_name: table } = entity; + const forbiddenEntities = FORBIDDEN_ENTITIES[schema]; + + return forbiddenEntities && forbiddenEntities.length && (forbiddenEntities[0] === '*' || forbiddenEntities.includes(table)); } function isSystemEntity (table) { From e4a07a46a806df4c8719f24f5a7898e8aa6cedda Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 4 Jul 2018 13:24:27 +0200 Subject: [PATCH 5/9] Remove unnecessary condition --- app/services/pg-entities-access-validator.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/services/pg-entities-access-validator.js b/app/services/pg-entities-access-validator.js index 2940929f1..e93b4da0e 100644 --- a/app/services/pg-entities-access-validator.js +++ b/app/services/pg-entities-access-validator.js @@ -18,11 +18,11 @@ function isForbiddenEntity (entity) { const { schema_name: schema, table_name: table } = entity; const forbiddenEntities = FORBIDDEN_ENTITIES[schema]; - return forbiddenEntities && forbiddenEntities.length && (forbiddenEntities[0] === '*' || forbiddenEntities.includes(table)); + return forbiddenEntities && (forbiddenEntities[0] === '*' || forbiddenEntities.includes(table)); } function isSystemEntity (table) { - return table.table_name.match(/\bpg_/; + return table.table_name.match(/\bpg_/); } module.exports = class Validator { From 615e1ce2bcb5016dc27ffeefc4ae7dae81d081ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 4 Jul 2018 13:25:12 +0200 Subject: [PATCH 6/9] Improve readability --- app/services/pg-entities-access-validator.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/app/services/pg-entities-access-validator.js b/app/services/pg-entities-access-validator.js index e93b4da0e..0c1438af4 100644 --- a/app/services/pg-entities-access-validator.js +++ b/app/services/pg-entities-access-validator.js @@ -21,8 +21,9 @@ function isForbiddenEntity (entity) { return forbiddenEntities && (forbiddenEntities[0] === '*' || forbiddenEntities.includes(table)); } -function isSystemEntity (table) { - return table.table_name.match(/\bpg_/); +function isSystemEntity (entity) { + const { table_name: table } = entity; + return table.match(/\bpg_/); } module.exports = class Validator { From 6b64eb8f84f8dd8ad7d3a116c93346094e2d78af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 4 Jul 2018 13:25:52 +0200 Subject: [PATCH 7/9] Rename class --- app/services/pg-entities-access-validator.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/pg-entities-access-validator.js b/app/services/pg-entities-access-validator.js index 0c1438af4..874e016f9 100644 --- a/app/services/pg-entities-access-validator.js +++ b/app/services/pg-entities-access-validator.js @@ -26,7 +26,7 @@ function isSystemEntity (entity) { return table.match(/\bpg_/); } -module.exports = class Validator { +module.exports = class PGEntitiesAccessValidator { validate(affectedTables, authorizationLevel) { let hardValidationResult = true; let softValidationResult = true; From 0bd0f8574e6a4b69fe7028017f19eb0d2fd7cb37 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Wed, 4 Jul 2018 15:04:40 +0200 Subject: [PATCH 8/9] Extract conditions --- app/services/pg-entities-access-validator.js | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/app/services/pg-entities-access-validator.js b/app/services/pg-entities-access-validator.js index 874e016f9..521d9fc59 100644 --- a/app/services/pg-entities-access-validator.js +++ b/app/services/pg-entities-access-validator.js @@ -14,11 +14,18 @@ const FORBIDDEN_ENTITIES = { ] }; +function isForbiddenTable (schema, table) { + return FORBIDDEN_ENTITIES[schema] && FORBIDDEN_ENTITIES[schema].includes(table); +} + +function isForbiddenSchema (schema) { + return FORBIDDEN_ENTITIES[schema] && FORBIDDEN_ENTITIES[schema][0] === '*'; +} + function isForbiddenEntity (entity) { const { schema_name: schema, table_name: table } = entity; - const forbiddenEntities = FORBIDDEN_ENTITIES[schema]; - return forbiddenEntities && (forbiddenEntities[0] === '*' || forbiddenEntities.includes(table)); + return isForbiddenSchema(schema) || isForbiddenTable(schema, table); } function isSystemEntity (entity) { From fcf72d340fcb1ff7afcf9a3120a77e9e07ffe9a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?= Date: Thu, 5 Jul 2018 12:15:26 +0200 Subject: [PATCH 9/9] Add tests --- test/acceptance/export/involved-tables.js | 101 ++++++++++++++++++++++ 1 file changed, 101 insertions(+) create mode 100644 test/acceptance/export/involved-tables.js diff --git a/test/acceptance/export/involved-tables.js b/test/acceptance/export/involved-tables.js new file mode 100644 index 000000000..f72489024 --- /dev/null +++ b/test/acceptance/export/involved-tables.js @@ -0,0 +1,101 @@ +require('../../helper'); + +const server = require('../../../app/server')(); +const assert = require('../../support/assert'); +const querystring = require('querystring'); + +describe('Read restriction on "geometry_columns"', function() { + describe('OGR formats', function () { + it('GET /api/v1/sql downloads KML file from a private table', function (done){ + const query = querystring.stringify({ + api_key: 1234, + q: "SELECT * FROM private_table LIMIT 1", + format: "kml", + filename: 'private_table' + }); + assert.response(server, { + url: '/api/v1/sql?' + query, + headers: { host: 'vizzuality.cartodb.com' }, + method: 'GET' + },{ }, function(err, res){ + assert.equal(res.statusCode, 200, res.body); + + const cd = res.headers['content-disposition']; + + assert.ok(/filename=private_table.kml/gi.test(cd), cd); + assert.equal(res.headers['content-type'], 'application/kml; charset=utf-8'); + + done(); + }); + }); + + it('GET /api/v1/sql downloads a KML file from a public table', function (done){ + const query = querystring.stringify({ + q: "SELECT * FROM populated_places_simple_reduced LIMIT 1", + format: "kml", + filename: 'populated_places_simple_reduced' + }); + assert.response(server, { + url: '/api/v1/sql?' + query, + headers: { host: 'vizzuality.cartodb.com' }, + method: 'GET' + },{ }, function(err, res){ + assert.equal(res.statusCode, 200, res.body); + + const cd = res.headers['content-disposition']; + + assert.ok(/filename=populated_places_simple_reduced.kml/gi.test(cd), cd); + assert.equal(res.headers['content-type'], 'application/kml; charset=utf-8'); + + done(); + }); + }); + + it('GET /api/v1/sql downloads a KML file from "geometry_columns" view', function (done){ + const query = querystring.stringify({ + q: "SELECT * FROM geometry_columns LIMIT 1", + format: "kml", + filename: 'geometry_columns' + }); + + assert.response(server, { + url: '/api/v1/sql?' + query, + headers: { host: 'vizzuality.cartodb.com' }, + method: 'GET' + },{ }, function(err, res){ + assert.equal(res.statusCode, 200, res.body); + + const cd = res.headers['content-disposition']; + + assert.ok(/filename=geometry_columns.kml/gi.test(cd), cd); + assert.equal(res.headers['content-type'], 'application/kml; charset=utf-8'); + + done(); + }); + }); + + it('GET /api/v1/sql downloads a KML file with master api_key from "geometry_columns" view', function (done){ + const query = querystring.stringify({ + api_key: 1234, + q: "SELECT * FROM geometry_columns LIMIT 1", + format: "kml", + filename: 'geometry_columns' + }); + + assert.response(server, { + url: '/api/v1/sql?' + query, + headers: { host: 'vizzuality.cartodb.com' }, + method: 'GET' + },{ }, function(err, res){ + assert.equal(res.statusCode, 200, res.body); + + const cd = res.headers['content-disposition']; + + assert.ok(/filename=geometry_columns.kml/gi.test(cd), cd); + assert.equal(res.headers['content-type'], 'application/kml; charset=utf-8'); + + done(); + }); + }); + }); +});