diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ee6855..4a977ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,11 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Fixed ### Removed +## [1.0.0-beta.3] - 2019-05-01 +### Added +- Add user adminPermissions property, intended for grant admin permissions to plugin users when login using apiKey +- Avoid any other user than admin updating adminPermissions property + ## [1.0.0-beta.2] - 2019-03-02 ### Added - Add web ui diff --git a/lib/api/users.js b/lib/api/users.js index 441797e..f6ead4b 100644 --- a/lib/api/users.js +++ b/lib/api/users.js @@ -53,12 +53,14 @@ const Operations = (service, commands) => { return commands.user.getById(params.path.id) .then(user => { if ( - // Only operators and admin users can be modified - [roles.ADMIN, roles.OPERATOR].includes(user.role) && - // Role can be only modified by administrators - (userData.role === roles.ADMIN || (params.path.id === userData._id && !body.role)) && + // Only operators and admin users can be modified, or plugins in case adminPermissions is the uniq property being modified + ([roles.ADMIN, roles.OPERATOR].includes(user.role) || (user.role === roles.PLUGIN && Object.keys(body).length === 1 && !isUndefined(body.adminPermissions))) && + // Role and adminPermissions can be only modified by administrators + (userData.role === roles.ADMIN || (params.path.id === userData._id && !body.role && !body.adminPermissions)) && // Role can be modified only to operator or admin roles - (!body.role || [roles.ADMIN, roles.OPERATOR].includes(body.role)) + (!body.role || [roles.ADMIN, roles.OPERATOR].includes(body.role)) && + // adminPermissions can be modified only to plugin roles + (!body.adminPermissions || user.role === roles.PLUGIN) ) { return Promise.resolve() } diff --git a/lib/api/users.json b/lib/api/users.json index dbf0608..1e2b38c 100644 --- a/lib/api/users.json +++ b/lib/api/users.json @@ -35,6 +35,10 @@ "updatedAt": { "description": "Last update date timestamp", "type": "string" + }, + "adminPermissions": { + "description": "Grant admin permissions to this user", + "type": "boolean" } }, "additionalProperties": false, @@ -98,6 +102,10 @@ "description": "Role assigned to the user", "type": "string", "enum": ["admin", "operator", "module", "plugin", "service-registerer"] + }, + "adminPermissions": { + "description": "Grant admin permissions to this user", + "type": "boolean" } }, "additionalProperties": false, diff --git a/lib/commands/securityToken.js b/lib/commands/securityToken.js index d6fbb78..3750994 100644 --- a/lib/commands/securityToken.js +++ b/lib/commands/securityToken.js @@ -15,11 +15,12 @@ const Commands = (service, models, client) => { return Promise.resolve(token) } - const add = (userData, type) => { + const add = (userData, type, creatorUser) => { const token = new models.SecurityToken({ token: randToken.generate(64), _user: userData._id, - type + type, + createdBy: creatorUser && creatorUser._id.toString() }) return token.save() .catch(error => utils.transformValidationErrors(error, service)) diff --git a/lib/commands/user.js b/lib/commands/user.js index 1d8e73f..a19b10a 100644 --- a/lib/commands/user.js +++ b/lib/commands/user.js @@ -4,7 +4,7 @@ const templates = require('../templates') const utils = require('../utils') const { INITIAL_ADMIN_USER, SERVICE_REGISTERER_USER, ANONYMOUS_USER } = require('../security/utils') -const PUBLIC_FIELDS = 'name email role updatedAt createdAt' +const PUBLIC_FIELDS = 'name email role updatedAt createdAt adminPermissions' const Commands = (service, models, client) => { const ensureUser = user => { diff --git a/lib/models/securityToken.js b/lib/models/securityToken.js index 09ed3c1..516b994 100644 --- a/lib/models/securityToken.js +++ b/lib/models/securityToken.js @@ -30,6 +30,9 @@ const Model = service => { securityUtils.API_KEY, securityUtils.JWT ] + }, + createdBy: { + type: String } }, { diff --git a/lib/models/user.js b/lib/models/user.js index 035d472..47f4967 100644 --- a/lib/models/user.js +++ b/lib/models/user.js @@ -52,6 +52,9 @@ const Model = service => { type: String, required: [true, templates.userRoleRequired()], enum: _.map(roles, role => role) + }, + adminPermissions: { + type: Boolean } }, { diff --git a/lib/security/apiKey.js b/lib/security/apiKey.js index 9771a58..bfd7fba 100644 --- a/lib/security/apiKey.js +++ b/lib/security/apiKey.js @@ -21,8 +21,8 @@ const Methods = (service, commands) => { return Promise.reject(new service.errors.Forbidden()) } - const authenticateHandler = (params, body) => commands.user.getById(body.user) - .then(user => commands.securityToken.add(user, utils.API_KEY) + const authenticateHandler = (params, body, res, userData) => commands.user.getById(body.user) + .then(user => commands.securityToken.add(user, utils.API_KEY, userData) .then(securityToken => Promise.resolve(securityToken.token)) ) diff --git a/lib/security/utils.js b/lib/security/utils.js index 29fb4f1..c602278 100644 --- a/lib/security/utils.js +++ b/lib/security/utils.js @@ -39,7 +39,12 @@ const cleanUserData = userData => ({ }) const GetUserBySecurityToken = commands => securityToken => commands.securityToken.getUser(securityToken) - .then(userData => Promise.resolve(cleanUserData(userData))) + .then(userData => { + if (userData.adminPermissions) { + userData.role = roles.ADMIN + } + return Promise.resolve(cleanUserData(userData)) + }) const GetAnonymousUser = commands => () => { if (anonymousUserPromise) { diff --git a/npm-shrinkwrap.json b/npm-shrinkwrap.json index 8ffbf65..15c0702 100644 --- a/npm-shrinkwrap.json +++ b/npm-shrinkwrap.json @@ -1,6 +1,6 @@ { "name": "domapic-controller", - "version": "1.0.0-beta.2", + "version": "1.0.0-beta.3", "lockfileVersion": 1, "requires": true, "dependencies": { @@ -1653,9 +1653,9 @@ } }, "domapic-controller-ui": { - "version": "1.0.0-beta.2", - "resolved": "https://registry.npmjs.org/domapic-controller-ui/-/domapic-controller-ui-1.0.0-beta.2.tgz", - "integrity": "sha512-gFxvSzO68O+1iE83IQ9I353jjd9+YBqSzFXcXM32r+CKaY62zIqfX7SklMLjM7h9qxkzG1gY2jAr7cmGySNK2w==", + "version": "1.0.0-beta.3", + "resolved": "https://registry.npmjs.org/domapic-controller-ui/-/domapic-controller-ui-1.0.0-beta.3.tgz", + "integrity": "sha512-bhnEUB2WFKVkx7YWPfEF9o84TrsXOm+y5JhtjHYWR3PE9Iep3QV23COzcUuZp3Tuc4fcFKQxr1KESUrHe7Z9Cg==", "requires": { "validator": "^10.11.0" } diff --git a/package.json b/package.json index 34cb4b4..28789c9 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "domapic-controller", - "version": "1.0.0-beta.2", + "version": "1.0.0-beta.3", "description": "Controller for Domapic systems", "main": "server.js", "keywords": [ @@ -24,7 +24,7 @@ }, "dependencies": { "domapic-base": "1.0.0-beta.20", - "domapic-controller-ui": "1.0.0-beta.2", + "domapic-controller-ui": "1.0.0-beta.3", "express-mongo-sanitize": "1.3.2", "inquirer": "6.2.1", "inquirer-autocomplete-prompt": "1.0.1", diff --git a/sonar-project.properties b/sonar-project.properties index 1546423..c6136db 100644 --- a/sonar-project.properties +++ b/sonar-project.properties @@ -1,6 +1,6 @@ sonar.organization=domapic sonar.projectKey=domapic-controller -sonar.projectVersion=1.0.0-beta.2 +sonar.projectVersion=1.0.0-beta.3 sonar.sources=. sonar.exclusions=node_modules/** diff --git a/test/functional/specs/users-api.specs.js b/test/functional/specs/users-api.specs.js index effd54f..6b02b99 100644 --- a/test/functional/specs/users-api.specs.js +++ b/test/functional/specs/users-api.specs.js @@ -8,6 +8,7 @@ test.describe('users api', function () { let authenticator = utils.Authenticator() let adminUserId let pluginId + let pluginUserId let entityId let operatorUserId @@ -72,6 +73,16 @@ test.describe('users api', function () { }) } + const getApiKey = user => { + return utils.request('/auth/apikey', { + method: 'POST', + body: { + user + }, + ...authenticator.credentials() + }) + } + const getAbilities = function (filters) { return utils.request('/abilities', { method: 'GET', @@ -471,7 +482,6 @@ test.describe('users api', function () { test.describe('update user', () => { let moduleUserId - let pluginUserId test.before(() => { return utils.doLogin(authenticator) @@ -536,6 +546,51 @@ test.describe('users api', function () { ]) }) }) + + test.it('should be able to update adminPermissions of plugin users', () => { + return getUsers().then(usersResponse => { + return Promise.resolve(usersResponse.body.find(userData => userData.name === 'foo-plugin')._id) + }).then(pluginId => { + return updateUser(pluginId, { + adminPermissions: true + }).then(response => { + return test.expect(response.statusCode).to.equal(204) + }) + }) + }) + }) + + test.describe('when user has role "plugin" with adminPermissions checked and is logged using api key', () => { + let pluginUserId + let pluginApiKey + + test.before(() => { + return utils.ensureUserAndDoLogin(authenticator, pluginUser).then(() => { + return getUserMe().then(response => { + pluginUserId = response.body._id + return getApiKey(pluginUserId).then(response => { + pluginApiKey = response.body.apiKey + authenticator.loginApiKey(response.body.name, pluginApiKey) + return Promise.resolve() + }) + }) + }) + }) + + test.after(() => { + return updateUser(pluginUserId, { + adminPermissions: false + }) + }) + + test.it('should be able to update data of operator users, including role', () => { + return updateUser(operatorUserId, { + password: 'foo', + role: 'operator' + }).then(response => { + return test.expect(response.statusCode).to.equal(204) + }) + }) }) test.describe('when user is operator', () => { @@ -630,6 +685,17 @@ test.describe('users api', function () { return utils.ensureUserAndDoLogin(authenticator, pluginUser) }) + test.it('should not be able to update self data', () => { + return updateUser(pluginUserId, { + adminPermissions: true + }).then(response => { + return Promise.all([ + test.expect(response.body.message).to.contain('Not authorized'), + test.expect(response.statusCode).to.equal(403) + ]) + }) + }) + test.describe('add user', () => { test.it('should return 201 when adding a new user with role "operator"', () => { return utils.createUser(authenticator, newOperatorUser).then(response => { diff --git a/test/unit/lib/api/users.specs.js b/test/unit/lib/api/users.specs.js index 59d175e..ba3c9ff 100644 --- a/test/unit/lib/api/users.specs.js +++ b/test/unit/lib/api/users.specs.js @@ -450,9 +450,50 @@ test.describe('users api', () => { }) }) + test.it('should resolve if target user has plugin role and only adminPermissions are being updated', () => { + commandsMocks.stubs.user.getById.resolves({ + role: 'plugin' + }) + + return operations.updateUser.auth(fooUser, fooParams, { + adminPermissions: true + }).then(() => { + return test.expect(true).to.be.true() + }) + }) + + test.it('should reject if target user has plugin role and any other key plus adminPermissions is being updated', () => { + commandsMocks.stubs.user.getById.resolves({ + role: 'plugin' + }) + + return operations.updateUser.auth(fooUser, fooParams, { + adminPermissions: true, + name: 'foo' + }).then(() => { + return test.assert.fail() + }, err => { + return test.expect(err).to.be.an.instanceof(Error) + }) + }) + + test.it('should reject if target user has plugin role and any other key than adminPermissions is being updated', () => { + commandsMocks.stubs.user.getById.resolves({ + role: 'plugin' + }) + + return operations.updateUser.auth(fooUser, fooParams, { + name: 'foo' + }).then(() => { + return test.assert.fail() + }, err => { + return test.expect(err).to.be.an.instanceof(Error) + }) + }) + test.it('should reject if user is not admin and wants to update role', () => { commandsMocks.stubs.user.getById.resolves({ - role: 'operator' + role: 'plugin' }) return operations.updateUser.auth({ @@ -467,6 +508,39 @@ test.describe('users api', () => { }) }) + test.it('should reject if user is admin and wants to update adminPermissions of a non plugin user', () => { + commandsMocks.stubs.user.getById.resolves({ + role: 'operator' + }) + + return operations.updateUser.auth({ + fooUser + }, fooParams, { + adminPermissions: true + }).then(() => { + return test.assert.fail() + }, err => { + return test.expect(err).to.be.an.instanceof(Error) + }) + }) + + test.it('should reject if user is admin and wants to update adminPermissions', () => { + commandsMocks.stubs.user.getById.resolves({ + role: 'operator' + }) + + return operations.updateUser.auth({ + ...fooUser, + role: 'plugin' + }, fooParams, { + adminPermissions: true + }).then(() => { + return test.assert.fail() + }, err => { + return test.expect(err).to.be.an.instanceof(Error) + }) + }) + test.it('should reject if user is admin and wants to update to a role different to admin or operator', () => { commandsMocks.stubs.user.getById.resolves({ role: 'operator' diff --git a/test/unit/lib/commands/securityToken.specs.js b/test/unit/lib/commands/securityToken.specs.js index e65497d..fefb95d 100644 --- a/test/unit/lib/commands/securityToken.specs.js +++ b/test/unit/lib/commands/securityToken.specs.js @@ -41,6 +41,10 @@ test.describe('securityToken commands', () => { const fooUserData = { _id: fooUserId } + const fooCreatorId = 'foo-creator-id' + const fooCreatorData = { + _id: fooCreatorId + } test.it('should create and save a SecurityToken model with the received user data', () => { return commands.add(fooUserData, 'jwt') .then(() => { @@ -49,7 +53,24 @@ test.describe('securityToken commands', () => { test.expect(modelsMocks.stubs.SecurityToken).to.have.been.calledWith({ _user: fooUserId, token: 'foo-token', - type: 'jwt' + type: 'jwt', + createdBy: undefined + }), + test.expect(modelsMocks.stubs.securityToken.save).to.have.been.called() + ]) + }) + }) + + test.it('should add the user creator id to the securityToken', () => { + return commands.add(fooUserData, 'apiKey', fooCreatorData) + .then(() => { + return Promise.all([ + test.expect(randTokenStub).to.have.been.called(), + test.expect(modelsMocks.stubs.SecurityToken).to.have.been.calledWith({ + _user: fooUserId, + token: 'foo-token', + type: 'apiKey', + createdBy: fooCreatorId }), test.expect(modelsMocks.stubs.securityToken.save).to.have.been.called() ]) diff --git a/test/unit/lib/security/utils.specs.js b/test/unit/lib/security/utils.specs.js index 5e13452..2fa0397 100644 --- a/test/unit/lib/security/utils.specs.js +++ b/test/unit/lib/security/utils.specs.js @@ -58,6 +58,30 @@ test.describe('security utils', () => { ]) }) }) + + test.it('should override role to admin if user has adminPermissions property set to true', () => { + const fooToken = 'fooToken' + commandsMocks.stubs.securityToken.getUser.resolves({ + _id: 'fooId', + name: 'fooName', + email: 'fooEmail', + role: 'fooRole', + password: 'fooPassword', + adminPermissions: true + }) + return getUserBySecurityToken(fooToken) + .then(result => { + return Promise.all([ + test.expect(commandsMocks.stubs.securityToken.getUser).to.have.been.calledWith(fooToken), + test.expect(result).to.deep.equal({ + _id: 'fooId', + name: 'fooName', + email: 'fooEmail', + role: 'admin' + }) + ]) + }) + }) }) test.describe('AdminOrOwner instance', () => {