Bug 1391702 - Replace Bugzilla::User::validate_password() with calls to Data::Password::passwdqc

7 files changed, 108 insertions, 70 deletions

diff --git a/Bugzilla/Auth/Verify.pm b/Bugzilla/Auth/Verify.pm
index 19d8dcc9e..5895534cd 100644
--- a/Bugzilla/Auth/Verify.pm
+++ b/Bugzilla/Auth/Verify.pm
@@ -72,7 +72,7 @@ sub create_or_update_user {
               || return { failure => AUTH_ERROR,
                           error   => 'auth_invalid_email',
                           details => {addr => $username} };
-            # Usually we'd call validate_password, but external authentication
+            # external authentication
             # systems might follow different standards than ours. So in this
             # place here, we call trick_taint without checks.
             trick_taint($password);
diff --git a/Bugzilla/Auth/Verify/DB.pm b/Bugzilla/Auth/Verify/DB.pm
index ac6b71ac0..e46d1cd82 100644
--- a/Bugzilla/Auth/Verify/DB.pm
+++ b/Bugzilla/Auth/Verify/DB.pm
@@ -62,13 +62,15 @@ sub check_credentials {
     if (Bugzilla->usage_mode == USAGE_MODE_BROWSER &&
         Bugzilla->params->{password_check_on_login})
     {
-        my $check = validate_password_check($password);
-        if ($check) {
-            return {
-                failure => AUTH_ERROR,
-                user_error => $check,
-                details => { locked_user => $user }
-            }
+        my $pwqc = Bugzilla->passwdqc;
+        unless ($pwqc->validate_password($password)) {
+            my $reason = $pwqc->reason;
+            Bugzilla->audit(sprintf "%s logged in with a weak password (reason: %s)", $user->login, $reason);
+            $user->set_password_change_required(1);
+            $user->set_password_change_reason(
+                "You must change your password for the following reason: $reason"
+            );
+            $user->update();
         }
     }
 
diff --git a/Bugzilla/Config/Auth.pm b/Bugzilla/Config/Auth.pm
index dddedd819..58a3d3cd7 100644
--- a/Bugzilla/Config/Auth.pm
+++ b/Bugzilla/Config/Auth.pm
@@ -12,6 +12,8 @@ use strict;
 use warnings;
 
 use Bugzilla::Config::Common;
+use Types::Standard qw(Tuple Maybe);
+use Types::Common::Numeric qw(PositiveInt);
 
 our $sortkey = 300;
 
@@ -119,6 +121,42 @@ sub get_param_list {
             type    => 'b',
             default => '1'
         },
+
+        {
+            name    => 'passwdqc_min',
+            type    => 't',
+            default => 'undef, 24, 11, 8, 7',
+            checker => \&_check_passwdqc_min,
+        },
+
+        {
+            name    => 'passwdqc_max',
+            type    => 't',
+            default => '40',
+            checker => \&_check_passwdqc_max,
+        },
+
+        {
+            name    => 'passwdqc_passphrase_words',
+            type    => 't',
+            default => '3',
+            checker => \&check_numeric,
+        },
+
+        {
+            name    => 'passwdqc_match_length',
+            type    => 't',
+            default => '4',
+            checker => \&check_numeric,
+        },
+
+        {
+            name    => 'passwdqc_random_bits',
+            type    => 't',
+            default => '47',
+            checker => \&_check_passwdqc_random_bits,
+        },
+
         {
             name    => 'auth_delegation',
             type    => 'b',
@@ -149,4 +187,51 @@ sub get_param_list {
     return @param_list;
 }
 
+my $passwdqc_min = Tuple[
+    Maybe[PositiveInt],
+    Maybe[PositiveInt],
+    Maybe[PositiveInt],
+    Maybe[PositiveInt],
+    Maybe[PositiveInt],
+];
+
+sub _check_passwdqc_min {
+    my ($value) = @_;
+    my @values = map { $_ eq 'undef' ? undef : $_ } split( /\s*,\s*/, $value );
+
+    unless ( $passwdqc_min->check( \@values ) ) {
+        return "must be list of five values, that are either integers > 0 or undef";
+    }
+
+    my ( $max, $max_pos );
+    my $pos = 0;
+    foreach my $value (@values) {
+        if ( defined $max && defined $value ) {
+            if ( $value > $max ) {
+                return "Int$pos is larger than Int$max_pos ($max)";
+            }
+        }
+        elsif ( defined $value ) {
+            $max     = $value;
+            $max_pos = $pos;
+        }
+        $pos++;
+    }
+    return "";
+}
+
+sub _check_passwdqc_max {
+    my ($value) = @_;
+    return "must be a positive integer" unless PositiveInt->check($value);
+    return "must be greater than 8"     unless $value > 8;
+    return "";
+}
+
+sub _check_passwdqc_random_bits {
+    my ($value) = @_;
+    return "must be a positive integer" unless PositiveInt->check($value);
+    return "must be between 24 and 85 inclusive" unless $value >= 24 && $value <= 85;
+    return "";
+}
+
 1;
diff --git a/Bugzilla/Install.pm b/Bugzilla/Install.pm
index 3f8d4bdfb..ced559111 100644
--- a/Bugzilla/Install.pm
+++ b/Bugzilla/Install.pm
@@ -500,9 +500,14 @@ sub _prompt_for_password {
         print "\n", get_text('install_confirm_password'), ' ';
         my $pass2 = <STDIN>;
         chomp $pass2;
-        eval { validate_password($password, $pass2); };
-        if ($@) {
-            print "\n$@\n";
+        my $pwqc = Bugzilla->passwdqc;
+        my $ok = $pwqc->validate_password($password);
+        if (!$ok) {
+            print "\n", $pwqc->reason, "\n";
+            undef $password;
+        }
+        elsif ($password ne $pass2) {
+            print "\n", "passwords do not match\n";
             undef $password;
         }
         system("stty","echo") unless ON_WINDOWS;
diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm
index 84fc1fb21..2d8256080 100644
--- a/Bugzilla/User.pm
+++ b/Bugzilla/User.pm
@@ -34,7 +34,7 @@ use Role::Tiny::With;
 
 use base qw(Bugzilla::Object Exporter);
 @Bugzilla::User::EXPORT = qw(is_available_username
-    login_to_id user_id_to_login validate_password validate_password_check
+    login_to_id user_id_to_login 
     USER_MATCH_MULTIPLE USER_MATCH_FAILED USER_MATCH_SUCCESS
     MATCH_SKIP_CONFIRM
 );
@@ -417,7 +417,7 @@ sub _check_password {
     # authentication.
     return $pass if $pass eq '*';
 
-    validate_password($pass);
+    Bugzilla->assert_password_is_secure($pass);
     my $cryptpassword = bz_crypt($pass);
     return $cryptpassword;
 }
@@ -2712,40 +2712,6 @@ sub user_id_to_login {
     return $login || '';
 }
 
-sub validate_password {
-    my $check = validate_password_check(@_);
-    ThrowUserError($check) if $check;
-    return 1;
-}
-
-sub validate_password_check {
-    my ($password, $matchpassword) = @_;
-
-    if (length($password) < USER_PASSWORD_MIN_LENGTH) {
-        return 'password_too_short';
-    } elsif ((defined $matchpassword) && ($password ne $matchpassword)) {
-        return 'passwords_dont_match';
-    }
-
-    my $complexity_level = Bugzilla->params->{password_complexity};
-    if ($complexity_level eq 'bmo') {
-        my $features = 0;
-
-        $features++ if $password =~ /[a-z]/;
-        $features++ if $password =~ /[A-Z]/;
-        $features++ if $password =~ /[0-9]/;
-        $features++ if $password =~ /[^A-Za-z0-9]/;
-        $features++ if length($password) > 12;
-
-        return 'password_not_complex' if $features < 3;
-    }
-
-    # Having done these checks makes us consider the password untainted.
-    trick_taint($_[0]);
-    return;
-}
-
-
 1;
 
 __END__
@@ -3369,26 +3335,6 @@ Returns the login name of the user account for the given user ID. If no
 valid user ID is given or the user has no entry in the profiles table,
 we return an empty string.
 
-=item C<validate_password($passwd1, $passwd2)>
-
-Returns true if a password is valid (i.e. meets Bugzilla's
-requirements for length and content), else throws an error.
-Untaints C<$passwd1> if successful.
-
-If a second password is passed in, this function also verifies that
-the two passwords match.
-
-=item C<validate_password_check($passwd1, $passwd2)>
-
-This sub routine is similair to C<validate_password>, except that it allows
-the calling code to handle its own errors.
-
-Returns undef and untaints C<$passwd1> if a password is valid (i.e. meets
-Bugzilla's requirements for length and content), else returns the error.
-
-If a second password is passed in, this function also verifies that
-the two passwords match.
-
 =item C<match_field($data, $fields, $behavior)>
 
 =over
diff --git a/Bugzilla/Util.pm b/Bugzilla/Util.pm
index 3c4b2fb65..df2f78823 100644
--- a/Bugzilla/Util.pm
+++ b/Bugzilla/Util.pm
@@ -905,6 +905,7 @@ sub extract_nicks {
     return grep { defined $_ } @nicks;
 }
 
+
 1;
 
 __END__
diff --git a/Bugzilla/WebService/Constants.pm b/Bugzilla/WebService/Constants.pm
index 3f5042f42..93fddfc2b 100644
--- a/Bugzilla/WebService/Constants.pm
+++ b/Bugzilla/WebService/Constants.pm
@@ -143,8 +143,7 @@ use constant WS_ERROR_CODE => {
     auth_invalid_email           => 302,
     extern_id_conflict           => -303,
     auth_failure                 => 304,
-    password_too_short           => 305,
-    password_not_complex         => 305,
+    password_insecure            => 305,
     api_key_not_valid            => 306,
     api_key_revoked              => 306,
     auth_invalid_token           => 307,
@@ -159,7 +158,7 @@ use constant WS_ERROR_CODE => {
     auth_cant_create_account    => 501,
     account_creation_disabled   => 501,
     account_creation_restricted => 501,
-    password_too_short    => 502,
+    # Error 502 password_too_short no longer exists.
     # Error 503 password_too_long no longer exists.
     invalid_username      => 504,
     # This is from strict_isolation, but it also basically means