Fix bugs in extend_remember_period (reprise)
#5711
Conversation
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
|
@dlwr This looks great - I have been trying to work out what's going on with devise... However it doesn't seem to fix the following problem (which seems to be very similar to the test case): Setting timeoutable timeout_in to 30s and rememberable remember_for to 90s (with extend remember period). Expect to be logged in because remember cookie should extended and honoured, but logged out. I created this repo to test it: https://github.com/timdiggins/devise-rememberable (with devise/main) I used this branch of devise in https://github.com/timdiggins/devise-rememberable/tree/use-original-fix-branch Neither of these branches fixed the problem (at least using rails 7.1 and ruby 3.0) Any ideas why this isn't working? |
|
Hi. I think you should set |
OMG 🤦 Thank you! I will retry with the correct setup |
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.