Add PreRouting and PostRouting pipeline filters

[ronaldbarendse]

Prerequisites

• I have added steps to test this contribution in the description below

This extends the options discussed in issue #10682 and added in PR #10702.

Description

Enabling CORS in Umbraco currently requires you to swap the WithMiddleware() call in Startup.cs to WithCustomMiddleware() and manually register all default Umbraco middleware, just to be able to add UseCors() in the right order (after routing, but before authentication/authorization, see Paul Seals blogpost).

You can already add middleware in different places by using IUmbracoPipelineFilter and the PrePipeline, PostPipeline and Endpoints callbacks, so this PR extends these options with PreRouting and PostRouting callbacks.

You can use PreRouting to add middleware that needs to run after the Umbraco static file middlewares are added, but before the routing middleware. And you can now use PostRouting to add middleware (like CORS) after routing, but before authentication/authorization. See the recommended middleware order.

Testing this PR can be done by adding the following composer:

using Umbraco.Cms.Core.Composing;
using Umbraco.Cms.Web.Common.ApplicationBuilder;

public class CorsComposer : IComposer
{
    public const string AllowAnyOriginPolicyName = nameof(AllowAnyOriginPolicyName);

    public void Compose(IUmbracoBuilder builder)
        => builder.Services
        .AddCors(options => options.AddPolicy(AllowAnyOriginPolicyName, policy => policy.AllowAnyOrigin()))
        .Configure<UmbracoPipelineOptions>(options => options.AddFilter(new UmbracoPipelineFilter("Cors", postRouting: app => app.UseCors())))
        // For testing only
        .Configure<UmbracoPipelineOptions>(options => options.AddFilter(new UmbracoPipelineFilter("CorsTest", endpoints: app => app.UseEndpoints(endpoints =>
        {
            endpoints.MapGet("/echo", context => context.Response.WriteAsync("echo")).RequireCors(AllowAnyOriginPolicyName);
            endpoints.MapGet("/echo2", context => context.Response.WriteAsync("echo2"));
        }))));
}

You should be able to request /echo from any origin, but get an error when trying to fetch /echo2. You can easily test this by pasting the following JavaScript code in your browser console when not on the local Umbraco website (e.g. on umbraco.com):

fetch('https://localhost:44331/echo', {method: 'GET'}).then(result => result.text().then(text => console.log(text)));
fetch('https://localhost:44331/echo2', {method: 'GET'}).then(result => result.text().then(text => console.log(text)));

This should return the following:

An additional bonus is that this doesn't require any changes in your Startup.cs file and also allows packages to enable CORS and configure their own policies. Users that currently use WithCustomMiddleware() will need to add calls to RunPreRouting() and RunPostRouting(), but that's similar to Umbraco adding additional middleware in future versions...

[nul800sebastiaan]

This is great! Pinging @prjseal and @poornimanayar on this one as well for some verification.

@ronaldbarendse The comments says to remove the default implementation on the interface in v13 - can we do that, is it not 2 majors that we need to wait?

[poornimanayar]

This is soooooooo good!!!!

[poornimanayar]

That totally works! And it works for almost everything that WithCustomMiddleware() was put in place for!

[prjseal]

This is great @ronaldbarendse, it simplifies the process so much.

[ronaldbarendse]

Great to see such good feedback 😄

@nul800sebastiaan We don't expect anyone to implement their own IUmbracoPipelineFilter or IUmbracoApplicationBuilderContext, so the default implementations are only really added to make these additions non-breaking. I've added some additional comments to further clean up the code, but those are all more 'nice to have' (and might be more breaking and therefore need to more strictly adhere to the 2 major versions rule) 👍🏻

[ronaldbarendse]

Although removing this constructor won't cause a source breaking change (because the new constructor with optional parameters can be used), it will be a binary breaking change. If you only provide the name parameter, there's no ambiguity either, since the constructor without optional parameters will be used (see overload resolution for named and optional parameters).

We might want to add a comment to remove this constructor in the next major version though:

Suggested change

	{ }
	{
	// TODO: Remove this constructor in Umbraco 13, because the signature is similar to the one with optional parameters
	}

[JasonElkin]

Would you want to mark it as obsolete now as well?

[ronaldbarendse]

There's no need to obsolete this constructor, but it can be safely removed in the next major, because that would only be a source breaking change (since it will then use the constructor with optional parameters instead).

[ronaldbarendse]

Another suggestion for a future major (since it would be a breaking change), is to change the property setters to init, so you can only set the callback when constructing a new instance:

Suggested change

	public Action<IApplicationBuilder>? PrePipeline { get; set; }
	public Action<IApplicationBuilder>? PrePipeline { get; init; }

This does raise the question whether we still want the constructor with all the optional parameters (which is currently already flagged as having too many parameters) or going the complete opposite way and completely removing the property setters...

Any thoughts on which syntax is preferred based on the following example?

// Using parameters will more likely produce longer lines or weird parameter alignment (if you break it over multiple lines)
builder.Services.Configure<UmbracoPipelineOptions>(options => options.AddFilter(new UmbracoPipelineFilter("Cors", postRouting: app => app.UseCors())))

// Using property setter will produce slightly more verbose code, but with nicer formatting
builder.Services.Configure<UmbracoPipelineOptions>(options => options.AddFilter(new UmbracoPipelineFilter("Cors")
{
    PostRouting = app => app.UseCors()
}));

[JasonElkin]

I personally prefer the first one, which means it's probably the wrong choice and the latter is best 😁

[ronaldbarendse]

There's nothing wrong with either choice, so let's leave this for now and allow both 😄

[JasonElkin]

Just a couple of quick things @ronaldbarendse, will test it in the meantime.

[JasonElkin]

I personally prefer the first one, which means it's probably the wrong choice and the latter is best 😁

[JasonElkin]

Would you want to mark it as obsolete now as well?

[JasonElkin]

Super, this works a treat and that composer/fetch combo is a very elegant test - thanks for making it easy @ronaldbarendse!

Will ignore the other questions for now so this can be squeezed into the 12.1 RC.