From 0fc152b2adf6a06d77422867c6e0ca9499b20c80 Mon Sep 17 00:00:00 2001
From: Jason Williams <jaswilli@gmail.com>
Date: Tue, 20 Jan 2015 19:21:50 +0000
Subject: [PATCH] Fix up users API so admin role can edit owner

No Issue.
- Make sure that a user with the Admin role can edit the Owner.
- Add test for behavior.
---
 core/server/api/users.js                    | 49 ++++++++++++---------
 core/test/integration/api/api_users_spec.js | 24 +++++++++-
 2 files changed, 49 insertions(+), 24 deletions(-)

diff --git a/core/server/api/users.js b/core/server/api/users.js
index ff12469522..3e08ed16a3 100644
--- a/core/server/api/users.js
+++ b/core/server/api/users.js
@@ -150,35 +150,40 @@ users = {
 
             // Check permissions
             return canThis(options.context).edit.user(options.id).then(function () {
-                if (data.users[0].roles && data.users[0].roles[0]) {
-                    var role = data.users[0].roles[0],
-                        roleId = parseInt(role.id || role, 10);
+                // if roles aren't in the payload, proceed with the edit
+                if (!(data.users[0].roles && data.users[0].roles[0])) {
+                    return editOperation();
+                }
 
-                    return dataProvider.User.findOne(
-                        {id: options.context.user, status: 'all'}, {include: ['roles']}
-                    ).then(function (contextUser) {
-                        var contextRoleId = contextUser.related('roles').toJSON()[0].id;
+                var role = data.users[0].roles[0],
+                    roleId = parseInt(role.id || role, 10),
+                    editedUserId = parseInt(options.id, 10);
 
-                        if (roleId !== contextRoleId &&
-                                parseInt(options.id, 10) === parseInt(options.context.user, 10)) {
-                            return Promise.reject(new errors.NoPermissionError('You cannot change your own role.'));
-                        } else if (roleId !== contextRoleId) {
-                            return dataProvider.User.findOne({role: 'Owner'}).then(function (result) {
-                                if (parseInt(result.id, 10) !== parseInt(options.id, 10)) {
-                                    return canThis(options.context).assign.role(role).then(function () {
-                                        return editOperation();
-                                    });
-                                } else {
-                                    return Promise.reject(new errors.NoPermissionError('There has to be one owner.'));
+                return dataProvider.User.findOne(
+                    {id: options.context.user, status: 'all'}, {include: ['roles']}
+                ).then(function (contextUser) {
+                    var contextRoleId = contextUser.related('roles').toJSON()[0].id;
+
+                    if (roleId !== contextRoleId && editedUserId === contextUser.id) {
+                        return Promise.reject(new errors.NoPermissionError('You cannot change your own role.'));
+                    }
+
+                    return dataProvider.User.findOne({role: 'Owner'}).then(function (owner) {
+                        if (contextUser.id !== owner.id) {
+                            if (editedUserId === owner.id) {
+                                if (owner.related('roles').at(0).id !== roleId) {
+                                    return Promise.reject(new errors.NoPermissionError('Cannot change Owner\'s role.'));
                                 }
-                            });
+                            } else if (roleId !== contextRoleId) {
+                                return canThis(options.context).assign.role(role).then(function () {
+                                    return editOperation();
+                                });
+                            }
                         }
 
                         return editOperation();
                     });
-                }
-
-                return editOperation();
+                });
             });
         }).catch(function (error) {
             return errors.handleAPIError(error, 'You do not have permission to edit this user');
diff --git a/core/test/integration/api/api_users_spec.js b/core/test/integration/api/api_users_spec.js
index 48fd9814a0..5308cf0c2b 100644
--- a/core/test/integration/api/api_users_spec.js
+++ b/core/test/integration/api/api_users_spec.js
@@ -219,7 +219,7 @@ describe('Users API', function () {
                 }).catch(done);
         });
 
-        it('Admin can edit all roles', function (done) {
+        it('Admin can edit all users in all roles', function (done) {
             UserAPI.edit({users: [{name: newName}]}, _.extend({}, context.admin, {id: userIdFor.owner}))
                 .then(function (response) {
                     checkEditResponse(response);
@@ -239,6 +239,26 @@ describe('Users API', function () {
                 }).catch(done);
         });
 
+        it('Admin can edit all users in all roles with roles in payload', function (done) {
+            UserAPI.edit({users: [{name: newName, roles: [roleIdFor.owner]}]}, _.extend({}, context.admin, {id: userIdFor.owner}))
+                .then(function (response) {
+                    checkEditResponse(response);
+
+                    return UserAPI.edit({users: [{name: newName, roles: [roleIdFor.admin]}]}, _.extend({}, context.admin, {id: userIdFor.admin}));
+                }).then(function (response) {
+                    checkEditResponse(response);
+                    return UserAPI.edit({users: [{name: newName, roles: [roleIdFor.editor]}]}, _.extend({}, context.admin, {id: userIdFor.editor}));
+                }).then(function (response) {
+                    checkEditResponse(response);
+
+                    return UserAPI.edit({users: [{name: newName, roles: [roleIdFor.author]}]}, _.extend({}, context.admin, {id: userIdFor.author}));
+                }).then(function (response) {
+                    checkEditResponse(response);
+
+                    done();
+                }).catch(done);
+        });
+
         it('Editor CANNOT edit Owner, Admin or Editor roles', function (done) {
             // Cannot edit Owner
             UserAPI.edit(
@@ -889,7 +909,7 @@ describe('Users API', function () {
                     }).catch(function (error) {
                         error.type.should.eql('NoPermissionError');
                         done();
-                    });
+                    }).catch(done);
                 });
             });
         });