-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Validator] Revert "feature #49789 New PasswordStrength
constraint (Spomky)"
#49831
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
PasswordStrength
constraint (Spomky)"PasswordStrength
constraint (Spomky)"
Fine by me to revert for the given reason. Having this on the server side makes sense to me, see eg #49821, which would enable nice password migration strategies. It'd be great to figure out a working algo/lib that could work both on the frontend and on the backend. @weaverryan spotted this lib, which looks quite simple to port to PHP: |
Note that instead of reverting, we would happily accept a PR that replaces the implementation. |
ping @Spomky FYI |
Hi, OK I understand the problem here. |
I think we need this to work out-of-the-box with a properly maintained reference implementation anyway :) |
The lib is in bad maintenance state so I'm not sure we need to make it easy to use it. Also, options are specific to each password checkers so I'm not sure we can have this abstraction. |
Regarding this, I am preparing a stimulus component to show the strength while typing the password. |
There is a modern TypeScript rewrite: https://github.com/zxcvbn-ts/zxcvbn |
As a symfony/* component or maybe I could handle this in a new repository I own |
Fine by me as long as you maintain it :) |
What I find problematic with I like @nicolas-grekas idea to port this JS library to PHP (it's just a few lines of code and it could be enough for most apps):
|
OK I get it now.
@chalasr yes! In paralell, I will work on a side project based on the typescript implementation shared by @jdreesen. |
I like the idea! I was thinking about similar as well 😁 |
If we really want to make the implementation swappable, I'd suggest using a closure instead of adding an interface. |
Alternative: #49856. Please close this one once it is ready |
… builtin solution (Spomky) This PR was merged into the 6.3 branch. Discussion ---------- [Validator] Remove `bjeavons/zxcvbn-php` in favor of a builtin solution | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | yes | New feature? | yes <!-- please update src/**/CHANGELOG.md files --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | 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](https://github.com/deanilvincent/check-password-strength/blob/master/index.js), but can be changed at will. Commits ------- 6b2bf22 Remove bjeavons/zxcvbn-php in favor of a builtin solution
This reverts commit c375406, reversing changes made to 497e966.
The dev dependency added in #49789 looks problematic:
https://github.com/bjeavons/zxcvbn-php has no commit in 2 years, ignored vulnerability reports + unmerged php compatibility fixes.
Worse, the JS project it's based on didn't make any change in 6 years, has ignored security reports as well and seems to be officially abandoned (dropbox/zxcvbn#290, dropbox/zxcvbn#295).
I propose to revert this change for now, then revisit the topic as soon as possible.
Also some core members argued that this feature could be better handled on the frontend i.e. rewritten as a UX component or a cookbook example.
Either way, if we want to do re-add this, we would need to fork and update the abandoned package. And if it's frontend, we need to find a maintained alternative.