Skip to content

Fixing CORS attribute #544

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
habbes merged 1 commit into from
Apr 4, 2022
Merged

Fixing CORS attribute #544

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 14 additions & 7 deletions src/Microsoft.AspNetCore.OData/Extensions/ActionModelExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using System.Collections.Generic;
using System.Diagnostics.Contracts;
using System.Linq;
using Microsoft.AspNetCore.Cors.Infrastructure;
using Microsoft.AspNetCore.Mvc.ActionConstraints;
using Microsoft.AspNetCore.Mvc.ApplicationModels;
using Microsoft.AspNetCore.Mvc.Routing;
Expand Down Expand Up @@ -173,7 +174,13 @@ public static void AddSelector(this ActionModel action, string httpMethods, stri
// let's always create new selector model for action.
// Since the new created selector model is absolute attribute route, the controller attribute route doesn't apply to this selector model.
bool hasAttributeRouteOnController = action.Controller.Selectors.Any(s => s.AttributeRouteModel != null);


// Check if CORS attribute is specified on action. New selectors need to be registered with CORS support.
bool acceptPreflight = action.Controller.Attributes.OfType<IDisableCorsAttribute>().Any() ||

@gathogojr gathogojr Apr 4, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@giulianob Based on this statement, how does a controller action with neither EnableCors nor DisableCors return a value of true for AcceptCorsPreflight of HttpMethodMetadata instance?
RE: https://github.com/OData/AspNetCoreOData/pull/544/files#diff-b4918c8cb621def5a0806e8f4e36a80bf254f78da5f1bb2b27f2eb9a1afd1de7R123

@habbes habbes Apr 4, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gathogojr the Get action in the tests does have an [EnableCors] attribute, see: https://github.com/OData/AspNetCoreOData/pull/544/files#diff-b4918c8cb621def5a0806e8f4e36a80bf254f78da5f1bb2b27f2eb9a1afd1de7R152

What doesn't have the attribute is the TestController.Index action. And for that one, the expected result is false.

action.Controller.Attributes.OfType<IEnableCorsAttribute>().Any() ||
action.Attributes.OfType<IDisableCorsAttribute>().Any() ||
action.Attributes.OfType<IEnableCorsAttribute>().Any();

// If the methods have different case sensitive, for example, "get", "Get", in the ASP.NET Core 3.1,
// It will throw "An item with the same key has already been added. Key: GET", in
// HttpMethodMatcherPolicy.BuildJumpTable(Int32 exitDestination, IReadOnlyList`1 edges)
Expand All @@ -189,13 +196,13 @@ public static void AddSelector(this ActionModel action, string httpMethods, stri
if (hasAttributeRouteOnController || selectorModel == null)
{
// Create a new selector model.
selectorModel = CreateSelectorModel(action, methods);
selectorModel = CreateSelectorModel(action, methods, acceptPreflight);
action.Selectors.Add(selectorModel);
}
else
{
// Update the existing non attribute routing selector model.
selectorModel = UpdateSelectorModel(selectorModel, methods);
selectorModel = UpdateSelectorModel(selectorModel, methods, acceptPreflight);
}

ODataRoutingMetadata odataMetadata = new ODataRoutingMetadata(prefix, model, path);
Expand All @@ -216,7 +223,7 @@ public static void AddSelector(this ActionModel action, string httpMethods, stri
}
}

internal static SelectorModel UpdateSelectorModel(SelectorModel selectorModel, string[] httpMethods)
internal static SelectorModel UpdateSelectorModel(SelectorModel selectorModel, string[] httpMethods, bool acceptPreflight)
{
Contract.Assert(selectorModel != null);

Expand Down Expand Up @@ -257,15 +264,15 @@ internal static SelectorModel UpdateSelectorModel(SelectorModel selectorModel, s
// append the http method metadata.
Contract.Assert(httpMethods.Length >= 1);
selectorModel.ActionConstraints.Add(new HttpMethodActionConstraint(httpMethods));
selectorModel.EndpointMetadata.Add(new HttpMethodMetadata(httpMethods));
selectorModel.EndpointMetadata.Add(new HttpMethodMetadata(httpMethods, acceptPreflight));

// append controller attributes to action selector model? -- NO
// Be noted: https://github.com/dotnet/aspnetcore/blob/main/src/Mvc/Mvc.Core/src/ApplicationModels/ActionAttributeRouteModel.cs#L74-L75

return selectorModel;
}

internal static SelectorModel CreateSelectorModel(ActionModel actionModel, string[] httpMethods)
internal static SelectorModel CreateSelectorModel(ActionModel actionModel, string[] httpMethods, bool acceptPreflight)
{
Contract.Assert(actionModel != null);

Expand Down Expand Up @@ -309,7 +316,7 @@ internal static SelectorModel CreateSelectorModel(ActionModel actionModel, strin

Contract.Assert(httpMethods.Length >= 1);
selectorModel.ActionConstraints.Add(new HttpMethodActionConstraint(httpMethods));
selectorModel.EndpointMetadata.Add(new HttpMethodMetadata(httpMethods));
selectorModel.EndpointMetadata.Add(new HttpMethodMetadata(httpMethods, acceptPreflight));

// append controller attributes to action selector model? -- NO
// Be noted: https://github.com/dotnet/aspnetcore/blob/main/src/Mvc/Mvc.Core/src/ApplicationModels/ActionAttributeRouteModel.cs#L74-L75
Expand Down
44 changes: 42 additions & 2 deletions test/Microsoft.AspNetCore.OData.Tests/Extensions/ActionModelExtensionsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,17 @@
// </copyright>
//------------------------------------------------------------------------------

using System;
using System.Collections.Generic;
using System.Linq;
using System.Reflection;
using Microsoft.AspNetCore.Cors;
using Microsoft.AspNetCore.Mvc.ApplicationModels;
using Microsoft.AspNetCore.OData.Extensions;
using Microsoft.AspNetCore.OData.Routing.Attributes;
using Microsoft.AspNetCore.OData.Routing.Template;
using Microsoft.AspNetCore.OData.Tests.Commons;
using Microsoft.AspNetCore.Routing;
using Microsoft.OData.Edm;
using Moq;
using Xunit;
Expand Down Expand Up @@ -113,15 +118,50 @@ public void AddSelector_ThrowsArgumentNull_ForInputParameter()
IEdmModel model = new Mock<IEdmModel>().Object;
ExceptionAssert.ThrowsArgumentNull(() => action.AddSelector(httpMethods, null, model, null), "path");
}

[Theory]
[InlineData(typeof(TestController), "Get", true)]
[InlineData(typeof(TestController), "Create", true)]
[InlineData(typeof(TestController), "Index", false)]
[InlineData(typeof(CorsTestController), "Index", true)]
public void AddSelector_AddsCors_ForActionsWithCorsAttribute(Type controllerType, string actionName, bool expectedCorsSetting)
{
// Arrange
IEdmModel model = new Mock<IEdmModel>().Object;
MethodInfo methodInfo = controllerType.GetMethod(actionName);
ActionModel action = methodInfo.BuildActionModel();
action.Controller = ControllerModelHelpers.BuildControllerModel(controllerType);

// Act
action.AddSelector("Get", string.Empty, model, new ODataPathTemplate(CountSegmentTemplate.Instance));

// Assert
SelectorModel newSelector = action.Selectors.FirstOrDefault();
Assert.NotNull(newSelector);
HttpMethodMetadata httpMethodMetadata = newSelector.EndpointMetadata.OfType<HttpMethodMetadata>().FirstOrDefault();
Assert.NotNull(httpMethodMetadata);
Assert.Equal(httpMethodMetadata.AcceptCorsPreflight, expectedCorsSetting);
}
}

internal class TestController
{
public void Index(int id)
{
}
{ }

[EnableCors]
public void Get(int key)
{ }

[DisableCors]
public void Create()
{ }
}

[EnableCors]
internal class CorsTestController
{
public void Index(int id)
{ }
}
}