From faba95287ee761f5a208ad1df42239ca9c9b3fa4 Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Thu, 28 Jan 2016 09:43:31 -0800 Subject: [PATCH] Fix for #3743 - CancellationToken validation This is a fix for the specific case reported in #3743 and also a few related cases where the object being validated does not come from user input. Because the CancellationToken is bound using the 'empty prefix', suppressing validation causes the validation system to suppress ALL keys in the MSD. This will wipe out validation info for other, unrelated model data. The same can occur for [FromServices], IFormCollection, IFormFile, and HttpRequestMessage (anythere when MSD key == null and SuppressValidation == true). The other change here is related to the [FromBody]. We don't want to create an entry in the model state for a 'valid' model with the empty prefix. This can cause the system to miss validation errors. If you end up in a situation where there's an entry with the empty prefix for an invalid state, you won't accidentally miss validation errors. --- .../ModelBinding/BodyModelBinder.cs | 19 +- .../CancellationTokenModelBinder.cs | 8 +- .../ModelBinding/CompositeModelBinder.cs | 9 +- .../ModelBinding/FormFileModelBinder.cs | 29 ++-- .../Validation/ValidationVisitor.cs | 21 ++- .../ModelBinding/BodyModelBinderTests.cs | 36 +++- .../CancellationTokenModelBinderTests.cs | 4 +- .../ModelBinding/FormFileModelBinderTest.cs | 4 +- ...nderTypeBasedModelBinderIntegrationTest.cs | 6 +- .../BodyValidationIntegrationTests.cs | 40 ++--- ...MutableObjectModelBinderIntegrationTest.cs | 27 +-- .../ValidationIntegrationTests.cs | 162 ++++++++++++++++-- 12 files changed, 262 insertions(+), 103 deletions(-) diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/BodyModelBinder.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/BodyModelBinder.cs index 4ad43c9168..f854657dd1 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/BodyModelBinder.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/BodyModelBinder.cs @@ -65,10 +65,18 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding throw new ArgumentNullException(nameof(bindingContext)); } - // For compatibility with MVC 5.0 for top level object we want to consider an empty key instead of - // the parameter name/a custom name. In all other cases (like when binding body to a property) we - // consider the entire ModelName as a prefix. - var modelBindingKey = bindingContext.IsTopLevelObject ? string.Empty : bindingContext.ModelName; + // Special logic for body, treat the model name as string.Empty for the top level + // object, but allow an override via BinderModelName. The purpose of this is to try + // and be similar to the behavior for POCOs bound via traditional model binding. + string modelBindingKey; + if (bindingContext.IsTopLevelObject) + { + modelBindingKey = bindingContext.BinderModelName ?? string.Empty; + } + else + { + modelBindingKey = bindingContext.ModelName; + } var httpContext = bindingContext.OperationBindingContext.HttpContext; @@ -102,9 +110,6 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding var result = await formatter.ReadAsync(formatterContext); var model = result.Model; - // Ensure a "modelBindingKey" entry exists whether or not formatting was successful. - bindingContext.ModelState.SetModelValue(modelBindingKey, rawValue: model, attemptedValue: null); - if (result.HasError) { // Formatter encountered an error. Do not use the model it returned. As above, tell the model diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/CancellationTokenModelBinder.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/CancellationTokenModelBinder.cs index 8df6e9b69c..0b0097e254 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/CancellationTokenModelBinder.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/CancellationTokenModelBinder.cs @@ -3,6 +3,7 @@ using System.Threading; using System.Threading.Tasks; +using Microsoft.AspNetCore.Mvc.ModelBinding.Validation; namespace Microsoft.AspNetCore.Mvc.ModelBinding { @@ -16,7 +17,12 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding { if (bindingContext.ModelType == typeof(CancellationToken)) { - var model = bindingContext.OperationBindingContext.HttpContext.RequestAborted; + // We need to force boxing now, so we can insert the same reference to the boxed CancellationToken + // in both the ValidationState and ModelBindingResult. + // + // DO NOT simplify this code by removing the cast. + var model = (object)bindingContext.OperationBindingContext.HttpContext.RequestAborted; + bindingContext.ValidationState.Add(model, new ValidationStateEntry() { SuppressValidation = true }); return ModelBindingResult.SuccessAsync(bindingContext.ModelName, model); } diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/CompositeModelBinder.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/CompositeModelBinder.cs index 94e2893c39..c46f914720 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/CompositeModelBinder.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/CompositeModelBinder.cs @@ -76,12 +76,13 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding ValidationStateEntry entry; if (!bindingContext.ValidationState.TryGetValue(result.Model, out entry)) { - entry = new ValidationStateEntry(); + entry = new ValidationStateEntry() + { + Key = result.Key, + Metadata = bindingContext.ModelMetadata, + }; bindingContext.ValidationState.Add(result.Model, entry); } - - entry.Key = entry.Key ?? result.Key; - entry.Metadata = entry.Metadata ?? bindingContext.ModelMetadata; } return result; diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/FormFileModelBinder.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/FormFileModelBinder.cs index 5a16880412..794dfb6ebd 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/FormFileModelBinder.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/FormFileModelBinder.cs @@ -43,15 +43,22 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding private async Task BindModelCoreAsync(ModelBindingContext bindingContext) { + // If we're at the top level, then use the FieldName (paramter or property name). + // This handles the fact that there will be nothing in the ValueProviders for this parameter + // and so we'll do the right thing even though we 'fell-back' to the empty prefix. + var modelName = bindingContext.IsTopLevelObject + ? bindingContext.BinderModelName ?? bindingContext.FieldName + : bindingContext.ModelName; + object value; if (bindingContext.ModelType == typeof(IFormFile)) { - var postedFiles = await GetFormFilesAsync(bindingContext); + var postedFiles = await GetFormFilesAsync(modelName, bindingContext); value = postedFiles.FirstOrDefault(); } else if (typeof(IEnumerable).IsAssignableFrom(bindingContext.ModelType)) { - var postedFiles = await GetFormFilesAsync(bindingContext); + var postedFiles = await GetFormFilesAsync(modelName, bindingContext); value = ModelBindingHelper.ConvertValuesToCollectionType(bindingContext.ModelType, postedFiles); } else @@ -67,9 +74,14 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding } else { - bindingContext.ValidationState.Add(value, new ValidationStateEntry() { SuppressValidation = true }); + bindingContext.ValidationState.Add(value, new ValidationStateEntry() + { + Key = modelName, + SuppressValidation = true + }); + bindingContext.ModelState.SetModelValue( - bindingContext.ModelName, + modelName, rawValue: null, attemptedValue: null); @@ -77,7 +89,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding } } - private async Task> GetFormFilesAsync(ModelBindingContext bindingContext) + private async Task> GetFormFilesAsync(string modelName, ModelBindingContext bindingContext) { var request = bindingContext.OperationBindingContext.HttpContext.Request; var postedFiles = new List(); @@ -85,13 +97,6 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding { var form = await request.ReadFormAsync(); - // If we're at the top level, then use the FieldName (paramter or property name). - // This handles the fact that there will be nothing in the ValueProviders for this parameter - // and so we'll do the right thing even though we 'fell-back' to the empty prefix. - var modelName = bindingContext.IsTopLevelObject - ? bindingContext.FieldName - : bindingContext.ModelName; - foreach (var file in form.Files) { ContentDispositionHeaderValue parsedContentDisposition; diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs index c9ee25b460..e4780832d7 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs @@ -77,11 +77,12 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation /// true if the object is valid, otherwise false. public bool Validate(ModelMetadata metadata, string key, object model) { - if (model == null) + if (model == null && key != null) { - if (_modelState.GetValidationState(key) != ModelValidationState.Valid) + var entry = _modelState[key]; + if (entry != null && entry.ValidationState != ModelValidationState.Valid) { - _modelState.MarkFieldValid(key); + entry.ValidationState = ModelValidationState.Valid; } return true; @@ -166,13 +167,14 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation SuppressValidation(key); return false; } - else if ((entry != null && entry.SuppressValidation)) + else if (entry != null && entry.SuppressValidation) { - SuppressValidation(key); + // Use the key on the entry, because we might not have entries in model state. + SuppressValidation(entry.Key); return true; } - using (StateManager.Recurse(this, key, metadata, model, strategy)) + using (StateManager.Recurse(this, key ?? string.Empty, metadata, model, strategy)) { if (_metadata.IsEnumerableType) { @@ -271,6 +273,13 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation private void SuppressValidation(string key) { + if (key == null) + { + // If the key is null, that means that we shouldn't expect any entries in ModelState for + // this value, so there's nothing to do. + return; + } + var entries = _modelState.FindKeysWithPrefix(key); foreach (var entry in entries) { diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/BodyModelBinderTests.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/BodyModelBinderTests.cs index 2179704e52..757b7cce60 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/BodyModelBinderTests.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/BodyModelBinderTests.cs @@ -10,7 +10,6 @@ using System.Threading.Tasks; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Http.Internal; using Microsoft.AspNetCore.Mvc.Formatters; -using Microsoft.AspNetCore.Routing; using Microsoft.Net.Http.Headers; using Moq; using Xunit; @@ -73,12 +72,40 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding Assert.False(binderResult.IsModelSet); Assert.Null(binderResult.Model); - // Key is empty because this was a top-level binding. + // Key is the empty string because this was a top-level binding. var entry = Assert.Single(bindingContext.ModelState); Assert.Equal(string.Empty, entry.Key); Assert.Single(entry.Value.Errors); } + [Fact] + public async Task BindModel_NoInputFormatterFound_SetsModelStateError_RespectsBinderModelName() + { + // Arrange + var provider = new TestModelMetadataProvider(); + provider.ForType().BindingDetails(d => d.BindingSource = BindingSource.Body); + + var bindingContext = GetBindingContext(typeof(Person), metadataProvider: provider); + bindingContext.BinderModelName = "custom"; + + var binder = bindingContext.OperationBindingContext.ModelBinder; + + // Act + var binderResult = await binder.BindModelAsync(bindingContext); + + // Assert + + // Returns non-null because it understands the metadata type. + Assert.NotNull(binderResult); + Assert.False(binderResult.IsModelSet); + Assert.Null(binderResult.Model); + + // Key is the bindermodelname because this was a top-level binding. + var entry = Assert.Single(bindingContext.ModelState); + Assert.Equal("custom", entry.Key); + Assert.Single(entry.Value.Errors); + } + [Fact] public async Task BindModel_IsGreedy() { @@ -165,7 +192,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding Assert.False(binderResult.IsModelSet); Assert.Null(binderResult.Model); - // Key is empty because this was a top-level binding. + // Key is the empty string because this was a top-level binding. var entry = Assert.Single(bindingContext.ModelState); Assert.Equal(string.Empty, entry.Key); var errorMessage = Assert.Single(entry.Value.Errors).Exception.Message; @@ -200,7 +227,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding Assert.False(binderResult.IsModelSet); Assert.Null(binderResult.Model); - // Key is empty because this was a top-level binding. + // Key is the empty string because this was a top-level binding. var entry = Assert.Single(bindingContext.ModelState); Assert.Equal(string.Empty, entry.Key); var errorMessage = Assert.Single(entry.Value.Errors).Exception.Message; @@ -267,6 +294,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding var bindingContext = new ModelBindingContext { + FieldName = "someField", IsTopLevelObject = true, ModelMetadata = metadataProvider.GetMetadataForType(modelType), ModelName = "someName", diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/CancellationTokenModelBinderTests.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/CancellationTokenModelBinderTests.cs index d2d9a4e35d..73afe109b1 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/CancellationTokenModelBinderTests.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/CancellationTokenModelBinderTests.cs @@ -5,6 +5,7 @@ using System; using System.Threading; using System.Threading.Tasks; using Microsoft.AspNetCore.Http.Internal; +using Microsoft.AspNetCore.Mvc.ModelBinding.Validation; using Xunit; namespace Microsoft.AspNetCore.Mvc.ModelBinding.Test @@ -60,7 +61,8 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Test }, ModelBinder = new CancellationTokenModelBinder(), MetadataProvider = metadataProvider, - } + }, + ValidationState = new ValidationStateDictionary(), }; return bindingContext; diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/FormFileModelBinderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/FormFileModelBinderTest.cs index 109c761b91..73ba91af60 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/FormFileModelBinderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/FormFileModelBinderTest.cs @@ -35,7 +35,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding var entry = bindingContext.ValidationState[result.Model]; Assert.True(entry.SuppressValidation); - Assert.Null(entry.Key); + Assert.Equal("file", entry.Key); Assert.Null(entry.Metadata); } @@ -59,7 +59,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding var entry = bindingContext.ValidationState[result.Model]; Assert.True(entry.SuppressValidation); - Assert.Null(entry.Key); + Assert.Equal("file", entry.Key); Assert.Null(entry.Metadata); var files = Assert.IsAssignableFrom>(result.Model); diff --git a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/BinderTypeBasedModelBinderIntegrationTest.cs b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/BinderTypeBasedModelBinderIntegrationTest.cs index 1b0f94549c..8d41962de0 100644 --- a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/BinderTypeBasedModelBinderIntegrationTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/BinderTypeBasedModelBinderIntegrationTest.cs @@ -44,11 +44,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests // ModelState (not set unless inner binder sets it) Assert.True(modelState.IsValid); - var entry = modelState[string.Empty]; - Assert.Null(entry.AttemptedValue); - Assert.Null(entry.RawValue); - Assert.Empty(entry.Errors); - Assert.Equal(ModelValidationState.Valid, entry.ValidationState); + Assert.Empty(modelState); } [Fact] diff --git a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/BodyValidationIntegrationTests.cs b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/BodyValidationIntegrationTests.cs index 5c8e262da9..9edfd1e322 100644 --- a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/BodyValidationIntegrationTests.cs +++ b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/BodyValidationIntegrationTests.cs @@ -100,8 +100,12 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Equal("CompanyName cannot be null or empty.", modelStateErrors["CompanyName"]); Assert.Equal("The field Price must be between 20 and 100.", modelStateErrors["Price"]); // Mono issue - https://github.com/aspnet/External/issues/19 - Assert.Equal(PlatformNormalizer.NormalizeContent("The Category field is required."), modelStateErrors["Category"]); - Assert.Equal(PlatformNormalizer.NormalizeContent("The Contact Us field is required."), modelStateErrors["Contact"]); + Assert.Equal( + PlatformNormalizer.NormalizeContent("The Category field is required."), + modelStateErrors["Category"]); + Assert.Equal( + PlatformNormalizer.NormalizeContent("The Contact Us field is required."), + modelStateErrors["Contact"]); Assert.Equal( PlatformNormalizer.NormalizeContent("The Detail2 field is required."), modelStateErrors["ProductDetails.Detail2"]); @@ -264,7 +268,9 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests var modelStateErrors = CreateValidationDictionary(modelState); Assert.Equal(2, modelStateErrors.Count); Assert.Equal("The field Price must be between 100 and 200.", modelStateErrors["Price"]); - Assert.Equal("The field Contact must be a string with a maximum length of 10.", modelStateErrors["Contact"]); + Assert.Equal( + "The field Contact must be a string with a maximum length of 10.", + modelStateErrors["Contact"]); } [Fact] @@ -380,9 +386,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Null(modelBindingResult.Model); Assert.True(modelState.IsValid); - var entry = Assert.Single(modelState); - Assert.Empty(entry.Key); - Assert.Null(entry.Value.RawValue); + Assert.Empty(modelState); } private class Person4 @@ -486,12 +490,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Equal(0, boundPerson.Address.RequiredNumber); Assert.True(modelState.IsValid); - var entry = Assert.Single(modelState); - Assert.Equal("CustomParameter.Address", entry.Key); - Assert.NotNull(entry.Value); - Assert.Null(entry.Value.AttemptedValue); - Assert.Same(boundPerson.Address, entry.Value.RawValue); - Assert.Empty(entry.Value.Errors); + Assert.Empty(modelState); } [Fact] @@ -527,16 +526,10 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Null(boundPerson.Address); Assert.False(modelState.IsValid); - Assert.Equal(2, modelState.Count); + Assert.Equal(1, modelState.Count); Assert.Equal(1, modelState.ErrorCount); - var state = modelState["CustomParameter.Address"]; - Assert.NotNull(state); - Assert.Null(state.AttemptedValue); - Assert.Null(state.RawValue); - Assert.Empty(state.Errors); - - state = modelState["CustomParameter.Address.Number"]; + var state = modelState["CustomParameter.Address.Number"]; Assert.NotNull(state); Assert.Null(state.AttemptedValue); Assert.Null(state.RawValue); @@ -597,9 +590,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.NotNull(boundPerson); Assert.False(modelState.IsValid); - Assert.Equal(2, modelState.Keys.Count); - var address = Assert.Single(modelState, kvp => kvp.Key == "CustomParameter.Address").Value; - Assert.Equal(ModelValidationState.Unvalidated, address.ValidationState); + Assert.Equal(1, modelState.Keys.Count); var street = Assert.Single(modelState, kvp => kvp.Key == "CustomParameter.Address.Street").Value; Assert.Equal(ModelValidationState.Invalid, street.ValidationState); @@ -655,8 +646,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.IsType(modelBindingResult.Model); Assert.True(modelState.IsValid); - var entry = Assert.Single(modelState); - Assert.Equal("CustomParameter.Address", entry.Key); + Assert.Empty(modelState); } private Dictionary CreateValidationDictionary(ModelStateDictionary modelState) diff --git a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/MutableObjectModelBinderIntegrationTest.cs b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/MutableObjectModelBinderIntegrationTest.cs index 068b0aaacb..e070aa0b74 100644 --- a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/MutableObjectModelBinderIntegrationTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/MutableObjectModelBinderIntegrationTest.cs @@ -10,8 +10,6 @@ using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Http.Features.Internal; using Microsoft.AspNetCore.Http.Internal; using Microsoft.AspNetCore.Mvc.Abstractions; -using Microsoft.AspNetCore.Mvc.Controllers; -using Microsoft.AspNetCore.Mvc.Infrastructure; using Microsoft.AspNetCore.Mvc.ModelBinding; using Microsoft.Extensions.Primitives; using Xunit; @@ -80,17 +78,13 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.NotNull(model.Customer.Address); Assert.Equal(AddressStreetContent, model.Customer.Address.Street); - Assert.Equal(2, modelState.Count); + Assert.Equal(1, modelState.Count); Assert.Equal(0, modelState.ErrorCount); Assert.True(modelState.IsValid); var entry = Assert.Single(modelState, e => e.Key == "parameter.Customer.Name").Value; Assert.Equal("bill", entry.AttemptedValue); Assert.Equal("bill", entry.RawValue); - - entry = Assert.Single(modelState, e => e.Key == "parameter.Customer.Address").Value; - Assert.Null(entry.AttemptedValue); // ModelState entries for body don't include original text. - Assert.Same(model.Customer.Address, entry.RawValue); } [Fact] @@ -125,17 +119,13 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.NotNull(model.Customer.Address); Assert.Equal(AddressStreetContent, model.Customer.Address.Street); - Assert.Equal(2, modelState.Count); + Assert.Equal(1, modelState.Count); Assert.Equal(0, modelState.ErrorCount); Assert.True(modelState.IsValid); var entry = Assert.Single(modelState, e => e.Key == "Customer.Name").Value; Assert.Equal("bill", entry.AttemptedValue); Assert.Equal("bill", entry.RawValue); - - entry = Assert.Single(modelState, e => e.Key == "Customer.Address").Value; - Assert.Null(entry.AttemptedValue); // ModelState entries for body don't include original text. - Assert.Same(model.Customer.Address, entry.RawValue); } [Fact] @@ -169,14 +159,11 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Equal("bill", model.Customer.Name); Assert.Null(model.Customer.Address); - Assert.Equal(2, modelState.Count); + Assert.Equal(1, modelState.Count); Assert.Equal(0, modelState.ErrorCount); Assert.True(modelState.IsValid); - var entry = Assert.Single(modelState, e => e.Key == "parameter.Customer.Address").Value; - Assert.Null(entry.AttemptedValue); - Assert.Null(entry.RawValue); - entry = Assert.Single(modelState, e => e.Key == "parameter.Customer.Name").Value; + var entry = Assert.Single(modelState, e => e.Key == "parameter.Customer.Name").Value; Assert.Equal("bill", entry.AttemptedValue); Assert.Equal("bill", entry.RawValue); } @@ -1355,13 +1342,9 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.NotNull(model.Customer.Address); Assert.Equal(AddressStreetContent, model.Customer.Address.Street); - Assert.Equal(1, modelState.Count); + Assert.Equal(0, modelState.Count); Assert.Equal(0, modelState.ErrorCount); Assert.True(modelState.IsValid); - - var entry = Assert.Single(modelState, e => e.Key == "Customer.Address").Value; - Assert.Null(entry.AttemptedValue); - Assert.Same(model.Customer.Address, entry.RawValue); } private class Order10 diff --git a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ValidationIntegrationTests.cs b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ValidationIntegrationTests.cs index 42da4b821b..9ad992e286 100644 --- a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ValidationIntegrationTests.cs +++ b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ValidationIntegrationTests.cs @@ -5,6 +5,7 @@ using System.Collections.Generic; using System.ComponentModel.DataAnnotations; using System.IO; using System.Text; +using System.Threading; using System.Threading.Tasks; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc.Abstractions; @@ -1141,7 +1142,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests // Act var modelBindingResult = await argumentBinder.BindModelAsync(parameter, operationContext); - Assert.Equal(4, modelState.Count); + Assert.Equal(3, modelState.Count); Assert.Equal(0, modelState.ErrorCount); Assert.True(modelState.IsValid); @@ -1159,14 +1160,6 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Equal("46", entry.AttemptedValue); Assert.Equal("46", entry.RawValue); Assert.Equal(ModelValidationState.Skipped, entry.ValidationState); - - entry = Assert.Single(modelState, e => e.Key == "OfficeAddress").Value; - Assert.Null(entry.AttemptedValue); - var address = Assert.IsType
(entry.RawValue); - Assert.Equal(47, address.Zip); - - // Address itself is not excluded from validation. - Assert.Equal(ModelValidationState.Valid, entry.ValidationState); } [Fact] @@ -1197,7 +1190,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests // We need to add another model state entry which should get marked as skipped so // we can prove that the JObject was skipped. - modelState.SetModelValue("message", "Hello", "Hello"); + modelState.SetModelValue("CustomParameter.message", "Hello", "Hello"); // Act var modelBindingResult = await argumentBinder.BindModelAsync(parameter, operationContext); @@ -1209,13 +1202,154 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Equal("Hello", message); Assert.True(modelState.IsValid); + Assert.Equal(1, modelState.Count); + + var entry = Assert.Single(modelState, kvp => kvp.Key == "CustomParameter.message"); + Assert.Equal(ModelValidationState.Skipped, entry.Value.ValidationState); + } + + // Regression test for https://github.com/aspnet/Mvc/issues/3743 + // + // A cancellation token that's bound with the empty prefix will end up suppressing + // the empty prefix. Since the empty prefix is a prefix of everything, this will + // basically result in clearing out all model errors, which is BAD. + // + // The fix is to treat non-user-input as have a key of null, which means that the MSD + // isn't even examined when it comes to supressing validation. + [Fact] + public async Task CancellationToken_WithEmptyPrefix_DoesNotSuppressUnrelatedErrors() + { + // Arrange + var argumentBinder = ModelBindingTestHelper.GetArgumentBinder(new TestMvcOptions().Value); + var parameter = new ParameterDescriptor + { + Name = "cancellationToken", + ParameterType = typeof(CancellationToken) + }; + + var operationContext = ModelBindingTestHelper.GetOperationBindingContext(); + + var httpContext = operationContext.HttpContext; + var modelState = operationContext.ActionContext.ModelState; + + // We need to add another model state entry - we want this to be ignored. + modelState.SetModelValue("message", "Hello", "Hello"); + + // Act + var modelBindingResult = await argumentBinder.BindModelAsync(parameter, operationContext); + + // Assert + Assert.True(modelBindingResult.IsModelSet); + Assert.NotNull(modelBindingResult.Model); + Assert.IsType(modelBindingResult.Model); + + Assert.False(modelState.IsValid); + Assert.Equal(1, modelState.Count); + + var entry = Assert.Single(modelState, kvp => kvp.Key == "message"); + Assert.Equal(ModelValidationState.Unvalidated, entry.Value.ValidationState); + } + + // Similar to CancellationToken_WithEmptyPrefix_DoesNotSuppressUnrelatedErrors - binding the body + // with the empty prefix should not cause unrelated modelstate entries to get suppressed. + [Fact] + public async Task FromBody_WithEmptyPrefix_DoesNotSuppressUnrelatedErrors_Valid() + { + // Arrange + var argumentBinder = ModelBindingTestHelper.GetArgumentBinder(new TestMvcOptions().Value); + var parameter = new ParameterDescriptor + { + Name = "Parameter1", + BindingInfo = new BindingInfo + { + BindingSource = BindingSource.Body + }, + ParameterType = typeof(Greeting) + }; + + var operationContext = ModelBindingTestHelper.GetOperationBindingContext( + request => + { + request.Body = new MemoryStream(Encoding.UTF8.GetBytes("{ message: \"Hello\" }")); + request.ContentType = "application/json"; + }); + + var httpContext = operationContext.HttpContext; + var modelState = operationContext.ActionContext.ModelState; + + // We need to add another model state entry which should not get changed. + modelState.SetModelValue("other.key", "1", "1"); + + // Act + var modelBindingResult = await argumentBinder.BindModelAsync(parameter, operationContext); + + // Assert + Assert.True(modelBindingResult.IsModelSet); + Assert.NotNull(modelBindingResult.Model); + var message = Assert.IsType(modelBindingResult.Model).Message; + Assert.Equal("Hello", message); + + Assert.False(modelState.IsValid); + Assert.Equal(1, modelState.Count); + + var entry = Assert.Single(modelState, kvp => kvp.Key == "other.key"); + Assert.Equal(ModelValidationState.Unvalidated, entry.Value.ValidationState); + } + + // Similar to CancellationToken_WithEmptyPrefix_DoesNotSuppressUnrelatedErrors - binding the body + // with the empty prefix should not cause unrelated modelstate entries to get suppressed. + [Fact] + public async Task FromBody_WithEmptyPrefix_DoesNotSuppressUnrelatedErrors_Invalid() + { + // Arrange + var argumentBinder = ModelBindingTestHelper.GetArgumentBinder(new TestMvcOptions().Value); + var parameter = new ParameterDescriptor + { + Name = "Parameter1", + BindingInfo = new BindingInfo + { + BindingSource = BindingSource.Body + }, + ParameterType = typeof(Greeting) + }; + + var operationContext = ModelBindingTestHelper.GetOperationBindingContext( + request => + { + // This string is too long and will have a validation error. + request.Body = new MemoryStream(Encoding.UTF8.GetBytes("{ message: \"Hello There\" }")); + request.ContentType = "application/json"; + }); + + var httpContext = operationContext.HttpContext; + var modelState = operationContext.ActionContext.ModelState; + + // We need to add another model state entry which should not get changed. + modelState.SetModelValue("other.key", "1", "1"); + + // Act + var modelBindingResult = await argumentBinder.BindModelAsync(parameter, operationContext); + + // Assert + Assert.True(modelBindingResult.IsModelSet); + Assert.NotNull(modelBindingResult.Model); + var message = Assert.IsType(modelBindingResult.Model).Message; + Assert.Equal("Hello There", message); + + Assert.False(modelState.IsValid); Assert.Equal(2, modelState.Count); - var entry = Assert.Single(modelState, kvp => kvp.Key == string.Empty); - Assert.Equal(ModelValidationState.Valid, entry.Value.ValidationState); + var entry = Assert.Single(modelState, kvp => kvp.Key == "Message"); + Assert.Equal(ModelValidationState.Invalid, entry.Value.ValidationState); - entry = Assert.Single(modelState, kvp => kvp.Key == "message"); - Assert.Equal(ModelValidationState.Skipped, entry.Value.ValidationState); + entry = Assert.Single(modelState, kvp => kvp.Key == "other.key"); + Assert.Equal(ModelValidationState.Unvalidated, entry.Value.ValidationState); + } + + private class Greeting + { + [StringLength(5)] + public string Message { get; set; } } private static void AssertRequiredError(string key, ModelError error)