When adding AuthN/Z, check if middleware is already added

[csharpfritz]

Is there an existing issue for this?

• I have searched the existing issues

Describe the bug

Issue #47664 automatically adds AuthN/Z middleware if services are registered. If UseAuthentication or UseAuthorization are called manually in the configuration process, it creates an error scenario indicating issues with Antiforgery.

Can we add a check in UseAuthN/Z to prevent duplicate middleware addition?

https://github.com/dotnet/aspnetcore/blob/main/src/Security/Authentication/Core/src/AuthAppBuilderExtensions.cs#L20
https://github.com/dotnet/aspnetcore/blob/main/src/Security/Authorization/Policy/src/AuthorizationAppBuilderExtensions.cs#L26

Expected Behavior

Executing UseAuthentication / UseAuthorization should not trigger an antiforgery error

Steps To Reproduce

Update the default template to include:

app.UseStaticFiles();
app.UseAntiforgery();

// new lines
app.UseAuthentication();
app.UseAuthorization();

Register and login as a new user and then attempt to logout

Exceptions (if any)

Microsoft.AspNetCore.Antiforgery.AntiforgeryValidationException: The provided antiforgery token was meant for a different claims-based user than the current user.

   at Microsoft.AspNetCore.Antiforgery.DefaultAntiforgery.ValidateTokens(HttpContext httpContext, AntiforgeryTokenSet antiforgeryTokenSet)

   at Microsoft.AspNetCore.Antiforgery.DefaultAntiforgery.ValidateRequestAsync(HttpContext httpContext)

   at Microsoft.AspNetCore.Antiforgery.Internal.AntiforgeryMiddleware.InvokeAwaited(HttpContext context)

.NET Version

8.0.1

Anything else?

No response

[kevinchalet]

Executing UseAuthentication / UseAuthorization should not trigger an antiforgery error

Are you sure it's not just because you added app.UseAuthentication() at the wrong place? The antiforgery stack uses the current identity - resolved from the default authentication scheme - to bind the antiforgery tokens to a specific user: if you add app.UseAuthentication() after app.UseAntiforgery(), that identity may not be available soon enough for things to work correctly.

If anything had to be changed, please be extremely careful: projects like OpenIddict require being able to register the authentication middleware at a different place (typically after CORS). If app.UseAuthentication() blindly no-oped just because it was already added by the default app builder, that would likely break things very badly.

[csharpfritz]

You’ve made EXACTLY my point. Antiforgery calls UseAuth if it hasn’t been called already. If UseAuth is in the wrong place, don’t double execute and perhaps throw an exception so developers can identify the problem. JeffOn Feb 3, 2024, at 16:14, Kévin Chalet ***@***.***> wrote:Re: [dotnet/aspnetcore] When adding AuthN/Z, check if middleware is already added (Issue #53760)Executing UseAuthentication / UseAuthorization should not trigger an antiforgery errorAre you sure it's not just because you added app.UseAuthentication() at the wrong place? The antiforgery stack uses the current identity - resolved from the default authentication scheme - to bind the antiforgery tokens to a specific user: if you add app.UseAuthentication() after app.UseAntiforgery(), that identity may not be available soon enough for things to work correctly.If anything had to be changed, please be extremely careful: projects like OpenIddict require being able to register the authentication middleware at a different place (typically after CORS). If app.UseAuthentication() blindly no-oped just because it was already added by the default app builder, that would likely break things very badly.—Reply to this email directly, view it on GitHub or unsubscribe.You are receiving this email because you authored the thread.Triage notifications on the go with GitHub Mobile for iOS or Android.                                                           

[kevinchalet]

Antiforgery calls UseAuth if it hasn’t been called already.

That's not how it works:

aspnetcore/src/Antiforgery/src/AntiforgeryApplicationBuilderExtensions.cs

Lines 21 to 30 in 8aea1cb

	public static IApplicationBuilder UseAntiforgery(this IApplicationBuilder builder)
	{
	ArgumentNullException.ThrowIfNull(builder);
	builder.VerifyAntiforgeryServicesAreRegistered();
	builder.Properties[AntiforgeryMiddlewareSetKey] = true;
	builder.UseMiddleware<AntiforgeryMiddleware>();
	return builder;
	}

The authentication middleware is added by the host when it detects that middleware hasn't been explicitly registered by the user, otherwise it already no-ops.

[csharpfritz]

You're right @kevinchalet - it does no-op and you can see that change here:
https://github.com/dotnet/aspnetcore/pull/47664/files

.. but it doesn't no-op if you call UseAuth after UseAntiforgery it throws an error on every form submission.

[kevinchalet]

.. but it doesn't no-op if you call UseAuth after UseAntiforgery it throws an error on every form submission.

The web host always no-ops independently of the order you use, but if you decide to explicitly register the authentication/authorization middleware, you become responsible for adding them at the right place.

Try to move app.UseAuthentication()/app.UseAuthorization() before app.UseAntiforgery() to see if it fixes your issue.

[csharpfritz]

Why are you arguing this point?Is there EVER a scenario when UseAuth should be called twice?Is there EVER a parameter to pass in to UseAuth?  I’ll answer this one, no.. those methods don’t take an argument. So if placing it before UseAntiforgery is “correct” and placing it after is “wrong” and omitting it entirely “just works” why don’t we solve this problem and add the same “already registered” check that’s present when you can UseAuth before Antiforgery?JeffOn Feb 4, 2024, at 11:25, Kévin Chalet ***@***.***> wrote:Re: [dotnet/aspnetcore] When adding AuthN/Z, check if middleware is already added (Issue #53760).. but it doesn't no-op if you call UseAuth after UseAntiforgery it throws an error on every form submission.The web host always no-ops independently of the order you use, but if you decide to explicitly register the authentication/authorization middleware, you become responsible for adding them at the right place.Try to move app.UseAuthentication()/app.UseAuthorization() before app.UseAntiforgery() to see if it fixes your issue.—Reply to this email directly, view it on GitHub or unsubscribe.You are receiving this email because you authored the thread.Triage notifications on the go with GitHub Mobile for iOS or Android.                                                           

[kevinchalet]

Is there EVER a scenario when UseAuth should be called twice?Is there EVER a parameter to pass in to UseAuth?

Again, it's not called twice: if you call it explicitly, the web host no-ops and doesn't add it a second time.

I guess you're not familiar with the authentication stack, which would explain why you're so affirmative but there are definitely cases where you want to control the middleware order and call app.UseAuthentication() explicitly.

I already shared one scenario earlier: when you have an IAuthenticationRequestHandler implementation and you want CORS to apply to the generated responses, you must call app.UseCors() and then app.UseAuthentication(), otherwise the authentication middleware added by the web app host is added too early and the CORS middleware doesn't have a chance to apply its headers to the response prepared by your IAuthenticationRequestHandler.

So if placing it before UseAntiforgery is “correct” and placing it after is “wrong” and omitting it entirely “just works” why don’t we solve this problem and add the same “already registered” check that’s present when you can UseAuth before Antiforgery?

There's a simpler approach: updating the web host to auto-register the antiforgery middleware.

You guys wanted this level of magic...

[kevinchalet]

BTW, this concern didn't exist in previous ASP.NET Core versions because the antiforgery middleware wasn't a thing: antiforgery was previously handled at the MVC filter level (so much later in the request processing pipeline). The antiforgery middleware (and the app.UseAntiforgery() extension that comes with it) was only introduced in 8.0.

The requirement to register the authentication middleware before the antiforgery middleware is explicitly listed in the PR that introduced it, BTW:

The anti-forgery middleware must run after authentication and authorization middlewares to avoid inadvertendly reading form data when the user is unauthenticated.

#49233

[csharpfritz]

Ok.. we agree - there is a requirement to register Auth before Antiforgery. I’ll  go back to the description of the issue: can a check be added to prevent duplication?   I’m not suggesting a solution if the improperly located UseAuth statement triggers an exception or is ignored. I’m a developer who wants to make this easier for novice folks to discover and understand.  Implicit ordering of pipeline processes that is only declared in a pull-request on GitHub should be surfaced to make it easier for developers to succeed. JeffOn Feb 4, 2024, at 11:56, Kévin Chalet ***@***.***> wrote:Re: [dotnet/aspnetcore] When adding AuthN/Z, check if middleware is already added (Issue #53760)BTW, this concern didn't exist in previous ASP.NET Core versions because the antiforgery middleware wasn't a thing: antiforgery was previously handled at the MVC filter level (so much later in the request processing pipeline). The antiforgery middleware (and the app.UseAntiforgery() extension that comes with it) was only introduced in 8.0.The requirement to register the authentication middleware before the antiforgery middleware is explicitly listed in the PR that introduced it, BTW:The anti-forgery middleware must run after authentication and authorization middlewares to avoid inadvertendly reading form data when the user is unauthenticated.#49233—Reply to this email directly, view it on GitHub or unsubscribe.You are receiving this email because you authored the thread.Triage notifications on the go with GitHub Mobile for iOS or Android.                                                           

[kevinchalet]

I’ll go back to the description of the issue: can a check be added to prevent duplication?

You're really confusing yourself by calling that a "duplication": there's no duplication at all, it's just a very classical middleware ordering issue here.

I’m not suggesting a solution if the improperly located UseAuth statement triggers an exception or is ignored.

I suggested a potential option that shouldn't be too invasive: adding more magic to the web host to automatically register the antiforgery middleware so you don't have to call app.UseAntiforgery() yourself.

Another potential option: adding an analyzer, but it's not foolproof and could be complicated if the app.Use*() extensions are called from custom extensions (which is a common practice to avoid bloating Program.cs/Startup.cs too much).

I’m a developer who wants to make this easier for novice folks to discover and understand. Implicit ordering of pipeline processes that is only declared in a pull-request on GitHub should be surfaced to make it easier for developers to succeed.

That's the problem with magic: you hide all these things away from the users who have no idea how things work. Middleware ordering has always been a super fundamental thing and I'm not sure hiding it has really helped (but it sure helped eliminate some lines in those super minimal samples 🤣)