AzureAD / microsoft-authentication-library-for-dotnet

Microsoft Authentication Library (MSAL) for .NET
https://aka.ms/msal-net
MIT License
1.36k stars 334 forks source link

[Feature Request] Allow clients to provide their own HttpClient instance. #2557

Closed mandel-macaque closed 3 years ago

mandel-macaque commented 3 years ago

Is your feature request related to a problem? Please describe.

The library creates its own HttpClient instance, this is an anti-pattern. Libraries should not create an instance of the HttpClient because it will bypass any settings that the client has set up. Forcing a HttpClient implementation on the user of the library is wrong.

The way in which it is implemented also ignores the default settings set by the user in the project, meaning that if a user requires a specific handler to be used (like the managed one) this is ignored.

Bypassing settings also results in applications having problems with:

Describe the solution you'd like

Provide an API in which the user can pass its own HttpClientHandler for the HttpClient or its own HttpClient. Doing a dependency injection of the instance is a saner approach over imposing a specific implementation.

Describe alternatives you've considered

There are no other alternatives.

Additional context

The Microsoft documentation clearly states that having more than one HttpClient is discourage at least:

The HttpClient class instance acts as a session to send HTTP requests. An HttpClient instance is a collection of settings applied to all requests executed by that instance. In addition, every HttpClient instance uses its own connection pool, isolating its requests from requests executed by other HttpClient instances.

Which follows with:

HttpClient is intended to be instantiated once and re-used throughout the life of an application.

Documentation ref: https://docs.microsoft.com/en-us/dotnet/api/system.net.http.httpclient?view=net-5.0

jennyf19 commented 3 years ago

we already allow customers to provide their own HttpClient -> https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/wiki/httpclient

udlose commented 2 years ago

@jennyf19, @jmprieur, @bgavrilMS - it should be noted that using your own HttpClient seems to bypass the token cache mechanism.

jmprieur commented 2 years ago

@udlose : do you have a repro?

bgavrilMS commented 2 years ago

I don't think so @udlose, we do this all the time in our integration tests. But maybe we missed something, happy to take a look if you have a repro.

udlose commented 2 years ago

@bgavrilMS , @jmprieur I'd have to dummy one up from our application code. I can provide the code though for your review/analysis. Let me know if you want me to provide the code in a different fashion.

I was able to fix the bypassing of the token cache simply by commenting out the line of the call to WithHttpClientFactory(msalApiClient) in the code below.

If I included WithHttpClientFactory(msalApiClient) I always saw the call to go and get an AccessToken in my debug output.

    /// <summary>
    /// Originally based on an example from Microsoft - https://github.com/microsoftgraph/aspnetcore-connect-sample/tree/master/MicrosoftGraphAspNetCoreConnectSample
    ///
    /// THIS CODE HAS BEEN MODIFIED TO FIT OUR IMPLEMENTATION!
    /// </summary>
    public class GraphAuthProvider : IGraphAuthProvider
    {
        private readonly IConfidentialClientApplication _app;

        // With client credentials flows the scopes is ALWAYS of the shape "resource/.default", as the 
        // application permissions need to be set statically (in the portal or by PowerShell), and then granted by
        // a tenant administrator
        private static readonly string[] _scopes = { "https://graph.microsoft.com/.default" };
        private readonly IHttpContextAccessor _httpContextAccessor;

        public GraphAuthProvider(IConfiguration configuration, IMsalApiClient msalApiClient, IHttpContextAccessor httpContextAccessor)
        {
            var azureOptions = new AzureAdOptions();
            configuration.Bind("AzureAd", azureOptions);

            // More info about MSAL Client Applications: https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/wiki/Client-Applications
            _app = ConfidentialClientApplicationBuilder.Create(azureOptions.ClientId)
                    .WithClientSecret(azureOptions.ClientSecret)
                    .WithAuthority(AzureCloudInstance.AzurePublic, Guid.Parse(azureOptions.TenantId), false)
                    .WithHttpClientFactory(msalApiClient)
                    .WithRedirectUri(azureOptions.BaseUrl + azureOptions.CallbackPath)
                    .Build();

            Authority = _app.Authority;

            _httpContextAccessor = httpContextAccessor;
        }

        public string Authority { get; }

        public Task<AuthenticationResult> GetAccessTokenAsync()
        {
            // Put the token into the built-in MSAL token cache first
            if (Guid.TryParse(_httpContextAccessor.HttpContext?.GetCorrelationId()?.Replace("web/", string.Empty), out var correlationId))
            {
                return _app.AcquireTokenForClient(_scopes)
                    .WithCorrelationId(correlationId)
                    .ExecuteAsync();
            }

            return _app.AcquireTokenForClient(_scopes)
                .ExecuteAsync();
        }

        //public Task<AuthenticationResult> GetUserAccessTokenByAuthorizationCode(string authorizationCode)
        //{
        //    //this puts the token in the cache according to 
        //    // https://docs.microsoft.com/en-us/dotnet/api/microsoft.identity.client.iconfidentialclientapplication.acquiretokenbyauthorizationcode?f1url=https%3A%2F%2Fmsdn.microsoft.com%2Fquery%2Fdev16.query%3FappId%3DDev16IDEF1%26l%3DEN-US%26k%3Dk(Microsoft.Identity.Client.IConfidentialClientApplication.AcquireTokenByAuthorizationCode);k(DevLang-csharp)%26rd%3Dtrue&view=azure-dotnet
        //    return _app.AcquireTokenByAuthorizationCode(_scopes, authorizationCode)
        //                      .ExecuteAsync();
        //}
    }

and the service registration for GraphAuthProvider is a singleton:

            //MSAL
            // IGraphAuthProvider uses a cache for tokens so it needs to be a singleton
            services.AddSingleton<IGraphAuthProvider, GraphAuthProvider>();
            services.AddTransient<MsalDelegatingHandler>();
udlose commented 2 years ago

Note that we use Polly for transient fault handling.

with a registered delegating handler for the HttpClient as:

            services.AddHttpClient<IMsalApiClient, MsalApiClient>()
                .AddHttpMessageHandler<MsalDelegatingHandler>()     // need a delegating handler to intercept requests to MSAL services
                .AddPolicyHandlerFromRegistry(PolicyKeys.TransientFault.MsalPolicy));

            services.AddHttpClient<IGraphApiClient, GraphApiClient>()
                .AddPolicyHandlerFromRegistry(PolicyKeys.TransientFault.MicrosoftGraphApiPolicy));

and

    public class MsalDelegatingHandler : DelegatingHandler
    {
        private readonly ILogger<MsalDelegatingHandler> _logger;

        public MsalDelegatingHandler(ILogger<MsalDelegatingHandler> logger)
        {
            _logger = logger;
        }

        /// <summary>
        /// Sends an HTTP request to the inner handler to send to the server as an asynchronous operation.
        /// </summary>
        /// <param name="request">The HTTP request message to send to the server.</param>
        /// <param name="cancellationToken">A cancellation token to cancel operation.</param>
        /// <returns>
        /// The task object representing the asynchronous operation.
        /// </returns>
        /// <remarks>This method allows for the interception of HttpClient calls to
        /// the services provided by Microsoft.Client.Identity (MSAL).</remarks>
        protected override Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, 
            CancellationToken cancellationToken)
        {
            var methodBase = MethodBase.GetCurrentMethod();
            var contextName = $"{methodBase.DeclaringType?.FullName}.{methodBase.Name}";
            var transientFaultContext = PolicyExecutor.CreateTransientFaultContext(contextName);
            transientFaultContext.WithHttpRequestMessage(request);
            transientFaultContext.WithLogger(_logger);
            request.SetPolicyExecutionContext(transientFaultContext);

            return base.SendAsync(request, cancellationToken);
        }
    }

and

    public interface IMsalApiClient : IMsalHttpClientFactory
    {
    }

and

    /// <summary>
    /// This class provides an absatraction over the HttpClient used by MSAL as described
    /// here https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/wiki/httpclient
    /// and
    /// here https://docs.microsoft.com/en-us/azure/active-directory/develop/msal-net-provide-httpclient
    /// </summary>
    [SuppressMessage("ReSharper", "ClassNeverInstantiated.Global")]
    public class MsalApiClient : ApiClientBase, IMsalApiClient
    {
        public MsalApiClient(HttpClient httpClient, ILogger<MsalApiClient> logger, 
            IHttpContextAccessor httpContextAccessor) : base(httpClient, logger, httpContextAccessor)
        {
        }

        public HttpClient GetHttpClient()
        {
            return HttpClient;
        }
    }
udlose commented 2 years ago

and the calls to MSGraph taking place from here:

    public class GraphRepository : IGraphRepository
    {
        // Per https://docs.microsoft.com/en-us/graph/sdks/create-client?tabs=CS a GraphServiceClient can
        // be used as a singleton.  This one is initialized by LazyInitializer.EnsureInitialized in GetAuthenticatedClient()
        private static GraphServiceClient _graphServiceClient;

        private readonly IGraphAuthProvider _authProvider;
        private readonly IGraphApiClient _graphApiClient;

        public GraphRepository(IGraphAuthProvider authProvider, IGraphApiClient graphApiClient)
        {
            _authProvider = authProvider;
            _graphApiClient = graphApiClient;
        }

        public async Task<ApiProxyResult<GraphUserResponse, string>> GetUserAsync(Models.User.UserRequest request)
        {
            if (request == null) throw new ArgumentNullException(nameof(request));
            if (string.IsNullOrWhiteSpace(request.Email))
                throw new ArgumentException("Email cannot be null or whitespace.", nameof(request));

            var result = new ApiProxyResult<GraphUserResponse, string>();
            try
            {
                var graphSdkClient = GetAuthenticatedClient();

                // this eventually calls out to MS Graph Api, but first calls out
                // to MSAL to get an Authorization access token
                var user = await GetUserFromMsGraphAsync(request, graphSdkClient);

            }
            catch (ServiceException e)
            {
                Set404ResultOrThrowOnError(request.Email, e, result);
            }
            catch (Exception e)
            {
                throw new ApiException("Unable to get user from Microsoft Graph", e, request);
            }

            return result;
        }

        private static async Task<Microsoft.Graph.User> GetUserFromMsGraphAsync(UserRequest request, GraphServiceClient graphSdkClient)
        {
            var userData = await graphSdkClient
                .Users[request.Email]
                .Request()
                .Select(_userPropertyNames)
                .GetAsync();

            return userData;
        }

        private GraphServiceClient GetAuthenticatedClient()
        {
            LazyInitializer.EnsureInitialized(ref _graphServiceClient, () =>
            {
                var authProvider = new DelegateAuthenticationProvider(
                    async requestMessage =>
                    {
                        try
                        {
                            var sw = System.Diagnostics.Stopwatch.StartNew();
                            var authResult = await _authProvider.GetAccessTokenAsync();

                            sw.Stop();
                            System.Diagnostics.Debug.WriteLine($"{nameof(GraphRepository.GetAuthenticatedClient)}: MSAL token access time: {sw.ElapsedMilliseconds} ms");
                            System.Diagnostics.Debug.WriteLine($"{nameof(GraphRepository.GetAuthenticatedClient)}: MSAL token source: {Enum.GetName<Microsoft.Identity.Client.TokenSource>(authResult.AuthenticationResultMetadata.TokenSource)}");
                            System.Diagnostics.Debug.WriteLine($"{nameof(GraphRepository.GetAuthenticatedClient)}: MSAL token is from cache: {authResult.AuthenticationResultMetadata.TokenSource == Microsoft.Identity.Client.TokenSource.Cache}");

                            // Append the access token to the request
                            requestMessage.Headers.Authorization = new AuthenticationHeaderValue("Bearer", authResult.AccessToken);
                        }
                        catch (Exception e)
                        {
                            throw new ApiException("Unable to acquire access token for Microsoft Graph", e);
                        }
                    });

                return new GraphServiceClient(authProvider, _graphApiClient);
            });

            return _graphServiceClient;
        }
    }
udlose commented 2 years ago

and the HttpClient for MSGraph as:

    public interface IGraphApiClient : IHttpProvider
    {
    }

    public sealed class GraphApiClient : ApiClientBase, IGraphApiClient
    {
        private static readonly ISerializer _serializer = new Serializer();

        public ISerializer Serializer => _serializer;

        public TimeSpan OverallTimeout
        {
            get => HttpClient.Timeout;
            set
            {
                try
                {
                    HttpClient.Timeout = value;
                }
                catch (InvalidOperationException ex)
                {
                    var error = new Error
                    {
                        Code = "notAllowed",
                        Message = "Overall timeout cannot be set after the first request is sent."
                    };
                    throw new ServiceException(error, ex);
                }
            }
        }

        public GraphApiClient(HttpClient httpClient, ILogger<GraphApiClient> logger,
            IHttpContextAccessor httpContextAccessor)
            : base(httpClient, logger, httpContextAccessor)
        {
        }

        public Task<HttpResponseMessage> SendAsync(HttpRequestMessage request)
        {
            return SendAsync(request, HttpCompletionOption.ResponseContentRead, CancellationToken.None);
        }

        public Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, 
            HttpCompletionOption completionOption, CancellationToken cancellationToken)
        {
            if (request == null) throw new ArgumentNullException(nameof(request));

            // ignore HttpCompletionOption as it will just use the default value
            return ExecuteRequestWithPolicyAsync(request, cancellationToken: cancellationToken);
        }

        [SuppressMessage("Design", "CA1063:Implement IDisposable Correctly", 
            Justification = "HttpClient instance lifetime is managed by IHttpClientFactory")]
        public void Dispose()
        {
        }
    }
udlose commented 2 years ago

Forgot to mention I'm using the latest versions (as of today 7/27/22) of all associated nugets and running .NET 6:

jmprieur commented 2 years ago

@udlose : why don't you use the GraphServiceClient (form Microsoft.Graph SDK) ?

udlose commented 2 years ago

@jmprieur I do use the GraphServiceClient. Look in the GetAuthenticatedClient method of the GraphRepository.

The GraphServiceClient doesn't come with an auth provider/mechanism. The application is responsible for providing one to the GraphServiceClient

bgavrilMS commented 2 years ago

@udlose - I think it's because you don't enable token caching and maybe the _app object goes out of scope / is re-created on each request? You can check if the token comes from the cache or from IDP by looking at AuthenticationResult.AuthenticationResultMetadata.TokenSource.

Can you try to add _app.AppTokenCache.SetCacheOptions(CacheOptions.EnableSharedCacheOptions); which will make MSAL's cache static (shared) ?

udlose commented 2 years ago

@bgavrilMS

  1. The _app object is a member of GraphAuthProvider which is registered as a singleton.
  2. I wasn't aware that I had to specifically enable token caching (I was able to get it to use the token cache without any specific code to set the cache options just by removing the WithHttpClientFactory(msalApiClient)
  3. What does the CacheOptions.EnableSharedCacheOptions) do? I was running locally so even if it wasn't in a shared cache, it still didn't work until I removed the WithHttpClientFactory(msalApiClient)
  4. I was able to confirm that it was using the token cache when I removed the WithHttpClientFactory(msalApiClient) by looking at the following debug lines in this method:

        private GraphServiceClient GetAuthenticatedClient()
        {
            LazyInitializer.EnsureInitialized(ref _graphServiceClient, () =>
            {
                var authProvider = new DelegateAuthenticationProvider(
                    async requestMessage =>
                    {
                        try
                        {
                            var sw = System.Diagnostics.Stopwatch.StartNew();
                            var authResult = await _authProvider.GetAccessTokenAsync();
    
                            sw.Stop();
                            System.Diagnostics.Debug.WriteLine($"{nameof(GraphRepository.GetAuthenticatedClient)}: MSAL token access time: {sw.ElapsedMilliseconds} ms");
                            System.Diagnostics.Debug.WriteLine($"{nameof(GraphRepository.GetAuthenticatedClient)}: MSAL token source: {Enum.GetName<Microsoft.Identity.Client.TokenSource>(authResult.AuthenticationResultMetadata.TokenSource)}");
                            System.Diagnostics.Debug.WriteLine($"{nameof(GraphRepository.GetAuthenticatedClient)}: MSAL token is from cache: {authResult.AuthenticationResultMetadata.TokenSource == Microsoft.Identity.Client.TokenSource.Cache}");
    
                            // Append the access token to the request
                            requestMessage.Headers.Authorization = new AuthenticationHeaderValue("Bearer", authResult.AccessToken);
                        }
                        catch (Exception e)
                        {
                            throw new ApiException("Unable to acquire access token for Microsoft Graph", e);
                        }
                    });
    
                return new GraphServiceClient(authProvider, _graphApiClient);
            });
    
            return _graphServiceClient;
        }