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

Flexible Authentication Hook #9

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

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.

@@ -159,15 +170,31 @@ func (conn *connection) readLoop(ctx context.Context, send sendFunc) {

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