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

tiff-o · 2022-08-11T13:28:26Z

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

lib/faker/default/internet.rb

Zeragamba · 2022-08-11T14:43:51Z

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 · 2022-08-11T15:09:03Z

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 · 2022-08-15T07:16:35Z

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...

lib/faker/default/internet.rb

Zeragamba

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

tiff-o · 2022-09-14T10:30:42Z

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.

stefannibrasil

This is looking great, @tiff-o thank you so much for your work on this! 😄

I left a couple of suggestions. Let me know what you think.

What do you think of updating the password docs to reflect the min_length required for mix_case and special_characters?

faker/doc/default/internet.md

Lines 34 to 39 in 9134280

    
           # Keyword arguments: min_length, max_length, mix_case, special_characters 
        
           Faker::Internet.password #=> "Vg5mSvY1UeRg7" 
        
           Faker::Internet.password(min_length: 8) #=> "YfGjIk0hGzDqS0" 
        
           Faker::Internet.password(min_length: 10, max_length: 20) #=> "EoC9ShWd1hWq4vBgFw" 
        
           Faker::Internet.password(min_length: 10, max_length: 20, mix_case: true) #=> "3k5qS15aNmG" 
        
           Faker::Internet.password(min_length: 10, max_length: 20, mix_case: true, special_characters: true) #=> "*%NkOnJsH4"

lib/faker/default/internet.rb

stefannibrasil · 2022-09-23T06:03:01Z

lib/faker/default/internet.rb

-          diff_rand = rand(diff_length + 1)
-          temp += Lorem.characters(number: diff_rand)
-        end
+        types = []


At a first glance, types does not share too much details about what's used for. Perhaps character_types or something along those lines would clarify its use here?

oh yes... i see what you mean! types was initially used for :upper, :lower etc but not anymore so i should have updated the name. what do you think of config / configuration to match the error message?

character_types would be my preference

I think character_types gives us a better idea of what it is doing.

lib/faker/default/internet.rb

tiff-o · 2022-09-23T07:10:03Z

thanks @stefannibrasil! will also update the docs 👍

Zeragamba · 2022-09-23T12:04:23Z

lib/faker/default/internet.rb

-          diff_rand = rand(diff_length + 1)
-          temp += Lorem.characters(number: diff_rand)
-        end
+        types = []


character_types would be my preference

Zeragamba · 2022-09-23T12:09:59Z

lib/faker/default/internet.rb

+          types << :mix_case
+          min_min_length += 2


Would be good to split this into :upper and :lower, with :lower as a default if mix_case is false.

The generator params could also be updated to have options { upper: true, lower: true } and have { mixed_case: true } be an alias for that

The generator params could also be updated to have options { upper: true, lower: true } and have { mixed_case: true } be an alias for that

hey @Zeragamba - would you mind please clarifying what you mean by this? have tried with variations of the below but don't think i'm on the right track...

def password(min_length: 8, max_length: 16, special_characters: false, mix_case: true, options: {} ) ... options[:lower] = true options[:upper] = true if mix_case options[:special_character] if special_characters ...

def password(min_length: 8, max_length: 16, special_characters: false, lower: true, upper: true) ... mix_case = true if lower && upper ...

@Zeragamba what do you think of this suggestion being done in a follow up PR? I see those as improvements that could be added later, and don't need to block this PR. For now, I feel like the priority is to fix the bug. What do you think?

Zeragamba · 2022-09-23T12:15:54Z

lib/faker/default/internet.rb

+        target_length = rand(min_length..max_length)
+
+        password = []
+        character_bag = Lorem.characters(number: target_length).chars


This would increase the probablity of those letters being more common in the final password. If the bag ends up being empty, then the user did not specify any types to use and thus should be an error.

Zeragamba · 2022-09-23T12:17:36Z

lib/faker/default/internet.rb


-        temp
+        password.shuffle.join


can you add a test to make sure the password is same when the same random seed is used?

stefannibrasil · 2022-10-12T14:44:55Z

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 · 2022-10-14T10:27:13Z

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 · 2022-11-27T01:35:45Z

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 · 2022-11-27T01:51:21Z

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 · 2022-11-27T02:08:27Z

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!

…_characters enabled

stefannibrasil

Thank you so much @tiff-o 💪 🎉

I'm going to merge this now. If you have any other improvements that you wanted to make, feel free to open a new PR 💯

doc/default/internet.md

tiff-o · 2022-12-01T04:21:40Z

amazing, will do. thanks @stefannibrasil!

thdaraujo · 2022-12-12T19:18:17Z

thanks @tiff-o for working on this! 🙏

tiff-o force-pushed the master branch 2 times, most recently from 0f638d5 to 48902c6 Compare August 11, 2022 13:32

Zeragamba suggested changes Aug 11, 2022

View reviewed changes

lib/faker/default/internet.rb Outdated Show resolved Hide resolved

lib/faker/default/internet.rb Outdated Show resolved Hide resolved

lib/faker/default/internet.rb Outdated Show resolved Hide resolved

Zeragamba self-assigned this Aug 11, 2022

Zeragamba added 🏃🏽 PR: In Progress 🐞 PR: Bugfix labels Aug 11, 2022

tiff-o force-pushed the master branch 2 times, most recently from fa96811 to dfdfc0b Compare August 15, 2022 07:09

stefannibrasil linked an issue Aug 16, 2022 that may be closed by this pull request

Short Faker::Internet.password are not including mix case chars. #2512

Closed

Zeragamba suggested changes Aug 19, 2022

View reviewed changes

lib/faker/default/internet.rb Outdated Show resolved Hide resolved

lib/faker/default/internet.rb Outdated Show resolved Hide resolved

tiff-o force-pushed the master branch from dfdfc0b to 4539d17 Compare August 26, 2022 04:51

tiff-o requested a review from Zeragamba August 26, 2022 05:01

Zeragamba suggested changes Aug 26, 2022

View reviewed changes

tiff-o force-pushed the master branch 2 times, most recently from 48641d3 to 3c17192 Compare September 14, 2022 10:16

tiff-o requested a review from Zeragamba September 14, 2022 10:30

stefannibrasil requested changes Sep 23, 2022

View reviewed changes

stefannibrasil requested a review from thdaraujo September 23, 2022 06:21

Zeragamba suggested changes Sep 23, 2022

View reviewed changes

stefannibrasil requested a review from Zeragamba November 19, 2022 19:49

fix: ensure passwords have correct characters when mix_case & special…

f049d9f

…_characters enabled

tiff-o force-pushed the master branch from 3c17192 to f049d9f Compare November 28, 2022 01:46

tiff-o requested review from stefannibrasil and removed request for thdaraujo and Zeragamba November 28, 2022 01:49

stefannibrasil approved these changes Nov 29, 2022

View reviewed changes

doc/default/internet.md Show resolved Hide resolved

stefannibrasil requested a review from Zeragamba November 29, 2022 02:16

stefannibrasil merged commit a76e075 into faker-ruby:main Nov 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

tiff-o commented Aug 11, 2022 •

edited

Loading

Zeragamba commented Aug 11, 2022

tiff-o commented Aug 11, 2022

tiff-o commented Aug 15, 2022

Zeragamba left a comment

tiff-o commented Sep 14, 2022

stefannibrasil left a comment •

edited

Loading

stefannibrasil Sep 23, 2022

tiff-o Sep 23, 2022

Zeragamba Sep 23, 2022

stefannibrasil Sep 23, 2022

tiff-o commented Sep 23, 2022

Zeragamba Sep 23, 2022

Zeragamba Sep 23, 2022

tiff-o Oct 16, 2022

stefannibrasil Nov 1, 2022

Zeragamba Sep 23, 2022

Zeragamba Sep 23, 2022

stefannibrasil commented Oct 12, 2022

tiff-o commented Oct 14, 2022

stefannibrasil commented Nov 27, 2022

tiff-o commented Nov 27, 2022

stefannibrasil commented Nov 27, 2022 •

edited

Loading

stefannibrasil left a comment

tiff-o commented Dec 1, 2022

thdaraujo commented Dec 12, 2022

	# Keyword arguments: min_length, max_length, mix_case, special_characters
	Faker::Internet.password #=> "Vg5mSvY1UeRg7"
	Faker::Internet.password(min_length: 8) #=> "YfGjIk0hGzDqS0"
	Faker::Internet.password(min_length: 10, max_length: 20) #=> "EoC9ShWd1hWq4vBgFw"
	Faker::Internet.password(min_length: 10, max_length: 20, mix_case: true) #=> "3k5qS15aNmG"
	Faker::Internet.password(min_length: 10, max_length: 20, mix_case: true, special_characters: true) #=> "*%NkOnJsH4"

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

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

Conversation

tiff-o commented Aug 11, 2022 • edited Loading

Summary

Zeragamba commented Aug 11, 2022

tiff-o commented Aug 11, 2022

tiff-o commented Aug 15, 2022

Zeragamba left a comment

Choose a reason for hiding this comment

tiff-o commented Sep 14, 2022

stefannibrasil left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiff-o commented Sep 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefannibrasil commented Oct 12, 2022

tiff-o commented Oct 14, 2022

stefannibrasil commented Nov 27, 2022

tiff-o commented Nov 27, 2022

stefannibrasil commented Nov 27, 2022 • edited Loading

stefannibrasil left a comment

Choose a reason for hiding this comment

tiff-o commented Dec 1, 2022

thdaraujo commented Dec 12, 2022

tiff-o commented Aug 11, 2022 •

edited

Loading

stefannibrasil left a comment •

edited

Loading

stefannibrasil commented Nov 27, 2022 •

edited

Loading