AzureIdentityAccessTokenProvider.GetAuthorizationTokenAsync

[schuettecarsten]

Method AzureIdentityAccessTokenProvider.GetAuthorizationTokenAsync will add the current uri to the instance variable _scopes. This is not thread-safe and might cause InvalidOperationExceptions because of collections that are modified concurrently. It also uses the current method parameter uri to initialize the instance variable. Next time, when a different uri is used, scopes might be wrong.

My exception:

System.InvalidOperationException: Operations that change non-concurrent collections must have exclusive access. A concurrent update was performed on this collection and corrupted its state. The collection's state is no longer correct.
at bool System.Collections.Generic.HashSet.AddIfNotPresent(T value, out int location)
at async Task Microsoft.Kiota.Authentication.Azure.AzureIdentityAccessTokenProvider.GetAuthorizationTokenAsync(Uri uri, Dictionary additionalAuthenticationContext, CancellationToken cancellationToken)
at async Task Microsoft.Kiota.Abstractions.Authentication.BaseBearerTokenAuthenticationProvider.AuthenticateRequestAsync(RequestInformation request, Dictionary additionalAuthenticationContext, CancellationToken cancellationToken)
at async Task Microsoft.Kiota.Http.HttpClientLibrary.HttpClientRequestAdapter.GetHttpResponseMessage(RequestInformation requestInfo, CancellationToken cancellationToken, Activity activityForAttributes, string claims)
at async Task Microsoft.Kiota.Http.HttpClientLibrary.HttpClientRequestAdapter.SendAsync(RequestInformation requestInfo, ParsableFactory factory, Dictionary> errorMapping, CancellationToken cancellationToken)
at async Task Microsoft.Graph.Users.UsersRequestBuilder.GetAsync(Action requestConfiguration, CancellationToken cancellationToken)
[...]

Current implementation:

        if(!_scopes.Any())
            _scopes.Add($"{uri.Scheme}://{uri.Host}/.default");
        span?.SetTag("com.microsoft.kiota.authentication.scopes", string.Join(",", _scopes));
        var result = await this._credential.GetTokenAsync(new TokenRequestContext(_scopes.ToArray(), claims: decodedClaim), cancellationToken);

Idea to fix this is to not modify _scopes instance variable but use a local array:

        string[] scopes = _scopes.ToArray();
        if (scopes.Length == 0)
            sopes = new string[] { $"{uri.Scheme}://{uri.Host}/.default" };
        span?.SetTag("com.microsoft.kiota.authentication.scopes", string.Join(",", scopes));
        var result = await this._credential.GetTokenAsync(new TokenRequestContext(scopes, claims: decodedClaim), cancellationToken);

[baywet]

Thanks for reporting this @schuettecarsten
Use a local variable array means we'd be making a copy on every call, which would add to the memory pressure and GC workload.
What do you think about using a concurrent dictionary (string, bool) for the field instead?
Would you be willing to submit a pull request to correct this issue?
Thanks!

[schuettecarsten]

Use a local variable array means we'd be making a copy on every call

That's correct, but the ToArray() call is there in the old code also. Its directly inside the this._credential.GetTokenAsync(new TokenRequestContext(_scopes.ToArray().... So there should be no additional array. Maybe lifetime is a few ticks longer for the length check, but that seems to be acceptable. A concurrent dictionary seems to have a mich higher overhead for locking stuff?

[schuettecarsten]

Optimized version with less allocations, what do you think @baywet?

        string[] scopes;
        if (_scopes.Any())
            scopes = _scopes.ToArray()
        else
            sopes = new string[] { $"{uri.Scheme}://{uri.Host}/.default" };
        span?.SetTag("com.microsoft.kiota.authentication.scopes", string.Join(",", scopes));

Yes, I can create a pull request on this.

[schuettecarsten]

The PR is ready, please review carefully, I've added two more minor changes.