Skip to content
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

Able to revoke single sessions #46

Closed
5 of 6 tasks
brynwhyman opened this issue Mar 30, 2021 · 6 comments
Closed
5 of 6 tasks

Able to revoke single sessions #46

brynwhyman opened this issue Mar 30, 2021 · 6 comments
Assignees

Comments

@brynwhyman
Copy link

brynwhyman commented Mar 30, 2021

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

  • 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
  • When you refresh a browser whose session has been invalidated, no other session is affected.
  • We've mapped out all expected behaviors for all config combinations and session manager installation status.

Notes

Related

PRs

@emteknetnz emteknetnz self-assigned this Apr 9, 2021
@emteknetnz
Copy link
Member

emteknetnz commented Apr 9, 2021

Map

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

Scenario 1

$logout_across_devices = 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_across_devices = 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_across_devices = 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_across_devices = 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
Copy link
Member

emteknetnz commented Apr 9, 2021

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
Copy link
Member

emteknetnz commented Apr 9, 2021

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
Copy link
Member

emteknetnz commented Apr 12, 2021

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

This was referenced Apr 13, 2021
@emteknetnz
Copy link
Member

emteknetnz commented Apr 13, 2021

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
Copy link
Member

Closing as linked PR has been merged

Doc update is being handled on the yml change issue #65

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants