Finbuckle / Finbuckle.MultiTenant

Finbuckle.MultiTenant is an open-source multitenancy middleware library for .NET. It enables tenant resolution, per-tenant app behavior, and per-tenant data isolation.
https://www.finbuckle.com/multitenant
Apache License 2.0
1.3k stars 265 forks source link

Routing Strategy and exception pages #228

Closed markgould closed 4 years ago

markgould commented 4 years ago

Hi Andrew,

Are you aware of anyway for the routing strategy to work properly when using exception pages?

We're finding that when we use a routing strategy (.NET Core 3.1) the route values are empty on the error pages.

Some sample code:

   app.UseStatusCodePages(context =>
            {
                var httpContext = context.HttpContext;
                var tenant = httpContext.GetRouteValue("tenant"); //Tenant is always null, route values are empty
                return Task.CompletedTask;
            });

The only way around this we've found is to switch to using the base path strategy, and then we can access the tenant just fine (using the IMultiTenantContextAccessor)

Thanks,

Mark

AndrewTriesToCode commented 4 years ago

@markgould

Hello, question, what is the status code in these cases? I suspect that if it a 404 or similar then routing failed which would explain why the tenant is null. Would it work in your case to register the routing strategy followed by the base path strategy? That if routing fails it will try the base path next.

markgould commented 4 years ago

Yeah, 404 was one case. It also seemed to happen with unhandled exceptions when using:

app.UseExceptionHandler("/Error");

I made sure that the Error page has a route template when I setup the page model conventions (Using Razor Pages)

If I'm going to fallback to base path, is there any reason to not just use the base path strategy only?

Thanks for always responding so quickly!

AndrewTriesToCode commented 4 years ago

Hm, I'm going to try to reproduce it. Do you mind sharing your Configure and ConfigureServices methods?

markgould commented 4 years ago

FinbuckleRepro.zip Here's a repro! It does appear to work with the unhandled exception page (if you navigate to TenantA/ExceptionPage you can see the behavior working)

If you try and go to an invalid page, such as TenantA/blah you'll see there is no route values as you suspected.

AndrewTriesToCode commented 4 years ago

Thanks for the example. I'm taking a look. I noticed the UseStatusCodePage is at the end of the pipeline rather than the beginning of the pipeline which microsoft recommends. Still seems to work ok though.

I stepped into the source code for Finbuckle and yeah for a status code response no route matched so it kind of makes sense.

For the exception page, I set the "Microsoft" logging level to default and noticed that when it executes the /Error page it somehow ends up the route pattern for Index and displays that page instead of the actual error page. If you set the UseExceptionPage parameters to /TenantA/Error it does show the expected error page (obviously you wouldn't want to always show Tenant A's error page though...)

I'm not sure why /Error is redirecting to index, but I'm still exploring.

AndrewTriesToCode commented 4 years ago

Thinking on it some more I realized its clear that just like "/" routes to the default Index Page, "/anything" routes to the default Index Page when it has a route template of /{routeparam}.

So in this case the "/Error" url for the exception middleware is matching the Index page route.

What is interesting is that the exception middleware doesn't redirect-- it just reruns the app pipeline on the current requests for the new "/Error" url. I believe in doing that it is hitting the multitenant middleware which detects that TenantA was already set from the first pipeline run prior to exception and leaves that as-is.

If it instead were to run it would try to find a tenant named "Error" and come up null, but I don't observer that happening.

Then the endpoint middlware displays the Index page for Tenant A.

Ideas:

  1. You could try setting the url in the UseExceptionPage to "/dummy/Error" which would properly route to the error page and as mentioned above the tenant from prior to the exception would apply (instead of it trying to use dummy)

  2. In your Pages routing convention class, check the Page being modified in the foreach loop and if the page is "/Error" then skip modifying its attribute routing. I think this will create a higher priority route match that selects the Error page over the Index page and the correct tenant should already have been set prior to the exception even though the route for this page doesn't include the tenant route paramter.

Does that help?

markgould commented 4 years ago

Yeah, I actually have 2 in our solution and it appears to work correctly. It doesn't seem like there's much way for the status pages to work, as like you said, the routing has failed.

I think we'll just use the BasePathStrategy knowing that the routing has that limitation.

Thanks for looking into this!

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.