umbraco / Umbraco-CMS

Umbraco is a free and open source .NET content management system helping you deliver delightful digital experiences.
https://umbraco.com
MIT License
4.42k stars 2.67k forks source link

InvalidOperationException when Autolinking external account due to missing identity 'Name' value. #12670

Closed d-gibbs closed 1 year ago

d-gibbs commented 2 years ago

Which exact Umbraco version are you using? For example: 9.0.1 - don't just write v9

10.0.1

Bug summary

Setting ExternalSignInAutoLinkOptions autoLinkExternalAccount property to true and signing in throws an InvalidOperationException and the external account is not linked.

Specifics

I am following the documentation to autolink external accounts (found here: https://our.umbraco.com/documentation/reference/security/auto-linking/) example:

public void Configure(BackOfficeExternalLoginProviderOptions options)
{
        options.ButtonStyle = "btn-primary";
        options.Icon = "fa fa-windows";
        options.AutoLinkOptions = new ExternalSignInAutoLinkOptions(
            autoLinkExternalAccount: true,
            defaultCulture: null,
            allowManualLinking: true)
        {
            OnAutoLinking = (autoLinkUser, loginInfo) =>
            {
            },
            OnExternalLogin = (user, loginInfo) =>
            {
                return true;
            },
        };

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

I'm finding that Umbraco throws an InvalidOperationException after authenticating with the external login provider and returning to the site.

The stack trace is:

InvalidOperationException: The Name value cannot be null
Umbraco.Cms.Web.BackOffice.Security.BackOfficeSignInManager.AutoLinkAndSignInExternalAccount(ExternalLoginInfo loginInfo, ExternalSignInAutoLinkOptions autoLinkOptions)
Umbraco.Cms.Web.BackOffice.Security.BackOfficeSignInManager.ExternalLoginSignInAsync(ExternalLoginInfo loginInfo, bool isPersistent, bool bypassTwoFactor)
Umbraco.Cms.Web.BackOffice.Controllers.BackOfficeController.ExternalSignInAsync(ExternalLoginInfo loginInfo, Func<IActionResult> response)
Umbraco.Cms.Web.BackOffice.Controllers.BackOfficeController.RenderDefaultOrProcessExternalLoginAsync(AuthenticateResult authenticateResult, Func<IActionResult> defaultResponse, Func<IActionResult> externalSignInResponse)
Umbraco.Cms.Web.BackOffice.Controllers.BackOfficeController.Default()
Microsoft.AspNetCore.Mvc.Infrastructure.ActionMethodExecutor+TaskOfIActionResultExecutor.Execute(IActionResultTypeMapper mapper, ObjectMethodExecutor executor, object controller, object[] arguments)
System.Threading.Tasks.ValueTask<TResult>.get_Result()
Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeActionMethodAsync>g__Awaited|12_0(ControllerActionInvoker invoker, ValueTask<IActionResult> actionResultValueTask)
Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeNextActionFilterAsync>g__Awaited|10_0(ControllerActionInvoker invoker, Task lastTask, State next, Scope scope, object state, bool isCompleted)
Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Rethrow(ActionExecutedContextSealed context)
Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Next(ref State next, ref Scope scope, ref object state, ref bool isCompleted)
Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.InvokeInnerFilterAsync()
Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeNextResourceFilter>g__Awaited|25_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, object state, bool isCompleted)
Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Rethrow(ResourceExecutedContextSealed context)
Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Next(ref State next, ref Scope scope, ref object state, ref bool isCompleted)
Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeFilterPipelineAsync()
Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeAsync>g__Awaited|17_0(ResourceInvoker invoker, Task task, IDisposable scope)
Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeAsync>g__Awaited|17_0(ResourceInvoker invoker, Task task, IDisposable scope)
Microsoft.AspNetCore.Routing.EndpointMiddleware.<Invoke>g__AwaitRequestTask|6_0(Endpoint endpoint, Task requestTask, ILogger logger)

It seems to be erroring on line 233 of BackOfficeSignInManager: https://github.com/umbraco/Umbraco-CMS/blob/c0c9c50e2110a88afab800abbf0e4a6b6a08c62d/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs#L233

Which indicates that the user Identity.Principal.Name value is null. I've inspected the user claims and I can see name, nameidentifier etc.. are all present however the user principal name is indeed null.

Steps to reproduce

Configure autolinking to an external login provider (e.g Azure AD/B2C) as per: https://our.umbraco.com/documentation/reference/security/auto-linking/

Expected result / actual result

Expected: After authentication with the external login provider, the user should be automatically created within Umbraco (if not already existing)

Actual: We received an InvalidOperationException with an error The Name value cannot be null because the user principal name value is null, even though all user claims (name etc...) are present.

Edited to add

After resolving the Name/Role Claim mappings which appeared to be the original issue - there seems to be a bug which I've submitted a PR to fix: https://github.com/umbraco/Umbraco-CMS/pull/12794

Newly linked accounts are created in an unapproved state e.g. IsApproved = false so they will hit 401 errors when redirected back to umbraco from the configured external login provider:

image

You can workaround this issue by hooking into the OnAutoLinking event and manually setting the user IsApproved flag to true, e.g:

OnAutoLinking = (autoLinkUser, loginInfo) =>
{
    autoLinkUser.IsApproved = true;
}
github-actions[bot] commented 2 years ago

Hi @d-gibbs,

We're writing to let you know that we would love some help with this issue. We feel that this issue is ideal to flag for a community member to work on it. Once flagged here, folk looking for issues to work on will know to look at yours. Of course, please feel free work on this yourself ;-). If there are any changes to this status, we'll be sure to let you know.

For more information about issues and states, have a look at this blog post.

Thanks muchly, from your friendly Umbraco GitHub bot :-)

thomashdk commented 2 years ago

Actual: We received an InvalidOperationException with an error The Name value cannot be null because the user principal name value is null, even though all user claims (name etc...) are present.

Have you found a work around ?d @d-gibbs

hallojoe commented 2 years ago

Hi @thomashdk - hope you are good 👍

Ran into a name is null problem this morning.

Umbraco 10.0.1 using Microsoft.AspNetCore.Authentication.OpenIdConnect against an Azure AD for external back-office login with auto-link enabled.

Setting TokenValidationParameters property on OpenIdConnectOptions, specifying the claim identifier to use, for getting the value used to populate Name on ClaimsIdentity was my fix:

options.TokenValidationParameters = new TokenValidationParameters()
{
    NameClaimType = ClaimTypes.Name // "http://schemas.xmlsoap.org/ws/2005/05/identity/claims/name"
};

In case of using another provider than Microsoft.AspNetCore.Authentication.OpenIdConnect then look for a similar approach in the provider docs.

I believe Umbraco is doing its thing by using Identity?.Name and not interfering with the configuration behind?

d-gibbs commented 2 years ago

@hallojoe - This does make perfect sense, I think the documentation could be updated for those particular lines. https://our.umbraco.com/documentation/reference/security/auto-linking/

Should probably be:

options.TokenValidationParameters.NameClaimType = ClaimTypes.Name;
options.TokenValidationParameters.RoleClaimType = ClaimTypes.Role;

Instead of:

options.TokenValidationParameters.NameClaimType = "name";
options.TokenValidationParameters.RoleClaimType = "role";

Although I'm still getting errors after setting these parameters and the CMS refuses to load. If I check the backend, the user still isn't created.

image

Can you share your complete code if possible?

Thank you

Domitnator commented 2 years ago

@d-gibbs

I'm running into the same issue as you have. So yesterday i cloned the umbraco-cms and did a deep dive on this issue.

In my case it appeared that the emailaddress from the external login user could not be obtained.

If you take a look at P:\development\Umbraco10\Umbraco-CMS\src\Umbraco.Web.BackOffice\Security\BackOfficeSignInManager.cs:

image

Notice that the emailaddress is tried to be obtained by using the ClaimTypes.Email constant. So umbraco is trying to get the claim by using the claim-type: "http://schemas.xmlsoap.org/ws/2005/05/identity/claims/emailaddress".

In my case there is no such claim-type, as the emailaddress is stored in the claim-type "email".

As i am not a openid or scope/claims guru it's kind of hard to say what would the prefered claim-type to use. But i guess this depends on the Identity-provider (in our case IdentityProvider4).

Unfortunatly there is no "options.TokenValidationParameters.EmailClaimType" which can be set to fix this issue.

Maybe the "ExternalSignInAutoLinkOptions" should be used to specify which claim-type you want to use for retrieving the emailaddress? Because right now there is no option to specify it otherwise.

The "retry" option i added myself is also a solution but it feels hacky:

image

d-gibbs commented 2 years ago

@Domitnator - I think the issues you're having there are more likely to do with your external provider setup.

I can only speak to OpenIdConnect and Azure AD B2C... if that's what you're using then make sure you've set the correct claim scopes when calling AddBackOfficeLogin e.g:

// add scopes
options.Scope.Add("openid");
options.Scope.Add("profile");
options.Scope.Add("offline_access");
options.Scope.Add("email");

And make sure your app has delegated permissions such as sign in/read.

It will look like this in Azure portal under App registrations > API Permissions:

image

And lastly, make sure that your identity has actually set the email field:

image

With all that being said... I think there is a bug here.

Newly linked accounts are created in an unapproved state e.g. IsApproved = false so they will hit 401 errors when redirected back to umbraco from the configured external login provider which is why I was seeing these errors in the console:

image

I've submitted a PR which fixes this:

https://github.com/umbraco/Umbraco-CMS/pull/12794

In the meantime, if you get the above working wrt to email claims, you can work around this issue by hooking into the OnAutoLinking event and manually setting the user IsApproved flag to true, e.g:

OnAutoLinking = (autoLinkUser, loginInfo) =>
{
    autoLinkUser.IsApproved = true;
}
Domitnator commented 2 years ago

@d-gibbs Thanks for your reply!

My issue, indeed, has to do with the way the IdentityProvider is configured. But the configuration of the identity provider is not always in your hands! The identity provider i need to work with (which is IdentityProvider4) only returns a claim with the claimtype "email".

I did some google-ing this morning and i believe that the "http://schemas.xmlsoap.org/ws/2005/05/identity/claims/emailaddress" claimtype is often used in the Microsoft AD realm (Azure B2C for instance). So, in my opinion, it is still debatable if Umbraco should use that specific claimtype to retrieve the emailaddress. The umbraco-documentation should point this out, because without looking at the umbraco source code there is no way someone would know this! I spent a whole morning figuring out why the user did not get created, only when i cloned and debugged the umbracoCMS repo I figured out the issue.

BUT Fortunatly I found a way to extend the authorization claims! There is a IClaimsTransformation you can implement, which let you add custom claims (original source: https://visualstudiomagazine.com/articles/2019/11/01/authorization-claims.aspx):

public class UserInfoClaims : IClaimsTransformation
    {
        public Task<ClaimsPrincipal> TransformAsync(ClaimsPrincipal principal)
        {
            var id = new ClaimsIdentity();

            string email = principal.FindFirstValue("email");

            if (!String.IsNullOrEmpty(email)){
                id.AddClaim(new Claim(ClaimTypes.Email, email));
                principal.AddIdentity(id);
            }

            return Task.FromResult(principal);
        }
    }

In this way i can add the "http://schemas.xmlsoap.org/ws/2005/05/identity/claims/emailaddress" claimtype (which Umbraco relies on)!

I hope you see my point now as it might be a bit confusing i added my issue in this thread. Maybe I should open a new issue/bug to raise this question? I'll by happy to issue a pull request eventually to update the documentation (or to update the code)!

d-gibbs commented 2 years ago

@Domitnator Ah yes I see your point, the code for fetching the email claim could definitely be more robust. It does seem like a separate problem to the one I’m experiencing though so I’d recommend opening a new issue for this.

nikolajlauridsen commented 1 year ago

Hey all sorry we haven't gotten to this issue yet, we must have missed it on our first pass through the tracker.

As others have also mentioned in this thread, this is not really a bug with Umbraco, but instead happens because some claims are required to be there, in this case the name, but another required claims is the email claim.

With that said there is a near-infinite amount of potential identity provider setups, so we cannot accommodate all of these, but would recommend mapping the claims from the identity provider to the required claims, how you do this depend on your setup, primarily what library you're using, for instance, OpenIdConnect, and should be in the documentation for the library.

There's also more discussion about this in a related issue here: https://github.com/umbraco/Umbraco-CMS/issues/13100

d-gibbs commented 1 year ago

@nikolajlauridsen Claims setup aside, I think there is still an issue with autolinking accounts where Umbraco will throw 401 errors if you try to login as the auto-linked account is created in an un-approved state.

The workaround is to hook into the OnAutoLinking event and set IsApproved to true manually, e.g:

OnAutoLinking = (autoLinkUser, loginInfo) =>
{
    autoLinkUser.IsApproved = true;
}

IMO, this should be something that happens in the background by Umbraco, or at the very least the documentation on auto-linking should be updated to make it clear you need to manually approve auto-linked users (kind of defeats the purpose).

Perhaps something like defaultIsApproved from the MemberExternalSignInAutoLinkOptions could be added to ExternalSignInAutoLinkOptions?