From bc82aa6e77e0fb7ae9e34321c97513fbedd9cf66 Mon Sep 17 00:00:00 2001
From: ivaosthu <ivar.conradi.osthus@finn.no>
Date: Mon, 12 Dec 2016 21:44:21 +0100
Subject: [PATCH] Add schema validation for strategies

---
 lib/routes/strategy-schema.js | 20 +++++++++++++++++
 lib/routes/strategy.js        | 42 ++++++++++++++++++++---------------
 migrator.js                   |  1 +
 test/e2e/strategy-api.test.js |  4 ++--
 4 files changed, 47 insertions(+), 20 deletions(-)
 create mode 100644 lib/routes/strategy-schema.js

diff --git a/lib/routes/strategy-schema.js b/lib/routes/strategy-schema.js
new file mode 100644
index 0000000000..9241c79737
--- /dev/null
+++ b/lib/routes/strategy-schema.js
@@ -0,0 +1,20 @@
+'use strict';
+
+const joi = require('joi');
+
+const strategySchema = joi.object().keys({
+    name: joi.string()
+        .regex(/^[a-zA-Z0-9\\.\\-]{3,30}$/)
+        .required(),
+    description: joi.string(),
+    parameters: joi.array()
+        .required()
+        .items(joi.object().keys({
+            name: joi.string().required(),
+            type: joi.string().required(),
+            description: joi.string(),
+            required: joi.boolean(),
+        })),
+});
+
+module.exports = strategySchema;
diff --git a/lib/routes/strategy.js b/lib/routes/strategy.js
index cad1d5ccd7..9647a8adbb 100644
--- a/lib/routes/strategy.js
+++ b/lib/routes/strategy.js
@@ -1,29 +1,28 @@
 'use strict';
 
+const joi = require('joi');
 const eventType = require('../event-type');
 const logger = require('../logger');
 const NameExistsError = require('../error/name-exists-error');
-const ValidationError = require('../error/validation-error.js');
-const NotFoundError = require('../error/notfound-error');
-const validateRequest = require('../error/validate-request');
 const extractUser = require('../extract-user');
+const strategySchema = require('./strategy-schema');
 const version = 1;
 
 const handleError = (req, res, error) => {
-    switch (error.constructor) {
-        case NotFoundError:
+    switch (error.name) {
+        case 'NotFoundError':
             return res
                 .status(404)
                 .end();
-        case NameExistsError:
+        case 'NameExistsError':
             return res
                 .status(403)
                 .json([{ msg: `A strategy named '${req.body.name}' already exists.` }])
                 .end();
-        case ValidationError:
+        case 'ValidationError':
             return res
                 .status(400)
-                .json(req.validationErrors())
+                .json(error)
                 .end();
         default:
             logger.error('Could perfom operation', error);
@@ -64,14 +63,10 @@ module.exports = function (app, config) {
     });
 
     app.post('/strategies', (req, res) => {
-        req.checkBody('name', 'Name is required').notEmpty();
-        req.checkBody('name', 'Name must match format ^[0-9a-zA-Z\\.\\-]+$').matches(/^[0-9a-zA-Z\\.\\-]+$/i);
-
-        const newStrategy = req.body;
-
-        validateRequest(req)
+        const data = req.body;
+        validateInput(data)
             .then(validateStrategyName)
-            .then(() => eventStore.store({
+            .then((newStrategy) => eventStore.store({
                 type: eventType.STRATEGY_CREATED,
                 createdBy: extractUser(req),
                 data: newStrategy,
@@ -80,11 +75,22 @@ module.exports = function (app, config) {
             .catch(error => handleError(req, res, error));
     });
 
-    function validateStrategyName (req) {
+    function validateStrategyName (data) {
         return new Promise((resolve, reject) => {
-            strategyStore.getStrategy(req.body.name)
+            strategyStore.getStrategy(data.name)
                 .then(() => reject(new NameExistsError('Feature name already exist')))
-                .catch(() => resolve(req));
+                .catch(() => resolve(data));
+        });
+    }
+
+    function validateInput (data) {
+        return new Promise((resolve, reject) => {
+            joi.validate(data, strategySchema, (err, cleaned) => {
+                if (err) {
+                    return reject(err);
+                }
+                return resolve(cleaned);
+            });
         });
     }
 };
diff --git a/migrator.js b/migrator.js
index 6c458bd5af..1b21e789ea 100644
--- a/migrator.js
+++ b/migrator.js
@@ -2,6 +2,7 @@
 
 const { getInstance } = require('db-migrate');
 const parseDbUrl = require('parse-database-url');
+require('db-migrate-shared').log.silence(true);
 
 function migrateDb ({ databaseUrl, databaseSchema = 'public' }) {
     const custom = parseDbUrl(databaseUrl);
diff --git a/test/e2e/strategy-api.test.js b/test/e2e/strategy-api.test.js
index 40aa3deb0e..8f47201a6d 100644
--- a/test/e2e/strategy-api.test.js
+++ b/test/e2e/strategy-api.test.js
@@ -42,7 +42,7 @@ test.serial('creates a new strategy', async (t) => {
     const { request, destroy } = await setupApp('strategy_api_serial');
     return request
         .post('/api/strategies')
-        .send({ name: 'myCustomStrategy', description: 'Best strategy ever.' })
+        .send({ name: 'myCustomStrategy', description: 'Best strategy ever.', parameters: [] })
         .set('Content-Type', 'application/json')
         .expect(201)
         .then(destroy);
@@ -62,7 +62,7 @@ test.serial('refuses to create a strategy with an existing name', async (t) => {
     const { request, destroy } = await setupApp('strategy_api_serial');
     return request
         .post('/api/strategies')
-        .send({ name: 'default' })
+        .send({ name: 'default', parameters: [] })
         .set('Content-Type', 'application/json')
         .expect(403)
         .then(destroy);