U4-11329 - GetExternalAuthenticationOptions gets called when AuthType is Null in Umbraco.Web/editors/BackOfficeController.cs

Created by Marc Goodson 11 May 2018, 14:43:58 Updated by Sebastiaan Janssen 14 Jun 2018, 13:20:46

Tags: PR Gold partner

If you look at the BackOfficeController.cs file at lines 381 - 387 //Here we can check if the provider associated with the request has been configured to allow // new users (auto-linked external accounts). This would never be used with public providers such as // Google, unless you for some reason wanted anybody to be able to access the backend if they have a Google account // .... not likely! var authType = OwinContext.Authentication.GetExternalAuthenticationTypes().FirstOrDefault(x => x.AuthenticationType == loginInfo.Login.LoginProvider); if (authType == null) { Logger.Warn("Could not find external authentication provider registered: " + loginInfo.Login.LoginProvider); }

        var autoLinkOptions = authType.GetExternalAuthenticationOptions();

You can see the authType is retrieved from the OwinContext, and a check is made to see if this is null, and this is logged...... ... however even when the authType IS NULL

the code goes on to call the method authType.GetExternalAuthenticationOptions()

which throws an exception because authType is NULL!!

This has broken our custom Owin Provider implementation for IDAM when upgrading from 7.6 to 7.9 / 7.10

I'm thinking it should only call that method if authType isn't null?

Comments

Marc Goodson 11 May 2018, 14:47:16

PR here: https://github.com/umbraco/Umbraco-CMS/pull/2627


Marc Goodson 16 May 2018, 11:31:18

Actual looking at this further

we have worked around this, or perhaps this is the recommended way, and we just weren't cottoning on and understanding properly

The problem being var authType = OwinContext.Authentication.GetExternalAuthenticationTypes().FirstOrDefault(x => x.AuthenticationType == loginInfo.Login.LoginProvider); if (authType == null)

returns null for our Owin implementation, eg we do not have an Authentication Type matching 'loginInfo.Login.LoginProvider' that Umbraco is expecting to be there

because the provider was previously working, we were quick to think this was the issue in the Core...

...so a potential workaround (or correct implementation depending on how you look at it) is to take responsibility in the Owin implementation to ensure the Claims 'Name Identifier' matches the one Umbraco is configured to look for, which is the name of the authentication type of the implementation prefixed with Umbraco.

so in our custom AuthenticationHandler when preparing the list of Claims, for the NameIdentifier bit

nameIdentifierClaim = new Claim(ClaimTypes.NameIdentifier, email, "Umbraco.xxx-Authentication-Type", "Umbraco.xxx-Authentication-Type");

ensures the name matches and then the authType isn't null!


Shannon Deminick 14 Jun 2018, 04:26:17

It would be great to understand what your "Authentication Handler" is and what it is doing, are you able to provide those details?

That said, i actually have a TODO in a pending commit about this exact bit of code since there's some null checking problems that will cause errors as you've probably seen and I can confirm that the PR fixes this check.

I'll merge that in but i still would like to understand what your auth mechanism is doing.

The point of this check:

var authType = OwinContext.Authentication.GetExternalAuthenticationTypes().FirstOrDefault(x => x.AuthenticationType == loginInfo.Login.LoginProvider);

is to bind Umbraco's ExternalSignInAutoLinkOptions with an OAuth provider and the authentication type must match. In most cases, the authentication type returned from the OAuth endpoint since normally this is configured on startup. Example, when configuring the Google OAuth provider you do this: app.ConfigureBackOfficeGoogleAuth(...) which uses this code: https://github.com/umbraco/UmbracoIdentityExtensions/blob/master/src/App_Start/UmbracoGoogleAuthExtensions.cs#L41 which automatically prefixes "Umbraco." as the authentication type and sets the icon by this method: https://github.com/umbraco/UmbracoIdentityExtensions/blob/master/src/App_Start/UmbracoGoogleAuthExtensions.cs#L55

This is important so that there is no conflict with setting up an OAuth provider for the front-end. BUT there are some providers that don't return the same AuthenticationType as the one configured like the Azure AD one which returns the authentication type as the issuer path which means there's an additional bit of code to make it match, see here: https://github.com/umbraco/UmbracoIdentityExtensions/blob/master/src/App_Start/UmbracoADAuthExtensions.cs#L65

I'll merge the PR and close this but happy to discuss this further here.


Marc Goodson 14 Jun 2018, 11:24:09

Thanks @Shandem, yes the PR is from us, as we upgraded the site and the existing 'working' implementation stopped, and so the PR was a bit of a guess, as it was choking on the null and I was looking at the source code, and just thinking 'that's not right'!

.. but I don't think it was the cause of our problem, as the AuthType shouldn't have been null.

The Authentication Handler the project uses is connecting to IdAM via OWIN, the implementation is inspired by the pattern of your UmbracoIdentityExtensions, and as I say, all was fine until we upgraded to 7.9...

Anyhow your description above is really helpful, our AuthType was missing the umbraco. prefix for the backoffice, hence the method returning null,(which we worked around by setting it in the nameidentifierclaim) but reading through your comments above and looking at our IdamConnectAuthenticationExtensions: public static class IdamConnectAuthenticationExtensions { public static IAppBuilder UseIdamConnectAuthentication(this IAppBuilder app, ApplicationContext appCtx, IdamConnectAuthenticationOptions idamConnectOptions, string style = "btn-openid", string icon = "fa-openid", string caption = "Idam Auth") { if (app == null) { throw new ArgumentNullException("app"); }

        if (idamConnectOptions == null)
        {
            throw new ArgumentNullException("idamConnectOptions");
        }

        return app.SetUpIdamConnectAuthentication(appCtx, idamConnectOptions, style, icon, caption);
    }

    static IAppBuilder SetUpIdamConnectAuthentication(this IAppBuilder app,
        ApplicationContext appCtx,
        IdamConnectAuthenticationOptions idamConnectOptions,
        string style,
        string icon,
        string caption)
    {
        app.Use(typeof(IdamConnectAuthenticationMiddleware), app, idamConnectOptions);

        app.ConfigureUserManagerForUmbracoBackOffice<IdamBackOfficeUserManager, BackOfficeIdentityUser>(
                appCtx,
                (options, context) =>
                {
                    MembershipProviderBase membershipProviderBase = (MembershipProviderBase)MembershipProviderExtensions.GetUsersMembershipProvider();
                    ServiceContext services = appCtx.Services;

                    BackOfficeUserStore userStore = new BackOfficeUserStore(
                        services.UserService,
                        services.EntityService,
                        services.ExternalLoginService,
                        membershipProviderBase.AsUmbracoMembershipProvider());

                    return IdamBackOfficeUserManager.Create(userStore, options, membershipProviderBase);
                });

        idamConnectOptions.ForUmbracoBackOffice(style, icon);
        idamConnectOptions.Description.Caption = caption;

        return app;
    }
}{code}

You can sort of see we're calling the ForUmbracoBackOffice(style, icon) method that triggers the adding of the prefix, but then we don't actually return those idamConnectionOptions anywhere!!

In all your examples in the 'setup' method you return like this, passing in the options! app.UseGoogleAuthentication(googleOptions);

whereas we call app.Use to pass the options... app.Use(typeof(IdamConnectAuthenticationMiddleware), app, idamConnectOptions);

but possibly significantly before ForUmbracoBackOffice is called...

when we have a moment, will get verification on whether this is the cause, or whether there is anything else that is going awry, but on paper, seems like it might be the issue?

This is our AuthenticationOptions class where we set the Auth Type:

public class IdamConnectAuthenticationOptions : AuthenticationOptions { public string OauthPath { get; private set; }

    public string SignInAsAuthenticationType { get; set; }

    public IIdamOAuthProvider IdamOAuthProvider { get; private set; }

    public IdamConnectAuthenticationOptions(IIdamOAuthProvider idamOAuthProvider, string oauthPath)
        : base("Idam-Authentication-Type")
    {
        this.IdamOAuthProvider = idamOAuthProvider;
        this.OauthPath = oauthPath;
        this.SignInAsAuthenticationType = Constants.Security.BackOfficeExternalAuthenticationType;
    }
}


Priority: Normal

Type: Bug

State: Fixed

Assignee:

Difficulty: Normal

Category:

Backwards Compatible: True

Fix Submitted: Pull request

Affected versions: 7.9.6, 7.10.5

Due in version: 7.11.0

Sprint: Sprint 87

Story Points:

Cycle: