From bc96737aa072ecf7000493b524eba0104ff7bbf1 Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Wed, 7 May 2025 18:18:36 +0200 Subject: [PATCH 1/9] Check and track registered login providers on startup. Remove external logins for providers no longer registered. --- .../IExternalLoginWithKeyRepository.cs | 14 ++++-- .../Services/ExternalLoginService.cs | 10 ++++ .../Services/IExternalLoginWithKeyService.cs | 26 ++++++---- .../Implement/ExternalLoginRepository.cs | 6 +++ .../UmbracoBuilder.BackOfficeAuth.cs | 3 ++ .../ExternalLoginProviderStartupHandler.cs | 32 ++++++++++++ .../BackOfficeExternalLoginProviders.cs | 49 +++++++++++++++++++ .../IBackOfficeExternalLoginProviders.cs | 7 +++ 8 files changed, 133 insertions(+), 14 deletions(-) create mode 100644 src/Umbraco.Web.BackOffice/NotificationHandlers/ExternalLoginProviderStartupHandler.cs diff --git a/src/Umbraco.Core/Persistence/Repositories/IExternalLoginWithKeyRepository.cs b/src/Umbraco.Core/Persistence/Repositories/IExternalLoginWithKeyRepository.cs index ec9a79530cdb..2839fc210760 100644 --- a/src/Umbraco.Core/Persistence/Repositories/IExternalLoginWithKeyRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/IExternalLoginWithKeyRepository.cs @@ -3,23 +3,29 @@ namespace Umbraco.Cms.Core.Persistence.Repositories; /// -/// Repository for external logins with Guid as key, so it can be shared for members and users +/// Repository for external logins with Guid as key, so it can be shared for members and users. /// public interface IExternalLoginWithKeyRepository : IReadWriteQueryRepository, IQueryRepository { /// - /// Replaces all external login providers for the user/member key + /// Replaces all external login providers for the user/member key. /// void Save(Guid userOrMemberKey, IEnumerable logins); /// - /// Replaces all external login provider tokens for the providers specified for the user/member key + /// Replaces all external login provider tokens for the providers specified for the user/member key. /// void Save(Guid userOrMemberKey, IEnumerable tokens); /// - /// Deletes all external logins for the specified the user/member key + /// Deletes all external logins for the specified the user/member key. /// void DeleteUserLogins(Guid userOrMemberKey); + + /// + /// Deletes external logins that aren't associated with the current collection of providers. + /// + /// The keys for the currently configured providers. + void DeleteUserLoginsForRemovedProviders(IEnumerable currentProviderKeys) { } } diff --git a/src/Umbraco.Core/Services/ExternalLoginService.cs b/src/Umbraco.Core/Services/ExternalLoginService.cs index be49927b3689..e5652b784dbb 100644 --- a/src/Umbraco.Core/Services/ExternalLoginService.cs +++ b/src/Umbraco.Core/Services/ExternalLoginService.cs @@ -80,4 +80,14 @@ public void DeleteUserLogins(Guid userOrMemberKey) scope.Complete(); } } + + /// + public void DeleteUserLoginsForRemovedProviders(IEnumerable currentProviderKeys) + { + using (ICoreScope scope = ScopeProvider.CreateCoreScope()) + { + _externalLoginRepository.DeleteUserLoginsForRemovedProviders(currentProviderKeys); + scope.Complete(); + } + } } diff --git a/src/Umbraco.Core/Services/IExternalLoginWithKeyService.cs b/src/Umbraco.Core/Services/IExternalLoginWithKeyService.cs index 42f0708aaa73..a973da70dca4 100644 --- a/src/Umbraco.Core/Services/IExternalLoginWithKeyService.cs +++ b/src/Umbraco.Core/Services/IExternalLoginWithKeyService.cs @@ -5,47 +5,53 @@ namespace Umbraco.Cms.Core.Services; public interface IExternalLoginWithKeyService : IService { /// - /// Returns all user logins assigned + /// Returns all user logins assigned. /// IEnumerable GetExternalLogins(Guid userOrMemberKey); /// - /// Returns all user login tokens assigned + /// Returns all user login tokens assigned. /// IEnumerable GetExternalLoginTokens(Guid userOrMemberKey); /// /// Returns all logins matching the login info - generally there should only be one but in some cases - /// there might be more than one depending on if an administrator has been editing/removing members + /// there might be more than one depending on if an administrator has been editing/removing members. /// IEnumerable Find(string loginProvider, string providerKey); /// - /// Saves the external logins associated with the user + /// Saves the external logins associated with the user. /// /// - /// The user or member key associated with the logins + /// The user or member key associated with the logins. /// /// /// - /// This will replace all external login provider information for the user + /// This will replace all external login provider information for the user. /// void Save(Guid userOrMemberKey, IEnumerable logins); /// - /// Saves the external login tokens associated with the user + /// Saves the external login tokens associated with the user. /// /// - /// The user or member key associated with the logins + /// The user or member key associated with the logins. /// /// /// - /// This will replace all external login tokens for the user + /// This will replace all external login tokens for the user. /// void Save(Guid userOrMemberKey, IEnumerable tokens); /// - /// Deletes all user logins - normally used when a member is deleted + /// Deletes all user logins - normally used when a member is deleted. /// void DeleteUserLogins(Guid userOrMemberKey); + + /// + /// Deletes external logins that aren't associated with the current collection of providers. + /// + /// The keys for the currently configured providers. + void DeleteUserLoginsForRemovedProviders(IEnumerable currentProviderKeys) { } } diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ExternalLoginRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ExternalLoginRepository.cs index 352b1dd3fd47..df9983a912eb 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ExternalLoginRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ExternalLoginRepository.cs @@ -56,6 +56,12 @@ public int Count(IQuery query) public void DeleteUserLogins(Guid userOrMemberKey) => Database.Delete("WHERE userOrMemberKey=@userOrMemberKey", new { userOrMemberKey }); + /// + public void DeleteUserLoginsForRemovedProviders(IEnumerable currentProviderKeys) => + Database.Execute(Sql() + .Delete() + .WhereNotIn(x => x.ProviderKey, currentProviderKeys)); + /// public void Save(Guid userOrMemberKey, IEnumerable logins) { diff --git a/src/Umbraco.Web.BackOffice/DependencyInjection/UmbracoBuilder.BackOfficeAuth.cs b/src/Umbraco.Web.BackOffice/DependencyInjection/UmbracoBuilder.BackOfficeAuth.cs index fd3bfe71bc31..dfa707146d1e 100644 --- a/src/Umbraco.Web.BackOffice/DependencyInjection/UmbracoBuilder.BackOfficeAuth.cs +++ b/src/Umbraco.Web.BackOffice/DependencyInjection/UmbracoBuilder.BackOfficeAuth.cs @@ -7,6 +7,7 @@ using Umbraco.Cms.Core.Security; using Umbraco.Cms.Web.BackOffice.Authorization; using Umbraco.Cms.Web.BackOffice.Middleware; +using Umbraco.Cms.Web.BackOffice.NotificationHandlers; using Umbraco.Cms.Web.BackOffice.Security; using Umbraco.Cms.Web.Common.Authorization; using Umbraco.Cms.Web.Common.Security; @@ -65,6 +66,8 @@ public static IUmbracoBuilder AddBackOfficeAuthentication(this IUmbracoBuilder b builder.AddNotificationHandler(); builder.AddNotificationHandler(); + builder.AddNotificationHandler(); + return builder; } diff --git a/src/Umbraco.Web.BackOffice/NotificationHandlers/ExternalLoginProviderStartupHandler.cs b/src/Umbraco.Web.BackOffice/NotificationHandlers/ExternalLoginProviderStartupHandler.cs new file mode 100644 index 000000000000..ad38af879d61 --- /dev/null +++ b/src/Umbraco.Web.BackOffice/NotificationHandlers/ExternalLoginProviderStartupHandler.cs @@ -0,0 +1,32 @@ +using Umbraco.Cms.Core; +using Umbraco.Cms.Core.Events; +using Umbraco.Cms.Core.Notifications; +using Umbraco.Cms.Core.Services; +using Umbraco.Cms.Web.BackOffice.Security; + +namespace Umbraco.Cms.Web.BackOffice.NotificationHandlers; + +/// +/// Invalidates all backoffice sessions if the external login provider setup has changed. +/// +public class ExternalLoginProviderStartupHandler : INotificationHandler +{ + private readonly IBackOfficeExternalLoginProviders _backOfficeExternalLoginProviders; + private readonly IRuntimeState _runtimeState; + + public ExternalLoginProviderStartupHandler( + IBackOfficeExternalLoginProviders backOfficeExternalLoginProviders, + IRuntimeState runtimeState) + { + _backOfficeExternalLoginProviders = backOfficeExternalLoginProviders; + _runtimeState = runtimeState; + } + + public void Handle(UmbracoApplicationStartingNotification notification) + { + if (_runtimeState.Level == RuntimeLevel.Run) + { + _backOfficeExternalLoginProviders.InvalidateSessionsIfExternalLoginProvidersChanged(); + } + } +} diff --git a/src/Umbraco.Web.BackOffice/Security/BackOfficeExternalLoginProviders.cs b/src/Umbraco.Web.BackOffice/Security/BackOfficeExternalLoginProviders.cs index 277fd06c6bb1..efc698d0a87c 100644 --- a/src/Umbraco.Web.BackOffice/Security/BackOfficeExternalLoginProviders.cs +++ b/src/Umbraco.Web.BackOffice/Security/BackOfficeExternalLoginProviders.cs @@ -1,4 +1,8 @@ using Microsoft.AspNetCore.Authentication; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; +using Umbraco.Cms.Core.DependencyInjection; +using Umbraco.Cms.Core.Services; namespace Umbraco.Cms.Web.BackOffice.Security; @@ -7,13 +11,37 @@ public class BackOfficeExternalLoginProviders : IBackOfficeExternalLoginProvider { private readonly IAuthenticationSchemeProvider _authenticationSchemeProvider; private readonly Dictionary _externalLogins; + private readonly IKeyValueService _keyValueService; + private readonly IExternalLoginWithKeyService _externalLoginWithKeyService; + private readonly ILogger _logger; + private const string ExternalLoginProvidersKey = "Umbraco.Cms.Web.BackOffice.Security.BackOfficeExternalLoginProviders"; + + [Obsolete("Please use the constructor taking all parameters. Scheduled for removal in Umbraco 17.")] public BackOfficeExternalLoginProviders( IEnumerable externalLogins, IAuthenticationSchemeProvider authenticationSchemeProvider) + : this( + externalLogins, + authenticationSchemeProvider, + StaticServiceProvider.Instance.GetRequiredService(), + StaticServiceProvider.Instance.GetRequiredService(), + StaticServiceProvider.Instance.GetRequiredService>()) + { + } + + public BackOfficeExternalLoginProviders( + IEnumerable externalLogins, + IAuthenticationSchemeProvider authenticationSchemeProvider, + IKeyValueService keyValueService, + IExternalLoginWithKeyService externalLoginWithKeyService, + ILogger logger) { _externalLogins = externalLogins.ToDictionary(x => x.AuthenticationType); _authenticationSchemeProvider = authenticationSchemeProvider; + _keyValueService = keyValueService; + _externalLoginWithKeyService = externalLoginWithKeyService; + _logger = logger; } /// @@ -66,4 +94,25 @@ public bool HasDenyLocalLogin() var found = _externalLogins.Values.Where(x => x.Options.DenyLocalLogin).ToList(); return found.Count > 0; } + + /// + public void InvalidateSessionsIfExternalLoginProvidersChanged() + { + var previousExternalLoginProvidersValue = _keyValueService.GetValue(ExternalLoginProvidersKey); + var currentExternalLoginProvidersValue = string.Join("|", _externalLogins.Keys); + + if ((previousExternalLoginProvidersValue ?? string.Empty) != currentExternalLoginProvidersValue) + { + _logger.LogWarning( + "The configured external login providers have changed. All existing backoffice tokens will be invalidated"); + + _externalLoginWithKeyService.DeleteUserLoginsForRemovedProviders(_externalLogins.Keys); + + _keyValueService.SetValue(ExternalLoginProvidersKey, currentExternalLoginProvidersValue); + } + else if (previousExternalLoginProvidersValue is null) + { + _keyValueService.SetValue(ExternalLoginProvidersKey, string.Empty); + } + } } diff --git a/src/Umbraco.Web.BackOffice/Security/IBackOfficeExternalLoginProviders.cs b/src/Umbraco.Web.BackOffice/Security/IBackOfficeExternalLoginProviders.cs index 6d0a699f9a54..8f75b6761018 100644 --- a/src/Umbraco.Web.BackOffice/Security/IBackOfficeExternalLoginProviders.cs +++ b/src/Umbraco.Web.BackOffice/Security/IBackOfficeExternalLoginProviders.cs @@ -30,4 +30,11 @@ public interface IBackOfficeExternalLoginProviders /// /// bool HasDenyLocalLogin(); + + /// + /// Used during startup to see if the configured external login providers is different from the persisted information. + /// If they are different, this will invalidates all backoffice sessions. + /// In particular we want to ensure that any tokens issued from a now removed login provider are invalidated. + /// + void InvalidateSessionsIfExternalLoginProvidersChanged() { } } From e7bb849e089b0bf74295c8ca471cb4879303d188 Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Thu, 8 May 2025 08:52:13 +0200 Subject: [PATCH 2/9] Add new setting for controlling the security stamp validation interval. Expire sessions associated with removed external login providers. --- .../Configuration/Models/SecuritySettings.cs | 20 ++++++++- .../Repositories/IUserRepository.cs | 8 +++- src/Umbraco.Core/Services/IUserService.cs | 6 +++ src/Umbraco.Core/Services/UserService.cs | 10 +++++ .../Repositories/Implement/UserRepository.cs | 41 +++++++++++++++++++ .../BackOfficeExternalLoginProviders.cs | 5 +++ .../Security/ConfigureSecurityStampOptions.cs | 9 ++-- .../Extensions/IntExtensionsTests.cs | 21 ++++++++++ .../Repositories/UserRepositoryTests.cs | 20 +++++++++ 9 files changed, 134 insertions(+), 6 deletions(-) create mode 100644 tests/Umbraco.Tests.UnitTests/Umbraco.Core/Extensions/IntExtensionsTests.cs create mode 100644 tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Persistence/Repositories/UserRepositoryTests.cs diff --git a/src/Umbraco.Core/Configuration/Models/SecuritySettings.cs b/src/Umbraco.Core/Configuration/Models/SecuritySettings.cs index 9570676ebaa2..ea1bfc623e8f 100644 --- a/src/Umbraco.Core/Configuration/Models/SecuritySettings.cs +++ b/src/Umbraco.Core/Configuration/Models/SecuritySettings.cs @@ -28,8 +28,11 @@ public class SecuritySettings internal const int StaticMemberDefaultLockoutTimeInMinutes = 30 * 24 * 60; internal const int StaticUserDefaultLockoutTimeInMinutes = 30 * 24 * 60; - private const long StaticUserDefaultFailedLoginDurationInMilliseconds = 1000; - private const long StaticUserMinimumFailedLoginDurationInMilliseconds = 250; + internal const long StaticUserDefaultFailedLoginDurationInMilliseconds = 1000; + internal const long StaticUserMinimumFailedLoginDurationInMilliseconds = 250; + internal const string StaticSecurityStampValidationInterval = "0.00:30:00"; // TimeSpan.FromMinutes(30); + + public static TimeSpan DefaultSecurityStampValidationInterval => TimeSpan.Parse(StaticSecurityStampValidationInterval); /// /// Gets or sets a value indicating whether to keep the user logged in. @@ -150,4 +153,17 @@ public class SecuritySettings /// [DefaultValue(StaticUserMinimumFailedLoginDurationInMilliseconds)] public long UserMinimumFailedLoginDurationInMilliseconds { get; set; } = StaticUserMinimumFailedLoginDurationInMilliseconds; + + /// + /// Gets or sets the after which security stamps are re-validated. Defaults to 30 minutes. + /// + /// + /// The after which security stamps are re-validated. + /// + /// + /// The default value aligns with the Microsoft.AspNetCore.Identity default: + /// https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.identity.securitystampvalidatoroptions.validationinterval + /// + [DefaultValue(StaticSecurityStampValidationInterval)] + public TimeSpan SecurityStampValidationInterval { get; set; } = TimeSpan.Parse(StaticSecurityStampValidationInterval); } diff --git a/src/Umbraco.Core/Persistence/Repositories/IUserRepository.cs b/src/Umbraco.Core/Persistence/Repositories/IUserRepository.cs index 35458d6eba10..7c2a7e266458 100644 --- a/src/Umbraco.Core/Persistence/Repositories/IUserRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/IUserRepository.cs @@ -1,4 +1,4 @@ -using System.Linq.Expressions; +using System.Linq.Expressions; using Umbraco.Cms.Core.Models.Membership; using Umbraco.Cms.Core.Persistence.Querying; @@ -110,4 +110,10 @@ IEnumerable GetPagedResultsByQuery( void ClearLoginSession(Guid sessionId); IEnumerable GetNextUsers(int id, int count); + + /// + /// Invalidates sessions for users that aren't associated with the current collection of providers. + /// + /// The keys for the currently configured providers. + void InvalidateSessionsForRemovedProviders(IEnumerable currentProviderKeys) { } } diff --git a/src/Umbraco.Core/Services/IUserService.cs b/src/Umbraco.Core/Services/IUserService.cs index 40a3fbd89978..801a4b4e02cb 100644 --- a/src/Umbraco.Core/Services/IUserService.cs +++ b/src/Umbraco.Core/Services/IUserService.cs @@ -233,6 +233,12 @@ IEnumerable GetAll( IEnumerable GetNextUsers(int id, int count); + /// + /// Invalidates sessions for users that aren't associated with the current collection of providers. + /// + /// The keys for the currently configured providers. + void InvalidateSessionsForRemovedProviders(IEnumerable currentProviderKeys) { } + #region User groups /// diff --git a/src/Umbraco.Core/Services/UserService.cs b/src/Umbraco.Core/Services/UserService.cs index e0f65cdd5c93..d96824c89fd0 100644 --- a/src/Umbraco.Core/Services/UserService.cs +++ b/src/Umbraco.Core/Services/UserService.cs @@ -720,6 +720,16 @@ public IEnumerable GetNextUsers(int id, int count) } } + /// + public void InvalidateSessionsForRemovedProviders(IEnumerable currentProviderKeys) + { + using (ICoreScope scope = ScopeProvider.CreateCoreScope()) + { + _userRepository.InvalidateSessionsForRemovedProviders(currentProviderKeys); + scope.Complete(); + } + } + /// /// Gets a list of objects associated with a given group /// diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs index eda072f04963..b0aaea40384a 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs @@ -11,6 +11,7 @@ using Umbraco.Cms.Core.Models.Membership; using Umbraco.Cms.Core.Persistence.Querying; using Umbraco.Cms.Core.Persistence.Repositories; +using Umbraco.Cms.Core.Scoping; using Umbraco.Cms.Core.Serialization; using Umbraco.Cms.Core.Services; using Umbraco.Cms.Infrastructure.Persistence.Dtos; @@ -1070,5 +1071,45 @@ public IEnumerable GetNextUsers(int id, int count) : GetMany(ids).OrderBy(x => x.Id) ?? Enumerable.Empty(); } + /// + public void InvalidateSessionsForRemovedProviders(IEnumerable currentProviderKeys) + { + // Get all the user or member keys associated with the removed providers. + Sql idsQuery = SqlContext.Sql() + .Select(x => x.UserOrMemberKey) + .From() + .WhereNotIn(x => x.ProviderKey, currentProviderKeys); + List userAndMemberKeysAssoicatedWithRemovedProviders = Database.Fetch(idsQuery); + + // Filter for actual users and convert to integer IDs. + var userIdsAssoicatedWithRemovedProviders = userAndMemberKeysAssoicatedWithRemovedProviders + .Select(ConvertUserKeyToUserId) + .Where(x => x.HasValue) + .Select(x => x!.Value) + .ToArray(); + + // Invalidate the security stamps on the users associated with the removed providers. + Sql updateQuery = Sql() + .Update(u => u.Set(x => x.SecurityStampToken, "0".PadLeft(32, '0'))) + .WhereIn(x => x.Id, userIdsAssoicatedWithRemovedProviders); + Database.Execute(updateQuery); + } + + // Marked as internal to expose for unit testing. + internal static int? ConvertUserKeyToUserId(Guid userOrMemberKey) + { + // User Ids are stored as userIds in the umbracoUser table, but as a GUID representation + // of that integer in umbracoExternalLogin (converted via IntExtensions.ToGuid()). + // We need to parse that to get the user Ids to invalidate. + // Note also that umbracoExternalLogin contains members too, as proper GUIDs, so we need to ignore them. + if (userOrMemberKey.ToString().EndsWith("-0000-0000-0000-000000000000") is false) + { + // We have a proper GUID, not an integer conversion, hence a member, not a user. + return null; + } + + return BitConverter.ToInt32(userOrMemberKey.ToByteArray()); + } + #endregion } diff --git a/src/Umbraco.Web.BackOffice/Security/BackOfficeExternalLoginProviders.cs b/src/Umbraco.Web.BackOffice/Security/BackOfficeExternalLoginProviders.cs index efc698d0a87c..6558ea593a45 100644 --- a/src/Umbraco.Web.BackOffice/Security/BackOfficeExternalLoginProviders.cs +++ b/src/Umbraco.Web.BackOffice/Security/BackOfficeExternalLoginProviders.cs @@ -13,6 +13,7 @@ public class BackOfficeExternalLoginProviders : IBackOfficeExternalLoginProvider private readonly Dictionary _externalLogins; private readonly IKeyValueService _keyValueService; private readonly IExternalLoginWithKeyService _externalLoginWithKeyService; + private readonly IUserService _userService; private readonly ILogger _logger; private const string ExternalLoginProvidersKey = "Umbraco.Cms.Web.BackOffice.Security.BackOfficeExternalLoginProviders"; @@ -26,6 +27,7 @@ public BackOfficeExternalLoginProviders( authenticationSchemeProvider, StaticServiceProvider.Instance.GetRequiredService(), StaticServiceProvider.Instance.GetRequiredService(), + StaticServiceProvider.Instance.GetRequiredService(), StaticServiceProvider.Instance.GetRequiredService>()) { } @@ -35,12 +37,14 @@ public BackOfficeExternalLoginProviders( IAuthenticationSchemeProvider authenticationSchemeProvider, IKeyValueService keyValueService, IExternalLoginWithKeyService externalLoginWithKeyService, + IUserService userService, ILogger logger) { _externalLogins = externalLogins.ToDictionary(x => x.AuthenticationType); _authenticationSchemeProvider = authenticationSchemeProvider; _keyValueService = keyValueService; _externalLoginWithKeyService = externalLoginWithKeyService; + _userService = userService; _logger = logger; } @@ -106,6 +110,7 @@ public void InvalidateSessionsIfExternalLoginProvidersChanged() _logger.LogWarning( "The configured external login providers have changed. All existing backoffice tokens will be invalidated"); + _userService.InvalidateSessionsForRemovedProviders(_externalLogins.Keys); _externalLoginWithKeyService.DeleteUserLoginsForRemovedProviders(_externalLogins.Keys); _keyValueService.SetValue(ExternalLoginProvidersKey, currentExternalLoginProvidersValue); diff --git a/src/Umbraco.Web.Common/Security/ConfigureSecurityStampOptions.cs b/src/Umbraco.Web.Common/Security/ConfigureSecurityStampOptions.cs index 71d644d489a2..55588fc72029 100644 --- a/src/Umbraco.Web.Common/Security/ConfigureSecurityStampOptions.cs +++ b/src/Umbraco.Web.Common/Security/ConfigureSecurityStampOptions.cs @@ -32,10 +32,13 @@ public static void ConfigureOptions(SecurityStampValidatorOptions options) /// The options. public static void ConfigureOptions(SecurityStampValidatorOptions options, SecuritySettings securitySettings) { + // Default to the configured value (which, by default is 30 minutes, matching the Microsoft.AspNetCore.Identity default). + options.ValidationInterval = securitySettings.SecurityStampValidationInterval; + // Adjust the security stamp validation interval to a shorter duration - // when concurrent logins are not allowed and the duration has the default interval value - // (currently defaults to 30 minutes), ensuring quicker re-validation. - if (securitySettings.AllowConcurrentLogins is false && options.ValidationInterval == TimeSpan.FromMinutes(30)) + // when concurrent logins are not allowed and the duration has the default interval value, + // ensuring quicker re-validation. + if (securitySettings.AllowConcurrentLogins is false && options.ValidationInterval == SecuritySettings.DefaultSecurityStampValidationInterval) { options.ValidationInterval = TimeSpan.FromSeconds(30); } diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Extensions/IntExtensionsTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Extensions/IntExtensionsTests.cs new file mode 100644 index 000000000000..6d6f66830521 --- /dev/null +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Extensions/IntExtensionsTests.cs @@ -0,0 +1,21 @@ +// Copyright (c) Umbraco. +// See LICENSE for more details. + +using NUnit.Framework; +using Umbraco.Extensions; + +namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Core.Extensions; + +[TestFixture] +public class IntExtensionsTests +{ + [TestCase(20, "00000014-0000-0000-0000-000000000000")] + [TestCase(106, "0000006a-0000-0000-0000-000000000000")] + [TestCase(999999, "000f423f-0000-0000-0000-000000000000")] + [TestCase(555555555, "211d1ae3-0000-0000-0000-000000000000")] + public void ToGuid_Creates_Expected_Guid(int input, string expected) + { + var result = input.ToGuid(); + Assert.AreEqual(expected, result.ToString()); + } +} diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Persistence/Repositories/UserRepositoryTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Persistence/Repositories/UserRepositoryTests.cs new file mode 100644 index 000000000000..75d66106999a --- /dev/null +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Persistence/Repositories/UserRepositoryTests.cs @@ -0,0 +1,20 @@ +using NUnit.Framework; +using Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement; + +namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Persistence.Repositories; + +[TestFixture] +public class UserRepositoryTests +{ + // These tests cases are the reverse of those in IntExtensionsTests. + [TestCase("00000014-0000-0000-0000-000000000000", 20)] + [TestCase("0000006a-0000-0000-0000-000000000000", 106)] + [TestCase("000f423f-0000-0000-0000-000000000000", 999999)] + [TestCase("211d1ae3-0000-0000-0000-000000000000", 555555555)] + [TestCase("0d93047e-558d-4311-8a9d-b89e6fca0337", null)] + public void ConvertUserKeyToUserId_Parses_Expected_Integer(string input, int? expected) + { + var result = UserRepository.ConvertUserKeyToUserId(Guid.Parse(input)); + Assert.AreEqual(expected, result); + } +} From 4c70210eb17fa3ab2c48276a8989c8c070a7d32f Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Thu, 8 May 2025 10:21:21 +0200 Subject: [PATCH 3/9] Fixed delete where clause. --- .../Repositories/IExternalLoginWithKeyRepository.cs | 4 ++-- src/Umbraco.Core/Services/ExternalLoginService.cs | 4 ++-- src/Umbraco.Core/Services/IExternalLoginWithKeyService.cs | 4 ++-- .../Repositories/Implement/ExternalLoginRepository.cs | 4 ++-- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/Umbraco.Core/Persistence/Repositories/IExternalLoginWithKeyRepository.cs b/src/Umbraco.Core/Persistence/Repositories/IExternalLoginWithKeyRepository.cs index 2839fc210760..0329ceb33bc0 100644 --- a/src/Umbraco.Core/Persistence/Repositories/IExternalLoginWithKeyRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/IExternalLoginWithKeyRepository.cs @@ -26,6 +26,6 @@ public interface IExternalLoginWithKeyRepository : IReadWriteQueryRepository /// Deletes external logins that aren't associated with the current collection of providers. /// - /// The keys for the currently configured providers. - void DeleteUserLoginsForRemovedProviders(IEnumerable currentProviderKeys) { } + /// The names of the currently configured providers. + void DeleteUserLoginsForRemovedProviders(IEnumerable currentLoginProviders) { } } diff --git a/src/Umbraco.Core/Services/ExternalLoginService.cs b/src/Umbraco.Core/Services/ExternalLoginService.cs index e5652b784dbb..762ec4cacb6a 100644 --- a/src/Umbraco.Core/Services/ExternalLoginService.cs +++ b/src/Umbraco.Core/Services/ExternalLoginService.cs @@ -82,11 +82,11 @@ public void DeleteUserLogins(Guid userOrMemberKey) } /// - public void DeleteUserLoginsForRemovedProviders(IEnumerable currentProviderKeys) + public void DeleteUserLoginsForRemovedProviders(IEnumerable currentLoginProviders) { using (ICoreScope scope = ScopeProvider.CreateCoreScope()) { - _externalLoginRepository.DeleteUserLoginsForRemovedProviders(currentProviderKeys); + _externalLoginRepository.DeleteUserLoginsForRemovedProviders(currentLoginProviders); scope.Complete(); } } diff --git a/src/Umbraco.Core/Services/IExternalLoginWithKeyService.cs b/src/Umbraco.Core/Services/IExternalLoginWithKeyService.cs index a973da70dca4..8ce5bc3fa6b7 100644 --- a/src/Umbraco.Core/Services/IExternalLoginWithKeyService.cs +++ b/src/Umbraco.Core/Services/IExternalLoginWithKeyService.cs @@ -52,6 +52,6 @@ public interface IExternalLoginWithKeyService : IService /// /// Deletes external logins that aren't associated with the current collection of providers. /// - /// The keys for the currently configured providers. - void DeleteUserLoginsForRemovedProviders(IEnumerable currentProviderKeys) { } + /// The names of the currently configured providers. + void DeleteUserLoginsForRemovedProviders(IEnumerable currentLoginProviders) { } } diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ExternalLoginRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ExternalLoginRepository.cs index df9983a912eb..f111cd5f0f78 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ExternalLoginRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ExternalLoginRepository.cs @@ -57,10 +57,10 @@ public void DeleteUserLogins(Guid userOrMemberKey) => Database.Delete("WHERE userOrMemberKey=@userOrMemberKey", new { userOrMemberKey }); /// - public void DeleteUserLoginsForRemovedProviders(IEnumerable currentProviderKeys) => + public void DeleteUserLoginsForRemovedProviders(IEnumerable currentLoginProviders) => Database.Execute(Sql() .Delete() - .WhereNotIn(x => x.ProviderKey, currentProviderKeys)); + .WhereNotIn(x => x.LoginProvider, currentLoginProviders)); /// public void Save(Guid userOrMemberKey, IEnumerable logins) From 4fec4fcfaa30d12bd7fe7c71d565ef9ebb9e5d4f Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Thu, 8 May 2025 10:28:00 +0200 Subject: [PATCH 4/9] Clarify code comments. --- .../Persistence/Repositories/Implement/UserRepository.cs | 3 +-- .../ExternalLoginProviderStartupHandler.cs | 3 ++- .../Security/BackOfficeExternalLoginProviders.cs | 2 +- .../Security/IBackOfficeExternalLoginProviders.cs | 4 ++-- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs index b0aaea40384a..d726acc69108 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs @@ -11,7 +11,6 @@ using Umbraco.Cms.Core.Models.Membership; using Umbraco.Cms.Core.Persistence.Querying; using Umbraco.Cms.Core.Persistence.Repositories; -using Umbraco.Cms.Core.Scoping; using Umbraco.Cms.Core.Serialization; using Umbraco.Cms.Core.Services; using Umbraco.Cms.Infrastructure.Persistence.Dtos; @@ -1098,7 +1097,7 @@ public void InvalidateSessionsForRemovedProviders(IEnumerable currentPro // Marked as internal to expose for unit testing. internal static int? ConvertUserKeyToUserId(Guid userOrMemberKey) { - // User Ids are stored as userIds in the umbracoUser table, but as a GUID representation + // User Ids are stored as integers in the umbracoUser table, but as a GUID representation // of that integer in umbracoExternalLogin (converted via IntExtensions.ToGuid()). // We need to parse that to get the user Ids to invalidate. // Note also that umbracoExternalLogin contains members too, as proper GUIDs, so we need to ignore them. diff --git a/src/Umbraco.Web.BackOffice/NotificationHandlers/ExternalLoginProviderStartupHandler.cs b/src/Umbraco.Web.BackOffice/NotificationHandlers/ExternalLoginProviderStartupHandler.cs index ad38af879d61..e892415592f7 100644 --- a/src/Umbraco.Web.BackOffice/NotificationHandlers/ExternalLoginProviderStartupHandler.cs +++ b/src/Umbraco.Web.BackOffice/NotificationHandlers/ExternalLoginProviderStartupHandler.cs @@ -7,7 +7,8 @@ namespace Umbraco.Cms.Web.BackOffice.NotificationHandlers; /// -/// Invalidates all backoffice sessions if the external login provider setup has changed. +/// Invalidates backoffice sessions and clears external logins for removed providers if the external login +/// provider setup has changed. /// public class ExternalLoginProviderStartupHandler : INotificationHandler { diff --git a/src/Umbraco.Web.BackOffice/Security/BackOfficeExternalLoginProviders.cs b/src/Umbraco.Web.BackOffice/Security/BackOfficeExternalLoginProviders.cs index 6558ea593a45..1b9a6374dbe5 100644 --- a/src/Umbraco.Web.BackOffice/Security/BackOfficeExternalLoginProviders.cs +++ b/src/Umbraco.Web.BackOffice/Security/BackOfficeExternalLoginProviders.cs @@ -108,7 +108,7 @@ public void InvalidateSessionsIfExternalLoginProvidersChanged() if ((previousExternalLoginProvidersValue ?? string.Empty) != currentExternalLoginProvidersValue) { _logger.LogWarning( - "The configured external login providers have changed. All existing backoffice tokens will be invalidated"); + "The configured external login providers have changed. Existing backoffice sessions using the removed providers will be invalidated and external login data removed."); _userService.InvalidateSessionsForRemovedProviders(_externalLogins.Keys); _externalLoginWithKeyService.DeleteUserLoginsForRemovedProviders(_externalLogins.Keys); diff --git a/src/Umbraco.Web.BackOffice/Security/IBackOfficeExternalLoginProviders.cs b/src/Umbraco.Web.BackOffice/Security/IBackOfficeExternalLoginProviders.cs index 8f75b6761018..1b1f86186edd 100644 --- a/src/Umbraco.Web.BackOffice/Security/IBackOfficeExternalLoginProviders.cs +++ b/src/Umbraco.Web.BackOffice/Security/IBackOfficeExternalLoginProviders.cs @@ -33,8 +33,8 @@ public interface IBackOfficeExternalLoginProviders /// /// Used during startup to see if the configured external login providers is different from the persisted information. - /// If they are different, this will invalidates all backoffice sessions. - /// In particular we want to ensure that any tokens issued from a now removed login provider are invalidated. + /// If they are different, this will invalidate backoffice sessions and clear external logins for removed providers + /// if the external login provider setup has changed. /// void InvalidateSessionsIfExternalLoginProvidersChanged() { } } From e907a4a79fd3e5e7e7c732bd5c348aadf725faa7 Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Thu, 8 May 2025 10:30:40 +0200 Subject: [PATCH 5/9] Fixed spelling of variable names. --- .../Persistence/Repositories/Implement/UserRepository.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs index d726acc69108..b78c3a1484dd 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs @@ -1078,10 +1078,10 @@ public void InvalidateSessionsForRemovedProviders(IEnumerable currentPro .Select(x => x.UserOrMemberKey) .From() .WhereNotIn(x => x.ProviderKey, currentProviderKeys); - List userAndMemberKeysAssoicatedWithRemovedProviders = Database.Fetch(idsQuery); + List userAndMemberKeysAssociatedWithRemovedProviders = Database.Fetch(idsQuery); // Filter for actual users and convert to integer IDs. - var userIdsAssoicatedWithRemovedProviders = userAndMemberKeysAssoicatedWithRemovedProviders + var userIdsAssociatedWithRemovedProviders = userAndMemberKeysAssociatedWithRemovedProviders .Select(ConvertUserKeyToUserId) .Where(x => x.HasValue) .Select(x => x!.Value) @@ -1090,7 +1090,7 @@ public void InvalidateSessionsForRemovedProviders(IEnumerable currentPro // Invalidate the security stamps on the users associated with the removed providers. Sql updateQuery = Sql() .Update(u => u.Set(x => x.SecurityStampToken, "0".PadLeft(32, '0'))) - .WhereIn(x => x.Id, userIdsAssoicatedWithRemovedProviders); + .WhereIn(x => x.Id, userIdsAssociatedWithRemovedProviders); Database.Execute(updateQuery); } From e60c6e1be17a863da87e86edafa8edd88b572322 Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Thu, 8 May 2025 15:48:04 +0200 Subject: [PATCH 6/9] Removed configuration setting. --- .../Configuration/Models/SecuritySettings.cs | 17 ----------------- .../Security/ConfigureSecurityStampOptions.cs | 9 +++------ 2 files changed, 3 insertions(+), 23 deletions(-) diff --git a/src/Umbraco.Core/Configuration/Models/SecuritySettings.cs b/src/Umbraco.Core/Configuration/Models/SecuritySettings.cs index ea1bfc623e8f..206686be4a53 100644 --- a/src/Umbraco.Core/Configuration/Models/SecuritySettings.cs +++ b/src/Umbraco.Core/Configuration/Models/SecuritySettings.cs @@ -2,7 +2,6 @@ // See LICENSE for more details. using System.ComponentModel; -using System.ComponentModel.DataAnnotations; namespace Umbraco.Cms.Core.Configuration.Models; @@ -30,9 +29,6 @@ public class SecuritySettings internal const int StaticUserDefaultLockoutTimeInMinutes = 30 * 24 * 60; internal const long StaticUserDefaultFailedLoginDurationInMilliseconds = 1000; internal const long StaticUserMinimumFailedLoginDurationInMilliseconds = 250; - internal const string StaticSecurityStampValidationInterval = "0.00:30:00"; // TimeSpan.FromMinutes(30); - - public static TimeSpan DefaultSecurityStampValidationInterval => TimeSpan.Parse(StaticSecurityStampValidationInterval); /// /// Gets or sets a value indicating whether to keep the user logged in. @@ -153,17 +149,4 @@ public class SecuritySettings /// [DefaultValue(StaticUserMinimumFailedLoginDurationInMilliseconds)] public long UserMinimumFailedLoginDurationInMilliseconds { get; set; } = StaticUserMinimumFailedLoginDurationInMilliseconds; - - /// - /// Gets or sets the after which security stamps are re-validated. Defaults to 30 minutes. - /// - /// - /// The after which security stamps are re-validated. - /// - /// - /// The default value aligns with the Microsoft.AspNetCore.Identity default: - /// https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.identity.securitystampvalidatoroptions.validationinterval - /// - [DefaultValue(StaticSecurityStampValidationInterval)] - public TimeSpan SecurityStampValidationInterval { get; set; } = TimeSpan.Parse(StaticSecurityStampValidationInterval); } diff --git a/src/Umbraco.Web.Common/Security/ConfigureSecurityStampOptions.cs b/src/Umbraco.Web.Common/Security/ConfigureSecurityStampOptions.cs index 55588fc72029..0cf4bd2a1fac 100644 --- a/src/Umbraco.Web.Common/Security/ConfigureSecurityStampOptions.cs +++ b/src/Umbraco.Web.Common/Security/ConfigureSecurityStampOptions.cs @@ -32,13 +32,10 @@ public static void ConfigureOptions(SecurityStampValidatorOptions options) /// The options. public static void ConfigureOptions(SecurityStampValidatorOptions options, SecuritySettings securitySettings) { - // Default to the configured value (which, by default is 30 minutes, matching the Microsoft.AspNetCore.Identity default). - options.ValidationInterval = securitySettings.SecurityStampValidationInterval; - // Adjust the security stamp validation interval to a shorter duration - // when concurrent logins are not allowed and the duration has the default interval value, - // ensuring quicker re-validation. - if (securitySettings.AllowConcurrentLogins is false && options.ValidationInterval == SecuritySettings.DefaultSecurityStampValidationInterval) + // when concurrent logins are not allowed and the duration has the default interval value + // (currently defaults to 30 minutes), ensuring quicker re-validation. + if (securitySettings.AllowConcurrentLogins is false && options.ValidationInterval == new SecurityStampValidatorOptions().ValidationInterval) { options.ValidationInterval = TimeSpan.FromSeconds(30); } From cb883e24bf9ca1f098c346cee70200d79620bfaf Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Fri, 9 May 2025 10:21:17 +0200 Subject: [PATCH 7/9] Handled issues raised in code review. --- src/Umbraco.Core/Extensions/IntExtensions.cs | 38 ++++++++++++++++--- .../Repositories/Implement/UserRepository.cs | 24 ++++++------ .../ExternalLoginProviderStartupHandler.cs | 25 ++++++++++-- .../BackOfficeExternalLoginProviders.cs | 2 +- 4 files changed, 66 insertions(+), 23 deletions(-) diff --git a/src/Umbraco.Core/Extensions/IntExtensions.cs b/src/Umbraco.Core/Extensions/IntExtensions.cs index d347993dd07e..9b3c2c60ae6b 100644 --- a/src/Umbraco.Core/Extensions/IntExtensions.cs +++ b/src/Umbraco.Core/Extensions/IntExtensions.cs @@ -1,15 +1,17 @@ // Copyright (c) Umbraco. // See LICENSE for more details. +using System.Diagnostics.CodeAnalysis; + namespace Umbraco.Extensions; public static class IntExtensions { /// - /// Does something 'x' amount of times + /// Does something 'x' amount of times. /// - /// - /// + /// Number of times to execute the action. + /// The action to execute. public static void Times(this int n, Action action) { for (var i = 0; i < n; i++) @@ -19,11 +21,11 @@ public static void Times(this int n, Action action) } /// - /// Creates a Guid based on an integer value + /// Creates a Guid based on an integer value. /// - /// value to convert + /// The value to convert. /// - /// + /// The converted . /// public static Guid ToGuid(this int value) { @@ -31,4 +33,28 @@ public static Guid ToGuid(this int value) BitConverter.GetBytes(value).CopyTo(bytes, 0); return new Guid(bytes); } + + /// + /// Restores a GUID previously created from an integer value using . + /// + /// The value to convert. + /// The converted . + /// + /// True if the value could be created, otherwise false. + /// + /// + /// This is used with Umbraco entities that only have integer references in the database (e.g. users). + /// + public static bool TryParseFromGuid(Guid value, [NotNullWhen(true)] out int? result) + { + if (value.ToString().EndsWith("-0000-0000-0000-000000000000") is false) + { + // We have a proper GUID, not one converted from an integer. + result = null; + return false; + } + + result = BitConverter.ToInt32(value.ToByteArray()); + return true; + } } diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs index b78c3a1484dd..3fd40fec1c79 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs @@ -1079,13 +1079,21 @@ public void InvalidateSessionsForRemovedProviders(IEnumerable currentPro .From() .WhereNotIn(x => x.ProviderKey, currentProviderKeys); List userAndMemberKeysAssociatedWithRemovedProviders = Database.Fetch(idsQuery); + if (userAndMemberKeysAssociatedWithRemovedProviders.Count == 0) + { + return; + } // Filter for actual users and convert to integer IDs. var userIdsAssociatedWithRemovedProviders = userAndMemberKeysAssociatedWithRemovedProviders .Select(ConvertUserKeyToUserId) .Where(x => x.HasValue) .Select(x => x!.Value) - .ToArray(); + .ToList(); + if (userIdsAssociatedWithRemovedProviders.Count == 0) + { + return; + } // Invalidate the security stamps on the users associated with the removed providers. Sql updateQuery = Sql() @@ -1094,21 +1102,13 @@ public void InvalidateSessionsForRemovedProviders(IEnumerable currentPro Database.Execute(updateQuery); } - // Marked as internal to expose for unit testing. - internal static int? ConvertUserKeyToUserId(Guid userOrMemberKey) - { + private static int? ConvertUserKeyToUserId(Guid userOrMemberKey) => + // User Ids are stored as integers in the umbracoUser table, but as a GUID representation // of that integer in umbracoExternalLogin (converted via IntExtensions.ToGuid()). // We need to parse that to get the user Ids to invalidate. // Note also that umbracoExternalLogin contains members too, as proper GUIDs, so we need to ignore them. - if (userOrMemberKey.ToString().EndsWith("-0000-0000-0000-000000000000") is false) - { - // We have a proper GUID, not an integer conversion, hence a member, not a user. - return null; - } - - return BitConverter.ToInt32(userOrMemberKey.ToByteArray()); - } + IntExtensions.TryParseFromGuid(userOrMemberKey, out int? userId) ? userId : null; #endregion } diff --git a/src/Umbraco.Web.BackOffice/NotificationHandlers/ExternalLoginProviderStartupHandler.cs b/src/Umbraco.Web.BackOffice/NotificationHandlers/ExternalLoginProviderStartupHandler.cs index e892415592f7..5e2758f79b9b 100644 --- a/src/Umbraco.Web.BackOffice/NotificationHandlers/ExternalLoginProviderStartupHandler.cs +++ b/src/Umbraco.Web.BackOffice/NotificationHandlers/ExternalLoginProviderStartupHandler.cs @@ -2,6 +2,7 @@ using Umbraco.Cms.Core.Events; using Umbraco.Cms.Core.Notifications; using Umbraco.Cms.Core.Services; +using Umbraco.Cms.Core.Sync; using Umbraco.Cms.Web.BackOffice.Security; namespace Umbraco.Cms.Web.BackOffice.NotificationHandlers; @@ -10,24 +11,40 @@ namespace Umbraco.Cms.Web.BackOffice.NotificationHandlers; /// Invalidates backoffice sessions and clears external logins for removed providers if the external login /// provider setup has changed. /// -public class ExternalLoginProviderStartupHandler : INotificationHandler +internal sealed class ExternalLoginProviderStartupHandler : INotificationHandler { private readonly IBackOfficeExternalLoginProviders _backOfficeExternalLoginProviders; private readonly IRuntimeState _runtimeState; + private readonly IServerRoleAccessor _serverRoleAccessor; public ExternalLoginProviderStartupHandler( IBackOfficeExternalLoginProviders backOfficeExternalLoginProviders, - IRuntimeState runtimeState) + IRuntimeState runtimeState, + IServerRoleAccessor serverRoleAccessor) { _backOfficeExternalLoginProviders = backOfficeExternalLoginProviders; _runtimeState = runtimeState; + _serverRoleAccessor = serverRoleAccessor; } public void Handle(UmbracoApplicationStartingNotification notification) { - if (_runtimeState.Level == RuntimeLevel.Run) + if (_runtimeState.Level != RuntimeLevel.Run) { - _backOfficeExternalLoginProviders.InvalidateSessionsIfExternalLoginProvidersChanged(); + return; } + + switch (_serverRoleAccessor.CurrentServerRole) + { + case ServerRole.Subscriber: + case ServerRole.Unknown: + return; + case ServerRole.Single: + case ServerRole.SchedulingPublisher: + default: + break; + } + + _backOfficeExternalLoginProviders.InvalidateSessionsIfExternalLoginProvidersChanged(); } } diff --git a/src/Umbraco.Web.BackOffice/Security/BackOfficeExternalLoginProviders.cs b/src/Umbraco.Web.BackOffice/Security/BackOfficeExternalLoginProviders.cs index 1b9a6374dbe5..d0140133f595 100644 --- a/src/Umbraco.Web.BackOffice/Security/BackOfficeExternalLoginProviders.cs +++ b/src/Umbraco.Web.BackOffice/Security/BackOfficeExternalLoginProviders.cs @@ -103,7 +103,7 @@ public bool HasDenyLocalLogin() public void InvalidateSessionsIfExternalLoginProvidersChanged() { var previousExternalLoginProvidersValue = _keyValueService.GetValue(ExternalLoginProvidersKey); - var currentExternalLoginProvidersValue = string.Join("|", _externalLogins.Keys); + var currentExternalLoginProvidersValue = string.Join("|", _externalLogins.Keys.OrderBy(key => key)); if ((previousExternalLoginProvidersValue ?? string.Empty) != currentExternalLoginProvidersValue) { From 74e087b166f9284a5ec8244227bd1f761ca8d70e Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Fri, 9 May 2025 10:33:46 +0200 Subject: [PATCH 8/9] Updated unit tests. --- .../Extensions/IntExtensionsTests.cs | 20 +++++++++++++++++++ .../Repositories/UserRepositoryTests.cs | 20 ------------------- 2 files changed, 20 insertions(+), 20 deletions(-) delete mode 100644 tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Persistence/Repositories/UserRepositoryTests.cs diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Extensions/IntExtensionsTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Extensions/IntExtensionsTests.cs index 6d6f66830521..46cab0df546a 100644 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Extensions/IntExtensionsTests.cs +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Extensions/IntExtensionsTests.cs @@ -18,4 +18,24 @@ public void ToGuid_Creates_Expected_Guid(int input, string expected) var result = input.ToGuid(); Assert.AreEqual(expected, result.ToString()); } + + [TestCase("00000014-0000-0000-0000-000000000000", 20)] + [TestCase("0000006a-0000-0000-0000-000000000000", 106)] + [TestCase("000f423f-0000-0000-0000-000000000000", 999999)] + [TestCase("211d1ae3-0000-0000-0000-000000000000", 555555555)] + [TestCase("0d93047e-558d-4311-8a9d-b89e6fca0337", null)] + public void TryParseFromGuid_Parses_Expected_Integer(string input, int? expected) + { + var result = IntExtensions.TryParseFromGuid(Guid.Parse(input), out int? intValue); + if (expected is null) + { + Assert.IsFalse(result); + Assert.IsFalse(intValue.HasValue); + } + else + { + Assert.IsTrue(result); + Assert.AreEqual(expected, intValue.Value); + } + } } diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Persistence/Repositories/UserRepositoryTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Persistence/Repositories/UserRepositoryTests.cs deleted file mode 100644 index 75d66106999a..000000000000 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Persistence/Repositories/UserRepositoryTests.cs +++ /dev/null @@ -1,20 +0,0 @@ -using NUnit.Framework; -using Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement; - -namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Persistence.Repositories; - -[TestFixture] -public class UserRepositoryTests -{ - // These tests cases are the reverse of those in IntExtensionsTests. - [TestCase("00000014-0000-0000-0000-000000000000", 20)] - [TestCase("0000006a-0000-0000-0000-000000000000", 106)] - [TestCase("000f423f-0000-0000-0000-000000000000", 999999)] - [TestCase("211d1ae3-0000-0000-0000-000000000000", 555555555)] - [TestCase("0d93047e-558d-4311-8a9d-b89e6fca0337", null)] - public void ConvertUserKeyToUserId_Parses_Expected_Integer(string input, int? expected) - { - var result = UserRepository.ConvertUserKeyToUserId(Guid.Parse(input)); - Assert.AreEqual(expected, result); - } -} From 6c1f4941dca3a3f4733e0a4638f4f2544e72b35a Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Fri, 9 May 2025 10:42:09 +0200 Subject: [PATCH 9/9] Fixed check for server role. --- .../ExternalLoginProviderStartupHandler.cs | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/src/Umbraco.Web.BackOffice/NotificationHandlers/ExternalLoginProviderStartupHandler.cs b/src/Umbraco.Web.BackOffice/NotificationHandlers/ExternalLoginProviderStartupHandler.cs index 5e2758f79b9b..22013d2bbc0c 100644 --- a/src/Umbraco.Web.BackOffice/NotificationHandlers/ExternalLoginProviderStartupHandler.cs +++ b/src/Umbraco.Web.BackOffice/NotificationHandlers/ExternalLoginProviderStartupHandler.cs @@ -29,22 +29,12 @@ public ExternalLoginProviderStartupHandler( public void Handle(UmbracoApplicationStartingNotification notification) { - if (_runtimeState.Level != RuntimeLevel.Run) + if (_runtimeState.Level != RuntimeLevel.Run || + _serverRoleAccessor.CurrentServerRole == ServerRole.Subscriber) { return; } - switch (_serverRoleAccessor.CurrentServerRole) - { - case ServerRole.Subscriber: - case ServerRole.Unknown: - return; - case ServerRole.Single: - case ServerRole.SchedulingPublisher: - default: - break; - } - _backOfficeExternalLoginProviders.InvalidateSessionsIfExternalLoginProvidersChanged(); } }