From 22b7484217b186fe0be0371f335ea32255205210 Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Fri, 8 May 2020 13:23:44 +0200 Subject: [PATCH 1/2] Implemented content model binder in netcore. --- .../Umbraco.Tests.UnitTests.csproj | 1 + .../ContentModelBinderTests.cs | 131 ++++++++++++ .../Filters/ModelBindingExceptionFilter.cs | 76 +++++++ .../ModelBinders/ContentModelBinder.cs | 192 ++++++++++++++++++ .../ContentModelBinderProvider.cs | 30 +++ .../ModelBinders/ModelBindingException.cs | 46 +++++ src/Umbraco.Web.UI.NetCore/Startup.cs | 6 +- src/Umbraco.Web/Mvc/ContentModelBinder.cs | 3 +- .../Mvc/ModelBindingExceptionFilter.cs | 1 + 9 files changed, 483 insertions(+), 3 deletions(-) create mode 100644 src/Umbraco.Tests.UnitTests/Umbraco.Web.Common/ContentModelBinderTests.cs create mode 100644 src/Umbraco.Web.Common/Filters/ModelBindingExceptionFilter.cs create mode 100644 src/Umbraco.Web.Common/ModelBinders/ContentModelBinder.cs create mode 100644 src/Umbraco.Web.Common/ModelBinders/ContentModelBinderProvider.cs create mode 100644 src/Umbraco.Web.Common/ModelBinders/ModelBindingException.cs diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Tests.UnitTests.csproj b/src/Umbraco.Tests.UnitTests/Umbraco.Tests.UnitTests.csproj index f60b400f3f..c5732870f3 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Tests.UnitTests.csproj +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Tests.UnitTests.csproj @@ -20,6 +20,7 @@ + diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Web.Common/ContentModelBinderTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Web.Common/ContentModelBinderTests.cs new file mode 100644 index 0000000000..99a3ea4fe5 --- /dev/null +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Web.Common/ContentModelBinderTests.cs @@ -0,0 +1,131 @@ +using System; +using System.Collections.Generic; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Mvc.Abstractions; +using Microsoft.AspNetCore.Mvc.ModelBinding; +using Microsoft.AspNetCore.Routing; +using Moq; +using NUnit.Framework; +using Umbraco.Core; +using Umbraco.Core.Models.PublishedContent; +using Umbraco.Web.Common.ModelBinders; +using Umbraco.Web.Models; + +namespace Umbraco.Tests.UnitTests.Umbraco.Web.Common +{ + [TestFixture] + public class ContentModelBinderTests + { + [Test] + public void Does_Not_Bind_Model_When_UmbracoDataToken_Not_In_Route_Data() + { + // Arrange + var bindingContext = CreateBindingContext(typeof(ContentModel), withUmbracoDataToken: false); + var binder = new ContentModelBinder(); + + // Act + binder.BindModelAsync(bindingContext); + + // Assert + Assert.False(bindingContext.Result.IsModelSet); + } + + [Test] + public void Does_Not_Bind_Model_When_Source_Not_Of_Expected_Type() + { + // Arrange + var bindingContext = CreateBindingContext(typeof(ContentModel), source: new NonContentModel()); + var binder = new ContentModelBinder(); + + // Act + binder.BindModelAsync(bindingContext); + + // Assert + Assert.False(bindingContext.Result.IsModelSet); + } + + [Test] + public void Does_Not_Bind_Model_When_Source_Type_Matches_Model_Type() + { + // Arrange + var bindingContext = CreateBindingContext(typeof(ContentModel), source: new ContentModel(CreatePublishedContent())); + var binder = new ContentModelBinder(); + + // Act + binder.BindModelAsync(bindingContext); + + // Assert + Assert.False(bindingContext.Result.IsModelSet); + } + + [Test] + public void Binds_From_IPublishedContent_To_Content_Model() + { + // Arrange + var bindingContext = CreateBindingContext(typeof(ContentModel), source: CreatePublishedContent()); + var binder = new ContentModelBinder(); + + // Act + binder.BindModelAsync(bindingContext); + + // Assert + Assert.True(bindingContext.Result.IsModelSet); + } + + [Test] + public void Binds_From_IPublishedContent_To_Content_Model_Of_T() + { + // Arrange + var bindingContext = CreateBindingContext(typeof(ContentModel), source: new ContentModel(new ContentType2(CreatePublishedContent()))); + var binder = new ContentModelBinder(); + + // Act + binder.BindModelAsync(bindingContext); + + // Assert + Assert.True(bindingContext.Result.IsModelSet); + } + + private ModelBindingContext CreateBindingContext(Type modelType, bool withUmbracoDataToken = true, object source = null) + { + var httpContext = new DefaultHttpContext(); + var routeData = new RouteData(); + if (withUmbracoDataToken) + { + routeData.DataTokens.Add(Constants.Web.UmbracoDataToken, source); + } + + var actionContext = new ActionContext(httpContext, routeData, new ActionDescriptor()); + var metadataProvider = new EmptyModelMetadataProvider(); + var routeValueDictionary = new RouteValueDictionary(); + var valueProvider = new RouteValueProvider(BindingSource.Path, routeValueDictionary); + return new DefaultModelBindingContext + { + ActionContext = actionContext, + ModelMetadata = metadataProvider.GetMetadataForType(modelType), + ModelName = modelType.Name, + ValueProvider = valueProvider, + }; + } + + private class NonContentModel + { + } + + private IPublishedContent CreatePublishedContent() + { + return new ContentType2(new Mock().Object); + } + + public class ContentType1 : PublishedContentWrapped + { + public ContentType1(IPublishedContent content) : base(content) { } + } + + public class ContentType2 : ContentType1 + { + public ContentType2(IPublishedContent content) : base(content) { } + } + } +} diff --git a/src/Umbraco.Web.Common/Filters/ModelBindingExceptionFilter.cs b/src/Umbraco.Web.Common/Filters/ModelBindingExceptionFilter.cs new file mode 100644 index 0000000000..5367af68d5 --- /dev/null +++ b/src/Umbraco.Web.Common/Filters/ModelBindingExceptionFilter.cs @@ -0,0 +1,76 @@ +using System; +using System.Net; +using System.Text.RegularExpressions; +using Microsoft.AspNetCore.Http.Extensions; +using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Mvc.Filters; +using Umbraco.Core; +using Umbraco.Core.Configuration; +using Umbraco.Core.Models.PublishedContent; +using Umbraco.Web.Common.ModelBinders; + +namespace Umbraco.Web.Common.Filters +{ + /// + /// An exception filter checking if we get a or with the same model. + /// In which case it returns a redirect to the same page after 1 sec if not in debug mode. + /// + /// + /// This is only enabled when running PureLive + /// + internal class ModelBindingExceptionFilter : ActionFilterAttribute, IExceptionFilter + { + private static readonly Regex _getPublishedModelsTypesRegex = new Regex("Umbraco.Web.PublishedModels.(\\w+)", RegexOptions.Compiled); + + private readonly IExceptionFilterSettings _exceptionFilterSettings; + private readonly IPublishedModelFactory _publishedModelFactory; + + public ModelBindingExceptionFilter(IExceptionFilterSettings exceptionFilterSettings, IPublishedModelFactory publishedModelFactory) + { + _exceptionFilterSettings = exceptionFilterSettings; + _publishedModelFactory = publishedModelFactory ?? throw new ArgumentNullException(nameof(publishedModelFactory)); + } + + public void OnException(ExceptionContext filterContext) + { + var disabled = _exceptionFilterSettings?.Disabled ?? false; + if (_publishedModelFactory.IsLiveFactory() + && !disabled + && !filterContext.ExceptionHandled + && ((filterContext.Exception is ModelBindingException || filterContext.Exception is InvalidCastException) + && IsMessageAboutTheSameModelType(filterContext.Exception.Message))) + { + filterContext.HttpContext.Response.Headers.Add(HttpResponseHeader.RetryAfter.ToString(), "1"); + filterContext.Result = new RedirectResult(filterContext.HttpContext.Request.GetDisplayUrl(), false); + + filterContext.ExceptionHandled = true; + } + } + + /// + /// Returns true if the message is about two models with the same name. + /// + /// + /// Message could be something like: + /// + /// InvalidCastException: + /// [A]Umbraco.Web.PublishedModels.Home cannot be cast to [B]Umbraco.Web.PublishedModels.Home. Type A originates from 'App_Web_all.generated.cs.8f9494c4.rtdigm_z, Version=0.0.0.3, Culture=neutral, PublicKeyToken=null' in the context 'Default' at location 'C:\Users\User\AppData\Local\Temp\Temporary ASP.NET Files\root\c5c63f4d\c168d9d4\App_Web_all.generated.cs.8f9494c4.rtdigm_z.dll'. Type B originates from 'App_Web_all.generated.cs.8f9494c4.rbyqlplu, Version=0.0.0.5, Culture=neutral, PublicKeyToken=null' in the context 'Default' at location 'C:\Users\User\AppData\Local\Temp\Temporary ASP.NET Files\root\c5c63f4d\c168d9d4\App_Web_all.generated.cs.8f9494c4.rbyqlplu.dll'. + /// + /// + /// ModelBindingException: + /// Cannot bind source content type Umbraco.Web.PublishedModels.Home to model type Umbraco.Web.PublishedModels.Home. Both view and content models are PureLive, with different versions. The application is in an unstable state and is going to be restarted. The application is restarting now. + /// + /// + private bool IsMessageAboutTheSameModelType(string exceptionMessage) + { + var matches = _getPublishedModelsTypesRegex.Matches(exceptionMessage); + + if (matches.Count >= 2) + { + return string.Equals(matches[0].Value, matches[1].Value, StringComparison.InvariantCulture); + } + + return false; + } + } +} diff --git a/src/Umbraco.Web.Common/ModelBinders/ContentModelBinder.cs b/src/Umbraco.Web.Common/ModelBinders/ContentModelBinder.cs new file mode 100644 index 0000000000..43d9df4586 --- /dev/null +++ b/src/Umbraco.Web.Common/ModelBinders/ContentModelBinder.cs @@ -0,0 +1,192 @@ +using System; +using System.Text; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Mvc.ModelBinding; +using Umbraco.Core; +using Umbraco.Core.Models.PublishedContent; +using Umbraco.Web.Models; + +namespace Umbraco.Web.Common.ModelBinders +{ + /// + /// Maps view models, supporting mapping to and from any IPublishedContent or IContentModel. + /// + public class ContentModelBinder : IModelBinder + { + public Task BindModelAsync(ModelBindingContext bindingContext) + { + if (bindingContext.ActionContext.RouteData.DataTokens.TryGetValue(Core.Constants.Web.UmbracoDataToken, out var source) == false) + { + return Task.CompletedTask; + } + + // This model binder deals with IContentModel and IPublishedContent by extracting the model from the route's + // datatokens. This data token is set in 2 places: RenderRouteHandler, UmbracoVirtualNodeRouteHandler + // and both always set the model to an instance of `ContentModel`. + + // No need for type checks to ensure we have the appropriate binder, as in .NET Core this is handled in the provider, + // in this case ContentModelBinderProvider. + + // Being defensice though.... if for any reason the model is not either IContentModel or IPublishedContent, + // then we return since those are the only types this binder is dealing with. + if (source is IContentModel == false && source is IPublishedContent == false) + { + return Task.CompletedTask; + } + + BindModelAsync(bindingContext, source, bindingContext.ModelType); + return Task.CompletedTask; + } + + // source is the model that we have + // modelType is the type of the model that we need to bind to + // + // create a model object of the modelType by mapping: + // { ContentModel, ContentModel, IPublishedContent } + // to + // { ContentModel, ContentModel, IPublishedContent } + // + public Task BindModelAsync(ModelBindingContext bindingContext, object source, Type modelType) + { + // Null model, return + if (source == null) + { + return Task.CompletedTask; + } + + // If types already match, return + var sourceType = source.GetType(); + if (sourceType.Inherits(modelType)) // includes == + { + return Task.CompletedTask; + } + + // Try to grab the content + var sourceContent = source as IPublishedContent; // check if what we have is an IPublishedContent + if (sourceContent == null && sourceType.Implements()) + // else check if it's an IContentModel, and get the content + sourceContent = ((IContentModel)source).Content; + if (sourceContent == null) + { + // else check if we can convert it to a content + var attempt1 = source.TryConvertTo(); + if (attempt1.Success) sourceContent = attempt1.Result; + } + + // If we have a content + if (sourceContent != null) + { + // If model is IPublishedContent, check content type and return + if (modelType.Implements()) + { + if (sourceContent.GetType().Inherits(modelType) == false) + { + ThrowModelBindingException(true, false, sourceContent.GetType(), modelType); + } + + bindingContext.Result = ModelBindingResult.Success(sourceContent); + return Task.CompletedTask; + } + + // If model is ContentModel, create and return + if (modelType == typeof(ContentModel)) + { + bindingContext.Result = ModelBindingResult.Success(new ContentModel(sourceContent)); + return Task.CompletedTask; + } + + // If model is ContentModel, check content type, then create and return + if (modelType.IsGenericType && modelType.GetGenericTypeDefinition() == typeof(ContentModel<>)) + { + var targetContentType = modelType.GetGenericArguments()[0]; + if (sourceContent.GetType().Inherits(targetContentType) == false) + { + ThrowModelBindingException(true, true, sourceContent.GetType(), targetContentType); + } + + bindingContext.Result = ModelBindingResult.Success(Activator.CreateInstance(modelType, sourceContent)); + return Task.CompletedTask; + } + } + + // Last chance : try to convert + var attempt2 = source.TryConvertTo(modelType); + if (attempt2.Success) + { + bindingContext.Result = ModelBindingResult.Success(attempt2.Result); + return Task.CompletedTask; + } + + // Fail + ThrowModelBindingException(false, false, sourceType, modelType); + return Task.CompletedTask; + } + + private void ThrowModelBindingException(bool sourceContent, bool modelContent, Type sourceType, Type modelType) + { + var msg = new StringBuilder(); + + // prepare message + msg.Append("Cannot bind source"); + if (sourceContent) msg.Append(" content"); + msg.Append(" type "); + msg.Append(sourceType.FullName); + msg.Append(" to model"); + if (modelContent) msg.Append(" content"); + msg.Append(" type "); + msg.Append(modelType.FullName); + msg.Append("."); + + // raise event, to give model factories a chance at reporting + // the error with more details, and optionally request that + // the application restarts. + + var args = new ModelBindingArgs(sourceType, modelType, msg); + ModelBindingException?.Invoke(this, args); + + throw new ModelBindingException(msg.ToString()); + } + + /// + /// Contains event data for the event. + /// + public class ModelBindingArgs : EventArgs + { + /// + /// Initializes a new instance of the class. + /// + public ModelBindingArgs(Type sourceType, Type modelType, StringBuilder message) + { + SourceType = sourceType; + ModelType = modelType; + Message = message; + } + + /// + /// Gets the type of the source object. + /// + public Type SourceType { get; set; } + + /// + /// Gets the type of the view model. + /// + public Type ModelType { get; set; } + + /// + /// Gets the message string builder. + /// + /// Handlers of the event can append text to the message. + public StringBuilder Message { get; } + + /// + /// Gets or sets a value indicating whether the application should restart. + /// + public bool Restart { get; set; } + } + + /// + /// Occurs on model binding exceptions. + /// + public static event EventHandler ModelBindingException; + } +} diff --git a/src/Umbraco.Web.Common/ModelBinders/ContentModelBinderProvider.cs b/src/Umbraco.Web.Common/ModelBinders/ContentModelBinderProvider.cs new file mode 100644 index 0000000000..9ce38abe03 --- /dev/null +++ b/src/Umbraco.Web.Common/ModelBinders/ContentModelBinderProvider.cs @@ -0,0 +1,30 @@ +using Microsoft.AspNetCore.Mvc.ModelBinding; +using Microsoft.AspNetCore.Mvc.ModelBinding.Binders; +using Umbraco.Core.Models.PublishedContent; +using Umbraco.Web.Models; + +namespace Umbraco.Web.Common.ModelBinders +{ + /// + /// The provider for mapping view models, supporting mapping to and from any IPublishedContent or IContentModel. + /// + public class ContentModelBinderProvider : IModelBinderProvider + { + public IModelBinder GetBinder(ModelBinderProviderContext context) + { + var modelType = context.Metadata.ModelType; + + // Can bind to ContentModel (exact type match) + // or to ContentModel (exact generic type match) + // or to TContent where TContent : IPublishedContent (any IPublishedContent implementation) + if (modelType == typeof(ContentModel) || + (modelType.IsGenericType && modelType.GetGenericTypeDefinition() == typeof(ContentModel<>)) || + typeof(IPublishedContent).IsAssignableFrom(modelType)) + { + return new BinderTypeModelBinder(typeof(ContentModelBinder)); + } + + return null; + } + } +} diff --git a/src/Umbraco.Web.Common/ModelBinders/ModelBindingException.cs b/src/Umbraco.Web.Common/ModelBinders/ModelBindingException.cs new file mode 100644 index 0000000000..66ad642412 --- /dev/null +++ b/src/Umbraco.Web.Common/ModelBinders/ModelBindingException.cs @@ -0,0 +1,46 @@ +using System; +using System.Runtime.Serialization; + +namespace Umbraco.Web.Common.ModelBinders +{ + /// + /// The exception that is thrown when an error occurs while binding a source to a model. + /// + /// + /// Migrated to .NET Core + [Serializable] + public class ModelBindingException : Exception + { + /// + /// Initializes a new instance of the class. + /// + public ModelBindingException() + { } + + /// + /// Initializes a new instance of the class. + /// + /// The message that describes the error. + public ModelBindingException(string message) + : base(message) + { } + + /// + /// Initializes a new instance of the class. + /// + /// The error message that explains the reason for the exception. + /// The exception that is the cause of the current exception, or a null reference ( in Visual Basic) if no inner exception is specified. + public ModelBindingException(string message, Exception innerException) + : base(message, innerException) + { } + + /// + /// Initializes a new instance of the class. + /// + /// The that holds the serialized object data about the exception being thrown. + /// The that contains contextual information about the source or destination. + protected ModelBindingException(SerializationInfo info, StreamingContext context) + : base(info, context) + { } + } +} diff --git a/src/Umbraco.Web.UI.NetCore/Startup.cs b/src/Umbraco.Web.UI.NetCore/Startup.cs index 37440006aa..19daf13e89 100644 --- a/src/Umbraco.Web.UI.NetCore/Startup.cs +++ b/src/Umbraco.Web.UI.NetCore/Startup.cs @@ -13,6 +13,7 @@ using Umbraco.Core.Logging; using Umbraco.Web.BackOffice.AspNetCore; using Umbraco.Web.Common.AspNetCore; using Umbraco.Web.Common.Extensions; +using Umbraco.Web.Common.ModelBinders; using Umbraco.Web.Website.AspNetCore; using IHostingEnvironment = Umbraco.Core.Hosting.IHostingEnvironment; @@ -47,7 +48,10 @@ namespace Umbraco.Web.UI.BackOffice services.AddUmbracoCore(_env, out var factory); services.AddUmbracoWebsite(); - services.AddMvc(); + services.AddMvc(options => + { + options.ModelBinderProviders.Insert(0, new ContentModelBinderProvider()); + }); services.AddMiniProfiler(options => { options.ShouldProfile = request => false; // WebProfiler determine and start profiling. We should not use the MiniProfilerMiddleware to also profile diff --git a/src/Umbraco.Web/Mvc/ContentModelBinder.cs b/src/Umbraco.Web/Mvc/ContentModelBinder.cs index a66f8e9089..376ce737b4 100644 --- a/src/Umbraco.Web/Mvc/ContentModelBinder.cs +++ b/src/Umbraco.Web/Mvc/ContentModelBinder.cs @@ -1,6 +1,4 @@ using System; -using System.Globalization; -using System.Linq; using System.Text; using System.Web; using System.Web.Mvc; @@ -14,6 +12,7 @@ namespace Umbraco.Web.Mvc /// /// Maps view models, supporting mapping to and from any IPublishedContent or IContentModel. /// + /// Migrated to .NET Core public class ContentModelBinder : DefaultModelBinder, IModelBinderProvider { // use Instance diff --git a/src/Umbraco.Web/Mvc/ModelBindingExceptionFilter.cs b/src/Umbraco.Web/Mvc/ModelBindingExceptionFilter.cs index 82c87c0e0b..60ca151ce6 100644 --- a/src/Umbraco.Web/Mvc/ModelBindingExceptionFilter.cs +++ b/src/Umbraco.Web/Mvc/ModelBindingExceptionFilter.cs @@ -16,6 +16,7 @@ namespace Umbraco.Web.Mvc /// /// This is only enabled when running PureLive /// + /// Migrated to .NET Core internal class ModelBindingExceptionFilter : FilterAttribute, IExceptionFilter { private static readonly Regex GetPublishedModelsTypesRegex = new Regex("Umbraco.Web.PublishedModels.(\\w+)", RegexOptions.Compiled); From 8ae0e12512c4727cbc5295dbc8e7f08fc4ac7c5e Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Fri, 15 May 2020 10:33:09 +0200 Subject: [PATCH 2/2] Removed global setting of content model binder and used encoded URL in model binding exception filter redirect. --- .../Filters/ModelBindingExceptionFilter.cs | 2 +- src/Umbraco.Web.UI.NetCore/Startup.cs | 8 +------- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/src/Umbraco.Web.Common/Filters/ModelBindingExceptionFilter.cs b/src/Umbraco.Web.Common/Filters/ModelBindingExceptionFilter.cs index 5367af68d5..559a02e149 100644 --- a/src/Umbraco.Web.Common/Filters/ModelBindingExceptionFilter.cs +++ b/src/Umbraco.Web.Common/Filters/ModelBindingExceptionFilter.cs @@ -41,7 +41,7 @@ namespace Umbraco.Web.Common.Filters && IsMessageAboutTheSameModelType(filterContext.Exception.Message))) { filterContext.HttpContext.Response.Headers.Add(HttpResponseHeader.RetryAfter.ToString(), "1"); - filterContext.Result = new RedirectResult(filterContext.HttpContext.Request.GetDisplayUrl(), false); + filterContext.Result = new RedirectResult(filterContext.HttpContext.Request.GetEncodedUrl(), false); filterContext.ExceptionHandled = true; } diff --git a/src/Umbraco.Web.UI.NetCore/Startup.cs b/src/Umbraco.Web.UI.NetCore/Startup.cs index 19daf13e89..918b483bfe 100644 --- a/src/Umbraco.Web.UI.NetCore/Startup.cs +++ b/src/Umbraco.Web.UI.NetCore/Startup.cs @@ -11,13 +11,10 @@ using Umbraco.Core.Configuration; using Umbraco.Core.IO; using Umbraco.Core.Logging; using Umbraco.Web.BackOffice.AspNetCore; -using Umbraco.Web.Common.AspNetCore; using Umbraco.Web.Common.Extensions; -using Umbraco.Web.Common.ModelBinders; using Umbraco.Web.Website.AspNetCore; using IHostingEnvironment = Umbraco.Core.Hosting.IHostingEnvironment; - namespace Umbraco.Web.UI.BackOffice { public class Startup @@ -48,10 +45,7 @@ namespace Umbraco.Web.UI.BackOffice services.AddUmbracoCore(_env, out var factory); services.AddUmbracoWebsite(); - services.AddMvc(options => - { - options.ModelBinderProviders.Insert(0, new ContentModelBinderProvider()); - }); + services.AddMvc(); services.AddMiniProfiler(options => { options.ShouldProfile = request => false; // WebProfiler determine and start profiling. We should not use the MiniProfilerMiddleware to also profile