Added expires property to the session #249
Conversation
|
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 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). |
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. |
|
P.S.
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 |
|
|
||
| getter session_key | ||
|
|
||
| def initialize(@session_key : String?) | ||
| end | ||
|
|
||
| def expires=(value : Int32) |
There was a problem hiding this comment.
Can we name this method (and the associated instance variable) #expires_at?
| def expires=(value : Int32) | ||
| @expires = value | ||
| @modified = true | ||
| end |
There was a problem hiding this comment.
I would prefer that we persist a Time object instead of an integer. Eventually, if we want to make it possible to configure that the session expires in a specific amount of time, then I think that we should support an additional #expires_in method that would accept a Time::Span object as input and that would update the @expires_at time instance variable accordingly.
There was a problem hiding this comment.
Ok. Just thought that for a store like Redis, an additional value conversion operation would be required (the TTL is specified as Int there)
| end | ||
|
|
||
| def expires_at | ||
| return nil if expires == 0 |
There was a problem hiding this comment.
We shouldn't allow for non-expiring sessions. Can we remove this from this PR? 🙏
If users want to set a very big expiry time by default that's fine, but I don't think the framework should allow never-expiring sessions as this is a very bad practice.
There was a problem hiding this comment.
For cookies, this will mean a short session, and when specifying TTL in the storage, you can specify the value sessions.cookie_max_age
For example like this
martenframework/marten-redis-session@8fde4d2#diff-6677a4928253a79a8b975ad522b82c587c233b1e0f9d40c9709d9ccd90a44368R68
Thus, this does not imply indefinite storage of the session
There was a problem hiding this comment.
The case of making the session cookie expire when the browser closes should be implemented with a different method in my opinion. How about adding an #expires_at_browser_close method that would set that would result in the #expires_at method returning nil? This would be more transparent for users (vs letting them specify a 0 expiry).
|
It seemed to me that storing Time:: Span in the @expires_in field would be more rational. |
|
@Cuspid88 Internally I think it's fine to persist a The only thing is that I think it should be possible to set the expiry using either a session.expires_at = 2.days.from_now
session.expires_in = 2.hoursAs long as we support this API, we can decide to store the expiry as a |
|
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 |
Now we can set the expiration time for cookies.
Note: Value 0 is would make the session last as long as the browser is open.