diff --git a/docs/deploy/migration-guide.md b/docs/deploy/migration-guide.md index a631eceef2..9319c6bf6e 100644 --- a/docs/deploy/migration-guide.md +++ b/docs/deploy/migration-guide.md @@ -5,6 +5,31 @@ title: Migration Guide Generally, the intention is that `unleash-server` should always provide support for clients one major version lower than the current one. This should make it possible to upgrade `unleash` gradually. +## Upgrading from v3.x to v4.x + +(Work In Process!) + +### Role-based Access Control (RBAC) + +We have implemented RBAC in Unleash v4. This has totally changed the permission system in Unleash. + +**Required actions:** If you have implemented "custom authentication" for your users you will need to make changes to your integration: + +- _extendedPermissions_ option has been removed. You can no longer specify custom permission per-user basis. All "logged_in users" must belong to a "root" role. This can be "Admin", "Editor" or "Viewer". This is taken care of when you create new users via userService. +- All "logged-in users" needs to be defined in Unleash and have a unique ID. This can be achieved by calling "createUser" on "userService". + +Code example: + +```js +const user = userService.loginUserWithoutPassword( + 'some@getunleash.io', + false, // autoCreateUser. Set to true if you want to create users on the fly. +); + +// The user needs to be set on the current active session +req.session.user = user; +``` + ## Upgrading from v2.x to v3.x The notable change introduced in Unleash v3.x is a strict separation of API paths for client requests and admin requests. This makes it easier to implement different authentication mechanisms for the admin UI and all unleash-clients. You can read more about [securing unleash](https://github.com/Unleash/unleash/blob/master/docs/securing-unleash.md). diff --git a/src/lib/app.test.js b/src/lib/app.test.js index f81e346169..e7c8babf82 100644 --- a/src/lib/app.test.js +++ b/src/lib/app.test.js @@ -14,13 +14,14 @@ const getApp = proxyquire('./app', { }); test('should not throw when valid config', t => { - const app = getApp({ getLogger }); + const app = getApp({ getLogger, stores: {} }); t.true(typeof app.listen === 'function'); }); test('should call preHook', t => { let called = 0; getApp({ + stores: {}, getLogger, preHook: () => { called++; @@ -32,6 +33,7 @@ test('should call preHook', t => { test('should call preRouterHook', t => { let called = 0; getApp({ + stores: {}, getLogger, preRouterHook: () => { called++; diff --git a/src/lib/app.ts b/src/lib/app.ts index e904937090..71ab25008e 100644 --- a/src/lib/app.ts +++ b/src/lib/app.ts @@ -30,7 +30,7 @@ module.exports = function(config, services = {}) { app.locals.baseUriPath = baseUriPath; if (typeof config.preHook === 'function') { - config.preHook(app); + config.preHook(app, config, services); } app.use(compression()); diff --git a/src/lib/db/access-store.ts b/src/lib/db/access-store.ts index aa05a29326..886da1d90e 100644 --- a/src/lib/db/access-store.ts +++ b/src/lib/db/access-store.ts @@ -44,13 +44,13 @@ export class AccessStore { }); } - async getPermissionsForUser(userId: Number): Promise { + async getPermissionsForUser(userId: number): Promise { const stopTimer = this.timer('getPermissionsForUser'); const rows = await this.db .select('project', 'permission') .from(`${T.ROLE_PERMISSION} AS rp`) .leftJoin(`${T.ROLE_USER} AS ur`, 'ur.role_id', 'rp.role_id') - .where('user_id', '=', userId); + .where('ur.user_id', '=', userId); stopTimer(); return rows; } diff --git a/src/lib/middleware/api-token-middleware.ts b/src/lib/middleware/api-token-middleware.ts index ddb7a84ceb..52c647b8f2 100644 --- a/src/lib/middleware/api-token-middleware.ts +++ b/src/lib/middleware/api-token-middleware.ts @@ -8,7 +8,7 @@ const apiAccessMiddleware = ( const logger = config.getLogger('/middleware/api-token.ts'); logger.info('Enabling api-token middleware'); - if(!config.authentication.enableApiToken) { + if (!config.authentication.enableApiToken) { return (req, res, next) => next(); } diff --git a/src/lib/middleware/no-authentication.js b/src/lib/middleware/no-authentication.js index 7797a75810..d43b00a8b4 100644 --- a/src/lib/middleware/no-authentication.js +++ b/src/lib/middleware/no-authentication.js @@ -1,10 +1,15 @@ 'use strict'; +const { ADMIN } = require('../permissions'); const User = require('../user'); function noneAuthentication(basePath = '', app) { app.use(`${basePath}/api/admin/`, (req, res, next) => { - req.user = new User({ username: 'unknown' }); + req.user = new User({ + username: 'unknown', + isAPI: true, + permissions: [ADMIN], + }); next(); }); } diff --git a/src/lib/middleware/permission-checker.js b/src/lib/middleware/permission-checker.js deleted file mode 100644 index 2aa331189b..0000000000 --- a/src/lib/middleware/permission-checker.js +++ /dev/null @@ -1,39 +0,0 @@ -'use strict'; - -const NoAccessError = require('../error/no-access-error'); -const { ADMIN } = require('../permissions'); -const { isRbacEnabled } = require('../util/feature-enabled'); - -module.exports = function(config, permission) { - if (!permission) { - return (req, res, next) => next(); - } - if (isRbacEnabled(config)) { - return async (req, res, next) => { - if (await req.checkRbac(permission)) { - return next(); - } - return res - .status(403) - .json(new NoAccessError(permission)) - .end(); - }; - } - if (!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 NoAccessError(permission)) - .end(); - }; -}; diff --git a/src/lib/middleware/permission-checker.test.js b/src/lib/middleware/permission-checker.test.js deleted file mode 100644 index c9d57f1c2e..0000000000 --- a/src/lib/middleware/permission-checker.test.js +++ /dev/null @@ -1,135 +0,0 @@ -'use strict'; - -const test = require('ava'); -const supertest = require('supertest'); -const sinon = require('sinon'); -const { EventEmitter } = require('events'); -const store = require('../../test/fixtures/store'); -const checkPermission = require('./permission-checker'); -const getApp = require('../app'); -const getLogger = require('../../test/fixtures/no-logger'); -const { CREATE_PROJECT } = require('../permissions'); - -const eventBus = new EventEmitter(); - -function getSetup(preRouterHook) { - const base = `/random${Math.round(Math.random() * 1000)}`; - const stores = store.createStores(); - const app = getApp({ - baseUriPath: base, - stores, - eventBus, - getLogger, - preRouterHook(_app) { - preRouterHook(_app); - - _app.get( - `${base}/protectedResource`, - checkPermission({ extendedPermissions: true }, 'READ'), - (req, res) => { - res.status(200) - .json({ message: 'OK' }) - .end(); - }, - ); - }, - }); - - return { - base, - request: supertest(app), - }; -} - -test('should return 403 when missing permission', t => { - t.plan(0); - const { base, request } = getSetup(() => {}); - - return request.get(`${base}/protectedResource`).expect(403); -}); - -test('should allow access with correct permissions', t => { - const { base, request } = getSetup(app => { - app.use((req, res, next) => { - req.user = { email: 'some@email.com', permissions: ['READ'] }; - next(); - }); - }); - - return request - .get(`${base}/protectedResource`) - .expect(200) - .expect(res => { - t.is(res.body.message, 'OK'); - }); -}); - -test('should allow access with admin permissions', t => { - const { base, request } = getSetup(app => { - app.use((req, res, next) => { - req.user = { email: 'some@email.com', permissions: ['ADMIN'] }; - next(); - }); - }); - - return request - .get(`${base}/protectedResource`) - .expect(200) - .expect(res => { - t.is(res.body.message, 'OK'); - }); -}); - -test('should call checkPermission if defined', async t => { - const config = { - experimental: { - rbac: true, - }, - }; - - const func = checkPermission(config, CREATE_PROJECT); - - const cb = sinon.fake(); - const req = { - checkRbac: sinon.fake.returns(Promise.resolve(true)), - }; - - func(req, undefined, cb); - - t.true(req.checkRbac.calledOnce); - t.is(req.checkRbac.firstArg, CREATE_PROJECT); -}); - -test('should call checkPermission if defined and give 403 response', async t => { - const config = { - experimental: { - rbac: true, - }, - }; - - const func = checkPermission(config, CREATE_PROJECT); - - const cb = sinon.fake(); - const req = { - checkRbac: sinon.fake.returns(Promise.resolve(false)), - }; - - const fakeJson = sinon.fake.returns({ - end: sinon.fake(), - }); - - const fakeStatus = sinon.fake.returns({ - json: fakeJson, - }); - - const res = { - status: fakeStatus, - }; - - await func(req, res, cb); - - t.true(req.checkRbac.calledOnce); - t.is(req.checkRbac.firstArg, CREATE_PROJECT); - t.false(cb.called); - t.is(fakeStatus.firstArg, 403); -}); diff --git a/src/lib/middleware/rbac-middleware.test.ts b/src/lib/middleware/rbac-middleware.test.ts index d03c2daa37..b5e7c2809a 100644 --- a/src/lib/middleware/rbac-middleware.test.ts +++ b/src/lib/middleware/rbac-middleware.test.ts @@ -24,21 +24,7 @@ test.beforeEach(() => { }; }); -test('should be disabled if rbac is disabled', t => { - const accessService = {}; - - const func = rbacMiddleware(config, { accessService }); - - const cb = sinon.fake(); - - func(undefined, undefined, cb); - - t.true(cb.calledOnce); -}); - -test('should add checkRbac to request if enabled', t => { - config.experimental.rbac = true; - +test('should add checkRbac to request', t => { const accessService = {}; const func = rbacMiddleware(config, { accessService }); diff --git a/src/lib/middleware/rbac-middleware.ts b/src/lib/middleware/rbac-middleware.ts index 355efc33cd..8fb2f4ea77 100644 --- a/src/lib/middleware/rbac-middleware.ts +++ b/src/lib/middleware/rbac-middleware.ts @@ -6,13 +6,7 @@ import { ADMIN, } from '../permissions'; -import { isRbacEnabled } from '../util/feature-enabled'; - const rbacMiddleware = (config: any, { accessService }: any): any => { - if (!isRbacEnabled(config)) { - return (req, res, next) => next(); - } - const logger = config.getLogger('/middleware/rbac-middleware.js'); logger.info('Enabling RBAC'); @@ -45,7 +39,7 @@ const rbacMiddleware = (config: any, { accessService }: any): any => { const { featureName } = params; projectId = await featureToggleStore.getProjectId(featureName); } else if (permission === CREATE_FEATURE) { - projectId = req.body.project; + projectId = req.body.project || 'default'; } return accessService.hasPermission(user, permission, projectId); diff --git a/src/lib/options.js b/src/lib/options.js index a3a836f3d1..9fa52ed209 100644 --- a/src/lib/options.js +++ b/src/lib/options.js @@ -67,7 +67,6 @@ function defaultOptions() { unleashUrl: process.env.UNLEASH_URL || 'http://localhost:4242', serverMetrics: true, enableLegacyRoutes: false, // deprecated. Remove in v4, - extendedPermissions: false, // deprecated. Remove in v4, publicFolder, versionCheck: { url: @@ -101,7 +100,6 @@ function defaultOptions() { enabled: process.env.CLIENT_FEATURE_MEMOIZE || false, maxAge: process.env.CLIENT_FEATURE_MAXAGE || 1000, }, - rbac: false, }, email: { host: process.env.EMAIL_HOST, diff --git a/src/lib/routes/admin-api/api-token-controller.ts b/src/lib/routes/admin-api/api-token-controller.ts index e19eba6e9c..002927df03 100644 --- a/src/lib/routes/admin-api/api-token-controller.ts +++ b/src/lib/routes/admin-api/api-token-controller.ts @@ -8,11 +8,10 @@ import { UPDATE_API_TOKEN, } from '../../permissions'; import { ApiTokenService } from '../../services/api-token-service'; -import { Logger, LogProvider } from '../../logger'; +import { Logger } from '../../logger'; import { ApiTokenType } from '../../db/api-token-store'; import { AccessService } from '../../services/access-service'; import { IAuthRequest } from '../unleash-types'; -import { isRbacEnabled } from '../../util/feature-enabled'; import User from '../../user'; import { IUnleashConfig } from '../../types/core'; @@ -26,18 +25,12 @@ class ApiTokenController extends Controller { private accessService: AccessService; - private extendedPermissions: boolean; - - private isRbacEnabled: boolean; - private logger: Logger; constructor(config: IUnleashConfig, services: IServices) { super(config); this.apiTokenService = services.apiTokenService; this.accessService = services.accessService; - this.extendedPermissions = config.extendedPermissions; - this.isRbacEnabled = isRbacEnabled(config); this.logger = config.getLogger('api-token-controller.js'); this.get('/', this.getAllApiTokens); @@ -46,21 +39,17 @@ class ApiTokenController extends Controller { this.delete('/:token', this.deleteApiToken, DELETE_API_TOKEN); } - private isTokenAdmin(user: User) { - if (this.isRbacEnabled) { - return this.accessService.hasPermission(user, UPDATE_API_TOKEN); + private async isTokenAdmin(user: User) { + if (user.isAPI) { + return user.permissions.includes(ADMIN); } - if (this.extendedPermissions) { - return user.permissions.some( - t => t === UPDATE_API_TOKEN || t === ADMIN, - ); - } - return true; + + return this.accessService.hasPermission(user, UPDATE_API_TOKEN); } async getAllApiTokens(req: IAuthRequest, res: Response): Promise { const { user } = req; - const isAdmin = this.isTokenAdmin(user); + const isAdmin = await this.isTokenAdmin(user); const tokens = await this.apiTokenService.getAllTokens(); diff --git a/src/lib/routes/admin-api/archive.test.js b/src/lib/routes/admin-api/archive.test.js index 52d1e8a7c2..f82a096f8e 100644 --- a/src/lib/routes/admin-api/archive.test.js +++ b/src/lib/routes/admin-api/archive.test.js @@ -8,7 +8,6 @@ const permissions = require('../../../test/fixtures/permissions'); const getLogger = require('../../../test/fixtures/no-logger'); const getApp = require('../../app'); const { createServices } = require('../../services'); -const { UPDATE_FEATURE } = require('../../permissions'); const eventBus = new EventEmitter(); @@ -20,8 +19,7 @@ function getSetup() { baseUriPath: base, stores, eventBus, - extendedPermissions: true, - preRouterHook: perms.hook, + preHook: perms.hook, getLogger, }; const services = createServices(stores, config); @@ -72,8 +70,7 @@ test('should get archived toggles via admin', t => { test('should revive toggle', t => { t.plan(0); const name = 'name1'; - const { request, base, archiveStore, perms } = getSetup(); - perms.withPermissions(UPDATE_FEATURE); + const { request, base, archiveStore } = getSetup(); archiveStore.addArchivedFeature({ name, strategies: [{ name: 'default' }], @@ -88,14 +85,7 @@ test('should revive toggle', t => { test('should create event when reviving toggle', async t => { t.plan(6); const name = 'name1'; - const { - request, - base, - featureToggleService, - eventStore, - perms, - } = getSetup(); - perms.withPermissions(UPDATE_FEATURE); + const { request, base, featureToggleService, eventStore } = getSetup(); await featureToggleService.addArchivedFeature({ name, diff --git a/src/lib/routes/admin-api/config.test.js b/src/lib/routes/admin-api/config.test.js index 1b84d0de8e..c7b50fa337 100644 --- a/src/lib/routes/admin-api/config.test.js +++ b/src/lib/routes/admin-api/config.test.js @@ -21,7 +21,6 @@ function getSetup() { baseUriPath: base, stores, eventBus, - extendedPermissions: false, ui: uiConfig, getLogger, }); diff --git a/src/lib/routes/admin-api/context.test.js b/src/lib/routes/admin-api/context.test.js index 608959e647..b85344d9be 100644 --- a/src/lib/routes/admin-api/context.test.js +++ b/src/lib/routes/admin-api/context.test.js @@ -6,6 +6,7 @@ const { EventEmitter } = require('events'); const store = require('../../../test/fixtures/store'); const { createServices } = require('../../services'); const getLogger = require('../../../test/fixtures/no-logger'); +const permissions = require('../../../test/fixtures/permissions'); const getApp = require('../../app'); const eventBus = new EventEmitter(); @@ -13,11 +14,12 @@ const eventBus = new EventEmitter(); function getSetup() { const base = `/random${Math.round(Math.random() * 1000)}`; const stores = store.createStores(); + const perms = permissions(); const config = { baseUriPath: base, stores, eventBus, - extendedPermissions: false, + preHook: perms.hook, customContextFields: [{ name: 'tenantId' }], getLogger, }; diff --git a/src/lib/routes/admin-api/email.test.js b/src/lib/routes/admin-api/email.test.js index f11624f112..dc772475f8 100644 --- a/src/lib/routes/admin-api/email.test.js +++ b/src/lib/routes/admin-api/email.test.js @@ -6,6 +6,7 @@ const { EventEmitter } = require('events'); const store = require('../../../test/fixtures/store'); const { createServices } = require('../../services'); const getLogger = require('../../../test/fixtures/no-logger'); +const permissions = require('../../../test/fixtures/permissions'); const getApp = require('../../app'); const eventBus = new EventEmitter(); @@ -13,11 +14,12 @@ const eventBus = new EventEmitter(); function getSetup() { const base = `/random${Math.round(Math.random() * 1000)}`; const stores = store.createStores(); + const perms = permissions(); const config = { baseUriPath: base, stores, eventBus, - extendedPermissions: false, + preHook: perms.hook, customContextFields: [{ name: 'tenantId' }], getLogger, }; diff --git a/src/lib/routes/admin-api/feature.test.js b/src/lib/routes/admin-api/feature.test.js index 9879fe9be9..833888930a 100644 --- a/src/lib/routes/admin-api/feature.test.js +++ b/src/lib/routes/admin-api/feature.test.js @@ -8,11 +8,6 @@ const { createServices } = require('../../services'); const permissions = require('../../../test/fixtures/permissions'); const getLogger = require('../../../test/fixtures/no-logger'); const getApp = require('../../app'); -const { - UPDATE_FEATURE, - CREATE_FEATURE, - DELETE_FEATURE, -} = require('../../permissions'); const eventBus = new EventEmitter(); @@ -24,8 +19,7 @@ function getSetup(databaseIsUp = true) { baseUriPath: base, stores, eventBus, - extendedPermissions: true, - preRouterHook: perms.hook, + preHook: perms.hook, getLogger, }; const services = createServices(stores, config); @@ -88,8 +82,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.withPermissions(CREATE_FEATURE); + const { request, base } = getSetup(); return request .post(`${base}/api/admin/features`) @@ -100,8 +93,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.withPermissions(CREATE_FEATURE); + const { request, base } = getSetup(); return request .post(`${base}/api/admin/features/validate`) @@ -112,8 +104,7 @@ test('should be allowed to use new toggle name', t => { test('should get unsupported media-type when posting as form-url-encoded', t => { t.plan(0); - const { request, base, perms } = getSetup(); - perms.withPermissions(CREATE_FEATURE); + const { request, base } = getSetup(); return request .post(`${base}/api/admin/features`) @@ -126,8 +117,7 @@ test('should get unsupported media-type when posting as form-url-encoded', t => test('should be allowed to have variants="null"', t => { t.plan(0); - const { request, base, perms } = getSetup(); - perms.withPermissions(CREATE_FEATURE); + const { request, base } = getSetup(); return request .post(`${base}/api/admin/features`) @@ -185,8 +175,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.withPermissions(UPDATE_FEATURE); + const { request, featureToggleStore, base } = getSetup(); featureToggleStore.createFeature({ name: 'ts', strategies: [{ name: 'default' }], @@ -201,8 +190,7 @@ test('should require at least one strategy when updating a feature toggle', t => test('updating a feature toggle also requires application/json as content-type', t => { t.plan(0); - const { request, featureToggleStore, base, perms } = getSetup(); - perms.withPermissions(UPDATE_FEATURE); + const { request, featureToggleStore, base } = getSetup(); featureToggleStore.createFeature({ name: 'ts', strategies: [{ name: 'default' }], @@ -218,8 +206,7 @@ test('updating a feature toggle also requires application/json as content-type', test('valid feature names should pass validation', t => { t.plan(0); - const { request, base, perms } = getSetup(); - perms.withPermissions(CREATE_FEATURE); + const { request, base } = getSetup(); const validNames = [ 'com.example', @@ -247,8 +234,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.withPermissions(CREATE_FEATURE); + const { request, base } = getSetup(); const invalidNames = [ 'some example', @@ -276,8 +262,7 @@ test('invalid feature names should not pass validation', t => { // Make sure current UI works. Should align on joi errors in future. test('invalid feature names should have error msg', t => { t.plan(1); - const { request, base, perms } = getSetup(); - perms.withPermissions(CREATE_FEATURE); + const { request, base } = getSetup(); const name = 'ØÆ`'; @@ -299,8 +284,7 @@ test('invalid feature names should have error msg', t => { test('should not allow variants with same name when creating feature flag', t => { t.plan(0); - const { request, base, perms } = getSetup(); - perms.withPermissions(CREATE_FEATURE); + const { request, base } = getSetup(); return request .post(`${base}/api/admin/features`) @@ -319,8 +303,7 @@ test('should not allow variants with same name when creating feature flag', t => test('should not allow variants with same name when updating feature flag', t => { t.plan(0); - const { request, featureToggleStore, base, perms } = getSetup(); - perms.withPermissions(UPDATE_FEATURE); + const { request, featureToggleStore, base } = getSetup(); featureToggleStore.createFeature({ name: 'ts', @@ -340,8 +323,7 @@ test('should not allow variants with same name when updating feature flag', t => test('should toggle on', t => { t.plan(1); - const { request, featureToggleStore, base, perms } = getSetup(); - perms.withPermissions(UPDATE_FEATURE); + const { request, featureToggleStore, base } = getSetup(); featureToggleStore.createFeature({ name: 'toggle.disabled', @@ -361,8 +343,7 @@ test('should toggle on', t => { test('should toggle off', t => { t.plan(1); - const { request, featureToggleStore, base, perms } = getSetup(); - perms.withPermissions(UPDATE_FEATURE); + const { request, featureToggleStore, base } = getSetup(); featureToggleStore.createFeature({ name: 'toggle.enabled', @@ -382,8 +363,7 @@ test('should toggle off', t => { test('should toggle', t => { t.plan(1); - const { request, featureToggleStore, base, perms } = getSetup(); - perms.withPermissions(UPDATE_FEATURE); + const { request, featureToggleStore, base } = getSetup(); featureToggleStore.createFeature({ name: 'toggle.disabled', @@ -403,8 +383,7 @@ test('should toggle', t => { test('should be able to add tag for feature', t => { t.plan(0); - const { request, featureToggleStore, base, perms } = getSetup(); - perms.withPermissions(UPDATE_FEATURE); + const { request, featureToggleStore, base } = getSetup(); featureToggleStore.createFeature({ name: 'toggle.disabled', enabled: false, @@ -421,8 +400,7 @@ test('should be able to add tag for feature', t => { }); test('should be able to get tags for feature', t => { t.plan(1); - const { request, featureToggleStore, base, perms } = getSetup(); - perms.withPermissions(UPDATE_FEATURE); + const { request, featureToggleStore, base } = getSetup(); featureToggleStore.createFeature({ name: 'toggle.disabled', @@ -445,8 +423,7 @@ test('should be able to get tags for feature', t => { test('Invalid tag for feature should be rejected', t => { t.plan(1); - const { request, featureToggleStore, base, perms } = getSetup(); - perms.withPermissions(UPDATE_FEATURE); + const { request, featureToggleStore, base } = getSetup(); featureToggleStore.createFeature({ name: 'toggle.disabled', @@ -469,8 +446,7 @@ test('Invalid tag for feature should be rejected', t => { test('Should be able to filter on tag', t => { t.plan(2); - const { request, featureToggleStore, base, perms } = getSetup(); - perms.withPermissions(UPDATE_FEATURE); + const { request, featureToggleStore, base } = getSetup(); featureToggleStore.createFeature({ name: 'toggle.tagged', @@ -499,8 +475,7 @@ test('Should be able to filter on tag', t => { test('Should be able to filter on name prefix', t => { t.plan(3); - const { request, featureToggleStore, base, perms } = getSetup(); - perms.withPermissions(UPDATE_FEATURE); + const { request, featureToggleStore, base } = getSetup(); featureToggleStore.createFeature({ name: 'a_team.toggle', @@ -531,8 +506,7 @@ test('Should be able to filter on name prefix', t => { test('Should be able to filter on project', t => { t.plan(3); - const { request, featureToggleStore, base, perms } = getSetup(); - perms.withPermissions(UPDATE_FEATURE); + const { request, featureToggleStore, base } = getSetup(); featureToggleStore.createFeature({ name: 'a_team.toggle', @@ -564,8 +538,7 @@ test('Should be able to filter on project', t => { }); test('Tags should be included in archive events', async t => { - const { request, eventStore, featureToggleStore, base, perms } = getSetup(); - perms.withPermissions(UPDATE_FEATURE, DELETE_FEATURE); + const { request, eventStore, featureToggleStore, base } = getSetup(); featureToggleStore.createFeature({ name: 'a_team.toggle', @@ -587,8 +560,7 @@ test('Tags should be included in archive events', async t => { }); test('Tags should be included in updated events', async t => { - const { request, eventStore, featureToggleStore, base, perms } = getSetup(); - perms.withPermissions(UPDATE_FEATURE, DELETE_FEATURE); + const { request, eventStore, featureToggleStore, base } = getSetup(); featureToggleStore.createFeature({ name: 'a_team.toggle', @@ -625,8 +597,7 @@ test('Trying to get features while database is down should yield 500', t => { test('should mark toggle as stale', t => { t.plan(1); const toggleName = 'toggle-stale'; - const { request, featureToggleStore, base, perms } = getSetup(); - perms.withPermissions(UPDATE_FEATURE, DELETE_FEATURE); + const { request, featureToggleStore, base } = getSetup(); featureToggleStore.createFeature({ name: toggleName, strategies: [{ name: 'default' }], @@ -644,8 +615,7 @@ test('should mark toggle as stale', t => { test('should mark toggle as NOT stale', t => { t.plan(1); const toggleName = 'toggle-stale'; - const { request, featureToggleStore, base, perms } = getSetup(); - perms.withPermissions(UPDATE_FEATURE, DELETE_FEATURE); + const { request, featureToggleStore, base } = getSetup(); featureToggleStore.createFeature({ name: toggleName, strategies: [{ name: 'default' }], diff --git a/src/lib/routes/admin-api/metrics.test.js b/src/lib/routes/admin-api/metrics.test.js index 47f1a311b1..94222256bc 100644 --- a/src/lib/routes/admin-api/metrics.test.js +++ b/src/lib/routes/admin-api/metrics.test.js @@ -7,7 +7,6 @@ const store = require('../../../test/fixtures/store'); const permissions = require('../../../test/fixtures/permissions'); const getLogger = require('../../../test/fixtures/no-logger'); const getApp = require('../../app'); -const { UPDATE_APPLICATION } = require('../../permissions'); const { createServices } = require('../../services'); const eventBus = new EventEmitter(); @@ -18,7 +17,6 @@ function getSetup() { const config = { baseUriPath: '', eventBus, - extendedPermissions: true, preRouterHook: perms.hook, getLogger, }; @@ -135,9 +133,8 @@ test('should return applications', t => { test('should store application', t => { t.plan(0); - const { request, perms } = getSetup(); + const { request } = getSetup(); const appName = '123!23'; - perms.withPermissions(UPDATE_APPLICATION); return request .post(`/api/admin/metrics/applications/${appName}`) @@ -147,9 +144,8 @@ test('should store application', t => { test('should store application details wihtout strategies', t => { t.plan(0); - const { request, perms } = getSetup(); + const { request } = getSetup(); const appName = '123!23'; - perms.withPermissions(UPDATE_APPLICATION); return request .post(`/api/admin/metrics/applications/${appName}`) @@ -159,9 +155,8 @@ test('should store application details wihtout strategies', t => { test('should accept a delete call to unknown application', t => { t.plan(0); - const { request, perms } = getSetup(); + const { request } = getSetup(); const appName = 'unknown'; - perms.withPermissions(UPDATE_APPLICATION); return request .delete(`/api/admin/metrics/applications/${appName}`) @@ -170,10 +165,9 @@ test('should accept a delete call to unknown application', t => { test('should delete application', t => { t.plan(0); - const { request, stores, perms } = getSetup(); + const { request, stores } = getSetup(); const appName = 'deletable-test'; - perms.withPermissions(UPDATE_APPLICATION); stores.clientApplicationsStore.upsert({ appName }); return request diff --git a/src/lib/routes/admin-api/strategy.test.js b/src/lib/routes/admin-api/strategy.test.js index 39bb9eb46c..c21c04f8ec 100644 --- a/src/lib/routes/admin-api/strategy.test.js +++ b/src/lib/routes/admin-api/strategy.test.js @@ -8,11 +8,6 @@ const permissions = require('../../../test/fixtures/permissions'); const getLogger = require('../../../test/fixtures/no-logger'); const getApp = require('../../app'); const { createServices } = require('../../services'); -const { - DELETE_STRATEGY, - CREATE_STRATEGY, - UPDATE_STRATEGY, -} = require('../../permissions'); const eventBus = new EventEmitter(); @@ -25,7 +20,6 @@ function getSetup(databaseIsUp = true) { stores, eventBus, getLogger, - extendedPermissions: true, preRouterHook: perms.hook, }; const services = createServices(stores, config); @@ -58,8 +52,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.withPermissions(CREATE_STRATEGY); + const { request, base } = getSetup(); return request .post(`${base}/api/admin/strategies`) @@ -72,8 +65,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.withPermissions(CREATE_STRATEGY); + const { request, base } = getSetup(); return request .post(`${base}/api/admin/strategies`) @@ -89,8 +81,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.withPermissions(CREATE_STRATEGY); + const { request, base } = getSetup(); return request .post(`${base}/api/admin/strategies`) @@ -100,8 +91,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.withPermissions(CREATE_STRATEGY); + const { request, base, strategyStore } = getSetup(); strategyStore.createStrategy({ name: 'Testing', parameters: [] }); return request @@ -113,8 +103,7 @@ test('not be possible to override name', t => { test('update strategy', t => { t.plan(0); const name = 'AnotherStrat'; - const { request, base, strategyStore, perms } = getSetup(); - perms.withPermissions(UPDATE_STRATEGY); + const { request, base, strategyStore } = getSetup(); strategyStore.createStrategy({ name, parameters: [] }); return request @@ -126,8 +115,7 @@ test('update strategy', t => { test('not update unknown strategy', t => { t.plan(0); const name = 'UnknownStrat'; - const { request, base, perms } = getSetup(); - perms.withPermissions(UPDATE_STRATEGY); + const { request, base } = getSetup(); return request .put(`${base}/api/admin/strategies/${name}`) @@ -138,8 +126,7 @@ test('not update unknown strategy', t => { test('validate format when updating strategy', t => { t.plan(0); const name = 'AnotherStrat'; - const { request, base, strategyStore, perms } = getSetup(); - perms.withPermissions(UPDATE_STRATEGY); + const { request, base, strategyStore } = getSetup(); strategyStore.createStrategy({ name, parameters: [] }); return request @@ -152,8 +139,7 @@ test('editable=false will stop delete request', t => { getLogger.setMuteError(true); t.plan(0); const name = 'default'; - const { request, base, perms } = getSetup(); - perms.withPermissions(DELETE_STRATEGY); + const { request, base } = getSetup(); return request.delete(`${base}/api/admin/strategies/${name}`).expect(500); }); @@ -162,8 +148,7 @@ test('editable=false will stop edit request', t => { getLogger.setMuteError(true); t.plan(0); const name = 'default'; - const { request, base, perms } = getSetup(); - perms.withPermissions(UPDATE_STRATEGY); + const { request, base } = getSetup(); return request .put(`${base}/api/admin/strategies/${name}`) @@ -174,8 +159,7 @@ test('editable=false will stop edit request', t => { test('editable=true will allow delete request', t => { t.plan(0); const name = 'deleteStrat'; - const { request, base, strategyStore, perms } = getSetup(); - perms.withPermissions(DELETE_STRATEGY); + const { request, base, strategyStore } = getSetup(); strategyStore.createStrategy({ name, parameters: [] }); return request @@ -187,8 +171,7 @@ test('editable=true will allow delete request', t => { test('editable=true will allow edit request', t => { t.plan(0); const name = 'editStrat'; - const { request, base, strategyStore, perms } = getSetup(); - perms.withPermissions(UPDATE_STRATEGY); + const { request, base, strategyStore } = getSetup(); strategyStore.createStrategy({ name, parameters: [] }); return request @@ -200,8 +183,7 @@ test('editable=true will allow edit request', t => { test('deprecating a strategy works', async t => { t.plan(1); const name = 'editStrat'; - const { request, base, strategyStore, perms } = getSetup(); - perms.withPermissions(UPDATE_STRATEGY); + const { request, base, strategyStore } = getSetup(); strategyStore.createStrategy({ name, parameters: [] }); await request @@ -217,8 +199,7 @@ test('deprecating a strategy works', async t => { test('deprecating a non-existent strategy yields 404', t => { t.plan(0); - const { request, base, perms } = getSetup(); - perms.withPermissions(UPDATE_STRATEGY); + const { request, base } = getSetup(); return request .post(`${base}/api/admin/strategies/non-existent-strategy/deprecate`) .set('Content-Type', 'application/json') @@ -228,8 +209,7 @@ test('deprecating a non-existent strategy yields 404', t => { test('reactivating a strategy works', async t => { t.plan(1); const name = 'editStrat'; - const { request, base, strategyStore, perms } = getSetup(); - perms.withPermissions(UPDATE_STRATEGY); + const { request, base, strategyStore } = getSetup(); strategyStore.createStrategy({ name, parameters: [] }); await request @@ -245,8 +225,7 @@ test('reactivating a strategy works', async t => { test('reactivating a non-existent strategy yields 404', t => { t.plan(0); - const { request, base, perms } = getSetup(); - perms.withPermissions(UPDATE_STRATEGY); + const { request, base } = getSetup(); return request .post(`${base}/api/admin/strategies/non-existent-strategy/reactivate`) .set('Content-Type', 'application/json') @@ -254,8 +233,7 @@ test('reactivating a non-existent strategy yields 404', t => { }); test("deprecating 'default' strategy will yield 403", t => { t.plan(0); - const { request, base, perms } = getSetup(); - perms.withPermissions(UPDATE_STRATEGY); + const { request, base } = getSetup(); return request .post(`${base}/api/admin/strategies/default/deprecate`) .set('Content-Type', 'application/json') diff --git a/src/lib/routes/admin-api/tag.test.js b/src/lib/routes/admin-api/tag.test.js index f7963e92aa..bff26d140d 100644 --- a/src/lib/routes/admin-api/tag.test.js +++ b/src/lib/routes/admin-api/tag.test.js @@ -8,7 +8,6 @@ const permissions = require('../../../test/fixtures/permissions'); const getLogger = require('../../../test/fixtures/no-logger'); const getApp = require('../../app'); const { createServices } = require('../../services'); -const { UPDATE_FEATURE } = require('../../permissions'); const eventBus = new EventEmitter(); @@ -20,7 +19,6 @@ function getSetup(databaseIsUp = true) { baseUriPath: base, stores, eventBus, - extendedPermissions: true, preRouterHook: perms.hook, getLogger, }; @@ -85,8 +83,7 @@ test('trying to get non-existing tag by name and type should not be found', t => }); test('should be able to delete a tag', t => { t.plan(0); - const { request, base, tagStore, perms } = getSetup(); - perms.withPermissions(UPDATE_FEATURE); + const { request, base, tagStore } = getSetup(); tagStore.createTag({ type: 'simple', value: 'TeamRed' }); return request .delete(`${base}/api/admin/tags/simple/TeamGreen`) diff --git a/src/lib/routes/admin-api/user.js b/src/lib/routes/admin-api/user.js index 6da6eaac80..3b2ee56361 100644 --- a/src/lib/routes/admin-api/user.js +++ b/src/lib/routes/admin-api/user.js @@ -13,11 +13,7 @@ class UserController extends Controller { getUser(req, res) { if (req.user) { const user = { ...req.user }; - if (!this.config.extendedPermissions) { - delete user.permissions; - } else if (!Array.isArray(user.permissions)) { - user.permissions = []; - } + delete user.permissions; // TODO: remove return res .status(200) .json(user) diff --git a/src/lib/routes/controller.js b/src/lib/routes/controller.js index ab839af1de..ecd05de4ad 100644 --- a/src/lib/routes/controller.js +++ b/src/lib/routes/controller.js @@ -1,8 +1,24 @@ 'use strict'; const { Router } = require('express'); -const checkPermission = require('../middleware/permission-checker'); +const NoAccessError = require('../error/no-access-error'); const requireContentType = require('../middleware/content_type_checker'); + +const checkPermission = permission => { + return async (req, res, next) => { + if (!permission) { + return next(); + } + if (await req.checkRbac(permission)) { + return next(); + } + return res + .status(403) + .json(new NoAccessError(permission)) + .end(); + }; +}; + /** * Base class for Controllers to standardize binding to express Router. */ @@ -14,17 +30,13 @@ class Controller { } get(path, handler, permission) { - this.app.get( - path, - checkPermission(this.config, permission), - handler.bind(this), - ); + this.app.get(path, checkPermission(permission), handler.bind(this)); } post(path, handler, permission, ...acceptedContentTypes) { this.app.post( path, - checkPermission(this.config, permission), + checkPermission(permission), requireContentType(...acceptedContentTypes), handler.bind(this), ); @@ -33,24 +45,20 @@ class Controller { put(path, handler, permission, ...acceptedContentTypes) { this.app.put( path, - checkPermission(this.config, permission), + checkPermission(permission), requireContentType(...acceptedContentTypes), handler.bind(this), ); } delete(path, handler, permission) { - this.app.delete( - path, - checkPermission(this.config, permission), - handler.bind(this), - ); + this.app.delete(path, checkPermission(permission), handler.bind(this)); } fileupload(path, filehandler, handler, permission) { this.app.post( path, - checkPermission(this.config, permission), + checkPermission(permission), filehandler, handler.bind(this), ); diff --git a/src/lib/services/access-service.ts b/src/lib/services/access-service.ts index d2b8145d7b..7a9adf66a9 100644 --- a/src/lib/services/access-service.ts +++ b/src/lib/services/access-service.ts @@ -97,7 +97,7 @@ export class AccessService { * @param projectId */ async hasPermission(user: User, permission: string, projectId?: string): Promise { - this.logger.info(`Checking permission=${permission}, userId=${user.id} projectId=${projectId}`) + this.logger.info(`Checking permission=${permission}, userId=${user.id} projectId=${projectId}`); const permissions = await this.store.getPermissionsForUser(user.id); diff --git a/src/lib/services/project-service.ts b/src/lib/services/project-service.ts index 25fa54817f..4f54c48ffc 100644 --- a/src/lib/services/project-service.ts +++ b/src/lib/services/project-service.ts @@ -1,6 +1,5 @@ import User from '../user'; import { AccessService, RoleName } from './access-service'; -import { isRbacEnabled } from '../util/feature-enabled'; const NameExistsError = require('../error/name-exists-error'); const InvalidOperationError = require('../error/invalid-operation-error'); @@ -30,8 +29,6 @@ class ProjectService { private logger: any; - private rbacEnabled: boolean; - constructor( { projectStore, eventStore, featureToggleStore }, config: any, @@ -42,7 +39,6 @@ class ProjectService { this.eventStore = eventStore; this.featureToggleStore = featureToggleStore; this.logger = config.getLogger('services/project-service.js'); - this.rbacEnabled = isRbacEnabled(config); } async getProjects() { @@ -59,9 +55,7 @@ class ProjectService { await this.projectStore.create(data); - if (this.rbacEnabled) { - await this.accessService.createDefaultProjectRoles(user, data.id); - } + await this.accessService.createDefaultProjectRoles(user, data.id); await this.eventStore.store({ type: eventType.PROJECT_CREATED, @@ -111,9 +105,7 @@ class ProjectService { data: { id }, }); - if (this.rbacEnabled) { - this.accessService.removeDefaultProjectRoles(user, id); - } + this.accessService.removeDefaultProjectRoles(user, id); } async validateId(id: string): Promise { diff --git a/src/lib/types/core.ts b/src/lib/types/core.ts index b099a56ebd..ca6db1903a 100644 --- a/src/lib/types/core.ts +++ b/src/lib/types/core.ts @@ -7,7 +7,6 @@ interface IExperimentalFlags { export interface IUnleashConfig { getLogger: LogProvider; baseUriPath: string; - extendedPermissions?: boolean; experimental?: IExperimentalFlags; authentication: { enableApiToken: boolean; diff --git a/src/lib/util/feature-enabled.ts b/src/lib/util/feature-enabled.ts deleted file mode 100644 index 035cd8bc5f..0000000000 --- a/src/lib/util/feature-enabled.ts +++ /dev/null @@ -1,7 +0,0 @@ -import { IUnleashConfig } from '../types/core'; - -export const isRbacEnabled = (config: IUnleashConfig): boolean => { - return config && config.experimental && config.experimental.rbac; -}; - -module.exports = { isRbacEnabled }; diff --git a/src/test/e2e/api/admin/api-token.e2e.test.ts b/src/test/e2e/api/admin/api-token.e2e.test.ts index a9be9cca7d..2af5ac6935 100644 --- a/src/test/e2e/api/admin/api-token.e2e.test.ts +++ b/src/test/e2e/api/admin/api-token.e2e.test.ts @@ -5,6 +5,7 @@ import getLogger from '../../../fixtures/no-logger'; import { ApiTokenType, IApiToken } from '../../../../lib/db/api-token-store'; import User from '../../../../lib/user'; import { CREATE_API_TOKEN, CREATE_FEATURE } from '../../../../lib/permissions'; +import { RoleName } from '../../../../lib/services/access-service'; let stores; let db; @@ -184,16 +185,22 @@ test.serial('removes api token', async t => { test.serial('none-admins should only get client tokens', async t => { t.plan(2); - const user = new User({ email: 'custom-user@mail.com', permissions: [] }); - const preHook = app => { - app.use('/api/', (req, res, next) => { + const email = 'custom-user@mail.com'; + + const preHook = (app, config, { userService, accessService }) => { + app.use('/api/admin/', async (req, res, next) => { + const role = await accessService.getRootRole(RoleName.REGULAR); + const user = await userService.createUser({ + email, + rootRole: role.id, + }); req.user = user; next(); }); }; - const request = await setupAppWithCustomAuth(stores, preHook, true); + const request = await setupAppWithCustomAuth(stores, preHook); await stores.apiTokenStore.insert({ username: 'test', @@ -219,19 +226,21 @@ test.serial('none-admins should only get client tokens', async t => { test.serial('Only token-admins should be allowed to create token', async t => { t.plan(0); - const user = new User({ - email: 'custom-user@mail.com', - permissions: [CREATE_FEATURE], - }); - const preHook = app => { - app.use('/api/', (req, res, next) => { - req.user = user; + const email = 'custom-user2@mail.com'; + + const preHook = (app, config, { userService, accessService }) => { + app.use('/api/admin/', async (req, res, next) => { + const role = await accessService.getRootRole(RoleName.REGULAR); + req.user = await userService.createUser({ + email, + rootRole: role.id, + }); next(); }); }; - const request = await setupAppWithCustomAuth(stores, preHook, true); + const request = await setupAppWithCustomAuth(stores, preHook); return request .post('/api/admin/api-tokens') @@ -245,19 +254,20 @@ test.serial('Only token-admins should be allowed to create token', async t => { test.serial('Token-admin should be allowed to create token', async t => { t.plan(0); - const user = new User({ - email: 'custom-user@mail.com', - permissions: [CREATE_API_TOKEN], - }); + const email = 'custom-user3@mail.com'; - const preHook = app => { - app.use('/api/', (req, res, next) => { - req.user = user; + const preHook = (app, config, { userService, accessService }) => { + app.use('/api/admin/', async (req, res, next) => { + const role = await accessService.getRootRole(RoleName.ADMIN); + req.user = await userService.createUser({ + email, + rootRole: role.id, + }); next(); }); }; - const request = await setupAppWithCustomAuth(stores, preHook, true); + const request = await setupAppWithCustomAuth(stores, preHook); return request .post('/api/admin/api-tokens') diff --git a/src/test/e2e/api/admin/feature.custom-auth.e2e.test.js b/src/test/e2e/api/admin/feature.custom-auth.e2e.test.js index 83af1525cf..078e43cec1 100644 --- a/src/test/e2e/api/admin/feature.custom-auth.e2e.test.js +++ b/src/test/e2e/api/admin/feature.custom-auth.e2e.test.js @@ -3,7 +3,6 @@ const test = require('ava'); const { setupAppWithCustomAuth } = require('../../helpers/test-helper'); const AuthenticationRequired = require('../../../../lib/authentication-required'); -const User = require('../../../../lib/user'); const dbInit = require('../../helpers/database-init'); const getLogger = require('../../../fixtures/no-logger'); @@ -42,11 +41,11 @@ test.serial('should require authenticated user', async t => { test.serial('creates new feature toggle with createdBy', async t => { t.plan(1); - const user = new User({ email: 'custom-user@mail.com' }); + const email = 'custom-user@mail.com'; - const preHook = app => { - app.use('/api/admin/', (req, res, next) => { - req.user = user; + const preHook = (app, config, { userService }) => { + app.use('/api/admin/', async (req, res, next) => { + req.user = await userService.loginUserWithoutPassword(email, true); next(); }); }; @@ -63,6 +62,6 @@ test.serial('creates new feature toggle with createdBy', async t => { .expect(201); await request.get('/api/admin/events').expect(res => { - t.is(res.body.events[0].createdBy, user.email); + t.is(res.body.events[0].createdBy, email); }); }); diff --git a/src/test/e2e/helpers/test-helper.js b/src/test/e2e/helpers/test-helper.js index 19d74d8b17..9f4012679f 100644 --- a/src/test/e2e/helpers/test-helper.js +++ b/src/test/e2e/helpers/test-helper.js @@ -11,18 +11,12 @@ const { createServices } = require('../../../lib/services'); const eventBus = new EventEmitter(); -function createApp( - stores, - adminAuthentication = 'none', - preHook, - extendedPermissions = false, -) { +function createApp(stores, adminAuthentication = 'none', preHook) { const config = { stores, eventBus, preHook, adminAuthentication, - extendedPermissions, secret: 'super-secret', session: { db: true, @@ -49,8 +43,8 @@ module.exports = { return supertest.agent(app); }, - async setupAppWithCustomAuth(stores, preHook, extendedPermissions) { - const app = createApp(stores, 'custom', preHook, extendedPermissions); + async setupAppWithCustomAuth(stores, preHook) { + const app = createApp(stores, 'custom', preHook); return supertest.agent(app); }, }; diff --git a/src/test/fixtures/permissions.js b/src/test/fixtures/permissions.js index 550c8bebed..a0adcdd5d0 100644 --- a/src/test/fixtures/permissions.js +++ b/src/test/fixtures/permissions.js @@ -1,17 +1,18 @@ 'use strict'; module.exports = () => { - let _perms = []; + const _perms = ['ADMIN']; return { hook(app) { app.use((req, res, next) => { - if (req.user) req.user.permissions = _perms; - else req.user = { email: 'unknown', permissions: _perms }; + req.user = { + isAPI: true, + id: 1, + email: 'unknown', + permissions: _perms, + }; next(); }); }, - withPermissions(...perms) { - _perms = perms; - }, }; };