Fix bugs in extend_remember_period
#5351
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.
| @@ -127,37 +127,83 @@ def cookie_expires(key) | |||
| end | |||
| end | |||
|
|
|||
| test 'extends remember period when extend remember period config is true' do | |||
There was a problem hiding this comment.
The tests added in #4039 were incorrect; this assertion was passing because the user was getting logged out (because the test moves backwards in time). Added an assert_response to confirm this, then refactored the test to be correct.
| end | ||
|
|
||
| travel_to 33.months.from_now do | ||
| # don't log in for over a year, we get logged out |
There was a problem hiding this comment.
I'm pretty stuck getting this working, because I can't work out a way to get around warden's short circuiting. I think the answer is another hook similar to Timeoutable but it's getting very messy.
There was a problem hiding this comment.
Shouldn't a timeout be set for these tests?
Otherwise the session itself has not timed out so the user is still logged in.
A valid remember token will prevent the session from timing out or automatically create one if the session doesn't exist. Neither a valid, invalid or expired remember token causes a logout.
| Warden::Manager.after_set_user only: :fetch do |record, warden, options| | ||
| if record.respond_to?(:extend_remember_me?) && record.extend_remember_me? && | ||
| options[:store] != false && warden.authenticated?(options[:scope]) | ||
|
|
||
| Devise::Hooks::Proxy.new(warden).remember_me(record) | ||
| end | ||
| end |
There was a problem hiding this comment.
Hi, I was playing with your fork after realising extend_remember_period is bugged, but I've run into a problem with it that seems to be linked to this hook.
I have custom User#remember_me! and User#remember_me? methods, and I noticed that this hook results in remember_me! getting called before remember_me?, meaning that remember_me? will always return true when it is tested later in the workflow.
Should this hook's if condition perhaps test that remember_me? is true, and only execute if it is?
There was a problem hiding this comment.
This fork 100% isn't working right yet. I've also stopped using devise on the project I needed it for, so I don't have a good memory of the problem here.
If you're having similar issues and want to have a go at fixing them feel free to build off this fork/branch.
There was a problem hiding this comment.
remember_me? is always true:
def remember_me?
true
end
There's a remember_me_is_active? in the Rememberable controller that can check that there's a valid remember cookie for the current user.
This PR adds better tests for
extend_remember_periodand 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 #3950 (comment) which both describe how the feature should work. To summarise:
extend_remember_periodwill keep your session aliveconfig.remember_for) you'll get logged outIn reality, it looks like what happens is:
config.remember_forconfig.remember_for) you'll stay logged in, as long as your warden session remains intactSo 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_periodworks is that it only extends the session whenStratgies::Rememberable#authenticategets 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.So the first thing this PR change is to add a hook so that
extend_remember_periodnow works on every request.The next bug is that if once
config.remember_foris up, the user is not currently forgotten.