-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Remember_token always regenerated when user logs in: existing one never kept anymore #3950
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
Comments
@michieldewit are you using the manual remember_token? If so, I believe you are right. I completely forgot about this case, sorry about this. We need to tackle it. |
@josevalim We use stored
|
@michieldewit you no longer need to renew or expire the token. So my suggestion would be to just set it if none is set. If you want to expire existing tokens, you just clean the database column for the desired rows. |
@josevalim About the expiration you are probably right. Still it feels a little safer to know that remembered sign ins will automatically expire when a user has not been active for X days |
@michieldewit this property is now intrinsic to the new system, that's why we don't need to worry about it at the token level. :) |
Any update on this? We've add the monkey patch, but would prefer this fixed in Devise. |
Thanks @reedloden and @michieldewit . I think a simpler fix on this is to rewrite the remember_me function to work like this: def remember_me!(*)
self.remember_token ||= self.class.remember_token if respond_to?(:remember_token)
self.remember_created_at ||= Time.now.utc
save(validate: false) if self.changed?
end Can you please confirm this also works as expected? I only changed the remember_token assignment to use |
I can confirm the simpler fix works, wouldn't this be a bit easier to read though? def remember_me!(*)
return unless respond_to?(:remember_token)
self.remember_token ||= self.class.remember_token
self.remember_created_at ||= Time.now.utc
save(validate: false) if self.changed?
end |
We still need to execute remember_created_at if you don't have |
@josevalim Could you explain why we need to execute |
@jjoos because you can use the remember token completely without tokens, by relying on the authentication salt |
Still awaiting a fix in Devise for this. |
This issue was related to extended remember me configuration was working as always true. It is fixed on Devise Feel free to reopen if the problem persist. Related PR: #4039 |
I just upgraded to Devise 3.5.8 and I'm still seeing this issue; the remember_token is reset every time the user logs in, so that if they log in on browser A, and then later on browser B, the remember_token changes and the session on browser A is invalidated. Am I supposed to be using |
@bsoule Thank you for the feedback. The The option you have suggested is support multiple remember tokens, I think today Devise does not have this option out of the box. I'll check with @josevalim about it. You may want to implement it by yourself or find gem that does. Feel free to open a pull request to improve the configuration template text or the README documentation with a better explanation about the extend remember me period. Best |
@josevalim What's the expected behaviour when user sign in using a browser with remember me checked with a valid existent remember token from another browser? Should it override the existent one? Is possible today Devise support multiple remember me cookie tokens from different browsers? |
Thanks for explaining the extend_remember_period option. Much clearer now :-) From reading the comments in this issue, it sounds like remember_token should not get overwritten when a user is signing in and a valid token already exists. Above @josevalim proposes a fix where you'd set the remember_token with ||= in remember_me! If the expected behavior is indeed as described in this thread, then the original issue described still remains in the master branch. I can submit a PR for this. |
@bsoule You're right. Feel free to submit a PR with a fix :) . Would be good have a regression test too. |
The remember_token should not get overwritten when a user is signing in and a valid token already exists. Fixes heartcombo#3950.
Hi people, if anyone has some time to try out the branch with the fix and give us some feedback. I can release a patch version. The PR with the fix: #4101 |
I think it's fixed now on |
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.
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.
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.
In commit c929966 a change was made that causes
remember_token
s to be regenerated without checking whether the old one has expired. This causes a problem when users log in on multiple browsers or devices. Every time they log in, their session on other browsers/devices is invalidated, because a new token is generated.My project is forced to stick with Devise 3.5.3 because of this breaking change. The fact that the commit I mentioned still contains a TODO is probably indicative of the fact this is a problem to be resolved.
The text was updated successfully, but these errors were encountered: