Apollo-server: Throwing an AuthenticationError in context creation should return a 401

[pragone]

In the documentation there's an example that shows throwing an AuthenticationError in the context creation function as a way to deny access to the whole API. See here.

While this works, it returns a 400 error which is not correct because the request is not invalid, it's an Authentication related issue so it should return a 401 or 403 depending on whether an AuthenticationError is thrown or a ForbiddenError is thrown.

From the discussion in #1709, it's become clear that this is not a desirable behaviour always, meaning that if only some fields should be denied access the GraphQL spec indicates that it should give a partial response. But in this case we're limiting the issue to just the case where the error is thrown in the context creation function.

[abernix]

I commented more extensively in https://github.com/apollographql/apollo-server/pull/2269#issuecomment-465536880, but I'm afraid that trying to narrow the scope to errors sourced from context creation by special-casing them and treating them differently than the errors within resolver execution might result in a confusing duality for error handling.

For what it's worth, context creation also happens on non-HTTP requests, so trying to pair an HTTP status code with that will only work in some cases. I respect that this is a need of yours, and I think we can find a solution, but I think we need to keep this aligned with #1709 because the inconsistency, particularly for something as important as errors, might create additional confusion.

I think we need to fully adopt the fact that GraphQL errors and data might very well be mixed. I'll comment in #1709, but I think that we'll need to land on a more configurable surface where you can opt into this behavior if you want it, but not be so firm with this being the best direction for everyone. Many would not agree with this approach, and I think the discussion in #1709 has made it clear that one-size-fits-all likely won't work well for this scenario.

Thanks for your input on the matter, it's been very helpful in pushing forward and solidying the thinking around this!

[pragone]

Hey @abernix .... commented on the PR too and thanks for the info... I get it.

One last thing I just wanted to call out is that the example on the documentation re: error handling in apollo-client then is a bit misleading. If you look at https://www.apollographql.com/docs/react/advanced/network-layer.html#linkAfterware
The example shows the client expecting a 401 as the result of a call to apollo and based in this conversation that's not really going to happen. It should instead inspect the GraphQL errors because the network layer will actually just return a 400.
Makes sense?
Hope this helps make this product better 👍

[theGlenn]

I agree with @pragone there is some asymmetry between information on ApolloServer and related client implementations.

If you look at the example below it states that the second case for .failure concerns "Network or response format errors", however error thrown as AuthenticationError will be handled as complete failures.

https://www.apollographql.com/docs/ios/fetching-queries/

apollo.fetch(query: HeroNameQuery(episode: .empire)) { result in
  switch result {
  case .success(let graphQLResult):
    if let name = graphQLResult.data?.hero?.name {
      print(name) // Luke Skywalker
    } else if let errors = graphQLResult.errors {
      // GraphQL errors
      print(errors)
    }
  case .failure(let error):
    // Network or response format errors
    print(error)
  }
}