Cookies #2

[Ryman]

cc @simonpersson

This took a bit more thinking than I'd anticipated, but it required something similar to solving #79 for some configuration without incurring ordering dependencies (which could be potentially insecure for signed cookies) that seems slightly obvious in hindsight (and is very similar to something you suggested there). Still feels a bit iffy though, but I think it's sound enough to be useful.

The last commit does a bit of weirdness with where clauses on Self to collapse the Cookies trait into a single trait, but I think it's a lot nicer for users to only have to worry about importing a single trait.

cc #234 (original cookies implementation)

[SimonTeixidor]

Cool!

I took a quick glance, and the new API looks nice to me. :)

Is this a breaking change?

[Ryman]

Ah, yes forgot about that, I'll annotate the commits when I get a chance later (as well as test a beta build which seems to have broken from inference changes in 1.2?)

[Ryman]

Managed to get a build on 1.1 and beta (1.2)... which ICEs on 1.0. Sweet.

[Ryman]

Alright, things work across rust versions and have an additional compile-fail test to check that the error message is 'reasonable' when using cookies without the appropriate bounds being met.

cc @cburgdorf

[cburgdorf]

Sorry, for jumping on it late. I skimmed through the changes and it's quite a brain twister for me to follow ;)

Looking at the example code I wonder what's the advantage of having the user to define a data structure for the cookie. Iron just defines Cookie for you which seems to make it easier from the perspective of the user: https://github.com/iron/cookie/blob/60832b7dd3020724bf2a0cecb631f59acfa7b0e0/README.md

There may be good reasons to implement it our way but I'd like to understand them better ;)

[SimonTeixidor]

That struct is needed to encrypt the Cookie, which Iron does not appear to be doing. So in Iron, the user would instead have to manually deal with encrypting whatever data they want to send, but as they don't have an example for that, Iron does come across are easier :)

My suggestion is that we provide a struct so that the user doesn't have to create one in the most simple case. Then the example could be:

let mut server = Nickel::with_data(SecretKey::new_with_rand_key());

or something, and the user would not have to create a struct and initialize it. We could also add a case for those who prefer use the same key:

let mut server = Nickel::with_data(SecretKey::new("abcdef123456789"));

What do you think, @Ryman?

[Ryman]

That repo looks like it hasn't been updated in a year, but to be clear, the advantage is that this way will offer a compile-time guarantee, as you can see this error message is given when not handled.

If we emulated the way a usual plugin would do it, then we get the dependency problem where you'll have a runtime crash due to the unwrap (see the first line in echo_cookies with the trailing unwrap) if the Cookies middleware is added after a handler which depends on it.

If we had specialization then I think we could provide a zero'd key for things which don't implement AsRef<cookies::SecretKey> and not allow the user to use encrypted or signed cookies, but I don't think there's a clean way to do that at present.

@simonpersson It does/did handle encryption (see here). But it does implement most of it by itself rather than leveraging cookie-rs (not sure if cookie-rs was available at the time though so it makes sense).

I think Nickel::with_data(cookies::SecretKey([32,4,125,... ])) works, but the problem is more general, like if there's two middleware which require bounds, e.g. cookies + database pool. The example code is perhaps trying to be too general by showing that the user is in control of the data and they're not restricted to nickel types.

I don't think generating a new key on every server restart is a good idea as that invalidates all old cookies, providing the SecretKey::new could be useful though.

Perhaps we need to consider how configuration will look for nickel in general, to place my vote, I'm in favor of sane implicit defaults (even so far as the server could autogenerate a config on first boot if no config file is available). This would put us nowhere near 'express like' though and align more with something like rails.

[SimonTeixidor]

@Ryman: It only signs the cookie, no encryption, no? Or am I missing something?

Perhaps we can break the example out to two examples, one where we show the most simple use of Cookies and one where we demonstrate sharing of data. I suppose the nickel-auth crate will also serve as a good example of this once it is done.

I suppose I prefer that users who know nothing about encryption gets a safe key generated, and having the cookies become invalidated on a server restart, then having the same user provide an insufficiently strong key. Users who know what they're doing can still provide their own key. I very much like the idea of generating a config file though! Then we would get around that problem entirely by storing a generated key for the user.

[Ryman]

@simonpersson Ah yes I'm totally wrong about the encryption :P

In the simple use of cookies, do you mean with no key, or with just passing something like SecretKey::new("Keystring")?

Yeah, I would prefer a system where there are safe defaults rather than defaulting to something with a null key or no encryption. Perhaps we can elaborate on another ticket how we will handle simpler configuration, or do you think it's a blocking issue for cookies for now?

[SimonTeixidor]

@Ryman To be fair though, it is a bit odd to sign but not encrypt, I was also a bit confused at first.

Yes, just passing a SecretKey without creating a new struct, so that no one thinks that it is necessary to create a struct in order to use cookies.

I don't think it is blocking cookies. We can always add that later. Feel free to open another ticket.

[Ryman]

Closing in favor of #272