search

silverstripe / silverstripe-session-manager

Allow users to manage and revoke access to multiple login sessions across devices.
BSD 3-Clause "New" or "Revised" License
9 stars 7 forks source link

Non perminent session appear individually #64

Closed maxime-rainville closed 3 years ago

maxime-rainville commented 3 years ago

Steps to reproduce

Expected result: I only see one chrome and one firefox session Actual result: I see two chrome sessions and one firefox session

https://youtu.be/n_kIXSlZ4dE

What's the problem

Basically, session manager doesn't have a way of linking non-permanent sessions on the same device. So when you close and reopen your browser, you get a new session each time. But session manager doesn't have a way of knowing that this is the same device as the old session.

Login::getCurrentSessions() will show non-permanent sessions if they have been active within the last hour. So it's unlikely you'll get into a situation where you have 5 entries for the same device ... unless you are opening and closing your browser like crazy.

Note

maxime-rainville commented 3 years ago

@silverstripeux @brynwhyman ^

brynwhyman commented 3 years ago

Do we rely on the garbage collection task to remove these entries?

maxime-rainville commented 3 years ago

Looking at the Garbage collection service, it relies on LoginSession::$default_session_lifetime to decide if your session is inactive. That's set to an hour by default.

If you don't install/configure the queued job, you'll get this problem. If you restart your computer/browser you'll get this problem as well.

Looks like this bit of code is trying to mitigate this problem by returning only session that our persistent or within the default_session_lifetime. I'm not 100% sure it's working thought, because I seem to be getting session carry over from day to day. https://github.com/silverstripe/silverstripe-session-manager/blob/f70cb2c7a8ae9159e2c185414ee1ca717cdd130c/src/Model/LoginSession.php#L282-L291

brynwhyman commented 3 years ago

Wait, I'm guessing that the garbage collection task should be deleting records for already expired sessions, and not be responsible for actually expiring the sessions. Is that right?

The readme covers this in a bit more detail too: https://github.com/silverstripe/silverstripe-session-manager#developer-details

On each request, a middleware (LoginSessionMiddleware) checks if the current PHP session is pointing to a valid LoginSession record. If a valid record is found, it will update the LastAccessed date. Otherwise, it will force a logout, destroying the PHP session.

A periodic process (GarbageCollectionService) cleans up expired LoginSession records. Due to the way PHP sessions operate, it can not expire those sessions as well. The PHP sessions will be invalidated on next request through LoginSessionMiddleware, unless they expire independently beforehand (through PHP's own session expiry logic).

Silverstripe allows persisting login state via a "remember me" feature. These RememberLoginHash records have their own expiry date. This module associates them to LoginSession records, and ensures their expiry is consistent with the new session behaviour (see "Configuration" below for details).

More generally speaking, I think being able to link non-permanent sessions on the same device to one record would be ideal. Especially because this component is centred around 'Devices', not 'Sessions'. I'm not sure how far away that would be from what the module is currently doing though... What do you think @maxime-rainville ?

clarkepaul commented 3 years ago

Thats going to cause concern for users when they see so many sessions active when a single device is used (probably worse than not showing any info at all). This is going to be something we really need to sort for this piece to be successful.

If we can't resolve technically (which I hope we can) I'm wondering if only displaying permanent sessions in the area we have designed makes sense, the other sessions are displayed as "session history" differently (e.g. hidden list).

maxime-rainville commented 3 years ago

I've confirmed that Login::getCurrentSessions() will only show non-permanent sessions if they have been active within the last hour. So it's unlikely you'll get into a situation where you have 5 entries for the same device ... unless you are opening and closing your browser like crazy.

But you might occasionally get 2 for a short while.

clarkepaul commented 3 years ago

Okay so not as big of an issue then, if it's of concern you can still force log out of sessions. Think it would still confuse people if they tested the functionality and raise a few eyebrows.

maxime-rainville commented 3 years ago

One way we could solve this is to have a device ID for all sessions, even the none-permanent ones. When you come back after closing your browser, we don't let you back in but we can invalidate other sessions with the same device ID.

That's probably not a big change, but it is a somewhat intricate. So it wouldn't be trivial.

brynwhyman commented 3 years ago

I've confirmed that Login::getCurrentSessions() will only show non-permanent sessions if they have been active within the last hour. So it's unlikely you'll get into a situation where you have 5 entries for the same device ... unless you are opening and closing your browser like crazy.

Thanks for checking this again @maxime-rainville. I've updated the description to note this.

Agree that even if it's less likely, it could still be confusing to the user.

brynwhyman commented 3 years ago

I've just come across this on GitHub, so it's not abnormal. One notable difference with their view is they title the information as 'Sessions', not 'Devices'.

Screenshot:

image

emteknetnz commented 3 years ago

Funnily enough the say both - "list of devices" then in the next sentence "revoke any sessions" - heh :-)

Agree that 'sessions' is the safer word and more technically correct term to here rather that 'devices'. I think we could probably just make this text change and avoid having to do an overhaul on the internals of session manager

brynwhyman commented 3 years ago

I think we could probably just make this text change and avoid having to do an overhaul on the internals of session manager

Agreed. I talked to @clarkepaul and he liked the UI idea. I've spun up a separate issue to talk about that...

Back to this issue though. @maxime-rainville I can't recreate your issue using just Chrome (I don't have access to Firefox right now, but I don't expect using that as well would make a difference?). @bergice reported that he couldn't recreate following your steps either. For me, my session will stay active on exiting and reopening Chrome (without selecting the 'remember me' option).

maxime-rainville commented 3 years ago

I use Chrome on Ubuntu to detect the issue: https://www.youtube.com/watch?v=n_kIXSlZ4dE.

I think MacOS might not technically close your browser when you close it ... it keeps it running in the background so it's faster to access the application again later on ... that probably keeps the session alive. Not sure what Windows will do in this situation. If you reboot your OS or if you force quite Chrome on MacOS, you should see the behaviour as well.

In any case, given my understanding of how session cookies work, there's no way our current implementation wouldn't have this bug. If it doesn't then we have a much bigger problem on our hands.

brynwhyman commented 3 years ago

While testing the module I had another look at this issue. Seems like in my previous case the browser is just being smart.

Both Chrome and Safari store session cookie information even after a force quit in at least MacOS - that's using the default OS user profiles so it doesn't really require any extra set up from the user's end - I have a feeling most people would find themselves in this position. IT administrators obviously might have some more control over this in a workplace setting.

Given the above, I can do this:

  1. Open Chrome, Incognito Chrome, and Safari in MacOS
  2. Log into Chrome, Incognito Chrome, and Firefox without checking the "Remember me" checkbox
  3. Look at your browser sessions in Chrome ... you'll see three sessions for each browser
  4. Force quite Chrome and Safari. You'll be logged out of the Incognito window because the session was impermanent
  5. Open Chrome again, you're session is retained, so access the CMS Profile section

Expected result: You see two sessions (one for Safari and one for Chrome) Actual result: You see three sessions (the incognito session remains when ideally it wouldn't).

Given the above, this feels like a low impact issue to me and not high on the list to resolve before the beta tag.

clarkepaul commented 3 years ago

I don't see us getting around the issue of the incognito session showing up after the browser is closed. It's kind of expected if they haven't signed out, at least if they are concerned they can manually kill the session now.

dhensby commented 3 years ago

To be honest, without user fingerprinting (which has privacy implications) or some other permanent cookie to identify a "device" (and thus de-dupe sessions), this isn't going to be possible to solve and I would say the current behaviour is expected.

michalkleiner commented 3 years ago

~Websockets to replace the ping mechanism and remove the session when it's not active?~ Sorry, jumped the gun here.

maxime-rainville commented 3 years ago

@brynwhyman I don't think the incognito session showing is a problem ... Incognito will blast whatever cookie gets created each time, so there's absolutely nothing we can do about that anyway.

The main problem I would see is someone rebooting their computer and seeing another session another session in there list.

brynwhyman commented 3 years ago

Closing this issue. While a little confusing, the behaviour is expected.