martenframework / marten

The pragmatic web framework.
https://martenframework.com
MIT License
425 stars 24 forks source link

Added expires property to the session #249

Closed Cuspid88 closed 3 months ago

Cuspid88 commented 3 months ago

Now we can set the expiration time for cookies.

session.expires = {TIME IN SECONDS}

Note: Value 0 is would make the session last as long as the browser is open.

ellmetha commented 3 months ago

Hey! Thanks for providing this PR.

I am not sure to understand how this capability would be useful given that it's already possible to configure the session cookie max age with the existing sessions.cookie_max_age setting. Could you please give me more details about your use case? Why do you need to set the session cookie max age on a per-session basis?

Additionally, I am not sure that changing the expiry of the session cookie is the best strategy for expiring sessions given that cookies can be tampered with on the client side. A better approach would be to make it possible for session stores to change the expiry of the data on the server side. This would involve ensuring that each session store properly makes use of the specified expiry time if any (instead of defaulting to the session cookie max age value).

Cuspid88 commented 3 months ago

A better approach would be to make it possible for session stores to change the expiry of the data on the server side.

You are absolutely right - this is exactly how I plan to use the session with Redis. But to control the TTL, the general value of the abstract class will be used. Here I added the TTL changes Redis store: https://github.com/martenframework/marten-redis-session/pull/1

Cuspid88 commented 3 months ago

P.S.

I am not sure to understand how this capability would be useful given that it's already possible to configure the session cookie max age with the existing sessions.cookie_max_age setting.

I believe it is extremely important to be able to limit the lifetime of a session (For example, when a user does not check the "remember me" option when logging in). It will also be useful to be able to adjust the TTL on the storage.

ellmetha commented 3 months ago

I believe it is extremely important to be able to limit the lifetime of a session (For example, when a user does not check the "remember me" option when logging in). It will also be useful to be able to adjust the TTL on the storage.

Sessions don't last forever by default. They are always expiring based on the sessions.cookie_max_age setting value. Obviously, this does not allow for fine-grained session expiry configuration.

Cuspid88 commented 3 months ago

It seemed to me that storing Time:: Span in the @expires_in field would be more rational. What do you think?

ellmetha commented 3 months ago

@Cuspid88 Internally I think it's fine to persist a Time::Span object.

The only thing is that I think it should be possible to set the expiry using either a Time or a Time::Span object. For example:

session.expires_at = 2.days.from_now
session.expires_in = 2.hours

As long as we support this API, we can decide to store the expiry as a Time::Span object internally within session stores.

Cuspid88 commented 3 months ago

Is there anything else I can do?

ellmetha commented 3 months ago

Is there anything else I can do?

@Cuspid88 Can we add unit tests for the changes introduced in this pull request? 🙏 All the public methods added to the Marten::HTTP::Session::Store::Base class should ideally be unit-tested.