GraphQL extension does not handle invalid authorization tokens

[chschroe74]

Describe the bug

When we add authentication to a GraphQL endpoint (as Phillip Krüger did in his example project), we should get an HTTP 200 response with a valid JSON document if authentication fails (e.g., because of a missing token).

However, if an unparseable token is provided, we instead get an HTTP 401 response with no body. This behavior is inconsistent and makes it difficult to handle authentication failures in the client.

Expected behavior

We should get the same HTTP 200 response with an exception inside for any possible authentication failures. It should not matter if authentication fails because of a missing, invalid, or unparseable token or because of insufficient permissions (i.e. authorization).

Actual behavior

No response

How to Reproduce?

Use the example at https://github.com/phillip-kruger/graphql-experimental/tree/main/security-example

Pass an unparseable bearer token, like "foobar".

Output of uname -a or ver

No response

Output of java -version

openjdk version "17" 2021-09-14 // OpenJDK Runtime Environment (build 17+35-2724) // OpenJDK 64-Bit Server VM (build 17+35-2724, mixed mode, sharing)

GraalVM version (if different from Java)

No response

Quarkus version or git rev

2.10.0.Final

Build tool (ie. output of mvnw --version or gradlew --version)

No response

Additional information

No response

[quarkus-bot]

/cc @jmartisk, @phillip-kruger

[sberyozkin]

@chschroe74 401 is an authentication error, which seems correct or is GraphQL always expected to return 200 with the failure details as a response ?

[phillip-kruger]

Have you tried adding quarkus.http.auth.proactive=false ? Let us know if that changes anything

[chschroe74]

@sberyozkin

401 is an authentication error, which seems correct or is GraphQL always expected to return 200 with the failure details as a response ?

To be honest, I am not sure. I did not find anything in the GraphQL specs about other HTTP response codes. http://spec.graphql.org/October2021/#sec-Errors describes how errors should be returned, and this is how the current implementation behaves when the token is missing or has expired.

However, I would at least expect the behavior to be consistent. It should either always return an HTTP 401 when authentication fails, or it should always return an HTTP 200 with a JSON body that shows the error.

[chschroe74]

Have you tried adding quarkus.http.auth.proactive=false ? Let us know if that changes anything

The behavior is the same from the client's perspective: I still get an empty 401 response when I provide an invalid token.

However, the server-side behavior seems to change: With proactive authentication enabled (the default behavior), I do not see anything in the logs when I provide an unparseable token. With proactive authentication disabled, I first get an AuthenticationFailed exception:

2022-06-29 09:10:29,963 ERROR [io.sma.graphql] (vert.x-eventloop-thread-2) SRGQL012000: Data Fetching Error: io.quarkus.security.AuthenticationFailedException
...
Caused by: io.quarkus.security.AuthenticationFailedException
...

I then see an IllegalStateException ("Response head already sent"):

2022-06-29 09:10:29,977 ERROR [io.qua.mut.run.MutinyInfrastructure] (vert.x-eventloop-thread-2) Mutiny had to drop the following exception: java.lang.IllegalStateException: Response head already sent

I had played around with authentication before in another project, and I encountered the same behavior. It seems that the authentication layer does not properly integrate into the reactive stack.

[phillip-kruger]

Ok this is good info. I'll look into this a.s.a.p

[sberyozkin]

IMHO 401 should be returned if the authentication fails, it sounds like it is an invalid issue. The issue appears to be about sending a request without a token to the services which requires authentication where 200 is returned but it may no longer be the case in Quarkus.

[phillip-kruger]

No I agree with @chschroe74 that we should handle it like all other errors. We should be able to do that. I'll have a look a.s.a.p

We follow the GraphQL over HTTP spec (see the Readme in the Smallrye project) and there we should basically always return a 200 with the error. Or at least we should be consistent. We can probably later introduce a config that use HTTP response codes or not (not being the default). So for now, like everything else, it should be not. So this needs a fix.

[sberyozkin]

@phillip-kruger Sure, if 200 alongside the error info in the response is how GraphQL handles it for HTTP errors then indeed +1 to that.

[phillip-kruger]

Yea. I'll do some work on this a.s.a.p, and might need your help @sberyozkin . I'll ping you at that time

[sberyozkin]

Sure

[chschroe74]

@phillip-kruger I saw some updates to the GraphQL extension in the last couple of releases, but I did not notice anything related to this issue (and the status is still "open"). Is it still something that is on the agenda, or should I better close the issue?