Infrastructure/ghost
0
Fork 0
mirror of https://github.com/TryGhost/Ghost.git synced 2025-03-11 02:12:21 -05:00
Code Issues Activity

🎨 more clean code in User Model (#7572)

Browse source 
* 🎨  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
This commit is contained in:
Katharina Irrgang 2016-10-14 19:24:38 +02:00 • committed by Hannah Wolfe
parent e1ac6a53dd
commit ca7b5643d5
3 changed files with 135 additions and 72 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
core/server/models/base/index.js
View file

@ -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;

109
core/server/models/user.js
View file

@ -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,15 +448,11 @@ 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) {
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) {
@ -455,6 +473,11 @@ User = ghostBookshelf.Model.extend({
});
},
/**
* 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;
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
});
});
},

60
core/test/integration/model/model_users_spec.js
View file

@ -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'));