From 902e735b27dea536ad868d13aa74d0e12017fa0f Mon Sep 17 00:00:00 2001 From: Pranav K Date: Tue, 2 Jun 2020 17:48:56 -0700 Subject: [PATCH] Configure the page handler / controller instance as the container when validating top-level properties (#22164) Validation attributes such as a CompareAttribute require a container to be configured in ValidationContext. This change configures uses the controller or page handler instance that the property is being bound on when binding top-level properties. Fixes https://github.com/dotnet/aspnetcore/issues/4895 --- ...icrosoft.AspNetCore.Mvc.Core.netcoreapp.cs | 5 +- .../ControllerBinderDelegateProvider.cs | 6 +- .../src/ModelBinding/ObjectModelValidator.cs | 24 +++++++- .../src/ModelBinding/ParameterBinder.cs | 35 ++++++++++-- .../Validation/ValidationVisitor.cs | 21 +++++++ src/Mvc/Mvc.Core/src/Resources.resx | 57 ++++++++++--------- .../ControllerBinderDelegateProviderTest.cs | 5 +- .../src/Infrastructure/PageBinderFactory.cs | 6 +- .../Infrastructure/PageBinderFactoryTest.cs | 9 +-- .../test/Mvc.FunctionalTests/BasicTests.cs | 28 ++++++++- .../RazorPagesWithBasePathTest.cs | 11 ++++ .../BindPropertiesWithValidationController.cs | 35 ++++++++++++ .../Validation/PageWithCompareValidation.cs | 27 +++++++++ .../PageWithCompareValidation.cshtml | 3 + 14 files changed, 227 insertions(+), 45 deletions(-) create mode 100644 src/Mvc/test/WebSites/BasicWebSite/Controllers/BindPropertiesWithValidationController.cs create mode 100644 src/Mvc/test/WebSites/RazorPagesWebSite/Pages/Validation/PageWithCompareValidation.cs create mode 100644 src/Mvc/test/WebSites/RazorPagesWebSite/Pages/Validation/PageWithCompareValidation.cshtml diff --git a/src/Mvc/Mvc.Core/ref/Microsoft.AspNetCore.Mvc.Core.netcoreapp.cs b/src/Mvc/Mvc.Core/ref/Microsoft.AspNetCore.Mvc.Core.netcoreapp.cs index 2aaa567743..d4672abb31 100644 --- a/src/Mvc/Mvc.Core/ref/Microsoft.AspNetCore.Mvc.Core.netcoreapp.cs +++ b/src/Mvc/Mvc.Core/ref/Microsoft.AspNetCore.Mvc.Core.netcoreapp.cs @@ -2685,13 +2685,15 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding public abstract Microsoft.AspNetCore.Mvc.ModelBinding.Validation.ValidationVisitor GetValidationVisitor(Microsoft.AspNetCore.Mvc.ActionContext actionContext, Microsoft.AspNetCore.Mvc.ModelBinding.Validation.IModelValidatorProvider validatorProvider, Microsoft.AspNetCore.Mvc.ModelBinding.Validation.ValidatorCache validatorCache, Microsoft.AspNetCore.Mvc.ModelBinding.IModelMetadataProvider metadataProvider, Microsoft.AspNetCore.Mvc.ModelBinding.Validation.ValidationStateDictionary validationState); public virtual void Validate(Microsoft.AspNetCore.Mvc.ActionContext actionContext, Microsoft.AspNetCore.Mvc.ModelBinding.Validation.ValidationStateDictionary validationState, string prefix, object model) { } public virtual void Validate(Microsoft.AspNetCore.Mvc.ActionContext actionContext, Microsoft.AspNetCore.Mvc.ModelBinding.Validation.ValidationStateDictionary validationState, string prefix, object model, Microsoft.AspNetCore.Mvc.ModelBinding.ModelMetadata metadata) { } + public virtual void Validate(Microsoft.AspNetCore.Mvc.ActionContext actionContext, Microsoft.AspNetCore.Mvc.ModelBinding.Validation.ValidationStateDictionary validationState, string prefix, object model, Microsoft.AspNetCore.Mvc.ModelBinding.ModelMetadata metadata, object container) { } } public partial class ParameterBinder { public ParameterBinder(Microsoft.AspNetCore.Mvc.ModelBinding.IModelMetadataProvider modelMetadataProvider, Microsoft.AspNetCore.Mvc.ModelBinding.IModelBinderFactory modelBinderFactory, Microsoft.AspNetCore.Mvc.ModelBinding.Validation.IObjectModelValidator validator, Microsoft.Extensions.Options.IOptions mvcOptions, Microsoft.Extensions.Logging.ILoggerFactory loggerFactory) { } protected Microsoft.Extensions.Logging.ILogger Logger { [System.Runtime.CompilerServices.CompilerGeneratedAttribute] get { throw null; } } - [System.Diagnostics.DebuggerStepThroughAttribute] public virtual System.Threading.Tasks.Task BindModelAsync(Microsoft.AspNetCore.Mvc.ActionContext actionContext, Microsoft.AspNetCore.Mvc.ModelBinding.IModelBinder modelBinder, Microsoft.AspNetCore.Mvc.ModelBinding.IValueProvider valueProvider, Microsoft.AspNetCore.Mvc.Abstractions.ParameterDescriptor parameter, Microsoft.AspNetCore.Mvc.ModelBinding.ModelMetadata metadata, object value) { throw null; } + [System.Diagnostics.DebuggerStepThroughAttribute] + public virtual System.Threading.Tasks.ValueTask BindModelAsync(Microsoft.AspNetCore.Mvc.ActionContext actionContext, Microsoft.AspNetCore.Mvc.ModelBinding.IModelBinder modelBinder, Microsoft.AspNetCore.Mvc.ModelBinding.IValueProvider valueProvider, Microsoft.AspNetCore.Mvc.Abstractions.ParameterDescriptor parameter, Microsoft.AspNetCore.Mvc.ModelBinding.ModelMetadata metadata, object value, object container) { throw null; } } public partial class PrefixContainer { @@ -3246,6 +3248,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation protected virtual void SuppressValidation(string key) { } public bool Validate(Microsoft.AspNetCore.Mvc.ModelBinding.ModelMetadata metadata, string key, object model) { throw null; } public virtual bool Validate(Microsoft.AspNetCore.Mvc.ModelBinding.ModelMetadata metadata, string key, object model, bool alwaysValidateAtTopLevel) { throw null; } + public virtual bool Validate(Microsoft.AspNetCore.Mvc.ModelBinding.ModelMetadata metadata, string key, object model, bool alwaysValidateAtTopLevel, object container) { throw null; } protected virtual bool ValidateNode() { throw null; } protected virtual bool Visit(Microsoft.AspNetCore.Mvc.ModelBinding.ModelMetadata metadata, string key, object model) { throw null; } protected virtual bool VisitChildren(Microsoft.AspNetCore.Mvc.ModelBinding.Validation.IValidationStrategy strategy) { throw null; } diff --git a/src/Mvc/Mvc.Core/src/Controllers/ControllerBinderDelegateProvider.cs b/src/Mvc/Mvc.Core/src/Controllers/ControllerBinderDelegateProvider.cs index e7668e7b96..a590b6b19d 100644 --- a/src/Mvc/Mvc.Core/src/Controllers/ControllerBinderDelegateProvider.cs +++ b/src/Mvc/Mvc.Core/src/Controllers/ControllerBinderDelegateProvider.cs @@ -84,7 +84,8 @@ namespace Microsoft.AspNetCore.Mvc.Controllers valueProvider, parameter, modelMetadata, - value: null); + value: null, + container: null); // Parameters do not have containers. if (result.IsModelSet) { @@ -110,7 +111,8 @@ namespace Microsoft.AspNetCore.Mvc.Controllers valueProvider, property, modelMetadata, - value: null); + value: null, + container: controller); if (result.IsModelSet) { diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/ObjectModelValidator.cs b/src/Mvc/Mvc.Core/src/ModelBinding/ObjectModelValidator.cs index 43785d55cd..dbcd90376c 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/ObjectModelValidator.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/ObjectModelValidator.cs @@ -77,6 +77,28 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding string prefix, object model, ModelMetadata metadata) + => Validate(actionContext, validationState, prefix, model, metadata, container: null); + + /// + /// Validates the provided object model. + /// If is and the 's + /// is , will add one or more + /// model state errors that + /// would not. + /// + /// The . + /// The . + /// The model prefix key. + /// The model object. + /// The . + /// The model container + public virtual void Validate( + ActionContext actionContext, + ValidationStateDictionary validationState, + string prefix, + object model, + ModelMetadata metadata, + object container) { var visitor = GetValidationVisitor( actionContext, @@ -85,7 +107,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding _modelMetadataProvider, validationState); - visitor.Validate(metadata, prefix, model, alwaysValidateAtTopLevel: metadata.IsRequired); + visitor.Validate(metadata, prefix, model, alwaysValidateAtTopLevel: metadata.IsRequired, container); } /// diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/ParameterBinder.cs b/src/Mvc/Mvc.Core/src/ModelBinding/ParameterBinder.cs index 77942ea538..c39ac3192a 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/ParameterBinder.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/ParameterBinder.cs @@ -82,13 +82,34 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding /// The . /// The initial model value. /// The result of model binding. - public virtual async Task BindModelAsync( + public virtual Task BindModelAsync( ActionContext actionContext, IModelBinder modelBinder, IValueProvider valueProvider, ParameterDescriptor parameter, ModelMetadata metadata, object value) + => BindModelAsync(actionContext, modelBinder, valueProvider, parameter, metadata, value, container: null).AsTask(); + + /// + /// Binds a model specified by using as the initial value. + /// + /// The . + /// The . + /// The . + /// The + /// The . + /// The initial model value. + /// The container for the model. + /// The result of model binding. + public virtual async ValueTask BindModelAsync( + ActionContext actionContext, + IModelBinder modelBinder, + IValueProvider valueProvider, + ParameterDescriptor parameter, + ModelMetadata metadata, + object value, + object container) { if (actionContext == null) { @@ -164,7 +185,8 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding parameter, metadata, modelBindingContext, - modelBindingResult); + modelBindingResult, + container); Logger.DoneAttemptingToValidateParameterOrProperty(parameter, metadata); } @@ -192,7 +214,8 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding ParameterDescriptor parameter, ModelMetadata metadata, ModelBindingContext modelBindingContext, - ModelBindingResult modelBindingResult) + ModelBindingResult modelBindingResult, + object container) { RecalculateModelMetadata(parameter, modelBindingResult, ref metadata); @@ -211,7 +234,8 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding modelBindingContext.ValidationState, modelBindingContext.ModelName, modelBindingResult.Model, - metadata); + metadata, + container); } else if (metadata.IsRequired) { @@ -240,7 +264,8 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding modelBindingContext.ValidationState, modelName, modelBindingResult.Model, - metadata); + metadata, + container); } } diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Validation/ValidationVisitor.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Validation/ValidationVisitor.cs index 963db52af1..ba2bbcf214 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Validation/ValidationVisitor.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Validation/ValidationVisitor.cs @@ -133,7 +133,24 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation /// If true, applies validation rules even if the top-level value is null. /// true if the object is valid, otherwise false. public virtual bool Validate(ModelMetadata metadata, string key, object model, bool alwaysValidateAtTopLevel) + => Validate(metadata, key, model, alwaysValidateAtTopLevel, container: null); + + /// + /// Validates a object. + /// + /// The associated with the model. + /// The model prefix key. + /// The model object. + /// If true, applies validation rules even if the top-level value is null. + /// The model container. + /// true if the object is valid, otherwise false. + public virtual bool Validate(ModelMetadata metadata, string key, object model, bool alwaysValidateAtTopLevel, object container) { + if (container != null && metadata.MetadataKind != ModelMetadataKind.Property) + { + throw new ArgumentException(Resources.FormatValidationVisitor_ContainerCannotBeSpecified(metadata.MetadataKind)); + } + if (model == null && key != null && !alwaysValidateAtTopLevel) { var entry = ModelState[key]; @@ -148,6 +165,10 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation return true; } + // Container is non-null only when validation top-level properties. Start off by treating "container" as the "Model" instance. + // Invoking StateManager.Recurse later in this invocation will result in it being correctly used as the container instance during the + // validation of "model". + Model = container; return Visit(metadata, key, model); } diff --git a/src/Mvc/Mvc.Core/src/Resources.resx b/src/Mvc/Mvc.Core/src/Resources.resx index 147aa93602..2450503a43 100644 --- a/src/Mvc/Mvc.Core/src/Resources.resx +++ b/src/Mvc/Mvc.Core/src/Resources.resx @@ -1,17 +1,17 @@  - @@ -516,4 +516,7 @@ Failed to read the request form. {0} + + A container cannot be specified when the ModelMetada is of kind '{0}'. + \ No newline at end of file diff --git a/src/Mvc/Mvc.Core/test/Controllers/ControllerBinderDelegateProviderTest.cs b/src/Mvc/Mvc.Core/test/Controllers/ControllerBinderDelegateProviderTest.cs index 97f879169e..0fc291db31 100644 --- a/src/Mvc/Mvc.Core/test/Controllers/ControllerBinderDelegateProviderTest.cs +++ b/src/Mvc/Mvc.Core/test/Controllers/ControllerBinderDelegateProviderTest.cs @@ -1151,8 +1151,9 @@ namespace Microsoft.AspNetCore.Mvc.Controllers It.IsAny(), It.IsAny(), It.IsAny(), + null, null)) - .Returns((ActionContext context, IModelBinder modelBinder, IValueProvider valueProvider, ParameterDescriptor descriptor, ModelMetadata metadata, object v) => + .Returns((ActionContext context, IModelBinder modelBinder, IValueProvider valueProvider, ParameterDescriptor descriptor, ModelMetadata metadata, object v, object c) => { ModelBindingResult result; if (descriptor.Name == "accountId") @@ -1172,7 +1173,7 @@ namespace Microsoft.AspNetCore.Mvc.Controllers result = ModelBindingResult.Failed(); } - return Task.FromResult(result); + return new ValueTask(result); }); var controllerContext = GetControllerContext(actionDescriptor); diff --git a/src/Mvc/Mvc.RazorPages/src/Infrastructure/PageBinderFactory.cs b/src/Mvc/Mvc.RazorPages/src/Infrastructure/PageBinderFactory.cs index aab346896f..72b72fdef8 100644 --- a/src/Mvc/Mvc.RazorPages/src/Infrastructure/PageBinderFactory.cs +++ b/src/Mvc/Mvc.RazorPages/src/Infrastructure/PageBinderFactory.cs @@ -78,7 +78,8 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Infrastructure valueProvider, property, modelMetadata, - value: null); + value: null, + container: instance); if (result.IsModelSet) { @@ -159,7 +160,8 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Infrastructure valueProvider, parameter, modelMetadata, - value: null); + value: null, + container: null); // Parameters do not have containers. if (result.IsModelSet) { diff --git a/src/Mvc/Mvc.RazorPages/test/Infrastructure/PageBinderFactoryTest.cs b/src/Mvc/Mvc.RazorPages/test/Infrastructure/PageBinderFactoryTest.cs index ca9078bd72..d32c3d96cb 100644 --- a/src/Mvc/Mvc.RazorPages/test/Infrastructure/PageBinderFactoryTest.cs +++ b/src/Mvc/Mvc.RazorPages/test/Infrastructure/PageBinderFactoryTest.cs @@ -827,22 +827,23 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Infrastructure public IList Descriptors { get; } = new List(); - public override Task BindModelAsync( + public override ValueTask BindModelAsync( ActionContext actionContext, IModelBinder modelBinder, IValueProvider valueProvider, ParameterDescriptor parameter, ModelMetadata metadata, - object value) + object value, + object container) { Descriptors.Add(parameter); if (_args.TryGetValue(parameter.Name, out var result)) { - return Task.FromResult(ModelBindingResult.Success(result)); + return new ValueTask(ModelBindingResult.Success(result)); } - return Task.FromResult(ModelBindingResult.Failed()); + return new ValueTask(ModelBindingResult.Failed()); } } diff --git a/src/Mvc/test/Mvc.FunctionalTests/BasicTests.cs b/src/Mvc/test/Mvc.FunctionalTests/BasicTests.cs index 7af4c54524..d935f9a2a5 100644 --- a/src/Mvc/test/Mvc.FunctionalTests/BasicTests.cs +++ b/src/Mvc/test/Mvc.FunctionalTests/BasicTests.cs @@ -3,13 +3,15 @@ using System; using System.Collections.Generic; +using System.Linq; using System.Net; using System.Net.Http; using System.Net.Http.Headers; +using System.Net.Http.Json; using System.Reflection; +using System.Text.Json; using System.Threading.Tasks; using BasicWebSite.Models; -using System.Text.Json; using Xunit; namespace Microsoft.AspNetCore.Mvc.FunctionalTests @@ -627,6 +629,30 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests Assert.Equal("OnGetTestName", content); } + [Fact] + public async Task BindPropertiesAppliesValidation() + { + // Act + var response = await Client.GetAsync("BindPropertiesWithValidation/Action?Password=Test&ConfirmPassword=different"); + + // Assert + await response.AssertStatusCodeAsync(HttpStatusCode.BadRequest); + var problem = await response.Content.ReadFromJsonAsync(); + + Assert.Collection( + problem.Errors.OrderBy(e => e.Key), + kvp => + { + Assert.Equal("ConfirmPassword", kvp.Key); + Assert.Equal("Password and confirm password do not match.", Assert.Single(kvp.Value)); + }, + kvp => + { + Assert.Equal("UserName", kvp.Key); + Assert.Equal("User name is required.", Assert.Single(kvp.Value)); + }); + } + [Fact] public async Task InvalidForm_ResultsInModelError() { diff --git a/src/Mvc/test/Mvc.FunctionalTests/RazorPagesWithBasePathTest.cs b/src/Mvc/test/Mvc.FunctionalTests/RazorPagesWithBasePathTest.cs index 2435142f2f..6318eb76ea 100644 --- a/src/Mvc/test/Mvc.FunctionalTests/RazorPagesWithBasePathTest.cs +++ b/src/Mvc/test/Mvc.FunctionalTests/RazorPagesWithBasePathTest.cs @@ -510,6 +510,17 @@ Hello from /Pages/Shared/"; Assert.Contains("18 ≤ Age ≤ 60", response); } + [Fact] + public async Task CompareValidationAttributes_OnTopLevelProperties() + { + // Act + var response = await Client.GetStringAsync("/Validation/PageWithCompareValidation?password=test&comparePassword=different"); + + // Assert + Assert.Contains("User name is required", response); + Assert.Contains("Password and confirm password do not match.", response); + } + [Fact] public async Task ValidationAttributes_OnHandlerParameters() { diff --git a/src/Mvc/test/WebSites/BasicWebSite/Controllers/BindPropertiesWithValidationController.cs b/src/Mvc/test/WebSites/BasicWebSite/Controllers/BindPropertiesWithValidationController.cs new file mode 100644 index 0000000000..810226de2b --- /dev/null +++ b/src/Mvc/test/WebSites/BasicWebSite/Controllers/BindPropertiesWithValidationController.cs @@ -0,0 +1,35 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System.ComponentModel.DataAnnotations; +using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Mvc.ModelBinding; + +namespace BasicWebSite +{ + [BindProperties(SupportsGet = true)] + public class BindPropertiesWithValidationController : Controller + { + [Required(ErrorMessage = "User name is required.")] + public string UserName { get; set; } + + [Required] + public string Password { get; set; } + + [Compare(nameof(Password), ErrorMessage = "Password and confirm password do not match.")] + public string ConfirmPassword { get; set; } + + [BindNever] + public string BindNeverProperty { get; set; } + + public IActionResult Action() + { + if (!ModelState.IsValid) + { + return ValidationProblem(); + } + + return Ok(); + } + } +} diff --git a/src/Mvc/test/WebSites/RazorPagesWebSite/Pages/Validation/PageWithCompareValidation.cs b/src/Mvc/test/WebSites/RazorPagesWebSite/Pages/Validation/PageWithCompareValidation.cs new file mode 100644 index 0000000000..8ca3f8dd2e --- /dev/null +++ b/src/Mvc/test/WebSites/RazorPagesWebSite/Pages/Validation/PageWithCompareValidation.cs @@ -0,0 +1,27 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System.ComponentModel.DataAnnotations; +using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Mvc.RazorPages; + +namespace RazorPagesWebSite +{ + public class PageWithCompareValidation : PageModel + { + [BindProperty(SupportsGet = true)] + [Required(ErrorMessage = "User name is required.")] + public string UserName { get; set; } + + [BindProperty(SupportsGet = true)] + [Required(ErrorMessage = "Password is required.")] + public string Password { get; set; } + + [BindProperty(SupportsGet = true)] + [Compare(nameof(Password), ErrorMessage = "Password and confirm password do not match.")] + public int ConfirmPassword { get; set; } + + [Required] // Here to make sure we do not validate an unbound property. + public string NotBoundProperty { get; set; } + } +} diff --git a/src/Mvc/test/WebSites/RazorPagesWebSite/Pages/Validation/PageWithCompareValidation.cshtml b/src/Mvc/test/WebSites/RazorPagesWebSite/Pages/Validation/PageWithCompareValidation.cshtml new file mode 100644 index 0000000000..2e4d157977 --- /dev/null +++ b/src/Mvc/test/WebSites/RazorPagesWebSite/Pages/Validation/PageWithCompareValidation.cshtml @@ -0,0 +1,3 @@ +@page +@model PageWithCompareValidation +