Race condition in RefreshTokenDelegatingHandler

[Nickolas-]

Hi, im not familiar is it expected behaviour or Im using library not in proper way, but... If add RefreshTokenDelegatingHandler in HttpClient pipe , you would get Unauthorized http response when you have a valid refresh token. This cause by

private async Task<bool> RefreshTokensAsync(CancellationToken cancellationToken)

When you run two http requests in parallel with invalid access token it would cause a race condition in logic.

        var refreshToken = RefreshToken;
        if (refreshToken.IsMissing())
        {
            return false;
        }

        if (await _lock.WaitAsync(Timeout, cancellationToken).ConfigureAwait(false))

Issue would be if one request finish work earlier than other. For example from the beginning refreshToken is equal to "FIRST". First call of RefreshTokensAsync would return that refreshToken is "TWO" , but previous http request would be still try to refresh token with token "FIRST" and would return response with error because that token is not exist in IS anymore. Can anyone give me some thoughts about it.

[KennyDM]

I can acknowledge this is an actual issue, we are having the same issue on our side.

We believe the issue lies in the fact that the retrieval of the refresh token is not contained within the critical section of the lock where the actual refresh happens (see code sample from Nickolas)

This allows a second call, that was awaiting the lock, to use an old value of the refresh token, even after a first call successfully refreshed the tokens. The result is as @Nickolas- described, that the second call fails to refresh the tokens, because it cannot find the handle anymore in the PersistedGrants (because the first call successfully refreshed both access and refresh tokens).

We just discovered this issue in our mobile app, we will try to work around this by implementing the RefreshTokenDelegatingHandler ourselves, and refactor the RefreshTokensAsynccall a bit, so that the retrieval of the refresh token happens within the critical section of the lock.

Something like this:

	private async Task<bool> RefreshTokensAsync(CancellationToken cancellationToken)
	{
		//code removed here

		if (await _lock.WaitAsync(Timeout, cancellationToken).ConfigureAwait(continueOnCapturedContext: false))
		{
			try
			{		
				//code moved here
				if (string.IsNullOrEmpty(_refreshToken))  //using the private field, since we are in the lock
				{
					return false;
				}

				RefreshTokenResult response = await _oidcClient.RefreshTokenAsync(_refreshToken, null, cancellationToken).ConfigureAwait(continueOnCapturedContext: false);