yv01p/Umbraco-CMS
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

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)

Browse Source
This commit is contained in:
Andy Butland
2025-05-20 15:58:27 +01:00
parent b1e447d0c5
commit 419625a919
6 changed files with 200 additions and 23 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
src/Umbraco.Core/Persistence/Repositories/IMemberRepository.cs
View File

@@ -42,4 +42,11 @@ public interface IMemberRepository : IContentRepository<int, IMember>
int GetCountByQuery(IQuery<IMember>? query);
Task<PagedModel<IMember>> GetPagedByFilterAsync(MemberFilter memberFilter,int skip, int take, Ordering? ordering = null);
/// <summary>
/// Saves only the properties related to login for the member, using an optimized, non-locking update.
/// </summary>
/// <param name="member">The member to update.</param>
/// <returns>Used to avoid the full save of the member object after a login operation.</returns>
Task UpdateLoginPropertiesAsync(IMember member) => Task.CompletedTask;
}

7
src/Umbraco.Core/Services/IMemberService.cs
View File

@@ -427,4 +427,11 @@ public interface IMemberService : IMembershipMemberService, IContentServiceBase<
/// <see cref="IEnumerable{IMember}" />
/// </returns>
IEnumerable<IMember>? GetMembersByPropertyValue(string propertyTypeAlias, DateTime value, ValuePropertyMatchType matchType = ValuePropertyMatchType.Exact);
/// <summary>
/// Saves only the properties related to login for the member, using an optimized, non-locking update.
/// </summary>
/// <param name="member">The member to update.</param>
/// <returns>Used to avoid the full save of the member object after a login operation.</returns>
Task UpdateLoginPropertiesAsync(IMember member) => Task.CompletedTask;
}

42
src/Umbraco.Core/Services/MemberService.cs
View File

@@ -854,6 +854,48 @@ namespace Umbraco.Cms.Core.Services
public void Save(IEnumerable<IMember> members)
=> Save(members, Constants.Security.SuperUserId);
/// <inheritdoc/>
/// <remarks>
/// <para>
/// 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.
/// </para>
/// <para>
/// 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).
/// </para>
/// <para>
/// We are though publishing notifications, to maintain backwards compatibility for any solutions using these for
/// processing following a member login.
/// </para>
/// <para>
/// 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.
/// </para>
/// </remarks>
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

45
src/Umbraco.Infrastructure/Persistence/Repositories/Implement/MemberRepository.cs
View File

@@ -947,5 +947,50 @@ public class MemberRepository : ContentRepositoryBase<int, IMember, MemberReposi
entity.ResetDirtyProperties();
}
/// <inheritdoc/>
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<MemberDto> GetMemberSetExpression(IMember member, NPocoSqlExtensions.SqlUpd<MemberDto> m)
{
var setExpression = new NPocoSqlExtensions.SqlUpd<MemberDto>(SqlContext);
if (updatedLastLoginDate)
{
setExpression.Set(x => x.LastLoginDate, member.LastLoginDate);
}
if (updatedSecurityStamp)
{
setExpression.Set(x => x.SecurityStampToken, member.SecurityStamp);
}
return setExpression;
}
member.UpdatingEntity();
Sql<ISqlContext> updateMemberQuery = Sql()
.Update<MemberDto>(m => GetMemberSetExpression(member, m))
.Where<MemberDto>(m => m.NodeId == member.Id);
await Database.ExecuteAsync(updateMemberQuery);
Sql<ISqlContext> updateContentVersionQuery = Sql()
.Update<ContentVersionDto>(m => m.Set(x => x.VersionDate, member.UpdateDate))
.Where<ContentVersionDto>(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
}

64
src/Umbraco.Infrastructure/Security/MemberUserStore.cs
View File

@@ -162,7 +162,7 @@ public class MemberUserStore : UmbracoUserStore<MemberIdentityUser, UmbracoIdent
}
/// <inheritdoc />
public override Task<IdentityResult> UpdateAsync(
public override async Task<IdentityResult> UpdateAsync(
MemberIdentityUser user,
CancellationToken cancellationToken = default)
{
@@ -190,9 +190,21 @@ public class MemberUserStore : UmbracoUserStore<MemberIdentityUser, UmbracoIdent
var isLoginsPropertyDirty = user.IsPropertyDirty(nameof(MemberIdentityUser.Logins));
var isTokensPropertyDirty = user.IsPropertyDirty(nameof(MemberIdentityUser.LoginTokens));
if (UpdateMemberProperties(found, user, out var updateRoles))
IReadOnlyList<string> 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<MemberIdentityUser, UmbracoIdent
}
scope.Complete();
return Task.FromResult(IdentityResult.Success);
return IdentityResult.Success;
}
catch (Exception ex)
{
return Task.FromResult(
IdentityResult.Failed(new IdentityError { Code = GenericIdentityErrorCode, Description = ex.Message }));
return IdentityResult.Failed(new IdentityError { Code = GenericIdentityErrorCode, Description = ex.Message });
}
}
private static bool UpdatingOnlyLoginProperties(IReadOnlyList<string> propertiesUpdated)
{
string[] loginPropertyUpdates = [nameof(MemberIdentityUser.LastLoginDateUtc), nameof(MemberIdentityUser.SecurityStamp)];
return (propertiesUpdated.Count == 2 && propertiesUpdated.ContainsAll(loginPropertyUpdates)) ||
(propertiesUpdated.Count == 1 && propertiesUpdated.ContainsAny(loginPropertyUpdates));
}
/// <inheritdoc />
public override Task<IdentityResult> DeleteAsync(
MemberIdentityUser user,
@@ -704,9 +722,9 @@ public class MemberUserStore : UmbracoUserStore<MemberIdentityUser, UmbracoIdent
return user;
}
private bool UpdateMemberProperties(IMember member, MemberIdentityUser identityUser, out bool updateRoles)
private IReadOnlyList<string> UpdateMemberProperties(IMember member, MemberIdentityUser identityUser, out bool updateRoles)
{
var anythingChanged = false;
var updatedProperties = new List<string>();
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<MemberIdentityUser, UmbracoIdent
|| (identityUser.LastLoginDateUtc.HasValue &&
member.LastLoginDate?.ToUniversalTime() != identityUser.LastLoginDateUtc.Value))
{
anythingChanged = true;
updatedProperties.Add(nameof(MemberIdentityUser.LastLoginDateUtc));
// if the LastLoginDate is being set to MinValue, don't convert it ToLocalTime
DateTime dt = identityUser.LastLoginDateUtc == DateTime.MinValue
@@ -729,14 +747,14 @@ public class MemberUserStore : UmbracoUserStore<MemberIdentityUser, UmbracoIdent
|| (identityUser.LastPasswordChangeDateUtc.HasValue && member.LastPasswordChangeDate?.ToUniversalTime() !=
identityUser.LastPasswordChangeDateUtc.Value))
{
anythingChanged = true;
updatedProperties.Add(nameof(MemberIdentityUser.LastPasswordChangeDateUtc));
member.LastPasswordChangeDate = identityUser.LastPasswordChangeDateUtc?.ToLocalTime() ?? DateTime.Now;
}
if (identityUser.IsPropertyDirty(nameof(MemberIdentityUser.Comments))
&& member.Comments != identityUser.Comments && identityUser.Comments.IsNullOrWhiteSpace() == false)
{
anythingChanged = true;
updatedProperties.Add(nameof(MemberIdentityUser.Comments));
member.Comments = identityUser.Comments;
}
@@ -746,34 +764,34 @@ public class MemberUserStore : UmbracoUserStore<MemberIdentityUser, UmbracoIdent
|| ((member.EmailConfirmedDate.HasValue == false || member.EmailConfirmedDate.Value == default) &&
identityUser.EmailConfirmed))
{
anythingChanged = true;
updatedProperties.Add(nameof(MemberIdentityUser.EmailConfirmed));
member.EmailConfirmedDate = identityUser.EmailConfirmed ? DateTime.Now : null;
}
if (identityUser.IsPropertyDirty(nameof(MemberIdentityUser.Name))
&& member.Name != identityUser.Name && identityUser.Name.IsNullOrWhiteSpace() == false)
{
anythingChanged = true;
updatedProperties.Add(nameof(MemberIdentityUser.Name));
member.Name = identityUser.Name ?? string.Empty;
}
if (identityUser.IsPropertyDirty(nameof(MemberIdentityUser.Email))
&& member.Email != identityUser.Email && identityUser.Email.IsNullOrWhiteSpace() == false)
{
anythingChanged = true;
updatedProperties.Add(nameof(MemberIdentityUser.Email));
member.Email = identityUser.Email!;
}
if (identityUser.IsPropertyDirty(nameof(MemberIdentityUser.AccessFailedCount))
&& member.FailedPasswordAttempts != identityUser.AccessFailedCount)
{
anythingChanged = true;
updatedProperties.Add(nameof(MemberIdentityUser.AccessFailedCount));
member.FailedPasswordAttempts = identityUser.AccessFailedCount;
}
if (member.IsLockedOut != identityUser.IsLockedOut)
{
anythingChanged = true;
updatedProperties.Add(nameof(MemberIdentityUser.IsLockedOut));
member.IsLockedOut = identityUser.IsLockedOut;
if (member.IsLockedOut)
@@ -785,14 +803,14 @@ public class MemberUserStore : UmbracoUserStore<MemberIdentityUser, UmbracoIdent
if (member.IsApproved != identityUser.IsApproved)
{
anythingChanged = true;
updatedProperties.Add(nameof(MemberIdentityUser.IsApproved));
member.IsApproved = identityUser.IsApproved;
}
if (identityUser.IsPropertyDirty(nameof(MemberIdentityUser.UserName))
&& member.Username != identityUser.UserName && identityUser.UserName.IsNullOrWhiteSpace() == false)
{
anythingChanged = true;
updatedProperties.Add(nameof(MemberIdentityUser.UserName));
member.Username = identityUser.UserName!;
}
@@ -800,33 +818,33 @@ public class MemberUserStore : UmbracoUserStore<MemberIdentityUser, UmbracoIdent
&& member.RawPasswordValue != identityUser.PasswordHash &&
identityUser.PasswordHash.IsNullOrWhiteSpace() == false)
{
anythingChanged = true;
updatedProperties.Add(nameof(MemberIdentityUser.PasswordHash));
member.RawPasswordValue = identityUser.PasswordHash;
member.PasswordConfiguration = identityUser.PasswordConfig;
}
if (member.PasswordConfiguration != identityUser.PasswordConfig)
{
anythingChanged = true;
updatedProperties.Add(nameof(MemberIdentityUser.PasswordConfig));
member.PasswordConfiguration = identityUser.PasswordConfig;
}
if (member.SecurityStamp != identityUser.SecurityStamp)
{
anythingChanged = true;
updatedProperties.Add(nameof(MemberIdentityUser.SecurityStamp));
member.SecurityStamp = identityUser.SecurityStamp;
}
if (identityUser.IsPropertyDirty(nameof(MemberIdentityUser.Roles)))
{
anythingChanged = true;
updatedProperties.Add(nameof(MemberIdentityUser.Roles));
updateRoles = true;
}
// reset all changes
identityUser.ResetDirtyProperties(false);
return anythingChanged;
return updatedProperties.AsReadOnly();
}
/// <inheritdoc />

58
tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberUserStoreTests.cs
View File

@@ -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<IMember>(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()
{