How to use the new ClaimStrategy in combination with PerTenantAuthentication

[stonesvillage]

Hi! I just started to use your excellent library in a new project where I use asp.net core (3.1) identity and external login (Pipedrive).

The only way to determine the tenant is from the users claims. Using the latest bits of your library gives me the opportunity to use the ClaimStrategy.

services.AddMultiTenant<TenantInfo>()
                .WithConfigurationStore()
                .WithPerTenantAuthentication()
                .WithClaimStrategy("tenant_claim")

But I can't get the ClaimStrategy to work with PerTenantAuthentication. I seems like that PerTenantAuthentication needs to know the tenant before challenging the login but the tenant is resolved after the user has logged in (ClaimStrategy).

Any thoughts of how I can solve this scenario?

Best regards Gunnar

[AndrewTriesToCode]

hi @stonesvillage

Currently it isn't feasible to use the new ClaimsStrategy with per-tenant auth since the former relies on HttpContext.User and the latter sets HttpContext.User but requires a tenant!

It is still possible to create a custom multitenant strategy to accomplish this though. It would extract the claim from the cookie without using HttpContext.User by calling HttpContext.AuthenticateAsync which returns an AuthenticationResult with the claims somewhere inside of it. Then the strategy would return the tenant identifier pulled from the claim so tenant resolution from the tenant store could proceed. I would be careful to limit the logic in the custom strategy to just the tenant claim if possible--you don't want to start pulling other user info at that point and should instead rely on regular authentication to do so.

You'll still need to call UseAuthentication as normal, and the strategy shouldn't interfere with regular authentication middleware because it doesn't set or require HttpContext.User. Also there is no performance penalty to authenticate twice, because UseAuthenticate is smart enough to reuse the cookie info extracted earlier from the multitenant strategy.

I believe this will work just fine with per-tenant authentication.

See if that helps and let me know. I can put together a sample project this weekend.

[stonesvillage]

Hi @AndrewTriesToCode,

I've tried your suggested solution with per-tenant authentication. The problem is that the MultiTenantAuthenticationService tries to add the tenant identifier to the AuthenticationProperties when calling ChallangeAsync. The MultiTenantContext isn't set yet because the tenant is resolved from the AuthenticationResult and the user isn't authenticated yet.

Code where it throws exception:

namespace Finbuckle.MultiTenant.AspNetCore
{
    internal class MultiTenantAuthenticationService<TTenantInfo> : IAuthenticationService
        where TTenantInfo : class, ITenantInfo, new()
    {
        private readonly IAuthenticationService inner;

        public MultiTenantAuthenticationService(IAuthenticationService inner)
        {
            this.inner = inner ?? throw new System.ArgumentNullException(nameof(inner));
        }

        private static void AddTenantIdentiferToProperties(HttpContext context, ref AuthenticationProperties properties)
        {
            // Add tenant identifier to the properties so on the callback we can use it to set the multitenant context.
            var multiTenantContext = context.GetMultiTenantContext<TTenantInfo>();
            if (multiTenantContext.TenantInfo != null) // here it throws exception because multiTenantContext is null
            {
                properties = properties ?? new AuthenticationProperties();
                if(!properties.Items.Keys.Contains(RemoteAuthenticationCallbackStrategy.TenantKey))
                    properties.Items.Add(RemoteAuthenticationCallbackStrategy.TenantKey, multiTenantContext.TenantInfo.Identifier);
            }
        }
...

Maybe there's something I've overlooked, I'll wait to see how you implement it in the sample project you mentioned.

[stonesvillage]

Further I notice that if the tenant isn't resolved before the user is authenticated the .WithPerTenantOptions<CookieAuthenticationOptions> won't be invoked. That means that the Identity.Application/External cookies won't be unique per tenant.

[AndrewTriesToCode]

Hi, yes I’m looking into into these today. This is a use case I definitely want to support

I plan to modify the claims strategy to work as we discussed and to make the per tenant authentication a little smarter one this situation.

On Aug 28, 2020, at 4:33 AM, stonesvillage notifications@github.com wrote:

 Further I notice that if the tenant isn't resolved before the user is authenticated the .WithPerTenantOptions won't be invoked. That means that the Identity.Application/External cookies won't be unique per tenant.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[stonesvillage]

Great! I would be happy to test the result of your work.

Thanks

[AndrewTriesToCode]

Thanks. Can you try it out on this branch? https://github.com/Finbuckle/Finbuckle.MultiTenant/tree/refactor/PerTenantAuthentication-Tweaks

This should allow you to use the existing ClaimsStrategy and it should behave when there is no tenant.

[stonesvillage]

It behaves a bit better now. It still throws in MultiTenantAuthenticationService.AddTenantIdentiferToProperties when multiTenantContext is null though.

The ClaimsStrategy now sets the tenant identifier correct but I think it sets it too late in the pipeline. I don't see how WithPerTenantOptions can be invoked when it should since MultiTenantOptionsFactory has this criteria: multiTenantContextAccessor?.MultiTenantContext?.TenantInfo != null.

I'm wondering if it's possible to solve the ClaimsStrategy at all. Maybe it's better to do the external authentication, grab the tenant from the claims and then redirect to /tenant/ and use the RouteStrategy instead. Then when signing in with Identity the tenant is known and the WithPerTenantOptions should be handled correctly. Or have you been able to setup a working sample with Identity + external login provider where WithPerTenantOptions works?

A minor bug shows up in RemoteAuthenticationCallbackStrategy when there's no state parameter - the logger is null because of the default constructor RemoteAuthenticationCallbackStrategy() That could happen in an OAuth1.0 flow - Twitter for example.

[AndrewTriesToCode]

@stonesvillage thanks for the feedback - I just added a few commits to the branch

I have modified MultiTenantAuthenticationService.AddTenantIdentiferToProperties to better handle a null context. I believe this is a critical fix:

if (multiTenantContext?.TenantInfo != null)

With respect to the ClaimStrategy being too late in your pipeline -- can you tell me more about your pipeline? I have UseMultiTenant before UseAuthentication in my testing. In the last change I modified WithPerTenantAuthentication to use regular options for the claim validation and settting and MultiTenantOptionsFactory still applies regular options/post-options configurations even when there is no tenant.

I don't have a sample with Identity + external login. I'm using the PerTenantAuthenticationSample which does have external login support, but isn't using Identity--but I think it is fundamentally similar. I am seeing the expected results in this application but unfortunately the testing in this area is weak since HttpContext and cookies are hard to unit test against.

Also, thanks for the tip on the bug in RemoteAuthenticationStrategy. I think the default ctor was just there for unit testing and will probably make it internal. I hadn't considered OAuth 1.0 -- it's fundamentally very different right?

[stonesvillage]

@AndrewTriesToCode thanks for your excellent support in this!

The modification to MultiTenantAuthenticationService.AddTenantIdentiferToProperties to better handle null context is exactly the same as I did.

What I think is challenging with ASP.NET Core Identity is that it uses several cookies to handle an external login:

• External cookie: stores information about the external provider. It's deleted when the user is signed in to the application. I've tweaked the Identity logic to delay the deletion of this cookie since it holds the tenant identifier for a while.
• Application cookie: stores the user session after the user is authenticated to the external provider and the local user store.

I want to use WithPerTenantOptions to give the cookies a unique name per tenant. This to only process the cookie for the specific tenant. The External cookie won't be handled by the WithPerTenantOptions (we don't know the tenant identifier yet) but I think that's ok since it's used only temporary and deleted later.

After further testing I think I've managed to solve the Identity specific challenges. Here's how my setup looks at the moment:

services.AddMultiTenant<LicenseServerTenantInfo>()
    .WithConfigurationStore()
    .WithPerTenantAuthentication()
    .WithDelegateStrategy(async context =>
    {
        var httpContext = (HttpContext)context;

        var authenicateResult = await httpContext.AuthenticateAsync("MyExternalOAuth2Provider");
        var identifier = authenicateResult.Principal?.FindFirst("MyClaimWithTenantIdentifier")?.Value;

        return await Task.FromResult(identifier);
    })
    .WithClaimStrategy()
    .WithPerTenantOptions<CookieAuthenticationOptions>((options, tenantInfo) =>
    {
        options.Cookie.Name += $".{tenantInfo.Identifier}";
    });
} 

The DelegateStrategy will look in the External-cookie for a claim because that's the SignInScheme of the external provider. The order of the strategies is important, else the principal will be rejected by the logic in FinbuckleMultiTenantBuilderExtensions.

Again, thanks for excellent support in this matter!

Do you have a release date for v6.0?

[AndrewTriesToCode]

Hi @stonesvillage, sorry for the slow response. I'm trying to get 6.0 out later today.

You are absolutely correct about ClaimStrategy and per-tenant cookie names not working well together-- it's kind of a chicken-and-egg situation--which comes first?

Thinking out loud here: The original version of WithPerTenantAuthentication used separate cookies -- but in the latest version I moved away from that and toward the normal cookie but with a __tenant__ claim and validation. I did this to better support use cases where there a user can belong to multiple tenants -- each one different claim -- and can still login and be validated. (By default only the user's current tenant is added as a claim so to add more claims requires some more code by the library user). The downside is that signing in under a tenant effectively signs out of other tenants.

That being said, per-tenant cookies still has good use cases and is actually what happens by default if the host strategy is used since cookies default to the subdomain of their request. It should work just fine in combination with WithPerTenantAuthentication you just have to do the extra WithPerTenantOptions on the cookie name or path.

Cheers!

[devalot76]

Hi, I'm trying to achive the same result and then stumbled upon this :) So what kind of authentication I'm supposed to implement in order to use the new ClaimStrategy? Can you show me some code, please? Thank you.

[AndrewTriesToCode]

@devalot76 The Claim strategy is compatible with any authentication that results in a `ClaimsPrincipal' -- which most ASP.NET Core authentication does. Cookie authentication as well as JWT based authentication are two examples. Other types of authentication like OpenID Connect actually end up using cookies for the signin cookie so it applies there too.

I will put together a sample project soon for this.

[devalot76]

@AndrewTriesToCode currently I'm using "WithConfigurationStore", "WithRouteStrategy" and "WithPerTenantAuthentication" and it works well. Now I'd like to switch to the claim strategy but it got a NullReference when I try to sign in with PasswordSignInAsync() Can you please tell me what changes should I make in order to get it work? Thank you.

[stonesvillage]

@AndrewTriesToCode Thank's for your dedicated work. When using WithPerTenantAuthentication and WithClaimStrategy one side effect is that the user always is signed out when starting a new browser session, regardless of persisting the cookie or not. This because the WithPerTenantAuthentication will validate the principal and reject it if there's a mismatch between the claim and the current tenant (chicken-and-egg situation again). I'm slowly moving away from WithPerTenantAuthentication to try to find a solution that suits my needs. Please advice if there's something I haven't thought about.

[AndrewTriesToCode]

@stonesvillage

I'm surprised that a new browser sessions prevents signin-- I will need to investigate that further. The cookie should still be sent by the browser, ClaimStrategy could get the tenant, and validation would check the cookie claim tenant matches the current tenant (which it should).

[stonesvillage]

@AndrewTriesToCode

Sorry for not elaborating my previous comment, will try to do that here.

I've tried to use WithPerTenantAuthentication in combination with the new ClaimStrategy. As I understand this setup doesn't work because of the chicken-and-egg situation stated earlier. The ClaimStrategy invokes HttpContext.AuthenticateAsync() which will trigger the event CookieAuthenticationEvents.OnValidatePrincipal in FinbuckleMultiTenantBuilderExtensions.WithPerTenantAuthentication:

// Validate that claimed tenant matches current tenant.
var origOnValidatePrincipal = options.Events.OnValidatePrincipal;
options.Events.OnValidatePrincipal = async context =>
{
    await origOnValidatePrincipal(context);

    if(context.Principal == null)
        return;

    var currentTenant = context.HttpContext.GetMultiTenantContext<TTenantInfo>()?.TenantInfo?.Identifier;

    // If no current tenant and no tenant claim then OK
    if(currentTenant == null && !context.Principal.Claims.Any(c => c.Type == Constants.TenantToken))
        return;

    // Does a tenant claim for the principle match the current tenant?
    if(!context.Principal.Claims.Where(c => c.Type == Constants.TenantToken && String.Equals(c.Value, currentTenant, StringComparison.OrdinalIgnoreCase)).Any())
        context.RejectPrincipal();
};

The variable currentTenant is null since the tenant isn't resolved yet (we're trying to resolve it, that's why we're here) but the claim exists which will result in a rejected principal. So the user logs in but is immediately logged again.

Please advice if there's something in the above reasoning that I've misunderstood.

Regards

[AndrewTriesToCode]

Yeah I recreated the issue yesterday. I will make a separate bug issue for it but I do have a workaround. One thing to note is that if you are only using claim strategy then WithPerTenantAuthentication doesn’t offer much value since it’s job is to ensure auth matches tenant... but claim strategy enforces this almost by definition. Although it claim strategy is used among other strategies this issue is worse.

I have a working fix I will push out this week which essentially lets the claim strategy bypass the validation. If I push a branch do you think you can take a look?

On Sep 16, 2020, at 4:51 AM, stonesvillage notifications@github.com wrote:

 @AndrewTriesToCode

Sorry for not elaborating my previous comment, will try to do that here.

I've tried to use WithPerTenantAuthentication in combination with the new ClaimStrategy. As I understand this setup doesn't work because of the chicken-and-egg situation stated earlier. The ClaimStrategy invokes HttpContext.AuthenticateAsync() which will trigger the event CookieAuthenticationEvents.OnValidatePrincipal in FinbuckleMultiTenantBuilderExtensions.WithPerTenantAuthentication:

// Validate that claimed tenant matches current tenant. var origOnValidatePrincipal = options.Events.OnValidatePrincipal; options.Events.OnValidatePrincipal = async context => { await origOnValidatePrincipal(context);

if(context.Principal == null) return;

var currentTenant = context.HttpContext.GetMultiTenantContext()?.TenantInfo?.Identifier;

// If no current tenant and no tenant claim then OK if(currentTenant == null && !context.Principal.Claims.Any(c => c.Type == Constants.TenantToken)) return;

// Does a tenant claim for the principle match the current tenant? if(!context.Principal.Claims.Where(c => c.Type == Constants.TenantToken && String.Equals(c.Value, currentTenant, StringComparison.OrdinalIgnoreCase)).Any()) context.RejectPrincipal(); }; The variable currentTenant is null since the tenant isn't resolved yet (we're trying to resolve it, that's why we're here) but the claim exists which will result in a rejected principal. So the user logs in but is immediately logged again.

Please advice if there's something in the above reasoning that I've misunderstood.

Regards

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[AndrewTriesToCode]

Created #324 to capture this

[stonesvillage]

@AndrewTriesToCode

I'd be happy to help out with testing.

I agree that when using Claim Strategy as the only strategy the need of matching tenant and auth is almost gone. Is there any other benefits with PerTenantAuthentication when Claim Strategy is the only strategy you think?

To support Claim Strategy and ASP.NET Core Identity I'm using two Claim Strategies. One that checks the external cookie and one for the default cookie. To do this I've made a Claim Strategy that takes auth schemes as argument in addition to the claim type. Maybe this is something that standard lib would benefit of?

[AndrewTriesToCode]

@stonesvillage

Auth schemes as an additional parameter would be great. Would you mind submitting a PR to the fix branch? https://github.com/Finbuckle/Finbuckle.MultiTenant/tree/fix/ClaimStrategy-PerTenantAuthentication-Compat

[stale[bot]]

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.