[10.x] Use AuthenticateSession on Sanctum requests

[patrickomeara]

This PR allows auth.session to be used after auth:sanctum to logout other session on password change.

It is a better approach to my existing sanctum PR where I attempted to solve the issue by introducing a stripped-down version of AuthenticateSession. This PR has a better DX as it keeps the auth.session implementation that developers already know.

The AuthenticateSession middleware makes it possible to invalidate sessions on other devices https://laravel.com/docs/10.x/authentication#invalidating-sessions-on-other-devices

The problem

If a user is logged in in two different browsers, and change their password in one, the web guard is invalidated in the other, but the sanctum guard is not and requests can still be sent to the API.

If a bad actor had access to a web session, they still have access to the sanctum guarded API after a password update.

Currently, using auth.session on Sanctum frontend requests will throw two fatal errors as RequestGuard doesn't have viaRemember or logoutCurrentDevice methods. These have been added to the GuardHelpers trait with sensible defaults for both framework and custom guards.

Password Hash

This PR also reverts a change where the password hash was stored per guard. #33876

I can't see an advantage of storing the password hash per guard as the same user is authenticated across guards even in multi guard auth. If a request changes the user's password the password hash should update for all guards in the session, not just the guard that changed it. Additionally, the author of that PR has been deleted from github. I'd be grateful if anyone can shed some light on this change and the issue it was addressing.

Test Repo

As this is a difficult scenario to test I have created a test repo.

The main branch of the test repo shows the issue, where the user can still make a sanctum request after a password change in another session. The web guard has been logged out here, but not the sanctum guard.

Screen.Recording.2023-08-14.at.11.45.12.mov

The auth.session branch of the test repo uses this PR branch of laravel/framework to show the request now returns a 401 and has logged the user out of both web and sanctum guards.

Screen.Recording.2023-08-14.at.11.58.19.mov

The difference between the branches can be viewed here.

Dusk/Jetstream

If this PR is accepted Dusk and Jetstream will need a small change to the logout mechanisms to remove the guard suffix.

https://github.com/laravel/dusk/blob/1040b15b78ed829ba1166ef10757385b0416ae8e/src/Http/Controllers/UserController.php#L63

https://github.com/search?q=org%3Alaravel+password_hash_+repo%3Alaravel%2Fjetstream&type=code

Backward Compatibility

This is a breaking change if developers are interacting with password_hash_<guard> in the application layer. In this case, we could iterate over the guards and set password_hash_<guard> for each one, and leave the password_hash change to 11.x. The downfall with this approach is: unless we can collect all of the guards the app uses, not just the request, we end up in the same situation.

The AuthenticateSession middleware adds password_hash if it is not currently present before the request proceeds. if the application layer does not interact withpassword_hash_<guard> this change will not break existing functionality.

Any existing sanctum guarded sessions will still be valid after the auth.session middleware is added as the first request will set it. However, all subsequent requests after a password change will be logged out. This will still be an issue if this change were to target 11.x.

It should be noted that Laravel 7.x used the key password_hash if a session is still in use from 7.x updating to this will log sessions out that have had a password change since the 7.x request was made. Changing the key to something new will alleviate this edge case ie hashed_password

[taylorotwell]

Surely this removes existing functionality where the passwords were maintained separately for each guard?

[patrickomeara]

Maintaining passwords for each guard is one of the reasons the current AuthenticateSession middleware can't be used on sanctum requests. It will log out other guards in the same session.

If it were always the config('auth.defaults.guard') being used there would be no issue, but sanctum requests swap the default guard via shouldUse

framework/src/Illuminate/Auth/AuthManager.php

Lines 191 to 225 in c1e6281

	/**
	* Get the default authentication driver name.
	*
	* @return string
	*/
	public function getDefaultDriver()
	{
	return $this->app['config']['auth.defaults.guard'];
	}
	/**
	* Set the default guard driver the factory should serve.
	*
	* @param string $name
	* @return void
	*/
	public function shouldUse($name)
	{
	$name = $name ?: $this->getDefaultDriver();
	$this->setDefaultDriver($name);
	$this->userResolver = fn ($name = null) => $this->guard($name)->user();
	}
	/**
	* Set the default authentication driver name.
	*
	* @param string $name
	* @return void
	*/
	public function setDefaultDriver($name)
	{
	$this->app['config']['auth.defaults.guard'] = $name;
	}

This creates a mismatch when updating the password, as the new password hash is only stored for the guard that changed it, the others are logged out (in the same session)

[taylorotwell]

But I guess what I'm saying is surely we added that for a reason and just removing it would break applications that rely on it? Maybe there is a better way to solve this?

[donnysim]

I think it's because you can have multiple user tables, some separate them as admins, users and this would break it as changing user or admin password would invalidate others. BUT, I know there also are situations where you separate the sessions but use the same users table (with rare additions such as user_profiles table etc.) and same password where existing logic makes less sense as the password is shared and should invalidate both sessions. Maybe instead of a driver name it should target user provider name, but this definitely should not go into 10.x.. That's my theoretical guess.

[crynobone]

Hi @patrickomeara

Can you make the test repository available for further debug and tests: https://github.com/patrickomeara/sanctum-logout-issue

[patrickomeara]

@crynobone Sorry I thought that was public already.

@taylorotwell I had the same thought when I first started working on this. I initially tried to iterate over all of the guards and update the password hash, which is essentially not storing it per guard (but does give some BC, as mentioned in my first comment). The original PR doesn't give much info away.

The storing of the hashed password is only for stateful requests in the same session. The problem is changing the password only updates the guard that changed it. But the desired outcome is to keep all guards in the session logged in, and log out other sessions.

I've targeted 10.x only for security reasons. This can be changed.

[crynobone]

After looking at the structure I do think the solution isn't the best:

• This only solved Sanctum where sanctum.guard resolved to web or equivalent password-based Stateful Guard. It doesn't make any solution for Sanctum using API tokens etc.
• Removing 'password_hash_'.$this->auth->getDefaultDriver() would break auth.session on projects with multiple logins and ability to impersonate between guards (members vs admins tables etc)

I believe AuthenticateSession is needed for stateless requests such as Sanctum. We need a separate middleware to handle such requirements without relying on sessions.

[patrickomeara]

@crynobone Thanks for looking into this!

The first approach I took introduced a sanctum only middleware. https://github.com/laravel/sanctum/pull/461/files

Let me know if I can help further.

[taylorotwell]

@patrickomeara yeah - I agree your original pull request was more on the right track.

[taylorotwell]

Closing this because at minimum we need to go back to the other PR. This one is breaking.

[patrickomeara]

I'll circle back on this when I get a chance.

[crynobone]

@patrickomeara Can you check and verify laravel/sanctum#467