BIG ISSUE: verifySignUp changes user's password

[beeplin]

during verifySignUp, user's password is among the fields to be updated:

users.update(user.id || user._id, user, {}, function (err, user1) {
...
}

password is in the user object and so could be updated:

user:  { _id: xxx,                                       
  username: 'xxxx',                                                             
  email: 'xxxx',                                                                
  password: 'XXXXXXXXXXXXXXX',   
  isVerified: true,                                                           
  verifyExpires: null,                                                        
  verifyToken: null, 
  ...
}                                                         

So here is the problem: if people use auth.hashPassword hook before updating, then the password would be hashed AGAIN.

I don't think password needs to be updated in this case. So it would be good to exclude password from user object before updating.

[beeplin]

and i think patch should be used here rather than update

[beeplin]

just realized saveResetPassword has the same issue. the new password has been hashed twice, if already using auth.hashPassword hook.

Any suggestions about this? Is there a way to tell if the update operation is initiated by server (verifyReset) or from client? If so, we could bypass auth.hashPassword for updates called from server.

[eddyystop]

hook.params.provider is falesy when the operation is initiated by the
server.

I will look into your issue today.

On Mon, Oct 17, 2016 at 9:47 AM, Beep LIN notifications@github.com wrote:

just realized saveResetPassword has the same issue. the new password has
been hashed twice, if already using auth.hashPassword hook.

Any suggestions about this? Is there a way to tell if the update
operation is initiated by server (verifyReset) or from client? If so, we
could bypass auth.hashPassword for updates called from server.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#9 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABezn8Vzov41FuO6GpWfKz4s3YvAZ76rks5q03xygaJpZM4KYlHP
.

[beeplin]

thanks~

i still think it would be better not to hash password or update password unnecessarily. When verifying signup, just patch related fields instead of updating all fields; When changing password, just pass the untouched new password to patch, and let users decide whether to hash it.

[eddyystop]

Thanks for bringing this issue up. Your suggestions are well received and I will work on it later today.

You can condition your hook for now with

const common  = require('feathers-hooks-common');
// replace auth.hashPassword() with:
common.iff(common.isProvider('external'), auth.hashPassword())

[beeplin]

cool~ thanks again for letting me know feathers-hooks-common. powerful.

[eddyystop]

Fixed in 0.5.5.

The user is updated with the patch method for verify (token) and reset (password) to maintain backward compatibility.

A new breaking version of feathers-authentication will be released soon and that would be an ideal time to introduce any breaking changes to this repo.

Thanks for the issue.

[beeplin]

yes, i think the new design (resetting password with patch and not allowing auth.hashPassword for patch) is reasonable, coz once this package is used, we don't want anyone to manually change password. All changes to password must be called through verifyReset.saveResetPassword or veriyReset.changePassword, for safety reasons. That means no need to hashPassword in the before hooks .