Improve behavior when setupAutomaticSilentRefresh is called *after* loadDiscoveryDocument methods

[jeroenheijmans]

In #722 we uncovered that calling setupAutomaticSilentRefresh() after e.g. loadDiscoveryDocumentAndTryLogin() can cause weird behaviors down the line. See the linked issue for those situations.

This issue proposes we improve this by either:

• at worst throwing a more descriptive error to explain that the order of methods called is unexpected (instead of some random other error down the line)
• at best "just work" with some sensible behavior

[aznarepse]

Hi,
I have done a bit of analysis and I believe there is a race in between the constructor of OAuthService, the function setupAutomaticSilentRefresh and the function loadDiscoveryDocumentAndTryLogin. I have not had the opportunity to do a debugging session but would like to hear your views before I jump on it. debugging a race in an event driven nonholonomic system will be a bit complex.
Please, allow me to develop a bit the reasons:
the constructor

Note that it will invoke setupRefreshTimer

setupRefreshTimer

There are several points of interest: hasValidToken and the subscription to the event token_received and then the event token_expires potentially being fired either because we have a valid token at this time or because the event token_received has been fired somewhere else.

loadDiscoveryDocumentAndTryLogin
This function invokes tryLogin, which, if successful, renews the token in the store and fires token_received and token_refreshed.

It follows that,
setupAutomaticSilentRefresh

subscribes to the event token_expires, which will then trigger a call to refreshInternal, which in turn calls refresfToken. The latter then renews the token in the store, and fires token_received and token_refreshed.

I have not found a synchronization mechanism between the constructor setupExpirationTimers, the loadDiscoveryDocumentAndTryLogin, and the setupAutomaticSilentRefresh, all accessing the store and potentially using different tokens when acting upon an event received.

Any views?

[jeroenheijmans]

Wow! That's a... thorough analysis 😅. I'm not sure if I'll personally have time on the short term to think/debug along, but maybe some other community member does?

On a side note, at this time it's sad there's no good test coverage. Would've been good to verify your assumptions with (marble) tests.

[manfredsteyer]

Thanks for this analysis. Can you help me to reproduce this issue with one of our example applications?

[aznarepse]

Hi, yes, I will have some time this next weeks. Sadly, this corona-virus crisis is bringing me to stand-down on some projects I had on the keyboard.
I will also be likely able to provide a simple app which 'bangs' all the time with this behaviour.

[aznarepse]

Quick update; I have been able to dedicate yesterday to this and am hoping to do it today as well.
I believe I'll have it nailed before Monday.

[aznarepse]

I believe I have found the culprit of the problem. Please, bear with me as I try to explain it.
Scenario:
The application is configured as follows in the constructor of the app.component.ts

constructor(private oauthService: OAuthService, private authConfigBuilder: AuthConfigBuilder) {
    this.configure();
  }

  private configure() {
    this.oauthService.configure(this.authConfigBuilder.GetAuthConfig());
    this.oauthService.setupAutomaticSilentRefresh();
    this.oauthService.tokenValidationHandler = new JwksValidationHandler();
    this.oauthService.loadDiscoveryDocumentAndTryLogin();
  }

Then, the login is performed by invoking initCodeFlow() on demand through the user clicking on a button.

Problem:
When the user logins, the code flow is triggered, a token is received and then the tokentimer is set. In my case, because I have a very short expiration for the id token, it is always this timer the one that it is set.

So far all things are all right and once the timer triggers, the application will request a new token using the refresh token that was received with the original token. Still, there are some oddities which makes the process a bit none-deterministic. For example, in the method processIdToken, the clockSkewInMSec may introduce a 10 minutes clock skew, which in the case of a very short lived Id token, will be problematic.

However, if we refresh the page after the user has login. Because the Oauth service is initialized again, the configuration is processed again. There are some guards in the methods to account for an existing token. Not in setupAutomaticSilentRefresh though, which calls restartRefreshTimerIfStillLoggedIn, which in turn calls setupExpirationTimers whithout calling first clearAccessTokenTimer and clearIdTokenTimer clearing the previous timers and hence creates another timer that will fire the event for expiration again.

Therefore, the next expiration will trigger two requests to the server. The server honours the two requests and responds with two new JWT with identical id and access token but with different refresh token. Only one of the refresh tokens is valid of course.
From here, we have a race with the two subscriptions and we have the scenario that the wrong refresh token is being used. The application cannot recover from this unless we trigger again initCodeFlow() to get a fresh token.

I can reproduce this every time and it is easily fixed by adding clearAccessTokenTimer() and clearIdTokenTimer() to restartRefreshTimerIfStillLoggedIn().

Fix:

restartRefreshTimerIfStillLoggedIn() {
        this.clearAccessTokenTimer();
        this.clearIdTokenTimer();
        this.setupExpirationTimers();
    }

A bit dirty but solves the problem...

The subscriptions are a bit messy in general without much synchronization. For example, the method setupExpirationTimers() is called multiple times (I counted 3) whenever a token expires because there are multiple subscriptions to the event, it seems harmless as it happens within microseconds and the timers are cleared first but it is a waste.

Hope this helps you guys.

[aznarepse]

Any feedback on this?