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

throwing AuthenticationError should set the response status to 401 Unauthorized #1709

Closed
dandv opened this issue Sep 22, 2018 · 33 comments
Closed
Labels
🌹 has-reproduction ❤ Has a reproduction in Glitch, CodeSandbox or Git repository.

Comments

@dandv
Copy link
Contributor

dandv commented Sep 22, 2018

apollo-server correctly returns a a "400 (Bad request)" error if the query fails to validate, but if the code throws an AuthenticationError per the docs, the status remains 200.

https://launchpad.graphql.com/z5n9190vz7

image

@ghost ghost added the 🌹 has-reproduction ❤ Has a reproduction in Glitch, CodeSandbox or Git repository. label Sep 22, 2018
@ruipaulo
Copy link

I have the same issue, but slightly different. I am using apollo-server-lambda and it returns status code 400 on throwing AuthenticationError. Instead, it should return 401.

@JakeDawkins
Copy link
Contributor

@dandv thanks for opening this! How are you wanting to use the 401, specifically? Why wouldn't the errors extension be enough?

From a design perspective, I have a couple thoughts. Since AuthenticationErrors can be thrown at any level, within any resolver, it wouldn't be great to have the whole operation scrapped for a single field's sake. It would remove the ability for partially-valid operations.

Also, if we changed the status for AuthorizationErrors, I'd imagine we'd need to change codes for any other kinds of error as well.

@dandv
Copy link
Contributor Author

dandv commented Sep 26, 2018

How are you wanting to use the 401, specifically? Why wouldn't the errors extension be enough?

One use case is that some 3rd party GraphQL clients are tripped by the 200 OK and don't look in the response object for an errors property.

Also, if we changed the status for AuthorizationErrors, I'd imagine we'd need to change codes for any other kinds of error as well.

Agree. This would bring apollo-server in line with standard HTTP error codes returned by REST APIs.

it wouldn't be great to have the whole operation scrapped for a single field's sake. It would remove the ability for partially-valid operations

I haven't run into that use case yet, but for the more typical use case where an operation needs to be entirely denied if unauthenticated or unauthorized, I think the dev should have the option to throw a genuine (401/403) auth* error. For partial validations, perhaps pass a flag somehow, or don't throw but return an error property?

@abelsoares
Copy link

@JakeDawkins, what's the rational behind HTTP status codes in apollo-server?

Is it that errors thrown at a point (include execution) where partial responses (data field may be available) are possible should return status code 200? And errors that make impossible to have partial responses (before execution) should generate an HTTP error status code?

If that's the rational, i think it's ok and makes sense that AuthenticationErrors have a 200 status code.

By the way, any reason why the response when a query validation fails is in this form:

{
  "error": {
    "errors": [...]
  }
}

Shouldn't be:

{
    "errors": [...]
}

?

@MrLoh
Copy link

MrLoh commented Dec 13, 2018

@JakeDawkins So in our case, we switched to using error extension codes, and I agree that apollo shouldn't generally handle all kinds if response codes. However our legacy app versions, still depend on a 401 being returned.

I think one valid improvement would be to at least return 401, where 400 is currently returned, when the context creation fails, since this is where global authentication logic lives, and it already returns 400, so apollo might as well return 401, if it knows that a authentication error occurred from the extension code.

if (typeof options.context === 'function') {
try {
(options.context as () => never)();
} catch (e) {
e.message = `Context creation failed: ${e.message}`;
// For errors that are not internal, such as authentication, we
// should provide a 400 response
if (
e.extensions &&
e.extensions.code &&
e.extensions.code !== 'INTERNAL_SERVER_ERROR'
) {
return throwHttpGraphQLError(400, [e], options);
} else {
return throwHttpGraphQLError(500, [e], options);
}
}

I created PR #2093, to give an idea, how this could work. It's not ready to merge, but I could provide tests etc., if this is something you would consider.

@pragone
Copy link

pragone commented Feb 4, 2019

Thanks @MrLoh. I think your fix was what was needed. Saw thought that you didn't finish it up, so I took the baton and finished it up in PR #2269
Please review, I've added tests, signed the contributor agreement, yada, yada, yada....

@danilobuerger
Copy link
Contributor

I don't think this is a great idea. As outlined by @JakeDawkins:

From a design perspective, I have a couple thoughts. Since AuthenticationErrors can be thrown at any level, within any resolver, it wouldn't be great to have the whole operation scrapped for a single field's sake. It would remove the ability for partially-valid operations.

If I request multiple fields and just one leads to an authentication error, it would be better to return the partial results instead of erroring the whole thing. This is in line with the GraphQL spec:

A response may contain both a partial response as well as encountered errors in the case that a field error occurred on a field which was replaced with null.

As per RFC 7235

The 401 (Unauthorized) status code indicates that the request has not
been applied because it lacks valid authentication credentials for
the target resource.

But it has been applied, just partially. So returning a 401 would be non-RFC compliant.

@pragone
Copy link

pragone commented Feb 5, 2019

Hi @danilobuerger ... I get your point.
I agree that GraphQL is meant to return partial answers and errors as you indicate, what @MrLoh proposed though is a bit different, because it's not about returning 401 on any occasion that an AuthenticationError is thrown, but specifically in the case that it's thrown in the creation of the context. This is a per-request action, and throwing an error there, which is shown as an example in the documentation should deny access to the whole API... therefore I think a 401 is appropriate and would make it more compatible

Actually, this was a separate issue #2093. And in this issue, the problem was specific to just throwing an authentication error during context creation.
It seems to me that perhaps merging the two issues was a mistake. Perhaps #2093 should be re-opened and my PR would focus on fixing that issue, and this issue should be closed indicating that an AuthenticationError thrown anywhere that is not the context creation is not to return a 401.

Makes sense?

@pragone
Copy link

pragone commented Feb 5, 2019

Sorry... it wasn't a separate issue... it was just a separate PR... I'll create a separate issue because I think that limiting the scope of the change is the right way to go about it.

@pragone
Copy link

pragone commented Feb 5, 2019

Ok... I've created #2275 to reflect the reduced scope. I'd suggest this one is closed as it seems it shouldn't be accepted.

@MrLoh
Copy link

MrLoh commented Feb 6, 2019

Exactly. This just throws a 401 where GraphQL already doesn’t return any data but just a 400

@abernix
Copy link
Member

abernix commented Feb 20, 2019

I commented in #2275 and quite extensively on #2269 (comment). The gist of those updates was:

  • I think consumers of a GraphQL server should fully embrace the fact that GraphQL supports partial failures (i.e. the presence of data and errors on a response). Special-casing the behavior so that an authentication behavior in a resolver behaves differently than an error in the context creation is likely to create confusion.
  • We need a way for those who desire other behavior to be able to opt-into this behavior.
  • Rallying around HTTP status codes is problematic for other transports like WebSockets, which are supported by Apollo Server, since they subscribe to completely different error codes.
  • Few HTTP status codes are capable of dealing with varied status responses. 207 being a strong "maybe".
  • Certain HTTP status codes (300-series and 500-series errors come to mind) do have very specific meaning in terms of transport errors which might make sense to support since they might determine whether or not a request needs to be retried, or redirected elsewhere. I believe hat behavior should be implemented at the transport level, not at the GraphQL request level. Other HTTP status codes (namely the 400-series errors) fall into the concerns of my previous bulleted concerns where they are better suited for GraphQL errors.
  • Again though, we need plumbing in place for those that need different behavior.

For more details, please check the linked PRs and issues above.

Strawperson

I'm thinking that we could cater to the needs of the few who would like to apply more heavy-handed "Forbidden"/"Unauthorized" determinations at the HTTP level by:

  1. Introducing a new life-cycle method to the request pipeline API (See also: Documentation for new request pipeline life-cycle hooks/p… #2008) called didEncounterErrors. Its signature might look like:

    didEncounterErrors(
      errors: GraphQLError[],
      requestContext: WithRequired<GraphQLRequestContext<TContext>, 'response'>
    )

    This would be invoked when there are GraphQLErrors present in the response. The requestContext would also be provided with a guarantee that response is present.

  2. Exposing the response status (number) on the http property of the GraphQLResponse interface:

    export interface GraphQLResponse {
    data?: Record<string, any>;
    errors?: GraphQLError[];
    extensions?: Record<string, any>;
    http?: Pick<Response, 'headers'>;
    }

    This would make the { response: { http: { status } } property available for use from the newly proposed didEncounterErrors, and also in the existing willSendResponse (which I suspect would be useful in similar ways):

    willSendResponse?(
    requestContext: WithRequired<GraphQLRequestContext<TContext>, 'response'>,
    ): ValueOrPromise<void>;

Roughly, I believe this would allow the following:

const server = ApolloServer({
  typeDefs,
  resolvers,
  plugins: [
    {
      requestDidStart: () => ({
        didEncounterErrors(errors, { response: { http } } ) {
          if (http && errors.some(err => err.name === "AuthenticationError")) {
            http.status = 401;
          }
        }
      })
    }
  ]
});

Thoughts?

@MrLoh
Copy link

MrLoh commented Feb 20, 2019

But the current implementation does throw 400 errors, when context creation fails. If the query fails globally during context creation and throws a 4XX error, it should throw the correct 4XX error. If the query fails locally in the resolver than it needs to return 200 and things need to be handled with error codes.

@dandv
Copy link
Contributor Author

dandv commented May 20, 2019

Also, if we changed the status for AuthorizationErrors, I'd imagine we'd need to change codes for any other kinds of error as well.

Agreed. From and end-developer's perspective, it just seems very counter-intuitive to, for example, validate user input in a resolver and throw an UserInputError that actually returns null data and 200 OK. This confuses HTTP API consumers that rely on HTTP status codes.

By contrast, query parsing errors correctly return 400 Bad Request.

A mechanism to opt into returning a specific error code would be great.

abernix added a commit that referenced this issue May 23, 2019
**Note:** This currently only covers error conditions.  Read below for details.

This commit aims to provide user-configurable opt-in to mapping
GraphQL-specific behavior to HTTP status codes under error conditions, which
are not always a 1:1 mapping and often implementation specific.

HTTP status codes — traditionally used for a single resource and meant to
represent the success or failure of an entire request — are less natural to
GraphQL which supports partial successes and failures within the same
response.

For example, some developers might be leveraging client implementations
which rely on HTTP status codes rather than checking the `errors` property
in a GraphQL response for an `AuthenticationError`.  These client
implementations might necessitate a 401 status code.  Or as another example,
perhaps there's some monitoring infrastructure in place that doesn't
understand the idea of partial successes and failures. (Side-note: Apollo
Engine aims to consider these partial failures, and we're continuing to
improve our error observability.  Feedback very much appreciated.)

I've provided some more thoughts on this matter in my comment on:
#2269 (comment)

This implementation falls short of providing the more complete
implementation which I aim to provide via a `didEnounterErrors` life-cycle
hook on the new request pipeline, but it's a baby-step forward.  It was
peculiar, for example, that we couldn't already mostly accomplish this
through the `willSendResponse` life-cycle hook.

Leveraging the `willSendResponse` life-cycle hook has its limitations
though.  Specifically, it requires that the implementer leverage the
already-formatted errors (i.e. those that are destined for the client in the
response), rather than the UN-formatted errors which are more ergonomic to
server-code (read: internal friendly).

Details on the `didEncounterErrors` proposal are roughly discussed in this
comment:
#1709 (comment)

(tl;dr The `didEncounterErrors` hook would receive an `errors` property
which is pre-`formatError`.)

As I opened this commit message with: It's critical to note that this DOES
NOT currently provide the ability to override the HTTP status code for
"success" conditions, which will continue to return "200 OK" for the
time-being.  This requires more digging around in various places to get
correct, and I'd like to give it a bit more consideration.  Errors seem to
be the pressing matter right now.

Hopefully the `didEncounterErrors` hook will come together this week.
@NajamShehzad
Copy link

Does anyone find a solution for this? I'm facing the same problem my server sending 400 error on authentication instead of 401

@abernix
Copy link
Member

abernix commented May 24, 2019

As of the latest 2.6.0 alpha, which was just published on the @next dist tag (Currently apollo-server@2.6.0-alpha.6; See #2669 for more details), and thanks to #2719 and #2714 (combined), it should be possible to set the HTTP status codes for the entire response based on attributes that the implementor chooses.

This can be accomplished using a plugin (defined in plugins) like this:

import { AuthenticationError } from 'apollo-server-errors';
import { ApolloServer } from 'apollo-server';
const server = new ApolloServer({
  typeDefs,
  resolvers,
  plugins: [
    {
      requestDidStart() {
        return {
          didEncounterErrors({ response, errors }) {
            if (errors.find(err => err instanceof AuthenticationError)) {
              response.http.status = 401;
            }
          }
        }
      },
    },
  ],
});

For more information on the plugins, check out the deploy preview on the #2008 PR.

@abernix
Copy link
Member

abernix commented May 24, 2019

Just to re-iterate what I've mentioned in the past. Some tooling and existing patterns may necessitate specific HTTP patterns which rally around a single HTTP status code. The 207 Multi-Status HTTP status code is the only code which has previously come close to accommodating partial failures and successes within the same request.

From and end-developer's perspective, it just seems very counter-intuitive to, for example, validate user input in a resolver and throw an UserInputError that actually returns null data and 200 OK. This confuses HTTP API consumers that rely on HTTP status codes.

GraphQL gives us the luxury of being free from that constraint and allowing for partial successes and failures mixed within the same response. This is a lovely benefit, but it does require developers to start thinking about the HTTP status codes in terms of transport errors, and not necessarily associating them to user errors or data validation errors.

That may seem counter-intuitive to you @dandv, but I'd claim it's not intuitive to fail an entire GraphQL request, which can and should be fetching as many fields as it can in one request, because of user-input (or lack of authentication) which violated one of those field's desires. Perhaps your concern might be partially because of past requirements and habits? I know that some tooling and infrastructure (e.g. reporting) relies on HTTP status codes, but it's certainly worth considering how those might need to be updated over time (in our minds, in our clients and in our tooling) to reflect the multi-status nature of GraphQL since, longer term, the results are of a great benefit to client consumers!

@abernix
Copy link
Member

abernix commented Jul 2, 2019

I believe that #1709 (comment) should provide the facilities to set this programmatically for those that require this functionality and was officially released into the final release of Apollo Server 2.6.0.

@abernix abernix closed this as completed Jul 2, 2019
@KATT
Copy link
Contributor

KATT commented Jul 25, 2019

The workaround provided in #1709 (comment) doesn't quite do it for me.

It seems to me that my response.data is null and not undefined when it reaches the following code:

if (response.errors && typeof response.data === 'undefined') {

I thought I could fix that by simply adding a || response.data === null-check but my attempt breaks a whole bunch of tests so I guess that's a no-go.

Also, I had to check err.originalError rather than err to get it to attempt to change the HTTP status - unsure if that's connected somehow.

Here's my plugin attempt:

const apolloServer = new ApolloServer({
  // [...]
  plugins: [
    {
      requestDidStart() {
        return {
          didEncounterErrors({ response, errors }) {
            if (errors.find(err => err instanceof AuthenticationError || err.originalError instanceof AuthenticationError)) {
              response!.data = undefined
              response!.http!.status = 401
            }
          },
        }
      },
    },
  ],
})

@abernix - any ideas?

@KATT
Copy link
Contributor

KATT commented Jul 25, 2019

I hacked together a workaround by making formatResponse setting data to undefined. It's ugly, but it works.

const apolloServer = new ApolloServer({
  // [..]
  formatResponse(body: GraphQLResponse) {
    if (body.errors && body.errors.find(err => err.extensions && err.extensions.code === 'UNAUTHENTICATED')) {
      return {
        ...body,
        data: undefined,
      }
    }

    return body
  },
  plugins: [
    {
      requestDidStart() {
        return {
          didEncounterErrors({ response, errors }) {
            if (errors.find(err => err instanceof AuthenticationError || err.originalError instanceof AuthenticationError)) {
              response!.data = undefined
              response!.http!.status = 401
            }
          },
        }
      },
    },
  ],
})

@airandfingers
Copy link

@KATT your workaround isn't working for me - formatResponse isn't even being called, only formatError.

Are you doing something else to force your server/context function to return a response early, rather than return an error at all?

@jonathanmv
Copy link

In my case I was able to set the response status to 401 (and the data to undefined) and even so the service returned 200. I was not able to get a 401 sent to the client

@MM3y3r
Copy link

MM3y3r commented Jan 14, 2020

We are having the same issue. Would love to create 401s but its always 200s we get.

@MM3y3r
Copy link

MM3y3r commented Jan 16, 2020

I did not manage to get the didEncounterError hook to do what i wanted so i used willSendResponse to modify my http response:

const customError401Plugin: ApolloServerPlugin = {
	requestDidStart: () => ({
		willSendResponse({ errors, response }) {
			if (response && response.http) {
				if (
					errors &&
					errors.some(
						(err: GraphQLError) => err.name === 'AuthenticationError' || err.message === 'Response not successful: Received status code 401'
					)
				) {
					response.data = undefined;
					response.http.status = 401;
				}
			}
		},
	}),
};

Maybe this helps someone.

@dorthwein
Copy link

@MM3y3r - THANK YOU!!!!

@ryansolid
Copy link

This behavior is incredibly problematic. Not returning 401 and not being able to reasonably patch it is simply broken from an http point of view. Partials I agree and am all for general but that does not make sense when you are rejecting the request wholistically because they aren't authenticated. Ie.. from lookup in context. Apollo Server plugin infrastructure doesn't hook in to that point as it is resolved before requestDidStart and trying to wrap it yourself atleast in the apollo-server-lambda context means trying to parse already formulated strings. This is broken.

@arizonatribe
Copy link

The trick seems to be what @MM3y3r identified. Apparently you have to set data to undefined first, even though that seems to have been left out of the conversation on the feature when it was added.

Can't believe sometimes it requires source-diving through their massive codebase to figure out why the things don't work the way the docs or emoji-rich medium blog posts say they do.

@stoicskyline
Copy link

stoicskyline commented Apr 5, 2020

Alternatively, to handle from client-side:

const errorLink = onError(errorResponse => {
  if (errorResponse?.graphQLErrors?.some(e => e.extensions?.code === 'UNAUTHENTICATED')) {
    // handle the auth error
  }
})

client = new ApolloClient({
  cache,
  link: errorLink.concat(httpLink),
  // ...

// Can also check graphQLErrors in each individual query/mutation

@anlauren
Copy link

There should be an easy way to chose how to catch these errors and chose how we want to deal with them in the network, we already have a way to format the errors how we like, why don't we have access to the http there to return whatever status code we feel like?
A lot of tool have been built for ages around these status codes and different non graphql application will need such integrations

@un33k
Copy link

un33k commented Jun 10, 2021

@Jorgechen the client solution is not viable in combination with the retry link. This is due to the fact that retryLink works at Network Error level and onError only makes the network & gql errors present. However, if you want to trigger the retryLink to let's say refreshToken, then you'd have to raise an exception.

@warent
Copy link

warent commented May 28, 2022

For anyone considering @MM3y3r's solution, keep in mind it doesn't work if you're trying to sever the request before it processes. For example, it will still run through all your resolvers

@gaurav1999
Copy link

But the current implementation does throw 400 errors, when context creation fails. If the query fails globally during context creation and throws a 4XX error, it should throw the correct 4XX error. If the query fails locally in the resolver then it needs to return 200 and things need to be handled with error codes.

Is there a doc reference to this ?, If I throw any error while context creation I am always getting HTTP 400, and also not able to modify it using the Plugin mechanism specified in the docs.

@glasser
Copy link
Member

glasser commented Aug 29, 2022

This is not configurable in Apollo Server at the moment, but I just implemented a supported and documented way to control this for Apollo Server 4: #6857

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🌹 has-reproduction ❤ Has a reproduction in Glitch, CodeSandbox or Git repository.
Projects
None yet
Development

Successfully merging a pull request may close this issue.