From 8a2d50b871df68f9aeca89b581b5bba97367cfbc Mon Sep 17 00:00:00 2001 From: Jason Williams Date: Wed, 23 Jul 2014 06:13:20 +0000 Subject: [PATCH] Improve handling of users and roles in admin Closes #3083 Refs #3229 - Populates the dropdown list in the invite user menu with the list of roles a user is permitted to create. - Users API now checks the invite user request for allowed roles. - Change API response from 200 to 201 on successful invitation. - Change API response from 500 to 201 when the user was created but the email was not sent. The client will show a warning notification when it sees 'invite-pending' as the new user's status. - Add support for "?status=all" to the /users endpoint. - Refactor the route and controller for the /settings/users page so that there's only one network API call to load users instead of two. --- .../controllers/modals/invite-new-user.js | 62 ++++++++++++------- .../controllers/settings/users/index.js | 12 +++- core/client/models/role.js | 3 + core/client/models/user.js | 3 + core/client/routes/settings/users/index.js | 27 +++----- .../templates/modals/invite-new-user.hbs | 6 +- core/client/validators/user.js | 4 +- core/server/api/index.js | 17 ++--- core/server/api/roles.js | 11 ++-- core/server/api/users.js | 35 +++++++---- core/server/models/user.js | 5 +- 11 files changed, 108 insertions(+), 77 deletions(-) diff --git a/core/client/controllers/modals/invite-new-user.js b/core/client/controllers/modals/invite-new-user.js index 7a1de6e21a..96f82654a0 100644 --- a/core/client/controllers/modals/invite-new-user.js +++ b/core/client/controllers/modals/invite-new-user.js @@ -9,47 +9,65 @@ var InviteNewUserController = Ember.Controller.extend({ } }, - // @TODO: replace with roles from server - see issue #3196 - roles: [ - { - id: 3, - name: 'Author' - } - ], + roles: Ember.computed(function () { + var roles = {}, + self = this; + + roles.promise = this.store.find('role', { permissions: 'assign' }).then(function (roles) { + return roles.rejectBy('name', 'Owner').sortBy('name'); + }).then(function (roles) { + // After the promise containing the roles has been resolved and the array + // has been sorted, explicitly set the selectedRole for the Ember.Select. + // The explicit set is needed because the data-select-text attribute is + // not being set until a change is made in the dropdown list. + // This is only required with Ember.Select when it is bound to async data. + self.set('selectedRole', roles.get('firstObject')); + + return roles; + }); + + return Ember.ArrayProxy.extend(Ember.PromiseProxyMixin).create(roles); + }), actions: { confirmAccept: function () { var email = this.get('email'), role_id = this.get('role'), self = this, - newUser; + newUser, + role; - this.notifications.closePassive(); - - newUser = this.store.createRecord('user', { + newUser = self.store.createRecord('user', { email: email, - role: role_id, status: 'invited' }); + // no need to make an API request, the store will already have this role + role = self.store.getById('role', role_id); + + newUser.get('roles').pushObject(role); + newUser.save().then(function () { var notificationText = 'Invitation sent! (' + email + ')'; - self.notifications.showSuccess(notificationText, false); - }).catch(function (errors) { - if (errors[0].message.indexOf('Email Error:') === -1) { - newUser.deleteRecord(); - } else { - newUser.set('status', 'invited-pending'); - } + self.notifications.closePassive(); + // If sending the invitation email fails, the API will still return a status of 201 + // but the user's status in the response object will be 'invited-pending'. + if (newUser.get('status') === 'invited-pending') { + self.notifications.showWarn('Invitation email was not sent. Please try resending.'); + } + else { + self.notifications.showSuccess(notificationText, false); + } + }).catch(function (errors) { + newUser.deleteRecord(); self.notifications.closePassive(); self.notifications.showErrors(errors); }); - this.set('email', null); - this.set('role', null); - + self.set('email', null); + self.set('role', null); }, confirmReject: function () { diff --git a/core/client/controllers/settings/users/index.js b/core/client/controllers/settings/users/index.js index 2bd4903678..c485ab8499 100644 --- a/core/client/controllers/settings/users/index.js +++ b/core/client/controllers/settings/users/index.js @@ -3,15 +3,21 @@ import PaginationControllerMixin from 'ghost/mixins/pagination-controller'; var UsersIndexController = Ember.ArrayController.extend(PaginationControllerMixin, { init: function () { //let the PaginationControllerMixin know what type of model we will be paginating - //this is necesariy because we do not have access to the model inside the Controller::init method + //this is necessary because we do not have access to the model inside the Controller::init method this._super({'modelType': 'user'}); }, users: Ember.computed.alias('model'), - activeUsers: Ember.computed.filterBy('users', 'active', true).property('users'), + activeUsers: Ember.computed.filter('users', function (user) { + return /^active|warn-[1-4]|locked$/.test(user.get('status')); + }), - invitedUsers: Ember.computed.filterBy('users', 'invited', true).property('users') + invitedUsers: Ember.computed.filter('users', function (user) { + var status = user.get('status'); + + return status === 'invited' || status === 'invited-pending'; + }) }); export default UsersIndexController; diff --git a/core/client/models/role.js b/core/client/models/role.js index c3d8023844..dca5c1bca7 100644 --- a/core/client/models/role.js +++ b/core/client/models/role.js @@ -2,6 +2,9 @@ var Role = DS.Model.extend({ uuid: DS.attr('string'), name: DS.attr('string'), description: DS.attr('string'), + created_at: DS.attr('moment-date'), + updated_at: DS.attr('moment-date'), + lowerCaseName: Ember.computed('name', function () { return this.get('name').toLocaleLowerCase(); }) diff --git a/core/client/models/user.js b/core/client/models/user.js index 01dab00d0e..c59be54c5b 100644 --- a/core/client/models/user.js +++ b/core/client/models/user.js @@ -20,6 +20,9 @@ var User = DS.Model.extend(NProgressSaveMixin, ValidationEngine, { meta_description: DS.attr('string'), last_login: DS.attr('moment-date'), created_at: DS.attr('moment-date'), + created_by: DS.attr('number'), + updated_at: DS.attr('moment-date'), + updated_by: DS.attr('number'), roles: DS.hasMany('role', { embedded: 'always' }), diff --git a/core/client/routes/settings/users/index.js b/core/client/routes/settings/users/index.js index 608bf1f77e..55fbda565c 100644 --- a/core/client/routes/settings/users/index.js +++ b/core/client/routes/settings/users/index.js @@ -1,33 +1,20 @@ import PaginationRouteMixin from 'ghost/mixins/pagination-route'; -var activeUsersPaginationSettings = { - include: 'roles', +var paginationSettings = { page: 1, - limit: 20 -}; - -var invitedUsersPaginationSettings = { - include: 'roles', - limit: 'all', - status: 'invited' + limit: 20, + status: 'all' }; var UsersIndexRoute = Ember.Route.extend(Ember.SimpleAuth.AuthenticatedRouteMixin, PaginationRouteMixin, { setupController: function (controller, model) { - this._super(controller, model.active); - this.setupPagination(activeUsersPaginationSettings); + this._super(controller, model); + this.setupPagination(paginationSettings); }, model: function () { - // using `.filter` allows the template to auto-update when new models are pulled in from the server. - // we just need to 'return true' to allow all models by default. - return Ember.RSVP.hash({ - inactive: this.store.filter('user', invitedUsersPaginationSettings, function () { - return true; - }), - active: this.store.filter('user', activeUsersPaginationSettings, function () { - return true; - }) + return this.store.filter('user', paginationSettings, function () { + return true; }); } }); diff --git a/core/client/templates/modals/invite-new-user.hbs b/core/client/templates/modals/invite-new-user.hbs index 51df1a8e2c..8b65c7a6d5 100644 --- a/core/client/templates/modals/invite-new-user.hbs +++ b/core/client/templates/modals/invite-new-user.hbs @@ -1,11 +1,11 @@ {{#gh-modal-dialog action="closeModal" showClose=true type="action" animation="fade" - title="Invite a New User" confirm=confirm class="invite-new-user" }} + title="Invite a New User" confirm=confirm class="invite-new-user"}}
- + {{input class="email" id="new-user-email" type="email" placeholder="Email Address" name="email" autofocus="autofocus" - autocapitalize="off" autocorrect="off" value=email }} + autocapitalize="off" autocorrect="off" value=email}}
diff --git a/core/client/validators/user.js b/core/client/validators/user.js index e7b01695f8..5c386c7e36 100644 --- a/core/client/validators/user.js +++ b/core/client/validators/user.js @@ -13,13 +13,13 @@ var UserValidator = Ember.Object.create({ invited: function (model) { var validationErrors = [], email = model.get('email'), - role = model.get('role'); + roles = model.get('roles'); if (!validator.isEmail(email)) { validationErrors.push({ message: 'Please supply a valid email address' }); } - if (!validator.isLength(role, 1)) { + if (roles.length < 1) { validationErrors.push({ message: 'Please select a role' }); } diff --git a/core/server/api/index.js b/core/server/api/index.js index 056d6a9790..cc1676014c 100644 --- a/core/server/api/index.js +++ b/core/server/api/index.js @@ -107,17 +107,18 @@ cacheInvalidationHeader = function (req, result) { locationHeader = function (req, result) { var apiRoot = config.urlFor('api'), location, - post, - notification, - endpoint = req._parsedUrl.pathname; + newObject; if (req.method === 'POST') { if (result.hasOwnProperty('posts')) { - post = result.posts[0]; - location = apiRoot + '/posts/' + post.id + '/?status=' + post.status; - } else if (endpoint === '/notifications/') { - notification = result.notifications; - location = apiRoot + endpoint + notification[0].id; + newObject = result.posts[0]; + location = apiRoot + '/posts/' + newObject.id + '/?status=' + newObject.status; + } else if (result.hasOwnProperty('notifications')) { + newObject = result.notifications[0]; + location = apiRoot + '/notifications/' + newObject.id; + } else if (result.hasOwnProperty('users')) { + newObject = result.users[0]; + location = apiRoot + '/users/' + newObject.id; } } diff --git a/core/server/api/roles.js b/core/server/api/roles.js index f205e772cd..6244dab3bc 100644 --- a/core/server/api/roles.js +++ b/core/server/api/roles.js @@ -1,5 +1,5 @@ -// # Posts API -// RESTful API for the Post resource +// # Roles API +// RESTful API for the Role resource var when = require('when'), _ = require('lodash'), canThis = require('../permissions').canThis, @@ -18,11 +18,13 @@ roles = { * ### Browse * Find all roles * - * Will return all roles that the current user is able to assign + * If a 'permissions' property is passed in the options object then + * the results will be filtered based on whether or not the context user has the given + * permission on a role. * * * @public - * @param {{context, page, limit, status, staticPages, tag}} options (optional) + * @param {{context, permissions}} options (optional) * @returns {Promise(Roles)} Roles Collection */ browse: function browse(options) { @@ -32,6 +34,7 @@ roles = { return canThis(options.context).browse.role().then(function () { return dataProvider.Role.findAll(options).then(function (foundRoles) { if (options.permissions === 'assign') { + // Hacky implementation of filtering because when.filter is only available in when 3.4.0, // but that's buggy and kills other tests and introduces Heisenbugs. Until we turn everything // to Bluebird, this works. Sorry. diff --git a/core/server/api/users.js b/core/server/api/users.js index 9489c6a83b..0240d85db6 100644 --- a/core/server/api/users.js +++ b/core/server/api/users.js @@ -10,6 +10,7 @@ var when = require('when'), globalUtils = require('../utils'), config = require('../config'), mail = require('./mail'), + rolesAPI = require('./roles'), docName = 'users', ONE_DAY = 60 * 60 * 24 * 1000, @@ -107,7 +108,7 @@ users = { return when.reject(new errors.NotFoundError('User not found.')); }); }, function () { - return when.reject(new errors.NoPermissionError('You do not have permission to edit this users.')); + return when.reject(new errors.NoPermissionError('You do not have permission to edit this user.')); }); }, @@ -145,16 +146,22 @@ users = { } newUser = checkedUserData.users[0]; + newUser.role = parseInt(newUser.roles[0].id || newUser.roles[0], 10); - if (newUser.email) { - newUser.name = object.users[0].email.substring(0, newUser.email.indexOf('@')); - newUser.password = globalUtils.uid(50); - newUser.status = 'invited'; - // TODO: match user role with db and enforce permissions - newUser.role = 3; - } else { - return when.reject(new errors.BadRequestError('No email provided.')); - } + return rolesAPI.browse({ context: options.context, permissions: 'assign' }).then(function (results) { + // Make sure user is allowed to add a user with this role + if (!_.any(results.roles, { id: newUser.role })) { + return when.reject(new errors.NoPermissionError('Not allowed to create user with that role.')); + } + + if (newUser.email) { + newUser.name = object.users[0].email.substring(0, newUser.email.indexOf('@')); + newUser.password = globalUtils.uid(50); + newUser.status = 'invited'; + } else { + return when.reject(new errors.BadRequestError('No email provided.')); + } + }); }).then(function () { return dataProvider.User.getByEmail(newUser.email); }).then(function (foundUser) { @@ -204,9 +211,13 @@ users = { }).otherwise(function (error) { if (error && error.type === 'EmailError') { error.message = 'Error sending email: ' + error.message + ' Please check your email settings and resend the invitation.'; + errors.logWarn(error.message); + // If sending the invitation failed, set status to invited-pending - return dataProvider.User.edit({status: 'invited-pending'}, {id: user.id}).then(function () { - return when.reject(error); + return dataProvider.User.edit({status: 'invited-pending'}, {id: user.id}).then(function (user) { + return dataProvider.User.findOne({ id: user.id }, options).then(function (user) { + return { users: [user] }; + }); }); } return when.reject(error); diff --git a/core/server/models/user.js b/core/server/models/user.js index 979ba2b1b4..a4be909f92 100644 --- a/core/server/models/user.js +++ b/core/server/models/user.js @@ -179,8 +179,8 @@ User = ghostBookshelf.Model.extend({ //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 - // the status provided. - if (options.status) { + // Filter on the status. A status of 'all' translates to no filter since we want all statuses + if (options.status && options.status !== 'all') { // make sure that status is valid //TODO: need a better way of getting a list of statuses other than hard-coding them... options.status = _.indexOf(['active', 'warn-1', 'warn-2', 'warn-3', 'locked', 'invited'], options.status) !== -1 ? options.status : 'active'; @@ -390,7 +390,6 @@ User = ghostBookshelf.Model.extend({ // Save the user with the hashed password return ghostBookshelf.Model.add.call(self, userData, options); }).then(function (addedUser) { - // Assign the userData to our created user so we can pass it back userData = addedUser; if (!data.role) {