Issue: Admins are unable to specify a complexity requirement for user passwords. Recommended Remediation: Provide an option that allows an admin to specify password complexity requirements for all passwords. A possible example is as follows: Require passwords to contain: [ ] lowercase letters [ ] uppercase letters [ ] numbers [ ] special characters Note: This bug is filed in addition to bug 330846 which is requesting an increase of the minimum password length requirement.

Not an issue that needs to be private.

Yea, I always debate (In reply to comment #1) > Not an issue that needs to be private. Yea, I debate that and generally default to marking the security flag for issues related to security. But I tend to agree with you.

We have been using the this patch and works fine. I can't claim the credit for the patch but can't remember where I got it from either.

Comment on attachment 496125 [details] [diff] [review] Complex Passwords Wrong flag status.

I have applied the same patch to the Bugzilla 4 RC_1

Comment on attachment 497274 [details] [diff] [review] ComplexPassword 4_RC1 We won't take this feature on the 4.0 branch. The code is feature-frozen for a long time.

Comment on attachment 496125 [details] [diff] [review] Complex Passwords >Index: Bugzilla/Config/Auth.pm >+ name => 'passwordregexp', >+ type => 't', >+ default => q:[1-9],[A-Za-z]:, >+ checker => \&check_regexp >+ }, >+ >+ { >+ name => 'passwordregexpfailmessage', >+ type => 't', >+ default => "Passwords must contain both letters and numbers.", >+ }, We shouldn't use regexps to configure password complexity. They are a pain to write correctly to do what you want, and only advanced Perl hackers would know regexps well enough to do it correctly. Better is to offer a select list, following the idea in comment 0. Also, the error message cannot be translated this way; it would be hardcoded. Using the select list above, it would be trivial to make the error message localizable.

Made regexs into <select> options and error message is in user-error.html.tmpl

Comment on attachment 497494 [details] [diff] [review] ComplexPassword v3 >=== modified file 'Bugzilla/Config/Auth.pm' >+ name => 'password_complexity', >+ type => 'o', >+ choices => [ '[1-9]', '[A-Z]', '[a-z]', '[@#$%^&+=]' ], As discussed on IRC, this is pretty meaningless to most admins. Regexps used by Bugzilla should never be displayed, but used in the backend. Put real text to admins. As I suggested yesterday, we should probably have the following options: - No constraints - Mixed letters - Letters and numbers - Letters, numbers and special characters Your regexps means that a user could have a pure numerical password, or a pure lowercase (respectively uppercase) password, which do not imply any complexity.

Comment on attachment 498551 [details] [diff] [review] ComplexPassword v3.1 >=== modified file 'Bugzilla/User.pm' >+sub test_password { >+ my $password_regexp = '.*'; Use qr// instead of single quotes for regexps. >+ warn "PASS: ".$password ." REGEX: ". $password_regexp; Debug code. >+ if ($password !~ $password_regexp) { >+ return 0 >+ } An error should be thrown instead. Also, this check should be done as part of $user->set_password(), which means this patch will conflict with the other one you are working on, see bug 284570. >=== modified file 'editusers.cgi' >+ if ($cgi->param('password')) { >+ $valid_password = Bugzilla::User::test_password($cgi->param('password')); >+ } Should probably be part of $user->set_password(). I didn't look further.

I made the test password part of validate_password as a password needs to be validated before set but this way Admins have to meet the complexity levels as well, which I think would be fair for a system with complex passwords.

Comment on attachment 499017 [details] [diff] [review] Calidated Complex Passwords >=== modified file 'Bugzilla/User.pm' >+ my $password_regexp = qr/.*/; >+ if ($complexity_level eq 'no_constraints') { >+ $password_regexp = qr/.*/; This IF block is useless as .* is already set above. >+ } elsif ($complexity_level eq 'mixed_letters') { >+ $password_regexp = qr/.*(?=.*[a-z])(?=.*[A-Z]).*/; >+ } elsif ($complexity_level eq 'letters_numbers') { >+ $password_regexp = qr/.*(?=.*[a-z])(?=.*[A-Z])(?=.*\d).*/; >+ } elsif ($complexity_level eq 'letters_numbers_specialchars') { >+ $password_regexp = qr/.*(?=.*[a-z])(?=.*[A-Z])(?=.*\d)(?=.*[@#\$%\^&+=]).*/; >+ } Using the (?=.*[...]) syntax is clever, however http://docstore.mik.ua/orelly/perl/cookbook/ch06_18.htm#ch06-19434 says that it may require more scans of the string than the usual $foo =~ /.../ && $foo =~ /.../ syntax. This shouldn't be a big deal, though. A few more things: 1) http://perldoc.perl.org/POSIX.html#islower recommends to use [[:lower:]] instead of [a-z] and [[:upper:]] instead of [A-Z]. I think this makes sense for languages using non-ASCII characters. Note that "é" is not seen as lowercase by [[:lower:]], \p{Ll} and [a-z]. Not sure how to fix this, nor am I sure how they would behave with languages like russian or japanese. Anyway, I think the [[:foo:]] syntax should be used. 2) We should use [[:punct:]] instead of [@#\$%\^&+=], see http://perldoc.perl.org/perlrecharclass.html#[5] ([5] is part of the URL). 3) To follow the examples given at http://docstore.mik.ua/orelly/perl/cookbook/ch06_18.htm#ch06-19434, we could remove the leading and trailing .* and replace them by ^ at the beginning, i.e. qr/.*(...).*/ would become qr/^(...)/. Not sure if this makes any difference, though. 4) For letters_numbers_specialchars, maybe it's enough to just look for \w instead of [a-z][A-Z]. I would say that ZT8ç%W/ is complex enough, even without having any lowercase character. This would also save us from the problem described above about accented characters. >=== modified file 'editusers.cgi' >+ validate_password($password); >+ if ($cgi->param('password')) { >+ validate_password($cgi->param('password')); >+ $otherUser->set_password($cgi->param('password')); >+ } Both changes are useless. set_password() already calls validate_password() via _check_password(). So you don't need to edit this file at all. >=== modified file 'template/en/default/admin/params/auth.html.tmpl' >+ password_complexity => "Controls management of complex passwords Maybe "Set the complexity required for passwords"? >+ no_constraints - Passwords are not restricted in anyway We should make it clearer that the minimum length of 6 characters is still applicable. >+ letters_numbers_specialchars - Passwords must contain at least one >+ UPPER and one lower case letter, a number and a >+ special character from @#$%^&+= Per my comment above about [[:punct:]], we shouldn't restrict the list of special characters, but maybe simply enumerate one or two of them. >=== modified file 'template/en/default/global/user-error.html.tmpl' >+ [% IF passregex.search('specialchars') %] >+ <li>special character from @,#,$,%,^,&,+,=</li> Same here. Otherwise, your patch looks good, and the comments above are easy to address. We are very close. :)

I am not totally happy about nitness of complexity if statement but couldn't come up with something nitter

Comment on attachment 501322 [details] [diff] [review] Complex Passwords w/ nit RegEx >=== modified file 'Bugzilla/Config/Auth.pm' >+ choices => [ 'no_constraints', 'mixed_letters', 'letters_numbers', 'letters_numbers_specialchars' ], This line is a bit too long. >=== modified file 'Bugzilla/User.pm' >+ if ($complexity_level eq 'letters_numbers_specialchars') { >+ ThrowUserError('password_not_complex') if ($password !~ /\w/ || >+ $password !~ /\d/ || >+ $password !~ /[[:punct:]]/ ); I agree with you that your previous regexpes were much nicer. :) Note that in Bugzilla, we usually prefer the $foo || $bar syntax. >=== modified file 'template/en/default/admin/params/auth.html.tmpl' >+ no_constraints - Passwords restriction off. ${terms.Bugzilla} passwords >+ must be at least ${constants.USER_PASSWORD_MIN_LENGTH} characters long I think we should be clearer that the min length applies in all cases. >+ letters_numbers_specialchars - Passwords must contain at least one >+ UPPER and one lower case letter, a number and a It should rather be "one UPPER *or* one lower", not *and*. >=== modified file 'template/en/default/global/user-error.html.tmpl' >+ <ul> >+ [% IF passregex.search('letters') %] The indentation is incorrect. It should be <ul> [% IF ... %] Note that there is a missing </ul> at the end of the list. All these comments are minor and easy to fix. I'm going to fix them myself on checkin. Thank you for your patch! :) r=LpSolit

Here is the patch I'm going to check in. It simply fixes the comments made above.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/ modified Bugzilla/User.pm modified Bugzilla/Config/Auth.pm modified template/en/default/admin/params/auth.html.tmpl modified template/en/default/global/user-error.html.tmpl Committed revision 7650.

Added to relnotes in bug 713346.

Proposed Test case

Comment on attachment 8369509 [details] [diff] [review] TestCasev1.diff I improved your testcase a bit to decrease the execution time by a factor of 2 (42 seconds instead of 85 seconds on my machine) and refactored it a bit to avoid lots of duplicated code. I also added tests to make sure that valid passwords are accepted. Thanks for the testcase! :)

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/qa/4.4/ added t/test_password_complexity.t Committed revision 246. Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/qa/4.2/ added t/test_password_complexity.t Committed revision 231.