Skip to content

Commit f7dc4af

Browse files
kubafloCopilot
andcommitted
Fix UpdatePasswordHash virtual method not called consistently
Change CreateAsync, AddPasswordAsync, ChangePasswordAsync, and the rehash path in CheckPasswordAsync to call the virtual UpdatePasswordHash(user, password, validatePassword) overload instead of the private helper. This allows subclasses that override the virtual method to intercept all password-change operations, not just ResetPasswordAsync. RemovePasswordAsync is left unchanged because it passes null, and the virtual method's parameter is non-nullable. Add theory test VirtualUpdatePasswordHashCalledForPasswordOperations covering CreateAsync, AddPasswordAsync, and ChangePasswordAsync. Fixes #60252 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent db592a6 commit f7dc4af

File tree

2 files changed

+66
-4
lines changed

2 files changed

+66
-4
lines changed

src/Identity/Extensions.Core/src/UserManager.cs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -642,7 +642,7 @@ public virtual async Task<IdentityResult> CreateAsync(TUser user, string passwor
642642
var passwordStore = GetPasswordStore();
643643
ArgumentNullThrowHelper.ThrowIfNull(user);
644644
ArgumentNullThrowHelper.ThrowIfNull(password);
645-
var result = await UpdatePasswordHash(passwordStore, user, password).ConfigureAwait(false);
645+
var result = await UpdatePasswordHash(user, password, validatePassword: true).ConfigureAwait(false);
646646
if (!result.Succeeded)
647647
{
648648
_metrics?.CreateUser(typeof(TUser).FullName!, result, startTimeStamp);
@@ -794,7 +794,7 @@ public virtual async Task<bool> CheckPasswordAsync(TUser user, string password)
794794
if (result == PasswordVerificationResult.SuccessRehashNeeded)
795795
{
796796
var startTimeStamp = Stopwatch.GetTimestamp();
797-
await UpdatePasswordHash(passwordStore, user, password, validatePassword: false).ConfigureAwait(false);
797+
await UpdatePasswordHash(user, password, validatePassword: false).ConfigureAwait(false);
798798
await UpdateUserAndRecordMetricAsync(user, UserUpdateType.PasswordRehash, startTimeStamp).ConfigureAwait(false);
799799
}
800800

@@ -856,7 +856,7 @@ private async Task<IdentityResult> AddPasswordCoreAsync(TUser user, string passw
856856
Logger.LogDebug(LoggerEventIds.UserAlreadyHasPassword, "User already has a password.");
857857
return IdentityResult.Failed(ErrorDescriber.UserAlreadyHasPassword());
858858
}
859-
var result = await UpdatePasswordHash(passwordStore, user, password).ConfigureAwait(false);
859+
var result = await UpdatePasswordHash(user, password, validatePassword: true).ConfigureAwait(false);
860860
if (!result.Succeeded)
861861
{
862862
return result;
@@ -899,7 +899,7 @@ private async Task<IdentityResult> ChangePasswordCoreAsync(TUser user, string cu
899899

900900
if (await VerifyPasswordAsync(passwordStore, user, currentPassword).ConfigureAwait(false) != PasswordVerificationResult.Failed)
901901
{
902-
var updateResult = await UpdatePasswordHash(passwordStore, user, newPassword).ConfigureAwait(false);
902+
var updateResult = await UpdatePasswordHash(user, newPassword, validatePassword: true).ConfigureAwait(false);
903903
if (!updateResult.Succeeded)
904904
{
905905
return updateResult;

src/Identity/test/Identity.Test/UserManagerTest.cs

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2220,4 +2220,66 @@ public override IdentityError DuplicateEmail(string email)
22202220
}
22212221
}
22222222

2223+
[Theory]
2224+
[InlineData("CreateAsync")]
2225+
[InlineData("AddPasswordAsync")]
2226+
[InlineData("ChangePasswordAsync")]
2227+
public async Task VirtualUpdatePasswordHashCalledForPasswordOperations(string operation)
2228+
{
2229+
var hasher = new PasswordHasher<PocoUser>();
2230+
var user = new PocoUser { UserName = "test" };
2231+
var currentPasswordHash = hasher.HashPassword(user, "currentpassword");
2232+
2233+
var store = new Mock<IUserPasswordStore<PocoUser>>();
2234+
store.Setup(s => s.GetPasswordHashAsync(It.IsAny<PocoUser>(), It.IsAny<CancellationToken>()))
2235+
.ReturnsAsync(operation == "AddPasswordAsync" ? null : currentPasswordHash);
2236+
store.Setup(s => s.SetPasswordHashAsync(It.IsAny<PocoUser>(), It.IsAny<string>(), It.IsAny<CancellationToken>()))
2237+
.Returns(Task.CompletedTask);
2238+
store.Setup(s => s.UpdateAsync(It.IsAny<PocoUser>(), It.IsAny<CancellationToken>()))
2239+
.ReturnsAsync(IdentityResult.Success);
2240+
store.Setup(s => s.CreateAsync(It.IsAny<PocoUser>(), It.IsAny<CancellationToken>()))
2241+
.ReturnsAsync(IdentityResult.Success);
2242+
store.Setup(s => s.GetUserIdAsync(It.IsAny<PocoUser>(), It.IsAny<CancellationToken>()))
2243+
.ReturnsAsync("userid");
2244+
store.Setup(s => s.GetUserNameAsync(It.IsAny<PocoUser>(), It.IsAny<CancellationToken>()))
2245+
.ReturnsAsync("test");
2246+
store.Setup(s => s.SetNormalizedUserNameAsync(It.IsAny<PocoUser>(), It.IsAny<string>(), It.IsAny<CancellationToken>()))
2247+
.Returns(Task.CompletedTask);
2248+
2249+
var manager = new TrackingUserManager(store.Object);
2250+
2251+
switch (operation)
2252+
{
2253+
case "CreateAsync":
2254+
await manager.CreateAsync(user, "password");
2255+
break;
2256+
case "AddPasswordAsync":
2257+
await manager.AddPasswordAsync(user, "password");
2258+
break;
2259+
case "ChangePasswordAsync":
2260+
await manager.ChangePasswordAsync(user, "currentpassword", "newpassword");
2261+
break;
2262+
}
2263+
2264+
Assert.True(manager.VirtualUpdatePasswordHashCalled, $"Virtual UpdatePasswordHash should be called from {operation}");
2265+
}
2266+
2267+
private class TrackingUserManager : UserManager<PocoUser>
2268+
{
2269+
public bool VirtualUpdatePasswordHashCalled { get; private set; }
2270+
2271+
public TrackingUserManager(IUserPasswordStore<PocoUser> store)
2272+
: base(store, null, new PasswordHasher<PocoUser>(), Array.Empty<IUserValidator<PocoUser>>(),
2273+
Array.Empty<IPasswordValidator<PocoUser>>(), new UpperInvariantLookupNormalizer(),
2274+
new IdentityErrorDescriber(), null, new Microsoft.Extensions.Logging.Abstractions.NullLogger<UserManager<PocoUser>>())
2275+
{
2276+
}
2277+
2278+
protected override Task<IdentityResult> UpdatePasswordHash(PocoUser user, string newPassword, bool validatePassword)
2279+
{
2280+
VirtualUpdatePasswordHashCalled = true;
2281+
return base.UpdatePasswordHash(user, newPassword, validatePassword);
2282+
}
2283+
}
2284+
22232285
}

0 commit comments

Comments
 (0)