Skip to content
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

Allowing http token auth to set the token_authentication_key if missing from params #2271

Merged
merged 6 commits into from Apr 14, 2013

Conversation

robhurring
Copy link
Contributor

Allows the user to configure devise to allow token auth headers to set the missing "token_authentication_key" param with a new setting in the config "allow_authorization_to_set_auth_token". When set to true it will parse out the token from the request headers and update the param for "token_authentication_key".

…ce of passing it in via params

It will not override existing token_authentication_key params if they are present.
@josevalim
Copy link
Contributor

Thanks @robhurring. However, you explained how the feature is implemented, you haven't explained why you need it thought. Could please tell us why? Why the current token mechanism is not enough?

@robhurring
Copy link
Contributor Author

There is really nothing in particular lacking with the current basic auth flow for authenticating, but for an API we felt using token auth was semantically better. The reasoning behind this was more for allowing the API client to pass in options with the token which the app can use. I didn't include that piece of the code in this PR since I wasn't exactly sure how to make it feel natural in devise.

In our current implementation we're passing in a request signature with the token, the signature (and any other metadata) is then bubbled up to the app in env['devise.token_options'] and handled from there. It can also be used to handle token expiration or other use cases. If this sounds like it could be useful for devise I can commit those change as well.

@@ -82,7 +82,7 @@ def authentication_token
generate_token(:authentication_token)
end

Devise::Models.config(self, :token_authentication_key, :expire_auth_token_on_timeout)
Devise::Models.config(self, :token_authentication_key, :allow_authorization_to_set_auth_token, :expire_auth_token_on_timeout)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we call this configuration option something like allow_token_authenticatable_via_headers or something like it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that much better -- fixed!

@josevalim
Copy link
Contributor

In our current implementation we're passing in a request signature with the token, the signature (and any other metadata) is then bubbled up to the app in env['devise.token_options'] and handled from there.

This approach is also fine. We could include the options in env['devise.token_options'] as well. Could you please update your pull request?

Thanks a lot!

@josevalim
Copy link
Contributor

Thanks a lot @robhurring! Sorry for the delay.

Maybe instead of merging into the params hash, we could add a valid_token_auth? check as well, similar to what we do with http auth:

https://github.com/robhurring/devise/blob/547439d94c32a63a11260f96e3713b88c6b6ac68/lib/devise/strategies/authenticatable.rb#L70

@robhurring
Copy link
Contributor Author

@josevalim no worries, its been a pretty busy month for me as well. I made those changes and mimicked the http auth flow pretty close. Give it a glance when you have a minute and let me know if theres any other changes that need to be made.

josevalim pushed a commit that referenced this pull request Apr 14, 2013
Allowing http token auth to set the token_authentication_key if missing from params
@josevalim josevalim merged commit 1b8fd7c into heartcombo:master Apr 14, 2013
@josevalim
Copy link
Contributor

Perfect pull request! I have merged it, sorry for the delay!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants