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

User authentication context not restored in subscription broadcast #1302

Closed
stayallive opened this issue Apr 18, 2020 · 4 comments · Fixed by #1306
Closed

User authentication context not restored in subscription broadcast #1302

stayallive opened this issue Apr 18, 2020 · 4 comments · Fixed by #1306
Labels
discussion Requires input from multiple people

Comments

@stayallive
Copy link
Collaborator

When broadcasting a subscription the GraphQL request is not performed with the same user authentication context, so if you use auth()->user() anywhere in your code this will fail in a subscription broadcast (or use the incorrect user if you don't use a queue).

We can argue it is bad to depend on the auth()->user() in code flowing through Lighthouse because it has it's own context with the authenticated user (which is properly set/restored on a subscription broadcast), and you might be right, but that's not the point of this 😄 Some getters in models will make use of the authenticated user (think getCanDeleteAttribute() for example) or do some stuff based on the authenticated status/role/config.

So depending on where you stand on the above, this is a non-issue or a big issue.

The biggest problem is actually when you are not queueing your broadcasts because then the subscriptions are executed as the currently authenticated user and might allow you to get broadcasted information retrieved for the wrong user. If you run a actual queue worker this is probably not an issue since your code will just crash / handle it on auth()->user() being null.

After some research I concluded there might only be one option to correctly "restore" the user context with 1 caveat.

Since the Laravel Guard has no real way of setting and clearing the current user (without sending all kinds of login/logout events) we would have to register a "fake" guard that allows us to do so and we have to store the current authenticated user ID and the used user repository so we can register the faked guard and set that as the default guard to use (and restore the previous guard after we're done).

The only cavaet is if the user would use auth('api')->user() in their code, because we cannot replace a named guard without some real hocus-pocus I'm not comfortable implementing. But this might be an acceptable trade-off seeing how the current behaviour is broken and/or even dangerous (when not using queued subscription broadcasts).

I would like to hear your thoughts on this issue and/or if a fix should be explored or this is considered a feature instead of a bug and auth()->user() should never be used in a GraphQL request (only $context->user()).

@stayallive stayallive added the discussion Requires input from multiple people label Apr 18, 2020
@spawnia
Copy link
Collaborator

spawnia commented Apr 18, 2020

Tough one. The convenient features of Laravel, such as just being able to call auth()->user() anywhere, often don't play nicely once we leave the realm of handling HTTP requests sent by a user.

@spawnia
Copy link
Collaborator

spawnia commented Apr 18, 2020

Semantically, it makes sense to log the user in for the duration of resolving their subscription request when we think of a subscription as the user telling the server "Run this query on my behalf whenever X changes". In that case, mechanisms such as access control should still apply.

The other way to look at a subscription would be to view it as users hooking in to some kind of broadcast, such as TV. When watching TV, users can not expect to get content tailored to them, specifically.

Lighthouse is in a weird place right now and doesn't properly log users in for every aspect of a query. We should get better here, if we decide that we want to support the first use case.

If we were to abandon logging in the user altogether when resolving subscriptions, we dictate that only the second use case is supported. That would diminish functionality we have now.

@spawnia
Copy link
Collaborator

spawnia commented Apr 18, 2020

I think the most generally useful and feasible approach is to make it so the user appears logged in when using the default mechanisms for retrieving the current user, e.g. auth()->user() or Auth::user().

I think we don't need to bend over backwards to cover corner cases, such as users calling auth('api') directly in their Model. That's just asking for trouble, and should rightfully fail.

@stayallive
Copy link
Collaborator Author

I agree with you on that. I will try and get a PR up in the coming days that will fake a guard and will run each subscription resolver in the context of that fake guard, I think I figured out how to do it, now only need to find a way to not make it a complete mess 🙈

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Requires input from multiple people
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants