OrchardCMS / OrchardCore

Orchard Core is an open-source modular and multi-tenant application framework built with ASP.NET Core, and a content management system (CMS) built on top of that framework.
https://orchardcore.net
BSD 3-Clause "New" or "Revised" License
7.43k stars 2.4k forks source link

Catch-all route interfering with static assets #11386

Closed hieucd04 closed 2 years ago

hieucd04 commented 2 years ago

I have a controller like below in an Orchard Core module:

public class CatchAllController : Controller
{
    [HttpGet("{*slug}", Order = 9999)]
    public async Task<IActionResult> Get(string slug)
    {
        // For testing purpose
        return NotFound();
    }
}

After I enabled the module all css and js files of the /admin page are handled by my CatchAllController. I can still access /admin page but it doesn't have any css or js loaded.

Basically, I want to create a catch-all controller which catches all unhandled requests. Is there anyway to get around this issue or am I doing anything wrong here?

kevinsmgov commented 2 years ago

What's your end game? For instance, I added the Views/Shared/NotFound.cshtml page in my theme to catch 404's and implemented suggested pages result in the output (based on strings in the requested URL). This does catch everything that the pages or controllers in a site are missing.

Skrypt commented 2 years ago

OrchardCore.Diagnostic Views can be overridden in that folder.

hieucd04 commented 2 years ago

@kevinsmgov No, I'm not writing a CatchAllController for handling errors. I'm writing an Orchard Core Module to programmatically create Orchard content types from dotnet classes. Similar to Episerver CMS if you've heard of it before.

The idea is: the CatchAllController will catch the request and then it will use Reflection to activate other controllers based on Request Path (a.k.a Slug), etc.

Hope the above make sense to you but I think it doesn't matter much. I just want to know if there is anyway to prevent CatchAllController from interfering with static assets. :)

ns8482e commented 2 years ago

@hieucd04 it seems your controller has higher priority than static file handler.

Add UseStaticFiles before your catch all controller route in module’s configure method

hieucd04 commented 2 years ago

@ns8482e I just tried it and unfortunately, it didn't work.

It looks to me that my CatchAllController interfere with Orchard Core's internal Static File Middleware and my assumption is somehow Orchard Core's Static File Middleware appears after the MVC Middleware in DotNet Middleware Pipeline which, I believe, should not be the way it is. It is a general rule of thumb that Static File Middleware appears Before MVC Middleware in the middleware pipeline.

ns8482e commented 2 years ago

@hieucd04 It should work - share your module startup code.

hieucd04 commented 2 years ago

@ns8482e Here it is:

public override void Configure(IApplicationBuilder app, IEndpointRouteBuilder routes, IServiceProvider serviceProvider)
{
    app.UseStaticFiles();
    routes.MapAreaControllerRoute(
        name: "Episerver.Cms",
        areaName: "Episerver.Cms",
        pattern: "{*slug}",
        defaults: new { controller = "CatchAll", action = "Get" }
    );
}
ns8482e commented 2 years ago

@hieucd04 Yes it seems controller pattern "{*slug}" is being greedy and Order has no effect, I guess that's how ASP.NET routing works, /cc @jtkech

However I tried without {*slug} and instead of using MapAreaControllerRoute I am using MapFallbackToAreaController - that worked and serves static files as you desire.

    app.UseStaticFiles();
   routes.MapFallbackToAreaController("Get", "CatchAll", "Episerver.Cms");
jtkech commented 2 years ago

Yes, with the new endpoints routing system, UseRouting() and UseEndpoints() wrap the whole, unless you call them explicitly in a specific order, which you can't at the tenant level as it is done for you.

In this context, for controller actions mvc just provides another dedicated endpoints data source where the Order is intended to be used to remove the ambiguity only between controller actions (with more or less specialized patterns).

So, as said by @ns8482e your route is too greedy and MapFallbackToAreaController() looks like to be the best solution as it checks if the request path doesn't contain a file name and it has an high Order value.

That said you can have a similar behavior by using the following.

public class CatchAllController : Controller
{
    [HttpGet("{**slug:nonfile}", Order = int.MaxValue)]
    public Task<IActionResult> Get(string slug)
    {
        // For testing purpose
        return Task.FromResult<IActionResult>(NotFound());
    }
}
hieucd04 commented 2 years ago

Thank you for your help! The nonfile route constraint works for me.