AndrewTriesToCode / MultiTenantIdentityServer4

15 stars 7 forks source link

Using separate DB's per-tenant? #2

Open DavidYon opened 1 year ago

DavidYon commented 1 year ago

Looking over the sample it appears that you're relying a lot on Finbuckle's DbContext filters to partition out tenants in a single database. At least that what I get when I run this: a single DB for Configuration, Operation, and User stores each.

For my use-case the constraints are that the User store be a separate database per-tenant, which is at odds with this sample's design.

I think it would be useful to have a sample that demonstrates this use-case, and I'd be happy to hammer one out and contribute it back to you. Big question is whether you'd be okay with having this sample be adjusted so that it can be configured for one strategy or the other (conditional compilation for example). Or whether you'd prefer it being its own sample codebase.

Thanks for this awesome library!

DavidYon commented 1 year ago

I've got #ifdef'd code (very few mods in fact) which can separately control the following:

  1. Uses per-tenant databases for ASP Identity information (Users, Roles, etc) instead of a single, partitioned database.
  2. Uses a single, unpartitioned database for the Configuration Store (vs the original sample's partitioned database). This for the use case of tenants having separate user-authentication needs but all share the same set of Clients, ApiResources, etc.

I'm not sure whether it's worth setting up Persisted Grants to be multi-tenant. Thoughts?

Oh, I also made a "Seed" launch profile which adds "/seed" to the command line for you. Check out my fork for code details.

Thanks again!

b1tzer0 commented 1 year ago

@DavidYon, this is my exact same use case (the only thing I need multi-tenant is the user store). When you commit it to your repository I would like to take a look to see how you accomplished it.

DavidYon commented 1 year ago

@b1tzer0, I pushed the changes just now. They are on the PerTenantStores branch.

I did take a shortcut and reused the MultiTenant schema because when I was making the change. I'm a little rusty with EF Core Migrations and couldn't remember this weekend how to get a Migration that used the stock ID4 schema.

It's fairly harmless since each database will only ever have one tenant. But now that I've refreshed my memory it wouldn't be all that hard to make it cleaner by adding a Migration for ID4 schema and creating a separate DbContext class for it (that is only per-tenant database rather than single database).

AndrewTriesToCode commented 1 year ago

Thanks guys. There are some challenges with IdentityServer that have prevented me from escalating this example from my personal repo up to Finbuckle and your additions are welcomed.

DavidYon commented 1 year ago

Do you have a list of issues that prevent folding this example into your main repo? Maybe I can help...?

b1tzer0 commented 1 year ago

@DavidYon, I have discovered that the code does not actually work with the latest version of Duende IdentityServer v6. Store options are no longer passed in via constructor parameters.

image

Making some adjustments.

DavidYon commented 1 year ago

@DavidYon, I have discovered that the code does not actually work with the latest version of Duende IdentityServer v6. Store options are no longer passed in via constructor parameters. image

Making some adjustments.

The sample was originally targeted for ID4 not DISv6, so it seems like the least of your issues will be the refactored DI API. I haven't looked closely, but I would think that the database schema for v6 changed from ID4? If so, all the migrations that are in this sample (the original plus my additions) would also be out of sync.

But I've totally been wrong before...

b1tzer0 commented 1 year ago

I am hacking away at it, I will let you know if I make any progress. I keep getting an DI activation error when I try to run the seed or migrations.

It is similar to what is listed here: https://github.com/DuendeSoftware/IdentityServer/issues/723 But, that fix does not seem to work for me.

b1tzer0 commented 1 year ago

So, this is where I am at:

    public class MultiTenantConfigurationDbContextFactory : IDesignTimeDbContextFactory<MultiTenantConfigurationDbContext>
    {
        public MultiTenantConfigurationDbContext CreateDbContext(string[] args)
        {
            var optionsBuilder = new DbContextOptionsBuilder<MultiTenantConfigurationDbContext>();
            optionsBuilder.UseSqlServer("Server=(localdb)\\mssqllocaldb;Database=IdPConfig;Trusted_Connection=True;MultipleActiveResultSets=true");

            var services = new ServiceCollection();
            services.AddIdentityServer()
                .AddConfigurationStore(
                    options =>
                    {
                        options.ConfigureDbContext = b => b.UseSqlServer(
                            "Server=(localdb)\\mssqllocaldb;Database=IdPConfig;Trusted_Connection=True;MultipleActiveResultSets=true",
                            sqlOptions =>
                            {
                                sqlOptions.MigrationsAssembly(
                                    typeof(MultiTenantConfigurationDbContext).Assembly.FullName);
                            });
                    });

            optionsBuilder.UseApplicationServiceProvider(services.BuildServiceProvider());

            var dummyTenant = new TenantInfo();
            return new MultiTenantConfigurationDbContext(dummyTenant, optionsBuilder.Options);
        }
    }

The above gets me past the ConfigurationDbContext error, now I am working on the seeding.

b1tzer0 commented 1 year ago

Looks like I am good to go, the seeding was a typo on my part.

Here is the ContextFactory for ApplicationDbConfig as well:

        public ApplicationDbContext CreateDbContext(string[] args)
        {
            var connectionString = "Server=(localdb)\\mssqllocaldb;Database=IdPUserStore;Trusted_Connection=True;MultipleActiveResultSets=true";

            var optionsBuilder = new DbContextOptionsBuilder<ApplicationDbContext>();
            optionsBuilder.UseSqlServer(connectionString);

            var services = new ServiceCollection();
            services.AddDbContext<ApplicationDbContext>(options =>
            {
                options.UseSqlServer(connectionString, o=>
                {
                    o.MigrationsAssembly(typeof(Program).Assembly.FullName);
                });
            });

            optionsBuilder.UseApplicationServiceProvider(services.BuildServiceProvider());

            var dummyTenant = new TenantInfo()
            {
                ConnectionString = connectionString
            };

            return new ApplicationDbContext(dummyTenant, optionsBuilder.Options);
        }
DavidYon commented 1 year ago

So it was the DbContext factories that caused the problem with Duende v6? The rest of the Multi-Tenant DbContext code is the same?

I guess for the purposes of this sample, it's up to @AndrewTriesToCode as to whether he wants to keep the original ID4 target (which is still free) or move it to Duende (which is not).

As for me, I've got conditionalized code for the PersistedGrants to be multi-tenant. Haven't actually tested operation, but the seeding creates the correct schema.

b1tzer0 commented 1 year ago

Just to clarify, the change in how Duende IdentityServer works is on in v6, in v5 you still had access to the constructor. More information can be found here: https://github.com/DuendeSoftware/IdentityServer/issues/723

But when coming up with a final solution, it is likely that will need to be taken into consideration since Duende IdentityServer is baked into VS2022.

It could be as simple as making two different library add-ons where the user chooses which one they want to use.
Something like Finbuckle.MultiTenant.IdentityServer4.EF (which should work with v5 as well) and Finbuckle.MultiTenant.DuendeIdentityServer6.EF

Or something like that.

b1tzer0 commented 1 year ago

The code I believe is the same, here is my code:

public class MultiTenantConfigurationDbContext : ConfigurationDbContext<MultiTenantConfigurationDbContext>, IMultiTenantDbContext
    {
        public MultiTenantConfigurationDbContext(ITenantInfo tenantInfo, DbContextOptions<MultiTenantConfigurationDbContext> options) : base(options)
        {
            TenantInfo = tenantInfo;
        }

        protected override void OnModelCreating(ModelBuilder builder)
        {
            base.OnModelCreating(builder);

            builder.Entity<Client>().IsMultiTenant();
            //builder.Entity<ClientClaim>().IsMultiTenant();
        }

        public override int SaveChanges(bool acceptAllChangesOnSuccess)
        {
            this.EnforceMultiTenant();
            return base.SaveChanges(acceptAllChangesOnSuccess);
        }

        public override async Task<int> SaveChangesAsync(bool acceptAllChangesOnSuccess,
            CancellationToken cancellationToken = default(CancellationToken))
        {
            this.EnforceMultiTenant();
            return await base.SaveChangesAsync(acceptAllChangesOnSuccess, cancellationToken);
        }

        public ITenantInfo TenantInfo { get; }
        public TenantMismatchMode TenantMismatchMode { get; } = TenantMismatchMode.Throw;
        public TenantNotSetMode TenantNotSetMode { get; } = TenantNotSetMode.Throw;
    }

It will spit out warnings because client is tied to all the other tables as well. So if you want to make every table multi-tenant then you should be fine.

DavidYon commented 1 year ago

I'm went ahead and pushed my PersistedGrants changes. The new factory probably suffers the same problem as the others with regards to v6.

AndrewTriesToCode commented 1 year ago

I'm impressed by by both of your thoughts here. I haven't looked into IS6 yet but I see here some things have definitely changed. I am not against IS6 samples because it is free to use for some use cases.

The biggest issue I recall is that they tend to internally cache options and/or ignore the Options pattern which is where a lot of flexibility in this library stems from. For example they directly inject IdentityServerOptions instead of using OptionsSnapshot<IdentityServerOptions> or similar. It's all throughout the code.

b1tzer0 commented 1 year ago

I have a few more things to clean up, but I will post my code here once I am done. I have a pre-multitenant version here: https://github.com/b1tzer0/IdentityServer_MultiTenant

I still need to get the FIDO2 working again using the Multi-Tenant, then I will upload the latest version.

b1tzer0 commented 1 year ago

While it is very rough, it should be out there now.

DavidYon commented 1 year ago

@AndrewTriesToCode is the caching issue an IS6 issue only? Or is that a problem across the various releases of IS?

Based on @b1tzer0's feedback on handling the changes in IS6, I'm thinking I could adjust my version of the sample to have two build configurations: one for ID4 and one for IS6. They would pull in the different assemblies and set different #define's based on the chosen configuration. Any interest?

Or I could leave it as-is at ID4 and send you a pull request.

b1tzer0 commented 1 year ago

Thoughts on how best to handle navigation from the protected website to IdentityServer site.

IdentityServer has been configured with Routing Strategy to handle everything when I am on the IdentityServer site, however; when I flip over to the Delegate strategy when I am coming from protected site. The protected site is using the AcrValues to pass the Tenant Id over.

It does not appear to correctly navigate to the pages since they are expecting the tenant id in the route. Any thoughts on this?

Thoughts on using sub-domain over routing for this type of setup? I see that Okta and Auth0 both use sub-domain for their configuration.

DavidYon commented 1 year ago

Personally I plan to use Route strategy for both the main site and the ID4 site, mostly because the sites I'm porting to ID4 already use a home-grown Route strategy.

b1tzer0 commented 1 year ago

I guess I am missing some basic understanding.

When you do the routing strategy, how do you pass the tenant information over from the protected site? The authority is the base path as far as I can tell.

For my protected site I have the following in startup:

 .AddOpenIdConnect("oidc", options =>
    {
        options.Authority = "https://localhost:5001/";

        options.ClientId = "interactive";
        options.ClientSecret = "SUPER SECRET VALUE HERE";
        options.ResponseType = "code";

        options.Scope.Clear();
        options.Scope.Add("openid");
        options.Scope.Add("profile");
        options.Scope.Add("tenant");

        options.SaveTokens = true;
        options.GetClaimsFromUserInfoEndpoint = true;
        options.ClaimActions.MapUniqueJsonKey("tenant", "tenant");
        options.Events.OnRedirectToIdentityProvider = ctx =>
        {
            //ctx.ProtocolMessage.AcrValues = $"idp:demoidsrv tenant:{ctx.Request.Host.Value}";
            ctx.ProtocolMessage.AcrValues = $"tenant:tenant-1"; //TODO: Convert to dynamic value
            return Task.FromResult(0);
        };
    });

on IdentityServer I have the following for strategies:

builder.Services
                .AddMultiTenant<TenantInfo>()
                .WithConfigurationStore()
                .WithRouteStrategy()
                .WithDelegateStrategy(
                    async context =>
                    {
                        if (context is not HttpContext httpContext)
                            return null;

                        if (httpContext.Request.Query.TryGetValue("acr_values", out StringValues tenantIdParam))
                            return await Task.FromResult(tenantIdParam.ToString().Split(":")[1]);
                        else
                            return null;
                    })
                .WithPerTenantAuthentication();

Do I need to rewrite the URL somehow?

I feel like I need a custom routing that would take in the AcrValues and alter the URL to put the tenant acr value in the tenant placeholder for the route.

DavidYon commented 1 year ago

Basically you configure your protected site for Route-based tenancy (which Finbuckle can do or there are a lot of manual methods for doing so). When it comes time to deal with the ID4 side of things, you ensure that your protected site includes the tenant in the URI for ID4's Discovery endpoint.

If you look at @AndrewTriesToCode's original sample, you'll see the MvcClient's appsettings.json is as follows:

{
  "Logging": {
    "LogLevel": {
      "Default": "Information",
      "Microsoft": "Warning",
      "Microsoft.Hosting.Lifetime": "Information"
    }
  },
  "AllowedHosts": "*",
  "Finbuckle:MultiTenant:Stores:ConfigurationStore": {
    "Defaults": {
      "ConnectionString": "default_connection_string"
    },
    "Tenants": [
      {
        "Id": "unique-id-0ff4adaf",
        "Identifier": "tenant-1",
        "Name": "Tenant 1 Company Name",
        "OpenIdConnectAuthority": "https://localhost:5001/tenant-1",
        "OpenIdConnectClientId": "mvc-tenant-1",
        "OpenIdConnectClientSecret": "secret"
      },
      {
        "Id": "unique-id-ao41n44",
        "Identifier": "tenant-2",
        "Name": "Name of Tenant 2",
        "OpenIdConnectAuthority": "https://localhost:5001/tenant-2",
        "OpenIdConnectClientId": "mvc-tenant-2",
        "OpenIdConnectClientSecret": "secret"
      }
    ]
  }
}

Note the tenant-1 and tenant-2 in the OpenIdConnectAuthority. With a properly-configured multi-tenant ID4, you'll find that the endpoints returned by discovery and subsequent interactions with ID4 will automagically have the tenant ID built into the URI.

b1tzer0 commented 1 year ago

I think I have got some bad configuration somewhere. When I redirect to ID, I get the following output in my URL:

image

You mentioned the discovery document should have the tenant id in it. When I go to the following URL: https://localhost:5001/tenant-1/.well-known/openid-configuration I get a 404.

DavidYon commented 1 year ago

Hmmm... here's what I get from the build from the most recent version of my repos:

Screen Shot 2022-08-29 at 6 14 22 PM

b1tzer0 commented 1 year ago

Found it, the code was pointing to BasePath instead of Routing.

I had to make some other changes elsewhere but managed to get past the initial set of concerns. Going to continue the rest of the implementation.

DavidYon commented 1 year ago

@AndrewTriesToCode is it appropriate to do a pull request for the enhancements I've made to date? Or are you planning on folding this into your main Finbuckle repos? (or both?)

AndrewTriesToCode commented 1 year ago

Hi I think incorporating as a sample will be nice. I need to review the changes and I'm recovering from a medical procedure at the moment. Go ahead and submit a PR and I'll look into it asap.

DavidYon commented 1 year ago

PR sent, let me know if you have any questions.