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 Exceptions logged using OnTenantNotResolved #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<T>?.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]

🎉 This issue has been resolved in version 6.13.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

[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

[AndrewTriesToCode]

This will be addressed in v9.0.0 released in November.

[AndrewTriesToCode]

@natelaff

PR #897 attempts to improve options. In short, after each strategy and store logic runs an event is called where you can inspect, react, and even change the results. A final event is called once total resolution is completed.

• OnStrategyResolveCompleted - Called after each strategy has attempted to resolve a tenant identifier. The
  IdentifierFound property will be true if the strategy resolved a tenant identifier. The Identifier property
  contains the resolved tenant identifier and can be changed by the event handler to override the strategy's result.
• OnStoreResolveCompleted - Called after each store has attempted to resolve a tenant. The TenantFound property
  will be true if the store resolved a tenant. The TenantInfo property contains the resolved tenant and can be
  changed by the event handler to override the store's result. A non-null TenantInfo object will stop the resolver
  from trying additional strategies and stores.
• OnTenantResolveCompleted - Called once after a tenant has been resolved. The MultiTenantContext property
  contains the resolved multi-tenant context and can be changed by the event handler to override the resolver's
  result.

[natelaff]

Thanks @AndrewTriesToCode I will give it a shot once you release. I'm having a hard time visualizing it right now and how I would implement why I added events in the first place. Namely, when tenant was not found to just redirect to a very specific 404. it looks like these keep firing in a chain maybe?

[natelaff]

@AndrewTriesToCode ok, finally updated to .NET 9 and v9 of Finbuckle and still have a bit of a disconnect.

Can we get the HttpContext in StoreResolveCompletedContext and/or the attempted identifier in TenantResolveCompletedContext?

Previously I was using the event I had implemented for tenant not found to simply show a custom 404 with the identifier name pass as a query string param. Neither of these events since this change are allowing me to accomplish this easily unless I'm missing something.

In fairness I haven't been able to really get anything to work in a build post v6.12.0 and am having all sorts of other strange issues that I haven't been able to figure yet, but as far as this change goes I still can't get back to what I had.

[natelaff]

And then kind of like I suspected in my comment from November, it just keeps firing and ends up in a loop. I redirect after the tenant isn't resolved but then eventually error on too many redirects.

And here you can see where the lack of identifier is tripping me up by not being able to add it to the query string.

var multiTenantBuilder = builder.Services.AddMultiTenant<CloudAppTenantInfo>(config =>
{
    config.IgnoredIdentifiers = new string[] { "signin-oidc", "privacy", "error", "favicon.ico" }.ToList();

    config.Events = new MultiTenantEvents<CloudAppTenantInfo>
    {
        OnTenantResolveCompleted = async (context) =>
        {
            if (!context.MultiTenantContext.IsResolved)
            {
                string tenantNotFoundRedirectUrl = builder.Configuration["MultiTenant:TenantNotResolvedRedirectUrl"] ?? "/name={Identifier}";
                tenantNotFoundRedirectUrl = tenantNotFoundRedirectUrl?.Replace("{Identifier}", "<i don't have access to this anymore>");
                ((HttpContext)context.Context).Response.Redirect(tenantNotFoundRedirectUrl);
                await Task.FromResult(0);
            }
        }
    };
})

[AndrewTriesToCode]

I'm sorry it's giving you should a touch time. I think what' happening is the tenatnNotFoundRedirectUrl itself doesn't find a tenant so that url redirects back to itself?

I think I need to make the event show if an ignored identifier was hit so your logic will no if it was truly a no-tenant situation or a purposefully ignored one (like in your desired no tenant url). Also it has been suggested that for ASP.NET Core based workflows we add options to ignore certain routes.

Do those ideas make sense to you?

[natelaff]

@AndrewTriesToCode Yeah, I redirect to what is essentially home index route with a parameter of the tenant it attempted to find. i use this as a "we couldn't find 'x'. Claim it today!" signup kind of thing. i don't want this to fire for ignored ones at all, just ones that weren't found in the store. previously i had access to the identifier that was not found.

being able to ignore routes would be a huge help to the one issue, but definitely would still like more context in the not resolved event so i can handle the above scenario.

i got all but this original scenario working in v9. I've just rolled back to 6 for now and it's working as i originally added that event in :)