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

feat: Handle websocket authentication via initial payload #918

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

alexandra-c
Copy link

@alexandra-c alexandra-c commented Jul 4, 2024

Fixing #915

  • Added a new authentication.Authenticator implementation that handles only the authentication via initial payload, websocketInitialPayloadAuthenticator.
  • Refactored and renamed jwksAuthenticator into httpHeaderAuthenticator because it only handles the authentication via header.
  • Extracted the jwt handling code in a separate interface in order to reuse it in both authenticators.

Motivation and Context

Cosmo router's built-in authentication currently looks for the JWT token only in the request header. For websocket requests made from a browser, headers cannot be altered, so the token can only be sent via URL Query, Cookie, or initial payload.

Currently, if the router configuration is set to authorization >> require_authentication: true, websocket requests will receive an Unauthorized response, causing Subscriptions to fail.

Cosmo router's websocket authentication behavior needed to be updated to allow the JWT token to be acquired from the initial payload as well as the header.

TODO

@jensneuse
Copy link
Member

@alexandra-c if a client authenticates via initial payload, is this then forwarded as initial payload to a subgraph subscription, via header, or both?

Subsequently, what about sub requests via http to other subgraphs, e.g. to join data to the subscription. In this case I would expect that we're able to map the initial payload field to a subgraph request header. This might be Authorization, but some services, e.g. on AWS cannot use this header name, so it needs to be configurable.

Have you thought about the two use cases?

@alexandra-c
Copy link
Author

@alexandra-c if a client authenticates via initial payload, is this then forwarded as initial payload to a subgraph subscription, via header, or both?

The initial payload is being sent to the subgraphs by default, so it will only be on the initial payload since right now no one is copying it to the header.

Subsequently, what about sub requests via http to other subgraphs, e.g. to join data to the subscription. In this case I would expect that we're able to map the initial payload field to a subgraph request header. This might be Authorization, but some services, e.g. on AWS cannot use this header name, so it needs to be configurable.

Have you thought about the two use cases?

I haven't really, I tested it right now and indeed we might need to copy the token to the headers also, for the sub requests. Not sure how the configuration should look like or where to put it... I think the header where we'd copy the token should be the same we configured in the authentication section, but there we actually have an array.

How do you suggest I should implement this?

@alexandra-c alexandra-c marked this pull request as draft July 5, 2024 07:44
@alexandra-c alexandra-c marked this pull request as ready for review July 8, 2024 11:23
@alexandra-c alexandra-c changed the title [DRAFT] Handle websocket authentication via initial payload Handle websocket authentication via initial payload Jul 8, 2024
@alexandra-c alexandra-c changed the title Handle websocket authentication via initial payload feat: Handle websocket authentication via initial payload Jul 9, 2024
@alexandra-c
Copy link
Author

Hey @jensneuse, I think I finished the implementation here, can you take a look? 😊

router-tests/testenv/testenv.go Show resolved Hide resolved
router-tests/websocket_test.go Outdated Show resolved Hide resolved
router-tests/websocket_test.go Show resolved Hide resolved
router-tests/websocket_test.go Show resolved Hide resolved
router/core/websocket.go Outdated Show resolved Hide resolved
router/core/websocket.go Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants