From d9804c0114354a6d43f8ab4f915fa33a4a3c0577 Mon Sep 17 00:00:00 2001 From: Benjamin Ludewig Date: Wed, 19 Dec 2018 13:17:44 +0100 Subject: [PATCH] Use full name instead of perms, 403 error message now includes expected permission --- lib/missing-permission.js | 8 +++++ lib/options.js | 1 + lib/permissions.js | 31 +++++++++++--------- lib/permissions.test.js | 4 +-- lib/routes/admin-api/archive.test.js | 4 +-- lib/routes/admin-api/feature.test.js | 12 ++++---- lib/routes/admin-api/metrics.test.js | 2 +- lib/routes/admin-api/strategy.test.js | 22 +++++++------- lib/routes/controller.js | 42 ++++++++++++++++++--------- test/fixtures/permissions.js | 4 +-- 10 files changed, 79 insertions(+), 51 deletions(-) create mode 100644 lib/missing-permission.js diff --git a/lib/missing-permission.js b/lib/missing-permission.js new file mode 100644 index 0000000000..c66dca34e6 --- /dev/null +++ b/lib/missing-permission.js @@ -0,0 +1,8 @@ +'use strict'; + +module.exports = class MissingPermission { + constructor({ permission, message }) { + this.permission = permission; + this.message = message; + } +}; diff --git a/lib/options.js b/lib/options.js index dce96bcb40..1fca109425 100644 --- a/lib/options.js +++ b/lib/options.js @@ -12,6 +12,7 @@ const DEFAULT_OPTIONS = { baseUriPath: process.env.BASE_URI_PATH || '', serverMetrics: true, enableLegacyRoutes: true, + extendedPermissions: false, publicFolder, enableRequestLogger: isDev(), secret: 'UNLEASH-SECRET', diff --git a/lib/permissions.js b/lib/permissions.js index 18f4aba937..0c75d0b0fd 100644 --- a/lib/permissions.js +++ b/lib/permissions.js @@ -1,5 +1,7 @@ 'use strict'; +const MissingPermission = require('./missing-permission'); + const ADMIN = 'ADMIN'; const CREATE_FEATURE = 'CREATE_FEATURE'; const UPDATE_FEATURE = 'UPDATE_FEATURE'; @@ -9,29 +11,30 @@ const UPDATE_STRATEGY = 'UPDATE_STRATEGY'; const DELETE_STRATEGY = 'DELETE_STRATEGY'; const UPDATE_APPLICATION = 'UPDATE_APPLICATION'; -function requirePerms(prms) { +function requirePermission(permission) { return (req, res, next) => { - for (const permission of prms) { - if ( - req.user && - req.user.permissions && - (req.user.permissions.indexOf(ADMIN) !== -1 || - req.user.permissions.indexOf(permission) !== -1) - ) { - return next(); - } + if ( + req.user && + req.user.permissions && + (req.user.permissions.indexOf(ADMIN) !== -1 || + req.user.permissions.indexOf(permission) !== -1) + ) { + return next(); } return res .status(403) - .json({ - message: 'Missing permissions to perform this action.', - }) + .json( + new MissingPermission({ + permission, + message: `You require ${permission} to perform this action`, + }) + ) .end(); }; } module.exports = { - requirePerms, + requirePermission, ADMIN, CREATE_FEATURE, UPDATE_FEATURE, diff --git a/lib/permissions.test.js b/lib/permissions.test.js index 8240a1ccb5..dd3e7577ff 100644 --- a/lib/permissions.test.js +++ b/lib/permissions.test.js @@ -2,7 +2,7 @@ const test = require('ava'); const store = require('./../test/fixtures/store'); -const { requirePerms } = require('./permissions'); +const { requirePermission } = require('./permissions'); const supertest = require('supertest'); const getApp = require('./app'); @@ -22,7 +22,7 @@ function getSetup(preRouterHook) { _app.get( `${base}/protectedResource`, - requirePerms(['READ']), + requirePermission('READ'), (req, res) => { res.status(200) .json({ message: 'OK' }) diff --git a/lib/routes/admin-api/archive.test.js b/lib/routes/admin-api/archive.test.js index 11c030b389..54e356e456 100644 --- a/lib/routes/admin-api/archive.test.js +++ b/lib/routes/admin-api/archive.test.js @@ -67,7 +67,7 @@ test('should revive toggle', t => { t.plan(0); const name = 'name1'; const { request, base, archiveStore, perms } = getSetup(); - perms.withPerms(UPDATE_FEATURE); + perms.withPermissions(UPDATE_FEATURE); archiveStore.addArchivedFeature({ name, strategies: [{ name: 'default' }], @@ -80,7 +80,7 @@ test('should create event when reviving toggle', async t => { t.plan(4); const name = 'name1'; const { request, base, archiveStore, eventStore, perms } = getSetup(); - perms.withPerms(UPDATE_FEATURE); + perms.withPermissions(UPDATE_FEATURE); archiveStore.addArchivedFeature({ name, strategies: [{ name: 'default' }], diff --git a/lib/routes/admin-api/feature.test.js b/lib/routes/admin-api/feature.test.js index 6a7074c8ff..0a78ec6407 100644 --- a/lib/routes/admin-api/feature.test.js +++ b/lib/routes/admin-api/feature.test.js @@ -79,7 +79,7 @@ test('should add version numbers for /features', t => { test('should require at least one strategy when creating a feature toggle', t => { t.plan(0); const { request, base, perms } = getSetup(); - perms.withPerms(CREATE_FEATURE); + perms.withPermissions(CREATE_FEATURE); return request .post(`${base}/api/admin/features`) @@ -91,7 +91,7 @@ test('should require at least one strategy when creating a feature toggle', t => test('should be allowed to use new toggle name', t => { t.plan(0); const { request, base, perms } = getSetup(); - perms.withPerms(CREATE_FEATURE); + perms.withPermissions(CREATE_FEATURE); return request .post(`${base}/api/admin/features/validate`) @@ -145,7 +145,7 @@ test('should not be allowed to reuse archived toggle name', t => { test('should require at least one strategy when updating a feature toggle', t => { t.plan(0); const { request, featureToggleStore, base, perms } = getSetup(); - perms.withPerms(UPDATE_FEATURE); + perms.withPermissions(UPDATE_FEATURE); featureToggleStore.addFeature({ name: 'ts', strategies: [{ name: 'default' }], @@ -161,7 +161,7 @@ test('should require at least one strategy when updating a feature toggle', t => test('valid feature names should pass validation', t => { t.plan(0); const { request, base, perms } = getSetup(); - perms.withPerms(CREATE_FEATURE); + perms.withPermissions(CREATE_FEATURE); const validNames = [ 'com.example', @@ -190,7 +190,7 @@ test('valid feature names should pass validation', t => { test('invalid feature names should not pass validation', t => { t.plan(0); const { request, base, perms } = getSetup(); - perms.withPerms(CREATE_FEATURE); + perms.withPermissions(CREATE_FEATURE); const invalidNames = [ 'some example', @@ -219,7 +219,7 @@ test('invalid feature names should not pass validation', t => { test('invalid feature names should have error msg', t => { t.plan(1); const { request, base, perms } = getSetup(); - perms.withPerms(CREATE_FEATURE); + perms.withPermissions(CREATE_FEATURE); const name = 'ØÆ`'; diff --git a/lib/routes/admin-api/metrics.test.js b/lib/routes/admin-api/metrics.test.js index 112464b4a2..0471aaed47 100644 --- a/lib/routes/admin-api/metrics.test.js +++ b/lib/routes/admin-api/metrics.test.js @@ -133,7 +133,7 @@ test('should store application', t => { t.plan(0); const { request, perms } = getSetup(); const appName = '123!23'; - perms.withPerms(UPDATE_APPLICATION); + perms.withPermissions(UPDATE_APPLICATION); return request .post(`/api/admin/metrics/applications/${appName}`) diff --git a/lib/routes/admin-api/strategy.test.js b/lib/routes/admin-api/strategy.test.js index c3cec26525..6b0499c545 100644 --- a/lib/routes/admin-api/strategy.test.js +++ b/lib/routes/admin-api/strategy.test.js @@ -50,7 +50,7 @@ test('add version numbers for /stategies', t => { test('require a name when creating a new stratey', t => { t.plan(1); const { request, base, perms } = getSetup(); - perms.withPerms(CREATE_STRATEGY); + perms.withPermissions(CREATE_STRATEGY); return request .post(`${base}/api/admin/strategies`) @@ -64,7 +64,7 @@ test('require a name when creating a new stratey', t => { test('require parameters array when creating a new stratey', t => { t.plan(1); const { request, base, perms } = getSetup(); - perms.withPerms(CREATE_STRATEGY); + perms.withPermissions(CREATE_STRATEGY); return request .post(`${base}/api/admin/strategies`) @@ -78,7 +78,7 @@ test('require parameters array when creating a new stratey', t => { test('create a new stratey with empty parameters', t => { t.plan(0); const { request, base, perms } = getSetup(); - perms.withPerms(CREATE_STRATEGY); + perms.withPermissions(CREATE_STRATEGY); return request .post(`${base}/api/admin/strategies`) @@ -89,7 +89,7 @@ test('create a new stratey with empty parameters', t => { test('not be possible to override name', t => { t.plan(0); const { request, base, strategyStore, perms } = getSetup(); - perms.withPerms(CREATE_STRATEGY); + perms.withPermissions(CREATE_STRATEGY); strategyStore.addStrategy({ name: 'Testing', parameters: [] }); return request @@ -102,7 +102,7 @@ test('update strategy', t => { t.plan(0); const name = 'AnotherStrat'; const { request, base, strategyStore, perms } = getSetup(); - perms.withPerms(UPDATE_STRATEGY); + perms.withPermissions(UPDATE_STRATEGY); strategyStore.addStrategy({ name, parameters: [] }); return request @@ -115,7 +115,7 @@ test('not update unknown strategy', t => { t.plan(0); const name = 'UnknownStrat'; const { request, base, perms } = getSetup(); - perms.withPerms(UPDATE_STRATEGY); + perms.withPermissions(UPDATE_STRATEGY); return request .put(`${base}/api/admin/strategies/${name}`) @@ -127,7 +127,7 @@ test('validate format when updating strategy', t => { t.plan(0); const name = 'AnotherStrat'; const { request, base, strategyStore, perms } = getSetup(); - perms.withPerms(UPDATE_STRATEGY); + perms.withPermissions(UPDATE_STRATEGY); strategyStore.addStrategy({ name, parameters: [] }); return request @@ -140,7 +140,7 @@ test('editable=false will stop delete request', t => { t.plan(0); const name = 'default'; const { request, base, perms } = getSetup(); - perms.withPerms(DELETE_STRATEGY); + perms.withPermissions(DELETE_STRATEGY); return request.delete(`${base}/api/admin/strategies/${name}`).expect(500); }); @@ -149,7 +149,7 @@ test('editable=false will stop edit request', t => { t.plan(0); const name = 'default'; const { request, base, perms } = getSetup(); - perms.withPerms(UPDATE_STRATEGY); + perms.withPermissions(UPDATE_STRATEGY); return request .put(`${base}/api/admin/strategies/${name}`) @@ -161,7 +161,7 @@ test('editable=true will allow delete request', t => { t.plan(0); const name = 'deleteStrat'; const { request, base, strategyStore, perms } = getSetup(); - perms.withPerms(DELETE_STRATEGY); + perms.withPermissions(DELETE_STRATEGY); strategyStore.addStrategy({ name, parameters: [] }); return request @@ -174,7 +174,7 @@ test('editable=true will allow edit request', t => { t.plan(0); const name = 'editStrat'; const { request, base, strategyStore, perms } = getSetup(); - perms.withPerms(UPDATE_STRATEGY); + perms.withPermissions(UPDATE_STRATEGY); strategyStore.addStrategy({ name, parameters: [] }); return request diff --git a/lib/routes/controller.js b/lib/routes/controller.js index 44441f7044..64a9535740 100644 --- a/lib/routes/controller.js +++ b/lib/routes/controller.js @@ -1,7 +1,7 @@ 'use strict'; const { Router } = require('express'); -const { requirePerms } = require('./../permissions'); +const { requirePermission } = require('./../permissions'); /** * Base class for Controllers to standardize binding to express Router. */ @@ -12,30 +12,46 @@ class Controller { this.extendedPerms = extendedPerms; } - get(path, handler, ...perms) { - if (this.extendedPerms && perms.length > 0) { - this.app.get(path, requirePerms(perms), handler.bind(this)); + get(path, handler, permission) { + if (this.extendedPerms && permission) { + this.app.get( + path, + requirePermission(permission), + handler.bind(this) + ); } this.app.get(path, handler.bind(this)); } - post(path, handler, ...perms) { - if (this.extendedPerms && perms.length > 0) { - this.app.post(path, requirePerms(perms), handler.bind(this)); + post(path, handler, permission) { + if (this.extendedPerms && permission) { + this.app.post( + path, + requirePermission(permission), + handler.bind(this) + ); } this.app.post(path, handler.bind(this)); } - put(path, handler, ...perms) { - if (this.extendedPerms && perms.length > 0) { - this.app.put(path, requirePerms(perms), handler.bind(this)); + put(path, handler, permission) { + if (this.extendedPerms && permission) { + this.app.put( + path, + requirePermission(permission), + handler.bind(this) + ); } this.app.put(path, handler.bind(this)); } - delete(path, handler, ...perms) { - if (this.extendedPerms && perms.length > 0) { - this.app.delete(path, requirePerms(perms), handler.bind(this)); + delete(path, handler, permission) { + if (this.extendedPerms && permission) { + this.app.delete( + path, + requirePermission(permission), + handler.bind(this) + ); } this.app.delete(path, handler.bind(this)); } diff --git a/test/fixtures/permissions.js b/test/fixtures/permissions.js index 4f02db0e4c..550c8bebed 100644 --- a/test/fixtures/permissions.js +++ b/test/fixtures/permissions.js @@ -10,8 +10,8 @@ module.exports = () => { next(); }); }, - withPerms(...prms) { - _perms = prms; + withPermissions(...perms) { + _perms = perms; }, }; };