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

Fix/unable to logout #511

Merged
merged 11 commits into from Apr 10, 2024
Merged

Conversation

GigaGiorgadze
Copy link
Contributor

@GigaGiorgadze GigaGiorgadze commented Apr 8, 2024

this is follow up to previous pull request

Issue description

AuthenticateSession middleware sets password hash in session after request is sent but uses user from request, this results password hash not being removed from the session after loggout.
this does not cause user to be counted as authenticated but in case user tries to log in with different user following request will return 401 because passwords do not match.

how to replicate

i created two repositories for this, to replicate all u have to do is configure them locally according to readme. back and front. after installing this you you should go in frontend and click buttons in following order: "login gmail", "fetch me", "logout", "login redberry", "fetch me". this last request from frontend will fail without this fix. here is video:

simplescreenrecorder-2024-04-06_14.48.30.mp4

why it does not break any existing features

all previous tests are passing and all i changed was place of getting user, meaning i changed getting user from request to getting them from auth() guard which is up to date after log out.

tests

i added one test case in FrontendRequestsAreStatefulTest which verifies that password_hash_web is removed if user was logged out during that request and is set if user was not logged out during that request.

without fix implemented this line fails:

$this->getJson('/sanctum/api/logout', [
            'origin' => config('app.url'),
        ])->assertOk()->assertSee('logged out')->assertSessionMissing('password_hash_web');

@taylorotwell
Copy link
Member

@GigaGiorgadze Why are you using the global auth helper if you already have an AuthFactory instance injected into the middleware? Please mark as ready for review when you want me to look again.

@taylorotwell taylorotwell marked this pull request as draft April 9, 2024 14:52
@GigaGiorgadze
Copy link
Contributor Author

@GigaGiorgadze Why are you using the global auth helper if you already have an AuthFactory instance injected into the middleware? Please mark as ready for review when you want me to look again.

@taylorotwell no reason besides the fact that i did not notice 😓. i changed to using injected auth factory, so i will reopen, let me know if anything else seems off ^^

@GigaGiorgadze GigaGiorgadze marked this pull request as ready for review April 9, 2024 15:07
$this->storePasswordHashInSession($request, $guards->keys()->first());
$guard = $guards->keys()->first();

if ($this->auth->guard($guard)->hasUser()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sanctum can be configured to attempt to use an array of guards to authenticate users. How do you know the first guard in the list is the one that actually was used and is the one that has the user?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point 👀. i did not give it much thought since it was like that before. i rewrote it to getting first guard that user is authenticated with.

@taylorotwell taylorotwell merged commit 9cfc0ce into laravel:4.x Apr 10, 2024
4 checks passed
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.

None yet

2 participants