Potential issue with error handling in TokenManager

[ScheckGuillermo]

Problem:

In the TokenManager class, there is an issue related to error handling during the token refresh process. Currently, the access token is refreshed at regular intervals using setInterval. However, this process lacks proper error handling. If the refresh request fails (e.g., due to a 400 or 500 response), an unhandled exception is thrown. This can lead to a node unhandledException and potentially cause the entire application to crash.

  //Create timed task to refresh token
    const marginInSeconds = 60;
    if (
      this.accessTokenExpireTime != undefined &&
      this.refreshTokenExpireTime != undefined
    ) {
      const accessTokenInterval = this.accessTokenExpireTime - marginInSeconds;
      const refreshTokenInterval =
        this.refreshTokenExpireTime - marginInSeconds;
      if (accessTokenInterval > refreshTokenInterval) {
        setInterval(() => {
          this.refreshAccessToken();
        }, refreshTokenInterval * 1000);
      } else {
        setInterval(() => {
          this.refreshAccessToken();
        }, accessTokenInterval * 1000);
      }
    }

Temporary Fix

To address the issue temporarily, we can put the refresh request inside a try-catch block within the setInterval. This way, any errors that occur during the token refresh will be caught and handled, preventing the application from crashing due to unhandled exceptions.

Proposed Solutions

1. Design Simplification: For a more robust solution, I suggest revisiting the overall design of the token management system. The current implementation uses the “subject and observers” pattern, where the TokenManager ensures that all its subscribers (UserManager, ClientManager, RealmManager) always have a valid token. An alternative approach could be to refresh the token on demand—only when an API request fails due to an expired token. This would simplify the codebase by eliminating the need to maintain a constantly valid token. One way to achieve this is by creating a KeycloakClient class that handles all API interactions, including token refresh when needed. Alternatively, you can simply add an axios interceptor to automatically manage token refreshes when an API request fails.
2. Enhanced Error Notification: If the "subject and observers" pattern is maintained, another potential solution is to incorporate a mechanism in TokenManager that notifies its subscribers when the token refresh fails. This would enable UserManager, ClientManager, and RealmManager to handle the error properly and take necessary actions

[euscanga-krasamo]

Hi @ScheckGuillermo!

Thanks for addressing this issue. We agree with you. This is a potential issue. The client project cannot handle this error and it will crash the project. The intention of not handling these errors was to let the client project know directly what error happened with the calls to Keycloak API.

Since the token refresh is not an error the client project can handle, we agree to add a try-catch block within the setInterval.

If you have a handy temporary fix, please don't hesitate to contribute. If not, I will assign this task to @KLloydNA :neckbeard:

[ScheckGuillermo]

Thank you for replying.

Yes, I agree that using a try/catch within the setInterval function ensures that the node app will not end with an unhandled exception.

I have not implemented a temporary fix because I have reported a genuine issue that causes the setInterval to fail, resulting in the entire application failing #42

[euscanga-krasamo]

Will close this one because we have a detailed version of it on #42. 👍