Use cookies only when available - #684
Conversation
Sorry, something went wrong.
|
Hello Maeda-san, thanks for the contribution! I haven't personally used ActionController::API, but this is something I'd like to help with. Just curious, how do you authenticate API requests, with params, aka. "single access token"? Regarding your implementation, it all makes sense at first glance. What suggestions to you have for how we can test these changes? We have Authlogic::TestCase::MockController. Maybe we can add Authlogic::TestCase::MockAPIController? |
Sorry, something went wrong.
|
Hello, Jared. Thank you for your response.
Yes, I plan to get single_access_token from params (or HTTP Authorization header, with monkey patching) for authentication.
I tried to add Authlogic::TestCase::MockAPIController and some test cases for my changes. Please review it. |
Sorry, something went wrong.
There was a problem hiding this comment.
I think this is close! I like how you've written an "adapter test" and a "persistence test".
- I've commented on a few lines that I don't understand. Please explain.
- Please add an entry to CHANGELOG.md under "Unreleased -> Added -> Support for ActionController::API"
- Please add a section 2.b.2. ActionController::API to the readme. Please include a small, complete example of an API controller, and a link to the "single access token" documentation in base.rb.
Sorry, something went wrong.
| # http://guides.rubyonrails.org/action_controller_overview.html#cookies | ||
| def cookies | ||
| controller.send(:cookies) | ||
| controller.respond_to?(:cookies, true) ? controller.send(:cookies) : nil |
There was a problem hiding this comment.
This looks good. I spent some time playing around with ActionController::API and I was able to reproduce the need for this.
Sorry, something went wrong.
|
|
||
| after_save :reset_perishable_token! | ||
| after_save :save_cookie | ||
| after_save :save_cookie, if: :cookie_enabled? |
There was a problem hiding this comment.
This seems like a reasonable change, but without it the tests still pass, so is it really necessary?
Sorry, something went wrong.
|
|
||
| def request | ||
| @request ||= MockRequest.new(controller) | ||
| @request ||= MockRequest.new(self) |
There was a problem hiding this comment.
I don't understand this change. The tests pass without it. Can you please explain?
Sorry, something went wrong.
| end | ||
|
|
||
| def format | ||
| controller.request_content_type if controller.respond_to? :request_content_type |
There was a problem hiding this comment.
The conditional seems unnecessary. I can remove the if controller.respond_to? and the tests still pass. I guess because both of our "test controllers" respond to request_content_type?
Sorry, something went wrong.
|
Merged, thanks!
I've left a documentation place-holder at 2.b.3.. Please expand upon it when you have time. Let me know how it goes with ActionController::API, I am curious. |
Sorry, something went wrong.
|
@jaredbeck @epaew Do you mind to share example for ActionController::API? It's still missing from the documentation. |
Sorry, something went wrong.
Because ActionController::API does not include ActionController::Cookies metal and ActionDispatch::Cookies rack module,
Therefore, our controller can not use the cookies method.
In this patch, Authlogic::Session::Base calls controller#cookies only when available.