-
Notifications
You must be signed in to change notification settings - Fork 636
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
Use cookies only when available #684
Use cookies only when available #684
Conversation
Hello Maeda-san, thanks for the contribution! I haven't personally used Just curious, how do you authenticate API requests, with Regarding your implementation, it all makes sense at first glance. What suggestions to you have for how we can test these changes? We have |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 inbase.rb
.
@@ -14,7 +14,7 @@ def authenticate_with_http_basic(&block) | |||
# Returns a `ActionDispatch::Cookies::CookieJar`. See the AC guide | |||
# 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. I spent some time playing around with ActionController::API
and I was able to reproduce the need for this.
@@ -415,10 +415,10 @@ def self.#{method}(*filter_list, &block) | |||
before_save :set_last_request_at | |||
|
|||
after_save :reset_perishable_token! | |||
after_save :save_cookie | |||
after_save :save_cookie, if: :cookie_enabled? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a reasonable change, but without it the tests still pass, so is it really necessary?
@@ -39,7 +39,7 @@ def params | |||
end | |||
|
|||
def request | |||
@request ||= MockRequest.new(controller) | |||
@request ||= MockRequest.new(self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this change. The tests pass without it. Can you please explain?
@@ -9,6 +9,10 @@ def initialize(controller) | |||
self.controller = controller | |||
end | |||
|
|||
def format | |||
controller.request_content_type if controller.respond_to? :request_content_type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
Merged, thanks!
I've left a documentation place-holder at Let me know how it goes with |
@jaredbeck @epaew Do you mind to share example for It's still missing from the documentation. |
Because
ActionController::API
does not includeActionController::Cookies
metal andActionDispatch::Cookies
rack module,Therefore, our controller can not use the
cookies
method.In this patch,
Authlogic::Session::Base
callscontroller#cookies
only when available.