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.

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/BodyModelBinder.cs

@@ -65,10 +65,18 @@ private async Task<ModelBindingResult> BindModelCoreAsync(ModelBindingContext bi
                 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 @@ private async Task<ModelBindingResult> BindModelCoreAsync(ModelBindingContext bi
                 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

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 @@ public Task<ModelBindingResult> BindModelAsync(ModelBindingContext bindingContex
         {
             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);
             }
 

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/CompositeModelBinder.cs

@@ -76,12 +76,13 @@ private async Task<ModelBindingResult> RunModelBinders(ModelBindingContext bindi
                             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;

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/FormFileModelBinder.cs

@@ -43,15 +43,22 @@ public Task<ModelBindingResult> BindModelAsync(ModelBindingContext bindingContex
 
         private async Task<ModelBindingResult> 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<IFormFile>).IsAssignableFrom(bindingContext.ModelType))
             {
-                var postedFiles = await GetFormFilesAsync(bindingContext);
+                var postedFiles = await GetFormFilesAsync(modelName, bindingContext);
                 value = ModelBindingHelper.ConvertValuesToCollectionType(bindingContext.ModelType, postedFiles);
             }
             else
@@ -67,31 +74,29 @@ private async Task<ModelBindingResult> BindModelCoreAsync(ModelBindingContext bi
             }
             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);
 
                 return ModelBindingResult.Success(bindingContext.ModelName, value);
             }
         }
 
-        private async Task<List<IFormFile>> GetFormFilesAsync(ModelBindingContext bindingContext)
+        private async Task<List<IFormFile>> GetFormFilesAsync(string modelName, ModelBindingContext bindingContext)
         {
             var request = bindingContext.OperationBindingContext.HttpContext.Request;
             var postedFiles = new List<IFormFile>();
             if (request.HasFormContentType)
             {
                 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;

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs

@@ -77,11 +77,12 @@ public ValidationVisitor(
         /// <returns><c>true</c> if the object is valid, otherwise <c>false</c>.</returns>
         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 @@ private bool Visit(ModelMetadata metadata, string key, object model)
                 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 @@ private bool VisitChildren(IValidationStrategy strategy)
 
         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)
             {

test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/BodyModelBinderTests.cs

@@ -10,7 +10,6 @@
 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 @@ public async Task BindModel_NoInputFormatterFound_SetsModelStateError()
             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<Person>().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 @@ public async Task CustomFormatterDeserializationException_AddedToModelState()
             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 @@ public async Task NullFormatterError_AddedToModelState()
             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 @@ private static ModelBindingContext GetBindingContext(
 
             var bindingContext = new ModelBindingContext
             {
+                FieldName = "someField",
                 IsTopLevelObject = true,
                 ModelMetadata = metadataProvider.GetMetadataForType(modelType),
                 ModelName = "someName",

test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/CancellationTokenModelBinderTests.cs

@@ -5,6 +5,7 @@
 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 @@ private static ModelBindingContext GetBindingContext(Type modelType)
                     },
                     ModelBinder = new CancellationTokenModelBinder(),
                     MetadataProvider = metadataProvider,
-                }
+                },
+                ValidationState = new ValidationStateDictionary(),
             };
 
             return bindingContext;

test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/FormFileModelBinderTest.cs

@@ -35,7 +35,7 @@ public async Task FormFileModelBinder_SuppressesValidation()
 
             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 @@ public async Task FormFileModelBinder_ExpectMultipleFiles_BindSuccessful()
 
             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<IList<IFormFile>>(result.Model);

test/Microsoft.AspNetCore.Mvc.IntegrationTests/BinderTypeBasedModelBinderIntegrationTest.cs

@@ -44,11 +44,7 @@ public async Task BindParameter_WithModelBinderType_NullData_ReturnsNull()
 
             // 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]