From 419625a9195b11e8a7ef80e540168de3a41ed488 Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Tue, 20 May 2025 15:58:27 +0100 Subject: [PATCH] Optimize the member save as part of the member login process, by-passing locking and audit steps and handling only the expected update properties (#19308) --- .../Repositories/IMemberRepository.cs | 7 ++ src/Umbraco.Core/Services/IMemberService.cs | 7 ++ src/Umbraco.Core/Services/MemberService.cs | 42 ++++++++++++ .../Implement/MemberRepository.cs | 45 +++++++++++++ .../Security/MemberUserStore.cs | 64 ++++++++++++------- .../Security/MemberUserStoreTests.cs | 58 +++++++++++++++++ 6 files changed, 200 insertions(+), 23 deletions(-) diff --git a/src/Umbraco.Core/Persistence/Repositories/IMemberRepository.cs b/src/Umbraco.Core/Persistence/Repositories/IMemberRepository.cs index 94f9bcac6f..456769e7e4 100644 --- a/src/Umbraco.Core/Persistence/Repositories/IMemberRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/IMemberRepository.cs @@ -42,4 +42,11 @@ public interface IMemberRepository : IContentRepository int GetCountByQuery(IQuery? query); Task> GetPagedByFilterAsync(MemberFilter memberFilter,int skip, int take, Ordering? ordering = null); + + /// + /// Saves only the properties related to login for the member, using an optimized, non-locking update. + /// + /// The member to update. + /// Used to avoid the full save of the member object after a login operation. + Task UpdateLoginPropertiesAsync(IMember member) => Task.CompletedTask; } diff --git a/src/Umbraco.Core/Services/IMemberService.cs b/src/Umbraco.Core/Services/IMemberService.cs index a84d7de07d..c44095a40f 100644 --- a/src/Umbraco.Core/Services/IMemberService.cs +++ b/src/Umbraco.Core/Services/IMemberService.cs @@ -427,4 +427,11 @@ public interface IMemberService : IMembershipMemberService, IContentServiceBase< /// /// IEnumerable? GetMembersByPropertyValue(string propertyTypeAlias, DateTime value, ValuePropertyMatchType matchType = ValuePropertyMatchType.Exact); + + /// + /// Saves only the properties related to login for the member, using an optimized, non-locking update. + /// + /// The member to update. + /// Used to avoid the full save of the member object after a login operation. + Task UpdateLoginPropertiesAsync(IMember member) => Task.CompletedTask; } diff --git a/src/Umbraco.Core/Services/MemberService.cs b/src/Umbraco.Core/Services/MemberService.cs index 86efb2abfd..c2499a9efe 100644 --- a/src/Umbraco.Core/Services/MemberService.cs +++ b/src/Umbraco.Core/Services/MemberService.cs @@ -854,6 +854,48 @@ namespace Umbraco.Cms.Core.Services public void Save(IEnumerable members) => Save(members, Constants.Security.SuperUserId); + /// + /// + /// + /// Note that in this optimized member save operation for use in the login process, where we only handle login related + /// properties, we aren't taking any locks. If we were updating "content" properties, that could have relations between each + /// other, we should following what we do for documents and lock. + /// But here we are just updating these system fields, and it's fine if they work in a "last one wins" fashion without locking. + /// + /// + /// Note also that we aren't calling "Audit" here (as well as to optimize performance, this is deliberate, because this is not + /// a full save operation on the member that we'd want to audit who made the changes via the backoffice or API; rather it's + /// just the member logging in as themselves). + /// + /// + /// We are though publishing notifications, to maintain backwards compatibility for any solutions using these for + /// processing following a member login. + /// + /// + /// These notification handlers will ensure that the records to umbracoLog are also added in the same way as they + /// are for a full save operation. + /// + /// + public async Task UpdateLoginPropertiesAsync(IMember member) + { + EventMessages evtMsgs = EventMessagesFactory.Get(); + + using ICoreScope scope = ScopeProvider.CreateCoreScope(); + var savingNotification = new MemberSavingNotification(member, evtMsgs); + savingNotification.State.Add("LoginPropertiesOnly", true); + if (scope.Notifications.PublishCancelable(savingNotification)) + { + scope.Complete(); + return; + } + + await _memberRepository.UpdateLoginPropertiesAsync(member); + + scope.Notifications.Publish(new MemberSavedNotification(member, evtMsgs).WithStateFrom(savingNotification)); + + scope.Complete(); + } + #endregion #region Delete diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/MemberRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/MemberRepository.cs index 9adc60426f..beff74a5e3 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/MemberRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/MemberRepository.cs @@ -947,5 +947,50 @@ public class MemberRepository : ContentRepositoryBase + public async Task UpdateLoginPropertiesAsync(IMember member) + { + var updatedLastLoginDate = member.IsPropertyDirty(nameof(member.LastLoginDate)); + var updatedSecurityStamp = member.IsPropertyDirty(nameof(member.SecurityStamp)); + if (updatedLastLoginDate is false && updatedSecurityStamp is false) + { + return; + } + + NPocoSqlExtensions.SqlUpd GetMemberSetExpression(IMember member, NPocoSqlExtensions.SqlUpd m) + { + var setExpression = new NPocoSqlExtensions.SqlUpd(SqlContext); + if (updatedLastLoginDate) + { + setExpression.Set(x => x.LastLoginDate, member.LastLoginDate); + } + + if (updatedSecurityStamp) + { + setExpression.Set(x => x.SecurityStampToken, member.SecurityStamp); + } + + return setExpression; + } + + member.UpdatingEntity(); + + Sql updateMemberQuery = Sql() + .Update(m => GetMemberSetExpression(member, m)) + .Where(m => m.NodeId == member.Id); + await Database.ExecuteAsync(updateMemberQuery); + + Sql updateContentVersionQuery = Sql() + .Update(m => m.Set(x => x.VersionDate, member.UpdateDate)) + .Where(m => m.NodeId == member.Id && m.Current == true); + await Database.ExecuteAsync(updateContentVersionQuery); + + OnUowRefreshedEntity(new MemberRefreshNotification(member, new EventMessages())); + + _memberByUsernameCachePolicy.DeleteByUserName(CacheKeys.MemberUserNameCachePrefix, member.Username); + + member.ResetDirtyProperties(); + } + #endregion } diff --git a/src/Umbraco.Infrastructure/Security/MemberUserStore.cs b/src/Umbraco.Infrastructure/Security/MemberUserStore.cs index b86458427f..a9c293b19c 100644 --- a/src/Umbraco.Infrastructure/Security/MemberUserStore.cs +++ b/src/Umbraco.Infrastructure/Security/MemberUserStore.cs @@ -162,7 +162,7 @@ public class MemberUserStore : UmbracoUserStore - public override Task UpdateAsync( + public override async Task UpdateAsync( MemberIdentityUser user, CancellationToken cancellationToken = default) { @@ -190,9 +190,21 @@ public class MemberUserStore : UmbracoUserStore propertiesUpdated = UpdateMemberProperties(found, user, out var updateRoles); + + if (propertiesUpdated.Count > 0) { - _memberService.Save(found); + // As part of logging in members we update the last login date, and, if concurrent logins are disabled, the security stamp. + // If and only if we are updating these properties, we can avoid the overhead of a full save of the member with the associated + // locking, property updates, tag handling etc., and make a more efficient update. + if (UpdatingOnlyLoginProperties(propertiesUpdated)) + { + await _memberService.UpdateLoginPropertiesAsync(found); + } + else + { + _memberService.Save(found); + } if (updateRoles) { @@ -223,15 +235,21 @@ public class MemberUserStore : UmbracoUserStore propertiesUpdated) + { + string[] loginPropertyUpdates = [nameof(MemberIdentityUser.LastLoginDateUtc), nameof(MemberIdentityUser.SecurityStamp)]; + return (propertiesUpdated.Count == 2 && propertiesUpdated.ContainsAll(loginPropertyUpdates)) || + (propertiesUpdated.Count == 1 && propertiesUpdated.ContainsAny(loginPropertyUpdates)); + } + /// public override Task DeleteAsync( MemberIdentityUser user, @@ -704,9 +722,9 @@ public class MemberUserStore : UmbracoUserStore UpdateMemberProperties(IMember member, MemberIdentityUser identityUser, out bool updateRoles) { - var anythingChanged = false; + var updatedProperties = new List(); updateRoles = false; // don't assign anything if nothing has changed as this will trigger the track changes of the model @@ -715,7 +733,7 @@ public class MemberUserStore : UmbracoUserStore diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberUserStoreTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberUserStoreTests.cs index 8b388a482d..1976f925b0 100644 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberUserStoreTests.cs +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberUserStoreTests.cs @@ -206,6 +206,64 @@ public class MemberUserStoreTests _mockMemberService.Verify(x => x.ReplaceRoles(new[] { 123 }, new[] { "role1", "role2" })); } + [Test] + public async Task GivenIUpdateAUsersLoginPropertiesOnly_ThenIShouldGetASuccessResultAsync() + { + // arrange + var sut = CreateSut(); + var fakeUser = new MemberIdentityUser + { + Id = "123", + Name = "a", + Email = "a@b.com", + UserName = "c", + Comments = "e", + LastLoginDateUtc = DateTime.UtcNow, + SecurityStamp = "abc", + }; + + IMemberType fakeMemberType = new MemberType(new MockShortStringHelper(), 77); + var mockMember = Mock.Of(m => + m.Id == 123 && + m.Name == "a" && + m.Email == "a@b.com" && + m.Username == "c" && + m.Comments == "e" && + m.ContentTypeAlias == fakeMemberType.Alias && + m.HasIdentity == true && + m.EmailConfirmedDate == DateTime.MinValue && + m.FailedPasswordAttempts == 0 && + m.LastLockoutDate == DateTime.MinValue && + m.IsApproved == false && + m.RawPasswordValue == "xyz" && + m.SecurityStamp == "xyz"); + + _mockMemberService.Setup(x => x.UpdateLoginPropertiesAsync(mockMember)); + _mockMemberService.Setup(x => x.GetById(123)).Returns(mockMember); + + // act + var identityResult = await sut.UpdateAsync(fakeUser, CancellationToken.None); + + // assert + Assert.IsTrue(identityResult.Succeeded); + Assert.IsTrue(!identityResult.Errors.Any()); + + Assert.AreEqual(fakeUser.Name, mockMember.Name); + Assert.AreEqual(fakeUser.Email, mockMember.Email); + Assert.AreEqual(fakeUser.UserName, mockMember.Username); + Assert.AreEqual(fakeUser.Comments, mockMember.Comments); + Assert.IsFalse(fakeUser.LastPasswordChangeDateUtc.HasValue); + Assert.AreEqual(fakeUser.LastLoginDateUtc.Value.ToLocalTime(), mockMember.LastLoginDate); + Assert.AreEqual(fakeUser.AccessFailedCount, mockMember.FailedPasswordAttempts); + Assert.AreEqual(fakeUser.IsLockedOut, mockMember.IsLockedOut); + Assert.AreEqual(fakeUser.IsApproved, mockMember.IsApproved); + Assert.AreEqual(fakeUser.SecurityStamp, mockMember.SecurityStamp); + + _mockMemberService.Verify(x => x.Save(mockMember, Constants.Security.SuperUserId), Times.Never); + _mockMemberService.Verify(x => x.UpdateLoginPropertiesAsync(mockMember)); + _mockMemberService.Verify(x => x.GetById(123)); + } + [Test] public async Task GivenIDeleteUser_AndTheUserIsNotPresent_ThenIShouldGetAFailedResultAsync() {