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

Help with custom strategy implementation #229

Closed hbermani closed 4 years ago

hbermani commented 4 years ago

Hello there,

First of all I can't be thankful enough for the existence of this library, thank you so much for all the efforts that have gone into this.

I was hoping someone can help me with the custom strategy implementation as the documentation didn't dive into the details of param 2, particularly how I can pass the HTTPContext to the custom strategy . services.AddMultiTenant().WithStrategy<CustomMultiTenancyStrategy>(ServiceLifetime.Transient, *need to pass HTTPContext here but how?!*)

It is probably something simple that I don't have the knowledge for.

As to why I want to do that... Here is the setup I have (I welcome thoughts on better design implementation)

I got it to work with Delegate Strategy but I need to make this transient for each request, unless there is a smart way to get this working in scoped.

So in a nutshell, each http request belongs to a different tenant and they are all being funnelled through the same application, route and host.

Thanks in advance for your help and suggestions.

natelaff commented 4 years ago

Your custom strategy class should use dependency injection to inject IHttpContextAccessor into the constructor. Unless I'm missing something :)

hbermani commented 4 years ago

Your custom strategy class should use dependency injection to inject IHttpContextAccessor into the constructor. Unless I'm missing something :)

Thank you @natelaff for the prompt reply, this seems to work but it's super sluggish, probably because it's creating a new context accessor with every request. Does this look right to you?

 //Multitenancy 
            services.AddHttpContextAccessor();
            services.AddMultiTenant()
                .WithStrategy<CustomMultiTenancyStrategy>(ServiceLifetime.Transient, new HttpContextAccessor())
                .WithConfigurationStore();

And on the strategy end...

public class CustomMultiTenancyStrategy : IMultiTenantStrategy
    {
        private readonly HttpContext _context;
        public CustomMultiTenancyStrategy(HttpContextAccessor context)
        {
            _context = context.HttpContext;
        }

        public Task<string> GetIdentifierAsync(object context)
        {  ... *implementation that ignores the context parameter and just use _context* }

Could it be done better?

natelaff commented 4 years ago

That's what I do (of course I don't use a custom strategy, but use a custom store that calls out to an API and I require the HttpContext within it). I can't speak to why it would be slugish on your end. I utilize memory cache in my custom store though so I don't have to keep hitting my API once I have a tenant hit. maybe you could do something similar.

Oh, I wouldn't new up an HttpContextAccessor though. Just remove that bit and let dependency injection do its magic.

        services.AddHttpContextAccessor();
        services.AddMultiTenant()
            .WithStrategy<CustomMultiTenancyStrategy>(ServiceLifetime.Transient)
            .WithConfigurationStore();
AndrewTriesToCode commented 4 years ago

Hello @hbermani and @natelaff

I'm glad you find the library helpful!

The 2nd parameter in WithStrategy is either a factory method that will build the strategy per scope (transient in your case) or a parameter that will be used as a constructor parameter with normal DI. If it helps check the source here.

If you implemented a custom strategy the GetIdentifier method has an Object parameter named context.

In ASP.NET Core the Finbuckle.MultiTenant middleware passes in the current HttpContext object for this parameter so you shouldn't need to use IHttpContextAccessor or anything like that. All you need to do is cast context to HttpContext in the method like this:

var httpContext = context as HttpContext;
if(httpContext == null)
    //error;

Will that get you where you need? Is the JWT stored somewhere in the HttpContext? Maybe I need to make the documentation better.

natelaff commented 4 years ago

@achandlerwhite it is quite helpful. i'm very new to it, but digging it so far. off topic, i had written a very similar multi-tenancy library for my own purposes, but once i got stuck in per tenant option on the cookie is when i stumbled upon this. the only thing i think this needs either via you, PR or custom strategy is to pull the tenant name from http header ;) easy enough to add.

AndrewTriesToCode commented 4 years ago

@hbermani

Separate response here for your code above-- if you did for some reason need to pass IHttpContextAccessor to the constructor you wouldn't need to pass in new HttpContextAccessor because it would use the default DI instance that AddHttpContextAccessor sets up....

.... and now I see @natelaff beat me to it!

In general IHttpContextAccessor is slow which is why you have to opt in to use it... but Finbuckle.MultiTenant includes it anyway (although I plan to eventually remove it).

hbermani commented 4 years ago

Thank you so much @natelaff And @achandlerwhite.

The casting is working a treat @achandlerwhite and the sluggishness turned out to be Windows Update :( !! I don't need to use HttpContextAccessor now. Having said that the library is still expecting me to pass a second parameter... do I just add new object() instead - the parameter is redundant in my implementation.

 services.AddMultiTenant()
                .WithStrategy<CustomMultiTenancyStrategy>(ServiceLifetime.Transient, new object())
                .WithConfigurationStore();
public class CustomMultiTenancyStrategy : IMultiTenantStrategy
    {

       // private readonly HttpContext _context;
        public CustomMultiTenancyStrategy(object context)
        {
            //_context = context.HttpContext;
        }

        public Task<string> GetIdentifierAsync(object context)
        {

            var _context = context as HttpContext;
            .. rest of implementation

Also in terms of transient and scoped behaviour... let's say I have 5 users (A & B for Tenant 1 and C,D,E for Tenant 2). My understanding is that with a transient implementation every request will be treated separately but with a scoped implementation A&B will use the same dbcontext 1 and C,D,E will use the same dbcontext 2 - which would be more efficient. Have I understood this correctly.

Apologies for the silly questions, I am a noob :( !

hbermani commented 4 years ago

@achandlerwhite it is quite helpful. i'm very new to it, but digging it so far. off topic, i had written a very similar multi-tenancy library for my own purposes, but once i got stuck in per tenant option on the cookie is when i stumbled upon this. the only thing i think this needs either via you, PR or custom strategy is to pull the tenant name from http header ;) easy enough to add.

That's kind of what I am doing with the custom strategy, storing the tenant identity reference for the user in the JWT token that I am getting from the identity provider (Azure AD B2C in my case) and in the custom strategy verify and decode the token to extract the tenant identity reference. I worry if it's just stored in a header/cookie outside a JWT token that it would present a security risk as it can't be trusted. Again- I am new to this so may be overthinking it.

natelaff commented 4 years ago

I wouldn't use header for that personally. In my case I just use the header for API access to know which JWT i should verify and database to attempt to connect to. I pass in x-tenant-name header value on any API call along with the token.

hbermani commented 4 years ago

@natelaff does your API then verify user X is part of x-tenant-name? I am just trying to understand what stops a malicious user of passing a different x-tenant-name alongside their JWT to view other tenant data?

natelaff commented 4 years ago

Actually, re-reading it, you're not doing anything too dissimilar from what I'm doing. Not sure where you're using Finbuckle.. I have three parts (IdentityServer, API and client). Finbuckle lives in my IdentityServer project (gets tenant name from path, and authenticates against ASP.NET Core Identity, then adds tenant name to the token). Then in API, i pass the token and tenant in request header. I validate the authority which it came from (which includes the tenant name). In reality i only use header as a fallback and could be removed. My tenant name actually comes from the claim.

hbermani commented 4 years ago

That makes sense... Instead of Identity Server I have Azure AD B2C . B2C hosts users from different tenants and provides a tenant Id as a claim to the client which then passes the JWT to the application API to verify and authenticate. I am using Finbuckle at the API level.

natelaff commented 4 years ago

then yes, what you're doing is pretty similar to me. finbuckle could live in my API theoretically too, but I use database per tenant... it's easy enough to extract the tenant name from the claim and build my connection string without this package there.

hbermani commented 4 years ago

Indeed, I am just too lazy (and frankly don't trust my skills) and this library is perfect to route the controller action to different/same databases based on the tenant. Ps. I don't know if this remains to be the case but be careful if using EF and only switching connection strings, I remember once watching a video where it said the caching engine of EF can be problematic in terms of caching data from different tenants. So adequate checks for tenancy mismatch will be required.... I will try and fetch the video link, it was for this amazing Nordic dude with a rockstar white hair do :D.

natelaff commented 4 years ago

I'd be curious to see the video, but it may be for previous versions. In ASP.NET Core 2.2+ (i think 2.2) it builds a new DbContextOptionsBuilder. So right in startup when I add the context I can build out a connectionString for each request that comes in. It should always go to the proper place from what's in that access token. They made this a whole lot easier in one of those recent versions.

        services.AddDbContext<AppDbContext>((serviceProvider, options) =>
        {
            string connectionString = <get tenant connection string here>
            options.UseSqlServer(connectionString, o =>
            {
                o.EnableRetryOnFailure();
            });
        });
hbermani commented 4 years ago

I'll try and find it on YouTube for you now.

Btw, I am assuming you are applying the same EF migrations to all tenant databases.. Have you figured out a nice easy way to apply to all other than just manually :-p

natelaff commented 4 years ago

I preprovision databases in an elastic pool using a bacpac. So whatever migration is applied in the bacpac is what each database gets.

Planning to use elastic jobs and apply migration script to all existing databases though. Haven't quite got there, but soon.

hbermani commented 4 years ago

Cool... I will look into that, thanks :D

I know ABP (Asp.net Boiler Plate) has a tool that applies migrations to different DBs, will explore if that's an option too.

hbermani commented 4 years ago

@natelaff took a lot of digging but finally found it, here you go. https://youtu.be/-1cTReZ9lzY

It's 56 minutes but I will watch again tomorrow to refresh my knowledge and extract the specific timestamp where the cashing issue is referenced.