fix: ensure generated passwords have correct characters when mix_case & special_characters enabled

[tiff-o]

Summary

Previously, when mix_case and special_characters were enabled, the generated password would sometimes exclude the mix_case characters because they were being overwritten by special characters instead.

This PR ensures that when mix_case and special_characters are both true (unless the generated password length is not enough to include all), the generated password includes at least:

• 1 upper case letter
• 1 lower case letter
• 1 special character

Should the generated password not allow for this (e.g. max_length is 3 or less), then raise an error.

Fixes Issue #2512

[Zeragamba]

I wonder if it might be better to simplify the password generation code:

Create a bag of characters
For each type of character required (upper, lower, digits, special, etc...)
  Add one into the bag (enforcing at least one of each). 
Until the password reaches the desired length (somewhere between min and max)
  Randomly add another character from the full set 
Then shuffle the bag
Join the contents and return the password.

[tiff-o]

I wonder if it might be better to simplify the password generation code:

Create a bag of characters
For each type of character required (upper, lower, digits, special, etc...)
  Add one into the bag (enforcing at least one of each). 
Until the password reaches the desired length (somewhere between min and max)
  Randomly add another character from the full set 
Then shuffle the bag
Join the contents and return the password.

i was thinking something along these lines too.. it does currently seem to be more convoluted than it probably needs to be. i'll give simplifying it a go.

[tiff-o]

hey @Zeragamba - have made some changes to ensure that we get the correct number of each character. noticed that the it currently does not error when max_length < min_length so maintained this behaviour. i'm not sure if it was intentionally allowed...

[tiff-o]

I still feel like the password generation can still be made a lot simplier.

There's lots of extra work to keep track of the number of different types, and specific goals for each type, when we only really care about there being at least one of each time.

I was hoping for something more like this:

types = [ :upper, :lower, :special, :spaces ]

password = ""
character_bag = []

if types.include?(:lower)
  lower_chars = ('a'..'z').to_a
  password += lower_chars[rand(lower_chars.count - 1)]
  character_bag += lower_chars
end

#... repeat for other types

target_length = rand(min_length..max_length)
while password.length < target_length
  password += character_bag[rand(character_bag.count - 1)]
end

return password.split.shuffle.join

thanks, have just updated... hopefully more in line with what you had in mind.

i didn't handle upper & lower case as separate types and instead considered mix_case to be 1 of each given that upper & lower case only really matters if mix_case is true, but can change that if there's a preference to have them as separate types.

also decided to use Lorem.characters(number: target_length).chars as the initial character_bag so we don't end up with 0 characters in the event that both mix_case & special_characters is false.

[tiff-o]

thanks @stefannibrasil! will also update the docs 👍

[stefannibrasil]

hi @tiff-o this is looking like it's going in a great direction. Do you need any help? I'd love to see your work merged. Thanks!

[tiff-o]

hi @tiff-o this is looking like it's going in a great direction. Do you need any help? I'd love to see your work merged. Thanks!

hey @stefannibrasil - thanks for the nudge & apologies for letting this go cold. have had a bit on but will be able to take another look at this in the next few days and let you know if any questions. appreciate your patience!

[stefannibrasil]

hey @tiff-o I think this PR is in a good shape to be merged after it's rebased. Any other improvements can be done in a follow up PR (we can open a new issue for it, for example). I don't want this one to stay open for too long since it solves the bug, and we can improve on it later. What do you think?

[tiff-o]

hey @tiff-o I think this PR is in a good shape to be merged after it's rebased. Any other improvements can be done in a follow up PR (we can open a new issue for it, for example). I don't want this one to stay open for too long since it solves the bug, and we can improve on it later. What do you think?

Hey @stefannibrasil - that sounds good, thanks! I can't get to my computer right now but can rebase and tag you for review within the next 24 hours. Does that work?

[stefannibrasil]

hey @tiff-o I think this PR is in a good shape to be merged after it's rebased. Any other improvements can be done in a follow up PR (we can open a new issue for it, for example). I don't want this one to stay open for too long since it solves the bug, and we can improve on it later. What do you think?

Hey @stefannibrasil - that sounds good, thanks! I can't get to my computer right now but can rebase and tag you for review within the next 24 hours. Does that work?

Absolutely! Just a reminder to add the tests that @Zeragamba mentioned. Thank you!

[tiff-o]

amazing, will do. thanks @stefannibrasil!

[thdaraujo]

thanks @tiff-o for working on this! 🙏