Add handler method to process websocket connections

[kristjanvalur]

Allow an application to process the connection request for graphql_transport_ws and graphql_ws
protocols, perform authentication, reject a connection and return custom response payload

Description

A previous PR, #2380 added support for connection_params. However, they were simply taken as provided and
injected into the context object. This PR adds an overridable hook in the AsyncBaseHTTPView, on_ws_connect()
which allows an application to examine the connection parameters when a connection is made.
The method receives an WSConnectionParams object, params.
The application can then

• choose to reject the connection (by calling params.reject())
• add additional information into the params.connection_params or modify it.
• set params.rsponse_params to return a payload to the client.

This is based on similar behaviour in the reference implementation for graphql_transport_ws, namely
this code here:
https://github.com/enisdenjo/graphql-ws/blob/972ed07ea263624bb556970cfd85bb05ea2c6eac/src/server.ts#L614
except that we use a parameter object to operate on.

This approach allows us to split authentication and authorization into two steps. Authentication can be done when establishing connection and authorization can be performed for each resolver.

Documentation is modified and also a piece added about how to perform authentication for single result operations (queries and mutations) since they can be invoked both via http and websockets.

Types of Changes

• Core
• Bugfix
• New feature
• Enhancement/optimization
• Documentation

Issues Fixed or Closed by This PR

Checklist

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• I have tested the changes and verified that they work and don't break anything (as well as I can manage).

[codecov]

Codecov Report

Merging #2617 (a755572) into main (6dd5552) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2617      +/-   ##
==========================================
+ Coverage   96.17%   96.18%   +0.01%     
==========================================
  Files         211      211              
  Lines        9223     9259      +36     
  Branches     1704     1709       +5     
==========================================
+ Hits         8870     8906      +36     
  Misses        227      227              
  Partials      126      126              

[botberry]

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:

---

A method on strawberry.Schema can now be overridden to validate connection attempts
for protocols graphql_transport_ws or graphql_ws.

class MyView(GraphQLView):
    async def on_ws_connect(self, params: WSConnectionParams) -> None:
        user = await authenticate(params.request_params.get("authorization"), "")
        if not user:
            params.reject()  # reject connection
            return
        params.request_params["user"] = user  # for use by resolvers
        # return custom connection response payload to user
        params.response_params = {"username": user.name, "karma": user.karma}

This offers similar functionality to other frameworks such as onConnect in Appollo Server.

---

Here's the preview release card for twitter:

Here's the tweet text:

🆕 Release (next) is out! Thanks to @kristjanvalur for the PR 👏

Get it here 👉 https://github.com/strawberry-graphql/strawberry/releases/tag/(next)

[kristjanvalur]

3.10 test runner seems to have FastAPI version problems.

[kristjanvalur]

Can we disable this warning from prettier, it is just silly. I mean, the documentation is about rejecting connections.

docs/general/subscriptions.md
  86:115-86:121  warning  Be careful with `reject`, it’s profane in some cases  reject  retext-profanities

[patrick91]

@kristjanvalur you can add reject to the .alexignore file 😊

[kristjanvalur]

ruff and mypy can't agree on whether to use return (ruff) or return None (mypy). How to resolve this?

[patrick91]

ruff and mypy can't agree on whether to use return (ruff) or return None (mypy). How to resolve this?

@kristjanvalur feel free to disable the rule that's causing issues in Ruff :)

[nrbnlulu]

IMO the public API for this should be in extensions, supporting http integrations as well.

class MyExtension(SchemaExtension):
    def on_gql_init(self):
        print("GraphQL connection initialized")
        yield
        print("GraphQL connection died")

other than that you could just wrap your asgi/wsgi application with something so it doesn't have much to do with strawberry.
#2097 related...

If you'll need to close the connection you can raise GraphQLError.
@kristjanvalur I am willing to collaborate on this if you want.

[kristjanvalur]

IMO the public API for this should be in extensions, supporting http integrations as well.

Not sure what you mean, by "http integrations". What exactly do you have in mind? This is specifically for websockets so that a connection can be accepted or rejected based on the payload provided in the sub-protocol's greeting message.

This PR has been edited so that the change is on the view not the schema, but I clearly forgot to change the RELEASE.md.

[nrbnlulu]

Not sure what you mean, by "http integrations". What exactly do you have in mind?

Currently there is no hook for when a graphql connection has started on extensions
this is not something that is specific to websockets...

[kristjanvalur]

Currently there is no hook for when a graphql connection has started on extensions

Well, I must confess that I'm not super familiar with extensions. But I do know that they are, for example, not supported for subscriptions at the moment. That might be something for me to look at in a separate PR. This particular PR is extending a core functionality for websockets, namely the "request_params" feature which was added int #2380

[kristjanvalur]

@DoctorJohn , what do you think about this approach to the api? Getting an object passed with a reject() method?

[kristjanvalur]

Note, as pointed out, a different approach might be to do this via an extension. There is already an extension PR for websocket operations (e.g. #2784 and #2825). We could extend the callbacks to add an on_ws_connect() hook there instead of doing it in the View.

[DoctorJohn]

what do you think about this approach to the api? Getting an object passed with a reject() method?

IMHO it's an interesting approach with clearer semantics compared to the approach of returning None, dict or the the False literal. That being said it's an extra object we create on every connect with only one method, a pattern we don't really use anywhere else. I should talk to @patrick91 about this API.

At this point I feel like there is three options: 1. special return values 2. params object with a reject method 3. maybe a exception the user could raise to reject the connection (just an idea, not suggesting it's superior at all) 4. maybe use an extension instead

I would be interested in seeing this as an extension. Sounds like a good usecase for an extension which could allow us to keep the implementation lean. Sorry for yet another delay, I'll take a look at the proposed subscription extension APIs.

[kristjanvalur]

I hope your objection isn't performance related. An "extra object on every connection" is not problematic for websockets connections where the connection is long lived and serves to support a number of requests and subscriptions. Performance always needs to be considered in perspective.

[DoctorJohn]

I hope your objection isn't performance related

Not trying to prematurely optimize here, it's purely about introducing a new pattern into the codebase. It's also less of an objection and more something I'd like to pitch to Patrick first since he has a good sense for API design that'll make devs happy ;)

Admittedly I should have done that weeks ago, sorry again, real life got in the way

[kristjanvalur]

Right. I'm happy to change the api however you think is best.
There is also a separate suggestion that all of this could be done via the "Extensions" mechanism, as I suggested here: #2617 (comment)

[XChikuX]

Does this allow for custom headers?
I need to add

  headers: {
    "User-Agent": "react-native",
  },

So I can get websockets on iOS working through wss://

[kristjanvalur]

Does this allow for custom headers? I need to add

This is just about looking at the "params" from the graphql_transport_ws connection request. Nothing to do with HTTP headers. What exactly is preventing you from getting websockets to work on IOS? Can you explain a bit better the problem you are having?

[XChikuX]

Does this allow for custom headers? I need to add

This is just about looking at the "params" from the graphql_transport_ws connection request. Nothing to do with HTTP headers. What exactly is preventing you from getting websockets to work on IOS? Can you explain a bit better the problem you are having?

I've been trying to triage a secure websocket issue I've been facing with iOS. Initially I thought there were extra security measures taken such as adding user-agent to the headers.

Turns out its a misconfiguration on apple's part.

See Here

I switched to TLSv1.2 and everything works now. Thanks for getting back to me though :)