Skip to content

[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

Merged
merged 1 commit into from
Mar 31, 2023

Conversation

Spomky
Copy link
Contributor

@Spomky Spomky commented Mar 29, 2023

Q A
Branch? 6.3
Bug fix? yes
New feature? yes
Deprecations? no
Tickets Fix #49831
License MIT
Doc PR symfony/symfony-docs#18124 will be updated

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.

Copy link
Contributor

@WebMamba WebMamba left a 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 😁

@Spomky
Copy link
Contributor Author

Spomky commented Mar 29, 2023

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:

  • First, it spots forbidden words such as username, family name, given name application name or any other words. These words are then removed from the passwords for the next step
  • Next, it counts the number of unique chars and computes the entropy

Compared to zxcvbn-like libraries, it lacks in a few things such as L33T detection, dictionnary mapping or adjancy graphs. However gives good results in most situations.

The score is given depending on the entropy. A good password should be above 80 bits (from several sources including this one).

Password entropy

Ping @chalasr, @WebMamba, @nicolas-grekas, @weaverryan, @javiereguiluz, @jdreesen, @fabpot

@Spomky Spomky marked this pull request as ready for review March 29, 2023 13:31
@GromNaN
Copy link
Member

GromNaN commented Mar 29, 2023

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.

@Spomky
Copy link
Contributor Author

Spomky commented Mar 29, 2023

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.

@Spomky Spomky marked this pull request as draft March 29, 2023 18:51
@Spomky Spomky marked this pull request as ready for review March 29, 2023 18:55
@kbond
Copy link
Member

kbond commented Mar 29, 2023

Feels like a common requirement would be to use fields on your user object as restricted words. Any way we can make that easier?

@Spomky Spomky marked this pull request as draft March 29, 2023 18:58
@Spomky Spomky force-pushed the features/no-zxcvbn branch from c527ebf to cde9cc6 Compare March 29, 2023 19:17
@Spomky
Copy link
Contributor Author

Spomky commented Mar 29, 2023

Feels like a common requirement would be to use fields on your user object as restricted words. Any way we can make that easier?

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.

@kbond
Copy link
Member

kbond commented Mar 29, 2023

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.

Copy link
Contributor

@jdreesen jdreesen left a 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.

@stof
Copy link
Member

stof commented Mar 30, 2023

Maybe the banned words and the strength computation should indeed be 2 separate constraints, allowing more flexibility.

@Spomky
Copy link
Contributor Author

Spomky commented Mar 30, 2023

Many thanks for your advices. I will split it in two constraints and include the comments from the other reviewers.

@Spomky Spomky marked this pull request as ready for review March 30, 2023 10:39
Copy link
Member

@nicolas-grekas nicolas-grekas left a 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.

@Spomky Spomky force-pushed the features/no-zxcvbn branch from 7643f88 to 8fe9137 Compare March 30, 2023 12:28
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Mar 30, 2023

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;

@Spomky Spomky force-pushed the features/no-zxcvbn branch from d604d85 to 6b2bf22 Compare March 30, 2023 16:51
@fabpot
Copy link
Member

fabpot commented Mar 31, 2023

Thank you @Spomky.

@fabpot fabpot merged commit 48d5236 into symfony:6.3 Mar 31, 2023
OskarStark added a commit to symfony/symfony-docs that referenced this pull request Mar 31, 2023
…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
@Spomky Spomky deleted the features/no-zxcvbn branch March 31, 2023 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.