99designs / gqlgen

go generate based graphql server library
https://gqlgen.com
MIT License
9.9k stars 1.15k forks source link

websocket not close if initFunc returns an error #2476

Open jjauzion opened 1 year ago

jjauzion commented 1 year ago

What happened?

When the initFunc of the transport.Websocket{} return an error the websocket is not close but spam the init function

What did you expect?

The websocket is closed and only one request is made to the serveur

Minimal graphql.schema and models to reproduce

you can use the websocket example from gqlgen repo

versions

More

You can run the code of the example as is or just add a debug print to make the things clear in the websocketInit:

func webSocketInit(ctx context.Context, initPayload transport.InitPayload) (context.Context, error) {
    // Get the token from payload
    any := initPayload["authToken"]
    token, ok := any.(string)
    if !ok || token == "" {
        fmt.Println("error in init")
        return nil, errors.New("authToken not found in transport payload")
    }

    // Perform token verification and authentication...
    userId := "john.doe" // e.g. userId, err := GetUserFromAuthentication(token)

    // put it in context
    ctxNew := context.WithValue(ctx, "username", userId)

    return ctxNew, nil
}

and then just run a query in the playground without any auth header:

subscription test {
  subscribe(subscriber:"jj")
}

the browser will be hanging in a loading cycle and the server is looping in the init function:

➜  websocket_gqlgen ./server                                  
2022/12/20 09:40:56 connect to http://localhost:8080/ for GraphQL playground
error in init
error in init
error in init
error in init
error in init
...
jakezhu9 commented 1 year ago

Hi, Thank you for bringing this issue to my attention. As you mentioned, it seems that when initFunc returns an error, the connection is closed with a normal close code, causing the browser to continuously attempt to reconnect. https://github.com/99designs/gqlgen/blob/5cb6e3ecb07a292daa37f5ce8e5bcf364e1190af/graphql/handler/transport/websocket.go#L182 I think that returning an auth error in initFunc may not be the best approach, as the error may not necessarily be related to the connection layer and may not be handled properly by most clients. Instead, it might be better to save the auth information in the context and throw an error in the resolver, similar to what is done in Apollo Server.

reference: https://www.apollographql.com/docs/apollo-server/data/subscriptions/#example-authentication-with-apollo-client

jjauzion commented 1 year ago

Hi, Thanks for your answer. So I did refactored my code so that authentication is made in the resolver and not in the initFunc which looks better, thanks for the advice.

However, I was wondering if I could have a "resolver middleware" to implement this authentication so that I don't forgot it when writing resolver functions. I found something called RootFieldMiddleware that might be it, but couldn't find it in the gqlgen documentation.

Can it be used to implement authentication ? Do you know where I can find some documentation / examples ? (I can create a sperate issue/question as this is not exactly the same topic as the original post)

rootkea commented 1 year ago

@jakezhu9

I think that returning an auth error in initFunc may not be the best approach

But the gqlgen authentication recipe is performing the authentication in WebsocketInit() only. Even returning the auth error from there.

The issue of InitFunc not returning an error to the client should be fixed. And also proper auth error (3000) should be returned when auth fails.

This issue is tracking that.

jjauzion commented 1 year ago

@rootkea

Another issue with this authentication recipe is that authentication is done twice: in the middleware and in the webSocketInit function. So when a websocket init connection arrives to server it goes first through the middleware and then the initFunc. And the first authentication (in the middleware) "fails".

I feel much better having my middleware and initFunc just copying the auth token into the context and then validating this auth token in the resolver

rootkea commented 1 year ago

@jjauzion

So when a websocket init connection arrives to server it goes first through the middleware and then the initFunc.

No, websocket doesn't use http middleware and hence we need auth in InitFunc. For websocket auth happens only in webSocketInitFunc.

And for the initial request before switching the protocol to wss we have this in auth recipe in Middleware:

if err != nil || c == nil {
    next.ServeHTTP(w, r)
    return
}

What I've done is: abstract away common auth handling in a separate function verifyToken which gets called by both webSocketInit() and Middleware.