Bug 349349: Use ->create from Bugzilla::Object instead of insert_new_user for Bugzilla::User

Patch By Max Kanat-Alexander <mkanat@bugzilla.org> r=LpSolit, a=myk

webtools/bugzilla/Bugzilla/Auth/Verify.pm

@@ -77,8 +77,13 @@ sub create_or_update_user {
               || return { failure => AUTH_ERROR, 
                           error   => 'auth_invalid_email',
                           details => {addr => $username} };
-            insert_new_user($username, $real_name, $password);
+            # XXX Theoretically this could fail with an error, but the fix for
-            $username_user_id = login_to_id($username);
+            # that is too involved to be done right now.
+            my $user = Bugzilla::User->create({ 
+                login_name    => $username, 
+                cryptpassword => $password,
+                realname      => $real_name});
+            $username_user_id = $user->id;
         }
 
         # If we have a valid username id and an extern_id, but no valid

webtools/bugzilla/Bugzilla/DB/Schema.pm

@@ -611,8 +611,10 @@ use constant ABSTRACT_SCHEMA => {
                                PRIMARYKEY => 1},
             login_name     => {TYPE => 'varchar(255)', NOTNULL => 1},
             cryptpassword  => {TYPE => 'varchar(128)'},
-            realname       => {TYPE => 'varchar(255)'},
+            realname       => {TYPE => 'varchar(255)', NOTNULL => 1,
-            disabledtext   => {TYPE => 'MEDIUMTEXT', NOTNULL => 1},
+                               DEFAULT => "''"},
+            disabledtext   => {TYPE => 'MEDIUMTEXT', NOTNULL => 1,
+                               DEFAULT => "''"},
             disable_mail   => {TYPE => 'BOOLEAN', NOTNULL => 1,
                                DEFAULT => 'FALSE'},
             mybugslink     => {TYPE => 'BOOLEAN', NOTNULL => 1,

webtools/bugzilla/Bugzilla/Install/DB.pm

@@ -485,6 +485,12 @@ sub update_table_definitions {
         $dbh->bz_add_index('bugs', 'bugs_short_desc_idx', [qw(short_desc)]);
     }
 
+    # The profiles table was missing some defaults.
+    $dbh->bz_alter_column('profiles', 'disabledtext',
+        {TYPE => 'MEDIUMTEXT', NOTNULL => 1, DEFAULT => "''"});
+    $dbh->bz_alter_column('profiles', 'realname',
+        {TYPE => 'varchar(255)', NOTNULL => 1, DEFAULT => "''"});
+
     ################################################################
     # New --TABLE-- changes should go *** A B O V E *** this point #
     ################################################################

webtools/bugzilla/Bugzilla/Object.pm

@@ -153,7 +153,7 @@ sub create {
     chop($qmarks);
     $dbh->do("INSERT INTO $table (" . join(', ', @field_names) 
              . ") VALUES ($qmarks)", undef, @values);
-    my $id = $dbh->bz_last_key($table, 'id');
+    my $id = $dbh->bz_last_key($table, $class->ID_FIELD);
 
     return $class->new($id);
 }
@@ -303,7 +303,7 @@ Params:      C<$params> - hashref - A value to put in each database
 Returns:     The Object just created in the database.
 
 Notes:       In order for this function to work in your subclass,
-             your subclass's C<id> field must be of C<SERIAL>
+             your subclass's L</ID_FIELD> must be of C<SERIAL>
              type in the database. Your subclass also must
              define L</REQUIRED_CREATE_FIELDS> and L</VALIDATORS>.
 

webtools/bugzilla/Bugzilla/User.pm

@@ -49,7 +49,7 @@ use Bugzilla::Classification;
 use Bugzilla::Field;
 
 use base qw(Bugzilla::Object Exporter);
-@Bugzilla::User::EXPORT = qw(insert_new_user is_available_username
+@Bugzilla::User::EXPORT = qw(is_available_username
     login_to_id user_id_to_login validate_password
     UserInGroup
     USER_MATCH_MULTIPLE USER_MATCH_FAILED USER_MATCH_SUCCESS
@@ -92,6 +92,16 @@ use constant DB_COLUMNS => (
 use constant NAME_FIELD => 'login_name';
 use constant ID_FIELD   => 'userid';
 
+use constant REQUIRED_CREATE_FIELDS => qw(login_name cryptpassword);
+
+use constant VALIDATORS => {
+    cryptpassword => \&_check_password,
+    disable_mail  => \&_check_disable_mail,
+    disabledtext  => \&_check_disabledtext,
+    login_name    => \&check_login_name_for_creation,
+    realname      => \&_check_realname,
+};
+
 ################################################################################
 # Functions
 ################################################################################
@@ -108,6 +118,45 @@ sub new {
     return $class->SUPER::new(@_);
 }
 
+################################################################################
+# Validators
+################################################################################
+
+sub _check_disable_mail { return $_[0] ? 1 : 0; }
+sub _check_disabledtext { return trim($_[0]) || ''; }
+
+# This is public since createaccount.cgi needs to use it before issuing
+# a token for account creation.
+sub check_login_name_for_creation {
+    my ($name) = @_;
+    $name = trim($name);
+    $name || ThrowUserError('user_login_required');
+    validate_email_syntax($name)
+        || ThrowUserError('illegal_email_address', { addr => $name });
+    is_available_username($name) 
+        || ThrowUserError('account_exists', { email => $name });
+    return $name;
+}
+
+sub _check_password {
+    my ($pass) = @_;
+
+    # If the password is '*', do not encrypt it or validate it further--we 
+    # are creating a user who should not be able to log in using DB 
+    # authentication.
+    return $pass if $pass eq '*';
+
+    validate_password($pass);
+    my $cryptpassword = bz_crypt($pass);
+    return $cryptpassword;
+}
+
+sub _check_realname { return trim($_[0]) || ''; }
+
+################################################################################
+# Methods
+################################################################################
+
 # Accessors for user attributes
 sub login { $_[0]->{login}; }
 sub email { $_[0]->{login} . Bugzilla->params->{'emailsuffix'}; }
@@ -1292,36 +1341,18 @@ sub get_userlist {
     return $self->{'userlist'};
 }
 
-sub insert_new_user {
+sub create {
-    my ($username, $realname, $password, $disabledtext, $disable_mail) = (@_);
+    my $invocant = shift;
+    my $class = ref($invocant) || $invocant;
     my $dbh = Bugzilla->dbh;
 
-    $disabledtext ||= '';
+    $dbh->bz_lock_tables('profiles WRITE', 'profiles_activity WRITE',
-    $disable_mail ||= 0;
+        'user_group_map WRITE', 'email_setting WRITE', 'groups READ', 
+        'tokens READ', 'fielddefs READ');
 
-    # If not specified, generate a new random password for the user.
+    my $user = $class->SUPER::create(@_);
-    # If the password is '*', do not encrypt it; we are creating a user
-    # based on the ENV auth method.
-    $password ||= generate_random_password();
-    my $cryptpassword = ($password ne '*') ? bz_crypt($password) : $password;
-
-    # XXX - These should be moved into is_available_username or validate_email_syntax
-    #       At the least, they shouldn't be here. They're safe for now, though.
-    trick_taint($username);
-    trick_taint($realname);
-
-    # Insert the new user record into the database.
-    $dbh->do("INSERT INTO profiles 
-                          (login_name, realname, cryptpassword, disabledtext,
-                           disable_mail) 
-                   VALUES (?, ?, ?, ?, ?)",
-             undef, 
-             ($username, $realname, $cryptpassword, $disabledtext, 
-              $disable_mail));
 
     # Turn on all email for the new user
-    my $new_userid = $dbh->bz_last_key('profiles', 'userid');
-
     foreach my $rel (RELATIONSHIPS) {
         foreach my $event (POS_EVENTS, NEG_EVENTS) {
             # These "exceptions" define the default email preferences.
@@ -1332,16 +1363,15 @@ sub insert_new_user {
             next if (($event == EVT_CC) && ($rel != REL_REPORTER));
 
             $dbh->do('INSERT INTO email_setting (user_id, relationship, event)
-                      VALUES (?, ?, ?)', undef, ($new_userid, $rel, $event));
+                      VALUES (?, ?, ?)', undef, ($user->id, $rel, $event));
         }
     }
 
     foreach my $event (GLOBAL_EVENTS) {
         $dbh->do('INSERT INTO email_setting (user_id, relationship, event)
-                  VALUES (?, ?, ?)', undef, ($new_userid, REL_ANY, $event));
+                  VALUES (?, ?, ?)', undef, ($user->id, REL_ANY, $event));
     }
 
-    my $user = new Bugzilla::User($new_userid);
     $user->derive_regexp_groups();
 
     # Add the creation date to the profiles_activity table.
@@ -1355,6 +1385,8 @@ sub insert_new_user {
                    VALUES (?, ?, NOW(), ?, NOW())',
                    undef, ($user->id, $who, $creation_date_fieldid));
 
+    $dbh->bz_unlock_tables();
+
     # Return the newly created user account.
     return $user;
 }
@@ -1461,7 +1493,12 @@ Bugzilla::User - Object for a Bugzilla user
       $user->get_selectable_classifications;
 
   # Class Functions
-  $user = insert_new_user($username, $realname, $password, $disabledtext);
+  $user = Bugzilla::User->create({ 
+      login_name    => $username, 
+      realname      => $realname, 
+      cryptpassword => $plaintext_password, 
+      disabledtext  => $disabledtext,
+      disable_mail  => 0});
 
 =head1 DESCRIPTION
 
@@ -1797,27 +1834,21 @@ called "statically," just like a normal procedural function.
 
 =over 4
 
-=item C<insert_new_user>
+=item C<create>
 
-Creates a new user in the database.
+The same as L<Bugzilla::Object/create>.
 
-Params: $username (scalar, string) - The login name for the new user.
+Params: login_name - B<Required> The login name for the new user.
-        $realname (scalar, string) - The full name for the new user.
+        realname - The full name for the new user.
-        $password (scalar, string) - Optional. The password for the new user;
+        cryptpassword  - B<Required> The password for the new user.
-                                     if not given, a random password will be
+            Even though the name says "crypt", you should just specify
-                                     generated.
+            a plain-text password. If you specify '*', the user will not
-        $disabledtext (scalar, string) - Optional. The disable text for the new
+            be able to log in using DB authentication.
-                                         user; if not given, it will be empty.
+        disabledtext - The disable-text for the new user. If given, the user 
-                                         If given, the user will be disabled,
+            will be disabled, meaning he cannot log in. Defaults to an
-                                         meaning the account will be
+            empty string.
-                                         unavailable for login.
+        disable_mail - If 1, bug-related mail will not be  sent to this user; 
-        $disable_mail (scalar, boolean) - Optional, defaults to 0.
+            if 0, mail will be sent depending on the user's  email preferences.
-                                          If 1, bug-related mail will not be 
-                                          sent to this user; if 0, mail will
-                                          be sent depending on the user's 
-                                          email preferences.
-
-Returns: The Bugzilla::User object representing the new user account.
 
 =item C<is_available_username>
 

webtools/bugzilla/checksetup.pl

@@ -326,7 +326,6 @@ import Bugzilla::Util qw(bz_crypt trim html_quote is_7bit_clean
                          clean_text url_quote);
 
 require Bugzilla::User;
-import Bugzilla::User qw(insert_new_user);
 
 require Bugzilla::Bug;
 import Bugzilla::Bug qw(is_open_state);
@@ -756,7 +755,10 @@ if ($sth->rows == 0) {
         $SIG{QUIT} = 'DEFAULT';
         $SIG{TERM} = 'DEFAULT';
 
-        insert_new_user($login, $realname, $pass1);
+        Bugzilla::User->create({
+            login_name => $login, 
+            realname   => $realname, 
+            cryptpassword => $pass1});
     }
 
     # Put the admin in each group if not already    

webtools/bugzilla/contrib/syncLDAP.pl

@@ -273,7 +273,10 @@ if($readonly == 0) {
    print "Phase 3: creating new users... " unless $quiet;
    if($nocreate == 0) {
       while( my ($key, $value) = each(%create_users) ) {
-        insert_new_user($key, @$value{'realname'});
+        Bugzilla::User->create({
+            login_name => $key, 
+            realname   => @$value{'realname'},
+            password   => '*'});
       }
       print "done!\n" unless $quiet;
    }

webtools/bugzilla/createaccount.cgi

@@ -60,18 +60,9 @@ unless ($createexp) {
 my $login = $cgi->param('login');
 
 if (defined($login)) {
-    validate_email_syntax($login)
+    $login = Bugzilla::User::check_login_name_for_creation($login);
-      || ThrowUserError('illegal_email_address', {addr => $login});
-
     $vars->{'login'} = $login;
 
-    if (!is_available_username($login)) {
-        # Account already exists
-        $template->process("account/exists.html.tmpl", $vars)
-          || ThrowTemplateError($template->error());
-        exit;
-    }
-
     if ($login !~ /$createexp/) {
         ThrowUserError("account_creation_disabled");
     }

webtools/bugzilla/editusers.cgi

@@ -191,36 +191,14 @@ if ($action eq 'search') {
                                                   action => "add",
                                                   object => "users"});
 
-    my $login        = $cgi->param('login');
+    my $new_user = Bugzilla::User->create({
-    my $password     = $cgi->param('password');
+        login_name    => $cgi->param('login'),
-    my $realname     = trim($cgi->param('name')         || '');
+        cryptpassword => $cgi->param('password'),
-    my $disabledtext = trim($cgi->param('disabledtext') || '');
+        realname      => $cgi->param('name'),
-    my $disable_mail = $cgi->param('disable_mail') ? 1 : 0;
+        disabledtext  => $cgi->param('disabledtext'),
+        disable_mail  => $cgi->param('disable_mail')});
 
-    # Lock tables during the check+creation session.
+    userDataToVars($new_user->id);
-    $dbh->bz_lock_tables('profiles WRITE', 'profiles_activity WRITE',
-                         'email_setting WRITE', 'user_group_map WRITE',
-                         'groups READ', 'tokens READ', 'fielddefs READ');
-
-    # Validity checks
-    $login || ThrowUserError('user_login_required');
-    validate_email_syntax($login)
-      || ThrowUserError('illegal_email_address', {addr => $login});
-    is_available_username($login)
-      || ThrowUserError('account_exists', {email => $login});
-    validate_password($password);
-
-    # Login and password are validated now, and realname and disabledtext
-    # are allowed to contain anything
-    trick_taint($login);
-    trick_taint($realname);
-    trick_taint($password);
-    trick_taint($disabledtext);
-
-    insert_new_user($login, $realname, $password, $disabledtext, $disable_mail);
-    my $new_user_id = $dbh->bz_last_key('profiles', 'userid');
-    $dbh->bz_unlock_tables();
-    userDataToVars($new_user_id);
 
     $vars->{'message'} = 'account_created';
     $template->process('admin/users/edit.html.tmpl', $vars)

webtools/bugzilla/token.cgi

@@ -369,31 +369,13 @@ sub request_create_account {
 sub confirm_create_account {
     my (undef, undef, $login_name) = Bugzilla::Token::GetTokenData($::token);
 
-    (defined $cgi->param('passwd1') && defined $cgi->param('passwd2'))
+    validate_password($cgi->param('passwd1') || '', 
-      || ThrowUserError('new_password_missing');
+                      $cgi->param('passwd2') || '');
-    validate_password($cgi->param('passwd1'), $cgi->param('passwd2'));
 
-    my $realname = $cgi->param('realname');
+    my $otheruser = Bugzilla::User->create({
-    my $password = $cgi->param('passwd1');
+        login_name => $login_name, 
-
+        realname   => $cgi->param('realname'), 
-    $dbh->bz_lock_tables('profiles WRITE', 'profiles_activity WRITE',
+        cryptpassword => $cgi->param('passwd1')});
-                         'email_setting WRITE', 'user_group_map WRITE',
-                         'groups READ', 'tokens READ', 'fielddefs READ');
-
-    # The email syntax may have changed since the initial creation request.
-    validate_email_syntax($login_name)
-      || ThrowUserError('illegal_email_address', {addr => $login_name});
-    # Also, maybe that this user account has already been created meanwhile.
-    is_available_username($login_name)
-      || ThrowUserError('account_exists', {email => $login_name});
-
-    # Login and password are validated now, and realname is allowed to
-    # contain anything.
-    trick_taint($realname);
-    trick_taint($password);
-
-    my $otheruser = insert_new_user($login_name, $realname, $password);
-    $dbh->bz_unlock_tables();
 
     # Now delete this token.
     Bugzilla::Token::DeleteToken($::token);