arkcore / activator

Easy user activation and password reset, with email confirmation, for nodejs
6 stars 2 forks source link

How should I handle password hashing for the password reset middleware? #8

Open bookercodes opened 9 years ago

bookercodes commented 9 years ago

Hello,

I am using Activator together with Mongoose and Passport Local Mongoose.

(_Note:_ Contrary to it's name, Passport Local Mongoose is not coupled with Passport - it is basically a Mongoose plugin that offers convenience functions for user models such as functions relating to password hashing.)

When I want to change a user's password, I call the Passport Local Mongoose setPassword function which generates a salt and hashes the password in a secure manner:

req.user.setPassword(req.body.new_password, function(error, user) {
  user.save(function(error, user) {
    req.flash("messages", "Your password has been changed.");
    res.redirect("/account");
  });
});

(_Note:_ Again, the fact that I am using Passport Local Mongoose here is not really that relevant. I could very well have defined the setPassword function myself.)

I need some way to call setPassword when Activator does a password reset.

At the moment, Activator simply stores the password as is which both insecure and erroneous.

model.save(user[idProperty], {
  $set: { password: password },
  $unset: { password_reset_code: '', password_reset_time: '' }
}, callback);

I tried to write some Mongoose middleware to hash the password on a call to save:

user.pre("update", function(next) {
  // if this is a new document, or if this is an updated document
  // and the password has not changed, skip this method
  if (this.isNew || !this.isModified("password")) {
    next();
    return;
  }
  this.setPassword(this.password, function(err, user) {
    next();
  });
});

However, user.save does not call save, it calls update:

config.user.save = function (id, data, callback) {
  User.update({ _id: id }, data, callback);
};

Mongoose does not allow you to hook update calls in the same way as it does save calls.

I think I need to amend the config.user.save function however the format of the data argument seems tailored for the update function. In other words, I do not know how to take data and use it to call save.

How do you recommend I handle this scenario?

bookercodes commented 9 years ago

I found a solution that works but it is ugly:

config,user.save = function (id, data, callback) {
  if (data.$set.password) {
    User.findById(id, function(error, user) {
      user.setPassword(data.$set.password, function(error, user) {
        user.save(callback);
      })
    });
    return;
  }
  User.update({ _id: id }, data, callback);
}

Do you know of a better way to do this?

AVVS commented 9 years ago

Mongoose have change hooks on fields, look at their examples with bcrypt

On 30 Jun 2015, at 05:18, Alex Booker notifications@github.com wrote:

I found a solution that works but it is ugly:

config,user.save = function (id, data, callback) { if (data.$set.password) { User.findById(id, function(error, user) { user.setPassword(data.$set.password, function(error, user) { user.save(callback); }) }); return; } User.update({ _id: id }, data, callback); } Do you know of a better way to do this?

— Reply to this email directly or view it on GitHub.

bookercodes commented 9 years ago

Can I please have a link? I cannot find anything that resembles field hooks.

bookercodes commented 9 years ago

If you could get back in touch with me I would be most grateful :smile:!

bookercodes commented 9 years ago

That code is actually quite erroneous because the password reset token cannot be removed because of the return statement.

Now I do this instead:

save: function (id, data, callback) {
      User.update({ _id: id }, data, callback);
      if (data.$set.password) {
        User.findById(id, function(error, user) {
          user.setPassword(data.$set.password, function(error, user) {
            user.save(callback);
          })
        });
      }
    }

But I am very interested in the hooks you talk about, @AVVS and would love to know more.

AVVS commented 9 years ago

http://mongoosejs.com/docs/middleware.html http://mongoosejs.com/docs/middleware.html

On Jul 1, 2015, at 11:22 AM, Alex Booker notifications@github.com wrote:

That code is actually quite erroneous because the password reset token cannot be removed because of the return statement.

Now I do this instead:

save: function (id, data, callback) { User.update({ _id: id }, data, callback); if (data.$set.password) { User.findById(id, function(error, user) { user.setPassword(data.$set.password, function(error, user) { user.save(callback); }) }); } } But I am very interested in the hooks you talk about, @AVVS https://github.com/AVVS and would love to know more.

— Reply to this email directly or view it on GitHub https://github.com/arkcore/activator/issues/8#issuecomment-117783012.

bookercodes commented 9 years ago

You said

Mongoose have change hooks on fields, look at their examples with bcrypt

I do not see a change hook on that page or an example with bcrypt.

bookercodes commented 9 years ago

And thank you.