Skip to content
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

Consider if migrating to tower-sessions is appropriate #174

Closed
maxcountryman opened this issue Sep 25, 2023 · 8 comments · Fixed by #175
Closed

Consider if migrating to tower-sessions is appropriate #174

maxcountryman opened this issue Sep 25, 2023 · 8 comments · Fixed by #175
Assignees
Labels
C-refactor Category: Refactor M-frontend Module: Frontend P-medium Priority: Medium

Comments

@maxcountryman
Copy link

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 Hirevo self-assigned this Sep 26, 2023
@Hirevo Hirevo added P-medium Priority: Medium M-frontend Module: Frontend C-refactor Category: Refactor labels Sep 26, 2023
@Hirevo
Copy link
Owner

Hirevo commented Sep 26, 2023

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:

//? Get the maximum duration of the session.
let expiry = match form.remember.as_deref() {
Some("on") => Duration::from_secs(2_592_000), // 30 days
_ => Duration::from_secs(86_400), // 1 day / 24 hours
};
//? Set the user's session.
session.insert("author.id", author_id)?;
session.expire_in(expiry);

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
Copy link
Author

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
Copy link
Owner

Hirevo commented Sep 26, 2023

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
Copy link
Author

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
Copy link
Author

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
Copy link
Owner

Hirevo commented Sep 28, 2023

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
Copy link
Author

That's great to hear!

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

@maxcountryman
Copy link
Author

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. 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-refactor Category: Refactor M-frontend Module: Frontend P-medium Priority: Medium
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants