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 user not being able to logout #508

Closed
wants to merge 2 commits into from

Conversation

GigaGiorgadze
Copy link
Contributor

issue description: when user logins as user A and then logs out and tries to login as user B they can not send request to route protected by auth:sanctum middleware

how to reproduce: i prepared two repositories (back, front) for reproducing, all you have to do is install backend and frontend repositories, following install guideline in the readme and then press buttons on frontend in following order: login gmail. fetch me, logout, login redberry, fetch me. this second "fetch me" should return 401 without this fix applied. instead of going and changing vendor i crated custom middleware which implements same fix so you can just replace 'authenticate_session' in config/sanctum.php with this 'authenticate_session' => App\Http\Middleware\AuthenticateSanctumRequest::class, and issue will be fixed

video proof without fix:

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

video proof with fix:

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

i believe this issue was caused because user was grabbed from the request even though user was logged out during that request. using auth guard to grab it from current session seemed to fix it

@taylorotwell
Copy link
Member

I am closing this pull request because it lacks sufficient explanation, tests, or both. It is difficult for us to merge pull requests without these things because the change may introduce breaking changes to the framework.

Feel free to re-submit your change with a thorough explanation of the feature and tests - integration tests are preferred over unit tests. Please include it's benefit to end users; the reasons it does not break any existing features; how it makes building web applications easier, etc.

Thanks!

@GigaGiorgadze
Copy link
Contributor Author

I am closing this pull request because it lacks sufficient explanation, tests, or both. It is difficult for us to merge pull requests without these things because the change may introduce breaking changes to the framework.

Feel free to re-submit your change with a thorough explanation of the feature and tests - integration tests are preferred over unit tests. Please include it's benefit to end users; the reasons it does not break any existing features; how it makes building web applications easier, etc.

Thanks!

okay ^^, i will write tests for this middleware and make sure my change does not break anything and re-submit with better description of the issue me and my team is facing in couple of days.

Thanks ^^

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