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] Add logoutCurrentDevice method #29397

Merged
merged 1 commit into from
Aug 4, 2019

Conversation

dwightwatson
Copy link
Contributor

Following on from #29376 I appreciate that there is a desire to not adjust the existing behaviour of the logout method. This takes the alternative approach of adding logoutCurrentDevice method - analogous to the existing logoutOtherDevices method - which allows this logout behaviour to be opted-in to if preferable.

I wasn't sure if your "no plans to modify this behaviour" was specific to changing the default functionality or related to changing the authentication guard at all, but put this together just in case it was the former.

Thanks!

@taylorotwell
Copy link
Member

So... if we have logoutCurrentDevice and logoutOtherDevices... what does logout do?

@dwightwatson
Copy link
Contributor Author

Logs out all devices. My personal feeling is that's not an intuitive default but it's a contentious issue.

I think it's worthwhile giving the developers to log users out only on their current device - as that's what happens when you log out from most websites online - and it's generally a better user experience.

For context, some of the stuff I work on is used by students on campus on public computers, and when they log out there it logs them out on their phone and personal computer - which I would prefer to avoid.

@taylorotwell taylorotwell merged commit 6cd0b1b into laravel:master Aug 4, 2019
@adrianorsouza
Copy link
Contributor

After implement logoutCurrentDevice and logoutOtherDevices one little problem arises.

The behaviour is to logout from other devices and keep me logged in the current device as docs says.

So, in order to invoke the method logoutOtherDevices we need to enabled the middleware Illuminate\Session\Middleware\AuthenticateSession, which is commented by default, thus middleware call the Auth::logout() when the user password's does not match, meaning the session was invalidated from other device then the user is logged out.

The problem is that by calling Auth::logout within this middleware will rotate the remember_token again, and from the device which should be kept logged in will then gets logout.

Here is a scenario explained: if Im logged in from my home device with remember me, then Im logged in at Work with remember me too, then from home device I do logout other devices, my current device still logged in, this is the right behaviour at this point. But when open the device at work, so the middleware Illuminate\Session\Middleware\AuthenticateSession will see that the user's password was invalidated then I get logout of the application from that device, however the middleware will call Auth::logout which will then recycle the remember_token. Later on when I get back home and open the browser, guess what?, Im not logged in anymore because the remember_token has changed by work device.

To mitigate this problem, I guess we should call Auth::logoutCurrentDevice from the middleware instead of Auth::logout like this:

class AuthenticateSession
{
    /**
     * Log the user out of the application.
     *
     * @param  \Illuminate\Http\Request  $request
     * @return void
     *
     * @throws \Illuminate\Auth\AuthenticationException
     */
    protected function logout($request)
    {
        $this->auth->logoutCurrentDevice();

        $request->session()->flush();

        throw new AuthenticationException;
    }
}

@driesvints
Copy link
Member

@adrianorsouza thanks. Can you send in a PR?

@adrianorsouza
Copy link
Contributor

@driesvints sure! I'll provide a PR for this.

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.

4 participants