Add configurable support for persisting a single remember me token across multiple browser logins

[eigenben]

This addresses functionality discussed in issue #190.

Previously (and still by default after this commit), logging in to one browser would effectively invalidate the remember me token stored as a cookie in all other browsers the user may have logged in with. This in practice made it difficult to support remembering logins for users with
different devices (desktop & tablet).

This change adds a new configuration option remember_me_token_persist_globally (default: false) which, if set to true, will maintain the first remember me token used for all subsequent
cookies.

Rails.application.config.sorcery.configure do |config|
  config.user_config do |user|
    user.remember_me_token_persist_globally = true  # Make remembered logins work across browsers
  end
end

This causes the remember me module to:

1. Skip overwriting the token when logging in, in Model#remember_me!
2. Skip clearing the token when logging out, in Model#forget_me!

Enabling this option has potential security ramifications, especially on websites not using SSL, a single token value being long-lived will increase the risk of session fixation attacks. Since even logging out does not clear the token (since other browsers may still be relying on it), there is also no direct way to invalidate a user's remembered login (short of manually setting the token to NULL).

As such, anyone enabling this option should understand and accept this security/functionality trade-off (and ideally use SSL across-the-board).

A few notes about this implementation for consideration:

• As discussed in issue "Remember Me" functionality doesn't work when users access my app from 2 separate browsers #190, a slightly more secure approach might involve maintaining individual remember me tokens for each login, allowing fine-grained control over which remembered sessions were still valid. But doing that means the implementing application must persist this model, which gets complicated fast.
• I have struggled to name this configuration option (remember_me_token_persist_globally) - it might deserve renaming for more clarity.
• Technically since there are two different things this option does, they could be separated out for more fine-grained control of what happens. For example, some folks may want to not overwrite the login token when logging in, but when logging out invalidate all of them. With this single option, its an all-or-nothing affair, so another approach might be separating this out into two separate options: remember_me_token_reset_on_login and remember_me_token_clear_on_logout. Thus you could set the former to false and the latter to true to still allow logging out clobbering all remembered logins for that user. I opted against this in the interest of simplicity, and figuring most users would want both.

[mokolabs]

Here's a suggestion for a better name for user.remember_me_token_persist_globally:

What about user.remember_me_on_multiple_browsers?

[arnvald]

Hi @rubiety,

thanks a lot for your contribution! I like the idea, I was thinking about it for some time, and I like the approach, where you leave the current situation as default, and add the feature as optional.

I'll think about the name for this setting. @mokolabs suggestion sounds good, although it does not suggest that logout will not clear all sessions. It would be useful to add this option to initializer template with proper comment.

I'll review code in detail later this week and I'll provide some comments on it. For now I think there are 2 things to add: comment in initializer template and method like logout_all_sessions that would force to recreate token on logout.
Would you be willing to add these 2 things to your PR?

I think this additional method is important, because it gives users protection - if they know they left session open at some public computer, they can invalidate it. Of course not all developers will add the option to invalidate all sessions, but if it's as easy as running logout_all_sessions instead of just logout, I'm pretty sure more of them will do.
Also, it would solve the problem you mentioned - if someone does not want to recreate token or login, but wants to invalidate all sessions on logout, they can just use another provided method.

[eigenben]

I've added to the initializer template, but implementing a logout_all_sessions isn't quite so easy due to the loose coupling through before/after callbacks, and may involve some restructuring of this implementation (perhaps deserved anyways).

Currently logout at the controller level eventually invokes the remember me User#forget_me! method through a Config.before_logout, the remember me module couples to this by just adding itself to this queue with Config.before_logout << :forget_me!.

Implementing a new logout_all_sessions would require some kind of similar coupling to allow the remember me module to hook specifically to force forget the token, we can't really just fire solely new callbacks like before_logout_all_sessions, because that then inconsistently changes the API to the user: the user may then have to register whatever callback they are currently using for logout in two places: before_logout and before_logout_all_sessions.

So the only real way to do that is to call both and then have remember me do Config.before_logout_all_sessions << :force_forget_me!. Yet that means it's also technically calling forget_me! as well (though that should be innocuous). In other words, the layers are too detached for there to be super clean way to implement this without architectural refactoring.

There is also some potential confusion in having this logout_all_sessions method around where it's only really useful/applicable if someone has set the the remember_me_token_persist_globally option to true, and so communicating why this method is there even in the README is somewhat difficult.

Despite these potential reservations, I have pushed a commit that attempts to implement this, but I'm not particularly happy with the implementation, for the reasons discussed above. It exposes new queues before_logout_all_sessions and after_logout_all_sessions which can be specifically hooked into by remember me, adding Config.before_logout_all_sessions << :force_forget_me! in that case, and a new logout_all_sessions method that basically invokes both those callbacks and the normal before_logout/after_logout ones.

Note that one option to help simplify this implementation is to just expose force_forget_me! (ideally renamed to something better) through the remember me module and not logout_all_sessions in the core sorcery API, which exists awkwardly alongside logout and requires those separate queues for coupling. As such, the user would have to call force_forget_me! from the controller upon logging out if they really did want to clobber the remember me token.

[arnvald]

@rubiety thanks for tackling this problem and for detailed summary. I agree that the approach with separate callbacks is too complex. Also, I should add changing the controller callbacks architecture to my TODO list for version 1.0.

How about an implementation where logout_all_sessions would be defined in controller/submodules/remember_me.rb as:

def logout_all_sessions
  if logged_in?
    force_forget_me!
    logout
  end
end

I need to check it locally, if it won't cause any issues.

I've checked your first 2 commits as well, nothing to add there, if we find a way to add logout_all_sessions easily I'll merge the whole PR, if not, I'll just cherry-pick them later this week 👍

[eigenben]

That makes sense, but it also means the logout_all_sessions method is only available with the remember me module. I've simplified the implementation in my latest commit, and moved documentation in the README of that method to the remember me section.

[arnvald]

Yes, I'm aware it will be available only if the module is included, but at this point it makes sense - since without remember_me module it works exactly as logout.

There is, however, one thing that bothers me (which is my fault, as I came up with the name). logout_all_sessions name is quite confusing, because people who are currently log in, still remain logged in. It invalidates cookies, but it does not really log out all the sessions.

I'll think for a while about a better name, and if I can't of anything less confusing, I'll leave it this way for now. I need to add option to invalidate all the sessions anyway, so that would be good place to start working on it.

@rubiety thanks once again for your work! If you have any idea how to rename logout_all_sessions, let me know. @mokolabs maybe you have some idea?

[eigenben]

About the only thing I can suggest is removing the logout_all_sessions method entirely, as the semantics are too nuanced and difficult to explain. Users of the app can continue using logout, and can additionally car force_forget_me! in their controllers if they really want to forget all remember me sessions.

Aside from your valid critique of the name, I cringe a bit at a name so generic being stuffed in a specific module. logout_all_sessions does not on the face of it seem to have anything to do with remember me.

[arnvald]

I agree, let's just leave force_forget_me! and we're ready to merge 👍

[mokolabs]

Sweet!

[eigenben]

Okay, this has been done and we're back to just a force_forget_me! method that would have to be called in conjunction with logout.

[groe]

Nice, finally a solution to this!
This hasn't been released at rubygems yet, has it?

[hendricius]

Thanks a bunch. This was like a Heisenbug to us 👯