Feature: auto-renew session expiry before expiry #206
Conversation
Fixed dependency version.
Later versions of connect-redis exports a {default: ..} object
| @@ -32,7 +32,7 @@ | |||
| "@fastify/pre-commit": "^2.0.2", | |||
| "@types/node": "^20.1.0", | |||
| "connect-mongo": "^5.0.0", | |||
| "connect-redis": "^7.0.0", | |||
| "connect-redis": "7.0.0", | |||
There was a problem hiding this comment.
I locked it because benchmark was not running due to following error
TypeError: redisStoreFactory is not a function
But, when I re-checked it now, I realized that I should have lock this version to ^6 rather than 7.0.0 because the error still happening in 7.0.0 version.
There was a problem hiding this comment.
The benchmark is fixed in #248 :)
| @@ -93,6 +93,12 @@ Setting this to `false` can save storage space and comply with the EU cookie law | |||
| ##### rolling (optional) | |||
| Forces the session identifier cookie to be set on every response. The expiration is reset to the original maxAge - effectively resetting the cookie lifetime. This is typically used in conjuction with short, non-session-length maxAge values to provide a quick expiration of the session data with reduced potential of session expiration occurring during ongoing server interactions. Defaults to true. | |||
|
|
|||
| ##### refresh (optional) | |||
| Automatically refresh ( extend the expiry of ) session before `<refresh>` milliseconds before `expiry`. This is more efficient way than setting `rolling` option. | |||
There was a problem hiding this comment.
Does it not clear what we should set - a boolean? a string?
There was a problem hiding this comment.
refresh option is an number . it is a duration in milliseconds . I can fix this by editing the Readme once the discussion is complete.
lib/fastifySession.js
Outdated
| } else { | ||
| const shouldRefresh = refresh ? request.session.cookie.expires.getTime() < (Date.now() + refresh) : false | ||
| return rollingSessions || shouldRefresh || (Boolean(request.session.cookie.expires) && request.session.isModified()) | ||
| } |
There was a problem hiding this comment.
very unclear - consider refactoring it:
if rollingSessions is true, we are calculating the shouldRefresh var - and it is unnecessary.
There was a problem hiding this comment.
very unclear - consider refactoring it:
The previous code was a big single ternary operator . I changed it to if else condition for the sake of readability .
I din't do anything else than calculating shouldRefresh
if rollingSessions is true, we are calculating the shouldRefresh var - and it is unnecessary.
Yes that is correct in terms of performance. I did this for the sake of code readability. I can fix this
| @@ -187,6 +188,7 @@ function fastifySession (fastify, options, next) { | |||
| return | |||
| } | |||
|
|
|||
| session.touch() | |||
There was a problem hiding this comment.
I found a bug like case while trying to implement this option.
The issue is this: the session.expiry was always dynamically calculated at the time of incoming request using maxage & Date.now() instead of loading it from session store.
This issue was not causing any problem since rolling option was enabled by default.
But if disable rolling option, session.expiry value will become a useless information because, it will be always unexpired value because it is calculated dynamically for each request.
But in overall, the system was working well due to proper session cleanup taking place in 1 second interval .
I fixed this behavior by loading expiry value from session store.
When that is done, we need to manually call touch() to extend the expiry of session before saving it into session store.
| @@ -13,6 +13,9 @@ module.exports = class Cookie { | |||
| this._expires = null | |||
|
|
|||
| if (originalMaxAge) { | |||
| if (cookie.expires) { | |||
There was a problem hiding this comment.
Should we use originalMaxAge here?
There was a problem hiding this comment.
No. Please see my previous comment regarding expiry calculation bug. In this case, if cookie.expiry is set, it will be a string loaded from session store. So, it is converted to proper Date object using this if condition
| @@ -40,7 +43,7 @@ module.exports = class Cookie { | |||
| } | |||
|
|
|||
| set maxAge (ms) { | |||
| this.expires = new Date(Date.now() + ms) | |||
| if (!this.expires) { this.expires = new Date(Date.now() + ms) } | |||
There was a problem hiding this comment.
We are not setting the maxAge any more here - why should we check it?
There was a problem hiding this comment.
This if conduction ensures that this.expiry is only set for newly created session and not for existing session loaded from session store .
For session loaded from session store, the expiry value will be already there ( it is determined at the time of session creation ).
Co-authored-by: Manuel Spigolon <behemoth89@gmail.com>
|
@Eomm Let me know what you think about this PR. If you are ready to proceed , I will can make the changes you suggested . |
Summary of the option ( copied from readme )
refresh (optional)
Automatically refresh ( extend the expiry of ) session before
<refresh>milliseconds beforeexpiry. This is more efficient way than settingrollingoption.The default value is
0 msmeaning, this feature is disabled.Consider
cookie.maxAgeis60 seconds. If we setrefresh=20 seconds, then it will auto refresh the session if sent any request after 40 second.it is recommended to disable
rollingandsaveUninitializedoptions if we set this optionChecklist
npm run testandnpm run benchmarkand the Code of conduct