Add bcrypt validation by loay · Pull Request #2580 · strongloop/loopback

loay · 2016-08-03T23:02:10Z

connect https://github.com/strongloop-internal/scrum-loopback/issues/994

loay · 2016-08-03T23:02:23Z

@raymondfeng @bajtos PTAL
I couldn't add the fix within the validation function as the hashPassword function uses the validation, so we would get a stack size error as they will be calling each other.
Let me know if this solution is not compatible. Thanks.

bajtos · 2016-08-04T09:32:02Z

@loay let's start with a failing unit test please.

bajtos · 2016-08-04T09:33:15Z

+    if (salt.length < 73) {
+      return bcrypt.hashSync(plain, salt);
+    } else {
+      throw new Error('Validation Error');


Notice that validatePassword provides a helpful error message and HTTP status code 422. We should do the same here.

bajtos · 2016-08-04T09:35:49Z

I couldn't add the fix within the validation function as the hashPassword function uses the validation, so we would get a stack size error as they will be calling each other.

I don't understand. My expectation is that it should be enough to extend the condition in validatePassword to check the password length is less than 72 chars.

Having said that, the error should make it clear to the user that they have entered a password that's too long and also what's the maximum allowed length.

bajtos · 2016-08-04T09:37:50Z

As for test coverage, I would like to see the following test cases (at least):

what happens when the user enters password that's MAXLEN+1 long - a helpful error is returned
what happens when the user enters password that's MAXLEN long - the operation succeeds
when logging in with a correct password that's MAXLEN long, login succeeds
when logging in with a password that's MAXLEN long and has only the last character incorrect, login fails

loay · 2016-08-05T03:50:51Z

@bajtos Fixed. PTAL

bajtos · 2016-08-05T11:41:18Z

      return true;
    }
+    if (plain.length > MAXLENGTH) {
+      err = new Error (g.f('Invalid password exceeds maximum length'));


Please include MAXLEN value in the message.

g.f('Password too long - maximum length is %s', MAXLEN);

superkhau · 2016-08-05T18:37:30Z

+    describe('Password validation with Maxlength', function() {
+      var pass72 = 'ThePasswordShouldNotExceedtheMaximumNumberOfcharactersAndItisInvalidIfIt';
+      var pass73 = 'ThePasswordShouldNotExceedtheMaximumNumberOfcharactersAndItisInvalidIfIt1';
+      var badPass = 'ThePasswordShouldNotExceedtheMaximumNumberOfcharactersAndItisInvalidIfItXYZ1';


Might be more accurate to use Array(72).join('a') or something to represent N chars.

And what is the diff between badpass and pass73? I assume they are both bad.

pass73 is a good password with 73 chars, so the expected behaviour that it cannot be used to neither create nor reset, while badpass is a completely bas password that cannot be used to login after the user is already created.

superkhau · 2016-08-10T21:37:15Z

+        } catch (e) {
+          expect(e).to.match(/Error: Password too long/);
+          done();
+        }


ditto on the try/catch

superkhau · 2016-08-10T21:42:02Z

@loay Added more comments and a few nits.

@bajtos Why do we allow try/catching the callback APIs? I find it really strange how we don't just return the error in the callback accordingly.

raymondfeng · 2016-08-11T22:38:06Z

Make the breaking change, assume there are no users with so long passwords.

IMO, this assumption is reasonable.

raymondfeng · 2016-08-11T22:42:41Z

+          expect(err).to.have.property('message', /Password too long/);
+        });
+      } catch (e) {
+        expect(e).to.match(/Password too long/);


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

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.

bajtos · 2016-08-12T14:22:28Z

@loay IMO, here are the steps remaining steps needed:

rebase on top of the current master
https://github.com/strongloop/loopback/pull/2580/files#r74517648

We should always call validatePasspord. Please note the extended user model can override validatePassword to enforce more rules.

https://github.com/strongloop/loopback/pull/2580/files#r74518751

In that case we can revert bfe674c completely (remove it from this patch)

Just make sure the final commit message mentions the breaking behaviour for password too long.

Now depending on whether we want to fix error handling in this pull request or not, you need to either:

3a) Fix user methods to catch errors thrown by `validatePassword'
or
3b) Fix the tests, as I commented in https://github.com/strongloop/loopback/pull/2580/files#r74598530 and https://github.com/strongloop/loopback/pull/2580/files#r74598777

@raymondfeng @superkhau is there anything else?

raymondfeng · 2016-08-12T15:18:52Z

I had a conversation with @bajtos and agreed to simplify the PR as follows:

We don’t change the behavior for login (even the user already has a long password than 72 chars). The risk of accepting a longer password over 72 chars is very low, even lower than guessing a regular password
The PR just have to validatePassword to make sure no future passwords can go beyond 72 chars. The enforcement should be added for create/reset only.

raymondfeng · 2016-08-12T17:40:56Z

      return cb.promise;
    }

+    if (options.password) {


What is the options.password for?

loay · 2016-08-12T19:11:29Z


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


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.

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

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.

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) return res.sendStatus(404); user.updateAttribute('password', req.body.password, function(err, user) {

raymondfeng · 2016-08-12T22:55:53Z

LGTM

loay · 2016-08-13T03:34:16Z

@slnode test please

#2580

loay added the #review label Aug 3, 2016

bajtos reviewed Aug 4, 2016
View reviewed changes

bajtos self-assigned this Aug 4, 2016

loay force-pushed the bcrypt branch 2 times, most recently from 74a289e to 422198f Compare August 5, 2016 03:32

bajtos reviewed Aug 5, 2016
View reviewed changes

bajtos assigned loay and unassigned bajtos Aug 5, 2016

superkhau reviewed Aug 5, 2016
View reviewed changes

superkhau reviewed Aug 10, 2016
View reviewed changes

raymondfeng reviewed Aug 11, 2016
View reviewed changes

raymondfeng reviewed Aug 12, 2016
View reviewed changes

loay force-pushed the bcrypt branch from 42641ab to 1ed6c81 Compare August 12, 2016 19:10

loay reviewed Aug 12, 2016
View reviewed changes

Add bcrypt validation

7aebf0d

loay force-pushed the bcrypt branch from 1ed6c81 to 7aebf0d Compare August 13, 2016 01:35

loay merged commit 21bdb28 into master Aug 13, 2016

loay deleted the bcrypt branch August 13, 2016 04:37

loay removed the #review label Aug 13, 2016

loay added a commit that referenced this pull request Aug 13, 2016

Add bcrypt validation

436a3aa

#2580

loay mentioned this pull request Aug 13, 2016

Add bcrypt validation #2618

Merged

loay added a commit that referenced this pull request Aug 13, 2016

Add bcrypt validation

a442910

#2580

loay added a commit that referenced this pull request Aug 15, 2016

Add bcrypt validation

d8aa6bd

#2580

bajtos mentioned this pull request Mar 27, 2017

Check max password length in User.changePassword #3312

Merged

2 tasks

bajtos mentioned this pull request Aug 11, 2017

fix(validatePassword): Reword password too long error. #3556

Merged

Uh oh!

Conversation

loay commented Aug 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

loay commented Aug 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bajtos commented Aug 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented Aug 4, 2016

Uh oh!

bajtos commented Aug 4, 2016 • edited by loay Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

loay commented Aug 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

superkhau Aug 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

superkhau commented Aug 10, 2016

Uh oh!

raymondfeng commented Aug 11, 2016

Uh oh!

raymondfeng Aug 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented Aug 12, 2016

Uh oh!

raymondfeng commented Aug 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raymondfeng commented Aug 12, 2016

Uh oh!

loay commented Aug 13, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

loay commented Aug 3, 2016 •

edited

Loading

loay commented Aug 3, 2016 •

edited

Loading

bajtos commented Aug 4, 2016 •

edited by loay

Loading

superkhau Aug 5, 2016 •

edited

Loading

raymondfeng Aug 11, 2016 •

edited

Loading