feathersjs-ecosystem / feathers-authentication-management

Adds sign up verification, forgotten password reset, and other capabilities to local feathers-authentication
https://feathers-a-m.netlify.app/
MIT License
246 stars 98 forks source link

Purpose of resetShortToken #83

Closed fiddler closed 6 years ago

fiddler commented 6 years ago

I have a question on the purpose of the resetShortToken. I understood that it would be a way for the user to receive a human-readable/writable reset token via text message for example and be able to reset their password using that code and their email as an identifier for the user. Is that the purpose of it?

Due to changes made here https://github.com/feathers-plus/feathers-authentication-management/pull/68 it's no longer possible. That adds the user id to the token and makes it so that the resetShortToken is no longer human readable/writable as it requires the user id in it to be usable.

Have I misunderstood the purpose of resetShortToken and is this working as intended?

eddyystop commented 6 years ago

I will look into this when I return from vacation next Wednesday.

eddyystop commented 6 years ago

The purpose of the resetShortToken is as you stated: to be a short, human-friendly code which may be sent and typed on mobile devices.

It, like all the tokens, is generated by the service and passed to the notifier function. That func sends it to the user. The service then encrypts the token before storing it in the DB. The unencrypted token returned by the user is compared to the encrypted one in the DB.

At least that's the way the submitted PR was to work. Are you saying the notifier func is not receiving the unencrypted token?

fiddler commented 6 years ago

Thank you for taking the time to reply!

So the notifier is receiving an unencrypted token BUT it is prepended with an ID. The token that comes to the notifier now looks like ID___123456, which is not really usable by the user.

The PR #68 adds that ID to both short and long reset tokens and changes the code for resetting the password in resetPassword.js so that it won't work unless the token contains the ID as well - as it now uses that ID to find the user for resetting the password. Previously it searched for the user by using the passed in user identification, which could be an email for example.

It seems to me that at least in the case of the short token the ID should not be added to the token and the reset should work by using the passed user identification instead of the user ID to find the user for doing the reset in resetPassword.js.

eddyystop commented 6 years ago

Thanks for the explaination. I agree that this is awkward and needs to be looked into, so I've labelled it as "scheduled".

I assume all the persisted tokens are encrypted the same way. So perhaps any received token (or just the short ones) can be encrypted on reseipt and a find done on the DB. That's not as secure as a user_token combo but may be good enough.

Perhaps an option can determine if a user_token or just token input is expected.

I'm tied up on other Feathers stuff for Buzzard. Do you have any interest in looking into it further? I'll get around to it eventually otherwise.

fiddler commented 6 years ago

How it worked before the PR #68 was so that you would have to pass in an identifier for the user (an email for example) and it would find the user based on that - and then compare the token. So it would not have to search the user with the encrypted token.

It feels like the short token should work like described above. And then the long token should work as it's now with the user_token combination.

I can do a PR for this if that sounds like an OK approach.

eddyystop commented 6 years ago

How about the short token routine accepting either the existing user_token string or the old (userIdentifier, token) param? This would not be a breaking change then.

fiddler commented 6 years ago

Hmm, if the purpose of the shortToken is to be used as a token that the user can input easily to do the reset - how it currently works this is not possible. So there should be no real reason to support the short token with the id concatenated as there is no use case for that. Sounds like this would add complexity to the code that is just to support a wrong use case? If you feel strongly that it should work like that I think I can manage it. :)

eddyystop commented 6 years ago

I was trying to avoid a breaking change but go ahead and we can bump the major number in the semserv.

fiddler commented 6 years ago

There's a PR #88 for this waiting for comments. :)

mabounassif commented 6 years ago

Any updates on the PR?

eddyystop commented 6 years ago

Will be published as v2.x.x.