Do not require unique characters in "Password character type" constraint

Hello,
When a first used this functionality I was also confused about this, when I read the description I thought this constraint was to count the amount of characters in the password for each type ( not about unique characters ), and that's why we had the issue #2894929: Instructions unclear for constraint of Uppercase and Lowercase Characters to change these descriptions to make it clearer.

Although I think this could be done, I also think that the initial functionality ( with unique characters ) might be a good idea... I think we could we add an option, a checkbox, to choose between counting unique characters or count every character, what do you think?

Hi @COBadger! Thanks for the patch! I tested it and did a code review. I installed the module, tested the password change before and reproduced the issue, then tested again after applying the patch and the expected behavior (counting two or more incidences of the same character) was working fine. The code logic and modifications seem good to me! I would just like to get some more info on the test changes, because I think it is redundant with the previous data. If you meant for it to test the new behavior, I think the data should be:

[
  4,
  'letter',
  'pass',
  TRUE,
],

To assert if it is really counting the two “s” as two letters. The way it is, with ‘passw’, it would pass in the previous behavior and in the proposed one, thus becoming redundant.

As seen by the last provided data in the test, I think it was expected during its implementation that this functionality enforced unique characters in its constraints.
I agree with @COBadger thought that it is counterintuitive and runs against common user experience. From what I understood reading the IS and the discussion, this constraint should either:

1. Remove the unique characters requirement and allow equals characters, counting the sum of them like in the patch @COBadger made and the IS proposed resolution; or
2. Be worded correctly in the description and explicitly show that it enforces unique characters in the constraint, and maybe allow the user to set another constraint that allows for equal characters, like what @hmendes said.

Right?
I prefer the first approach, which is the one proposed, and agree with @COBadger arguments. Then we could add the unique characters constraint as a feature request if the community thinks it is needed.
I’m moving it back to Needs Work and assigning myself to make that test change. Apologies if I misunderstood something and thank you for the patch. I hope my thoughts were useful for the discussion.

Hi! Attached is the patch with the change in the test data and the interdiff from the previous patch.

Steps performed:
(1) Module installed
(2) Reproduction of the problem.
(3) Patch applied.
(4) Code review on changes.
(5) Retested with patch, issue resolved.

Assigning to myself as I'm reviewing/merging ready RTBC fixes/updates over the next few days.

This should be against 4.0.x as 8.x branches are no longer supported, so changing version.

Thanks everyone for the work on this issue. Moving back to needs work based on the following:

I've tested the original behavior and read the summary and comments above.

1. Regarding repeating characters, I found this:

Repeating characters is a different story. The current NIST standards explicitly say that complexity rules should not be used. (Examples of complexity rules include requiring upper AND lower case, special characters or numbers, denying passwords with repeating characters, etc.) So current NIST guidelines recommend against restrictions on repeating characters.

https://www.quora.com/How-does-restricting-password-length-or-repeating-...

I ready some other posts that talk about this as well. So, rather than add a "unique" checkbox per #6, I think we can move forward with this approach.

But this claims to be a list of NIST guidelines:

https://blog.netwrix.com/2022/11/14/nist-password-guidelines

which has:

Users should be prevented from using sequential characters (e.g., “1234”) or repeated characters (e.g., “aaaa”).

There is already a consecutive character constraint that can be used though I don't see a sequential one. A separate feature request issue can be created to add sequential character constraint.

2. The MR does not match the most recent patch. I'm okay with an MR or patch, so whoever updates can do either.

3. Nitpick: The logic could be slightly simpler, e.g.

$count_numeric = $count_numeric + $val;

could be:

$count_numeric += $val;

4. The documentation/messages should be updated to reflect these changes:

#2894929: Instructions unclear for constraint of Uppercase and Lowercase Characters

Assigning to myself for review.

Thanks everyone. I reviewed the code and tested MR 72 and it works as expected. This has been merged and will be in the next release.

That said, there still might be missing tests. Don't we want to test things like the following? If so, we can have a followup issue with more tests.

1. We set min numbers to 3 and we set password to 3 numbers that are the same.

2. We set min numbers to 3 and we set password to 3 numbers that are the different.

3. We set min numbers to 3 and we set password to 2 numbers that are the same (or different).

4. We set min numbers to 3 and we set password to 4 numbers that are the same (or different).

This is part of the new 4.0.1 release.