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

[8.x] Clear a cached user in RequestGuard if a request is changed #35692

Merged
merged 2 commits into from Dec 22, 2020

Conversation

n7olkachev
Copy link
Contributor

@n7olkachev n7olkachev commented Dec 22, 2020

This guard retrieves a user based on a request data, so a cached user should be cleared on a request change.

Personally, I've faced a problem with it while creating a test for my API with a help of default Laravel testing methods (postJson, getJson etc). Requests with different API tokens got the same user (from the first request) because it is cached which is counter-intuitive.

This guard retrieves a user from a request data, so a cached user should be cleared on a request change.
@taylorotwell taylorotwell merged commit 0d601f5 into laravel:8.x Dec 22, 2020
@GrahamCampbell GrahamCampbell changed the title Clear a cached user in RequestGuard if a request is changed [8.x] Clear a cached user in RequestGuard if a request is changed Dec 22, 2020
@aaronhuisinga
Copy link

@taylorotwell @GrahamCampbell This breaks the Passport::actingAs method, thus breaking all tests using Passport.

@driesvints
Copy link
Member

We reverted this

@bomas13
Copy link

bomas13 commented May 23, 2022

I'm actually a bit confused because without this change it's not possible to test Sanctum logout and/ or token refresh properly. See https://laracasts.com/discuss/channels/testing/tdd-with-sanctum-issue-with-user-logout-case?page=1&replyId=797893

@n7olkachev
Copy link
Contributor Author

This change still makes sense to me, but I don't remember any details why I didn't continue with it. Looks like there is a bug in actingAs method but I don't see it at first glance.
Also, @sebastiaanluca, IIRC we discussed something about this on Twitter?

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

5 participants