Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

The virtual UpdatePasswordHash is only invoked by ResetPasswordAsync #60252

Open
1 task done
equist opened this issue Feb 7, 2025 · 4 comments · May be fixed by #60592
Open
1 task done

The virtual UpdatePasswordHash is only invoked by ResetPasswordAsync #60252

equist opened this issue Feb 7, 2025 · 4 comments · May be fixed by #60592
Labels
area-identity Includes: Identity and providers bug This issue describes a behavior which is not expected - a bug. help wanted Up for grabs. We would accept a PR to help resolve this issue
Milestone
Backlog

Comments

@equist
Copy link

equist commented Feb 7, 2025

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
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-identity Includes: Identity and providers label Feb 7, 2025
@MackinnonBuck
Copy link
Member

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

@MackinnonBuck MackinnonBuck added the Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue. label Feb 19, 2025
@equist
Copy link
Author

equist commented Feb 19, 2025

@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.

@dotnet-policy-service dotnet-policy-service bot added Needs: Attention 👋 This issue needs the attention of a contributor, typically because the OP has provided an update. and removed Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue. labels Feb 19, 2025
@halter73
Copy link
Member

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.

@MackinnonBuck MackinnonBuck added bug This issue describes a behavior which is not expected - a bug. help wanted Up for grabs. We would accept a PR to help resolve this issue and removed Needs: Attention 👋 This issue needs the attention of a contributor, typically because the OP has provided an update. labels Feb 24, 2025
@MackinnonBuck MackinnonBuck added this to the Backlog milestone Feb 24, 2025
@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

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

@equist equist linked a pull request Feb 24, 2025 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area-identity Includes: Identity and providers bug This issue describes a behavior which is not expected - a bug. help wanted Up for grabs. We would accept a PR to help resolve this issue
Projects
None yet
Milestone
Backlog
Development

Successfully merging a pull request may close this issue.

Always call virtual UpdatePasswordHash #60252 equist/aspnetcore
3 participants
@halter73 @equist @MackinnonBuck