From 9eb0d2e53561a43f2f538b1f951c568cc23b07ff Mon Sep 17 00:00:00 2001 From: ivaosthu Date: Sat, 1 Dec 2018 12:03:47 +0100 Subject: [PATCH] chore(modernize): Use joi schema-validation in FeatureController --- lib/error/name-invalid-error.js | 14 ++++ lib/routes/admin-api/feature-schema.js | 30 +++++++ lib/routes/admin-api/feature.js | 109 ++++++++++--------------- lib/routes/admin-api/feature.test.js | 20 +++++ lib/routes/admin-api/util.js | 5 ++ 5 files changed, 114 insertions(+), 64 deletions(-) create mode 100644 lib/error/name-invalid-error.js create mode 100644 lib/routes/admin-api/feature-schema.js create mode 100644 lib/routes/admin-api/util.js diff --git a/lib/error/name-invalid-error.js b/lib/error/name-invalid-error.js new file mode 100644 index 0000000000..36f4dad07b --- /dev/null +++ b/lib/error/name-invalid-error.js @@ -0,0 +1,14 @@ +'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 new file mode 100644 index 0000000000..8e6aadb814 --- /dev/null +++ b/lib/routes/admin-api/feature-schema.js @@ -0,0 +1,30 @@ +'use strict'; + +const joi = require('joi'); + +const strategiesSchema = joi.object().keys({ + name: joi + .string() + .regex(/^[a-zA-Z0-9\\.\\-]{3,100}$/) + .required(), + parameters: joi.object(), +}); + +const featureShema = joi + .object() + .keys({ + name: joi + .string() + .regex(/^[a-zA-Z0-9\\.\\-]{3,100}$/) + .required(), + enabled: joi.boolean().default(false), + description: joi.string(), + strategies: joi + .array() + .required() + .min(1) + .items(strategiesSchema), + }) + .options({ allowUnknown: false, stripUnknown: true }); + +module.exports = { featureShema, strategiesSchema }; diff --git a/lib/routes/admin-api/feature.js b/lib/routes/admin-api/feature.js index f24592778e..f52278228b 100644 --- a/lib/routes/admin-api/feature.js +++ b/lib/routes/admin-api/feature.js @@ -10,25 +10,32 @@ const { FEATURE_ARCHIVED, } = require('../../event-type'); const NameExistsError = require('../../error/name-exists-error'); -const NotFoundError = require('../../error/notfound-error'); -const ValidationError = require('../../error/validation-error.js'); -const validateRequest = require('../../error/validate-request'); +const NameInvalidError = require('../../error/name-invalid-error'); +const { isUrlFriendlyName } = require('./util'); + const extractUser = require('../../extract-user'); +const { featureShema } = require('./feature-schema'); + const handleErrors = (req, res, error) => { logger.warn('Error creating or updating feature', error); - switch (error.constructor) { - case NotFoundError: + switch (error.name) { + case 'NotFoundError': return res.status(404).end(); - case NameExistsError: + case 'NameInvalidError': + return res + .status(400) + .json([{ msg: error.message }]) + .end(); + case 'NameExistsError': return res .status(403) .json([{ msg: error.message }]) .end(); - case ValidationError: + case 'ValidationError': return res .status(400) - .json(req.validationErrors()) + .json(error) .end(); default: logger.error('Server failed executing request', error); @@ -36,39 +43,6 @@ const handleErrors = (req, res, error) => { } }; -const strategiesSchema = joi.object().keys({ - name: joi - .string() - .regex(/^[a-zA-Z0-9\\.\\-]{3,100}$/) - .required(), - parameters: joi.object(), -}); - -function validateStrategy(featureToggle) { - return new Promise((resolve, reject) => { - if ( - !featureToggle.strategies || - featureToggle.strategies.length === 0 - ) { - return reject( - new ValidationError('You must define at least one strategy') - ); - } - - featureToggle.strategies = featureToggle.strategies.map( - strategyConfig => { - const result = joi.validate(strategyConfig, strategiesSchema); - if (result.error) { - throw result.error; - } - return result.value; - } - ); - - return resolve(featureToggle); - }); -} - const version = 1; class FeatureController extends Controller { @@ -104,42 +78,49 @@ class FeatureController extends Controller { } async validate(req, res) { - req.checkBody('name', 'Name is required').notEmpty(); - req.checkBody('name', 'Name must be URL friendly').isUrlFriendlyName(); + const name = req.body.name; try { - await validateRequest(req); - await this.validateUniqueName(req); + await this.validateNameFormat(name); + await this.validateUniqueName(name); res.status(201).end(); } catch (error) { handleErrors(req, res, error); } } - validateUniqueName(req) { - return new Promise((resolve, reject) => { - this.featureToggleStore - .hasFeature(req.body.name) - .then(definition => { - const msg = definition.archived - ? 'An archived toggle with that name already exist' - : 'A toggle with that name already exist'; - reject(new NameExistsError(msg)); - }) - .catch(() => resolve(req)); - }); + // TODO: cleanup this validation + async validateUniqueName(name) { + let msg; + try { + const definition = await this.featureToggleStore.hasFeature(name); + msg = definition.archived + ? 'An archived toggle with that name already exist' + : 'A toggle with that name already exist'; + } catch (error) { + // No conflict, everything ok! + return; + } + + // Interntional throw here! + 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) { - req.checkBody('name', 'Name is required').notEmpty(); - req.checkBody('name', 'Name must be URL friendly').isUrlFriendlyName(); - + const toggleName = req.body.name; const userName = extractUser(req); try { - await validateRequest(req); - await this.validateUniqueName(req); - const featureToggle = await validateStrategy(req.body); + await this.validateNameFormat(toggleName); + await this.validateUniqueName(toggleName); + const featureToggle = await joi.validate(req.body, featureShema); await this.eventStore.store({ type: FEATURE_CREATED, createdBy: userName, @@ -160,7 +141,7 @@ class FeatureController extends Controller { try { await this.featureToggleStore.getFeature(featureName); - await validateStrategy(updatedFeature); + await joi.validate(updatedFeature, featureShema); await this.eventStore.store({ type: FEATURE_UPDATED, createdBy: userName, diff --git a/lib/routes/admin-api/feature.test.js b/lib/routes/admin-api/feature.test.js index 2a49bbdb45..dd08b7687e 100644 --- a/lib/routes/admin-api/feature.test.js +++ b/lib/routes/admin-api/feature.test.js @@ -200,3 +200,23 @@ test('invalid feature names should not pass validation', t => { ) ); }); + +test('invalid feature names should have error msg', t => { + t.plan(1); + const { request, base } = getSetup(); + + const name = 'ØÆ`'; + + return request + .post(`${base}/api/admin/features`) + .send({ + name, + enabled: false, + strategies: [{ name: 'default' }], + }) + .set('Content-Type', 'application/json') + .expect(400) + .expect(res => { + t.true(res.body[0].msg === 'Name must be URL friendly'); + }); +}); diff --git a/lib/routes/admin-api/util.js b/lib/routes/admin-api/util.js new file mode 100644 index 0000000000..6664e034ad --- /dev/null +++ b/lib/routes/admin-api/util.js @@ -0,0 +1,5 @@ +'use strict'; + +const isUrlFriendlyName = input => encodeURIComponent(input) === input; + +module.exports = { isUrlFriendlyName };