From c055c616ea36b7d4c53f75f0ee3b46da70cac12d Mon Sep 17 00:00:00 2001 From: Zeegaan Date: Thu, 14 Mar 2024 12:41:36 +0100 Subject: [PATCH 01/20] Implement using keymap for member --- src/Umbraco.Core/Services/MemberService.cs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/Umbraco.Core/Services/MemberService.cs b/src/Umbraco.Core/Services/MemberService.cs index bf0b0935f501..69c198164fa8 100644 --- a/src/Umbraco.Core/Services/MemberService.cs +++ b/src/Umbraco.Core/Services/MemberService.cs @@ -20,8 +20,8 @@ public class MemberService : RepositoryService, IMemberService private readonly IMemberTypeRepository _memberTypeRepository; private readonly IMemberGroupRepository _memberGroupRepository; private readonly IAuditRepository _auditRepository; - private readonly IMemberGroupService _memberGroupService; + private readonly Lazy _idKeyMap; #region Constructor @@ -33,13 +33,15 @@ public MemberService( IMemberRepository memberRepository, IMemberTypeRepository memberTypeRepository, IMemberGroupRepository memberGroupRepository, - IAuditRepository auditRepository) + IAuditRepository auditRepository, + Lazy idKeyMap) : base(provider, loggerFactory, eventMessagesFactory) { _memberRepository = memberRepository; _memberTypeRepository = memberTypeRepository; _memberGroupRepository = memberGroupRepository; _auditRepository = auditRepository; + _idKeyMap = idKeyMap; _memberGroupService = memberGroupService ?? throw new ArgumentNullException(nameof(memberGroupService)); } @@ -319,8 +321,7 @@ private IMember CreateMemberWithIdentity(string username, string email, string n { using ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true); scope.ReadLock(Constants.Locks.MemberTree); - IQuery query = Query().Where(x => x.Key == id); - return _memberRepository.Get(query)?.FirstOrDefault(); + return GetMemberFromRepository(id); } [Obsolete($"Use {nameof(GetById)}. Will be removed in V15.")] @@ -1055,6 +1056,12 @@ public ContentDataIntegrityReport CheckDataIntegrity(ContentDataIntegrityReportO private void Audit(AuditType type, int userId, int objectId, string? message = null) => _auditRepository.Save(new AuditItem(objectId, type, userId, ObjectTypes.GetName(UmbracoObjectTypes.Member), message)); + private IMember? GetMemberFromRepository(Guid id) + => _idKeyMap.Value.GetIdForKey(id, UmbracoObjectTypes.Member) switch + { + { Success: false } => null, + { Result: var intId } => _memberRepository.Get(intId), + }; #endregion #region Membership From 6c1c2bd9fd70dc9afa79f22c611530a1a29e1a1d Mon Sep 17 00:00:00 2001 From: Zeegaan Date: Thu, 14 Mar 2024 14:00:56 +0100 Subject: [PATCH 02/20] Remove current usages of GetUserById --- .../AuditLog/CurrentUserAuditLogController.cs | 13 +++++-------- .../CreateUnattendedUserNotificationHandler.cs | 2 +- src/Umbraco.Core/Events/UserNotificationsHandler.cs | 2 +- .../Handlers/AuditNotificationsHandler.cs | 2 +- src/Umbraco.Core/Services/MetricsConsentService.cs | 2 +- .../Installer/Steps/CreateUserStep.cs | 2 +- .../Security/BackofficeSecurity.cs | 12 ++++++++++-- 7 files changed, 20 insertions(+), 15 deletions(-) diff --git a/src/Umbraco.Cms.Api.Management/Controllers/AuditLog/CurrentUserAuditLogController.cs b/src/Umbraco.Cms.Api.Management/Controllers/AuditLog/CurrentUserAuditLogController.cs index 0867e0796ca7..da64ac093f5a 100644 --- a/src/Umbraco.Cms.Api.Management/Controllers/AuditLog/CurrentUserAuditLogController.cs +++ b/src/Umbraco.Cms.Api.Management/Controllers/AuditLog/CurrentUserAuditLogController.cs @@ -8,6 +8,7 @@ using Umbraco.Cms.Core.Exceptions; using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Models.Membership; +using Umbraco.Cms.Core.Security; using Umbraco.Cms.Core.Services; namespace Umbraco.Cms.Api.Management.Controllers.AuditLog; @@ -17,16 +18,16 @@ public class CurrentUserAuditLogController : AuditLogControllerBase { private readonly IAuditService _auditService; private readonly IAuditLogPresentationFactory _auditLogPresentationFactory; - private readonly IUserService _userService; + private readonly IBackOfficeSecurityAccessor _backOfficeSecurityAccessor; public CurrentUserAuditLogController( IAuditService auditService, IAuditLogPresentationFactory auditLogPresentationFactory, - IUserService userService) + IBackOfficeSecurityAccessor backOfficeSecurityAccessor) { _auditService = auditService; _auditLogPresentationFactory = auditLogPresentationFactory; - _userService = userService; + _backOfficeSecurityAccessor = backOfficeSecurityAccessor; } [HttpGet] @@ -34,11 +35,7 @@ public CurrentUserAuditLogController( [ProducesResponseType(typeof(PagedViewModel), StatusCodes.Status200OK)] public async Task CurrentUser(Direction orderDirection = Direction.Descending, DateTime? sinceDate = null, int skip = 0, int take = 100) { - // FIXME: Pull out current backoffice user when its implemented. - // var userId = _backOfficeSecurityAccessor.BackOfficeSecurity?.GetUserId().Result ?? -1; - var userId = Constants.Security.SuperUserId; - - IUser? user = _userService.GetUserById(userId); + IUser? user = _backOfficeSecurityAccessor.BackOfficeSecurity?.CurrentUser; if (user is null) { diff --git a/src/Umbraco.Cms.Api.Management/Install/CreateUnattendedUserNotificationHandler.cs b/src/Umbraco.Cms.Api.Management/Install/CreateUnattendedUserNotificationHandler.cs index a1ee165da7ee..396df4eede7d 100644 --- a/src/Umbraco.Cms.Api.Management/Install/CreateUnattendedUserNotificationHandler.cs +++ b/src/Umbraco.Cms.Api.Management/Install/CreateUnattendedUserNotificationHandler.cs @@ -52,7 +52,7 @@ public async Task HandleAsync(UnattendedInstallNotification notification, Cancel return; } - IUser? admin = _userService.GetUserById(Constants.Security.SuperUserId); + IUser? admin = _userService.GetAsync(Constants.Security.SuperUserKey).GetAwaiter().GetResult(); if (admin == null) { throw new InvalidOperationException("Could not find the super user!"); diff --git a/src/Umbraco.Core/Events/UserNotificationsHandler.cs b/src/Umbraco.Core/Events/UserNotificationsHandler.cs index 6d15ec36aadb..ebc7840fa1df 100644 --- a/src/Umbraco.Core/Events/UserNotificationsHandler.cs +++ b/src/Umbraco.Core/Events/UserNotificationsHandler.cs @@ -197,7 +197,7 @@ public void Notify(IAction? action, params IContent[] entities) _logger.LogDebug( "There is no current Umbraco user logged in, the notifications will be sent from the administrator"); } - user = _userService.GetUserById(Constants.Security.SuperUserId); + user = _userService.GetAsync(Constants.Security.SuperUserKey).GetAwaiter().GetResult(); if (user == null) { _logger.LogWarning( diff --git a/src/Umbraco.Core/Handlers/AuditNotificationsHandler.cs b/src/Umbraco.Core/Handlers/AuditNotificationsHandler.cs index 23393b77f04a..89b15d7f37e6 100644 --- a/src/Umbraco.Core/Handlers/AuditNotificationsHandler.cs +++ b/src/Umbraco.Core/Handlers/AuditNotificationsHandler.cs @@ -82,7 +82,7 @@ private IUser CurrentPerformingUser get { IUser? identity = _backOfficeSecurityAccessor.BackOfficeSecurity?.CurrentUser; - IUser? user = identity == null ? null : _userService.GetUserById(Convert.ToInt32(identity.Id)); + IUser? user = identity == null ? null : _userService.GetAsync(identity.Key).GetAwaiter().GetResult(); return user ?? UnknownUser(_globalSettings); } } diff --git a/src/Umbraco.Core/Services/MetricsConsentService.cs b/src/Umbraco.Core/Services/MetricsConsentService.cs index be30458af619..e9b15452eb1b 100644 --- a/src/Umbraco.Core/Services/MetricsConsentService.cs +++ b/src/Umbraco.Core/Services/MetricsConsentService.cs @@ -71,7 +71,7 @@ public void SetConsentLevel(TelemetryLevel telemetryLevel) IUser? currentUser = _backOfficeSecurityAccessor.BackOfficeSecurity?.CurrentUser; if (currentUser is null) { - currentUser = _userService.GetUserById(Constants.Security.SuperUserId); + currentUser = _userService.GetAsync(Constants.Security.SuperUserKey).GetAwaiter().GetResult(); } _logger.LogInformation("Telemetry level set to {telemetryLevel} by {username}", telemetryLevel, currentUser?.Username); diff --git a/src/Umbraco.Infrastructure/Installer/Steps/CreateUserStep.cs b/src/Umbraco.Infrastructure/Installer/Steps/CreateUserStep.cs index 09a291d5277f..bd457bc3e604 100644 --- a/src/Umbraco.Infrastructure/Installer/Steps/CreateUserStep.cs +++ b/src/Umbraco.Infrastructure/Installer/Steps/CreateUserStep.cs @@ -59,7 +59,7 @@ public CreateUserStep( public async Task> ExecuteAsync(InstallData model) { - IUser? admin = _userService.GetUserById(Constants.Security.SuperUserId); + IUser? admin = _userService.GetAsync(Constants.Security.SuperUserKey).GetAwaiter().GetResult(); if (admin is null) { return FailWithMessage("Could not find the super user"); diff --git a/src/Umbraco.Web.Common/Security/BackofficeSecurity.cs b/src/Umbraco.Web.Common/Security/BackofficeSecurity.cs index f772ad583cfd..e8b13af1b156 100644 --- a/src/Umbraco.Web.Common/Security/BackofficeSecurity.cs +++ b/src/Umbraco.Web.Common/Security/BackofficeSecurity.cs @@ -36,10 +36,10 @@ public IUser? CurrentUser // Check again if (_currentUser == null) { - Attempt id = GetUserId(); + Attempt id = GetUserKey(); if (id.Success) { - _currentUser = id.Success ? _userService.GetUserById(id.Result) : null; + _currentUser = id.Success ? _userService.GetAsync(id.Result).GetAwaiter().GetResult() : null; } } } @@ -49,6 +49,14 @@ public IUser? CurrentUser } } + private Attempt GetUserKey() + { + ClaimsIdentity? identity = _httpContextAccessor.HttpContext?.GetCurrentIdentity(); + + Guid? id = identity?.GetUserKey(); + return id.HasValue is false ? Attempt.Fail() : Attempt.Succeed(id.Value); + } + /// public Attempt GetUserId() { From daeb19597de6488fa9c237ecd877aa27d0a00db0 Mon Sep 17 00:00:00 2001 From: Zeegaan Date: Thu, 14 Mar 2024 14:15:03 +0100 Subject: [PATCH 03/20] User userId resolver to resolve user key --- .../Factories/AuditLogPresentationFactory.cs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/Umbraco.Cms.Api.Management/Factories/AuditLogPresentationFactory.cs b/src/Umbraco.Cms.Api.Management/Factories/AuditLogPresentationFactory.cs index 64ada9d07457..6bde8ac27a77 100644 --- a/src/Umbraco.Cms.Api.Management/Factories/AuditLogPresentationFactory.cs +++ b/src/Umbraco.Cms.Api.Management/Factories/AuditLogPresentationFactory.cs @@ -17,14 +17,16 @@ public class AuditLogPresentationFactory : IAuditLogPresentationFactory private readonly MediaFileManager _mediaFileManager; private readonly IImageUrlGenerator _imageUrlGenerator; private readonly IEntityService _entityService; + private readonly IUserIdKeyResolver _userIdKeyResolver; - public AuditLogPresentationFactory(IUserService userService, AppCaches appCaches, MediaFileManager mediaFileManager, IImageUrlGenerator imageUrlGenerator, IEntityService entityService) + public AuditLogPresentationFactory(IUserService userService, AppCaches appCaches, MediaFileManager mediaFileManager, IImageUrlGenerator imageUrlGenerator, IEntityService entityService, IUserIdKeyResolver userIdKeyResolver) { _userService = userService; _appCaches = appCaches; _mediaFileManager = mediaFileManager; _imageUrlGenerator = imageUrlGenerator; _entityService = entityService; + _userIdKeyResolver = userIdKeyResolver; } public IEnumerable CreateAuditLogViewModel(IEnumerable auditItems) => auditItems.Select(CreateAuditLogViewModel); @@ -46,7 +48,8 @@ private AuditLogResponseModel CreateAuditLogViewModel(IAuditItem auditItem) private T CreateResponseModel(IAuditItem auditItem, out IUser user) where T : AuditLogBaseModel, new() { - user = _userService.GetUserById(auditItem.UserId) + Guid userKey = _userIdKeyResolver.GetAsync(auditItem.UserId).GetAwaiter().GetResult(); + user = _userService.GetAsync(userKey).GetAwaiter().GetResult() ?? throw new ArgumentException($"Could not find user with id {auditItem.UserId}"); IEntitySlim? entitySlim = _entityService.Get(auditItem.Id); From 0d9a3de4dc5aea6e9fbd2b487f17ced28c768d43 Mon Sep 17 00:00:00 2001 From: Zeegaan Date: Thu, 14 Mar 2024 14:41:08 +0100 Subject: [PATCH 04/20] Refactor user repository to use GUID not int --- .../Repositories/IUserRepository.cs | 4 +- src/Umbraco.Core/Services/IUserService.cs | 2 +- .../Services/NotificationService.cs | 65 ++++++++----------- src/Umbraco.Core/Services/UserService.cs | 64 ++++++++++++++++-- .../Repositories/Implement/UserRepository.cs | 39 +++-------- .../Security/BackOfficeUserStore.cs | 14 ++-- 6 files changed, 107 insertions(+), 81 deletions(-) diff --git a/src/Umbraco.Core/Persistence/Repositories/IUserRepository.cs b/src/Umbraco.Core/Persistence/Repositories/IUserRepository.cs index dbb96478d6a4..c7763baf3deb 100644 --- a/src/Umbraco.Core/Persistence/Repositories/IUserRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/IUserRepository.cs @@ -4,7 +4,7 @@ namespace Umbraco.Cms.Core.Persistence.Repositories; -public interface IUserRepository : IReadWriteQueryRepository +public interface IUserRepository : IReadWriteQueryRepository { /// /// Gets the count of items based on a complex query @@ -142,5 +142,5 @@ IEnumerable GetPagedResultsByQuery( void ClearLoginSession(Guid sessionId); - IEnumerable GetNextUsers(int id, int count); + IEnumerable GetNextUsers(Guid key, int count); } diff --git a/src/Umbraco.Core/Services/IUserService.cs b/src/Umbraco.Core/Services/IUserService.cs index 8d6259b1712d..5047b12ee855 100644 --- a/src/Umbraco.Core/Services/IUserService.cs +++ b/src/Umbraco.Core/Services/IUserService.cs @@ -322,7 +322,7 @@ IEnumerable GetAll( /// IEnumerable GetAllNotInGroup(int groupId); - IEnumerable GetNextUsers(int id, int count); + IEnumerable GetNextUsers(Guid key, int count); #region User groups diff --git a/src/Umbraco.Core/Services/NotificationService.cs b/src/Umbraco.Core/Services/NotificationService.cs index 336435d97ad5..142738d49f0e 100644 --- a/src/Umbraco.Core/Services/NotificationService.cs +++ b/src/Umbraco.Core/Services/NotificationService.cs @@ -94,52 +94,41 @@ public void SendNotifications( // lazily get versions var prevVersionDictionary = new Dictionary(); - // see notes above - var id = Constants.Security.SuperUserId; - const int pagesz = 400; // load batches of 400 users - do + var notifications = GetUsersNotifications(new List(), action, Enumerable.Empty(), Constants.ObjectTypes.Document)?.ToList(); + if (notifications is null || notifications.Count == 0) { - var notifications = GetUsersNotifications(new List(), action, Enumerable.Empty(), Constants.ObjectTypes.Document)?.ToList(); - if (notifications is null || notifications.Count == 0) - { - break; - } + return; + } - // users are returned ordered by id, notifications are returned ordered by user id - var users = _userService.GetNextUsers(id, pagesz).Where(x => x.IsApproved).ToList(); - foreach (IUser user in users) + IUser[] users = _userService.GetAll(0, int.MaxValue, out _).ToArray(); + foreach (IUser user in users) + { + Notification[] userNotifications = notifications.Where(n => n.UserId == user.Id).ToArray(); + foreach (Notification notification in userNotifications) { - Notification[] userNotifications = notifications.Where(n => n.UserId == user.Id).ToArray(); - foreach (Notification notification in userNotifications) + // notifications are inherited down the tree - find the topmost entity + // relevant to this notification (entity list is sorted by path) + IContent? entityForNotification = entitiesL + .FirstOrDefault(entity => + pathsByEntityId.TryGetValue(entity.Id, out var path) && + path.Contains(notification.EntityId)); + + if (entityForNotification == null) { - // notifications are inherited down the tree - find the topmost entity - // relevant to this notification (entity list is sorted by path) - IContent? entityForNotification = entitiesL - .FirstOrDefault(entity => - pathsByEntityId.TryGetValue(entity.Id, out var path) && - path.Contains(notification.EntityId)); - - if (entityForNotification == null) - { - continue; - } - - if (prevVersionDictionary.ContainsKey(entityForNotification.Id) == false) - { - prevVersionDictionary[entityForNotification.Id] = GetPreviousVersion(entityForNotification.Id); - } + continue; + } - // queue notification - NotificationRequest req = CreateNotificationRequest(operatingUser, user, entityForNotification, prevVersionDictionary[entityForNotification.Id], actionName, siteUri, createSubject, createBody); - Enqueue(req); - break; + if (prevVersionDictionary.ContainsKey(entityForNotification.Id) == false) + { + prevVersionDictionary[entityForNotification.Id] = GetPreviousVersion(entityForNotification.Id); } - } - // load more users if any - id = users.Count == pagesz ? users.Last().Id + 1 : -1; + // queue notification + NotificationRequest req = CreateNotificationRequest(operatingUser, user, entityForNotification, prevVersionDictionary[entityForNotification.Id], actionName, siteUri, createSubject, createBody); + Enqueue(req); + break; + } } - while (id > 0); } /// diff --git a/src/Umbraco.Core/Services/UserService.cs b/src/Umbraco.Core/Services/UserService.cs index 00d158590aac..84e121fe2467 100644 --- a/src/Umbraco.Core/Services/UserService.cs +++ b/src/Umbraco.Core/Services/UserService.cs @@ -7,6 +7,7 @@ using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; using Umbraco.Cms.Core.Configuration.Models; +using Umbraco.Cms.Core.DependencyInjection; using Umbraco.Cms.Core.Editors; using Umbraco.Cms.Core.Events; using Umbraco.Cms.Core.Exceptions; @@ -50,7 +51,9 @@ internal class UserService : RepositoryService, IUserService private readonly IIsoCodeValidator _isoCodeValidator; private readonly IUserRepository _userRepository; private readonly ContentSettings _contentSettings; + private readonly IUserIdKeyResolver _userIdKeyResolver; + [Obsolete("Use the constructor that takes an IUserIdKeyResolver instead. Scheduled for removal in V15.")] public UserService( ICoreScopeProvider provider, ILoggerFactory loggerFactory, @@ -70,6 +73,49 @@ public UserService( IOptions contentSettings, IIsoCodeValidator isoCodeValidator, IUserForgotPasswordSender forgotPasswordSender) + : this( + provider, + loggerFactory, + eventMessagesFactory, + userRepository, + userGroupRepository, + globalSettings, + securitySettings, + userEditorAuthorizationHelper, + serviceScopeFactory, + entityService, + localLoginSettingProvider, + inviteSender, + mediaFileManager, + temporaryFileService, + shortStringHelper, + contentSettings, + isoCodeValidator, + forgotPasswordSender, + StaticServiceProvider.Instance.GetRequiredService()) + { + } + + public UserService( + ICoreScopeProvider provider, + ILoggerFactory loggerFactory, + IEventMessagesFactory eventMessagesFactory, + IUserRepository userRepository, + IUserGroupRepository userGroupRepository, + IOptions globalSettings, + IOptions securitySettings, + UserEditorAuthorizationHelper userEditorAuthorizationHelper, + IServiceScopeFactory serviceScopeFactory, + IEntityService entityService, + ILocalLoginSettingProvider localLoginSettingProvider, + IUserInviteSender inviteSender, + MediaFileManager mediaFileManager, + ITemporaryFileService temporaryFileService, + IShortStringHelper shortStringHelper, + IOptions contentSettings, + IIsoCodeValidator isoCodeValidator, + IUserForgotPasswordSender forgotPasswordSender, + IUserIdKeyResolver userIdKeyResolver) : base(provider, loggerFactory, eventMessagesFactory) { _userRepository = userRepository; @@ -84,6 +130,7 @@ public UserService( _shortStringHelper = shortStringHelper; _isoCodeValidator = isoCodeValidator; _forgotPasswordSender = forgotPasswordSender; + _userIdKeyResolver = userIdKeyResolver; _globalSettings = globalSettings.Value; _securitySettings = securitySettings.Value; _contentSettings = contentSettings.Value; @@ -187,11 +234,13 @@ public IUser CreateUserWithIdentity(string username, string email) => /// /// /// + [Obsolete("Please use GetAsync instead. Scheduled for removal in V15.")] public IUser? GetById(int id) { using (ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true)) { - return _userRepository.Get(id); + Guid userKey = _userIdKeyResolver.GetAsync(id).GetAwaiter().GetResult(); + return _userRepository.Get(userKey); } } @@ -1669,11 +1718,11 @@ public IEnumerable GetAll(long pageIndex, int pageSize, out long totalRec } } - public IEnumerable GetNextUsers(int id, int count) + public IEnumerable GetNextUsers(Guid key, int count) { using (ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true)) { - return _userRepository.GetNextUsers(id, count); + return _userRepository.GetNextUsers(key, count); } } @@ -1969,10 +2018,17 @@ public void Save(IUserGroup userGroup, int[]? userIds = null) userGroup.HasIdentity ? _userRepository.GetAllInGroup(userGroup.Id).ToArray() : empty; var xGroupUsers = groupUsers.ToDictionary(x => x.Id, x => x); var groupIds = groupUsers.Select(x => x.Id).ToArray(); + var addedUserKeys = new List(); + foreach (var userId in userIds.Except(groupIds)) + { + Guid userKey = _userIdKeyResolver.GetAsync(userId).GetAwaiter().GetResult(); + addedUserKeys.Add(userKey); + } + IEnumerable addedUserIds = userIds.Except(groupIds); addedUsers = addedUserIds.Count() > 0 - ? _userRepository.GetMany(addedUserIds.ToArray()).Where(x => x.Id != 0).ToArray() + ? _userRepository.GetMany(addedUserKeys.ToArray()).Where(x => x.Id != 0).ToArray() : new IUser[] { }; removedUsers = groupIds.Except(userIds).Select(x => xGroupUsers[x]).Where(x => x.Id != 0).ToArray(); } diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs index 0b78a30f0f29..40523d0dc14c 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs @@ -25,7 +25,7 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement; /// /// Represents the UserRepository for doing CRUD operations for /// -internal class UserRepository : EntityRepositoryBase, IUserRepository +internal class UserRepository : EntityRepositoryBase, IUserRepository { private readonly IMapperCollection _mapperCollection; private readonly GlobalSettings _globalSettings; @@ -108,32 +108,12 @@ private IEnumerable ConvertFromDtos(IEnumerable dtos) => #region Overrides of RepositoryBase - protected override IUser? PerformGet(int id) + protected override IUser? PerformGet(Guid key) { - // This will never resolve to a user, yet this is asked - // for all of the time (especially in cases of members). - // Don't issue a SQL call for this, we know it will not exist. - if (_runtimeState.Level == RuntimeLevel.Upgrade) - { - // when upgrading people might come from version 7 where user 0 was the default, - // only in upgrade mode do we want to fetch the user of Id 0 - if (id < -1) - { - return null; - } - } - else - { - if (id == default || id < -1) - { - return null; - } - } - Sql sql = SqlContext.Sql() .Select() .From() - .Where(x => x.Id == id); + .Where(x => x.Key == key); List? dtos = Database.Fetch(sql); if (dtos.Count == 0) @@ -345,7 +325,8 @@ public void ClearLoginSession(Guid sessionId) => .Update(u => u.Set(x => x.LoggedOutUtc, DateTime.UtcNow)) .Where(x => x.SessionId == sessionId)); - protected override IEnumerable PerformGetAll(params int[]? ids) + + protected override IEnumerable PerformGetAll(params Guid[]? ids) { List dtos = ids?.Length == 0 ? GetDtosWith(null, true) @@ -1196,16 +1177,16 @@ private Sql ApplySort(Sql sql, Expression GetNextUsers(int id, int count) + public IEnumerable GetNextUsers(Guid key, int count) { Sql idsQuery = SqlContext.Sql() - .Select(x => x.Id) + .Select(x => x.Key) .From() - .Where(x => x.Id >= id) - .OrderBy(x => x.Id); + .Where(x => x.Key >= key) + .OrderBy(x => x.Key); // first page is index 1, not zero - var ids = Database.Page(1, count, idsQuery).Items.ToArray(); + Guid[] ids = Database.Page(1, count, idsQuery).Items.ToArray(); // now get the actual users and ensure they are ordered properly (same clause) return ids.Length == 0 diff --git a/src/Umbraco.Infrastructure/Security/BackOfficeUserStore.cs b/src/Umbraco.Infrastructure/Security/BackOfficeUserStore.cs index 9a639926d5b8..abda8d139ba9 100644 --- a/src/Umbraco.Infrastructure/Security/BackOfficeUserStore.cs +++ b/src/Umbraco.Infrastructure/Security/BackOfficeUserStore.cs @@ -249,7 +249,8 @@ public Task DisableAsync(IUser user) try { - return Task.FromResult(_userRepository.Get(id)); + IQuery query = _scopeProvider.CreateQuery().Where(x => x.Id == id); + return Task.FromResult(_userRepository.Get(query).FirstOrDefault()); } catch (DbException) { @@ -269,13 +270,14 @@ public Task DisableAsync(IUser user) public Task> GetUsersAsync(params int[]? ids) { - if (ids?.Length <= 0) + if (ids is null || ids.Length <= 0) { return Task.FromResult(Enumerable.Empty()); } using ICoreScope scope = _scopeProvider.CreateCoreScope(autoComplete: true); - IEnumerable users = _userRepository.GetMany(ids); + IQuery query = _scopeProvider.CreateQuery().Where(x => ids.Contains(x.Id)); + IEnumerable users = _userRepository.Get(query); return Task.FromResult(users); } @@ -288,8 +290,7 @@ public Task> GetUsersAsync(params Guid[]? keys) } using ICoreScope scope = _scopeProvider.CreateCoreScope(autoComplete: true); - IQuery query = _scopeProvider.CreateQuery().Where(x => keys.Contains(x.Key)); - IEnumerable users = _userRepository.Get(query); + IEnumerable users = _userRepository.GetMany(keys); return Task.FromResult(users); } @@ -298,8 +299,7 @@ public Task> GetUsersAsync(params Guid[]? keys) public Task GetAsync(Guid key) { using ICoreScope scope = _scopeProvider.CreateCoreScope(autoComplete: true); - IQuery query = _scopeProvider.CreateQuery().Where(x => x.Key == key); - return Task.FromResult(_userRepository.Get(query).FirstOrDefault()); + return Task.FromResult(_userRepository.Get(key)); } /// From b8466ab4df1ba887db3adbcd72a4f14375d987ff Mon Sep 17 00:00:00 2001 From: Zeegaan Date: Fri, 15 Mar 2024 09:34:33 +0100 Subject: [PATCH 05/20] Add happy path test --- .../Services/UserServiceCrudTests.Update.cs | 28 +++++++++++++++++++ .../Repositories/UserRepositoryTest.cs | 16 +++++------ 2 files changed, 36 insertions(+), 8 deletions(-) diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/UserServiceCrudTests.Update.cs b/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/UserServiceCrudTests.Update.cs index acb035774ed1..12f430cf1fcc 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/UserServiceCrudTests.Update.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/UserServiceCrudTests.Update.cs @@ -126,6 +126,34 @@ public async Task UserName_And_Email_Must_Be_same_When_UserNameIsEmail_Equals_Tr Assert.AreEqual(email, updatedUser.Email); } + [Test] + public async Task Can_Update_User_Name() + { + const string userName = "UpdateUserName"; + const string name = "UpdatedName"; + const string email = "update@email.com"; + var userService = CreateUserService(securitySettings: new SecuritySettings { UsernameIsEmail = false }); + + var (updateModel, createdUser) = await CreateUserForUpdate(userService); + + updateModel.UserName = userName; + updateModel.Email = email; + updateModel.Name = name; + + var result = await userService.UpdateAsync(Constants.Security.SuperUserKey, updateModel); + + Assert.IsTrue(result.Success); + var updatedUser = await userService.GetAsync(createdUser.Key); + Assert.Multiple(() => + { + Assert.IsNotNull(updatedUser); + Assert.AreEqual(userName, updatedUser.Username); + Assert.AreEqual(email, updatedUser.Email); + Assert.AreEqual(name, updatedUser.Name); + }); + + } + [Test] public async Task Cannot_Change_Email_To_Duplicate_Email_On_Update() { diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/UserRepositoryTest.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/UserRepositoryTest.cs index cce400878a37..a5ed0b40efd8 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/UserRepositoryTest.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/UserRepositoryTest.cs @@ -126,7 +126,7 @@ public void Can_Verify_Fresh_Entity_Is_Not_Dirty() repository.Save(user); // Act - var resolved = repository.Get(user.Id); + var resolved = repository.Get(user.Key); var dirty = ((User)resolved).IsDirty(); // Assert @@ -148,7 +148,7 @@ public void Can_Perform_Delete_On_UserRepository() // Act repository.Save(user); - var id = user.Id; + var id = user.Key; var mockRuntimeState = CreateMockRuntimeState(RuntimeLevel.Run); @@ -185,7 +185,7 @@ public void Can_Perform_Get_On_UserRepository() var user = CreateAndCommitUserWithGroup(repository, userGroupRepository); // Act - var updatedItem = repository.Get(user.Id); + var updatedItem = repository.Get(user.Key); // TODO: this test cannot work, user has 2 sections but the way it's created, // they don't show, so the comparison with updatedItem fails - fix! @@ -227,7 +227,7 @@ public void Can_Perform_GetAll_By_Param_Ids_On_UserRepository() var users = CreateAndCommitMultipleUsers(repository); // Act - var result = repository.GetMany(users[0].Id, users[1].Id).ToArray(); + var result = repository.GetMany(users[0].Key, users[1].Key).ToArray(); // Assert Assert.That(result, Is.Not.Null); @@ -269,7 +269,7 @@ public void Can_Perform_Exists_On_UserRepository() var users = CreateAndCommitMultipleUsers(repository); // Act - var exists = repository.Exists(users[0].Id); + var exists = repository.Exists(users[0].Key); // Assert Assert.That(exists, Is.True); @@ -396,7 +396,7 @@ public void Can_Invalidate_SecurityStamp_On_Username_Change() repository.Save(user); // Get the user - var updatedUser = repository.Get(user.Id); + var updatedUser = repository.Get(user.Key); // Ensure the Security Stamp is invalidated & no longer the same Assert.AreNotEqual(originalSecurityStamp, updatedUser.SecurityStamp); @@ -460,7 +460,7 @@ public void Can_Perform_Update_On_UserRepository() var user = CreateAndCommitUserWithGroup(userRepository, userGroupRepository); // Act - var resolved = (User)userRepository.Get(user.Id); + var resolved = (User)userRepository.Get(user.Key); resolved.Name = "New Name"; @@ -478,7 +478,7 @@ public void Can_Perform_Update_On_UserRepository() userRepository.Save(resolved); - var updatedItem = (User)userRepository.Get(user.Id); + var updatedItem = (User)userRepository.Get(user.Key); // Assert Assert.That(updatedItem.Id, Is.EqualTo(resolved.Id)); From 1adc45ee966016c9bb8a535ec3416e7d6be730e9 Mon Sep 17 00:00:00 2001 From: Zeegaan Date: Fri, 15 Mar 2024 09:57:37 +0100 Subject: [PATCH 06/20] Remove user in cache when user gets updated --- .../Cache/DistributedCacheExtensions.cs | 8 +++++++- .../Refreshers/Implement/UserCacheRefresher.cs | 15 +++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/src/Umbraco.Core/Cache/DistributedCacheExtensions.cs b/src/Umbraco.Core/Cache/DistributedCacheExtensions.cs index e02bee857b4f..1b89689a50cd 100644 --- a/src/Umbraco.Core/Cache/DistributedCacheExtensions.cs +++ b/src/Umbraco.Core/Cache/DistributedCacheExtensions.cs @@ -32,7 +32,13 @@ public static void RefreshUserCache(this DistributedCache dc, int userId) => dc.Refresh(UserCacheRefresher.UniqueId, userId); public static void RefreshUserCache(this DistributedCache dc, IEnumerable users) - => dc.Refresh(UserCacheRefresher.UniqueId, users.Select(x => x.Id).Distinct().ToArray()); + { + foreach (IUser user in users) + { + dc.Refresh(UserCacheRefresher.UniqueId, user.Key); + dc.Refresh(UserCacheRefresher.UniqueId, user.Key); + } + } public static void RefreshAllUserCache(this DistributedCache dc) => dc.RefreshAll(UserCacheRefresher.UniqueId); diff --git a/src/Umbraco.Core/Cache/Refreshers/Implement/UserCacheRefresher.cs b/src/Umbraco.Core/Cache/Refreshers/Implement/UserCacheRefresher.cs index d1dc194f9b20..84602e3887fe 100644 --- a/src/Umbraco.Core/Cache/Refreshers/Implement/UserCacheRefresher.cs +++ b/src/Umbraco.Core/Cache/Refreshers/Implement/UserCacheRefresher.cs @@ -36,6 +36,21 @@ public override void Refresh(int id) base.Refresh(id); } + public override void Refresh(Guid id) + { + Attempt userCache = AppCaches.IsolatedCaches.Get(); + if (userCache.Success) + { + userCache.Result?.Clear(RepositoryCacheKeys.GetKey(id)); + userCache.Result?.ClearByKey(CacheKeys.UserContentStartNodePathsPrefix + id); + userCache.Result?.ClearByKey(CacheKeys.UserMediaStartNodePathsPrefix + id); + userCache.Result?.ClearByKey(CacheKeys.UserAllContentStartNodesPrefix + id); + userCache.Result?.ClearByKey(CacheKeys.UserAllMediaStartNodesPrefix + id); + } + + base.Refresh(id); + } + public override void Remove(int id) { Attempt userCache = AppCaches.IsolatedCaches.Get(); From 14fdf07e67bf431a54b2e1625f5da2ae8a6438c8 Mon Sep 17 00:00:00 2001 From: Zeegaan Date: Fri, 15 Mar 2024 12:19:23 +0100 Subject: [PATCH 07/20] Use await in async method --- .../Install/CreateUnattendedUserNotificationHandler.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Umbraco.Cms.Api.Management/Install/CreateUnattendedUserNotificationHandler.cs b/src/Umbraco.Cms.Api.Management/Install/CreateUnattendedUserNotificationHandler.cs index 396df4eede7d..0a96dcbe6b75 100644 --- a/src/Umbraco.Cms.Api.Management/Install/CreateUnattendedUserNotificationHandler.cs +++ b/src/Umbraco.Cms.Api.Management/Install/CreateUnattendedUserNotificationHandler.cs @@ -52,7 +52,7 @@ public async Task HandleAsync(UnattendedInstallNotification notification, Cancel return; } - IUser? admin = _userService.GetAsync(Constants.Security.SuperUserKey).GetAwaiter().GetResult(); + IUser? admin = await _userService.GetAsync(Constants.Security.SuperUserKey); if (admin == null) { throw new InvalidOperationException("Could not find the super user!"); From a75acaaa08439b7f4d0dbe55eef25813d9d6c997 Mon Sep 17 00:00:00 2001 From: Nikolaj Geisle <70372949+Zeegaan@users.noreply.github.com> Date: Thu, 21 Mar 2024 11:42:27 +0100 Subject: [PATCH 08/20] Fix up according to review --- .../Controllers/AuditLog/CurrentUserAuditLogController.cs | 7 +------ src/Umbraco.Core/Cache/DistributedCacheExtensions.cs | 2 +- src/Umbraco.Core/Services/MemberService.cs | 6 +++--- 3 files changed, 5 insertions(+), 10 deletions(-) diff --git a/src/Umbraco.Cms.Api.Management/Controllers/AuditLog/CurrentUserAuditLogController.cs b/src/Umbraco.Cms.Api.Management/Controllers/AuditLog/CurrentUserAuditLogController.cs index da64ac093f5a..d951b8266098 100644 --- a/src/Umbraco.Cms.Api.Management/Controllers/AuditLog/CurrentUserAuditLogController.cs +++ b/src/Umbraco.Cms.Api.Management/Controllers/AuditLog/CurrentUserAuditLogController.cs @@ -35,12 +35,7 @@ public CurrentUserAuditLogController( [ProducesResponseType(typeof(PagedViewModel), StatusCodes.Status200OK)] public async Task CurrentUser(Direction orderDirection = Direction.Descending, DateTime? sinceDate = null, int skip = 0, int take = 100) { - IUser? user = _backOfficeSecurityAccessor.BackOfficeSecurity?.CurrentUser; - - if (user is null) - { - throw new PanicException("Could not find current user"); - } + IUser user = CurrentUser(_backOfficeSecurityAccessor); PagedModel result = await _auditService.GetPagedItemsByUserAsync( user.Key, diff --git a/src/Umbraco.Core/Cache/DistributedCacheExtensions.cs b/src/Umbraco.Core/Cache/DistributedCacheExtensions.cs index 1b89689a50cd..8282466a542d 100644 --- a/src/Umbraco.Core/Cache/DistributedCacheExtensions.cs +++ b/src/Umbraco.Core/Cache/DistributedCacheExtensions.cs @@ -36,7 +36,7 @@ public static void RefreshUserCache(this DistributedCache dc, IEnumerable foreach (IUser user in users) { dc.Refresh(UserCacheRefresher.UniqueId, user.Key); - dc.Refresh(UserCacheRefresher.UniqueId, user.Key); + dc.Refresh(UserCacheRefresher.UniqueId, user.Id); } } diff --git a/src/Umbraco.Core/Services/MemberService.cs b/src/Umbraco.Core/Services/MemberService.cs index 6435bf5c5c58..5ddec1e3aaa1 100644 --- a/src/Umbraco.Core/Services/MemberService.cs +++ b/src/Umbraco.Core/Services/MemberService.cs @@ -21,7 +21,7 @@ public class MemberService : RepositoryService, IMemberService private readonly IMemberGroupRepository _memberGroupRepository; private readonly IAuditRepository _auditRepository; private readonly IMemberGroupService _memberGroupService; - private readonly Lazy _idKeyMap; + private readonly IIdKeyMap _idKeyMap; #region Constructor @@ -34,7 +34,7 @@ public MemberService( IMemberTypeRepository memberTypeRepository, IMemberGroupRepository memberGroupRepository, IAuditRepository auditRepository, - Lazy idKeyMap) + IIdKeyMap idKeyMap) : base(provider, loggerFactory, eventMessagesFactory) { _memberRepository = memberRepository; @@ -1071,7 +1071,7 @@ public ContentDataIntegrityReport CheckDataIntegrity(ContentDataIntegrityReportO private void Audit(AuditType type, int userId, int objectId, string? message = null) => _auditRepository.Save(new AuditItem(objectId, type, userId, ObjectTypes.GetName(UmbracoObjectTypes.Member), message)); private IMember? GetMemberFromRepository(Guid id) - => _idKeyMap.Value.GetIdForKey(id, UmbracoObjectTypes.Member) switch + => _idKeyMap.GetIdForKey(id, UmbracoObjectTypes.Member) switch { { Success: false } => null, { Result: var intId } => _memberRepository.Get(intId), From c5682e8def32a422dd4c09d6a7d7bf41e82eeb94 Mon Sep 17 00:00:00 2001 From: Nikolaj Geisle <70372949+Zeegaan@users.noreply.github.com> Date: Thu, 21 Mar 2024 11:51:07 +0100 Subject: [PATCH 09/20] Update IMetricsConsentService.cs to have async method --- .../Controllers/Telemetry/SetTelemetryController.cs | 2 +- src/Umbraco.Core/Services/IMetricsConsentService.cs | 3 +++ src/Umbraco.Core/Services/MetricsConsentService.cs | 11 +++++------ .../Installer/Steps/CreateUserStep.cs | 2 +- .../Services/MetricsConsentServiceTest.cs | 8 ++++---- .../Umbraco.Core/Telemetry/TelemetryServiceTests.cs | 4 ++-- 6 files changed, 16 insertions(+), 14 deletions(-) diff --git a/src/Umbraco.Cms.Api.Management/Controllers/Telemetry/SetTelemetryController.cs b/src/Umbraco.Cms.Api.Management/Controllers/Telemetry/SetTelemetryController.cs index d61133f60e44..9cc02f6c4e8e 100644 --- a/src/Umbraco.Cms.Api.Management/Controllers/Telemetry/SetTelemetryController.cs +++ b/src/Umbraco.Cms.Api.Management/Controllers/Telemetry/SetTelemetryController.cs @@ -31,7 +31,7 @@ public async Task SetConsentLevel(TelemetryRequestModel telemetry return BadRequest(invalidModelProblem); } - _metricsConsentService.SetConsentLevel(telemetryRepresentationBase.TelemetryLevel); + await _metricsConsentService.SetConsentLevelAsync(telemetryRepresentationBase.TelemetryLevel); return await Task.FromResult(Ok()); } } diff --git a/src/Umbraco.Core/Services/IMetricsConsentService.cs b/src/Umbraco.Core/Services/IMetricsConsentService.cs index 72f3ebe87361..e210a8925d23 100644 --- a/src/Umbraco.Core/Services/IMetricsConsentService.cs +++ b/src/Umbraco.Core/Services/IMetricsConsentService.cs @@ -6,5 +6,8 @@ public interface IMetricsConsentService { TelemetryLevel GetConsentLevel(); + [Obsolete("Please use SetConsentLevelAsync instead, scheduled for removal in V15")] void SetConsentLevel(TelemetryLevel telemetryLevel); + + Task SetConsentLevelAsync(TelemetryLevel telemetryLevel); } diff --git a/src/Umbraco.Core/Services/MetricsConsentService.cs b/src/Umbraco.Core/Services/MetricsConsentService.cs index e9b15452eb1b..9b57069842ac 100644 --- a/src/Umbraco.Core/Services/MetricsConsentService.cs +++ b/src/Umbraco.Core/Services/MetricsConsentService.cs @@ -66,13 +66,12 @@ public TelemetryLevel GetConsentLevel() return analyticsLevel; } - public void SetConsentLevel(TelemetryLevel telemetryLevel) + [Obsolete("Please use SetConsentLevelAsync instead, scheduled for removal in V15")] + public void SetConsentLevel(TelemetryLevel telemetryLevel) => SetConsentLevelAsync(telemetryLevel).GetAwaiter().GetResult(); + + public async Task SetConsentLevelAsync(TelemetryLevel telemetryLevel) { - IUser? currentUser = _backOfficeSecurityAccessor.BackOfficeSecurity?.CurrentUser; - if (currentUser is null) - { - currentUser = _userService.GetAsync(Constants.Security.SuperUserKey).GetAwaiter().GetResult(); - } + IUser? currentUser = _backOfficeSecurityAccessor.BackOfficeSecurity?.CurrentUser ?? await _userService.GetAsync(Constants.Security.SuperUserKey); _logger.LogInformation("Telemetry level set to {telemetryLevel} by {username}", telemetryLevel, currentUser?.Username); _keyValueService.SetValue(Key, telemetryLevel.ToString()); diff --git a/src/Umbraco.Infrastructure/Installer/Steps/CreateUserStep.cs b/src/Umbraco.Infrastructure/Installer/Steps/CreateUserStep.cs index bd457bc3e604..b4fe7b23b9b8 100644 --- a/src/Umbraco.Infrastructure/Installer/Steps/CreateUserStep.cs +++ b/src/Umbraco.Infrastructure/Installer/Steps/CreateUserStep.cs @@ -92,7 +92,7 @@ public async Task> ExecuteAsync(InstallData model) return FailWithMessage("Could not reset password: " + string.Join(", ", resetResult.Errors.ToErrorMessage())); } - _metricsConsentService.SetConsentLevel(model.TelemetryLevel); + await _metricsConsentService.SetConsentLevelAsync(model.TelemetryLevel); if (model.User.SubscribeToNewsletter) { diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/MetricsConsentServiceTest.cs b/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/MetricsConsentServiceTest.cs index a1fdabe6862c..b3c55c5deaed 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/MetricsConsentServiceTest.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/MetricsConsentServiceTest.cs @@ -18,9 +18,9 @@ public class MetricsConsentServiceTest : UmbracoIntegrationTest [TestCase(TelemetryLevel.Minimal)] [TestCase(TelemetryLevel.Basic)] [TestCase(TelemetryLevel.Detailed)] - public void Can_Store_Consent(TelemetryLevel level) + public async Task Can_Store_Consent(TelemetryLevel level) { - MetricsConsentService.SetConsentLevel(level); + await MetricsConsentService.SetConsentLevelAsync(level); var actual = MetricsConsentService.GetConsentLevel(); Assert.IsNotNull(actual); @@ -28,9 +28,9 @@ public void Can_Store_Consent(TelemetryLevel level) } [Test] - public void Enum_Stored_as_string() + public async Task Enum_Stored_as_string() { - MetricsConsentService.SetConsentLevel(TelemetryLevel.Detailed); + await MetricsConsentService.SetConsentLevelAsync(TelemetryLevel.Detailed); var stringValue = KeyValueService.GetValue(Cms.Core.Services.MetricsConsentService.Key); diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Core/Telemetry/TelemetryServiceTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Core/Telemetry/TelemetryServiceTests.cs index 03c4fd8633e5..f736240e7659 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Core/Telemetry/TelemetryServiceTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Core/Telemetry/TelemetryServiceTests.cs @@ -23,7 +23,7 @@ protected override void CustomTestSetup(IUmbracoBuilder builder) => private IMetricsConsentService MetricsConsentService => GetRequiredService(); [Test] - public void Expected_Detailed_Telemetry_Exists() + public async Task Expected_Detailed_Telemetry_Exists() { var expectedData = new[] { @@ -54,7 +54,7 @@ public void Expected_Detailed_Telemetry_Exists() Constants.Telemetry.DeliveryApiPublicAccess }; - MetricsConsentService.SetConsentLevel(TelemetryLevel.Detailed); + await MetricsConsentService.SetConsentLevelAsync(TelemetryLevel.Detailed); var success = TelemetryService.TryGetTelemetryReportData(out var telemetryReportData); var detailed = telemetryReportData.Detailed.ToArray(); From 92ca3bacbf3f12a05b7596c620b4a4f2b942c14e Mon Sep 17 00:00:00 2001 From: Nikolaj Geisle <70372949+Zeegaan@users.noreply.github.com> Date: Thu, 21 Mar 2024 11:55:29 +0100 Subject: [PATCH 10/20] Fix according to review --- src/Umbraco.Core/Security/IBackofficeSecurity.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Umbraco.Core/Security/IBackofficeSecurity.cs b/src/Umbraco.Core/Security/IBackofficeSecurity.cs index 2de9104a95df..d0a60e18d8c5 100644 --- a/src/Umbraco.Core/Security/IBackofficeSecurity.cs +++ b/src/Umbraco.Core/Security/IBackofficeSecurity.cs @@ -21,6 +21,7 @@ public interface IBackOfficeSecurity /// The current user's Id that has been authenticated for the request. /// If authentication hasn't taken place this will be unsuccessful. // TODO: This should just be an extension method on ClaimsIdentity + [Obsolete("Scheduled for removal in V15")] Attempt GetUserId(); /// From 526bda460f67ed7c02b33b647788d9cee430c5ce Mon Sep 17 00:00:00 2001 From: Nikolaj Geisle <70372949+Zeegaan@users.noreply.github.com> Date: Thu, 21 Mar 2024 12:00:29 +0100 Subject: [PATCH 11/20] Fix more according to comments --- .../Cache/DistributedCacheExtensions.cs | 1 - .../Implement/UserCacheRefresher.cs | 21 ------------------- .../Repositories/IUserRepository.cs | 2 -- src/Umbraco.Core/Services/IUserService.cs | 2 -- src/Umbraco.Core/Services/UserService.cs | 8 ------- .../Repositories/Implement/UserRepository.cs | 18 ---------------- 6 files changed, 52 deletions(-) diff --git a/src/Umbraco.Core/Cache/DistributedCacheExtensions.cs b/src/Umbraco.Core/Cache/DistributedCacheExtensions.cs index 8282466a542d..2fb74d079e1e 100644 --- a/src/Umbraco.Core/Cache/DistributedCacheExtensions.cs +++ b/src/Umbraco.Core/Cache/DistributedCacheExtensions.cs @@ -36,7 +36,6 @@ public static void RefreshUserCache(this DistributedCache dc, IEnumerable foreach (IUser user in users) { dc.Refresh(UserCacheRefresher.UniqueId, user.Key); - dc.Refresh(UserCacheRefresher.UniqueId, user.Id); } } diff --git a/src/Umbraco.Core/Cache/Refreshers/Implement/UserCacheRefresher.cs b/src/Umbraco.Core/Cache/Refreshers/Implement/UserCacheRefresher.cs index 84602e3887fe..094bb11a4aa2 100644 --- a/src/Umbraco.Core/Cache/Refreshers/Implement/UserCacheRefresher.cs +++ b/src/Umbraco.Core/Cache/Refreshers/Implement/UserCacheRefresher.cs @@ -30,12 +30,6 @@ public override void RefreshAll() base.RefreshAll(); } - public override void Refresh(int id) - { - Remove(id); - base.Refresh(id); - } - public override void Refresh(Guid id) { Attempt userCache = AppCaches.IsolatedCaches.Get(); @@ -51,20 +45,5 @@ public override void Refresh(Guid id) base.Refresh(id); } - public override void Remove(int id) - { - Attempt userCache = AppCaches.IsolatedCaches.Get(); - if (userCache.Success) - { - userCache.Result?.Clear(RepositoryCacheKeys.GetKey(id)); - userCache.Result?.ClearByKey(CacheKeys.UserContentStartNodePathsPrefix + id); - userCache.Result?.ClearByKey(CacheKeys.UserMediaStartNodePathsPrefix + id); - userCache.Result?.ClearByKey(CacheKeys.UserAllContentStartNodesPrefix + id); - userCache.Result?.ClearByKey(CacheKeys.UserAllMediaStartNodesPrefix + id); - } - - base.Remove(id); - } - #endregion } diff --git a/src/Umbraco.Core/Persistence/Repositories/IUserRepository.cs b/src/Umbraco.Core/Persistence/Repositories/IUserRepository.cs index c7763baf3deb..0f0bdc962619 100644 --- a/src/Umbraco.Core/Persistence/Repositories/IUserRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/IUserRepository.cs @@ -141,6 +141,4 @@ IEnumerable GetPagedResultsByQuery( int ClearLoginSessions(TimeSpan timespan); void ClearLoginSession(Guid sessionId); - - IEnumerable GetNextUsers(Guid key, int count); } diff --git a/src/Umbraco.Core/Services/IUserService.cs b/src/Umbraco.Core/Services/IUserService.cs index 5047b12ee855..d386800f219d 100644 --- a/src/Umbraco.Core/Services/IUserService.cs +++ b/src/Umbraco.Core/Services/IUserService.cs @@ -322,8 +322,6 @@ IEnumerable GetAll( /// IEnumerable GetAllNotInGroup(int groupId); - IEnumerable GetNextUsers(Guid key, int count); - #region User groups /// diff --git a/src/Umbraco.Core/Services/UserService.cs b/src/Umbraco.Core/Services/UserService.cs index 84e121fe2467..89791e0ad60e 100644 --- a/src/Umbraco.Core/Services/UserService.cs +++ b/src/Umbraco.Core/Services/UserService.cs @@ -1718,14 +1718,6 @@ public IEnumerable GetAll(long pageIndex, int pageSize, out long totalRec } } - public IEnumerable GetNextUsers(Guid key, int count) - { - using (ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true)) - { - return _userRepository.GetNextUsers(key, count); - } - } - /// /// 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 40523d0dc14c..2fe1e4cfba65 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs @@ -1176,23 +1176,5 @@ private Sql ApplySort(Sql sql, Expression GetNextUsers(Guid key, int count) - { - Sql idsQuery = SqlContext.Sql() - .Select(x => x.Key) - .From() - .Where(x => x.Key >= key) - .OrderBy(x => x.Key); - - // first page is index 1, not zero - Guid[] ids = Database.Page(1, count, idsQuery).Items.ToArray(); - - // now get the actual users and ensure they are ordered properly (same clause) - return ids.Length == 0 - ? Enumerable.Empty() - : GetMany(ids).OrderBy(x => x.Id) ?? Enumerable.Empty(); - } - #endregion } From 1688f95e1d1d4b44e6e8fa41624a2f8fe908bcfc Mon Sep 17 00:00:00 2001 From: Nikolaj Geisle <70372949+Zeegaan@users.noreply.github.com> Date: Thu, 21 Mar 2024 14:22:55 +0100 Subject: [PATCH 12/20] Revert "Fix up according to review" This reverts commit a75acaaa --- .../Controllers/AuditLog/CurrentUserAuditLogController.cs | 7 ++++++- src/Umbraco.Core/Cache/DistributedCacheExtensions.cs | 1 + src/Umbraco.Core/Services/MemberService.cs | 6 +++--- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/Umbraco.Cms.Api.Management/Controllers/AuditLog/CurrentUserAuditLogController.cs b/src/Umbraco.Cms.Api.Management/Controllers/AuditLog/CurrentUserAuditLogController.cs index d951b8266098..da64ac093f5a 100644 --- a/src/Umbraco.Cms.Api.Management/Controllers/AuditLog/CurrentUserAuditLogController.cs +++ b/src/Umbraco.Cms.Api.Management/Controllers/AuditLog/CurrentUserAuditLogController.cs @@ -35,7 +35,12 @@ public CurrentUserAuditLogController( [ProducesResponseType(typeof(PagedViewModel), StatusCodes.Status200OK)] public async Task CurrentUser(Direction orderDirection = Direction.Descending, DateTime? sinceDate = null, int skip = 0, int take = 100) { - IUser user = CurrentUser(_backOfficeSecurityAccessor); + IUser? user = _backOfficeSecurityAccessor.BackOfficeSecurity?.CurrentUser; + + if (user is null) + { + throw new PanicException("Could not find current user"); + } PagedModel result = await _auditService.GetPagedItemsByUserAsync( user.Key, diff --git a/src/Umbraco.Core/Cache/DistributedCacheExtensions.cs b/src/Umbraco.Core/Cache/DistributedCacheExtensions.cs index 2fb74d079e1e..8282466a542d 100644 --- a/src/Umbraco.Core/Cache/DistributedCacheExtensions.cs +++ b/src/Umbraco.Core/Cache/DistributedCacheExtensions.cs @@ -36,6 +36,7 @@ public static void RefreshUserCache(this DistributedCache dc, IEnumerable foreach (IUser user in users) { dc.Refresh(UserCacheRefresher.UniqueId, user.Key); + dc.Refresh(UserCacheRefresher.UniqueId, user.Id); } } diff --git a/src/Umbraco.Core/Services/MemberService.cs b/src/Umbraco.Core/Services/MemberService.cs index 5ddec1e3aaa1..6435bf5c5c58 100644 --- a/src/Umbraco.Core/Services/MemberService.cs +++ b/src/Umbraco.Core/Services/MemberService.cs @@ -21,7 +21,7 @@ public class MemberService : RepositoryService, IMemberService private readonly IMemberGroupRepository _memberGroupRepository; private readonly IAuditRepository _auditRepository; private readonly IMemberGroupService _memberGroupService; - private readonly IIdKeyMap _idKeyMap; + private readonly Lazy _idKeyMap; #region Constructor @@ -34,7 +34,7 @@ public MemberService( IMemberTypeRepository memberTypeRepository, IMemberGroupRepository memberGroupRepository, IAuditRepository auditRepository, - IIdKeyMap idKeyMap) + Lazy idKeyMap) : base(provider, loggerFactory, eventMessagesFactory) { _memberRepository = memberRepository; @@ -1071,7 +1071,7 @@ public ContentDataIntegrityReport CheckDataIntegrity(ContentDataIntegrityReportO private void Audit(AuditType type, int userId, int objectId, string? message = null) => _auditRepository.Save(new AuditItem(objectId, type, userId, ObjectTypes.GetName(UmbracoObjectTypes.Member), message)); private IMember? GetMemberFromRepository(Guid id) - => _idKeyMap.GetIdForKey(id, UmbracoObjectTypes.Member) switch + => _idKeyMap.Value.GetIdForKey(id, UmbracoObjectTypes.Member) switch { { Success: false } => null, { Result: var intId } => _memberRepository.Get(intId), From e8481e3f8a9080cb34c914e0cab15437db2dd31e Mon Sep 17 00:00:00 2001 From: Nikolaj Geisle <70372949+Zeegaan@users.noreply.github.com> Date: Thu, 21 Mar 2024 14:37:37 +0100 Subject: [PATCH 13/20] Get current backoffice user from method --- .../Controllers/AuditLog/CurrentUserAuditLogController.cs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/Umbraco.Cms.Api.Management/Controllers/AuditLog/CurrentUserAuditLogController.cs b/src/Umbraco.Cms.Api.Management/Controllers/AuditLog/CurrentUserAuditLogController.cs index da64ac093f5a..d951b8266098 100644 --- a/src/Umbraco.Cms.Api.Management/Controllers/AuditLog/CurrentUserAuditLogController.cs +++ b/src/Umbraco.Cms.Api.Management/Controllers/AuditLog/CurrentUserAuditLogController.cs @@ -35,12 +35,7 @@ public CurrentUserAuditLogController( [ProducesResponseType(typeof(PagedViewModel), StatusCodes.Status200OK)] public async Task CurrentUser(Direction orderDirection = Direction.Descending, DateTime? sinceDate = null, int skip = 0, int take = 100) { - IUser? user = _backOfficeSecurityAccessor.BackOfficeSecurity?.CurrentUser; - - if (user is null) - { - throw new PanicException("Could not find current user"); - } + IUser user = CurrentUser(_backOfficeSecurityAccessor); PagedModel result = await _auditService.GetPagedItemsByUserAsync( user.Key, From 40406072e2144539aa42fd0d505e01c8ad997dff Mon Sep 17 00:00:00 2001 From: Nikolaj Geisle <70372949+Zeegaan@users.noreply.github.com> Date: Fri, 22 Mar 2024 09:20:13 +0100 Subject: [PATCH 14/20] Update user repository delete functionality --- .../Persistence/Repositories/Implement/UserRepository.cs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs index 2fe1e4cfba65..4f4c9f3b335a 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs @@ -106,7 +106,7 @@ private string? DefaultPasswordConfigJson private IEnumerable ConvertFromDtos(IEnumerable dtos) => dtos.Select(x => UserFactory.BuildEntity(_globalSettings, x, _permissionMappers)); - #region Overrides of RepositoryBase + #region Overrides of RepositoryBase protected override IUser? PerformGet(Guid key) { @@ -125,6 +125,8 @@ private IEnumerable ConvertFromDtos(IEnumerable dtos) => return UserFactory.BuildEntity(_globalSettings, dtos[0], _permissionMappers); } + protected override Guid GetEntityId(IUser entity) => entity.Key; + /// /// Returns a user by username /// @@ -330,7 +332,7 @@ protected override IEnumerable PerformGetAll(params Guid[]? ids) { List dtos = ids?.Length == 0 ? GetDtosWith(null, true) - : GetDtosWith(sql => sql.WhereIn(x => x.Id, ids), true); + : GetDtosWith(sql => sql.WhereIn(x => x.Key, ids), true); var users = new IUser[dtos.Count]; var i = 0; foreach (UserDto dto in dtos) @@ -664,12 +666,13 @@ protected override IEnumerable GetDeleteClauses() }; return list; } + protected override void PersistDeletedItem(IUser entity) { IEnumerable deletes = GetDeleteClauses(); foreach (var delete in deletes) { - Database.Execute(delete, new { id = GetEntityId(entity), key = entity.Key }); + Database.Execute(delete, new { id = entity.Id, key = GetEntityId(entity) }); } entity.DeleteDate = DateTime.Now; From 950c73568412f5f381ad4e38ed8fe0df7e5087c3 Mon Sep 17 00:00:00 2001 From: Nikolaj Geisle <70372949+Zeegaan@users.noreply.github.com> Date: Fri, 22 Mar 2024 09:41:40 +0100 Subject: [PATCH 15/20] Fix up more test --- .../Repositories/Implement/UserRepository.cs | 10 ++++++++++ .../Scoping/ScopedRepositoryTests.cs | 16 ++++++++-------- 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs index 4f4c9f3b335a..655b73396dc5 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/UserRepository.cs @@ -896,6 +896,16 @@ public int GetCountByQuery(IQuery? query) return Database.ExecuteScalar(sql); } + protected override bool PerformExists(Guid key) + { + Sql sql = SqlContext.Sql() + .SelectCount() + .From() + .Where(x => x.Key == key); + + return Database.ExecuteScalar(sql) > 0; + } + public bool Exists(string username) => ExistsByUserName(username); public bool ExistsByUserName(string username) diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Scoping/ScopedRepositoryTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Scoping/ScopedRepositoryTests.cs index 0c09bf14e8f5..8e9206b1bf62 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Scoping/ScopedRepositoryTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Scoping/ScopedRepositoryTests.cs @@ -63,7 +63,7 @@ protected override void CustomTestSetup(IUmbracoBuilder builder) [TestCase(true)] [TestCase(false)] - public void DefaultRepositoryCachePolicy(bool complete) + public async Task DefaultRepositoryCachePolicy(bool complete) { var scopeProvider = (ScopeProvider)ScopeProvider; var service = (UserService)UserService; @@ -72,13 +72,13 @@ public void DefaultRepositoryCachePolicy(bool complete) service.Save(user); // User has been saved so the cache has been cleared of it - var globalCached = (IUser)globalCache.Get(GetCacheIdKey(user.Id), () => null); + var globalCached = (IUser)globalCache.Get(GetCacheIdKey(user.Key), () => null); Assert.IsNull(globalCached); // Get user again to load it into the cache again, this also ensure we don't modify the one that's in the cache. - user = service.GetUserById(user.Id); + user = await service.GetAsync(user.Key); // global cache contains the entity - globalCached = (IUser)globalCache.Get(GetCacheIdKey(user.Id), () => null); + globalCached = (IUser)globalCache.Get(GetCacheIdKey(user.Key), () => null); Assert.IsNotNull(globalCached); Assert.AreEqual(user.Id, globalCached.Id); Assert.AreEqual("name", globalCached.Name); @@ -104,7 +104,7 @@ public void DefaultRepositoryCachePolicy(bool complete) Assert.AreEqual("changed", scopeCached.Name); // global cache is unchanged - globalCached = (IUser)globalCache.Get(GetCacheIdKey(user.Id), () => null); + globalCached = (IUser)globalCache.Get(GetCacheIdKey(user.Key), () => null); Assert.IsNotNull(globalCached); Assert.AreEqual(user.Id, globalCached.Id); Assert.AreEqual("name", globalCached.Name); @@ -117,7 +117,7 @@ public void DefaultRepositoryCachePolicy(bool complete) Assert.IsNull(scopeProvider.AmbientScope); - globalCached = (IUser)globalCache.Get(GetCacheIdKey(user.Id), () => null); + globalCached = (IUser)globalCache.Get(GetCacheIdKey(user.Key), () => null); if (complete) { // global cache has been cleared @@ -130,11 +130,11 @@ public void DefaultRepositoryCachePolicy(bool complete) } // get again, updated if completed - user = service.GetUserById(user.Id); + user = await service.GetAsync(user.Key); Assert.AreEqual(complete ? "changed" : "name", user.Name); // global cache contains the entity again - globalCached = (IUser)globalCache.Get(GetCacheIdKey(user.Id), () => null); + globalCached = (IUser)globalCache.Get(GetCacheIdKey(user.Key), () => null); Assert.IsNotNull(globalCached); Assert.AreEqual(user.Id, globalCached.Id); Assert.AreEqual(complete ? "changed" : "name", globalCached.Name); From 865e1cc71226ec71927f020cb1bbf500a9cab0d8 Mon Sep 17 00:00:00 2001 From: Nikolaj Geisle <70372949+Zeegaan@users.noreply.github.com> Date: Fri, 22 Mar 2024 12:51:51 +0100 Subject: [PATCH 16/20] Try to get user by id if key fails --- .../Security/BackofficeSecurity.cs | 14 +++++++++++--- .../BackOfficeExamineSearcherTests.cs | 5 ++--- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/Umbraco.Web.Common/Security/BackofficeSecurity.cs b/src/Umbraco.Web.Common/Security/BackofficeSecurity.cs index e8b13af1b156..e873646a4cbe 100644 --- a/src/Umbraco.Web.Common/Security/BackofficeSecurity.cs +++ b/src/Umbraco.Web.Common/Security/BackofficeSecurity.cs @@ -36,10 +36,18 @@ public IUser? CurrentUser // Check again if (_currentUser == null) { - Attempt id = GetUserKey(); - if (id.Success) + Attempt keyAttempt = GetUserKey(); + if (keyAttempt.Success) { - _currentUser = id.Success ? _userService.GetAsync(id.Result).GetAwaiter().GetResult() : null; + _currentUser = keyAttempt.Success ? _userService.GetAsync(keyAttempt.Result).GetAwaiter().GetResult() : null; + } + + // The key attempt can fail in certain scenarios (especially our integration tests) so we can fall back on using the non-cached user by id + // This also aligns with behavior in our IAuthorizationHelper + else + { + Attempt idAttempt = GetUserId(); + _currentUser = idAttempt.Success ? _userService.GetUserById(idAttempt.Result) : null; } } } diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Examine.Lucene/UmbracoExamine/BackOfficeExamineSearcherTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Examine.Lucene/UmbracoExamine/BackOfficeExamineSearcherTests.cs index f0545a552d77..e7d8ebbdb27d 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Examine.Lucene/UmbracoExamine/BackOfficeExamineSearcherTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Examine.Lucene/UmbracoExamine/BackOfficeExamineSearcherTests.cs @@ -85,8 +85,7 @@ private IEnumerable BackOfficeExamineSearch(string query, int pag private async Task SetupUserIdentity(string userId) { - var identity = - await BackOfficeUserStore.FindByIdAsync(userId, CancellationToken.None); + var identity = await BackOfficeUserStore.FindByIdAsync(userId, CancellationToken.None); await BackOfficeSignInManager.SignInAsync(identity, false); var principal = await BackOfficeSignInManager.CreateUserPrincipalAsync(identity); HttpContextAccessor.HttpContext.SetPrincipalForRequest(principal); @@ -595,7 +594,7 @@ public async Task Search_Published_Content_With_Two_Languages_By_Id() public async Task Check_All_Indexed_Values_For_Published_Content_With_No_Properties() { // Arrange - await SetupUserIdentity(Constants.Security.SuperUserIdAsString); + await SetupUserIdentity(Constants.Security.SuperUserKey.ToString()); const string contentName = "TestContent"; From b9cf5f4961e2f953eddddc8bc1ad735860582b56 Mon Sep 17 00:00:00 2001 From: Zeegaan Date: Tue, 2 Apr 2024 09:41:42 +0200 Subject: [PATCH 17/20] Add user key as required claim --- .../Extensions/ClaimsIdentityExtensions.cs | 15 ++++++++++++++- .../Security/BackOfficeClaimsPrincipalFactory.cs | 1 + .../Security/BackofficeSecurity.cs | 13 +------------ 3 files changed, 16 insertions(+), 13 deletions(-) diff --git a/src/Umbraco.Core/Extensions/ClaimsIdentityExtensions.cs b/src/Umbraco.Core/Extensions/ClaimsIdentityExtensions.cs index 726801279004..234eea77c409 100644 --- a/src/Umbraco.Core/Extensions/ClaimsIdentityExtensions.cs +++ b/src/Umbraco.Core/Extensions/ClaimsIdentityExtensions.cs @@ -193,6 +193,7 @@ public static bool VerifyBackOfficeIdentity( /// /// this /// The users Id + /// The users key /// Username /// Real name /// Start content nodes @@ -201,7 +202,7 @@ public static bool VerifyBackOfficeIdentity( /// Security stamp /// Allowed apps /// Roles - public static void AddRequiredClaims(this ClaimsIdentity identity, string userId, string username, string realName, IEnumerable? startContentNodes, IEnumerable? startMediaNodes, string culture, string securityStamp, IEnumerable allowedApps, IEnumerable roles) + public static void AddRequiredClaims(this ClaimsIdentity identity, string userId, Guid userKey, string username, string realName, IEnumerable? startContentNodes, IEnumerable? startMediaNodes, string culture, string securityStamp, IEnumerable allowedApps, IEnumerable roles) { // This is the id that 'identity' uses to check for the user id if (identity.HasClaim(x => x.Type == ClaimTypes.NameIdentifier) == false) @@ -215,6 +216,18 @@ public static void AddRequiredClaims(this ClaimsIdentity identity, string userId identity)); } + // This is the id that 'identity' uses to check for the user id + if (identity.HasClaim(x => x.Type == "sub") == false) + { + identity.AddClaim(new Claim( + "sub", + userKey.ToString(), + ClaimValueTypes.String, + AuthenticationType, + AuthenticationType, + identity)); + } + if (identity.HasClaim(x => x.Type == ClaimTypes.Name) == false) { identity.AddClaim(new Claim( diff --git a/src/Umbraco.Infrastructure/Security/BackOfficeClaimsPrincipalFactory.cs b/src/Umbraco.Infrastructure/Security/BackOfficeClaimsPrincipalFactory.cs index c1a69d24e7c9..eeac48801d3c 100644 --- a/src/Umbraco.Infrastructure/Security/BackOfficeClaimsPrincipalFactory.cs +++ b/src/Umbraco.Infrastructure/Security/BackOfficeClaimsPrincipalFactory.cs @@ -51,6 +51,7 @@ protected override async Task GenerateClaimsAsync(BackOfficeIden // ensure our required claims are there id.AddRequiredClaims( user.Id, + user.Key, user.UserName!, user.Name!, user.CalculatedContentStartNodeIds, diff --git a/src/Umbraco.Web.Common/Security/BackofficeSecurity.cs b/src/Umbraco.Web.Common/Security/BackofficeSecurity.cs index e873646a4cbe..270bea8f01a0 100644 --- a/src/Umbraco.Web.Common/Security/BackofficeSecurity.cs +++ b/src/Umbraco.Web.Common/Security/BackofficeSecurity.cs @@ -37,18 +37,7 @@ public IUser? CurrentUser if (_currentUser == null) { Attempt keyAttempt = GetUserKey(); - if (keyAttempt.Success) - { - _currentUser = keyAttempt.Success ? _userService.GetAsync(keyAttempt.Result).GetAwaiter().GetResult() : null; - } - - // The key attempt can fail in certain scenarios (especially our integration tests) so we can fall back on using the non-cached user by id - // This also aligns with behavior in our IAuthorizationHelper - else - { - Attempt idAttempt = GetUserId(); - _currentUser = idAttempt.Success ? _userService.GetUserById(idAttempt.Result) : null; - } + _currentUser = keyAttempt.Success ? _userService.GetAsync(keyAttempt.Result).GetAwaiter().GetResult() : null; } } } From f1ac8cce843c1111b0fa0af55156f12f9285755f Mon Sep 17 00:00:00 2001 From: Nikolaj Geisle <70372949+Zeegaan@users.noreply.github.com> Date: Wed, 3 Apr 2024 09:25:15 +0200 Subject: [PATCH 18/20] Fix tests --- .../Umbraco.Core/BackOffice/UmbracoBackOfficeIdentityTests.cs | 3 ++- .../Umbraco.Core/Extensions/ClaimsPrincipalExtensionsTests.cs | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/BackOffice/UmbracoBackOfficeIdentityTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/BackOffice/UmbracoBackOfficeIdentityTests.cs index d1e980b19c92..a8ac651f9fc2 100644 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/BackOffice/UmbracoBackOfficeIdentityTests.cs +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/BackOffice/UmbracoBackOfficeIdentityTests.cs @@ -111,6 +111,7 @@ public void Create_With_Claims_And_User_Data() claimsIdentity.AddRequiredClaims( "1234", + Guid.NewGuid(), "testing", "hello world", new[] { 654 }, @@ -120,7 +121,7 @@ public void Create_With_Claims_And_User_Data() new[] { "content", "media" }, new[] { "admin" }); - Assert.AreEqual(12, claimsIdentity.Claims.Count()); + Assert.AreEqual(13, claimsIdentity.Claims.Count()); Assert.IsNull(claimsIdentity.Actor); } } diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Extensions/ClaimsPrincipalExtensionsTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Extensions/ClaimsPrincipalExtensionsTests.cs index 9d5c4bb48a92..4e21e422bd8e 100644 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Extensions/ClaimsPrincipalExtensionsTests.cs +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Extensions/ClaimsPrincipalExtensionsTests.cs @@ -18,6 +18,7 @@ public void Get_Remaining_Ticket_Seconds() var backOfficeIdentity = new ClaimsIdentity(); backOfficeIdentity.AddRequiredClaims( Constants.Security.SuperUserIdAsString, + Constants.Security.SuperUserKey, "test", "test", Enumerable.Empty(), @@ -55,6 +56,7 @@ public void AddOrUpdateClaim__Should_ensure_a_claim_is_not_added_twice() var backOfficeIdentity = new ClaimsIdentity(); backOfficeIdentity.AddRequiredClaims( Constants.Security.SuperUserIdAsString, + Constants.Security.SuperUserKey, "test", "test", Enumerable.Empty(), From cad96c808b73ef5bcd9b380f3ce8997c3681a18c Mon Sep 17 00:00:00 2001 From: Zeegaan Date: Thu, 11 Apr 2024 12:51:30 +0200 Subject: [PATCH 19/20] Don't set claim in BackofficeController --- .../Controllers/Security/BackOfficeController.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Umbraco.Cms.Api.Management/Controllers/Security/BackOfficeController.cs b/src/Umbraco.Cms.Api.Management/Controllers/Security/BackOfficeController.cs index f65e7f597ed0..e65eafb457e4 100644 --- a/src/Umbraco.Cms.Api.Management/Controllers/Security/BackOfficeController.cs +++ b/src/Umbraco.Cms.Api.Management/Controllers/Security/BackOfficeController.cs @@ -253,7 +253,6 @@ private async Task AuthorizeExternal(OpenIddictRequest request) private async Task SignInBackOfficeUser(BackOfficeIdentityUser backOfficeUser, OpenIddictRequest request) { ClaimsPrincipal backOfficePrincipal = await _backOfficeSignInManager.CreateUserPrincipalAsync(backOfficeUser); - backOfficePrincipal.SetClaim(OpenIddictConstants.Claims.Subject, backOfficeUser.Key.ToString()); Claim[] backOfficeClaims = backOfficePrincipal.Claims.ToArray(); foreach (Claim backOfficeClaim in backOfficeClaims) From 924678d71745cf38f0bd30da8cb009f471c631a6 Mon Sep 17 00:00:00 2001 From: Zeegaan Date: Thu, 11 Apr 2024 13:19:14 +0200 Subject: [PATCH 20/20] Create constant for the Sub claim --- src/Umbraco.Core/Constants-Security.cs | 5 +++++ src/Umbraco.Core/Extensions/ClaimsIdentityExtensions.cs | 4 ++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/Umbraco.Core/Constants-Security.cs b/src/Umbraco.Core/Constants-Security.cs index ca8d5efb2543..842add06fa7d 100644 --- a/src/Umbraco.Core/Constants-Security.cs +++ b/src/Umbraco.Core/Constants-Security.cs @@ -112,6 +112,11 @@ public static class Security /// public const string SecurityStampClaimType = "AspNet.Identity.SecurityStamp"; + /// + /// The claim type for the mandatory OpenIdDict sub claim + /// + public const string OpenIdDictSubClaimType = "sub"; + public const string AspNetCoreV3PasswordHashAlgorithmName = "PBKDF2.ASPNETCORE.V3"; public const string AspNetCoreV2PasswordHashAlgorithmName = "PBKDF2.ASPNETCORE.V2"; public const string AspNetUmbraco8PasswordHashAlgorithmName = "HMACSHA256"; diff --git a/src/Umbraco.Core/Extensions/ClaimsIdentityExtensions.cs b/src/Umbraco.Core/Extensions/ClaimsIdentityExtensions.cs index 234eea77c409..4ff3782019a5 100644 --- a/src/Umbraco.Core/Extensions/ClaimsIdentityExtensions.cs +++ b/src/Umbraco.Core/Extensions/ClaimsIdentityExtensions.cs @@ -217,10 +217,10 @@ public static void AddRequiredClaims(this ClaimsIdentity identity, string userId } // This is the id that 'identity' uses to check for the user id - if (identity.HasClaim(x => x.Type == "sub") == false) + if (identity.HasClaim(x => x.Type == Constants.Security.OpenIdDictSubClaimType) == false) { identity.AddClaim(new Claim( - "sub", + Constants.Security.OpenIdDictSubClaimType, userKey.ToString(), ClaimValueTypes.String, AuthenticationType,