Use stateful CookieJar in Redirector #613
Conversation
Kache
force-pushed
the
redirect-jar
branch
2 times, most recently
from
02183ad to
b03076a
Compare
There was a problem hiding this comment.
This solves an SSO authentication redirect problem I have, and I'd prefer not to use curb. (I actually still have to use curb anyway for one endpoint b/c libcurl implements SPNEGO via GSS-API to authenticate /w Kerberos, and I don't yet know how to do the same in http.rb)
I saw #306, and although HTTP::Session is supposed to do something similar, I can't tell what HTTP::Redirector's relationship with HTTP::Session is intended to be.
I'm getting test #<OpenSSL::SSL::SSLError: SSL_read: sslv3 alert unsupported certificate> errors when running locally and in TravisCI, even on master and v4.4.1. Is there someone that can help with that? I still need to add new tests.
| def initialize(opts = {}) # rubocop:disable Style/OptionHash | ||
| @strict = opts.fetch(:strict, true) | ||
| @max_hops = opts.fetch(:max_hops, 5).to_i | ||
| @jar = opts.fetch(:jar, CookieJar.new) |
There was a problem hiding this comment.
To use an existing cookiejar from disk:
persisted_jar = HTTP::CookieJar.new.load(from_disk_path, format: :cookiestxt)
response = HTTP.follow(jar: persisted_jar).get(endpoint)
persisted_jar.save(to_disk_path, format: :cookiestxt) # if you'd likeI suppose putting the persistent cookie jar option in HTTP.follow might not be considered intuitive. Also, Redirector is somewhat similar to Session, as they are both external to Client and hold longer-lived state.
There was a problem hiding this comment.
Yeah, in fact I was thinking that we should completely refactor redirector. And provide some sort of Session object that will "mimic" the browser behaviour. So I think it's OK-ish for now.
| @@ -97,9 +97,10 @@ def initialize(opts) | |||
| end | |||
|
|
|||
| # Returns new Request with updated uri | |||
| def redirect(uri, verb = @verb) | |||
| def redirect(uri, verb = @verb, cookies_raw: nil) | |||
There was a problem hiding this comment.
I would've liked to inline redirect -- I think it's more fitting to have the higher level controller, Redirector, do it rather than asking Request to redirect itself.
It'd also avoid having to cookies_raw: nil just to protect against api change.
thoughts?
Kache
force-pushed
the
redirect-jar
branch
2 times, most recently
from
525aad7 to
c894167
Compare
As [mentioned](..httprb/issues/264#issuecomment-157070867) in httprb#264, CookieJar implements cookie domain scoping rules, which is useful when issuing redirects across domains. Resolves: httprb#264
|
Hey guys, what's up with this PR? |
|
I'd agree with @ixti it looks OK-ish for now. I think it'd really be better to extract an |
|
Sorry it took me some time to get back in line but I will try to work on PRs this weekend. |
Client shoud not know about CookieJar, it's just a client not a browser or session. Also, I've decided to keep `Request#redirect` as it was before #613 and deprecate it instead (will be removed in 5.0.0). Redirected request builing moved into redirector itself.
|
I've merged this PR with some changes:
Notice that most likely branch will fail CI. We need to upgrade test tooling. And some tests. |
As mentioned in #264, CookieJar
implements cookie domain scoping rules, which is useful when issuing
redirects across domains.
Resolves: #264