Improvements for Authorization #53

Urigo · 2016-12-20T20:01:07Z

Use ws package instead of websocket (https://npmcompare.com/compare/websocket,ws)
Add onConnect
Add onDisconnect
Add onUnsubscribe
Add connectionParams support (like headers) to support authentication
Add support for rejection on new connection (to support authentication)
Add socket options to support port/path options
Unit tests

some cleanup for old code

… INIT message

…d headers

some cleanup for old code

… INIT message

glasser · 2016-12-22T22:50:04Z

src/client.ts

  public client: any;
  public subscriptions: Subscriptions;
  private url: string;
  private maxId: number;
+  private connectionParams: ConnectionParams;
  private subscriptionTimeout: number;


This is a pre-existing issue but one worth considering: if you use reconnect, then subscriptionTimeout (which cannot be disabled) seems broken. Like, any subscribe is going to time out if you call it while you're offline, even though you might later reconnect! Will apps actually respond usefully to failed subscriptions by trying to subscribe again later? Tracing the timeout error looks like it would eventually get passed to an onError handler passed to subscribeToMore, which neither your GS PR nor GitHunt-react passes. Hmm...

Yeah, need to handle this with onError, will update the GS PR.

glasser · 2016-12-22T23:08:37Z

README.md

- `options?: Object` : optional object to modify default client behavior
-  * `timeout: number` : how long the client should wait in ms for a subscription to be started (default 5000 ms)
+- `options?: Object` : optional, object to modify default client behavior
+  * `timeout?: number` : how long the client should wait in ms for a subscription to be started (default 5000 ms)how long the client should wait in ms for a subscription to be started (default 5000 ms)


This line is doubled.

glasser · 2016-12-22T23:58:46Z

README.md

-  * `onSubscribe?: (message: SubscribeMessage, params: SubscriptionOptions, webSocketRequest: WebSocketRequest)` : optional method to create custom params that will be used when resolving this subscription
+  * `onSubscribe?: (message: SubscribeMessage, params: SubscriptionOptions, webSocket: WebSocket)` : optional method to create custom params that will be used when resolving this subscription
+  * `onUnsubscribe?: (webSocket: WebSocket)` : optional method that called when a client unsubscribe
+  * `onConnect?: (connectionParams: Object, webSocket: WebSocket)` : optional method that called when a client connects to the socket, called with the `connectionParams` from the client, the return value of this callback will be available as part of the `context` of the subscription. return `false` or throw and exception to reject the connection.


document that it may return a Promise?

also "an exception" not "and"

and be more specific about how the result is used: "if the return value is an object, its elements will be added to the context" or something

glasser · 2016-12-22T23:59:46Z

README.md


 ## Client-server messages
 Each message has a type, as well as associated fields depending on the message type.
 ### Client -> Server
+
+#### INIT


So interesting naming wrinkle here. The existing messages all start with SUBSCRIPTION_ and your new ones don't. Abstractly I think it's great to separate the INIT messages from the SUBSCRIPTION messages. But now SUBSCRIPTION_KEEPALIVE is weirdly named. Since we're doing enough backwards-incompatible stuff here maybe rename that one to KEEPALIVE?

Renamed to KEEPALIVE

Good but there are a couple remaining references to SUBSCRIPTION_KEEPALIVE

glasser · 2016-12-23T00:04:22Z

src/client-websocket.ts

+declare let window: any;
+let _global = typeof global !== 'undefined' ? global : (typeof window !== 'undefined' ? window: {});
+
+import * as NodeSocket from 'ws';


So the old websocket package has a browser field in its package.json which points to a tiny file which just exports the native WebSocket object. This means that when, eg, webpack (probably Meteor too, I only tested with GitHunt-react) bundles up subscriptions-transport-ws, it won't actually include all of the Node-oriented code from websocket in the client bundle.

That doesn't seem to work here: ws doesn't have the browser field so you're always going to bundle the entire ws library into the client bundle even though none of it is used. This seems bad. I think this would happen even if you converted the import into a require and put it under a conditional?

I think you can probably fix this by using the "browser" field yourself in this package's package.json somehow? I know we want to be able to use Client on the server too (for tests if nothing else) but there's probably some way to do it that keeps code splitting working.

The previous package, websocket had the logic that inside the file client-websocket internally, and this was the browser file.

I changed the usage so ws will be required only when there is no native WebSocket available, as suggested here: websockets/ws#837 , and also added browser property to expose ./dist/client.js only.

Hmm. dist/client.js still pulls in dist/client-websocket.js which has a require('ws') written in it (as opposed to in the old websocket module whose lib/client.js literally doesn't mention require). Are you sure this will work properly when webpacked? I admit I forget how I tested this a few weeks ago...

Not sure. I will verify it.

Please do verify this.

I changed the implementation of the client package, and now the client package look for the native WebSocket object, and you can provide an optional implementation if you are using Node.
This way, ws package is not part of the Meteor client bundle, and not loaded at all.

Other option, is to use object for browser field and specify that ws is ignored for browsers. But this is not supported yet for Meteor build (meteor/meteor#6890).

glasser · 2016-12-23T00:17:59Z

src/server.ts

        return;
      }

      // accept connection
-      const connection: Connection = request.accept(GRAPHQL_SUBSCRIPTIONS, request.origin);
+      request.resume();


Huh, we call resume synchronously with pause? That seems weird... why call pause in the first place? Also pause/resume is the ancient streams API, is this really the right thing to use?

Yeah we don't need it at all.. removed the pause and resume

glasser · 2016-12-23T00:21:45Z

src/server.ts

+        case INIT:
+          let onConnectPromise = Promise.resolve(true);
+          if (this.onConnect) {
+            onConnectPromise = Promise.resolve(this.onConnect(parsedMessage.payload, connection));


It's good that onConnect doesn't have to be synchronous, but you shouldn't process other messages until it's done running! As far as I can tell more onMessages can be invoked while onConnect is running. (This should be easy to write a test for.)

Perhaps store the Promise that contains the initResult on this, and then it before doing anything else?

Would it be simpler to get rid of initResult and just resolve the initPromise with initResult directly? You don't use initResult anywhere that's not already inside a initPromise.then...

Fixed! Thanks

glasser · 2016-12-23T00:23:29Z

src/server.ts


        case SUBSCRIPTION_START:
          const baseParams: SubscriptionOptions = {
            query: parsedMessage.query,
            variables: parsedMessage.variables,
            operationName: parsedMessage.operationName,
-            context: {},
+            context: Object.assign({}, this.initResult),


can't this.initResult be true? Maaaybe that works with Object.assign but it sure feels weird.

Really? Still looks the same to me...

Oh, I see, you only set initResult if it's an object. Maybe this is also the answer to my last question about why initResult isn't the value of initPromise? (But you still could probably make it be the value of initPromise by converting non-objects to {}.)

glasser · 2016-12-23T00:25:10Z

src/server.ts

            delete connectionSubscriptions[subId];
          }
          break;

        default:
          this.sendSubscriptionFail(connection, subId, {
            errors: [{
-              message: 'Invalid message type. Message type must be `subscription_start` or `subscription_end`.'
-            }]
+              message: 'Invalid message type!.',


! or . not both

glasser · 2016-12-23T00:27:50Z

src/server.ts

+  private sendInitResult(connection: WebSocket, result: any): void {
+    connection.send(JSON.stringify(result), () => {
+      if (result.type === INIT_FAIL) {
+        connection.close();


Why call both? Deserves at least a comment on why it's not appropriate to call just one of the similar methods

Added a comment about this.
close closes the socket and sends the error code, while terminate sends the actual FIN packet and terminates the entire WS connection

Are you sure that this actually works? Does the close packet actually get sent out over the network or does calling terminate synchronously close the socket too fast (I think it does). You can test this with wireshark or tcpdump or something...

This needs to be verified too.

Tested it and it looks fine, the client side gets the close message with the code, and the the socket terminates. Also, this is how ws uses it in it's unit tests.

…ntext and init promise for each connection

helfer · 2017-01-10T14:22:36Z

@Urigo I merged some PRs on the other branch, but then I saw that you guys are still working here, so I'll let you publish the new version. Because of the new typings a minor version bump will be required.

# Conflicts: # src/server.ts # src/test/tests.ts # tsconfig.json

stubailo · 2017-01-20T04:30:42Z

@mlp5ab this is the pr for auth!

…load "ws" package

…ative WebSocket)

# Conflicts: # src/index.ts

* typings.json: shut up typings install * package.json: bump all dependencies Signed-off-by: Pierre Carrier <pierre@meteor.com>

wawhal · 2018-07-16T06:05:51Z

@Urigo Any documentation related to the changes by this PR?
To be more specific, I am looking to set custom HTTP headers while using subscription-transport-ws. Just adding it to the connection params doesn't seem to be working.

Sashko Stubailo and others added 26 commits December 20, 2016 18:39

Add to changelog

662bf29

0.3.1

7a19511

feat(ws): replace websocket with ws package which supports UPGRADE an…

cd8cf46

…d headers

feat(events): added support for onUnsubscribe, onConnect, onDisconnect

a248587

feat(ws): added connection params

8837e13

feat(ws): added support for browser client

1e87aaf

feat(ws): added support for full external config for the websocket

aca6b20

some cleanup for old code

feat(ws): add support for connectionParams and connectionCallback and…

40d5ec5

… INIT message

feat(ws): fixed failing tests and remove console.log

6a60bf0

feat(ws): some fixed and added unit tests for client side

3c5a933

feat(ws): added server side tests

153f6a6

docs(README): updated README

8e7082b

feat(ws): replace websocket with ws package which supports UPGRADE an…

2ab5711

…d headers

feat(events): added support for onUnsubscribe, onConnect, onDisconnect

9eeaf82

feat(ws): added connection params

b603475

feat(ws): added support for browser client

d3911a7

feat(ws): added support for full external config for the websocket

66e7b0a

some cleanup for old code

feat(ws): add support for connectionParams and connectionCallback and…

b268be0

… INIT message

feat(ws): fixed failing tests and remove console.log

0bda794

feat(ws): some fixed and added unit tests for client side

bd706c7

feat(ws): added server side tests

f50e074

docs(README): updated README

dcfd6b9

docs(README): updated README

248c662

Merge remote-tracking branch 'origin/feature/ws' into feature/ws

f15f803

feat(ws): added more tests and some fixed

e1681cb

feat(ws): test fixes

7c8b769

Urigo requested review from glasser and stubailo December 21, 2016 18:14

glasser suggested changes Dec 23, 2016

View reviewed changes

feat(ws): fixes after code review

3bf41df

Urigo added 3 commits December 26, 2016 20:27

feat(ws): added client side events for connect, disconnect, reconnect

b700388

feat(ws): fixed naming, exports and environment separation

a90e5f4

feat(ws): fixed a bug with multiple connections - created a unique co…

b79da71

…ntext and init promise for each connection

Urigo mentioned this pull request Dec 30, 2016

Reconnection strategy for Subscriptions apollographql/apollo-client#1025

Closed

Urigo added 4 commits January 10, 2017 18:54

Merge branch 'master' into feature/ws

58203c4

# Conflicts: # src/server.ts # src/test/tests.ts # tsconfig.json

feat(ws): fixes after merge

9cddcc2

feat(ws): fixed after code review

66dbcb4

feat(ws): fixed typo

d15e9ad

Urigo added 3 commits January 30, 2017 18:28

feat(ws): modify usage of websocket implementation, so browser won't …

989870a

…load "ws" package

feat(ws): remove connection status (now need to use the native/altern…

0fe5c5e

…ative WebSocket)

feat(ws): fixed tests

330e10c

Urigo mentioned this pull request Feb 2, 2017

Module browser bundle configuration using webpack #48

Closed

4 tasks

Urigo added 3 commits February 3, 2017 10:09

Merge branch 'master' into feature/ws

e762028

# Conflicts: # src/index.ts

feat(ws): rebase and fixes

3c72ddf

feat(ws): updated CHANGELOG

eaf872d

Urigo merged commit e6a12d8 into apollographql:master Feb 5, 2017

Urigo deleted the feature/ws branch February 5, 2017 12:03

This was referenced Mar 3, 2017

INIT_FAIL message appears to not be properly formatted #88

Closed

authorizing subscriptions -- set a default context for client #75

Closed

baconz pushed a commit to PhiloInc/subscriptions-transport-ws that referenced this pull request Apr 24, 2018

round of dependencies updates (apollographql#53)

dd05d27

* typings.json: shut up typings install * package.json: bump all dependencies Signed-off-by: Pierre Carrier <pierre@meteor.com>

Improvements for Authorization #53

Improvements for Authorization #53

Conversation

Urigo commented Dec 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glasser Jan 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helfer commented Jan 10, 2017

stubailo commented Jan 20, 2017

wawhal commented Jul 16, 2018

glasser Jan 9, 2017 •

edited

Loading