Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[project-base] Automatically delete sessions after 7 days of user inactivity #1842

Merged
merged 3 commits into from Jul 1, 2020
Merged

[project-base] Automatically delete sessions after 7 days of user inactivity #1842

merged 3 commits into from Jul 1, 2020

Conversation

stanoMilan
Copy link
Contributor

@stanoMilan stanoMilan commented May 12, 2020 •
edited by TomasLudvik

Q A
Description, reason for the PR Session were never deleted from Redis and this caused some high disk space usage. This is fixed by this PR.
New feature No
BC breaks No
Fixes issues ...
Have you read and signed our License Agreement for contributions? Yes

Delete a session after 24 hours of user inactivity, because the session stays on the server forever and takes up memory space.

@henzigo
Copy link
Member

henzigo commented May 12, 2020 •
edited

We should delete sessions after 30 days because of remember me option. See

shopsys/project-base/config/packages/security.yaml

Line 66 in 6b54709

lifetime: 2592000 # 30 days

@pk16011990
Copy link
Member

@henzigo, remember_me is performed by cookie and it is not depend on session, or how do you mean it?

@stanoMilan stanoMilan changed the title Delete a session after 24 days of user inactivity. Delete a session after 24 hours of user inactivity. May 12, 2020
henzigo
henzigo approved these changes May 12, 2020
View reviewed changes
Copy link
Member

@henzigo henzigo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok sorry, my comment doesn't make any sense.

@grossmannmartin
Copy link
Member

This should work (including remember me feature)
Admin will be forced to log again into administration after 24hours of inactivity even when he does not close the browser window.

@pk16011990
Copy link
Member

@s3tezsky why 👎 ? It is realy problem for production eshops, because it creates records in redis to forever (and it requires RAM). Please explaint it to me.

@grossmannmartin grossmannmartin mentioned this pull request May 22, 2020
Closed
@henzigo
Copy link
Member

henzigo commented Jun 8, 2020

Hello, could you merge this PR? Sessions in Redis may increase more than 10GB of RAM after some time in running production because of this

TomasLudvik
TomasLudvik requested changes Jun 10, 2020
View reviewed changes
Copy link
Member

@TomasLudvik TomasLudvik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @stanoMilan,

thank you for your PR. We have discussed this and it is nice change. Only think I want you change is TTL to one week, as 24 hours login for users might be not enough.

project-base/config/packages/snc_redis.yaml Outdated Show resolved Hide resolved
@TomasLudvik TomasLudvik changed the title Delete a session after 24 hours of user inactivity. [project-base] Delete a session after 24 hours of user inactivity Jun 10, 2020
@sslt sslt added the Status: in implementation Resolving of this issue is currently in progress label Jun 11, 2020
@stanoMilan
Copy link
Contributor Author

@TomasLudvik for the old session does not set the TTL to 604800 but this sessions remains set to -1(for all time ).

@TomasLudvik TomasLudvik changed the title [project-base] Delete a session after 24 hours of user inactivity [project-base] Automatically delete sessions after 24 hours of user inactivity Jun 30, 2020
@TomasLudvik TomasLudvik changed the title [project-base] Automatically delete sessions after 24 hours of user inactivity [project-base] Automatically delete sessions after 7 days of user inactivity Jun 30, 2020
@TomasLudvik TomasLudvik changed the base branch from master to 9.0 June 30, 2020 10:17
@TomasLudvik TomasLudvik added Enhancement New feature or request for change from user point of view and removed Status: in implementation Resolving of this issue is currently in progress labels Jun 30, 2020
@s3tezsky
Copy link
Contributor

@s3tezsky why 👎 ? It is realy problem for production eshops, because it creates records in redis to forever (and it requires RAM). Please explaint it to me.

Just wanted to bring more attention here.
At first I thought we may reduce size of session first. After some digging I find out they are pretty small.

@TomasLudvik TomasLudvik added this to the SSFW 9.0.1 milestone Jul 1, 2020
@sonarcloud
Copy link

sonarcloud bot commented Jul 1, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@s3tezsky s3tezsky merged commit 2fbafaa into shopsys:9.0 Jul 1, 2020
grossmannmartin pushed a commit that referenced this pull request Sep 2, 2020
@stanoMilan @grossmannmartin
session are now automatically deleted after 7 days of user inactivity (…
fcf7a07 
…#1842)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@henzigo henzigo henzigo approved these changes

@TomasLudvik TomasLudvik TomasLudvik approved these changes

@sslt sslt sslt approved these changes

@s3tezsky s3tezsky s3tezsky approved these changes

Assignees
No one assigned
Labels
Enhancement New feature or request for change from user point of view
Projects
None yet
Milestone
SSFW 9.0.1
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@stanoMilan @henzigo @pk16011990 @grossmannmartin @s3tezsky @TomasLudvik @sslt @msshopsys