Generated tokens are not valid for password reset

[claustres]

Steps to reproduce

Perform a sendResetPwd then a resetPwdLong/resetPwdShort with generated token.

This is a suspected bug following changes from https://github.com/feathers-plus/feathers-authentication-management/pull/68 and linked to https://github.com/feathers-plus/feathers-authentication-management/issues/64. Some remediation seems to have started for short tokens in https://github.com/feathers-plus/feathers-authentication-management/pull/88. Either some good documentation is missing or there is some misconception, here is what I am currently suspecting.

Reset tokens are now stored hashed to harden security in https://github.com/feathers-plus/feathers-authentication-management/blob/master/src/sendResetPwd.js#L48. However in this case there is no way to deconstruct it like required in https://github.com/feathers-plus/feathers-authentication-management/blob/master/src/resetPassword.js#L53. This causes the issue with long tokens because it at least assumes that the ID___ part should be in clear to allow deconstruction, we should probably invert operations here https://github.com/feathers-plus/feathers-authentication-management/blob/master/src/sendResetPwd.js#L41: first hash the token then concat ID with hashed token instead of concat ID and token first then hash the whole.

As far as I now bcrypt compare function works between a clear password and a hashed one. However, when used here https://github.com/feathers-plus/feathers-authentication-management/blob/master/src/resetPassword.js#L69 to compare input token and user token both are hashed, causing the comparison to fail and the issue with short tokens. We should probably send to user the non-hashed token instead of the hashed one here https://github.com/feathers-plus/feathers-authentication-management/blob/master/src/sendResetPwd.js#L55.

Actual behavior

Error raised with long tokens: Token is not in the correct format. Error raised with short tokens: BadRequest: Invalid token. Get for a new one. (authManagement).

It seems that there is no integration tests between resetPwdLong/resetPwdShort and sendResetPwd. Tests are currently using hard-coded reset tokens.

Expected behavior

Password reset with resetPwdLong and resetPwdShort should be possible using short/long tokens generated by sendResetPwd.

Integration tests between resetPwdLong/resetPwdShort and sendResetPwd, see e.g. https://github.com/feathers-plus/feathers-authentication-management/pull/111.

System configuration

Module versions: 2.0.1

NodeJS version: 8.9

Operating System: Windows

[claustres]

It appears my tests were not correct since I used the hashed token instead of the clear token to perform the password reset. I fixed the integration tests and ran the linter.

So no urge to release a new version with my PR https://github.com/feathers-plus/feathers-authentication-management/pull/111, it will just improve testing.

[eddyystop]

Just so I'm sure, the repo is working properly?

[claustres]

Yes it actually works as is.

[eddyystop]

I play to rewrite the repo around Oct, so this is a good time to suggest changes and improvements.

My interests right now are:

• Convert to async/await to simplify the code.
• Make encripting tokens optional, as they are such a pain during testing.
• Allow params to be customized on calls to the user-entity service.
• Provide a template for each type of potential notification.
• Integrate into @feathers-plus/cli.

[claustres]

Converting to async/await should be great because I had some hard time with the sequencing logic behind reset password, will really simplify the code !

I do not think encryption should be optional, just like storing password in plain text, Feathers should enforce a high level security and policy by default. What makes it successful is that you get all the boring things like security out-of-the-box. By the way in tests you can always add a "fake" notifier and catch encrypted tokens like I did in my tests.

We built a modular framework on top of Feathers for our own applications and mainly perform integration testing. kNotify manages all kind of notifications (emails, push) and about retrieving information like tokens from a user gmail inbox you can have a look to https://github.com/kalisio/kNotify/blob/master/test/account.test.js, this is painful but possible.

We also worked in this module on email templating. This can be a good enhancement for your module but please take into account i18n. We also worked on invitation requests. This might also be a good enhancement for your module, although you might tackle it differently, we can discuss about that.

Maybe it is worth opening a dedicated issue ?

[eddyystop]

Done in https://github.com/feathers-plus/feathers-authentication-management/issues/114 . Could you add your suggestions there with more detail? Thanks.