Backoffice login with Azure Ad broken after upgrade (13.5.1 to 13.5.2)

[Oxygen-cl]

Which Umbraco version are you using? (Please write the exact version, example: 10.1.0)

13.5.2

Bug summary

After applying the security upgrade to 13.5.2 the users get an error on login with Azure AD.

We testet on 2 different solutions getting the same error.

We implemented the login using the guide from the docs: https://docs.umbraco.com/umbraco-cms/13.latest-lts/reference/security/external-login-providers

We get this error after login:

Specifics

No response

Steps to reproduce

Login using Azure AD (Entra ID)
The error shows.

Expected result / actual result

No response

---

This item has been added to our backlog AB#45472

[github-actions]

Hi there @Oxygen-cl!

Firstly, a big thank you for raising this issue. Every piece of feedback we receive helps us to make Umbraco better.

We really appreciate your patience while we wait for our team to have a look at this but we wanted to let you know that we see this and share with you the plan for what comes next.

• We'll assess whether this issue relates to something that has already been fixed in a later version of the release that it has been raised for.
• If it's a bug, is it related to a release that we are actively supporting or is it related to a release that's in the end-of-life or security-only phase?
• We'll replicate the issue to ensure that the problem is as described.
• We'll decide whether the behavior is an issue or if the behavior is intended.

We wish we could work with everyone directly and assess your issue immediately but we're in the fortunate position of having lots of contributions to work with and only a few humans who are able to do it. We are making progress though and in the meantime, we will keep you in the loop and let you know when we have any questions.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

[Oxygen-cl]

After some testing, I can see that this commit was the smoking gun:
1bc5466

If this code is commented out, the external login works again.

[Oxygen-cl]

After some investigation, I found that the issue lies in the GetId method in Umbraco.Extensions.ClaimsIdentityExtensions.

The identity, after the external login from Entra ID, already contains a NameIdentifier claim. The format of this claim is not an integer; therefore, int.Parse will throw an error.

I tried changing int.Parse to int.TryParse. This fixed the error, but I don’t know if this could cause other issues with providers other than Entra ID.

I’ll make a pull request with this fix.

[nikolajlauridsen]

Hi @Oxygen-cl I've been looking into this, but unfortunately, I cannot reproduce it using Azure B2C and OpenIdConnect, I'm assuming that's what you're using?

The whole flow is pretty convoluted but this is what I've tracked down:

1. Initially we hit BackOfficeController.RenderDefaultOrProcessExternalLoginAsync which does a call to _signInManager.GetExternalLoginInfoAsync() here we authenticate using Context.AuthenticateAsync(ExternalAuthenticationType) at this point there is indeed already a NameIdentifier claim, however this is the provider key for the external login. this is then used to create ExternalLoginInfo.
2. This ExternalLoginInfo is then used in ExternalSignInAsync to find the user using the LoginProvider and ProviderKey, this information is mapped in the umbracoExternalLogin table
3. This user is then used in SignInOrTwoFactorAsync to create a new ClaimsIdentitty with the correct ID and parses successfully.

So while in theory I agree that it's not a bad idea to use a TryParse, it doesn't really seem to fix the underlying issue, I'm very curious how you ended up hitting that specific code with the ProviderKey mapped to the NameIdentifier it should be remapped long before that code is hit.

[nikolajlauridsen]

Also, just to clarify so I'm not barking up the wrong tree, are we talking backoffice users, or members?

[Oxygen-cl]

Hi @nikolajlauridsen

Yes, the error is occurring for Backoffice users.

In the latest update (13.5.2), the following code was added to src/Umbraco.Web.Common/Extensions/HttpContextExtensions.cs:

if (result.Succeeded)
{
    httpContext.User = result.Principal;
}

I’ve noticed that when we return from Entra, this is one of the first things that gets called.

While reviewing the pull request, I observed that this commit has not been merged into the v13/contrib branch. If you're using that branch for testing, the error will not appear.

Let me know if you need further clarification.

This is the code we are using to implement the login:

    public class OpenIdConnectBackOfficeExternalLoginProviderOptions : IConfigureNamedOptions<BackOfficeExternalLoginProviderOptions>
    {
        public const string SchemeName = MicrosoftAccountDefaults.AuthenticationScheme;
        public void Configure(string name, BackOfficeExternalLoginProviderOptions options)
        {
            if (name != "Umbraco." + SchemeName)
            {
                return;
            }

            Configure(options);
        }

        public void Configure(BackOfficeExternalLoginProviderOptions options)
        {
            options.ButtonStyle = "btn-primary";
            options.Icon = "fa fa-key";
            options.AutoLinkOptions = new ExternalSignInAutoLinkOptions(
                // must be true for auto-linking to be enabled
                autoLinkExternalAccount: true,

                // Optionally specify default user group, else
                // assign in the OnAutoLinking callback
                // (default is editor)
                defaultUserGroups: new[] { "writer" },

                // Optionally specify the default culture to create
                // the user as. If null it will use the default
                // culture defined in the web.config, or it can
                // be dynamically assigned in the OnAutoLinking
                // callback.
                
                defaultCulture: "da",
                // Optionally you can disable the ability to link/unlink
                // manually from within the back office. Set this to false
                // if you don't want the user to unlink from this external
                // provider.
                allowManualLinking: false
            )
            {
                // Optional callback
                OnAutoLinking = (autoLinkUser, loginInfo) =>
                {
                    // You can customize the user before it's linked.
                    // i.e. Modify the user's groups based on the Claims returned
                    // in the externalLogin info

                    autoLinkUser.Roles = new List<IdentityUserRole<string>>();
                    autoLinkUser.AddRole("sensitiveData");
                    autoLinkUser.AddRole("writer");

                    autoLinkUser.IsApproved = true;
                },
                OnExternalLogin = (user, loginInfo) =>
                {
                    // You can customize the user before it's saved whenever they have
                    // logged in with the external provider.
                    // i.e. Sync the user's name based on the Claims returned
                    // in the externalLogin info
                    
                    return true; //returns a boolean indicating if sign in should continue or not.
                }
            };

            // Optionally you can disable the ability for users
            // to login with a username/password. If this is set
            // to true, it will disable username/password login
            // even if there are other external login providers installed.
            options.DenyLocalLogin = false;

            // Optionally choose to automatically redirect to the
            // external login provider so the user doesn't have
            // to click the login button. This is
            options.AutoRedirectLoginToExternalProvider = false;
        }
    }

public static IUmbracoBuilder ConfigureOpenIdAuthenticationForBackend(this IUmbracoBuilder builder)
{
    var opt = builder.Config.GetSection("AzureAdForBackend").Get<AzureAdOptions>();
        if (opt == null)
            throw new Exception("Missing AzureAdForBackend configuration.");
    

    // Register OpenIdConnectBackOfficeExternalLoginProviderOptions here rather than require it in startup
        builder.Services.ConfigureOptions<OpenIdConnectBackOfficeExternalLoginProviderOptions>();

    builder.AddBackOfficeExternalLogins(logins =>
    {
        logins.AddBackOfficeLogin(
            backOfficeAuthenticationBuilder =>
            {
                backOfficeAuthenticationBuilder.AddOpenIdConnect(
                    // The scheme must be set with this method to work for the back office
                    backOfficeAuthenticationBuilder.SchemeForBackOffice(OpenIdConnectBackOfficeExternalLoginProviderOptions.SchemeName),
                    options =>
                    {
                        options.CallbackPath = "/umbraco-signin-microsoft";
                        // use cookies
                        options.SignInScheme = CookieAuthenticationDefaults.AuthenticationScheme;
                        // pass configured options along
                        options.Authority = opt.Authority;
                        options.ClientId = opt.ClientId;
                        options.ClientSecret = opt.ClientSecret;
                        // Use the authorization code flow
                        options.ResponseType = OpenIdConnectResponseType.Code;
                        options.AuthenticationMethod = OpenIdConnectRedirectBehavior.RedirectGet;
                        // map claims
                        options.TokenValidationParameters.NameClaimType = "name";
                        options.TokenValidationParameters.RoleClaimType = "role";
                        options.TokenValidationParameters.ValidIssuers = GetValidIssuers(opt.ValidTenants);

                        options.RequireHttpsMetadata = true;
                        options.GetClaimsFromUserInfoEndpoint = true;
                        options.SaveTokens = true;
                        options.UsePkce = true;

                        options.Scope.Add("email");
                        options.Scope.Add("openid"); 
                        options.Scope.Add("profile"); 
                        options.Scope.Add("offline_access");
                        options.Events.OnTokenValidated = context =>
                        {
                            var claimsIdentity = context.Principal.Identities.FirstOrDefault();
                            var lang = claimsIdentity.FindFirstValue(ClaimTypes.Locality) ?? "da";
                            var userId = claimsIdentity.FindFirstValue("http://schemas.microsoft.com/identity/claims/objectidentifier");
                            var username = claimsIdentity.FindFirstValue("preferred_username");
                            var name = claimsIdentity.FindFirstValue("name");

                            context.Principal.Identities.FirstOrDefault().AddClaim(new Claim("email", username));
                            context.Principal.Identities.FirstOrDefault().AddClaim(new Claim(ClaimTypes.Email, username));
                            context.Principal.Identities.FirstOrDefault().AddRequiredClaims(userId, username, name, null, null, lang, Guid.NewGuid().ToString(), new List<string>(), new string[]{});
                            
                            return Task.CompletedTask;
                        };
                        options.Events.OnRedirectToIdentityProvider = context =>
                        {
                            context.ProtocolMessage.DomainHint = "customer name";
                            return Task.FromResult(0);
                        };
                    });
            });
    });
    return builder;
}

[nikolajlauridsen]

Thanks for adding the sample code, that helped a ton 😄.

I've had another look at this and this is what I've found.

What happens is that your code calls
context.Principal.Identities.FirstOrDefault().AddRequiredClaims(userId, username, name, null, null, lang, Guid.NewGuid().ToString(), new List<string>(), new string[]{});

What this does is adding all the claims required to be an UmbracoBackOfficeIdentity, however on the OnTokenValidated this is not yet an UmbracoBackOfficeIdentity, and shouldn't be, because you don't have the correct values to place yet here.
This is also why this has broken after the patch you mentioned, because after the patch the principal is set as the user on the context.

This meant that later, when BackofficeSecurity.CurrentUsser is called we try to get the ID by getting the principal (ClaimsIdentity) , on the httpcontext user, and validate it, however since it has all the required claims, because you called AddRequiredClaims, it's technically a valid UmbracoBackOfficeIdentity so we try to use it to get the ID, which of course fails because the values of the actual claims themselves are invalid as you mentioned.

When all that is said, I actually still think your PR makes sense, it should be a TryParse after all, however I'd recommend removing the context.Principal.Identities.FirstOrDefault().AddRequiredClaims(userId, username, name, null, null, lang, Guid.NewGuid().ToString(), new List<string>(), new string[]{});, which will also solve your issue 😄

[nikolajlauridsen]

And a big thank you for both the investigation and the PR! 🎉 H5YR 😄

[nikolajlauridsen]

I've gone ahead and merged #17414 so I'll close this issue 😄