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.32k stars 267 forks source link

MultiTenantException: Error occurred resolving tenant for remote authentication. #208

Closed chernihiv closed 4 years ago

chernihiv commented 4 years ago

I did integration with Idaptive external provider and started to receive MultiTenantException when user has not got access to application (permissions to app of idaptive).

Most likely is't not connected Finbuckle.MultiTenant implementation. It might be either my configuration or implementation of OpenIdConnect on Idaptive side. But, anyway, decided to create an issue.

What I do,

  1. I have simple integration with Idaptive over OpenIDConnect (like I did with OKTA or other providers).
  2. Click my "Login with Idaptive" button, on my site.
  3. Challenge on my oidc scheme is occurred and I can see challenge request in my browser/fiddler with correct request data (including state)
  4. On Idaptive site I successfully complete authentication, but my user does not have access to application I try to authenticate from
  5. Next I see the following request in my browser/fiddler (I suppose Idaptive should display error message on site instead)
Request URL: https://localhost:44380/signin-oidc?error=access_denied&error_description=user%20not%20allowed%20access%20to%20app
Request Method: GET

and receive the following exception

Finbuckle.MultiTenant.MultiTenantException: Error occurred resolving tenant for remote authentication. ---> System.NullReferenceException: Object reference not set to an instance of an object.
   at Finbuckle.MultiTenant.Strategies.RemoteAuthenticationStrategy.GetIdentifierAsync(Object context)
   --- End of inner exception stack trace ---
   at Finbuckle.MultiTenant.Strategies.RemoteAuthenticationStrategy.GetIdentifierAsync(Object context)
   at Finbuckle.MultiTenant.AspNetCore.MultiTenantMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.StaticFiles.StaticFileMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.StaticFiles.StaticFileMiddleware.Invoke(HttpContext context)
   at Swashbuckle.AspNetCore.SwaggerUI.SwaggerUIMiddleware.Invoke(HttpContext httpContext)
   at Swashbuckle.AspNetCore.Swagger.SwaggerMiddleware.Invoke(HttpContext httpContext, ISwaggerProvider swaggerProvider)
   at Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore.MigrationsEndPointMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore.DatabaseErrorPageMiddleware.Invoke(HttpContext httpContext)
   at Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore.DatabaseErrorPageMiddleware.Invoke(HttpContext httpContext)
   at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)

Non of OpenIdConnectEvents events is occurred to handle properly response. When RemoteAuthenticationStrategy.GetIdentifierAsync is triggered, my request does not contain state, that's why properties is null and NullReferenceException exception is occurred.

I tried to fix NullReferenceException in RemoteAuthenticationStrategy, just return null instead tenantIdentifier, therefore

  1. On MessageReceived event of OpenIdConnectEvents is occurred with
    ProtocolMessage.Error = "access_denied"
    ProtocolMessage.Description = "user not allowed access to app"
  2. On RemoteFailure event of OpenIdConnectEvents is occurred with
    Correlation failed.

System info: Finbuckle.MultiTenant 3.2.0 .NET Core 2.2 (latest)

chernihiv commented 4 years ago

In addition,

I have custom implementation of IMultiTenantStrategy interface and in case when step 5. https://localhost:44380/signin-oidc?... is occurred, I do not know current Tenant, so my GetIdentifierAsync returns NULL. I noticed that in such case default RemoteAuthenticationStrategy.GetIdentifierAsync is called, but since request doesn't contain state it leads to NullReference.

I decided hard-code and return proper ID, now RemoteAuthenticationStrategy.GetIdentifierAsync is not called, however, result the same as above:

  1. On MessageReceived event of OpenIdConnectEvents is occurred with
    ProtocolMessage.Error = "access_denied"
    ProtocolMessage.Description = "user not allowed access to app"
  2. On RemoteFailure event of OpenIdConnectEvents is occurred with
    Correlation failed.

Maybe it makes sense to fix NullReference in the following way:

var properties = oAuthOptions?.StateDataFormat.Unprotect(state) ??
                 openIdConnectOptions?.StateDataFormat.Unprotect(state);

if (properties == null)
{
    return null;
}

if (properties.Items.Keys.Contains("tenantIdentifier"))
{
    return properties.Items["tenantIdentifier"] as string;
}
chernihiv commented 4 years ago

My workaround is:

From perspective of Finbuckle.MultiTenant Fixed null reference exception in RemoteAuthenticationStrategy

From perspective of MyProject Overrode MessageReceived of OpenIdConnectEvents as:

public override Task MessageReceived(MessageReceivedContext context)
{
    if (context?.ProtocolMessage?.Error != null)
    {
        context.Response.Redirect($"/error?message={context.ProtocolMessage.Error}&title={context.ProtocolMessage.ErrorDescription}");
        context.HandleResponse();
    }

    return base.MessageReceived(context);
}
AndrewTriesToCode commented 4 years ago

hi @chernihiv thanks for reporting this and providing all the details. I think you have a good approach here. Curious, does Okta or others return the state when access is denied? Any way to get Idaptive to return it? I need to check the OpenId Connect specs on that.

I agree with your fix on the RemoteAuthenticationStrategy -- I am just about to release 5.0, but do you want to submit a PR real quick with your null reference fix? If not I can add it later today.

AndrewTriesToCode commented 4 years ago

@chernihiv

I am going to go ahead and put in a null check and log a warning if that condition occurs. I checked the OpenID Connect spec here and section 3.1.2.6 (and the OAuth 2.0 spec if references) say that a state response parameter is REQUIRED if one was passed. You might want to log this as an issue with Idaptive.