[core-http] Token refresher fix: Refresh in the background, keep returning valid token

[sadasant]

This PR intends to fix the logic in the bearerTokenAuthenticationPolicy to:

• Return a token for as long as it hasn't expired.
• Wait for the token refresh only the first time we authenticate, or if the token has expired.

We can do this in a million ways. In this approach we avoid doing public API changes, but we add a redundant copy of the token inside of the bearerTokenAuthenticationPolicy.

If the design isn't good enough, please recommend me other ways to do this! We can use these same tests to verify any design.

The new design for core-rest-pipelines won't have this issue!

---

Backstory:

Recently I did this change in the bearer token authentication policy: #13736

It fixed an issue in which we were refreshing the token every 30 seconds, instead of waiting for the token's expiration date (or a bit earlier than that, which we can call the refresh time window, in which we start refreshing).

That PR introduced a bug that is breaking ai-form-recognizer.

There are to time variables here:

• The time before the token expires, which we somehow defined in the TokenCache, but it's intended to be used to start the refreshing of the token.
• How frequently we allow token refreshes to happen. At least this one is tracked by the token refresher.

With those things in mind, we can explain a bug introduced by my last PR:

Bug: When we reach the refresh window, now we only try to refresh ONCE in this refresh time frequency, other incoming requests while the refresher is not ready will result in NO TOKEN.

This PR fixes that bug!

[sadasant]

We didn't have this promise check. But there's no reason we should prevent refreshes if the promise exists, since we do re-use the promise 😁

[sadasant]

This reminds me to a feedback you (@xirzec) gave me earlier. Why do we set the bearer token if we don't have a token? This seems like an improvement.

[xirzec]

what about this case:

1. this.tokenCache.getCachedToken() returns a valid token
2. this.token is expired

Since the token from the cache is valid, readyToRefresh will be false and refreshPromise will be undefined. Then line 107 assigns the expired token over the one we got from the cache. Now we get here and try to await undefined.

[sadasant]

This can be solved by the conditional if (this.token) some lines above, if we change it to:

    // We don't use the local copy if the token cache has a token that has an expiration date greater than the local copy.
    if (this.token && this.token.expiresOnTimestamp > (token?.expiresOnTimestamp || 0)) {
      token = this.token;
    }

I know it's not pretty! But I'll push it while we come up with something better.

[witemple-msft]

I really think a core issue here is that the cache is configured to return undefined instead of the token it contains when we're within 30 seconds of the token expiring, meaning that the cache purges at the same time as the refresher becomes ready. We can alternatively just configure the cache to have a much shorter window (by passing the parameter to its constructor rather than using the default value) instead of holding the token again inside the policy.

It makes a lot more sense to me to be able to reduce this to simple rules: if the cache is purged, we always wait for a refresh. If the refresher is ready, we trigger a refresh, but don't wait. I have a hard time following the logic in getToken here because of stuff that I assume is historical and difficult to change.

[witemple-msft]

Should we add a buffer of time here? If the token is within 100ms of expiring, then there's a decent chance we won't get the request out the door and through the entire pipeline to validation at the service before it expires. Maybe we can say "if the token is within a second of expiring, then we wait."

[sadasant]

As long as the refresh window is not < 100ms, then there's no need for a buffer. We currently we default the refresh window to 2 minutes, and the refresh interval to 30 seconds.

[witemple-msft]

Closing in favor of #14223 where we've been collaborating on a different solution.