From c01c7075be053d8be822aeff53fcb125c7fc485c Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Sat, 28 Jul 2018 16:12:54 +1200 Subject: [PATCH 1/2] Add EndpointMetadata to ActionDescriptor and hookup CORS (#8158) --- build/dependencies.props | 144 +++---- .../Abstractions/ActionDescriptor.cs | 2 + .../ApplicationModels/SelectorModel.cs | 4 + .../ControllerActionDescriptorBuilder.cs | 4 + .../DefaultApplicationModelProvider.cs | 3 + .../Internal/MvcEndpointDataSource.cs | 14 +- .../Internal/CorsApplicationModelProvider.cs | 15 +- ...ControllerActionDescriptorProviderTests.cs | 57 +++ .../CorsApplicationModelProviderTest.cs | 32 +- .../CorsDispatchingTests.cs | 34 ++ .../CorsTests.cs | 345 +---------------- .../CorsTestsBase.cs | 359 ++++++++++++++++++ .../VersioningGlobalRoutingTests.cs | 23 ++ .../VersioningTestsBase.cs | 4 +- test/WebSites/CorsWebSite/Program.cs | 26 ++ test/WebSites/CorsWebSite/Startup.cs | 15 - .../CorsWebSite/StartupWithGlobalRouting.cs | 80 ++++ 17 files changed, 716 insertions(+), 445 deletions(-) create mode 100644 test/Microsoft.AspNetCore.Mvc.FunctionalTests/CorsDispatchingTests.cs create mode 100644 test/Microsoft.AspNetCore.Mvc.FunctionalTests/CorsTestsBase.cs create mode 100644 test/WebSites/CorsWebSite/Program.cs create mode 100644 test/WebSites/CorsWebSite/StartupWithGlobalRouting.cs diff --git a/build/dependencies.props b/build/dependencies.props index e24177e98d..f20cec2237 100644 --- a/build/dependencies.props +++ b/build/dependencies.props @@ -16,87 +16,87 @@ 0.42.1 2.1.0 2.1.0-rc1-final - 2.2.0-preview1-34784 + 2.2.0-preview1-34816 2.2.0-preview1-17099 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 5.2.6 2.8.0 2.8.0 - 2.2.0-preview1-34784 + 2.2.0-preview1-34816 1.7.0 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 2.1.0 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 2.0.9 2.1.2 2.2.0-preview1-26618-02 - 2.2.0-preview1-34784 - 2.2.0-preview1-34784 + 2.2.0-preview1-34816 + 2.2.0-preview1-34816 15.6.1 4.7.49 2.0.3 diff --git a/src/Microsoft.AspNetCore.Mvc.Abstractions/Abstractions/ActionDescriptor.cs b/src/Microsoft.AspNetCore.Mvc.Abstractions/Abstractions/ActionDescriptor.cs index a1ced7244b..20ab53e773 100644 --- a/src/Microsoft.AspNetCore.Mvc.Abstractions/Abstractions/ActionDescriptor.cs +++ b/src/Microsoft.AspNetCore.Mvc.Abstractions/Abstractions/ActionDescriptor.cs @@ -36,6 +36,8 @@ namespace Microsoft.AspNetCore.Mvc.Abstractions /// public IList ActionConstraints { get; set; } + public IList EndpointMetadata { get; set; } + /// /// The set of parameters associated with this action. /// diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ApplicationModels/SelectorModel.cs b/src/Microsoft.AspNetCore.Mvc.Core/ApplicationModels/SelectorModel.cs index e838e17c04..d376f6c0d2 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ApplicationModels/SelectorModel.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ApplicationModels/SelectorModel.cs @@ -12,6 +12,7 @@ namespace Microsoft.AspNetCore.Mvc.ApplicationModels public SelectorModel() { ActionConstraints = new List(); + EndpointMetadata = new List(); } public SelectorModel(SelectorModel other) @@ -22,6 +23,7 @@ namespace Microsoft.AspNetCore.Mvc.ApplicationModels } ActionConstraints = new List(other.ActionConstraints); + EndpointMetadata = new List(other.EndpointMetadata); if (other.AttributeRouteModel != null) { @@ -32,5 +34,7 @@ namespace Microsoft.AspNetCore.Mvc.ApplicationModels public AttributeRouteModel AttributeRouteModel { get; set; } public IList ActionConstraints { get; } + + public IList EndpointMetadata { get; } } } diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerActionDescriptorBuilder.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerActionDescriptorBuilder.cs index f343701440..f24859eb20 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerActionDescriptorBuilder.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerActionDescriptorBuilder.cs @@ -12,6 +12,7 @@ using Microsoft.AspNetCore.Mvc.ApplicationModels; using Microsoft.AspNetCore.Mvc.Controllers; using Microsoft.AspNetCore.Mvc.Filters; using Microsoft.AspNetCore.Mvc.Routing; +using Microsoft.AspNetCore.Routing.Metadata; using Resources = Microsoft.AspNetCore.Mvc.Core.Resources; namespace Microsoft.AspNetCore.Mvc.Internal @@ -163,6 +164,9 @@ namespace Microsoft.AspNetCore.Mvc.Internal } AddActionConstraints(actionDescriptor, actionSelector, controllerConstraints); + + // REVIEW: Need to get metadata from controller + actionDescriptor.EndpointMetadata = actionSelector.EndpointMetadata.ToList(); } return actionDescriptors; diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/DefaultApplicationModelProvider.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/DefaultApplicationModelProvider.cs index fc380933c2..00ad6bf594 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/DefaultApplicationModelProvider.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/DefaultApplicationModelProvider.cs @@ -11,6 +11,7 @@ using Microsoft.AspNetCore.Mvc.ApplicationModels; using Microsoft.AspNetCore.Mvc.Filters; using Microsoft.AspNetCore.Mvc.ModelBinding; using Microsoft.AspNetCore.Mvc.Routing; +using Microsoft.AspNetCore.Routing.Metadata; using Microsoft.Extensions.Internal; using Microsoft.Extensions.Options; @@ -641,6 +642,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal } AddRange(selectorModel.ActionConstraints, attributes.OfType()); + AddRange(selectorModel.EndpointMetadata, attributes); // Simple case, all HTTP method attributes apply var httpMethods = attributes @@ -652,6 +654,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal if (httpMethods.Length > 0) { selectorModel.ActionConstraints.Add(new HttpMethodActionConstraint(httpMethods)); + selectorModel.EndpointMetadata.Add(new HttpMethodMetadata(httpMethods)); } return selectorModel; diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcEndpointDataSource.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcEndpointDataSource.cs index dda3d9f47f..649f5febfa 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcEndpointDataSource.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcEndpointDataSource.cs @@ -3,15 +3,18 @@ using System; using System.Collections.Generic; +using System.Collections.ObjectModel; using System.Linq; using System.Threading.Tasks; using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc.Abstractions; +using Microsoft.AspNetCore.Mvc.ActionConstraints; using Microsoft.AspNetCore.Mvc.Infrastructure; using Microsoft.AspNetCore.Routing; using Microsoft.AspNetCore.Routing.EndpointConstraints; using Microsoft.AspNetCore.Routing.Matchers; +using Microsoft.AspNetCore.Routing.Metadata; using Microsoft.AspNetCore.Routing.Patterns; using Microsoft.AspNetCore.Routing.Template; using Microsoft.Extensions.Primitives; @@ -316,6 +319,11 @@ namespace Microsoft.AspNetCore.Mvc.Internal metadata.Add(source); metadata.Add(action); + if (action.EndpointMetadata != null) + { + metadata.AddRange(action.EndpointMetadata); + } + if (!string.IsNullOrEmpty(routeName)) { metadata.Add(new RouteNameMetadata(routeName)); @@ -334,11 +342,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal // Currently they need to implement IActionConstraintMetadata foreach (var actionConstraint in action.ActionConstraints) { - if (actionConstraint is HttpMethodActionConstraint httpMethodActionConstraint) - { - metadata.Add(new HttpMethodEndpointConstraint(httpMethodActionConstraint.HttpMethods)); - } - else if (actionConstraint is IEndpointConstraintMetadata) + if (actionConstraint is IEndpointConstraintMetadata) { // The constraint might have been added earlier, e.g. it is also a filter descriptor if (!metadata.Contains(actionConstraint)) diff --git a/src/Microsoft.AspNetCore.Mvc.Cors/Internal/CorsApplicationModelProvider.cs b/src/Microsoft.AspNetCore.Mvc.Cors/Internal/CorsApplicationModelProvider.cs index 4410412e28..3c5d23010d 100644 --- a/src/Microsoft.AspNetCore.Mvc.Cors/Internal/CorsApplicationModelProvider.cs +++ b/src/Microsoft.AspNetCore.Mvc.Cors/Internal/CorsApplicationModelProvider.cs @@ -6,7 +6,7 @@ using System.Linq; using Microsoft.AspNetCore.Cors.Infrastructure; using Microsoft.AspNetCore.Mvc.ApplicationModels; using Microsoft.AspNetCore.Mvc.Internal; -using Microsoft.Extensions.Options; +using Microsoft.AspNetCore.Routing.Metadata; namespace Microsoft.AspNetCore.Mvc.Cors.Internal { @@ -67,17 +67,18 @@ namespace Microsoft.AspNetCore.Mvc.Cors.Internal if (isCorsEnabledGlobally || corsOnController || corsOnAction) { - UpdateHttpMethodActionConstraint(actionModel); + UpdateActionToAcceptCorsPreflight(actionModel); } } } } - private static void UpdateHttpMethodActionConstraint(ActionModel actionModel) + private static void UpdateActionToAcceptCorsPreflight(ActionModel actionModel) { for (var i = 0; i < actionModel.Selectors.Count; i++) { var selectorModel = actionModel.Selectors[i]; + for (var j = 0; j < selectorModel.ActionConstraints.Count; j++) { if (selectorModel.ActionConstraints[j] is HttpMethodActionConstraint httpConstraint) @@ -85,6 +86,14 @@ namespace Microsoft.AspNetCore.Mvc.Cors.Internal selectorModel.ActionConstraints[j] = new CorsHttpMethodActionConstraint(httpConstraint); } } + + for (int j = 0; j < selectorModel.EndpointMetadata.Count; j++) + { + if (selectorModel.EndpointMetadata[j] is HttpMethodMetadata httpMethodMetadata) + { + selectorModel.EndpointMetadata[j] = new HttpMethodMetadata(httpMethodMetadata.HttpMethods, true); + } + } } } } diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ControllerActionDescriptorProviderTests.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ControllerActionDescriptorProviderTests.cs index 56b44dc4fa..db7a1e3ce1 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ControllerActionDescriptorProviderTests.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ControllerActionDescriptorProviderTests.cs @@ -14,6 +14,7 @@ using Microsoft.AspNetCore.Mvc.Controllers; using Microsoft.AspNetCore.Mvc.Filters; using Microsoft.AspNetCore.Mvc.ModelBinding; using Microsoft.AspNetCore.Mvc.Routing; +using Microsoft.AspNetCore.Routing.Metadata; using Microsoft.Extensions.Options; using Moq; using Xunit; @@ -251,6 +252,62 @@ namespace Microsoft.AspNetCore.Mvc.Internal Assert.Equal(nameof(ConventionallyRoutedController.ConventionalAction), actionConstraint.Value); } + [Fact] + public void GetDescriptors_ActionWithHttpMethods_AddedToEndpointMetadata() + { + // Arrange & Act + var descriptors = GetDescriptors( + typeof(AttributeRoutedController).GetTypeInfo()); + + // Assert + var action = Assert.Single(descriptors); + + Assert.NotNull(action.EndpointMetadata); + + Assert.Collection(action.EndpointMetadata, + metadata => Assert.IsType(metadata), + metadata => + { + var httpMethodMetadata = Assert.IsType(metadata); + + Assert.False(httpMethodMetadata.AcceptCorsPreflight); + Assert.Equal("GET", Assert.Single(httpMethodMetadata.HttpMethods)); + }); + } + + [Fact] + public void GetDescriptors_ActionWithMultipleHttpMethods_SingleHttpMethodMetadata() + { + // Arrange & Act + var descriptors = GetDescriptors( + typeof(NonDuplicatedAttributeRouteController).GetTypeInfo()); + + // Assert + var actions = descriptors + .OfType() + .Where(d => d.ActionName == nameof(NonDuplicatedAttributeRouteController.DifferentHttpMethods)); + + Assert.Collection(actions, + InspectElement("GET"), + InspectElement("POST"), + InspectElement("PUT"), + InspectElement("PATCH"), + InspectElement("DELETE")); + + Action InspectElement(string httpMethod) + { + return (descriptor) => + { + var httpMethodAttribute = Assert.Single(descriptor.EndpointMetadata.OfType()); + Assert.Equal(httpMethod, httpMethodAttribute.HttpMethods.Single(), ignoreCase: true); + + var httpMethodMetadata = Assert.Single(descriptor.EndpointMetadata.OfType()); + Assert.Equal(httpMethod, httpMethodMetadata.HttpMethods.Single(), ignoreCase: true); + Assert.False(httpMethodMetadata.AcceptCorsPreflight); + }; + } + } + [Fact] public void GetDescriptors_AddsControllerAndActionDefaults_ToAttributeRoutedActions() { diff --git a/test/Microsoft.AspNetCore.Mvc.Cors.Test/Internal/CorsApplicationModelProviderTest.cs b/test/Microsoft.AspNetCore.Mvc.Cors.Test/Internal/CorsApplicationModelProviderTest.cs index 3b4c88964f..a9538a9b24 100644 --- a/test/Microsoft.AspNetCore.Mvc.Cors.Test/Internal/CorsApplicationModelProviderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Cors.Test/Internal/CorsApplicationModelProviderTest.cs @@ -2,6 +2,7 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; +using System.Linq; using System.Reflection; using System.Threading.Tasks; using Microsoft.AspNetCore.Cors; @@ -10,6 +11,7 @@ using Microsoft.AspNetCore.Mvc.ApplicationModels; using Microsoft.AspNetCore.Mvc.Filters; using Microsoft.AspNetCore.Mvc.Internal; using Microsoft.AspNetCore.Mvc.ModelBinding; +using Microsoft.AspNetCore.Routing.Metadata; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; using Moq; @@ -36,6 +38,8 @@ namespace Microsoft.AspNetCore.Mvc.Cors.Internal var selector = Assert.Single(action.Selectors); var constraint = Assert.Single(selector.ActionConstraints, c => c is HttpMethodActionConstraint); Assert.IsType(constraint); + var httpMethodMetadata = Assert.Single(selector.EndpointMetadata.OfType()); + Assert.True(httpMethodMetadata.AcceptCorsPreflight); } [Fact] @@ -55,10 +59,12 @@ namespace Microsoft.AspNetCore.Mvc.Cors.Internal var selector = Assert.Single(action.Selectors); var constraint = Assert.Single(selector.ActionConstraints, c => c is HttpMethodActionConstraint); Assert.IsType(constraint); + var httpMethodMetadata = Assert.Single(selector.EndpointMetadata.OfType()); + Assert.True(httpMethodMetadata.AcceptCorsPreflight); } [Fact] - public void CreateControllerModel_CustomCorsFilter_ReplacesHttpConstraints() + public void CreateControllerModel_CustomCorsFilter_EnablesCorsPreflight() { // Arrange var corsProvider = new CorsApplicationModelProvider(); @@ -73,6 +79,8 @@ namespace Microsoft.AspNetCore.Mvc.Cors.Internal var selector = Assert.Single(action.Selectors); var constraint = Assert.Single(selector.ActionConstraints, c => c is HttpMethodActionConstraint); Assert.IsType(constraint); + var httpMethodMetadata = Assert.Single(selector.EndpointMetadata.OfType()); + Assert.True(httpMethodMetadata.AcceptCorsPreflight); } [Fact] @@ -92,6 +100,8 @@ namespace Microsoft.AspNetCore.Mvc.Cors.Internal var selector = Assert.Single(action.Selectors); var constraint = Assert.Single(selector.ActionConstraints, c => c is HttpMethodActionConstraint); Assert.IsType(constraint); + var httpMethodMetadata = Assert.Single(selector.EndpointMetadata.OfType()); + Assert.True(httpMethodMetadata.AcceptCorsPreflight); } [Fact] @@ -111,10 +121,12 @@ namespace Microsoft.AspNetCore.Mvc.Cors.Internal var selector = Assert.Single(action.Selectors); var constraint = Assert.Single(selector.ActionConstraints, c => c is HttpMethodActionConstraint); Assert.IsType(constraint); + var httpMethodMetadata = Assert.Single(selector.EndpointMetadata.OfType()); + Assert.True(httpMethodMetadata.AcceptCorsPreflight); } [Fact] - public void BuildActionModel_CustomCorsAuthorizationFilterOnAction_ReplacesHttpConstraints() + public void BuildActionModel_CustomCorsAuthorizationFilterOnAction_EnablesCorsPreflight() { // Arrange var corsProvider = new CorsApplicationModelProvider(); @@ -129,10 +141,12 @@ namespace Microsoft.AspNetCore.Mvc.Cors.Internal var selector = Assert.Single(action.Selectors); var constraint = Assert.Single(selector.ActionConstraints, c => c is HttpMethodActionConstraint); Assert.IsType(constraint); + var httpMethodMetadata = Assert.Single(selector.EndpointMetadata.OfType()); + Assert.True(httpMethodMetadata.AcceptCorsPreflight); } [Fact] - public void CreateControllerModel_EnableCorsGloballyReplacesHttpMethodConstraints() + public void CreateControllerModel_EnableCorsGloballyEnablesCorsPreflight() { // Arrange var corsProvider = new CorsApplicationModelProvider(); @@ -150,10 +164,12 @@ namespace Microsoft.AspNetCore.Mvc.Cors.Internal var selector = Assert.Single(action.Selectors); var constraint = Assert.Single(selector.ActionConstraints, c => c is HttpMethodActionConstraint); Assert.IsType(constraint); + var httpMethodMetadata = Assert.Single(selector.EndpointMetadata.OfType()); + Assert.True(httpMethodMetadata.AcceptCorsPreflight); } [Fact] - public void CreateControllerModel_DisableCorsGloballyReplacesHttpMethodConstraints() + public void CreateControllerModel_DisableCorsGloballyEnablesCorsPreflight() { // Arrange var corsProvider = new CorsApplicationModelProvider(); @@ -169,10 +185,12 @@ namespace Microsoft.AspNetCore.Mvc.Cors.Internal var selector = Assert.Single(action.Selectors); var constraint = Assert.Single(selector.ActionConstraints, c => c is HttpMethodActionConstraint); Assert.IsType(constraint); + var httpMethodMetadata = Assert.Single(selector.EndpointMetadata.OfType()); + Assert.True(httpMethodMetadata.AcceptCorsPreflight); } [Fact] - public void CreateControllerModel_CustomCorsFilterGloballyReplacesHttpMethodConstraints() + public void CreateControllerModel_CustomCorsFilterGloballyEnablesCorsPreflight() { // Arrange var corsProvider = new CorsApplicationModelProvider(); @@ -188,6 +206,8 @@ namespace Microsoft.AspNetCore.Mvc.Cors.Internal var selector = Assert.Single(action.Selectors); var constraint = Assert.Single(selector.ActionConstraints, c => c is HttpMethodActionConstraint); Assert.IsType(constraint); + var httpMethodMetadata = Assert.Single(selector.EndpointMetadata.OfType()); + Assert.True(httpMethodMetadata.AcceptCorsPreflight); } [Fact] @@ -206,6 +226,8 @@ namespace Microsoft.AspNetCore.Mvc.Cors.Internal var selector = Assert.Single(action.Selectors); var constraint = Assert.Single(selector.ActionConstraints, c => c is HttpMethodActionConstraint); Assert.IsNotType(constraint); + var httpMethodMetadata = Assert.Single(selector.EndpointMetadata.OfType()); + Assert.False(httpMethodMetadata.AcceptCorsPreflight); } private static ApplicationModelProviderContext GetProviderContext(Type controllerType) diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/CorsDispatchingTests.cs b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/CorsDispatchingTests.cs new file mode 100644 index 0000000000..f5281eee9e --- /dev/null +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/CorsDispatchingTests.cs @@ -0,0 +1,34 @@ +// 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.Net; +using System.Net.Http; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Cors.Infrastructure; +using Xunit; + +namespace Microsoft.AspNetCore.Mvc.FunctionalTests +{ + public class CorsGlobalRoutingTests : CorsTestsBase + { + public CorsGlobalRoutingTests(MvcTestFixture fixture) + : base(fixture) + { + } + + [Fact] // This intentionally returns a 405 with global routing + public override async Task PreflightRequestOnNonCorsEnabledController_DoesNotMatchTheAction() + { + // Arrange + var request = new HttpRequestMessage(new HttpMethod("OPTIONS"), "http://localhost/NonCors/Post"); + request.Headers.Add(CorsConstants.Origin, "http://example.com"); + request.Headers.Add(CorsConstants.AccessControlRequestMethod, "POST"); + + // Act + var response = await Client.SendAsync(request); + + // Assert + Assert.Equal(HttpStatusCode.MethodNotAllowed, response.StatusCode); + } + } +} \ No newline at end of file diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/CorsTests.cs b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/CorsTests.cs index 5d92c7e6eb..4d241ddeb0 100644 --- a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/CorsTests.cs +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/CorsTests.cs @@ -1,354 +1,13 @@ // 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.Linq; -using System.Net; -using System.Net.Http; -using System.Threading.Tasks; -using Microsoft.AspNetCore.Cors.Infrastructure; -using Xunit; - namespace Microsoft.AspNetCore.Mvc.FunctionalTests { - public class CorsTests : IClassFixture> + public class CorsTests : CorsTestsBase { public CorsTests(MvcTestFixture fixture) + : base(fixture) { - Client = fixture.CreateDefaultClient(); - } - - public HttpClient Client { get; } - - [Theory] - [InlineData("GET")] - [InlineData("HEAD")] - [InlineData("POST")] - public async Task ResourceWithSimpleRequestPolicy_Allows_SimpleRequests(string method) - { - // Arrange - var origin = "http://example.com"; - var request = new HttpRequestMessage(new HttpMethod(method), "http://localhost/Cors/GetBlogComments"); - request.Headers.Add(CorsConstants.Origin, origin); - - // Act - var response = await Client.SendAsync(request); - - // Assert - Assert.Equal(HttpStatusCode.OK, response.StatusCode); - var content = await response.Content.ReadAsStringAsync(); - Assert.Equal("[\"comment1\",\"comment2\",\"comment3\"]", content); - var responseHeaders = response.Headers; - var header = Assert.Single(response.Headers); - Assert.Equal(CorsConstants.AccessControlAllowOrigin, header.Key); - Assert.Equal(new[] { "*" }, header.Value.ToArray()); - } - - [Fact] - public async Task OptionsRequest_NonPreflight_ExecutesOptionsAction() - { - // Arrange - var request = new HttpRequestMessage(new HttpMethod("OPTIONS"), "http://localhost/NonCors/GetOptions"); - - // Act - var response = await Client.SendAsync(request); - - // Assert - Assert.Equal(HttpStatusCode.OK, response.StatusCode); - var content = await response.Content.ReadAsStringAsync(); - Assert.Equal("[\"Create\",\"Update\",\"Delete\"]", content); - Assert.Empty(response.Headers); - } - - [Fact] - public async Task PreflightRequestOnNonCorsEnabledController_ExecutesOptionsAction() - { - // Arrange - var request = new HttpRequestMessage(new HttpMethod("OPTIONS"), "http://localhost/NonCors/GetOptions"); - request.Headers.Add(CorsConstants.Origin, "http://example.com"); - request.Headers.Add(CorsConstants.AccessControlRequestMethod, "POST"); - - // Act - var response = await Client.SendAsync(request); - - // Assert - Assert.Equal(HttpStatusCode.OK, response.StatusCode); - var content = await response.Content.ReadAsStringAsync(); - Assert.Equal("[\"Create\",\"Update\",\"Delete\"]", content); - Assert.Empty(response.Headers); - } - - [Fact] - public async Task PreflightRequestOnNonCorsEnabledController_DoesNotMatchTheAction() - { - // Arrange - var request = new HttpRequestMessage(new HttpMethod("OPTIONS"), "http://localhost/NonCors/Post"); - request.Headers.Add(CorsConstants.Origin, "http://example.com"); - request.Headers.Add(CorsConstants.AccessControlRequestMethod, "POST"); - - // Act - var response = await Client.SendAsync(request); - - // Assert - Assert.Equal(HttpStatusCode.NotFound, response.StatusCode); - } - - [Theory] - [InlineData("GET")] - [InlineData("HEAD")] - [InlineData("POST")] - [InlineData("PUT")] - public async Task PolicyFailed_Disallows_PreFlightRequest(string method) - { - // Arrange - var request = new HttpRequestMessage( - new HttpMethod(CorsConstants.PreflightHttpMethod), - "http://localhost/Cors/GetBlogComments"); - - // Adding a custom header makes it a non-simple request. - request.Headers.Add(CorsConstants.Origin, "http://example.com"); - request.Headers.Add(CorsConstants.AccessControlRequestMethod, method); - request.Headers.Add(CorsConstants.AccessControlRequestHeaders, "Custom"); - - // Act - var response = await Client.SendAsync(request); - - // Assert - // MVC applied the policy and since that did not pass, there were no access control headers. - Assert.Equal(HttpStatusCode.OK, response.StatusCode); - Assert.Empty(response.Headers); - - // It should short circuit and hence no result. - var content = await response.Content.ReadAsStringAsync(); - Assert.Equal(string.Empty, content); - } - - [Fact] - public async Task SuccessfulCorsRequest_AllowsCredentials_IfThePolicyAllowsCredentials() - { - // Arrange - var request = new HttpRequestMessage( - HttpMethod.Put, - "http://localhost/Cors/EditUserComment?userComment=abcd"); - - // Adding a custom header makes it a non-simple request. - request.Headers.Add(CorsConstants.Origin, "http://example.com"); - request.Headers.Add(CorsConstants.AccessControlExposeHeaders, "exposed1,exposed2"); - - // Act - var response = await Client.SendAsync(request); - - // Assert - Assert.Equal(HttpStatusCode.OK, response.StatusCode); - var responseHeaders = response.Headers; - Assert.Equal( - new[] { "http://example.com" }, - responseHeaders.GetValues(CorsConstants.AccessControlAllowOrigin).ToArray()); - Assert.Equal( - new[] { "true" }, - responseHeaders.GetValues(CorsConstants.AccessControlAllowCredentials).ToArray()); - Assert.Equal( - new[] { "exposed1,exposed2" }, - responseHeaders.GetValues(CorsConstants.AccessControlExposeHeaders).ToArray()); - - var content = await response.Content.ReadAsStringAsync(); - Assert.Equal("abcd", content); - } - - [Fact] - public async Task SuccessfulPreflightRequest_AllowsCredentials_IfThePolicyAllowsCredentials() - { - // Arrange - var request = new HttpRequestMessage( - new HttpMethod(CorsConstants.PreflightHttpMethod), - "http://localhost/Cors/EditUserComment?userComment=abcd"); - - // Adding a custom header makes it a non-simple request. - request.Headers.Add(CorsConstants.Origin, "http://example.com"); - request.Headers.Add(CorsConstants.AccessControlRequestMethod, "PUT"); - request.Headers.Add(CorsConstants.AccessControlRequestHeaders, "header1,header2"); - - // Act - var response = await Client.SendAsync(request); - - // Assert - Assert.Equal(HttpStatusCode.OK, response.StatusCode); - var responseHeaders = response.Headers; - Assert.Equal( - new[] { "http://example.com" }, - responseHeaders.GetValues(CorsConstants.AccessControlAllowOrigin).ToArray()); - Assert.Equal( - new[] { "true" }, - responseHeaders.GetValues(CorsConstants.AccessControlAllowCredentials).ToArray()); - Assert.Equal( - new[] { "header1,header2" }, - responseHeaders.GetValues(CorsConstants.AccessControlAllowHeaders).ToArray()); - Assert.Equal( - new[] { "PUT" }, - responseHeaders.GetValues(CorsConstants.AccessControlAllowMethods).ToArray()); - - var content = await response.Content.ReadAsStringAsync(); - Assert.Empty(content); - } - - [Fact] - public async Task PolicyFailed_Allows_ActualRequest_WithMissingResponseHeaders() - { - // Arrange - var request = new HttpRequestMessage(HttpMethod.Put, "http://localhost/Cors/GetUserComments"); - - // Adding a custom header makes it a non simple request. - request.Headers.Add(CorsConstants.Origin, "http://example2.com"); - - // Act - var response = await Client.SendAsync(request); - - // Assert - // MVC applied the policy and since that did not pass, there were no access control headers. - Assert.Equal(HttpStatusCode.OK, response.StatusCode); - Assert.Empty(response.Headers); - - // It still have executed the action. - var content = await response.Content.ReadAsStringAsync(); - Assert.Equal("[\"usercomment1\",\"usercomment2\",\"usercomment3\"]", content); - } - - [Theory] - [InlineData("GET")] - [InlineData("HEAD")] - [InlineData("POST")] - public async Task DisableCors_ActionsCanOverride_ControllerLevel(string method) - { - // Arrange - var request = new HttpRequestMessage(new HttpMethod(method), "http://localhost/Cors/GetExclusiveContent"); - - // Exclusive content is not available on other sites. - request.Headers.Add(CorsConstants.Origin, "http://example.com"); - - // Act - var response = await Client.SendAsync(request); - - // Assert - Assert.Equal(HttpStatusCode.OK, response.StatusCode); - - // Since there are no response headers, the client should step in to block the content. - Assert.Empty(response.Headers); - var content = await response.Content.ReadAsStringAsync(); - Assert.Equal("exclusive", content); - } - - [Theory] - [InlineData("GET")] - [InlineData("HEAD")] - [InlineData("POST")] - public async Task DisableCors_PreFlight_ActionsCanOverride_ControllerLevel(string method) - { - // Arrange - var request = new HttpRequestMessage( - new HttpMethod(CorsConstants.PreflightHttpMethod), - "http://localhost/Cors/GetExclusiveContent"); - - // Exclusive content is not available on other sites. - request.Headers.Add(CorsConstants.Origin, "http://example.com"); - request.Headers.Add(CorsConstants.AccessControlRequestMethod, method); - request.Headers.Add(CorsConstants.AccessControlRequestHeaders, "Custom"); - - // Act - var response = await Client.SendAsync(request); - - // Assert - // Since there are no response headers, the client should step in to block the content. - Assert.Equal(HttpStatusCode.OK, response.StatusCode); - Assert.Empty(response.Headers); - - // Nothing gets executed for a pre-flight request. - var content = await response.Content.ReadAsStringAsync(); - Assert.Empty(content); - } - - [Theory] - [InlineData("http://localhost/api/store/actionusingcontrollercorssettings")] - [InlineData("http://localhost/api/store/actionwithcorssettings")] - public async Task CorsFilter_RunsBeforeOtherAuthorizationFilters(string url) - { - // Arrange - var request = new HttpRequestMessage(new HttpMethod(CorsConstants.PreflightHttpMethod), url); - - // Adding a custom header makes it a non-simple request. - request.Headers.Add(CorsConstants.Origin, "http://example.com"); - request.Headers.Add(CorsConstants.AccessControlRequestMethod, "GET"); - request.Headers.Add(CorsConstants.AccessControlRequestHeaders, "Custom"); - - // Act - var response = await Client.SendAsync(request); - - // Assert - Assert.Equal(HttpStatusCode.OK, response.StatusCode); - var responseHeaders = response.Headers; - Assert.Equal( - new[] { "http://example.com" }, - responseHeaders.GetValues(CorsConstants.AccessControlAllowOrigin).ToArray()); - Assert.Equal( - new[] { "true" }, - responseHeaders.GetValues(CorsConstants.AccessControlAllowCredentials).ToArray()); - Assert.Equal( - new[] { "Custom" }, - responseHeaders.GetValues(CorsConstants.AccessControlAllowHeaders).ToArray()); - - var content = await response.Content.ReadAsStringAsync(); - Assert.Empty(content); - } - - [Fact] - public async Task DisableCorsFilter_RunsBeforeOtherAuthorizationFilters() - { - // Controller has an authorization filter and Cors filter and the action has a DisableCors filter - // In this scenario, the CorsFilter should be executed before any other authorization filters - // i.e irrespective of where the Cors filter is applied(controller or action), Cors filters must - // always be executed before any other type of authorization filters. - - // Arrange - var request = new HttpRequestMessage( - new HttpMethod(CorsConstants.PreflightHttpMethod), - "http://localhost/api/store/actionwithcorsdisabled"); - - // Adding a custom header makes it a non-simple request. - request.Headers.Add(CorsConstants.Origin, "http://example.com"); - request.Headers.Add(CorsConstants.AccessControlRequestMethod, "GET"); - request.Headers.Add(CorsConstants.AccessControlRequestHeaders, "Custom"); - - // Act - var response = await Client.SendAsync(request); - - // Assert - Assert.Equal(HttpStatusCode.OK, response.StatusCode); - Assert.Empty(response.Headers); - - // Nothing gets executed for a pre-flight request. - var content = await response.Content.ReadAsStringAsync(); - Assert.Empty(content); - } - - [Fact] - public async Task CorsFilter_OnAction_PreferredOverController_AndAuthorizationFiltersRunAfterCors() - { - // Arrange - var request = new HttpRequestMessage( - new HttpMethod(CorsConstants.PreflightHttpMethod), - "http://localhost/api/store/actionwithdifferentcorspolicy"); - request.Headers.Add(CorsConstants.Origin, "http://notexpecteddomain.com"); - request.Headers.Add(CorsConstants.AccessControlRequestMethod, "GET"); - request.Headers.Add(CorsConstants.AccessControlRequestHeaders, "Custom"); - - // Act - var response = await Client.SendAsync(request); - - // Assert - Assert.Equal(HttpStatusCode.OK, response.StatusCode); - Assert.Empty(response.Headers); - - // Nothing gets executed for a pre-flight request. - var content = await response.Content.ReadAsStringAsync(); - Assert.Empty(content); } } } \ No newline at end of file diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/CorsTestsBase.cs b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/CorsTestsBase.cs new file mode 100644 index 0000000000..899ce4e792 --- /dev/null +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/CorsTestsBase.cs @@ -0,0 +1,359 @@ +// 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.Linq; +using System.Net; +using System.Net.Http; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Cors.Infrastructure; +using Microsoft.AspNetCore.Hosting; +using Xunit; + +namespace Microsoft.AspNetCore.Mvc.FunctionalTests +{ + public abstract class CorsTestsBase : IClassFixture> where TStartup : class + { + protected CorsTestsBase(MvcTestFixture fixture) + { + var factory = fixture.Factories.FirstOrDefault() ?? fixture.WithWebHostBuilder(ConfigureWebHostBuilder); + Client = factory.CreateDefaultClient(); + } + + private static void ConfigureWebHostBuilder(IWebHostBuilder builder) => + builder.UseStartup(); + + public HttpClient Client { get; } + + [Theory] + [InlineData("GET")] + [InlineData("HEAD")] + [InlineData("POST")] + public async Task ResourceWithSimpleRequestPolicy_Allows_SimpleRequests(string method) + { + // Arrange + var origin = "http://example.com"; + var request = new HttpRequestMessage(new HttpMethod(method), "http://localhost/Cors/GetBlogComments"); + request.Headers.Add(CorsConstants.Origin, origin); + + // Act + var response = await Client.SendAsync(request); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + var content = await response.Content.ReadAsStringAsync(); + Assert.Equal("[\"comment1\",\"comment2\",\"comment3\"]", content); + var responseHeaders = response.Headers; + var header = Assert.Single(response.Headers); + Assert.Equal(CorsConstants.AccessControlAllowOrigin, header.Key); + Assert.Equal(new[] { "*" }, header.Value.ToArray()); + } + + [Fact] + public async Task OptionsRequest_NonPreflight_ExecutesOptionsAction() + { + // Arrange + var request = new HttpRequestMessage(new HttpMethod("OPTIONS"), "http://localhost/NonCors/GetOptions"); + + // Act + var response = await Client.SendAsync(request); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + var content = await response.Content.ReadAsStringAsync(); + Assert.Equal("[\"Create\",\"Update\",\"Delete\"]", content); + Assert.Empty(response.Headers); + } + + [Fact] + public async Task PreflightRequestOnNonCorsEnabledController_ExecutesOptionsAction() + { + // Arrange + var request = new HttpRequestMessage(new HttpMethod("OPTIONS"), "http://localhost/NonCors/GetOptions"); + request.Headers.Add(CorsConstants.Origin, "http://example.com"); + request.Headers.Add(CorsConstants.AccessControlRequestMethod, "POST"); + + // Act + var response = await Client.SendAsync(request); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + var content = await response.Content.ReadAsStringAsync(); + Assert.Equal("[\"Create\",\"Update\",\"Delete\"]", content); + Assert.Empty(response.Headers); + } + + [Fact] + public virtual async Task PreflightRequestOnNonCorsEnabledController_DoesNotMatchTheAction() + { + // Arrange + var request = new HttpRequestMessage(new HttpMethod("OPTIONS"), "http://localhost/NonCors/Post"); + request.Headers.Add(CorsConstants.Origin, "http://example.com"); + request.Headers.Add(CorsConstants.AccessControlRequestMethod, "POST"); + + // Act + var response = await Client.SendAsync(request); + + // Assert + Assert.Equal(HttpStatusCode.NotFound, response.StatusCode); + } + + [Theory] + [InlineData("GET")] + [InlineData("HEAD")] + [InlineData("POST")] + [InlineData("PUT")] + public async Task PolicyFailed_Disallows_PreFlightRequest(string method) + { + // Arrange + var request = new HttpRequestMessage( + new HttpMethod(CorsConstants.PreflightHttpMethod), + "http://localhost/Cors/GetBlogComments"); + + // Adding a custom header makes it a non-simple request. + request.Headers.Add(CorsConstants.Origin, "http://example.com"); + request.Headers.Add(CorsConstants.AccessControlRequestMethod, method); + request.Headers.Add(CorsConstants.AccessControlRequestHeaders, "Custom"); + + // Act + var response = await Client.SendAsync(request); + + // Assert + // MVC applied the policy and since that did not pass, there were no access control headers. + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.Empty(response.Headers); + + // It should short circuit and hence no result. + var content = await response.Content.ReadAsStringAsync(); + Assert.Equal(string.Empty, content); + } + + [Fact] + public async Task SuccessfulCorsRequest_AllowsCredentials_IfThePolicyAllowsCredentials() + { + // Arrange + var request = new HttpRequestMessage( + HttpMethod.Put, + "http://localhost/Cors/EditUserComment?userComment=abcd"); + + // Adding a custom header makes it a non-simple request. + request.Headers.Add(CorsConstants.Origin, "http://example.com"); + request.Headers.Add(CorsConstants.AccessControlExposeHeaders, "exposed1,exposed2"); + + // Act + var response = await Client.SendAsync(request); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + var responseHeaders = response.Headers; + Assert.Equal( + new[] { "http://example.com" }, + responseHeaders.GetValues(CorsConstants.AccessControlAllowOrigin).ToArray()); + Assert.Equal( + new[] { "true" }, + responseHeaders.GetValues(CorsConstants.AccessControlAllowCredentials).ToArray()); + Assert.Equal( + new[] { "exposed1,exposed2" }, + responseHeaders.GetValues(CorsConstants.AccessControlExposeHeaders).ToArray()); + + var content = await response.Content.ReadAsStringAsync(); + Assert.Equal("abcd", content); + } + + [Fact] + public async Task SuccessfulPreflightRequest_AllowsCredentials_IfThePolicyAllowsCredentials() + { + // Arrange + var request = new HttpRequestMessage( + new HttpMethod(CorsConstants.PreflightHttpMethod), + "http://localhost/Cors/EditUserComment?userComment=abcd"); + + // Adding a custom header makes it a non-simple request. + request.Headers.Add(CorsConstants.Origin, "http://example.com"); + request.Headers.Add(CorsConstants.AccessControlRequestMethod, "PUT"); + request.Headers.Add(CorsConstants.AccessControlRequestHeaders, "header1,header2"); + + // Act + var response = await Client.SendAsync(request); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + var responseHeaders = response.Headers; + Assert.Equal( + new[] { "http://example.com" }, + responseHeaders.GetValues(CorsConstants.AccessControlAllowOrigin).ToArray()); + Assert.Equal( + new[] { "true" }, + responseHeaders.GetValues(CorsConstants.AccessControlAllowCredentials).ToArray()); + Assert.Equal( + new[] { "header1,header2" }, + responseHeaders.GetValues(CorsConstants.AccessControlAllowHeaders).ToArray()); + Assert.Equal( + new[] { "PUT" }, + responseHeaders.GetValues(CorsConstants.AccessControlAllowMethods).ToArray()); + + var content = await response.Content.ReadAsStringAsync(); + Assert.Empty(content); + } + + [Fact] + public async Task PolicyFailed_Allows_ActualRequest_WithMissingResponseHeaders() + { + // Arrange + var request = new HttpRequestMessage(HttpMethod.Put, "http://localhost/Cors/GetUserComments"); + + // Adding a custom header makes it a non simple request. + request.Headers.Add(CorsConstants.Origin, "http://example2.com"); + + // Act + var response = await Client.SendAsync(request); + + // Assert + // MVC applied the policy and since that did not pass, there were no access control headers. + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.Empty(response.Headers); + + // It still have executed the action. + var content = await response.Content.ReadAsStringAsync(); + Assert.Equal("[\"usercomment1\",\"usercomment2\",\"usercomment3\"]", content); + } + + [Theory] + [InlineData("GET")] + [InlineData("HEAD")] + [InlineData("POST")] + public async Task DisableCors_ActionsCanOverride_ControllerLevel(string method) + { + // Arrange + var request = new HttpRequestMessage(new HttpMethod(method), "http://localhost/Cors/GetExclusiveContent"); + + // Exclusive content is not available on other sites. + request.Headers.Add(CorsConstants.Origin, "http://example.com"); + + // Act + var response = await Client.SendAsync(request); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + + // Since there are no response headers, the client should step in to block the content. + Assert.Empty(response.Headers); + var content = await response.Content.ReadAsStringAsync(); + Assert.Equal("exclusive", content); + } + + [Theory] + [InlineData("GET")] + [InlineData("HEAD")] + [InlineData("POST")] + public async Task DisableCors_PreFlight_ActionsCanOverride_ControllerLevel(string method) + { + // Arrange + var request = new HttpRequestMessage( + new HttpMethod(CorsConstants.PreflightHttpMethod), + "http://localhost/Cors/GetExclusiveContent"); + + // Exclusive content is not available on other sites. + request.Headers.Add(CorsConstants.Origin, "http://example.com"); + request.Headers.Add(CorsConstants.AccessControlRequestMethod, method); + request.Headers.Add(CorsConstants.AccessControlRequestHeaders, "Custom"); + + // Act + var response = await Client.SendAsync(request); + + // Assert + // Since there are no response headers, the client should step in to block the content. + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.Empty(response.Headers); + + // Nothing gets executed for a pre-flight request. + var content = await response.Content.ReadAsStringAsync(); + Assert.Empty(content); + } + + [Theory] + [InlineData("http://localhost/api/store/actionusingcontrollercorssettings")] + [InlineData("http://localhost/api/store/actionwithcorssettings")] + public async Task CorsFilter_RunsBeforeOtherAuthorizationFilters(string url) + { + // Arrange + var request = new HttpRequestMessage(new HttpMethod(CorsConstants.PreflightHttpMethod), url); + + // Adding a custom header makes it a non-simple request. + request.Headers.Add(CorsConstants.Origin, "http://example.com"); + request.Headers.Add(CorsConstants.AccessControlRequestMethod, "GET"); + request.Headers.Add(CorsConstants.AccessControlRequestHeaders, "Custom"); + + // Act + var response = await Client.SendAsync(request); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + var responseHeaders = response.Headers; + Assert.Equal( + new[] { "http://example.com" }, + responseHeaders.GetValues(CorsConstants.AccessControlAllowOrigin).ToArray()); + Assert.Equal( + new[] { "true" }, + responseHeaders.GetValues(CorsConstants.AccessControlAllowCredentials).ToArray()); + Assert.Equal( + new[] { "Custom" }, + responseHeaders.GetValues(CorsConstants.AccessControlAllowHeaders).ToArray()); + + var content = await response.Content.ReadAsStringAsync(); + Assert.Empty(content); + } + + [Fact] + public async Task DisableCorsFilter_RunsBeforeOtherAuthorizationFilters() + { + // Controller has an authorization filter and Cors filter and the action has a DisableCors filter + // In this scenario, the CorsFilter should be executed before any other authorization filters + // i.e irrespective of where the Cors filter is applied(controller or action), Cors filters must + // always be executed before any other type of authorization filters. + + // Arrange + var request = new HttpRequestMessage( + new HttpMethod(CorsConstants.PreflightHttpMethod), + "http://localhost/api/store/actionwithcorsdisabled"); + + // Adding a custom header makes it a non-simple request. + request.Headers.Add(CorsConstants.Origin, "http://example.com"); + request.Headers.Add(CorsConstants.AccessControlRequestMethod, "GET"); + request.Headers.Add(CorsConstants.AccessControlRequestHeaders, "Custom"); + + // Act + var response = await Client.SendAsync(request); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.Empty(response.Headers); + + // Nothing gets executed for a pre-flight request. + var content = await response.Content.ReadAsStringAsync(); + Assert.Empty(content); + } + + [Fact] + public async Task CorsFilter_OnAction_PreferredOverController_AndAuthorizationFiltersRunAfterCors() + { + // Arrange + var request = new HttpRequestMessage( + new HttpMethod(CorsConstants.PreflightHttpMethod), + "http://localhost/api/store/actionwithdifferentcorspolicy"); + request.Headers.Add(CorsConstants.Origin, "http://notexpecteddomain.com"); + request.Headers.Add(CorsConstants.AccessControlRequestMethod, "GET"); + request.Headers.Add(CorsConstants.AccessControlRequestHeaders, "Custom"); + + // Act + var response = await Client.SendAsync(request); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.Empty(response.Headers); + + // Nothing gets executed for a pre-flight request. + var content = await response.Content.ReadAsStringAsync(); + Assert.Empty(content); + } + } +} \ No newline at end of file diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/VersioningGlobalRoutingTests.cs b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/VersioningGlobalRoutingTests.cs index af2147949f..a79ebc9516 100644 --- a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/VersioningGlobalRoutingTests.cs +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/VersioningGlobalRoutingTests.cs @@ -2,6 +2,7 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System.Net; +using System.Net.Http; using System.Threading.Tasks; using Newtonsoft.Json; using Xunit; @@ -29,5 +30,27 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests Assert.True(result); } + + // This behaves differently right now because the action/endpoint constraints are always + // executed after the DFA nodes like (HttpMethodMatcherPolicy). You don't have the flexibility + // to do what this test is doing in old-style routing. + [Fact] + public override async Task VersionedApi_CanUseConstraintOrder_ToChangeSelectedAction() + { + // Arrange + var message = new HttpRequestMessage(HttpMethod.Delete, "http://localhost/" + "Customers/5?version=2"); + + // Act + var response = await Client.SendAsync(message); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + + var body = await response.Content.ReadAsStringAsync(); + var result = JsonConvert.DeserializeObject(body); + + Assert.Equal("Customers", result.Controller); + Assert.Equal("AnyV2OrHigherWithId", result.Action); + } } } \ No newline at end of file diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/VersioningTestsBase.cs b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/VersioningTestsBase.cs index 152c08a799..01e6ef154b 100644 --- a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/VersioningTestsBase.cs +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/VersioningTestsBase.cs @@ -508,7 +508,7 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests } [Fact] - public async Task VersionedApi_CanUseConstraintOrder_ToChangeSelectedAction() + public virtual async Task VersionedApi_CanUseConstraintOrder_ToChangeSelectedAction() { // Arrange var message = new HttpRequestMessage(HttpMethod.Delete, "http://localhost/" + "Customers/5?version=2"); @@ -551,7 +551,7 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests Assert.Equal(path, actualUrl); } - private class RoutingResult + protected class RoutingResult { public string[] ExpectedUrls { get; set; } diff --git a/test/WebSites/CorsWebSite/Program.cs b/test/WebSites/CorsWebSite/Program.cs new file mode 100644 index 0000000000..b58c5ff2f4 --- /dev/null +++ b/test/WebSites/CorsWebSite/Program.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.IO; +using Microsoft.AspNetCore.Hosting; + +namespace CorsWebSite +{ + public class Program + { + public static void Main(string[] args) + { + var host = CreateWebHostBuilder(args) + .Build(); + + host.Run(); + } + + public static IWebHostBuilder CreateWebHostBuilder(string[] args) => + new WebHostBuilder() + .UseContentRoot(Directory.GetCurrentDirectory()) + .UseStartup() + .UseKestrel() + .UseIISIntegration(); + } +} diff --git a/test/WebSites/CorsWebSite/Startup.cs b/test/WebSites/CorsWebSite/Startup.cs index 6ee44e7f39..5c8e115755 100644 --- a/test/WebSites/CorsWebSite/Startup.cs +++ b/test/WebSites/CorsWebSite/Startup.cs @@ -76,20 +76,5 @@ namespace CorsWebSite { app.UseMvc(); } - - public static void Main(string[] args) - { - var host = CreateWebHostBuilder(args) - .Build(); - - host.Run(); - } - - public static IWebHostBuilder CreateWebHostBuilder(string[] args) => - new WebHostBuilder() - .UseContentRoot(Directory.GetCurrentDirectory()) - .UseStartup() - .UseKestrel() - .UseIISIntegration(); } } diff --git a/test/WebSites/CorsWebSite/StartupWithGlobalRouting.cs b/test/WebSites/CorsWebSite/StartupWithGlobalRouting.cs new file mode 100644 index 0000000000..f9e2a744c7 --- /dev/null +++ b/test/WebSites/CorsWebSite/StartupWithGlobalRouting.cs @@ -0,0 +1,80 @@ +// 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.IO; +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Cors.Infrastructure; +using Microsoft.AspNetCore.Hosting; +using Microsoft.Extensions.DependencyInjection; + +namespace CorsWebSite +{ + public class StartupWithGlobalRouting + { + public void ConfigureServices(IServiceCollection services) + { + services.AddMvc(options => options.EnableGlobalRouting = true); + services.Configure(options => + { + options.AddPolicy( + "AllowAnySimpleRequest", + builder => + { + builder.AllowAnyOrigin() + .WithMethods("GET", "POST", "HEAD"); + }); + + options.AddPolicy( + "AllowSpecificOrigin", + builder => + { + builder.WithOrigins("http://example.com"); + }); + + options.AddPolicy( + "WithCredentials", + builder => + { + builder.AllowCredentials() + .WithOrigins("http://example.com"); + }); + + options.AddPolicy( + "WithCredentialsAnyOrigin", + builder => + { + builder.AllowCredentials() + .AllowAnyOrigin() + .AllowAnyHeader() + .WithMethods("PUT", "POST") + .WithExposedHeaders("exposed1", "exposed2"); + }); + + options.AddPolicy( + "AllowAll", + builder => + { + builder.AllowCredentials() + .AllowAnyMethod() + .AllowAnyHeader() + .AllowAnyOrigin(); + }); + + options.AddPolicy( + "Allow example.com", + builder => + { + builder.AllowCredentials() + .AllowAnyMethod() + .AllowAnyHeader() + .WithOrigins("http://example.com"); + }); + }); + } + + public void Configure(IApplicationBuilder app) + { + app.UseMvc(); + } + } +} From fbae57cde107c3a004748c60a83db3918782e7c5 Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Fri, 27 Jul 2018 17:30:07 -0700 Subject: [PATCH 2/2] React to the removal of EndpointConstraint --- build/dependencies.props | 4 +- .../ConsumesAttribute.cs | 98 +--- .../MvcCoreServiceCollectionExtensions.cs | 7 +- .../Internal/ActionConstraintCache.cs | 43 +- .../Internal/IConsumesActionConstraint.cs | 9 - .../Internal/MvcEndpointDataSource.cs | 21 +- .../Routing/ActionConstraintMatcherPolicy.cs | 237 ++++++++++ .../Routing/IConsumesMetadata.cs | 12 + .../ConsumesAttributeTests.cs | 193 -------- .../MvcCoreServiceCollectionExtensionsTest.cs | 8 + .../ActionConstraintMatcherPolicyTest.cs | 425 ++++++++++++++++++ .../VersioningGlobalRoutingTests.cs | 21 +- .../VersioningTestsBase.cs | 2 +- .../RequestScopedActionConstraint.cs | 15 +- .../VersioningWebSite/VersionAttribute.cs | 8 +- .../VersionRangeValidator.cs | 8 +- .../VersionRouteAttribute.cs | 8 +- 17 files changed, 764 insertions(+), 355 deletions(-) create mode 100644 src/Microsoft.AspNetCore.Mvc.Core/Routing/ActionConstraintMatcherPolicy.cs create mode 100644 src/Microsoft.AspNetCore.Mvc.Core/Routing/IConsumesMetadata.cs create mode 100644 test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/ActionConstraintMatcherPolicyTest.cs diff --git a/build/dependencies.props b/build/dependencies.props index f20cec2237..9fc6186a0c 100644 --- a/build/dependencies.props +++ b/build/dependencies.props @@ -48,8 +48,8 @@ 2.2.0-preview1-34816 2.2.0-preview1-34816 2.2.0-preview1-34816 - 2.2.0-preview1-34816 - 2.2.0-preview1-34816 + 2.2.0-a-preview1-action-constraints-come-home-16802 + 2.2.0-a-preview1-action-constraints-come-home-16802 2.2.0-preview1-34816 2.2.0-preview1-34816 2.2.0-preview1-34816 diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ConsumesAttribute.cs b/src/Microsoft.AspNetCore.Mvc.Core/ConsumesAttribute.cs index 7e047b1367..8a6efd7362 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ConsumesAttribute.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ConsumesAttribute.cs @@ -10,8 +10,8 @@ using Microsoft.AspNetCore.Mvc.ApiExplorer; using Microsoft.AspNetCore.Mvc.Filters; using Microsoft.AspNetCore.Mvc.Formatters; using Microsoft.AspNetCore.Mvc.Internal; +using Microsoft.AspNetCore.Mvc.Routing; using Microsoft.AspNetCore.Routing; -using Microsoft.AspNetCore.Routing.EndpointConstraints; using Microsoft.Net.Http.Headers; using Resources = Microsoft.AspNetCore.Mvc.Core.Resources; @@ -26,8 +26,7 @@ namespace Microsoft.AspNetCore.Mvc Attribute, IResourceFilter, IConsumesActionConstraint, - IApiRequestMetadataProvider, - IConsumesEndpointConstraint + IApiRequestMetadataProvider { public static readonly int ConsumesActionConstraintOrder = 200; @@ -58,11 +57,6 @@ namespace Microsoft.AspNetCore.Mvc /// int IActionConstraint.Order => ConsumesActionConstraintOrder; - // The value used is a non default value so that it avoids getting mixed with other endpoint constraints - // with default order. - /// - int IEndpointConstraint.Order => ConsumesActionConstraintOrder; - /// /// Gets or sets the supported request content types. Used to select an action when there would otherwise be /// multiple matches. @@ -192,83 +186,6 @@ namespace Microsoft.AspNetCore.Mvc return true; } - /// - public bool Accept(EndpointConstraintContext context) - { - // If this constraint is not closest to the endpoint, it will be skipped. - if (!IsApplicable(context.CurrentCandidate.Endpoint)) - { - // Since the constraint is to be skipped, returning true here - // will let the current candidate ignore this constraint and will - // be selected based on other constraints for this endpoint. - return true; - } - - var requestContentType = context.HttpContext.Request.ContentType; - - // If the request content type is null we need to act like pass through. - // In case there is a single candidate with a constraint it should be selected. - // If there are multiple endpoints with consumes endpoint constraints this should result in ambiguous exception - // unless there is another endpoint without a consumes constraint. - if (requestContentType == null) - { - var isEndpointWithoutConsumeConstraintPresent = context.Candidates.Any( - candidate => candidate.Constraints == null || - !candidate.Constraints.Any(constraint => constraint is IConsumesEndpointConstraint)); - - return !isEndpointWithoutConsumeConstraintPresent; - } - - // Confirm the request's content type is more specific than (a media type this endpoint supports e.g. OK - // if client sent "text/plain" data and this endpoint supports "text/*". - if (IsSubsetOfAnyContentType(requestContentType)) - { - return true; - } - - var firstCandidate = context.Candidates[0]; - if (firstCandidate.Endpoint != context.CurrentCandidate.Endpoint) - { - // If the current candidate is not same as the first candidate, - // we need not probe other candidates to see if they apply. - // Only the first candidate is allowed to probe other candidates and based on the result select itself. - return false; - } - - // Run the matching logic for all IConsumesEndpointConstraints we can find, and see what matches. - // 1). If we have a unique best match, then only that constraint should return true. - // 2). If we have multiple matches, then all constraints that match will return true - // , resulting in ambiguity(maybe). - // 3). If we have no matches, then we choose the first constraint to return true.It will later return a 415 - foreach (var candidate in context.Candidates) - { - if (candidate.Endpoint == firstCandidate.Endpoint) - { - continue; - } - - var tempContext = new EndpointConstraintContext() - { - Candidates = context.Candidates, - HttpContext = context.HttpContext, - CurrentCandidate = candidate - }; - - if (candidate.Constraints == null || candidate.Constraints.Count == 0 || - candidate.Constraints.Any(constraint => constraint is IConsumesEndpointConstraint && - constraint.Accept(tempContext))) - { - // There is someone later in the chain which can handle the request. - // end the process here. - return false; - } - } - - // There is no one later in the chain that can handle this content type return a false positive so that - // later we can detect and return a 415. - return true; - } - private bool IsApplicable(ActionDescriptor actionDescriptor) { // If there are multiple IConsumeActionConstraints which are defined at the class and @@ -280,17 +197,6 @@ namespace Microsoft.AspNetCore.Mvc filter => filter.Filter is IConsumesActionConstraint).Filter == this; } - private bool IsApplicable(Endpoint endpoint) - { - // If there are multiple IConsumeActionConstraints which are defined at the class and - // at the action level, the one closest to the action overrides the others. To ensure this - // we take advantage of the fact that ConsumesAttribute is both an IActionFilter and an - // IConsumeActionConstraint. Since filterdescriptor collection is ordered (the last filter is the one - // closest to the action), we apply this constraint only if there is no IConsumeActionConstraint after this. - return endpoint.Metadata.Last( - metadata => metadata is IConsumesEndpointConstraint) == this; - } - private MediaTypeCollection GetContentTypes(string firstArg, string[] args) { var completeArgs = new List(); diff --git a/src/Microsoft.AspNetCore.Mvc.Core/DependencyInjection/MvcCoreServiceCollectionExtensions.cs b/src/Microsoft.AspNetCore.Mvc.Core/DependencyInjection/MvcCoreServiceCollectionExtensions.cs index 3d50285d76..477a67ac0a 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/DependencyInjection/MvcCoreServiceCollectionExtensions.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/DependencyInjection/MvcCoreServiceCollectionExtensions.cs @@ -20,7 +20,6 @@ using Microsoft.AspNetCore.Mvc.ModelBinding.Validation; using Microsoft.AspNetCore.Mvc.Routing; using Microsoft.AspNetCore.Routing; using Microsoft.Extensions.DependencyInjection.Extensions; -using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; namespace Microsoft.Extensions.DependencyInjection @@ -173,8 +172,10 @@ namespace Microsoft.Extensions.DependencyInjection services.TryAddSingleton(); // Will be cached by the DefaultActionSelector - services.TryAddEnumerable( - ServiceDescriptor.Transient()); + services.TryAddEnumerable(ServiceDescriptor.Transient()); + + // Policies for Endpoints + services.TryAddEnumerable(ServiceDescriptor.Singleton()); // // Controller Factory diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ActionConstraintCache.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ActionConstraintCache.cs index 3ba049a494..ff50a463b0 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ActionConstraintCache.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ActionConstraintCache.cs @@ -27,7 +27,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal _actionConstraintProviders = actionConstraintProviders.OrderBy(item => item.Order).ToArray(); } - private InnerCache CurrentCache + internal InnerCache CurrentCache { get { @@ -36,7 +36,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal if (current == null || current.Version != actionDescriptors.Version) { - current = new InnerCache(actionDescriptors.Version); + current = new InnerCache(actionDescriptors); _currentCache = current; } @@ -165,20 +165,49 @@ namespace Microsoft.AspNetCore.Mvc.Internal return actionConstraints; } - private class InnerCache + internal class InnerCache { - public InnerCache(int version) + private readonly ActionDescriptorCollection _actions; + private bool? _hasActionConstraints; + + public InnerCache(ActionDescriptorCollection actions) { - Version = version; + _actions = actions; } public ConcurrentDictionary Entries { get; } = new ConcurrentDictionary(); - public int Version { get; } + public int Version => _actions.Version; + + public bool HasActionConstraints + { + get + { + // This is a safe race-condition, since it always transitions from null to non-null. + // All writers will always get the same result. + if (_hasActionConstraints == null) + { + var found = false; + for (var i = 0; i < _actions.Items.Count; i++) + { + var action = _actions.Items[i]; + if (action.ActionConstraints?.Count > 0) + { + found = true; + break; + } + } + + _hasActionConstraints = found; + } + + return _hasActionConstraints.Value; + } + } } - private struct CacheEntry + internal readonly struct CacheEntry { public CacheEntry(IReadOnlyList actionConstraints) { diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/IConsumesActionConstraint.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/IConsumesActionConstraint.cs index 2806bd496d..b004cbdcf3 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/IConsumesActionConstraint.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/IConsumesActionConstraint.cs @@ -2,7 +2,6 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using Microsoft.AspNetCore.Mvc.ActionConstraints; -using Microsoft.AspNetCore.Routing.EndpointConstraints; namespace Microsoft.AspNetCore.Mvc.Internal { @@ -13,12 +12,4 @@ namespace Microsoft.AspNetCore.Mvc.Internal public interface IConsumesActionConstraint : IActionConstraint { } - - /// - /// An constraint that identifies a type which can be used to select an action - /// based on incoming request. - /// - public interface IConsumesEndpointConstraint : IEndpointConstraint - { - } } \ No newline at end of file diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcEndpointDataSource.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcEndpointDataSource.cs index 649f5febfa..b16b9ed018 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcEndpointDataSource.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcEndpointDataSource.cs @@ -5,14 +5,12 @@ using System; using System.Collections.Generic; using System.Collections.ObjectModel; using System.Linq; -using System.Threading.Tasks; using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc.Abstractions; using Microsoft.AspNetCore.Mvc.ActionConstraints; using Microsoft.AspNetCore.Mvc.Infrastructure; using Microsoft.AspNetCore.Routing; -using Microsoft.AspNetCore.Routing.EndpointConstraints; using Microsoft.AspNetCore.Routing.Matchers; using Microsoft.AspNetCore.Routing.Metadata; using Microsoft.AspNetCore.Routing.Patterns; @@ -338,17 +336,22 @@ namespace Microsoft.AspNetCore.Mvc.Internal if (action.ActionConstraints != null && action.ActionConstraints.Count > 0) { - // REVIEW: What is the best way to pick up endpoint constraints of an ActionDescriptor? - // Currently they need to implement IActionConstraintMetadata + // We explicitly convert a few types of action constraints into MatcherPolicy+Metadata + // to better integrate with the DFA matcher. + // + // Other IActionConstraint data will trigger a back-compat path that can execute + // action constraints. foreach (var actionConstraint in action.ActionConstraints) { - if (actionConstraint is IEndpointConstraintMetadata) + if (actionConstraint is HttpMethodActionConstraint httpMethodActionConstraint && + !metadata.OfType().Any()) + { + metadata.Add(new HttpMethodMetadata(httpMethodActionConstraint.HttpMethods)); + } + else if (!metadata.Contains(actionConstraint)) { // The constraint might have been added earlier, e.g. it is also a filter descriptor - if (!metadata.Contains(actionConstraint)) - { - metadata.Add(actionConstraint); - } + metadata.Add(actionConstraint); } } } diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Routing/ActionConstraintMatcherPolicy.cs b/src/Microsoft.AspNetCore.Mvc.Core/Routing/ActionConstraintMatcherPolicy.cs new file mode 100644 index 0000000000..60a1abe6b1 --- /dev/null +++ b/src/Microsoft.AspNetCore.Mvc.Core/Routing/ActionConstraintMatcherPolicy.cs @@ -0,0 +1,237 @@ +// 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.Http; +using Microsoft.AspNetCore.Mvc.Abstractions; +using Microsoft.AspNetCore.Mvc.ActionConstraints; +using Microsoft.AspNetCore.Mvc.Internal; +using Microsoft.AspNetCore.Routing; +using Microsoft.AspNetCore.Routing.Matchers; +using Microsoft.Extensions.Logging; + +namespace Microsoft.AspNetCore.Mvc.Routing +{ + // This is a bridge that allows us to execute IActionConstraint instance when + // used with Matcher. + internal class ActionConstraintMatcherPolicy : MatcherPolicy, IEndpointSelectorPolicy + { + private static readonly IReadOnlyList EmptyEndpoints = Array.Empty(); + + // We need to be able to run IActionConstraints on Endpoints that aren't associated + // with an action. This is a sentinel value we use when the endpoint isn't from MVC. + internal static readonly ActionDescriptor NonAction = new ActionDescriptor(); + + private readonly ActionConstraintCache _actionConstraintCache; + + public ActionConstraintMatcherPolicy(ActionConstraintCache actionConstraintCache) + { + _actionConstraintCache = actionConstraintCache; + } + + // Run really late. + public override int Order => 100000; + + public void Apply(HttpContext httpContext, CandidateSet candidateSet) + { + // PERF: we can skip over action constraints if there aren't any app-wide. + // + // Running action constraints (or just checking for them) in a candidate set + // is somewhat expensive compared to other routing operations. This should only + // happen if user-code adds action constraints. + var actions = _actionConstraintCache.CurrentCache; + if (actions.HasActionConstraints) + { + ApplyActionConstraints(httpContext, candidateSet); + } + } + + private void ApplyActionConstraints( + HttpContext httpContext, + CandidateSet candidateSet) + { + var finalMatches = EvaluateActionConstraints(httpContext, candidateSet); + + // We've computed the set of actions that still apply (and their indices) + // First, mark everything as invalid, and then mark everything in the matching + // set as valid. This is O(2n) vs O(n**2) + for (var i = 0; i < candidateSet.Count; i++) + { + candidateSet[i].IsValidCandidate = false; + } + + if (finalMatches != null) + { + for (var i = 0; i < finalMatches.Count; i++) + { + candidateSet[finalMatches[i].index].IsValidCandidate = true; + } + } + } + + // This is almost the same as the code in ActionSelector, but we can't really share the logic + // because we need to track the index of each candidate - and, each candidate has its own route + // values. + private IReadOnlyList<(int index, ActionSelectorCandidate candidate)> EvaluateActionConstraints( + HttpContext httpContext, + CandidateSet candidateSet) + { + var items = new List<(int index, ActionSelectorCandidate candidate)>(); + + // We want to execute a group at a time (based on score) so keep track of the score that we've seen. + int? score = null; + + // Perf: Avoid allocations + for (var i = 0; i < candidateSet.Count; i++) + { + ref var candidate = ref candidateSet[i]; + if (candidate.IsValidCandidate) + { + if (score != null && score != candidate.Score) + { + // This is the end of a group. + var matches = EvaluateActionConstraintsCore(httpContext, candidateSet, items, startingOrder: null); + if (matches?.Count > 0) + { + return matches; + } + + // If we didn't find matches, then reset. + items.Clear(); + } + + score = candidate.Score; + + // If we get here, this is either the first endpoint or the we just (unsuccessfully) + // executed constraints for a group. + // + // So keep adding constraints. + var endpoint = candidate.Endpoint; + var actionDescriptor = endpoint.Metadata.GetMetadata(); + + IReadOnlyList constraints = Array.Empty(); + if (actionDescriptor != null) + { + constraints = _actionConstraintCache.GetActionConstraints(httpContext, actionDescriptor); + } + + // Capture the index. We need this later to look up the endpoint/route values. + items.Add((i, new ActionSelectorCandidate(actionDescriptor ?? NonAction, constraints))); + } + } + + // Handle residue + return EvaluateActionConstraintsCore(httpContext, candidateSet, items, startingOrder: null); + } + + private IReadOnlyList<(int index, ActionSelectorCandidate candidate)> EvaluateActionConstraintsCore( + HttpContext httpContext, + CandidateSet candidateSet, + IReadOnlyList<(int index, ActionSelectorCandidate candidate)> items, + int? startingOrder) + { + // Find the next group of constraints to process. This will be the lowest value of + // order that is higher than startingOrder. + int? order = null; + + // Perf: Avoid allocations + for (var i = 0; i < items.Count; i++) + { + var item = items[i]; + var constraints = item.candidate.Constraints; + if (constraints != null) + { + for (var j = 0; j < constraints.Count; j++) + { + var constraint = constraints[j]; + if ((startingOrder == null || constraint.Order > startingOrder) && + (order == null || constraint.Order < order)) + { + order = constraint.Order; + } + } + } + } + + // If we don't find a next then there's nothing left to do. + if (order == null) + { + return items; + } + + // Since we have a constraint to process, bisect the set of endpoints into those with and without a + // constraint for the current order. + var endpointsWithConstraint = new List<(int index, ActionSelectorCandidate candidate)>(); + var endpointsWithoutConstraint = new List<(int index, ActionSelectorCandidate candidate)>(); + + var constraintContext = new ActionConstraintContext(); + constraintContext.Candidates = items.Select(i => i.candidate).ToArray(); + + // Perf: Avoid allocations + for (var i = 0; i < items.Count; i++) + { + var item = items[i]; + var isMatch = true; + var foundMatchingConstraint = false; + + var constraints = item.candidate.Constraints; + if (constraints != null) + { + constraintContext.CurrentCandidate = item.candidate; + for (var j = 0; j < constraints.Count; j++) + { + var constraint = constraints[j]; + if (constraint.Order == order) + { + foundMatchingConstraint = true; + + // Before we run the constraint, we need to initialize the route values. + // In global routing, the route values are per-endpoint. + constraintContext.RouteContext = new RouteContext(httpContext) + { + RouteData = new RouteData(candidateSet[item.index].Values), + }; + if (!constraint.Accept(constraintContext)) + { + isMatch = false; + break; + } + } + } + } + + if (isMatch && foundMatchingConstraint) + { + endpointsWithConstraint.Add(item); + } + else if (isMatch) + { + endpointsWithoutConstraint.Add(item); + } + } + + // If we have matches with constraints, those are better so try to keep processing those + if (endpointsWithConstraint.Count > 0) + { + var matches = EvaluateActionConstraintsCore(httpContext, candidateSet, endpointsWithConstraint, order); + if (matches?.Count > 0) + { + return matches; + } + } + + // If the set of matches with constraints can't work, then process the set without constraints. + if (endpointsWithoutConstraint.Count == 0) + { + return null; + } + else + { + return EvaluateActionConstraintsCore(httpContext, candidateSet, endpointsWithoutConstraint, order); + } + } + } +} diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Routing/IConsumesMetadata.cs b/src/Microsoft.AspNetCore.Mvc.Core/Routing/IConsumesMetadata.cs new file mode 100644 index 0000000000..83e04931b7 --- /dev/null +++ b/src/Microsoft.AspNetCore.Mvc.Core/Routing/IConsumesMetadata.cs @@ -0,0 +1,12 @@ +// 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.Collections.Generic; + +namespace Microsoft.AspNetCore.Mvc.Routing +{ + internal interface IConsumesMetadata + { + IReadOnlyList ContentTypes { get; } + } +} diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ConsumesAttributeTests.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ConsumesAttributeTests.cs index 30514d5ac8..57a0bbddf1 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ConsumesAttributeTests.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ConsumesAttributeTests.cs @@ -12,9 +12,6 @@ using Microsoft.AspNetCore.Mvc.Formatters; using Microsoft.AspNetCore.Mvc.Internal; using Microsoft.AspNetCore.Mvc.ModelBinding; using Microsoft.AspNetCore.Routing; -using Microsoft.AspNetCore.Routing.EndpointConstraints; -using Microsoft.AspNetCore.Routing.Matchers; -using Microsoft.AspNetCore.Routing.Patterns; using Microsoft.Net.Http.Headers; using Moq; using Xunit; @@ -296,192 +293,6 @@ namespace Microsoft.AspNetCore.Mvc Assert.True(constraint2.Accept(context)); } - private MatcherEndpoint CreateEndpoint(params IEndpointConstraint[] constraints) - { - var endpointMetadata = new EndpointMetadataCollection(constraints); - - return new MatcherEndpoint( - (r) => null, - RoutePatternFactory.Parse("/"), - new RouteValueDictionary(), - 0, - endpointMetadata, - ""); - } - - [Theory] - [InlineData("application/json")] - [InlineData("application/json;Parameter1=12")] - [InlineData("text/xml")] - public void EndpointConstraint_Accept_MatchesForMachingRequestContentType(string contentType) - { - // Arrange - var constraint = new ConsumesAttribute("application/json", "text/xml"); - var endpoint = CreateEndpoint(constraint); - - var context = new EndpointConstraintContext(); - context.Candidates = new List() - { - new EndpointSelectorCandidate(endpoint, new [] { constraint }), - }; - - context.CurrentCandidate = context.Candidates[0]; - context.HttpContext = CreateHttpContext(contentType: contentType); - - // Act & Assert - Assert.True(constraint.Accept(context)); - } - - [Fact] - public void EndpointConstraint_Accept_TheFirstCandidateReturnsFalse_IfALaterOneMatches() - { - // Arrange - var constraint1 = new ConsumesAttribute("application/json", "text/xml"); - var endpoint1 = CreateEndpoint(constraint1); - - var constraint2 = new Mock(); - var endpoint2 = CreateEndpoint(constraint2.Object); - - constraint2.Setup(o => o.Accept(It.IsAny())) - .Returns(true); - - var context = new EndpointConstraintContext(); - context.Candidates = new List() - { - new EndpointSelectorCandidate(endpoint1, new [] { constraint1 }), - new EndpointSelectorCandidate(endpoint2, new [] { constraint2.Object }), - }; - - context.CurrentCandidate = context.Candidates[0]; - context.HttpContext = CreateHttpContext(contentType: "application/custom"); - - // Act & Assert - Assert.False(constraint1.Accept(context)); - } - - [Theory] - [InlineData("application/custom")] - [InlineData("")] - [InlineData(null)] - public void EndpointConstraint_Accept_ForNoMatchingCandidates_SelectsTheFirstCandidate(string contentType) - { - // Arrange - var constraint1 = new ConsumesAttribute("application/json", "text/xml"); - var endpoint1 = CreateEndpoint(constraint1); - - var constraint2 = new Mock(); - var endpoint2 = CreateEndpoint(constraint2.Object); - - constraint2.Setup(o => o.Accept(It.IsAny())) - .Returns(false); - - var context = new EndpointConstraintContext(); - context.Candidates = new List() - { - new EndpointSelectorCandidate(endpoint1, new [] { constraint1 }), - new EndpointSelectorCandidate(endpoint2, new [] { constraint2.Object }), - }; - - context.CurrentCandidate = context.Candidates[0]; - context.HttpContext = CreateHttpContext(contentType: contentType); - - // Act & Assert - Assert.True(constraint1.Accept(context)); - } - - [Theory] - [InlineData("")] - [InlineData(null)] - public void EndpointConstraint_Accept_ForNoRequestType_SelectsTheCandidateWithoutConstraintIfPresent(string contentType) - { - // Arrange - var constraint1 = new ConsumesAttribute("application/json"); - var endpointWithConstraint = CreateEndpoint(constraint1); - - var constraint2 = new ConsumesAttribute("text/xml"); - var endpointWithConstraint2 = CreateEndpoint(constraint2); - - var endpointWithoutConstraint = CreateEndpoint(); - - var context = new EndpointConstraintContext(); - context.Candidates = new List() - { - new EndpointSelectorCandidate(endpointWithConstraint, new [] { constraint1 }), - new EndpointSelectorCandidate(endpointWithConstraint2, new [] { constraint2 }), - new EndpointSelectorCandidate(endpointWithoutConstraint, new List()), - }; - - context.HttpContext = CreateHttpContext(contentType: contentType); - - // Act & Assert - context.CurrentCandidate = context.Candidates[0]; - Assert.False(constraint1.Accept(context)); - context.CurrentCandidate = context.Candidates[1]; - Assert.False(constraint2.Accept(context)); - } - - [Theory] - [InlineData("application/xml")] - [InlineData("application/custom")] - [InlineData("invalid/invalid")] - public void EndpointConstraint_Accept_UnrecognizedMediaType_SelectsTheCandidateWithoutConstraintIfPresent(string contentType) - { - // Arrange - var endpointWithoutConstraint = CreateEndpoint(); - var constraint1 = new ConsumesAttribute("application/json"); - var endpointWithConstraint = CreateEndpoint(constraint1); - - var constraint2 = new ConsumesAttribute("text/xml"); - var endpointWithConstraint2 = CreateEndpoint(constraint2); - - var context = new EndpointConstraintContext(); - context.Candidates = new List() - { - new EndpointSelectorCandidate(endpointWithConstraint, new [] { constraint1 }), - new EndpointSelectorCandidate(endpointWithConstraint2, new [] { constraint2 }), - new EndpointSelectorCandidate(endpointWithoutConstraint, new List()), - }; - - context.HttpContext = CreateHttpContext(contentType: contentType); - - // Act & Assert - context.CurrentCandidate = context.Candidates[0]; - Assert.False(constraint1.Accept(context)); - - context.CurrentCandidate = context.Candidates[1]; - Assert.False(constraint2.Accept(context)); - } - - [Theory] - [InlineData("")] - [InlineData(null)] - public void EndpointConstraint_Accept_ForNoRequestType_ReturnsTrueForAllConstraints(string contentType) - { - // Arrange - var constraint1 = new ConsumesAttribute("application/json"); - var endpointWithConstraint = CreateEndpoint(constraint1); - - var constraint2 = new ConsumesAttribute("text/xml"); - var endpointWithConstraint2 = CreateEndpoint(constraint2); - - var endpointWithoutConstraint = CreateEndpoint(); - - var context = new EndpointConstraintContext(); - context.Candidates = new List() - { - new EndpointSelectorCandidate(endpointWithConstraint, new [] { constraint1 }), - new EndpointSelectorCandidate(endpointWithConstraint2, new [] { constraint2 }), - }; - - context.HttpContext = CreateHttpContext(contentType: contentType); - - // Act & Assert - context.CurrentCandidate = context.Candidates[0]; - Assert.True(constraint1.Accept(context)); - context.CurrentCandidate = context.Candidates[1]; - Assert.True(constraint2.Accept(context)); - } - [Theory] [InlineData("application/xml")] [InlineData("application/custom")] @@ -620,9 +431,5 @@ namespace Microsoft.AspNetCore.Mvc public interface ITestActionConsumeConstraint : IConsumesActionConstraint, IResourceFilter { } - - public interface ITestEndpointConsumeConstraint : IConsumesEndpointConstraint, IResourceFilter - { - } } } \ No newline at end of file diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/DependencyInjection/MvcCoreServiceCollectionExtensionsTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/DependencyInjection/MvcCoreServiceCollectionExtensionsTest.cs index 3e6c558518..9660812ec5 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/DependencyInjection/MvcCoreServiceCollectionExtensionsTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/DependencyInjection/MvcCoreServiceCollectionExtensionsTest.cs @@ -14,6 +14,7 @@ using Microsoft.AspNetCore.Mvc.Controllers; using Microsoft.AspNetCore.Mvc.Filters; using Microsoft.AspNetCore.Mvc.Infrastructure; using Microsoft.AspNetCore.Mvc.Internal; +using Microsoft.AspNetCore.Mvc.Routing; using Microsoft.AspNetCore.Routing; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.DependencyInjection.Extensions; @@ -320,6 +321,13 @@ namespace Microsoft.AspNetCore.Mvc typeof(MiddlewareFilterBuilderStartupFilter) } }, + { + typeof(MatcherPolicy), + new Type[] + { + typeof(ActionConstraintMatcherPolicy), + } + }, }; } } diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/ActionConstraintMatcherPolicyTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/ActionConstraintMatcherPolicyTest.cs new file mode 100644 index 0000000000..51a00d6ddd --- /dev/null +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/ActionConstraintMatcherPolicyTest.cs @@ -0,0 +1,425 @@ +// 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.Reflection; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc.Abstractions; +using Microsoft.AspNetCore.Mvc.ActionConstraints; +using Microsoft.AspNetCore.Mvc.ApplicationParts; +using Microsoft.AspNetCore.Mvc.Controllers; +using Microsoft.AspNetCore.Mvc.Infrastructure; +using Microsoft.AspNetCore.Mvc.Internal; +using Microsoft.AspNetCore.Routing; +using Microsoft.AspNetCore.Routing.Matchers; +using Microsoft.AspNetCore.Routing.Patterns; +using Moq; +using Xunit; + +namespace Microsoft.AspNetCore.Mvc.Routing +{ + public class ActionConstraintMatcherPolicyTest + { + [Fact] + public void Apply_CanBeAmbiguous() + { + // Arrange + var actions = new ActionDescriptor[] + { + new ActionDescriptor() { DisplayName = "A1" }, + new ActionDescriptor() { DisplayName = "A2" }, + }; + + var candidateSet = CreateCandidateSet(actions); + + var selector = CreateSelector(actions); + + // Act + selector.Apply(new DefaultHttpContext(), candidateSet); + + // Assert + Assert.True(candidateSet[0].IsValidCandidate); + Assert.True(candidateSet[1].IsValidCandidate); + } + + [Fact] + public void Apply_PrefersActionWithConstraints() + { + // Arrange + var actionWithConstraints = new ActionDescriptor() + { + ActionConstraints = new List() + { + new HttpMethodActionConstraint(new string[] { "POST" }), + }, + Parameters = new List(), + }; + + var actionWithoutConstraints = new ActionDescriptor() + { + Parameters = new List(), + }; + + var actions = new ActionDescriptor[] { actionWithConstraints, actionWithoutConstraints }; + var candidateSet = CreateCandidateSet(actions); + + var selector = CreateSelector(actions); + + var httpContext = CreateHttpContext("POST"); + + // Act + selector.Apply(httpContext, candidateSet); + + // Assert + Assert.True(candidateSet[0].IsValidCandidate); + Assert.False(candidateSet[1].IsValidCandidate); + } + + [Fact] + public void Apply_ConstraintsRejectAll() + { + // Arrange + var action1 = new ActionDescriptor() + { + ActionConstraints = new List() + { + new BooleanConstraint() { Pass = false, }, + }, + }; + + var action2 = new ActionDescriptor() + { + ActionConstraints = new List() + { + new BooleanConstraint() { Pass = false, }, + }, + }; + + var actions = new ActionDescriptor[] { action1, action2 }; + var candidateSet = CreateCandidateSet(actions); + + var selector = CreateSelector(actions); + + var httpContext = CreateHttpContext("POST"); + + // Act + selector.Apply(httpContext, candidateSet); + + // Assert + Assert.False(candidateSet[0].IsValidCandidate); + Assert.False(candidateSet[1].IsValidCandidate); + } + + [Fact] + public void Apply_ConstraintsRejectAll_DifferentStages() + { + // Arrange + var action1 = new ActionDescriptor() + { + ActionConstraints = new List() + { + new BooleanConstraint() { Pass = false, Order = 0 }, + new BooleanConstraint() { Pass = true, Order = 1 }, + }, + }; + + var action2 = new ActionDescriptor() + { + ActionConstraints = new List() + { + new BooleanConstraint() { Pass = true, Order = 0 }, + new BooleanConstraint() { Pass = false, Order = 1 }, + }, + }; + + var actions = new ActionDescriptor[] { action1, action2 }; + var candidateSet = CreateCandidateSet(actions); + + var selector = CreateSelector(actions); + var httpContext = CreateHttpContext("POST"); + + // Act + selector.Apply(httpContext, candidateSet); + + // Assert + Assert.False(candidateSet[0].IsValidCandidate); + Assert.False(candidateSet[1].IsValidCandidate); + } + + // Due to ordering of stages, the first action will be better. + [Fact] + public void Apply_ConstraintsInOrder() + { + // Arrange + var best = new ActionDescriptor() + { + ActionConstraints = new List() + { + new BooleanConstraint() { Pass = true, Order = 0, }, + }, + }; + + var worst = new ActionDescriptor() + { + ActionConstraints = new List() + { + new BooleanConstraint() { Pass = true, Order = 1, }, + }, + }; + + var actions = new ActionDescriptor[] { best, worst }; + var candidateSet = CreateCandidateSet(actions); + + var selector = CreateSelector(actions); + var httpContext = CreateHttpContext("POST"); + + // Act + selector.Apply(httpContext, candidateSet); + + // Assert + Assert.True(candidateSet[0].IsValidCandidate); + Assert.False(candidateSet[1].IsValidCandidate); + } + + [Fact] + public void Apply_SkipsOverInvalidEndpoints() + { + // Arrange + var best = new ActionDescriptor() + { + ActionConstraints = new List() + { + new BooleanConstraint() { Pass = true, Order = 0, }, + }, + }; + + var another = new ActionDescriptor(); + + var worst = new ActionDescriptor() + { + ActionConstraints = new List() + { + new BooleanConstraint() { Pass = true, Order = 1, }, + }, + }; + + var actions = new ActionDescriptor[] { best, another, worst }; + var candidateSet = CreateCandidateSet(actions); + candidateSet[0].IsValidCandidate = false; + candidateSet[1].IsValidCandidate = false; + + var selector = CreateSelector(actions); + var httpContext = CreateHttpContext("POST"); + + // Act + selector.Apply(httpContext, candidateSet); + + // Assert + Assert.False(candidateSet[0].IsValidCandidate); + Assert.False(candidateSet[1].IsValidCandidate); + Assert.True(candidateSet[2].IsValidCandidate); + } + + [Fact] + public void Apply_IncludesNonMvcEndpoints() + { + // Arrange + var action1 = new ActionDescriptor() + { + ActionConstraints = new List() + { + new BooleanConstraint() { Pass = false, Order = 0, }, + }, + }; + + var action2 = new ActionDescriptor() + { + ActionConstraints = new List() + { + new BooleanConstraint() { Pass = false, Order = 1, }, + }, + }; + + var actions = new ActionDescriptor[] { action1, null, action2 }; + var candidateSet = CreateCandidateSet(actions); + + var selector = CreateSelector(actions); + var httpContext = CreateHttpContext("POST"); + + // Act + selector.Apply(httpContext, candidateSet); + + // Assert + Assert.False(candidateSet[0].IsValidCandidate); + Assert.True(candidateSet[1].IsValidCandidate); + Assert.False(candidateSet[2].IsValidCandidate); + } + + // Due to ordering of stages, the first action will be better. + [Fact] + public void Apply_ConstraintsInOrder_MultipleStages() + { + // Arrange + var best = new ActionDescriptor() + { + ActionConstraints = new List() + { + new BooleanConstraint() { Pass = true, Order = 0, }, + new BooleanConstraint() { Pass = true, Order = 1, }, + new BooleanConstraint() { Pass = true, Order = 2, }, + }, + }; + + var worst = new ActionDescriptor() + { + ActionConstraints = new List() + { + new BooleanConstraint() { Pass = true, Order = 0, }, + new BooleanConstraint() { Pass = true, Order = 1, }, + new BooleanConstraint() { Pass = true, Order = 3, }, + }, + }; + + var actions = new ActionDescriptor[] { best, worst }; + var candidateSet = CreateCandidateSet(actions); + + var selector = CreateSelector(actions); + + var httpContext = CreateHttpContext("POST"); + + // Act + selector.Apply(httpContext, candidateSet); + + // Assert + Assert.True(candidateSet[0].IsValidCandidate); + Assert.False(candidateSet[1].IsValidCandidate); + } + + [Fact] + public void Apply_Fallback_ToActionWithoutConstraints() + { + // Arrange + var nomatch1 = new ActionDescriptor() + { + ActionConstraints = new List() + { + new BooleanConstraint() { Pass = true, Order = 0, }, + new BooleanConstraint() { Pass = true, Order = 1, }, + new BooleanConstraint() { Pass = false, Order = 2, }, + }, + }; + + var nomatch2 = new ActionDescriptor() + { + ActionConstraints = new List() + { + new BooleanConstraint() { Pass = true, Order = 0, }, + new BooleanConstraint() { Pass = true, Order = 1, }, + new BooleanConstraint() { Pass = false, Order = 3, }, + }, + }; + + var best = new ActionDescriptor(); + + var actions = new ActionDescriptor[] { best, nomatch1, nomatch2 }; + var candidateSet = CreateCandidateSet(actions); + + var selector = CreateSelector(actions); + + var httpContext = CreateHttpContext("POST"); + + // Act + selector.Apply(httpContext, candidateSet); + + // Assert + Assert.True(candidateSet[0].IsValidCandidate); + Assert.False(candidateSet[1].IsValidCandidate); + Assert.False(candidateSet[2].IsValidCandidate); + } + + private ActionConstraintMatcherPolicy CreateSelector(ActionDescriptor[] actions) + { + // We need to actually provide some actions with some action constraints metadata + // or else the policy will No-op. + var actionDescriptorProvider = new Mock(); + actionDescriptorProvider + .Setup(a => a.OnProvidersExecuted(It.IsAny())) + .Callback(c => + { + for (var i = 0; i < actions.Length; i++) + { + c.Results.Add(actions[i]); + } + }); + + var actionDescriptorCollectionProvider = new ActionDescriptorCollectionProvider( + new IActionDescriptorProvider[] { actionDescriptorProvider.Object, }, + Enumerable.Empty()); + + var cache = new ActionConstraintCache(actionDescriptorCollectionProvider, new[] + { + new DefaultActionConstraintProvider(), + }); + + return new ActionConstraintMatcherPolicy(cache); + } + + private static HttpContext CreateHttpContext(string httpMethod) + { + var httpContext = new DefaultHttpContext(); + httpContext.Request.Method = httpMethod; + return httpContext; + } + + private static MatcherEndpoint CreateEndpoint(ActionDescriptor action) + { + var metadata = new List() { action, }; + return new MatcherEndpoint( + (r) => null, + RoutePatternFactory.Parse("/"), + new RouteValueDictionary(), + 0, + new EndpointMetadataCollection(metadata), + $"test: {action?.DisplayName}"); + } + + private static CandidateSet CreateCandidateSet(ActionDescriptor[] actions) + { + var candidateSet = new CandidateSet( + actions.Select(CreateEndpoint).ToArray(), + new int[actions.Length]); + + for (var i = 0; i < actions.Length; i++) + { + if (candidateSet[i].IsValidCandidate) + { + candidateSet[i].Values = new RouteValueDictionary(); + } + } + + return candidateSet; + } + + private static ActionConstraintCache GetActionConstraintCache(IActionConstraintProvider[] actionConstraintProviders = null) + { + var descriptorProvider = new ActionDescriptorCollectionProvider( + Enumerable.Empty(), + Enumerable.Empty()); + return new ActionConstraintCache(descriptorProvider, actionConstraintProviders.AsEnumerable() ?? new List()); + } + + private class BooleanConstraint : IActionConstraint + { + public bool Pass { get; set; } + + public int Order { get; set; } + + public bool Accept(ActionConstraintContext context) + { + return Pass; + } + } + } +} diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/VersioningGlobalRoutingTests.cs b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/VersioningGlobalRoutingTests.cs index a79ebc9516..69af08c14b 100644 --- a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/VersioningGlobalRoutingTests.cs +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/VersioningGlobalRoutingTests.cs @@ -40,6 +40,25 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests // Arrange var message = new HttpRequestMessage(HttpMethod.Delete, "http://localhost/" + "Customers/5?version=2"); + // Act + var response = await Client.SendAsync(message); + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + var body = await response.Content.ReadAsStringAsync(); + var result = JsonConvert.DeserializeObject(body); + Assert.Equal("Customers", result.Controller); + Assert.Equal("Delete", result.Action); + } + + // This behaves differently right now because the action/endpoint constraints are always + // executed after the DFA nodes like (HttpMethodMatcherPolicy). You don't have the flexibility + // to do what this test is doing in old-style routing. + [Fact] + public override async Task VersionedApi_ConstraintOrder_IsRespected() + { + // Arrange + var message = new HttpRequestMessage(HttpMethod.Post, "http://localhost/" + "Customers?version=2"); + // Act var response = await Client.SendAsync(message); @@ -50,7 +69,7 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests var result = JsonConvert.DeserializeObject(body); Assert.Equal("Customers", result.Controller); - Assert.Equal("AnyV2OrHigherWithId", result.Action); + Assert.Equal("Post", result.Action); } } } \ No newline at end of file diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/VersioningTestsBase.cs b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/VersioningTestsBase.cs index 01e6ef154b..a2b7c36325 100644 --- a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/VersioningTestsBase.cs +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/VersioningTestsBase.cs @@ -489,7 +489,7 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests } [Fact] - public async Task VersionedApi_ConstraintOrder_IsRespected() + public virtual async Task VersionedApi_ConstraintOrder_IsRespected() { // Arrange var message = new HttpRequestMessage(HttpMethod.Post, "http://localhost/" + "Customers?version=2"); diff --git a/test/WebSites/BasicWebSite/RequestScopedActionConstraint.cs b/test/WebSites/BasicWebSite/RequestScopedActionConstraint.cs index 0b351b389a..ed5a267e3b 100644 --- a/test/WebSites/BasicWebSite/RequestScopedActionConstraint.cs +++ b/test/WebSites/BasicWebSite/RequestScopedActionConstraint.cs @@ -4,13 +4,12 @@ using System; using System.Collections.Concurrent; using Microsoft.AspNetCore.Mvc.ActionConstraints; -using Microsoft.AspNetCore.Routing.EndpointConstraints; using Microsoft.Extensions.DependencyInjection; namespace BasicWebSite { // Only matches when the requestId is the same as the one passed in the constructor. - public class RequestScopedConstraintAttribute : Attribute, IActionConstraintFactory, IEndpointConstraintFactory + public class RequestScopedConstraintAttribute : Attribute, IActionConstraintFactory { private readonly string _requestId; private readonly Func CreateFactory = @@ -30,18 +29,13 @@ namespace BasicWebSite return CreateInstanceCore(services); } - IEndpointConstraint IEndpointConstraintFactory.CreateInstance(IServiceProvider services) - { - return CreateInstanceCore(services); - } - private Constraint CreateInstanceCore(IServiceProvider services) { var constraintType = typeof(Constraint); return (Constraint)ActivatorUtilities.CreateInstance(services, typeof(Constraint), new[] { _requestId }); } - private class Constraint : IActionConstraint, IEndpointConstraint + private class Constraint : IActionConstraint { private readonly RequestIdService _requestIdService; private readonly string _requestId; @@ -59,11 +53,6 @@ namespace BasicWebSite return AcceptCore(); } - bool IEndpointConstraint.Accept(EndpointConstraintContext context) - { - return AcceptCore(); - } - private bool AcceptCore() { return _requestId == _requestIdService.RequestId; diff --git a/test/WebSites/VersioningWebSite/VersionAttribute.cs b/test/WebSites/VersioningWebSite/VersionAttribute.cs index 827aaef23c..5a90faf2fb 100644 --- a/test/WebSites/VersioningWebSite/VersionAttribute.cs +++ b/test/WebSites/VersioningWebSite/VersionAttribute.cs @@ -3,11 +3,10 @@ using System; using Microsoft.AspNetCore.Mvc.ActionConstraints; -using Microsoft.AspNetCore.Routing.EndpointConstraints; namespace VersioningWebSite { - public class VersionAttribute : Attribute, IActionConstraintFactory, IEndpointConstraintFactory + public class VersionAttribute : Attribute, IActionConstraintFactory { private int? _maxVersion; private int? _minVersion; @@ -37,10 +36,5 @@ namespace VersioningWebSite { return new VersionRangeValidator(_minVersion, _maxVersion) { Order = _order ?? 0 }; } - - IEndpointConstraint IEndpointConstraintFactory.CreateInstance(IServiceProvider services) - { - return new VersionRangeValidator(_minVersion, _maxVersion) { Order = _order ?? 0 }; - } } } \ No newline at end of file diff --git a/test/WebSites/VersioningWebSite/VersionRangeValidator.cs b/test/WebSites/VersioningWebSite/VersionRangeValidator.cs index ec8d244de3..858aacc16e 100644 --- a/test/WebSites/VersioningWebSite/VersionRangeValidator.cs +++ b/test/WebSites/VersioningWebSite/VersionRangeValidator.cs @@ -3,11 +3,10 @@ using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc.ActionConstraints; -using Microsoft.AspNetCore.Routing.EndpointConstraints; namespace VersioningWebSite { - public class VersionRangeValidator : IActionConstraint, IEndpointConstraint + public class VersionRangeValidator : IActionConstraint { private readonly int? _minVersion; private readonly int? _maxVersion; @@ -30,11 +29,6 @@ namespace VersioningWebSite return ProcessRequest(context.RouteContext.HttpContext.Request); } - public bool Accept(EndpointConstraintContext context) - { - return ProcessRequest(context.HttpContext.Request); - } - private bool ProcessRequest(HttpRequest request) { int version; diff --git a/test/WebSites/VersioningWebSite/VersionRouteAttribute.cs b/test/WebSites/VersioningWebSite/VersionRouteAttribute.cs index 288a488790..e12ec2d805 100644 --- a/test/WebSites/VersioningWebSite/VersionRouteAttribute.cs +++ b/test/WebSites/VersioningWebSite/VersionRouteAttribute.cs @@ -5,11 +5,10 @@ using System; using System.Text.RegularExpressions; using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.Mvc.ActionConstraints; -using Microsoft.AspNetCore.Routing.EndpointConstraints; namespace VersioningWebSite { - public class VersionRouteAttribute : RouteAttribute, IActionConstraintFactory, IEndpointConstraintFactory + public class VersionRouteAttribute : RouteAttribute, IActionConstraintFactory { private readonly IActionConstraint _actionConstraint; @@ -127,10 +126,5 @@ namespace VersioningWebSite { return _actionConstraint; } - - IEndpointConstraint IEndpointConstraintFactory.CreateInstance(IServiceProvider services) - { - return (IEndpointConstraint)_actionConstraint; - } } } \ No newline at end of file