Skip to content
This repository has been archived by the owner on Dec 20, 2018. It is now read-only.

RequireNonLetterOrDigit does not allow to use letters and digits #709

Closed
JGrzybowski opened this issue Jan 12, 2016 · 15 comments
Closed

RequireNonLetterOrDigit does not allow to use letters and digits #709

JGrzybowski opened this issue Jan 12, 2016 · 15 comments
Assignees
Milestone

Comments

@JGrzybowski
Copy link

I used ASP.NET 5 MVC6 Project Template with User Authentication, changed default password requirements to:

o.Password.RequireDigit = false;
o.Password.RequireLowercase = true;
o.Password.RequireUppercase = true;
o.Password.RequireNonLetterOrDigit = true; 
o.Password.RequiredLength = 6;

When I try to register with passwords having Upper, lower and digit the user manager fails to create an account because Failed : PasswordRequiresNonLetterAndDigit When I change RequireNonLetterOrDigit to false problem disappears.

1.0.0-rc1-final

@MaximRouiller
Copy link

Do you have a sample password that fails?

@JGrzybowski
Copy link
Author

user0Pass

@MaximRouiller
Copy link

Seems to be a bug.

Here's the corresponding line: https://github.com/aspnet/Identity/blob/3.0.0-rc1/src/Microsoft.AspNet.Identity/PasswordValidator.cs#L55

And relevant code:

if (options.RequireNonLetterOrDigit && password.All(IsLetterOrDigit))
{
    errors.Add(Describer.PasswordRequiresNonLetterAndDigit());
}

@MaximRouiller
Copy link

That seems to be the bug. If a password have a digit, with RequireNonLetterOrDigit, it should be valid... yet if the password is all letters or digits... it fails.

@MaximRouiller
Copy link

Maybe the name should have been RequireNonLetter since this is exactly what this is doing.

@rustd? What do you think?

@rustd rustd added this to the 3.0.0-rc2 milestone Jan 12, 2016
@rustd
Copy link

rustd commented Jan 12, 2016

@HaoK?

@HaoK
Copy link
Member

HaoK commented Jan 12, 2016

It's always been this way. it means there must be one non letter and non digit.

@divega
Copy link

divega commented Jan 12, 2016

Ah, I think it is starting to make sense to me... So, if we could use parenthesis in names it would be called RequireNon(LetterOrDigit) 😄 But honestly, when I read it the first time I interpreted the same way as @JGrzybowski and @MaximRouiller apparently did, i.e. Require(NonLetter)OrDigit.

Maybe we can come up with a name that is no ambiguous. Was RequireNonAlphanumeric ever considered?

@HaoK
Copy link
Member

HaoK commented Jan 12, 2016

I forget the exact naming history. I think it was called that at some point. Also RequireNonLetterAndDigit I think too.

@divega
Copy link

divega commented Jan 12, 2016

See the SO questions:

http://stackoverflow.com/questions/25865952/requirenonletterordigit-not-correctly-validated

(I think there is a bug in the question, e.g. the flag should be set to true instead of false)

http://stackoverflow.com/questions/24796454/how-to-change-password-validation-in-asp-net-mvc-identity-2/24819318#24819318

And this forum thread:

http://forums.asp.net/t/1998093.aspx?Error+message+unclear+Passwords+must+have+at+least+one+non+letter+or+digit+character+

It seems customers have been hitting the ambiguity in the property name, the API documentation for it and even the validation error for a while.

@JGrzybowski
Copy link
Author

Also, there should be a way to require nonletter character. I think it was possible in previous version of framework.

@HaoK
Copy link
Member

HaoK commented Jan 13, 2016

How about we go with RequireNonAlphanumeric and change the error text to be: "Passwords must have at least one non alphanumeric character."

@HaoK HaoK self-assigned this Jan 13, 2016
@HaoK HaoK added the bug label Jan 13, 2016
@MaximRouiller
Copy link

@HaoK sounds very good! 😄

@HaoK
Copy link
Member

HaoK commented Jan 14, 2016

84804fe

@HaoK HaoK closed this as completed Jan 14, 2016
@HaoK HaoK added the 3 - Done label Jan 14, 2016
@MaximRouiller
Copy link

👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants