devise icon indicating copy to clipboard operation
devise copied to clipboard

Fix bugs in `extend_remember_period`

Open ghiculescu opened this issue 4 years ago • 0 comments

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 https://github.com/heartcombo/devise/issues/3950#issuecomment-217326227 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 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.

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.

ghiculescu avatar Feb 23 '21 04:02 ghiculescu