Code Monkey home page Code Monkey logo

Comments (21)

brockallen avatar brockallen commented on May 22, 2024

This is a fundamental design flaw of ASP.NET Core's DI system -- there is no official way to have distinct DI systems for different branches in the pipeline. We (and many others) have provided this feedback to Microsoft. They say they're aware of it and will be doing something at some point to allow for this. BTW, they call this "multi-tenancy" support, which is just... an awful.. awful name.

There are others who have shown how to get it to work with various workarounds/hacks. You could look for them and give them a try. If you do, please let us know your results.

Thanks.

from identityserver4.

joeaudette avatar joeaudette commented on May 22, 2024

I am also facing this issue. I had a similar issue with asp.net identity, but the solution I used for that doesn't seem possible with IdentityServer4.

The difference is that asp.net identity uses IOptions to get options used when setting the cookie options and auth scheme from IApplicationBuilder and they use

IOptions<IdentityOptions>

which allowed me to implement a custom implementation of the same so that when the actual options are accessed from the IOptions.Value I could resolve the correct settings based on the request url. even though the IOptions itself is a singleton the Value when accessed can return a different result based on the request.

The way IdentityServerOptions is a singleton and not wrapped in IOptions poses a challenge.

I'm using sasskit for multi-tenancy which enables me to support tenants resolved from the first folder segment of the url (as one possibility), so in startup I wire up the per folder tenant authentication like this:

        app.UseMultitenancy<cloudscribe.Core.Models.SiteSettings>();

        app.UsePerTenant<cloudscribe.Core.Models.SiteSettings>((ctx, builder) =>
        {
            builder.UseCloudscribeCoreDefaultAuthentication(
                loggerFactory,
                multiTenantOptions,
                ctx.Tenant);

        });

in that extension I'm setting up the the auth middleware using the correctly resolved IdentityOptions to set the cookie auth middleware cookieoptions. I wish there was a similar way for me to pass the IdentityServerOptions with app.UseIdentityServer so they could be resolved per request or configured per middleware branch

I'm struggling a bit to figure out how to do something similar with IdentityServer so if anyone has any suggestions please let me know, otherwise if I do figure out a solution I will post it here.

from identityserver4.

brockallen avatar brockallen commented on May 22, 2024

I don't think you can use IOptions<T> in any middleware, since there could be many cookie middlewares in the pipeline, thus many options with different settings. This is what I meant above by the design just being flawed. This is not an IdSvr issue, it's a pipeline architecture flaw. IOW, the right solution is to combine the DI system with the pipeline itself -- in a sense have the DI configured at different levels in the pipeline (e.g. different/additional config in a Map). Each level would inherit the parent DI config. But, no one at Microsoft asked my feedback (or by the time they did, it was too late).

As for IdentityServer -- what's the specific thing you're struggling with? I can't decipher what it is from the above text. If you need different implementations of the various interfaces based on the tenant (via the host name for example) then you'd need to do this by registering the implementations in DI via the callback/factory APIs. So in short something like this pseudo-code:

services.AddTransient<IFoo>(container => {
    var tenant = container.Resolve<IHttpContextAccessor>().HttpContext.GetTenantFromRequest();
    return CreateFooFromTenant(tenant);
});

Again, you can thank Microsoft for this.

from identityserver4.

joeaudette avatar joeaudette commented on May 22, 2024

Thanks for the prompt reply, on a Saturday morning, that is more than I expected!

I'm trying to build/provide integration between my cloudscribe Core project and IdentityServer4. My project would be the user store similar as you have provided for ASP.NET Identity in your quick starts.

My project is a mutli-tenant implementation of ASP.NET Identity, basically implemented with tenant aware UserStore and RoleStore so it has the tenant resolved from the request and it looks up other things like users and roles with the tenantid as a parameter.

My project supports 2 mutually exclusive configuration options for mutli-tenancy, it can be configured to use host name to differentiate tenants or alternatively it can have one root level tenant and other tenants demarcated by the first folder segment of the url. These additional tenants can be added from the UI and it does not require restarting the app for the new tenants to become active because of the way we can branch the middleware with saaskit. But in this case since we do not get cookie isolation from the browser it is important to be able to set a different authscheme and cookie name per tenant, and I have been able to accomplish that. In general my solution does not use different dependencies per tenant, if I really needed different dependencies per tenant in general I probably would just host the sites separately with different startup code. But all the tenants in my scenario are using the same software components.

With the host name tenant scenario I don't think there will be any integration problem because it doesn't matter if the authscheme and cookie name are the same for each tenant since we get cookie isolation in the browser per host.

With the folder tenant scenario is where I face a challenge, because I need different auth scheme and cookie names per folder tenant and I need additional endpoints setup for IdentityServer prefixed with the folder name corresponding to the folder tenants.

I "could" just not support IdentityServer integration with the folder tenant scenario, but I would like to be able to if I can find a solution.

Are you saying that pseudo code callback syntax for DI works? Can I do that to setup multiple singletons of the IdentityServerOptions?

from identityserver4.

brockallen avatar brockallen commented on May 22, 2024

My project is a mutli-tenant implementation of ASP.NET Identity, basically implemented with tenant aware UserStore and RoleStore so it has the tenant resolved from the request and it looks up other things like users and roles with the tenantid as a parameter.

So how are you handling this, given that AspId doesn't support multi-tenancy? Is this what saaskit does for you (I'm not familiar with it).

If you need multiple cookie names, and sasskit is forking the pipeline for you then why not put the cookie middleware into the forked pipeline with different cookie names?

from identityserver4.

joeaudette avatar joeaudette commented on May 22, 2024

yes saaskit provides helpful functionality to branch the middleware per tenant resolved from the request, and I am configuring the cookie middleware per tenant that way. But asp.net identity SignInManager invokes the authentication using auth scheme from IOptions of IdentityOptions so I had to override that per tenant with a custom implementation so that any place where it is used gets the appropriate tenant specific settings.

that is the part I'm trying to figure out with IdentityServer middleware, how to make it use the same settings. maybe I can fork and modify the extension methods for app.AddUseIdentityServer so that I can pass in the needed settings when setting up tenant branches inside the app.UsePerTenant section shown above in the code of my first post

from identityserver4.

brockallen avatar brockallen commented on May 22, 2024

Why not give each branched cookie MW the same scheme, but assign each one different cookie names (based on the tenant)?

with IdSvr, if you register the cookie MW yourself we can use your cookie and won't register our own. just set the AuthenticationScheme on our authentication options and we'll use that.

from identityserver4.

joeaudette avatar joeaudette commented on May 22, 2024

with folder tenants I'm thinking I need different auth scheme per tenant since cookies are shared within the same host I don't want someone to be able to just rename a cookie and be authenticated in another tenant.

I will do some experimenting over the next few days and report back here how it goes. so far I don't see how to set a per tenant auth scheme on Identity Server middleware unless I make my own extension methods. I can only set it to one value from DI

from identityserver4.

joeaudette avatar joeaudette commented on May 22, 2024

if all else fails, the author of saaskit, Ben Foster, also wrote blog post about per tenant dependencies and shows how to do it with structure map. I would prefer a DI framework agnostic solution so I will save that approach as something to try if I can't get it working by other means

from identityserver4.

joeaudette avatar joeaudette commented on May 22, 2024

actually I already have a tenant claim in the cookie that must match the tenant id or the claims principal will be rejected, so maybe I don't need to have per tenant auth scheme and could get away with just having per tenant cookie names for the folder tenant scenario.

from identityserver4.

joeaudette avatar joeaudette commented on May 22, 2024

cookie issues seem to be solved by using the same authscheme name across tenants and just varying the cookie name for folder tenants.

next up is trying to figure out how to add additional endpoints for folder tenants. it looks like I could possibly implement a custom IEndpointRouter to match the endpoint when the tenant folder is part of the url. Does that sound like the right idea?

to do that looks like I'll have to copy/modify your IdentityServerServiceCollectionExtensions and IdentityServerBuilderExtensions. If your method used Services.TryAddSingleton instead of .AddSingleton when adding IEndpointRouter then I wouldn't need to do that I could just add a custom one first before calling services.AddIdentityServer

from identityserver4.

joeaudette avatar joeaudette commented on May 22, 2024

making progress, turned out I did not need to copy/modify IdentityServiceCollectionextensions and IdentityBuilderExtensions, simply registering my custom IEndpointRouter after calling .AddIdentityServer did the trick. That was just a little DI confusion on my part making me think I would need to do that. In order to get the discovery url in the folder tenant to return the urls with the folder tenant segment included, I just had to call .SetBasePath on the HttpContext from within my IEndpointRouter if the current request matches a folder tenant.

Now I think the final piece of the puzzle is that since I'm using a single database for all tenants, I need to implement tenant aware configuration and operational stores so I can segregate and filter the data with a tenantid.
Does that sound about right or are there any other pieces I need to think about?

from identityserver4.

brockallen avatar brockallen commented on May 22, 2024
¯\_(ツ)_/¯

in all seriousness, you're far deeper into what you're doing and i've lost track -- i'm just too focused on the internals of idsvr yet to know if all these internal things are right for replacing.

from identityserver4.

joeaudette avatar joeaudette commented on May 22, 2024

I got the discovery url working for both root tenants and folder tenants by implementing a custom IEndpointRouter and I implemented tenant aware storage for configuration and operational data. The root level tenant is working and I thought things were looking good but I just hit a brick wall trying to get folder tenants working.

The problem for me is in the AuthorizeEndpoint. Even though my IEndpointRouter is returning the AuthorizeEndpoint, the endpoint itself is responding with a 404 because of its internal logic which compares the request path against constants.

So I thought maybe I need to implement a custom AuthorizeEndpoint in order to adjust the patch matching logic to take into account my folder tenant segment. But I can't do that because ir depends on a lot of things inaccessible from my code. Specifically these things are inaccessible:

** IAuthorizeRequestValidator
** IAuthorizeInteractionResponseGenerator
** IAuthorizeResponseGenerator
** context.GetIdentityServerUserAsync()
** AuthorizeResult
** AuthorizeRequestValidationLog
** AuthorizeResponse
** RaiseSuccessfulEndpointEventAsync
** RaiseFailureEndpointEventAsync

I know I cannot ask you to make those things accessible and actually I would rather not go down the path of implementing a custom AuthorizeEndpoint. However I do have an idea I would like to suggest that would solve the problem for me and I wonder if you would consider a pull request?

My idea would be to make an interface with methods to match the path, with a default implementation that has the current logic, which would allow me to plugin custom logic via DI.

    public interface IMatchAuthorizeProtocolRoutePaths
    {
        bool IsAuthorizePath(string requestPath);

        bool IsAuthorizeAfterLoginPath(string requestPath);

        bool IsAuthorizeAfterConsentPath(string requestPath);
    }

    public class DefaultAuthorizeProtocolRouteMatcher : IMatchAuthorizeProtocolRoutePaths
    {
        public bool IsAuthorizePath(string requestPath)
        {
            return (requestPath == Constants.ProtocolRoutePaths.Authorize.EnsureLeadingSlash()));
        }

        public bool IsAuthorizeAfterLoginPath(string requestPath)
        {
            return (requestPath == Constants.ProtocolRoutePaths.AuthorizeAfterLogin.EnsureLeadingSlash()));
        }

        public bool IsAuthorizeAfterConsentPath(string requestPath)
        {
            return (requestPath == Constants.ProtocolRoutePaths.AuthorizeAfterConsent.EnsureLeadingSlash()));
        }
    }

the the AuthorizeEndpoint would take a constructor dependency on IMatchAuthorizeProtocolRoutePaths and its internal logic would change like this:

public async Task<IEndpointResult> ProcessAsync(HttpContext context)
{
    if (context.Request.Method != "GET")
    {
        _logger.LogWarning("Invalid HTTP method for authorize endpoint.");
        return new StatusCodeResult(HttpStatusCode.MethodNotAllowed);
    }

    if (_matcher.IsAuthorizePath(context.Request.Path))
    {
        return await ProcessAuthorizeAsync(context);
    }

    if (_matcher.IsAuthorizeAfterLoginPath(context.Request.Path))
    {
        return await ProcessAuthorizeAfterLoginAsync(context);
    }

    if (_matcher.IsAuthorizeAfterConsentPath(context.Request.Path))
    {
        return await ProcessAuthorizeAfterConsentAsync(context);
    }

    return new StatusCodeResult(HttpStatusCode.NotFound);
}

would you be open to a pull request or making changes like this?

from identityserver4.

brockallen avatar brockallen commented on May 22, 2024

I don't have time right now to read all of this, but I'm going to reopen so I don't lose track of this thread.

from identityserver4.

joeaudette avatar joeaudette commented on May 22, 2024

thanks for re-opening. I'm going to submit a pull request with the minimal changes that enabled me to get things working correctly in the folder based multi tenant scenario of my cloudscribe Core project. I want to say right up front that I have no expectation that the pull request will be merged just hoping it could start a conversation to get something merged that would enable my scenario. The changes in this pull request do enable me to accomplish my goal and I did manage to get things integrated and working. I really wasn't sure what namespace to put the files in and am sure I chose wrong on that but again this is just to get a conversation going.

I've published a nuget of my fork named as cloudscribe.IDS4Fork that I can use for now to be unblocked, but obviously I do not want to have to maintain any fork, this is only intended to unblock me and allow time for you guys to review the issue whenever you have the time to do so without any pressure.

I'm going to move forward now working to add UI to my integration to manage clients and scopes. I think my cloudscribe project combined with IdentityServer4 integration provides a pretty compelling solution to provision and manage IdentityServer4 authority endpoints with user management and soon client and scope management. It is especially easy to provision new folder tenants that provide both the endpoint and the management since no additional dns is needed for each new tenant. Once I get the management of scopes and clients implemented I'll setup a sample app for anyone who is interested in seeing how this combination could be useful to them. Specifically it would solve the request of @edwinf in that tenants could easily be provisioned for host.com/users, host.com/clients, etc with completely separate users, clients, and scopes.

My only hope is that eventually something will make its way into the main IdentityServer4 package to enable this scenario so that I can drop my temporary fork.

Anyway I appreciate you taking the time to read and consider this stuff whenever you can get to it and I will go ahead and submit my pull request next referencing this issue.

from identityserver4.

joeaudette avatar joeaudette commented on May 22, 2024

thought my pull request would auto link, but for reference #382

from identityserver4.

joeaudette avatar joeaudette commented on May 22, 2024

To get my folder tenant scenario working I needed another small change to make cors policy work.

For anyone interested I have a working sample here, pre-populated with 2 folder tenants along with working apis and javascript clients for each tenant. This sample provides a complete solution for standing up a multi-tenant OP Server with full management for tenants, users, roles, claims, clients, and scopes. This sample uses NoDb file system storage which made it easy to pre-populate data to demo. I also have another sample here that uses EFCore but is not pre-populated with any data and doesn't have any clients or apis in the sample solution but is a ready to run multi-tenant OPServer with the same management features.

Again, to get these samples working I had to fork IdentityServer4 which I would rather not do, so hoping someday some changes can be made to facilitate the use of custom folder segments in the endpoint urls. The primary benefit of folder tenants vs host name tenants (which is also supported in my solution) is that with folder tenants one can easily provision new OP Server tenants, each with their own users, roles, claims, clients, and scopes, without need of any new DNS records or additional SSL certificates. Any feedback on this work would be greatly appreciated.

from identityserver4.

joeaudette avatar joeaudette commented on May 22, 2024

I would like to revive this discussion. Previously I made a pull request which is now old and has been closed with the comment:

“Given that this PR is so old, I think it makes sense to close it and follow up on the open issue so we can understand the real ask here.”

So I want to clarify what the ask is. The ask is to make it possible to support custom urls for the endpoints and cors policy which is not possible in the current code because paths are stored as constants that are checked in endpoints such as AuthorizeEndpoint, ie with code like this:

if (context.Request.Path == Constants.ProtocolRoutePaths.Authorize.EnsureLeadingSlash())
{
       return await ProcessAuthorizeAsync(context);
}

Specifically I am supporting a multi-tenant scenario with a root tenant at the root url and additional tenants based on the first folder segment of the url. Since the presence or not of the folder segment is used to resolve the current tenant in my scenario, all of the urls for the given tenant must start with a unique folder segment. So the problem for me is that IDS4 will only accept those hard coded url paths:

public const string Authorize              = "connect/authorize";
public const string AuthorizeAfterConsent  = Authorize + "/consent";
public const string AuthorizeAfterLogin    = Authorize + "/login";
public const string DiscoveryConfiguration = ".well-known/openid-configuration";
public const string DiscoveryWebKeys       = DiscoveryConfiguration + "/jwks";
public const string Token                  = "connect/token";
public const string Revocation             = "connect/revocation";
public const string UserInfo               = "connect/userinfo";
public const string Introspection          = "connect/introspect";
public const string EndSession             = "connect/endsession";
public const string EndSessionCallback     = EndSession + "/callback";
public const string CheckSession           = "connect/checksession";

Whereas I need to be able to prefix those paths {tenant}/connect/authorize, and so on.

In my previous pull request I abstracted that url checking logic into an interface with a default implementation that kept the original logic, but this allowed me to inject a custom implementation to support custom endpoint urls.

public interface IMatchAuthorizeProtocolRoutePaths
{
    bool IsAuthorizePath(string requestPath);

    bool IsAuthorizeAfterLoginPath(string requestPath);

    bool IsAuthorizeAfterConsentPath(string requestPath);
}

public class DefaultAuthorizeProtocolRouteMatcher : IMatchAuthorizeProtocolRoutePaths
{
    public bool IsAuthorizePath(string requestPath)
    {
        return (requestPath == Constants.ProtocolRoutePaths.Authorize.EnsureLeadingSlash());
    }

    public bool IsAuthorizeAfterLoginPath(string requestPath)
    {
        return (requestPath == Constants.ProtocolRoutePaths.AuthorizeAfterLogin.EnsureLeadingSlash());
    }

    public bool IsAuthorizeAfterConsentPath(string requestPath)
    {
        return (requestPath == Constants.ProtocolRoutePaths.AuthorizeAfterConsent.EnsureLeadingSlash());
    }

EndSessionEndpoint also had similar hard coded path validation, which again, I wrapped in an interface with a default implementation keeping the current logic.

public interface IMatchEndSessionProtocolRoutePaths
{
    bool IsEndSessionPath(string requestPath);

    bool IsEndSessionCallbackPath(string requestPath);
}

public class DefaultEndSessionProtocolRouteMatcher : IMatchEndSessionProtocolRoutePaths
{
    public bool IsEndSessionPath(string requestPath)
    {
        return (requestPath == Constants.ProtocolRoutePaths.EndSession.EnsureLeadingSlash());
    }

    public bool IsEndSessionCallbackPath(string requestPath)
    {
        return (requestPath == Constants.ProtocolRoutePaths.EndSessionCallback.EnsureLeadingSlash());
    }       
}

A similar problem exists also in the CorsPolicyProvider where again the allowed paths come from hard coded constants. There is some possibility to add allowed paths in the IdentityServerOptions but it would mean knowing at startup time all the allowed paths, but basically what I need is to support those same paths with a prefix segment and since I can create new tenants at runtime, startup configuration is not a good solution for me. So in my previous pull request I created an interface and a default implementation of the interface to keep the current logic but allowing me to plugin custom logic.

public interface ICorsPathValidator
{
    bool IsPathAllowed(PathString path, CorsOptions options);
}

public class DefaultCorsPathValidator : ICorsPathValidator
{
    public bool IsPathAllowed(PathString path, CorsOptions options)
    {
        return options.CorsPaths.Any(x => path == x);
    }
}

With these changes I was able to achieve my goal but now I am stuck maintaining a fork of IdentityServer4 which has been challenging given the frequent changes and releases. I will keep maintaining it if I must but would really really appreciate it if these changes or something similar could make it upstream so that I can support my scenario without a fork. Note that the changes identified in this post are all the changes I need, those interface just need to be injected into the places where the current code is, ie IMatchAuthorizeProtocolRoutePaths is injected into AuthorizeEndpoint and the logic changes to use the interface like this:

if (_pathMatcher.IsAuthorizePath(context.Request.Path))
 {
     return await ProcessAuthorizeAsync(context);
 }

It is a total of 2 new files and 3 modified files to achieve my goal. I am happy to submit a new pull request against the latest code if there is any indication from you that you would accept it.

from identityserver4.

leastprivilege avatar leastprivilege commented on May 22, 2024

Things have changed in 2.0. Please open a more specific issue if the problem still exists

from identityserver4.

lock avatar lock commented on May 22, 2024

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

from identityserver4.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.