Bug 332598: Move ValidatePassword() and DBNameToIdAndCheck() from globals.pl into User.pm - Patch by Frédéric Buclin <LpSolit@gmail.com> r=mkanat a=myk

12 files changed, 70 insertions, 58 deletions

diff --git a/Bugzilla/BugMail.pm b/Bugzilla/BugMail.pm
index d7be12a1a..3919c0ec6 100644
--- a/Bugzilla/BugMail.pm
+++ b/Bugzilla/BugMail.pm
@@ -178,16 +178,16 @@ sub ProcessOneBug {
     # At this point, we don't care if there are duplicates in these arrays.
     my $changer = $forced->{'changer'};
     if ($forced->{'owner'}) {
-        push (@assignees, &::DBNameToIdAndCheck($forced->{'owner'}));
+        push (@assignees, login_to_id($forced->{'owner'}, THROW_ERROR));
     }
     
     if ($forced->{'qacontact'}) {
-        push (@qa_contacts, &::DBNameToIdAndCheck($forced->{'qacontact'}));
+        push (@qa_contacts, login_to_id($forced->{'qacontact'}, THROW_ERROR));
     }
     
     if ($forced->{'cc'}) {
         foreach my $cc (@{$forced->{'cc'}}) {
-            push(@ccs, &::DBNameToIdAndCheck($cc));
+            push(@ccs, login_to_id($cc, THROW_ERROR));
         }
     }
     
diff --git a/Bugzilla/Constants.pm b/Bugzilla/Constants.pm
index 0b612cbba..8e245d0b6 100644
--- a/Bugzilla/Constants.pm
+++ b/Bugzilla/Constants.pm
@@ -44,6 +44,9 @@ use base qw(Exporter);
     AUTH_LOGINFAILED
     AUTH_DISABLED
 
+    USER_PASSWORD_MIN_LENGTH
+    USER_PASSWORD_MAX_LENGTH
+
     LOGIN_OPTIONAL
     LOGIN_NORMAL
     LOGIN_REQUIRED
@@ -71,6 +74,7 @@ use base qw(Exporter);
     COMMENT_COLS
 
     UNLOCK_ABORT
+    THROW_ERROR
     
     RELATIONSHIPS
     REL_ASSIGNEE REL_QA REL_REPORTER REL_CC REL_VOTER 
@@ -141,6 +145,10 @@ use constant AUTH_ERROR => 2;
 use constant AUTH_LOGINFAILED => 3;
 use constant AUTH_DISABLED => 4;
 
+# The minimum and maximum lengths a password must have.
+use constant USER_PASSWORD_MIN_LENGTH => 3;
+use constant USER_PASSWORD_MAX_LENGTH => 16;
+
 use constant LOGIN_OPTIONAL => 0;
 use constant LOGIN_NORMAL => 1;
 use constant LOGIN_REQUIRED => 2;
@@ -192,6 +200,10 @@ use constant COMMENT_COLS => 80;
 # because of error
 use constant UNLOCK_ABORT => 1;
 
+# Determine whether a validation routine should return 0 or throw
+# an error when the validation fails.
+use constant THROW_ERROR => 1;
+
 use constant REL_ASSIGNEE           => 0;
 use constant REL_QA                 => 1;
 use constant REL_REPORTER           => 2;
diff --git a/Bugzilla/Search.pm b/Bugzilla/Search.pm
index 960ff336d..352147331 100644
--- a/Bugzilla/Search.pm
+++ b/Bugzilla/Search.pm
@@ -239,7 +239,7 @@ sub init {
             foreach my $name (split(',', $email)) {
                 $name = trim($name);
                 if ($name) {
-                    &::DBNameToIdAndCheck($name);
+                    login_to_id($name, THROW_ERROR);
                 }
             }
         }
@@ -550,7 +550,7 @@ sub init {
              my $table = "longdescs_$chartid";
              push(@supptables, "INNER JOIN longdescs AS $table " .
                                "ON $table.bug_id = bugs.bug_id");
-             my $id = &::DBNameToIdAndCheck($v);
+             my $id = login_to_id($v, THROW_ERROR);
              $term = "$table.who = $id";
          },
          "^long_?desc,changedbefore" => sub {
@@ -691,7 +691,7 @@ sub init {
              my $table = "longdescs_$chartid";
              push(@supptables, "INNER JOIN longdescs AS $table " .
                                "ON $table.bug_id = bugs.bug_id");
-             my $id = &::DBNameToIdAndCheck($v);
+             my $id = login_to_id($v, THROW_ERROR);
              $term = "(($table.who = $id";
              $term .= ") AND ($table.work_time <> 0))";
          },
@@ -805,7 +805,7 @@ sub init {
              $f =~ m/^attachments\.(.*)$/;
              my $field = $1;
              if ($t eq "changedby") {
-                 $v = &::DBNameToIdAndCheck($v);
+                 $v = login_to_id($v, THROW_ERROR);
                  $q = &::SqlQuote($v);
                  $field = "submitter_id";
                  $t = "equals";
@@ -1126,7 +1126,7 @@ sub init {
              if (!$fieldid) {
                  ThrowCodeError("invalid_field_name", {field => $f});
              }
-             my $id = &::DBNameToIdAndCheck($v);
+             my $id = login_to_id($v, THROW_ERROR);
              push(@supptables, "LEFT JOIN bugs_activity AS $table " .
                                "ON $table.bug_id = bugs.bug_id " .
                                "AND $table.fieldid = $fieldid " .
diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm
index 3ce346812..4fb41d852 100644
--- a/Bugzilla/User.pm
+++ b/Bugzilla/User.pm
@@ -48,7 +48,7 @@ use Bugzilla::Classification;
 
 use base qw(Exporter);
 @Bugzilla::User::EXPORT = qw(insert_new_user is_available_username
-    login_to_id
+    login_to_id validate_password
     UserInGroup
     USER_MATCH_MULTIPLE USER_MATCH_FAILED USER_MATCH_SUCCESS
     MATCH_SKIP_CONFIRM
@@ -1360,7 +1360,7 @@ sub is_available_username {
 }
 
 sub login_to_id {
-    my ($login) = (@_);
+    my ($login, $throw_error) = @_;
     my $dbh = Bugzilla->dbh;
     # $login will only be used by the following SELECT statement, so it's safe.
     trick_taint($login);
@@ -1369,11 +1369,26 @@ sub login_to_id {
                                         undef, $login);
     if ($user_id) {
         return $user_id;
+    } elsif ($throw_error) {
+        ThrowUserError('invalid_username', { name => $login });
     } else {
         return 0;
     }
 }
 
+sub validate_password {
+    my ($password, $matchpassword) = @_;
+
+    if (length($password) < USER_PASSWORD_MIN_LENGTH) {
+        ThrowUserError('password_too_short');
+    } elsif (length($password) > USER_PASSWORD_MAX_LENGTH) {
+        ThrowUserError('password_too_long');
+    } elsif ((defined $matchpassword) && ($password ne $matchpassword)) {
+        ThrowUserError('passwords_dont_match');
+    }
+    return 1;
+}
+
 sub UserInGroup {
     return exists Bugzilla->user->groups->{$_[0]} ? 1 : 0;
 }
@@ -1774,13 +1789,15 @@ Params: $username (scalar, string) - The full login name of the username
             can change his username to $username. (That is, this function
             will return a boolean true value).
 
-=item C<login_to_id($login)>
+=item C<login_to_id($login, $throw_error)>
 
 Takes a login name of a Bugzilla user and changes that into a numeric
 ID for that user. This ID can then be passed to Bugzilla::User::new to
 create a new user.
 
-If no valid user exists with that login name, then the function will return 0.
+If no valid user exists with that login name, then the function returns 0.
+However, if $throw_error is set, the function will throw a user error
+instead of returning.
 
 This function can also be used when you want to just find out the userid
 of a user, but you don't want the full weight of Bugzilla::User.
@@ -1788,6 +1805,14 @@ of a user, but you don't want the full weight of Bugzilla::User.
 However, consider using a Bugzilla::User object instead of this function
 if you need more information about the user than just their ID.
 
+=item C<validate_password($passwd1, $passwd2)>
+
+Returns true if a password is valid (i.e. meets Bugzilla's
+requirements for length and content), else returns false.
+
+If a second password is passed in, this function also verifies that
+the two passwords match.
+
 =item C<UserInGroup($groupname)>
 
 Takes a name of a group, and returns 1 if a user is in the group, 0 otherwise.
diff --git a/editusers.cgi b/editusers.cgi
index c5e67e28b..df31c8a4f 100755
--- a/editusers.cgi
+++ b/editusers.cgi
@@ -209,7 +209,7 @@ if ($action eq 'search') {
       || ThrowUserError('illegal_email_address', {addr => $login});
     is_available_username($login)
       || ThrowUserError('account_exists', {email => $login});
-    ValidatePassword($password);
+    validate_password($password);
 
     # Login and password are validated now, and realname and disabledtext
     # are allowed to contain anything
@@ -296,7 +296,7 @@ if ($action eq 'search') {
         }
         if ($password) {
             # Validate, then trick_taint.
-            ValidatePassword($password) if $password;
+            validate_password($password) if $password;
             trick_taint($password);
             push(@changedFields, 'cryptpassword');
             push(@values, bz_crypt($password));
diff --git a/globals.pl b/globals.pl
index ac95974d2..ad6cfd761 100644
--- a/globals.pl
+++ b/globals.pl
@@ -204,22 +204,6 @@ sub AnyDefaultGroups {
     return $::CachedAnyDefaultGroups;
 }
 
-sub ValidatePassword {
-    # Determines whether or not a password is valid (i.e. meets Bugzilla's
-    # requirements for length and content).    
-    # If a second password is passed in, this function also verifies that
-    # the two passwords match.
-    my ($password, $matchpassword) = @_;
-    
-    if (length($password) < 3) {
-        ThrowUserError("password_too_short");
-    } elsif (length($password) > 16) {
-        ThrowUserError("password_too_long");
-    } elsif ((defined $matchpassword) && ($password ne $matchpassword)) {
-        ThrowUserError("passwords_dont_match");
-    }
-}
-
 sub DBID_to_name {
     my ($id) = (@_);
     return "__UNKNOWN__" if !defined $id;
@@ -242,16 +226,6 @@ sub DBID_to_name {
     return $::cachedNameArray{$id};
 }
 
-sub DBNameToIdAndCheck {
-    my ($name) = (@_);
-    my $result = login_to_id($name);
-    if ($result > 0) {
-        return $result;
-    }
-
-    ThrowUserError("invalid_username", { name => $name });
-}
-
 sub get_product_id {
     my ($prod) = @_;
     PushGlobalSQLState();
diff --git a/post_bug.cgi b/post_bug.cgi
index 14763151a..273117c6b 100755
--- a/post_bug.cgi
+++ b/post_bug.cgi
@@ -155,7 +155,7 @@ if (!UserInGroup("editbugs") || $cgi->param('assigned_to') eq "") {
     $cgi->param(-name => 'assigned_to', -value => $initialowner);
 } else {
     $cgi->param(-name => 'assigned_to',
-                -value => DBNameToIdAndCheck(trim($cgi->param('assigned_to'))));
+                -value => login_to_id(trim($cgi->param('assigned_to')), THROW_ERROR));
 }
 
 my @bug_fields = ("version", "rep_platform",
@@ -182,7 +182,7 @@ if (Param("useqacontact")) {
                                                  WHERE id = ?},
                                                 undef, $component_id);
     } else {
-        $qa_contact = DBNameToIdAndCheck(trim($cgi->param('qa_contact')));
+        $qa_contact = login_to_id(trim($cgi->param('qa_contact')), THROW_ERROR);
     }
 
     if ($qa_contact) {
@@ -267,7 +267,7 @@ my %ccids;
 if (defined $cgi->param('cc')) {
     foreach my $person ($cgi->param('cc')) {
         next unless $person;
-        my $ccid = DBNameToIdAndCheck($person);
+        my $ccid = login_to_id($person, THROW_ERROR);
         if ($ccid && !$ccids{$ccid}) {
            $ccids{$ccid} = 1;
         }
diff --git a/process_bug.cgi b/process_bug.cgi
index 8dfa4018d..9ef459bec 100755
--- a/process_bug.cgi
+++ b/process_bug.cgi
@@ -1050,7 +1050,7 @@ if (defined $cgi->param('newcc')
     if ($cc_add) {
         $cc_add =~ s/[\s,]+/ /g; # Change all delimiters to a single space
         foreach my $person ( split(" ", $cc_add) ) {
-            my $pid = DBNameToIdAndCheck($person);
+            my $pid = login_to_id($person, THROW_ERROR);
             $cc_add{$pid} = $person;
         }
     }
@@ -1060,7 +1060,7 @@ if (defined $cgi->param('newcc')
     if ($cc_remove) {
         $cc_remove =~ s/[\s,]+/ /g; # Change all delimiters to a single space
         foreach my $person ( split(" ", $cc_remove) ) {
-            my $pid = DBNameToIdAndCheck($person);
+            my $pid = login_to_id($person, THROW_ERROR);
             $cc_remove{$pid} = $person;
         }
     }
@@ -1087,7 +1087,7 @@ if (defined $cgi->param('qa_contact')
     my $name = trim($cgi->param('qa_contact'));
     # The QA contact cannot be deleted from show_bug.cgi for a single bug!
     if ($name ne $cgi->param('dontchange')) {
-        $qacontact = DBNameToIdAndCheck($name) if ($name ne "");
+        $qacontact = login_to_id($name, THROW_ERROR) if ($name ne "");
         if ($qacontact && Param("strict_isolation")) {
                 $usercache{$qacontact} ||= Bugzilla::User->new($qacontact);
                 my $qa_user = $usercache{$qacontact};
@@ -1172,7 +1172,7 @@ SWITCH: for ($cgi->param('knob')) {
         DoComma();
         if (defined $cgi->param('assigned_to')
             && trim($cgi->param('assigned_to')) ne "") { 
-            $assignee = DBNameToIdAndCheck(trim($cgi->param('assigned_to')));
+            $assignee = login_to_id(trim($cgi->param('assigned_to')), THROW_ERROR);
             if (Param("strict_isolation")) {
                 $usercache{$assignee} ||= Bugzilla::User->new($assignee);
                 my $assign_user = $usercache{$assignee};
diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl
index db3bb682f..1af63b179 100644
--- a/template/en/default/global/user-error.html.tmpl
+++ b/template/en/default/global/user-error.html.tmpl
@@ -1022,13 +1022,13 @@
 
   [% ELSIF error == "password_too_long" %]
     [% title = "Password Too Long" %]
-    The password is more than 16 characters long. It must be no more than
-    16 characters.
+    The password must be no more than
+    [%+ constants.USER_PASSWORD_MAX_LENGTH FILTER html %] characters long.
 
   [% ELSIF error == "password_too_short" %]
     [% title = "Password Too Short" %]
-    The password is less than three characters long. It must be at least
-    three characters.
+    The password must be at least
+    [%+ constants.USER_PASSWORD_MIN_LENGTH FILTER html %] characters long.
 
   [% ELSIF error == "patch_too_large" %]
     [% title = "File Too Large" %]
diff --git a/token.cgi b/token.cgi
index aaac4f7ac..cbb502c67 100755
--- a/token.cgi
+++ b/token.cgi
@@ -68,7 +68,7 @@ if ($cgi->param('t')) {
   
   # Make sure the token contains only valid characters in the right amount.
   # Validate password will throw an error if token is invalid
-  ValidatePassword($::token);
+  validate_password($::token);
   trick_taint($::token); # Only used in placeholders
 
   Bugzilla::Token::CleanTokenTable();
@@ -128,7 +128,7 @@ if ( $::action eq 'chgpw' ) {
       && defined $cgi->param('matchpassword')
       || ThrowUserError("require_new_password");
 
-    ValidatePassword($cgi->param('password'), $cgi->param('matchpassword'));
+    validate_password($cgi->param('password'), $cgi->param('matchpassword'));
 }
 
 ################################################################################
diff --git a/userprefs.cgi b/userprefs.cgi
index 3dc68121e..7eb78af42 100755
--- a/userprefs.cgi
+++ b/userprefs.cgi
@@ -96,7 +96,7 @@ sub SaveAccount {
         {
             $cgi->param('new_password1')
               || ThrowUserError("new_password_missing");
-            ValidatePassword($pwd1, $pwd2);
+            validate_password($pwd1, $pwd2);
 
             if ($cgi->param('Bugzilla_password') ne $pwd1) {
                 my $cryptedpassword = bz_crypt($pwd1);
@@ -313,7 +313,7 @@ sub SaveEmail {
         my @new_watch_names = split(/[,\s]+/, $cgi->param('watchedusers'));
         my %new_watch_ids;
         foreach my $username (@new_watch_names) {
-            my $watched_userid = DBNameToIdAndCheck(trim($username));
+            my $watched_userid = login_to_id(trim($username), THROW_ERROR);
             $new_watch_ids{$watched_userid} = 1;
         }
         my ($removed, $added) = diff_arrays($old_watch_ids, [keys %new_watch_ids]);
diff --git a/votes.cgi b/votes.cgi
index e1bfb3616..61154a069 100755
--- a/votes.cgi
+++ b/votes.cgi
@@ -29,6 +29,7 @@ use lib ".";
 use Bugzilla;
 use Bugzilla::Constants;
 use Bugzilla::Bug;
+use Bugzilla::User;
 
 require "globals.pl";
 
@@ -117,11 +118,11 @@ sub show_user {
 
     # If a bug_id is given, and we're editing, we'll add it to the votes list.
     $bug_id ||= "";
-    
+
     my $name = $cgi->param('user') || $user->login;
-    my $who = DBNameToIdAndCheck($name);
+    my $who = login_to_id($name, THROW_ERROR);
     my $userid = $user->id;
-    
+
     my $canedit = (Param('usevotes') && $userid == $who) ? 1 : 0;
 
     $dbh->bz_lock_tables('bugs READ', 'products READ', 'votes WRITE',