Consolidate user editing code

Summary:
  - We currently have some bugs in account creation due to nontransactional user/email editing.
    - We save $user, then try to save $email. This may fail for various reasons, commonly because the email isn't unique.
    - This leaves us with a $user with no email.
  - Also, logging of edits is somewhat inconsistent across various edit mechanisms.
  - Move all editing to a `PhabricatorUserEditor` class.
  - Handle some broken-data cases more gracefully.

Test Plan:
  - Created and edited a user with `accountadmin`.
  - Created a user with `add_user.php`
  - Created and edited a user with People editor.
  - Created a user with OAuth.
  - Edited user information via Settings.
  - Tried to create an OAuth user with a duplicate email address, got a proper error.
  - Tried to create a user via People with a duplicate email address, got a proper error.

Reviewers: btrahan, vrana, jungejason

Reviewed By: btrahan

CC: tberman, aran

Maniphest Tasks: T1184

Differential Revision: https://secure.phabricator.com/D2569

Author: epriestley

scripts/user/account_admin.php

@@ -121,7 +121,6 @@
 $set_admin = phutil_console_confirm(
   'Should this user be an administrator?',
   $default_no = !$is_admin);
-$user->setIsAdmin($set_admin);
 
 echo "\n\nACCOUNT SUMMARY\n\n";
 $tpl = "%12s   %-30s   %-30s\n";
@@ -149,21 +148,30 @@
   exit(1);
 }
 
-$user->save();
-if ($changed_pass !== false) {
-  // This must happen after saving the user because we use their PHID as a
-  // component of the password hash.
-  $user->setPassword($changed_pass);
-  $user->save();
-}
+$user->openTransaction();
 
-if ($new_email) {
-  id(new PhabricatorUserEmail())
-    ->setUserPHID($user->getPHID())
-    ->setAddress($new_email)
-    ->setIsVerified(1)
-    ->setIsPrimary(1)
-    ->save();
-}
+  $editor = new PhabricatorUserEditor();
+
+  // TODO: This is wrong, but we have a chicken-and-egg problem when you use
+  // this script to create the first user.
+  $editor->setActor($user);
+
+  if ($new_email) {
+    $email = id(new PhabricatorUserEmail())
+      ->setAddress($new_email)
+      ->setIsVerified(1);
+
+    $editor->createNewUser($user, $email);
+  } else {
+    $editor->updateUser($user);
+  }
+
+  $editor->makeAdminUser($user, $set_admin);
+
+  if ($changed_pass !== false) {
+    $editor->changePassword($user, $changed_pass);
+  }
+
+$user->saveTransaction();
 
 echo "Saved changes.\n";

scripts/user/add_user.php

@@ -61,14 +61,14 @@
 $user = new PhabricatorUser();
 $user->setUsername($username);
 $user->setRealname($realname);
-$user->save();
 
 $email_object = id(new PhabricatorUserEmail())
-  ->setUserPHID($user->getPHID())
   ->setAddress($email)
-  ->setIsVerified(1)
-  ->setIsPrimary(1)
-  ->save();
+  ->setIsVerified(1);
+
+id(new PhabricatorUserEditor())
+  ->setActor($admin)
+  ->createNewUser($user, $email_object);
 
 $user->sendWelcomeEmail($admin);
 

src/__phutil_library_map__.php

@@ -956,6 +956,7 @@
     'PhabricatorUserAccountSettingsPanelController' => 'applications/people/controller/settings/panels/account',
     'PhabricatorUserConduitSettingsPanelController' => 'applications/people/controller/settings/panels/conduit',
     'PhabricatorUserDAO' => 'applications/people/storage/base',
+    'PhabricatorUserEditor' => 'applications/people/editor',
     'PhabricatorUserEmail' => 'applications/people/storage/email',
     'PhabricatorUserEmailPreferenceSettingsPanelController' => 'applications/people/controller/settings/panels/emailpref',
     'PhabricatorUserEmailSettingsPanelController' => 'applications/people/controller/settings/panels/email',

src/applications/auth/controller/oauthregistration/default/PhabricatorOAuthDefaultRegistrationController.php

@@ -83,7 +83,6 @@ public function processRequest() {
         }
 
         try {
-          $user->save();
 
           // NOTE: We don't verify OAuth email addresses by default because
           // OAuth providers might associate email addresses with accounts that
@@ -92,12 +91,14 @@ public function processRequest() {
           // verifying an email address are high because having a corporate
           // address at a company is sometimes the key to the castle.
 
-          $new_email = id(new PhabricatorUserEmail())
-            ->setUserPHID($user->getPHID())
+
+          $email_obj = id(new PhabricatorUserEmail())
             ->setAddress($new_email)
-            ->setIsPrimary(1)
-            ->setIsVerified(0)
-            ->save();
+            ->setIsVerified(0);
+
+          id(new PhabricatorUserEditor())
+            ->setActor($user)
+            ->createNewUser($user, $email_obj);
 
           $oauth_info->setUserID($user->getID());
           $oauth_info->save();
@@ -106,7 +107,7 @@ public function processRequest() {
           $request->setCookie('phusr', $user->getUsername());
           $request->setCookie('phsid', $session_key);
 
-          $new_email->sendVerificationEmail($user);
+          $email_obj->sendVerificationEmail($user);
 
           return id(new AphrontRedirectResponse())->setURI('/');
         } catch (AphrontQueryDuplicateKeyException $exception) {

src/applications/auth/controller/oauthregistration/default/__init__.php

@@ -9,6 +9,7 @@
 phutil_require_module('phabricator', 'aphront/response/redirect');
 phutil_require_module('phabricator', 'applications/auth/controller/oauthregistration/base');
 phutil_require_module('phabricator', 'applications/files/storage/file');
+phutil_require_module('phabricator', 'applications/people/editor');
 phutil_require_module('phabricator', 'applications/people/storage/email');
 phutil_require_module('phabricator', 'applications/people/storage/user');
 phutil_require_module('phabricator', 'view/form/base');

src/applications/conduit/method/user/disable/ConduitAPI_user_disable_Method.php

@@ -44,7 +44,8 @@ public function defineErrorTypes() {
   }
 
   protected function execute(ConduitAPIRequest $request) {
-    if (!$request->getUser()->getIsAdmin()) {
+    $actor = $request->getUser();
+    if (!$actor->getIsAdmin()) {
       throw new ConduitException('ERR-PERMISSIONS');
     }
 
@@ -59,8 +60,9 @@ protected function execute(ConduitAPIRequest $request) {
     }
 
     foreach ($users as $user) {
-      $user->setIsDisabled(true);
-      $user->save();
+      id(new PhabricatorUserEditor())
+        ->setActor($actor)
+        ->disableUser($user, true);
     }
   }
 

src/applications/conduit/method/user/disable/__init__.php

@@ -8,6 +8,7 @@
 
 phutil_require_module('phabricator', 'applications/conduit/method/user/base');
 phutil_require_module('phabricator', 'applications/conduit/protocol/exception');
+phutil_require_module('phabricator', 'applications/people/editor');
 phutil_require_module('phabricator', 'applications/people/storage/user');
 
 phutil_require_module('phutil', 'utils');

src/applications/people/controller/edit/PhabricatorPeopleEditController.php

@@ -165,22 +165,18 @@ private function processBasicRequest(PhabricatorUser $user) {
         try {
           $is_new = !$user->getID();
 
-          $user->save();
-
-          if ($is_new) {
-
+          if (!$is_new) {
+            id(new PhabricatorUserEditor())
+              ->setActor($admin)
+              ->updateUser($user);
+          } else {
             $email = id(new PhabricatorUserEmail())
-              ->setUserPHID($user->getPHID())
               ->setAddress($new_email)
-              ->setIsPrimary(1)
-              ->setIsVerified(0)
-              ->save();
+              ->setIsVerified(0);
 
-            $log = PhabricatorUserLog::newLog(
-              $admin,
-              $user,
-              PhabricatorUserLog::ACTION_CREATE);
-            $log->save();
+            id(new PhabricatorUserEditor())
+              ->setActor($admin)
+              ->createNewUser($user, $email);
 
             if ($welcome_checked) {
               $user->sendWelcomeEmail($admin);
@@ -255,13 +251,17 @@ private function processBasicRequest(PhabricatorUser $user) {
           ->setValue($new_email)
           ->setError($e_email));
     } else {
+      $email = $user->loadPrimaryEmail();
+      if ($email) {
+        $status = $email->getIsVerified() ? 'Verified' : 'Unverified';
+      } else {
+        $status = 'No Email Address';
+      }
+
       $form->appendChild(
         id(new AphrontFormStaticControl())
           ->setLabel('Email')
-          ->setValue(
-              $user->loadPrimaryEmail()->getIsVerified()
-                ? 'Verified'
-                : 'Unverified'));
+          ->setValue($status));
     }
 
     $form->appendChild($this->getRoleInstructions());
@@ -354,31 +354,21 @@ private function processRoleRequest(PhabricatorUser $user) {
         $new_admin = (bool)$request->getBool('is_admin');
         $old_admin = (bool)$user->getIsAdmin();
         if ($new_admin != $old_admin) {
-          $log = clone $log_template;
-          $log->setAction(PhabricatorUserLog::ACTION_ADMIN);
-          $log->setOldValue($old_admin);
-          $log->setNewValue($new_admin);
-          $user->setIsAdmin($new_admin);
-          $logs[] = $log;
+          id(new PhabricatorUserEditor())
+            ->setActor($admin)
+            ->makeAdminUser($user, $new_admin);
         }
 
         $new_disabled = (bool)$request->getBool('is_disabled');
         $old_disabled = (bool)$user->getIsDisabled();
         if ($new_disabled != $old_disabled) {
-          $log = clone $log_template;
-          $log->setAction(PhabricatorUserLog::ACTION_DISABLE);
-          $log->setOldValue($old_disabled);
-          $log->setNewValue($new_disabled);
-          $user->setIsDisabled($new_disabled);
-          $logs[] = $log;
+          id(new PhabricatorUserEditor())
+            ->setActor($admin)
+            ->disableUser($user, $new_disabled);
         }
       }
 
       if (!$errors) {
-        $user->save();
-        foreach ($logs as $log) {
-          $log->save();
-        }
         return id(new AphrontRedirectResponse())
           ->setURI($request->getRequestURI()->alter('saved', 'true'));
       }

src/applications/people/controller/edit/__init__.php

@@ -9,6 +9,7 @@
 phutil_require_module('phabricator', 'aphront/response/404');
 phutil_require_module('phabricator', 'aphront/response/redirect');
 phutil_require_module('phabricator', 'applications/people/controller/base');
+phutil_require_module('phabricator', 'applications/people/editor');
 phutil_require_module('phabricator', 'applications/people/storage/email');
 phutil_require_module('phabricator', 'applications/people/storage/log');
 phutil_require_module('phabricator', 'applications/people/storage/user');

src/applications/people/controller/settings/panels/email/PhabricatorUserEmailSettingsPanelController.php

@@ -175,13 +175,14 @@ private function returnNewAddressResponse(PhutilURI $uri, $new) {
 
       if (!$errors) {
         $object = id(new PhabricatorUserEmail())
-          ->setUserPHID($user->getPHID())
           ->setAddress($email)
-          ->setIsVerified(0)
-          ->setIsPrimary(0);
+          ->setIsVerified(0);
 
         try {
-          $object->save();
+
+          id(new PhabricatorUserEditor())
+            ->setActor($user)
+            ->addEmail($user, $object);
 
           $object->sendVerificationEmail($user);
 
@@ -245,7 +246,11 @@ private function returnDeleteAddressResponse(PhutilURI $uri, $email_id) {
     }
 
     if ($request->isFormPost()) {
-      $email->delete();
+
+      id(new PhabricatorUserEditor())
+        ->setActor($user)
+        ->removeEmail($user, $email);
+
       return id(new AphrontRedirectResponse())->setURI($uri);
     }
 
@@ -314,21 +319,9 @@ private function returnPrimaryAddressResponse(PhutilURI $uri, $email_id) {
 
     if ($request->isFormPost()) {
 
-      // TODO: Transactions!
-
-      $email->setIsPrimary(1);
-
-      $old_primary = $user->loadPrimaryEmail();
-      if ($old_primary) {
-        $old_primary->setIsPrimary(0);
-        $old_primary->save();
-      }
-      $email->save();
-
-      if ($old_primary) {
-        $old_primary->sendOldPrimaryEmail($user, $email);
-      }
-      $email->sendNewPrimaryEmail($user);
+      id(new PhabricatorUserEditor())
+        ->setActor($user)
+        ->changePrimaryEmail($user, $email);
 
       return id(new AphrontRedirectResponse())->setURI($uri);
     }

src/applications/people/controller/settings/panels/email/__init__.php

@@ -11,6 +11,7 @@
 phutil_require_module('phabricator', 'aphront/response/redirect');
 phutil_require_module('phabricator', 'aphront/response/reload');
 phutil_require_module('phabricator', 'applications/people/controller/settings/panels/base');
+phutil_require_module('phabricator', 'applications/people/editor');
 phutil_require_module('phabricator', 'applications/people/storage/email');
 phutil_require_module('phabricator', 'infrastructure/javelin/markup');
 phutil_require_module('phabricator', 'view/control/table');

src/applications/people/controller/settings/panels/password/PhabricatorUserPasswordSettingsPanelController.php

@@ -79,13 +79,16 @@ public function processRequest() {
       }
 
       if (!$errors) {
-        $user->setPassword($pass);
         // This write is unguarded because the CSRF token has already
         // been checked in the call to $request->isFormPost() and
         // the CSRF token depends on the password hash, so when it
         // is changed here the CSRF token check will fail.
         $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
-        $user->save();
+
+          id(new PhabricatorUserEditor())
+            ->setActor($user)
+            ->changePassword($user, $pass);
+
         unset($unguarded);
 
         if ($valid_token) {

src/applications/people/controller/settings/panels/password/__init__.php

@@ -10,6 +10,7 @@
 phutil_require_module('phabricator', 'aphront/response/redirect');
 phutil_require_module('phabricator', 'aphront/writeguard');
 phutil_require_module('phabricator', 'applications/people/controller/settings/panels/base');
+phutil_require_module('phabricator', 'applications/people/editor');
 phutil_require_module('phabricator', 'applications/people/storage/email');
 phutil_require_module('phabricator', 'infrastructure/env');
 phutil_require_module('phabricator', 'view/form/base');