-
-
Notifications
You must be signed in to change notification settings - Fork 529
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
Change generatePassword() logic #15894
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Mark-H I would like your opinion on this change. |
if ($options['alphabet']) { | ||
$alphabet = array_merge(range('a', 'z'), range('A', 'Z')); | ||
shuffle($alphabet); | ||
return substr(implode($alphabet),0,$length); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return substr(implode($alphabet),0,$length); | |
return substr(implode($alphabet), 0, $length); |
} | ||
|
||
return $pass; | ||
return substr(bin2hex(random_bytes($length)),$length); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return substr(bin2hex(random_bytes($length)),$length); | |
return substr(bin2hex(random_bytes($length)), $length); |
Other than the phpcs issues I don't have a strong opinion on this - the use of The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests need to be updated to match new implementation.
@crystaldaking would it be possible for you to update the Unit Tests to match your implementation? |
for ($i = 0; $i < $length; $i++) { | ||
$pass .= $options['allowable_characters'][mt_rand(0, $ps_len - 1)]; | ||
|
||
if ($options['alphabet']) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may also require a check as I don't see the alphabet
key being required elsewhere. So something like if (!empty($options['alphabet'])) {
to avoid php warnings.
@crystaldaking ping... |
As this PR has gone stale, I recreated it at #16521. |
### What does it do? Re-up of #15894 Changes password generation method to be more secure. ### Why is it needed? Actually random generation. ### How to test Apply and see passwords still get generated. ### Related issue(s)/PR(s) Re-up of #15894 Fixes #15740 --------- Co-authored-by: crystaldaking <crystal@crystalapps.by> Co-authored-by: Jan Peca <pecajan@gmail.com>
What does it do?
Updated the password generation function using a truly random string
Why is it needed?
I am deeply convinced that the more a password looks like a hash, the less it attracts the attention of hackers. Moreover, this implementation uses a truly random string in contrast to the stock implementation
The
options
parameter as part of password generation is a rather useless parameter. It is not safe to specify salt, any other parameters simply do not occur to me. As an option, I left the implementation of the classicalphabet
password that can be invoked using optionsHow to test
Standard tests work, no logic is affected.
You can also test password generation in the user control panel
Related issue(s)/PR(s)
#15740