From 4bc14d2c4bce00577d94c6316cbb346d49d8e1f7 Mon Sep 17 00:00:00 2001 From: Naz Date: Wed, 1 Jun 2022 17:47:57 +0800 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20invalid=20user=20role=20?= =?UTF-8?q?assignment?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit closes https://github.com/TryGhost/Toolbox/issues/351 - When an invalid value was passed in `roles` parameter when editing a user it resulted in incorrect database state (all roles appeared to be unassigned from the user). - The fix includes ability to set user role by an allowed name, one of: 'Administrator', 'Editor', 'Author', 'Contributor'. - Also added a validation in case a non-ObjectID value is passed in roles to the users edit method. --- core/server/models/user.js | 34 ++++++++++++++++++++-- test/e2e-api/admin/users.test.js | 49 ++++++++++++++++++++++++++++++-- 2 files changed, 78 insertions(+), 5 deletions(-) diff --git a/core/server/models/user.js b/core/server/models/user.js index a6d86339db..19e100bfe6 100644 --- a/core/server/models/user.js +++ b/core/server/models/user.js @@ -13,11 +13,17 @@ const validatePassword = require('../lib/validate-password'); const permissions = require('../services/permissions'); const urlUtils = require('../../shared/url-utils'); const activeStates = ['active', 'warn-1', 'warn-2', 'warn-3', 'warn-4']; +const ASSIGNABLE_ROLES = ['Administrator', 'Editor', 'Author', 'Contributor']; const messages = { valueCannotBeBlank: 'Value in [{tableName}.{columnKey}] cannot be blank.', onlyOneRolePerUserSupported: 'Only one role per user is supported at the moment.', methodDoesNotSupportOwnerRole: 'This method does not support assigning the owner role', + invalidRoleValue: { + message: 'Role should be an existing role id or a role name', + context: 'Invalid role assigned to the user', + help: 'Change the provided role to a role id or use a role name.' + }, userNotFound: 'User not found', ownerNotFound: 'Owner not found', notEnoughPermission: 'You do not have permission to perform this action', @@ -507,7 +513,7 @@ User = ghostBookshelf.Model.extend({ } ops.push(function update() { - return ghostBookshelf.Model.edit.call(self, data, options).then((user) => { + return ghostBookshelf.Model.edit.call(self, data, options).then(async (user) => { let roleId; if (!data.roles || !data.roles.length) { @@ -521,7 +527,29 @@ User = ghostBookshelf.Model.extend({ if (roles.models[0].id === roleId) { return; } - return ghostBookshelf.model('Role').findOne({id: roleId}); + + if (ASSIGNABLE_ROLES.includes(roleId)) { + // return if the role is already assigned + if (roles.models[0].get('name') === roleId) { + return; + } + + return ghostBookshelf.model('Role').findOne({ + name: roleId + }); + } else if (ObjectId.isValid(roleId)){ + return ghostBookshelf.model('Role').findOne({ + id: roleId + }); + } else { + return Promise.reject( + new errors.ValidationError({ + message: tpl(messages.invalidRoleValue.message), + context: tpl(messages.invalidRoleValue.context), + help: tpl(messages.invalidRoleValue.help) + }) + ); + } }).then((roleToAssign) => { if (roleToAssign && roleToAssign.get('name') === 'Owner') { return Promise.reject( @@ -531,7 +559,7 @@ User = ghostBookshelf.Model.extend({ ); } else { // assign all other roles - return user.roles().updatePivot({role_id: roleId}); + return user.roles().updatePivot({role_id: roleToAssign.id}); } }).then(() => { options.status = 'all'; diff --git a/test/e2e-api/admin/users.test.js b/test/e2e-api/admin/users.test.js index d850f2c269..215b95d3d2 100644 --- a/test/e2e-api/admin/users.test.js +++ b/test/e2e-api/admin/users.test.js @@ -185,8 +185,8 @@ describe('User API', function () { } }); - it('Can edit user with empty roles data', async function () { - await request.put(localUtils.API.getApiQuery('users/me')) + it('Can edit user with empty roles data and does not change the role', async function () { + const res = await request.put(localUtils.API.getApiQuery('users/me?include=roles')) .set('Origin', config.get('url')) .send({ users: [{ @@ -194,6 +194,51 @@ describe('User API', function () { }] }) .expect(200); + + const jsonResponse = res.body; + should.exist(jsonResponse.users); + jsonResponse.users.should.have.length(1); + + should.exist(jsonResponse.users[0].roles); + jsonResponse.users[0].roles.should.have.length(1); + jsonResponse.users[0].roles[0].name.should.equal('Owner'); + }); + + it('Cannot edit user with invalid roles data', async function () { + const userId = testUtils.getExistingData().users[1].id; + const res = await request.put(localUtils.API.getApiQuery(`users/${userId}?include=roles`)) + .set('Origin', config.get('url')) + .send({ + users: [{ + roles: ['Invalid Role Name'] + }] + }) + .expect(422); + + const jsonResponse = res.body; + should.exist(jsonResponse.errors); + jsonResponse.errors.should.have.length(1); + jsonResponse.errors[0].message.should.match(/cannot edit user/); + }); + + it('Can edit user roles by name', async function () { + const userId = testUtils.getExistingData().users[1].id; + const res = await request.put(localUtils.API.getApiQuery(`users/${userId}?include=roles`)) + .set('Origin', config.get('url')) + .send({ + users: [{ + roles: ['Administrator'] + }] + }) + .expect(200); + + const jsonResponse = res.body; + should.exist(jsonResponse.users); + jsonResponse.users.should.have.length(1); + + should.exist(jsonResponse.users[0].roles); + jsonResponse.users[0].roles.should.have.length(1); + jsonResponse.users[0].roles[0].name.should.equal('Administrator'); }); it('Can destroy an active user and transfer posts to the owner', async function () {