From ca7b5643d50b99f5a5e9d19e1337a163fd6ef472 Mon Sep 17 00:00:00 2001 From: Katharina Irrgang Date: Fri, 14 Oct 2016 19:24:38 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=8E=A8=20more=20clean=20code=20in=20User?= =?UTF-8?q?=20Model=20(#7572)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 🎨 do not call generateSlug twice for User.setup * 🎨 call generatePasswordHash onSaving only - now we can add defaults to User Model - it was not possible before because add User model did the following: 1. validate password length 2. hash password manually 3. call ghostBookshelf.Model.add and THEN bookshelf defaults fn gets triggered - call generatePasswordHash in onSaving hook for all use case - add more tests to user model, juhu --- core/server/models/base/index.js | 2 + core/server/models/user.js | 147 +++++++++--------- .../integration/model/model_users_spec.js | 60 ++++++- 3 files changed, 136 insertions(+), 73 deletions(-) diff --git a/core/server/models/base/index.js b/core/server/models/base/index.js index 4a52c47b8b79..ec9a13fff4ee 100644 --- a/core/server/models/base/index.js +++ b/core/server/models/base/index.js @@ -491,10 +491,12 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({ checkIfSlugExists = function checkIfSlugExists(slugToFind) { var args = {slug: slugToFind}; + // status is needed for posts if (options && options.status) { args.status = options.status; } + return Model.findOne(args, options).then(function then(found) { var trimSpace; diff --git a/core/server/models/user.js b/core/server/models/user.js index 111140b09856..0d7b61dcf222 100644 --- a/core/server/models/user.js +++ b/core/server/models/user.js @@ -25,10 +25,11 @@ function validatePasswordLength(password) { return validator.isLength(password, 8); } +/** + * generate a random salt and then hash the password with that salt + */ function generatePasswordHash(password) { - // Generate a new salt return bcryptGenSalt().then(function (salt) { - // Hash the provided password with bcrypt return bcryptHash(password, salt); }); } @@ -37,6 +38,14 @@ User = ghostBookshelf.Model.extend({ tableName: 'users', + defaults: function defaults() { + var baseDefaults = ghostBookshelf.Model.prototype.defaults.call(this); + + return _.merge({ + password: utils.uid(50) + }, baseDefaults); + }, + emitChange: function emitChange(event) { events.emit('user' + '.' + event, this); }, @@ -111,6 +120,29 @@ User = ghostBookshelf.Model.extend({ })(); } + /** + * CASE: add model, hash password + * CASE: update model, hash password + * + * Important: + * - Password hashing happens when we import a database + * - we do some pre-validation checks, because onValidate is called AFTER onSaving + */ + if (self.isNew() || self.hasChanged('password')) { + this.set('password', String(this.get('password'))); + + if (!validatePasswordLength(this.get('password'))) { + return Promise.reject(new errors.ValidationError({message: i18n.t('errors.models.user.passwordDoesNotComplyLength')})); + } + + tasks.hashPassword = (function hashPassword() { + return generatePasswordHash(self.get('password')) + .then(function (hash) { + self.set('password', hash); + }); + })(); + } + return Promise.props(tasks); }, @@ -399,12 +431,6 @@ User = ghostBookshelf.Model.extend({ userData = this.filterData(data), roles; - if (!userData.password) { - userData.password = utils.uid(50); - } - - userData.password = _.toString(userData.password); - options = this.filterOptions(options, 'add'); options.withRelated = _.union(options.withRelated, options.include); @@ -413,10 +439,6 @@ User = ghostBookshelf.Model.extend({ return Promise.reject(new errors.ValidationError({message: i18n.t('errors.models.user.onlyOneRolePerUserSupported')})); } - if (!validatePasswordLength(userData.password)) { - return Promise.reject(new errors.ValidationError({message: i18n.t('errors.models.user.passwordDoesNotComplyLength')})); - } - function getAuthorRole() { return ghostBookshelf.model('Role').findOne({name: 'Author'}, _.pick(options, 'transacting')).then(function then(authorRole) { return [authorRole.get('id')]; @@ -426,35 +448,36 @@ User = ghostBookshelf.Model.extend({ roles = data.roles || getAuthorRole(); delete data.roles; - return generatePasswordHash(userData.password).then(function then(hash) { - // Assign the hashed password - userData.password = hash; - - // Save the user with the hashed password - return ghostBookshelf.Model.add.call(self, userData, options); - }).then(function then(addedUser) { - // Assign the userData to our created user so we can pass it back - userData = addedUser; - // if we are given a "role" object, only pass in the role ID in place of the full object - return Promise.resolve(roles).then(function then(roles) { - roles = _.map(roles, function mapper(role) { - if (_.isString(role)) { - return parseInt(role, 10); - } else if (_.isNumber(role)) { - return role; - } else { - return parseInt(role.id, 10); - } - }); + return ghostBookshelf.Model.add.call(self, userData, options) + .then(function then(addedUser) { + // Assign the userData to our created user so we can pass it back + userData = addedUser; + + // if we are given a "role" object, only pass in the role ID in place of the full object + return Promise.resolve(roles).then(function then(roles) { + roles = _.map(roles, function mapper(role) { + if (_.isString(role)) { + return parseInt(role, 10); + } else if (_.isNumber(role)) { + return role; + } else { + return parseInt(role.id, 10); + } + }); - return addedUser.roles().attach(roles, options); + return addedUser.roles().attach(roles, options); + }); + }).then(function then() { + // find and return the added user + return self.findOne({id: userData.id, status: 'all'}, options); }); - }).then(function then() { - // find and return the added user - return self.findOne({id: userData.id, status: 'all'}, options); - }); }, + /** + * We override the owner! + * Owner already has a slug -> force setting a new one by setting slug to null + * @TODO: kill setup function? + */ setup: function setup(data, options) { var self = this, userData = this.filterData(data); @@ -465,17 +488,9 @@ User = ghostBookshelf.Model.extend({ options = this.filterOptions(options, 'setup'); options.withRelated = _.union(options.withRelated, options.include); - options.shortSlug = true; - - return generatePasswordHash(data.password).then(function then(hash) { - // Assign the hashed password - userData.password = hash; - return ghostBookshelf.Model.generateSlug.call(this, User, userData.name, options); - }).then(function then(slug) { - userData.slug = slug; - return self.edit.call(self, userData, options); - }); + userData.slug = null; + return self.edit.call(self, userData, options); }, permissible: function permissible(userModelOrId, action, context, loadedPermissions, hasUserPermission, hasAppPermission) { @@ -644,17 +659,14 @@ User = ghostBookshelf.Model.extend({ return Promise.reject(new errors.ValidationError({message: i18n.t('errors.models.user.passwordRequiredForOperation')})); } - // If password is not complex enough - if (!validatePasswordLength(newPassword)) { - return Promise.reject(new errors.ValidationError({message: i18n.t('errors.models.user.passwordDoesNotComplyLength')})); - } - return self.forge({id: userId}).fetch({require: true}).then(function then(_user) { user = _user; + // If the user is the current user, check old password if (userId === options.context.user) { return bcryptCompare(oldPassword, user.get('password')); } + // If user is admin and changing another user's password, old password isn't compared to the old one return true; }).then(function then(matched) { @@ -662,9 +674,7 @@ User = ghostBookshelf.Model.extend({ return Promise.reject(new errors.ValidationError({message: i18n.t('errors.models.user.incorrectPassword')})); } - return generatePasswordHash(newPassword); - }).then(function then(hash) { - return user.save({password: hash}); + return user.save({password: newPassword}); }); }, @@ -756,30 +766,23 @@ User = ghostBookshelf.Model.extend({ dbHash = options.dbHash; if (newPassword !== ne2Password) { - return Promise.reject(new errors.ValidationError({message: i18n.t('errors.models.user.newPasswordsDoNotMatch')})); - } - - if (!validatePasswordLength(newPassword)) { - return Promise.reject(new errors.ValidationError({message: i18n.t('errors.models.user.passwordDoesNotComplyLength')})); + return Promise.reject(new errors.ValidationError({ + message: i18n.t('errors.models.user.newPasswordsDoNotMatch') + })); } // Validate the token; returns the email address from token return self.validateToken(utils.decodeBase64URLsafe(token), dbHash).then(function then(email) { - // Fetch the user by email, and hash the password at the same time. - return Promise.join( - self.getByEmail(email), - generatePasswordHash(newPassword) - ); - }).then(function then(results) { - if (!results[0]) { + return self.getByEmail(email); + }).then(function then(foundUser) { + if (!foundUser) { return Promise.reject(new errors.NotFoundError({message: i18n.t('errors.models.user.userNotFound')})); } - // Update the user with the new password hash - var foundUser = results[0], - passwordHash = results[1]; - - return foundUser.save({password: passwordHash, status: 'active'}); + return foundUser.save({ + status: 'active', + password: newPassword + }); }); }, diff --git a/core/test/integration/model/model_users_spec.js b/core/test/integration/model/model_users_spec.js index 6bf92686cb9c..44ee62e00654 100644 --- a/core/test/integration/model/model_users_spec.js +++ b/core/test/integration/model/model_users_spec.js @@ -7,6 +7,7 @@ var testUtils = require('../../utils'), // Stuff we are testing utils = require('../../../server/utils'), + errors = require('../../../server/errors'), gravatar = require('../../../server/utils/gravatar'), UserModel = require('../../../server/models/user').User, RoleModel = require('../../../server/models/role').Role, @@ -542,6 +543,40 @@ describe('User Model', function run() { }); }); + describe('Password change', function () { + beforeEach(testUtils.setup('users:roles')); + + describe('error', function () { + it('wrong old password', function (done) { + UserModel.changePassword({ + newPassword: '12345678', + ne2Password: '12345678', + oldPassword: '123456789', + user_id: 1 + }, testUtils.context.owner).then(function () { + done(new Error('expected error!')); + }).catch(function (err) { + (err instanceof errors.ValidationError).should.eql(true); + done(); + }); + }); + }); + + describe('success', function () { + it('can change password', function (done) { + UserModel.changePassword({ + newPassword: '12345678', + ne2Password: '12345678', + oldPassword: 'Sl1m3rson', + user_id: 1 + }, testUtils.context.owner).then(function (user) { + user.get('password').should.not.eql('12345678'); + done(); + }).catch(done); + }); + }); + }); + describe('Password Reset', function () { beforeEach(testUtils.setup('users:roles')); @@ -613,7 +648,6 @@ describe('User Model', function run() { var resetPassword = resetUser.get('password'); should.exist(resetPassword); - resetPassword.should.not.equal(origPassword); done(); @@ -675,6 +709,30 @@ describe('User Model', function run() { }); }); + describe('User setup', function () { + beforeEach(testUtils.setup('owner')); + + it('setup user', function (done) { + var userData = { + name: 'Max Mustermann', + email: 'test@ghost.org', + password: '12345678' + }; + + UserModel.setup(userData, {id: 1}) + .then(function (user) { + user.get('name').should.eql(userData.name); + user.get('email').should.eql(userData.email); + user.get('slug').should.eql('max'); + + // naive check that password was hashed + user.get('password').should.not.eql(userData.password); + done(); + }) + .catch(done); + }); + }); + describe('User Login', function () { beforeEach(testUtils.setup('owner'));