feat(subscriptions): support graphql-ws and all operations over websockets

[enisdenjo]

Story

Support graphql-ws clients alongside subscriptions-transport-ws.

A single breaking change of dropping the hook postgraphile:ws:onOperation in favour of the new postgraphile:ws:onSubscription is introduced by opting-in.

Additionally, support all 3 GraphQL operations by setting the websocketOperations option to all. Defaults to subscriptions which restricts operations to subscriptions only.

TODOs:

• WebSocket transport support for query & mutation
• Add support for live queries
• Add support for silent auto-reconnect (Commit c6f7872)
• Expose client listeners for connected, connecting and disconnected (Commit 627775b)
• Implement keep-alive (PR #11)

[pyramation]

this looks awesome! following :)

[asafyish]

What's the advantage of using graphql-transport-ws instead of subscriptions-transport-ws ?
subscriptions-transport-ws has over a million weekly downloads, while graphql-transport-ws has just over 100.

[enisdenjo]

Hey @asafyish, good question! In short - the subscriptions-transport-ws is stale, not actively maintained, "Work in progress" for 4 years and has quite an amount of bugs and security implications. (Check out a rough overview of them here)

On the other hand, graphql-transport-ws is a fresh, zero-dependency, plug-n-play, rewrite of the transport using modern JS. Not only is it a rewrite of the lib itself, but it also offers a redesigned, security first, GraphQL over WebSocket Protocol which aims to be standardised with the help of the foundation's GraphQL over HTTP work group.

[enisdenjo]

Hey @benjie,

As of [email protected] the lib is feature full and ready for the world! I've been using it in production for a whole week now - except for the occasional closure caused by network problems, I ran in no problems at all.

Will make a PR for v5 later on. However, besides resolving conflicts, rebasing, dropping the version bumps and deciding on how to handle breaking changes; this PR is ready too!

[benjie]

Awesome 🙌 We're definitely going to have to work to minimize breaking changes; but fortunately the server plugin API has this note at the very top:

Stability: experimental, may change in semver minor versions.
-- https://www.graphile.org/postgraphile/plugins/

So basically I'm happy to break the server plugin API if it's absolutely necessary, so long as that breakage is super obvious. So we need to make it so people calling the older hooks causes a panic and the system refuses to start. Do you want to have a go at doing this?

[benjie]

(Before anything else, please rebase. I'm on the verge of releasing 4.9.0 so I don't think there's any more major changes coming in soon.)

[enisdenjo]

Thats great! We should be leaving the experimental phase soon 😁. From the looks of it, there are 2 breaking changes:

• subscriptions-transport-ws is not supported anymore. The client counterpart must use this lib!
• The hook name change from postgraphile:ws:onOperation to postgraphile:ws:onSubscription

[enisdenjo]

Ready 👍

[benjie]

The first of those:

subscriptions-transport-ws is not supported anymore. The client counterpart must use this lib!

This is a major breaking change. We can't do it on a semver minor update. Protocol-wise, what specifically has broken? Can we use both libraries and use a header/sniff the traffic to pipe it into one or the other implementation? Maybe we can change the --subscriptions flag to accept something like --subscriptions=v2 so they can opt in to the new library. Maybe we give them warnings about security/etc if they don't.

I'd like to sort this out conceptually before I review the specific code.

[enisdenjo]

I like your idea of using the flag. I see it something like this:

type SubscriptionsFlag = 'v0' | 'v1' | boolean;
// true = 'v0' 👈  to reduce breaking changes. For all those who had this flag set to true - nothing will change.

I am thinking about v0 and v1 because subscriptions-transport-ws is still on the 0th major version, while the graphql-transport-ws is on the 1st.

Also, if you choose v1, the plugin hook would change from postgraphile:ws:onOperation to postgraphile:ws:onSubscription.

What do you say?

[benjie]

This seems very reasonable. Could you attempt to integrate this in? Also I don't think we actually have tests for websocket connections right now, but with two separate websocket implementations in play we're going to need them - are you interested in taking this on or do you need me to attempt it? I'd like to see tests for the existing implementation merged separately (with this PR in mind) and then see how this PR changes those tests.

[benjie]

Actually; can you add a --websockets=none/v0/v1 flag, since the --subscriptions flag is already quite overloaded (it's treated as a feature flag that enables a bunch of stuff inside Graphile Engine that's arguably a breaking change but I don't expect anyone to notice). It should default to v0 if --subscriptions is specified, and none otherwise. We should also keep in mind the websocketOperations option proposed in #1233.

[enisdenjo]

I'll do the websocket flag implementation for sure, no worries! Will try doing that today already.

Regarding the tests, I'd be happy to contribute with test cases; but, I would be very grateful if you could lay down the ground work? Just some basic subscriptions test setup to help me understand more about PostGraphile's approach to testing and to allow the tests to fit with the rest of the codebase.

My lib already fully supports all operations, so I can integrate that in this PR too.

I also oversaw the discussion about auth in #1233 - so I'd just like to add my 2 cents with how I approach this in my production environments. Besides relying on CORS to protect against connections originating from unknown sources I authenticate the requester on every subscription request and kick him out if I sense something fishy (almost the same way you authenticate plain ol' HTTP requests). However, the connectionParams I check against are set-up once (during the connection initialisation phase) and persisted during subsequent subscription requests - but, is still think this is completely OK - for example: if you use JWT to authenticate and it has an expiration date - it would expire for the sockets the same way it expires for the usual HTTP req/res. Since the lib supports silent auto-reconnects; further improvements could also potentially be gracefully closing the sockets with clients and communicating a "re-auth" challenge through the close event, something like: 4511: Authentication Required - but this might cause losing some emitted events in critical moments (can be avoided by using @pgSubscription(intialEvent: ...)).

[benjie]

Sounds good; hopefully I can take a look at this on Tuesday 👍

[enisdenjo]

Rebased.

[enisdenjo]

Hey @benjie, I have addressed all raised concerns. Renamed the name and updated the PR description to reflect that. Would be awesome if you can check it out!

[enisdenjo]

👋 @benjie, me again. I guess you are very busy and I don't want to be annoying, I just wanted to ping and check about this PR. I am looking forward to using the official release instead of a packaged fork 😁 .

[enisdenjo]

Rebased.

[jwrascoe]

@enisdenjo I was just speaking to @benjie and was looking to see how to support changing JWT tokens on the fly for a WS connection... basically check it every time the onOperation is called in subscriptions.ts
https://github.com/graphile/postgraphile/blob/1da44d37c18afb3627cd8e9fc41b275514faee7a/src/postgraphile/http/subscriptions.ts#L227

I would like to use this obviously to change roles when the query hits.

Have you considered this at all?

[enisdenjo]

Hey @jwrascoe,

The onOperation equivalent in this case is the onSubscribe callback (and the matching postgraphile:ws:onSubscription is plugin hook):

https://github.com/enisdenjo/postgraphile/blob/36e3561f0ea17cbd74932dac98898bd01dbf218e/src/postgraphile/http/subscriptions.ts#L387-L416

By "changing JWT tokens on the fly" you mean changing the actual token in the original connectionParams request? You could do that on every subscribe request (operation) like this:

...
async onSubscribe(ctx, message, args) {
  // ctx is the context of the connection and the connectionParams are from the initialisation message
  ctx.connectionParams.authorization = yourBearerToken(request, message, args);
  // request is the initial connection HTTP request from the client
  ctx.request.headers.authorization = ctx.connectionParams.authorization;
  ...
}
...

Out of curiousity, why would you change the clients JWT token to switch roles?

[jwrascoe]

@enisdenjo Interesting... let me see if I can figure out how to implement your suggestion.

Without a token, I have the default access set to only allowing the request of a token... once the user provides credentials, I look it up and then decide which role they need to be so I can enforce security..

The reason for changing the token is not to change roles (only if they log in and out as someone else) but rather to secure the data a bit more.

In the event that the JWT token was caught somehow in a packet trace, I only wanted it to be good for a few minutes. The way I wrote my client it looks at the TTL in the JWT and renews it (if its able to) for continued access.

I could close and restart the WS connection, but if I had outstanding subscriptions I would have to re-issue them and wanted to just supply the replacement(s) token to keep the connection alive with the subscriptions intact.

Maybe there is a better way? Or im just being paranoid?

[enisdenjo]

I'd say you should always be aware of the exact details a client provides (or has provided) you; so, personally I wouldn't change the token on the fly in the server ever...

I would recommend relying on the library and the @pgSubscription's initialEvent argument. Keep checking the JWT token validity on every subscription request using the subscription directive topic builder: @pgSubscription(topic: ${embed(checkAuthAndBuildTopic)}). If the checks fails because of whatever, terminate the WS connection completely, the client will silently try to reconnect again and all "hot" subscribers on the client side will re-subscribe. Using the initialEvent argument, you can issue an event on every subscription and therefore supplement any data that the client might have missed during the time it was trying to reconnect. Furthermore, the client has a separate process of requesting a new token on expiry. So, if the server kicks the client off, the reconnection attempts should already be using the new, refreshed, token.

[jwrascoe]

@enisdenjo ok, thanks for the info... I'll see if I can come up with something that fits my use case.

[enisdenjo]

After a brief discussion with @benjie over GraphQL Slack a decision was made to allow choosing the websockets library on the back with an array. (If subscriptions are active, it will default to websockets=[v0, v1]).

Furthermore, the GraphiQL was reverted to also support both libraries. subscriptions-transport-ws will be used only if the server websockets argument provides just v0 ([v0]) - in all other cases (with enabled subscriptions), v1 will be used.

[benjie]

Really close now 👍

[benjie]

Awesome; looks good to me! Thanks for all your hard work on this 🙌