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.26k stars 261 forks source link

MultiTentantContext is null in HttpContext.Response.OnStarting on sign out #572

Open tnc1997 opened 2 years ago

tnc1997 commented 2 years ago

Firstly, I would like to thank you very much for your work on this package. I am using it combined with IdentityServer in order to support multi-tenancy, however I am experiencing a NullReferenceException caused by the MultiTentantContext being null in the HttpContext.Response.OnStarting function on sign out. I have extended IdentityServer's ConfigurationDbContext to be multi-tenanted and it is the code here on sign out where the MultiTenantConfigurationDbContext is unable to get the TenantInfo because the MultiTentantContext is null. Interestingly, it only seems to be during sign out that the MultiTentantContext is null in the HttpContext.Response.OnStarting function, which makes me wonder if maybe there is something going on with the application's threads during sign out that doesn't play nicely with the AsyncLocal here. I have created a minimal reproducible example here with steps in the readme and I would really appreciate any pointers that you might be able to provide to help overcome this issue.

app.Use(async (context, next) =>
{
    context.Response.OnStarting(() =>
    {
        // This accessor does not have a multi-tenant context during sign-out.
        var accessor = context.RequestServices.GetRequiredService<IMultiTenantContextAccessor<SampleTenantInfo>>();

        return Task.CompletedTask;
    });

    // This accessor has a multi-tenant context during sign-out.
    var accessor = context.RequestServices.GetRequiredService<IMultiTenantContextAccessor<SampleTenantInfo>>();

    await next();
});
AndrewTriesToCode commented 1 year ago

Hi, sorry for the late reply. I was able to clone and reproduce the issue. I think this is a deep one and I'm not 100% what is going on. I think your theory on the AsyncResult getting out of "scope" is probably correct.

Did you find any decent workaround?

One quick workaround might be to cache your tenant info in the db context so it only uses the accessor for the first time -- I'm pretty sure each request to IS will hit the configuration database at least once so when 'OnStarting' is hit the dbcontext will not need to use the accessor -- this assume you are using standard scoped life for the dbcontext.

tnc1997 commented 1 year ago

One quick workaround might be to cache your tenant info in the db context so it only uses the accessor for the first time -- I'm pretty sure each request to IS will hit the configuration database at least once so when 'OnStarting' is hit the dbcontext will not need to use the accessor -- this assume you are using standard scoped life for the dbcontext.

Thank you for your reply, no need to apologise! That's a good idea that I hadn't thought of but unfortunately the scoped MultiTenantConfigurationDbContext isn't accessed during the logout request until it is within the OnStarting function here. We could add a custom middleware to each request that initialises the MultiTenantConfigurationDbContext and accesses the TenantInfo property before the IdentityServer middleware executes to workaround this edge case.

app.Use(async (context, next) =>
{
    var _ = context.RequestServices.GetRequiredService<MultiTenantConfigurationDbContext>().TenantInfo;

    await next();
});
public class MultiTenantConfigurationDbContext : ConfigurationDbContext<MultiTenantConfigurationDbContext>, IMultiTenantDbContext
{
    private ITenantInfo? _tenantInfo;

    public MultiTenantConfigurationDbContext(DbContextOptions<MultiTenantConfigurationDbContext> options, IMultiTenantContextAccessor<ApplicationTenantInfo> multiTenantContextAccessor) : base(options)
    {
        MultiTenantContextAccessor = multiTenantContextAccessor;
    }

    private IMultiTenantContextAccessor<ApplicationTenantInfo> MultiTenantContextAccessor { get; }

    public ITenantInfo TenantInfo => _tenantInfo ??= MultiTenantContextAccessor.MultiTenantContext!.TenantInfo!;

    public TenantMismatchMode TenantMismatchMode => TenantMismatchMode.Throw;

    public TenantNotSetMode TenantNotSetMode => TenantNotSetMode.Throw;
}

We also have an added complication in our solution because we have a transient DistributedMultiTenantCache in-front of the MultiTenantConfigurationDbContext which fails to get the MultiTentantContext due to the same issue.

AndrewTriesToCode commented 1 year ago

Going to label this a bug and consider a few options any logic that happens after the multi tenant middleware runs on the "way out" of the pipeline will not be able to see the tenant and I'd like to think it through.

tnc1997 commented 1 year ago

Thank you very much for your ongoing help with this issue, if there's anything I can do then don't hesitate to let me know!

tnc1997 commented 8 months ago

Going to label this a bug and consider a few options any logic that happens after the multi tenant middleware runs on the "way out" of the pipeline will not be able to see the tenant and I'd like to think it through.

Hi @AndrewTriesToCode, I was just wondering if you've been able to consider any options that might resolve this issue?

I've updated to the latest versions of the Finbuckle.MultiTenant packages (6.12.0) but this issue is still reproducible.

I appreciate that you are likely extremely busy so is there anything that I might be able to do to help resolve this issue?

AndrewTriesToCode commented 8 months ago

Hi, one easy workaround is to stack the multitenant context in the HttpContext items collection during your request so that you can access it there after the normal multitenant context goes out of scope for the request. I'm toying with different implementations of IMultiTenantContext that might build something like that in the future.