From 037c2ed53287f32bad1a0e4f12a7b17d64f059dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Chalet?= Date: Sun, 6 Dec 2020 20:48:51 +0100 Subject: [PATCH] Make various improvements to the OpenIddict.Validation/System.Net.Http integration package --- ...lidationSystemNetHttpHandlers.Discovery.cs | 6 +- ...tionSystemNetHttpHandlers.Introspection.cs | 35 ++-- ...enIddictValidationSystemNetHttpHandlers.cs | 162 +++++++++++++++--- ...penIddictValidationSystemNetHttpOptions.cs | 2 +- 4 files changed, 159 insertions(+), 46 deletions(-) diff --git a/src/OpenIddict.Validation.SystemNetHttp/OpenIddictValidationSystemNetHttpHandlers.Discovery.cs b/src/OpenIddict.Validation.SystemNetHttp/OpenIddictValidationSystemNetHttpHandlers.Discovery.cs index ab7cfee6..41c97de5 100644 --- a/src/OpenIddict.Validation.SystemNetHttp/OpenIddictValidationSystemNetHttpHandlers.Discovery.cs +++ b/src/OpenIddict.Validation.SystemNetHttp/OpenIddictValidationSystemNetHttpHandlers.Discovery.cs @@ -20,11 +20,13 @@ namespace OpenIddict.Validation.SystemNetHttp PrepareGetHttpRequest.Descriptor, AttachQueryStringParameters.Descriptor, SendHttpRequest.Descriptor, + DisposeHttpRequest.Descriptor, /* * Configuration response processing: */ ExtractJsonHttpResponse.Descriptor, + DisposeHttpResponse.Descriptor, /* * Cryptography request processing: @@ -32,11 +34,13 @@ namespace OpenIddict.Validation.SystemNetHttp PrepareGetHttpRequest.Descriptor, AttachQueryStringParameters.Descriptor, SendHttpRequest.Descriptor, + DisposeHttpRequest.Descriptor, /* * Configuration response processing: */ - ExtractJsonHttpResponse.Descriptor); + ExtractJsonHttpResponse.Descriptor, + DisposeHttpResponse.Descriptor); } } } diff --git a/src/OpenIddict.Validation.SystemNetHttp/OpenIddictValidationSystemNetHttpHandlers.Introspection.cs b/src/OpenIddict.Validation.SystemNetHttp/OpenIddictValidationSystemNetHttpHandlers.Introspection.cs index 7c99bfa9..3dc2b196 100644 --- a/src/OpenIddict.Validation.SystemNetHttp/OpenIddictValidationSystemNetHttpHandlers.Introspection.cs +++ b/src/OpenIddict.Validation.SystemNetHttp/OpenIddictValidationSystemNetHttpHandlers.Introspection.cs @@ -30,11 +30,13 @@ namespace OpenIddict.Validation.SystemNetHttp AttachBasicAuthenticationCredentials.Descriptor, AttachFormParameters.Descriptor, SendHttpRequest.Descriptor, + DisposeHttpRequest.Descriptor, /* * Introspection response processing: */ - ExtractJsonHttpResponse.Descriptor); + ExtractJsonHttpResponse.Descriptor, + DisposeHttpResponse.Descriptor); /// /// Contains the logic responsible of attaching the client credentials to the HTTP Authorization header. @@ -70,45 +72,44 @@ namespace OpenIddict.Validation.SystemNetHttp throw new InvalidOperationException(SR.GetResourceString(SR.ID0173)); } + // If no client identifier was attached to the request, skip the following logic. + if (string.IsNullOrEmpty(context.Request.ClientId)) + { + return; + } + var configuration = await context.Options.ConfigurationManager.GetConfigurationAsync(default) ?? throw new InvalidOperationException(SR.GetResourceString(SR.ID0140)); // The OAuth 2.0 specification recommends sending the client credentials using basic authentication. - // However, this authentication method is known to have compatibility issues with how the - // client credentials are encoded, that MUST be formURL-encoded before being base64-encoded. + // However, this authentication method is known to have compatibility issues with the way the + // client credentials are encoded (they MUST be formURL-encoded before being base64-encoded). // To guarantee that the OpenIddict validation handler can be used with servers implementing // non-standard encoding, the client_secret_post is always preferred when it's explicitly // listed as a supported client authentication method for the introspection endpoint. // If client_secret_post is not listed or if the server returned an empty methods list, - // client_secret_basic is always used, as it MUST be supported by all OAuth 2.0 servers. + // client_secret_basic is always used, as it MUST be implemented by all OAuth 2.0 servers. // // See https://tools.ietf.org/html/rfc8414#section-2 // and https://tools.ietf.org/html/rfc6749#section-2.3.1 for more information. if (!configuration.IntrospectionEndpointAuthMethodsSupported.Contains(ClientAuthenticationMethods.ClientSecretPost)) { - var builder = new StringBuilder() + // Important: the credentials MUST be formURL-encoded before being base64-encoded. + var credentials = Convert.ToBase64String(Encoding.ASCII.GetBytes(new StringBuilder() .Append(EscapeDataString(context.Request.ClientId)) .Append(':') - .Append(EscapeDataString(context.Request.ClientSecret)); - - var credentials = Convert.ToBase64String(Encoding.ASCII.GetBytes(builder.ToString())); + .Append(EscapeDataString(context.Request.ClientSecret)) + .ToString())); // Attach the authorization header containing the client credentials to the HTTP request. request.Headers.Authorization = new AuthenticationHeaderValue(Schemes.Basic, credentials); - // Remove the client credentials from the request. + // Remove the client credentials from the request payload to ensure they are not sent twice. context.Request.ClientId = context.Request.ClientSecret = null; } static string? EscapeDataString(string? value) - { - if (string.IsNullOrEmpty(value)) - { - return null; - } - - return Uri.EscapeDataString(value).Replace("%20", "+"); - } + => value is not null ? Uri.EscapeDataString(value).Replace("%20", "+") : null; } } } diff --git a/src/OpenIddict.Validation.SystemNetHttp/OpenIddictValidationSystemNetHttpHandlers.cs b/src/OpenIddict.Validation.SystemNetHttp/OpenIddictValidationSystemNetHttpHandlers.cs index 67d701b2..666aef88 100644 --- a/src/OpenIddict.Validation.SystemNetHttp/OpenIddictValidationSystemNetHttpHandlers.cs +++ b/src/OpenIddict.Validation.SystemNetHttp/OpenIddictValidationSystemNetHttpHandlers.cs @@ -9,10 +9,12 @@ using System.Collections.Generic; using System.Collections.Immutable; using System.ComponentModel; using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; using System.Linq; using System.Net.Http; using System.Net.Http.Headers; using System.Net.Http.Json; +using System.Text; using System.Threading.Tasks; using OpenIddict.Abstractions; using static OpenIddict.Validation.OpenIddictValidationEvents; @@ -46,6 +48,8 @@ namespace OpenIddict.Validation.SystemNetHttp .Build(); /// + [SuppressMessage("Reliability", "CA2000:Dispose objects before losing scope", + Justification = "The HTTP request message is disposed later by a dedicated handler.")] public ValueTask HandleAsync(TContext context) { if (context is null) @@ -53,12 +57,17 @@ namespace OpenIddict.Validation.SystemNetHttp throw new ArgumentNullException(nameof(context)); } - var request = new HttpRequestMessage(HttpMethod.Get, context.Address); - request.Headers.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json")); - request.Headers.AcceptCharset.Add(new StringWithQualityHeaderValue("utf-8")); + var request = new HttpRequestMessage(HttpMethod.Get, context.Address) + { + Headers = + { + Accept = { new MediaTypeWithQualityHeaderValue("application/json") }, + AcceptCharset = { new StringWithQualityHeaderValue("utf-8") } + } + }; // Store the HttpRequestMessage in the transaction properties. - context.Transaction.Properties[typeof(HttpRequestMessage).FullName!] = request; + context.Transaction.SetProperty(typeof(HttpRequestMessage).FullName!, request); return default; } @@ -81,6 +90,8 @@ namespace OpenIddict.Validation.SystemNetHttp .Build(); /// + [SuppressMessage("Reliability", "CA2000:Dispose objects before losing scope", + Justification = "The HTTP request message is disposed later by a dedicated handler.")] public ValueTask HandleAsync(TContext context) { if (context is null) @@ -88,12 +99,17 @@ namespace OpenIddict.Validation.SystemNetHttp throw new ArgumentNullException(nameof(context)); } - var request = new HttpRequestMessage(HttpMethod.Post, context.Address); - request.Headers.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json")); - request.Headers.AcceptCharset.Add(new StringWithQualityHeaderValue("utf-8")); + var request = new HttpRequestMessage(HttpMethod.Post, context.Address) + { + Headers = + { + Accept = { new MediaTypeWithQualityHeaderValue("application/json") }, + AcceptCharset = { new StringWithQualityHeaderValue("utf-8") } + } + }; // Store the HttpRequestMessage in the transaction properties. - context.Transaction.Properties[typeof(HttpRequestMessage).FullName!] = request; + context.Transaction.SetProperty(typeof(HttpRequestMessage).FullName!, request); return default; } @@ -116,7 +132,7 @@ namespace OpenIddict.Validation.SystemNetHttp .Build(); /// - public async ValueTask HandleAsync(TContext context) + public ValueTask HandleAsync(TContext context) { if (context is null) { @@ -134,25 +150,35 @@ namespace OpenIddict.Validation.SystemNetHttp throw new InvalidOperationException(SR.GetResourceString(SR.ID0173)); } - if (request.RequestUri is not null) + if (request.RequestUri is null || context.Transaction.Request.Count == 0) + { + return default; + } + + var builder = new StringBuilder(); + + foreach (var (key, value) in + from parameter in context.Transaction.Request.GetParameters() + let values = (string?[]?) parameter.Value + where values is not null + from value in values + where !string.IsNullOrEmpty(value) + select (parameter.Key, Value: value)) { - // Note: System.Net.Http doesn't expose convenient methods allowing to create - // query strings from existing key/value pairs. To work around this limitation, - // a FormUrlEncodedContent is instantiated and used to manually create the URL. - using var content = new FormUrlEncodedContent( - from parameter in context.Transaction.Request.GetParameters() - let values = (string[]?) parameter.Value - where values is not null - from value in values - select new KeyValuePair(parameter.Key, value)); - - var builder = new UriBuilder(request.RequestUri) + if (builder.Length > 0) { - Query = await content.ReadAsStringAsync() - }; + builder.Append('&'); + } - request.RequestUri = builder.Uri; + builder.Append(Uri.EscapeDataString(key)); + builder.Append('='); + builder.Append(Uri.EscapeDataString(value)); } + + // Compute the final request URI using the base address and the query string. + request.RequestUri = new UriBuilder(request.RequestUri) { Query = builder.ToString() }.Uri; + + return default; } } @@ -218,7 +244,7 @@ namespace OpenIddict.Validation.SystemNetHttp = OpenIddictValidationHandlerDescriptor.CreateBuilder() .AddFilter() .UseSingletonHandler>() - .SetOrder(int.MaxValue - 100_000) + .SetOrder(DisposeHttpRequest.Descriptor.Order - 50_000) .SetType(OpenIddictValidationHandlerType.BuiltIn) .Build(); @@ -252,7 +278,48 @@ namespace OpenIddict.Validation.SystemNetHttp } // Store the HttpResponseMessage in the transaction properties. - context.Transaction.Properties[typeof(HttpResponseMessage).FullName!] = response; + context.Transaction.SetProperty(typeof(HttpResponseMessage).FullName!, response); + } + } + + /// + /// Contains the logic responsible of disposing of the HTTP request message. + /// + public class DisposeHttpRequest : IOpenIddictValidationHandler where TContext : BaseExternalContext + { + /// + /// Gets the default descriptor definition assigned to this handler. + /// + public static OpenIddictValidationHandlerDescriptor Descriptor { get; } + = OpenIddictValidationHandlerDescriptor.CreateBuilder() + .AddFilter() + .UseSingletonHandler>() + .SetOrder(int.MaxValue - 100_000) + .SetType(OpenIddictValidationHandlerType.BuiltIn) + .Build(); + + /// + public ValueTask HandleAsync(TContext context) + { + if (context is null) + { + throw new ArgumentNullException(nameof(context)); + } + + // This handler only applies to System.Net.Http requests. If the HTTP request cannot be resolved, + // this may indicate that the request was incorrectly processed by another client stack. + var request = context.Transaction.GetHttpRequestMessage(); + if (request is null) + { + throw new InvalidOperationException(SR.GetResourceString(SR.ID0173)); + } + + request.Dispose(); + + // Remove the request from the transaction properties. + context.Transaction.SetProperty(typeof(HttpRequestMessage).FullName!, null); + + return default; } } @@ -268,7 +335,7 @@ namespace OpenIddict.Validation.SystemNetHttp = OpenIddictValidationHandlerDescriptor.CreateBuilder() .AddFilter() .UseSingletonHandler>() - .SetOrder(int.MaxValue - 100_000) + .SetOrder(DisposeHttpResponse.Descriptor.Order - 50_000) .SetType(OpenIddictValidationHandlerType.BuiltIn) .Build(); @@ -296,5 +363,46 @@ namespace OpenIddict.Validation.SystemNetHttp context.Transaction.Response = await response.Content.ReadFromJsonAsync(); } } + + /// + /// Contains the logic responsible of disposing of the HTTP response message. + /// + public class DisposeHttpResponse : IOpenIddictValidationHandler where TContext : BaseExternalContext + { + /// + /// Gets the default descriptor definition assigned to this handler. + /// + public static OpenIddictValidationHandlerDescriptor Descriptor { get; } + = OpenIddictValidationHandlerDescriptor.CreateBuilder() + .AddFilter() + .UseSingletonHandler>() + .SetOrder(int.MaxValue - 100_000) + .SetType(OpenIddictValidationHandlerType.BuiltIn) + .Build(); + + /// + public ValueTask HandleAsync(TContext context) + { + if (context is null) + { + throw new ArgumentNullException(nameof(context)); + } + + // This handler only applies to System.Net.Http requests. If the HTTP response cannot be resolved, + // this may indicate that the request was incorrectly processed by another client stack. + var response = context.Transaction.GetHttpResponseMessage(); + if (response is null) + { + throw new InvalidOperationException(SR.GetResourceString(SR.ID0173)); + } + + response.Dispose(); + + // Remove the response from the transaction properties. + context.Transaction.SetProperty(typeof(HttpResponseMessage).FullName!, null); + + return default; + } + } } } diff --git a/src/OpenIddict.Validation.SystemNetHttp/OpenIddictValidationSystemNetHttpOptions.cs b/src/OpenIddict.Validation.SystemNetHttp/OpenIddictValidationSystemNetHttpOptions.cs index aded41f7..c2c32338 100644 --- a/src/OpenIddict.Validation.SystemNetHttp/OpenIddictValidationSystemNetHttpOptions.cs +++ b/src/OpenIddict.Validation.SystemNetHttp/OpenIddictValidationSystemNetHttpOptions.cs @@ -23,6 +23,6 @@ namespace OpenIddict.Validation.SystemNetHttp public IAsyncPolicy? HttpErrorPolicy { get; set; } = HttpPolicyExtensions.HandleTransientHttpError() .OrResult(response => response.StatusCode == HttpStatusCode.NotFound) - .WaitAndRetryAsync(4, attempt => TimeSpan.FromSeconds(Math.Pow(2, attempt))); + .WaitAndRetryAsync(3, attempt => TimeSpan.FromSeconds(Math.Pow(2, attempt))); } }