-
Notifications
You must be signed in to change notification settings - Fork 38
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
Sessions may never expire: make Renewal Timeout opt-in, implement Absolute Timeout #553
Comments
This is pretty much what all session systems do?
Disagree here too: session stays hot while being used 🤔
I didn't understand this part. |
Our company and customers policy mandates that every 12 hours each user must re-authenticate and re-authorised for any work to be done. That's what Absolute Timeouts are for: I don't care if your are active or not, I don't care if you are interacting with the app or not, after 12 hours you must re-login. The An advanced user may just throw a script that keeps refreshing its session every 30 seconds. |
I'd say that this is domain-specific logic then, and shouldn't be part of this lower-level infrastructural layer. What you are looking for is something akin to the
|
What I'm arguing here is that the underlying storage is mostly tracking the session storage: things like "you must tap your feet 3 times at the 3rd day, then wait for the planets to align and sing the song of time" should be at a higher level :D |
I partially disagree on this: binding high level requirements to low level bonds is efficient and effective. But understand your argument and accept the reason for marking issues 2 and 3 as invalid I'll open a PR for issue 1 though. |
Closing as per #554 |
There are 3 Automatic Session Expiration mechanism this library should provide:
Issue 1/3: Renewal Timeout is a hidden behaviour
Currently both
README.md
anddocs/configuration.md
lack of a proper explanation of what's going on under the hood, which is that by default the session is potentially infinite if the user keeps interacting with the application and the configured$expirationTime
is greater than$refreshTime
, and that's highly likely given that the default$refreshTime
is 60 seconds.This is even worse at a communication level considering that it deeply differs from the default
ext-session
behaviour; I bet most consumer of this library expect just replacement of code, not of timeout mechanisms.Solution: write a better documentation
Issue 2/3: Renewal Timeout should be opt-in
Renewal Timeout mechanism it's not recommended to be a default.
Re-authentication should occur if not explicitly configured otherwise.
I'm using the undocumented workaround of setting
$refreshTime = 1 + $expirationTime
to disable it, if anyone wonders how to opt-out in current major release.Solution: disable the default Renewal Timeout, make it opt-in. BC break
Issue 3/3: Absolute Timeout is unachievable
Even when the Refresh Timeout is disabled with the
$refreshTime = 1 + $expirationTime
trick, the session gets renewed if the content changes, and the Timeout shifts afar again because the expiration time always starts from$this->clock->now()
:storageless/src/Storageless/Http/SessionMiddleware.php
Lines 201 to 203 in fb7ab95
storageless/src/Storageless/Http/SessionMiddleware.php
Lines 224 to 225 in fb7ab95
Solution: when Renewal Timeout is disabled, inherit
$expiresAt
value from current Session token. BC breakPing @malukenho and @lcobucci for feedbacks, you've worked on this in 89719b3 and #14
I'll propose a PR once you've reviewed this issue.
The text was updated successfully, but these errors were encountered: