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

Unhandled Exception on Invalid Refresh Token #28

Closed
ryuuzake opened this issue Jan 19, 2022 · 5 comments
Closed

Unhandled Exception on Invalid Refresh Token #28

ryuuzake opened this issue Jan 19, 2022 · 5 comments
Labels
bug Something isn't working

Comments

@ryuuzake
Copy link
Contributor

The Interceptor used for refreshing the token has an unhandled exception. It happens when the server gives error 401 on an invalid refresh token.

Here's the interceptor

Future<void> refreshExpiredTokenInterceptor(
    RequestOptions options,
    RequestInterceptorHandler handler,
  ) async {
    ...

    final response = await manuallyRefresh();
    if (response?.accessToken != null) {
      options.headers['Authorization'] = response!.accessToken;
    } else {
      options.headers.remove('Authorization');
    }

    return handler.next(options);
  }

Here's the method that throws the exception.

Future<AuthResponse?> manuallyRefresh() async {
    ...

    try {
     ...
    } catch (e) {
      client.unlock();
      throw DirectusError.fromDio(e);
    }
    ...
  }

I think it should handle the logout automatically after an invalid refresh token.

@ryuuzake
Copy link
Contributor Author

My idea for a quick fix is by deleting the token to force the user login again to get a new access token and refresh token.

For the exception, I try to remove it because there's no way to handle it when using this package. Dio Interceptor won't rethrow the DirectusError.

Future<AuthResponse?> manuallyRefresh() async {
    ...

    try {
     ...
    } catch (e) {
      currentUser = null;
      tfa = null;
      tokens = null;
      await _emitter.emitAsync('logout', null);
      client.unlock();
      return null;
    }
    ...
}

@apstanisic
Copy link
Owner

I'm not sure if we should logout user on error. But it's definitely a bug if we can't handle exception.
manuallyRefresh should not ignore errors, since user can call that method directly, and is a problem if instead of a new access token, he/she was logged out.
I think the problem is with the interceptor, it's his job to handle errors.
What do you think about this? It should properly throw error, but I haven't tested it yet.

// Inside `refreshExpiredTokenInterceptor`
// line 174
   try {
      final response = await manuallyRefresh();
      if (response?.accessToken != null) {
        options.headers['Authorization'] = response!.accessToken;
      } else {
        options.headers.remove('Authorization');
      }
    } catch (e) {
      return handler.reject(DioError(requestOptions: options, error: e));
    }

@apstanisic apstanisic added the bug Something isn't working label Jan 19, 2022
@ryuuzake
Copy link
Contributor Author

When I think about it again, I agree with you. We shouldn't immediately log out the user. I didn't think thoroughly earlier.

Yeah, I think it's a good idea to let manuallyRefresh still throw Exception. But the Exception from it is DirectusError, can it be converted back to DioError? (I don't know about that).
I'll try testing your solution and make some tests around it.

@apstanisic
Copy link
Owner

Not currently, but we can add DioError? dioError property, since most of the errors are from http, and assign it when using fromDio constructor. Can you open a PR?

@apstanisic
Copy link
Owner

Closed in #29

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants