Skip to content

Commit 127c6c4

Browse files
AndyButlandCopilotMigaroez
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)
* Optimize the member save as part of the member login process, by-passing locking and audit steps and handling only the expected update properties. * Added unit test to verify new behaviour. * Update src/Umbraco.Infrastructure/Security/MemberUserStore.cs Co-authored-by: Copilot <[email protected]> * Updates from code review. * Improved comments. * Add state information to notification indicating whether a member is saved via only the update of login properties. --------- Co-authored-by: Copilot <[email protected]> Co-authored-by: Migaroez <[email protected]>
1 parent 8433b2b commit 127c6c4

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
@@ -38,4 +38,11 @@ public interface IMemberRepository : IContentRepository<int, IMember>
3838
/// <param name="query"></param>
3939
/// <returns></returns>
4040
int GetCountByQuery(IQuery<IMember>? query);
41+
42+
/// <summary>
43+
/// Saves only the properties related to login for the member, using an optimized, non-locking update.
44+
/// </summary>
45+
/// <param name="member">The member to update.</param>
46+
/// <returns>Used to avoid the full save of the member object after a login operation.</returns>
47+
Task UpdateLoginPropertiesAsync(IMember member) => Task.CompletedTask;
4148
}

src/Umbraco.Core/Services/IMemberService.cs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -343,4 +343,11 @@ IEnumerable<IMember> FindMembersByDisplayName(
343343
/// <see cref="IEnumerable{IMember}" />
344344
/// </returns>
345345
IEnumerable<IMember>? GetMembersByPropertyValue(string propertyTypeAlias, DateTime value, ValuePropertyMatchType matchType = ValuePropertyMatchType.Exact);
346+
347+
/// <summary>
348+
/// Saves only the properties related to login for the member, using an optimized, non-locking update.
349+
/// </summary>
350+
/// <param name="member">The member to update.</param>
351+
/// <returns>Used to avoid the full save of the member object after a login operation.</returns>
352+
Task UpdateLoginPropertiesAsync(IMember member) => Task.CompletedTask;
346353
}

src/Umbraco.Core/Services/MemberService.cs

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -820,6 +820,48 @@ public void Save(IEnumerable<IMember> members)
820820
scope.Complete();
821821
}
822822

823+
/// <inheritdoc/>
824+
/// <remarks>
825+
/// <para>
826+
/// Note that in this optimized member save operation for use in the login process, where we only handle login related
827+
/// properties, we aren't taking any locks. If we were updating "content" properties, that could have relations between each
828+
/// other, we should following what we do for documents and lock.
829+
/// But here we are just updating these system fields, and it's fine if they work in a "last one wins" fashion without locking.
830+
/// </para>
831+
/// <para>
832+
/// Note also that we aren't calling "Audit" here (as well as to optimize performance, this is deliberate, because this is not
833+
/// 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
834+
/// just the member logging in as themselves).
835+
/// </para>
836+
/// <para>
837+
/// We are though publishing notifications, to maintain backwards compatibility for any solutions using these for
838+
/// processing following a member login.
839+
/// </para>
840+
/// <para>
841+
/// These notification handlers will ensure that the records to umbracoLog are also added in the same way as they
842+
/// are for a full save operation.
843+
/// </para>
844+
/// </remarks>
845+
public async Task UpdateLoginPropertiesAsync(IMember member)
846+
{
847+
EventMessages evtMsgs = EventMessagesFactory.Get();
848+
849+
using ICoreScope scope = ScopeProvider.CreateCoreScope();
850+
var savingNotification = new MemberSavingNotification(member, evtMsgs);
851+
savingNotification.State.Add("LoginPropertiesOnly", true);
852+
if (scope.Notifications.PublishCancelable(savingNotification))
853+
{
854+
scope.Complete();
855+
return;
856+
}
857+
858+
await _memberRepository.UpdateLoginPropertiesAsync(member);
859+
860+
scope.Notifications.Publish(new MemberSavedNotification(member, evtMsgs).WithStateFrom(savingNotification));
861+
862+
scope.Complete();
863+
}
864+
823865
#endregion
824866

825867
#region Delete

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

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -848,5 +848,50 @@ protected override void PersistUpdatedItem(IMember entity)
848848
entity.ResetDirtyProperties();
849849
}
850850

851+
/// <inheritdoc/>
852+
public async Task UpdateLoginPropertiesAsync(IMember member)
853+
{
854+
var updatedLastLoginDate = member.IsPropertyDirty(nameof(member.LastLoginDate));
855+
var updatedSecurityStamp = member.IsPropertyDirty(nameof(member.SecurityStamp));
856+
if (updatedLastLoginDate is false && updatedSecurityStamp is false)
857+
{
858+
return;
859+
}
860+
861+
NPocoSqlExtensions.SqlUpd<MemberDto> GetMemberSetExpression(IMember member, NPocoSqlExtensions.SqlUpd<MemberDto> m)
862+
{
863+
var setExpression = new NPocoSqlExtensions.SqlUpd<MemberDto>(SqlContext);
864+
if (updatedLastLoginDate)
865+
{
866+
setExpression.Set(x => x.LastLoginDate, member.LastLoginDate);
867+
}
868+
869+
if (updatedSecurityStamp)
870+
{
871+
setExpression.Set(x => x.SecurityStampToken, member.SecurityStamp);
872+
}
873+
874+
return setExpression;
875+
}
876+
877+
member.UpdatingEntity();
878+
879+
Sql<ISqlContext> updateMemberQuery = Sql()
880+
.Update<MemberDto>(m => GetMemberSetExpression(member, m))
881+
.Where<MemberDto>(m => m.NodeId == member.Id);
882+
await Database.ExecuteAsync(updateMemberQuery);
883+
884+
Sql<ISqlContext> updateContentVersionQuery = Sql()
885+
.Update<ContentVersionDto>(m => m.Set(x => x.VersionDate, member.UpdateDate))
886+
.Where<ContentVersionDto>(m => m.NodeId == member.Id && m.Current == true);
887+
await Database.ExecuteAsync(updateContentVersionQuery);
888+
889+
OnUowRefreshedEntity(new MemberRefreshNotification(member, new EventMessages()));
890+
891+
_memberByUsernameCachePolicy.DeleteByUserName(CacheKeys.MemberUserNameCachePrefix, member.Username);
892+
893+
member.ResetDirtyProperties();
894+
}
895+
851896
#endregion
852897
}

src/Umbraco.Infrastructure/Security/MemberUserStore.cs

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

163163
/// <inheritdoc />
164-
public override Task<IdentityResult> UpdateAsync(
164+
public override async Task<IdentityResult> UpdateAsync(
165165
MemberIdentityUser user,
166166
CancellationToken cancellationToken = default)
167167
{
@@ -189,9 +189,21 @@ public override Task<IdentityResult> UpdateAsync(
189189
var isLoginsPropertyDirty = user.IsPropertyDirty(nameof(MemberIdentityUser.Logins));
190190
var isTokensPropertyDirty = user.IsPropertyDirty(nameof(MemberIdentityUser.LoginTokens));
191191

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

196208
if (updateRoles)
197209
{
@@ -222,15 +234,21 @@ public override Task<IdentityResult> UpdateAsync(
222234
}
223235

224236
scope.Complete();
225-
return Task.FromResult(IdentityResult.Success);
237+
return IdentityResult.Success;
226238
}
227239
catch (Exception ex)
228240
{
229-
return Task.FromResult(
230-
IdentityResult.Failed(new IdentityError { Code = GenericIdentityErrorCode, Description = ex.Message }));
241+
return IdentityResult.Failed(new IdentityError { Code = GenericIdentityErrorCode, Description = ex.Message });
231242
}
232243
}
233244

245+
private static bool UpdatingOnlyLoginProperties(IReadOnlyList<string> propertiesUpdated)
246+
{
247+
string[] loginPropertyUpdates = [nameof(MemberIdentityUser.LastLoginDateUtc), nameof(MemberIdentityUser.SecurityStamp)];
248+
return (propertiesUpdated.Count == 2 && propertiesUpdated.ContainsAll(loginPropertyUpdates)) ||
249+
(propertiesUpdated.Count == 1 && propertiesUpdated.ContainsAny(loginPropertyUpdates));
250+
}
251+
234252
/// <inheritdoc />
235253
public override Task<IdentityResult> DeleteAsync(
236254
MemberIdentityUser user,
@@ -681,9 +699,9 @@ public override Task SetTokenAsync(MemberIdentityUser user, string loginProvider
681699
return user;
682700
}
683701

684-
private bool UpdateMemberProperties(IMember member, MemberIdentityUser identityUser, out bool updateRoles)
702+
private IReadOnlyList<string> UpdateMemberProperties(IMember member, MemberIdentityUser identityUser, out bool updateRoles)
685703
{
686-
var anythingChanged = false;
704+
var updatedProperties = new List<string>();
687705
updateRoles = false;
688706

689707
// don't assign anything if nothing has changed as this will trigger the track changes of the model
@@ -692,7 +710,7 @@ private bool UpdateMemberProperties(IMember member, MemberIdentityUser identityU
692710
|| (identityUser.LastLoginDateUtc.HasValue &&
693711
member.LastLoginDate?.ToUniversalTime() != identityUser.LastLoginDateUtc.Value))
694712
{
695-
anythingChanged = true;
713+
updatedProperties.Add(nameof(MemberIdentityUser.LastLoginDateUtc));
696714

697715
// if the LastLoginDate is being set to MinValue, don't convert it ToLocalTime
698716
DateTime dt = identityUser.LastLoginDateUtc == DateTime.MinValue
@@ -706,14 +724,14 @@ private bool UpdateMemberProperties(IMember member, MemberIdentityUser identityU
706724
|| (identityUser.LastPasswordChangeDateUtc.HasValue && member.LastPasswordChangeDate?.ToUniversalTime() !=
707725
identityUser.LastPasswordChangeDateUtc.Value))
708726
{
709-
anythingChanged = true;
727+
updatedProperties.Add(nameof(MemberIdentityUser.LastPasswordChangeDateUtc));
710728
member.LastPasswordChangeDate = identityUser.LastPasswordChangeDateUtc?.ToLocalTime() ?? DateTime.Now;
711729
}
712730

713731
if (identityUser.IsPropertyDirty(nameof(MemberIdentityUser.Comments))
714732
&& member.Comments != identityUser.Comments && identityUser.Comments.IsNullOrWhiteSpace() == false)
715733
{
716-
anythingChanged = true;
734+
updatedProperties.Add(nameof(MemberIdentityUser.Comments));
717735
member.Comments = identityUser.Comments;
718736
}
719737

@@ -723,34 +741,34 @@ private bool UpdateMemberProperties(IMember member, MemberIdentityUser identityU
723741
|| ((member.EmailConfirmedDate.HasValue == false || member.EmailConfirmedDate.Value == default) &&
724742
identityUser.EmailConfirmed))
725743
{
726-
anythingChanged = true;
744+
updatedProperties.Add(nameof(MemberIdentityUser.EmailConfirmed));
727745
member.EmailConfirmedDate = identityUser.EmailConfirmed ? DateTime.Now : null;
728746
}
729747

730748
if (identityUser.IsPropertyDirty(nameof(MemberIdentityUser.Name))
731749
&& member.Name != identityUser.Name && identityUser.Name.IsNullOrWhiteSpace() == false)
732750
{
733-
anythingChanged = true;
751+
updatedProperties.Add(nameof(MemberIdentityUser.Name));
734752
member.Name = identityUser.Name ?? string.Empty;
735753
}
736754

737755
if (identityUser.IsPropertyDirty(nameof(MemberIdentityUser.Email))
738756
&& member.Email != identityUser.Email && identityUser.Email.IsNullOrWhiteSpace() == false)
739757
{
740-
anythingChanged = true;
758+
updatedProperties.Add(nameof(MemberIdentityUser.Email));
741759
member.Email = identityUser.Email!;
742760
}
743761

744762
if (identityUser.IsPropertyDirty(nameof(MemberIdentityUser.AccessFailedCount))
745763
&& member.FailedPasswordAttempts != identityUser.AccessFailedCount)
746764
{
747-
anythingChanged = true;
765+
updatedProperties.Add(nameof(MemberIdentityUser.AccessFailedCount));
748766
member.FailedPasswordAttempts = identityUser.AccessFailedCount;
749767
}
750768

751769
if (member.IsLockedOut != identityUser.IsLockedOut)
752770
{
753-
anythingChanged = true;
771+
updatedProperties.Add(nameof(MemberIdentityUser.IsLockedOut));
754772
member.IsLockedOut = identityUser.IsLockedOut;
755773

756774
if (member.IsLockedOut)
@@ -762,48 +780,48 @@ private bool UpdateMemberProperties(IMember member, MemberIdentityUser identityU
762780

763781
if (member.IsApproved != identityUser.IsApproved)
764782
{
765-
anythingChanged = true;
783+
updatedProperties.Add(nameof(MemberIdentityUser.IsApproved));
766784
member.IsApproved = identityUser.IsApproved;
767785
}
768786

769787
if (identityUser.IsPropertyDirty(nameof(MemberIdentityUser.UserName))
770788
&& member.Username != identityUser.UserName && identityUser.UserName.IsNullOrWhiteSpace() == false)
771789
{
772-
anythingChanged = true;
790+
updatedProperties.Add(nameof(MemberIdentityUser.UserName));
773791
member.Username = identityUser.UserName!;
774792
}
775793

776794
if (identityUser.IsPropertyDirty(nameof(MemberIdentityUser.PasswordHash))
777795
&& member.RawPasswordValue != identityUser.PasswordHash &&
778796
identityUser.PasswordHash.IsNullOrWhiteSpace() == false)
779797
{
780-
anythingChanged = true;
798+
updatedProperties.Add(nameof(MemberIdentityUser.PasswordHash));
781799
member.RawPasswordValue = identityUser.PasswordHash;
782800
member.PasswordConfiguration = identityUser.PasswordConfig;
783801
}
784802

785803
if (member.PasswordConfiguration != identityUser.PasswordConfig)
786804
{
787-
anythingChanged = true;
805+
updatedProperties.Add(nameof(MemberIdentityUser.PasswordConfig));
788806
member.PasswordConfiguration = identityUser.PasswordConfig;
789807
}
790808

791809
if (member.SecurityStamp != identityUser.SecurityStamp)
792810
{
793-
anythingChanged = true;
811+
updatedProperties.Add(nameof(MemberIdentityUser.SecurityStamp));
794812
member.SecurityStamp = identityUser.SecurityStamp;
795813
}
796814

797815
if (identityUser.IsPropertyDirty(nameof(MemberIdentityUser.Roles)))
798816
{
799-
anythingChanged = true;
817+
updatedProperties.Add(nameof(MemberIdentityUser.Roles));
800818
updateRoles = true;
801819
}
802820

803821
// reset all changes
804822
identityUser.ResetDirtyProperties(false);
805823

806-
return anythingChanged;
824+
return updatedProperties.AsReadOnly();
807825
}
808826

809827
/// <inheritdoc />

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

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

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

0 commit comments

Comments
 (0)