-
Notifications
You must be signed in to change notification settings - Fork 74
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
Comments
Related: wp-graphql/wp-graphql#192 |
@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 |
I think a model like the WP REST framework would be simple and work well. They have a hook |
@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. |
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 |
Awesome! Thanks for sharing. I’m optimistic we can have this wrapped up tomorrow 🤞 |
@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. |
… 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.
@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? |
@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 |
@jasonbahl sure, here you go: First I get a Token and it works: Then I query user and it works: And last I query posts. a post named "test" is public and post "hello world" is private. I got response 200 and below result Strangely when I log in to Wordpress and try to view this private post as REST it also doesn't work. |
I'm trying the same setup on a different server and I can't even log in... Is there any way to debug this? |
…69-jwt-could-not-be-returned-after-user-registered # Conflicts: # wp-graphql-jwt-authentication.php
Closed by 9df6cf5 |
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 aWP_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
The text was updated successfully, but these errors were encountered: