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

5 files changed, 101 insertions, 57 deletions

diff --git a/Bugzilla/Auth/Verify.pm b/Bugzilla/Auth/Verify.pm
index 952998caf..52cebb5ea 100644
--- a/Bugzilla/Auth/Verify.pm
+++ b/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);
-            $username_user_id = login_to_id($username);
+            # XXX Theoretically this could fail with an error, but the fix for
+            # 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
diff --git a/Bugzilla/DB/Schema.pm b/Bugzilla/DB/Schema.pm
index adac0c6d8..3967c8282 100644
--- a/Bugzilla/DB/Schema.pm
+++ b/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)'},
-            disabledtext   => {TYPE => 'MEDIUMTEXT', NOTNULL => 1},
+            realname       => {TYPE => 'varchar(255)', NOTNULL => 1,
+                               DEFAULT => "''"},
+            disabledtext   => {TYPE => 'MEDIUMTEXT', NOTNULL => 1,
+                               DEFAULT => "''"},
             disable_mail   => {TYPE => 'BOOLEAN', NOTNULL => 1,
                                DEFAULT => 'FALSE'},
             mybugslink     => {TYPE => 'BOOLEAN', NOTNULL => 1,
diff --git a/Bugzilla/Install/DB.pm b/Bugzilla/Install/DB.pm
index bf1fbcccf..06c46e1ec 100644
--- a/Bugzilla/Install/DB.pm
+++ b/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 #
     ################################################################
diff --git a/Bugzilla/Object.pm b/Bugzilla/Object.pm
index fa6c4e9cb..219658a92 100644
--- a/Bugzilla/Object.pm
+++ b/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>.
 
diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm
index 54d84020f..c037f317a 100644
--- a/Bugzilla/User.pm
+++ b/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 {
-    my ($username, $realname, $password, $disabledtext, $disable_mail) = (@_);
+sub create {
+    my $invocant = shift;
+    my $class = ref($invocant) || $invocant;
     my $dbh = Bugzilla->dbh;
 
-    $disabledtext ||= '';
-    $disable_mail ||= 0;
+    $dbh->bz_lock_tables('profiles WRITE', 'profiles_activity WRITE',
+        'user_group_map WRITE', 'email_setting WRITE', 'groups READ', 
+        'tokens READ', 'fielddefs READ');
 
-    # If not specified, generate a new random password for the user.
-    # 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));
+    my $user = $class->SUPER::create(@_);
 
     # 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>
-
-Creates a new user in the database.
-
-Params: $username (scalar, string) - The login name for the new user.
-        $realname (scalar, string) - The full name for the new user.
-        $password (scalar, string) - Optional. The password for the new user;
-                                     if not given, a random password will be
-                                     generated.
-        $disabledtext (scalar, string) - Optional. The disable text for the new
-                                         user; if not given, it will be empty.
-                                         If given, the user will be disabled,
-                                         meaning the account will be
-                                         unavailable for login.
-        $disable_mail (scalar, boolean) - Optional, defaults to 0.
-                                          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<create>
+
+The same as L<Bugzilla::Object/create>.
+
+Params: login_name - B<Required> The login name for the new user.
+        realname - The full name for the new user.
+        cryptpassword  - B<Required> The password for the new user.
+            Even though the name says "crypt", you should just specify
+            a plain-text password. If you specify '*', the user will not
+            be able to log in using DB authentication.
+        disabledtext - The disable-text for the new user. If given, the user 
+            will be disabled, meaning he cannot log in. Defaults to an
+            empty string.
+        disable_mail - 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.
 
 =item C<is_available_username>