mcordell / grape_token_auth

Token auth for grape apps
MIT License
52 stars 19 forks source link

Mailer doesnt provide token expiration for :reset_password #16

Closed frobichaud closed 8 years ago

frobichaud commented 8 years ago

The PasswordApi should add the expiry params in the query string when performing a send_reset_password_instructions.

The send_password_instructions should use resource.build_auth_url Then, the query string would be properly built.

frobichaud commented 8 years ago

@mcordell thanks for the quick follow-up. I'm currently analyzing how the reset_password feature is managed by the ng-token-auth lib. I dislike the password/edit route which returns a 302 if it succeeds or a json if it fails.

It looks like ng-token-auth supports using a reset password token when we call validateUser (see https://github.com/lynndylanhurley/ng-token-auth/blob/master/src/ng-token-auth.coffee#L492).

This seems to require the following params: expiry, reset_password. This means grape_token_auth should use "find_with_reset_token" in the TokenValidationAPI route.

There's then a "auth:password-reset-confirm-success" or "-error" event that's broadcasted. It makes sense for an app to listen for these events and responds accordingly (e.g. show a token error dialog, or forward the user to the change password state)

mcordell commented 8 years ago

I'm not sure that is the intent, especially since I don't know that devise/devise_token_auth necessairly has expiries on reset password tokens.

I believe what this client side token validation is talking about is after the user has clicked the link in the email, hit the endpoint and received a positive response. At that point the normal auth headers are set (which is where the expiry comes from).

This is the param that the line you linked to is keying on: https://github.com/mcordell/grape_token_auth/blob/master/lib/grape_token_auth/apis/password_api.rb#L86

frobichaud commented 8 years ago

@mcordell what I dislike from the library is the fact that on success you get redirected (302) while you get a 404 when the token is no longer valid. It causes an inconsistent flow for the user. devise_token_auth has the same weird behavior, which causes the user to see a json "success: false" in the rendered page instead of being properly redirected to a route with a param that an app can parse and show an appropriate error message. See https://github.com/frobichaud/grape_token_auth/blob/master/lib/grape_token_auth/apis/password_api.rb#L92

This might just be a preference. And what I was actually looking for initially was to use ng-token-auth to call the password/edit route and generate a token if the reset_password_token is valid. It might be an incomplete feature in ng-token-auth, but it looks like there was an intent to support password/edit through an ajax call instead of rendering a 404 or a 302.

mcordell commented 8 years ago

So I haven't completely explored the ng-token-auth code, but my understanding of the method you first linked is that it is a multi-purpose method for dealing with setting/validating the auth headers. The "expiry" in question refers to the expiry of the usual tokens that GTA/DTA set. So, if the user clicks through a reset emailed link, the auth headers will be set as normal and this 'reset_param' is set telling the front end to go through the password resetting routine. So all that to say is that I don't think an expiry param should be set in the email link.

I'm going to close this for now because I don't want to deviate from what DTA is doing because it may break compatibility with ng-token-auth. I'd be happy to change the flow if you want to open an issue with ng-token-auth to change it.