From 175208c105e6cf3b9c9643826475b3edf8b2c60e Mon Sep 17 00:00:00 2001 From: Benjamin Ludewig Date: Wed, 19 Dec 2018 14:50:01 +0100 Subject: [PATCH] Refactored controllers, moved checkPermission to permission-checker.js middleware --- lib/middleware/permission-checker.js | 29 +++++++++++++++++++ .../permission-checker.test.js} | 9 +++--- lib/permissions.js | 25 ---------------- lib/routes/admin-api/archive.js | 8 ++--- lib/routes/admin-api/event.js | 6 ++-- lib/routes/admin-api/feature.js | 8 ++--- lib/routes/admin-api/index.js | 20 +++++-------- lib/routes/admin-api/metrics.js | 11 ++++--- lib/routes/admin-api/strategy.js | 8 ++--- lib/routes/admin-api/user.js | 8 +++-- lib/routes/controller.js | 21 +++++--------- 11 files changed, 72 insertions(+), 81 deletions(-) create mode 100644 lib/middleware/permission-checker.js rename lib/{permissions.test.js => middleware/permission-checker.test.js} (89%) diff --git a/lib/middleware/permission-checker.js b/lib/middleware/permission-checker.js new file mode 100644 index 0000000000..5f982f5f99 --- /dev/null +++ b/lib/middleware/permission-checker.js @@ -0,0 +1,29 @@ +'use strict'; + +const MissingPermission = require('../missing-permission'); +const { ADMIN } = require('../permissions'); + +module.exports = function(config, permission) { + if (!permission || !config.extendedPermissions) { + return (req, res, next) => next(); + } + return (req, res, 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( + new MissingPermission({ + permission, + message: `You require ${permission} to perform this action`, + }) + ) + .end(); + }; +}; diff --git a/lib/permissions.test.js b/lib/middleware/permission-checker.test.js similarity index 89% rename from lib/permissions.test.js rename to lib/middleware/permission-checker.test.js index dd3e7577ff..3bc48014bc 100644 --- a/lib/permissions.test.js +++ b/lib/middleware/permission-checker.test.js @@ -1,10 +1,10 @@ 'use strict'; const test = require('ava'); -const store = require('./../test/fixtures/store'); -const { requirePermission } = require('./permissions'); +const store = require('../../test/fixtures/store'); +const checkPermission = require('./permission-checker'); const supertest = require('supertest'); -const getApp = require('./app'); +const getApp = require('../app'); const { EventEmitter } = require('events'); const eventBus = new EventEmitter(); @@ -16,13 +16,12 @@ function getSetup(preRouterHook) { baseUriPath: base, stores, eventBus, - extendedPermissions: true, preRouterHook(_app) { preRouterHook(_app); _app.get( `${base}/protectedResource`, - requirePermission('READ'), + checkPermission({ extendedPermissions: true }, 'READ'), (req, res) => { res.status(200) .json({ message: 'OK' }) diff --git a/lib/permissions.js b/lib/permissions.js index 0c75d0b0fd..4c3bfe3e96 100644 --- a/lib/permissions.js +++ b/lib/permissions.js @@ -1,7 +1,5 @@ 'use strict'; -const MissingPermission = require('./missing-permission'); - const ADMIN = 'ADMIN'; const CREATE_FEATURE = 'CREATE_FEATURE'; const UPDATE_FEATURE = 'UPDATE_FEATURE'; @@ -11,30 +9,7 @@ const UPDATE_STRATEGY = 'UPDATE_STRATEGY'; const DELETE_STRATEGY = 'DELETE_STRATEGY'; const UPDATE_APPLICATION = 'UPDATE_APPLICATION'; -function requirePermission(permission) { - return (req, res, 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( - new MissingPermission({ - permission, - message: `You require ${permission} to perform this action`, - }) - ) - .end(); - }; -} - module.exports = { - requirePermission, ADMIN, CREATE_FEATURE, UPDATE_FEATURE, diff --git a/lib/routes/admin-api/archive.js b/lib/routes/admin-api/archive.js index 138d4ef0b3..25b4e8ac44 100644 --- a/lib/routes/admin-api/archive.js +++ b/lib/routes/admin-api/archive.js @@ -8,10 +8,10 @@ const extractUser = require('../../extract-user'); const { UPDATE_FEATURE } = require('../../permissions'); class ArchiveController extends Controller { - constructor(extendedPerms, { featureToggleStore, eventStore }) { - super(extendedPerms); - this.featureToggleStore = featureToggleStore; - this.eventStore = eventStore; + constructor(config) { + super(config); + this.featureToggleStore = config.stores.featureToggleStore; + this.eventStore = config.stores.eventStore; this.get('/features', this.getArchivedFeatures); this.post('/revive/:name', this.reviveFeatureToggle, UPDATE_FEATURE); diff --git a/lib/routes/admin-api/event.js b/lib/routes/admin-api/event.js index e0a6a187c8..963306564c 100644 --- a/lib/routes/admin-api/event.js +++ b/lib/routes/admin-api/event.js @@ -6,9 +6,9 @@ const eventDiffer = require('../../event-differ'); const version = 1; class EventController extends Controller { - constructor({ eventStore }) { - super(); - this.eventStore = eventStore; + constructor(config) { + super(config); + this.eventStore = config.stores.eventStore; this.get('/', this.getEvents); this.get('/:name', this.getEventsForToggle); } diff --git a/lib/routes/admin-api/feature.js b/lib/routes/admin-api/feature.js index 35cba1dcfd..26e103ab51 100644 --- a/lib/routes/admin-api/feature.js +++ b/lib/routes/admin-api/feature.js @@ -20,10 +20,10 @@ const { featureShema, nameSchema } = require('./feature-schema'); const version = 1; class FeatureController extends Controller { - constructor(extendedPerms, { featureToggleStore, eventStore }) { - super(extendedPerms); - this.featureToggleStore = featureToggleStore; - this.eventStore = eventStore; + constructor(config) { + super(config); + this.featureToggleStore = config.stores.featureToggleStore; + this.eventStore = config.stores.eventStore; this.get('/', this.getAllToggles); this.post('/', this.createToggle, CREATE_FEATURE); diff --git a/lib/routes/admin-api/index.js b/lib/routes/admin-api/index.js index 38d171cc98..a5af241638 100644 --- a/lib/routes/admin-api/index.js +++ b/lib/routes/admin-api/index.js @@ -11,21 +11,15 @@ const apiDef = require('./api-def.json'); class AdminApi extends Controller { constructor(config) { - super(); - - const stores = config.stores; - const perms = config.extendedPermissions; + super(config); this.app.get('/', this.index); - this.app.use('/features', new FeatureController(perms, stores).router); - this.app.use('/archive', new ArchiveController(perms, stores).router); - this.app.use( - '/strategies', - new StrategyController(perms, stores).router - ); - this.app.use('/events', new EventController(stores).router); - this.app.use('/metrics', new MetricsController(perms, stores).router); - this.app.use('/user', new UserController(perms).router); + this.app.use('/features', new FeatureController(config).router); + this.app.use('/archive', new ArchiveController(config).router); + this.app.use('/strategies', new StrategyController(config).router); + this.app.use('/events', new EventController(config).router); + this.app.use('/metrics', new MetricsController(config).router); + this.app.use('/user', new UserController(config).router); } index(req, res) { diff --git a/lib/routes/admin-api/metrics.js b/lib/routes/admin-api/metrics.js index 4d4f51a997..ea32f63160 100644 --- a/lib/routes/admin-api/metrics.js +++ b/lib/routes/admin-api/metrics.js @@ -8,17 +8,16 @@ const schema = require('./metrics-schema'); const { UPDATE_APPLICATION } = require('../../permissions'); class MetricsController extends Controller { - constructor( - extendedPerms, - { + constructor(config) { + super(config); + const { clientMetricsStore, clientInstanceStore, clientApplicationsStore, strategyStore, featureToggleStore, - } - ) { - super(extendedPerms); + } = config.stores; + this.metrics = new ClientMetrics(clientMetricsStore); this.clientInstanceStore = clientInstanceStore; this.clientApplicationsStore = clientApplicationsStore; diff --git a/lib/routes/admin-api/strategy.js b/lib/routes/admin-api/strategy.js index e0f73e7f7b..9d079fd352 100644 --- a/lib/routes/admin-api/strategy.js +++ b/lib/routes/admin-api/strategy.js @@ -16,10 +16,10 @@ const { const version = 1; class StrategyController extends Controller { - constructor(extendedPerms, { strategyStore, eventStore }) { - super(extendedPerms); - this.strategyStore = strategyStore; - this.eventStore = eventStore; + constructor(config) { + super(config); + this.strategyStore = config.stores.strategyStore; + this.eventStore = config.stores.eventStore; this.get('/', this.getAllStratgies); this.get('/:name', this.getStrategy); diff --git a/lib/routes/admin-api/user.js b/lib/routes/admin-api/user.js index 19b87a5c34..e678670bbb 100644 --- a/lib/routes/admin-api/user.js +++ b/lib/routes/admin-api/user.js @@ -3,8 +3,8 @@ const Controller = require('../controller'); class UserController extends Controller { - constructor(perms) { - super(perms); + constructor(config) { + super(config); this.get('/', this.getUser); this.get('/logout', this.logout); } @@ -12,8 +12,10 @@ class UserController extends Controller { getUser(req, res) { if (req.user) { const user = Object.assign({}, req.user); - if (!this.extendedPermissions) { + if (!this.config.extendedPermissions) { delete user.permissions; + } else if (!Array.isArray(user.permissions)) { + user.permissions = []; } return res .status(200) diff --git a/lib/routes/controller.js b/lib/routes/controller.js index 015995f829..7909dc3f0b 100644 --- a/lib/routes/controller.js +++ b/lib/routes/controller.js @@ -1,28 +1,21 @@ 'use strict'; const { Router } = require('express'); -const { requirePermission } = require('./../permissions'); +const checkPermission = require('../middleware/permission-checker'); /** * Base class for Controllers to standardize binding to express Router. */ class Controller { - constructor(extendedPermissions) { + constructor(config) { const router = Router(); this.app = router; - this.extendedPermissions = extendedPermissions; - } - - checkPermission(permission) { - if (this.extendedPermissions && permission) { - return requirePermission(permission); - } - return (res, req, next) => next(); + this.config = config; } get(path, handler, permission) { this.app.get( path, - this.checkPermission(permission), + checkPermission(this.config, permission), handler.bind(this) ); } @@ -30,7 +23,7 @@ class Controller { post(path, handler, permission) { this.app.post( path, - this.checkPermission(permission), + checkPermission(this.config, permission), handler.bind(this) ); } @@ -38,7 +31,7 @@ class Controller { put(path, handler, permission) { this.app.put( path, - this.checkPermission(permission), + checkPermission(this.config, permission), handler.bind(this) ); } @@ -46,7 +39,7 @@ class Controller { delete(path, handler, permission) { this.app.delete( path, - this.checkPermission(permission), + checkPermission(this.config, permission), handler.bind(this) ); }