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.33k stars 265 forks source link

IdentityServer4 - MultiTenancy & Fallback Strategy #290

Closed ashin-s closed 3 years ago

ashin-s commented 4 years ago

Hi..I've been trying to integrate Finbuckle Mutitenancy with IdentityServer4. When a client requests the connect/authorize endpoint of IdentityServer (without the tenant identifier in the route URL), the login URL, that the identity server redirects to, misses the tenant identifier. The custom multitenant strategy gets executed successfully and sets the tenantInfo in the expected way: http://localhost/Identity/Account/Login is used instead of http://localhost/tenant1/Account/Login.

I have provided my own IMultiTenantStrategy implementation to retrieve the tenant identifier from route first, if not, IdentityServer's request parameters (custom parameter, "tenant" added from the client that requests the token.). The MultiTenantMiddleware succeeds in retrieving the tenant identifier using my customs strategy and sets the required tenantInfo, store, strategy etc. But then the connect/authorize endpoint redirects to http://localhost/Identity/Account/Login skipping the tenant template.

However, if I use the FallbackStrategy, the authorize endpoint redirects the browser to http://localhost/tenant1/Account/Login, provided tenant1 as used in the FallbackStrategy's static identifier. Here's the code snippet:

IdentityServer's ConfigureServices:

    public void ConfigureServices(IServiceCollection services)
    {
        services.AddDbContext<ApplicationDbContext>();
        services.AddDefaultIdentity<IdentityUser>()
            .AddEntityFrameworkStores<ApplicationDbContext>();

        services.AddControllersWithViews().AddRazorRuntimeCompilation();
        services.AddRazorPages(options =>
        {
            // Since we are using the route multitenant strategy we must add the
            // route parameter to the Pages conventions used by Identity.
            options.Conventions.AddAreaFolderRouteModelConvention("Identity", "/Account", model =>
            {
                foreach (var selector in model.Selectors)
                {
                    selector.AttributeRouteModel.Template =
                        AttributeRouteModel.CombineTemplates("{__tenant__}", selector.AttributeRouteModel.Template);
                }
            });
        });

        services.DecorateService<LinkGenerator, AmbientValueLinkGenerator>(new List<string> { "__tenant__" });

        services.AddMultiTenant()
                .WithStrategy<CustomMultiTenantStrategy>(ServiceLifetime.Transient, "__tenant__") // Looks at route first then specific to idserver4's request
                .WithFallbackStrategy("tenant1")
                .WithConfigurationStore()
                .WithRemoteAuthentication()
                .WithPerTenantOptions<AuthenticationOptions>((options, tenantInfo) =>
                {
                    // Allow each tenant to have a different default challenge scheme.
                    if (tenantInfo.Items.TryGetValue("ChallengeScheme", out object challengeScheme))
                    {
                        options.DefaultChallengeScheme = (string)challengeScheme;
                        // options.DefaultSignOutScheme = (string)challengeScheme;
                    }
                })
                .WithPerTenantOptions<CookieAuthenticationOptions>((options, tenantInfo) =>
                {
                    // Since we are using the route strategy configure each tenant
                    // to have a different cookie name and adjust the paths.
                    options.Cookie.Path = $"/{tenantInfo.Identifier}";
                    options.Cookie.Name = $"{tenantInfo.Id}_authentication";
                    options.LoginPath = $"{options.Cookie.Path}{options.LoginPath}";
                    options.LogoutPath = $"{options.Cookie.Path}{options.LogoutPath}";
                });

        // configure identity server with in-memory stores, keys, clients and scopes
        services.AddIdentityServer()
            .AddDeveloperSigningCredential()
            .AddInMemoryPersistedGrants()
            .AddInMemoryIdentityResources(ResourceStore.GetIdentityResources())
            .AddInMemoryApiResources(ResourceStore.GetApiResources())
            .AddInMemoryClients(ClientStore.Get())
            .AddAspNetIdentity<IdentityUser>()
            .AddProfileService<CustomProfileService>();
    }

IdentityServer's Configure method:

    public void Configure(IApplicationBuilder app, IWebHostEnvironment env)
    {
        if (env.IsDevelopment())
        {
            app.UseDeveloperExceptionPage();
            app.UseDatabaseErrorPage();
        }
        else
        {
            app.UseExceptionHandler("/Home/Error");
        }

        app.UseStaticFiles();
        app.UseRouting();
        app.UseMultiTenant();
        app.UseIdentityServer();
        app.UseAuthorization();

        app.UseEndpoints(endpoints =>
        {
            endpoints.MapControllerRoute("default", "{__tenant__=}/{controller=Home}/{action=Index}");
            endpoints.MapRazorPages();
        });
    }

Custom Multitenant Strategy:

public class CustomMultiTenantStrategy : IMultiTenantStrategy
{
    internal readonly string tenantParam;
    private readonly IIdentityServerInteractionService _interactionService;
    public CustomMultiTenantStrategy(string tenantParam, IIdentityServerInteractionService interactionService)
    {
        if (string.IsNullOrWhiteSpace(tenantParam))
        {
            throw new ArgumentException($"\"{nameof(tenantParam)}\" must not be null or whitespace", nameof(tenantParam));
        }
        this.tenantParam = tenantParam;
        this._interactionService = interactionService;
    }

    public async Task<string> GetIdentifierAsync(object context)
    {
        if (!(context is HttpContext))
            throw new MultiTenantException(null,
                new ArgumentException($"\"{nameof(context)}\" type must be of type HttpContext", nameof(context)));

        var httpContext = context as HttpContext;

        object identifier = null;
        httpContext.Request.RouteValues.TryGetValue(tenantParam, out identifier);

        // Fallback to read from the request query params
        if (identifier == null)
        {
            httpContext.Request.Query.TryGetValue("tenant", out StringValues tenant);

            if (tenant.Count > 0)
            {
                identifier = tenant.ToString();
            }
            else // authorize/connect request would go in here
            {
                if (httpContext.Request.Query.ContainsKey("returnUrl"))
                {
                    var returnUrl = httpContext.Request.Query["returnUrl"];

                    var authContext = await this._interactionService.GetAuthorizationContextAsync(returnUrl);

                    identifier = authContext.Parameters["tenant"];
                }
            }
        }

        return await Task.FromResult(identifier as string);
    }
 }

MVC Client's Configure Services:

        services.AddAuthentication(options =>
        {
            options.DefaultScheme = "Cookies";
            options.DefaultChallengeScheme = "oidc";
        })
            .AddCookie("Cookies")
            .AddOpenIdConnect("oidc", options =>
            {
                options.Authority = "http://localhost";
                options.RequireHttpsMetadata = false;
                options.ClientId = "test-ui";
                options.ClientSecret = "XXXXX";
                options.ResponseType = "code id_token";
                options.Scope.Add("custom-profile");
                options.Scope.Add("offline_access");
                options.SaveTokens = true;

                options.Events = new OpenIdConnectEvents
                {
                    OnRedirectToIdentityProvider = (ctx) =>
                    {
                        var tenant = ctx.HttpContext.Request.IsHttps ? "tenant1" : "tenant2"; // Temporary way to swtich between two tenants - the same mvc client is used by different tenants, in prod, it would be identified based on subdomain.
                        ctx.ProtocolMessage.Parameters.Add("tenant", tenant );
                        ctx.ProtocolMessage.AcrValues = $"tenant:{tenant}";

                        return Task.CompletedTask;
                    }
                };
            });

        services.AddControllersWithViews();

If I remove .WithFallbackStrategy("tenant1") from ConfigureServices, then the identity server would redirect to http://localhost/Identity/Account/Login resulting in a 404. I would want the tenant name to be dynamically set based on the parameter that the MVC client passes via request parameters. Could you please tell me if I am doing something wrong here?

AndrewTriesToCode commented 4 years ago

hi @ashin-s thanks for the detailed question. I'm taking a look at it now.

I suspect it might be how Identiy Server is building the login url to redirect-- They might not be checking the CookieAuthenticationOptions to see what the URL should be. I've also run into problems with Identity Sever and the route strategy if you want a separate OATH authority for each tenant -- it is (hard?) coded to go to a path /.well-known/... without subbing in the tenant. That might not be a problem in your user case, but just wanted to let you know.

AndrewTriesToCode commented 4 years ago

I'm going to dig into this because I need to write some documentation on this. Is your project based off this approach but with Finbuckle.MultiTenant added in?

ashin-s commented 4 years ago

Hi @AndrewTriesToCode thanks for responding! You might be right about the cookie authentication options, but I was wondering how it would work with the fallback strategy in place - I even tried to extend the FallbackStrategy to use my custom strategy but did not help! Appreciate a lot for looking into this and you're right, my project is based off the approach that you have mentioned. Thanks in advance!

AndrewTriesToCode commented 4 years ago

@ashin-s

In looking further into this I think I can simplify your setup. Idenity Server 4 already supports sending the tenant via acr_values and exposes it in the AuthorizationContext Tenant property.

To set the acr_value hook into the OnRedirectToIdentityProvider and set AcrValues to tenant:my_tenant_identifier like this:

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

                    options.ClientId = "mvc";
                    options.ClientSecret = "49C1A7E1-0C79-4A89-A3D6-A37998FB86B0";
                    options.ResponseType = "code";
                    options.Events.OnRedirectToIdentityProvider = async c =>
                    {
                        // see link for details on how tenant is passed with this acr_value parameter: 
                        // https://identityserver4.readthedocs.io/en/latest/reference/interactionservice.html#authorizationrequest
                        c.ProtocolMessage.AcrValues = $"tenant:{c.Request.HttpContext.GetMultiTenantContext().TenantInfo.Identifier}";
                    };
                    options.SaveTokens = true;
                });

Then in your custom strategy you can simplify it to this:

public class CustomMultiTenantStrategy : IMultiTenantStrategy
{
    private readonly IIdentityServerInteractionService _interactionService;

    public CustomMultiTenantStrategy(IIdentityServerInteractionService interactionService)
    {
        this._interactionService = interactionService;
    }

    public async Task<string> GetIdentifierAsync(object context)
    {
        // the authorization context "Tenant" property will reflect the acr_value passed in by the client
        var identifier = await _interactionService.GetAuthorizationContextAsync().Tenant;

        return identifier;
    }
 }

I think with this simpler approach this may be easier to troubleshoot. Let me know how that goes. Tomorrow I will specifically look at how the redirect to the login page works. I suspect its just the ASP.NET Core standard authorization redirect so it should work OK in theory.

ashin-s commented 4 years ago

Thanks for the suggestion! I have followed it but it does not seem to fix the issue..However, there is a slight difference in the way I setup the project (contrary to what I told you earlier, sorry about that!) In fact, I did not use the Quickstart template of Identity Server4. These are the steps that I have followed:

  1. Created Asp.Net Core Web Application (netcore 3.1) using Visual Studio 2019
  2. Chose Individual Accounts under Authentication in the above step
  3. Added IdentityServer4.AspNetIdentity (4.0.0)
  4. Added FinBuckle.MultiTenant (5.0.4)
  5. Added an MVC project (Client) with no FinBuckle capability just to make sure the identityserver implementation works fine.

I have attached the sample project where the issue is repro-able. And Thanks a lot for helping out! :) Idsrv.MultiTenant.zip

AndrewTriesToCode commented 4 years ago

@ashin-s

Just wanted you to know I'm still looking at this. I did find a bug that might be related. I will put out a new preview release later today and I plan to test for the issue then.

ashin-s commented 4 years ago

Thanks @AndrewTriesToCode for working on this. I'm looking forward to it!

AndrewTriesToCode commented 4 years ago

@ashin-s

I found a few things.

First, I was wrong in my suggestion to use IIdentityServerInteractionService in your strategy-- is hasn't been setup yet because our middleware runs before IdentityServers.

Second, despite this post IdentityServer is not respecting the cookie options login path. It is using the IdentityServerOptions instead. I am going to post an issue with them to get clarification on this.

Third, WithPerTenantOptions doesn't work with IdentityServerOptions the way they use it... I'm working on a workaround.

I'm not sure why I thought I had it working in my test project -- I plan to take a 2nd look at that too.

ashin-s commented 4 years ago

Thanks for the update, @AndrewTriesToCode! You're right, the IIdentityServerInterationService cannot be used - I have used a custom strategy to identify the tenant - available in the sample project I shared.

I took a look at the post - does that mean, we'd have to implement a custom cookie handler? At the moment, I have customized Identity Server to enable multi tenancy without using FinBuckle as I was running out of time..The user-agent issue (with multi-tenant login) is something I need to tackle still.. So I look forward to hearing from you!

Appreciate the effort that you put in!

AndrewTriesToCode commented 4 years ago

In your sample project I updated to the finbuckle release 6 preview. I think if you wanted to try a method not relying on the route it would work.

For the initial request to the authorize endpoint just check your tenant query param or the acr_values query param of present. Then at the login page the IIdentityServiceAuthenticationService approach will work. You’ll know the tenant without needing to use the route or url. Tenants will have the same login url but will have the correct tenant info. Im not sure about the other Identity pages like account management. There is a session based strategy that might work well for that.

I’d say do what works for you now but stay tuned as I try to figure out a recommended approach.

ashin-s commented 4 years ago

Thanks @AndrewTriesToCode..Sorry for getting back to you a little late..Could you please share that sample project with me, otherwise, please let me know if I could get it working by just upgrading to release 6 preview. Like I said, the user agent issue with multi-tenant login is something I still need to address..

alexmaie commented 4 years ago

@AndrewTriesToCode any news regarding the "recommended approach"

AndrewTriesToCode commented 4 years ago

@alexmaie I'm still working on it. It has prompted me to rethink some aspects of per tenant authentication.

I have a working sample that works for the authorize, signin, and authorize callback endpoints. This works for a toy sample application. However for a more realistic app which exposes user account management like Identity offers I need to think it through more. Thanks for asking the hard questions!

I will apply what I have so far to the sample app your provided and share how it goes.

ashin-s commented 4 years ago

Thanks...I'm looking forward to it! :) However, I have a workaround in place for the "same user-agent - multiple tenant login" issue by rejecting the token from the client application (if the token belongs to a different tenant) and forcing another login. But that's not so elegant. Any help from your end would really be great!

AndrewTriesToCode commented 4 years ago

@ashin-s Actually your solution is what I have landed on as well. I am basically adding tenant claim to the signed in user and on followup requests I validate that the resolved tenant matches the user principal's tenant claim and reject if they don't. It's not ideal but I think the same user-agent scenario is not prevalent in practice and at worst the prior tenant will just have to re-sign in -- and with SS0 even that won't be much of a problem.

Another thing this will support is if you added more tenant claims to the user in your own code than matching any 1 of them would pass user principle validation -- effectively giving an easy way to support an admin login to multiple tenants. I plan to also build in a custom claim check such as role = admin to bypass this validation so a sys admin could be valid in any tenant.

And a final thought is that this tenant claim itself could be used as a strategy to detect the tenant rather than a route or other means.

Edit: i'm actually planning this behavior for 6.0, so actually a move away from separate cookies per tenant as the recommended per-tenant authentication method. Of course per-tenant cookies can still be done if needed...

adamsproul commented 4 years ago

@ashin-s Actually your solution is what I have landed on as well. I am basically adding tenant claim to the signed in user and on followup requests I validate that the resolved tenant matches the user principal's tenant claim and reject if they don't. It's not ideal but I think the same user-agent scenario is not prevalent in practice and at worst the prior tenant will just have to re-sign in -- and with SS0 even that won't be much of a problem.

Another thing this will support is if you added more tenant claims to the user in your own code than matching any 1 of them would pass user principle validation -- effectively giving an easy way to support an admin login to multiple tenants. I plan to also build in a custom claim check such as role = admin to bypass this validation so a sys admin could be valid in any tenant.

And a final thought is that this tenant claim itself could be used as a strategy to detect the tenant rather than a route or other means.

Edit: i'm actually planning this behavior for 6.0, so actually a move away from separate cookies per tenant as the recommended per-tenant authentication method. Of course per-tenant cookies can still be done if needed...

@AndrewTriesToCode / @ashin-s - do you have a demo project with this approach now 6.0 is out? I started going down the Per-Tenant authentication route until I read the latest comment in this issue

ferreira-guilherme commented 4 years ago

@AndrewTriesToCode / @ashin-s Were you able to solve this problem? I am starting the development of a multi-tenant application and I would like to use Finbuckle

AndrewTriesToCode commented 4 years ago

@ferreira-guilherme Yes, I was able to get it working. Using Identity Server 4 in conjunction with ASP.NET Core Identity / EFCore it's very doable. I will eventually do a blog post on it with the details.

imanov commented 4 years ago

Hi, do you have a link to a blog post or sample, where you implemented this? I tried implementing the solution by adding a tenant claim to the logged in user, but it is never populated back in the current principle claims for the following requests?

AndrewTriesToCode commented 4 years ago

@imanov I don't have anything ready to share at the moment.

Can you post a link to your repo? I'll take a look.

imanov commented 4 years ago

@AndrewTriesToCode , Unfortunately I had to move to route strategy in order to move forward, but using claims and route/query/form for anonymous requests is still the preferred way for me.

Another issue I experienced is that when a user is logged in for tenant1 and I change the route to tenant2, there is no check that the current principal tenant is different than the current context tenant. Is there a standard way for checking that or I should implement manual check like an action filter.

natelaff commented 3 years ago

This thread takes me back to the same challenges I had with IS4 and multi-tenancy. Even with the tenant in acr_values, the behavior for determining the tenant was too unpredictable because it doesn't flow through all endpoints. I submitted an issue about this but didn't get anywhere with that team.

To fix all of these issues, as well as what @imanov most recently hit was to configure IdentityServer to use the tenant name in the path. This solved easily resolving the tenant, as well as having the issuer contain the tenant, so when switching between them they were authenticated properly.

This is a fairly easy change to make with a custom IEndpointRouter, then in my API I add a tenant aware issuer to ValidIssuers validation so my JWTs are always properly validated against the tenant I should be connecting to.

AndrewTriesToCode commented 3 years ago

@imanov I'm sorry I wasn't able to help but I hope the route based method is working on for you. @natelaff has some good insight there.

With respect to preventing tenant login sessions from leaking WithPerTenantAuthentication should prevent it--are you using that?

imanov commented 3 years ago

@AndrewTriesToCode I have problems using WithPerTenantAuthentication. We serve IdentityServer endpoints from the same project and when I enable WithPerTenantAuthentication, the redirects to /connect/authorize/callback are redirected to login screen. I was not able to make this flow work normally with WithPerTenantAuthentication.

natelaff commented 3 years ago

Oh, can't help much with IS in the same project as your client. I can see where WithPerTenantAuthentication would have potential to conflict with that though. I'm not sure how you'd go about handling that.

Personally, unless otherwise avoidable, I'd consider moving IS to its own project which will make life much easier. Separation of concerns aside, consider also with IS4 essentially reaching end of life shortly and moving to commercial, you may find yourself in a situation where you want to update your web project to .NET 6 but may not be able to because IS4 only supports <= .NET 5.

Just one mans opinion!

imanov commented 3 years ago

@natelaff Totally agree on your points, but our goal was to implement fast and working demo/POC with common UI template for IS screens and admin screens. At the end we'll have several projects using the same identity and IS is going to be a separate service for sure. Currently my workaround for this is a custom middleware which checks the current tenant context ( from route strategy) against the tenant of the current user. This requires a couple of requests to user and tenant stores on each request and wanted to avoid it by having this info stored in the claims initially. But if I had it in a claim correctly, I'd have used the claims strategy instead of route :)

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

jaltenbernd commented 3 years ago

@ashin-s Were you able to get IdentityServer4 to work with Finbuckle? If so, do you have a Repo with an example?