tqt
/
Ghost
mirror of https://github.com/TryGhost/Ghost.git
2
1
Fork 0
Code Issues 1 Releases Wiki Activity

🎨 remove token logic from user model (#7622) 

* 🔥  remove User model functions

- validateToken
- generateToken
- resetPassword
- all this logic will re-appear in a different way

Token logic:
- was already extracted as separate PR, see https://github.com/TryGhost/Ghost/pull/7554
- we will use this logic in the controller, you will see in the next commits

Reset Password:
Was just a wrapper for calling the token logic and change the password.
We can reconsider keeping the function to call: changePassword and activate the status of the user - but i think it's fine to trigger these two actions from the controlling unit.

* 🔥  remove password reset tests from User model

- we already have unit tests for change password and the token logic
- i will re-check at the end if any test case is missing - but for now i will just burn the tests

* ✨  add token logic to controlling unit

generateResetToken endpoint
- the only change here is instead of calling the User model to generate a token, we generate the token via utils
- we fetch the user by email, and generate a hash and return

resetPassword endpoint
- here we have changed a little bit more
- first of all: we have added the validation check if the new passwords match
- a new helper method to extract the token informations
- the brute force security check, which can be handled later from the new bruteforce middleware (see TODO)
- the actual reset function is doing the steps: load me the user, compare the token, change the password and activate the user
- we can think of wrapping these steps into a User model function
- i was not sure about it, because it is actually part of the controlling unit

[ci skip]

* 🎨  tidy up

- jscs
- jshint
- naming functions
- fixes

* ✨  add a test for resetting the password

- there was none
- added a test to reset the password

* 🎨  add more token tests

- ensure quality
- ensure logic we had

* 🔥  remove compare new password check from User Model

- this part of controlling unit

* ✨  compare new passwords for user endpoint

- we deleted the logic in User Model
- we are adding the logic to controlling unit

* 🐛  spam prevention forgotten can crash

- no validation happend before this middleware
- it just assumes that the root key is present
- when we work on our API, we need to ensure that
  1. pre validation happens
  2. we call middlewares
  3. ...

* 🎨  token translation key
This commit is contained in:
Katharina Irrgang 2016-11-07 12:18:50 +01:00 committed by Hannah Wolfe
parent a6226e4832
commit 4e7779b783
9 changed files with 218 additions and 300 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

158
core/server/api/authentication.js
View File

@ -13,7 +13,8 @@ var _ = require('lodash'),
events = require('../events'),
config = require('../config'),
i18n = require('../i18n'),
authentication;
authentication,
tokenSecurity = {};
/**
* Returns setup status
@ -164,14 +165,14 @@ authentication = {
/**
* @description generate a reset token for a given email address
* @param {Object} resetRequest
* @param {Object} object
* @returns {Promise<Object>} message
*/
generateResetToken: function generateResetToken(resetRequest) {
generateResetToken: function generateResetToken(object) {
var tasks;
function validateRequest(resetRequest) {
return utils.checkObject(resetRequest, 'passwordreset').then(function then(data) {
function validateRequest(object) {
return utils.checkObject(object, 'passwordreset').then(function then(data) {
var email = data.passwordreset[0].email;
if (typeof email !== 'string' || !validator.isEmail(email)) {
@ -185,19 +186,32 @@ authentication = {
}
function generateToken(email) {
var settingsQuery = {context: {internal: true}, key: 'dbHash'};
var options = {context: {internal: true}},
dbHash, token;
return settings.read(settingsQuery).then(function then(response) {
var dbHash = response.settings[0].value,
expiresAt = Date.now() + globalUtils.ONE_DAY_MS;
return settings.read(_.merge({key: 'dbHash'}, options))
.then(function fetchedSettings(response) {
dbHash = response.settings[0].value;
return models.User.generateResetToken(email, expiresAt, dbHash);
}).then(function then(resetToken) {
return {
email: email,
resetToken: resetToken
};
});
return models.User.getByEmail(email, options);
})
.then(function fetchedUser(user) {
if (!user) {
throw new errors.NotFoundError({message: i18n.t('errors.api.users.userNotFound')});
}
token = globalUtils.tokens.resetToken.generateHash({
expires: Date.now() + globalUtils.ONE_DAY_MS,
email: email,
dbHash: dbHash,
password: user.get('password')
});
return {
email: email,
resetToken: token
};
});
}
function sendResetNotification(data) {
@ -244,39 +258,103 @@ authentication = {
formatResponse
];
return pipeline(tasks, resetRequest);
return pipeline(tasks, object);
},
/**
* ## Reset Password
* reset password if a valid token and password (2x) is passed
* @param {Object} resetRequest
* @param {Object} object
* @returns {Promise<Object>} message
*/
resetPassword: function resetPassword(resetRequest) {
var tasks;
resetPassword: function resetPassword(object) {
var tasks, tokenIsCorrect, dbHash, options = {context: {internal: true}}, tokenParts;
function validateRequest(resetRequest) {
return utils.checkObject(resetRequest, 'passwordreset');
function validateRequest() {
return utils.validate('passwordreset')(object, options)
.then(function (options) {
var data = options.data.passwordreset[0];
if (data.newPassword !== data.ne2Password) {
return Promise.reject(new errors.ValidationError({
message: i18n.t('errors.models.user.newPasswordsDoNotMatch')
}));
}
return Promise.resolve(options);
});
}
function doReset(resetRequest) {
var settingsQuery = {context: {internal: true}, key: 'dbHash'},
data = resetRequest.passwordreset[0],
resetToken = data.token,
newPassword = data.newPassword,
ne2Password = data.ne2Password;
function extractTokenParts(options) {
options.data.passwordreset[0].token = globalUtils.decodeBase64URLsafe(options.data.passwordreset[0].token);
return settings.read(settingsQuery).then(function then(response) {
return models.User.resetPassword({
token: resetToken,
newPassword: newPassword,
ne2Password: ne2Password,
dbHash: response.settings[0].value
});
}).catch(function (err) {
throw new errors.UnauthorizedError({err: err});
tokenParts = globalUtils.tokens.resetToken.extract({
token: options.data.passwordreset[0].token
});
if (!tokenParts) {
return Promise.reject(new errors.UnauthorizedError({
message: i18n.t('errors.api.common.invalidTokenStructure')
}));
}
return Promise.resolve(options);
}
// @TODO: use brute force middleware (see https://github.com/TryGhost/Ghost/pull/7579)
function protectBruteForce(options) {
if (tokenSecurity[tokenParts.email + '+' + tokenParts.expires] &&
tokenSecurity[tokenParts.email + '+' + tokenParts.expires].count >= 10) {
return Promise.reject(new errors.NoPermissionError({
message: i18n.t('errors.models.user.tokenLocked')
}));
}
return Promise.resolve(options);
}
function doReset(options) {
var data = options.data.passwordreset[0],
resetToken = data.token,
oldPassword = data.oldPassword,
newPassword = data.newPassword;
return settings.read(_.merge({key: 'dbHash'}, options))
.then(function fetchedSettings(response) {
dbHash = response.settings[0].value;
return models.User.getByEmail(tokenParts.email, options);
})
.then(function fetchedUser(user) {
if (!user) {
throw new errors.NotFoundError({message: i18n.t('errors.api.users.userNotFound')});
}
tokenIsCorrect = globalUtils.tokens.resetToken.compare({
token: resetToken,
dbHash: dbHash,
password: user.get('password')
});
if (!tokenIsCorrect) {
return Promise.reject(new errors.BadRequestError({
message: i18n.t('errors.api.common.invalidTokenStructure')
}));
}
return models.User.changePassword({
oldPassword: oldPassword,
newPassword: newPassword,
user_id: user.id
}, options);
})
.then(function changedPassword(updatedUser) {
updatedUser.set('status', 'active');
return updatedUser.save(options);
})
.catch(function (err) {
throw new errors.UnauthorizedError({err: err});
});
}
function formatResponse() {
@ -288,13 +366,15 @@ authentication = {
}
tasks = [
assertSetupCompleted(true),
validateRequest,
assertSetupCompleted(true),
extractTokenParts,
protectBruteForce,
doReset,
formatResponse
];
return pipeline(tasks, resetRequest);
return pipeline(tasks, object, options);
},
/**

17
core/server/api/users.js
View File

@ -269,6 +269,21 @@ users = {
changePassword: function changePassword(object, options) {
var tasks;
function validateRequest() {
return utils.validate('password')(object, options)
.then(function (options) {
var data = options.data.password[0];
if (data.newPassword !== data.ne2Password) {
return Promise.reject(new errors.ValidationError({
message: i18n.t('errors.models.user.newPasswordsDoNotMatch')
}));
}
return Promise.resolve(options);
});
}
/**
* ### Handle Permissions
* We need to be an authorised user to perform this action
@ -301,7 +316,7 @@ users = {
// Push all of our tasks into a `tasks` array in the correct order
tasks = [
utils.validate('password'),
validateRequest,
handlePermissions,
utils.convertOptions(allowedIncludes),
doQuery

7
core/server/middleware/api/spam-prevention.js
View File

@ -54,7 +54,14 @@ spamPrevention = {
// limit forgotten password requests to five requests per IP per hour for different email addresses
// limit forgotten password requests to five requests per email address
// @TODO: add validation check to validation middleware
forgotten: function forgotten(req, res, next) {
if (!req.body.passwordreset) {
return next(new errors.BadRequestError({
message: i18n.t('errors.api.utils.noRootKeyProvided', {docName: 'passwordreset'})
}));
}
var currentTime = process.hrtime()[0],
remoteAddress = req.connection.remoteAddress,
rateForgottenPeriod = config.rateForgottenPeriod || 3600,

116
core/server/models/user.js
View File

@ -1,7 +1,6 @@
var _ = require('lodash'),
Promise = require('bluebird'),
bcrypt = require('bcryptjs'),
crypto = require('crypto'),
validator = require('validator'),
ghostBookshelf = require('./base'),
errors = require('../errors'),
@ -16,7 +15,6 @@ var _ = require('lodash'),
bcryptHash = Promise.promisify(bcrypt.hash),
bcryptCompare = Promise.promisify(bcrypt.compare),
tokenSecurity = {},
activeStates = ['active', 'warn-1', 'warn-2', 'warn-3', 'warn-4', 'locked'],
User,
Users;
@ -680,17 +678,11 @@ User = ghostBookshelf.Model.extend({
changePassword: function changePassword(object, options) {
var self = this,
newPassword = object.newPassword,
ne2Password = object.ne2Password,
userId = parseInt(object.user_id),
oldPassword = object.oldPassword,
isLoggedInUser = userId === options.context.user,
user;
// If the two passwords do not match
if (newPassword !== ne2Password) {
return Promise.reject(new errors.ValidationError({message: i18n.t('errors.models.user.newPasswordsDoNotMatch')}));
}
return self.forge({id: userId}).fetch({require: true})
.then(function then(_user) {
user = _user;
@ -707,114 +699,6 @@ User = ghostBookshelf.Model.extend({
});
},
generateResetToken: function generateResetToken(email, expires, dbHash) {
return this.getByEmail(email).then(function then(foundUser) {
if (!foundUser) {
return Promise.reject(new errors.NotFoundError({message: i18n.t('errors.models.user.noUserWithEnteredEmailAddr')}));
}
var hash = crypto.createHash('sha256'),
text = '';
// Token:
// BASE64(TIMESTAMP + email + HASH(TIMESTAMP + email + oldPasswordHash + dbHash ))
hash.update(String(expires));
hash.update(email.toLocaleLowerCase());
hash.update(foundUser.get('password'));
hash.update(String(dbHash));
text += [expires, email, hash.digest('base64')].join('|');
return new Buffer(text).toString('base64');
});
},
validateToken: function validateToken(token, dbHash) {
/*jslint bitwise:true*/
// TODO: Is there a chance the use of ascii here will cause problems if oldPassword has weird characters?
var tokenText = new Buffer(token, 'base64').toString('ascii'),
parts,
expires,
email;
parts = tokenText.split('|');
// Check if invalid structure
if (!parts || parts.length !== 3) {
return Promise.reject(new errors.BadRequestError({message: i18n.t('errors.models.user.invalidTokenStructure')}));
}
expires = parseInt(parts[0], 10);
email = parts[1];
if (isNaN(expires)) {
return Promise.reject(new errors.BadRequestError({message: i18n.t('errors.models.user.invalidTokenExpiration')}));
}
// Check if token is expired to prevent replay attacks
if (expires < Date.now()) {
return Promise.reject(new errors.ValidationError({message: i18n.t('errors.models.user.expiredToken')}));
}
// to prevent brute force attempts to reset the password the combination of email+expires is only allowed for
// 10 attempts
if (tokenSecurity[email + '+' + expires] && tokenSecurity[email + '+' + expires].count >= 10) {
return Promise.reject(new errors.NoPermissionError({message: i18n.t('errors.models.user.tokenLocked')}));
}
return this.generateResetToken(email, expires, dbHash).then(function then(generatedToken) {
// Check for matching tokens with timing independent comparison
var diff = 0,
i;
// check if the token length is correct
if (token.length !== generatedToken.length) {
diff = 1;
}
for (i = token.length - 1; i >= 0; i = i - 1) {
diff |= token.charCodeAt(i) ^ generatedToken.charCodeAt(i);
}
if (diff === 0) {
return email;
}
// increase the count for email+expires for each failed attempt
tokenSecurity[email + '+' + expires] = {
count: tokenSecurity[email + '+' + expires] ? tokenSecurity[email + '+' + expires].count + 1 : 1
};
return Promise.reject(new errors.BadRequestError({message: i18n.t('errors.models.user.invalidToken')}));
});
},
resetPassword: function resetPassword(options) {
var self = this,
token = options.token,
newPassword = options.newPassword,
ne2Password = options.ne2Password,
dbHash = options.dbHash;
if (newPassword !== ne2Password) {
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) {
return self.getByEmail(email);
}).then(function then(foundUser) {
if (!foundUser) {
return Promise.reject(new errors.NotFoundError({message: i18n.t('errors.models.user.userNotFound')}));
}
return foundUser.save({
status: 'active',
password: newPassword
});
});
},
transferOwnership: function transferOwnership(object, options) {
var ownerRole,
contextUser;

5
core/server/translations/en.json
View File

@ -245,8 +245,6 @@
"accountLocked": "Your account is locked. Please reset your password to log in again by clicking the \"Forgotten password?\" link!",
"newPasswordsDoNotMatch": "Your new passwords do not match",
"passwordRequiredForOperation": "Password is required for this operation",
"invalidTokenStructure": "Invalid token structure",
"invalidTokenExpiration": "Invalid token expiration",
"expiredToken": "Expired token",
"tokenLocked": "Token locked",
"invalidToken": "Invalid token",
@ -289,6 +287,9 @@
}
},
"api": {
"common": {
"invalidTokenStructure": "Invalid token structure"
},
"authentication": {
"setupUnableToRun": "Database missing fixture data. Please reset database and try again.",
"setupMustBeCompleted": "Setup must be completed before making this request.",

8
core/server/utils/tokens.js
View File

@ -11,10 +11,10 @@ exports.resetToken = {
password = options.password,
text = '';
hash.update(String(expires).toLocaleLowerCase());
hash.update(String(email).toLocaleLowerCase());
hash.update(String(dbHash).toLocaleLowerCase());
hash.update(String(password).toLocaleLowerCase());
hash.update(String(expires));
hash.update(email.toLocaleLowerCase());
hash.update(password);
hash.update(String(dbHash));
text += [expires, email, hash.digest('base64')].join('|');
return new Buffer(text).toString('base64');

37
core/test/integration/api/api_authentication_spec.js
View File

@ -4,6 +4,7 @@ var testUtils = require('../../utils'),
sinon = require('sinon'),
Promise = require('bluebird'),
uid = require('../../../server/utils').uid,
globalUtils = require('../../../server/utils'),
AuthAPI = require('../../../server/api/authentication'),
mail = require('../../../server/api/mail'),
models = require('../../../server/models'),
@ -14,6 +15,7 @@ var testUtils = require('../../utils'),
Refreshtoken,
User;
// @TODO: group tests by api call, not by setup completed or not
describe('Authentication API', function () {
var testInvite = {
invitation: [{
@ -31,6 +33,7 @@ describe('Authentication API', function () {
testReset = {
passwordreset: [{
token: 'abc',
oldPassword: 'Sl1m3rson',
newPassword: 'abcdefgh',
ne2Password: 'abcdefgh'
}]
@ -347,7 +350,7 @@ describe('Authentication API', function () {
}).catch(done);
});
it('should allow a password reset', function (done) {
it('should NOT allow a password reset', function (done) {
AuthAPI.resetPassword(testReset).then(function () {
done(new Error('password reset did not fail on token validation'));
}).catch(function (err) {
@ -361,6 +364,38 @@ describe('Authentication API', function () {
}).catch(done);
});
it('should allow a password reset', function (done) {
sandbox.stub(globalUtils.tokens.resetToken, 'generateHash').returns('valid-token');
sandbox.stub(globalUtils.tokens.resetToken, 'extract').returns({email: 'jbloggs@example.com', expires: Date.now() + (60 * 1000)});
sandbox.stub(globalUtils.tokens.resetToken, 'compare').returns(true);
models.User.edit({status: 'locked'}, {id: 1})
.then(function (user) {
user.get('status').should.eql('locked');
return AuthAPI.generateResetToken(testGenerateReset);
})
.then(function () {
return AuthAPI.resetPassword(testReset);
})
.then(function () {
return models.User.findOne({id: 1});
})
.then(function (user) {
user.get('status').should.eql('active');
return models.User.isPasswordCorrect({
plainPassword: testReset.passwordreset[0].newPassword,
hashedPassword: user.get('password')
});
})
.then(function () {
done();
})
.catch(function (err) {
done(err);
});
});
it('should allow an access token to be revoked', function (done) {
var id = uid(191);

134
core/test/integration/model/model_users_spec.js
View File

@ -2,11 +2,9 @@ var testUtils = require('../../utils'),
should = require('should'),
Promise = require('bluebird'),
sinon = require('sinon'),
uuid = require('node-uuid'),
_ = require('lodash'),
// Stuff we are testing
utils = require('../../../server/utils'),
errors = require('../../../server/errors'),
gravatar = require('../../../server/utils/gravatar'),
UserModel = require('../../../server/models/user').User,
@ -591,138 +589,6 @@ describe('User Model', function run() {
});
});
describe('Password Reset', function () {
beforeEach(testUtils.setup('users:roles'));
it('can generate reset token', function (done) {
// Expires in one minute
var expires = Date.now() + 60000,
dbHash = uuid.v4();
UserModel.findAll().then(function (results) {
return UserModel.generateResetToken(results.models[0].attributes.email, expires, dbHash);
}).then(function (token) {
should.exist(token);
token.length.should.be.above(0);
done();
}).catch(done);
});
it('can validate a reset token', function (done) {
// Expires in one minute
var expires = Date.now() + 60000,
dbHash = uuid.v4();
UserModel.findAll().then(function (results) {
return UserModel.generateResetToken(results.models[1].attributes.email, expires, dbHash);
}).then(function (token) {
return UserModel.validateToken(token, dbHash);
}).then(function () {
done();
}).catch(done);
});
it('can validate an URI encoded reset token', function (done) {
// Expires in one minute
var expires = Date.now() + 60000,
dbHash = uuid.v4();
UserModel.findAll().then(function (results) {
return UserModel.generateResetToken(results.models[1].attributes.email, expires, dbHash);
}).then(function (token) {
token = utils.encodeBase64URLsafe(token);
token = encodeURIComponent(token);
token = decodeURIComponent(token);
token = utils.decodeBase64URLsafe(token);
return UserModel.validateToken(token, dbHash);
}).then(function () {
done();
}).catch(done);
});
it('can reset a password with a valid token', function (done) {
// Expires in one minute
var origPassword,
expires = Date.now() + 60000,
dbHash = uuid.v4();
UserModel.findAll().then(function (results) {
var firstUser = results.models[0],
origPassword = firstUser.attributes.password;
should.exist(origPassword);
return UserModel.generateResetToken(firstUser.attributes.email, expires, dbHash);
}).then(function (token) {
token = utils.encodeBase64URLsafe(token);
return UserModel.resetPassword({token: token, newPassword: 'newpassword', ne2Password: 'newpassword', dbHash: dbHash});
}).then(function (resetUser) {
var resetPassword = resetUser.get('password');
should.exist(resetPassword);
resetPassword.should.not.equal(origPassword);
done();
}).catch(done);
});
it('doesn\'t allow expired timestamp tokens', function (done) {
var email,
// Expired one minute ago
expires = Date.now() - 60000,
dbHash = uuid.v4();
UserModel.findAll().then(function (results) {
// Store email for later
email = results.models[0].attributes.email;
return UserModel.generateResetToken(email, expires, dbHash);
}).then(function (token) {
return UserModel.validateToken(token, dbHash);
}).then(function () {
throw new Error('Allowed expired token');
}).catch(function (err) {
should.exist(err);
err.message.should.equal('Expired token');
done();
});
});
it('doesn\'t allow tampered timestamp tokens', function (done) {
// Expired one minute ago
var expires = Date.now() - 60000,
dbHash = uuid.v4();
UserModel.findAll().then(function (results) {
return UserModel.generateResetToken(results.models[0].attributes.email, expires, dbHash);
}).then(function (token) {
var tokenText = new Buffer(token, 'base64').toString('ascii'),
parts = tokenText.split('|'),
fakeExpires,
fakeToken;
fakeExpires = Date.now() + 60000;
fakeToken = [String(fakeExpires), parts[1], parts[2]].join('|');
fakeToken = new Buffer(fakeToken).toString('base64');
return UserModel.validateToken(fakeToken, dbHash);
}).then(function () {
throw new Error('allowed invalid token');
}).catch(function (err) {
should.exist(err);
err.message.should.equal('Invalid token');
done();
});
});
});
describe('User setup', function () {
beforeEach(testUtils.setup('owner'));

36
core/test/unit/utils/tokens_spec.js
View File

@ -12,6 +12,7 @@ describe('Utils: tokens', function () {
token = utils.tokens.resetToken.generateHash({
email: 'test1@ghost.org',
expires: expires,
password: 'password',
dbHash: dbHash
});
@ -80,12 +81,34 @@ describe('Utils: tokens', function () {
should.not.exist(parts.dbHash);
});
it('can validate an URI encoded reset token', function () {
it('extract', function () {
var expires = Date.now() + 60 * 1000,
dbHash = uuid.v4(), token, tokenIsCorrect;
dbHash = uuid.v4(), token, parts, email = 'test3@ghost.org';
token = utils.tokens.resetToken.generateHash({
email: 'test1@ghost.org',
email: email,
expires: expires,
password: '$2a$10$t5dY1uRRdjvqfNlXhae3uuc0nuhi.Rd7/K/9JaHHwSkLm6UUa3NsW',
dbHash: dbHash
});
parts = utils.tokens.resetToken.extract({
token: token
});
parts.email.should.eql(email);
parts.expires.should.eql(expires);
should.not.exist(parts.password);
should.not.exist(parts.dbHash);
});
it('can validate an URI encoded reset token', function () {
var expires = Date.now() + 60 * 1000,
email = 'test1@ghost.org',
dbHash = uuid.v4(), token, tokenIsCorrect, parts;
token = utils.tokens.resetToken.generateHash({
email: email,
expires: expires,
password: '12345678',
dbHash: dbHash
@ -96,6 +119,13 @@ describe('Utils: tokens', function () {
token = decodeURIComponent(token);
token = utils.decodeBase64URLsafe(token);
parts = utils.tokens.resetToken.extract({
token: token
});
parts.email.should.eql(email);
parts.expires.should.eql(expires);
tokenIsCorrect = utils.tokens.resetToken.compare({
token: token,
dbHash: dbHash,