-
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[Validator] Remove bjeavons/zxcvbn-php
in favor of a builtin solution
#49856
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
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.
Thanks for this new PR @Spomky 😁
src/Symfony/Component/Validator/Constraints/NaivePasswordStrengthEstimator.php
Outdated
Show resolved
Hide resolved
Hi all, Here is my proposal for a builtin password strength estimator. It is based on the entropy as showed in the illustration here below and that I used in the wrokshops last week. Note that this not a bullet proof method and any improvments or suggestions are welcome. The estimator does 2 things:
Compared to The score is given depending on the entropy. A good password should be above 80 bits (from several sources including this one). Ping @chalasr, @WebMamba, @nicolas-grekas, @weaverryan, @javiereguiluz, @jdreesen, @fabpot |
Here is a late reaction to this validation constraint. If we can't explain to the end user why their "password strength is too low," that's bad for UX. The more complex the algorithm, the more users will get lost. The UI will have to explain that the username/first name/application name is forbidden. |
Hi @GromNaN, This is what the constraint validator does. But it only returns matches in the analyzed password and not the whole list of restricted data for privacy purpose. |
src/Symfony/Component/Validator/Constraints/DefaultPasswordStrengthEstimator.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Validator/Constraints/PasswordStrength.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Validator/Constraints/DefaultPasswordStrengthEstimator.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Validator/Constraints/DefaultPasswordStrengthEstimator.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Validator/Constraints/DefaultPasswordStrengthEstimator.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Validator/Constraints/DefaultPasswordStrengthEstimator.php
Outdated
Show resolved
Hide resolved
Feels like a common requirement would be to use fields on your user object as restricted words. Any way we can make that easier? |
c527ebf
to
cde9cc6
Compare
Yes is it. I usually have it in a form where the list is passed through an option. But when it is set as an attribute, I do not see how to refer to the user fields. |
What about if we allow it as a class-level attribute? Something like: #PasswordStrength(field: 'password', restrictedFields: ['username', 'email', 'lastName', 'firstName'])]
class User {} But... Let's save for a possible future enhancement, using in the form type directly works well enough. |
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.
Some suggestions for modern syntax.
src/Symfony/Component/Validator/Constraints/PasswordStrengthValidator.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Validator/Constraints/DefaultPasswordStrengthEstimator.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Validator/Constraints/DefaultPasswordStrengthEstimator.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Validator/Constraints/DefaultPasswordStrengthEstimator.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Validator/Constraints/DefaultPasswordStrengthEstimator.php
Outdated
Show resolved
Hide resolved
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php
Outdated
Show resolved
Hide resolved
Maybe the banned words and the strength computation should indeed be 2 separate constraints, allowing more flexibility. |
Many thanks for your advices. I will split it in two constraints and include the comments from the other reviewers. |
src/Symfony/Component/Validator/Constraints/PasswordStrengthEstimatorInterface.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Validator/Constraints/DefaultPasswordStrengthEstimator.php
Outdated
Show resolved
Hide resolved
src/Symfony/Bundle/FrameworkBundle/Resources/config/validator.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Validator/Constraints/PasswordStrengthValidator.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Validator/Constraints/NoBannedWordsValidator.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Validator/Constraints/NoBannedWordsValidator.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Validator/Constraints/NoBannedWordsValidator.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Validator/Constraints/NoBannedWordsValidator.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Validator/Constraints/PasswordStrengthValidator.php
Outdated
Show resolved
Hide resolved
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.
LGTM, I just have minor comments.
src/Symfony/Component/Validator/Constraints/PasswordStrengthValidator.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Validator/Constraints/PasswordStrengthValidator.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Validator/Constraints/PasswordStrengthValidator.php
Show resolved
Hide resolved
7643f88
to
8fe9137
Compare
src/Symfony/Component/Validator/Constraints/PasswordStrengthValidator.php
Show resolved
Hide resolved
8088aa1
to
d604d85
Compare
To compute the entropy, WDYT of this? $length = \strlen($password);
// Define character groups
$groups = [
'lowercase' => 'abcdefghijklmnopqrstuvwxyz',
'uppercase' => 'ABCDEFGHIJKLMNOPQRSTUVWXYZ',
'digits' => '0123456789',
'special' => '\\ !@#$%^&*()-_=+[]{}|;:<>,.?/`~\'"',
];
// Count the occurrences of each character group
$groupCounts = [];
$extraCharsCount = 256;
foreach ($groups as $group => $chars) {
$password = preg_replace('/['.preg_quote($chars, '/').']/', '', $password, -1, $groupCounts[$group]);
$extraCharsCount -= \strlen($chars);
}
// Create an extra group for characters not explicitly listed
$groupCounts['extra'] = \strlen($password);
$groups['extra'] = str_repeat('-', $extraCharsCount);
// Calculate size of the symbol pool
$symbolPool = 0;
foreach ($groupCounts as $group => $count) {
if ($count) {
$symbolPool += \strlen($groups[$group]);
}
}
return $length * log($symbolPool, 2); Or alternatively for the last part: // Calculate the entropy of each character group
$entropy = 0;
foreach ($groupCounts as $group => $count) {
if ($count) {
$entropy += $count * log(\strlen($groups[$group]), 2);
}
}
return $entropy; |
src/Symfony/Component/Validator/Constraints/PasswordStrength.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Validator/Constraints/PasswordStrength.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Validator/Constraints/PasswordStrength.php
Outdated
Show resolved
Hide resolved
d604d85
to
6b2bf22
Compare
Thank you @Spomky. |
…omky) This PR was merged into the 6.3 branch. Discussion ---------- [Validator] Document `PasswordStrength` constraint These modifications are proposed as per ~symfony/symfony#49789 => symfony/symfony#49856 Commits ------- ec51dd2 PasswordStrength Documentation pages
As per the discussion in #49831, this PR aims at removing
bjeavons/zxcvbn-php
in favor of a builtin solution.The password strength estimator is a PHP implementation of deanilvincent/check-password-strength, but can be changed at will.