Session management

[aturon]

Work out a good story for session management in Tide. No need to reinvent the wheel here: this can probably follow an approach taken by an existing framework, like actix-web.

[skade]

Ideally, I'd prefer all the algorithmic parts of session handling (and strategies) implemented as libraries somewhere.

[aturon]

Definitely! The whole idea with Tide is to keep as much in the library ecosystem as possible. In some cases we're starting with code here for convenience and then moving it out.

I'm not sure what's currently available in the ecosystem here that's not tied to an existing framework. If we need new general purpose crates, can add to the rust-net-web org.

[bIgBV]

I wen through the following frameworks when understanding how they track state for a request during its lifecycle.

• Rocket
• Actix-web
• Tower-web

I'm typing out what I learned about the different implementations with a recommendation of how to proceed with the same in Tide at the end.

Rocket

Rocket only seems to support storing state in a request through the Cookie type. I think it is because they use a custom Request type. The cookies themselves are per request and are stored in a RefCell (though I'm not entirely sure why).

This approach seems a little inflexible as there is no state store across requests on the server side of things. You have to store information as a private cookie and retrieve it every time in order to persist state across requests.

Tower-web

Tower web doesn't seem to have a user facing API for storing cookies or any kind of session for a given request. It does have a AnyMap for storing app wide config.

I think since tower-web is still under development, the API ins't decided yet.

Actix-web

Out of the three, actix-web seems to have the most complete session storage implementation. It has a generic Session which provides the interface the session storage. The backend for this storage could be anything that implements the SessionBackend trait. The interface for accessing data from the backend is through the SessionImpl trait. By default, the SessionBackend trait this is implemented for the CookieSessionBackend, which wraps over the CookieSession type which implements the SessionImpl trait. This is built on top of the cookie crate.

This is definitely the most flexible and complete solution out of the three frameworks I've looked into.

Conclusion

While Rocket provides a straightforward way to get started with store state for a request, the implementation provided by actix-web is definitely more feature complete. I think we should definitely take inspiration from this when we consider session storage in tide. Since there can be different session backends, this could provide a great way to integrate into the authentication and authorization API by interfacing with the generic SessionBackend type.

I did not do an extremely deep dive into these frameworks, so if I missed something, please feel free to let me know 😄

[tomhoule]

I just published a rough prototype of what it could look like: https://github.com/tomhoule/tide-cookie-session - it's closest to the actix_web design, with pluggable session storage. I would love if some of you had feedback on it :)

[bIgBV]

@tomhoule that is a really cool implementation. I was planning on writing something very similar to what you've written. So a couple of things come to mind.

Right now the API around extractors might be changing, so that's something you might want to keep an eye out for. I think it's best to think about including this into tide itself once that's in place. That way, we could also use the app wide config to store the in memory session store instead of extracting it from the request object. I can see both pros and cons for this approach.

I think it'd be better to separate the storage, session and middleware types form their implementations. Just a thought, since it took me a little while to understand what's going on (though you could attribute that to my inexperience in Rust 😅)

I really like the design and feel it's really flexible and I'd love to try and write a simple redis storage, just to get a hang of how it works. Is it okay if I fork your repo and send a PR across once I have something running?

[tomhoule]

Thanks for the feedback! I put the session storage in the middleware because it seemed the easiest to implement, but you are right that we could put it in application state and it might be better, I'll try to see how that would be done.

And yes it's probably hard to read, I'll try to put some order and write minimal documentation today to make it easier for others to evaluate it.

[bIgBV]

@tomhoule just saw your changes, these are awesome. I think we can move this into tide, and start working on integrating Storage and Session types into tide itself, and then have the CookieSessionMiddleware middleware as a part of the middleware module.

That's my idea at least and I might be missing something. @aturon @yoshuawuyts , do you guys want to pitch in?

[yoshuawuyts]

@tomhoule thanks for making this! The API looks like a cool basis to build on further!

I think we can move this into tide, and start working on integrating Storage and Session types into tide itself, and then have the CookieSessionMiddleware middleware as a part of the middleware module.

@bIgBV I think that would be a great next step!

[bIgBV]

@yoshuawuyts aren't the changes from #126 going to affect the changes made by @tomhoule ?

[yoshuawuyts]

@bIgBV ah yes, that's fair. We should make a decision on how to proceed.

[bIgBV]

@yoshuawuyts after going through today's meeting notes, I think its better to wait for aturon's proposal and the changes around that before going ahead with this.

[tomhoule]

I just read yesterday's meeting on Discord, and the proposed new design looks great. I'll update the crate to fit it when it lands.

[tomhoule]

I saw @aturon open #156 earlier today and couldn't help but try to adapt my session middleware to see how it would fit with the new design, so here it is. My impression is that the new traits made the implementation cleaner, and the final API is nicer. In particular I couldn't find a way to use extractors from inside a middleware before these changes, but with the new design it's trivially accomplished.

The crate in its current state does not compile with the WIP PR because it lacks a public interface to add middleware (I hacked that together locally as a temporary workaround). Working with that code again made me think about error handling in middlewares. It's not obvious to me what the idiomatic approach would be.

[tomhoule]

I am wondering, with the new design, whether it makes sense for the cookie session middleware to fetch the session data itself. It could be more minimal and provide only session IDs - users can then choose how to fetch session data by implementing an extension trait on their Context<AppData>. The middleware would do very little and fit in very few lines of code, so I am not sure it would really provide a lot of value.

One big advantage compared to the implementation I linked in the previous comment would be that session data can be fetched lazily, and potentially concurrently with other things.

There wouldn't be a trait for storage backends anymore, which can be a good thing (implementations can be more specialized) or a bad thing (it's harder to swap session storage implementations), but in my opinion more a good thing.

[bIgBV]

I think providing an extension trait with a default implementation using an in memory session backend would be useful. If we want to be able to get people up and running quickly, implementing a session backend is additional overhead for any sort of simple experiment.

Then, we could provide more involved implementations using an actual session store as an external crate or maybe as a part of a contrib package.

[tomhoule]

That seems good. The in-memory session storage would probably have to be owned by application data (implementing a trait) or a middleware.

[bIgBV]

A middleware would be the best approach. Though, I'm not entirely sure if it should be a default middleware. This takes the conversation towards explicit vs implicit-ness of the framework.

Do we want users to be explicit about what they are using, or do we provide things implicitly. Because the in-memory session management middleware will come with an associated cost, I think it should be explicit.

Meaning I would prefer it that we provide the middleware as a part of the framework, but people would have to include it by calling app.middleware(SessionMiddleware::new()) (or something along those lines).

[jonathanKingston]

I fixed up the session handler written by @tomhoule so that it works with the latest master: https://github.com/tomhoule/tide-cookie-session/pull/2/files

Setting up the state middleware:

#[derive(Default, Debug, Clone)]
struct AppSession {
  logged_in: bool
}
let mut state: InMemorySession<AppSession> = InMemorySession::new();
app.middleware(CookieSessionMiddleware::new("MySession".to_string(), state));

Write and reading the session:

cx.set_session(AppSession { logged_in: false });
let session: Result<AppSession, _> = cx.take_session();

I moved the code to reliance on the cookies middleware as I think it reduces it's complexity.

Also I would like to suggest that the session management take the strictest setup as a default and perhaps later provide ways to relax that.

Suggestions:

• Cookie suggestions
  • Require __Host- cookie prefix (provide an opt out for localhost debug)
  • Require http only to prevent JS getting access to the cookie to prevent XSS attacks
  • Require SameSite:Strict to prevent XSRF attacks
  • Require Secure (provide an opt out for localhost debug)
  • Set Path / as required by __Host
  • Set a shortish expiry (perhaps 1 week?)
• Require regeneration of the uuid on a regular basis to prevent session fixation

[yoshuawuyts]

It feels we could probably put out an RFC for the design in this so we can discuss the API before implementing it.

We've done an RFC before in https://github.com/rustasync/tide/blob/master/rfcs/001-app-new.md, but modeling it after https://github.com/rustwasm/gloo/blob/master/rfcs/001-mid-level-file-api.md is probably better.

@tomhoule @jonathanKingston would either of you want to start the process of drafting an RFC for session management?

[tomhoule]

I would love to, but realistically I won't have enough free time for it in the near future. I'm happy to participate in the discussion though :)

[jonathanKingston]

@yoshuawuyts I will get something drafted up tonight, I agree there are some open questions regarding API for users of tide that I have so the process will likely be fruitful.

@tomhoule if you have time to review the PR and see if this session issue was caused by it that would probably help advance things forward the quickest.

[tomhoule]

Will do (this week), sorry I haven't been able to get to it or even reply yet.

[jonathanKingston]

Will do (this week), sorry I haven't been able to get to it or even reply yet.

No rush, I was just trying to prioritise rather than expecting you to have read it 😃 thanks!

[tomhoule]

The fixup PR is merged, and I invited @jonathanKingston as collaborator to the repo so I'm not a blocker in case you want to work there.

[chrisdickinson]

I had some session middleware code underway for a blog site for learning rust; would it be useful to open a PR with that as a jumping off point for conversation? It can absolutely get thrown away – code is cheap etc – but my hope is that it'd help identify expected API affordances. Also, selfishly, I might learn a lot about what not to do in Tide middleware 😅

[jonathanKingston]

Please do post the code. I think the API design proposed in the existing code is pretty robust however the implementation likely can be approved. I'm still writing up the RFC too, it should be finished early this week.

…

On Mon, 27 May 2019, 21:05 Chris Dickinson, ***@***.***> wrote: I had some session middleware code underway for a blog site for learning rust; would it be useful to open a PR with that as a jumping off point for conversation? It can absolutely get thrown away – code is cheap etc – but my hope is that it'd help identify expected API affordances. Also, selfishly, I might learn a lot about what *not* to do in Tide middleware 😅 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACSYLAQB6WGG36MY2KX6Y3PXQ5KFANCNFSM4GCP75PQ> .

[jonathanKingston]

I did some further analysis with @JamelleG looking into the two implementations, in doing so I resolved the synchronisation issue of saving sessions.

@chrisdickinson implementation:

TL;DR: Uses RefCell data between middleware and route to indicate the session has been manipulated. Uses extensions_mut on the middleware and extensions on the route and synchronises with the RefCell.

Pro:

• Deref implementation is much simpler than futures oneshot
• Session rotation on change
• Store responsible for session key creation

Con

• Exposing the api through the RefCell might confuse consumers how the Session is being written.

Uncertainty:

• Send+Sync unsafe code doesn't seem a good idea. As the backing store uses RefCell it's certainly not Sync which seems bad. (Thanks for highlighting)
  • Perhaps this can use a Mutex instead to make it safe?

---

@tomhoule implementation

TL;DR: Uses futures::channel::oneshot to indicate to the middleware that the session has been manipulated. Uses extensions_mut on both middleware and route and synchronises with the oneshot.

Pros

• Using uuid implementation is a strong hash default
• Reliance on using the cookie lib for everything
• Typing of SessionId is clean

Con

• Requires handling a result

[chrisdickinson]

Yeah – the RefCell bits I borrowed from actix made me a bit nervous. I found last night that if you were to grab ctx.session() ahead of an .await and access it after the borrow checker would complain (correctly) about RefCell not being Send. That might be disqualifying?

Re: the @tomhoule implementation:

• I might suggest making the session id generation pluggable – or at least, controlled by the pluggable storage. Some session approaches will pack info into the cookie instead of using the cookie to convey a key to the locally stored data, and it'd be nice to support that use case. In other cases, api consumers will wish to decorate session keys with username info, to enable easy clearing of a single user's sessions from (e.g.) redis.
• Do I understand correctly that session ids and cookies are set and returned even if an endpoint does not modify session data? If so, in the npm website's case we wanted to specifically avoid this so that we could cache pages that didn't vary on session data. It would be handy to do dirty tracking on the session and optionally not send a set-cookie if the session data hadn't changed / wasn't created.
• This is kind of a pony request, but we used iron on the npm website to seal & verify the session id. It would be nice if Tide sessions provided this functionality out of the box.

I'm available to help look into any of these items, if you'd like!

[jonathanKingston]

That might be disqualifying?

I'm not sure, I think however it can be refactored not to need the unsafe code which is worth checking.

I might suggest making the session id generation pluggable

+1 agree, I was expecting to make the implementation part of the default trait implementation

Do I understand correctly that session ids and cookies are set and returned even if an endpoint does not modify session data?

I don't think it does, it's only saved on explicit save. I see the benefit you mention of.

This is kind of a pony request, but we used iron on the npm website to seal & verify the session id.

I'm not a fan of this approach but certainly permitting this is a good idea through extension.

In short I think I have come up with some requirements that would be great to meet whichever the solution is chosen:

• Use a strong UUID implementation for the session creation
• Require the use of futures for the storage backend
• The default implementation use Cookies with a preset of secure, http only, same site cookies.
• Not require handling results in the route
• Don't rotate the session id on every write (could cause race conditions for an AJAX page)
• Use the cookie middleware/ext to prevent duplication.
• Use secure defaults

Before finishing up my RFC I want to spend just as much time with the @chrisdickinson implementation integrating it into the same code I was using to test the @tomhoule code I was using. I'm fairly certain the pro's can be unified into a single implementation. I expect this to take until the weekend but I think it's worthwhile.

[yoshuawuyts]

@jonathanKingston ping, I was curious if you got around to trying out the unification.

[jonathanKingston]

Sorry with all hands coming up this has taken a back burner. I will get time to work on it after then, I understand if people want to progress without me.
I had some progress of unifying but I was struggling to get it to work with the application I am working on.

[yoshuawuyts]

@jonathanKingston no worries!

[fundon]

I wrote a general sessions module for web services. I will write an example for tide. https://github.com/trek-rs/sessions.

[fundon]

I wrote a test case for tide with redis. https://github.com/trek-rs/sessions/blob/rwlock/tests/redis-tide.rs