Skip to content

Commit

Permalink
🎨 more clean code in User Model (#7572)
Browse files Browse the repository at this point in the history 
* 🎨  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
  • Loading branch information
@kirrg001 @ErisDS
kirrg001 authored and ErisDS committed Oct 14, 2016
1 parent e1ac6a5 commit ca7b564
Show file tree
Hide file tree
Showing 3 changed files with 136 additions and 73 deletions.
2 changes: 2 additions & 0 deletions core/server/models/base/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
147 changes: 75 additions & 72 deletions core/server/models/user.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
}
Expand All @@ -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);
},
Expand Down Expand Up @@ -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);
},

Expand Down Expand Up @@ -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);

Expand All @@ -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')];
Expand All @@ -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);
Expand All @@ -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) {
Expand Down Expand Up @@ -644,27 +659,22 @@ 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) {
if (!matched) {
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});
});
},

Expand Down Expand Up @@ -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
});
});
},

Expand Down
60 changes: 59 additions & 1 deletion core/test/integration/model/model_users_spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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'));

Expand Down Expand Up @@ -613,7 +648,6 @@ describe('User Model', function run() {
var resetPassword = resetUser.get('password');

should.exist(resetPassword);

resetPassword.should.not.equal(origPassword);

done();
Expand Down Expand Up @@ -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'));

Expand Down

0 comments on commit ca7b564

Please sign in to comment.