Skip to content

Basic Cookie and Set-Cookie headers implementation. (WIP) #71

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

Closed

Conversation

s-panferov
Copy link
Contributor

This piece of code is not finished yet because i have some questions:

  • How i need to format Set-Cookie header? I have some troubles because it's multiline nature: one line per cookie.
  • There are nice library to work with cookies: https://github.com/alexcrichton/cookie-rs. We can use Cookie type from that library to parse Cookie header, but (1) the library has several dependencies and i'm not sure that we want to deal with them and (2) maybe someone wants to use another cookie library. I can leave the Cookie header as-is, use cookie-rs or make some more deep parsing to simple key-value pairs.

@seanmonstar
Copy link
Member

The dependencies of cookie-rs are already dependencies in hyper, so that's okay.

I'd definitely like to have a Cookie type, with decent typed interface to set various properties of a cookie. My only hesitation is the cookie signing that is in jar.rs. Signing is good, but it seems extra for an http library. @alexcrichton what do you think?

@s-panferov
Copy link
Contributor Author

I don't see any places to use CookieJar in hyper for now. I imagine such flow:

  1. Hyper uses cookie::Cookie to parse Cookie header.
  2. Client code loads cookie::Cookie's from Cookie header to own cookie::CookieJar (maybe with signing).
  3. Client code uses the cookie::CookieJar to make some changes.
  4. Client code creates SetCookie header with help of cookie::CookieJar and passes it to hyper.

@s-panferov
Copy link
Contributor Author

For convenience we can create some helpers for the last step: SetCookie::from_jar(&cookie_jar)

@alexcrichton
Copy link
Contributor

I wouldn't be confident in saying that cookie-rs is super standards compliant (it hasn't seen much real-world usage yet, I'm just using it for the cargo registry), but I'd love to help out here!

The major questionable dependency is rust-openssl, which isn't quite what you'd want on windows. I certainly don't mind refactoring out signing into a separate sub-package. Just let me know!

If you guys end up creating your own cookie parser, I also wouldn't mind depending on that for the signing and such!

@s-panferov
Copy link
Contributor Author

I sent two commits that add support of cookie-rs. Now i'm looking for some generic solution to make cookie-rs replaceable.

@s-panferov
Copy link
Contributor Author

I added a default cookie_rs feature to Cargo.toml. One can completely disable cookie-rs and use default pub type Cookie = TypedCookie<String> or make it's own header type with another library.

@s-panferov
Copy link
Contributor Author

The last thing we need here is a Set-Cookie's formatter. I don't know the entire hyper codebase enough, so i need advice. I see #72, it is related?

@s-panferov s-panferov changed the title Basic Cookie and Set-Cookie (only parsing) headers implementation. (WIP) Basic Cookie and Set-Cookie headers implementation. (WIP) Oct 8, 2014
s-panferov added a commit to rustless/rustless that referenced this pull request Oct 8, 2014
@seanmonstar
Copy link
Member

I'm working on adding support for headers that must output on multiple
lines.

@s-panferov s-panferov force-pushed the feature/implement-cookie-headers branch from 6050ae8 to 7b7bce8 Compare October 9, 2014 08:35
@s-panferov
Copy link
Contributor Author

Maybe we can merge this and implement formatter later?

@reem
Copy link
Contributor

reem commented Oct 11, 2014

We're gonna want to ensure that we design the Header trait to make this safely possible, so I think it's important that we don't merge something that can't work in the future.

@seanmonstar
Copy link
Member

@s-panferov I know you've done a lot of work in this PR, and thank you for that! I want to fix up being able to have multiple header lines, otherwise SetCookie is kind of useless on it's own. I've been busy this week, but I've just finally gotten permission to work on this 1 day out of the work week, so I can do more than just nights/weekends.

@seanmonstar
Copy link
Member

@s-panferov just wanted you to know that I haven't forgotten. Adjust the Header trait to support this has been filed in #80. I've been waiting on associated items to be more fully implemented in rustc before tackling.

@s-panferov
Copy link
Contributor Author

Hi all! What do you think about accepting this MR with limitation to set one SetCookie header? I found out that it is quite enough for simple web-servers to store single cookie with _session_token and use it for authentication.

Feel free to say no, I can just use the my own hyper fork to unblock myself while we are waiting.

@seanmonstar
Copy link
Member

@s-panferov I was actually working on this PR today. I'm going to add in a hacky way to produce multiple lines, as so far, this is the only header that needs it, and I want to unblock people.

@seanmonstar
Copy link
Member

closing in favor of #106

@seanmonstar seanmonstar closed this Nov 8, 2014
@s-panferov s-panferov deleted the feature/implement-cookie-headers branch December 3, 2014 07:17
s-panferov added a commit to rustless/rustless that referenced this pull request Dec 6, 2014
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.

4 participants