From f0732e9e214d72db813acf6df4e7b2036ca1b8b0 Mon Sep 17 00:00:00 2001 From: jacalvar Date: Mon, 25 Jul 2016 16:49:01 -0700 Subject: [PATCH] [Fixes #5038] HTTP Verbs mapping error GET and DELETE When an action contained an attribute derived from HttpMethodAttribute, doesn't specify an attribute route and there is also another attribute extending HttpMethodAttribute that has a route defined on it; we ignored the HttpMethodAttribute attribute without a defined route when building the set of action selectors for the method. This caused the resulting action to be unbounded and to accept requests for other verbs not associated with it. The root cause of the problem was that attributes override equality and do a field by field comparison but ignore fields in the base classes of the type, so if an attribute is part of a class hierarchy (like Http*Attributes) there might be two different attributes that get considered equal. The fix for the problem has been to change using Contains on a couple of collections (that uses the equals method on the underlying object) and check for the existence of the attribute on the collection directly by using reference equality. --- .../DefaultApplicationModelProvider.cs | 38 ++++++++++-- .../DefaultApplicationModelProviderTest.cs | 40 ++++++++++++ .../RoutingTests.cs | 62 ++++++++++++++++++- .../Controllers/FriendsController.cs | 31 ++++++++++ 4 files changed, 166 insertions(+), 5 deletions(-) create mode 100644 test/WebSites/RoutingWebSite/Controllers/FriendsController.cs diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/DefaultApplicationModelProvider.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/DefaultApplicationModelProvider.cs index 9b9c345df0..ccfead7293 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/DefaultApplicationModelProvider.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/DefaultApplicationModelProvider.cs @@ -293,7 +293,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal { actionModel.RouteValues.Add(routeValueProvider.RouteKey, routeValueProvider.RouteValue); } - + //TODO: modify comment // Now we need to determine the action selection info (cross-section of routes and constraints) // @@ -468,6 +468,16 @@ namespace Microsoft.AspNetCore.Mvc.Internal // 1. [HttpPost("Api/Things")] // 2. [HttpGet], [AcceptVerbs("POST", "PUT")] // + // Another example of this situation is: + // + // [Route("api/Products")] + // [AcceptVerbs("GET", "HEAD")] + // [HttpPost("api/Products/new")] + // + // This will generate 2 selectors: + // 1. [AcceptVerbs("GET", "HEAD")] + // 2. [HttpPost] + // // Note that having a route attribute that doesn't define a route template _might_ be an error. We // don't have enough context to really know at this point so we just pass it on. var routeProviders = new List(); @@ -530,18 +540,25 @@ namespace Microsoft.AspNetCore.Mvc.Internal var filteredAttributes = new List(); foreach (var attribute in attributes) { - if (attribute == routeProvider) + if (ReferenceEquals(attribute, routeProvider)) { filteredAttributes.Add(attribute); } - else if (routeProviders.Contains(attribute)) + else if (InRouteProviders(routeProviders, attribute)) { // Exclude other route template providers + // Example: + // [HttpGet("template")] + // [Route("template/{id}")] } else if ( routeProvider is IActionHttpMethodProvider && attribute is IActionHttpMethodProvider) { + // Example: + // [HttpGet("template")] + // [AcceptVerbs("GET", "POST")] + // // Exclude other http method providers if this route is an // http method provider. } @@ -559,7 +576,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal var filteredAttributes = new List(); foreach (var attribute in attributes) { - if (!routeProviders.Contains(attribute)) + if (!InRouteProviders(routeProviders, attribute)) { filteredAttributes.Add(attribute); } @@ -572,6 +589,19 @@ namespace Microsoft.AspNetCore.Mvc.Internal return selectorModels; } + private static bool InRouteProviders(List routeProviders, object attribute) + { + foreach (var rp in routeProviders) + { + if (ReferenceEquals(rp, attribute)) + { + return true; + } + } + + return false; + } + private static SelectorModel CreateSelectorModel(IRouteTemplateProvider route, IList attributes) { var selectorModel = new SelectorModel(); diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/DefaultApplicationModelProviderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/DefaultApplicationModelProviderTest.cs index 5931328b24..bfe0be0051 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/DefaultApplicationModelProviderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/DefaultApplicationModelProviderTest.cs @@ -792,6 +792,29 @@ namespace Microsoft.AspNetCore.Mvc.Internal Assert.Equal(new string[] { "POST" }, methodConstraint.HttpMethods); } + [Fact] + public void CreateActionModel_MixedHttpVerbsAndRoutes_WithRouteOnController() + { + // Arrange + var builder = new TestApplicationModelProvider(); + var typeInfo = typeof(RouteAttributeOnController).GetTypeInfo(); + var actionName = nameof(RouteAttributeOnController.Get); + + // Act + var action = builder.CreateActionModel(typeInfo, typeInfo.AsType().GetMethod(actionName)); + + // Assert + Assert.Equal(2, action.Selectors.Count); + + var selectorModel = Assert.Single(action.Selectors, s => s.AttributeRouteModel == null); + var methodConstraint = Assert.Single(selectorModel.ActionConstraints.OfType()); + Assert.Equal(new string[] { "GET" }, methodConstraint.HttpMethods); + + selectorModel = Assert.Single(action.Selectors, s => s.AttributeRouteModel?.Template == "id/{id?}"); + methodConstraint = Assert.Single(selectorModel.ActionConstraints.OfType()); + Assert.Equal(new string[] { "GET" }, methodConstraint.HttpMethods); + } + [Fact] public void CreateActionModel_MixedHttpVerbsAndRoutes_MultipleEmptyAndNonEmptyVerbs() { @@ -1124,6 +1147,23 @@ namespace Microsoft.AspNetCore.Mvc.Internal public void Invalid() { } } + [Route("api/[controller]")] + private class RouteAttributeOnController : Controller + { + [HttpGet] + [HttpGet("id/{id?}")] + public object Get(short? id) + { + return null; + } + + [HttpDelete("{id}")] + public object Delete(int id) + { + return null; + } + } + // Here the constraints on the methods are acting as an IActionHttpMethodProvider and // not as an IRouteTemplateProvider given that there is no RouteAttribute // on the controller and the template for all the constraints on a method is null. diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/RoutingTests.cs b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/RoutingTests.cs index 055824a4b5..c3ee4d0757 100644 --- a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/RoutingTests.cs +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/RoutingTests.cs @@ -157,6 +157,66 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests result.RouteValues); } + [Theory] + [InlineData("Get", "/Friends")] + [InlineData("Get", "/Friends/Peter")] + [InlineData("Delete", "/Friends")] + public async Task AttributeRoutedAction_AcceptRequestsWithValidMethods_InRoutesWithoutExtraTemplateSegmentsOnTheAction( + string method, + string url) + { + // Arrange + var request = new HttpRequestMessage(new HttpMethod(method), $"http://localhost{url}"); + + // Assert + var response = await Client.SendAsync(request); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + + var body = await response.Content.ReadAsStringAsync(); + var result = JsonConvert.DeserializeObject(body); + + Assert.Contains(url, result.ExpectedUrls); + Assert.Equal("Friends", result.Controller); + Assert.Equal(method, result.Action); + + Assert.Contains( + new KeyValuePair("controller", "Friends"), + result.RouteValues); + + Assert.Contains( + new KeyValuePair("action", method), + result.RouteValues); + + if (result.RouteValues.ContainsKey("id")) + { + Assert.Contains( + new KeyValuePair("id", "Peter"), + result.RouteValues); + } + } + + [Theory] + [InlineData("Post", "/Friends")] + [InlineData("Put", "/Friends")] + [InlineData("Patch", "/Friends")] + [InlineData("Options", "/Friends")] + [InlineData("Head", "/Friends")] + public async Task AttributeRoutedAction_RejectsRequestsWithWrongMethods_InRoutesWithoutExtraTemplateSegmentsOnTheAction( + string method, + string url) + { + // Arrange + var request = new HttpRequestMessage(new HttpMethod(method), $"http://localhost{url}"); + + // Assert + var response = await Client.SendAsync(request); + + // Assert + Assert.Equal(HttpStatusCode.NotFound, response.StatusCode); + } + [Theory] [InlineData("http://localhost/api/v1/Maps")] [InlineData("http://localhost/api/v2/Maps")] @@ -1190,7 +1250,7 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests return Url + "?" + string.Join("&", Values.Select(kvp => kvp.Key + "=" + kvp.Value)); } - public static implicit operator string (LinkBuilder builder) + public static implicit operator string(LinkBuilder builder) { return builder.ToString(); } diff --git a/test/WebSites/RoutingWebSite/Controllers/FriendsController.cs b/test/WebSites/RoutingWebSite/Controllers/FriendsController.cs new file mode 100644 index 0000000000..b6f3779e2d --- /dev/null +++ b/test/WebSites/RoutingWebSite/Controllers/FriendsController.cs @@ -0,0 +1,31 @@ +// 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.Mvc; + +namespace RoutingWebSite +{ + [Route("Friends")] + public class FriendsController : Controller + { + private readonly TestResponseGenerator _generator; + + public FriendsController(TestResponseGenerator generator) + { + _generator = generator; + } + + [HttpGet] + [HttpGet("{id}")] + public IActionResult Get([FromRoute]string id) + { + return _generator.Generate(id == null ? "/Friends" : $"/Friends/{id}"); + } + + [HttpDelete] + public IActionResult Delete() + { + return _generator.Generate("/Friends"); + } + } +}