Bug 198391 - Add support of zxcvbn password strength checker to bugs.webkit.org website.
Summary: Add support of zxcvbn password strength checker to bugs.webkit.org website.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Website (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: lingho@apple.com
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-05-30 15:18 PDT by Ling Ho
Modified: 2019-06-05 20:22 PDT (History)
6 users (show)

See Also:


Attachments
Added option to select zxcvbn password estimator (107.10 KB, patch)
2019-05-30 15:30 PDT, Ling Ho
no flags Details | Formatted Diff | Diff
Patch (4.78 KB, patch)
2019-05-30 16:38 PDT, lingho@apple.com
no flags Details | Formatted Diff | Diff
Patch (5.57 KB, patch)
2019-06-05 12:44 PDT, lingho@apple.com
no flags Details | Formatted Diff | Diff
Patch (5.58 KB, patch)
2019-06-05 13:37 PDT, lingho@apple.com
no flags Details | Formatted Diff | Diff
Patch (5.58 KB, patch)
2019-06-05 14:06 PDT, lingho@apple.com
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ling Ho 2019-05-30 15:18:47 PDT
Currently bugs.webkit.org check for password complexity based on several simple rules that allows many weak passwords to be used. We would like to make sure our users, especially contributors use strong passwords.

Installing a password strength estimator will better serve the purpose.  zxcvbn is a reputable password strength estimator, and currently also deployed at trac.webkit.org.
Comment 1 Ling Ho 2019-05-30 15:30:32 PDT
Created attachment 370985 [details]
Added option to select zxcvbn password estimator
Comment 2 lingho@apple.com 2019-05-30 16:38:53 PDT
Created attachment 370994 [details]
Patch
Comment 3 David Kilzer (:ddkilzer) 2019-06-04 20:48:38 PDT
Comment on attachment 370994 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370994&action=review

r- only to update Bugzilla/Install/Requirements.pm to add the Data::Password::zxcvbn module as required.  The rest of the changes are optional.

> Websites/bugs.webkit.org/Bugzilla/Config/Auth.pm:115
> +                'letters_numbers_specialchars', 'zxcvbn' ],

Nit:  I hadn't heard of zxcvbn before, so using something a little more description (like 'zxcvbn_password_checker') would be easier to grok the first time you see it.

Not necessary to land the patch.

> Websites/bugs.webkit.org/Bugzilla/User.pm:34
> +use Data::Password::zxcvbn 'password_strength'; # WEBKIT_CHANGES

I think it's more idiomatic Perl to use qw() here:

use Data::Password::zxcvbn qw(password_strength); # WEBKIT_CHANGES

--

You also need to update Bugzilla/Install/Requirements.pm to list the Data::Password::zxcvbn module as required.

> Websites/bugs.webkit.org/Bugzilla/User.pm:2492
> +    } elsif ($complexity_level eq 'zxcvbn') {

Update 'zxcvbn' if you changed the name above.
Comment 4 lingho@apple.com 2019-06-05 12:44:47 PDT
Created attachment 371428 [details]
Patch
Comment 5 lingho@apple.com 2019-06-05 12:50:10 PDT
- Renamed selection "zxcvbn" to "zxcvbn_password_checker".
- Add Data::Password::zxcvbn module to Bugzilla/Install/Requirements.pm.
- Changed "use Data::Password::zxcvbn 'password_strength';" to "use Data::Password::zxcvbn qw(password_strength);"
Comment 6 David Kilzer (:ddkilzer) 2019-06-05 13:11:13 PDT
Comment on attachment 371428 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=371428&action=review

r=me, but please fix the [% error filter html %] issue at minimum.  After that, this is good to go.

> Websites/bugs.webkit.org/Bugzilla/User.pm:2495
> +        return 'Password is weak. ' . $est_strength->{feedback}->{warning} . '. '

Nit:  Format of this message is not like the others; would like something like `password_too_weak_SCORE' where "SCORE" is replaced with the numeric score, then you could change the template to pull out the score (which may not even be needed--if so, just drop "_SCORE" from the return value) and write the text to display to the user there (which would be easier to translate if we cared about this).

You can land the patch without this change, but please consider it.

> Websites/bugs.webkit.org/template/en/default/global/user-error.html.tmpl:1468
> +    [% error %]

This should be:

    [% error filter html %]
Comment 7 David Kilzer (:ddkilzer) 2019-06-05 13:12:38 PDT
Comment on attachment 371428 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=371428&action=review

>> Websites/bugs.webkit.org/template/en/default/global/user-error.html.tmpl:1468
>> +    [% error %]
> 
> This should be:
> 
>     [% error filter html %]

If you make the other change, you can just put the literal text here like this:

    The password is too weak.
Comment 8 lingho@apple.com 2019-06-05 13:37:41 PDT
Created attachment 371435 [details]
Patch
Comment 9 lingho@apple.com 2019-06-05 14:06:31 PDT
Created attachment 371436 [details]
Patch
Comment 10 lingho@apple.com 2019-06-05 14:10:20 PDT
- Added "FILTER html" to [% error %]
- Keeping the way message is displayed because it will relay warning message string generated by the zxcvbn module. Removed the trailing period since the string could be empty.
Comment 11 David Kilzer (:ddkilzer) 2019-06-05 20:15:52 PDT
Comment on attachment 371436 [details]
Patch

r=me
Comment 12 WebKit Commit Bot 2019-06-05 20:20:05 PDT
Comment on attachment 371436 [details]
Patch

Clearing flags on attachment: 371436

Committed r246140: <https://trac.webkit.org/changeset/246140>
Comment 13 WebKit Commit Bot 2019-06-05 20:20:07 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Radar WebKit Bug Importer 2019-06-05 20:22:22 PDT
<rdar://problem/51467679>