From 180f735ac8c4e96ffe78aed4b93acfcbb2359599 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Wed, 16 Jan 2019 11:16:35 +1300 Subject: [PATCH] Fix non-parameter route constraints not called with endpoint routing for 2.2 (#6587) --- eng/PatchConfig.props | 1 + .../Internal/MvcEndpointDataSource.cs | 15 ++- .../Internal/MvcEndpointDataSourceTests.cs | 104 ++++++++++++++++++ .../RoutingTestsBase.cs | 26 +++++ .../NonParameterConstraintController.cs | 26 +++++ .../RoutingWebSite/QueryStringConstraint.cs | 16 +++ .../test/WebSites/RoutingWebSite/Startup.cs | 6 + .../RoutingWebSite/StartupWith21Compat.cs | 6 + 8 files changed, 199 insertions(+), 1 deletion(-) create mode 100644 src/Mvc/test/WebSites/RoutingWebSite/Controllers/NonParameterConstraintController.cs create mode 100644 src/Mvc/test/WebSites/RoutingWebSite/QueryStringConstraint.cs diff --git a/eng/PatchConfig.props b/eng/PatchConfig.props index 42789ba1b1..cb12734457 100644 --- a/eng/PatchConfig.props +++ b/eng/PatchConfig.props @@ -31,6 +31,7 @@ Later on, this will be checked using this condition: Microsoft.AspNetCore.AspNetCoreModuleV2; Microsoft.AspNetCore.Authentication.Google; Microsoft.AspNetCore.Http; + Microsoft.AspNetCore.Mvc.Core; Microsoft.AspNetCore.Server.IIS; java:signalr; diff --git a/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcEndpointDataSource.cs b/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcEndpointDataSource.cs index df110bee25..e63cda39fe 100644 --- a/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcEndpointDataSource.cs +++ b/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcEndpointDataSource.cs @@ -224,6 +224,15 @@ namespace Microsoft.AspNetCore.Mvc.Internal var newPathSegments = routePattern.PathSegments.ToList(); var hasLinkGenerationEndpoint = false; + // This is required because we create modified copies of the route pattern using its segments + // A segment with a parameter will automatically include its policies + // Non-parameter policies need to be manually included + var nonParameterPolicyValues = routePattern.ParameterPolicies + .Where(p => routePattern.GetParameter(p.Key ?? string.Empty) == null && p.Value.Count > 0 && p.Value.First().ParameterPolicy != null) // Only GetParameter is required. Extra is for safety + .Select(p => new KeyValuePair(p.Key, p.Value.First().ParameterPolicy)) // Can only pass a single non-parameter to RouteParameter + .ToArray(); + var nonParameterPolicies = RouteValueDictionary.FromArray(nonParameterPolicyValues); + // Create a mutable copy var nonInlineDefaultsCopy = nonInlineDefaults != null ? new RouteValueDictionary(nonInlineDefaults) @@ -259,6 +268,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal resolvedRouteValues, name, GetPattern(ref patternStringBuilder, newPathSegments), + nonParameterPolicies, newPathSegments, nonInlineDefaultsCopy, routeOrder++, @@ -277,6 +287,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal resolvedRouteValues, name, GetPattern(ref patternStringBuilder, subPathSegments), + nonParameterPolicies, subPathSegments, nonInlineDefaultsCopy, routeOrder++, @@ -294,6 +305,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal resolvedRouteValues, name, GetPattern(ref patternStringBuilder, newPathSegments), + nonParameterPolicies, newPathSegments, nonInlineDefaultsCopy, routeOrder++, @@ -531,6 +543,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal IDictionary actionRouteValues, string routeName, string patternRawText, + object nonParameterPolicies, IEnumerable segments, object nonInlineDefaults, int order, @@ -561,7 +574,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal var endpoint = new RouteEndpoint( requestDelegate, - RoutePatternFactory.Pattern(patternRawText, defaults, parameterPolicies: null, segments), + RoutePatternFactory.Pattern(patternRawText, defaults, nonParameterPolicies, segments), order, metadataCollection, action.DisplayName); diff --git a/src/Mvc/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/MvcEndpointDataSourceTests.cs b/src/Mvc/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/MvcEndpointDataSourceTests.cs index a3e0ebe5c3..40c51069f5 100644 --- a/src/Mvc/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/MvcEndpointDataSourceTests.cs +++ b/src/Mvc/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/MvcEndpointDataSourceTests.cs @@ -14,6 +14,7 @@ using Microsoft.AspNetCore.Mvc.Filters; using Microsoft.AspNetCore.Mvc.Infrastructure; using Microsoft.AspNetCore.Mvc.Routing; using Microsoft.AspNetCore.Routing; +using Microsoft.AspNetCore.Routing.Constraints; using Microsoft.AspNetCore.Routing.Matching; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Options; @@ -311,6 +312,109 @@ namespace Microsoft.AspNetCore.Mvc.Internal Assert.Empty(endpoints); } + [Fact] + public void Endpoints_SingleAction_ConventionalRoute_ContainsNonParameterConstraint() + { + // Arrange + var actionDescriptorCollection = GetActionDescriptorCollection( + new { controller = "TestController", action = "TestAction", page = (string)null }); + var dataSource = CreateMvcEndpointDataSource(actionDescriptorCollection); + dataSource.ConventionalEndpointInfos.Add(CreateEndpointInfo( + string.Empty, + "{controller}/{action}/{id:range(0, 100)}", + new RouteValueDictionary(new { action = "TestAction" }), + new RouteValueDictionary(new { controller = "TestController", nonParameter = new CustomConstraint(), id = new IntRouteConstraint() }))); + + // Act + var endpoints = dataSource.Endpoints; + + // Assert + var endpoint = Assert.IsType(Assert.Single(endpoints)); + + var routePattern = endpoint.RoutePattern; + + Assert.Equal("TestController/TestAction/{id::range(0, 100)}", routePattern.RawText); + Assert.Collection(routePattern.ParameterPolicies.OrderBy(p => p.Key), + p => + { + Assert.Equal("id", p.Key); + Assert.Collection(p.Value, + c => Assert.IsType(c.ParameterPolicy), + c => Assert.Equal("range(0, 100)", c.Content)); + }, + p => + { + Assert.Equal("nonParameter", p.Key); + Assert.IsType(p.Value.Single().ParameterPolicy); + }); + } + + [Fact] + public void Endpoints_SingleAction_ConventionalRouteWithOptional_ContainsNonParameterConstraint() + { + // Arrange + var actionDescriptorCollection = GetActionDescriptorCollection( + new { controller = "TestController", action = "TestAction", page = (string)null }); + var dataSource = CreateMvcEndpointDataSource(actionDescriptorCollection); + dataSource.ConventionalEndpointInfos.Add(CreateEndpointInfo( + string.Empty, + "{controller}/{action}/{id?}", + new RouteValueDictionary(new { action = "TestAction" }), + new RouteValueDictionary(new { controller = "TestController", nonParameter = new CustomConstraint(), id = new IntRouteConstraint() }))); + + // Act + var endpoints = dataSource.Endpoints; + + // Assert + var endpoint1 = Assert.IsType(endpoints[0]); + var routePattern1 = endpoint1.RoutePattern; + Assert.Equal("TestController/{action=TestAction}/{id:?}", routePattern1.RawText); + Assert.Collection(routePattern1.ParameterPolicies.OrderBy(p => p.Key), + p => + { + Assert.Equal("id", p.Key); + Assert.IsType(p.Value.Single().ParameterPolicy); + }, + p => + { + Assert.Equal("nonParameter", p.Key); + Assert.IsType(p.Value.Single().ParameterPolicy); + }); + + var endpoint2 = Assert.IsType(endpoints[1]); + var routePattern2 = endpoint2.RoutePattern; + Assert.Equal("TestController", routePattern2.RawText); + Assert.Collection(routePattern2.ParameterPolicies.OrderBy(p => p.Key), + p => + { + Assert.Equal("nonParameter", p.Key); + Assert.IsType(p.Value.Single().ParameterPolicy); + }); + + var endpoint3 = Assert.IsType(endpoints[2]); + var routePattern3 = endpoint3.RoutePattern; + Assert.Equal("TestController/TestAction/{id:?}", routePattern3.RawText); + Assert.Collection(routePattern3.ParameterPolicies.OrderBy(p => p.Key), + p => + { + Assert.Equal("id", p.Key); + Assert.IsType(p.Value.Single().ParameterPolicy); + }, + p => + { + Assert.Equal("nonParameter", p.Key); + Assert.IsType(p.Value.Single().ParameterPolicy); + }); + } + + private class CustomConstraint : IRouteConstraint + { + public bool Match(HttpContext httpContext, IRouter route, string routeKey, RouteValueDictionary values, RouteDirection routeDirection) + { + throw new NotImplementedException(); + } + } + [Fact] public void Endpoints_SingleAction_AttributeRoute_ContainsParameterWithNullRequiredRouteValue() { diff --git a/src/Mvc/test/Microsoft.AspNetCore.Mvc.FunctionalTests/RoutingTestsBase.cs b/src/Mvc/test/Microsoft.AspNetCore.Mvc.FunctionalTests/RoutingTestsBase.cs index a58e745b47..f896e5e301 100644 --- a/src/Mvc/test/Microsoft.AspNetCore.Mvc.FunctionalTests/RoutingTestsBase.cs +++ b/src/Mvc/test/Microsoft.AspNetCore.Mvc.FunctionalTests/RoutingTestsBase.cs @@ -26,6 +26,32 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests public HttpClient Client { get; } + [Fact] + public async Task ConventionalRoutedAction_RouteHasNonParameterConstraint_RouteConstraintRun_Allowed() + { + // Arrange & Act + var response = await Client.GetAsync("http://localhost/NonParameterConstraintRoute/NonParameterConstraint/Index?allowed=true"); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + + var body = await response.Content.ReadAsStringAsync(); + var result = JsonConvert.DeserializeObject(body); + + Assert.Equal("NonParameterConstraint", result.Controller); + Assert.Equal("Index", result.Action); + } + + [Fact] + public async Task ConventionalRoutedAction_RouteHasNonParameterConstraint_RouteConstraintRun_Denied() + { + // Arrange & Act + var response = await Client.GetAsync("http://localhost/NonParameterConstraintRoute/NonParameterConstraint/Index?allowed=false"); + + // Assert + Assert.Equal(HttpStatusCode.NotFound, response.StatusCode); + } + [Fact] public async Task ConventionalRoutedAction_RouteContainsPage_RouteNotMatched() { diff --git a/src/Mvc/test/WebSites/RoutingWebSite/Controllers/NonParameterConstraintController.cs b/src/Mvc/test/WebSites/RoutingWebSite/Controllers/NonParameterConstraintController.cs new file mode 100644 index 0000000000..af9ac7e600 --- /dev/null +++ b/src/Mvc/test/WebSites/RoutingWebSite/Controllers/NonParameterConstraintController.cs @@ -0,0 +1,26 @@ +// 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; +using System.Collections.Generic; +using System.Linq; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Mvc; + +namespace RoutingWebSite +{ + public class NonParameterConstraintController : Controller + { + private readonly TestResponseGenerator _generator; + + public NonParameterConstraintController(TestResponseGenerator generator) + { + _generator = generator; + } + + public IActionResult Index() + { + return _generator.Generate("/NonParameterConstraintRoute/NonParameterConstraint/Index"); + } + } +} diff --git a/src/Mvc/test/WebSites/RoutingWebSite/QueryStringConstraint.cs b/src/Mvc/test/WebSites/RoutingWebSite/QueryStringConstraint.cs new file mode 100644 index 0000000000..d4661e0139 --- /dev/null +++ b/src/Mvc/test/WebSites/RoutingWebSite/QueryStringConstraint.cs @@ -0,0 +1,16 @@ +// 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 Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Routing; + +namespace RoutingWebSite +{ + public class QueryStringConstraint : IRouteConstraint + { + public bool Match(HttpContext httpContext, IRouter route, string routeKey, RouteValueDictionary values, RouteDirection routeDirection) + { + return httpContext.Request.Query["allowed"].ToString() == "true"; + } + } +} diff --git a/src/Mvc/test/WebSites/RoutingWebSite/Startup.cs b/src/Mvc/test/WebSites/RoutingWebSite/Startup.cs index 44bc08a25f..f43b96e2e9 100644 --- a/src/Mvc/test/WebSites/RoutingWebSite/Startup.cs +++ b/src/Mvc/test/WebSites/RoutingWebSite/Startup.cs @@ -49,6 +49,12 @@ namespace RoutingWebSite { app.UseMvc(routes => { + routes.MapRoute( + "NonParameterConstraintRoute", + "NonParameterConstraintRoute/{controller}/{action}", + defaults: null, + constraints: new { controller = "NonParameterConstraint", nonParameter = new QueryStringConstraint() }); + routes.MapRoute( "DataTokensRoute", "DataTokensRoute/{controller}/{action}", diff --git a/src/Mvc/test/WebSites/RoutingWebSite/StartupWith21Compat.cs b/src/Mvc/test/WebSites/RoutingWebSite/StartupWith21Compat.cs index 9b78b66d32..93a4ca2ea0 100644 --- a/src/Mvc/test/WebSites/RoutingWebSite/StartupWith21Compat.cs +++ b/src/Mvc/test/WebSites/RoutingWebSite/StartupWith21Compat.cs @@ -54,6 +54,12 @@ namespace RoutingWebSite { app.UseMvc(routes => { + routes.MapRoute( + "NonParameterConstraintRoute", + "NonParameterConstraintRoute/{controller}/{action}", + defaults: null, + constraints: new { controller = "NonParameterConstraint", nonParameter = new QueryStringConstraint() }); + routes.MapRoute( "DataTokensRoute", "DataTokensRoute/{controller}/{action}",