Skip to content

Add bcrypt validation#2580

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
loay merged 1 commit into from
Aug 13, 2016
Merged

Add bcrypt validation #2580

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 17 additions & 5 deletions common/models/user.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -14,7 +14,7 @@ var utils = require('../../lib/utils');
var path = require('path');
var SALT_WORK_FACTOR = 10;
var crypto = require('crypto');

var MAX_PASSWORD_LENGTH = 72;
var bcrypt;
try {
// Try the native module first
Expand DownExpand Up@@ -548,7 +548,6 @@ module.exports = function(User) {
cb = cb || utils.createPromiseCallback();
var UserModel = this;
var ttl = UserModel.settings.resetPasswordTokenTTL || DEFAULT_RESET_PW_TTL;

options = options || {};
if (typeof options.email !== 'string') {
var err = new Error(g.f('Email is required'));
Expand All@@ -558,6 +557,13 @@ module.exports = function(User) {
return cb.promise;
}

try {
if (options.password) {
UserModel.validatePassword(options.password);

@loayloayAug 12, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to use options.password because otherwise length will not be defined and will break few other test cases and cause errors. I mean length in main validatePassword function.

@raymondfengraymondfengAug 12, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. The new password is passed in as options.password.

@bajtosbajtosAug 15, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that's true. resetMethod does not touch the password, it only creates an access token. See also the API docs.

 * Create a short lived acess token for temporary login. Allows users
* to change passwords if forgotten.
*
* @options {Object} options
* @prop {String} email The user's email address
* @callback {Function} callback
* @param {Error} err
* @promise

Let's not worry about that right now and fix it as part of #382.

@bajtosbajtosAug 16, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the documentation explaining how to implement password reset: https://github.com/strongloop/loopback-example-user-management#how-do-you-perform-a-password-reset-for-a-registered-user

Gist - the password is updated via user.updateAttribute, which calls user.updateAttributes under the hood IIRC.

User.findById(req.accessToken.userId,function(err,user){if(err)returnres.sendStatus(404);user.updateAttribute('password',req.body.password,function(err,user){

}
} catch (err) {
return cb(err);
}
UserModel.findOne({ where: { email: options.email }}, function(err, user) {
if (err) {
return cb(err);
Expand DownExpand Up@@ -596,14 +602,20 @@ module.exports = function(User) {
};

User.validatePassword = function(plain) {
if (typeof plain === 'string' && plain) {
var err;
if (plain && typeof plain === 'string' && plain.length <= MAX_PASSWORD_LENGTH) {
return true;
}
var err = new Error(g.f('Invalid password: %s', plain));
if (plain.length > MAX_PASSWORD_LENGTH) {
err = new Error (g.f('Password too long: %s', plain));

@superkhausuperkhauAug 10, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

var err

@superkhausuperkhauAug 10, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually we use the same statusCode for both, we should move it both out.

var err;
if (plain.length > MAX_PASSWORD_LENGTH) {
err = new Error (g.f('Password too long: %s', plain));
err.code = 'PASSWORD_TOO_LONG';
} else {
err = new Error(g.f('Invalid password: %s', plain));
err.code = 'INVALID_PASSWORD'; // maybe add this
}
err.statusCode = 422;
throw err;

@loayloayAug 11, 2016•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I should have refactored that !
fixed in the next commit

err.code = 'PASSWORD_TOO_LONG';
} else {
err = new Error(g.f('Invalid password: %s', plain));
err.code = 'INVALID_PASSWORD';
}

@superkhausuperkhauAug 5, 2016•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should validate the other things first and reject earlier. Instead of testing the valid case above on line 612 first, you should check all the negative cases first. ie:

// if undefined reject
// if not a string reject
// if invalid length reject
now all valid, process accordingly

err.statusCode = 422;
throw err;
};

/*!
* Setup an extended user model.
*/
Expand Down
2 changes: 1 addition & 1 deletion index.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -4,7 +4,7 @@
// License text available at https://opensource.org/licenses/MIT

var SG = require('strong-globalize');
SG.SetRootDir(__dirname, {autonomousMsgLoading: 'all'});
SG.SetRootDir(__dirname, {autonomousMsgLoading: 'all'});

@bajtosbajtosAug 5, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this will pass our linter, please check.

@loayloayAug 5, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah that space was actually added when I ran a linting test. It is still at 1.0.0.
I will fix that once I am done with this PR.


/**
* loopback ~ public api
Expand Down
79 changes: 79 additions & 0 deletions test/user.test.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -363,6 +363,68 @@ describe('User', function() {
});
});

describe('Password length validation', function() {
var pass72Char = new Array(70).join('a') + '012';
var pass73Char = pass72Char + '3';
var passTooLong = pass72Char + 'WXYZ1234';

@superkhausuperkhauAug 10, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be consistent with the other 2. new Array(80).join('a'); or whatever length you prefer.

@loayloayAug 11, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

basically we need the first 72 chars to be identical with passTooLong, and Array(80) would still fulfill that because the word is all a but I wanted something different starting at the 73rd index so we can show how the test fails.

@bajtosbajtosAug 12, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for something different starting at the 73rd index so we can show how the test fails.

varpass72Char=newArray(70).join('a')+'012';varpass73Char=pass72Char+'3';varpassTooLong=pass73Char+'456789';


it('rejects passwords longer than 72 characters', function(done) {
try {
User.create({ email: 'b@c.com', password: pass73Char }, function(err) {
if (err) return done (err);

@bajtosbajtosAug 12, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we decide to keep the current implementation, where invalid passwords throw an error, then in this test you should call

done(newError('User.create() should have thrown an error.'));

done(new Error('User.create() should have thrown an error.'));
});
} catch (e) {
expect(e).to.match(/Password too long/);

@raymondfengraymondfengAug 11, 2016•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is Password too long reported via callback or throw? It should be exclusive.

@superkhausuperkhauAug 11, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is validatePassword is throwing and it's bubbling up. However, we should be try/catching internally and returning the error via callback so it's transparent to the end user.

done();
}

@superkhausuperkhauAug 10, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something is still weird here. If we expect error in the callback API, why do we need try catch. We talked about this over hangout last time @loay. IMO, we should be try/catching before we return the response to the user (they should only have to handle the err returned in the callback. If you decide the try/catch is valid, then we should at least have an assertion in 378 to ensure when the error is throw that we assert it's the right type of error being caught -- assert(e, undefined, 'some-fail-message', '=').

@bajtos Thoughts?

@superkhausuperkhauAug 10, 2016•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually forget the second part of my answer, I completely disagree with having to use try/catch at all with User.create whatsoever. It's fine for new User() though.

@loayloayAug 11, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we tried together, nothing works except the try/catch .. Also, the only way this function is tested elsewhere is also using try/catch. If I had to remove that, I would have to dig more in dao.js and disect the function Create, which is not the goal of this pr because focus would be lost. The test case works and the proper error message is obtained.

@bajtos@raymondfeng Any thoughts?

@superkhausuperkhauAug 11, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@loay I walked through it a bit with you and said dig deeper, not give up and use try/catch as the solution. I find it hard to believe we must throw try/catch for a function that has a callback API. It is probably ok to move forward since you need this merged in ASAP and the previous behaviour also threw for the validation. Anyways, let's get @bajtos to chime in here.

@bajtosbajtosAug 11, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. IIRC, the password validation throws an error. As a result, calling User.create may throw an error. I agree that's weird and it would be better to fix it. However, I think it's ok to leave this out of scope of this patch.

  2. The test is still weird though. The callback function is non-sense, it short-circuits if err is set and then asserts err has some properties. The catch block is wrong too, because it accepts any error as test passing, even TypeError: undefined is not a function.

@loayloayAug 11, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added expect(e).to.match(/Password too long/); within the catch block and it works fine. I will update the next commit

});

it('rejects a new user with password longer than 72 characters', function(done) {
try {
var u = new User({ username: 'foo', password: pass73Char });
assert(false, 'Error should have been thrown');
} catch (e) {
expect(e).to.match(/Password too long/);
done();
}
});

it('accepts passwords that are exactly 72 characters long', function(done) {
User.create({ email: 'b@c.com', password: pass72Char }, function(err, user) {
if (err) return done(err);
User.findById(user.id, function(err, userFound) {
if (err) return done (err);
assert(userFound);
done();
});
});
});

it('allows login with password exactly 72 characters long', function(done) {
User.create({ email: 'b@c.com', password: pass72Char }, function(err) {
if (err) return done(err);
User.login({ email: 'b@c.com', password: pass72Char }, function(err, accessToken) {
if (err) return done(err);
assertGoodToken(accessToken);
assert(accessToken.id);
done();
});
});
});

it('rejects password reset when password is more than 72 chars', function(done) {
User.create({ email: 'b@c.com', password: pass72Char }, function(err) {
if (err) return done (err);
User.resetPassword({ email: 'b@c.com', password: pass73Char }, function(err) {
assert(err);
expect(err).to.match(/Password too long/);
done();
});
});
});
});

describe('Access-hook for queries with email NOT case-sensitive', function() {
it('Should not throw an error if the query does not contain {where: }', function(done) {
User.find({}, function(err) {
Expand DownExpand Up@@ -678,6 +740,23 @@ describe('User', function() {
done();
});
});

it('allows login with password too long but created in old LB version',
function(done) {
var bcrypt = require('bcryptjs');
var longPassword = new Array(80).join('a');
var oldHash = bcrypt.hashSync(longPassword, bcrypt.genSaltSync(1));

User.create({ email: 'b@c.com', password: oldHash }, function(err) {
if (err) return done(err);
User.login({ email: 'b@c.com', password: longPassword }, function(err, accessToken) {
if (err) return done(err);
assert(accessToken.id);
// we are logged in, the test passed

@superkhausuperkhauAug 10, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please assert the user is logged in instead of adding a comment (ie. verify the token, etc).

done();
});
});
});
});

function assertGoodToken(accessToken) {
Expand Down