The virtual UpdatePasswordHash is only invoked by ResetPasswordAsync

[equist]

Is there an existing issue for this?

• I have searched the existing issues

Describe the bug

We have made an override of UpdatePasswordHash, but it turns out it is only invoked by ResetPasswordAsync. For all other operations (CreateAsync, AddPasswordAsync, CheckPasswordAsync, RemovePasswordAsync) the private implementation with a password store is invoked. From an API perspective this is a very strange behavior since we had expected all updates of password hashes to use our override.

Our end goal was actually to extend the ValidatePasswordAsync method but it isn't virtual.

Expected Behavior

The protected virtual UpdatePasswordHash should be called so the customized logic is used for all operations and not only one. All operations should behave in the same way.

Steps To Reproduce

Make a custom UserManager class that derives from the built-in. Override the UpdatePasswordHash method and add some custom logic. This logic will only be invoked when resetting passwords and never in any other situation.

Exceptions (if any)

No response

.NET Version

.NET 6, 7, 8 and 9

Anything else?

Somewhat related to #12344

[MackinnonBuck]

@equist Could you clarify what you're trying to do in UpdatePasswordHash? Would it work to use an IPasswordHasher?

[equist]

@MackinnonBuck we override UpdatePasswordHash because we have to do a new implementation of ValidatePasswordAsync, which isn't virtual, so to make sure that our custom validation method is used we needed to override UpdatePasswordHash too.

ValidatePasswordAsync is overriden because we have validators for breached passwords (could be more than one), but we also have some special handling around how we handle errors (special error code for breached passwords) from our breach validators depending on configuration.

From an API perspective, I would argue that it is impossible to understand that a virtual method (UpdatePasswordHash) is just called from 1 of 5 (don't remember the exact number) places. As current implementation allows to override the actual behavior it shouldn't be virtual or it should always be used.

[halter73]

I did some digging, and it appears that the intention behind making UpdatePasswordHash protected was to allow calling it from other overridden methods like CreateAsync, AddPasswordAsync, CheckPasswordAsync, RemovePasswordAsync. It appears that no real thought was put into whether it actually needed to be virtual or where it should be called given that it is. See aspnet/Identity#1028 for context.

It's annoying, because making the change now to call it in more places theoretically could break existing applications that weren't expecting the overridden to be called from the new places. However, I doubt many people are actually overiding UpdatePasswordHash today given all the places it isn't called, so I'm comfortable with changing the behavior to always call the virtual.

[dotnet-policy-service]

Looks like this issue has been identified as a candidate for community contribution. If you're considering sending a PR for this issue, look for the Summary Comment link in the issue description. That comment has been left by an engineer on our team to help you get started with handling this issue. You can learn more about our Help Wanted process here