laravel / jetstream

Tailwind scaffolding for the Laravel framework.
https://jetstream.laravel.com
MIT License
3.96k stars 813 forks source link

Auth::login and Auth::loginUsingId causes log out #997

Closed fylzero closed 2 years ago

fylzero commented 2 years ago

Description:

Auth::login and Auth::loginUsingId causes user to be logged out

Steps To Reproduce:

https://github.com/fylzero/bug-report

driesvints commented 2 years ago

Heya, thanks for reporting.

We'll need more info and/or code to debug this further. Can you please create a repository with the command below, commit the code that reproduces the issue as separate commits on the main/master branch and share the repository here? Please make sure that you have the latest version of the Laravel installer in order to run this command. Please also make sure you have both Git & the GitHub CLI tool properly set up.

laravel new bug-report --github="--public"

Please do not amend and create a separate commit with your custom changes. After you've posted the repository, we'll try to reproduce the issue.

Thanks!

driesvints commented 2 years ago

@fylzero is this one resolved as well now?

fylzero commented 2 years ago

@driesvints Yes, this has resolved as well. Thanks again!

fylzero commented 2 years ago

@driesvints Sorry to say, this seems to have resolved in a fresh Laravel app but not in my existing app after updating. Apologies in advance if I am missing something obvious.

Steps I've taken:

fylzero commented 2 years ago

@driesvints I was mistaken. This issue still exists when creating a new project. I'll put together a bug-report repo.

fylzero commented 2 years ago

@driesvints Here is the repo, I've linked in the OP as well.

https://github.com/fylzero/bug-report

Instructions:

relaypilot commented 2 years ago

@fylzero Can you try reverting those changes made in 9.4.1 and see if it resolves the issue? https://github.com/laravel/framework/commit/50b46db563e11ba52a53e3046c23e92878aed395

Line to comment out: https://github.com/404labfr/laravel-impersonate/issues/154#issuecomment-1062986064

fylzero commented 2 years ago

@fylzero Can you try reverting those changes made in 9.4.1 and see if it resolves the issue? laravel/framework@50b46db

Line to comment out: 404labfr/laravel-impersonate#154 (comment)

My app/Http/Kernel.php file does not contain \Illuminate\Contracts\Session\Middleware\AuthenticatesSessions::class, however, if. I comment out \Laravel\Jetstream\Http\Middleware\AuthenticateSession::class I do not get logged out.

fylzero commented 2 years ago

@driesvints I have verified that when trying to use this functionality the evaluations for $request->session()->get('password_hash_'.$this->auth->getDefaultDriver()) and $request->user()->getAuthPassword() are not equal to each other and this is causing the system to log the user out.

See: vendor/laravel/framework/src/Illuminate/Session/Middleware/AuthenticateSession.php:55

driesvints commented 2 years ago

@fylzero those changes are in the foundation kernel, not the app one. Have a closer look.

fylzero commented 2 years ago

@driesvints You are correct. Yes, commenting this out in the Foundation Kernel also resolves the logout issue.

I don't understand why but the evaluation on line 55 of vendor/laravel/framework/src/Illuminate/Session/Middleware/AuthenticateSession.php is coming back as true and logging an impersonated user out. Any thoughts on this? I don't really understand this code and my linter is saying these functions don't exist, so I can't seem to make it deeper down the rabbit hole.

fylzero commented 2 years ago

@driesvints It appears in Jetstream/Sanctum this is causing this to compare the password_hash_sanctum to the password_hash_web tokens. $request->user()->getAuthPassword() is pulling the web token, not Sanctum.

fylzero commented 2 years ago

@driesvints I read the changelog in Laravel and yes reverting the change here: https://github.com/laravel/framework/commit/50b46db563e11ba52a53e3046c23e92878aed395

...also resolved the issue. I believe that is the problem.

taylorotwell commented 2 years ago

I can't recreate this.

taylorotwell commented 2 years ago

@fylzero what you are reporting in your example repo is NOT a bug. That is in fact PROVING that session authentication is working correctly. You used login but didn't update the flashed password in the session that authenticate session uses to compare the current user's password to the one in the session - thus, you are logged out.

I can't recreate it when using the impersonation package in question because they fully flush the session by logging out and then logging in again.

CleanShot 2022-03-15 at 16 21 22@2x

This works on Jetstream using the lab404/laravel-impersonate package.

When using authenticated sessions, a naive Auth::login() call is not sufficient. More work has to be done in the session (which the impersonate package does) to make everything work right.

fylzero commented 2 years ago

@taylorotwell Fair enough. This was working before that update so, wasn't sure. I'll look at what the package is doing and make the necessary changes. Obviously this only "broke" user impersonation so, not super mission-critical.

Thanks for providing this info!

taylorotwell commented 2 years ago

Yeah - it's a bit complicated. It helps if you have a good grasp of how authenticated sessions work. I'll try to summarize:

When using authenticated sessions...

So, with this in place, when a user updates their password in a user profile section of an application, all other sessions for that user will automatically be logged out because their copy of the password in their sessions no longer matches the database. Pretty slick actually!

So, back to your example, you log a new user into the application, but you never update the session password hash accordingly. Therefore, they no longer match and you are logged out.

fylzero commented 2 years ago

For anyone else running into what I did, this seems to work for me but definitely feels backwards / like I'm cheating. getAuthPassword() seems to be grabbing the password_hash_web from session and I'm just forcing the sanctum token to match that to hack through things. I could be wrong, but it would be nice to do this the other way around imo. I also couldn't figure out how to dynamically grab the session key variable how vendor/laravel/framework/src/Illuminate/Session/Middleware/AuthenticateSession.php does. I may keep working on this later... but this does solve my problem for now.

// Log in as impersonated user
$user = Auth::loginUsingId($userId);

// Store impersonated user's password hash in session
session(['password_hash_sanctum' => $user->getAuthPassword()]);
larskoole commented 2 years ago

The simplest solution is to put auth()->shouldUse('web'); above any auth()->login() or auth()->logout() in your controller. No need for setting the new password hash.

I don't know if this brings any security risks though.