From ded6aa6ac05429408f3baff7dcc50d9aa7230b71 Mon Sep 17 00:00:00 2001 From: Sebastian Gierlinger Date: Wed, 30 Jul 2014 17:40:30 +0200 Subject: [PATCH] Transfer ownership end point MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit closes #3426 - added transfer ownership endpoint - added owner to roles.permissible - manually removed owner from roles.browse - removed hard coded author role - fixed tests that were passing due to hard coded author role - added testUtils.setup(‚roles‘) --- .../controllers/modals/transfer-owner.js | 23 +++---- core/server/api/roles.js | 3 + core/server/api/users.js | 25 ++++++- core/server/data/fixtures/index.js | 2 +- core/server/models/role.js | 5 +- core/server/models/user.js | 68 +++++++++++-------- core/server/routes/api.js | 1 + core/test/integration/api/api_users_spec.js | 68 +++++++++---------- .../integration/model/model_users_spec.js | 2 +- core/test/utils/index.js | 5 ++ 10 files changed, 119 insertions(+), 83 deletions(-) diff --git a/core/client/controllers/modals/transfer-owner.js b/core/client/controllers/modals/transfer-owner.js index c4d1cbf272..5afa0a36d3 100644 --- a/core/client/controllers/modals/transfer-owner.js +++ b/core/client/controllers/modals/transfer-owner.js @@ -2,27 +2,24 @@ var TransferOwnerController = Ember.Controller.extend({ actions: { confirmAccept: function () { var user = this.get('model'), + url = this.get('ghostPaths.url').api('users', 'owner'), self = this; self.get('popover').closePopovers(); - // Get owner role - this.store.find('role').then(function (result) { - return result.findBy('name', 'Owner'); - }).then(function (ownerRole) { - // remove roles and assign owner role - user.get('roles').clear(); - user.get('roles').pushObject(ownerRole); - - return user.saveOnly('roles'); + ic.ajax.request(url, { + type: 'PUT', + data: { + owner: [{ + 'id': user.get('id') + }] + } }).then(function () { self.notifications.closePassive(); self.notifications.showSuccess('Ownership successfully transferred to ' + user.get('name')); - }).catch(function (errors) { + }).catch(function (error) { self.notifications.closePassive(); - - errors = Ember.isArray(errors) ? errors : Ember.A([errors]); - self.notifications.showErrors(errors); + self.notifications.showAPIError(error); }); }, diff --git a/core/server/api/roles.js b/core/server/api/roles.js index 6244dab3bc..9f8781b5d9 100644 --- a/core/server/api/roles.js +++ b/core/server/api/roles.js @@ -41,6 +41,9 @@ roles = { // TODO: replace with better filter when bluebird lands _.each(foundRoles.toJSON(), function (role) { permissionMap.push(canThis(options.context).assign.role(role).then(function () { + if (role.name === 'Owner') { + return null; + } return role; }, function () { return null; diff --git a/core/server/api/users.js b/core/server/api/users.js index 75e745c319..f9c408bcc9 100644 --- a/core/server/api/users.js +++ b/core/server/api/users.js @@ -142,6 +142,11 @@ users = { } newUser = checkedUserData.users[0]; + + if (_.isEmpty(newUser.roles)) { + return when.reject(new errors.BadRequestError('No role provided.')); + } + roleId = parseInt(newUser.roles[0].id || newUser.roles[0], 10); // Make sure user is allowed to add a user with this role @@ -281,8 +286,26 @@ users = { return when.reject(new errors.ValidationError(error.message)); }); }); - } + }, + /** + * + */ + transferOwnership: function transferOwnership(object, options) { + return dataProvider.Role.findOne({name: 'Owner'}).then(function (ownerRole) { + return canThis(options.context).assign.role(ownerRole); + }).then(function () { + return utils.checkObject(object, 'owner').then(function (checkedOwnerTransfer) { + return dataProvider.User.transferOwnership(checkedOwnerTransfer.owner[0], options).then(function () { + return when.resolve({owner: [{message: 'Ownership transferred successfully.'}]}); + }).catch(function (error) { + return when.reject(new errors.ValidationError(error.message)); + }); + }); + }).catch(function (error) { + return errors.handleAPIError(error); + }); + } }; module.exports = users; diff --git a/core/server/data/fixtures/index.js b/core/server/data/fixtures/index.js index 9e9cd7c53d..d7294c5544 100644 --- a/core/server/data/fixtures/index.js +++ b/core/server/data/fixtures/index.js @@ -58,7 +58,7 @@ createOwner = function () { var user = fixtures.users[0]; return models.Role.findOne({name: 'Owner'}).then(function (ownerRole) { - user.role = ownerRole.id; + user.roles = [ownerRole.id]; user.password = utils.uid(50); logInfo('Creating owner'); diff --git a/core/server/models/role.js b/core/server/models/role.js index e528f1314d..0981ebbedd 100644 --- a/core/server/models/role.js +++ b/core/server/models/role.js @@ -60,8 +60,9 @@ Role = ghostBookshelf.Model.extend({ } if (action === 'assign' && loadedPermissions.user) { - if (_.any(loadedPermissions.user.roles, { 'name': 'Owner' }) || - _.any(loadedPermissions.user.roles, { 'name': 'Administrator' })) { + if (_.any(loadedPermissions.user.roles, { 'name': 'Owner' })) { + checkAgainst = ['Owner', 'Administrator', 'Editor', 'Author']; + } else if (_.any(loadedPermissions.user.roles, { 'name': 'Administrator' })) { checkAgainst = ['Administrator', 'Editor', 'Author']; } else if (_.any(loadedPermissions.user.roles, { 'name': 'Editor' })) { checkAgainst = ['Author']; diff --git a/core/server/models/user.js b/core/server/models/user.js index 7d94523c27..26be27a076 100644 --- a/core/server/models/user.js +++ b/core/server/models/user.js @@ -40,12 +40,6 @@ User = ghostBookshelf.Model.extend({ /*jshint unused:false*/ var self = this; - // disabling sanitization until we can implement a better version - // this.set('name', this.sanitize('name')); - // this.set('email', this.sanitize('email')); - // this.set('location', this.sanitize('location')); - // this.set('website', this.sanitize('website')); - // this.set('bio', this.sanitize('bio')); ghostBookshelf.Model.prototype.saving.apply(this, arguments); @@ -184,8 +178,8 @@ User = ghostBookshelf.Model.extend({ }, options); //TODO: there are multiple statuses that make a user "active" or "invited" - we a way to translate/map them: - //TODO (cont'd from above): * valid "active" statuses: active, warn-1, warn-2, warn-3, warn-4, locked - //TODO (cont'd from above): * valid "invited" statuses" invited, invited-pending + //TODO (cont'd from above): * valid "active" statuses: active, warn-1, warn-2, warn-3, warn-4, locked + //TODO (cont'd from above): * valid "invited" statuses" invited, invited-pending // Filter on the status. A status of 'all' translates to no filter since we want all statuses if (options.status && options.status !== 'all') { @@ -334,7 +328,9 @@ User = ghostBookshelf.Model.extend({ return Role.findOne({id: roleId}); }).then(function (roleToAssign) { if (roleToAssign && roleToAssign.get('name') === 'Owner') { - return self.transferOwnership(user, roleToAssign, options.context); + return when.reject( + new errors.ValidationError('This method does not support assigning the owner role') + ); } else { // assign all other roles return user.roles().updatePivot({role_id: roleId}); @@ -359,24 +355,23 @@ User = ghostBookshelf.Model.extend({ */ add: function (data, options) { var self = this, - // Clone the _user so we don't expose the hashed password unnecessarily userData = this.filterData(data), - // Get the role we're going to assign to this user, or the author role if there isn't one - // TODO: don't reference Author role by ID! - roles = data.roles || [1]; - - // remove roles from the object - delete data.roles; - - // check for too many roles - if (roles.length > 1) { - return when.reject(new errors.ValidationError('Only one role per user is supported at the moment.')); - } + roles; options = this.filterOptions(options, 'add'); options.withRelated = _.union([ 'roles' ], options.include); + return Role.findOne({name: 'Author'}).then(function (authorRole) { + // Get the role we're going to assign to this user, or the author role if there isn't one + roles = data.roles || authorRole.id; + // check for too many roles + if (roles.length > 1) { + return when.reject(new errors.ValidationError('Only one role per user is supported at the moment.')); + } + // remove roles from the object + delete data.roles; - return validatePasswordLength(userData.password).then(function () { + return validatePasswordLength(userData.password); + }).then(function () { return self.forge().fetch(); }).then(function () { // Generate a new password hash @@ -693,23 +688,40 @@ User = ghostBookshelf.Model.extend({ }); }, - transferOwnership: function (user, ownerRole, context) { - var adminRole; + transferOwnership: function (object, options) { + var adminRole, + ownerRole, + contextUser, + assignUser; + // Get admin role return Role.findOne({name: 'Administrator'}).then(function (result) { adminRole = result; - return User.findOne({id: context.user}); - }).then(function (contextUser) { + return Role.findOne({name: 'Owner'}); + }).then(function (result) { + ownerRole = result; + return User.findOne({id: options.context.user}); + }).then(function (ctxUser) { // check if user has the owner role - var currentRoles = contextUser.toJSON().roles; + var currentRoles = ctxUser.toJSON().roles; if (!_.contains(currentRoles, ownerRole.id)) { return when.reject(new errors.NoPermissionError('Only owners are able to transfer the owner role.')); } + contextUser = ctxUser; + return User.findOne({id: object.id}); + }).then(function (user) { + + var currentRoles = user.toJSON().roles; + if (!_.contains(currentRoles, adminRole.id)) { + return when.reject(new errors.ValidationError('Only administrators can be assigned the owner role.')); + } + + assignUser = user; // convert owner to admin return contextUser.roles().updatePivot({role_id: adminRole.id}); }).then(function () { // assign owner role to a new user - return user.roles().updatePivot({role_id: ownerRole.id}); + return assignUser.roles().updatePivot({role_id: ownerRole.id}); }); }, diff --git a/core/server/routes/api.js b/core/server/routes/api.js index b1a8cf6532..8f713e9c83 100644 --- a/core/server/routes/api.js +++ b/core/server/routes/api.js @@ -27,6 +27,7 @@ apiRoutes = function (middleware) { router.get('/users/slug/:slug', api.http(api.users.read)); router.get('/users/email/:email', api.http(api.users.read)); router.put('/users/password', api.http(api.users.changePassword)); + router.put('/users/owner', api.http(api.users.transferOwnership)); router.put('/users/:id', api.http(api.users.edit)); router.post('/users', api.http(api.users.add)); router.del('/users/:id', api.http(api.users.destroy)); diff --git a/core/test/integration/api/api_users_spec.js b/core/test/integration/api/api_users_spec.js index 0887fa6d1c..1ceadadf1a 100644 --- a/core/test/integration/api/api_users_spec.js +++ b/core/test/integration/api/api_users_spec.js @@ -856,47 +856,41 @@ describe('Users API', function () { }); describe('Transfer ownership', function () { -// Temporarily commenting this test out until #3426 is fixed -// it('Owner can transfer ownership', function (done) { -// // transfer ownership to admin user id:2 -// UserAPI.edit( -// {users: [ -// {name: 'Joe Blogger', roles: [roleIdFor.owner]} -// ]}, _.extend({}, context.owner, {id: userIdFor.admin}) -// ).then(function (response) { -// should.exist(response); -// should.not.exist(response.meta); -// should.exist(response.users); -// response.users.should.have.length(1); -// testUtils.API.checkResponse(response.users[0], 'user', ['roles']); -// response.users[0].name.should.equal('Joe Blogger'); -// response.users[0].id.should.equal(2); -// response.users[0].roles[0].should.equal(4); -// response.users[0].updated_at.should.be.a.Date; -// done(); -// }).catch(done); -// }); + it('Owner can transfer ownership', function (done) { + // transfer ownership to admin user id:2 + UserAPI.transferOwnership( + {owner: [ + {id: userIdFor.admin} + ]}, context.owner + ).then(function (response) { + should.exist(response); + should.exist(response.owner); + response.owner.should.have.length(1); + response.owner[0].message.should.eql('Ownership transferred successfully.'); + done(); + }).catch(done); + }); it('Owner CANNOT downgrade own role', function (done) { // Cannot change own role to admin - UserAPI.edit( - {users: [ - {name: 'Joe Blogger', roles: [roleIdFor.admin]} - ]}, _.extend({}, context.owner, {id: userIdFor.owner}) + UserAPI.transferOwnership( + {owner: [ + {id: userIdFor.owner} + ]}, context.owner ).then(function (response) { done(new Error('Owner should not be able to downgrade their role')); }).catch(function (error) { - error.type.should.eql('NoPermissionError'); + error.type.should.eql('ValidationError'); done(); }); }); it('Admin CANNOT transfer ownership', function (done) { // transfer ownership to user id: 2 - UserAPI.edit( - {users: [ - {name: 'Joe Blogger', roles: [roleIdFor.owner]} - ]}, _.extend({}, context.admin, {id: userIdFor.admin}) + UserAPI.transferOwnership( + {owner: [ + {id: userIdFor.editor} + ]}, context.admin ).then(function () { done(new Error('Admin is not denied transferring ownership.')); }).catch(function (error) { @@ -907,10 +901,10 @@ describe('Users API', function () { it('Editor CANNOT transfer ownership', function (done) { // transfer ownership to user id: 2 - UserAPI.edit( - {users: [ - {name: 'Joe Blogger', roles: [roleIdFor.owner]} - ]}, _.extend({}, context.editor, {id: userIdFor.admin}) + UserAPI.transferOwnership( + {owner: [ + {id: userIdFor.admin} + ]}, context.editor ).then(function () { done(new Error('Admin is not denied transferring ownership.')); }).catch(function (error) { @@ -921,10 +915,10 @@ describe('Users API', function () { it('Author CANNOT transfer ownership', function (done) { // transfer ownership to user id: 2 - UserAPI.edit( - {users: [ - {name: 'Joe Blogger', roles: [roleIdFor.owner]} - ]}, _.extend({}, context.author, {id: userIdFor.admin}) + UserAPI.transferOwnership( + {owner: [ + {id: userIdFor.admin} + ]}, context.author ).then(function () { done(new Error('Admin is not denied transferring ownership.')); }).catch(function (error) { diff --git a/core/test/integration/model/model_users_spec.js b/core/test/integration/model/model_users_spec.js index 6ae7fc51b8..e19ffb378a 100644 --- a/core/test/integration/model/model_users_spec.js +++ b/core/test/integration/model/model_users_spec.js @@ -25,7 +25,7 @@ describe('User Model', function run() { should.exist(UserModel); describe('Registration', function runRegistration() { - beforeEach(testUtils.setup()); + beforeEach(testUtils.setup('roles')); it('can add first', function (done) { var userData = testUtils.DataGenerator.forModel.users[0]; diff --git a/core/test/utils/index.js b/core/test/utils/index.js index 66a1160100..b5cbb45121 100644 --- a/core/test/utils/index.js +++ b/core/test/utils/index.js @@ -101,6 +101,10 @@ fixtures = { })); }); }, + insertRoles: function insertRoles() { + var knex = config.database.knex; + return knex('roles').insert(DataGenerator.forKnex.roles); + }, initOwnerUser: function initOwnerUser() { var user = DataGenerator.Content.users[0], @@ -258,6 +262,7 @@ toDoList = { }, 'permission': function insertPermission() { return fixtures.insertOne('permissions', 'createPermission'); }, 'role': function insertRole() { return fixtures.insertOne('roles', 'createRole'); }, + 'roles': function insertRoles() { return fixtures.insertRoles(); }, 'tag': function insertRole() { return fixtures.insertOne('tags', 'createTag'); }, 'posts': function insertPosts() { return fixtures.insertPosts(); }, 'apps': function insertApps() { return fixtures.insertApps(); },