From 494c9f1b42d2eb9103a17374238230d6334596e7 Mon Sep 17 00:00:00 2001 From: Shannon Date: Fri, 17 Sep 2021 10:29:24 -0600 Subject: [PATCH 1/5] UmbracoRouteValueTransformer fixes --- .../UmbracoBuilderExtensions.cs | 2 +- .../Routing/UmbracoRouteValueTransformer.cs | 56 +++++++++++++------ 2 files changed, 41 insertions(+), 17 deletions(-) diff --git a/src/Umbraco.Web.Website/DependencyInjection/UmbracoBuilderExtensions.cs b/src/Umbraco.Web.Website/DependencyInjection/UmbracoBuilderExtensions.cs index 62ec5a9921..744bd13388 100644 --- a/src/Umbraco.Web.Website/DependencyInjection/UmbracoBuilderExtensions.cs +++ b/src/Umbraco.Web.Website/DependencyInjection/UmbracoBuilderExtensions.cs @@ -38,7 +38,7 @@ namespace Umbraco.Extensions builder.Services.AddDataProtection(); builder.Services.AddAntiforgery(); - builder.Services.AddScoped(); + builder.Services.AddSingleton(); builder.Services.AddSingleton(); builder.Services.AddSingleton(); builder.Services.AddSingleton(); diff --git a/src/Umbraco.Web.Website/Routing/UmbracoRouteValueTransformer.cs b/src/Umbraco.Web.Website/Routing/UmbracoRouteValueTransformer.cs index 41fd20a69b..9e0b71b61e 100644 --- a/src/Umbraco.Web.Website/Routing/UmbracoRouteValueTransformer.cs +++ b/src/Umbraco.Web.Website/Routing/UmbracoRouteValueTransformer.cs @@ -92,31 +92,41 @@ namespace Umbraco.Cms.Web.Website.Routing // If we aren't running, then we have nothing to route if (_runtime.Level != RuntimeLevel.Run) { - return values; + return null; } // will be null for any client side requests like JS, etc... - if (!_umbracoContextAccessor.TryGetUmbracoContext(out var umbracoContext)) + if (!_umbracoContextAccessor.TryGetUmbracoContext(out IUmbracoContext umbracoContext)) { - return values; + return null; } if (!_routableDocumentFilter.IsDocumentRequest(httpContext.Request.Path)) { - return values; + return null; + } + + // Don't execute if there are already UmbracoRouteValues assigned. + // This can occur if someone else is dynamically routing and in which case we don't want to overwrite + // the routing work being done there. + UmbracoRouteValues umbracoRouteValues = httpContext.Features.Get(); + if (umbracoRouteValues != null) + { + return null; } // Check if there is no existing content and return the no content controller if (!umbracoContext.Content.HasContent()) { - values[ControllerToken] = ControllerExtensions.GetControllerName(); - values[ActionToken] = nameof(RenderNoContentController.Index); - - return values; + return new RouteValueDictionary + { + [ControllerToken] = ControllerExtensions.GetControllerName(), + [ActionToken] = nameof(RenderNoContentController.Index) + }; } IPublishedRequest publishedRequest = await RouteRequestAsync(httpContext, umbracoContext); - UmbracoRouteValues umbracoRouteValues = await _routeValuesFactory.CreateAsync(httpContext, publishedRequest); + umbracoRouteValues = await _routeValuesFactory.CreateAsync(httpContext, publishedRequest); // Store the route values as a httpcontext feature httpContext.Features.Set(umbracoRouteValues); @@ -125,16 +135,27 @@ namespace Umbraco.Cms.Web.Website.Routing PostedDataProxyInfo postedInfo = GetFormInfo(httpContext, values); if (postedInfo != null) { - return HandlePostedValues(postedInfo, httpContext, values); + return HandlePostedValues(postedInfo, httpContext); } - values[ControllerToken] = umbracoRouteValues.ControllerName; + // See https://docs.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.routing.dynamicroutevaluetransformer.transformasync?view=aspnetcore-5.0#Microsoft_AspNetCore_Mvc_Routing_DynamicRouteValueTransformer_TransformAsync_Microsoft_AspNetCore_Http_HttpContext_Microsoft_AspNetCore_Routing_RouteValueDictionary_ + // We should apparenlty not be modified these values. + // So we create new ones. + var newValues = new RouteValueDictionary + { + [ControllerToken] = umbracoRouteValues.ControllerName + }; if (string.IsNullOrWhiteSpace(umbracoRouteValues.ActionName) == false) { - values[ActionToken] = umbracoRouteValues.ActionName; + newValues[ActionToken] = umbracoRouteValues.ActionName; } - return values; + // NOTE: If we are never returning null it means that it is not possible for another + // DynamicRouteValueTransformer to execute to set the route values. This one will + // always win even if it is a 404 because we manage all 404s via Umbraco and 404 + // handlers. + + return newValues; } private async Task RouteRequestAsync(HttpContext httpContext, IUmbracoContext umbracoContext) @@ -196,11 +217,14 @@ namespace Umbraco.Cms.Web.Website.Routing }; } - private RouteValueDictionary HandlePostedValues(PostedDataProxyInfo postedInfo, HttpContext httpContext, RouteValueDictionary values) + private RouteValueDictionary HandlePostedValues(PostedDataProxyInfo postedInfo, HttpContext httpContext) { // set the standard route values/tokens - values[ControllerToken] = postedInfo.ControllerName; - values[ActionToken] = postedInfo.ActionName; + var values = new RouteValueDictionary + { + [ControllerToken] = postedInfo.ControllerName, + [ActionToken] = postedInfo.ActionName + }; ControllerActionDescriptor surfaceControllerDescriptor = _controllerActionSearcher.Find(httpContext, postedInfo.ControllerName, postedInfo.ActionName); From d27dc05f328c20f13fa3ff09e9850f6950e2ebb3 Mon Sep 17 00:00:00 2001 From: Shannon Date: Fri, 17 Sep 2021 12:02:04 -0600 Subject: [PATCH 2/5] Return null from UmbracoRouteValueTransformer when there is no matches, use a custom IEndpointSelectorPolicy to deal with 404s. --- .../UmbracoBuilderExtensions.cs | 4 + .../Routing/NotFoundSelectorPolicy.cs | 79 +++++++++++++++++++ .../Routing/UmbracoRouteValueTransformer.cs | 16 ++-- 3 files changed, 94 insertions(+), 5 deletions(-) create mode 100644 src/Umbraco.Web.Website/Routing/NotFoundSelectorPolicy.cs diff --git a/src/Umbraco.Web.Website/DependencyInjection/UmbracoBuilderExtensions.cs b/src/Umbraco.Web.Website/DependencyInjection/UmbracoBuilderExtensions.cs index 744bd13388..72d8ec58ce 100644 --- a/src/Umbraco.Web.Website/DependencyInjection/UmbracoBuilderExtensions.cs +++ b/src/Umbraco.Web.Website/DependencyInjection/UmbracoBuilderExtensions.cs @@ -1,5 +1,7 @@ using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Routing; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.DependencyInjection.Extensions; using Microsoft.Extensions.Options; using Umbraco.Cms.Core.DependencyInjection; using Umbraco.Cms.Infrastructure.DependencyInjection; @@ -11,6 +13,7 @@ using Umbraco.Cms.Web.Website.Middleware; using Umbraco.Cms.Web.Website.Models; using Umbraco.Cms.Web.Website.Routing; using Umbraco.Cms.Web.Website.ViewEngines; +using static Microsoft.Extensions.DependencyInjection.ServiceDescriptor; namespace Umbraco.Extensions { @@ -40,6 +43,7 @@ namespace Umbraco.Extensions builder.Services.AddSingleton(); builder.Services.AddSingleton(); + builder.Services.TryAddEnumerable(Singleton()); builder.Services.AddSingleton(); builder.Services.AddSingleton(); diff --git a/src/Umbraco.Web.Website/Routing/NotFoundSelectorPolicy.cs b/src/Umbraco.Web.Website/Routing/NotFoundSelectorPolicy.cs new file mode 100644 index 0000000000..589c424629 --- /dev/null +++ b/src/Umbraco.Web.Website/Routing/NotFoundSelectorPolicy.cs @@ -0,0 +1,79 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Mvc.Controllers; +using Microsoft.AspNetCore.Routing; +using Microsoft.AspNetCore.Routing.Matching; +using Umbraco.Cms.Core.Routing; +using Umbraco.Cms.Web.Common.Controllers; +using Umbraco.Cms.Web.Common.Routing; + +namespace Umbraco.Cms.Web.Website.Routing +{ + /// + /// Used to handle 404 routes that haven't been handled by the end user + /// + internal class NotFoundSelectorPolicy : MatcherPolicy, IEndpointSelectorPolicy + { + private readonly Lazy _notFound; + private readonly EndpointDataSource _endpointDataSource; + + public NotFoundSelectorPolicy(EndpointDataSource endpointDataSource) + { + _notFound = new Lazy(GetNotFoundEndpoint); + _endpointDataSource = endpointDataSource; + } + + // return the endpoint for the RenderController.Index action. + private Endpoint GetNotFoundEndpoint() + { + Endpoint e = _endpointDataSource.Endpoints.First(x => + { + // return the endpoint for the RenderController.Index action. + ControllerActionDescriptor descriptor = x.Metadata?.GetMetadata(); + return descriptor.ControllerTypeInfo == typeof(RenderController) + && descriptor.ActionName == nameof(RenderController.Index); + }); + return e; + } + + public override int Order => 0; + + public bool AppliesToEndpoints(IReadOnlyList endpoints) + { + // Don't apply this filter to any endpoint group that is a controller route + // i.e. only dynamic routes. + foreach(Endpoint endpoint in endpoints) + { + ControllerAttribute controller = endpoint.Metadata?.GetMetadata(); + if (controller != null) + { + return false; + } + } + + // then ensure this is only applied if all endpoints are IDynamicEndpointMetadata + return endpoints.All(x => x.Metadata?.GetMetadata() != null); + } + + public Task ApplyAsync(HttpContext httpContext, CandidateSet candidates) + { + if (candidates.Count == 1 && candidates[0].Values == null) + { + UmbracoRouteValues umbracoRouteValues = httpContext.Features.Get(); + if (umbracoRouteValues?.PublishedRequest != null + && !umbracoRouteValues.PublishedRequest.HasPublishedContent() + && umbracoRouteValues.PublishedRequest.ResponseStatusCode == StatusCodes.Status404NotFound) + { + // not found/404 + httpContext.SetEndpoint(_notFound.Value); + } + } + + return Task.CompletedTask; + } + } +} diff --git a/src/Umbraco.Web.Website/Routing/UmbracoRouteValueTransformer.cs b/src/Umbraco.Web.Website/Routing/UmbracoRouteValueTransformer.cs index 9e0b71b61e..fc54350097 100644 --- a/src/Umbraco.Web.Website/Routing/UmbracoRouteValueTransformer.cs +++ b/src/Umbraco.Web.Website/Routing/UmbracoRouteValueTransformer.cs @@ -28,6 +28,7 @@ using RouteDirection = Umbraco.Cms.Core.Routing.RouteDirection; namespace Umbraco.Cms.Web.Website.Routing { + /// /// The route value transformer for Umbraco front-end routes /// @@ -138,6 +139,16 @@ namespace Umbraco.Cms.Web.Website.Routing return HandlePostedValues(postedInfo, httpContext); } + if (!umbracoRouteValues?.PublishedRequest?.HasPublishedContent() ?? false) + { + // No content was found, not by any registered 404 handlers and + // not by the IContentLastChanceFinder. In this case we want to return + // our default 404 page but we cannot return route values now because + // it's possible that a developer is handling dynamic routes too. + // Our 404 page will be handled with the NotFoundSelectorPolicy + return null; + } + // See https://docs.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.routing.dynamicroutevaluetransformer.transformasync?view=aspnetcore-5.0#Microsoft_AspNetCore_Mvc_Routing_DynamicRouteValueTransformer_TransformAsync_Microsoft_AspNetCore_Http_HttpContext_Microsoft_AspNetCore_Routing_RouteValueDictionary_ // We should apparenlty not be modified these values. // So we create new ones. @@ -150,11 +161,6 @@ namespace Umbraco.Cms.Web.Website.Routing newValues[ActionToken] = umbracoRouteValues.ActionName; } - // NOTE: If we are never returning null it means that it is not possible for another - // DynamicRouteValueTransformer to execute to set the route values. This one will - // always win even if it is a 404 because we manage all 404s via Umbraco and 404 - // handlers. - return newValues; } From 2ede06485cde6b28d06b66b377f5408bd3aae6ae Mon Sep 17 00:00:00 2001 From: Shannon Date: Fri, 17 Sep 2021 12:47:41 -0600 Subject: [PATCH 3/5] fix/add tests --- .../UmbracoRouteValueTransformerTests.cs | 34 ++++++++++++++----- 1 file changed, 26 insertions(+), 8 deletions(-) diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Web.Website/Routing/UmbracoRouteValueTransformerTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Web.Website/Routing/UmbracoRouteValueTransformerTests.cs index 47fcbd010c..d00f85fd0a 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Web.Website/Routing/UmbracoRouteValueTransformerTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Web.Website/Routing/UmbracoRouteValueTransformerTests.cs @@ -14,6 +14,7 @@ using NUnit.Framework; using Umbraco.Cms.Core; using Umbraco.Cms.Core.Configuration.Models; using Umbraco.Cms.Core.Events; +using Umbraco.Cms.Core.Models.PublishedContent; using Umbraco.Cms.Core.PublishedCache; using Umbraco.Cms.Core.Routing; using Umbraco.Cms.Core.Services; @@ -92,28 +93,28 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Web.Website.Routing => Mock.Of(x => x.RouteRequestAsync(It.IsAny(), It.IsAny()) == Task.FromResult(request)); [Test] - public async Task Noop_When_Runtime_Level_Not_Run() + public async Task Null_When_Runtime_Level_Not_Run() { UmbracoRouteValueTransformer transformer = GetTransformer( Mock.Of(), Mock.Of()); RouteValueDictionary result = await transformer.TransformAsync(new DefaultHttpContext(), new RouteValueDictionary()); - Assert.AreEqual(0, result.Count); + Assert.IsNull(result); } [Test] - public async Task Noop_When_No_Umbraco_Context() + public async Task Null_When_No_Umbraco_Context() { UmbracoRouteValueTransformer transformer = GetTransformerWithRunState( Mock.Of()); RouteValueDictionary result = await transformer.TransformAsync(new DefaultHttpContext(), new RouteValueDictionary()); - Assert.AreEqual(0, result.Count); + Assert.IsNull(result); } [Test] - public async Task Noop_When_Not_Document_Request() + public async Task Null_When_Not_Document_Request() { var umbracoContext = Mock.Of(); UmbracoRouteValueTransformer transformer = GetTransformerWithRunState( @@ -121,7 +122,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Web.Website.Routing Mock.Of(x => x.IsDocumentRequest(It.IsAny()) == false)); RouteValueDictionary result = await transformer.TransformAsync(new DefaultHttpContext(), new RouteValueDictionary()); - Assert.AreEqual(0, result.Count); + Assert.IsNull(result); } [Test] @@ -173,10 +174,10 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Web.Website.Routing } [Test] - public async Task Assigns_Values_To_RouteValueDictionary() + public async Task Assigns_Values_To_RouteValueDictionary_When_Content() { IUmbracoContext umbracoContext = GetUmbracoContext(true); - IPublishedRequest request = Mock.Of(); + IPublishedRequest request = Mock.Of(x => x.PublishedContent == Mock.Of()); UmbracoRouteValues routeValues = GetRouteValues(request); UmbracoRouteValueTransformer transformer = GetTransformerWithRunState( @@ -190,6 +191,23 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Web.Website.Routing Assert.AreEqual(routeValues.ActionName, result[ActionToken]); } + [Test] + public async Task Returns_Null_RouteValueDictionary_When_No_Content() + { + IUmbracoContext umbracoContext = GetUmbracoContext(true); + IPublishedRequest request = Mock.Of(x => x.PublishedContent == null); + UmbracoRouteValues routeValues = GetRouteValues(request); + + UmbracoRouteValueTransformer transformer = GetTransformerWithRunState( + Mock.Of(x => x.TryGetUmbracoContext(out umbracoContext)), + router: GetRouter(request), + routeValuesFactory: GetRouteValuesFactory(request)); + + RouteValueDictionary result = await transformer.TransformAsync(new DefaultHttpContext(), new RouteValueDictionary()); + + Assert.IsNull(result); + } + private class TestController : RenderController { public TestController(ILogger logger, ICompositeViewEngine compositeViewEngine, IUmbracoContextAccessor umbracoContextAccessor) From 57bbbfa7d3104b46bb48d403147d8b26bc3fe34e Mon Sep 17 00:00:00 2001 From: Shannon Date: Fri, 17 Sep 2021 13:35:43 -0600 Subject: [PATCH 4/5] better null checks --- src/Umbraco.Web.Website/Routing/NotFoundSelectorPolicy.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Umbraco.Web.Website/Routing/NotFoundSelectorPolicy.cs b/src/Umbraco.Web.Website/Routing/NotFoundSelectorPolicy.cs index 589c424629..e9370c94ce 100644 --- a/src/Umbraco.Web.Website/Routing/NotFoundSelectorPolicy.cs +++ b/src/Umbraco.Web.Website/Routing/NotFoundSelectorPolicy.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.Linq; using System.Threading.Tasks; @@ -34,8 +34,8 @@ namespace Umbraco.Cms.Web.Website.Routing { // return the endpoint for the RenderController.Index action. ControllerActionDescriptor descriptor = x.Metadata?.GetMetadata(); - return descriptor.ControllerTypeInfo == typeof(RenderController) - && descriptor.ActionName == nameof(RenderController.Index); + return descriptor?.ControllerTypeInfo == typeof(RenderController) + && descriptor?.ActionName == nameof(RenderController.Index); }); return e; } From 9306861d76a393801fe42a2f6cb0ed6b4dc81737 Mon Sep 17 00:00:00 2001 From: Shannon Date: Fri, 17 Sep 2021 13:49:12 -0600 Subject: [PATCH 5/5] Better candidate checking. --- .../Routing/NotFoundSelectorPolicy.cs | 22 ++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/src/Umbraco.Web.Website/Routing/NotFoundSelectorPolicy.cs b/src/Umbraco.Web.Website/Routing/NotFoundSelectorPolicy.cs index e9370c94ce..dbc06175fd 100644 --- a/src/Umbraco.Web.Website/Routing/NotFoundSelectorPolicy.cs +++ b/src/Umbraco.Web.Website/Routing/NotFoundSelectorPolicy.cs @@ -24,7 +24,7 @@ namespace Umbraco.Cms.Web.Website.Routing public NotFoundSelectorPolicy(EndpointDataSource endpointDataSource) { _notFound = new Lazy(GetNotFoundEndpoint); - _endpointDataSource = endpointDataSource; + _endpointDataSource = endpointDataSource; } // return the endpoint for the RenderController.Index action. @@ -46,7 +46,7 @@ namespace Umbraco.Cms.Web.Website.Routing { // Don't apply this filter to any endpoint group that is a controller route // i.e. only dynamic routes. - foreach(Endpoint endpoint in endpoints) + foreach (Endpoint endpoint in endpoints) { ControllerAttribute controller = endpoint.Metadata?.GetMetadata(); if (controller != null) @@ -61,7 +61,7 @@ namespace Umbraco.Cms.Web.Website.Routing public Task ApplyAsync(HttpContext httpContext, CandidateSet candidates) { - if (candidates.Count == 1 && candidates[0].Values == null) + if (AllInvalid(candidates)) { UmbracoRouteValues umbracoRouteValues = httpContext.Features.Get(); if (umbracoRouteValues?.PublishedRequest != null @@ -72,8 +72,20 @@ namespace Umbraco.Cms.Web.Website.Routing httpContext.SetEndpoint(_notFound.Value); } } - - return Task.CompletedTask; + + return Task.CompletedTask; + } + + private static bool AllInvalid(CandidateSet candidates) + { + for (int i = 0; i < candidates.Count; i++) + { + if (candidates.IsValidCandidate(i)) + { + return false; + } + } + return true; } } }