Fix bugs in extend_remember_period (reprise)
#5711
Open
+228
−17
Conversation
This file contains 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
This PR adds better tests for `extend_remember_period` and tries to better document what the feature is supposed to do. Currently it's not very well specified, and I'm not sure it's behaving as expected. I found https://gitlab.com/gitlab-org/gitlab/-/merge_requests/32730 and heartcombo#3950 (comment) which both describe how the feature should work. To summarise: - if you log in to the application regularly, `extend_remember_period` will keep your session alive - if you *don't* log into the application for a while (longer than `config.remember_for`) you'll get logged out In reality, it looks like what happens is: - if you log in to the application regularly, your session will stay alive for as long as `config.remember_for` - if you *don't* log into the application for a while (longer than `config.remember_for`) you'll stay logged in, as long as your warden session remains intact So this means for example if you keep your browser open for longer than `config.remember_for`, your rememberable cookie will expire, but you'll still be logged in. Currently how `extend_remember_period` works is that it only extends the session when [`Stratgies::Rememberable#authenticate`](https://github.com/heartcombo/devise/blob/master/lib/devise/strategies/rememberable.rb#L30) gets called. This gets called when the user logs in and a strategy is used. But if there's already a user logged in and recognised by warden, then warden [short circuits the strategies](https://github.com/wardencommunity/warden/blob/master/lib/warden/proxy.rb#L334). So the first thing this PR change is to add a hook so that `extend_remember_period` now works on every request. The next bug is that if once `config.remember_for` is up, the user is not currently forgotten.
The remember_me token will be irrelevant unless the session would timeout. A session timeout (which also won't happen if there is a valid remember_me token) is required to logout the user.
If the session expires then a re-authentication will occur via the remember token, which could automatically extend it via the authentication process instead of the extend process. The remember period needs to be extended even if the session has not yet expired. Arrange for this to happen and then let the session expire late enough that the remember token must have been extended for the user to still be logged in. Ensure that remember me isn't set by the extend process when remember me is not being used for the current session.
…d-remember-period-true-behavior
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.
close #5351, close #5418, close #5701
This pull request contains the same changes as #5418. However, since PR #5418 was created three years ago, it needed conflict resolution. While I could have asked @nomis to resolve the conflicts, I decided to create a new PR myself since it was just a matter of resolving those conflicts. With this fix,
extend_remember_periodnow works as expected on my end.