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

Bearer Token Authorization Header To Connection #299

Merged
merged 4 commits into from Oct 31, 2018

Conversation

lucasklaassen
Copy link
Contributor

Since subclasses of the base class ActiveResource::Base copy-on-read http request headers, there is no easy way to define and override Authorization headers across all subclasses between multiple requests. Basic and Digest Authorization headers have already been built into the ActiveResource::Connection class, this PR adds a third approach: Bearer Token. JWT has become a lot more popular over the past few years and I believe that others will benefit from this change.

Issue: #277 (comment)
Thank you to @jeremy for coming up with this solution.

@rails-bot
Copy link

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @rafaelfranca (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@lucasklaassen
Copy link
Contributor Author

@rafaelfranca Since this change is pretty forward facing, do we foresee any issues with a merge? Let me know if the tests and added documentation need any tweaks, here to help!

@lucasklaassen
Copy link
Contributor Author

Hey @rafaelfranca I'm new to open source, does review on this just take time or is there anything else I need to do to get this PR considered?

Copy link
Member

@jeremy jeremy left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks @lucasklaassen!

@jeremy jeremy merged commit b713f67 into rails:master Oct 31, 2018
@lucasklaassen lucasklaassen deleted the bearer_token_authorization branch October 31, 2018 23:45
@rafaelfranca
Copy link
Member

Thank you! Sorry for the delay, I was a little bit disconnected from open source those last days.

@lucasklaassen
Copy link
Contributor Author

Thanks guys! What's the process for the releases today? Since this is a feature addition would it just be included in the next minor release?

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

Successfully merging this pull request may close these issues.

None yet

4 participants