From 14429721d933104e17bd5cae0b7f2399ca55ab2d Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Tue, 13 Mar 2018 14:09:47 -0700 Subject: [PATCH] Make handler selector more flexible Some details of this pending discussion, but this is a new 2.1 change and compatibility switch in the spirit of making pages handler selection less error-prone. In particular we don't want anyone to have to define HEAD to do the trivial thing. This currently routes all 'safe' HTTP methods to the GET handler and all other HTTP methods to the POST handler. This is technically not the correct thing to do for OPTIONS and TRACE, so we might still do something different. The tests will change a little depending on exactly what we decide to do, but this is the main idea of the change. --- .../DefaultPageHandlerMethodSelector.cs | 124 ++++++++-- .../PageActionDescriptor.cs | 2 +- .../RazorPagesOptions.cs | 43 ++++ ...gesOptionsConfigureCompatibilityOptions.cs | 1 + .../RazorPagesTest.cs | 9 +- .../DefaultPageHandlerMethodSelectorTest.cs | 221 ++++++++++++++++-- test/WebSites/RazorPagesWebSite/Startup.cs | 3 +- .../RazorPagesWebSite/StartupWithBasePath.cs | 3 +- .../TempData/SetMessageAndRedirect.cshtml | 2 +- .../TempData/TempDataPageModelProperty.cshtml | 2 +- 10 files changed, 365 insertions(+), 45 deletions(-) diff --git a/src/Microsoft.AspNetCore.Mvc.RazorPages/Internal/DefaultPageHandlerMethodSelector.cs b/src/Microsoft.AspNetCore.Mvc.RazorPages/Internal/DefaultPageHandlerMethodSelector.cs index cabbcc8e86..e5d53f6203 100644 --- a/src/Microsoft.AspNetCore.Mvc.RazorPages/Internal/DefaultPageHandlerMethodSelector.cs +++ b/src/Microsoft.AspNetCore.Mvc.RazorPages/Internal/DefaultPageHandlerMethodSelector.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Generic; using System.Linq; +using Microsoft.Extensions.Options; using Microsoft.Extensions.Primitives; namespace Microsoft.AspNetCore.Mvc.RazorPages.Infrastructure @@ -12,6 +13,25 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Infrastructure { private const string Handler = "handler"; + private readonly IOptions _options; + + [Obsolete("This constructor will be removed in a future release. Use the other constructor.")] + public DefaultPageHandlerMethodSelector() + { + } + + public DefaultPageHandlerMethodSelector(IOptions options) + { + if (options == null) + { + throw new ArgumentNullException(nameof(options)); + } + + _options = options; + } + + private bool AllowFuzzyHttpMethodMatching => _options?.Value.AllowMappingHeadRequestsToGetHandler ?? false; + public HandlerMethodDescriptor Select(PageContext context) { var handlers = SelectHandlers(context); @@ -60,42 +80,71 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Infrastructure return null; } - private static List SelectHandlers(PageContext context) + private List SelectHandlers(PageContext context) { var handlers = context.ActionDescriptor.HandlerMethods; - List handlersToConsider = null; + var candidates = new List(); - var handlerName = Convert.ToString(context.RouteData.Values[Handler]); + // Name is optional, may not be provided. + var handlerName = GetHandlerName(context); - if (string.IsNullOrEmpty(handlerName) && - context.HttpContext.Request.Query.TryGetValue(Handler, out StringValues queryValues)) - { - handlerName = queryValues[0]; - } + // The handler selection process considers handlers according to a few criteria. Handlers + // have a defined HTTP method that they handle, and also optionally a 'name'. + // + // We don't really have a scenario for handler methods without a verb (we don't provide a way + // to create one). If we see one, it will just never match. + // + // The verb must match (with some fuzzy matching) and the handler name must match if + // there is one. + // + // The process is like this: + // + // 1. Match the possible candidates on HTTP method + // 1a. **Added in 2.1** if no candidates matched in 1, then do *fuzzy matching* + // 2. Match the candidates from 1 or 1a on handler name. + // Step 1: match on HTTP method. + var httpMethod = context.HttpContext.Request.Method; for (var i = 0; i < handlers.Count; i++) { var handler = handlers[i]; if (handler.HttpMethod != null && - !string.Equals(handler.HttpMethod, context.HttpContext.Request.Method, StringComparison.OrdinalIgnoreCase)) + string.Equals(handler.HttpMethod, httpMethod, StringComparison.OrdinalIgnoreCase)) { - continue; + candidates.Add(handler); } - else if (handler.Name != null && - !handler.Name.Equals(handlerName, StringComparison.OrdinalIgnoreCase)) - { - continue; - } - - if (handlersToConsider == null) - { - handlersToConsider = new List(); - } - - handlersToConsider.Add(handler); } - return handlersToConsider; + // Step 1a: do fuzzy HTTP method matching if needed. + if (candidates.Count == 0 && AllowFuzzyHttpMethodMatching) + { + var fuzzyHttpMethod = GetFuzzyMatchHttpMethod(context); + if (fuzzyHttpMethod != null) + { + for (var i = 0; i < handlers.Count; i++) + { + var handler = handlers[i]; + if (handler.HttpMethod != null && + string.Equals(handler.HttpMethod, fuzzyHttpMethod, StringComparison.OrdinalIgnoreCase)) + { + candidates.Add(handler); + } + } + } + } + + // Step 2: remove candiates with non-matching handlers. + for (var i = candidates.Count - 1; i >= 0; i--) + { + var handler = candidates[i]; + if (handler.Name != null && + !handler.Name.Equals(handlerName, StringComparison.OrdinalIgnoreCase)) + { + candidates.RemoveAt(i); + } + } + + return candidates; } private static int GetScore(HandlerMethodDescriptor descriptor) @@ -113,5 +162,34 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Infrastructure return 0; } } + + private static string GetHandlerName(PageContext context) + { + var handlerName = Convert.ToString(context.RouteData.Values[Handler]); + if (!string.IsNullOrEmpty(handlerName)) + { + return handlerName; + } + + if (context.HttpContext.Request.Query.TryGetValue(Handler, out StringValues queryValues)) + { + return queryValues[0]; + } + + return null; + } + + private static string GetFuzzyMatchHttpMethod(PageContext context) + { + var httpMethod = context.HttpContext.Request.Method; + + // Map HEAD to get. + if (string.Equals("HEAD", httpMethod, StringComparison.OrdinalIgnoreCase)) + { + return "GET"; + } + + return null; + } } } \ No newline at end of file diff --git a/src/Microsoft.AspNetCore.Mvc.RazorPages/PageActionDescriptor.cs b/src/Microsoft.AspNetCore.Mvc.RazorPages/PageActionDescriptor.cs index b4101c9309..bf4780d165 100644 --- a/src/Microsoft.AspNetCore.Mvc.RazorPages/PageActionDescriptor.cs +++ b/src/Microsoft.AspNetCore.Mvc.RazorPages/PageActionDescriptor.cs @@ -7,7 +7,7 @@ using Microsoft.AspNetCore.Mvc.Abstractions; namespace Microsoft.AspNetCore.Mvc.RazorPages { - [DebuggerDisplay(nameof(DebuggerDisplayString))] + [DebuggerDisplay("{DebuggerDisplayString,nq}")] public class PageActionDescriptor : ActionDescriptor { /// diff --git a/src/Microsoft.AspNetCore.Mvc.RazorPages/RazorPagesOptions.cs b/src/Microsoft.AspNetCore.Mvc.RazorPages/RazorPagesOptions.cs index 8a5fdf440e..ca2a991f2b 100644 --- a/src/Microsoft.AspNetCore.Mvc.RazorPages/RazorPagesOptions.cs +++ b/src/Microsoft.AspNetCore.Mvc.RazorPages/RazorPagesOptions.cs @@ -15,6 +15,7 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages public class RazorPagesOptions : IEnumerable { private readonly CompatibilitySwitch _allowAreas; + private readonly CompatibilitySwitch _allowMappingHeadRequestsToGetHandler; private readonly ICompatibilitySwitch[] _switches; private string _root = "/Pages"; @@ -23,10 +24,12 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages public RazorPagesOptions() { _allowAreas = new CompatibilitySwitch(nameof(AllowAreas)); + _allowMappingHeadRequestsToGetHandler = new CompatibilitySwitch(nameof(AllowMappingHeadRequestsToGetHandler)); _switches = new ICompatibilitySwitch[] { _allowAreas, + _allowMappingHeadRequestsToGetHandler, }; } @@ -94,6 +97,46 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages set => _allowAreas.Value = value; } + /// + /// Gets or sets a value that determines if HTTP method matching for Razor Pages handler methods will use + /// fuzzy matching. + /// Defaults to false. + /// + /// + /// + /// When enabled, Razor Pages handler methods will be more flexible in which HTTP methods will be accepted + /// by GET and POST handler methods. This allows a GET handler methods to accept the HEAD HTTP methods in + /// addition to GET. A more specific handler method can still be defined to accept HEAD, and the most + /// specific handler will be invoked. + /// + /// + /// This setting reduces the number of handler methods that must be written to correctly respond to typical + /// web traffic including requests from internet infrastructure such as web crawlers. + /// + /// + /// This property is associated with a compatibility switch and can provide a different behavior depending on + /// the configured compatibility version for the application. See for + /// guidance and examples of setting the application's compatibility version. + /// + /// + /// Configuring the desired of the value compatibility switch by calling this property's setter will take precedence + /// over the value implied by the application's . + /// + /// + /// If the application's compatibility version is set to then + /// this setting will have value false unless explicitly configured. + /// + /// + /// If the application's compatibility version is set to or + /// higher then this setting will have value true unless explicitly configured. + /// + /// + public bool AllowMappingHeadRequestsToGetHandler + { + get => _allowMappingHeadRequestsToGetHandler.Value; + set => _allowMappingHeadRequestsToGetHandler.Value = value; + } + /// /// Application relative path used as the root of discovery for Razor Page files associated with areas. /// Defaults to the /Areas directory under application root. diff --git a/src/Microsoft.AspNetCore.Mvc.RazorPages/RazorPagesOptionsConfigureCompatibilityOptions.cs b/src/Microsoft.AspNetCore.Mvc.RazorPages/RazorPagesOptionsConfigureCompatibilityOptions.cs index ddc8d77a93..8f5d21d8bc 100644 --- a/src/Microsoft.AspNetCore.Mvc.RazorPages/RazorPagesOptionsConfigureCompatibilityOptions.cs +++ b/src/Microsoft.AspNetCore.Mvc.RazorPages/RazorPagesOptionsConfigureCompatibilityOptions.cs @@ -26,6 +26,7 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages if (Version >= CompatibilityVersion.Version_2_1) { values[nameof(RazorPagesOptions.AllowAreas)] = true; + values[nameof(RazorPagesOptions.AllowMappingHeadRequestsToGetHandler)] = true; } return values; diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/RazorPagesTest.cs b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/RazorPagesTest.cs index 2783d7ee44..4958aeaa54 100644 --- a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/RazorPagesTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/RazorPagesTest.cs @@ -385,11 +385,14 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests Assert.StartsWith("Hello, You posted!", content.Trim()); } - [Fact] - public async Task HelloWorldWithPageModelHandler_CanGetContent() + [Theory] + [InlineData("GET")] + [InlineData("HEAD")] + public async Task HelloWorldWithPageModelHandler_CanGetContent(string httpMethod) { // Arrange - var request = new HttpRequestMessage(HttpMethod.Get, "http://localhost/HelloWorldWithPageModelHandler?message=pagemodel"); + var url = "http://localhost/HelloWorldWithPageModelHandler?message=pagemodel"; + var request = new HttpRequestMessage(new HttpMethod(httpMethod), url); // Act var response = await Client.SendAsync(request); diff --git a/test/Microsoft.AspNetCore.Mvc.RazorPages.Test/Internal/DefaultPageHandlerMethodSelectorTest.cs b/test/Microsoft.AspNetCore.Mvc.RazorPages.Test/Internal/DefaultPageHandlerMethodSelectorTest.cs index 421e65ac42..f4fe43fdf2 100644 --- a/test/Microsoft.AspNetCore.Mvc.RazorPages.Test/Internal/DefaultPageHandlerMethodSelectorTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.RazorPages.Test/Internal/DefaultPageHandlerMethodSelectorTest.cs @@ -3,11 +3,10 @@ using System; using System.Collections.Generic; -using System.Reflection; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc.RazorPages.Infrastructure; using Microsoft.AspNetCore.Routing; -using Microsoft.Extensions.Primitives; +using Microsoft.Extensions.Options; using Xunit; namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal @@ -15,7 +14,7 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal public class DefaultPageHandlerMethodSelectorTest { [Fact] - public void Select_ReturnsNull_WhenNoHandlerMatchesHttpMethod() + public void LegacyBehavior_Select_ReturnsNull_WhenNoHandlerMatchesHttpMethod() { // Arrange var descriptor1 = new HandlerMethodDescriptor @@ -47,7 +46,7 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal }, }, }; - var selector = new DefaultPageHandlerMethodSelector(); + var selector = CreateSelector(legacyBehavior: true); // Act var actual = selector.Select(pageContext); @@ -56,6 +55,191 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal Assert.Null(actual); } + [Fact] + public void NewBehavior_Select_ReturnsFuzzyMatchForHead_WhenNoHeadHandlerDefined() + { + // Arrange + var descriptor1 = new HandlerMethodDescriptor + { + HttpMethod = "GET" + }; + + var descriptor2 = new HandlerMethodDescriptor + { + HttpMethod = "POST" + }; + + var pageContext = new PageContext + { + ActionDescriptor = new CompiledPageActionDescriptor + { + HandlerMethods = new List() + { + descriptor1, + descriptor2, + }, + }, + RouteData = new RouteData(), + HttpContext = new DefaultHttpContext + { + Request = + { + Method = "HEAD" + }, + }, + }; + var selector = CreateSelector(); + + // Act + var actual = selector.Select(pageContext); + + // Assert + Assert.Same(descriptor1, actual); + } + + [Fact] + public void NewBehavior_Select_PrefersExactMatch() + { + // Arrange + var descriptor1 = new HandlerMethodDescriptor + { + HttpMethod = "GET" + }; + + var descriptor2 = new HandlerMethodDescriptor + { + HttpMethod = "POST" + }; + + var descriptor3 = new HandlerMethodDescriptor + { + HttpMethod = "HEAD" + }; + + var pageContext = new PageContext + { + ActionDescriptor = new CompiledPageActionDescriptor + { + HandlerMethods = new List() + { + descriptor1, + descriptor2, + descriptor3, + }, + }, + RouteData = new RouteData(), + HttpContext = new DefaultHttpContext + { + Request = + { + Method = "HEAD", + }, + }, + }; + var selector = CreateSelector(); + + // Act + var actual = selector.Select(pageContext); + + // Assert + Assert.Same(descriptor3, actual); + } + + [Fact] + public void NewBehavior_Select_PrefersExactMatch_ReturnsNullWhenHandlerNameDoesntMatch() + { + // Arrange + var descriptor1 = new HandlerMethodDescriptor + { + HttpMethod = "GET" + }; + + var descriptor2 = new HandlerMethodDescriptor + { + HttpMethod = "POST" + }; + + // This will match the HTTP method 'round' of selection, but won't match the + // handler name. + var descriptor3 = new HandlerMethodDescriptor + { + HttpMethod = "HEAD", + Name = "not-provided", + }; + + var pageContext = new PageContext + { + ActionDescriptor = new CompiledPageActionDescriptor + { + HandlerMethods = new List() + { + descriptor1, + descriptor2, + descriptor3, + }, + }, + RouteData = new RouteData(), + HttpContext = new DefaultHttpContext + { + Request = + { + Method = "HEAD", + }, + }, + }; + var selector = CreateSelector(); + + // Act + var actual = selector.Select(pageContext); + + // Assert + Assert.Null(actual); + } + + [Theory] + [InlineData("HEAD")] + [InlineData("heAd")] + public void NewBehavior_Select_ReturnsFuzzyMatch_SafeVerbs(string httpMethod) + { + // Arrange + var descriptor1 = new HandlerMethodDescriptor + { + HttpMethod = "GET" + }; + + var descriptor2 = new HandlerMethodDescriptor + { + HttpMethod = "POST" + }; + + var pageContext = new PageContext + { + ActionDescriptor = new CompiledPageActionDescriptor + { + HandlerMethods = new List() + { + descriptor1, + descriptor2, + }, + }, + RouteData = new RouteData(), + HttpContext = new DefaultHttpContext + { + Request = + { + Method = httpMethod + }, + }, + }; + var selector = CreateSelector(); + + // Act + var actual = selector.Select(pageContext); + + // Assert + Assert.Same(descriptor1, actual); + } + [Fact] public void Select_ReturnsOnlyHandler() { @@ -83,7 +267,7 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal }, }, }; - var selector = new DefaultPageHandlerMethodSelector(); + var selector = CreateSelector(); // Act var actual = selector.Select(pageContext); @@ -126,7 +310,7 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal }, }, }; - var selector = new DefaultPageHandlerMethodSelector(); + var selector = CreateSelector(); // Act var actual = selector.Select(pageContext); @@ -176,7 +360,7 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal }, }, }; - var selector = new DefaultPageHandlerMethodSelector(); + var selector = CreateSelector(); // Act var actual = selector.Select(pageContext); @@ -226,7 +410,7 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal }, }, }; - var selector = new DefaultPageHandlerMethodSelector(); + var selector = CreateSelector(); // Act var actual = selector.Select(pageContext); @@ -271,7 +455,7 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal }, }, }; - var selector = new DefaultPageHandlerMethodSelector(); + var selector = CreateSelector(); // Act var actual = selector.Select(pageContext); @@ -322,7 +506,7 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal }, }, }; - var selector = new DefaultPageHandlerMethodSelector(); + var selector = CreateSelector(); // Act var actual = selector.Select(pageContext); @@ -367,7 +551,7 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal }, }, }; - var selector = new DefaultPageHandlerMethodSelector(); + var selector = CreateSelector(); // Act var actual = selector.Select(pageContext); @@ -416,7 +600,7 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal }, }, }; - var selector = new DefaultPageHandlerMethodSelector(); + var selector = CreateSelector(); // Act var actual = selector.Select(pageContext); @@ -466,7 +650,7 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal }, }, }; - var selector = new DefaultPageHandlerMethodSelector(); + var selector = CreateSelector(); // Act & Assert var ex = Assert.Throws(() => selector.Select(pageContext)); @@ -526,7 +710,7 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal }, }, }; - var selector = new DefaultPageHandlerMethodSelector(); + var selector = CreateSelector(); // Act & Assert var ex = Assert.Throws(() => selector.Select(pageContext)); @@ -544,5 +728,14 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal protected void PostAsync() { } + + private static DefaultPageHandlerMethodSelector CreateSelector(bool legacyBehavior = false) + { + var options = Options.Create(new RazorPagesOptions() + { + AllowMappingHeadRequestsToGetHandler = !legacyBehavior + }); + return new DefaultPageHandlerMethodSelector(options); + } } } diff --git a/test/WebSites/RazorPagesWebSite/Startup.cs b/test/WebSites/RazorPagesWebSite/Startup.cs index fe3cecc4e9..98de9be426 100644 --- a/test/WebSites/RazorPagesWebSite/Startup.cs +++ b/test/WebSites/RazorPagesWebSite/Startup.cs @@ -25,7 +25,8 @@ namespace RazorPagesWebSite options.Conventions.AddPageRoute("/Pages/NotTheRoot", string.Empty); options.Conventions.Add(new CustomModelTypeConvention()); }) - .WithRazorPagesAtContentRoot(); + .WithRazorPagesAtContentRoot() + .SetCompatibilityVersion(Microsoft.AspNetCore.Mvc.CompatibilityVersion.Version_2_1); } public void Configure(IApplicationBuilder app) diff --git a/test/WebSites/RazorPagesWebSite/StartupWithBasePath.cs b/test/WebSites/RazorPagesWebSite/StartupWithBasePath.cs index 394710fb3b..669c1dba24 100644 --- a/test/WebSites/RazorPagesWebSite/StartupWithBasePath.cs +++ b/test/WebSites/RazorPagesWebSite/StartupWithBasePath.cs @@ -24,7 +24,8 @@ namespace RazorPagesWebSite options.Conventions.AuthorizeAreaFolder("Accounts", "/RequiresAuth"); options.Conventions.AllowAnonymousToAreaPage("Accounts", "/RequiresAuth/AllowAnonymous"); options.Conventions.Add(new CustomModelTypeConvention()); - }); + }) + .SetCompatibilityVersion(Microsoft.AspNetCore.Mvc.CompatibilityVersion.Version_2_1); } public void Configure(IApplicationBuilder app) diff --git a/test/WebSites/RazorPagesWebSite/TempData/SetMessageAndRedirect.cshtml b/test/WebSites/RazorPagesWebSite/TempData/SetMessageAndRedirect.cshtml index 9eeac27791..8ec5426c3b 100644 --- a/test/WebSites/RazorPagesWebSite/TempData/SetMessageAndRedirect.cshtml +++ b/test/WebSites/RazorPagesWebSite/TempData/SetMessageAndRedirect.cshtml @@ -4,7 +4,7 @@ { public IActionResult OnGet() { - TempData["TempDataProperty-Message"] = "Secret Message"; + TempData["Message"] = "Secret Message"; return Redirect("~/TempData/TempDataPageModelProperty"); } } \ No newline at end of file diff --git a/test/WebSites/RazorPagesWebSite/TempData/TempDataPageModelProperty.cshtml b/test/WebSites/RazorPagesWebSite/TempData/TempDataPageModelProperty.cshtml index 50749c4c63..24bbf83b53 100644 --- a/test/WebSites/RazorPagesWebSite/TempData/TempDataPageModelProperty.cshtml +++ b/test/WebSites/RazorPagesWebSite/TempData/TempDataPageModelProperty.cshtml @@ -6,4 +6,4 @@ Message: @Model.Message { @Html.AntiForgeryToken() } -TempData: @TempData["TempDataProperty-Message"] +TempData: @TempData["Message"]