Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

extend_remember_period not updating remember_user_token cookie expiry on session timeout #5701

Open
seanvm opened this issue Jul 24, 2024 · 1 comment · May be fixed by #5711
Open

extend_remember_period not updating remember_user_token cookie expiry on session timeout #5701

seanvm opened this issue Jul 24, 2024 · 1 comment · May be fixed by #5711

Comments

@seanvm
Copy link

seanvm commented Jul 24, 2024

Environment

  • Ruby 3.1.4
  • Rails 6.1.5
  • Devise 4.9.3

Current behavior

Devise config: both :timeoutable and :rememberable. Set timeout to 30 seconds: timeout_in: 30.seconds, and enable extend_remember_period

With extend_remember_period:

  • Close your browser (or delete the session store cookie) within the config.remember_for period the remember_user_token cookie will be updated with a new expiration when you revisit the site, and you won't need to login.

  • If you don't close your browser or remove the session cookie, but your session has timed out, the remember_user_token is not updated. If the config.remember_for period has elapsed you will need to login again.

Expected behavior

I would expect a session timeout to be treated the same as a cleared session. If a user is active within the remember_for period, they should not need to login again.

It seems that the session is only extended when Stratgies::Rememberable#authenticate gets called. This doesn't seem to be called when no session cookie is found - even if the user is "remembered" due to a valid remember_user_token cookie.

@dlwr
Copy link

dlwr commented Aug 29, 2024

I faced the same issue. When I searched for PRs, I found this one: #5418. It’s from three years ago, so it’s understandably in conflict, but the changes themselves seem good. I feel that aiming to merge this PR would be a good idea. 💭

@dlwr dlwr linked a pull request Aug 29, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants