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

Invalid or expired JWT tokens should prevent request from being serviced #38

Closed
mlipscombe opened this issue Jul 14, 2019 · 12 comments
Closed

Comments

@mlipscombe
Copy link

Currently, the JWT plugin returns a 403 status when an invalid or expired JWT token is sent in the Authorization header, but it does not stop processing the request. I think this is logically and semantically incorrect, and that no processing of the request should take place.

The current approach prevents a retry handler being used on the client side to refresh a token and re-attempt the query without deep understanding of every query or mutation that was executed, because the query or mutation may not fail, but may fail to return the expected data (for example, a customer's orders) or fail to execute the mutation in the way intended (for example, creating an anonymous comment vs a comment from an authenticated user).

Aborting the request all together is, IMHO, the path of least surprise. It allows a quick recovery when a token has expired (reducing round trip time because less processing has to be done on the 403 response), and the client is not left trying to figure out what did work, what didn't work and what had an unintended side effect.

WP REST has the [rest_authentication_errors](https://developer.wordpress.org/reference/hooks/rest_authentication_errors/) hook that authentication plugins can return a WP_Error with that will stop processing. I think that this is a good pattern to follow.

Other GraphQL implementations fail without processing the request on invalid/expired authentication credentials. Some examples: Postgraphile, Prisma

@CodeProKid
Copy link
Member

Related: wp-graphql/wp-graphql#192

@jasonbahl
Copy link
Collaborator

@mlipscombe ok, I think I'm mostly sold on invalid auth attempts just not executing at all.

The issue I have now, is determining the best approach for handling this.

It seems that it's a bit broader than just this JWT Auth plugin. We can patch something in here for the short-term, but there are dozens of ways to authenticate with WordPress, and I feel like the core WPGraphQL plugin should have some responsibility in identifying if the request is a failed attempt at authenticating.

For example, if the request is on same domain and the request is using cookie auth, but the cookie is no longer valid, then WPGraphQL should have some way of detecting that the request is an attempt at an authenticated request, but not a successful one, and could shut down execution before it even begins.

🤔

There definitely seems to be some core WP responsibility here as well. Perhaps there's already some WP Core mechanism (global or something) that provides this info. . .to my knowledge, no such thing exists though.


So, short-term, I think we can have this plugin prevent authentication if the AuthToken is passed but invalid, and can maybe open an issue on the core WPGraphQL plugin to look into handling this for wider use cases

@mlipscombe
Copy link
Author

I think a model like the WP REST framework would be simple and work well.

They have a hook rest_authentication_errors which plugins can use to return a WP_Error, which, in turn, will prevent the request. We could have graphql_authentication_errors.

https://github.com/WordPress/WordPress/blob/master/wp-includes/rest-api/class-wp-rest-server.php#L326-L330

@jasonbahl
Copy link
Collaborator

@mlipscombe ya, that could work. I'll think through things a bit. I have things technically working right now by making some changes to this JWT Auth plugin, but not super happy with it at the moment. I'll revisit in the morning and see what I can come up with.

Do you have any example client side code that fixing this feature will benefit from? Might be good for me to get eyes on how a consumer will handle things if the status is 403 and nothing is executed vs. the current way where some fields won't execute but some will.

@mlipscombe
Copy link
Author

Sure, here is some example client-side code using Apollo to refresh the token on authentication failure:

const retryLink = new RetryLink({
  delay: {
    initial: 0,
  },
  attempts: {
    max: 2,
    retryIf: async err => {
      switch (err.statusCode) {
        case 403:
          // token is invalid, try and renew
          setAccessToken(null);
          try {
            await refresh();
          } catch (error) {
            return false;
          }
          return true;
        default:
          return true;
      }
    },
  },
});

const client = new ApolloClient({
    link: ApolloLink.from([
        retryLink,
        setContext((_, { headers }) => ({
            headers: {
              ...headers,
              Authorization: accessToken ? `Bearer ${accessToken}` : '',
            },
        })),
        new HttpLink({
            uri: 'https://url',
        }),
    ])
});

The Apollo RetryLink will intercept any network errors, including the 403. If it's a 403, it will refresh the token, and then retry the operation, now with a valid authentication header.

@jasonbahl
Copy link
Collaborator

Awesome! Thanks for sharing. I’m optimistic we can have this wrapped up tomorrow 🤞

@jasonbahl
Copy link
Collaborator

@mlipscombe ok, after chatting with a few other people and digging a bit more, I'm totally cool with the 403 returning and not executing any operations.

I'll have a PR here shortly.

jasonbahl added a commit that referenced this issue Jul 19, 2019
… serviced

- This validates the Auth Token when the GraphQL Request is being initiated, and if it's invalid the request does not execute and a 403 status is returned.
@BartoszLobejko
Copy link

@jasonbahl Just to let you know, that after pulling recently updated version of this plugin, finally resolve my issue. I'm able now to authenticate and use the token to query viewer and get restricted data. However, if I try to query posts and one of them is set as "Private" I get response 200 but Private posts are not there...

Any ideas why?

@jasonbahl
Copy link
Collaborator

@bartosz, I’ll need more info to investigate. Can you provide me with example queries that don’t seem to be resolving properly and info on the capabilities of the user being used, the code used to send the auth header, etc

@BartoszLobejko
Copy link

BartoszLobejko commented Jul 25, 2019

@jasonbahl sure, here you go:
https://staging.api.lobejko.com/graphql
User is Admin.

First I get a Token and it works:
mutation LoginUser { login( input: { clientMutationId:"uniqueId" username: "{{username}}" password: "{{password}}" } ) { authToken refreshToken user { id name } } }

Then I query user and it works:
query GetViewer { viewer { id username email capabilities extraCapabilities isRestricted jwtAuthToken jwtRefreshToken jwtUserSecret roles { nodes { name id } } } }

And last I query posts. a post named "test" is public and post "hello world" is private.
This only returns Test even if I use authentication query the same way as for previous "viewer" query.
query GetPosts { posts { nodes { slug content } } }

I got response 200 and below result
{ "data": { "posts": { "nodes": [ { "slug": "test", "content": "<p>Cum sociis natoque penatibus et magnis dis parturient montes, nascetur ridiculus mus. Cum sociis natoque penatibus et magnis dis parturient montes, nascetur ridiculus mus. Praesent commodo cursus magna, vel scelerisque nisl consectetur et. Cum sociis natoque penatibus et magnis dis parturient montes, nascetur ridiculus mus.<\/p>\n" } ] } } }

Strangely when I log in to Wordpress and try to view this private post as REST it also doesn't work.
https://staging.api.lobejko.com/wp-json/wp/v2/posts/1

@BartoszLobejko
Copy link

I'm trying the same setup on a different server and I can't even log in... Is there any way to debug this?

jasonbahl added a commit that referenced this issue Feb 17, 2020
…69-jwt-could-not-be-returned-after-user-registered

# Conflicts:
#	wp-graphql-jwt-authentication.php
@jasonbahl
Copy link
Collaborator

Closed by 9df6cf5

This was referenced Feb 17, 2020
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

No branches or pull requests

4 participants