[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.
This commit is contained in:
jacalvar
parent
5092e75387
commit
f0732e9e21
|
|
@ -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<IRouteTemplateProvider>();
|
||||
|
|
@ -530,18 +540,25 @@ namespace Microsoft.AspNetCore.Mvc.Internal
|
|||
var filteredAttributes = new List<object>();
|
||||
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<object>();
|
||||
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<IRouteTemplateProvider> routeProviders, object attribute)
|
||||
{
|
||||
foreach (var rp in routeProviders)
|
||||
{
|
||||
if (ReferenceEquals(rp, attribute))
|
||||
{
|
||||
return true;
|
||||
}
|
||||
}
|
||||
|
||||
return false;
|
||||
}
|
||||
|
||||
private static SelectorModel CreateSelectorModel(IRouteTemplateProvider route, IList<object> attributes)
|
||||
{
|
||||
var selectorModel = new SelectorModel();
|
||||
|
|
|
|||
|
|
@ -792,6 +792,29 @@ namespace Microsoft.AspNetCore.Mvc.Internal
|
|||
Assert.Equal<string>(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<HttpMethodActionConstraint>());
|
||||
Assert.Equal(new string[] { "GET" }, methodConstraint.HttpMethods);
|
||||
|
||||
selectorModel = Assert.Single(action.Selectors, s => s.AttributeRouteModel?.Template == "id/{id?}");
|
||||
methodConstraint = Assert.Single(selectorModel.ActionConstraints.OfType<HttpMethodActionConstraint>());
|
||||
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.
|
||||
|
|
|
|||
|
|
@ -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<RoutingResult>(body);
|
||||
|
||||
Assert.Contains(url, result.ExpectedUrls);
|
||||
Assert.Equal("Friends", result.Controller);
|
||||
Assert.Equal(method, result.Action);
|
||||
|
||||
Assert.Contains(
|
||||
new KeyValuePair<string, object>("controller", "Friends"),
|
||||
result.RouteValues);
|
||||
|
||||
Assert.Contains(
|
||||
new KeyValuePair<string, object>("action", method),
|
||||
result.RouteValues);
|
||||
|
||||
if (result.RouteValues.ContainsKey("id"))
|
||||
{
|
||||
Assert.Contains(
|
||||
new KeyValuePair<string, object>("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();
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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");
|
||||
}
|
||||
}
|
||||
}
|
||||
Loading…
Reference in New Issue