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

[6.0] Split out logoutAllDevices method #29376

Closed
wants to merge 2 commits into from

Conversation

dwightwatson
Copy link
Contributor

This PR resolves the issue described in #29244 where logging out on one device will actually log you out on all devices. This seems like unexpected behaviour to me - most sites wouldn't log you out everywhere like this.

This changes the logout method to not cycle the remember token, and adds another method logoutAllDevices (which sits alongside logoutOtherDevices) so that developers can opt-in to this behaviour if that is what they are after.

In the linked issue Dries suggested adding logoutCurrentDevice instead - I'm happy to make that alternative PR if that's what you guys prefer - but I did want to suggest initially that I think this should be the default behaviour of logout.

@crynobone
Copy link
Member

I would prefer to have logoutCurrentDevice() as the new feature, where else logout() should remains the same.

  • logout() - de-authenticate users on all devices
  • logoutCurrentDevice() - de-authenticate user only on current devices.
  • logoutOtherDevices() - reset password and logout user from other devices for security purpose.

@dwightwatson
Copy link
Contributor Author

Happy to make that change if it's definitely the preference.

Out of curiousity why is it that you want users logged out from all devices by default? When I log out from GitHub or Facebook on one computer it doesn't happen on my other devices, and if it did it would be annoying.

@antonkomarev
Copy link
Contributor

antonkomarev commented Aug 2, 2019

I'd prefer to go with:

  • logout() - de-authenticate user only on current device.
  • logoutAllDevices() - de-authenticate users on all devices.
  • logoutOtherDevices() - reset password and logout user from other devices for security purpose.

But @crynobone concept is very good too. When you call logout you say that you want to destroy this session on all devices. It depends on which side to look at it.

@crynobone
Copy link
Member

crynobone commented Aug 2, 2019

Out of curiousity why is it that you want users logged out from all devices by default?

It is a structure that Laravel is been build on, Laravel doesn't keep track where all devices are currently logged on from, unliked GitHub or Facebook.

Screen Shot 2019-08-02 at 9 48 08 AM

Screen Shot 2019-08-02 at 9 49 36 AM

Therefore, it much easier to control remote de-authentication on devices that you no longer want to have authentication. In Laravel we been depending on remember_token to de-authenticate user until logoutOtherDevices() where introduce.

logoutOtherDevices() was ideally introduced to force de-authenticate other devices while confirming current user password in event where the account might potentially been hacked or the password is no longer safe.

I'm against a BC to logout() because developers that upgrade to 6.0 might not realised the impact of what has been changed and exposing them to scenario when logout() used to safeguard them in the past, where else introduction of logoutCurrentDevice() is more explicit.

@dwightwatson
Copy link
Contributor Author

Perhaps GitHub/Facebook were poor examples for the point I was trying to make then - take a look at any Wordpress, Ruby on Rails or generic other web app that isn't actually tracking individual logins and I believe they won't log you out everywhere.

I don't believe the common web user would expect that logging out from one device would log them out everywhere, and when it does it's a pretty poor user experience.

@driesvints
Copy link
Member

I'm also in favor of keeping the current logout behavior and introducing a logoutCurrentDevice method instead.

@taylorotwell
Copy link
Member

Has anyone explored the security implications of not rotating the remember me token at all?

@halaei
Copy link
Contributor

halaei commented Aug 2, 2019

I am not sure the current log-out behaviour exactly logs out other devices. It only invalidates the remember-token so they will be eventually logged-out if inactive for 2 hours? It is hard to explain it to users. I don't know what exactly is required to have a real immediate "log-out other devices" feature in Laravel. I think it is already a missing security feature.

My preference is to have these 2 functions:

  • logOut(): only de-authenticate user only on current device.
  • logoutOtherDevices(): by some mechanism "immediately" log-out other devices without any affect on the current session.

An ideal implementation of logoutOtherDevices() maybe is not that easy because it should work under this scenario:

  1. A logged-in user (with a valid remember token) clicks on "Log out other devices" for some security reason.
  2. The server receives this request and process it by applying some changes in database, session and cookies. All other sessions get immediately logged out.
  3. For some reason the response of server gets lost and never reached the client. So the client misses the new cookies/tokens.
  4. The user must still remains logged in after a long period of being inactive.

@taylorotwell
Copy link
Member

I don't have any plans to modify this behavior right now.

@florentpoujol
Copy link
Contributor

florentpoujol commented Aug 2, 2019

Hey

Just for the record (and because I had already written something much longer before the PR was closed ^^), as Halaei explained, the main issue with the current behaviour is that people without remember cookie, but a valid regular cookie/session are just not logged-out at all.

I would see two ways to actually achieve that:

  • loop over all sessions and actually delete all the ones that are linked to the user (except maybe the current one)
  • or put the remember token in the session, and match all 3 (cookie, session, DB) on auth. Cycling the token effectively deprecates all sessions/cookies that have the old one

Anyway, we can easily extend the SessionGard as a custom auth driver, to override logout().
Or even just listen for the Logout event actually, the user instance is not yet saved at that point, so we may retrieve the original token, or even refresh it from the DB directly. (I Haven't tested)

Cheers all 😺

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

Successfully merging this pull request may close these issues.

7 participants