diff --git a/lib/error/name-invalid-error.js b/lib/error/name-invalid-error.js deleted file mode 100644 index 36f4dad07b..0000000000 --- a/lib/error/name-invalid-error.js +++ /dev/null @@ -1,14 +0,0 @@ -'use strict'; - -class NameInvalidError extends Error { - constructor(message) { - super(); - Error.captureStackTrace(this, this.constructor); - - this.name = this.constructor.name; - this.message = message; - this.msg = message; - } -} - -module.exports = NameInvalidError; diff --git a/lib/routes/admin-api/feature-schema.js b/lib/routes/admin-api/feature-schema.js index 8e6aadb814..f8bdde07ce 100644 --- a/lib/routes/admin-api/feature-schema.js +++ b/lib/routes/admin-api/feature-schema.js @@ -1,22 +1,19 @@ 'use strict'; const joi = require('joi'); +const { nameType } = require('./util'); + +const nameSchema = joi.object().keys({ name: nameType }); const strategiesSchema = joi.object().keys({ - name: joi - .string() - .regex(/^[a-zA-Z0-9\\.\\-]{3,100}$/) - .required(), + name: nameType, parameters: joi.object(), }); const featureShema = joi .object() .keys({ - name: joi - .string() - .regex(/^[a-zA-Z0-9\\.\\-]{3,100}$/) - .required(), + name: nameType, enabled: joi.boolean().default(false), description: joi.string(), strategies: joi @@ -27,4 +24,4 @@ const featureShema = joi }) .options({ allowUnknown: false, stripUnknown: true }); -module.exports = { featureShema, strategiesSchema }; +module.exports = { featureShema, strategiesSchema, nameSchema }; diff --git a/lib/routes/admin-api/feature-schema.test.js b/lib/routes/admin-api/feature-schema.test.js new file mode 100644 index 0000000000..a36175cbcf --- /dev/null +++ b/lib/routes/admin-api/feature-schema.test.js @@ -0,0 +1,27 @@ +'use strict'; + +const { test } = require('ava'); +const { featureShema } = require('./feature-schema'); +const joi = require('joi'); + +test('should require URL firendly name', t => { + const toggle = { + name: 'io`dasd', + enabled: false, + strategies: [{ name: 'default' }], + }; + + const { error } = joi.validate(toggle, featureShema); + t.deepEqual(error.details[0].message, '"name" must be URL friendly'); +}); + +test('should be valid toggle name', t => { + const toggle = { + name: 'app.name', + enabled: false, + strategies: [{ name: 'default' }], + }; + + const { value } = joi.validate(toggle, featureShema); + t.deepEqual(value, toggle); +}); diff --git a/lib/routes/admin-api/feature.js b/lib/routes/admin-api/feature.js index 093e2057f2..0eb4b3dc57 100644 --- a/lib/routes/admin-api/feature.js +++ b/lib/routes/admin-api/feature.js @@ -9,10 +9,9 @@ const { FEATURE_ARCHIVED, } = require('../../event-type'); const NameExistsError = require('../../error/name-exists-error'); -const NameInvalidError = require('../../error/name-invalid-error'); -const { isUrlFriendlyName, handleErrors } = require('./util'); +const { handleErrors } = require('./util'); const extractUser = require('../../extract-user'); -const { featureShema } = require('./feature-schema'); +const { featureShema, nameSchema } = require('./feature-schema'); const version = 1; class FeatureController extends Controller { @@ -37,10 +36,8 @@ class FeatureController extends Controller { async getToggle(req, res) { try { - const featureName = req.params.featureName; - const feature = await this.featureToggleStore.getFeature( - featureName - ); + const name = req.params.featureName; + const feature = await this.featureToggleStore.getFeature(name); res.json(feature).end(); } catch (err) { res.status(404).json({ error: 'Could not find feature' }); @@ -51,7 +48,7 @@ class FeatureController extends Controller { const name = req.body.name; try { - await this.validateNameFormat(name); + await joi.validate({ name }, nameSchema); await this.validateUniqueName(name); res.status(201).end(); } catch (error) { @@ -76,19 +73,11 @@ class FeatureController extends Controller { throw new NameExistsError(msg); } - // TODO: this should be part of the schema validation! - validateNameFormat(name) { - if (!isUrlFriendlyName(name)) { - throw new NameInvalidError('Name must be URL friendly'); - } - } - async createToggle(req, res) { const toggleName = req.body.name; const userName = extractUser(req); try { - await this.validateNameFormat(toggleName); await this.validateUniqueName(toggleName); const featureToggle = await joi.validate(req.body, featureShema); await this.eventStore.store({ diff --git a/lib/routes/admin-api/feature.test.js b/lib/routes/admin-api/feature.test.js index 4c807896a6..35fa401a3d 100644 --- a/lib/routes/admin-api/feature.test.js +++ b/lib/routes/admin-api/feature.test.js @@ -218,6 +218,8 @@ test('invalid feature names should have error msg', t => { .set('Content-Type', 'application/json') .expect(400) .expect(res => { - t.true(res.body[0].msg === 'Name must be URL friendly'); + t.true( + res.body.details[0].message === '"name" must be URL friendly' + ); }); }); diff --git a/lib/routes/admin-api/strategy-schema.js b/lib/routes/admin-api/strategy-schema.js index 7db826ee33..3a61c015a9 100644 --- a/lib/routes/admin-api/strategy-schema.js +++ b/lib/routes/admin-api/strategy-schema.js @@ -1,12 +1,10 @@ 'use strict'; const joi = require('joi'); +const { nameType } = require('./util'); const strategySchema = joi.object().keys({ - name: joi - .string() - .regex(/^[a-zA-Z0-9\\.\\-]{3,100}$/) - .required(), + name: nameType, editable: joi.boolean().default(true), description: joi.string(), parameters: joi diff --git a/lib/routes/admin-api/util.js b/lib/routes/admin-api/util.js index 8c62d31cd3..0ff2366952 100644 --- a/lib/routes/admin-api/util.js +++ b/lib/routes/admin-api/util.js @@ -2,18 +2,45 @@ const logger = require('../../logger')('/admin-api/util.js'); -const isUrlFriendlyName = input => encodeURIComponent(input) === input; +const joi = require('joi'); + +const customJoi = joi.extend(j => ({ + base: j.string(), + name: 'string', + language: { + isUrlFriendly: 'must be URL friendly', + }, + rules: [ + { + name: 'isUrlFriendly', + validate(params, value, state, options) { + if (encodeURIComponent(value) !== value) { + // Generate an error, state and options need to be passed + return this.createError( + 'string.isUrlFriendly', + { v: value }, + state, + options + ); + } + return value; // Everything is OK + }, + }, + ], +})); + +const nameType = customJoi + .string() + .isUrlFriendly() + .min(2) + .max(100) + .required(); const handleErrors = (res, error) => { logger.warn(error.message); switch (error.name) { case 'NotFoundError': return res.status(404).end(); - case 'NameInvalidError': - return res - .status(400) - .json([{ msg: error.message }]) - .end(); case 'NameExistsError': return res .status(403) @@ -30,4 +57,4 @@ const handleErrors = (res, error) => { } }; -module.exports = { isUrlFriendlyName, handleErrors }; +module.exports = { nameType, handleErrors };