Skip to content
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.

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

Re-run routing for implicit middlewares that require endpoints #49732

Merged
merged 3 commits into from
Aug 2, 2023

Conversation

captainsafia
Copy link
Member

Closes #49654.

Auth-related middlewares set state on the HttpContext to indicate that they have run when an endpoint is available so that the endpoints middleware can warn if endpoints that should've been evaluated by the middleware have not been.

The WebApplicationBuilder registers a set of auth-related middlewares implicitly when their associated services are registered. When the user calls UseRouting explicitly in their application code, the middlewares are registered after and are not able to examine the endpoint set by the routing middleware.

A combination of these factors means that users receive erroneous warnings that auth-related middlewares have not run on endpoints when they have (as in the issue above).

To resolve this, we force routing to run in the middlewares that need to set state in reaction to it.

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer label Jul 30, 2023
@captainsafia captainsafia added area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware labels Jul 30, 2023
@captainsafia captainsafia force-pushed the safia/wab-implicit-middleware-fix branch from ba8b19f to 8a1c68c Compare July 30, 2023 20:38
Copy link
Member

@BrennanConroy BrennanConroy left a comment

Choose a reason for hiding this comment

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

Missing antiforgery tests, and tests with explicit UseAuth* calls

Copy link
Member

@BrennanConroy BrennanConroy left a comment

Choose a reason for hiding this comment

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

Backport worthy?

@@ -2697,7 +2699,7 @@ public void DebugView_UseMiddleware_HasMiddleware()
// 3. Generated delegate name from app.Use(...)
Assert.Collection(debugView.Middleware,
m => Assert.Equal(typeof(MiddlewareWithInterface).FullName, m),
m => Assert.Equal("Microsoft.AspNetCore.Authentication.AuthenticationMiddleware", m),
m => Assert.StartsWith("Microsoft.AspNetCore.Builder.AuthAppBuilderExtensions", m),
Copy link
Member

Choose a reason for hiding this comment

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

Do we care about these values?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure exactly what you mean. I updated the check because I wanted to avoid having the generated name of the delegate-based middleware here. I think it stays mostly consistent but I wanted to avoid typing out the whole thing.

Also, it's a helpful indicator of what codepath you went through (re-routed middleware or plain middleware).

Copy link
Member

Choose a reason for hiding this comment

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

I meant in the case of consumable data, can customers only see this in the debugger? Or should we treat it more like public API and try to have nicer names show up?

@captainsafia
Copy link
Member Author

Backport worthy?

I think so. I'll have to double check that it does repro in .NET 7 (probably) and author a separate bug fix for it. We can't backport directly since we made substantial changes to the WebApplicationBuilder in .NET 8.

@captainsafia captainsafia enabled auto-merge (squash) August 1, 2023 21:36
@captainsafia captainsafia merged commit e231c61 into main Aug 2, 2023
@captainsafia captainsafia deleted the safia/wab-implicit-middleware-fix branch August 2, 2023 00:26
@ghost ghost added this to the 8.0-rc1 milestone Aug 2, 2023
captainsafia added a commit that referenced this pull request Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc
Projects
None yet
4 participants