Cookie backed sessions

[joseferben]

This is just a draft of cookie backed sessions based on how the Play framework does it. (https://www.playframework.com/documentation/2.8.x/JavaSessionFlash).

It needs tests and some more thought before it can be merged.

[tmattio]

Awesome work @Jerben! 🎉

A few questions:

• Since Session.find takes a Request.t and Session.set takes a Request.t, what's the advantage of having a middleware that stores the sessions in the context?

• Sessions should probably come with a modular backend. I initially wanted to provide the cookie backend and the in-memory backend with Opium, and offer an API to allow users to implement their own backend. Does it make sense to do this as part of this PR?

• The next step, once we have the sessions, will be to add a flash middleware. I think you've done this in Sihl already. Is the implementation in this PR taking this use case into account?

[joseferben]

Thank for your quick feedback @tmattio

Since Session.find takes a Request.t and Session.set takes a Request.t, what's the advantage of having a middleware that stores the sessions in memory?

Session.set takes a Response.t, so this is the session API:

val find : string -> Request.t -> string option
val set : string * string option -> Response.t -> Response.t

Usage could look like this in an Opium handler:

...
let user_id = Opium.Session.find "user_id" req in
(* doing something with the user, then logging out *)
Opium.Response.of_plain_text "" |> Opium.Session.set ("user_id", None) |> Lwt.return

Sessions should probably come with a modular backend. I initially wanted to provide the cookie backend and the in-memory backend with Opium, and offer an API to allow users to implement their own backend. Does it make sense to do this as part of this PR?

Good point. For Sihl I think we will have the cookie based session management enabled by defaults since it's stateless. So far we have been using MariaDB and Postgres backends as default. Play suggests to take care of session data that is big (> 4 KB) or sensitive yourself.

With this PR we could easily add other backends as separate middlewares like Middleware_memory_session and the session API Opium.Session stays the same. This would be a non-breaking change.

The next step, once we have the sessions, will be to add a flash middleware. I think you've done this in Sihl already. Is the implementation in this PR taking this use case into account?

Yes that is planned :) We already have a session based flash middleware in Sihl, but my plan was to submit a cookie based flash message implementation for Opium and switch to that. The flash message cookie doesn't have to be signed.
This means an app with both the cookie based session middleware and cookie based flash message middleware enabled uses two different cookies and there is no dependencies between the middlewares.

We could still implement a session based flash message middleware next to each other if the payload size limit or performance impact is an issue, but use this as default because it is stateless and has no dependencies. (https://www.playframework.com/documentation/2.8.x/JavaSessionFlash#Flash-scope)

We want to improve the out-of-box experience with Sihl and having those two middlewares with cookie based implementation is a good first step. The API considers other implementations that can be used for instance for performance reasons.

Also we would like to contribute the CSRF and body parsing middlewares.

[tmattio]

Session.set takes a Response.t, so this is the session API:

That's what I meant, sorry. That's why I'm not sure to understand the need for the middleware - couldn't Session.set and Session.get read/write the cookie from the request/response directly?

With this PR we could easily add other backends as separate middlewares like Middleware_memory_session and the session API Opium.Session stays the same

In the PR, Session.{get,set} is hardcoded to Middleware_cookie_session.{get,set}, so I think if we want to make it modular, the API would have to break somehow. Not a big deal, but could be worth thinking of it while we're at it. Wouldn't want to block the merge for this though, so if you prefer us to do it as a second step, that's fine by me.

my plan was to submit a cookie based flash message implementation for Opium and switch to that

Sounds great!

[tmattio]

read/write the cookie from the request/response directly

You'd have to pass the Cookie.Signer.t when calling {get,set}, which isn't great, so maybe you'd want some kind of functor, which would fit with a modular backend design too.

[joseferben]

That's what I meant, sorry. That's why I'm not sure to understand the need for the middleware - couldn't Session.set and Session.get read/write the cookie from the request/response directly?

My plan was to move Session.t, Session.{get, set}, and Env.key to Opium.Session eventually and have a dependency on it from the middleware implementations. The API is in Opium.Session while the session persistence happens in the middleware.

[tmattio]

The API is in Opium.Session while the session persistence happens in the middleware.

I'm starting to think there's something obvious I'm not seeing, but: why couldn't the persistence happen in Opium.Session?

[joseferben]

I'm starting to think there's something obvious I'm not seeing, but: why couldn't the persistence happen in Opium.Session?

It could and maybe it should :) I am just using middlewares to swap implementations.. If you give me some more pointers I can try to come up with something else.

[tmattio]

It could and maybe it should :) I am just using middlewares to swap implementations.. If you give me some more pointers I can try to come up with something else.

I'm not against middlewares, but it does come with some drawbacks:

• The users have to remember to add the session middlewares in their routers, otherwise, the sessions can't be used.
• It's harder to think about the code: when you call Session.set, what happens? e.g. If you overwrite the cookie with the same key, will it really be overwritten or will the session be stored anyways?

So I tend to prefer keeping things outside of middlewares when possible.

In the case of sessions, I liked the design of https://github.com/inhabitedtype/ocaml-session (although admittedly I found the names of the functions confusing). We can't implement cookie backends with it, but we could take some inspiration from it.

If we want to hide the functors from users, we could imagine an API like this:

let (module User_session) = Opium.Session.signed_cookie ~signer:Config.user_session_signer

(* In the handler *)

User_session.get "_session" request

Or, if we accept to require users to have to set their app for sessions to work:

Opium.Session.set_default_signer signer

(* In the handler *)

Opium.Session.Cookie.get "_session" request

[tmattio]

That being said, most frameworks I have used require a middleware for sessions to work, I'm failing to understand the reasoning behind it.

[joseferben]

I updated the PR and contributed some tests that shows the usage.

I agree with the limitations of middlewares. This is a simple way to associate 4KB of data to a user session. It is completely up to the user to store a reference such as user_id and to use a key-value store to persist actual session data.

All you need to understand to use Opium are Request.t -> Response.t Lwt.t and Rock.Handler.t -> Rock.Handler.t, which are simple yet powerful abstractions. Shouldn't Opium focus on these abstractions and leave things with lifecycles and (mutable) state to users or higher-level frameworks?

[joseferben]

We could also read from Opium.Request.t and write to Opium.Response.tdirectly without using a middleware, then passing configuration is a bit awkward as there is no central place where you can do that.

[rawleyfowler]

Is this still being worked on?

[joseferben]

We implemented this downstream, it should be straightforward to use that. I am a bit out of the loop, especially regarding the switch back to cohttp. @rgrinberg How would that impact the request/response API?

If there is interest I could update this PR to use Sihl's implementation.

[rgrinberg]

If there is interest I could update this PR to use Sihl's implementation.

That would be useful for sure.