From 8972e09a0938c88c96488acba6a6fbd81a5c33ab Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Fri, 10 Apr 2026 10:32:35 +0200 Subject: [PATCH 1/5] Avoid requirement for IBackOfficeStore registrations for non-backoffice configured setups. --- src/Umbraco.Core/Services/UserService.cs | 15 ++++-- .../CoreConfigurationHttpTests.cs | 47 +++++++++++++++++++ 2 files changed, 59 insertions(+), 3 deletions(-) diff --git a/src/Umbraco.Core/Services/UserService.cs b/src/Umbraco.Core/Services/UserService.cs index fe177d1fbe2f..838d131de339 100644 --- a/src/Umbraco.Core/Services/UserService.cs +++ b/src/Umbraco.Core/Services/UserService.cs @@ -1788,10 +1788,19 @@ public async Task, UserOperationStatus>> /// public IEnumerable GetUsersById(params int[]? ids) { - using IServiceScope scope = _serviceScopeFactory.CreateScope(); - IBackOfficeUserStore backOfficeUserStore = scope.ServiceProvider.GetRequiredService(); + if (ids is null || ids.Length <= 0) + { + return Enumerable.Empty(); + } + + using ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true); - return backOfficeUserStore.GetUsersAsync(ids).GetAwaiter().GetResult(); + // Use the repository directly to avoid a service-locator dependency on IBackOfficeUserStore, + // which is only registered when AddBackOffice() is called. This method is also invoked in + // delivery-only scenarios (e.g. Examine indexing via ContentValueSetBuilder). + List idsAsList = [.. ids]; + IQuery query = Query().Where(x => idsAsList.Contains(x.Id)); + return _userRepository.Get(query); } /// diff --git a/tests/Umbraco.Tests.Integration/TestServerTest/CoreConfigurationHttpTests.cs b/tests/Umbraco.Tests.Integration/TestServerTest/CoreConfigurationHttpTests.cs index cca9e30bd851..9ea5d5182bcb 100644 --- a/tests/Umbraco.Tests.Integration/TestServerTest/CoreConfigurationHttpTests.cs +++ b/tests/Umbraco.Tests.Integration/TestServerTest/CoreConfigurationHttpTests.cs @@ -13,6 +13,7 @@ using Umbraco.Cms.Core.Cache; using Umbraco.Cms.Core.Composing; using Umbraco.Cms.Core.Persistence.Repositories; +using Umbraco.Cms.Core.Services; using Umbraco.Cms.Persistence.Sqlite; using Umbraco.Cms.Persistence.SqlServer; using Umbraco.Cms.Tests.Common.Testing; @@ -299,6 +300,52 @@ public async Task CoreWithDeliveryApi_BootsSuccessfully() Assert.That(backofficeMarker, Is.Null, "IBackOfficeEnabledMarker should NOT be registered when using AddCore() without AddBackOffice()"); } + /// + /// Verifies that IUserService.GetUsersById works in a delivery-only scenario (no backoffice). + /// Regression test for https://github.com/umbraco/Umbraco-CMS/issues/22404 where + /// GetUsersById used service location to resolve IBackOfficeUserStore, which isn't + /// registered without AddBackOffice(). This crashed Examine indexing via ContentValueSetBuilder. + /// + [Test] + public async Task CoreWithDeliveryApi_UserServiceGetUsersByIdDoesNotThrow() + { + // Arrange + using var factory = CreateFactory( + configureUmbraco: builder => + { + builder + .AddCore() + .AddDeliveryApi() + .AddUmbracoSqlServerSupport() + .AddUmbracoSqliteSupport() + .AddComposers(); + }, + configureApp: app => + { + app.UseUmbraco() + .WithMiddleware(u => + { + }) + .WithEndpoints(u => + { + u.UseDeliveryApiEndpoints(); + }); + }); + + // Boot the application + using var client = factory.CreateClient(new WebApplicationFactoryClientOptions + { + AllowAutoRedirect = false, + BaseAddress = new Uri("https://localhost/", UriKind.Absolute), + }); + await client.GetAsync("/"); + + // Act & Assert - resolve IUserService and call GetUsersById without throwing. + using var scope = factory.Services.CreateScope(); + var userService = scope.ServiceProvider.GetRequiredService(); + Assert.DoesNotThrow(() => userService.GetUsersById([])); + } + /// /// Verifies that the delivery-only scenario (core + website + delivery API, no backoffice) boots successfully. /// From eb1bb9e01f061a17fa27f4b777162820319d0afc Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Fri, 10 Apr 2026 10:51:43 +0200 Subject: [PATCH 2/5] Apply same update to other read method potentially called from non backoffice setups. --- src/Umbraco.Core/Services/UserService.cs | 29 +++++++++---------- .../CoreConfigurationHttpTests.cs | 13 ++++++--- 2 files changed, 23 insertions(+), 19 deletions(-) diff --git a/src/Umbraco.Core/Services/UserService.cs b/src/Umbraco.Core/Services/UserService.cs index 838d131de339..b041d1f9fff4 100644 --- a/src/Umbraco.Core/Services/UserService.cs +++ b/src/Umbraco.Core/Services/UserService.cs @@ -1695,10 +1695,8 @@ public IEnumerable GetAllInGroup(int? groupId) return Array.Empty(); } - using IServiceScope scope = _serviceScopeFactory.CreateScope(); - IBackOfficeUserStore backOfficeUserStore = scope.ServiceProvider.GetRequiredService(); - - return backOfficeUserStore.GetAllInGroupAsync(groupId.Value).GetAwaiter().GetResult(); + using ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true); + return _userRepository.GetAllInGroup(groupId.Value); } /// @@ -1740,28 +1738,29 @@ public IEnumerable GetAllNotInGroup(int groupId) /// public IUser? GetUserById(int id) { - using IServiceScope scope = _serviceScopeFactory.CreateScope(); - IBackOfficeUserStore backOfficeUserStore = scope.ServiceProvider.GetRequiredService(); - - return backOfficeUserStore.GetAsync(id).GetAwaiter().GetResult(); + using ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true); + return _userRepository.Get(id); } /// public Task GetAsync(Guid key) { - using IServiceScope scope = _serviceScopeFactory.CreateScope(); - IBackOfficeUserStore backOfficeUserStore = scope.ServiceProvider.GetRequiredService(); - - return backOfficeUserStore.GetAsync(key); + using ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true); + return Task.FromResult(_userRepository.Get(key)); } /// public Task> GetAsync(IEnumerable keys) { - using IServiceScope scope = _serviceScopeFactory.CreateScope(); - IBackOfficeUserStore backOfficeUserStore = scope.ServiceProvider.GetRequiredService(); + Guid[] keysArray = keys.ToArray(); + if (keysArray.Length == 0) + { + return Task.FromResult(Enumerable.Empty()); + } - return backOfficeUserStore.GetUsersAsync(keys.ToArray()); + using ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true); + IEnumerable users = _userRepository.GetMany(keysArray); + return Task.FromResult(users); } /// diff --git a/tests/Umbraco.Tests.Integration/TestServerTest/CoreConfigurationHttpTests.cs b/tests/Umbraco.Tests.Integration/TestServerTest/CoreConfigurationHttpTests.cs index 9ea5d5182bcb..1660b6511d0c 100644 --- a/tests/Umbraco.Tests.Integration/TestServerTest/CoreConfigurationHttpTests.cs +++ b/tests/Umbraco.Tests.Integration/TestServerTest/CoreConfigurationHttpTests.cs @@ -301,13 +301,13 @@ public async Task CoreWithDeliveryApi_BootsSuccessfully() } /// - /// Verifies that IUserService.GetUsersById works in a delivery-only scenario (no backoffice). + /// Verifies that IUserService read operations work in a delivery-only scenario (no backoffice). /// Regression test for https://github.com/umbraco/Umbraco-CMS/issues/22404 where - /// GetUsersById used service location to resolve IBackOfficeUserStore, which isn't + /// these methods used service location to resolve IBackOfficeUserStore, which isn't /// registered without AddBackOffice(). This crashed Examine indexing via ContentValueSetBuilder. /// [Test] - public async Task CoreWithDeliveryApi_UserServiceGetUsersByIdDoesNotThrow() + public async Task CoreWithDeliveryApi_UserServiceReadMethodsDoNotThrow() { // Arrange using var factory = CreateFactory( @@ -340,10 +340,15 @@ public async Task CoreWithDeliveryApi_UserServiceGetUsersByIdDoesNotThrow() }); await client.GetAsync("/"); - // Act & Assert - resolve IUserService and call GetUsersById without throwing. + // Act & Assert - all read methods must work without IBackOfficeUserStore. using var scope = factory.Services.CreateScope(); var userService = scope.ServiceProvider.GetRequiredService(); + Assert.DoesNotThrow(() => userService.GetUsersById([])); + Assert.DoesNotThrow(() => userService.GetUserById(-1)); + Assert.DoesNotThrow(() => userService.GetAsync(Guid.Empty).GetAwaiter().GetResult()); + Assert.DoesNotThrow(() => userService.GetAsync([]).GetAwaiter().GetResult()); + Assert.DoesNotThrow(() => userService.GetAllInGroup(null)); } /// From 3ea81e0b1181456cc6f83d99894e3eb975645b80 Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Fri, 10 Apr 2026 10:55:05 +0200 Subject: [PATCH 3/5] Remove comment. --- src/Umbraco.Core/Services/UserService.cs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/Umbraco.Core/Services/UserService.cs b/src/Umbraco.Core/Services/UserService.cs index b041d1f9fff4..b7fccbcd8b59 100644 --- a/src/Umbraco.Core/Services/UserService.cs +++ b/src/Umbraco.Core/Services/UserService.cs @@ -1793,10 +1793,6 @@ public IEnumerable GetUsersById(params int[]? ids) } using ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true); - - // Use the repository directly to avoid a service-locator dependency on IBackOfficeUserStore, - // which is only registered when AddBackOffice() is called. This method is also invoked in - // delivery-only scenarios (e.g. Examine indexing via ContentValueSetBuilder). List idsAsList = [.. ids]; IQuery query = Query().Where(x => idsAsList.Contains(x.Id)); return _userRepository.Get(query); From 3614390ea3800c44281b7fa3ff753d4ebffe827e Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Fri, 10 Apr 2026 11:12:24 +0200 Subject: [PATCH 4/5] Preserve GetUserById upgrade fallback; strengthen test assertions - Add IRuntimeState to UserService and mirror the DbException catch from BackOfficeUserStore.GetAsync(int) in GetUserById, so the upgrade-time fallback to GetForUpgrade is preserved. - Use non-empty arguments in the delivery-only integration test so the repository-backed code paths are actually exercised, not just the early-return guards. - Update UserServiceCrudTests to pass IRuntimeState to the new constructor parameter. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/Umbraco.Core/Services/UserService.cs | 24 +++++++++++++++++-- .../CoreConfigurationHttpTests.cs | 11 +++++---- .../Services/UserServiceCrudTests.cs | 3 ++- 3 files changed, 31 insertions(+), 7 deletions(-) diff --git a/src/Umbraco.Core/Services/UserService.cs b/src/Umbraco.Core/Services/UserService.cs index b7fccbcd8b59..b4233acf5417 100644 --- a/src/Umbraco.Core/Services/UserService.cs +++ b/src/Umbraco.Core/Services/UserService.cs @@ -1,4 +1,5 @@ using System.ComponentModel.DataAnnotations; +using System.Data.Common; using System.Linq.Expressions; using System.Security.Claims; using System.Security.Cryptography; @@ -51,6 +52,7 @@ internal partial class UserService : RepositoryService, IUserService private readonly IUserRepository _userRepository; private readonly ContentSettings _contentSettings; private readonly IUserIdKeyResolver _userIdKeyResolver; + private readonly IRuntimeState _runtimeState; /// /// Initializes a new instance of the class. @@ -74,6 +76,7 @@ internal partial class UserService : RepositoryService, IUserService /// The validator for ISO codes. /// The sender for forgot password emails. /// The resolver for user ID to key mappings. + /// The runtime state for checking upgrade status. public UserService( ICoreScopeProvider provider, ILoggerFactory loggerFactory, @@ -93,7 +96,8 @@ public UserService( IOptions contentSettings, IIsoCodeValidator isoCodeValidator, IUserForgotPasswordSender forgotPasswordSender, - IUserIdKeyResolver userIdKeyResolver) + IUserIdKeyResolver userIdKeyResolver, + IRuntimeState runtimeState) : base(provider, loggerFactory, eventMessagesFactory) { _userRepository = userRepository; @@ -109,6 +113,7 @@ public UserService( _isoCodeValidator = isoCodeValidator; _forgotPasswordSender = forgotPasswordSender; _userIdKeyResolver = userIdKeyResolver; + _runtimeState = runtimeState; _globalSettings = globalSettings.Value; _securitySettings = securitySettings.Value; _contentSettings = contentSettings.Value; @@ -1739,7 +1744,22 @@ public IEnumerable GetAllNotInGroup(int groupId) public IUser? GetUserById(int id) { using ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true); - return _userRepository.Get(id); + + try + { + return _userRepository.Get(id); + } + catch (DbException) + { + // During upgrades the user table schema may be mid-migration, so fall back to + // a minimal query that only resolves the fields needed for authorization. + if (_runtimeState.Level is RuntimeLevel.Install or RuntimeLevel.Upgrade or RuntimeLevel.Upgrading) + { + return _userRepository.GetForUpgrade(id); + } + + throw; + } } /// diff --git a/tests/Umbraco.Tests.Integration/TestServerTest/CoreConfigurationHttpTests.cs b/tests/Umbraco.Tests.Integration/TestServerTest/CoreConfigurationHttpTests.cs index 1660b6511d0c..be80bc2ff5a3 100644 --- a/tests/Umbraco.Tests.Integration/TestServerTest/CoreConfigurationHttpTests.cs +++ b/tests/Umbraco.Tests.Integration/TestServerTest/CoreConfigurationHttpTests.cs @@ -274,7 +274,7 @@ public async Task CoreWithDeliveryApi_BootsSuccessfully() app.UseUmbraco() .WithMiddleware(u => { - // Delivery API doesn't need special middleware + // Delivery API doesn't need special middleware. }) .WithEndpoints(u => { @@ -325,6 +325,7 @@ public async Task CoreWithDeliveryApi_UserServiceReadMethodsDoNotThrow() app.UseUmbraco() .WithMiddleware(u => { + // Delivery API doesn't need special middleware. }) .WithEndpoints(u => { @@ -344,11 +345,13 @@ public async Task CoreWithDeliveryApi_UserServiceReadMethodsDoNotThrow() using var scope = factory.Services.CreateScope(); var userService = scope.ServiceProvider.GetRequiredService(); - Assert.DoesNotThrow(() => userService.GetUsersById([])); + // Use non-empty arguments so the repository-backed code paths are exercised, + // not just the early-return guards for null/empty input. + Assert.DoesNotThrow(() => userService.GetUsersById(-1)); Assert.DoesNotThrow(() => userService.GetUserById(-1)); Assert.DoesNotThrow(() => userService.GetAsync(Guid.Empty).GetAwaiter().GetResult()); - Assert.DoesNotThrow(() => userService.GetAsync([]).GetAwaiter().GetResult()); - Assert.DoesNotThrow(() => userService.GetAllInGroup(null)); + Assert.DoesNotThrow(() => userService.GetAsync(new[] { Guid.Empty }).GetAwaiter().GetResult()); + Assert.DoesNotThrow(() => userService.GetAllInGroup(-1)); } /// diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/UserServiceCrudTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/UserServiceCrudTests.cs index c6533aeff3ad..1d08206c5c8d 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/UserServiceCrudTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/UserServiceCrudTests.cs @@ -83,7 +83,8 @@ private IUserService CreateUserService( GetRequiredService>(), GetRequiredService(), GetRequiredService(), - GetRequiredService()); + GetRequiredService(), + GetRequiredService()); } From 416a347c3cd6dca3af5138e983d542ab8d757c22 Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Mon, 20 Apr 2026 14:23:52 +0200 Subject: [PATCH 5/5] Introduce IBackOfficeUserReader to avoid code duplication for user read methods between UserService and BackOfficeUserStore. --- .../UmbracoBuilder.BackOfficeIdentity.cs | 3 +- .../Services/IBackOfficeUserReader.cs | 49 +++++++ src/Umbraco.Core/Services/UserService.cs | 61 ++------- .../UmbracoBuilder.Services.cs | 2 + .../Security/BackOfficeUserReader.cs | 103 +++++++++++++++ .../Security/BackOfficeUserStore.cs | 121 +++++++++--------- .../Services/UserServiceCrudTests.cs | 2 +- .../Security/BackOfficeUserStoreTests.cs | 7 +- .../Security/BackOfficeUserReaderTests.cs | 93 ++++++++++++++ 9 files changed, 323 insertions(+), 118 deletions(-) create mode 100644 src/Umbraco.Core/Services/IBackOfficeUserReader.cs create mode 100644 src/Umbraco.Infrastructure/Security/BackOfficeUserReader.cs create mode 100644 tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/BackOfficeUserReaderTests.cs diff --git a/src/Umbraco.Cms.Api.Management/DependencyInjection/UmbracoBuilder.BackOfficeIdentity.cs b/src/Umbraco.Cms.Api.Management/DependencyInjection/UmbracoBuilder.BackOfficeIdentity.cs index ea1e42d4f40a..7c177c03496b 100644 --- a/src/Umbraco.Cms.Api.Management/DependencyInjection/UmbracoBuilder.BackOfficeIdentity.cs +++ b/src/Umbraco.Cms.Api.Management/DependencyInjection/UmbracoBuilder.BackOfficeIdentity.cs @@ -54,7 +54,8 @@ public static IUmbracoBuilder AddBackOfficeIdentity(this IUmbracoBuilder builder factory.GetRequiredService(), factory.GetRequiredService(), factory.GetRequiredService(), - factory.GetRequiredService>())) + factory.GetRequiredService>(), + factory.GetRequiredService())) .AddUserManager() .AddSignInManager() .AddClaimsPrincipalFactory() diff --git a/src/Umbraco.Core/Services/IBackOfficeUserReader.cs b/src/Umbraco.Core/Services/IBackOfficeUserReader.cs new file mode 100644 index 000000000000..69abed88300a --- /dev/null +++ b/src/Umbraco.Core/Services/IBackOfficeUserReader.cs @@ -0,0 +1,49 @@ +using Umbraco.Cms.Core.Models.Membership; + +namespace Umbraco.Cms.Core.Services; + +/// +/// Provides shared read access to back office users for both and . +/// +/// +/// Centralises the repository calls so the two consumers cannot drift. Registered independently of +/// so it is available in delivery-only setups. +/// +public interface IBackOfficeUserReader +{ + /// + /// Retrieves a back office user by their integer identifier, falling back to a minimal upgrade-safe query + /// if the user table schema is mid-migration. + /// + /// The integer identifier of the user. + /// The if found; otherwise, null. + IUser? GetById(int id); + + /// + /// Retrieves a back office user by their unique key. + /// + /// The unique key of the user. + /// The if found; otherwise, null. + IUser? GetByKey(Guid key); + + /// + /// Retrieves the back office users with the specified integer identifiers. + /// + /// The integer identifiers of the users to retrieve. + /// The matching users, or an empty collection if is empty. + IEnumerable GetManyById(IEnumerable ids); + + /// + /// Retrieves the back office users with the specified unique keys. + /// + /// The unique keys of the users to retrieve. + /// The matching users, or an empty collection if is empty. + IEnumerable GetManyByKey(IEnumerable keys); + + /// + /// Retrieves all back office users that belong to the specified user group. + /// + /// The integer identifier of the user group. + /// The users in the group, or an empty collection if the group has no members. + IEnumerable GetAllInGroup(int groupId); +} diff --git a/src/Umbraco.Core/Services/UserService.cs b/src/Umbraco.Core/Services/UserService.cs index ce82b79b4d71..f84211f20da8 100644 --- a/src/Umbraco.Core/Services/UserService.cs +++ b/src/Umbraco.Core/Services/UserService.cs @@ -1,5 +1,4 @@ using System.ComponentModel.DataAnnotations; -using System.Data.Common; using System.Linq.Expressions; using System.Security.Claims; using System.Security.Cryptography; @@ -52,7 +51,7 @@ internal partial class UserService : RepositoryService, IUserService private readonly IUserRepository _userRepository; private readonly ContentSettings _contentSettings; private readonly IUserIdKeyResolver _userIdKeyResolver; - private readonly IRuntimeState _runtimeState; + private readonly IBackOfficeUserReader _backOfficeUserReader; /// /// Initializes a new instance of the class. @@ -76,7 +75,7 @@ internal partial class UserService : RepositoryService, IUserService /// The validator for ISO codes. /// The sender for forgot password emails. /// The resolver for user ID to key mappings. - /// The runtime state for checking upgrade status. + /// The shared reader used for back office user lookups. public UserService( ICoreScopeProvider provider, ILoggerFactory loggerFactory, @@ -97,7 +96,7 @@ public UserService( IIsoCodeValidator isoCodeValidator, IUserForgotPasswordSender forgotPasswordSender, IUserIdKeyResolver userIdKeyResolver, - IRuntimeState runtimeState) + IBackOfficeUserReader backOfficeUserReader) : base(provider, loggerFactory, eventMessagesFactory) { _userRepository = userRepository; @@ -113,7 +112,7 @@ public UserService( _isoCodeValidator = isoCodeValidator; _forgotPasswordSender = forgotPasswordSender; _userIdKeyResolver = userIdKeyResolver; - _runtimeState = runtimeState; + _backOfficeUserReader = backOfficeUserReader; _globalSettings = globalSettings.Value; _securitySettings = securitySettings.Value; _contentSettings = contentSettings.Value; @@ -1700,8 +1699,7 @@ public IEnumerable GetAllInGroup(int? groupId) return Array.Empty(); } - using ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true); - return _userRepository.GetAllInGroup(groupId.Value); + return _backOfficeUserReader.GetAllInGroup(groupId.Value); } /// @@ -1742,46 +1740,15 @@ public IEnumerable GetAllNotInGroup(int groupId) /// public IUser? GetUserById(int id) - { - using ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true); - - try - { - return _userRepository.Get(id); - } - catch (DbException) - { - // During upgrades the user table schema may be mid-migration, so fall back to - // a minimal query that only resolves the fields needed for authorization. - if (_runtimeState.Level is RuntimeLevel.Install or RuntimeLevel.Upgrade or RuntimeLevel.Upgrading) - { - return _userRepository.GetForUpgrade(id); - } - - throw; - } - } + => _backOfficeUserReader.GetById(id); /// public Task GetAsync(Guid key) - { - using ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true); - return Task.FromResult(_userRepository.Get(key)); - } + => Task.FromResult(_backOfficeUserReader.GetByKey(key)); /// public Task> GetAsync(IEnumerable keys) - { - Guid[] keysArray = keys.ToArray(); - if (keysArray.Length == 0) - { - return Task.FromResult(Enumerable.Empty()); - } - - using ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true); - IEnumerable users = _userRepository.GetMany(keysArray); - return Task.FromResult(users); - } + => Task.FromResult(_backOfficeUserReader.GetManyByKey(keys)); /// public async Task, UserOperationStatus>> GetLinkedLoginsAsync(Guid userKey) @@ -1806,17 +1773,7 @@ public async Task, UserOperationStatus>> /// public IEnumerable GetUsersById(params int[]? ids) - { - if (ids is null || ids.Length <= 0) - { - return Enumerable.Empty(); - } - - using ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true); - List idsAsList = [.. ids]; - IQuery query = Query().Where(x => idsAsList.Contains(x.Id)); - return _userRepository.Get(query); - } + => ids is null ? Enumerable.Empty() : _backOfficeUserReader.GetManyById(ids); /// public void ReplaceUserGroupPermissions(int groupId, ISet permissions, params int[] entityIds) diff --git a/src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.Services.cs b/src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.Services.cs index 5fa0248f95c4..df8b7cd1f998 100644 --- a/src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.Services.cs +++ b/src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.Services.cs @@ -13,6 +13,7 @@ using Umbraco.Cms.Core.Packaging; using Umbraco.Cms.Core.PropertyEditors; using Umbraco.Cms.Core.Routing; +using Umbraco.Cms.Core.Security; using Umbraco.Cms.Core.Serialization; using Umbraco.Cms.Core.Services; using Umbraco.Cms.Core.Services.Implement; @@ -43,6 +44,7 @@ internal static IUmbracoBuilder AddServices(this IUmbracoBuilder builder) // register the special idk map builder.Services.AddUnique(); builder.Services.AddUnique(); + builder.Services.AddUnique(); builder.Services.AddUnique(); builder.Services.AddUnique(); diff --git a/src/Umbraco.Infrastructure/Security/BackOfficeUserReader.cs b/src/Umbraco.Infrastructure/Security/BackOfficeUserReader.cs new file mode 100644 index 000000000000..8e6864d0e9e1 --- /dev/null +++ b/src/Umbraco.Infrastructure/Security/BackOfficeUserReader.cs @@ -0,0 +1,103 @@ +using System.Data.Common; +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.Services; + +namespace Umbraco.Cms.Core.Security; + +/// +/// Default implementation of ; wraps +/// with the scope handling and upgrade-state fallback required by back office user lookups. +/// +internal sealed class BackOfficeUserReader : IBackOfficeUserReader +{ + private readonly ICoreScopeProvider _scopeProvider; + private readonly IUserRepository _userRepository; + private readonly IRuntimeState _runtimeState; + + /// + /// Initializes a new instance of the class. + /// + /// Provides database transaction scopes for data operations. + /// Repository for accessing user data. + /// Represents the current runtime state of the Umbraco application. + public BackOfficeUserReader( + ICoreScopeProvider scopeProvider, + IUserRepository userRepository, + IRuntimeState runtimeState) + { + _scopeProvider = scopeProvider; + _userRepository = userRepository; + _runtimeState = runtimeState; + } + + /// + public IUser? GetById(int id) + { + using ICoreScope scope = _scopeProvider.CreateCoreScope(autoComplete: true); + + try + { + return _userRepository.Get(id); + } + catch (DbException) + { + // During upgrades the user table schema may be mid-migration, so fall back to + // a minimal query that only resolves the fields needed for authorization. + if (IsUpgrading) + { + return _userRepository.GetForUpgrade(id); + } + + throw; + } + } + + /// + public IUser? GetByKey(Guid key) + { + using ICoreScope scope = _scopeProvider.CreateCoreScope(autoComplete: true); + return _userRepository.Get(key); + } + + /// + public IEnumerable GetManyById(IEnumerable ids) + { + // Need to use a List here because the expression tree cannot convert the array when used in Contains. + // See ExpressionTests.Sql_In(). + List idsAsList = [.. ids]; + if (idsAsList.Count == 0) + { + return Enumerable.Empty(); + } + + using ICoreScope scope = _scopeProvider.CreateCoreScope(autoComplete: true); + IQuery query = _scopeProvider.CreateQuery().Where(x => idsAsList.Contains(x.Id)); + return _userRepository.Get(query); + } + + /// + public IEnumerable GetManyByKey(IEnumerable keys) + { + Guid[] keysArray = keys as Guid[] ?? keys.ToArray(); + if (keysArray.Length == 0) + { + return Enumerable.Empty(); + } + + using ICoreScope scope = _scopeProvider.CreateCoreScope(autoComplete: true); + return _userRepository.GetMany(keysArray); + } + + /// + public IEnumerable GetAllInGroup(int groupId) + { + using ICoreScope scope = _scopeProvider.CreateCoreScope(autoComplete: true); + return _userRepository.GetAllInGroup(groupId); + } + + private bool IsUpgrading => + _runtimeState.Level is RuntimeLevel.Install or RuntimeLevel.Upgrade or RuntimeLevel.Upgrading; +} diff --git a/src/Umbraco.Infrastructure/Security/BackOfficeUserStore.cs b/src/Umbraco.Infrastructure/Security/BackOfficeUserStore.cs index 971640d998d2..b5ea91dd8bc6 100644 --- a/src/Umbraco.Infrastructure/Security/BackOfficeUserStore.cs +++ b/src/Umbraco.Infrastructure/Security/BackOfficeUserStore.cs @@ -8,6 +8,7 @@ using Microsoft.Extensions.Options; using Umbraco.Cms.Core.Cache; using Umbraco.Cms.Core.Configuration.Models; +using Umbraco.Cms.Core.DependencyInjection; using Umbraco.Cms.Core.Events; using Umbraco.Cms.Core.Mapping; using Umbraco.Cms.Core.Models; @@ -42,6 +43,7 @@ public class BackOfficeUserStore : private readonly IRuntimeState _runtimeState; private readonly IEventMessagesFactory _eventMessagesFactory; private readonly ILogger _logger; + private readonly IBackOfficeUserReader _backOfficeUserReader; /// /// Initializes a new instance of the class. @@ -59,6 +61,7 @@ public class BackOfficeUserStore : /// Represents the current runtime state of the Umbraco application. /// Factory for creating event message collections. /// Logger instance for logging operations related to the user store. + /// The shared reader used for back office user lookups. [ActivatorUtilitiesConstructor] public BackOfficeUserStore( ICoreScopeProvider scopeProvider, @@ -73,7 +76,8 @@ public BackOfficeUserStore( IUserRepository userRepository, IRuntimeState runtimeState, IEventMessagesFactory eventMessagesFactory, - ILogger logger) + ILogger logger, + IBackOfficeUserReader backOfficeUserReader) : base(describer) { _scopeProvider = scopeProvider; @@ -88,7 +92,56 @@ public BackOfficeUserStore( _runtimeState = runtimeState; _eventMessagesFactory = eventMessagesFactory; _logger = logger; - _externalLoginService = externalLoginService; + _backOfficeUserReader = backOfficeUserReader; + } + + /// + /// Initializes a new instance of the class. + /// + /// Provides database transaction scopes for data operations. + /// Service for managing Umbraco entities. + /// Handles external login providers with key support. + /// The global configuration settings for Umbraco. + /// Maps between domain and view models in Umbraco. + /// Provides error descriptions for back office user operations. + /// Provides access to application-level caches. + /// Service for managing two-factor authentication for users. + /// Service for managing user groups in the back office. + /// Repository for accessing and persisting user data. + /// Represents the current runtime state of the Umbraco application. + /// Factory for creating event message collections. + /// Logger instance for logging operations related to the user store. + [Obsolete("Please use the constructor with all parameters. Scheduled for removal in Umbraco 19.")] + public BackOfficeUserStore( + ICoreScopeProvider scopeProvider, + IEntityService entityService, + IExternalLoginWithKeyService externalLoginService, + IOptionsSnapshot globalSettings, + IUmbracoMapper mapper, + BackOfficeErrorDescriber describer, + AppCaches appCaches, + ITwoFactorLoginService twoFactorLoginService, + IUserGroupService userGroupService, + IUserRepository userRepository, + IRuntimeState runtimeState, + IEventMessagesFactory eventMessagesFactory, + ILogger logger) + : this( + scopeProvider, + entityService, + externalLoginService, + globalSettings, + mapper, + describer, + appCaches, + twoFactorLoginService, + userGroupService, + userRepository, + runtimeState, + eventMessagesFactory, + logger, + StaticServiceProvider.Instance.GetRequiredService()) + { } /// @@ -265,28 +318,7 @@ public Task DisableAsync(IUser user) /// The unique identifier of the user to retrieve. /// A task representing the asynchronous operation. The task result contains the if found; otherwise, null. public Task GetAsync(int id) - { - using ICoreScope scope = _scopeProvider.CreateCoreScope(autoComplete: true); - - try - { - return Task.FromResult(_userRepository.Get(id)); - } - catch (DbException) - { - // TODO: refactor users/upgrade - // currently kinda accepting anything on upgrade, but that won't deal with all cases - // so we need to do it differently, see the custom UmbracoPocoDataBuilder which should - // be better BUT requires that the app restarts after the upgrade! - if (IsUpgrading) - { - // NOTE: this will not be cached - return Task.FromResult(_userRepository.GetForUpgrade(id)); - } - - throw; - } - } + => Task.FromResult(_backOfficeUserReader.GetById(id)); /// /// Asynchronously retrieves the users with the specified IDs. @@ -296,23 +328,7 @@ public Task DisableAsync(IUser user) /// A task that represents the asynchronous operation. The task result contains an of users matching the specified IDs. /// public Task> GetUsersAsync(params int[]? ids) - { - if (ids is null || ids.Length <= 0) - { - return Task.FromResult(Enumerable.Empty()); - } - - using ICoreScope scope = _scopeProvider.CreateCoreScope(autoComplete: true); - - // Need to use a List here because the expression tree cannot convert the array when used in Contains. - // See ExpressionTests.Sql_In(). - List idsAsList = [.. ids]; - IQuery query = _scopeProvider.CreateQuery().Where(x => idsAsList.Contains(x.Id)); - - IEnumerable users = _userRepository.Get(query); - - return Task.FromResult(users); - } + => Task.FromResult(ids is null ? Enumerable.Empty() : _backOfficeUserReader.GetManyById(ids)); /// /// Asynchronously retrieves the users with the specified IDs. @@ -322,24 +338,11 @@ public Task> GetUsersAsync(params int[]? ids) /// A task that represents the asynchronous operation. The task result contains an of users matching the specified IDs. /// public Task> GetUsersAsync(params Guid[]? keys) - { - if (keys is null || keys.Length <= 0) - { - return Task.FromResult(Enumerable.Empty()); - } - - using ICoreScope scope = _scopeProvider.CreateCoreScope(autoComplete: true); - IEnumerable users = _userRepository.GetMany(keys); - - return Task.FromResult(users); - } + => Task.FromResult(keys is null ? Enumerable.Empty() : _backOfficeUserReader.GetManyByKey(keys)); /// public Task GetAsync(Guid key) - { - using ICoreScope scope = _scopeProvider.CreateCoreScope(autoComplete: true); - return Task.FromResult(_userRepository.Get(key)); - } + => Task.FromResult(_backOfficeUserReader.GetByKey(key)); /// public Task GetByUserNameAsync(string username) @@ -395,11 +398,7 @@ public Task> GetUsersAsync(params Guid[]? keys) /// public Task> GetAllInGroupAsync(int groupId) - { - using ICoreScope scope = _scopeProvider.CreateCoreScope(autoComplete: true); - IEnumerable usersInGroup = _userRepository.GetAllInGroup(groupId); - return Task.FromResult(usersInGroup); - } + => Task.FromResult(_backOfficeUserReader.GetAllInGroup(groupId)); private bool IsUpgrading => _runtimeState.Level == RuntimeLevel.Install || _runtimeState.Level == RuntimeLevel.Upgrade || _runtimeState.Level == RuntimeLevel.Upgrading; diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/UserServiceCrudTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/UserServiceCrudTests.cs index 1d08206c5c8d..ca67beeba165 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/UserServiceCrudTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/UserServiceCrudTests.cs @@ -84,7 +84,7 @@ private IUserService CreateUserService( GetRequiredService(), GetRequiredService(), GetRequiredService(), - GetRequiredService()); + GetRequiredService()); } diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Security/BackOfficeUserStoreTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Security/BackOfficeUserStoreTests.cs index 4d88c2693903..6ebef7b230c3 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Security/BackOfficeUserStoreTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Security/BackOfficeUserStoreTests.cs @@ -38,8 +38,9 @@ internal sealed class BackOfficeUserStoreTests : UmbracoIntegrationTest private IEventMessagesFactory EventMessagesFactory => GetRequiredService(); - private readonly ILogger _logger = NullLogger.Instance; + private IBackOfficeUserReader BackOfficeUserReader => GetRequiredService(); + private readonly ILogger _logger = NullLogger.Instance; private BackOfficeUserStore GetUserStore() => new( @@ -55,8 +56,8 @@ private BackOfficeUserStore GetUserStore() UserRepository, RuntimeState, EventMessagesFactory, - _logger - ); + _logger, + BackOfficeUserReader); [Test] public async Task Can_Persist_Is_Approved() diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/BackOfficeUserReaderTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/BackOfficeUserReaderTests.cs new file mode 100644 index 000000000000..829e7bbf9289 --- /dev/null +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/BackOfficeUserReaderTests.cs @@ -0,0 +1,93 @@ +using System.Data; +using System.Data.Common; +using Moq; +using NUnit.Framework; +using Umbraco.Cms.Core; +using Umbraco.Cms.Core.Events; +using Umbraco.Cms.Core.Models.Membership; +using Umbraco.Cms.Core.Persistence.Repositories; +using Umbraco.Cms.Core.Scoping; +using Umbraco.Cms.Core.Security; +using Umbraco.Cms.Core.Services; + +namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security; + +[TestFixture] +public class BackOfficeUserReaderTests +{ + private const int UserId = 1; + + private Mock _scopeProviderMock = null!; + private Mock _userRepositoryMock = null!; + private Mock _runtimeStateMock = null!; + private BackOfficeUserReader _sut = null!; + + [SetUp] + public void Setup() + { + _scopeProviderMock = new Mock(); + _userRepositoryMock = new Mock(); + _runtimeStateMock = new Mock(); + + _scopeProviderMock + .Setup(x => x.CreateCoreScope( + It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny())) + .Returns(Mock.Of()); + + _sut = new BackOfficeUserReader( + _scopeProviderMock.Object, + _userRepositoryMock.Object, + _runtimeStateMock.Object); + } + + [Test] + public void GetById_ReturnsUser_WhenRepositoryReturnsUser() + { + IUser expected = Mock.Of(); + _userRepositoryMock.Setup(x => x.Get(UserId)).Returns(expected); + + IUser? result = _sut.GetById(UserId); + + Assert.That(result, Is.SameAs(expected)); + } + + [TestCase(RuntimeLevel.Install)] + [TestCase(RuntimeLevel.Upgrade)] + [TestCase(RuntimeLevel.Upgrading)] + public void GetById_FallsBackToGetForUpgrade_WhenDbExceptionAndUpgrading(RuntimeLevel level) + { + IUser upgradeUser = Mock.Of(); + _userRepositoryMock.Setup(x => x.Get(UserId)).Throws(new TestDbException()); + _userRepositoryMock.Setup(x => x.GetForUpgrade(UserId)).Returns(upgradeUser); + _runtimeStateMock.SetupGet(x => x.Level).Returns(level); + + IUser? result = _sut.GetById(UserId); + + Assert.That(result, Is.SameAs(upgradeUser)); + } + + [TestCase(RuntimeLevel.Run)] + [TestCase(RuntimeLevel.Boot)] + [TestCase(RuntimeLevel.BootFailed)] + public void GetById_RethrowsDbException_WhenNotUpgrading(RuntimeLevel level) + { + _userRepositoryMock.Setup(x => x.Get(UserId)).Throws(new TestDbException()); + _runtimeStateMock.SetupGet(x => x.Level).Returns(level); + + Assert.Throws(() => _sut.GetById(UserId)); + } + + private sealed class TestDbException : DbException + { + public TestDbException() + : base("Simulated database exception") + { + } + } +}