From 6d46a21005a73f8cbb067f349a5197fc306fadf8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Garc=C3=ADa=20Aubert?=
 <danielgarciaaubert@gmail.com>
Date: Tue, 12 Dec 2017 19:23:21 +0100
Subject: [PATCH] Validate aggregation query param

---
 .../adapter/aggregation-mapconfig-adapter.js  | 18 +++++--
 test/acceptance/aggregation.js                | 49 +++++++++++++++++--
 test/support/test-client.js                   |  4 +-
 3 files changed, 61 insertions(+), 10 deletions(-)

diff --git a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js
index 440b21a2..8df069c0 100644
--- a/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js
+++ b/lib/cartodb/models/mapconfig/adapter/aggregation-mapconfig-adapter.js
@@ -7,6 +7,9 @@ const MISSING_AGGREGATION_COLUMNS = 'Missing columns in the aggregation. The map
 const unsupportedGeometryTypeErrorMessage = ctx =>
 `Unsupported geometry type: ${ctx.geometryType}. Aggregation is available only for geometry type: ST_Point`;
 
+const invalidAggregationParamValueErrorMessage = ctx =>
+`Invalid value for 'aggregation' query param: ${ctx.value}. Valid ones are 'true' or 'false'`;
+
 module.exports = class AggregationMapConfigAdapter {
     constructor (pgConnection) {
         this.pgConnection = pgConnection;
@@ -15,6 +18,10 @@ module.exports = class AggregationMapConfigAdapter {
     getMapConfig (user, requestMapConfig, params, context, callback) {
         const mapConfig = new MapConfig(requestMapConfig);
 
+        if (!this._isValidAggregationParam(params)) {
+            return callback(new Error(invalidAggregationParamValueErrorMessage({ value: params.aggregation })));
+        }
+
         if (!this._shouldAdapt(mapConfig, params)) {
             return callback(null, requestMapConfig);
         }
@@ -36,6 +43,11 @@ module.exports = class AggregationMapConfigAdapter {
         });
     }
 
+    _isValidAggregationParam (params) {
+        const { aggregation } = params;
+        return aggregation === undefined || aggregation === 'true' || aggregation === 'false';
+    }
+
     _hasMissingColumns (mapConfig) {
         const layers = mapConfig.getLayers();
 
@@ -77,11 +89,7 @@ module.exports = class AggregationMapConfigAdapter {
             return false;
         }
 
-        if (aggregation === 'true') {
-            return true;
-        }
-
-        if (mapConfig.isAggregationMapConfig()) {
+        if (aggregation === 'true' || mapConfig.isAggregationMapConfig()) {
             return true;
         }
 
diff --git a/test/acceptance/aggregation.js b/test/acceptance/aggregation.js
index 9c1fed45..56815ccc 100644
--- a/test/acceptance/aggregation.js
+++ b/test/acceptance/aggregation.js
@@ -291,7 +291,8 @@ describe('aggregation', function () {
                                         aggregate_function: 'sum',
                                         aggregated_column: 'value'
                                     }
-                                }
+                                },
+                                threshold: 1
                             }
                         }
                     }
@@ -308,7 +309,49 @@ describe('aggregation', function () {
                     assert.equal(typeof body.metadata, 'object');
                     assert.ok(Array.isArray(body.metadata.layers));
 
-                    body.metadata.layers.forEach(layer => assert.ok(layer.meta.aggregation === undefined));
+                    body.metadata.layers.forEach(layer => assert.equal(layer.meta.aggregation, undefined));
+
+                    done();
+                });
+            });
+
+            it('when the aggregation param is not valid should respond with error', function (done) {
+                const mapConfig = createVectorMapConfig([
+                    {
+                        type: 'cartodb',
+                        options: {
+                            sql: POINTS_SQL_1,
+                            aggregation: {
+                                threshold: 1
+                            }
+                        }
+                    }
+                ]);
+
+                this.testClient = new TestClient(mapConfig);
+                const options = {
+                    response: {
+                        status: 400
+                    },
+                    aggregation: 'wadus'
+                };
+
+                this.testClient.getLayergroup(options, (err, body) => {
+                    if (err) {
+                        return done(err);
+                    }
+
+                    assert.deepEqual(body, {
+                        errors: [
+                            "Invalid value for 'aggregation' query param: wadus." +
+                                " Valid ones are 'true' or 'false'"
+                        ],
+                        errors_with_context:[{
+                            type: 'unknown',
+                            message: "Invalid value for 'aggregation' query param: wadus." +
+                                " Valid ones are 'true' or 'false'"
+                        }]
+                    });
 
                     done();
                 });
@@ -352,7 +395,7 @@ describe('aggregation', function () {
                 });
             });
 
-            it('when the layer\'s geometry type is not point should responds with error', function (done) {
+            it('when the layer\'s geometry type is not point should respond with error', function (done) {
                 const mapConfig = createVectorMapConfig([
                     {
                         type: 'cartodb',
diff --git a/test/support/test-client.js b/test/support/test-client.js
index 54581f99..f4437a39 100644
--- a/test/support/test-client.js
+++ b/test/support/test-client.js
@@ -625,7 +625,7 @@ TestClient.prototype.getTile = function(z, x, y, params, callback) {
                 queryParams.api_key = self.apiKey;
             }
 
-            if (typeof params.aggregation === 'boolean') {
+            if (params.aggregation !== undefined) {
                 queryParams.aggregation = params.aggregation;
             }
 
@@ -801,7 +801,7 @@ TestClient.prototype.getLayergroup = function (params, callback) {
         queryParams.api_key = self.apiKey;
     }
 
-    if (typeof params.aggregation === 'boolean') {
+    if (params.aggregation !== undefined) {
         queryParams.aggregation = params.aggregation;
     }