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.
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
Need to check if password field has been modified in pre findOneAndUpdate #4575
Comments
I've been testing this some more and thought I would add some more notes. At this point, I can't use If I try to update the user using
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 If I try using 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. |
Ok so I mentioned in my original comment that @vkarpov15 posted a potential solution in another issue:
I have attempted to implement this in a pre hook for
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 Still not sure what to do at this point. |
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
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 This allows my pre save hook for the password hashing to run as well:
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 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. |
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 So it looks like I won't be able to use If anyone has nay idea how I can fix this I would love to hear from you. |
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 |
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 |
Interesting. What would an alternative to this look like @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 |
@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: 'user@example.com',
hash: '$2a$10$Bovjefiwfdwu/q9liKpdyluQTFIFT/VcrpzUWYPc.bGUYUIFIG',
__v: 2,
} And this is how it looks when returned as JSON: {
email: 'user@example.com',
} 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. |
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. |
@lgomez I have since shifted away from using |
Maybe this could help ?
|
@NicolasBlois that works. There's a potential race condition there but it'll work for most cases. |
I'm currently using this solution and it works. You don't need to execute another query.
|
What's the difference between the above solution and something like this to replace the first four lines of the middleware? |
|
Does it traverse all fields when a path is given? |
No idea. Maybe look inside the source code to figure it out? |
I did but thanks for the suggestion. |
@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.
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.
|
@aaronfulkerson how are you able to access As far as I understand you So I don't believe there's another way but the way @ezamelczyk had suggested |
@aminnaggar I can't remember. I had it working but then I swapped out Mongo for Postgres. |
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. |
I solved this issue with the following code, hope it helps
|
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:
The
encryptPassword
function is a custom one I added to myUser Schema
: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: #964 (comment)
@vkarpov15 responded to his question with the following code suggestion:
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.
The text was updated successfully, but these errors were encountered: