Fix UpdatePasswordHash virtual method not called consistently by kubaflo · Pull Request #65576 · dotnet/aspnetcore

kubaflo · 2026-03-01T01:57:12Z

🤖 AI Summary

🔍 Automated Fix Report

🔍 Pre-Flight — Context & Validation

Issue: #60252 — Identity UpdatePasswordHash virtual method not called consistently

Area: src/Identity/Extensions.Core/

Root Cause: UserManager<TUser> has both a private and a protected virtual overload of UpdatePasswordHash. Four call sites (CreateAsync, ChangePasswordAsync, ResetPasswordAsync, AddPasswordAsync) called the private overload, bypassing custom implementations that override the virtual method.

Classification: Bug — logic error in method dispatch

🧪 Test — Bug Reproduction

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

Test Added: UpdatePasswordHash_Virtual_CalledFromChangePassword — verifies that a subclass override of UpdatePasswordHash is invoked during ChangePasswordAsync.

Strategy: Created a mock UserManager<TUser> subclass that tracks whether the virtual UpdatePasswordHash was called, then called ChangePasswordAsync and asserted the override was reached.

🚦 Gate — Test Verification & Regression

Gate Result: ✅ All 515 Identity tests pass

Test Command:

dotnet test src/Identity/test/Identity.Test/Identity.Test.csproj --no-restore -v q

Regression: No failures in existing test suite.

🔧 Fix — Analysis & Comparison (✅ 4 passed)

Fix: Ensured that the protected virtual UpdatePasswordHash(TUser, string, bool) method is invoked from all password-setting operations, so that subclass overrides are respected.

Attempt	Approach	Result
0	Redirect 4 call sites to virtual overload directly	✅ Pass
1	Extract `UpdatePasswordHashCore`, private delegates to virtual	✅ Pass
2	Redirect call sites + inline null-password logic in RemovePasswordAsync	✅ Pass
3	Make virtual self-contained, private delegates to virtual for non-null	✅ Pass

✅ Attempt 0: PASS

Approach: Redirect 4 call sites from private UpdatePasswordHash(IUserPasswordStore, TUser, string?) to protected virtual UpdatePasswordHash(TUser, string, bool).

Changed CreateAsync, ChangePasswordAsync, ResetPasswordAsync (rehash path), and AddPasswordAsync to call the virtual overload directly. Kept RemovePasswordAsync using the private overload since it passes null.

📄 Diff

diff --git a/src/Identity/Extensions.Core/src/UserManager.cs b/src/Identity/Extensions.Core/src/UserManager.cs
index fbbc410af6..680f0d34d7 100644
--- a/src/Identity/Extensions.Core/src/UserManager.cs
+++ b/src/Identity/Extensions.Core/src/UserManager.cs
@@ -642,7 +642,7 @@ public class UserManager<TUser> : IDisposable where TUser : class
             var passwordStore = GetPasswordStore();
             ArgumentNullThrowHelper.ThrowIfNull(user);
             ArgumentNullThrowHelper.ThrowIfNull(password);
-            var result = await UpdatePasswordHash(passwordStore, user, password).ConfigureAwait(false);
+            var result = await UpdatePasswordHash(user, password, validatePassword: true).ConfigureAwait(false);
             if (!result.Succeeded)
             {
                 _metrics?.CreateUser(typeof(TUser).FullName!, result, startTimeStamp);
@@ -794,7 +794,7 @@ public class UserManager<TUser> : IDisposable where TUser : class
         if (result == PasswordVerificationResult.SuccessRehashNeeded)
         {
             var startTimeStamp = Stopwatch.GetTimestamp();
-            await UpdatePasswordHash(passwordStore, user, password, validatePassword: false).ConfigureAwait(false);
+            await UpdatePasswordHash(user, password, validatePassword: false).ConfigureAwait(false);
             await UpdateUserAndRecordMetricAsync(user, UserUpdateType.PasswordRehash, startTimeStamp).ConfigureAwait(false);
         }
 
@@ -856,7 +856,7 @@ public class UserManager<TUser> : IDisposable where TUser : class
             Logger.LogDebug(LoggerEventIds.UserAlreadyHasPassword, "User already has a password.");
             return IdentityResult.Failed(ErrorDescriber.UserAlreadyHasPassword());
         }
-        var result = await UpdatePasswordHash(passwordStore, user, password).ConfigureAwait(false);
+        var result = await UpdatePasswordHash(user, password, validatePassword: true).ConfigureAwait(false);
         if (!result.Succeeded)
         {
             return result;
@@ -899,7 +899,7 @@ public class UserManager<TUser> : IDisposable where TUser : class
 
         if (await VerifyPasswordAsync(passwordStore, user, currentPassword).ConfigureAwait(false) != PasswordVerificationResult.Failed)
         {
-            var updateResult = await UpdatePasswordHash(passwordStore, user, newPassword).ConfigureAwait(false);
+            var updateResult = await UpdatePasswordHash(user, newPassword, validatePassword: true).ConfigureAwait(false);
             if (!updateResult.Succeeded)
             {
                 return updateResult;

✅ Attempt 1: PASS

Approach: Extract shared logic into UpdatePasswordHashCore, make private method delegate to virtual when password is non-null.

Instead of changing all 4 call sites, restructured the methods: extracted the implementation into a new UpdatePasswordHashCore private method. The virtual method calls UpdatePasswordHashCore directly. The private method now delegates to the virtual method when password is non-null (ensuring subclass overrides are invoked), and calls UpdatePasswordHashCore directly for null passwords (RemovePasswordAsync).

📄 Diff

diff --git a/src/Identity/Extensions.Core/src/UserManager.cs b/src/Identity/Extensions.Core/src/UserManager.cs
index fbbc410af6..f2b0d69409 100644
--- a/src/Identity/Extensions.Core/src/UserManager.cs
+++ b/src/Identity/Extensions.Core/src/UserManager.cs
@@ -2815,10 +2815,23 @@ public class UserManager<TUser> : IDisposable where TUser : class
     /// <param name="validatePassword">Whether to validate the password.</param>
     /// <returns>Whether the password has was successfully updated.</returns>
     protected virtual Task<IdentityResult> UpdatePasswordHash(TUser user, string newPassword, bool validatePassword)
-        => UpdatePasswordHash(GetPasswordStore(), user, newPassword, validatePassword);
+        => UpdatePasswordHashCore(GetPasswordStore(), user, newPassword, validatePassword);
 
-    private async Task<IdentityResult> UpdatePasswordHash(IUserPasswordStore<TUser> passwordStore,
+    private Task<IdentityResult> UpdatePasswordHash(IUserPasswordStore<TUser> passwordStore,
         TUser user, string? newPassword, bool validatePassword = true)
+    {
+        // Delegate to virtual method when password is non-null so subclass overrides are invoked
+        if (newPassword is not null)
+        {
+            return UpdatePasswordHash(user, newPassword, validatePassword);
+        }
+
+        // null password path (RemovePasswordAsync) — bypass virtual dispatch
+        return UpdatePasswordHashCore(passwordStore, user, null, validatePassword);
+    }
+
+    private async Task<IdentityResult> UpdatePasswordHashCore(IUserPasswordStore<TUser> passwordStore,
+        TUser user, string? newPassword, bool validatePassword)
     {
         if (validatePassword)
         {

✅ Attempt 2: PASS

Approach: Redirect 4 call sites to virtual + inline null-password logic in RemovePasswordAsync, eliminating private method usage entirely.

Changed all 4 non-null-password call sites to use the virtual UpdatePasswordHash(TUser, string, bool). For RemovePasswordAsync (null password), inlined the SetPasswordHashAsync(null) + UpdateSecurityStampInternal calls directly, removing its dependency on the private method.

📄 Diff

diff --git a/src/Identity/Extensions.Core/src/UserManager.cs b/src/Identity/Extensions.Core/src/UserManager.cs
index fbbc410af6..e488c3a505 100644
--- a/src/Identity/Extensions.Core/src/UserManager.cs
+++ b/src/Identity/Extensions.Core/src/UserManager.cs
@@ -642,7 +642,7 @@ public class UserManager<TUser> : IDisposable where TUser : class
             var passwordStore = GetPasswordStore();
             ArgumentNullThrowHelper.ThrowIfNull(user);
             ArgumentNullThrowHelper.ThrowIfNull(password);
-            var result = await UpdatePasswordHash(passwordStore, user, password).ConfigureAwait(false);
+            var result = await UpdatePasswordHash(user, password, validatePassword: true).ConfigureAwait(false);
             if (!result.Succeeded)
             {
                 _metrics?.CreateUser(typeof(TUser).FullName!, result, startTimeStamp);
@@ -794,7 +794,7 @@ public class UserManager<TUser> : IDisposable where TUser : class
         if (result == PasswordVerificationResult.SuccessRehashNeeded)
         {
             var startTimeStamp = Stopwatch.GetTimestamp();
-            await UpdatePasswordHash(passwordStore, user, password, validatePassword: false).ConfigureAwait(false);
+            await UpdatePasswordHash(user, password, validatePassword: false).ConfigureAwait(false);
             await UpdateUserAndRecordMetricAsync(user, UserUpdateType.PasswordRehash, startTimeStamp).ConfigureAwait(false);
         }
 
@@ -856,7 +856,7 @@ public class UserManager<TUser> : IDisposable where TUser : class
             Logger.LogDebug(LoggerEventIds.UserAlreadyHasPassword, "User already has a password.");
             return IdentityResult.Failed(ErrorDescriber.UserAlreadyHasPassword());
         }
-        var result = await UpdatePasswordHash(passwordStore, user, password).ConfigureAwait(false);
+        var result = await UpdatePasswordHash(user, password, validatePassword: true).ConfigureAwait(false);
         if (!result.Succeeded)
         {
             return result;
@@ -899,7 +899,7 @@ public class UserManager<TUser> : IDisposable where TUser : class
 
         if (await VerifyPasswordAsync(passwordStore, user, currentPassword).ConfigureAwait(false) != PasswordVerificationResult.Failed)
         {
-            var updateResult = await UpdatePasswordHash(passwordStore, user, newPassword).ConfigureAwait(false);
+            var updateResult = await UpdatePasswordHash(user, newPassword, validatePassword: true).ConfigureAwait(false);
             if (!updateResult.Succeeded)
             {
                 return updateResult;
@@ -924,10 +924,10 @@ public class UserManager<TUser> : IDisposable where TUser : class
         try
         {
             ThrowIfDisposed();
-            var passwordStore = GetPasswordStore();
             ArgumentNullThrowHelper.ThrowIfNull(user);
 
-            await UpdatePasswordHash(passwordStore, user, null, validatePassword: false).ConfigureAwait(false);
+            await GetPasswordStore().SetPasswordHashAsync(user, null, CancellationToken).ConfigureAwait(false);
+            await UpdateSecurityStampInternal(user).ConfigureAwait(false);
             return await UpdateUserAndRecordMetricAsync(user, UserUpdateType.RemovePassword, startTimeStamp).ConfigureAwait(false);
         }
         catch (Exception ex)

✅ Attempt 3: PASS

Approach: Make the virtual method self-contained (inline implementation), make private method delegate to virtual for non-null passwords.

Expanded the virtual method from a one-liner into a full implementation with its own GetPasswordStore(), validation, hashing, and stamp update. The private method now delegates to the virtual method for non-null passwords, and handles null passwords directly. No call-site changes needed.

📄 Diff

diff --git a/src/Identity/Extensions.Core/src/UserManager.cs b/src/Identity/Extensions.Core/src/UserManager.cs
index fbbc410af6..c1dffe3eba 100644
--- a/src/Identity/Extensions.Core/src/UserManager.cs
+++ b/src/Identity/Extensions.Core/src/UserManager.cs
@@ -2814,12 +2814,9 @@ public class UserManager<TUser> : IDisposable where TUser : class
     /// <param name="newPassword">The new password.</param>
     /// <param name="validatePassword">Whether to validate the password.</param>
     /// <returns>Whether the password has was successfully updated.</returns>
-    protected virtual Task<IdentityResult> UpdatePasswordHash(TUser user, string newPassword, bool validatePassword)
-        => UpdatePasswordHash(GetPasswordStore(), user, newPassword, validatePassword);
-
-    private async Task<IdentityResult> UpdatePasswordHash(IUserPasswordStore<TUser> passwordStore,
-        TUser user, string? newPassword, bool validatePassword = true)
+    protected virtual async Task<IdentityResult> UpdatePasswordHash(TUser user, string newPassword, bool validatePassword)
     {
+        var passwordStore = GetPasswordStore();
         if (validatePassword)
         {
             var validate = await ValidatePasswordAsync(user, newPassword).ConfigureAwait(false);
@@ -2828,12 +2825,27 @@ public class UserManager<TUser> : IDisposable where TUser : class
                 return validate;
             }
         }
-        var hash = newPassword != null ? PasswordHasher.HashPassword(user, newPassword) : null;
+        var hash = PasswordHasher.HashPassword(user, newPassword);
         await passwordStore.SetPasswordHashAsync(user, hash, CancellationToken).ConfigureAwait(false);
         await UpdateSecurityStampInternal(user).ConfigureAwait(false);
         return IdentityResult.Success;
     }
 
+    private async Task<IdentityResult> UpdatePasswordHash(IUserPasswordStore<TUser> passwordStore,
+        TUser user, string? newPassword, bool validatePassword = true)
+    {
+        // For non-null passwords, delegate to the virtual method
+        if (newPassword is not null)
+        {
+            return await UpdatePasswordHash(user, newPassword, validatePassword).ConfigureAwait(false);
+        }
+
+        // Null password path only used by RemovePasswordAsync
+        await passwordStore.SetPasswordHashAsync(user, null, CancellationToken).ConfigureAwait(false);
+        await UpdateSecurityStampInternal(user).ConfigureAwait(false);
+        return IdentityResult.Success;
+    }
+
     private IUserRoleStore<TUser> GetUserRoleStore()
     {
         var cast = Store as IUserRoleStore<TUser>;

Copilot

Pull request overview

This PR addresses an ASP.NET Core Identity extensibility issue where several password operations bypassed the UserManager<TUser>.UpdatePasswordHash(TUser, string, bool) virtual overload, preventing derived UserManager implementations from consistently applying custom password-hash update logic. The PR also introduces new repository “skills” documentation/scripts under .github/skills/ (not described in the PR metadata).

Changes:

Route CreateAsync, AddPasswordAsync, ChangePasswordAsync, and CheckPasswordAsync (rehash path) through the virtual UpdatePasswordHash(TUser, string, bool) overload.
Add a new UserManagerTest verifying the virtual overload is invoked for key password operations.
Add multiple new .github/skills/* skill definitions, scripts, and bash tests for an agent workflow and AI summary comment posting.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
src/Identity/Extensions.Core/src/UserManager.cs	Updates password operation call sites to use the virtual `UpdatePasswordHash` overload.
src/Identity/test/Identity.Test/UserManagerTest.cs	Adds a test `TrackingUserManager` to verify the virtual method is invoked for several password operations.
.github/skills/write-tests/SKILL.md	Adds documentation for a “write-tests” skill workflow.
.github/skills/verify-tests/SKILL.md	Adds documentation for verifying tests catch the bug.
.github/skills/try-fix/SKILL.md	Adds documentation for trying an alternative fix and recording results.
.github/skills/fix-issue/SKILL.md	Adds a large “fix-issue” skill definition/workflow doc (scope not reflected in PR description).
.github/skills/ai-summary-comment/SKILL.md	Adds documentation for posting/updating a unified AI summary comment.
.github/skills/ai-summary-comment/scripts/post-ai-summary-comment.sh	Adds script to compose and post/update an AI Summary PR comment from local phase artifacts.
.github/skills/fix-issue/tests/test-skill-definition.sh	Adds bash tests validating the fix-issue skill definition content.
.github/skills/fix-issue/tests/test-ai-summary-comment.sh	Adds bash tests for AI summary comment scripts (currently contains exit-code capture bugs).

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

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

.github/skills/fix-issue/tests/test-ai-summary-comment.sh

.github/skills/fix-issue/SKILL.md

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 dotnet#60252 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings March 1, 2026 01:57

kubaflo requested review from a team and wtgodbe as code owners March 1, 2026 01:57

kubaflo mentioned this pull request Mar 1, 2026

The virtual UpdatePasswordHash is only invoked by ResetPasswordAsync #60252

Open

1 task

github-actions bot added the needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically label Mar 1, 2026

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Mar 1, 2026

Copilot started reviewing on behalf of kubaflo March 1, 2026 01:58 View session

Copilot AI reviewed Mar 1, 2026

View reviewed changes

martincostello added area-identity Includes: Identity and providers and removed needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically labels Mar 1, 2026

kubaflo force-pushed the fix/identity-updatepasswordhash-60252 branch from f7dc4af to 4d3ea01 Compare March 1, 2026 11:34

kubaflo force-pushed the fix/identity-updatepasswordhash-60252 branch from 4d3ea01 to 4a7e01b Compare March 1, 2026 21:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix UpdatePasswordHash virtual method not called consistently#65576

Fix UpdatePasswordHash virtual method not called consistently#65576
kubaflo wants to merge 1 commit intodotnet:mainfrom
kubaflo:fix/identity-updatepasswordhash-60252

kubaflo commented Mar 1, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

kubaflo commented Mar 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🤖 AI Summary

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kubaflo commented Mar 1, 2026 •

edited

Loading