Skip to content

Commit 419625a

Browse files
committed
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)
1 parent b1e447d commit 419625a

File tree

6 files changed

+200
-23
lines changed

6 files changed

+200
-23
lines changed

src/Umbraco.Core/Persistence/Repositories/IMemberRepository.cs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,4 +42,11 @@ public interface IMemberRepository : IContentRepository<int, IMember>
4242
int GetCountByQuery(IQuery<IMember>? query);
4343

4444
Task<PagedModel<IMember>> GetPagedByFilterAsync(MemberFilter memberFilter,int skip, int take, Ordering? ordering = null);
45+
46+
/// <summary>
47+
/// Saves only the properties related to login for the member, using an optimized, non-locking update.
48+
/// </summary>
49+
/// <param name="member">The member to update.</param>
50+
/// <returns>Used to avoid the full save of the member object after a login operation.</returns>
51+
Task UpdateLoginPropertiesAsync(IMember member) => Task.CompletedTask;
4552
}

src/Umbraco.Core/Services/IMemberService.cs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -427,4 +427,11 @@ IEnumerable<IMember> FindMembersByDisplayName(
427427
/// <see cref="IEnumerable{IMember}" />
428428
/// </returns>
429429
IEnumerable<IMember>? GetMembersByPropertyValue(string propertyTypeAlias, DateTime value, ValuePropertyMatchType matchType = ValuePropertyMatchType.Exact);
430+
431+
/// <summary>
432+
/// Saves only the properties related to login for the member, using an optimized, non-locking update.
433+
/// </summary>
434+
/// <param name="member">The member to update.</param>
435+
/// <returns>Used to avoid the full save of the member object after a login operation.</returns>
436+
Task UpdateLoginPropertiesAsync(IMember member) => Task.CompletedTask;
430437
}

src/Umbraco.Core/Services/MemberService.cs

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -854,6 +854,48 @@ public void Save(IMember member)
854854
public void Save(IEnumerable<IMember> members)
855855
=> Save(members, Constants.Security.SuperUserId);
856856

857+
/// <inheritdoc/>
858+
/// <remarks>
859+
/// <para>
860+
/// Note that in this optimized member save operation for use in the login process, where we only handle login related
861+
/// properties, we aren't taking any locks. If we were updating "content" properties, that could have relations between each
862+
/// other, we should following what we do for documents and lock.
863+
/// But here we are just updating these system fields, and it's fine if they work in a "last one wins" fashion without locking.
864+
/// </para>
865+
/// <para>
866+
/// Note also that we aren't calling "Audit" here (as well as to optimize performance, this is deliberate, because this is not
867+
/// 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
868+
/// just the member logging in as themselves).
869+
/// </para>
870+
/// <para>
871+
/// We are though publishing notifications, to maintain backwards compatibility for any solutions using these for
872+
/// processing following a member login.
873+
/// </para>
874+
/// <para>
875+
/// These notification handlers will ensure that the records to umbracoLog are also added in the same way as they
876+
/// are for a full save operation.
877+
/// </para>
878+
/// </remarks>
879+
public async Task UpdateLoginPropertiesAsync(IMember member)
880+
{
881+
EventMessages evtMsgs = EventMessagesFactory.Get();
882+
883+
using ICoreScope scope = ScopeProvider.CreateCoreScope();
884+
var savingNotification = new MemberSavingNotification(member, evtMsgs);
885+
savingNotification.State.Add("LoginPropertiesOnly", true);
886+
if (scope.Notifications.PublishCancelable(savingNotification))
887+
{
888+
scope.Complete();
889+
return;
890+
}
891+
892+
await _memberRepository.UpdateLoginPropertiesAsync(member);
893+
894+
scope.Notifications.Publish(new MemberSavedNotification(member, evtMsgs).WithStateFrom(savingNotification));
895+
896+
scope.Complete();
897+
}
898+
857899
#endregion
858900

859901
#region Delete

src/Umbraco.Infrastructure/Persistence/Repositories/Implement/MemberRepository.cs

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -947,5 +947,50 @@ protected override void PersistUpdatedItem(IMember entity)
947947
entity.ResetDirtyProperties();
948948
}
949949

950+
/// <inheritdoc/>
951+
public async Task UpdateLoginPropertiesAsync(IMember member)
952+
{
953+
var updatedLastLoginDate = member.IsPropertyDirty(nameof(member.LastLoginDate));
954+
var updatedSecurityStamp = member.IsPropertyDirty(nameof(member.SecurityStamp));
955+
if (updatedLastLoginDate is false && updatedSecurityStamp is false)
956+
{
957+
return;
958+
}
959+
960+
NPocoSqlExtensions.SqlUpd<MemberDto> GetMemberSetExpression(IMember member, NPocoSqlExtensions.SqlUpd<MemberDto> m)
961+
{
962+
var setExpression = new NPocoSqlExtensions.SqlUpd<MemberDto>(SqlContext);
963+
if (updatedLastLoginDate)
964+
{
965+
setExpression.Set(x => x.LastLoginDate, member.LastLoginDate);
966+
}
967+
968+
if (updatedSecurityStamp)
969+
{
970+
setExpression.Set(x => x.SecurityStampToken, member.SecurityStamp);
971+
}
972+
973+
return setExpression;
974+
}
975+
976+
member.UpdatingEntity();
977+
978+
Sql<ISqlContext> updateMemberQuery = Sql()
979+
.Update<MemberDto>(m => GetMemberSetExpression(member, m))
980+
.Where<MemberDto>(m => m.NodeId == member.Id);
981+
await Database.ExecuteAsync(updateMemberQuery);
982+
983+
Sql<ISqlContext> updateContentVersionQuery = Sql()
984+
.Update<ContentVersionDto>(m => m.Set(x => x.VersionDate, member.UpdateDate))
985+
.Where<ContentVersionDto>(m => m.NodeId == member.Id && m.Current == true);
986+
await Database.ExecuteAsync(updateContentVersionQuery);
987+
988+
OnUowRefreshedEntity(new MemberRefreshNotification(member, new EventMessages()));
989+
990+
_memberByUsernameCachePolicy.DeleteByUserName(CacheKeys.MemberUserNameCachePrefix, member.Username);
991+
992+
member.ResetDirtyProperties();
993+
}
994+
950995
#endregion
951996
}

src/Umbraco.Infrastructure/Security/MemberUserStore.cs

Lines changed: 41 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,7 @@ public override Task<IdentityResult> CreateAsync(
162162
}
163163

164164
/// <inheritdoc />
165-
public override Task<IdentityResult> UpdateAsync(
165+
public override async Task<IdentityResult> UpdateAsync(
166166
MemberIdentityUser user,
167167
CancellationToken cancellationToken = default)
168168
{
@@ -190,9 +190,21 @@ public override Task<IdentityResult> UpdateAsync(
190190
var isLoginsPropertyDirty = user.IsPropertyDirty(nameof(MemberIdentityUser.Logins));
191191
var isTokensPropertyDirty = user.IsPropertyDirty(nameof(MemberIdentityUser.LoginTokens));
192192

193-
if (UpdateMemberProperties(found, user, out var updateRoles))
193+
IReadOnlyList<string> propertiesUpdated = UpdateMemberProperties(found, user, out var updateRoles);
194+
195+
if (propertiesUpdated.Count > 0)
194196
{
195-
_memberService.Save(found);
197+
// As part of logging in members we update the last login date, and, if concurrent logins are disabled, the security stamp.
198+
// If and only if we are updating these properties, we can avoid the overhead of a full save of the member with the associated
199+
// locking, property updates, tag handling etc., and make a more efficient update.
200+
if (UpdatingOnlyLoginProperties(propertiesUpdated))
201+
{
202+
await _memberService.UpdateLoginPropertiesAsync(found);
203+
}
204+
else
205+
{
206+
_memberService.Save(found);
207+
}
196208

197209
if (updateRoles)
198210
{
@@ -223,15 +235,21 @@ public override Task<IdentityResult> UpdateAsync(
223235
}
224236

225237
scope.Complete();
226-
return Task.FromResult(IdentityResult.Success);
238+
return IdentityResult.Success;
227239
}
228240
catch (Exception ex)
229241
{
230-
return Task.FromResult(
231-
IdentityResult.Failed(new IdentityError { Code = GenericIdentityErrorCode, Description = ex.Message }));
242+
return IdentityResult.Failed(new IdentityError { Code = GenericIdentityErrorCode, Description = ex.Message });
232243
}
233244
}
234245

246+
private static bool UpdatingOnlyLoginProperties(IReadOnlyList<string> propertiesUpdated)
247+
{
248+
string[] loginPropertyUpdates = [nameof(MemberIdentityUser.LastLoginDateUtc), nameof(MemberIdentityUser.SecurityStamp)];
249+
return (propertiesUpdated.Count == 2 && propertiesUpdated.ContainsAll(loginPropertyUpdates)) ||
250+
(propertiesUpdated.Count == 1 && propertiesUpdated.ContainsAny(loginPropertyUpdates));
251+
}
252+
235253
/// <inheritdoc />
236254
public override Task<IdentityResult> DeleteAsync(
237255
MemberIdentityUser user,
@@ -704,9 +722,9 @@ public override Task SetTokenAsync(MemberIdentityUser user, string loginProvider
704722
return user;
705723
}
706724

707-
private bool UpdateMemberProperties(IMember member, MemberIdentityUser identityUser, out bool updateRoles)
725+
private IReadOnlyList<string> UpdateMemberProperties(IMember member, MemberIdentityUser identityUser, out bool updateRoles)
708726
{
709-
var anythingChanged = false;
727+
var updatedProperties = new List<string>();
710728
updateRoles = false;
711729

712730
// don't assign anything if nothing has changed as this will trigger the track changes of the model
@@ -715,7 +733,7 @@ private bool UpdateMemberProperties(IMember member, MemberIdentityUser identityU
715733
|| (identityUser.LastLoginDateUtc.HasValue &&
716734
member.LastLoginDate?.ToUniversalTime() != identityUser.LastLoginDateUtc.Value))
717735
{
718-
anythingChanged = true;
736+
updatedProperties.Add(nameof(MemberIdentityUser.LastLoginDateUtc));
719737

720738
// if the LastLoginDate is being set to MinValue, don't convert it ToLocalTime
721739
DateTime dt = identityUser.LastLoginDateUtc == DateTime.MinValue
@@ -729,14 +747,14 @@ private bool UpdateMemberProperties(IMember member, MemberIdentityUser identityU
729747
|| (identityUser.LastPasswordChangeDateUtc.HasValue && member.LastPasswordChangeDate?.ToUniversalTime() !=
730748
identityUser.LastPasswordChangeDateUtc.Value))
731749
{
732-
anythingChanged = true;
750+
updatedProperties.Add(nameof(MemberIdentityUser.LastPasswordChangeDateUtc));
733751
member.LastPasswordChangeDate = identityUser.LastPasswordChangeDateUtc?.ToLocalTime() ?? DateTime.Now;
734752
}
735753

736754
if (identityUser.IsPropertyDirty(nameof(MemberIdentityUser.Comments))
737755
&& member.Comments != identityUser.Comments && identityUser.Comments.IsNullOrWhiteSpace() == false)
738756
{
739-
anythingChanged = true;
757+
updatedProperties.Add(nameof(MemberIdentityUser.Comments));
740758
member.Comments = identityUser.Comments;
741759
}
742760

@@ -746,34 +764,34 @@ private bool UpdateMemberProperties(IMember member, MemberIdentityUser identityU
746764
|| ((member.EmailConfirmedDate.HasValue == false || member.EmailConfirmedDate.Value == default) &&
747765
identityUser.EmailConfirmed))
748766
{
749-
anythingChanged = true;
767+
updatedProperties.Add(nameof(MemberIdentityUser.EmailConfirmed));
750768
member.EmailConfirmedDate = identityUser.EmailConfirmed ? DateTime.Now : null;
751769
}
752770

753771
if (identityUser.IsPropertyDirty(nameof(MemberIdentityUser.Name))
754772
&& member.Name != identityUser.Name && identityUser.Name.IsNullOrWhiteSpace() == false)
755773
{
756-
anythingChanged = true;
774+
updatedProperties.Add(nameof(MemberIdentityUser.Name));
757775
member.Name = identityUser.Name ?? string.Empty;
758776
}
759777

760778
if (identityUser.IsPropertyDirty(nameof(MemberIdentityUser.Email))
761779
&& member.Email != identityUser.Email && identityUser.Email.IsNullOrWhiteSpace() == false)
762780
{
763-
anythingChanged = true;
781+
updatedProperties.Add(nameof(MemberIdentityUser.Email));
764782
member.Email = identityUser.Email!;
765783
}
766784

767785
if (identityUser.IsPropertyDirty(nameof(MemberIdentityUser.AccessFailedCount))
768786
&& member.FailedPasswordAttempts != identityUser.AccessFailedCount)
769787
{
770-
anythingChanged = true;
788+
updatedProperties.Add(nameof(MemberIdentityUser.AccessFailedCount));
771789
member.FailedPasswordAttempts = identityUser.AccessFailedCount;
772790
}
773791

774792
if (member.IsLockedOut != identityUser.IsLockedOut)
775793
{
776-
anythingChanged = true;
794+
updatedProperties.Add(nameof(MemberIdentityUser.IsLockedOut));
777795
member.IsLockedOut = identityUser.IsLockedOut;
778796

779797
if (member.IsLockedOut)
@@ -785,48 +803,48 @@ private bool UpdateMemberProperties(IMember member, MemberIdentityUser identityU
785803

786804
if (member.IsApproved != identityUser.IsApproved)
787805
{
788-
anythingChanged = true;
806+
updatedProperties.Add(nameof(MemberIdentityUser.IsApproved));
789807
member.IsApproved = identityUser.IsApproved;
790808
}
791809

792810
if (identityUser.IsPropertyDirty(nameof(MemberIdentityUser.UserName))
793811
&& member.Username != identityUser.UserName && identityUser.UserName.IsNullOrWhiteSpace() == false)
794812
{
795-
anythingChanged = true;
813+
updatedProperties.Add(nameof(MemberIdentityUser.UserName));
796814
member.Username = identityUser.UserName!;
797815
}
798816

799817
if (identityUser.IsPropertyDirty(nameof(MemberIdentityUser.PasswordHash))
800818
&& member.RawPasswordValue != identityUser.PasswordHash &&
801819
identityUser.PasswordHash.IsNullOrWhiteSpace() == false)
802820
{
803-
anythingChanged = true;
821+
updatedProperties.Add(nameof(MemberIdentityUser.PasswordHash));
804822
member.RawPasswordValue = identityUser.PasswordHash;
805823
member.PasswordConfiguration = identityUser.PasswordConfig;
806824
}
807825

808826
if (member.PasswordConfiguration != identityUser.PasswordConfig)
809827
{
810-
anythingChanged = true;
828+
updatedProperties.Add(nameof(MemberIdentityUser.PasswordConfig));
811829
member.PasswordConfiguration = identityUser.PasswordConfig;
812830
}
813831

814832
if (member.SecurityStamp != identityUser.SecurityStamp)
815833
{
816-
anythingChanged = true;
834+
updatedProperties.Add(nameof(MemberIdentityUser.SecurityStamp));
817835
member.SecurityStamp = identityUser.SecurityStamp;
818836
}
819837

820838
if (identityUser.IsPropertyDirty(nameof(MemberIdentityUser.Roles)))
821839
{
822-
anythingChanged = true;
840+
updatedProperties.Add(nameof(MemberIdentityUser.Roles));
823841
updateRoles = true;
824842
}
825843

826844
// reset all changes
827845
identityUser.ResetDirtyProperties(false);
828846

829-
return anythingChanged;
847+
return updatedProperties.AsReadOnly();
830848
}
831849

832850
/// <inheritdoc />

tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberUserStoreTests.cs

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -206,6 +206,64 @@ public async Task GivenIUpdateAUser_ThenIShouldGetASuccessResultAsync()
206206
_mockMemberService.Verify(x => x.ReplaceRoles(new[] { 123 }, new[] { "role1", "role2" }));
207207
}
208208

209+
[Test]
210+
public async Task GivenIUpdateAUsersLoginPropertiesOnly_ThenIShouldGetASuccessResultAsync()
211+
{
212+
// arrange
213+
var sut = CreateSut();
214+
var fakeUser = new MemberIdentityUser
215+
{
216+
Id = "123",
217+
Name = "a",
218+
Email = "[email protected]",
219+
UserName = "c",
220+
Comments = "e",
221+
LastLoginDateUtc = DateTime.UtcNow,
222+
SecurityStamp = "abc",
223+
};
224+
225+
IMemberType fakeMemberType = new MemberType(new MockShortStringHelper(), 77);
226+
var mockMember = Mock.Of<IMember>(m =>
227+
m.Id == 123 &&
228+
m.Name == "a" &&
229+
m.Email == "[email protected]" &&
230+
m.Username == "c" &&
231+
m.Comments == "e" &&
232+
m.ContentTypeAlias == fakeMemberType.Alias &&
233+
m.HasIdentity == true &&
234+
m.EmailConfirmedDate == DateTime.MinValue &&
235+
m.FailedPasswordAttempts == 0 &&
236+
m.LastLockoutDate == DateTime.MinValue &&
237+
m.IsApproved == false &&
238+
m.RawPasswordValue == "xyz" &&
239+
m.SecurityStamp == "xyz");
240+
241+
_mockMemberService.Setup(x => x.UpdateLoginPropertiesAsync(mockMember));
242+
_mockMemberService.Setup(x => x.GetById(123)).Returns(mockMember);
243+
244+
// act
245+
var identityResult = await sut.UpdateAsync(fakeUser, CancellationToken.None);
246+
247+
// assert
248+
Assert.IsTrue(identityResult.Succeeded);
249+
Assert.IsTrue(!identityResult.Errors.Any());
250+
251+
Assert.AreEqual(fakeUser.Name, mockMember.Name);
252+
Assert.AreEqual(fakeUser.Email, mockMember.Email);
253+
Assert.AreEqual(fakeUser.UserName, mockMember.Username);
254+
Assert.AreEqual(fakeUser.Comments, mockMember.Comments);
255+
Assert.IsFalse(fakeUser.LastPasswordChangeDateUtc.HasValue);
256+
Assert.AreEqual(fakeUser.LastLoginDateUtc.Value.ToLocalTime(), mockMember.LastLoginDate);
257+
Assert.AreEqual(fakeUser.AccessFailedCount, mockMember.FailedPasswordAttempts);
258+
Assert.AreEqual(fakeUser.IsLockedOut, mockMember.IsLockedOut);
259+
Assert.AreEqual(fakeUser.IsApproved, mockMember.IsApproved);
260+
Assert.AreEqual(fakeUser.SecurityStamp, mockMember.SecurityStamp);
261+
262+
_mockMemberService.Verify(x => x.Save(mockMember, Constants.Security.SuperUserId), Times.Never);
263+
_mockMemberService.Verify(x => x.UpdateLoginPropertiesAsync(mockMember));
264+
_mockMemberService.Verify(x => x.GetById(123));
265+
}
266+
209267
[Test]
210268
public async Task GivenIDeleteUser_AndTheUserIsNotPresent_ThenIShouldGetAFailedResultAsync()
211269
{

0 commit comments

Comments
 (0)