Mongoose: Need to check if password field has been modified in pre findOneAndUpdate

[mitchellporter]

I ran into a problem last night and still haven't been able to figure out a solution so hopefully someone can offer some advice. I am allowing users to update their username and password, and whenever the password field has been modified I need to hash it before storing it in mongo.

So far I had been using this code to hash the user's password:

UserSchema.pre('save', function(next) {
    if (this.isModified('password')) // If the pw has been modified, then encrypt it again
    this.password = this.encryptPassword(this.password);

    next();
});

The encryptPassword function is a custom one I added to my User Schema:

// Add custom methods to our User schema
UserSchema.methods = {
    // Hash the password
    encryptPassword: function(plainTextPassword) {
        if (!plainTextPassword) {
            return ''
        } else {
            var salt = bcrypt.genSaltSync(10);
            return bcrypt.hashSync(plainTextPassword, salt);
        }
    }
};

This only works though when the user first signs up and I create a new user instance and then save it. I need the same functionality when updating the user as well.

I was doing research in past issues here and came across this one: pre, post middleware are not executed on findByIdAndUpdate, and came across a comment where another dev was trying to do something very similar to what I am taking about: https://github.com/Automattic/mongoose/issues/964#issuecomment-154332797

@vkarpov15 responded to his question with the following code suggestion:

TodoSchema.pre('findOneAndUpdate', function() {
  this.findOneAndUpdate({}, { password: hashPassword(this.getUpdate().$set.password) });
});

The only problem is, that this will cause us to rehash the password every time you update a user, which is wrong. You should only be hashing the password when the user initially signs up, and whenever they send a request up to change their password. If you run the above code every single time the user is updated, you will just end up rehashing the old hash, so when the user enters in their password next time on the client, it wont match with the hash stored in mongo anymore...

Is there any way I can use something like this.isModified('password') inside of the .pre('findOneAndUpdate', so that I only hash the password when the user has updated it?

At this point I'm open to any and all suggestions thanks.

[mitchellporter]

I've been testing this some more and thought I would add some more notes. At this point, I can't use user.save or User.update.

If I try to update the user using user.save, I hit a validation error because of a custom validation hook that I setup:

//// Custom error for unique username
UserSchema.path('username').validate(function(value, done) {
    mongoose.model('User', UserSchema).count({ username: value }, function(err, count) {
        if (err) {
            return done(err);
        } 
        // If `count` is greater than zero, "invalidate"
        done(!count);
    });
}, 'Sorry but this username is already taken');

The above is necessary so I can return a custom validation error message to the client if a new user is trying to signup and chooses a username that someone else has already signed up with. If I try to update a user's info by doing user.save, this custom validator runs and throws an error because the username is already taken.

If I try using User.update instead... I can't call this.isModified('password') because this refers to the model and not a document.

Once again, I'm stuck at this point and don't know what to do. Hopefully I've provided enough info. This is the first issue I've hit in mongoose that I haven't been able to solve on my own so any help is greatly appreciated thanks.

[mitchellporter]

Ok so I mentioned in my original comment that @vkarpov15 posted a potential solution in another issue:

TodoSchema.pre('findOneAndUpdate', function() {
  this.findOneAndUpdate({}, { password: hashPassword(this.getUpdate().$set.password) });
});

I have attempted to implement this in a pre hook for findOneAndUpdate and update:

UserSchema.pre('findOneAndUpdate', function(next) {
    this.findOneAndUpdate({}, { password: encryptPassword(this.getUpdate().$set.password) });
    next();
});

This works and I can log the hashed password, but now I can't get it past my password field validation. I have password field validation that says the password can only be 4 characters long. I'm not here to debate about how short or long a password should be allowed to be... the point is that the above hook won't work because it can't get past validation, where as when I was doing the hashing in the pre save hook, it was running after validation had happened.

Still not sure what to do at this point.

[mitchellporter]

So I finally came up with a solution for this. I stopped using the update methods for this specific use case and went back to using save. I deleted the UserSchema.path('username') code that I shared previously and this is what my pre save hook looks like now:

UserSchema.pre('save', function(next, done) {
    var self = this;
    mongoose.models["User"].findOne({username: self.username}, function(err, user) {
        if(err) {
            done(err);
        } else if(user) {            
            if (user._id.equals(self._id)) return next(); // If id's are equal, then we don't care about false dupe username error because its the same user!

            self.invalidate('username', 'Sorry but this username is already taken');
            done(new Error('Sorry but this username is already taken'));
        } else {
            next();
        }
    });
});

In the above code, I check to see if we already have a user in mongo that has the username trying to be saved. If the username already exists, then I compare the _id's of the found user doc and the user doc trying to be saved. If their _id's are equal, then we don't care about the unique username validation error because they're the same user so we just call next() and move on.

This allows my pre save hook for the password hashing to run as well:

UserSchema.pre('save', function(next) {
    console.log('pre save password: ' + this.password);
    if (this.isModified('password')) // If the pw has been modified, then encrypt it again
        this.password = this.encryptPassword(this.password);
    next();
});

So I still get to hash the password if it's updated, and I still get all of my validation rules to run that I have setup for my UserSchema. I have tested this in a demo project and I can't seem to find any flaws with this.

I'm going to close this issue, but will come back after I have integrated this into my production app and leave an update with my progress. I find it hard to believe I'm the only one who has ever had this problem, so I would still love to hear any advice or feedback on the solution I came up with, or how you would do it yourself.

Thanks.

[mitchellporter]

So I tested the solution in my previous comment in a production app and realized that it is flawed and will not work. Unfortunately if I am calling save on the user doc, and the password field has not been updated, so it's still just a hash... it will not pass my validation for my password field.

So it looks like I won't be able to use save without multiple workarounds, which just brings me back to dealing with the update functions and their problems that I already mentioned above.

If anyone has nay idea how I can fix this I would love to hear from you.

[mitchellporter]

Turns out there is no way to do this. Its just a weird edge case you run into when using validation. I ended up removing the mongoose validations for my password field and just manually handled it using a custom regex. This gave me full control and allowed me to accommodate all save/update scenarios for my User model.

[vkarpov15]

Well there is a solution, you use update or findOneAndUpdate and only pass the password field from the client when you mean for the password to be updated. IMO storing the password hash in the user doc is not good practice anyway, because then you need to be extra disciplined about your api not leaking the password hash

[mitchellporter]

IMO storing the password hash in the user doc is not good practice anyway, because then you need to be extra disciplined about your api not leaking the password hash

Interesting. What would an alternative to this look like @vkarpov15?

[vkarpov15]

Separate collection with 2 fields, id and password hash. To check the users password, you query the hashes collection for their hash and check it normally. Makes it easier to fit password modifications in a restful paradigm, and easier to make sure you don't leak the users password when displaying a list of users because getting the users password hash is opt-in

[lgomez]

@mitchellporter hi...

Came across your post while dealing with the same issue. I tried this just now and I think it's gonna cover my needs. Wanted to share it.

const bcrypt = require('bcrypt')
const debug = require('debug')('db')
const mongoose = require('mongoose')
mongoose.Promise = global.Promise;
const { Schema } = mongoose
const { Types } = Schema
const { ObjectId } = Types
const db = mongoose.connection

const UserSchema = new Schema({
  email: { type: String, index: { unique: true, dropDups: true } },
  hash: { type: String, required: true }
})

// This is the important bit
// Using a virtual lets me pass `{ password: 'xyz' }` 
// without actually having it save.
// Instead it is caught by this setter 
// which performs the hashing and 
// saves the hash to the document's hash property.
UserSchema.virtual('password').set(function(value) {
  const salt = bcrypt.genSaltSync(10)
  this.hash = bcrypt.hashSync(value, salt)
})

// A method for checking the password
UserSchema.methods.comparePassword = function(password) {
  return bcrypt.compareSync(password, this.hash)
}

// Here I make sure I never return the
// password in the JSON representation
// Note that I don't do the same to
// `toObject` so i can still see the hash
// in, say, the console.
UserSchema.set('toJSON', {
  getters: true,
  transform: (doc, ret, options) => {
    delete ret.hash;
    return ret;
  }
})

const User = mongoose.model('Store', UserSchema)

module.exports = { db }

This is how this shows up when outputting the object to the console:

{
  _id: '595317e5f3afec2e0453bc64',
  email: '[email protected]',
  hash: '$2a$10$Bovjefiwfdwu/q9liKpdyluQTFIFT/VcrpzUWYPc.bGUYUIFIG',
  __v: 2,
}

And this is how it looks when returned as JSON:

{
  email: '[email protected]',
}

This doesn't separate the hashes into a separate collection like @vkarpov15 suggested which I would like to do sooner than later but perhaps is it helps your case.

[lgomez]

Well... That was a waste of time... Sorry.

The unit test i was using to verify this was wrong. It turns out the virtual only sets that hash in memory and it's not persisted.

So back to figuring out how to hash on updates.

[mitchellporter]

@lgomez I have since shifted away from using MongoDB in my projects so no longer using mongoose either.. good luck with this!

[NicolasBlois]

Maybe this could help ?

schema.pre('update', function(next) {
  this.findOne({"_id":this.getUpdate().$set._id},function(err, doc){
    if(doc.password != this.getUpdate().$set.password){
      this.getUpdate().$set.password = bcrypt.hashSync(this.getUpdate().$set.password, 10);
    }
    next();
  })
});

[vkarpov15]

@NicolasBlois that works. There's a potential race condition there but it'll work for most cases.

[ezamelczyk]

I'm currently using this solution and it works. You don't need to execute another query.

schema.pre("update", function(next) {
            const password = this.getUpdate().$set.password;
            if (!password) {
                return next();
            }
            try {
                const salt = Bcrypt.genSaltSync();
                const hash = Bcrypt.hashSync(password, salt);
                this.getUpdate().$set.password = hash;
                next();
            } catch (error) {
                return next(error);
            }
        });

[aaronfulkerson]

What's the difference between the above solution and something like this to replace the first four lines of the middleware?
if (!this.isModified('password')) return next()

[ezamelczyk]

isModified would be better if your password was a complex object because it traverses all of it's fields in depth, but would probably cost a little more. Though I'd assume it wouldn't make much difference in this use case.

[aaronfulkerson]

Does it traverse all fields when a path is given?

[ezamelczyk]

No idea. Maybe look inside the source code to figure it out?

[aaronfulkerson]

I did but thanks for the suggestion.

[cyonder]

@ezamelczyk thanks for your answer. It works great!

I'll share here how I make the update request for the password; in case people are looking for that.

exports.updateUserPassword = ({_id}, passObj) => {
    const userId = _id;

    if(passObj.newPassword !== passObj.passwordConfirmation){
        throw new Error('New password and confirmation do not match!');
    }

    return new Promise(async(resolve, reject) => {
        try{
            await User.update({ _id: userId }, {
                $set: { password: passObj.newPassword }
            })            
            resolve({})
        }catch(error){
            reject({ message: error.message, error: error })
        }
    })
}

Although, this update is triggered from settings page, not "forgot password" and in the settings page, user needs to enter the current password to be able to update the password. So, I need to check if current password matches. I have comparePassword method available but can't figure out how to call? passObj holds currentPassword, newPassword, passwordConfirmation.

userSchema.methods.comparePassword = function(candidatePassword, callback){
    bcrypt.compare(candidatePassword, this.password, (err, isMatch) => {
        if(err){ return callback(err); }
        callback(null, isMatch);
    });
}

[aminnaggar]

@aaronfulkerson how are you able to access this.isModified in the pre update hook?

As far as I understand you isModified is only applicable in the Document API, while in the pre update hook this refers to the Query instead.

So I don't believe there's another way but the way @ezamelczyk had suggested

[aaronfulkerson]

@aminnaggar I can't remember. I had it working but then I swapped out Mongo for Postgres.

[ar-rahimi]

setting the hashedpassword on getUpdate().$set.password or getUpdate.password, will still store the password unhashed. Instead of this.getUpdate, use this._update. That worked for me.

[juancarloselorriaga]

I solved this issue with the following code, hope it helps

userSchema.pre('findOneAndUpdate', async function() {
  const docToUpdate = await this.model.findOne(this.getQuery())

  if (docToUpdate.password !== this._update.password) {
    const newPassword = await hash(this._update.password, 10)
    this._update.password = newPassword
  }
})