dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.44k stars 10.02k forks source link

Consider updating AuthorizationMiddleware and CorsMiddleware to only set state in HttpContext if auth / cors metadata is present #38342

Open pranavkm opened 2 years ago

pranavkm commented 2 years ago

Endpoint routing only checks for state in HttpContext if the route has Auth or CORs metadata:

https://github.com/dotnet/aspnetcore/blob/main/src/Http/Routing/src/EndpointMiddleware.cs#L38-L48

However, the middlewares update HttpContext.Items any time it sees an endpoint:

We might benefit from slight improvements for endpoints with Auth or CORS metadata by updating these middlewares to only update context when they know EndpointMiddleware is going to observe it i.e.

 if (endpoint?..GetMetadata<ICorsMetadata>() is not null)
{
        // EndpointRoutingMiddleware uses this flag to check if the CORS middleware processed CORS metadata on the endpoint.
        // The CORS middleware can only make this claim if it observes an actual endpoint.
        context.Items[CorsMiddlewareWithEndpointInvokedKey] = CorsMiddlewareWithEndpointInvokedValue;
}
pranavkm commented 2 years ago

/cc @davidfowl

Tratcher commented 2 years ago

Which is more expensive on average, GetMetadata<ICorsMetadata>(), or context.Items[CorsMiddlewareWithEndpointInvokedKey] = CorsMiddlewareWithEndpointInvokedValue;?

pranavkm commented 2 years ago

We know we are calling GetMetadata at least once now (in the routing middleware), so it calling it twice is likely come to out a bit better since the lookup gets cached: https://github.com/dotnet/aspnetcore/blob/main/src/Http/Http.Abstractions/src/Routing/EndpointMetadataCollection.cs#L75-L85. But yeah, I filed an issue versus making a code change because we probably need to verify the benefits of this change.

adityamandaleeka commented 2 years ago

Triage: the change here is fairly small, but we need to have a good performance test to measure the impact.

ghost commented 2 years ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

Kahbazi commented 2 years ago

While we are at it, how about also checking RouteOptions.SuppressCheckForUnhandledSecurityMetadata?