From ce0bcbe8fb14f22cbcd7a580343583ba8ea59461 Mon Sep 17 00:00:00 2001 From: martincostello Date: Sat, 6 Nov 2021 14:40:53 +0000 Subject: [PATCH 01/23] Support binding to form parameters in minimal actions Add support for binding to parameters of types IFormFileCollection and IFormFile on the HttpRequest in minimal actions. --- .../src/Metadata/IFromFileMetadata.cs | 15 + .../src/PublicAPI.Unshipped.txt | 2 + .../src/RequestDelegateFactory.cs | 371 ++++++++++++--- .../test/RequestDelegateFactoryTests.cs | 430 ++++++++++++++++++ .../EndpointMetadataApiDescriptionProvider.cs | 39 +- ...pointMetadataApiDescriptionProviderTest.cs | 195 +++++++- .../SimpleWithWebApplicationBuilderTests.cs | 19 + .../Program.cs | 6 + 8 files changed, 996 insertions(+), 81 deletions(-) create mode 100644 src/Http/Http.Abstractions/src/Metadata/IFromFileMetadata.cs diff --git a/src/Http/Http.Abstractions/src/Metadata/IFromFileMetadata.cs b/src/Http/Http.Abstractions/src/Metadata/IFromFileMetadata.cs new file mode 100644 index 000000000000..b159e375e569 --- /dev/null +++ b/src/Http/Http.Abstractions/src/Metadata/IFromFileMetadata.cs @@ -0,0 +1,15 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +namespace Microsoft.AspNetCore.Http.Metadata; + +/// +/// Interface marking attributes that specify a parameter should be bound using a form file from the request body. +/// +public interface IFromFileMetadata +{ + /// + /// The file name. + /// + string? Name { get; } +} diff --git a/src/Http/Http.Abstractions/src/PublicAPI.Unshipped.txt b/src/Http/Http.Abstractions/src/PublicAPI.Unshipped.txt index ae1c045751cd..a0741f182baf 100644 --- a/src/Http/Http.Abstractions/src/PublicAPI.Unshipped.txt +++ b/src/Http/Http.Abstractions/src/PublicAPI.Unshipped.txt @@ -14,6 +14,8 @@ Microsoft.AspNetCore.Http.Metadata.IAcceptsMetadata.IsOptional.get -> bool Microsoft.AspNetCore.Http.Metadata.IAcceptsMetadata.RequestType.get -> System.Type? Microsoft.AspNetCore.Http.Metadata.IFromBodyMetadata Microsoft.AspNetCore.Http.Metadata.IFromBodyMetadata.AllowEmpty.get -> bool +Microsoft.AspNetCore.Http.Metadata.IFromFileMetadata +Microsoft.AspNetCore.Http.Metadata.IFromFileMetadata.Name.get -> string? Microsoft.AspNetCore.Http.Metadata.IFromHeaderMetadata Microsoft.AspNetCore.Http.Metadata.IFromHeaderMetadata.Name.get -> string? Microsoft.AspNetCore.Http.Metadata.IFromQueryMetadata diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 5864b8bd27dd..aae379ee94a9 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -59,6 +59,8 @@ public static partial class RequestDelegateFactory private static readonly MemberExpression RouteValuesExpr = Expression.Property(HttpRequestExpr, typeof(HttpRequest).GetProperty(nameof(HttpRequest.RouteValues))!); private static readonly MemberExpression QueryExpr = Expression.Property(HttpRequestExpr, typeof(HttpRequest).GetProperty(nameof(HttpRequest.Query))!); private static readonly MemberExpression HeadersExpr = Expression.Property(HttpRequestExpr, typeof(HttpRequest).GetProperty(nameof(HttpRequest.Headers))!); + private static readonly MemberExpression FormExpr = Expression.Property(HttpRequestExpr, typeof(HttpRequest).GetProperty(nameof(HttpRequest.Form))!); + private static readonly MemberExpression FormFilesExpr = Expression.Property(FormExpr, typeof(IFormCollection).GetProperty(nameof(IFormCollection.Files))!); private static readonly MemberExpression StatusCodeExpr = Expression.Property(HttpResponseExpr, typeof(HttpResponse).GetProperty(nameof(HttpResponse.StatusCode))!); private static readonly MemberExpression CompletedTaskExpr = Expression.Property(null, (PropertyInfo)GetMemberInfo>(() => Task.CompletedTask)); @@ -66,6 +68,7 @@ public static partial class RequestDelegateFactory private static readonly BinaryExpression TempSourceStringNotNullExpr = Expression.NotEqual(TempSourceStringExpr, Expression.Constant(null)); private static readonly BinaryExpression TempSourceStringNullExpr = Expression.Equal(TempSourceStringExpr, Expression.Constant(null)); private static readonly string[] DefaultAcceptsContentType = new[] { "application/json" }; + private static readonly string[] FormFileContentType = new[] { "multipart/form-data" }; /// /// Creates a implementation for . @@ -195,6 +198,12 @@ private static Expression[] CreateArguments(ParameterInfo[]? parameters, Factory var errorMessage = BuildErrorMessageForInferredBodyParameter(factoryContext); throw new InvalidOperationException(errorMessage); } + if (factoryContext.JsonRequestBodyParameter is not null && + factoryContext.FormRequestBodyParameter is not null) + { + var errorMessage = BuildErrorMessageForFormAndJsonBodyParameters(factoryContext); + throw new InvalidOperationException(errorMessage); + } if (factoryContext.HasMultipleBodyParameters) { var errorMessage = BuildErrorMessageForMultipleBodyParameters(factoryContext); @@ -239,6 +248,10 @@ private static Expression CreateArgument(ParameterInfo parameter, FactoryContext factoryContext.TrackedParameters.Add(parameter.Name, RequestDelegateFactoryConstants.BodyAttribute); return BindParameterFromBody(parameter, bodyAttribute.AllowEmpty, factoryContext); } + else if (parameterCustomAttributes.OfType().FirstOrDefault() is { } fileAttribute) + { + return BindParameterFromFormFile(parameter, fileAttribute.Name ?? parameter.Name, factoryContext, RequestDelegateFactoryConstants.FormFileAttribute); + } else if (parameter.CustomAttributes.Any(a => typeof(IFromServiceMetadata).IsAssignableFrom(a.AttributeType))) { factoryContext.TrackedParameters.Add(parameter.Name, RequestDelegateFactoryConstants.ServiceAttribute); @@ -264,6 +277,14 @@ private static Expression CreateArgument(ParameterInfo parameter, FactoryContext { return RequestAbortedExpr; } + else if (parameter.ParameterType == typeof(IFormFileCollection)) + { + return BindParameterFromFormFiles(parameter, factoryContext); + } + else if (parameter.ParameterType == typeof(IFormFile)) + { + return BindParameterFromFormFile(parameter, parameter.Name, factoryContext, RequestDelegateFactoryConstants.FormFileParameter); + } else if (ParameterBindingMethodCache.HasBindAsyncMethod(parameter)) { return BindParameterFromBindAsync(parameter, factoryContext); @@ -511,7 +532,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, private static Func HandleRequestBodyAndCompileRequestDelegate(Expression responseWritingMethodCall, FactoryContext factoryContext) { - if (factoryContext.JsonRequestBodyParameter is null) + if (factoryContext.JsonRequestBodyParameter is null && !factoryContext.ReadForm) { if (factoryContext.ParameterBinders.Count > 0) { @@ -540,6 +561,20 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, responseWritingMethodCall, TargetExpr, HttpContextExpr).Compile(); } + if (factoryContext.ReadForm) + { + return HandleRequestBodyAndCompileRequestDelegateForForm(responseWritingMethodCall, factoryContext); + } + else + { + return HandleRequestBodyAndCompileRequestDelegateForJson(responseWritingMethodCall, factoryContext); + } + } + + private static Func HandleRequestBodyAndCompileRequestDelegateForJson(Expression responseWritingMethodCall, FactoryContext factoryContext) + { + Debug.Assert(factoryContext.JsonRequestBodyParameter is not null, "factoryContext.JsonRequestBodyParameter is null for a JSON body."); + var bodyType = factoryContext.JsonRequestBodyParameter.ParameterType; var parameterTypeName = TypeNameHelper.GetTypeDisplayName(factoryContext.JsonRequestBodyParameter.ParameterType, fullName: false); var parameterName = factoryContext.JsonRequestBodyParameter.Name; @@ -565,8 +600,8 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, return async (target, httpContext) => { - // Run these first so that they can potentially read and rewind the body - var boundValues = new object?[count]; + // Run these first so that they can potentially read and rewind the body + var boundValues = new object?[count]; for (var i = 0; i < count; i++) { @@ -579,7 +614,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, { if (!httpContext.Request.HasJsonContentType()) { - Log.UnexpectedContentType(httpContext, httpContext.Request.ContentType, factoryContext.ThrowOnBadRequest); + Log.UnexpectedJsonContentType(httpContext, httpContext.Request.ContentType, factoryContext.ThrowOnBadRequest); httpContext.Response.StatusCode = StatusCodes.Status415UnsupportedMediaType; return; } @@ -617,7 +652,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, { if (!httpContext.Request.HasJsonContentType()) { - Log.UnexpectedContentType(httpContext, httpContext.Request.ContentType, factoryContext.ThrowOnBadRequest); + Log.UnexpectedJsonContentType(httpContext, httpContext.Request.ContentType, factoryContext.ThrowOnBadRequest); httpContext.Response.StatusCode = StatusCodes.Status415UnsupportedMediaType; return; } @@ -632,7 +667,6 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, } catch (JsonException ex) { - Log.InvalidJsonRequestBody(httpContext, parameterTypeName, parameterName, ex, factoryContext.ThrowOnBadRequest); httpContext.Response.StatusCode = StatusCodes.Status400BadRequest; return; @@ -643,12 +677,112 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, } } - private static Expression GetValueFromProperty(Expression sourceExpression, string key) + private static Func HandleRequestBodyAndCompileRequestDelegateForForm( + Expression responseWritingMethodCall, + FactoryContext factoryContext) + { + Debug.Assert(factoryContext.FormRequestBodyParameter is not null, "factoryContext.FormRequestBodyParameter is null for a form body."); + + var parameterTypeName = TypeNameHelper.GetTypeDisplayName(factoryContext.FormRequestBodyParameter.ParameterType, fullName: false); + var parameterName = factoryContext.FormRequestBodyParameter.Name; + + Debug.Assert(parameterName is not null, "CreateArgument() should throw if parameter.Name is null."); + + if (factoryContext.ParameterBinders.Count > 0) + { + // We need to generate the code for reading from the body or form before calling into the delegate + var continuation = Expression.Lambda>( + responseWritingMethodCall, TargetExpr, HttpContextExpr, BodyValueExpr, BoundValuesArrayExpr).Compile(); + + // Looping over arrays is faster + var binders = factoryContext.ParameterBinders.ToArray(); + var count = binders.Length; + + return async (target, httpContext) => + { + // Run these first so that they can potentially read and rewind the body + var boundValues = new object?[count]; + + for (var i = 0; i < count; i++) + { + boundValues[i] = await binders[i](httpContext); + } + + object? formValue = null; + var feature = httpContext.Features.Get(); + if (feature?.CanHaveBody == true) + { + if (!httpContext.Request.HasJsonContentType()) + { + Log.UnexpectedFormContentType(httpContext, httpContext.Request.ContentType, factoryContext.ThrowOnBadRequest); + httpContext.Response.StatusCode = StatusCodes.Status415UnsupportedMediaType; + return; + } + try + { + formValue = await httpContext.Request.ReadFormAsync(); + } + catch (IOException ex) + { + Log.RequestBodyIOException(httpContext, ex); + return; + } + catch (InvalidDataException ex) + { + Log.InvalidFormRequestBody(httpContext, parameterTypeName, parameterName, ex, factoryContext.ThrowOnBadRequest); + httpContext.Response.StatusCode = StatusCodes.Status400BadRequest; + return; + } + } + + await continuation(target, httpContext, formValue, boundValues); + }; + } + else + { + // We need to generate the code for reading from the form before calling into the delegate + var continuation = Expression.Lambda>( + responseWritingMethodCall, TargetExpr, HttpContextExpr, BodyValueExpr).Compile(); + + return async (target, httpContext) => + { + object? formValue = null; + var feature = httpContext.Features.Get(); + if (feature?.CanHaveBody == true) + { + if (!httpContext.Request.HasFormContentType) + { + Log.UnexpectedFormContentType(httpContext, httpContext.Request.ContentType, factoryContext.ThrowOnBadRequest); + httpContext.Response.StatusCode = StatusCodes.Status415UnsupportedMediaType; + return; + } + try + { + formValue = await httpContext.Request.ReadFormAsync(); + } + catch (IOException ex) + { + Log.RequestBodyIOException(httpContext, ex); + return; + } + catch (InvalidDataException ex) + { + Log.InvalidFormRequestBody(httpContext, parameterTypeName, parameterName, ex, factoryContext.ThrowOnBadRequest); + httpContext.Response.StatusCode = StatusCodes.Status400BadRequest; + return; + } + } + await continuation(target, httpContext, formValue); + }; + } + } + + private static Expression GetValueFromProperty(Expression sourceExpression, string key, Type? returnType = null) { var itemProperty = sourceExpression.Type.GetProperty("Item"); var indexArguments = new[] { Expression.Constant(key) }; var indexExpression = Expression.MakeIndex(sourceExpression, itemProperty, indexArguments); - return Expression.Convert(indexExpression, typeof(string)); + return Expression.Convert(indexExpression, returnType ?? typeof(string)); } private static Expression BindParameterFromService(ParameterInfo parameter, FactoryContext factoryContext) @@ -672,52 +806,10 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres var parameterNameConstant = Expression.Constant(parameter.Name); var sourceConstant = Expression.Constant(source); - if (parameter.ParameterType == typeof(string)) + if (parameter.ParameterType == typeof(string) || + parameter.ParameterType == typeof(IFormFileCollection)) { - if (!isOptional) - { - // The following is produced if the parameter is required: - // - // tempSourceString = httpContext.RouteValue["param1"] ?? httpContext.Query["param1"]; - // if (tempSourceString == null) - // { - // wasParamCheckFailure = true; - // Log.RequiredParameterNotProvided(httpContext, "Int32", "param1"); - // } - var checkRequiredStringParameterBlock = Expression.Block( - Expression.Assign(argument, valueExpression), - Expression.IfThen(Expression.Equal(argument, Expression.Constant(null)), - Expression.Block( - Expression.Assign(WasParamCheckFailureExpr, Expression.Constant(true)), - Expression.Call(LogRequiredParameterNotProvidedMethod, - HttpContextExpr, parameterTypeNameConstant, parameterNameConstant, sourceConstant, - Expression.Constant(factoryContext.ThrowOnBadRequest)) - ) - ) - ); - - factoryContext.ExtraLocals.Add(argument); - factoryContext.ParamCheckExpressions.Add(checkRequiredStringParameterBlock); - return argument; - } - - // Allow nullable parameters that don't have a default value - var nullability = factoryContext.NullabilityContext.Create(parameter); - if (nullability.ReadState != NullabilityState.NotNull && !parameter.HasDefaultValue) - { - return valueExpression; - } - - // The following is produced if the parameter is optional. Note that we convert the - // default value to the target ParameterType to address scenarios where the user is - // is setting null as the default value in a context where nullability is disabled. - // - // param1_local = httpContext.RouteValue["param1"] ?? httpContext.Query["param1"]; - // param1_local != null ? param1_local : Convert(null, Int32) - return Expression.Block( - Expression.Condition(Expression.NotEqual(valueExpression, Expression.Constant(null)), - valueExpression, - Expression.Convert(Expression.Constant(parameter.DefaultValue), parameter.ParameterType))); + return BindParameterFromExpression(parameter, valueExpression, factoryContext, source); } factoryContext.UsingTempSourceString = true; @@ -835,8 +927,68 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres return argument; } - private static Expression BindParameterFromProperty(ParameterInfo parameter, MemberExpression property, string key, FactoryContext factoryContext, string source) => - BindParameterFromValue(parameter, GetValueFromProperty(property, key), factoryContext, source); + private static Expression BindParameterFromExpression( + ParameterInfo parameter, + Expression valueExpression, + FactoryContext factoryContext, + string source) + { + var nullability = factoryContext.NullabilityContext.Create(parameter); + var isOptional = IsOptionalParameter(parameter, factoryContext); + + var argument = Expression.Variable(parameter.ParameterType, $"{parameter.Name}_local"); + + var parameterTypeNameConstant = Expression.Constant(TypeNameHelper.GetTypeDisplayName(parameter.ParameterType, fullName: false)); + var parameterNameConstant = Expression.Constant(parameter.Name); + var sourceConstant = Expression.Constant(source); + + if (!isOptional) + { + // The following is produced if the parameter is required: + // + // argument = value["param1"]; + // if (argument == null) + // { + // wasParamCheckFailure = true; + // Log.RequiredParameterNotProvided(httpContext, "TypeOfValue", "param1"); + // } + var checkRequiredStringParameterBlock = Expression.Block( + Expression.Assign(argument, valueExpression), + Expression.IfThen(Expression.Equal(argument, Expression.Constant(null)), + Expression.Block( + Expression.Assign(WasParamCheckFailureExpr, Expression.Constant(true)), + Expression.Call(LogRequiredParameterNotProvidedMethod, + HttpContextExpr, parameterTypeNameConstant, parameterNameConstant, sourceConstant, + Expression.Constant(factoryContext.ThrowOnBadRequest)) + ) + ) + ); + + factoryContext.ExtraLocals.Add(argument); + factoryContext.ParamCheckExpressions.Add(checkRequiredStringParameterBlock); + return argument; + } + + // Allow nullable parameters that don't have a default value + if (nullability.ReadState != NullabilityState.NotNull && !parameter.HasDefaultValue) + { + return valueExpression; + } + + // The following is produced if the parameter is optional. Note that we convert the + // default value to the target ParameterType to address scenarios where the user is + // is setting null as the default value in a context where nullability is disabled. + // + // param1_local = httpContext.RouteValue["param1"] ?? httpContext.Query["param1"]; + // param1_local != null ? param1_local : Convert(null, Int32) + return Expression.Block( + Expression.Condition(Expression.NotEqual(valueExpression, Expression.Constant(null)), + valueExpression, + Expression.Convert(Expression.Constant(parameter.DefaultValue), parameter.ParameterType))); + } + + private static Expression BindParameterFromProperty(ParameterInfo parameter, MemberExpression property, string key, FactoryContext factoryContext, string source, Type? returnType = null) => + BindParameterFromValue(parameter, GetValueFromProperty(property, key, returnType), factoryContext, source); private static Expression BindParameterFromRouteValueOrQueryString(ParameterInfo parameter, string key, FactoryContext factoryContext) { @@ -889,6 +1041,46 @@ private static Expression BindParameterFromBindAsync(ParameterInfo parameter, Fa return Expression.Convert(boundValueExpr, parameter.ParameterType); } + private static Expression BindParameterFromFormFiles( + ParameterInfo parameter, + FactoryContext factoryContext) + { + factoryContext.FormRequestBodyParameter = parameter; + factoryContext.TrackedParameters.Add(parameter.Name!, RequestDelegateFactoryConstants.FormFileParameter); + + // Do not duplicate the metadata if there are multiple form parameters + if (!factoryContext.ReadForm) + { + factoryContext.Metadata.Add(new AcceptsMetadata(parameter.ParameterType, factoryContext.AllowEmptyRequestBody, FormFileContentType)); + } + + factoryContext.ReadForm = true; + + return BindParameterFromExpression(parameter, FormFilesExpr, factoryContext, "body"); + } + + private static Expression BindParameterFromFormFile( + ParameterInfo parameter, + string key, + FactoryContext factoryContext, + string trackedParameterSource) + { + factoryContext.FormRequestBodyParameter = parameter; + factoryContext.TrackedParameters.Add(key, trackedParameterSource); + + // Do not duplicate the metadata if there are multiple form parameters + if (!factoryContext.ReadForm) + { + factoryContext.Metadata.Add(new AcceptsMetadata(parameter.ParameterType, factoryContext.AllowEmptyRequestBody, FormFileContentType)); + } + + factoryContext.ReadForm = true; + + var valueExpression = GetValueFromProperty(FormFilesExpr, key, typeof(IFormFile)); + + return BindParameterFromExpression(parameter, valueExpression, factoryContext, "form file"); + } + private static Expression BindParameterFromBody(ParameterInfo parameter, bool allowEmpty, FactoryContext factoryContext) { if (factoryContext.JsonRequestBodyParameter is not null) @@ -1194,6 +1386,9 @@ private class FactoryContext public List Metadata { get; } = new(); public NullabilityInfoContext NullabilityContext { get; } = new(); + + public bool ReadForm { get; set; } + public ParameterInfo? FormRequestBodyParameter { get; set; } } private static class RequestDelegateFactoryConstants @@ -1203,11 +1398,13 @@ private static class RequestDelegateFactoryConstants public const string HeaderAttribute = "Header (Attribute)"; public const string BodyAttribute = "Body (Attribute)"; public const string ServiceAttribute = "Service (Attribute)"; + public const string FormFileAttribute = "Form File (Attribute)"; public const string RouteParameter = "Route (Inferred)"; public const string QueryStringParameter = "Query String (Inferred)"; public const string ServiceParameter = "Services (Inferred)"; public const string BodyParameter = "Body (Inferred)"; public const string RouteOrQueryStringParameter = "Route or Query String (Inferred)"; + public const string FormFileParameter = "Form File (Inferred)"; } private static partial class Log @@ -1221,12 +1418,18 @@ private static partial class Log private const string RequiredParameterNotProvidedLogMessage = @"Required parameter ""{ParameterType} {ParameterName}"" was not provided from {Source}."; private const string RequiredParameterNotProvidedExceptionMessage = @"Required parameter ""{0} {1}"" was not provided from {2}."; - private const string UnexpectedContentTypeLogMessage = @"Expected a supported JSON media type but got ""{ContentType}""."; - private const string UnexpectedContentTypeExceptionMessage = @"Expected a supported JSON media type but got ""{0}""."; + private const string UnexpectedJsonContentTypeLogMessage = @"Expected a supported JSON media type but got ""{ContentType}""."; + private const string UnexpectedJsonContentTypeExceptionMessage = @"Expected a supported JSON media type but got ""{0}""."; private const string ImplicitBodyNotProvidedLogMessage = @"Implicit body inferred for parameter ""{ParameterName}"" but no body was provided. Did you mean to use a Service instead?"; private const string ImplicitBodyNotProvidedExceptionMessage = @"Implicit body inferred for parameter ""{0}"" but no body was provided. Did you mean to use a Service instead?"; + private const string InvalidFormRequestBodyMessage = @"Failed to read parameter ""{ParameterType} {ParameterName}"" from the request body as form."; + private const string InvalidFormRequestBodyExceptionMessage = @"Failed to read parameter ""{0} {1}"" from the request body as form."; + + private const string UnexpectedFormContentTypeLogMessage = @"Expected a supported form media type but got ""{ContentType}""."; + private const string UnexpectedFormContentTypeExceptionMessage = @"Expected a supported form media type but got ""{0}""."; + // This doesn't take a shouldThrow parameter because an IOException indicates an aborted request rather than a "bad" request so // a BadHttpRequestException feels wrong. The client shouldn't be able to read the Developer Exception Page at any rate. public static void RequestBodyIOException(HttpContext httpContext, IOException exception) @@ -1291,19 +1494,47 @@ public static void ImplicitBodyNotProvided(HttpContext httpContext, string param [LoggerMessage(5, LogLevel.Debug, ImplicitBodyNotProvidedLogMessage, EventName = "ImplicitBodyNotProvided")] private static partial void ImplicitBodyNotProvided(ILogger logger, string parameterName); - public static void UnexpectedContentType(HttpContext httpContext, string? contentType, bool shouldThrow) + public static void UnexpectedJsonContentType(HttpContext httpContext, string? contentType, bool shouldThrow) + { + if (shouldThrow) + { + var message = string.Format(CultureInfo.InvariantCulture, UnexpectedJsonContentTypeExceptionMessage, contentType); + throw new BadHttpRequestException(message, StatusCodes.Status415UnsupportedMediaType); + } + + UnexpectedJsonContentType(GetLogger(httpContext), contentType ?? "(none)"); + } + + [LoggerMessage(6, LogLevel.Debug, UnexpectedJsonContentTypeLogMessage, EventName = "UnexpectedContentType")] + private static partial void UnexpectedJsonContentType(ILogger logger, string contentType); + + public static void UnexpectedFormContentType(HttpContext httpContext, string? contentType, bool shouldThrow) { if (shouldThrow) { - var message = string.Format(CultureInfo.InvariantCulture, UnexpectedContentTypeExceptionMessage, contentType); + var message = string.Format(CultureInfo.InvariantCulture, UnexpectedFormContentTypeExceptionMessage, contentType); throw new BadHttpRequestException(message, StatusCodes.Status415UnsupportedMediaType); } - UnexpectedContentType(GetLogger(httpContext), contentType ?? "(none)"); + UnexpectedFormContentType(GetLogger(httpContext), contentType ?? "(none)"); + } + + [LoggerMessage(7, LogLevel.Debug, UnexpectedFormContentTypeLogMessage, EventName = "UnexpectedContentType")] + private static partial void UnexpectedFormContentType(ILogger logger, string contentType); + + public static void InvalidFormRequestBody(HttpContext httpContext, string parameterTypeName, string parameterName, Exception exception, bool shouldThrow) + { + if (shouldThrow) + { + var message = string.Format(CultureInfo.InvariantCulture, InvalidFormRequestBodyExceptionMessage, parameterTypeName, parameterName); + throw new BadHttpRequestException(message, exception); + } + + InvalidFormRequestBody(GetLogger(httpContext), parameterTypeName, parameterName, exception); } - [LoggerMessage(6, LogLevel.Debug, UnexpectedContentTypeLogMessage, EventName = "UnexpectedContentType")] - private static partial void UnexpectedContentType(ILogger logger, string contentType); + [LoggerMessage(8, LogLevel.Debug, InvalidFormRequestBodyMessage, EventName = "InvalidFormRequestBody")] + private static partial void InvalidFormRequestBody(ILogger logger, string parameterType, string parameterName, Exception exception); private static ILogger GetLogger(HttpContext httpContext) { @@ -1380,4 +1611,20 @@ private static string BuildErrorMessageForInferredBodyParameter(FactoryContext f .AppendLine(); return errorMessage.ToString(); } + + private static string BuildErrorMessageForFormAndJsonBodyParameters(FactoryContext factoryContext) + { + var errorMessage = new StringBuilder(); + errorMessage.AppendLine("An action cannot use both form and JSON body parameters."); + errorMessage.AppendLine("Below is the list of parameters that we found: "); + errorMessage.AppendLine(); + errorMessage.AppendLine(FormattableString.Invariant($"{"Parameter",-20}| {"Source",-30}")); + errorMessage.AppendLine("---------------------------------------------------------------------------------"); + + foreach (var kv in factoryContext.TrackedParameters) + { + errorMessage.AppendLine(FormattableString.Invariant($"{kv.Key,-19} | {kv.Value,-15}")); + } + return errorMessage.ToString(); + } } diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index 7640eb1c556e..fdb3104e0448 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -7,6 +7,7 @@ using System.IO.Pipelines; using System.Linq.Expressions; using System.Net; +using System.Net.Http; using System.Net.Sockets; using System.Numerics; using System.Reflection; @@ -3096,6 +3097,430 @@ public async Task RequestDelegateCanProcessTimeOnlyValues(Delegate @delegate, st Assert.Equal(expectedResponse, decodedResponseBody); } + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task RequestDelegateLogsIOExceptionsForFormAsDebugDoesNotAbortAndNeverThrows(bool throwOnBadRequests) + { + var invoked = false; + + void TestAction(IFormFile file) + { + invoked = true; + } + + var ioException = new IOException(); + + var httpContext = CreateHttpContext(); + httpContext.Request.Headers["Content-Type"] = "application/x-www-form-urlencoded"; + httpContext.Request.Headers["Content-Length"] = "1"; + httpContext.Request.Body = new ExceptionThrowingRequestBodyStream(ioException); + httpContext.Features.Set(new RequestBodyDetectionFeature(true)); + + var factoryResult = RequestDelegateFactory.Create(TestAction, new() { ThrowOnBadRequest = throwOnBadRequests }); + var requestDelegate = factoryResult.RequestDelegate; + + await requestDelegate(httpContext); + + Assert.False(invoked); + Assert.False(httpContext.RequestAborted.IsCancellationRequested); + + var logMessage = Assert.Single(TestSink.Writes); + Assert.Equal(new EventId(1, "RequestBodyIOException"), logMessage.EventId); + Assert.Equal(LogLevel.Debug, logMessage.LogLevel); + Assert.Equal("Reading the request body failed with an IOException.", logMessage.Message); + Assert.Same(ioException, logMessage.Exception); + } + + [Fact] + public async Task RequestDelegateLogsMalformedFormAsDebugAndSets400Response() + { + var invoked = false; + + void TestAction(IFormFile file) + { + invoked = true; + } + + var httpContext = CreateHttpContext(); + httpContext.Request.Headers["Content-Type"] = "application/x-www-form-urlencoded"; + httpContext.Request.Headers["Content-Length"] = "2049"; + httpContext.Request.Body = new MemoryStream(Encoding.UTF8.GetBytes(new string('x', 2049))); + httpContext.Features.Set(new RequestBodyDetectionFeature(true)); + + var factoryResult = RequestDelegateFactory.Create(TestAction); + var requestDelegate = factoryResult.RequestDelegate; + + await requestDelegate(httpContext); + + Assert.False(invoked); + Assert.False(httpContext.RequestAborted.IsCancellationRequested); + Assert.Equal(400, httpContext.Response.StatusCode); + Assert.False(httpContext.Response.HasStarted); + + var logMessage = Assert.Single(TestSink.Writes); + Assert.Equal(new EventId(8, "InvalidFormRequestBody"), logMessage.EventId); + Assert.Equal(LogLevel.Debug, logMessage.LogLevel); + Assert.Equal(@"Failed to read parameter ""IFormFile file"" from the request body as form.", logMessage.Message); + Assert.IsType(logMessage.Exception); + } + + [Fact] + public async Task RequestDelegateThrowsForMalformedFormIfThrowOnBadRequest() + { + var invoked = false; + + void TestAction(IFormFile file) + { + invoked = true; + } + + var httpContext = CreateHttpContext(); + httpContext.Request.Headers["Content-Type"] = "application/x-www-form-urlencoded"; + httpContext.Request.Headers["Content-Length"] = "2049"; + httpContext.Request.Body = new MemoryStream(Encoding.UTF8.GetBytes(new string('x', 2049))); + httpContext.Features.Set(new RequestBodyDetectionFeature(true)); + + var factoryResult = RequestDelegateFactory.Create(TestAction, new() { ThrowOnBadRequest = true }); + var requestDelegate = factoryResult.RequestDelegate; + + var badHttpRequestException = await Assert.ThrowsAsync(() => requestDelegate(httpContext)); + + Assert.False(invoked); + + // The httpContext should be untouched. + Assert.False(httpContext.RequestAborted.IsCancellationRequested); + Assert.Equal(200, httpContext.Response.StatusCode); + Assert.False(httpContext.Response.HasStarted); + + // We don't log bad requests when we throw. + Assert.Empty(TestSink.Writes); + + Assert.Equal(@"Failed to read parameter ""IFormFile file"" from the request body as form.", badHttpRequestException.Message); + Assert.Equal(400, badHttpRequestException.StatusCode); + Assert.IsType(badHttpRequestException.InnerException); + } + + [Fact] + public void BuildRequestDelegateThrowsInvalidOperationExceptionBodyAndFormFileParameters() + { + void TestFormAndJson(IFormFile value1, Todo value2) { } + void TestFormCollectionAndJson(IFormFileCollection value1, Todo value2) { } + void TestFormAndJsonWithAttribute(IFormFile value1, [FromBody] int value2) { } + void TestJsonAndForm(Todo value1, IFormFile value2) { } + void TestJsonAndFormCollection(Todo value1, IFormFileCollection value2) { } + void TestJsonAndFormWithAttribute(Todo value1, [FromFormFile] IFormFile value2) { } + + Assert.Throws(() => RequestDelegateFactory.Create(TestFormAndJson)); + Assert.Throws(() => RequestDelegateFactory.Create(TestFormAndJsonWithAttribute)); + Assert.Throws(() => RequestDelegateFactory.Create(TestFormCollectionAndJson)); + Assert.Throws(() => RequestDelegateFactory.Create(TestJsonAndForm)); + Assert.Throws(() => RequestDelegateFactory.Create(TestJsonAndFormCollection)); + Assert.Throws(() => RequestDelegateFactory.Create(TestJsonAndFormWithAttribute)); + } + + [Fact] + public async Task RequestDelegatePopulatesFromIFormFileCollectionParameter() + { + IFormFileCollection? formFilesArgument = null; + + void TestAction(IFormFileCollection formFiles) + { + formFilesArgument = formFiles; + } + + var fileContent = new StringContent("hello", Encoding.UTF8, "application/octet-stream"); + var form = new MultipartFormDataContent("some-boundary"); + form.Add(fileContent, "file", "file.txt"); + + var stream = new MemoryStream(); + await form.CopyToAsync(stream); + + stream.Seek(0, SeekOrigin.Begin); + + var httpContext = CreateHttpContext(); + httpContext.Request.Body = stream; + httpContext.Request.Headers["Content-Type"] = "multipart/form-data;boundary=some-boundary"; + httpContext.Features.Set(new RequestBodyDetectionFeature(true)); + + var factoryResult = RequestDelegateFactory.Create(TestAction); + var requestDelegate = factoryResult.RequestDelegate; + + await requestDelegate(httpContext); + + Assert.Equal(httpContext.Request.Form.Files, formFilesArgument); + Assert.NotNull(formFilesArgument!["file"]); + + var allAcceptsMetadata = factoryResult.EndpointMetadata.OfType(); + var acceptsMetadata = Assert.Single(allAcceptsMetadata); + + Assert.NotNull(acceptsMetadata); + Assert.Equal(new[] { "multipart/form-data" }, acceptsMetadata.ContentTypes); + } + + [Fact] + public async Task RequestDelegatePopulatesFromIFormFileParameter() + { + IFormFile? fileArgument = null; + + void TestAction(IFormFile file) + { + fileArgument = file; + } + + var fileContent = new StringContent("hello", Encoding.UTF8, "application/octet-stream"); + var form = new MultipartFormDataContent("some-boundary"); + form.Add(fileContent, "file", "file.txt"); + + var stream = new MemoryStream(); + await form.CopyToAsync(stream); + + stream.Seek(0, SeekOrigin.Begin); + + var httpContext = CreateHttpContext(); + httpContext.Request.Body = stream; + httpContext.Request.Headers["Content-Type"] = "multipart/form-data;boundary=some-boundary"; + httpContext.Features.Set(new RequestBodyDetectionFeature(true)); + + var factoryResult = RequestDelegateFactory.Create(TestAction); + var requestDelegate = factoryResult.RequestDelegate; + + await requestDelegate(httpContext); + + Assert.Equal(httpContext.Request.Form.Files["file"], fileArgument); + Assert.Equal("file.txt", fileArgument!.FileName); + Assert.Equal("file", fileArgument.Name); + } + + [Fact] + public async Task RequestDelegatePopulatesFromOptionalIFormFileParameter() + { + IFormFile? fileArgument = null; + + void TestAction(IFormFile? file) + { + fileArgument = file; + } + + var fileContent = new StringContent("hello", Encoding.UTF8, "application/octet-stream"); + var form = new MultipartFormDataContent("some-boundary"); + form.Add(fileContent, "file", "file.txt"); + + var stream = new MemoryStream(); + await form.CopyToAsync(stream); + + stream.Seek(0, SeekOrigin.Begin); + + var httpContext = CreateHttpContext(); + httpContext.Request.Body = stream; + httpContext.Request.Headers["Content-Type"] = "multipart/form-data;boundary=some-boundary"; + httpContext.Features.Set(new RequestBodyDetectionFeature(true)); + + var factoryResult = RequestDelegateFactory.Create(TestAction); + var requestDelegate = factoryResult.RequestDelegate; + + await requestDelegate(httpContext); + + Assert.Equal(httpContext.Request.Form.Files["file"], fileArgument); + Assert.Equal("file.txt", fileArgument!.FileName); + Assert.Equal("file", fileArgument.Name); + } + + [Fact] + public async Task RequestDelegatePopulatesFromOptionalMissingIFormFileParameter() + { + IFormFile? file1Argument = null; + IFormFile? file2Argument = null; + + void TestAction(IFormFile? file1, IFormFile? file2) + { + file1Argument = file1; + file2Argument = file2; + } + + var fileContent = new StringContent("hello", Encoding.UTF8, "application/octet-stream"); + var form = new MultipartFormDataContent("some-boundary"); + form.Add(fileContent, "file1", "file.txt"); + + var stream = new MemoryStream(); + await form.CopyToAsync(stream); + + stream.Seek(0, SeekOrigin.Begin); + + var httpContext = CreateHttpContext(); + httpContext.Request.Body = stream; + httpContext.Request.Headers["Content-Type"] = "multipart/form-data;boundary=some-boundary"; + httpContext.Features.Set(new RequestBodyDetectionFeature(true)); + + var factoryResult = RequestDelegateFactory.Create(TestAction); + var requestDelegate = factoryResult.RequestDelegate; + + await requestDelegate(httpContext); + + Assert.Equal(httpContext.Request.Form.Files["file1"], file1Argument); + Assert.NotNull(file1Argument); + + Assert.Equal(httpContext.Request.Form.Files["file2"], file2Argument); + Assert.Null(file2Argument); + + var allAcceptsMetadata = factoryResult.EndpointMetadata.OfType(); + var acceptsMetadata = Assert.Single(allAcceptsMetadata); + + Assert.NotNull(acceptsMetadata); + Assert.Equal(new[] { "multipart/form-data" }, acceptsMetadata.ContentTypes); + } + + [Fact] + public async Task RequestDelegatePopulatesFromIFormFileParameterWithMetadata() + { + IFormFile? fileArgument = null; + + void TestAction([FromFormFile(Name = "my_file")] IFormFile file) + { + fileArgument = file; + } + + var fileContent = new StringContent("hello", Encoding.UTF8, "application/octet-stream"); + var form = new MultipartFormDataContent("some-boundary"); + form.Add(fileContent, "my_file", "file.txt"); + + var stream = new MemoryStream(); + await form.CopyToAsync(stream); + + stream.Seek(0, SeekOrigin.Begin); + + var httpContext = CreateHttpContext(); + httpContext.Request.Body = stream; + httpContext.Request.Headers["Content-Type"] = "multipart/form-data;boundary=some-boundary"; + httpContext.Features.Set(new RequestBodyDetectionFeature(true)); + + var factoryResult = RequestDelegateFactory.Create(TestAction); + var requestDelegate = factoryResult.RequestDelegate; + + await requestDelegate(httpContext); + + Assert.Equal(httpContext.Request.Form.Files["my_file"], fileArgument); + Assert.Equal("file.txt", fileArgument!.FileName); + Assert.Equal("my_file", fileArgument.Name); + } + + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task RequestDelegateRejectsNonFormContent(bool shouldThrow) + { + var httpContext = new DefaultHttpContext(); + httpContext.Request.Headers["Content-Type"] = "application/xml"; + httpContext.Request.Headers["Content-Length"] = "1"; + httpContext.Features.Set(new RequestBodyDetectionFeature(true)); + + var serviceCollection = new ServiceCollection(); + serviceCollection.AddSingleton(LoggerFactory); + httpContext.RequestServices = serviceCollection.BuildServiceProvider(); + + var factoryResult = RequestDelegateFactory.Create((HttpContext context, IFormFile file) => + { + }, new RequestDelegateFactoryOptions() { ThrowOnBadRequest = shouldThrow }); + var requestDelegate = factoryResult.RequestDelegate; + + var request = requestDelegate(httpContext); + + if (shouldThrow) + { + var ex = await Assert.ThrowsAsync(() => request); + Assert.Equal("Expected a supported form media type but got \"application/xml\".", ex.Message); + Assert.Equal(StatusCodes.Status415UnsupportedMediaType, ex.StatusCode); + } + else + { + await request; + + Assert.Equal(415, httpContext.Response.StatusCode); + var logMessage = Assert.Single(TestSink.Writes); + Assert.Equal(new EventId(7, "UnexpectedContentType"), logMessage.EventId); + Assert.Equal(LogLevel.Debug, logMessage.LogLevel); + Assert.Equal("Expected a supported form media type but got \"application/xml\".", logMessage.Message); + } + } + + [Fact] + public async Task RequestDelegateSets400ResponseIfRequiredFileNotSpecified() + { + var invoked = false; + + void TestAction(IFormFile file) + { + invoked = true; + } + + var fileContent = new StringContent("hello", Encoding.UTF8, "application/octet-stream"); + var form = new MultipartFormDataContent("some-boundary"); + form.Add(fileContent, "some-other-file", "file.txt"); + + var stream = new MemoryStream(); + await form.CopyToAsync(stream); + + stream.Seek(0, SeekOrigin.Begin); + + var httpContext = CreateHttpContext(); + httpContext.Request.Body = stream; + httpContext.Request.Headers["Content-Type"] = "multipart/form-data;boundary=some-boundary"; + httpContext.Features.Set(new RequestBodyDetectionFeature(true)); + + var factoryResult = RequestDelegateFactory.Create(TestAction); + var requestDelegate = factoryResult.RequestDelegate; + + await requestDelegate(httpContext); + + Assert.False(invoked); + Assert.Equal(400, httpContext.Response.StatusCode); + } + + [Fact] + public async Task RequestDelegatePopulatesFromBothFormFileCollectionAndFormFileParameters() + { + IFormFileCollection? formFilesArgument = null; + IFormFile? fileArgument = null; + + void TestAction(IFormFileCollection formFiles, IFormFile file) + { + formFilesArgument = formFiles; + fileArgument = file; + } + + var fileContent = new StringContent("hello", Encoding.UTF8, "application/octet-stream"); + var form = new MultipartFormDataContent("some-boundary"); + form.Add(fileContent, "file", "file.txt"); + + var stream = new MemoryStream(); + await form.CopyToAsync(stream); + + stream.Seek(0, SeekOrigin.Begin); + + var httpContext = CreateHttpContext(); + httpContext.Request.Body = stream; + httpContext.Request.Headers["Content-Type"] = "multipart/form-data;boundary=some-boundary"; + httpContext.Features.Set(new RequestBodyDetectionFeature(true)); + + var factoryResult = RequestDelegateFactory.Create(TestAction); + var requestDelegate = factoryResult.RequestDelegate; + + await requestDelegate(httpContext); + + Assert.Equal(httpContext.Request.Form.Files, formFilesArgument); + Assert.NotNull(formFilesArgument!["file"]); + + Assert.Equal(httpContext.Request.Form.Files["file"], fileArgument); + Assert.Equal("file.txt", fileArgument!.FileName); + Assert.Equal("file", fileArgument.Name); + + var allAcceptsMetadata = factoryResult.EndpointMetadata.OfType(); + var acceptsMetadata = Assert.Single(allAcceptsMetadata); + + Assert.NotNull(acceptsMetadata); + Assert.Equal(new[] { "multipart/form-data" }, acceptsMetadata.ContentTypes); + } + private DefaultHttpContext CreateHttpContext() { var responseFeature = new TestHttpResponseFeature(); @@ -3217,6 +3642,11 @@ private class FromBodyAttribute : Attribute, IFromBodyMetadata public bool AllowEmpty { get; set; } } + private class FromFormFileAttribute : Attribute, IFromFileMetadata + { + public string? Name { get; set; } + } + private class FromServiceAttribute : Attribute, IFromServiceMetadata { } diff --git a/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs b/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs index 6c960cf77fa0..be81fa5db417 100644 --- a/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs +++ b/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs @@ -114,17 +114,27 @@ private ApiDescription CreateApiDescription(RouteEndpoint routeEndpoint, string var acceptsMetadata = routeEndpoint.Metadata.GetMetadata(); if (acceptsMetadata is not null) { - var acceptsRequestType = acceptsMetadata.RequestType; - var isOptional = acceptsMetadata.IsOptional; - var parameterDescription = new ApiParameterDescription + // Do not add an implicit body parameter description if a parameter already + // exists for something derived from the body's content, such as a form file. + var alreadyHasAnyBodyParameter = apiDescription.ParameterDescriptions.Any(x => + x.Source == BindingSource.Body || + x.Source == BindingSource.Form || + x.Source == BindingSource.FormFile); + + if (!alreadyHasAnyBodyParameter) { - Name = acceptsRequestType is not null ? acceptsRequestType.Name : typeof(void).Name, - ModelMetadata = CreateModelMetadata(acceptsRequestType ?? typeof(void)), - Source = BindingSource.Body, - Type = acceptsRequestType ?? typeof(void), - IsRequired = !isOptional, - }; - apiDescription.ParameterDescriptions.Add(parameterDescription); + var acceptsRequestType = acceptsMetadata.RequestType; + var isOptional = acceptsMetadata.IsOptional; + var parameterDescription = new ApiParameterDescription + { + Name = acceptsRequestType is not null ? acceptsRequestType.Name : typeof(void).Name, + ModelMetadata = CreateModelMetadata(acceptsRequestType ?? typeof(void)), + Source = BindingSource.Body, + Type = acceptsRequestType ?? typeof(void), + IsRequired = !isOptional, + }; + apiDescription.ParameterDescriptions.Add(parameterDescription); + } var supportedRequestFormats = apiDescription.SupportedRequestFormats; @@ -239,12 +249,17 @@ private static ParameterDescriptor CreateParameterDescriptor(ParameterInfo param { return (BindingSource.Body, parameter.Name ?? string.Empty, fromBodyAttribute.AllowEmpty, parameter.ParameterType); } + else if (attributes.OfType().FirstOrDefault() is { } formFileAttribute) + { + return (BindingSource.FormFile, formFileAttribute.Name ?? parameter.Name ?? string.Empty, false, parameter.ParameterType); + } else if (parameter.CustomAttributes.Any(a => typeof(IFromServiceMetadata).IsAssignableFrom(a.AttributeType)) || parameter.ParameterType == typeof(HttpContext) || parameter.ParameterType == typeof(HttpRequest) || parameter.ParameterType == typeof(HttpResponse) || parameter.ParameterType == typeof(ClaimsPrincipal) || parameter.ParameterType == typeof(CancellationToken) || + parameter.ParameterType == typeof(IFormFileCollection) || ParameterBindingMethodCache.HasBindAsyncMethod(parameter) || _serviceProviderIsService?.IsService(parameter.ParameterType) == true) { @@ -265,6 +280,10 @@ private static ParameterDescriptor CreateParameterDescriptor(ParameterInfo param return (BindingSource.Query, parameter.Name ?? string.Empty, false, displayType); } } + else if (parameter.ParameterType == typeof(IFormFile)) + { + return (BindingSource.FormFile, parameter.Name ?? string.Empty, false, parameter.ParameterType); + } else { return (BindingSource.Body, parameter.Name ?? string.Empty, false, parameter.ParameterType); diff --git a/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs b/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs index 8f25def7e576..c89ece0d783c 100644 --- a/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs +++ b/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs @@ -6,6 +6,7 @@ using System.Security.Claims; using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Http.Metadata; using Microsoft.AspNetCore.Mvc.Abstractions; using Microsoft.AspNetCore.Mvc.Infrastructure; using Microsoft.AspNetCore.Mvc.ModelBinding; @@ -783,10 +784,6 @@ public void HandleDefaultIAcceptsMetadataForRequiredBodyParameter() Assert.Equal("application/json", defaultRequestFormat.MediaType); } -#nullable restore - -#nullable enable - [Fact] public void HandleDefaultIAcceptsMetadataForOptionalBodyParameter() { @@ -821,10 +818,6 @@ public void HandleDefaultIAcceptsMetadataForOptionalBodyParameter() Assert.Equal("application/json", defaultRequestFormat.MediaType); } -#nullable restore - -#nullable enable - [Fact] public void HandleIAcceptsMetadataWithConsumesAttributeAndInferredOptionalFromBodyType() { @@ -859,6 +852,185 @@ public void HandleIAcceptsMetadataWithConsumesAttributeAndInferredOptionalFromBo Assert.Equal("application/xml", defaultRequestFormat.MediaType); } + [Fact] + public void HandleDefaultIAcceptsMetadataForRequiredFormFileParameter() + { + // Arrange + var services = new ServiceCollection(); + var serviceProvider = services.BuildServiceProvider(); + var builder = new TestEndpointRouteBuilder(new ApplicationBuilder(serviceProvider)); + builder.MapPost("/file/upload", (IFormFile formFile) => ""); + var context = new ApiDescriptionProviderContext(Array.Empty()); + + var endpointDataSource = builder.DataSources.OfType().Single(); + var provider = CreateEndpointMetadataApiDescriptionProvider(endpointDataSource); + + // Act + provider.OnProvidersExecuting(context); + provider.OnProvidersExecuted(context); + + // Assert + var parameterDescriptions = context.Results.SelectMany(r => r.ParameterDescriptions); + var bodyParameterDescription = parameterDescriptions.Single(); + Assert.Equal(typeof(IFormFile), bodyParameterDescription.Type); + Assert.Equal("formFile", bodyParameterDescription.Name); + Assert.True(bodyParameterDescription.IsRequired); + + // Assert + var requestFormats = context.Results.SelectMany(r => r.SupportedRequestFormats); + var defaultRequestFormat = requestFormats.Single(); + Assert.Equal("multipart/form-data", defaultRequestFormat.MediaType); + Assert.Null(defaultRequestFormat.Formatter); + } + + [Fact] + public void HandleDefaultIAcceptsMetadataForOptionalFormFileParameter() + { + // Arrange + var services = new ServiceCollection(); + var serviceProvider = services.BuildServiceProvider(); + var builder = new TestEndpointRouteBuilder(new ApplicationBuilder(serviceProvider)); + builder.MapPost("/file/upload", (IFormFile? inferredFormFile) => ""); + var context = new ApiDescriptionProviderContext(Array.Empty()); + + var endpointDataSource = builder.DataSources.OfType().Single(); + var provider = CreateEndpointMetadataApiDescriptionProvider(endpointDataSource); + + // Act + provider.OnProvidersExecuting(context); + provider.OnProvidersExecuted(context); + + // Assert + var parameterDescriptions = context.Results.SelectMany(r => r.ParameterDescriptions); + var bodyParameterDescription = parameterDescriptions.Single(); + Assert.Equal(typeof(IFormFile), bodyParameterDescription.Type); + Assert.Equal("inferredFormFile", bodyParameterDescription.Name); + Assert.False(bodyParameterDescription.IsRequired); + + // Assert + var requestFormats = context.Results.SelectMany(r => r.SupportedRequestFormats); + var defaultRequestFormat = requestFormats.Single(); + Assert.Equal("multipart/form-data", defaultRequestFormat.MediaType); + Assert.Null(defaultRequestFormat.Formatter); + } + + [Fact] + public void AddsMultipartFormDataResponseFormatWhenFormFileSpecified() + { + // Arrange + var services = new ServiceCollection(); + var serviceProvider = services.BuildServiceProvider(); + var builder = new TestEndpointRouteBuilder(new ApplicationBuilder(serviceProvider)); + builder.MapPost("/file/upload", (IFormFile file) => Results.NoContent()); + var context = new ApiDescriptionProviderContext(Array.Empty()); + + var endpointDataSource = builder.DataSources.OfType().Single(); + var provider = CreateEndpointMetadataApiDescriptionProvider(endpointDataSource); + + // Act + provider.OnProvidersExecuting(context); + provider.OnProvidersExecuted(context); + + // Assert + var parameterDescriptions = context.Results.SelectMany(r => r.ParameterDescriptions); + var bodyParameterDescription = parameterDescriptions.Single(); + Assert.Equal(typeof(IFormFile), bodyParameterDescription.Type); + Assert.Equal("file", bodyParameterDescription.Name); + Assert.True(bodyParameterDescription.IsRequired); + + // Assert + var requestFormats = context.Results.SelectMany(r => r.SupportedRequestFormats); + var defaultRequestFormat = requestFormats.Single(); + Assert.Equal("multipart/form-data", defaultRequestFormat.MediaType); + Assert.Null(defaultRequestFormat.Formatter); + } + + [Fact] + public void HasMultipleRequestFormatsWhenFormFileSpecifiedWithConsumedAttribute() + { + var apiDescription = GetApiDescription( + [Consumes("application/custom0", "application/custom1")] + (IFormFile file) => Results.NoContent()); + + Assert.Equal(2, apiDescription.SupportedRequestFormats.Count); + + var requestFormat0 = apiDescription.SupportedRequestFormats[0]; + Assert.Equal("application/custom0", requestFormat0.MediaType); + Assert.Null(requestFormat0.Formatter); + + var requestFormat1 = apiDescription.SupportedRequestFormats[1]; + Assert.Equal("application/custom1", requestFormat1.MediaType); + Assert.Null(requestFormat1.Formatter); + } + + [Fact] + public void TestIsRequiredFromFormFile() + { + var apiDescription0 = GetApiDescription((IFormFile fromFile) => { }); + var apiDescription1 = GetApiDescription((IFormFile? fromFile) => { }); + Assert.Equal(1, apiDescription0.ParameterDescriptions.Count); + Assert.Equal(1, apiDescription1.ParameterDescriptions.Count); + + var fromFileParam0 = apiDescription0.ParameterDescriptions[0]; + Assert.Equal(typeof(IFormFile), fromFileParam0.Type); + Assert.Equal(typeof(IFormFile), fromFileParam0.ModelMetadata.ModelType); + Assert.Equal(BindingSource.FormFile, fromFileParam0.Source); + Assert.True(fromFileParam0.IsRequired); + + var fromFileParam1 = apiDescription1.ParameterDescriptions[0]; + Assert.Equal(typeof(IFormFile), fromFileParam1.Type); + Assert.Equal(typeof(IFormFile), fromFileParam1.ModelMetadata.ModelType); + Assert.Equal(BindingSource.FormFile, fromFileParam1.Source); + Assert.False(fromFileParam1.IsRequired); + } + + [Fact] + public void AddsFromFormParameterAsFormFile() + { + static void AssertFormFileParameter(ApiDescription apiDescription, Type expectedType, string expectedName) + { + var param = Assert.Single(apiDescription.ParameterDescriptions); + Assert.Equal(expectedType, param.Type); + Assert.Equal(expectedType, param.ModelMetadata.ModelType); + Assert.Equal(BindingSource.FormFile, param.Source); + Assert.Equal(expectedName, param.Name); + } + + AssertFormFileParameter(GetApiDescription((IFormFile file) => { }), typeof(IFormFile), "file"); + AssertFormFileParameter(GetApiDescription(([FromFile(Name = "file_name")] IFormFile file) => { }), typeof(IFormFile), "file_name"); + } + + [Fact] + public void AddsMultipartFormDataResponseFormatWhenFormFileCollectionSpecified() + { + // Arrange + var services = new ServiceCollection(); + var serviceProvider = services.BuildServiceProvider(); + var builder = new TestEndpointRouteBuilder(new ApplicationBuilder(serviceProvider)); + builder.MapPost("/file/upload", (IFormFileCollection files) => Results.NoContent()); + var context = new ApiDescriptionProviderContext(Array.Empty()); + + var endpointDataSource = builder.DataSources.OfType().Single(); + var provider = CreateEndpointMetadataApiDescriptionProvider(endpointDataSource); + + // Act + provider.OnProvidersExecuting(context); + provider.OnProvidersExecuted(context); + + // Assert + var parameterDescriptions = context.Results.SelectMany(r => r.ParameterDescriptions); + var bodyParameterDescription = parameterDescriptions.Single(); + Assert.Equal(typeof(IFormFileCollection), bodyParameterDescription.Type); + Assert.Equal(typeof(IFormFileCollection).Name, bodyParameterDescription.Name); + Assert.True(bodyParameterDescription.IsRequired); + + // Assert + var requestFormats = context.Results.SelectMany(r => r.SupportedRequestFormats); + var defaultRequestFormat = requestFormats.Single(); + Assert.Equal("multipart/form-data", defaultRequestFormat.MediaType); + Assert.Null(defaultRequestFormat.Formatter); + } + #nullable restore [Fact] @@ -928,6 +1100,12 @@ public void HandlesEndpointWithRouteConstraints() constraint => Assert.IsType(constraint)); } + // TODO Can remove this when there's an agreed concrete implementation of IFromFileMetadata + private sealed class FromFileAttribute : Attribute, IFromFileMetadata + { + public string Name { get; set; } + } + private static IEnumerable GetSortedMediaTypes(ApiResponseType apiResponseType) { return apiResponseType.ApiResponseFormats @@ -970,7 +1148,6 @@ private static IList GetApiDescriptions( private static TestEndpointRouteBuilder CreateBuilder() => new TestEndpointRouteBuilder(new ApplicationBuilder(new TestServiceProvider())); - private static ApiDescription GetApiDescription(Delegate action, string pattern = null, string displayName = null) => Assert.Single(GetApiDescriptions(action, pattern, displayName: displayName)); diff --git a/src/Mvc/test/Mvc.FunctionalTests/SimpleWithWebApplicationBuilderTests.cs b/src/Mvc/test/Mvc.FunctionalTests/SimpleWithWebApplicationBuilderTests.cs index dddaaf110dbd..367fcdf4f79c 100644 --- a/src/Mvc/test/Mvc.FunctionalTests/SimpleWithWebApplicationBuilderTests.cs +++ b/src/Mvc/test/Mvc.FunctionalTests/SimpleWithWebApplicationBuilderTests.cs @@ -247,4 +247,23 @@ public async Task Accepts_NonJsonMediaType() // Assert await response.AssertStatusCodeAsync(HttpStatusCode.Accepted); } + + [Fact] + public async Task FileUpload_Works() + { + // Arrange + var expected = "42"; + var content = new MultipartFormDataContent(); + content.Add(new StringContent(new string('a', 42)), "file", "file.txt"); + + using var client = _fixture.CreateDefaultClient(); + + // Act + var response = await client.PostAsync("/fileupload", content); + + // Assert + await response.AssertStatusCodeAsync(HttpStatusCode.OK); + var actual = await response.Content.ReadAsStringAsync(); + Assert.Equal(expected, actual); + } } diff --git a/src/Mvc/test/WebSites/SimpleWebSiteWithWebApplicationBuilder/Program.cs b/src/Mvc/test/WebSites/SimpleWebSiteWithWebApplicationBuilder/Program.cs index 71d77264a07f..768b1dc7fb97 100644 --- a/src/Mvc/test/WebSites/SimpleWebSiteWithWebApplicationBuilder/Program.cs +++ b/src/Mvc/test/WebSites/SimpleWebSiteWithWebApplicationBuilder/Program.cs @@ -41,6 +41,12 @@ app.MapPost("/accepts-default", (Person person) => Results.Ok(person.Name)); app.MapPost("/accepts-xml", () => Accepted()).Accepts("application/xml"); +app.MapPost("/fileupload", async (IFormFile file) => +{ + await using var uploadStream = file.OpenReadStream(); + return uploadStream.Length; +}); + app.Run(); record Person(string Name, int Age); From 8a0b5988eccdcc6c0444939e6ad8e45b0fa78a39 Mon Sep 17 00:00:00 2001 From: martincostello Date: Sat, 6 Nov 2021 15:14:06 +0000 Subject: [PATCH 02/23] Remove dead branch Method is never called for IFormFileCollection. --- src/Http/Http.Extensions/src/RequestDelegateFactory.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index aae379ee94a9..48a77c2a69ef 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -806,8 +806,7 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres var parameterNameConstant = Expression.Constant(parameter.Name); var sourceConstant = Expression.Constant(source); - if (parameter.ParameterType == typeof(string) || - parameter.ParameterType == typeof(IFormFileCollection)) + if (parameter.ParameterType == typeof(string)) { return BindParameterFromExpression(parameter, valueExpression, factoryContext, source); } From f531d3a559ded341178f7e3e36a6bf8b25f442c2 Mon Sep 17 00:00:00 2001 From: martincostello Date: Sat, 6 Nov 2021 15:14:57 +0000 Subject: [PATCH 03/23] Add test case for two IFormFile values Add an explicit test case for handling multiple files. --- .../test/RequestDelegateFactoryTests.cs | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index fdb3104e0448..e1d731e630a1 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -3326,6 +3326,48 @@ void TestAction(IFormFile? file) Assert.Equal("file", fileArgument.Name); } + [Fact] + public async Task RequestDelegatePopulatesFromMultipleRequiredIFormFileParameters() + { + IFormFile? file1Argument = null; + IFormFile? file2Argument = null; + + void TestAction(IFormFile file1, IFormFile file2) + { + file1Argument = file1; + file2Argument = file2; + } + + var fileContent1 = new StringContent("hello", Encoding.UTF8, "application/octet-stream"); + var fileContent2 = new StringContent("there", Encoding.UTF8, "application/octet-stream"); + var form = new MultipartFormDataContent("some-boundary"); + form.Add(fileContent1, "file1", "file1.txt"); + form.Add(fileContent2, "file2", "file2.txt"); + + var stream = new MemoryStream(); + await form.CopyToAsync(stream); + + stream.Seek(0, SeekOrigin.Begin); + + var httpContext = CreateHttpContext(); + httpContext.Request.Body = stream; + httpContext.Request.Headers["Content-Type"] = "multipart/form-data;boundary=some-boundary"; + httpContext.Features.Set(new RequestBodyDetectionFeature(true)); + + var factoryResult = RequestDelegateFactory.Create(TestAction); + var requestDelegate = factoryResult.RequestDelegate; + + await requestDelegate(httpContext); + + Assert.Equal(httpContext.Request.Form.Files["file1"], file1Argument); + Assert.Equal("file1.txt", file1Argument!.FileName); + Assert.Equal("file1", file1Argument.Name); + + Assert.Equal(httpContext.Request.Form.Files["file2"], file2Argument); + Assert.Equal("file2.txt", file2Argument!.FileName); + Assert.Equal("file2", file2Argument.Name); + } + [Fact] public async Task RequestDelegatePopulatesFromOptionalMissingIFormFileParameter() { From c4ac0043a239023ea89c841b3b633e0e9094a6d5 Mon Sep 17 00:00:00 2001 From: martincostello Date: Sat, 6 Nov 2021 15:16:11 +0000 Subject: [PATCH 04/23] Fix IFormFile with bound parameters Fix copy-pasta which broke form binding when using BindAsync() and add test. --- .../src/RequestDelegateFactory.cs | 2 +- .../test/RequestDelegateFactoryTests.cs | 56 +++++++++++++++++++ 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 48a77c2a69ef..2305f0449a51 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -712,7 +712,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, var feature = httpContext.Features.Get(); if (feature?.CanHaveBody == true) { - if (!httpContext.Request.HasJsonContentType()) + if (!httpContext.Request.HasFormContentType) { Log.UnexpectedFormContentType(httpContext, httpContext.Request.ContentType, factoryContext.ThrowOnBadRequest); httpContext.Response.StatusCode = StatusCodes.Status415UnsupportedMediaType; diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index e1d731e630a1..4151a84fd5ea 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -3446,6 +3446,62 @@ void TestAction([FromFormFile(Name = "my_file")] IFormFile file) Assert.Equal("my_file", fileArgument.Name); } + [Fact] + public async Task RequestDelegatePopulatesFromIFormFileAndBoundParameter() + { + IFormFile? fileArgument = null; + TraceIdentifier traceIdArgument = default; + + void TestAction(IFormFile? file, TraceIdentifier traceId) + { + fileArgument = file; + traceIdArgument = traceId; + } + + var fileContent = new StringContent("hello", Encoding.UTF8, "application/octet-stream"); + var form = new MultipartFormDataContent("some-boundary"); + form.Add(fileContent, "file", "file.txt"); + + var stream = new MemoryStream(); + await form.CopyToAsync(stream); + + stream.Seek(0, SeekOrigin.Begin); + + var httpContext = CreateHttpContext(); + httpContext.Request.Body = stream; + httpContext.Request.Headers["Content-Type"] = "multipart/form-data;boundary=some-boundary"; + httpContext.Features.Set(new RequestBodyDetectionFeature(true)); + httpContext.TraceIdentifier = "my-trace-id"; + + var factoryResult = RequestDelegateFactory.Create(TestAction); + var requestDelegate = factoryResult.RequestDelegate; + + await requestDelegate(httpContext); + + Assert.Equal(httpContext.Request.Form.Files["file"], fileArgument); + Assert.Equal("file.txt", fileArgument!.FileName); + Assert.Equal("file", fileArgument.Name); + + Assert.Equal("my-trace-id", traceIdArgument.Id); + } + + private readonly struct TraceIdentifier + { + private TraceIdentifier(string id) + { + Id = id; + } + + public string Id { get; } + + public static implicit operator string(TraceIdentifier value) => value.Id; + + public static ValueTask BindAsync(HttpContext context) + { + return ValueTask.FromResult(new TraceIdentifier(context.TraceIdentifier)); + } + } + [Theory] [InlineData(true)] [InlineData(false)] From ce19f22a8326c4d4c6a50c4cfb568a53d5f2398e Mon Sep 17 00:00:00 2001 From: martincostello Date: Sat, 6 Nov 2021 15:30:05 +0000 Subject: [PATCH 05/23] Deduplicate JSON/form body reading Deduplicate the methods for reading the body as JSON or as a form for with and without bound parameters. --- .../src/RequestDelegateFactory.cs | 210 ++++++++++-------- 1 file changed, 114 insertions(+), 96 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 2305f0449a51..78082a17f3ad 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -608,31 +608,17 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, boundValues[i] = await binders[i](httpContext); } - var bodyValue = defaultBodyValue; - var feature = httpContext.Features.Get(); - if (feature?.CanHaveBody == true) + (var bodyValue, var successful) = await TryReadBodyAsync( + parameterTypeName, + parameterName, + bodyType, + defaultBodyValue, + httpContext, + factoryContext.ThrowOnBadRequest); + + if (!successful) { - if (!httpContext.Request.HasJsonContentType()) - { - Log.UnexpectedJsonContentType(httpContext, httpContext.Request.ContentType, factoryContext.ThrowOnBadRequest); - httpContext.Response.StatusCode = StatusCodes.Status415UnsupportedMediaType; - return; - } - try - { - bodyValue = await httpContext.Request.ReadFromJsonAsync(bodyType); - } - catch (IOException ex) - { - Log.RequestBodyIOException(httpContext, ex); - return; - } - catch (JsonException ex) - { - Log.InvalidJsonRequestBody(httpContext, parameterTypeName, parameterName, ex, factoryContext.ThrowOnBadRequest); - httpContext.Response.StatusCode = StatusCodes.Status400BadRequest; - return; - } + return; } await continuation(target, httpContext, bodyValue, boundValues); @@ -646,35 +632,61 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, return async (target, httpContext) => { - var bodyValue = defaultBodyValue; - var feature = httpContext.Features.Get(); - if (feature?.CanHaveBody == true) + (var bodyValue, var successful) = await TryReadBodyAsync( + parameterTypeName, + parameterName, + bodyType, + defaultBodyValue, + httpContext, + factoryContext.ThrowOnBadRequest); + + if (!successful) { - if (!httpContext.Request.HasJsonContentType()) - { - Log.UnexpectedJsonContentType(httpContext, httpContext.Request.ContentType, factoryContext.ThrowOnBadRequest); - httpContext.Response.StatusCode = StatusCodes.Status415UnsupportedMediaType; - return; - } - try - { - bodyValue = await httpContext.Request.ReadFromJsonAsync(bodyType); - } - catch (IOException ex) - { - Log.RequestBodyIOException(httpContext, ex); - return; - } - catch (JsonException ex) - { - Log.InvalidJsonRequestBody(httpContext, parameterTypeName, parameterName, ex, factoryContext.ThrowOnBadRequest); - httpContext.Response.StatusCode = StatusCodes.Status400BadRequest; - return; - } + return; } + await continuation(target, httpContext, bodyValue); }; } + + static async Task<(object? FormValue, bool Successful)> TryReadBodyAsync( + string parameterTypeName, + string parameterName, + Type bodyType, + object? defaultBodyValue, + HttpContext httpContext, + bool throwOnBadRequest) + { + var bodyValue = defaultBodyValue; + var feature = httpContext.Features.Get(); + + if (feature?.CanHaveBody == true) + { + if (!httpContext.Request.HasJsonContentType()) + { + Log.UnexpectedJsonContentType(httpContext, httpContext.Request.ContentType, throwOnBadRequest); + httpContext.Response.StatusCode = StatusCodes.Status415UnsupportedMediaType; + return (null, false); + } + try + { + bodyValue = await httpContext.Request.ReadFromJsonAsync(bodyType); + } + catch (IOException ex) + { + Log.RequestBodyIOException(httpContext, ex); + return (null, false); + } + catch (JsonException ex) + { + Log.InvalidJsonRequestBody(httpContext, parameterTypeName, parameterName, ex, throwOnBadRequest); + httpContext.Response.StatusCode = StatusCodes.Status400BadRequest; + return (null, false); + } + } + + return (bodyValue, true); + } } private static Func HandleRequestBodyAndCompileRequestDelegateForForm( @@ -708,31 +720,15 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, boundValues[i] = await binders[i](httpContext); } - object? formValue = null; - var feature = httpContext.Features.Get(); - if (feature?.CanHaveBody == true) + (var formValue, var successful) = await TryReadFormAsync( + parameterTypeName, + parameterName, + httpContext, + factoryContext.ThrowOnBadRequest); + + if (!successful) { - if (!httpContext.Request.HasFormContentType) - { - Log.UnexpectedFormContentType(httpContext, httpContext.Request.ContentType, factoryContext.ThrowOnBadRequest); - httpContext.Response.StatusCode = StatusCodes.Status415UnsupportedMediaType; - return; - } - try - { - formValue = await httpContext.Request.ReadFormAsync(); - } - catch (IOException ex) - { - Log.RequestBodyIOException(httpContext, ex); - return; - } - catch (InvalidDataException ex) - { - Log.InvalidFormRequestBody(httpContext, parameterTypeName, parameterName, ex, factoryContext.ThrowOnBadRequest); - httpContext.Response.StatusCode = StatusCodes.Status400BadRequest; - return; - } + return; } await continuation(target, httpContext, formValue, boundValues); @@ -746,35 +742,57 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, return async (target, httpContext) => { - object? formValue = null; - var feature = httpContext.Features.Get(); - if (feature?.CanHaveBody == true) + (var formValue, var successful) = await TryReadFormAsync( + parameterTypeName, + parameterName, + httpContext, + factoryContext.ThrowOnBadRequest); + + if (!successful) { - if (!httpContext.Request.HasFormContentType) - { - Log.UnexpectedFormContentType(httpContext, httpContext.Request.ContentType, factoryContext.ThrowOnBadRequest); - httpContext.Response.StatusCode = StatusCodes.Status415UnsupportedMediaType; - return; - } - try - { - formValue = await httpContext.Request.ReadFormAsync(); - } - catch (IOException ex) - { - Log.RequestBodyIOException(httpContext, ex); - return; - } - catch (InvalidDataException ex) - { - Log.InvalidFormRequestBody(httpContext, parameterTypeName, parameterName, ex, factoryContext.ThrowOnBadRequest); - httpContext.Response.StatusCode = StatusCodes.Status400BadRequest; - return; - } + return; } + await continuation(target, httpContext, formValue); }; } + + static async Task<(object? FormValue, bool Successful)> TryReadFormAsync( + string parameterTypeName, + string parameterName, + HttpContext httpContext, + bool throwOnBadRequest) + { + object? formValue = null; + var feature = httpContext.Features.Get(); + + if (feature?.CanHaveBody == true) + { + if (!httpContext.Request.HasFormContentType) + { + Log.UnexpectedFormContentType(httpContext, httpContext.Request.ContentType, throwOnBadRequest); + httpContext.Response.StatusCode = StatusCodes.Status415UnsupportedMediaType; + return (null, false); + } + try + { + formValue = await httpContext.Request.ReadFormAsync(); + } + catch (IOException ex) + { + Log.RequestBodyIOException(httpContext, ex); + return (null, false); + } + catch (InvalidDataException ex) + { + Log.InvalidFormRequestBody(httpContext, parameterTypeName, parameterName, ex, throwOnBadRequest); + httpContext.Response.StatusCode = StatusCodes.Status400BadRequest; + return (null, false); + } + } + + return (formValue, true); + } } private static Expression GetValueFromProperty(Expression sourceExpression, string key, Type? returnType = null) From fc017c2ab54f6ab076f059a2ba7c49683645ade4 Mon Sep 17 00:00:00 2001 From: martincostello Date: Sat, 6 Nov 2021 15:37:46 +0000 Subject: [PATCH 06/23] Add more invalid parameter tests Add more tests for invalid body parameter combinations with multiple files. --- .../test/RequestDelegateFactoryTests.cs | 30 +++++++++++-------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index 4151a84fd5ea..8700414044c9 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -3204,19 +3204,23 @@ void TestAction(IFormFile file) [Fact] public void BuildRequestDelegateThrowsInvalidOperationExceptionBodyAndFormFileParameters() { - void TestFormAndJson(IFormFile value1, Todo value2) { } - void TestFormCollectionAndJson(IFormFileCollection value1, Todo value2) { } - void TestFormAndJsonWithAttribute(IFormFile value1, [FromBody] int value2) { } - void TestJsonAndForm(Todo value1, IFormFile value2) { } - void TestJsonAndFormCollection(Todo value1, IFormFileCollection value2) { } - void TestJsonAndFormWithAttribute(Todo value1, [FromFormFile] IFormFile value2) { } - - Assert.Throws(() => RequestDelegateFactory.Create(TestFormAndJson)); - Assert.Throws(() => RequestDelegateFactory.Create(TestFormAndJsonWithAttribute)); - Assert.Throws(() => RequestDelegateFactory.Create(TestFormCollectionAndJson)); - Assert.Throws(() => RequestDelegateFactory.Create(TestJsonAndForm)); - Assert.Throws(() => RequestDelegateFactory.Create(TestJsonAndFormCollection)); - Assert.Throws(() => RequestDelegateFactory.Create(TestJsonAndFormWithAttribute)); + void TestFormFileAndJson(IFormFile value1, Todo value2) { } + void TestFormFilesAndJson(IFormFile value1, IFormFile value2, Todo value3) { } + void TestFormFileCollectionAndJson(IFormFileCollection value1, Todo value2) { } + void TestFormFileAndJsonWithAttribute(IFormFile value1, [FromBody] int value2) { } + void TestJsonAndFormFile(Todo value1, IFormFile value2) { } + void TestJsonAndFormFiles(Todo value1, IFormFile value2, IFormFile value3) { } + void TestJsonAndFormFileCollection(Todo value1, IFormFileCollection value2) { } + void TestJsonAndFormFileWithAttribute(Todo value1, [FromFormFile] IFormFile value2) { } + + Assert.Throws(() => RequestDelegateFactory.Create(TestFormFileAndJson)); + Assert.Throws(() => RequestDelegateFactory.Create(TestFormFilesAndJson)); + Assert.Throws(() => RequestDelegateFactory.Create(TestFormFileAndJsonWithAttribute)); + Assert.Throws(() => RequestDelegateFactory.Create(TestFormFileCollectionAndJson)); + Assert.Throws(() => RequestDelegateFactory.Create(TestJsonAndFormFile)); + Assert.Throws(() => RequestDelegateFactory.Create(TestJsonAndFormFiles)); + Assert.Throws(() => RequestDelegateFactory.Create(TestJsonAndFormFileCollection)); + Assert.Throws(() => RequestDelegateFactory.Create(TestJsonAndFormFileWithAttribute)); } [Fact] From 599e72c17741089a40c18e0da36659e59fa11d58 Mon Sep 17 00:00:00 2001 From: martincostello Date: Sat, 6 Nov 2021 15:48:43 +0000 Subject: [PATCH 07/23] Reduce arguments on TryRead*Async methods Just accept the HttpContext and the FactoryContext and do the work with them in the static local methods, rather than in the caller. --- .../src/RequestDelegateFactory.cs | 86 +++++++------------ 1 file changed, 32 insertions(+), 54 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 78082a17f3ad..e96c91f9e120 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -575,19 +575,6 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, { Debug.Assert(factoryContext.JsonRequestBodyParameter is not null, "factoryContext.JsonRequestBodyParameter is null for a JSON body."); - var bodyType = factoryContext.JsonRequestBodyParameter.ParameterType; - var parameterTypeName = TypeNameHelper.GetTypeDisplayName(factoryContext.JsonRequestBodyParameter.ParameterType, fullName: false); - var parameterName = factoryContext.JsonRequestBodyParameter.Name; - - Debug.Assert(parameterName is not null, "CreateArgument() should throw if parameter.Name is null."); - - object? defaultBodyValue = null; - - if (factoryContext.AllowEmptyRequestBody && bodyType.IsValueType) - { - defaultBodyValue = Activator.CreateInstance(bodyType); - } - if (factoryContext.ParameterBinders.Count > 0) { // We need to generate the code for reading from the body before calling into the delegate @@ -608,13 +595,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, boundValues[i] = await binders[i](httpContext); } - (var bodyValue, var successful) = await TryReadBodyAsync( - parameterTypeName, - parameterName, - bodyType, - defaultBodyValue, - httpContext, - factoryContext.ThrowOnBadRequest); + (var bodyValue, var successful) = await TryReadBodyAsync(httpContext, factoryContext); if (!successful) { @@ -632,13 +613,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, return async (target, httpContext) => { - (var bodyValue, var successful) = await TryReadBodyAsync( - parameterTypeName, - parameterName, - bodyType, - defaultBodyValue, - httpContext, - factoryContext.ThrowOnBadRequest); + (var bodyValue, var successful) = await TryReadBodyAsync(httpContext, factoryContext); if (!successful) { @@ -650,13 +625,24 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, } static async Task<(object? FormValue, bool Successful)> TryReadBodyAsync( - string parameterTypeName, - string parameterName, - Type bodyType, - object? defaultBodyValue, HttpContext httpContext, - bool throwOnBadRequest) + FactoryContext factoryContext) { + Debug.Assert(factoryContext.JsonRequestBodyParameter is not null, "TryReadFormAsync() should not be called if there is no JSON body parameter."); + + var bodyType = factoryContext.JsonRequestBodyParameter.ParameterType; + var parameterTypeName = TypeNameHelper.GetTypeDisplayName(factoryContext.JsonRequestBodyParameter.ParameterType, fullName: false); + var parameterName = factoryContext.JsonRequestBodyParameter.Name; + + Debug.Assert(parameterName is not null, "CreateArgument() should throw if parameter.Name is null."); + + object? defaultBodyValue = null; + + if (factoryContext.AllowEmptyRequestBody && bodyType.IsValueType) + { + defaultBodyValue = Activator.CreateInstance(bodyType); + } + var bodyValue = defaultBodyValue; var feature = httpContext.Features.Get(); @@ -664,7 +650,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, { if (!httpContext.Request.HasJsonContentType()) { - Log.UnexpectedJsonContentType(httpContext, httpContext.Request.ContentType, throwOnBadRequest); + Log.UnexpectedJsonContentType(httpContext, httpContext.Request.ContentType, factoryContext.ThrowOnBadRequest); httpContext.Response.StatusCode = StatusCodes.Status415UnsupportedMediaType; return (null, false); } @@ -679,7 +665,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, } catch (JsonException ex) { - Log.InvalidJsonRequestBody(httpContext, parameterTypeName, parameterName, ex, throwOnBadRequest); + Log.InvalidJsonRequestBody(httpContext, parameterTypeName, parameterName, ex, factoryContext.ThrowOnBadRequest); httpContext.Response.StatusCode = StatusCodes.Status400BadRequest; return (null, false); } @@ -695,11 +681,6 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, { Debug.Assert(factoryContext.FormRequestBodyParameter is not null, "factoryContext.FormRequestBodyParameter is null for a form body."); - var parameterTypeName = TypeNameHelper.GetTypeDisplayName(factoryContext.FormRequestBodyParameter.ParameterType, fullName: false); - var parameterName = factoryContext.FormRequestBodyParameter.Name; - - Debug.Assert(parameterName is not null, "CreateArgument() should throw if parameter.Name is null."); - if (factoryContext.ParameterBinders.Count > 0) { // We need to generate the code for reading from the body or form before calling into the delegate @@ -720,11 +701,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, boundValues[i] = await binders[i](httpContext); } - (var formValue, var successful) = await TryReadFormAsync( - parameterTypeName, - parameterName, - httpContext, - factoryContext.ThrowOnBadRequest); + (var formValue, var successful) = await TryReadFormAsync(httpContext, factoryContext); if (!successful) { @@ -742,11 +719,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, return async (target, httpContext) => { - (var formValue, var successful) = await TryReadFormAsync( - parameterTypeName, - parameterName, - httpContext, - factoryContext.ThrowOnBadRequest); + (var formValue, var successful) = await TryReadFormAsync(httpContext, factoryContext); if (!successful) { @@ -758,11 +731,16 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, } static async Task<(object? FormValue, bool Successful)> TryReadFormAsync( - string parameterTypeName, - string parameterName, HttpContext httpContext, - bool throwOnBadRequest) + FactoryContext factoryContext) { + Debug.Assert(factoryContext.FormRequestBodyParameter is not null, "TryReadFormAsync() should not be called if there are no form parameters."); + + var parameterTypeName = TypeNameHelper.GetTypeDisplayName(factoryContext.FormRequestBodyParameter.ParameterType, fullName: false); + var parameterName = factoryContext.FormRequestBodyParameter.Name; + + Debug.Assert(parameterName is not null, "CreateArgument() should throw if parameter.Name is null."); + object? formValue = null; var feature = httpContext.Features.Get(); @@ -770,7 +748,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, { if (!httpContext.Request.HasFormContentType) { - Log.UnexpectedFormContentType(httpContext, httpContext.Request.ContentType, throwOnBadRequest); + Log.UnexpectedFormContentType(httpContext, httpContext.Request.ContentType, factoryContext.ThrowOnBadRequest); httpContext.Response.StatusCode = StatusCodes.Status415UnsupportedMediaType; return (null, false); } @@ -785,7 +763,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, } catch (InvalidDataException ex) { - Log.InvalidFormRequestBody(httpContext, parameterTypeName, parameterName, ex, throwOnBadRequest); + Log.InvalidFormRequestBody(httpContext, parameterTypeName, parameterName, ex, factoryContext.ThrowOnBadRequest); httpContext.Response.StatusCode = StatusCodes.Status400BadRequest; return (null, false); } From 8129be9b3e536251622de3eb8a68a18f55f7e274 Mon Sep 17 00:00:00 2001 From: martincostello Date: Sat, 6 Nov 2021 15:51:51 +0000 Subject: [PATCH 08/23] Clarify property name Clarify that the parameter is just the first body parameter and is used for error reporting. Do not overwrite the first value found if there are multiple values. --- .../src/RequestDelegateFactory.cs | 26 +++++++++++++------ 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index e96c91f9e120..f50d6c70b625 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -199,7 +199,7 @@ private static Expression[] CreateArguments(ParameterInfo[]? parameters, Factory throw new InvalidOperationException(errorMessage); } if (factoryContext.JsonRequestBodyParameter is not null && - factoryContext.FormRequestBodyParameter is not null) + factoryContext.FirstFormRequestBodyParameter is not null) { var errorMessage = BuildErrorMessageForFormAndJsonBodyParameters(factoryContext); throw new InvalidOperationException(errorMessage); @@ -679,7 +679,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, Expression responseWritingMethodCall, FactoryContext factoryContext) { - Debug.Assert(factoryContext.FormRequestBodyParameter is not null, "factoryContext.FormRequestBodyParameter is null for a form body."); + Debug.Assert(factoryContext.FirstFormRequestBodyParameter is not null, "factoryContext.FirstFormRequestBodyParameter is null for a form body."); if (factoryContext.ParameterBinders.Count > 0) { @@ -734,10 +734,12 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, HttpContext httpContext, FactoryContext factoryContext) { - Debug.Assert(factoryContext.FormRequestBodyParameter is not null, "TryReadFormAsync() should not be called if there are no form parameters."); + Debug.Assert(factoryContext.FirstFormRequestBodyParameter is not null, "TryReadFormAsync() should not be called if there are no form parameters."); - var parameterTypeName = TypeNameHelper.GetTypeDisplayName(factoryContext.FormRequestBodyParameter.ParameterType, fullName: false); - var parameterName = factoryContext.FormRequestBodyParameter.Name; + // If there are multiple parameters associated with the form, just use the name of + // the first one to report the failure to bind the parameter if reading the form fails. + var parameterTypeName = TypeNameHelper.GetTypeDisplayName(factoryContext.FirstFormRequestBodyParameter.ParameterType, fullName: false); + var parameterName = factoryContext.FirstFormRequestBodyParameter.Name; Debug.Assert(parameterName is not null, "CreateArgument() should throw if parameter.Name is null."); @@ -1040,7 +1042,11 @@ private static Expression BindParameterFromFormFiles( ParameterInfo parameter, FactoryContext factoryContext) { - factoryContext.FormRequestBodyParameter = parameter; + if (factoryContext.FirstFormRequestBodyParameter is null) + { + factoryContext.FirstFormRequestBodyParameter = parameter; + } + factoryContext.TrackedParameters.Add(parameter.Name!, RequestDelegateFactoryConstants.FormFileParameter); // Do not duplicate the metadata if there are multiple form parameters @@ -1060,7 +1066,11 @@ private static Expression BindParameterFromFormFile( FactoryContext factoryContext, string trackedParameterSource) { - factoryContext.FormRequestBodyParameter = parameter; + if (factoryContext.FirstFormRequestBodyParameter is null) + { + factoryContext.FirstFormRequestBodyParameter = parameter; + } + factoryContext.TrackedParameters.Add(key, trackedParameterSource); // Do not duplicate the metadata if there are multiple form parameters @@ -1383,7 +1393,7 @@ private class FactoryContext public NullabilityInfoContext NullabilityContext { get; } = new(); public bool ReadForm { get; set; } - public ParameterInfo? FormRequestBodyParameter { get; set; } + public ParameterInfo? FirstFormRequestBodyParameter { get; set; } } private static class RequestDelegateFactoryConstants From d4383309da0fa1f8f24d46c323a022530ba13492 Mon Sep 17 00:00:00 2001 From: martincostello Date: Sat, 6 Nov 2021 15:55:09 +0000 Subject: [PATCH 09/23] Rename log message Make the error case for an invalid form have a unique name compared to a JSON body. --- src/Http/Http.Extensions/src/RequestDelegateFactory.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index f50d6c70b625..fd966b6b8278 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -750,7 +750,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, { if (!httpContext.Request.HasFormContentType) { - Log.UnexpectedFormContentType(httpContext, httpContext.Request.ContentType, factoryContext.ThrowOnBadRequest); + Log.UnexpectedNonFormContentType(httpContext, httpContext.Request.ContentType, factoryContext.ThrowOnBadRequest); httpContext.Response.StatusCode = StatusCodes.Status415UnsupportedMediaType; return (null, false); } @@ -1513,7 +1513,7 @@ public static void UnexpectedJsonContentType(HttpContext httpContext, string? co [LoggerMessage(6, LogLevel.Debug, UnexpectedJsonContentTypeLogMessage, EventName = "UnexpectedContentType")] private static partial void UnexpectedJsonContentType(ILogger logger, string contentType); - public static void UnexpectedFormContentType(HttpContext httpContext, string? contentType, bool shouldThrow) + public static void UnexpectedNonFormContentType(HttpContext httpContext, string? contentType, bool shouldThrow) { if (shouldThrow) { @@ -1521,11 +1521,11 @@ public static void UnexpectedFormContentType(HttpContext httpContext, string? co throw new BadHttpRequestException(message, StatusCodes.Status415UnsupportedMediaType); } - UnexpectedFormContentType(GetLogger(httpContext), contentType ?? "(none)"); + UnexpectedNonFormContentType(GetLogger(httpContext), contentType ?? "(none)"); } - [LoggerMessage(7, LogLevel.Debug, UnexpectedFormContentTypeLogMessage, EventName = "UnexpectedContentType")] - private static partial void UnexpectedFormContentType(ILogger logger, string contentType); + [LoggerMessage(7, LogLevel.Debug, UnexpectedFormContentTypeLogMessage, EventName = "UnexpectedNonFormContentType")] + private static partial void UnexpectedNonFormContentType(ILogger logger, string contentType); public static void InvalidFormRequestBody(HttpContext httpContext, string parameterTypeName, string parameterName, Exception exception, bool shouldThrow) { From e03aaf5e13a2afe63e30a0b0d344de617b72c598 Mon Sep 17 00:00:00 2001 From: martincostello Date: Sat, 6 Nov 2021 15:57:30 +0000 Subject: [PATCH 10/23] Reuse formatting code for parameters Add a helper method to format the tracked parameters. --- .../src/RequestDelegateFactory.cs | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index fd966b6b8278..32cce52b43bb 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -1588,10 +1588,8 @@ private static string BuildErrorMessageForMultipleBodyParameters(FactoryContext errorMessage.AppendLine(FormattableString.Invariant($"{"Parameter",-20}| {"Source",-30}")); errorMessage.AppendLine("---------------------------------------------------------------------------------"); - foreach (var kv in factoryContext.TrackedParameters) - { - errorMessage.AppendLine(FormattableString.Invariant($"{kv.Key,-19} | {kv.Value,-15}")); - } + FormatTrackedParameters(factoryContext, errorMessage); + errorMessage.AppendLine().AppendLine(); errorMessage.AppendLine("Did you mean to register the \"UNKNOWN\" parameters as a Service?") .AppendLine(); @@ -1607,10 +1605,8 @@ private static string BuildErrorMessageForInferredBodyParameter(FactoryContext f errorMessage.AppendLine(FormattableString.Invariant($"{"Parameter",-20}| {"Source",-30}")); errorMessage.AppendLine("---------------------------------------------------------------------------------"); - foreach (var kv in factoryContext.TrackedParameters) - { - errorMessage.AppendLine(FormattableString.Invariant($"{kv.Key,-19} | {kv.Value,-15}")); - } + FormatTrackedParameters(factoryContext, errorMessage); + errorMessage.AppendLine().AppendLine(); errorMessage.AppendLine("Did you mean to register the \"Body (Inferred)\" parameter(s) as a Service or apply the [FromService] or [FromBody] attribute?") .AppendLine(); @@ -1626,10 +1622,16 @@ private static string BuildErrorMessageForFormAndJsonBodyParameters(FactoryConte errorMessage.AppendLine(FormattableString.Invariant($"{"Parameter",-20}| {"Source",-30}")); errorMessage.AppendLine("---------------------------------------------------------------------------------"); + FormatTrackedParameters(factoryContext, errorMessage); + + return errorMessage.ToString(); + } + + private static void FormatTrackedParameters(FactoryContext factoryContext, StringBuilder errorMessage) + { foreach (var kv in factoryContext.TrackedParameters) { errorMessage.AppendLine(FormattableString.Invariant($"{kv.Key,-19} | {kv.Value,-15}")); } - return errorMessage.ToString(); } } From 45b30d2efe3f993a0b25326d834e4406a300cf44 Mon Sep 17 00:00:00 2001 From: martincostello Date: Sat, 6 Nov 2021 16:34:16 +0000 Subject: [PATCH 11/23] Disallow form use with authentication Prevent the form being read if the request is considered to contain a secure value while there are no anti-forgery protections. --- .../src/RequestDelegateFactory.cs | 31 +++++ .../test/RequestDelegateFactoryTests.cs | 114 ++++++++++++++++++ 2 files changed, 145 insertions(+) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 32cce52b43bb..6de53ff338f6 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -14,6 +14,7 @@ using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Internal; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Primitives; namespace Microsoft.AspNetCore.Http; @@ -754,6 +755,9 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, httpContext.Response.StatusCode = StatusCodes.Status415UnsupportedMediaType; return (null, false); } + + ThrowIfRequestIsAuthenticated(httpContext); + try { formValue = await httpContext.Request.ReadFormAsync(); @@ -773,6 +777,33 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, return (formValue, true); } + + static void ThrowIfRequestIsAuthenticated(HttpContext httpContext) + { + if (httpContext.Connection.ClientCertificate is not null) + { + throw new BadHttpRequestException( + "Support for binding parameters from an HTTP request's form is not currently supported " + + "if the request is associated with a client certificate. Use of an HTTP request form is " + + "not currently secure for HTTP requests in scenarios which require authentication."); + } + + if (!StringValues.IsNullOrEmpty(httpContext.Request.Headers.Authorization)) + { + throw new BadHttpRequestException( + "Support for binding parameters from an HTTP request's form is not currently supported " + + "if the request contains an \"Authorization\" HTTP request header. Use of an HTTP request form is " + + "not currently secure for HTTP requests in scenarios which require authentication."); + } + + if (!StringValues.IsNullOrEmpty(httpContext.Request.Headers.Cookie)) + { + throw new BadHttpRequestException( + "Support for binding parameters from an HTTP request's form is not currently supported " + + "if the request contains a \"Cookie\" HTTP request header. Use of an HTTP request form is " + + "not currently secure for HTTP requests in scenarios which require authentication."); + } + } } private static Expression GetValueFromProperty(Expression sourceExpression, string key, Type? returnType = null) diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index 8700414044c9..c2da30584380 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -13,6 +13,7 @@ using System.Reflection; using System.Reflection.Metadata; using System.Security.Claims; +using System.Security.Cryptography.X509Certificates; using System.Text; using System.Text.Json; using System.Text.Json.Serialization; @@ -3623,6 +3624,104 @@ void TestAction(IFormFileCollection formFiles, IFormFile file) Assert.Equal(new[] { "multipart/form-data" }, acceptsMetadata.ContentTypes); } + [Theory] + [InlineData("Authorization", "bearer my-token", "Support for binding parameters from an HTTP request's form is not currently supported if the request contains an \"Authorization\" HTTP request header. Use of an HTTP request form is not currently secure for HTTP requests in scenarios which require authentication.")] + [InlineData("Cookie", ".AspNetCore.Auth=abc123", "Support for binding parameters from an HTTP request's form is not currently supported if the request contains a \"Cookie\" HTTP request header. Use of an HTTP request form is not currently secure for HTTP requests in scenarios which require authentication.")] + public async Task RequestDelegateThrowsIfRequestUsingFormContainsSecureHeader( + string headerName, + string headerValue, + string expectedMessage) + { + var invoked = false; + + void TestAction(IFormFile file) + { + invoked = true; + } + + var fileContent = new StringContent("hello", Encoding.UTF8, "application/octet-stream"); + var form = new MultipartFormDataContent("some-boundary"); + form.Add(fileContent, "file", "file.txt"); + + var stream = new MemoryStream(); + await form.CopyToAsync(stream); + + stream.Seek(0, SeekOrigin.Begin); + + var httpContext = CreateHttpContext(); + httpContext.Request.Body = stream; + httpContext.Request.Headers[headerName] = headerValue; + httpContext.Request.Headers["Content-Type"] = "multipart/form-data;boundary=some-boundary"; + httpContext.Features.Set(new RequestBodyDetectionFeature(true)); + + var factoryResult = RequestDelegateFactory.Create(TestAction); + var requestDelegate = factoryResult.RequestDelegate; + + var badHttpRequestException = await Assert.ThrowsAsync(() => requestDelegate(httpContext)); + + Assert.False(invoked); + + // The httpContext should be untouched. + Assert.False(httpContext.RequestAborted.IsCancellationRequested); + Assert.Equal(200, httpContext.Response.StatusCode); + Assert.False(httpContext.Response.HasStarted); + + // We don't log bad requests when we throw. + Assert.Empty(TestSink.Writes); + + Assert.Equal(expectedMessage, badHttpRequestException.Message); + Assert.Equal(400, badHttpRequestException.StatusCode); + } + + [Fact] + public async Task RequestDelegateThrowsIfRequestUsingFormHasClientCertificate() + { + var invoked = false; + + void TestAction(IFormFile file) + { + invoked = true; + } + + var fileContent = new StringContent("hello", Encoding.UTF8, "application/octet-stream"); + var form = new MultipartFormDataContent("some-boundary"); + form.Add(fileContent, "file", "file.txt"); + + var stream = new MemoryStream(); + await form.CopyToAsync(stream); + + stream.Seek(0, SeekOrigin.Begin); + + var httpContext = CreateHttpContext(); + httpContext.Request.Body = stream; + httpContext.Request.Headers["Content-Type"] = "multipart/form-data;boundary=some-boundary"; + httpContext.Features.Set(new RequestBodyDetectionFeature(true)); + +#pragma warning disable SYSLIB0026 // Type or member is obsolete + var clientCertificate = new X509Certificate2(); +#pragma warning restore SYSLIB0026 // Type or member is obsolete + + httpContext.Features.Set(new TlsConnectionFeature(clientCertificate)); + + var factoryResult = RequestDelegateFactory.Create(TestAction); + var requestDelegate = factoryResult.RequestDelegate; + + var badHttpRequestException = await Assert.ThrowsAsync(() => requestDelegate(httpContext)); + + Assert.False(invoked); + + // The httpContext should be untouched. + Assert.False(httpContext.RequestAborted.IsCancellationRequested); + Assert.Equal(200, httpContext.Response.StatusCode); + Assert.False(httpContext.Response.HasStarted); + + // We don't log bad requests when we throw. + Assert.Empty(TestSink.Writes); + + Assert.Equal("Support for binding parameters from an HTTP request's form is not currently supported if the request is associated with a client certificate. Use of an HTTP request form is not currently secure for HTTP requests in scenarios which require authentication.", badHttpRequestException.Message); + Assert.Equal(400, badHttpRequestException.StatusCode); + } + private DefaultHttpContext CreateHttpContext() { var responseFeature = new TestHttpResponseFeature(); @@ -3962,6 +4061,21 @@ public RequestBodyDetectionFeature(bool canHaveBody) public bool CanHaveBody { get; } } + + private class TlsConnectionFeature : ITlsConnectionFeature + { + public TlsConnectionFeature(X509Certificate2 clientCertificate) + { + ClientCertificate = clientCertificate; + } + + public X509Certificate2? ClientCertificate { get; set; } + + public Task GetClientCertificateAsync(CancellationToken cancellationToken) + { + throw new NotImplementedException(); + } + } } internal static class TestExtensionResults From 9a0e5e22d51098a4288090d77c2a26b3def28727 Mon Sep 17 00:00:00 2001 From: martincostello Date: Tue, 9 Nov 2021 17:49:28 +0000 Subject: [PATCH 12/23] Rename metadata to IFromFormMetadata Rename IFromFileMetadata to IFromFormMetadata. --- .../Metadata/{IFromFileMetadata.cs => IFromFormMetadata.cs} | 6 +++--- src/Http/Http.Abstractions/src/PublicAPI.Unshipped.txt | 4 ++-- src/Http/Http.Extensions/src/RequestDelegateFactory.cs | 2 +- .../Http.Extensions/test/RequestDelegateFactoryTests.cs | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) rename src/Http/Http.Abstractions/src/Metadata/{IFromFileMetadata.cs => IFromFormMetadata.cs} (74%) diff --git a/src/Http/Http.Abstractions/src/Metadata/IFromFileMetadata.cs b/src/Http/Http.Abstractions/src/Metadata/IFromFormMetadata.cs similarity index 74% rename from src/Http/Http.Abstractions/src/Metadata/IFromFileMetadata.cs rename to src/Http/Http.Abstractions/src/Metadata/IFromFormMetadata.cs index b159e375e569..96b872eb3f13 100644 --- a/src/Http/Http.Abstractions/src/Metadata/IFromFileMetadata.cs +++ b/src/Http/Http.Abstractions/src/Metadata/IFromFormMetadata.cs @@ -4,12 +4,12 @@ namespace Microsoft.AspNetCore.Http.Metadata; /// -/// Interface marking attributes that specify a parameter should be bound using a form file from the request body. +/// Interface marking attributes that specify a parameter should be bound using a form field from the request body. /// -public interface IFromFileMetadata +public interface IFromFormMetadata { /// - /// The file name. + /// The form field name. /// string? Name { get; } } diff --git a/src/Http/Http.Abstractions/src/PublicAPI.Unshipped.txt b/src/Http/Http.Abstractions/src/PublicAPI.Unshipped.txt index a0741f182baf..e6eee09af147 100644 --- a/src/Http/Http.Abstractions/src/PublicAPI.Unshipped.txt +++ b/src/Http/Http.Abstractions/src/PublicAPI.Unshipped.txt @@ -14,8 +14,8 @@ Microsoft.AspNetCore.Http.Metadata.IAcceptsMetadata.IsOptional.get -> bool Microsoft.AspNetCore.Http.Metadata.IAcceptsMetadata.RequestType.get -> System.Type? Microsoft.AspNetCore.Http.Metadata.IFromBodyMetadata Microsoft.AspNetCore.Http.Metadata.IFromBodyMetadata.AllowEmpty.get -> bool -Microsoft.AspNetCore.Http.Metadata.IFromFileMetadata -Microsoft.AspNetCore.Http.Metadata.IFromFileMetadata.Name.get -> string? +Microsoft.AspNetCore.Http.Metadata.IFromFormMetadata +Microsoft.AspNetCore.Http.Metadata.IFromFormMetadata.Name.get -> string? Microsoft.AspNetCore.Http.Metadata.IFromHeaderMetadata Microsoft.AspNetCore.Http.Metadata.IFromHeaderMetadata.Name.get -> string? Microsoft.AspNetCore.Http.Metadata.IFromQueryMetadata diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 6de53ff338f6..6fa6e927b8d4 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -249,7 +249,7 @@ private static Expression CreateArgument(ParameterInfo parameter, FactoryContext factoryContext.TrackedParameters.Add(parameter.Name, RequestDelegateFactoryConstants.BodyAttribute); return BindParameterFromBody(parameter, bodyAttribute.AllowEmpty, factoryContext); } - else if (parameterCustomAttributes.OfType().FirstOrDefault() is { } fileAttribute) + else if (parameterCustomAttributes.OfType().FirstOrDefault() is { } fileAttribute) { return BindParameterFromFormFile(parameter, fileAttribute.Name ?? parameter.Name, factoryContext, RequestDelegateFactoryConstants.FormFileAttribute); } diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index c2da30584380..f9c5cf9fddff 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -3843,7 +3843,7 @@ private class FromBodyAttribute : Attribute, IFromBodyMetadata public bool AllowEmpty { get; set; } } - private class FromFormFileAttribute : Attribute, IFromFileMetadata + private class FromFormFileAttribute : Attribute, IFromFormMetadata { public string? Name { get; set; } } From 8c1c4b89d3b16e015ea484b6f5f3e29e170d0305 Mon Sep 17 00:00:00 2001 From: martincostello Date: Tue, 9 Nov 2021 17:51:49 +0000 Subject: [PATCH 13/23] Rename test class Rename test implementation to FromFormAttribute. --- .../Http.Extensions/test/RequestDelegateFactoryTests.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index f9c5cf9fddff..c33f768900d0 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -3212,7 +3212,7 @@ void TestFormFileAndJsonWithAttribute(IFormFile value1, [FromBody] int value2) { void TestJsonAndFormFile(Todo value1, IFormFile value2) { } void TestJsonAndFormFiles(Todo value1, IFormFile value2, IFormFile value3) { } void TestJsonAndFormFileCollection(Todo value1, IFormFileCollection value2) { } - void TestJsonAndFormFileWithAttribute(Todo value1, [FromFormFile] IFormFile value2) { } + void TestJsonAndFormFileWithAttribute(Todo value1, [FromForm] IFormFile value2) { } Assert.Throws(() => RequestDelegateFactory.Create(TestFormFileAndJson)); Assert.Throws(() => RequestDelegateFactory.Create(TestFormFilesAndJson)); @@ -3422,7 +3422,7 @@ public async Task RequestDelegatePopulatesFromIFormFileParameterWithMetadata() { IFormFile? fileArgument = null; - void TestAction([FromFormFile(Name = "my_file")] IFormFile file) + void TestAction([FromForm(Name = "my_file")] IFormFile file) { fileArgument = file; } @@ -3843,7 +3843,7 @@ private class FromBodyAttribute : Attribute, IFromBodyMetadata public bool AllowEmpty { get; set; } } - private class FromFormFileAttribute : Attribute, IFromFormMetadata + private class FromFormAttribute : Attribute, IFromFormMetadata { public string? Name { get; set; } } From 7eb02428425dc09f52fdb5dd6a7062a30f627bc6 Mon Sep 17 00:00:00 2001 From: martincostello Date: Tue, 9 Nov 2021 18:09:34 +0000 Subject: [PATCH 14/23] Support IFromFormMetadata for IFormFileCollection Support the use of IFromFormMetadata for parameters of type IFormFileCollection. Throw if a name is used for the metadata. --- .../src/RequestDelegateFactory.cs | 15 +++++- .../test/RequestDelegateFactoryTests.cs | 53 +++++++++++++++++++ 2 files changed, 66 insertions(+), 2 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 6fa6e927b8d4..00d54c4e1b04 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -249,9 +249,20 @@ private static Expression CreateArgument(ParameterInfo parameter, FactoryContext factoryContext.TrackedParameters.Add(parameter.Name, RequestDelegateFactoryConstants.BodyAttribute); return BindParameterFromBody(parameter, bodyAttribute.AllowEmpty, factoryContext); } - else if (parameterCustomAttributes.OfType().FirstOrDefault() is { } fileAttribute) + else if (parameterCustomAttributes.OfType().FirstOrDefault() is { } formAttribute) { - return BindParameterFromFormFile(parameter, fileAttribute.Name ?? parameter.Name, factoryContext, RequestDelegateFactoryConstants.FormFileAttribute); + if (parameter.ParameterType == typeof(IFormFileCollection)) + { + if (!string.IsNullOrEmpty(formAttribute.Name)) + { + throw new NotSupportedException( + $"Assigning a value to the {nameof(IFromFormMetadata)}.{nameof(IFromFormMetadata.Name)} property is not supported for parameters of type {nameof(IFormFileCollection)}."); + } + + return BindParameterFromFormFiles(parameter, factoryContext); + } + + return BindParameterFromFormFile(parameter, formAttribute.Name ?? parameter.Name, factoryContext, RequestDelegateFactoryConstants.FormFileAttribute); } else if (parameter.CustomAttributes.Any(a => typeof(IFromServiceMetadata).IsAssignableFrom(a.AttributeType))) { diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index c33f768900d0..a7c76566f5d6 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -3263,6 +3263,59 @@ void TestAction(IFormFileCollection formFiles) Assert.Equal(new[] { "multipart/form-data" }, acceptsMetadata.ContentTypes); } + [Fact] + public async Task RequestDelegatePopulatesFromIFormFileCollectionParameterWithAttribute() + { + IFormFileCollection? formFilesArgument = null; + + void TestAction([FromForm] IFormFileCollection formFiles) + { + formFilesArgument = formFiles; + } + + var fileContent = new StringContent("hello", Encoding.UTF8, "application/octet-stream"); + var form = new MultipartFormDataContent("some-boundary"); + form.Add(fileContent, "file", "file.txt"); + + var stream = new MemoryStream(); + await form.CopyToAsync(stream); + + stream.Seek(0, SeekOrigin.Begin); + + var httpContext = CreateHttpContext(); + httpContext.Request.Body = stream; + httpContext.Request.Headers["Content-Type"] = "multipart/form-data;boundary=some-boundary"; + httpContext.Features.Set(new RequestBodyDetectionFeature(true)); + + var factoryResult = RequestDelegateFactory.Create(TestAction); + var requestDelegate = factoryResult.RequestDelegate; + + await requestDelegate(httpContext); + + Assert.Equal(httpContext.Request.Form.Files, formFilesArgument); + Assert.NotNull(formFilesArgument!["file"]); + + var allAcceptsMetadata = factoryResult.EndpointMetadata.OfType(); + var acceptsMetadata = Assert.Single(allAcceptsMetadata); + + Assert.NotNull(acceptsMetadata); + Assert.Equal(new[] { "multipart/form-data" }, acceptsMetadata.ContentTypes); + } + + [Fact] + public void CreateThrowsNotSupportedExceptionIfIFormFileCollectionHasMetadataParameterName() + { + IFormFileCollection? formFilesArgument = null; + + void TestAction([FromForm(Name = "foo")] IFormFileCollection formFiles) + { + formFilesArgument = formFiles; + } + + var nse = Assert.Throws(() => RequestDelegateFactory.Create(TestAction)); + Assert.Equal("Assigning a value to the IFromFormMetadata.Name property is not supported for parameters of type IFormFileCollection.", nse.Message); + } + [Fact] public async Task RequestDelegatePopulatesFromIFormFileParameter() { From de1ace25bd439794874aca8e71031122e2adf04a Mon Sep 17 00:00:00 2001 From: martincostello Date: Tue, 9 Nov 2021 18:15:34 +0000 Subject: [PATCH 15/23] Add IFromFormMetadata Add IFromFormMetadata as an implemented interface. --- src/Mvc/Mvc.Core/src/FromFormAttribute.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Mvc/Mvc.Core/src/FromFormAttribute.cs b/src/Mvc/Mvc.Core/src/FromFormAttribute.cs index aec07a14fc1a..3e1b705e35af 100644 --- a/src/Mvc/Mvc.Core/src/FromFormAttribute.cs +++ b/src/Mvc/Mvc.Core/src/FromFormAttribute.cs @@ -1,7 +1,6 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System; using Microsoft.AspNetCore.Http.Metadata; using Microsoft.AspNetCore.Mvc.ModelBinding; @@ -11,7 +10,7 @@ namespace Microsoft.AspNetCore.Mvc; /// Specifies that a parameter or property should be bound using form-data in the request body. /// [AttributeUsage(AttributeTargets.Parameter | AttributeTargets.Property, AllowMultiple = false, Inherited = true)] -public class FromFormAttribute : Attribute, IBindingSourceMetadata, IModelNameProvider +public class FromFormAttribute : Attribute, IBindingSourceMetadata, IModelNameProvider, IFromFormMetadata { /// public BindingSource BindingSource => BindingSource.Form; From 9a7cf878b4c379e6b95ca3612a4474fa6ba6d704 Mon Sep 17 00:00:00 2001 From: martincostello Date: Tue, 9 Nov 2021 18:16:46 +0000 Subject: [PATCH 16/23] Fix test compilation Fix tests that wouldn't compile after the rename in 9a0e5e22d51098a4288090d77c2a26b3def28727 that was done from another solution file. Add TODO for how to correctly handle IFormFileCollection with IFromFormMetadata. --- .../src/EndpointMetadataApiDescriptionProvider.cs | 6 ++++-- .../test/EndpointMetadataApiDescriptionProviderTest.cs | 9 +-------- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs b/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs index be81fa5db417..fd52b30f5cec 100644 --- a/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs +++ b/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs @@ -249,9 +249,11 @@ private static ParameterDescriptor CreateParameterDescriptor(ParameterInfo param { return (BindingSource.Body, parameter.Name ?? string.Empty, fromBodyAttribute.AllowEmpty, parameter.ParameterType); } - else if (attributes.OfType().FirstOrDefault() is { } formFileAttribute) + else if (attributes.OfType().FirstOrDefault() is { } fromFormAttribute) { - return (BindingSource.FormFile, formFileAttribute.Name ?? parameter.Name ?? string.Empty, false, parameter.ParameterType); + // TODO Should this be Form or FormFile if parameter.ParameterType is IFormFileCollection? + // Currently it's Services (see below) - which is the more appropriate? + return (BindingSource.FormFile, fromFormAttribute.Name ?? parameter.Name ?? string.Empty, false, parameter.ParameterType); } else if (parameter.CustomAttributes.Any(a => typeof(IFromServiceMetadata).IsAssignableFrom(a.AttributeType)) || parameter.ParameterType == typeof(HttpContext) || diff --git a/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs b/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs index c89ece0d783c..0f128457c0f6 100644 --- a/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs +++ b/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs @@ -6,7 +6,6 @@ using System.Security.Claims; using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Http; -using Microsoft.AspNetCore.Http.Metadata; using Microsoft.AspNetCore.Mvc.Abstractions; using Microsoft.AspNetCore.Mvc.Infrastructure; using Microsoft.AspNetCore.Mvc.ModelBinding; @@ -997,7 +996,7 @@ static void AssertFormFileParameter(ApiDescription apiDescription, Type expected } AssertFormFileParameter(GetApiDescription((IFormFile file) => { }), typeof(IFormFile), "file"); - AssertFormFileParameter(GetApiDescription(([FromFile(Name = "file_name")] IFormFile file) => { }), typeof(IFormFile), "file_name"); + AssertFormFileParameter(GetApiDescription(([FromForm(Name = "file_name")] IFormFile file) => { }), typeof(IFormFile), "file_name"); } [Fact] @@ -1100,12 +1099,6 @@ public void HandlesEndpointWithRouteConstraints() constraint => Assert.IsType(constraint)); } - // TODO Can remove this when there's an agreed concrete implementation of IFromFileMetadata - private sealed class FromFileAttribute : Attribute, IFromFileMetadata - { - public string Name { get; set; } - } - private static IEnumerable GetSortedMediaTypes(ApiResponseType apiResponseType) { return apiResponseType.ApiResponseFormats From ec7b8df0feff8b07e47580e3f8ea46666c36fc5a Mon Sep 17 00:00:00 2001 From: martincostello Date: Fri, 12 Nov 2021 11:26:14 +0000 Subject: [PATCH 17/23] Capture values on start-up Revert changes from 599e72c17741089a40c18e0da36659e59fa11d58. --- .../src/RequestDelegateFactory.cs | 78 ++++++++++++------- 1 file changed, 50 insertions(+), 28 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 00d54c4e1b04..6ba121df9f4c 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -587,6 +587,12 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, { Debug.Assert(factoryContext.JsonRequestBodyParameter is not null, "factoryContext.JsonRequestBodyParameter is null for a JSON body."); + var bodyType = factoryContext.JsonRequestBodyParameter.ParameterType; + var parameterTypeName = TypeNameHelper.GetTypeDisplayName(factoryContext.JsonRequestBodyParameter.ParameterType, fullName: false); + var parameterName = factoryContext.JsonRequestBodyParameter.Name; + + Debug.Assert(parameterName is not null, "CreateArgument() should throw if parameter.Name is null."); + if (factoryContext.ParameterBinders.Count > 0) { // We need to generate the code for reading from the body before calling into the delegate @@ -607,7 +613,13 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, boundValues[i] = await binders[i](httpContext); } - (var bodyValue, var successful) = await TryReadBodyAsync(httpContext, factoryContext); + var (bodyValue, successful) = await TryReadBodyAsync( + httpContext, + bodyType, + parameterTypeName, + parameterName, + factoryContext.AllowEmptyRequestBody, + factoryContext.ThrowOnBadRequest); if (!successful) { @@ -625,7 +637,13 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, return async (target, httpContext) => { - (var bodyValue, var successful) = await TryReadBodyAsync(httpContext, factoryContext); + var (bodyValue, successful) = await TryReadBodyAsync( + httpContext, + bodyType, + parameterTypeName, + parameterName, + factoryContext.AllowEmptyRequestBody, + factoryContext.ThrowOnBadRequest); if (!successful) { @@ -638,19 +656,15 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, static async Task<(object? FormValue, bool Successful)> TryReadBodyAsync( HttpContext httpContext, - FactoryContext factoryContext) + Type bodyType, + string parameterTypeName, + string parameterName, + bool allowEmptyRequestBody, + bool throwOnBadRequest) { - Debug.Assert(factoryContext.JsonRequestBodyParameter is not null, "TryReadFormAsync() should not be called if there is no JSON body parameter."); - - var bodyType = factoryContext.JsonRequestBodyParameter.ParameterType; - var parameterTypeName = TypeNameHelper.GetTypeDisplayName(factoryContext.JsonRequestBodyParameter.ParameterType, fullName: false); - var parameterName = factoryContext.JsonRequestBodyParameter.Name; - - Debug.Assert(parameterName is not null, "CreateArgument() should throw if parameter.Name is null."); - object? defaultBodyValue = null; - if (factoryContext.AllowEmptyRequestBody && bodyType.IsValueType) + if (allowEmptyRequestBody && bodyType.IsValueType) { defaultBodyValue = Activator.CreateInstance(bodyType); } @@ -662,7 +676,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, { if (!httpContext.Request.HasJsonContentType()) { - Log.UnexpectedJsonContentType(httpContext, httpContext.Request.ContentType, factoryContext.ThrowOnBadRequest); + Log.UnexpectedJsonContentType(httpContext, httpContext.Request.ContentType, throwOnBadRequest); httpContext.Response.StatusCode = StatusCodes.Status415UnsupportedMediaType; return (null, false); } @@ -677,7 +691,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, } catch (JsonException ex) { - Log.InvalidJsonRequestBody(httpContext, parameterTypeName, parameterName, ex, factoryContext.ThrowOnBadRequest); + Log.InvalidJsonRequestBody(httpContext, parameterTypeName, parameterName, ex, throwOnBadRequest); httpContext.Response.StatusCode = StatusCodes.Status400BadRequest; return (null, false); } @@ -693,6 +707,13 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, { Debug.Assert(factoryContext.FirstFormRequestBodyParameter is not null, "factoryContext.FirstFormRequestBodyParameter is null for a form body."); + // If there are multiple parameters associated with the form, just use the name of + // the first one to report the failure to bind the parameter if reading the form fails. + var parameterTypeName = TypeNameHelper.GetTypeDisplayName(factoryContext.FirstFormRequestBodyParameter.ParameterType, fullName: false); + var parameterName = factoryContext.FirstFormRequestBodyParameter.Name; + + Debug.Assert(parameterName is not null, "CreateArgument() should throw if parameter.Name is null."); + if (factoryContext.ParameterBinders.Count > 0) { // We need to generate the code for reading from the body or form before calling into the delegate @@ -713,7 +734,11 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, boundValues[i] = await binders[i](httpContext); } - (var formValue, var successful) = await TryReadFormAsync(httpContext, factoryContext); + var (formValue, successful) = await TryReadFormAsync( + httpContext, + parameterTypeName, + parameterName, + factoryContext.ThrowOnBadRequest); if (!successful) { @@ -731,7 +756,11 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, return async (target, httpContext) => { - (var formValue, var successful) = await TryReadFormAsync(httpContext, factoryContext); + var (formValue, successful) = await TryReadFormAsync( + httpContext, + parameterTypeName, + parameterName, + factoryContext.ThrowOnBadRequest); if (!successful) { @@ -744,17 +773,10 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, static async Task<(object? FormValue, bool Successful)> TryReadFormAsync( HttpContext httpContext, - FactoryContext factoryContext) + string parameterTypeName, + string parameterName, + bool throwOnBadRequest) { - Debug.Assert(factoryContext.FirstFormRequestBodyParameter is not null, "TryReadFormAsync() should not be called if there are no form parameters."); - - // If there are multiple parameters associated with the form, just use the name of - // the first one to report the failure to bind the parameter if reading the form fails. - var parameterTypeName = TypeNameHelper.GetTypeDisplayName(factoryContext.FirstFormRequestBodyParameter.ParameterType, fullName: false); - var parameterName = factoryContext.FirstFormRequestBodyParameter.Name; - - Debug.Assert(parameterName is not null, "CreateArgument() should throw if parameter.Name is null."); - object? formValue = null; var feature = httpContext.Features.Get(); @@ -762,7 +784,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, { if (!httpContext.Request.HasFormContentType) { - Log.UnexpectedNonFormContentType(httpContext, httpContext.Request.ContentType, factoryContext.ThrowOnBadRequest); + Log.UnexpectedNonFormContentType(httpContext, httpContext.Request.ContentType, throwOnBadRequest); httpContext.Response.StatusCode = StatusCodes.Status415UnsupportedMediaType; return (null, false); } @@ -780,7 +802,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, } catch (InvalidDataException ex) { - Log.InvalidFormRequestBody(httpContext, parameterTypeName, parameterName, ex, factoryContext.ThrowOnBadRequest); + Log.InvalidFormRequestBody(httpContext, parameterTypeName, parameterName, ex, throwOnBadRequest); httpContext.Response.StatusCode = StatusCodes.Status400BadRequest; return (null, false); } From 7a761993181609d965c07d1e41ae58d75f697f93 Mon Sep 17 00:00:00 2001 From: martincostello Date: Fri, 12 Nov 2021 11:27:30 +0000 Subject: [PATCH 18/23] Remove unused parameter Remove unused optional parameter. --- src/Http/Http.Extensions/src/RequestDelegateFactory.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 6ba121df9f4c..f8cafbef35a8 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -1048,8 +1048,8 @@ private static Expression BindParameterFromExpression( Expression.Convert(Expression.Constant(parameter.DefaultValue), parameter.ParameterType))); } - private static Expression BindParameterFromProperty(ParameterInfo parameter, MemberExpression property, string key, FactoryContext factoryContext, string source, Type? returnType = null) => - BindParameterFromValue(parameter, GetValueFromProperty(property, key, returnType), factoryContext, source); + private static Expression BindParameterFromProperty(ParameterInfo parameter, MemberExpression property, string key, FactoryContext factoryContext, string source) => + BindParameterFromValue(parameter, GetValueFromProperty(property, key), factoryContext, source); private static Expression BindParameterFromRouteValueOrQueryString(ParameterInfo parameter, string key, FactoryContext factoryContext) { From 2ad668d1af1a613bbb8392fe37a54a2170a1e6fe Mon Sep 17 00:00:00 2001 From: martincostello Date: Fri, 12 Nov 2021 11:39:41 +0000 Subject: [PATCH 19/23] Throw if [FromForm] used with invalid types Throw a NotSupportedException if [FromForm]/IFromFormMetadata is used for parameters of types other than IFormFile and IFormFileCollection. --- .../src/RequestDelegateFactory.cs | 5 ++++ .../test/RequestDelegateFactoryTests.cs | 28 +++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index f8cafbef35a8..0035808cbb3b 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -261,6 +261,11 @@ private static Expression CreateArgument(ParameterInfo parameter, FactoryContext return BindParameterFromFormFiles(parameter, factoryContext); } + else if (parameter.ParameterType != typeof(IFormFile)) + { + throw new NotSupportedException( + $"{nameof(IFromFormMetadata)} is only supported for parameters of type {nameof(IFormFileCollection)} and {nameof(IFormFile)}."); + } return BindParameterFromFormFile(parameter, formAttribute.Name ?? parameter.Name, factoryContext, RequestDelegateFactoryConstants.FormFileAttribute); } diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index a7c76566f5d6..b659ba4c152c 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -3316,6 +3316,34 @@ void TestAction([FromForm(Name = "foo")] IFormFileCollection formFiles) Assert.Equal("Assigning a value to the IFromFormMetadata.Name property is not supported for parameters of type IFormFileCollection.", nse.Message); } + [Fact] + public void CreateThrowsNotSupportedExceptionIfFromFormParameterIsNotIFormFileCollectionOrIFormFile() + { + void TestActionBool([FromForm] bool value) { }; + void TestActionInt([FromForm] int value) { }; + void TestActionObject([FromForm] object value) { }; + void TestActionString([FromForm] string value) { }; + void TestActionCancellationToken([FromForm] CancellationToken value) { }; + void TestActionClaimsPrincipal([FromForm] ClaimsPrincipal value) { }; + void TestActionHttpContext([FromForm] HttpContext value) { }; + void TestActionIFormCollection([FromForm] IFormCollection value) { }; + + AssertNotSupportedExceptionThrown(TestActionBool); + AssertNotSupportedExceptionThrown(TestActionInt); + AssertNotSupportedExceptionThrown(TestActionObject); + AssertNotSupportedExceptionThrown(TestActionString); + AssertNotSupportedExceptionThrown(TestActionCancellationToken); + AssertNotSupportedExceptionThrown(TestActionClaimsPrincipal); + AssertNotSupportedExceptionThrown(TestActionHttpContext); + AssertNotSupportedExceptionThrown(TestActionIFormCollection); + + static void AssertNotSupportedExceptionThrown(Delegate handler) + { + var nse = Assert.Throws(() => RequestDelegateFactory.Create(handler)); + Assert.Equal("IFromFormMetadata is only supported for parameters of type IFormFileCollection and IFormFile.", nse.Message); + } + } + [Fact] public async Task RequestDelegatePopulatesFromIFormFileParameter() { From d2de191ebe63d18617d3cc4383f1f3f2f7402b84 Mon Sep 17 00:00:00 2001 From: martincostello Date: Fri, 12 Nov 2021 11:55:30 +0000 Subject: [PATCH 20/23] Update IFormFileCollection BindingSource Update IFormFileCollection to bind as BindingSource.FormFile. --- .../EndpointMetadataApiDescriptionProvider.cs | 5 +- ...pointMetadataApiDescriptionProviderTest.cs | 55 ++++++++++--------- 2 files changed, 31 insertions(+), 29 deletions(-) diff --git a/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs b/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs index fd52b30f5cec..21539619b8cf 100644 --- a/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs +++ b/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs @@ -251,8 +251,6 @@ private static ParameterDescriptor CreateParameterDescriptor(ParameterInfo param } else if (attributes.OfType().FirstOrDefault() is { } fromFormAttribute) { - // TODO Should this be Form or FormFile if parameter.ParameterType is IFormFileCollection? - // Currently it's Services (see below) - which is the more appropriate? return (BindingSource.FormFile, fromFormAttribute.Name ?? parameter.Name ?? string.Empty, false, parameter.ParameterType); } else if (parameter.CustomAttributes.Any(a => typeof(IFromServiceMetadata).IsAssignableFrom(a.AttributeType)) || @@ -261,7 +259,6 @@ private static ParameterDescriptor CreateParameterDescriptor(ParameterInfo param parameter.ParameterType == typeof(HttpResponse) || parameter.ParameterType == typeof(ClaimsPrincipal) || parameter.ParameterType == typeof(CancellationToken) || - parameter.ParameterType == typeof(IFormFileCollection) || ParameterBindingMethodCache.HasBindAsyncMethod(parameter) || _serviceProviderIsService?.IsService(parameter.ParameterType) == true) { @@ -282,7 +279,7 @@ private static ParameterDescriptor CreateParameterDescriptor(ParameterInfo param return (BindingSource.Query, parameter.Name ?? string.Empty, false, displayType); } } - else if (parameter.ParameterType == typeof(IFormFile)) + else if (parameter.ParameterType == typeof(IFormFile) || parameter.ParameterType == typeof(IFormFileCollection)) { return (BindingSource.FormFile, parameter.Name ?? string.Empty, false, parameter.ParameterType); } diff --git a/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs b/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs index 0f128457c0f6..e9c5624b6cd9 100644 --- a/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs +++ b/src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs @@ -1002,32 +1002,37 @@ static void AssertFormFileParameter(ApiDescription apiDescription, Type expected [Fact] public void AddsMultipartFormDataResponseFormatWhenFormFileCollectionSpecified() { - // Arrange - var services = new ServiceCollection(); - var serviceProvider = services.BuildServiceProvider(); - var builder = new TestEndpointRouteBuilder(new ApplicationBuilder(serviceProvider)); - builder.MapPost("/file/upload", (IFormFileCollection files) => Results.NoContent()); - var context = new ApiDescriptionProviderContext(Array.Empty()); - - var endpointDataSource = builder.DataSources.OfType().Single(); - var provider = CreateEndpointMetadataApiDescriptionProvider(endpointDataSource); - - // Act - provider.OnProvidersExecuting(context); - provider.OnProvidersExecuted(context); + AssertFormFileCollection((IFormFileCollection files) => Results.NoContent(), "files"); + AssertFormFileCollection(([FromForm] IFormFileCollection uploads) => Results.NoContent(), "uploads"); - // Assert - var parameterDescriptions = context.Results.SelectMany(r => r.ParameterDescriptions); - var bodyParameterDescription = parameterDescriptions.Single(); - Assert.Equal(typeof(IFormFileCollection), bodyParameterDescription.Type); - Assert.Equal(typeof(IFormFileCollection).Name, bodyParameterDescription.Name); - Assert.True(bodyParameterDescription.IsRequired); - - // Assert - var requestFormats = context.Results.SelectMany(r => r.SupportedRequestFormats); - var defaultRequestFormat = requestFormats.Single(); - Assert.Equal("multipart/form-data", defaultRequestFormat.MediaType); - Assert.Null(defaultRequestFormat.Formatter); + static void AssertFormFileCollection(Delegate handler, string expectedName) + { + // Arrange + var services = new ServiceCollection(); + var serviceProvider = services.BuildServiceProvider(); + var builder = new TestEndpointRouteBuilder(new ApplicationBuilder(serviceProvider)); + builder.MapPost("/file/upload", handler); + var context = new ApiDescriptionProviderContext(Array.Empty()); + + var endpointDataSource = builder.DataSources.OfType().Single(); + var provider = CreateEndpointMetadataApiDescriptionProvider(endpointDataSource); + + // Act + provider.OnProvidersExecuting(context); + provider.OnProvidersExecuted(context); + + // Assert + var parameterDescriptions = context.Results.SelectMany(r => r.ParameterDescriptions); + var bodyParameterDescription = parameterDescriptions.Single(); + Assert.Equal(typeof(IFormFileCollection), bodyParameterDescription.Type); + Assert.Equal(expectedName, bodyParameterDescription.Name); + Assert.True(bodyParameterDescription.IsRequired); + + var requestFormats = context.Results.SelectMany(r => r.SupportedRequestFormats); + var defaultRequestFormat = requestFormats.Single(); + Assert.Equal("multipart/form-data", defaultRequestFormat.MediaType); + Assert.Null(defaultRequestFormat.Formatter); + } } #nullable restore From 538a82ea382e0449217c98c6a548a8c0b80a1f1a Mon Sep 17 00:00:00 2001 From: martincostello Date: Fri, 12 Nov 2021 12:09:55 +0000 Subject: [PATCH 21/23] Remove BindingSource.Form case Remove check for BindingSource.Form as that's not a used value with Minimal APIs. --- .../src/EndpointMetadataApiDescriptionProvider.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs b/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs index 21539619b8cf..0d3f7bc2e17a 100644 --- a/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs +++ b/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs @@ -118,7 +118,6 @@ private ApiDescription CreateApiDescription(RouteEndpoint routeEndpoint, string // exists for something derived from the body's content, such as a form file. var alreadyHasAnyBodyParameter = apiDescription.ParameterDescriptions.Any(x => x.Source == BindingSource.Body || - x.Source == BindingSource.Form || x.Source == BindingSource.FormFile); if (!alreadyHasAnyBodyParameter) From e7ba8819677ac48dbf422cc46461460454f70ee3 Mon Sep 17 00:00:00 2001 From: martincostello Date: Sun, 14 Nov 2021 14:31:29 +0000 Subject: [PATCH 22/23] Update default body parameter handling Update the default parameter handling for inferred body parameters so the default parameter is only added via IAcceptsMetadata if there were no other parameters already defined. --- .../EndpointMetadataApiDescriptionProvider.cs | 26 +++++----- ...pointMetadataApiDescriptionProviderTest.cs | 47 ++++++++++++++----- 2 files changed, 48 insertions(+), 25 deletions(-) diff --git a/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs b/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs index 0d3f7bc2e17a..a24ff4e51f12 100644 --- a/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs +++ b/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs @@ -92,12 +92,14 @@ private ApiDescription CreateApiDescription(RouteEndpoint routeEndpoint, string { DisplayName = routeEndpoint.DisplayName, RouteValues = - { - ["controller"] = controllerName, - }, + { + ["controller"] = controllerName, + }, }, }; + var hasBodyOrFormFileParameter = false; + foreach (var parameter in methodInfo.GetParameters()) { var parameterDescription = CreateApiParameterDescription(parameter, routeEndpoint.RoutePattern); @@ -108,19 +110,20 @@ private ApiDescription CreateApiDescription(RouteEndpoint routeEndpoint, string } apiDescription.ParameterDescriptions.Add(parameterDescription); + + hasBodyOrFormFileParameter |= + parameterDescription.Source == BindingSource.Body || + parameterDescription.Source == BindingSource.FormFile; } // Get IAcceptsMetadata. var acceptsMetadata = routeEndpoint.Metadata.GetMetadata(); if (acceptsMetadata is not null) { - // Do not add an implicit body parameter description if a parameter already - // exists for something derived from the body's content, such as a form file. - var alreadyHasAnyBodyParameter = apiDescription.ParameterDescriptions.Any(x => - x.Source == BindingSource.Body || - x.Source == BindingSource.FormFile); - - if (!alreadyHasAnyBodyParameter) + // Add a default body parameter if there was no explicitly defined parameter associated with + // either the body or a form and the user explicity defined some metadata describing the + // content types the endpoint consumes (such as Accepts { }), typeof(InferredJsonClass)); - AssertBodyParameter(GetApiDescription(([FromBody] int foo) => { }), typeof(int)); + AssertBodyParameter(GetApiDescription((InferredJsonClass foo) => { }), "foo", typeof(InferredJsonClass)); + AssertBodyParameter(GetApiDescription(([FromBody] int bar) => { }), "bar", typeof(int)); } [Fact] @@ -382,25 +390,38 @@ public void AddsDefaultValueFromParameters() Assert.Equal(42, param.DefaultValue); } +#nullable enable + [Fact] public void AddsMultipleParameters() { var apiDescription = GetApiDescription(([FromRoute] int foo, int bar, InferredJsonClass fromBody) => { }); - Assert.Equal(2, apiDescription.ParameterDescriptions.Count); + Assert.Equal(3, apiDescription.ParameterDescriptions.Count); var fooParam = apiDescription.ParameterDescriptions[0]; + Assert.Equal("foo", fooParam.Name); Assert.Equal(typeof(int), fooParam.Type); Assert.Equal(typeof(int), fooParam.ModelMetadata.ModelType); Assert.Equal(BindingSource.Path, fooParam.Source); Assert.True(fooParam.IsRequired); var barParam = apiDescription.ParameterDescriptions[1]; + Assert.Equal("bar", barParam.Name); Assert.Equal(typeof(int), barParam.Type); Assert.Equal(typeof(int), barParam.ModelMetadata.ModelType); Assert.Equal(BindingSource.Query, barParam.Source); Assert.True(barParam.IsRequired); + + var fromBodyParam = apiDescription.ParameterDescriptions[2]; + Assert.Equal("fromBody", fromBodyParam.Name); + Assert.Equal(typeof(InferredJsonClass), fromBodyParam.Type); + Assert.Equal(typeof(InferredJsonClass), fromBodyParam.ModelMetadata.ModelType); + Assert.Equal(BindingSource.Body, fromBodyParam.Source); + Assert.True(fromBodyParam.IsRequired); } +#nullable disable + [Fact] public void TestParameterIsRequired() { @@ -711,8 +732,8 @@ public void HandleAcceptsMetadataWithTypeParameter() var parameterDescriptions = context.Results.SelectMany(r => r.ParameterDescriptions); var bodyParameterDescription = parameterDescriptions.Single(); Assert.Equal(typeof(InferredJsonClass), bodyParameterDescription.Type); - Assert.Equal(typeof(InferredJsonClass).Name, bodyParameterDescription.Name); - Assert.True(bodyParameterDescription.IsRequired); + Assert.Equal("inferredJsonClass", bodyParameterDescription.Name); + Assert.False(bodyParameterDescription.IsRequired); } [Fact] @@ -774,7 +795,7 @@ public void HandleDefaultIAcceptsMetadataForRequiredBodyParameter() var parameterDescriptions = context.Results.SelectMany(r => r.ParameterDescriptions); var bodyParameterDescription = parameterDescriptions.Single(); Assert.Equal(typeof(InferredJsonClass), bodyParameterDescription.Type); - Assert.Equal(typeof(InferredJsonClass).Name, bodyParameterDescription.Name); + Assert.Equal("inferredJsonClass", bodyParameterDescription.Name); Assert.True(bodyParameterDescription.IsRequired); // Assert @@ -808,7 +829,7 @@ public void HandleDefaultIAcceptsMetadataForOptionalBodyParameter() var parameterDescriptions = context.Results.SelectMany(r => r.ParameterDescriptions); var bodyParameterDescription = parameterDescriptions.Single(); Assert.Equal(typeof(InferredJsonClass), bodyParameterDescription.Type); - Assert.Equal(typeof(InferredJsonClass).Name, bodyParameterDescription.Name); + Assert.Equal("inferredJsonClass", bodyParameterDescription.Name); Assert.False(bodyParameterDescription.IsRequired); // Assert @@ -841,9 +862,9 @@ public void HandleIAcceptsMetadataWithConsumesAttributeAndInferredOptionalFromBo // Assert var parameterDescriptions = context.Results.SelectMany(r => r.ParameterDescriptions); var bodyParameterDescription = parameterDescriptions.Single(); - Assert.Equal(typeof(void), bodyParameterDescription.Type); - Assert.Equal(typeof(void).Name, bodyParameterDescription.Name); - Assert.True(bodyParameterDescription.IsRequired); + Assert.Equal(typeof(InferredJsonClass), bodyParameterDescription.Type); + Assert.Equal("inferredJsonClass", bodyParameterDescription.Name); + Assert.False(bodyParameterDescription.IsRequired); // Assert var requestFormats = context.Results.SelectMany(r => r.SupportedRequestFormats); From a169c00b268064746a9033ed92468abbc8c1c7e2 Mon Sep 17 00:00:00 2001 From: martincostello Date: Sun, 14 Nov 2021 14:32:10 +0000 Subject: [PATCH 23/23] Fix typo Add missing >. --- .../src/EndpointMetadataApiDescriptionProvider.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs b/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs index a24ff4e51f12..ed5eea1f602e 100644 --- a/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs +++ b/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs @@ -122,7 +122,7 @@ private ApiDescription CreateApiDescription(RouteEndpoint routeEndpoint, string { // Add a default body parameter if there was no explicitly defined parameter associated with // either the body or a form and the user explicity defined some metadata describing the - // content types the endpoint consumes (such as Accepts(...) or [Consumes(...)]). if (!hasBodyOrFormFileParameter) { var acceptsRequestType = acceptsMetadata.RequestType;