Update CORS middleware to use endpoint metadata #4460

JamesNK · 2018-12-06T07:50:05Z

Fetch policy name from endpoint metadata
Run functional tests with endpoint routing and traditional routing

Addresses #4210

JamesNK · 2018-12-06T07:57:16Z

Design questions:

Right now IDisableCorsAttribute is not being used. Should it be? If so:
- Does IDisableCorsAttribute override IEnableCorsAttribute when both are present?
- Or does IDisableCorsAttribute override IEnableCorsAttribute when it has a higher precedence?
The middleware is currently on for every request. This is its previous behavior. IEnableCorsAttribute metadata only allows the policy to be customized per endpoint. Should it be changed to be opt in?

JamesNK · 2018-12-06T08:18:51Z

The middleware is currently on for every request. This is its previous behavior. IEnableCorsAttribute metadata only allows the policy to be customized per endpoint. Should it be changed to be opt in?

Scratch that. While it is on for every request, the default policy name will resolve to __DefaultCorsPolicy that has no value in the policy map and the middleware will no-op (apart from a log message saying that it can't find the policy name). The middleware is kind of opt-in today 👍 I think the logging should be changed to when the policy name that can't be found isn't the default policy name.

src/CORS/test/Microsoft.AspNetCore.Cors.Test/CorsMiddlewareTests.cs

src/CORS/samples/SampleDestination/Startup.cs

rynowak · 2018-12-06T16:11:18Z

The middleware is currently on for every request. This is its previous behavior.

What does it do in that case? Does it have a default policy that it applies unless overridden? That seems super nice.

rynowak · 2018-12-06T16:11:30Z

The middleware is currently on for every request. This is its previous behavior.

What does it do in that case? Does it have a default policy that it applies unless overridden? That seems super nice.

rynowak · 2018-12-06T16:12:36Z

The middleware is currently on for every request. This is its previous behavior.

What does it do in that case? Does it have a default policy that it applies unless overridden? That seems super nice.

rynowak · 2018-12-06T16:13:19Z

Github is telling me that my comments don't work, but then adding them anyway. Leaving the dupes because it's more A E S T H E T I C

rynowak · 2018-12-06T16:14:50Z

...CORS/src/Microsoft.AspNetCore.Cors/Infrastructure/CorsEndpointConventionBuilderExtensions.cs

+{
+    public static class CorsEndpointConventionBuilderExtensions
+    {
+        public static IEndpointConventionBuilder RequireCors(this IEndpointConventionBuilder builder, string policyName)


can policy name by null? What does that mean?

A null policy name means either the default policy or default policy name configured passed to the middleware constructor is used.

...CORS/src/Microsoft.AspNetCore.Cors/Infrastructure/CorsEndpointConventionBuilderExtensions.cs

src/CORS/src/Microsoft.AspNetCore.Cors/Infrastructure/CorsMiddleware.cs

rynowak

This looks on the right track 😁

natemcmaster · 2018-12-06T20:13:56Z

We're adding required PR checks to ensure the repo compiles before you merge. Can you update this PR by rebasing on master?

git fetch origin master:master
git rebase master

Alternative: if you plan to 'squash merge' this PR, you can also use a merge to update this PR. This can be easier to manage if you have lots of changes in this PR.

git fetch origin master:master
git merge master

JamesNK · 2018-12-07T00:55:37Z

@natemcmaster The build is failing with this message:

Unable to find package Microsoft.AspNetCore.Routing with version (>= 3.0.0-preview-18606-34) [D:\a\1\s\src\CORS\CORS.sln]

Doe the order that solutions are compiled need to change now that CORS depends on routing?

JamesNK · 2018-12-07T00:56:03Z

🆙 📅

// @rynowak

pranavkm · 2018-12-07T02:00:25Z

src/CORS/samples/SampleDestination/Startup.cs

+        {
+            app.UseEndpointRouting(routing =>
+            {
+                routing.Map("/allow-origin", HandleRequest).WithCorsPolicy("AllowOrigin");


Could we add an overload that also accepts an actual instance of the policy e.g. WithCorsPolicy(builder => ...)?

Possible. It would require new metadata. The current IEnableCorsAttribute only has a name on it.

pranavkm · 2018-12-07T02:03:39Z

src/CORS/src/Microsoft.AspNetCore.Cors/Infrastructure/ICorsAttribute.cs

+    /// <summary>
+    /// A marker interface which can be used to identify CORS metdata.
+    /// </summary>
+    public interface ICorsAttribute


internal interface?

It needs to be public so if anyone else is searching for CORS metadata they can use the marker interface to find the most significant metadata.

ICorsPolicyMetadata?

natemcmaster · 2018-12-07T18:31:35Z

Doe the order that solutions are compiled need to change now that CORS depends on routing?

Yes, CORS builds as a peer of Routing, which is why you're getting errors. https://github.com/aspnet/AspNetCore/blob/94e317f411ce618a56a64d1061f02b9eaa4998a3/build/buildorder.props#L18-L19

You can either bump the build order. Or, wait until I merge #4449 and rebase on master.

JamesNK · 2018-12-08T02:54:42Z

@natemcmaster

Updated. It now builds on the server but requires an update. When I attempt to update the local deps I get this error:

C:\Development\Source\AspNetCore [jamesnk/cors-endpoint ≡ +0 ~2 -0 !]> ./run.cmd -Path src/CORS/ -LockFile ./korebuild-lock.txt upgrade deps
Using KoreBuild 3.0.0-build-20181206.3
Executing 'C:\Users\James\.dotnet\x64\dotnet.exe msbuild C:\Users\James\.dotnet\buildtools\korebuild\3.0.0-build-20181206.3\KoreBuild.proj -t:UpgradeDependencies -v:n'
Microsoft (R) Build Engine version 15.9.19+g938f3292a0 for .NET Core
Copyright (C) Microsoft Corporation. All rights reserved.

Build started 8/12/2018 3:53:10 PM.
     1>Project "C:\Users\James\.dotnet\buildtools\korebuild\3.0.0-build-20181206.3\KoreBuild.proj" on node 1 (UpgradeDependencies target(s)).
     1>C:\Users\James\.dotnet\buildtools\korebuild\3.0.0-build-20181206.3\modules\KoreBuild.Tasks\module.targets(95,5): error : LineupPackageId was not set. [C:\Users\James\.dotnet\buildtools\korebuild\3.0.0-build-20181206.3\KoreBuild.proj]
     1>Done Building Project "C:\Users\James\.dotnet\buildtools\korebuild\3.0.0-build-20181206.3\KoreBuild.proj" (UpgradeDependencies target(s)) -- FAILED.

Build FAILED.

       "C:\Users\James\.dotnet\buildtools\korebuild\3.0.0-build-20181206.3\KoreBuild.proj" (UpgradeDependencies target) (1) ->
       (UpgradeDependencies target) ->
         C:\Users\James\.dotnet\buildtools\korebuild\3.0.0-build-20181206.3\modules\KoreBuild.Tasks\module.targets(95,5): error : LineupPackageId was not set. [C:\Users\James\.dotnet\buildtools\korebuild\3.0.0-build-20181206.3\KoreBuild.proj]

    0 Warning(s)
    1 Error(s)

...CORS/src/Microsoft.AspNetCore.Cors/Infrastructure/CorsEndpointConventionBuilderExtensions.cs

src/CORS/src/Microsoft.AspNetCore.Cors/Infrastructure/CorsMiddleware.cs

src/CORS/src/Microsoft.AspNetCore.Cors/Infrastructure/ICorsAttribute.cs

src/CORS/src/Microsoft.AspNetCore.Cors/Infrastructure/CorsMiddleware.cs

rynowak

This looks super duper! Only a few minor comments

src/CORS/samples/SampleDestination/Program.cs

pranavkm · 2018-12-11T05:02:21Z

src/CORS/test/FunctionalTests/CorsMiddlewareFunctionalTest.cs

+        [Theory]
+        [InlineData("Startup")]
+        [InlineData("StartupWithoutEndpointRouting")]
+        public async Task RunClientTests(string startup)


Could we make these two separate tests? RunClientTests_WithEndpoint and RunClientTests_WithoutEndpoint? Theories can be annoying to debug.

natemcmaster · 2018-12-12T02:26:08Z

The CORS reorg was merged to master. Let me know if you have trouble rebasing this PR.

JamesNK · 2018-12-12T02:34:26Z

Thanks. Rebasing is not a problem. This isn't urgent so I'm holding off until Middleware.sln builds in VS.

JamesNK · 2018-12-12T04:38:42Z

Middleware.sln is building for me 👍

Rebased

rynowak · 2018-12-13T02:10:52Z

src/Middleware/CORS/src/Infrastructure/ICorsAttribute.cs

+    /// <summary>
+    /// A marker interface which can be used to identify CORS metdata.
+    /// </summary>
+    public interface ICorsAttribute


What about ICorsMetadata? Putting attribute in the name seems silly since it's not an attritube.

Already done locally.

Will look like:

ICorsMetadata

IEnableCorsAttribute

IDisableCorsAttribute

ICorsPolicyMetadata

ICorsPolicyMetadata would have the policy name and a policy on it to support #4460 (comment). Constructor would only allow one.

Thoughts? An alternate is ICorsPolicyMetadata only have the CorsPolicy on it and we leave IEnableCorsAttribute as the way to set the policy name.

I like this.

JamesNK requested review from pranavkm and rynowak December 6, 2018 07:50

dcarr42 reviewed Dec 6, 2018

View reviewed changes

src/CORS/test/Microsoft.AspNetCore.Cors.Test/CorsMiddlewareTests.cs Outdated Show resolved Hide resolved