Avoid an exception on sign out when the principal is populated from an incomplete external login

[AndyButland]

Prerequisites

• I have added steps to test this contribution in the description below

Potentially fixes: #18002

Description

This one has proved tricky as I've not been able to reproduce - but I've seen two people who can, so it's a real issue. I can sort of see what's happening, and believe this will fix it, but it's difficult to fully get to the bottom of it without being able to reproduce the issue and verify the fix.

So I've mostly diagnosed by following the stack trace on the issue:

Umbraco.Cms.Core.Security.UmbracoUserStore<TUser, TRole>.UserIdToInt(string userId)
Umbraco.Cms.Core.Security.BackOfficeUserStore.FindUserAsync(string userId, CancellationToken cancellationToken)
Umbraco.Cms.Core.Security.UmbracoUserStore<TUser, TRole>.FindByIdAsync(string userId, CancellationToken cancellationToken)
Microsoft.AspNetCore.Identity.UserManager<TUser>.GetUserAsync(ClaimsPrincipal principal)
Umbraco.Cms.Web.Common.Security.UmbracoSignInManager<TUser>.SignOutAsync()
Umbraco.Cms.Web.BackOffice.Controllers.BackOfficeController.ExternalSignInAsync(ExternalLoginInfo loginInfo, Func<IActionResult> response)
Umbraco.Cms.Web.BackOffice.Controllers.BackOfficeController.RenderDefaultOrProcessExternalLoginAsync(AuthenticateResult authenticateResult, Func<IActionResult> defaultResponse)
Umbraco.Cms.Web.BackOffice.Controllers.BackOfficeController.Default()

In ExternalSignInAsync, at the end of the method, if there has been an error, we call SignOutAsync. This attempts to retrieve the current Umbraco user, in order to update the security stamp. It seems that in some cases - at least in the once who have been able to reproduce - the current principal at this point has an ID that's not the integer Umbraco ID, but the provider key from the external login provider.

In the Google case, it's a long integer, so the previous UserIdToInt method would throw an exception. I've added a new TryUserIdToInt instead, and where that fails returned a null user (the same as if we had a valid integer but no matching user in the database). That way the sign out method will just not update the security stamp - it can't anyway if it hasn't found an Umbraco user) - rather it will continue with the other sign out steps and return to the caller for the usual error handling of external logins.

Presumably the real issue here is something else, which is why we have this "partially logged in" state. With this update, we would now see the errors as we wouldn't get this exception thrown on the sign out, so hopefully it'll reveal what's going on. Which may be something to do with the reproducer's environment, or maybe something else to resolve in Umbraco.

To Test:

• Set up Google external login and auto linking for the backoffice as per the documentation.
• Verify you can login with a Google account.
• Ideally put a breakpoint in the new TryUserIdToInt method and see what userId has arrived here. For me, it's always the Umbraco integer ID, but perhaps you'll see the long integer Google provider key, in which case you may be able to see what's actually going on to get in this situation.

[nikolajlauridsen]

I can't reproduce either, unfortunately, however, the code looks fine, and everything still works as intended, so I'll go ahead and merge this 😄