search

maxcountryman / tower-sessions

🥠 Sessions as a `tower` and `axum` middleware.
MIT License
249 stars 43 forks source link

Ensure to load session expiry from session store #207

Closed longsleep closed 3 months ago

longsleep commented 3 months ago

Sessions are stored together with their expiration date in the store. Whenever loading sessions from the store, the Session type should reflect the backend data exactly. With this change, the stored expiration date is set to the Session type expiry data if it has not been set at initialization or by calling set_expiry.

This might introduce a behavior change for code using the Session type without setting an expiry. Before this change, the expiry would be relative to the current time implicitly even for existing sessions after load. With this change, the loaded Session expiry would be the timestamp which was set the last time the Session was saved.

Fixes: https://github.com/maxcountryman/tower-sessions/issues/206

longsleep commented 3 months ago

Discussion from https://github.com/maxcountryman/tower-sessions/issues/206#issuecomment-2266214599 continues here.

While #178 might be a little bit related, but the issue this PR fixes is entirely unrelated to calling "set_expiry" or really any other optional API one could call. This here is about changing the internal behavior and I think it is simply missing this and the discussion if this behavior should be changed or not should have its dedicated place.

So, to make my point clear - here is the change which does the trick without doing any harm as no behavior changes if the expiry is actually set manually.

maxcountryman commented 3 months ago

With this change, the loaded Session expiry would be the timestamp which was set the last time the Session was saved.

As I've mentioned already, it's not possible to hydrate Expiry this way: the data is simply not stored in the session store as it stands.

Could you please use the existing discussion thread and issue I linked to engage with the conversation around this API?

longsleep commented 3 months ago

As I've mentioned already, it's not possible to hydrate Expiry this way: the data is simply not stored in the session store as it stands.

What makes you say that? I assumed that session store implementations use serde to serialize and deserialze the Record.

This is what the redis-store does: https://github.com/maxcountryman/tower-sessions-stores/blob/cecfba06e7ebc031d59d51a5926e19b3bc2c2d0d/redis-store/src/lib.rs#L77 and this change works perfectly fine with it since the expiry_date field will be serialized/deserialized.

https://github.com/maxcountryman/tower-sessions/blob/05517aaf3f1caf7fc548b3eaf831734b37aab96a/tower-sessions-core/src/session.rs#L916

I guess a store implementation could opt to just serialize the Record.Data field but how would that then produce the Record when loading, after all the store API https://github.com/maxcountryman/tower-sessions-stores/blob/cecfba06e7ebc031d59d51a5926e19b3bc2c2d0d/redis-store/src/lib.rs#L110 requires a Record to be returned - what expiry_date is in there if it was not stored?

maxcountryman commented 3 months ago

Could you please use the existing discussion thread and issue I linked to engage with the conversation around this API?