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

Add a Password Option to set the number of unique chars #1097

Closed
sebastienros opened this issue Feb 9, 2017 · 7 comments
Closed

Add a Password Option to set the number of unique chars #1097

sebastienros opened this issue Feb 9, 2017 · 7 comments

Comments

@sebastienros
Copy link
Member

I think it's a pretty common idea that forcing different types of chars (numeric, special, upper, ...) is not more secure than the chars just being non-repetitive and use a long password. We can already set the minimum length for a password, but I would like to also set how many different chars it should be made of.

PasswordOptions.RequiredUniqueChars

Obviously it would have to be less than PasswordOptions.RequiredLength or equal.

c.f. https://twitter.com/codinghorror/status/829484158208471041

@VahidN
Copy link

VahidN commented Feb 9, 2017

You can always customize the built-in password validator and replace it completely:

    /// <summary>
    /// Extending the Built-in Password Validation
    /// </summary>
    public class CustomPasswordValidator : PasswordValidator<User>
    {

        public CustomPasswordValidator(
            IdentityErrorDescriber errors // This is necessary for the globalization to work
            ) : base(errors)
        {}

        public override async Task<IdentityResult> ValidateAsync(UserManager<User> manager, User user, string password)
        {
            // First use the built-in validator
            var result = await base.ValidateAsync(manager, user, password).ConfigureAwait(false);
            var errors = result.Succeeded ? new List<IdentityError>() : result.Errors.ToList();

            // Extending the built-in validator
            if (!areAllCharsEuqal(password))
            {
                errors.Add(new IdentityError
                {
                    Code = "PasswordIsNotSafe",
                    Description = "All password chars are the same."
                });
            }

            return !errors.Any() ? IdentityResult.Success : IdentityResult.Failed(errors.ToArray());
        }

        private static bool areAllCharsEuqal(string data)
        {
            if (string.IsNullOrWhiteSpace(data)) return false;
            data = data.ToLowerInvariant();
            var firstElement = data.ElementAt(0);
            var euqalCharsLen = data.ToCharArray().Count(x => x == firstElement);
            if (euqalCharsLen == data.Length) return true;
            return false;
        }
    }

Then replace the built-in one:

services.AddScoped<IPasswordValidator<User>, CustomPasswordValidator>();
services.AddScoped<PasswordValidator<User>, CustomPasswordValidator>();

@sebastienros
Copy link
Member Author

@VahidN you are totally right, right after creating this issue I was thinking there had to be a way to provide custom logic, thanks for the sample.

Is it replacing completely the current implementation based on PasswordOptions or just adding another layer of validation?

So I assume having a special password option is just about how often would people need it, and probably this one was already discussed internally and didn't make it to the list. We'll see what they say about it.

@VahidN
Copy link

VahidN commented Feb 9, 2017

Is it replacing completely the current implementation based on PasswordOptions or just adding another layer of validation?

It still calls the base.ValidateAsync of the original PasswordValidator. So the current validation mechanism will be applied first and then adds another layer of validation.

@HaoK
Copy link
Member

HaoK commented Feb 9, 2017

Yeah its up to the custom validator to decide if it wants to chain or not, I don't remember if @blowdart came up with the original list of password options or not, but I'll leave it to him to make the call on whether its a candidate for support by default.

There is also a related work item in the backlog to consider tweaking the validators to be a list that are all run, instead of a single one that is replaced, which would make chaining less necessary. There haven't been any customer requests for this change so it hasn't been high on my priority list to this point...

@danroth27
Copy link
Member

No milestone?

@HaoK HaoK added this to the 2.0.0 milestone Feb 13, 2017
@HaoK
Copy link
Member

HaoK commented Feb 13, 2017

Think of the poor release notes!!

@HaoK
Copy link
Member

HaoK commented Feb 13, 2017

Good catch btw @danroth27 :)

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

4 participants