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

should be able to inject error into response if token refreshing fails #48

Open
antholeole opened this issue May 17, 2021 · 3 comments
Open
Assignees
Labels
question Further information is requested

Comments

@antholeole
Copy link

antholeole commented May 17, 2021

Hi there! This is by far the most intuitive package for refreshing tokens in graphql. Here is one enhancement that I would like to see:

in a case like this:

refreshToken: (token, client) async {
        await tokenManager.refresh();
}

if there is an error in refreshing the token, it is not possible to get it to the client; any thrown error here is "swallowed". If the api looked something like this:

refreshToken: (token, client, resp) async {
        await tokenManager.refresh();
}

then you could do something like:

refreshToken: (token, client, resp) async {
       try {
        await tokenManager.refresh();
       } catch {
          resp.errors.add(GraphQLError(message: 'Error refreshing access tokens`));
      }
}

and then it would be easy to log the user out or similar in a situation like this. Right now, I'm just assuming if response.hasErrors && response.errors == null then this is an error in the refresh token, which is obviously not a robust assumption (as it's possible that the response would have errors but no errors have been added for other reasons.)

I don't think it's an unfair assumption to believe that for some reason, a token refresh could fail: for instance, if a refresh token is compromised manually resetting it on the server would cause the refresh to fail; in a situation like this, I probably want to log the user out.

Thanks for the package! It helps a lot in my development.

@felangel felangel self-assigned this May 17, 2021
@felangel felangel added the question Further information is requested label May 17, 2021
@felangel
Copy link
Owner

Hi @antholeole 👋
Thanks for opening an issue and for the positive feedback!

You should be able to throw a RevokeTokenException within refreshToken to trigger a logout. Let me know if that helps 👍

@antholeole
Copy link
Author

Awesome - thanks so much, does exactly what I need.

@antholeole
Copy link
Author

Hi there - sorry to reopen the issue, but as I was writing unit tests I realized I misunderstood the RevokeTokenException. To me, it looks like it doesn't do anything - taking a look at the code in fresh_link:

         } on RevokeTokenException catch (_) {
            unawaited(revokeToken());
            yield result;
          }

it seems that all it does is yield the response already given and nothing else.

Is the intended use case something like this:

  1. Server says "JWT expired", returns JWT expired error code
  2. client attempts refresh, it fails, RevokeTokenException is thrown.
  3. Whenever doing any error handling, if the error is the GQL response is JWT expired error, then that means the refresh failed and log the user out.

If I understand this correctly, it would still feel better if the RevokeToken exception would be pushed onto the GQL response errors so we know that what happened is the keys got revoked, and we can handle the revoked token directly.

The second thing here is: if a user's keys gets revoked, and then he gets logged out (FreshLink is now in unauthenticated mode) how do we get freshlink back in authenticated mode? At this point, the GQL client was already created and fresh link was already added to it; I don't think it's possible to do freshLink.setToken anymore as it's embedded in the gql client.

As an example for q2:

Future<Client> buildGqlClient() async {
      final refreshLink = FreshLink.oAuth2(...)
     refreshLink.setToken(...) //easy to call set token here, we can access freshLink directly
    final link = Link.from([
        refreshLink,
       HttpLink(...)
    ]);
    return Client(link: link, cache: await buildCache());
}

now, unless we return a reference to the link in the client, it's impossible to setTokens on the client.

I think it may be better to manually override fresh_link so always be in "authenticated mode": that way, if there are no tokens it tries to grab them from the usual methods.

Thanks for your work on this package.

@antholeole antholeole reopened this May 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants