Allow passwords to be all numbers

[notbrain]

Allow number passwords by converting number to string before hashing.

connect to #2324

[slnode]

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

[superkhau]

Can you add some tests to verify your changes and prevent regressions in the future?

[richardpringle]

I don't think that this check should be removed entirely. What if someone makes a mistake somewhere and plain is of type object? Instead, I think that you should add the check for the number above the check for string.

This could also be cleaned up to one line:

if (typeof plain !== 'string') return;

[reichert621]

It's probably better to take advantage of the validatePassword function instead of just doing an early return... maybe by checking if plain is a string on line 610 below, which allows us to reach the hashPassword function where validatePassword is called:

if (typeof plain === 'string' && plain.indexOf('$2a$') === 0 && plain.length === 60) {
  // ...
} else {
  this.$password = this.constructor.hashPassword(plain);
}

so if plain is of type object, it will get passed along and validated (https://github.com/strongloop/loopback/blob/master/common/models/user.js#L570-L583):

User.hashPassword = function(plain) {
  this.validatePassword(plain);
  // ...
};

User.validatePassword = function(plain) {
  if (typeof plain === 'string' && plain) {
    return true;
  }
  var err =  new Error('Invalid password: ' + plain);
  err.statusCode = 422;
  throw err;
};

at which point it will throw a better error message Invalid password: { pw: 'abc123' }, instead of what we currently get (The User instance is not valid. Details: password can't be blank (value: undefined))

What do you think?

[notbrain]

@reichert621 I like it. But doesn't that still leave the loophole where all-number passwords are still not converted to strings? Your version will throw the error in that case, but I'm not sure it should be up to the framework to say if they should be allowed or not. 18181818 should be converted to '18181818' at some point, and in such a way that an object would not get converted to a string.

[richardpringle]

The enforcement of at least one non-numeric character is a bug as labelled in #2324. The hashing algorithm requires a string of characters but a number should be coerced to a string and that should be the outcome of this PR.

I just want to be clear that the current behavior is not what is expected.

@notbrain, looking forward to seeing the rest of your changes.

[bajtos]

There is one thing I don't understand. How is it possible that the password argument is of type number? I think that points to bug in strong remoting that should have been fixed by strongloop/strong-remoting#343.

Before landing this patch, could you @superkhau or @richardpringle please verify that the problem is present in LoopBack 3.0 too?

Here is the test case I am thinking about:

• request POST /api/users

• send a digits-only password in the body, e.g.

  {
    "email": "[email protected]", 
    "password": "123"
  }

In LB 2.x, I suspect that strong-remoting converts the password property to a number. In LB 3.x, the value should stays as a string.

[slnode]

Can one of the admins verify this patch?

[slnode]

Can one of the admins verify this patch?

[slnode]

Can one of the admins verify this patch?

[slnode]

Can one of the admins verify this patch?

[superkhau]

There is one thing I don't understand. How is it possible that the password argument is of type number?

Good catch, I believe this is wrong too, we need to make sure strings and only numbers work and coerce the numbers into strings as stated by @bajtos. @richardpringle Can you verify this?

[bajtos]

Good catch, I believe this is wrong too, we need to make sure strings and only numbers work and coerce the numbers into strings as stated by

No magic please. Number arguments should be rejected, it's the responsibility of the client to correctly encode model property values (send password as strings, not numbers).

[superkhau]

@notbrain Are you still interesting in working on this? Please address the review comments and add some tests to verify your changes.