Sessions: do not save on each request #9402
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ericholscher
approved these changes
Jul 6, 2022
There was a problem hiding this comment.
This seems reasonable. 15 days is pretty short, and might be annoying in some ways. I think it would be good if we could get to 30 days in the future, and perhaps just trigger a task automatically for users that have logged in login_days_ago % 7 = 0 or something?
|
OK, I will put this back as it was: 30 days. I will create a task to do the resync as I wanted, as well. |
Currently, we have a 30 days expiry time for cookies with "save on every request" enabled. That means users won't need to re-login if they hit at least one Read the Docs page every 30 days. In an attempt to reduce this "infinit session time", I'm disabling the "save on every request". This way we are forcing the users to re-login every 30 days --no matter if they hit a Read the Docs page during that time or not. This will help us to know what are the active users because we will be able to check `User.last_login` and get reasonable numbers. With the current configuration, a user that used the platform _today_ could have a pretty old `last_login` since we are not forcing re-login at all.
humitos
force-pushed
the
humitos/session-cookie
branch
from
a0820b5 to
f193886
Compare
|
I updated the commit message and the description as well |
humitos
changed the title
humitos
added a commit
that referenced
this pull request
Jul 7, 2022
Copy the logic from `readthedocs-corporate` to resync `RemoteRepository` each time the user logs in. We are forcing users to re-login every 30 days minimum. Related: #9402
davidfischer
approved these changes
Jul 7, 2022
humitos
added a commit
that referenced
this pull request
Jul 11, 2022
Copy the logic from `readthedocs-corporate` to resync `RemoteRepository` each time the user logs in. We are forcing users to re-login every 30 days minimum. Related: #9402
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, we have a 30 days expiry time for cookies with "save on every request"
enabled. That means users won't need to re-login if they hit at least one Read
the Docs page every 30 days.
In an attempt to reduce this "infinit session time", I'm disabling the "save on
every request". This way we are forcing the users to re-login every 30 days --no
matter if they hit a Read the Docs page during that time or not.
This will help us to know what are the active users because we will be able to
check
User.last_loginand get reasonable numbers. With the currentconfiguration, a user that used the platform today could have a pretty old
last_loginsince we are not forcing re-login at all.Discussion: https://github.com/readthedocs/meta/discussions/31