From 28b03ec87e8d7f917e1a7242c171a87e61dcadd9 Mon Sep 17 00:00:00 2001 From: Sebastian Gierlinger Date: Mon, 21 Jul 2014 22:50:43 +0200 Subject: [PATCH] Add edit roles refs #3087 - added ability to edit user/roles relation - user is not allowed assign roles to himself - only one role per user is supported atm - added tests --- core/server/api/users.js | 5 + core/server/models/post.js | 2 +- core/server/models/user.js | 48 +++++++- core/server/permissions/effective.js | 2 +- core/test/integration/api/api_tags_spec.js | 58 ++++++---- core/test/integration/api/api_users_spec.js | 119 +++++++++++++++++--- core/test/utils/index.js | 20 +++- 7 files changed, 211 insertions(+), 43 deletions(-) diff --git a/core/server/api/users.js b/core/server/api/users.js index afd82380c3..5d1a24a623 100644 --- a/core/server/api/users.js +++ b/core/server/api/users.js @@ -88,6 +88,11 @@ users = { } return canThis(options.context).edit.user(options.id).then(function () { + // TODO: add permission check for roles + // if (data.roles) { + // return canThis(options.context).assign.role() + // } + // }.then(function (){ return utils.checkObject(object, docName).then(function (checkedUserData) { if (options.include) { diff --git a/core/server/models/post.js b/core/server/models/post.js index 6020995d7b..08f09987bc 100644 --- a/core/server/models/post.js +++ b/core/server/models/post.js @@ -6,7 +6,7 @@ var _ = require('lodash'), Showdown = require('showdown'), ghostgfm = require('../../shared/lib/showdown/extensions/ghostgfm'), converter = new Showdown.converter({extensions: [ghostgfm]}), - Tag = require('./tag').Tag, + Tag = require('./tag').Tag, Tags = require('./tag').Tags, ghostBookshelf = require('./base'), xmlrpc = require('../xmlrpc'), diff --git a/core/server/models/user.js b/core/server/models/user.js index aef2418034..7986a0f7ac 100644 --- a/core/server/models/user.js +++ b/core/server/models/user.js @@ -7,6 +7,7 @@ var _ = require('lodash'), http = require('http'), crypto = require('crypto'), validator = require('validator'), + Role = require('./role').Role, tokenSecurity = {}, User, @@ -298,10 +299,55 @@ User = ghostBookshelf.Model.extend({ * **See:** [ghostBookshelf.Model.edit](base.js.html#edit) */ edit: function (data, options) { + var self = this, + adminRole, + ownerRole; + options = options || {}; options.withRelated = _.union([ 'roles' ], options.include); - return ghostBookshelf.Model.edit.call(this, data, options); + return ghostBookshelf.Model.edit.call(this, data, options).then(function (user) { + + if (data.roles) { + + if (user.id === options.context.user) { + return when.reject(new errors.ValidationError('You are not allowed to assign a new role to yourself')); + } + if (data.roles.length > 1) { + return when.reject(new errors.ValidationError('Only one role per user is supported at the moment.')); + } + + return Role.findOne({id: data.roles[0]}).then(function (role) { + if (role.get('name') === 'Owner') { + // Get admin and owner role + return Role.findOne({name: 'Administrator'}).then(function (result) { + adminRole = result; + return Role.findOne({name: 'Owner'}); + }).then(function (result) { + ownerRole = result; + return User.findOne({id: options.context.user}); + }).then(function (contextUser) { + // check if user has the owner role + var currentRoles = contextUser.toJSON().roles; + if (!_.contains(currentRoles, ownerRole.id)) { + return when.reject(new errors.ValidationError('Only owners are able to transfer the owner role.')); + } + // 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}); + }); + } else { + // assign all other roles + return user.roles().updatePivot({role_id: data.roles[0]}); + } + }).then(function () { + return self.findOne(user, options); + }); + } + return user; + }); }, /** diff --git a/core/server/permissions/effective.js b/core/server/permissions/effective.js index 886118c8b4..61652faa4a 100644 --- a/core/server/permissions/effective.js +++ b/core/server/permissions/effective.js @@ -14,7 +14,7 @@ var effective = { user = foundUser.toJSON(); // TODO: using 'Owner' as return value is a bit hacky. - if (user.roles[0] && user.roles[0].name === 'Owner') { + if (_.find(user.roles, { 'name': 'Owner' })) { return 'Owner'; } diff --git a/core/test/integration/api/api_tags_spec.js b/core/test/integration/api/api_tags_spec.js index cbda057b3d..004739bd4f 100644 --- a/core/test/integration/api/api_tags_spec.js +++ b/core/test/integration/api/api_tags_spec.js @@ -16,6 +16,8 @@ describe('Tags API', function () { testUtils.initData() .then(function () { return testUtils.insertDefaultFixtures(); + }).then(function () { + return testUtils.insertAdminUser(); }).then(function () { return testUtils.insertEditorUser(); }).then(function () { @@ -41,31 +43,31 @@ describe('Tags API', function () { }).catch(done); }); - it('can browse (admin)', function (done) { - TagAPI.browse({context: {user: 1}}).then(function (results) { - should.exist(results); - should.exist(results.tags); - results.tags.length.should.be.above(0); - testUtils.API.checkResponse(results.tags[0], 'tag'); - results.tags[0].created_at.should.be.an.instanceof(Date); + it('can browse (owner)', function (done) { + TagAPI.browse({context: {user: 1}}).then(function (results) { + should.exist(results); + should.exist(results.tags); + results.tags.length.should.be.above(0); + testUtils.API.checkResponse(results.tags[0], 'tag'); + results.tags[0].created_at.should.be.an.instanceof(Date); - done(); - }).catch(done); - }); + done(); + }).catch(done); + }); + + it('can browse (admin)', function (done) { + TagAPI.browse({context: {user: 2}}).then(function (results) { + should.exist(results); + should.exist(results.tags); + results.tags.length.should.be.above(0); + testUtils.API.checkResponse(results.tags[0], 'tag'); + results.tags[0].created_at.should.be.an.instanceof(Date); + + done(); + }).catch(done); + }); it('can browse (editor)', function (done) { - TagAPI.browse({context: {user: 2}}).then(function (results) { - should.exist(results); - should.exist(results.tags); - results.tags.length.should.be.above(0); - testUtils.API.checkResponse(results.tags[0], 'tag'); - results.tags[0].created_at.should.be.an.instanceof(Date); - - done(); - }).catch(done); - }); - - it('can browse (author)', function (done) { TagAPI.browse({context: {user: 3}}).then(function (results) { should.exist(results); should.exist(results.tags); @@ -76,4 +78,16 @@ describe('Tags API', function () { done(); }).catch(done); }); + + it('can browse (author)', function (done) { + TagAPI.browse({context: {user: 4}}).then(function (results) { + should.exist(results); + should.exist(results.tags); + results.tags.length.should.be.above(0); + testUtils.API.checkResponse(results.tags[0], 'tag'); + results.tags[0].created_at.should.be.an.instanceof(Date); + + done(); + }).catch(done); + }); }); \ No newline at end of file diff --git a/core/test/integration/api/api_users_spec.js b/core/test/integration/api/api_users_spec.js index 6406ef8df4..440bd902dd 100644 --- a/core/test/integration/api/api_users_spec.js +++ b/core/test/integration/api/api_users_spec.js @@ -53,6 +53,8 @@ describe('Users API', function () { beforeEach(function (done) { testUtils.initData().then(function () { return testUtils.insertDefaultFixtures(); + }).then(function () { + return testUtils.insertAdminUser(); }).then(function () { return testUtils.insertEditorUser(); }).then(function () { @@ -78,42 +80,59 @@ describe('Users API', function () { }).catch(done); }); - it('admin can browse', function (done) { + it('can browse (owner)', function (done) { UsersAPI.browse({context: {user: 1}}).then(function (results) { should.exist(results); testUtils.API.checkResponse(results, 'users'); should.exist(results.users); - results.users.should.have.length(3); + results.users.should.have.length(4); testUtils.API.checkResponse(results.users[0], 'user', ['roles']); testUtils.API.checkResponse(results.users[1], 'user', ['roles']); testUtils.API.checkResponse(results.users[2], 'user', ['roles']); + testUtils.API.checkResponse(results.users[3], 'user', ['roles']); done(); }).catch(done); }); - it('editor can browse', function (done) { + it('can browse (admin)', function (done) { UsersAPI.browse({context: {user: 2}}).then(function (results) { should.exist(results); testUtils.API.checkResponse(results, 'users'); should.exist(results.users); - results.users.should.have.length(3); + results.users.should.have.length(4); testUtils.API.checkResponse(results.users[0], 'user', ['roles']); testUtils.API.checkResponse(results.users[1], 'user', ['roles']); testUtils.API.checkResponse(results.users[2], 'user', ['roles']); + testUtils.API.checkResponse(results.users[3], 'user', ['roles']); done(); }).catch(done); }); - it('author can browse', function (done) { + it('can browse (editor)', function (done) { UsersAPI.browse({context: {user: 3}}).then(function (results) { should.exist(results); testUtils.API.checkResponse(results, 'users'); should.exist(results.users); - results.users.should.have.length(3); + results.users.should.have.length(4); testUtils.API.checkResponse(results.users[0], 'user', ['roles']); testUtils.API.checkResponse(results.users[1], 'user', ['roles']); testUtils.API.checkResponse(results.users[2], 'user', ['roles']); + testUtils.API.checkResponse(results.users[3], 'user', ['roles']); + done(); + }).catch(done); + }); + + it('can browse (author)', function (done) { + UsersAPI.browse({context: {user: 4}}).then(function (results) { + should.exist(results); + testUtils.API.checkResponse(results, 'users'); + should.exist(results.users); + results.users.should.have.length(4); + testUtils.API.checkResponse(results.users[0], 'user', ['roles']); + testUtils.API.checkResponse(results.users[1], 'user', ['roles']); + testUtils.API.checkResponse(results.users[2], 'user', ['roles']); + testUtils.API.checkResponse(results.users[3], 'user', ['roles']); done(); }).catch(done); }); @@ -126,7 +145,7 @@ describe('Users API', function () { }).catch(done); }); - it('admin can read', function (done) { + it('can read (owner)', function (done) { UsersAPI.read({id: 1, context: {user: 1}}).then(function (results) { should.exist(results); should.not.exist(results.meta); @@ -140,19 +159,33 @@ describe('Users API', function () { }).catch(done); }); - it('editor can read', function (done) { + it('can read (admin)', function (done) { UsersAPI.read({id: 1, context: {user: 2}}).then(function (results) { should.exist(results); should.not.exist(results.meta); should.exist(results.users); results.users[0].id.should.eql(1); testUtils.API.checkResponse(results.users[0], 'user', ['roles']); + + results.users[0].created_at.should.be.a.Date; + + done(); + }).catch(done); + }); + + it('can read (editor)', function (done) { + UsersAPI.read({id: 1, context: {user: 3}}).then(function (results) { + should.exist(results); + should.not.exist(results.meta); + should.exist(results.users); + results.users[0].id.should.eql(1); + testUtils.API.checkResponse(results.users[0], 'user', ['roles']); done(); }).catch(done); }); - it('author can read', function (done) { - UsersAPI.read({id: 1, context: {user: 3}}).then(function (results) { + it('can read (author)', function (done) { + UsersAPI.read({id: 1, context: {user: 4}}).then(function (results) { should.exist(results); should.not.exist(results.meta); should.exist(results.users); @@ -173,7 +206,7 @@ describe('Users API', function () { }).catch(done); }); - it('admin can edit', function (done) { + it('can edit (owner)', function (done) { UsersAPI.edit({users: [{name: 'Joe Blogger'}]}, {id: 1, context: {user: 1}}).then(function (response) { should.exist(response); should.not.exist(response.meta); @@ -186,28 +219,41 @@ describe('Users API', function () { }).catch(done); }); - it('editor can edit', function (done) { + it('can edit (admin)', function (done) { UsersAPI.edit({users: [{name: 'Joe Blogger'}]}, {id: 1, context: {user: 2}}).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.eql('Joe Blogger'); + response.users[0].name.should.equal('Joe Blogger'); + response.users[0].updated_at.should.be.a.Date; + done(); + }).catch(done); + }); + it('can edit (editor)', function (done) { + UsersAPI.edit({users: [{name: 'Joe Blogger'}]}, {id: 1, context: {user: 2}}).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].updated_at.should.be.a.Date; done(); }).catch(done); }); it('author can edit only self', function (done) { // Test author cannot edit admin user - UsersAPI.edit({users: [{name: 'Joe Blogger'}]}, {id: 1, context: {user: 3}}).then(function () { + UsersAPI.edit({users: [{name: 'Joe Blogger'}]}, {id: 1, context: {user: 4}}).then(function () { done(new Error('Author should not be able to edit account which is not their own')); }).catch(function (error) { error.type.should.eql('NoPermissionError'); }).finally(function () { // Next test that author CAN edit self - return UsersAPI.edit({users: [{name: 'Timothy Bogendath'}]}, {id: 3, context: {user: 3}}) + return UsersAPI.edit({users: [{name: 'Timothy Bogendath'}]}, {id: 4, context: {user: 4}}) .then(function (response) { should.exist(response); should.not.exist(response.meta); @@ -219,5 +265,48 @@ describe('Users API', function () { }).catch(done); }); }); + + it('admin can\'t transfer ownership', function (done) { + // transfer ownership to user id: 2 + UsersAPI.edit({users: [{name: 'Joe Blogger', roles:[4]}]}, {id: 3, context: {user: 2}}).then(function (response) { + done(new Error('Admin is not dienied transferring ownership.')); + }, function () { + done(); + }).catch(done); + }); + + it('editor can\'t transfer ownership', function (done) { + // transfer ownership to user id: 2 + UsersAPI.edit({users: [{name: 'Joe Blogger', roles:[4]}]}, {id: 2, context: {user: 3}}).then(function (response) { + done(new Error('Admin is not dienied transferring ownership.')); + }, function () { + done(); + }).catch(done); + }); + + it('author can\'t transfer ownership', function (done) { + // transfer ownership to user id: 2 + UsersAPI.edit({users: [{name: 'Joe Blogger', roles:[4]}]}, {id: 2, context: {user: 4}}).then(function (response) { + done(new Error('Admin is not dienied transferring ownership.')); + }, function () { + done(); + }).catch(done); + }); + + it('owner can transfer ownership', function (done) { + // transfer ownership to user id: 2 + UsersAPI.edit({users: [{name: 'Joe Blogger', roles:[4]}]}, {id: 2, context: {user: 1}}).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); + }); }); }); diff --git a/core/test/utils/index.js b/core/test/utils/index.js index c8ae2af860..c55dea9815 100644 --- a/core/test/utils/index.js +++ b/core/test/utils/index.js @@ -97,7 +97,7 @@ function insertDefaultUser() { .update(user); } -function insertEditorUser() { +function insertAdminUser() { var users = [], userRoles = []; @@ -110,12 +110,25 @@ function insertEditorUser() { }); } -function insertAuthorUser() { +function insertEditorUser() { var users = [], userRoles = []; users.push(DataGenerator.forKnex.createUser(DataGenerator.Content.users[2])); - userRoles.push(DataGenerator.forKnex.createUserRole(3, 3)); + userRoles.push(DataGenerator.forKnex.createUserRole(3, 2)); + return knex('users') + .insert(users) + .then(function () { + return knex('roles_users').insert(userRoles); + }); +} + +function insertAuthorUser() { + var users = [], + userRoles = []; + + users.push(DataGenerator.forKnex.createUser(DataGenerator.Content.users[3])); + userRoles.push(DataGenerator.forKnex.createUserRole(4, 3)); return knex('users') .insert(users) .then(function () { @@ -236,6 +249,7 @@ module.exports = { insertMorePosts: insertMorePosts, insertMorePostsTags: insertMorePostsTags, insertDefaultUser: insertDefaultUser, + insertAdminUser: insertAdminUser, insertEditorUser: insertEditorUser, insertAuthorUser: insertAuthorUser, insertDefaultApp: insertDefaultApp,