🐛 Disallowed locked/suspended users from being made owner via the API (#10647)

- closes #10555
- Added a check to the user modal that the new owner is active 
- Had to refactor Owner->Author unit test (also renamed it)
  - Based on the first 2 lines, owner->editor change is attempted (hence the rename)
  - Since both stubs return a 'modal' with owner role which means owner->owner change is actually attempted
  - Now that there's a user status check, added the `status` property to the user receiving owernship

core/server/models/user.js

@@ -894,20 +894,20 @@ User = ghostBookshelf.Model.extend({
     },
 
     transferOwnership: function transferOwnership(object, unfilteredOptions) {
-        var options = ghostBookshelf.Model.filterOptions(unfilteredOptions, 'transferOwnership'),
-            ownerRole,
-            contextUser;
+        const options = ghostBookshelf.Model.filterOptions(unfilteredOptions, 'transferOwnership');
+        let ownerRole;
+        let contextUser;
 
         return Promise.join(
             ghostBookshelf.model('Role').findOne({name: 'Owner'}),
             User.findOne({id: options.context.user}, {withRelated: ['roles']})
         )
-            .then(function then(results) {
+            .then((results) => {
                 ownerRole = results[0];
                 contextUser = results[1];
 
                 // check if user has the owner role
-                var currentRoles = contextUser.toJSON(options).roles;
+                const currentRoles = contextUser.toJSON(options).roles;
                 if (!_.some(currentRoles, {id: ownerRole.id})) {
                     return Promise.reject(new common.errors.NoPermissionError({
                         message: common.i18n.t('errors.models.user.onlyOwnerCanTransferOwnerRole')
@@ -917,7 +917,7 @@ User = ghostBookshelf.Model.extend({
                 return Promise.join(ghostBookshelf.model('Role').findOne({name: 'Administrator'}),
                     User.findOne({id: object.id}, {withRelated: ['roles']}));
             })
-            .then(function then(results) {
+            .then((results) => {
                 const adminRole = results[0];
                 const user = results[1];
 
@@ -927,7 +927,7 @@ User = ghostBookshelf.Model.extend({
                     }));
                 }
 
-                const currentRoles = user.toJSON(options).roles;
+                const {roles: currentRoles, status} = user.toJSON(options);
 
                 if (!_.some(currentRoles, {id: adminRole.id})) {
                     return Promise.reject(new common.errors.ValidationError({
@@ -935,12 +935,18 @@ User = ghostBookshelf.Model.extend({
                     }));
                 }
 
+                if (status !== 'active') {
+                    return Promise.reject(new common.errors.ValidationError({
+                        message: common.i18n.t('errors.models.user.onlyActiveAdmCanBeAssignedOwnerRole')
+                    }));
+                }
+
                 // convert owner to admin
                 return Promise.join(contextUser.roles().updatePivot({role_id: adminRole.id}),
                     user.roles().updatePivot({role_id: ownerRole.id}),
                     user.id);
             })
-            .then(function then(results) {
+            .then((results) => {
                 return Users.forge()
                     .query('whereIn', 'id', [contextUser.id, results[2]])
                     .fetch({withRelated: ['roles']});

core/server/translations/en.json

@@ -264,7 +264,8 @@
                 "userNotFound": "User not found",
                 "ownerNotFound": "Owner not found",
                 "onlyOwnerCanTransferOwnerRole": "Only owners are able to transfer the owner role.",
-                "onlyAdmCanBeAssignedOwnerRole": "Only administrators can be assigned the owner role."
+                "onlyAdmCanBeAssignedOwnerRole": "Only administrators can be assigned the owner role.",
+                "onlyActiveAdmCanBeAssignedOwnerRole": "Only active administrators can be assigned the owner role."
             },
             "api_key": {
                 "apiKeyNotFound": "API Key not found"

core/test/unit/models/user_spec.js

@@ -425,11 +425,7 @@ describe('Unit: models/user', function () {
     });
 
     describe('transferOwnership', function () {
-        let ownerRole;
-
         beforeEach(function () {
-            ownerRole = sinon.stub();
-
             sinon.stub(models.Role, 'findOne');
 
             models.Role.findOne
@@ -462,27 +458,67 @@ describe('Unit: models/user', function () {
                 });
         });
 
-        it('Owner tries to transfer ownership to author', function () {
+        it('Owner tries to transfer ownership to editor', function () {
             const loggedInUser = testUtils.context.owner;
             const userToChange = testUtils.context.editor;
-            const contextUser = sinon.stub();
 
-            contextUser.toJSON = sinon.stub().returns(testUtils.permissions.owner.user);
+            const loggedInContext = {
+                toJSON: sinon.stub().returns(testUtils.permissions.owner.user)
+            };
+            const userToChangeContext = {
+                toJSON: sinon.stub().returns(
+                    // Test utils don't contain `status` which is required
+                    Object.assign({status: 'active'}, testUtils.permissions.editor.user)
+                )
+            };
 
             models.User
                 .findOne
                 .withArgs({id: loggedInUser.context.user}, {withRelated: ['roles']})
-                .resolves(contextUser);
+                .resolves(loggedInContext);
 
             models.User
                 .findOne
                 .withArgs({id: userToChange.context.user}, {withRelated: ['roles']})
-                .resolves(contextUser);
+                .resolves(userToChangeContext);
 
             return models.User.transferOwnership({id: userToChange.context.user}, loggedInUser)
                 .then(Promise.reject)
                 .catch((err) => {
                     err.should.be.an.instanceof(common.errors.ValidationError);
+                    err.message.indexOf('Only administrators can')
+                        .should.be.aboveOrEqual(0, 'contains correct error message');
+                });
+        });
+
+        it('Owner tries to transfer ownership to suspended user', function () {
+            const loggedInUser = testUtils.context.owner;
+            const userToChange = testUtils.context.admin;
+
+            const userToChangeJSON = Object.assign({status: 'inactive'}, testUtils.permissions.admin.user);
+            const loggedInContext = {
+                toJSON: sinon.stub().returns(testUtils.permissions.owner.user)
+            };
+            const userToChangeContext = {
+                toJSON: sinon.stub().returns(userToChangeJSON)
+            };
+
+            models.User
+                .findOne
+                .withArgs({id: loggedInUser.context.user}, {withRelated: ['roles']})
+                .resolves(loggedInContext);
+
+            models.User
+                .findOne
+                .withArgs({id: userToChange.context.user}, {withRelated: ['roles']})
+                .resolves(userToChangeContext);
+
+            return models.User.transferOwnership({id: userToChange.context.user}, loggedInUser)
+                .then(Promise.reject)
+                .catch((err) => {
+                    err.should.be.an.instanceof(common.errors.ValidationError);
+                    err.message.indexOf('Only active administrators can')
+                        .should.be.aboveOrEqual(0, 'contains correct error message');
                 });
         });
     });