Skip to content

Conversation

@shethaadit
Copy link
Contributor

Do not throw an exception when password is null.

Summary of the changes (Less than 80 chars)

Description

PR Summary

This PR refactors and optimizes our password validation tests while also fixing a critical bug that was causing unintended exceptions for null passwords. The password validation method is designed to accept null values (handled as empty strings), but it was incorrectly throwing exceptions. This fix resolves the issue and ensures correct behavior.

Key Improvements:

  1. Bug Fix for null Password Handling: The ValidateAsync method is expected to accept null as input, treating it as an empty string. However, it was throwing an exception, resulting in failed tests for null passwords. This PR corrects that behavior, ensuring that null passwords are properly validated without errors, in line with the expected behavior.

  2. Combined Test Cases: Separate tests for null, empty, and too-short passwords have been consolidated into one comprehensive test using the [Theory] attribute with InlineData. This eliminates redundancy and ensures all edge cases (null, empty, and short passwords) are covered in one unified test.

  3. Improved Readability and Maintainability: The refactored test cases provide clearer intent and minimize repetitive setup, making the code easier to maintain in the long run.

  4. Performance Gains: By reducing the number of test methods and eliminating duplicate code, we optimize the test execution process, leading to improved performance without sacrificing test coverage.

This change not only resolves the bug but also enhances the maintainability and efficiency of our validation framework, driving significant long-term value.

Fixes #58133

@ghost ghost added the area-identity Includes: Identity and providers label Sep 30, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Sep 30, 2024
Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The alternative would be to make the password parameter a non-nullable string. We've been willing to make this kind of "breaking" change in the past since it's low impact. You'd get a runtime exception if you ever passed in null previously after all.

Changing the nullable annotation would be preferrable if we're worried about developers being confused by our use of IdentityErrorDescriber.PasswordTooShort for this case. An ArgumentNullException is clearer about exactly what's wrong with the password.

However, I'm fine with allowing null considering IPasswordValidator<TUser> and PasswordValidator<TUser> has declared the password parameter as a nullable string? since .NET 7. Using PasswordTooShort for null doesn't seem that much more confusing than doing the same for a string with just whitespace which we already do today, and it does seem nice to be able to leave the null checking to the password validator given null is a possible input. And I don't think it's worth adding anything to IdentityErrorDescriber just for this case.

@JamesNK @BrennanConroy Do either of you have opinions on this?

@JamesNK
Copy link
Member

JamesNK commented Oct 1, 2024

I think this fix is fine as is.

I would be in favor of changing the nullable annotation if ValidateAsync wasn't from an interface, but there is a chance that other validation implementations have different rules for null. Changing the annotation would impact them (they'd need to change their signature to avoid a warning).

@shethaadit
Copy link
Contributor Author

shethaadit commented Oct 1, 2024

@dotnet-policy-service agree

@halter73 halter73 merged commit 51b0c34 into dotnet:main Oct 1, 2024
27 checks passed
@halter73
Copy link
Member

halter73 commented Oct 1, 2024

Thanks @shethaadit!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-identity Includes: Identity and providers community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PasswordValidator should not throw when password is null

3 participants