Silverstripe-framework: UX weakness with "remember me" leading to unnecessary forgotten logins

[sminnee]

There are a couple of issues with the "remember me" function that lead to it giving the appearance of being unreliable. This probably has flow-on issues such as resistance to rolling out MFA as it relies on this function for a good UX.

In my view, the first scenario has the higher impact, but that's a bit subjective.

Scenario 1

Steps:

• Log in from Safari, tick remember me
• Close and restart Safari, verify that login has been remembered
• Log in from Chrome, do not tick remember me
• Close and restart Safari

Expected:

• Login has been remembered

Actual

• Login on Safari has been forgotten

Explanation

I believe that when the "remember me" box is not checked, it is clearing out any ALC token from all devices. It would make sense to clear out the ALC from _this_ device, but not other devices.

From my perspective, this is a bug, without any security implications.

Scenario 2

Steps:

• Log in from Safari, tick remember me
• Close and restart Safari, verify that login has been remembered
• Log in from Chrome, tick remember me
• Close and restart Safari, verify that login has been remembered
• Explicitly log out from Chrome
• Reload Safari, verify that it hasn't been logged out
• Close and restart Safari

Expected:

• Login has been remembered

Actual

• Login on Safari has been forgotten

Explanation

The "log out" functionality is hard-coded to perform "log out from all devices". This has some security benefit but doesn't make a lot of sense as a default.

From my perspective, this is a bug, but it _does_ have some security implications. Ideally, the log out action (at least in the CMS) would pop up a box with a "log out from all devices" checkbox, defaulting to unchecked.

ACs

• [ ] A technical risk assessment is performed assuming the below is implemented
• [x] Neglecting to check the remember me checkbox in one browser does not remove settings for other browsers
• [ ] By default, logging out of one browser session does not invalidate other browser sessions (currently managed in the true/false configuration value)
• [x] Review existing documentation and any changes required based on this fix
• [x] Changelog notes this as a bug, not a feature change

Update on ACs

• We choose to go with a 'do mostly nothing' approach so in the end there was no need for a technical risk assessment. We choose not to change the default behaviour of logging out of one browser invalidating other sessions (that has check the remember me checkbox)

Notes

• This fix should be tested with the silverstripe/session-manager module - we'll cover that in a separate issues

Pull request

• [x] #9895

[clarkepaul]

Regarding scenario 2, one of the options we are investigating is device management in the users profile. The "log out from all devices" is a good solution that I'll add to our list of possibilities, it doesn't let the user know which devices or how many devices have tokens.

[sminnee]

Regarding scenario 2, one of the options we are investigating is device management in the users profile.

Yeah that would be a more complete solution, I agree.

[brynwhyman]

I'm anticipating that both of these scenarios would be resolved with the inclusion of the following module, which we are enhancing to be the baseline for device/ session management within the CMS UI. I'll do some testing this afternoon to confirm.

• https://github.com/silverstripe/silverstripe-session-manager

That might still leave a gap where these bugs are present in framework, but we've also talked about making the session-manager module a requirement of the security-extensions module and making that a requirement of core.

[emteknetnz]

Code which is probably leading to everyone getting logged out:

• https://github.com/silverstripe/silverstripe-framework/blob/4/src/Security/MemberAuthenticator/CookieAuthenticationHandler.php#L251
• https://github.com/silverstripe/silverstripe-framework/blob/4/src/Security/RememberLoginHash.php#L183
• https://github.com/silverstripe/silverstripe-framework/blob/4/src/Security/RememberLoginHash.php#L55 << default config value of true

Easy solution to this is probably to just change the config value of logout_across_devices from true to false

[maxime-rainville]

Replicating

I tried replicating both scenario with Chromium and Edge:

• Could NOT replicate scenario 1.
• Replicated scenario 2

[maxime-rainville]

How the "Remember me" feature works

Key classes

• CookieAuthenticationHandler handles the authentication of the Member and the creation of cookies on the user's device.
• RememberLoginHash is a DataObject that link up the DeviceID, the Remember me Hash and the Member. When the user come back without a session, CookieAuthenticationHandler links back those 3 piece of data to let the user back in.

The "Remember me" workflows

New login

This behaviour is triggered when a user logins and checks the "remember me" option.

• CookieAuthenticationHandler::logIn() is called
• We check if there's any pre-existing device cookie for this request and clear any matching RememberLoginHash. *
• If the user has requested a "persistent" session:
  
  
  
  • RememberLoginHash::generate() is called for the provided $member. This automatically generates a device ID (sha1 hash) and a member token (sha1 hash, stored encrypted with the Member's Salt). An expiry date is also set. *
  
  
  • If RememberLoginHash::force_single_token' is set to true, any otherRememberLoginHash` for this user well be deleted (default is false)
  
  
  • Two cookies are set:
  
  
  • The token cookie which contains the member ID and the RememberLoginHash token UNencryted (TTL defined by RememberLoginHash::$token_expiry_days, defaults to 90 days)
  
  
  • The device cookie which contains the RememberLoginHash device ID (TTL defined by RememberLoginHash::$device_expiry_days, defaults to 365 days)
  
  

* You can't have multiple "remember me" session on the same device
* Encrypting the member hash with the Member's Salt will invalidate the "remember me" hash when the user changes their password. Encryption is done via $member->encryptWithUserSettings().
** These 2 cookies are unrelated to PHPSESSID which contains the ID for the current session and is only valid for the duration of the browser session.

Starting a new session

This workflow is triggered when a user returns to the site after having logged in with the "remember me" option, but having close their browser, therefore invalidating their session.

• CookieAuthenticationHandler::authenticateRequest() is called with the HTTPRequest
• We try reading a token cookie and a device cookie. If any of those check failed, we return null:
  
  
  
  • Both cookies are provided
  
  
  • We try fetching an existing user matching the member ID in the Token cookie.
  
  
  • We use the Member's Salt to hash the unencrypted token in the cookie.
  
  
  • We look for a RememberLoginHash matching the hash, the device id cookie and the member ID
  
  
  • We check that the RememberLoginHash is not expired.
  
  
• We call the logIn function on the Identity store provided by CookieAuthenticationHandler::CascadeInTo
• We call RememberLoginHash::renew()
  
  
  
  • This generates a new token for the member. Again, that token is hashed with the member's Salt.
  
  
  • The expiry date and device ID remain the same.
  
  
  • RememberLoginHash is written to the DB with the new token hash *
  
  
• We set a new Token Cookie in the user's browser with the new unencrypted token. The TTL for this cookie is reset to RememberLoginHash::$token_expiry_days.

* The purpose of this step is to make sure you can't reuse the same token cookie multiple times
** The cookie time TTL seemed a bit weird here. The token cookie TTL gets extended after each new session, so it will live past the RememberLoginHash expiry date. The device ID cookie never gets extended, but its TTL is 365 days vs RememberLoginHash's 90 days, so this cookie will also outlive RememberLoginHash. The key thing, is that the expiry date on RememberLoginHash never changes. So no matter what, it will eventually expire even if the cookies are still in the user's browser.

Logout

• CookieAuthenticationHandler::logIn() is called.
• We try to find the current device ID cookie
• We call RememberLoginHash::clear() with the current member and the device ID.
  
  
  
  • If RememberLoginHash::$logout_across_devices is false and a device ID was provided *
  
  
  • We delete all RememberLoginHash object for the provided member and device ID.
  
  
  • Otherwise,
  
  
  • We delete all RememberLoginHash object the current member
  
  
• We clear any token cookieor device ID cookie in the user browser.

* I didn't try this, but I get the impression this logic is buggy. If logout_across_devices is false and you try to logout from a non-persistent device, you won't have any device ID cookie, which will cause all your RememberLoginHash to be deleted.

[maxime-rainville]

@clarkepaul Could use some feedback on this.

Possible approach to fix this

Minimal fix

We could:

• Change the default value RememberLoginHash::$logout_across_devices to false
• Fix that ambiguity in RememberLoginHash::clear() so that nothing happens if no device ID is provided.

In that scenario, there's no way to invalidate a "Remember me" cookie on a lost device, short of changing your password.

Minimal fix + add a UI switch to "log out across all devices"

Basically this would give the option to user to explicitly clear all their RememberLoginHash objects

Minimal fix + give users a Gridfield to manage their RememberLoginHash

That's probably a very bad idea. It would allow users to delete specific remembered device. However, the only identifier we have for those device is a very unfriendly sha1 hash.

Plus it would duplicate a the logic currently being implemented in session manager.

Do mostly nothing

If we're serious about making session-manager a supported module and shipping it in our default recipe, then maybe doing nothing here is the best approach long term.

Session manager sounds like a better place to have a "logout everywhere" feature. It would also allow you to invalidate individual session directly.

So we could do this:

• Fix that ambiguity in RememberLoginHash::clear()
• Keep the current RememberLoginHash::$logout_across_devices default
• Recommend that people who are unhappy with the current behaviour:
  
  
  
  • Set RememberLoginHash::$logout_across_devices to false OR
  
  
  • Install session manager
  
  

[brynwhyman]

My feeling here is we're investing in improving the session-manager module with the goal of improving user and site security of the CMS. The best way to make that become a reality is to make that module as easy as possible to adopt.

We're still to confirm how the session-manager work will be released, but I'd like to see it go into the CMS core recipe so adoption is as pain-free as possible and requires one of:

• composer update
• read the CMS recipe changelogs and (if not using recipes) see the recommendations to install the module separately

Given the above, I think we should consider the investment already going into the session-manager module and take the "minimal fix" approach to resolve this issue.

In that scenario, there's no way to invalidate a "Remember me" cookie on a lost device, short of changing your password.

The best answer here is to use the session management feature that we've enhanced exactly for that purpose.

[clarkepaul]

If we weren't investing in the session manager already then I'd be all for the minimal fix. With the session manager module we have less need for the logout of all devices—not the most important addition unless its needed technically but adds a little value.

+1 for seeing this in core via the session manager module as it will provide the most benefit to the most people, makes it easier writing guides and better usability all round.

Do we have any idea how this might affect projects upgrading using the likes of SSO or identity managers? Is it worth checking in with a few projects to see howe this might affect them just so we have it in mind?

[brynwhyman]

Do we have any idea how this might affect projects upgrading using the likes of SSO or identity managers?

No confident idea at this stage. We've got a dedicated issue to run through test scenarios with SSO in place: Compatibility with SSO modules.

With the session manager module we have less need for the logout of all devices—not the most important addition unless its needed technically but adds a little value.

Totally open to raising a separate issue to discuss including a 'sign out of all devices' feature if you like that idea @clarkepaul? I just think it should be handled as an additional and not be relied on to close this issue :P

[clarkepaul]

@brynwhyman nope I don't think we need a "sign out of all" thing, as you were :)

[maxime-rainville]

It seems like the "Do mostly nothing" solution is the favourite. I'll confirm that setting RememberLoginHash::$logout_across_devices to false behave as expected and I'll review the existing doc.

My guess is this makes most of the ACs moot since we're going to keep the existing default.

[maxime-rainville]

Just clarifying that "Do mostly nothing" solution involves keeping the current default for RememberLoginHash::$logout_across_devices and the current behaviour.

The rational for this is mostly one around comms in the next release. If we change the default now, we'll have to explain to developers upgrading the ins and oust of the new behaviour. Then in the next release, we'll probably ship session-manager in core, which will again change the logout behaviour. I don't think there's much value in changing the default if that default is only going to matter for one release cycle.

Also the current default, is arguably more secure since it invalidates all your "remember me" tokens when you logout.

My thinking is this:

• The vanilla install should keep the current default behaviour even if we don't think it's the best one in most cases. You need to set $logout_across_devices to false if you want the better log out behaviour.
• Session manager should implicitly set $logout_across_devices to false. If you logout, only the "Remember Me" token for the current session will be invalidated.
• Irrespective of what value is set for $logout_across_devices, when you use session manager to terminate a specific session, any "remember me" token associated to that session must be invalidated.

[emteknetnz]

Sounds good

[maxime-rainville]

I've created a card to add unit test to the remember me feature: https://github.com/silverstripe/silverstripe-framework/issues/9896

[chillu]

Agree with Max and Bryn (and the "Do mostly nothing" option): We're fixing this properly with session-manager, and we don't have enough capacity to also fix anything in core without session-manager.

In terms of making this part of the core recipe, I think we need to have a broader discussion about what that means for support expectations from Silverstripe Ltd. That's already on Bryn's radar.

[emteknetnz]

Linked PR has been merged