Merge pull request #242 from Unleash/editable-strategies

protection against edit on  built in strategies

CHANGELOG.md

@@ -1,6 +1,7 @@
 # Changelog
 
 ## [Unreleased]
+- disable edit of built-in strategies
 
 ## 3.0.0-alpha.1
 - upgrade unleash-frontend to 3.0.0-alpha.1

lib/db/strategy-store.js

@@ -7,7 +7,7 @@ const {
 } = require('../event-type');
 const logger = require('../logger');
 const NotFoundError = require('../error/notfound-error');
-const STRATEGY_COLUMNS = ['name', 'description', 'parameters'];
+const STRATEGY_COLUMNS = ['name', 'description', 'parameters', 'built_in'];
 const TABLE = 'strategies';
 
 class StrategyStore {
@@ -46,9 +46,9 @@ class StrategyStore {
         if (!row) {
             throw new NotFoundError('No strategy found');
         }
-
         return {
             name: row.name,
+            editable: row.built_in !== 1,
             description: row.description,
             parameters: row.parameters,
         };

lib/routes/admin-api/strategy-schema.js

@@ -4,6 +4,7 @@ const joi = require('joi');
 
 const strategySchema = joi.object().keys({
     name: joi.string().regex(/^[a-zA-Z0-9\\.\\-]{3,100}$/).required(),
+    editable: joi.boolean().default(true),
     description: joi.string(),
     parameters: joi.array().required().items(
         joi.object().keys({

lib/routes/admin-api/strategy.js

@@ -33,6 +33,28 @@ const handleError = (req, res, error) => {
     }
 };
 
+function validateEditable(strategyName) {
+    return strategy => {
+        if (strategy.editable === false) {
+            throw new Error(
+                `Cannot edit strategy ${strategyName}, editable is false`
+            );
+        }
+        return strategy;
+    };
+}
+
+function validateInput(data) {
+    return new Promise((resolve, reject) => {
+        joi.validate(data, strategySchema, (err, cleaned) => {
+            if (err) {
+                return reject(err);
+            }
+            return resolve(cleaned);
+        });
+    });
+}
+
 exports.router = function(config) {
     const { strategyStore, eventStore } = config.stores;
     const router = Router();
@@ -57,6 +79,7 @@ exports.router = function(config) {
 
         strategyStore
             .getStrategy(strategyName)
+            .then(validateEditable(strategyName))
             .then(() =>
                 eventStore.store({
                     type: eventType.STRATEGY_DELETED,
@@ -70,6 +93,17 @@ exports.router = function(config) {
             .catch(error => handleError(req, res, error));
     });
 
+    function validateStrategyName(data) {
+        return new Promise((resolve, reject) => {
+            strategyStore
+                .getStrategy(data.name)
+                .then(() =>
+                    reject(new NameExistsError('Feature name already exist'))
+                )
+                .catch(() => resolve(data));
+        });
+    }
+
     router.post('/', (req, res) => {
         const data = req.body;
         validateInput(data)
@@ -93,6 +127,7 @@ exports.router = function(config) {
 
         strategyStore
             .getStrategy(strategyName)
+            .then(validateEditable(strategyName))
             .then(() => validateInput(updatedStrategy))
             .then(() =>
                 eventStore.store({
@@ -105,27 +140,5 @@ exports.router = function(config) {
             .catch(error => handleError(req, res, error));
     });
 
-    function validateStrategyName(data) {
-        return new Promise((resolve, reject) => {
-            strategyStore
-                .getStrategy(data.name)
-                .then(() =>
-                    reject(new NameExistsError('Feature name already exist'))
-                )
-                .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);
-            });
-        });
-    }
-
     return router;
 };

lib/routes/admin-api/strategy.test.js

@@ -29,7 +29,7 @@ test.beforeEach(() => {
     logger.setLevel('FATAL');
 });
 
-test('should add version numbers for /stategies', t => {
+test('add version numbers for /stategies', t => {
     t.plan(1);
     const { request, base } = getSetup();
 
@@ -42,7 +42,7 @@ test('should add version numbers for /stategies', t => {
         });
 });
 
-test('should require a name when creating a new stratey', t => {
+test('require a name when creating a new stratey', t => {
     t.plan(1);
     const { request, base } = getSetup();
 
@@ -55,7 +55,7 @@ test('should require a name when creating a new stratey', t => {
         });
 });
 
-test('should require parameters array when creating a new stratey', t => {
+test('require parameters array when creating a new stratey', t => {
     t.plan(1);
     const { request, base } = getSetup();
 
@@ -68,7 +68,7 @@ test('should require parameters array when creating a new stratey', t => {
         });
 });
 
-test('should create a new stratey with empty parameters', t => {
+test('create a new stratey with empty parameters', t => {
     t.plan(0);
     const { request, base } = getSetup();
 
@@ -78,7 +78,7 @@ test('should create a new stratey with empty parameters', t => {
         .expect(201);
 });
 
-test('should not be possible to override name', t => {
+test('not be possible to override name', t => {
     t.plan(0);
     const { request, base, strategyStore } = getSetup();
     strategyStore.addStrategy({ name: 'Testing', parameters: [] });
@@ -89,7 +89,7 @@ test('should not be possible to override name', t => {
         .expect(403);
 });
 
-test('should update strategy', t => {
+test('update strategy', t => {
     t.plan(0);
     const name = 'AnotherStrat';
     const { request, base, strategyStore } = getSetup();
@@ -101,7 +101,7 @@ test('should update strategy', t => {
         .expect(200);
 });
 
-test('should not update uknown strategy', t => {
+test('not update uknown strategy', t => {
     t.plan(0);
     const name = 'UnknownStrat';
     const { request, base } = getSetup();
@@ -112,7 +112,7 @@ test('should not update uknown strategy', t => {
         .expect(404);
 });
 
-test('should validate format when updating strategy', t => {
+test('validate format when updating strategy', t => {
     t.plan(0);
     const name = 'AnotherStrat';
     const { request, base, strategyStore } = getSetup();
@@ -123,3 +123,46 @@ test('should validate format when updating strategy', t => {
         .send({})
         .expect(400);
 });
+
+test('editable=false will stop delete request', t => {
+    t.plan(0);
+    const name = 'default';
+    const { request, base } = getSetup();
+
+    return request.delete(`${base}/api/admin/strategies/${name}`).expect(500);
+});
+
+test('editable=false will stop edit request', t => {
+    t.plan(0);
+    const name = 'default';
+    const { request, base } = getSetup();
+
+    return request
+        .put(`${base}/api/admin/strategies/${name}`)
+        .send({ name, parameters: [] })
+        .expect(500);
+});
+
+test('editable=true will allow delete request', t => {
+    t.plan(0);
+    const name = 'deleteStrat';
+    const { request, base, strategyStore } = getSetup();
+    strategyStore.addStrategy({ name, parameters: [] });
+
+    return request
+        .delete(`${base}/api/admin/strategies/${name}`)
+        .send({})
+        .expect(200);
+});
+
+test('editable=true will allow edit request', t => {
+    t.plan(0);
+    const name = 'editStrat';
+    const { request, base, strategyStore } = getSetup();
+    strategyStore.addStrategy({ name, parameters: [] });
+
+    return request
+        .put(`${base}/api/admin/strategies/${name}`)
+        .send({ name, parameters: [] })
+        .expect(200);
+});

test/fixtures/fake-strategies-store.js

@@ -3,7 +3,7 @@
 const NotFoundError = require('../../lib/error/notfound-error');
 
 module.exports = () => {
-    const _strategies = [{ name: 'default', parameters: {} }];
+    const _strategies = [{ name: 'default', editable: false, parameters: {} }];
 
     return {
         getStrategies: () => Promise.resolve(_strategies),