Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve randomness of Password Generator #62

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

DavisNT
Copy link

@DavisNT DavisNT commented Nov 15, 2024

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • New feature or enhancement
  • UI change (please include screenshot!)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Internationalization and localization
  • Other (please describe):

What is the current behavior?

Currently when inserting a new character in the password string all selected character classes (uppercase letters, lowercase letters, digits and special characters) are picked first, then within the picked character class a specific character is picked.
This leads to passwords being skewed towards characters from smaller classes. I.e. the probability of a character from a smaller (containing less characters) character class to appear in the password is higher. This is skewed even more by exclusions.

This can be tested by opening Password Generator, selecting Lowercase characters, Uppercase characters and Digit characters, and typing "234567890" (without quotes) in ExcludedCharacters field, and generating 10 passwords of 100 characters each. It should be very visible that around 1/3 of the characters in the generated passwords is the digit 1.

Issue Number: N/A

What is the new behavior?

When inserting a new character in the password string the character is picked from a string containing all the non-excluded characters from all the selected character classes hence the probability of any character being picked is equal.

Other information

Quality check

Before creating this PR:

  • Did you follow the code style guideline as described in CONTRIBUTING.md I didn't find coding style mentioned in this link.
  • Did you build the app and test your changes?
  • Did you check for accessibility? On Windows, you can use Accessibility Insights for this.
  • Did you verify that the change work in Release build configuration
  • Did you verify that all unit tests pass
  • If necessary and if possible, did you verify your changes on:
    • Windows
    • macOS
    • Linux

* Equalize probability that any character (instead of character class) might be picked when inserting a character into the password string.
* Fix off-by-one error that prevented insertion of new characters at the end of password string (theoretically this insertion is not needed assuming the PRNG is good, but it should do no harm either).
Reduce probability of PasswordGeneratorHelperTests failing due to randomly generated passwords not containing certain characters.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant