OnTenantNotResolved

[DanieleSky]

Hi, I'm using your library in a ASP.NET Core project. This is the configuration:

services
  .AddMultiTenant<T>(config =>
  {
      config.Events.OnTenantNotResolved = context =>
      {
          throw new BadHttpRequestException($"Tenant {context.Identifier} not found");
      };
  })
  .WithDistributedCacheStore()
  .WithHeaderStrategy("x-tenant-id");

I've two questions:

1. How I can return a 404 response when a tenant is not resolved instead to throw an exception? I think that OnTenantNotResolved should be a func in Task<T> so I could return an IActionResult. I tried with this solution https://github.com/Finbuckle/Finbuckle.MultiTenant/issues/554 but don't work because the line Task.CompleteTask don't interrupt the execution of the code and I receive this exception Unable to resolve service for type 'Finbuckle.MultiTenant.ITenantInfo' while attempting to activate when i use the dependency injection in my controller public MyController(IHttpClientFactory httpClientFactory, ITenantInfo tenantInfo)

2. Why when the header "x-tenant-id" is missing the event OnTenantNotResolved don't raise? Seems the strategy is ignored. Is right?

Thanks!

[AndrewTriesToCode]

Hi, for (1) I plan to build in support for this, but for now the easiest method is to place a simple middleware right after the UseMultiTenant<T> middleware that checks for null tenant and returns a 404 error. Something like:

app.Use(async (context, next) => { if(context.GetMultiTenantContext?.TenantInfo is null) { // build and return a 404 response... }

await next.Invoke();

});

For (2) I'm not sure, I'll need to test that. I would want it to raise in that situation.

[AndrewTriesToCode]

:tada: This issue has been resolved in version 6.13.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket:

[natelaff]

This change broke my code and I was the original contributor of the OnTenantNotResolved haha. Now it seems to be firing way too early?

@AndrewTriesToCode Actually what seems to be happening is its no longer respecting IgnoredIdentifiers before that OnTenantNotResolved event fires. Which is odd because I don't see anything in the commit. But between 6.12 and 6.13 that broke.

[AndrewTriesToCode]

Hi @natelaff glad to hear from you.

The change in implementation here was intended to fire the not found event if no tenant identifier was found -- prior it was only firing this event if an identifier was found but no tenant resolved from store. I'll take a closer look when I can but feel free to submit any fixes as you see fit. Maybe a distinction between identifier found but no tenant resolved and no identifier found would make sense but I still think the primary use case in general is tenant not found -- either way. Of course it should also respect explicitly ignore identifiers.

[natelaff]

@AndrewTriesToCode i haven't had time to wrap my head around the change. I looked at it, but it looks like that event system changed even a little bit since I submitted it. But to me, the purpose of the TenantNotFound event was it identified there was a tenant (unless explicitly ignored by identifiers) but couldn't find it in the store.

Seems to me routing or whatever other pattern you're using should take care of the tenant identifier null, and not this event. Kind of seems like someone shoe horned this in and broke its original intent.

[AndrewTriesToCode]

I’m leaning toward a TenantIdentifierNotFound event to complement the existing one and revert that one back to how it was. What do you think? Maybe event rename the current one to TenantInfoNotFound to be explicit.

[natelaff]

Not being in the group of people that experienced this issue, it's hard to say. Granted, that event was added by me for a very specific purpose so it always just worked for me haha -- until now.

From my understanding of the issue that influenced this change, it's almost like they want TenantIdentifierNotFound event? Not even TenantInfo, because info would be resolved past that point? Seems like they want this to trigger earlier than that?

EDIT: Ah, yes we're thinking the exact same thing!

[goforebroke]

@AndrewTriesToCode

Hi Andrew,

After reading through this, the current implementation of "OnTenantNotResolved" checks if there is an identifier found based on your strategy(s). You are considering reverting "OnTenantNotResolved" to its original implementation, which fires if a tenant is not found in the tenant store(s). You are considering creating another event, possibly "TenantIdentifierNotFound", that that implements the current logic found in "OnTenantNotResolved"? If so, I think this would be a nice addition to the Finbuckle.

[AndrewTriesToCode]

Thanks guys. I'll work up a PR soon. Working on getting the new options functionality out first, then this.

[AHansen-Planview]

Hi,

I can understand the importance of these events being within the resolver, see the issues showcasing the ability to add to a cache on a successful resolve. However, the not found events seem to mainly be useful from a web perspective. This is quite apparent by the event's passing through a context, which most people are using to hack away at routing. Obviously this isn't always the case, so I'm not proposing the removal of them, however I think the middleware needs a major rebuffing to support more complicated scenarios. I hope we can agree that the resolver shouldn't be altering much to do with web infrastructure, even if it was done through an event. Not the resolvers fault, but again the middleware is where these things should be occurring.

For instance, from an API perspective, the following ends up throwing errors because the response has been started, yet the middleware calls next and kestrel handlers fail to reread the stream.

this.AddMultiTenant<TenantInfo>(config =>
{
    config.Events = new MultiTenantEvents
    {
        OnTenantNotResolved = async notResolvedContext =>
        {
            if (notResolvedContext.Context is HttpContext httpContext)
            {
                var problemDetailsFactory = httpContext.RequestServices.GetRequiredService<ProblemDetailsFactory>();

                var problemDetails = problemDetailsFactory.CreateProblemDetails(
                    httpContext,
                    StatusCodes.Status400BadRequest,
                    detail: "Tenant not resolved.");

                var problemDetailsService = httpContext.RequestServices.GetRequiredService<IProblemDetailsService>();

                await problemDetailsService.TryWriteAsync(new ProblemDetailsContext
                {
                    HttpContext = httpContext,
                    ProblemDetails = problemDetails
                });
            }
        }
    };
});

I proceeded with the following, however it's opinionated toward problem details, which we wouldn't want. Just throwing it out there as an example.

/// <summary>
/// Middleware for resolving the MultiTenantContext and storing it in HttpContext.
/// </summary>
public class MultiTenantMiddleware
{
    private readonly ProblemDetailsFactory _factory;
    private readonly IProblemDetailsService _service;
    private readonly RequestDelegate _next;

    public MultiTenantMiddleware(ProblemDetailsFactory factory, IProblemDetailsService service, RequestDelegate next)
    {
        _factory = factory;
        _service = service;
        _next = next;
    }

    public async Task Invoke(HttpContext context)
    {
        var requestPath = context.Request.Path.Value;

        // TODO: Move ignored routes to configurable options to allow customization
        if (!string.IsNullOrEmpty(requestPath) && requestPath.StartsWith("/swagger", StringComparison.OrdinalIgnoreCase))
        {
            await _next(context);

            return;
        }

        var resolver = context.RequestServices.GetRequiredService<ITenantResolver>();

        var multiTenantContext = await resolver.ResolveAsync(context);

        if (!multiTenantContext.IsResolved)
        {
            var problemDetails = _factory.CreateProblemDetails(
                context,
                StatusCodes.Status400BadRequest,
                detail: "Tenant not resolved."); // TODO: Move message to configuration options to allow customization

            await _service.TryWriteAsync(new ProblemDetailsContext
            {
                HttpContext = context,
                ProblemDetails = problemDetails
            });

            return;
        }

        var mtcSetter = context.RequestServices.GetRequiredService<IMultiTenantContextSetter>();
        mtcSetter.MultiTenantContext = multiTenantContext;

        context.Items[typeof(IMultiTenantContext)] = multiTenantContext;

        await _next(context);
    }
}

[natelaff]

I tried to dig into this today to help out but the original scope of what I added this feature for and the changes that were made that broke it are still unclear to me as to why. So I had to leave it. Hopefully @AndrewTriesToCode can get this one straightened back out soon :D

[AndrewTriesToCode]

I’m planning some updates soon as we approach .NET 9