Skip to content

Conversation

fproulx-dfuse
Copy link

According to Apollo Authentication Over WebsSocket spec (i.e. https://www.apollographql.com/docs/graphql-subscriptions/authentication) the authentication credentials shall be passed in the connection_init message payload as authToken.

In order to support this - and - more flexible authentication / authorization schemes which may require inspection of HTTP request headers in addition of the message payload, we add this optional onConnect hook.

switch msg.Type {
case typeConnectionInit:
var initMsg initMessagePayload

Copy link
Member

Choose a reason for hiding this comment

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

No need for empty line here. Please, remove it.

continue
}
}
conn.authenticated = true
Copy link
Member

Choose a reason for hiding this comment

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

So if authenticateFunc is nil the connection is marked as authenticated? Why is that? This doesn't sound ok to me. Am I missing something? Should you move this line

conn.authenticated = true

inside of the if statement above it?

send("", typeConnectionAck, nil)

case typeStart:

Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary empty line. Please, remove it.

@pavelnikolov
Copy link
Member

@fproulx-dfuse could you, please, use the opts to provide the authFunc as an option?

@pavelnikolov
Copy link
Member

Also, the code needs to be rebased.

ws wsConnection
authenticated bool
authenticateFunc AuthenticateFunc
request *http.Request
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need the request as a field of the connection?

@pavelnikolov
Copy link
Member

@fproulx-dfuse I just figured out that you can use a custom ContextGenerator and use it to access the request and its headers and store the auth info in the current context.

@tot-ra
Copy link

tot-ra commented Jun 9, 2021

any progress on this? @fproulx-dfuse
@pavelnikolov maybe someone can take this PR over & finish it?

@pavelnikolov
Copy link
Member

@tot-ra contributions would be accepted. This PR is not in a mergeable state.

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