umbraco / Umbraco-CMS

Umbraco is a free and open source .NET content management system helping you deliver delightful digital experiences.
https://umbraco.com
Other
4.49k stars 2.69k forks source link

Azure AD integration in backoffice throws a ArgumentNullException #13100

Closed frederiktoft closed 1 year ago

frederiktoft commented 2 years ago

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

10.2.1

Bug summary

I have added Azure AD B2C login in an Umbraco 10.2.1 installation, I followed this documentation here: https://our.umbraco.com/Documentation/Reference/Security/Authenticate-with-Active-Directory/ Only difference is I have also added options.AuthorizationEndPoint and options.TokenEndPoint. Otherwise its the same, I have provided the code snippet further below. The reason for adding AutorizationEndpoint and TokenEndPoint is because the application is setup in our Azure AD as single tenant. The documentation is for multi tenant setups (which IMO is not very good practice for the backoffice in some cases) where these endpoints are not needed. The documentation should be expanded with this.

Either way, the issue is the authorisation process works, but once you get to the backoffice its just a blank screen. So I checked the umbracotracelog to see if I could find anything, and one exception appeared every time I used azure ad login, and not a local user:

System.ArgumentNullException: Value cannot be null. (Parameter 'culture')
   at Umbraco.Cms.Core.Services.LocalizedTextService.GetAllStoredValues(CultureInfo culture)
   at Umbraco.Cms.Web.BackOffice.Controllers.BackOfficeController.LocalizedText(String culture)
   at lambda_method102(Closure , Object )
   at Microsoft.AspNetCore.Mvc.Infrastructure.ActionMethodExecutor.AwaitableObjectResultExecutor.Execute(IActionResultTypeMapper mapper, ObjectMethodExecutor executor, Object controller, Object[] arguments)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeActionMethodAsync>g__Awaited|12_0(ControllerActionInvoker invoker, ValueTask`1 actionResultValueTask)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeNextActionFilterAsync>g__Awaited|10_0(ControllerActionInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Rethrow(ActionExecutedContextSealed context)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.InvokeInnerFilterAsync()
--- End of stack trace from previous location ---
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeNextResourceFilter>g__Awaited|25_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Rethrow(ResourceExecutedContextSealed context)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeFilterPipelineAsync()
--- End of stack trace from previous location ---
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeAsync>g__Awaited|17_0(ResourceInvoker invoker, Task task, IDisposable scope)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeAsync>g__Awaited|17_0(ResourceInvoker invoker, Task task, IDisposable scope)
   at Microsoft.AspNetCore.Routing.EndpointMiddleware.<Invoke>g__AwaitRequestTask|6_0(Endpoint endpoint, Task requestTask, ILogger logger)
   at Umbraco.Cms.Web.Common.Middleware.BasicAuthenticationMiddleware.InvokeAsync(HttpContext context, RequestDelegate next)
   at Microsoft.AspNetCore.Builder.UseMiddlewareExtensions.<>c__DisplayClass6_1.<<UseMiddlewareInterface>b__1>d.MoveNext()
--- End of stack trace from previous location ---
   at Umbraco.Cms.Web.BackOffice.Middleware.BackOfficeExternalLoginProviderErrorMiddleware.InvokeAsync(HttpContext context, RequestDelegate next)
   at Microsoft.AspNetCore.Builder.UseMiddlewareExtensions.<>c__DisplayClass6_1.<<UseMiddlewareInterface>b__1>d.MoveNext()
--- End of stack trace from previous location ---
   at Microsoft.AspNetCore.Session.SessionMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Session.SessionMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Localization.RequestLocalizationMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Authorization.AuthorizationMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)
   at SixLabors.ImageSharp.Web.Middleware.ImageSharpMiddleware.Invoke(HttpContext httpContext, Boolean retry)
   at StackExchange.Profiling.MiniProfilerMiddleware.Invoke(HttpContext context) in C:\projects\dotnet\src\MiniProfiler.AspNetCore\MiniProfilerMiddleware.cs:line 119
   at Umbraco.Cms.Web.Common.Middleware.UmbracoRequestMiddleware.InvokeAsync(HttpContext context, RequestDelegate next)
   at Umbraco.Cms.Web.Common.Middleware.UmbracoRequestMiddleware.InvokeAsync(HttpContext context, RequestDelegate next)
   at Microsoft.AspNetCore.Builder.UseMiddlewareExtensions.<>c__DisplayClass6_1.<<UseMiddlewareInterface>b__1>d.MoveNext()
--- End of stack trace from previous location ---
   at Umbraco.Cms.Web.Common.Middleware.PreviewAuthenticationMiddleware.InvokeAsync(HttpContext context, RequestDelegate next)
   at Microsoft.AspNetCore.Builder.UseMiddlewareExtensions.<>c__DisplayClass6_1.<<UseMiddlewareInterface>b__1>d.MoveNext()
--- End of stack trace from previous location ---
   at Umbraco.Cms.Web.Common.Middleware.UmbracoRequestLoggingMiddleware.InvokeAsync(HttpContext context, RequestDelegate next)
   at Microsoft.AspNetCore.Builder.UseMiddlewareExtensions.<>c__DisplayClass6_1.<<UseMiddlewareInterface>b__1>d.MoveNext()
--- End of stack trace from previous location ---
   at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)

I thought perhaps a claim was missing, but there is no such claim from azure ad with the name "culture".

I also have an umbraco 8.17 installation with azure ad backoffice integration and that works just fine with the exact same settings in azure.

Specifics

Code snippet:

namespace xxx.Docs.Web.Extensions
{
    public static class BackofficeAuthenticationExtensions
    {
        public static IUmbracoBuilder ConfigureAuthentication(this IUmbracoBuilder builder)
        {
            var configuration = builder.Config;
            var microsoftTenantId = configuration["AzureAd:TenantId"];
            var microsoftClientId = configuration["AzureAd:ClientId"];
            var microsoftClientSecret = configuration["AzureAd:ClientSecret"];

            builder.AddBackOfficeExternalLogins(logins =>
            {
                const string schema = MicrosoftAccountDefaults.AuthenticationScheme;

                logins.AddBackOfficeLogin(
                    backOfficeAuthenticationBuilder =>
                    {
                        backOfficeAuthenticationBuilder.AddMicrosoftAccount(
                            // the scheme must be set with this method to work for the back office
                            backOfficeAuthenticationBuilder.SchemeForBackOffice(schema) ?? string.Empty,
                            options =>
                            {
                                options.AuthorizationEndpoint = $"https://login.microsoftonline.com/{microsoftTenantId}/oauth2/v2.0/authorize";
                                options.TokenEndpoint = $"https://login.microsoftonline.com/{microsoftTenantId}/oauth2/v2.0/token";
                                //By default this is '/signin-microsoft' but it needs to be changed to this
                                options.CallbackPath = "/umbraco-signin-microsoft/";
                                //Obtained from the AZURE AD B2C WEB APP
                                options.ClientId = microsoftClientId;
                                //Obtained from the AZURE AD B2C WEB APP
                                options.ClientSecret = microsoftClientSecret;                                
                            });
                    });
            });
            return builder;
        }
    }
}

Steps to reproduce

Expected result / actual result

Expected result was that the authentication was successful, the backoffice was available for use, and that my microsoft account user is visible in the user section of the backoffice.

Actual result was a blank white screen, and when logging in with a local user, the microsoft account user is NOT in the list of backoffice users.

github-actions[bot] commented 2 years ago

Hi there @frederiktoft!

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 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 :robot: :slightly_smiling_face:

jrunestone commented 2 years ago

EDIT: I realize you are using AddMicrosoftAccount and not AddOpenIdConnect which I resorted to using. I'm not sure if you have access to the particular event I mention below. If not, maybe there's a similar one you can use.

I had the same issue when integrating Azure AD to our backoffice and after digging through the source code I realized a couple of claims were required but not documented. I had (have) a TODO on adding it to the documentation but haven't done so yet. Perhaps this is what you need to do as well?

I added a method for ensuring all claims are present in the OpenIdConnectOptions.Events.OnTokenValidated event in which I ensure there's a culture claim available:

// make sure Locality claim is present, required by Umbraco
        const string azurePreferredLanguageClaim = "xms_pl";

        var lang = claimsIdentity.FindFirstValue(ClaimTypes.Locality) 
                   ?? claimsIdentity.FindFirstValue(azurePreferredLanguageClaim)
                   ?? _configuration["Umbraco:CMS:Global:DefaultUILanguage"]
                   ?? throw new InvalidOperationException($"Failed to set {nameof(ClaimTypes.Locality)} claim");

I also needed to provide a security stamp claim, which I just set to a random GUID for my purposes:

var securityStamp = claimsIdentity.FindFirstValue(Constants.Security.SecurityStampClaimType) ?? Guid.NewGuid().ToString();

Then I set the claims with an Umbraco extension: claimsIdentity.AddRequiredClaims(...)

Hope this helps

Behodar commented 2 years ago

I've confirmed that this is a new issue in Umbraco 10.

There is some discussion on the forum.

sergiutesu commented 2 years ago

Hello, we are currently looking into this issue and I tried to reproduce the issue following this article. https://skrift.io/issues/integrating-azure-active-directory-b2c-with-umbraco-s-users-and-members/

Initially, I used version 10.1.0 and it works, now I just updated my solution to 10.2.1 it still works.

I'm wondering if you give this a try if it works for you or if it's necessary to have options.AuthorizationEndPoint and options.TokenEndPoint @frederiktoft

frederiktoft commented 2 years ago

Hello, we are currently looking into this issue and I tried to reproduce the issue following this article. https://skrift.io/issues/integrating-azure-active-directory-b2c-with-umbraco-s-users-and-members/

Initially, I used version 10.1.0 and it works, now I just updated my solution to 10.2.1 it still works.

I'm wondering if you give this a try if it works for you or if it's necessary to have options.AuthorizationEndPoint and options.TokenEndPoint @frederiktoft

Did you test with a single tenant setup in Azure AD? Because AFAIK you need the endpoints in that kind of setup.

Behodar commented 1 year ago

Is there any update available on this issue? Is it expected to be fixed in Umbraco 11 or do we need to stick with 9 at this point?

meyntony commented 1 year ago

With Microsoft Accounts both single and multi-tenant setups encounter the blank backoffice after login with an error in the Umbraco.Cms.Core.Services.LocalizedTextService.GetAllStoredValues(CultureInfo culture). Value cannot be null. (Parameter 'culture') We have not been able to get the claims correctly from Azure and create the backoffice user which we believe is the main root for the blank back office.

Just to confirm this the same error everyone here is encountering too, right?

HalldorLyngmo commented 1 year ago

Hi guys, here are some results from a debug session with @bergmania on this issue.

Using the code example from the original post we reproduced the issue on Umbraco v10.2.1. We saw it both for Single and Multi-Tenant setups when using AddMicrosoftAccount. Following the sign-in flow we could see that it fails here in the BackOfficeSignInManager

 // If there are no autolink options then the attempt is failed (user does not exist)
        if (autoLinkOptions == null || !autoLinkOptions.AutoLinkExternalAccount)
        {
            return SignInResult.Failed;
        }

By default, the AutoLinkExternalAccount value is set to false and since there are no AutoLinkOptions specified this returns a failed sign-in. It's a bit surprising that we don't see an error on the frontend instead of the blank screen.

The BackOfficeControllerwhich receives the failed sign-in creates this error message which never seems to reach the user.

 else if (result == SignInResult.Failed)
        {
            // Failed only occurs when the user does not exist
            errors.Add("The requested provider (" + loginInfo.LoginProvider +
                       ") has not been linked to an account, the provider must be linked from the back office.");
        }

Comparing the code from v10.2.1 to v9.5.4 I could not spot any changes surrounding these actions so I'm not sure how this same setup could work in v9...

Solution/Workaround

Adding a BackOfficeExternalLoginProviderOptions and enabling AutoLinkExternalAccount we were able to progress further into the sign-in flow but we did hit an issue with the claims coming back from Azure. After the sign-in the backoffice was still blank and looking at the database the user was not created. Turns out that if the user does not have an email value the fallback is the "User Principal Name".

ClaimActions.MapCustomJson(ClaimTypes.Email, user => user.GetString("mail") ?? user.GetString("userPrincipalName"));

For our user who did not have an email, the "User Principal Name" looked something like this user_gmail.com#EXT#@usergmail.onmicrosoft.com. The hashtags turned out to be an issue and therefore Umbraco failed to create the user. Not all users will have this version of the "User Principal Name" but we found posts from other people hitting the same situation f.ex. - https://stackoverflow.com/questions/62457365/ms-graph-api-does-not-return-users-email-address-from-azure-ad-b2c

Adding in an email value for the user resulted in a successful login and creation of the user. One interesting thing was that the user did have an email in another field but that was not sent with the claims.

image

That property "Alternate email" was possible to get by using the UserInformationEndpoint like in the code example below.

Code

using Microsoft.AspNetCore.Authentication;
using Microsoft.AspNetCore.Authentication.MicrosoftAccount;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Options;
using System.Security.Claims;
using Umbraco.Cms.Core;
using Umbraco.Cms.Web.BackOffice.Security;

namespace AzureAD
{
    public static class BackofficeAuthenticationExtensions
    {
        public static IUmbracoBuilder ConfigureAuthentication(this IUmbracoBuilder builder)
        {
            var configuration = builder.Config;

            var microsoftTenantId = "";
            var microsoftClientId = "";
            var microsoftClientSecret = "";

            builder.Services.ConfigureOptions<AzureB2CBackofficeExternalLoginProviderOptions>();

            builder.AddBackOfficeExternalLogins(logins =>
            {
                const string schema = MicrosoftAccountDefaults.AuthenticationScheme;

                logins.AddBackOfficeLogin(
                    backOfficeAuthenticationBuilder =>
                    {
                        backOfficeAuthenticationBuilder.AddMicrosoftAccount(
                            // the scheme must be set with this method to work for the back office
                            backOfficeAuthenticationBuilder.SchemeForBackOffice(schema) ?? string.Empty,
                            options =>
                            {
                                options.AuthorizationEndpoint = $"https://login.microsoftonline.com/{microsoftTenantId}/oauth2/v2.0/authorize";
                                options.TokenEndpoint = $"https://login.microsoftonline.com/{microsoftTenantId}/oauth2/v2.0/token";
                                //By default this is '/signin-microsoft' but it needs to be changed to this
                                options.CallbackPath = "/umbraco-signin-microsoft/";
                                //Obtained from the AZURE AD B2C WEB APP
                                options.ClientId = microsoftClientId;
                                //Obtained from the AZURE AD B2C WEB APP
                                options.ClientSecret = microsoftClientSecret;

                                // Example on how to get a different field from the user profile
                                // options.UserInformationEndpoint = "https://graph.microsoft.com/v1.0/me?$select=otherMails,displayName,givenName,surname,id";
                                // options.ClaimActions.MapCustomJson(ClaimTypes.Email, x =>
                                // {
                                //     return x.GetProperty("otherMails").EnumerateArray().First().ToString();
                                // });

                            });
                    });
            });
            return builder;
        }
    }

    public class AzureB2CBackofficeExternalLoginProviderOptions : IConfigureNamedOptions<BackOfficeExternalLoginProviderOptions>
    {
        public const string SchemeName = "Microsoft";
        public void Configure(string name, BackOfficeExternalLoginProviderOptions options)
        {
            if (name != Constants.Security.BackOfficeExternalAuthenticationTypePrefix + SchemeName)
            {
                return;
            }

            Configure(options);
        }

        public void Configure(BackOfficeExternalLoginProviderOptions options)
        {
            options.ButtonStyle = "btn-danger";
            options.Icon = "fa fa-cloud";
            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[] { Constants.Security.AdminGroupAlias },

                // 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: null,
                // 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) =>
                {
                    //This has to be set! Or else when the user is created it will be marked as disabled!

                    autoLinkUser.IsApproved = true;

                    // 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
                },
                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;

        }
    }
}
anh-duc-le commented 1 year ago

Thank you for the workaround solution @HalldorLyngmo! I've been looking all morning for why I was getting the following error upon logging in with an unlinked Azure AD account.

I am in version 10.3.2 and was seeing the same errors:

[13:44:09 WRN] User validation failed: InvalidUserName.
[13:44:09 ERR] An unhandled exception has occurred while executing the request.
System.ArgumentNullException: Value cannot be null. (Parameter 'culture')

The problem was indeed as you described the user principle name foo.bar@contoso.com#EXT#@current-tenant.onmicrosoft.com breaking the auto-link.

Your code-bit for fetching and using the otherMails property was exactly what fixed my problem.

 // Example on how to get a different field from the user profile
options.UserInformationEndpoint = "https://graph.microsoft.com/v1.0/me?$select=otherMails,displayName,givenName,surname,id";
options.ClaimActions.MapCustomJson(ClaimTypes.Email, x =>
{
    return x.GetProperty("otherMails").EnumerateArray().First().ToString();
});
frederiktoft commented 1 year ago

So this issue has been closed. Has the issue been fixed? Has the documentation been updated?

bergmania commented 1 year ago

@frederiktoft, this is not an issue in Umbraco. Umbraco have a hard requirement to have an Email on users. 🙁

It could make sense to add a note in our documentation, that describes how to get the email if it is not provided, even that it has nothing to do with Umbraco, as the solution with "otherMails" is only touching code from Microsoft.AspNetCore.Authentication.MicrosoftAccount.

I'll ask the docs team for their opinion.

frederiktoft commented 1 year ago

I would argue to make a note in the documentation, because with the documentation as it is right now, it wont work.

frederiktoft commented 1 year ago

Besides, adding the following snippet does not work for me, it says the collection is empty:

// Example on how to get a different field from the user profile
options.UserInformationEndpoint = "https://graph.microsoft.com/v1.0/me?$select=otherMails,displayName,givenName,surname,id";
options.ClaimActions.MapCustomJson(ClaimTypes.Email, x =>
{
    return x.GetProperty("otherMails").EnumerateArray().First().ToString();
});

It's really strange, I will have to dig a bit deeper into the settings in Azure it looks like. Its just really odd, it worked totally fine in Umbraco 9, but not from v10 and onwards with zero changes to the code.

EDIT It seems that adding the AzureB2CBackofficeExternalLoginProviderOptions @HalldorLyngmo example and outcommenting the "otherEmails" action, it seems to be working. Really strange. I will need to do some further testing but so far so good.

Behodar commented 1 year ago

"otherEmails" is an empty array for me too, so I wonder why it worked for @anh-duc-le. I haven't tried the other wodge of code yet but will post an update once I've given it a go.

Its just really odd, it worked totally fine in Umbraco 9, but not from v10 and onwards with zero changes to the code.

Same here; it was fine in 9 but stopped working in 10.

Edit: I can confirm that the "AzureB2CBackofficeExternalLoginProviderOptions" code above works in my environment. I also agree with @frederiktoft that the docs need an update :)

anh-duc-le commented 1 year ago

@Behodar it worked for me since my AD tenant user was an external user within the tenant. I can imagine a locally created Azure AD user wouldn't have any records within otherEmail. For those type of users the regular email field would suffice

Brighty28 commented 1 year ago

Sorry but I am on Umbraco version 10.3.2 / 10.4.0 and none of the above works for me at all have tried looking at the updated readme and adding this snippet of code options.UserInformationEndpoint = "https://graph.microsoft.com/v1.0/me?$select=otherMails,displayName,givenName,surname,id"; options.ClaimActions.MapCustomJson(ClaimTypes.Email, x => { return x.GetProperty("otherMails").EnumerateArray().First().ToString(); }); but no luck I have also tried various other options in respect to email so email emailaddress all with no luck am I missing something as I am in the early stages of a project and stuck on this.

Edit: With the "AzureB2CBackofficeExternalLoginProviderOptions" code above I link a Microsoft account by login in through Umbraco the normal with linking it here image Then if I log out and click the sign-in with Microsoft button below image Its fine however when logged in and unlinking my Microsoft account in Umbraco and then logging out back in via the sign-in with Microsoft I get the blank screen as mentioned above by others.