Able to revoke single sessions

brynwhyman commented 3 years ago

Overview

It is not possible to invalidate a single session with this module.

Replication steps

I've got a Chromium session and Firefox session.
In Firefox, I force logout my Chromium session through the session manager component.
I refresh Firefox and I'm still logged in
I go in Chromium and I refresh. I'm now logged out
I go back to Firefox and refresh. I'm now logged out in Firefox as well.

This is appears to be due to inheriting an underlying framework configuration default where RememberLoginHash::$logout_across_devices is set to true.

Regardless of what's stored in the above setting, the session manager needs to be capable of revoking single sessions. Logging out (or revoking) one, should not invalidate the others.

ACs

[x] Irrespective of what value is set for $logout_across_devices in framework, when you use session manager to terminate a specific session, any "remember me" token associated to that session must be invalidated.
[ ] Developer documentation captures the relationship between the session manager and framework login hash logic
[x] When you refresh a browser whose session has been invalidated, no other session is affected.
[x] We've mapped out all expected behaviors for all config combinations and session manager installation status.

Notes

This was uncovered in this issue (see comments for details): https://github.com/silverstripe/silverstripe-framework/issues/9794

PRs

emteknetnz commented 3 years ago

Map

Device A - logged in (current browser) Device B - logged in (other browser)

Scenario 1

_$logout_acrossdevices = true session-manager not installed

When I log out of Device A, I am logged out of Device B

Device A - delete RememberLoginHash
Device B - delete RememberLoginHash

Scenario 2

_$logout_acrossdevices = false session-manager not installed

When I log out of Device A, I am still logged in with Device B

Device A - delete RememberLoginHash
Device B - no change

Scenario 3

_$logout_acrossdevices = true session-manager installed

When I log out of Device A I am logged out of Device B

Device A - delete RememberLoginHash, delete LoginSession
Device B - delete RememberLoginHash, delete LoginSession

When I revoke Device B, I am still logged in with Device A (this is currently not working correctly)

Device A - no change
Device B - delete RememberLoginHash, delete LoginSession

Scenario 4

_$logout_acrossdevices = false session-manager installed

When I log out of Device A, I am still logged in with Device B

Device A - delete RememberLoginHash, delete LoginSession
Device B - no change

When I revoke Device B, I am still logged in with Device A

Device A - no change
Device B - delete RememberLoginHash, delete LoginSession

emteknetnz commented 3 years ago

I looked at the docs in session-manager and there's a good point about users without CMS access. Some silverstripe website allow users to log in which will create a record on the Member table. They'll never have access to session manager to revoke sessions.

### Logout across devices

This module respects the `SilverStripe\Security\RememberLoginHash.logout_across_devices` config setting,
which defaults to `true`. This means that the default behaviour is to revoke _all_ a user’s sessions when they
log out.

To change this so that logging out will only revoke the session for that one device, use the following config
setting:

SilverStripe\Security\RememberLoginHash:
  logout_across_devices: false

**Important:** do not set this value to false if users do not have access to the CMS (or a custom UI where
they can revoke sessions). Doing so would make it impossible to a user to revoke a session if they suspect
their device has been compromised.

Means that we should keep logout_across_devices: true as the default in framework, even with session-manager installed

Update: It's worth clarifying that the existing behaviour of logout_across_devices when session-manager is not installed is to only delete the RememberLoginHash record, it will not actively log out the current session. This will prevent future auto-logins from occuring only.

My feeling that the session-manager module shouldn't even consider logout_across_devices.

Instead we should handle this 'auto log out no-admin-access users' differently

emteknetnz commented 3 years ago

The currently behaviour of logging out Device A after revoking Device B happens after you refresh Device B if it was revoked. This is a really odd behaviour. The logout happens via session-manager/LogOutAuthenticationhandler::logout()

I think we'll need to move some more or all of the logout logic in LogOutAuthenticationhandler::logout() to somewhere a bit closer to the LoginSessionController

emteknetnz commented 3 years ago

I've got a PR up that changes the 'destroy all the login session on logout' functionality from testing if logout_across_devices is true to now "checking if the member has no admin access"

logout_across_devices is now totally decoupled from session-manager

emteknetnz commented 3 years ago

I've split off the non-admin users scenario off to its own issues and pull-request

I've created a new PR for this issue that ignores the 'non-admin users' scenario.

Again, I've removed any reference to logout_accross_devices because I just think it shouldn't be here. I've tested that locally RememberLoginHashes are still removed correctly for Members who ticked the remember me checkboxes - that logic is all handled within framework when a user is logged out by any means, including being revoked via session-manager

emteknetnz commented 3 years ago

Closing as linked PR has been merged

Doc update is being handled on the yml change issue https://github.com/silverstripe/silverstripe-session-manager/issues/65

silverstripe / silverstripe-session-manager