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

Cookies are only preserved for the single next request #363

Closed
henrik opened this Issue Jan 12, 2013 · 27 comments

Comments

Projects
None yet
4 participants
@henrik

henrik commented Jan 12, 2013

https://github.com/savonrb/savon/blob/master/lib/savon/request.rb#L62

Requests use the cookie that was set in the last request. So this works:

  • Auth request, cookie is set.
  • Authenticated request, uses that cookie.

But if you add a third step, it doesn't get the cookie, since its "last request" (step 2) didn't set any cookie in its request.

  • Authenticated request, doesn't work because there is no cookie.

Not sure what would be the best solution. Maybe store away the response.http.cookies in @global only if they are present (a non-empty array), and pass that into httpi?

@henrik

This comment has been minimized.

Show comment
Hide comment
@henrik

henrik Jan 12, 2013

Working around it like this for now:

response = client.call(:connect, message: credentials)
client.globals[:headers] = { "Cookie" => response.http.headers["Set-Cookie"] }

henrik commented Jan 12, 2013

Working around it like this for now:

response = client.call(:connect, message: credentials)
client.globals[:headers] = { "Cookie" => response.http.headers["Set-Cookie"] }
@sbu

This comment has been minimized.

Show comment
Hide comment
@rubiii

This comment has been minimized.

Show comment
Hide comment
@rubiii

rubiii Jan 15, 2013

Contributor

i don't know too much about cookies. is it "valid" to simple aggregate all cookies from all responses?
maybe @rogerleite knows more about this?

Contributor

rubiii commented Jan 15, 2013

i don't know too much about cookies. is it "valid" to simple aggregate all cookies from all responses?
maybe @rogerleite knows more about this?

@henrik

This comment has been minimized.

Show comment
Hide comment
@henrik

henrik Jan 15, 2013

I think aggregating all cookies from all responses would be close to the correct solution. Definitely a big improvement.

There is probably an edge case where two responses both pass "Set-Cookie" with the same cookie name but different values. In that case only the latest one should be kept.

E.g. response 1:

Set-Cookie: foo=bar; Expires=whatever

Response 2:

Set-Cookie: foo=baz; Expires=whatever

In that case, we want only "foo=baz".

henrik commented Jan 15, 2013

I think aggregating all cookies from all responses would be close to the correct solution. Definitely a big improvement.

There is probably an edge case where two responses both pass "Set-Cookie" with the same cookie name but different values. In that case only the latest one should be kept.

E.g. response 1:

Set-Cookie: foo=bar; Expires=whatever

Response 2:

Set-Cookie: foo=baz; Expires=whatever

In that case, we want only "foo=baz".

@rogerleite

This comment has been minimized.

Show comment
Hide comment
@rogerleite

rogerleite Jan 15, 2013

Member

@rubiii This issue is better classified as feature request (in my opinion). @henrik wants savon to persist cookies between requests, you can aggregate all cookies, but when you request something, you have to add cookies only from request domain.

You can see this http_monkey's middleware https://github.com/rogerleite/http_monkey-cookie, on notes section from Readme have great links about cookies.
It's important to have in mind my comment #257 (comment)

Member

rogerleite commented Jan 15, 2013

@rubiii This issue is better classified as feature request (in my opinion). @henrik wants savon to persist cookies between requests, you can aggregate all cookies, but when you request something, you have to add cookies only from request domain.

You can see this http_monkey's middleware https://github.com/rogerleite/http_monkey-cookie, on notes section from Readme have great links about cookies.
It's important to have in mind my comment #257 (comment)

@henrik

This comment has been minimized.

Show comment
Hide comment
@henrik

henrik Jan 15, 2013

Well, it's somewhere inbetween a feature request and a bug. Savon 1 didn't attempt to remember cookies for you, but documented a solution you could implement yourself. Savon 2 attempts to remember it for you but does so in a way that is limited or buggy depending on your perspective :)

henrik commented Jan 15, 2013

Well, it's somewhere inbetween a feature request and a bug. Savon 1 didn't attempt to remember cookies for you, but documented a solution you could implement yourself. Savon 2 attempts to remember it for you but does so in a way that is limited or buggy depending on your perspective :)

@rogerleite

This comment has been minimized.

Show comment
Hide comment
@rogerleite

rogerleite Jan 15, 2013

Member

@henrik you're right. Savon implementation on remembering cookies is wrong.

Member

rogerleite commented Jan 15, 2013

@henrik you're right. Savon implementation on remembering cookies is wrong.

@rubiii

This comment has been minimized.

Show comment
Hide comment
@rubiii

rubiii Jan 15, 2013

Contributor

shouldn't cookies be repeated for every request? i'm probably wrong, but isn't that what browsers do?
sooo ... not really sure how to solve this right now. but #257 doesn't really help with this, right?

Contributor

rubiii commented Jan 15, 2013

shouldn't cookies be repeated for every request? i'm probably wrong, but isn't that what browsers do?
sooo ... not really sure how to solve this right now. but #257 doesn't really help with this, right?

@rubiii

This comment has been minimized.

Show comment
Hide comment
@rubiii

rubiii Jan 15, 2013

Contributor

@rogerleite ok, as far as i understand, this could be a completely separate option that would not change savon's
default cookie handling (passing cookies from the last response to the next request) after #257 is fixed, right?

Contributor

rubiii commented Jan 15, 2013

@rogerleite ok, as far as i understand, this could be a completely separate option that would not change savon's
default cookie handling (passing cookies from the last response to the next request) after #257 is fixed, right?

@rubiii

This comment has been minimized.

Show comment
Hide comment
@rubiii

rubiii Jan 15, 2013

Contributor

@rogerleite regarding your comment "Savon implementation on remembering cookies is wrong."
you mean it's currently broken, but 60a67a would fix this problem, right?
or are there any other problems with the way this is supposed to work?

Contributor

rubiii commented Jan 15, 2013

@rogerleite regarding your comment "Savon implementation on remembering cookies is wrong."
you mean it's currently broken, but 60a67a would fix this problem, right?
or are there any other problems with the way this is supposed to work?

@rogerleite

This comment has been minimized.

Show comment
Hide comment
@rogerleite

rogerleite Jan 15, 2013

Member

@rubiii When i said "Savon implementation on remembering cookies is wrong.", i mean that Savon send cookies from only last response is wrong (unless you document this and warns users like @henrik said).

If you want to correctly implement cookies, you should persist all response cookies (in memory or somewhere else) and when you do a request, you have to look for cookies from request's domain and add on Cookie request header. It's very boring to implement. If you always send all cookies, you open apps to Cookie Spoofing.

Member

rogerleite commented Jan 15, 2013

@rubiii When i said "Savon implementation on remembering cookies is wrong.", i mean that Savon send cookies from only last response is wrong (unless you document this and warns users like @henrik said).

If you want to correctly implement cookies, you should persist all response cookies (in memory or somewhere else) and when you do a request, you have to look for cookies from request's domain and add on Cookie request header. It's very boring to implement. If you always send all cookies, you open apps to Cookie Spoofing.

@rubiii

This comment has been minimized.

Show comment
Hide comment
@rubiii

rubiii Jan 15, 2013

Contributor

that's what i understood. thanks for explaining. still not sure what to do :(
i looked at your middleware and it looks quite hard to get this right.

Contributor

rubiii commented Jan 15, 2013

that's what i understood. thanks for explaining. still not sure what to do :(
i looked at your middleware and it looks quite hard to get this right.

@rogerleite

This comment has been minimized.

Show comment
Hide comment
@rogerleite

rogerleite Jan 16, 2013

Member

The middleware's code is really ugly, i did in hurry, just to validate the idea.
You can follow @henrik advice:
"Savon 1 didn't attempt to remember cookies for you, but documented a solution you could implement yourself."

You could try a "snippet" solution, using https://github.com/dwaite/cookiejar to save and retrieve cookies between requests.

Member

rogerleite commented Jan 16, 2013

The middleware's code is really ugly, i did in hurry, just to validate the idea.
You can follow @henrik advice:
"Savon 1 didn't attempt to remember cookies for you, but documented a solution you could implement yourself."

You could try a "snippet" solution, using https://github.com/dwaite/cookiejar to save and retrieve cookies between requests.

@henrik

This comment has been minimized.

Show comment
Hide comment
@henrik

henrik Jan 16, 2013

I didn't intend that as advice, but you're right, that makes for a simple
solution.

I'm not sure what other cookie scenarios are possible elsewhere, but with
the service we're consuming, we only need to get the cookie right after the
auth request, and use for all future requests. That's what my workaround
above does.

Seems like a fun thing to try to fix properly, though, if I ever find the
time.

On Wed, Jan 16, 2013 at 1:39 PM, Roger Leite notifications@github.comwrote:

The middleware's code is really ugly, i did in hurry, just to validate the
idea.
You can follow @henrik https://github.com/henrik advice:
"Savon 1 didn't attempt to remember cookies for you, but documented a
solution you could implement yourself."

You could try a "snippet" solution, using
https://github.com/dwaite/cookiejar to save and retrieve cookies between
requests.


Reply to this email directly or view it on GitHubhttps://github.com/savonrb/savon/issues/363#issuecomment-12316713.

henrik commented Jan 16, 2013

I didn't intend that as advice, but you're right, that makes for a simple
solution.

I'm not sure what other cookie scenarios are possible elsewhere, but with
the service we're consuming, we only need to get the cookie right after the
auth request, and use for all future requests. That's what my workaround
above does.

Seems like a fun thing to try to fix properly, though, if I ever find the
time.

On Wed, Jan 16, 2013 at 1:39 PM, Roger Leite notifications@github.comwrote:

The middleware's code is really ugly, i did in hurry, just to validate the
idea.
You can follow @henrik https://github.com/henrik advice:
"Savon 1 didn't attempt to remember cookies for you, but documented a
solution you could implement yourself."

You could try a "snippet" solution, using
https://github.com/dwaite/cookiejar to save and retrieve cookies between
requests.


Reply to this email directly or view it on GitHubhttps://github.com/savonrb/savon/issues/363#issuecomment-12316713.

@sbu

This comment has been minimized.

Show comment
Hide comment
@sbu

sbu Jan 16, 2013

I think that "multiple cookies bug" is more urgent than the one related to preserving cookies along multiple requests, because there isn't any workaround for the first.

sbu commented Jan 16, 2013

I think that "multiple cookies bug" is more urgent than the one related to preserving cookies along multiple requests, because there isn't any workaround for the first.

@rubiii

This comment has been minimized.

Show comment
Hide comment
@rubiii

rubiii Jan 25, 2013

Contributor

released httpi v2.0.1 to solve #257.

Contributor

rubiii commented Jan 25, 2013

released httpi v2.0.1 to solve #257.

@rubiii

This comment has been minimized.

Show comment
Hide comment
@rubiii

rubiii Jan 26, 2013

Contributor

i'm removing the current implementation of remembering cookies from 2.1.0.
@henrik would the following piece of code correctly describe your use case?

# first request
response     = client.call(:connect, message: credentials)
auth_cookies = response.http.cookies

# subsequent requests
client.call(:whatever, message: whatever, cookies: auth_cookies)

httpi can return an array of cookie objects and i'm just not sure if those work for you or if you would
prefer to manually set the http header. this would require a new local cookies option though.

let me know what you think.

Contributor

rubiii commented Jan 26, 2013

i'm removing the current implementation of remembering cookies from 2.1.0.
@henrik would the following piece of code correctly describe your use case?

# first request
response     = client.call(:connect, message: credentials)
auth_cookies = response.http.cookies

# subsequent requests
client.call(:whatever, message: whatever, cookies: auth_cookies)

httpi can return an array of cookie objects and i'm just not sure if those work for you or if you would
prefer to manually set the http header. this would require a new local cookies option though.

let me know what you think.

@henrik

This comment has been minimized.

Show comment
Hide comment
@henrik

henrik Jan 26, 2013

@rubiii Yeah, I think that would work fine for my use case. I kind of like how explicit it would be. Especially if the current alternative is implicit but with edge cases.

henrik commented Jan 26, 2013

@rubiii Yeah, I think that would work fine for my use case. I kind of like how explicit it would be. Especially if the current alternative is implicit but with edge cases.

@rubiii

This comment has been minimized.

Show comment
Hide comment
@rubiii

rubiii Jan 26, 2013

Contributor

@henrik the example above should now work on master. please give it a try.

Contributor

rubiii commented Jan 26, 2013

@henrik the example above should now work on master. please give it a try.

@henrik

This comment has been minimized.

Show comment
Hide comment
@henrik

henrik Jan 26, 2013

@rubiii Just tried it and it works fine. Liked the API, too. Thanks!

It took me a while to figure out cookie handling in Savon 2 since I don't believe it was documented, at least not on savonrb.com, so maybe a paragraph about cookies could be added to http://savonrb.com/version2.html?

henrik commented Jan 26, 2013

@rubiii Just tried it and it works fine. Liked the API, too. Thanks!

It took me a while to figure out cookie handling in Savon 2 since I don't believe it was documented, at least not on savonrb.com, so maybe a paragraph about cookies could be added to http://savonrb.com/version2.html?

@rubiii

This comment has been minimized.

Show comment
Hide comment
@rubiii

rubiii Jan 26, 2013

Contributor

nice! i documented the changes a few hours ago.

Contributor

rubiii commented Jan 26, 2013

nice! i documented the changes a few hours ago.

@henrik

This comment has been minimized.

Show comment
Hide comment
@henrik

henrik Jan 26, 2013

@rubiii You've thought of everything :) Appreciate it!

henrik commented Jan 26, 2013

@rubiii You've thought of everything :) Appreciate it!

@rogerleite

This comment has been minimized.

Show comment
Hide comment
@rogerleite

rogerleite Jan 27, 2013

Member

@rubiii great job ㊗️

Member

rogerleite commented Jan 27, 2013

@rubiii great job ㊗️

@rubiii

This comment has been minimized.

Show comment
Hide comment
@rubiii

rubiii Jan 27, 2013

Contributor

thanks 😊

Contributor

rubiii commented Jan 27, 2013

thanks 😊

@sbu

This comment has been minimized.

Show comment
Hide comment
@sbu

sbu Jan 28, 2013

@rubiii Many thanks! Is it possible to release v. 2.0.4 with this fix, in order to prevent any kind of problems when deploying in production server?

sbu commented Jan 28, 2013

@rubiii Many thanks! Is it possible to release v. 2.0.4 with this fix, in order to prevent any kind of problems when deploying in production server?

@rubiii

This comment has been minimized.

Show comment
Hide comment
@rubiii

rubiii Jan 28, 2013

Contributor

this requires at least a minor version increase. following semver, this could not be changed at all before 3.0,
but i think it's ok to release this with 2.1, because it's somewhat of a regression.

Contributor

rubiii commented Jan 28, 2013

this requires at least a minor version increase. following semver, this could not be changed at all before 3.0,
but i think it's ok to release this with 2.1, because it's somewhat of a regression.

@rubiii

This comment has been minimized.

Show comment
Hide comment
@rubiii

rubiii Feb 3, 2013

Contributor

released v2.1.0. please refer to the updated changelog and documentation for details. let me know how it works!

Contributor

rubiii commented Feb 3, 2013

released v2.1.0. please refer to the updated changelog and documentation for details. let me know how it works!

@rubiii rubiii closed this Feb 3, 2013

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