From 8d1fc30ff43caad016538f62ad61873b212cf94b Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Wed, 22 Jan 2025 15:22:14 +0100 Subject: [PATCH 1/2] Avoid an exception on signout when the principal is populated from an incomplete external login. --- .../Security/BackOfficeUserStore.cs | 15 ++++++++++++++- .../Security/UmbracoUserStore.cs | 18 +++++++++++++++--- 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/src/Umbraco.Infrastructure/Security/BackOfficeUserStore.cs b/src/Umbraco.Infrastructure/Security/BackOfficeUserStore.cs index 0d2767dd25c8..d356f0668011 100644 --- a/src/Umbraco.Infrastructure/Security/BackOfficeUserStore.cs +++ b/src/Umbraco.Infrastructure/Security/BackOfficeUserStore.cs @@ -281,7 +281,20 @@ public override Task DeleteAsync( cancellationToken.ThrowIfCancellationRequested(); ThrowIfDisposed(); - IUser? user = _userService.GetUserById(UserIdToInt(userId)); + // In the external login flow - see BackOfficeController.ExternalSignInAsync - we can have a situation where an + // error has occured but the user is signed in. For that reason, at the end of the process, when the errors are + // found the user is signed out. + // Before signing out, we request the user in order to update the security stamp - see UmbracoSignInManager.SignOutAsync. + // But we can have a situation where the signed in principal has the ID from the external provider, which may not be something + // we can parse to an integer. + // If that's the case, return null rather than throwing an exception. Without an Umbraco user, we can't update the security stamp, + // so no need to fail here. + if (!TryUserIdToInt(userId, out var userIdAsInt)) + { + return Task.FromResult((BackOfficeIdentityUser?)null)!; + } + + IUser? user = _userService.GetUserById(userIdAsInt); if (user == null) { return Task.FromResult((BackOfficeIdentityUser?)null)!; diff --git a/src/Umbraco.Infrastructure/Security/UmbracoUserStore.cs b/src/Umbraco.Infrastructure/Security/UmbracoUserStore.cs index 35a8f2eea9f0..77affcafff26 100644 --- a/src/Umbraco.Infrastructure/Security/UmbracoUserStore.cs +++ b/src/Umbraco.Infrastructure/Security/UmbracoUserStore.cs @@ -1,4 +1,5 @@ using System.ComponentModel; +using System.Diagnostics.CodeAnalysis; using System.Globalization; using System.Security.Claims; using Microsoft.AspNetCore.Identity; @@ -33,18 +34,29 @@ protected UmbracoUserStore(IdentityErrorDescriber describer) protected static int UserIdToInt(string? userId) { - if (int.TryParse(userId, NumberStyles.Integer, CultureInfo.InvariantCulture, out var result)) + if (TryUserIdToInt(userId, out int result)) { return result; } + throw new InvalidOperationException($"Unable to convert user ID ({userId})to int using InvariantCulture"); + } + + protected static bool TryUserIdToInt(string? userId, out int result) + { + if (int.TryParse(userId, NumberStyles.Integer, CultureInfo.InvariantCulture, out result)) + { + return true; + } + if (Guid.TryParse(userId, out Guid key)) { // Reverse the IntExtensions.ToGuid - return BitConverter.ToInt32(key.ToByteArray(), 0); + result = BitConverter.ToInt32(key.ToByteArray(), 0); + return true; } - throw new InvalidOperationException($"Unable to convert user ID ({userId})to int using InvariantCulture"); + return false; } protected static string UserIdToString(int userId) => string.Intern(userId.ToString(CultureInfo.InvariantCulture)); From 16f195b7dc869cf9f2bd9bf7ba0698db5e43c790 Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Wed, 22 Jan 2025 15:34:40 +0100 Subject: [PATCH 2/2] Tidied up comment. --- src/Umbraco.Infrastructure/Security/BackOfficeUserStore.cs | 4 ++-- src/Umbraco.Infrastructure/Security/UmbracoUserStore.cs | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/Umbraco.Infrastructure/Security/BackOfficeUserStore.cs b/src/Umbraco.Infrastructure/Security/BackOfficeUserStore.cs index d356f0668011..21f8978e7188 100644 --- a/src/Umbraco.Infrastructure/Security/BackOfficeUserStore.cs +++ b/src/Umbraco.Infrastructure/Security/BackOfficeUserStore.cs @@ -282,8 +282,8 @@ public override Task DeleteAsync( ThrowIfDisposed(); // In the external login flow - see BackOfficeController.ExternalSignInAsync - we can have a situation where an - // error has occured but the user is signed in. For that reason, at the end of the process, when the errors are - // found the user is signed out. + // error has occured but the user is signed in. For that reason, at the end of the process, if errors are + // recorded, the user is signed out. // Before signing out, we request the user in order to update the security stamp - see UmbracoSignInManager.SignOutAsync. // But we can have a situation where the signed in principal has the ID from the external provider, which may not be something // we can parse to an integer. diff --git a/src/Umbraco.Infrastructure/Security/UmbracoUserStore.cs b/src/Umbraco.Infrastructure/Security/UmbracoUserStore.cs index 77affcafff26..544a8dbd745a 100644 --- a/src/Umbraco.Infrastructure/Security/UmbracoUserStore.cs +++ b/src/Umbraco.Infrastructure/Security/UmbracoUserStore.cs @@ -1,5 +1,4 @@ using System.ComponentModel; -using System.Diagnostics.CodeAnalysis; using System.Globalization; using System.Security.Claims; using Microsoft.AspNetCore.Identity;