🐛 Fixed invalid user role assignment

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

@@ -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';

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 () {