-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
GraphQL authenticated subscription #20092
Comments
/cc @jmartisk, @phillip-kruger |
/cc @sberyozkin |
#16847 is probably related |
Sounds right yes, as we are using Websockets under the covers. |
Do we have a workaround ? |
@SLedunois Where doe the credentials come from ? Do you mean you'd like to authenticate into Quarkus first say with the bearer JWT token and then have it associated with the GraphQL channel ? |
@SLedunois Where do the credentials come from ? Do you mean you'd like to authenticate into Quarkus first say with the bearer JWT token and then have it associated with the GraphQL channel ? |
@sberyozkin It's just a Cookie authentication provided by build int authentication system. |
@SLedunois I don't think the cookies are retained in a web socket channel. The following suggests how it can be done: https://coletiv.com/blog/using-websockets-with-cookie-based-authentication/ One can probably have a random value created at the server concatenated with the cookie obtained via the form auth. And bind to the web socket channel - and then when it comes in back to Quarkus via WebSockets - attach to This is just a possible idea, may not be workable. I doubt Quarkus can help much in this particular case but I may be wrong... |
@sberyozkin Hum. A nice option could be to switch on Cookie authentication for loading my application and using a JWT to consume GraphQL API if GraphQL susbription authentication works fine with JWT ? |
The biggest problem I'm seeing with all the WebSocket implementations is their lack of an ability to handle contextual data. Would it possibly be beneficial to add some form of abstracted extension capability to allow users to populate the security identity for websocket requests? And possibly refresh them? My thinking is that users with a single Quarkus application running can just create an in-memory map similar to Map<Token, SecurityIdentity> authTokens Whenever a websocket request comes in, you're able to authenticate it and remove the entry from the map. Similar to a TenantResolver. If someone has a clustered application, they would be able to swap the map out for a clustered key value like Redis with expirations. What my takeaways are from websocket authentication are:
|
one comment about websocket is that the websocket session has way of storing some context data |
In addition, I'd like to also bring up option to be able to filter events push to the subscription based on the security context as described here smallrye/smallrye-graphql#1072 |
Looking into the Apollo Spec, ConnectionParams is what should be used for WebSockets and GraphQL authentication. ConnectionParams is an event that is sent over the Websocket from the consumer, it contains user-defined data to add metadata to the subscription such as credentials. Because this is done within the request there is no risk of leaking authentication credentials (bearers) in the query params. Thinking of how this could plug into OIDC, would it be possible to handle ConnectionParams with an interceptor-style bean? The default implementation could accept something like Are there any contextual problems that may come up when authenticating WebSockets in the traditional sense? (Request Context lifecycle) @phillip-kruger |
@jmartisk - f.y.i - if you are keen to look at this ^^^ |
Is there any progress on this issue? |
Nothing yet, are you keen to contribute ? |
I will do my best to find time to solve this issue in the nearest future |
Is there anything new related with this? Tried to do some workaround but without success. |
@chemistfriend the best way I figured out to do this was with the raw Vert.x graphql API. We ditched Smallrye completely in favor of vertx. For our GraphQL API (non-websocket) we resorted to using Spring DGS (by Netflix) as it was a far better schema-first workflow. If anyone on the Quarkus team is onboard to helping me get PRs merged between Quarkus and Smallrye GraphQL I am more than open to taking the time to implement the needed changes, but it's going to be an ongoing effort. Just give me a shout if this is something that is wanted on the Roadmap. |
Describe the bug
Using a subscription behind an authentication system produces a
DataFetchingException
like :Like this class (https://github.com/SLedunois/authenticated-graphql/blob/master/src/main/java/fr/sledunois/AuthenticatedGraphql.java) Graphql allow queries and mutation behind an authentication mechanism. Opening a subscription accept the websocket. However, it seems that security handler rejects the security context during subscription initialization :
Expected behavior
Subscription should works fine with authentication system.
Actual behavior
Opening a subscription workflow:
{"query":"subscription userCreated{\n userCreated{\n id\n }\n}","variables":null,"operationName":"userCreated"}
{"errors":[{"message":"Exception while fetching data (/userCreated) : null","locations":[{"line":2,"column":3}],"path":["userCreated"],"extensions":{"classification":"DataFetchingException"}}],"data":null}
Server throws the following exception:
How to Reproduce?
Reproducer: https://github.com/SLedunois/authenticated-graphql
Steps to reproduce the behavior:
quarkus dev
graphql
and passwordpassword
query { get { id } }
It works 👍mutation createUser{ create{ id } }
It works 👍subscription userCreated{ userCreated{ id } }
It failes 👎Output of
uname -a
orver
Windows 10
Output of
java -version
openjdk version "14.0.1" 2020-04-14 OpenJDK Runtime Environment (build 14.0.1+7) OpenJDK 64-Bit Server VM (build 14.0.1+7, mixed mode, sharing)
GraalVM version (if different from Java)
No response
Quarkus version or git rev
2.2.2.Final
Build tool (ie. output of
mvnw --version
orgradlew --version
)./mvnw clean package
Additional information
No response
The text was updated successfully, but these errors were encountered: