search

Hirevo / alexandrie

An alternative crate registry, implemented in Rust.
https://hirevo.github.io/alexandrie/
Apache License 2.0
493 stars 55 forks source link

Consider if migrating to `tower-sessions` is appropriate #174

Closed maxcountryman closed 11 months ago

maxcountryman commented 11 months ago

Hi folks,

I'm the author of axum-sessions, which is a crate I see you all are using--I'm glad to see you've found it useful!

Over the course of the last year or so we've hit some roadblocks with our key dependency, async-session. The long and short of that is in order to unblock that and address some problems with axum-sessions's design, we've released a new crate which aims to replace axum-sessions: tower-sessions.

tower-sessions no longer relies on a third-party crate for its session implementation and this has allowed us to change its design to better fit tower and the broader tower ecosystem (i.e. axum). For instance, we no longer need writable and readable sessions, and have simplified the interface as a result.

I'd be curious if there's interest in migrating and am happy to help if so.

Hirevo commented 11 months ago

Hi, thank you very much for notifying this project of this initiative !

I've went ahead and started the work for this migration in PR #175, as its seemed to be quite straightforward to make these changes.

One question I have, which I don't know yet how to handle, is that axum-sessions had an WritableSession::expire_in, which allowed to change/extend the expiration date/time of an existing session.

It is a feature that Alexandrie used to implement the Remember Me checkbox on the login page of the frontend, like so:

https://github.com/Hirevo/alexandrie/blob/4813442d0bc4ff419725c1684ffff94960d2cce7/crates/alexandrie/src/frontend/account/login.rs#L153-L161

I wasn't able to find a similar method in tower-sessions.
There is a Session::with_max_age method, but it seems like a method meant for use in a SessionStore impl rather than in endpoint handlers and, looking at the code of SessionManager, it doesn't seem like it has any effect on the max_age of the response cookie.

So I guess, my question is:
Is there currently a way to achieve something similar (maybe my previous implemented is misguided) ? Or would tower-sessions be interested in implementing a similar feature in the future ?

maxcountryman commented 11 months ago

Hi, thanks for having a look and exploring this direction!

Just to make sure I'm understanding correctly, the use case is to extend the session TTL while it's being used?

If so, you're correct that's missing from the current release.

That said, that is a feature I would like to incorporate, especially if folks who use axum-sessions are using it.

Hirevo commented 11 months ago

Extending the session's TTL is indeed the use case.

The default duration of a session in Alexandrie currently (using axum-sessions) is 24 hours, and I was using this method to not only reset it (make the session valid for 24 hours after the login succeeded), but also potentially extend it to 30 days if the user asked to be remembered.

I'm glad to hear that this is considered, as I think it could be useful to others to, for example, automatically regenerate sessions to prevent the user from being suddenly logged out while navigating.

maxcountryman commented 11 months ago

This makes sense to me. I've opened an issue to track on the tower-sessions repo. It shouldn't be too difficult: I imagine we can add a method, set_expiration_time(expiration_time: OffsetDateTime) or similar to support this.

maxcountryman commented 11 months ago

I've merged in some unreleased changes that provide set_expiration_time and set_expiration_time_from_max_age.

This will allow applications to set the expiration time to some arbitrary time::OffsetDateTime or specify a time::Duration from OffsetDateTime::now_utc at which the session will be considered expired.

Using either of these methods will persist the session back to the store and set a cookie on the client with the updated max-age attribute.

Let me know if you think this would fit your use case.

Hirevo commented 11 months ago

Thank you for acting so quick on this.
This is indeed all I needed in Alexandrie's case.

I've tested the changes in #175 by taking a git dependency and it works perfectly.
I'll make sure to merge it whenever these changes are released in crates.io.

maxcountryman commented 11 months ago

That's great to hear!

I'm planning to cut a new release this weekend, once I resolve a couple of outstanding changes.

maxcountryman commented 11 months ago

Hi again, I've just released v0.2.0, which includes the above changes. Please do let me know if you run into any other problems or missing feature overlap with axum-sessions. 🙂