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

Designs #27

Merged
merged 2 commits into from
Dec 2, 2016
Merged

Designs #27

merged 2 commits into from
Dec 2, 2016

Conversation

Urigo
Copy link
Contributor

@Urigo Urigo commented Dec 1, 2016

@helfer thanks for the great idea

@glasser I'll add here some of the discussions we had internally

* https://auth0.com/blog/auth-with-socket-io/
* https://auth0.com/blog/auth-with-socket-io/
* Suggests using authorization on connection using url parameter with the auth token.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth mentioning what Meteor does.

The DDP protocol itself has no concept of authentication. Authentication is done via normal method calls.

The Meteor DDP server implementation allows method calls to store state on the connection object representing the current user ID, which is accessible from other methods and publications. If the user ID ever changes, all publications are basically re-evaluated from scratch (inside the server). Nothing special is done on the server side to allow methods to notice if the user ID changes while they are running; however, methods run in series unless they explicitly ask to unblock the connection (and the client tries to not send login methods in parallel with other methods).

The Meteor Accounts package tracks DDP connections associated with resume tokens and disconnects them if the resume token associated with that connection is deleted from the database.

Personal opinion: having auth just be "another method" wasn't the best idea. It works better if it's an established-at-beginning-of-connection, disconnect-to-change thing. However, the general idea of having a way for changes to authn/authz to "rerun publishers" or "disconnect connections" is nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@glasser I'll merge the PR and then maybe you could PR your comment on top of it to add it to the right place?
I'm actually not sure what's the best format for collaborating on this, maybe comments on this PR is the best place? @helfer what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mmm I'll merge now anyway, at least so we'll have the folder itself.
we can still keep the discussion here or on related PRs that people do to it

@Urigo Urigo merged commit 01f4922 into master Dec 2, 2016
@Urigo
Copy link
Contributor Author

Urigo commented Dec 2, 2016

@DxCx feel free to add your comments in a PR

Copy link

@DxCx DxCx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

Changing the websocket interface to provide a callback that returns an GraphQLConfig object will solve the header issue since you will be able to send it on the ws upgrade request.
Then, we just need to solve token invalidation issue since token generation can be done using graphql mutation

@sullivanpt
Copy link

sullivanpt commented Dec 6, 2016

I'd really like to be able to pass headers (and origin for that matter) into the Client constructor.

I'm using simple session cookie authentication and I've got the graphql server and the subscriptionsServer listening on the same httpServer so they share cookies. Works in the browser but since I cannot attach cookies to subscriptions-transport-ws Client I cannot send cookies during a mocha integration test. Letting me pass a headers object into the client Constructor that gets passed to the underlying socket would solve my problem.

Currently my code checks the session cookie in the server onSubscribe callback (since the 3rd argument is the wsRequest object with cookies attached), which means authentication happens after the connection is accepted and multiple times per connection. It also means client origin checks aren't being performed. Ideally I'd like a server side onConnect prior to the webSocket accept to check the client origin and set up the session from the cookie. (As noted previously, this hook would enable non-cookie based authentication as well).

PSA: applications implementing cookie based WS authentication as above need to be aware of the CSRF vulnerability described in detail here (http://www.christian-schneider.net/CrossSiteWebSocketHijacking.html).

On a related note, a client side onSubscribe event is needed for integration tests too; although at least this can be monkey patched.

I'm happy to create a PR for these additions. Thanks

@glasser
Copy link
Member

glasser commented Dec 14, 2016

Added my comment in b3a6c8d

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants