Ensure Faker::Internet.password method behavior is consistent with length parameters

[francktrouillez]

Motivation / Background

When using Faker::Interner.password, the min_length and max_length options are not always respected.

For instance, running the following snippet will give password out of the desired length range:

Faker::Internet.password(min_length: 2, max_length: 2) # => "Pa1" => 3 characters while I was expecting 2
Faker::Internet.password(min_length: 1, max_length: 1, mix_case: false) # => "9n" => 2 characters while I was expecting 1
Faker::Internet.password(min_length: 1, max_length: 1, mix_case: false, special_characters: true) # => "!5u" => 3 characters while I was expecting 1
Faker::Internet.password(min_length: 3, max_length: 3, mix_case: true, special_characters: true) # => "Y2#h" => 4 characters while I was expecting 3

This can lead to unexpected behavior when using the Faker::Internet.password method.

Changes proposed in this pull request

This PR fixes the issue by ensuring that the password generated respects the min_length and max_length options.

• This is done so by specifying the behavior of the mix_case option from the Faker::Internet.password method, which is to either force-include an uppercase letter AND a lowercase letter if set to true, or to only use lowercase letters if set to false (without enforcing the presence of a letter).
• This also removes the "force-addition" of digit, which lead to the password being one character longer than expected. I've opened a separate PR to enable or not the force-addition and the use of digits Allow to disable digits in Faker::Internet.password method #3033

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
• Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
• Tests are added or updated if you fix a bug, refactor something, or add a feature.
• Tests and Rubocop are passing before submitting your proposed changes.

If you're proposing a new generator or locale:

• [ ] Double-check the existing generators documentation to make sure the new generator you want to add doesn't already exist.
• [ ] You've reviewed and followed the Contributing guidelines.

Closing notes

I'm open to any feedback or suggestions on how to improve this PR if this solution makes sense.

Thanks!

[stefannibrasil]

Hi @francktrouillez I wanted to say this is in my todo list and should give a review by next week.

[stefannibrasil]

hi @francktrouillez thank you for your patience.

I think what we need to do instead is improve the docs. This is expected behaviour, it's just not clear because the examples do not showcase this specific scenario where the values you set are not enough to work with the extra configurations.

When setting mix_case or special_characters, the generator requires some min and max lengths. Here are some examples:

Faker::Internet.password(min_length: 1, max_length: 2, mix_case: false, special_characters: true) => "3y" # special_characters require max_length: 3, 
Faker::Internet.password(min_length: 2, max_length: 2, mix_case: true, special_characters: false) => "1iK"
Faker::Internet.password(min_length: 2, max_length: 3, mix_case: true, special_characters: false)
=> "jB0"
Faker::Internet.password(min_length: 2, max_length: 3, mix_case: false, special_characters: true) => "%i8"

When setting mix_case and special_characters, it's required some minimum length so it might add one extra character, otherwise it's not long enough to combine these extra configurations. mix_case adds two more characters so it can have enough characters to work:

https://github.com/faker-ruby/faker/blob/main/lib/faker/default/internet.rb#L135

The docs do mention setting these extra configuration will add at least one more character: https://github.com/faker-ruby/faker/blob/main/lib/faker/default/internet.rb#L107-110

Would you be up for improving the docs to have more examples of these scenarios? We can be more specific on the minimum requirements for these extra configurations to work (i.e, a minimum of 3 max_length to be able to work). If you're up for it, I'd appreciate it. Otherwise, let me know and I can do it.

I'm thinking of more examples to focus the combination of arguments:

Faker::Internet.password(min_length: 3, max_length: 4, mix_case: true, special_characters: true) => "B4z!"
Faker::Internet.password(min_length: 3, max_length: 3, mix_case: true, special_characters: true) => "!sQ2"
Faker::Internet.password(min_length: 3, max_length: 3, mix_case: false, special_characters: true) => "3^w"

Thank you.

[francktrouillez]

Thanks for the answer @stefannibrasil! No worries for the time, nothing critical here.

I think what we need to do instead is improve the docs. This is expected behaviour, it's just not clear because the examples do not showcase this specific scenario where the values you set are not enough to work with the extra configurations.

I personally feel that besides the documentation, the behavior of the method itself isn't expected.

Faker::Internet.password(min_length: 2, max_length: 2) # => "Pa1" => 3 characters while I was expecting 2

I personally would never expect this method call to generate a string of length 3. I could check the documentation afterwards, but even if it said there, I would be confused as to what's the point of having a min_length and max_length if the method doesn't respect those options.

If you agree with the above, I believe there are two ways to fix this:

• We make the validation of the minimum length of the password more strict to reflect reality, by changing it from 1 to 2, as the current implementation will ALWAYS add both a digit and a lowercase letter, regardless of the min_length and max_length options, meaning all password are always of length 2 at least.
• We avoid automatically adding a digit and a lowercase letter that bypasses the min_length and max_length options, and we instead consider them as part of a "characters set" from which we sample the password, without enforcing their presence. (we could argue whether we should allow to configure the presence of a digit and a lowercase letter, but that's another story [see Allow to disable digits in Faker::Internet.password method #3033])

I'm happy to implement changes for either of these solutions, but I'm personally not a huge fan of only changing the documentation, as it highlights a mismatch between the method's behavior and the documentation.

What do you think?