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
fix: Parse Server option extendSessionOnUse
not working for session lengths < 24 hours
#9113
base: alpha
Are you sure you want to change the base?
Conversation
Thanks for opening this pull request! |
Hello @mman, |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## alpha #9113 +/- ##
==========================================
- Coverage 94.13% 93.79% -0.35%
==========================================
Files 186 186
Lines 14687 14731 +44
==========================================
- Hits 13826 13817 -9
- Misses 861 914 +53 ☔ View full report in Codecov by Sentry. |
Thanks a lot @vivekjoshi556, I will take a look first thing next week, now OOO. |
Hey @mman. Just wanted to check if you got some time to look at the PR. |
@vivekjoshi556 The code looks nice and clean, the tests as well. Will try to deploy to my stage env later today. |
Signed-off-by: Manuel <5673677+mtrezza@users.noreply.github.com>
This fix just shifts the problem downwards from 24 hours to 1 minute. Because for a session length of <1 minute, the problem in #8981 persists. I wonder whether we need such a complex logic for this. If I understand #8981 (comment) correctly, the purpose of this logic is to find a middle ground between:
Wouldn't it be simpler to extend the session when the remaining session token validity time reached < 50% of the session length? For example:
Pros:
And could you please document this new behavior in the Parse Server option |
tests.forEach(({ title, sessionLength, sessionUpdatedAt, result }) => { | ||
it(`shouldUpdateSessionExpiry() when ${title}`, async () => { | ||
const { shouldUpdateSessionExpiry } = require('../lib/Auth'); | ||
const update = new Date(); | ||
update.setTime(update.getTime() - sessionUpdatedAt * 1000); | ||
const res = shouldUpdateSessionExpiry( | ||
{ sessionLength: sessionLength }, | ||
{ updatedAt: update } | ||
); | ||
|
||
expect(res).toBe(result); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please refactor by moving the loop into the test. We don't need a separate test log for every param. Considering #9113 (comment), 1 simple test would be enough, not sure whether you need a loop at all here.
extendSessionOnUse
not working for session lengths < 24 hours
@vivekjoshi556 Just a friendly ping here; if you could simplify the logic and add the functionality description to the option docs (this repo, not the docs repo), then we could go ahead and merge. |
So sorry just got a bit swamped with work. Give me some time |
Pull Request
This PR helps with dynamically extending session based on session Length.
Issue
Closes: #8981
Approach
I followed a similar approach to what @mman suggested here.
Tasks