Skip to content
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

Allow passwords to be all numbers #2335

Closed
wants to merge 1 commit into from
Closed

Allow passwords to be all numbers #2335

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
4 changes: 2 additions & 2 deletions common/models/user.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -604,8 +604,8 @@ module.exports = function(User) {
};

UserModel.setter.password = function(plain) {
if (typeof plain !== 'string') {
return;
if (typeof plain === 'number') {
plain = plain.toString();
Copy link
Contributor

Choose a reason for hiding this comment

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

Not necessary but could be cleaned up to one line as suggested in my other comment.

}
Copy link
Contributor

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 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;

Copy link

@reichert621 reichert621 May 13, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

if (plain.indexOf('$2a$') === 0 && plain.length === 60) {
// The password is already hashed. It can be the case
Expand Down