Skip to content

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

Closed
michieldewit opened this issue Feb 11, 2016 · 20 comments
Labels

Comments

@michieldewit
Copy link

In commit c929966 a change was made that causes remember_tokens 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.

@josevalim
Copy link
Contributor

@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.

@michieldewit
Copy link
Author

@josevalim We use stored remember_tokens indeed. Going back to 3.5.3 did not work for us either, btw, because of the new format of the remember cookies. Right now, we monkey patched Devise::Models::Rememberable.remember_me! in our User model to work more or less like in the old situation. It's ugly, but it works:

def remember_me!
    token_expired = self.remember_token.blank? || self.remember_created_at.blank? || self.remember_created_at + self.class.remember_for < Time.now.utc
    if token_expired
      self.remember_token = self.class.remember_token
      self.remember_created_at = Time.now.utc
      save(validate: false) if self.changed?
    end
  end

@josevalim
Copy link
Contributor

@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.

@michieldewit
Copy link
Author

@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

@josevalim
Copy link
Contributor

@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. :)

@reedloden
Copy link

Any update on this? We've add the monkey patch, but would prefer this fixed in Devise.

@josevalim
Copy link
Contributor

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 ||=.

@jjoos
Copy link

jjoos commented Mar 16, 2016

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

@josevalim
Copy link
Contributor

We still need to execute remember_created_at if you don't have respond_to?(:remember_token).

@jjoos
Copy link

jjoos commented Mar 16, 2016

@josevalim Could you explain why we need to execute self.remember_created_at ||= Time.now.utc when self.remember_token isn't being set?

@josevalim
Copy link
Contributor

@jjoos because you can use the remember token completely without tokens, by relying on the authentication salt

@liamwhite
Copy link

Still awaiting a fix in Devise for this.

@ulissesalmeida
Copy link
Contributor

ulissesalmeida commented Apr 19, 2016

This issue was related to extended remember me configuration was working as always true.

It is fixed on Devise 3.5.7 and 4.0.0.

Feel free to reopen if the problem persist.

Related PR: #4039

@bsoule
Copy link

bsoule commented May 4, 2016

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 extend_remember_period = true to get the behavior I want (i.e. logging in to a second device doesn't log them out on the first). The documentation isn't very verbose about what extend_remember_period does.

@ulissesalmeida
Copy link
Contributor

ulissesalmeida commented May 6, 2016

@bsoule Thank you for the feedback. The extend_rememeber_period configuration about extends the period of remember token validity. When you set to true, every time the authenticate user use the application with a valid remember token the expiration date of the token will be extended. When it is set to false, the expiration date will never be updated and it will expires.

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

@ulissesalmeida
Copy link
Contributor

@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?

@bsoule
Copy link

bsoule commented May 7, 2016

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.

@ulissesalmeida
Copy link
Contributor

ulissesalmeida commented May 7, 2016

@bsoule You're right. Feel free to submit a PR with a fix :) . Would be good have a regression test too.

@ulissesalmeida ulissesalmeida reopened this May 7, 2016
ralinc pushed a commit to ralinc/devise that referenced this issue May 10, 2016
The remember_token should not get overwritten when a user is
signing in and a valid token already exists.

Fixes heartcombo#3950.
@ulissesalmeida
Copy link
Contributor

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

ulissesalmeida pushed a commit that referenced this issue May 15, 2016
…4101)

The remember_token should not get overwritten when a user is
signing in and a valid token already exists.

Fixes #3950.
ulissesalmeida pushed a commit that referenced this issue May 15, 2016
…4101)

The remember_token should not get overwritten when a user is
signing in and a valid token already exists.

Fixes #3950.
ulissesalmeida pushed a commit that referenced this issue May 15, 2016
…4101)

The remember_token should not get overwritten when a user is
signing in and a valid token already exists.

Fixes #3950.
ulissesalmeida pushed a commit that referenced this issue May 15, 2016
…4101)

The remember_token should not get overwritten when a user is
signing in and a valid token already exists.

Fixes #3950.
@ulissesalmeida
Copy link
Contributor

ulissesalmeida commented May 15, 2016

I think it's fixed now on 3.5.10, 4.0.3 and 4.1.1. 🙏

ghiculescu added a commit to ghiculescu/devise that referenced this issue Feb 23, 2021
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.
nomis pushed a commit to nomis/devise that referenced this issue Nov 7, 2021
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.
timdiggins pushed a commit to fieldnotescommunities/devise that referenced this issue Dec 19, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

7 participants