Update repository identities after all mutations to users and email a…

…ddresses

Summary:
Ref T13444. Currently, many mutations to users and email addresses (particularly: user creation; and user and address destruction) do not propagate properly to repository identities.

Add hooks to all mutation workflows so repository identities get rebuilt properly when users are created, email addresses are removed, users or email addresses are destroyed, or email addresses are reassigned.

Test Plan:
- Added random email address to account, removed it.
- Added unassociated email address to account, saw identity update (and associate).
  - Removed it, saw identity update (and disassociate).
- Registered an account with an unassociated email address, saw identity update (and associate).
  - Destroyed the account, saw identity update (and disassociate).
- Added address X to account A, unverified.
  - Invited address X.
  - Clicked invite link as account B.
  - Confirmed desire to steal address.
  - Saw identity update and reassociate.

Maniphest Tasks: T13444

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

src/__phutil_library_map__.php

@@ -984,6 +984,7 @@
     'DiffusionRepositoryEditUpdateController' => 'applications/diffusion/controller/DiffusionRepositoryEditUpdateController.php',
     'DiffusionRepositoryFunctionDatasource' => 'applications/diffusion/typeahead/DiffusionRepositoryFunctionDatasource.php',
     'DiffusionRepositoryHistoryManagementPanel' => 'applications/diffusion/management/DiffusionRepositoryHistoryManagementPanel.php',
+    'DiffusionRepositoryIdentityDestructionEngineExtension' => 'applications/diffusion/identity/DiffusionRepositoryIdentityDestructionEngineExtension.php',
     'DiffusionRepositoryIdentityEditor' => 'applications/diffusion/editor/DiffusionRepositoryIdentityEditor.php',
     'DiffusionRepositoryIdentityEngine' => 'applications/diffusion/identity/DiffusionRepositoryIdentityEngine.php',
     'DiffusionRepositoryIdentitySearchEngine' => 'applications/diffusion/query/DiffusionRepositoryIdentitySearchEngine.php',
@@ -6968,6 +6969,7 @@
     'DiffusionRepositoryEditUpdateController' => 'DiffusionRepositoryManageController',
     'DiffusionRepositoryFunctionDatasource' => 'PhabricatorTypeaheadCompositeDatasource',
     'DiffusionRepositoryHistoryManagementPanel' => 'DiffusionRepositoryManagementPanel',
+    'DiffusionRepositoryIdentityDestructionEngineExtension' => 'PhabricatorDestructionEngineExtension',
     'DiffusionRepositoryIdentityEditor' => 'PhabricatorApplicationTransactionEditor',
     'DiffusionRepositoryIdentityEngine' => 'Phobject',
     'DiffusionRepositoryIdentitySearchEngine' => 'PhabricatorApplicationSearchEngine',

src/applications/diffusion/identity/DiffusionRepositoryIdentityDestructionEngineExtension.php

@@ -0,0 +1,40 @@
+<?php
+
+final class DiffusionRepositoryIdentityDestructionEngineExtension
+  extends PhabricatorDestructionEngineExtension {
+
+  const EXTENSIONKEY = 'repository-identities';
+
+  public function getExtensionName() {
+    return pht('Repository Identities');
+  }
+
+  public function didDestroyObject(
+    PhabricatorDestructionEngine $engine,
+    $object) {
+
+    // When users or email addresses are destroyed, queue a task to update
+    // any repository identities that are associated with them. See T13444.
+
+    $related_phids = array();
+    $email_addresses = array();
+
+    if ($object instanceof PhabricatorUser) {
+      $related_phids[] = $object->getPHID();
+    }
+
+    if ($object instanceof PhabricatorUserEmail) {
+      $email_addresses[] = $object->getAddress();
+    }
+
+    if ($related_phids || $email_addresses) {
+      PhabricatorWorker::scheduleTask(
+        'PhabricatorRepositoryIdentityChangeWorker',
+        array(
+          'relatedPHIDs' => $related_phids,
+          'emailAddresses' => $email_addresses,
+        ));
+    }
+  }
+
+}

src/applications/diffusion/identity/DiffusionRepositoryIdentityEngine.php

@@ -95,4 +95,12 @@ private function updateIdentity(PhabricatorRepositoryIdentity $identity) {
     return $identity;
   }
 
+  public function didUpdateEmailAddress($raw_address) {
+    PhabricatorWorker::scheduleTask(
+      'PhabricatorRepositoryIdentityChangeWorker',
+      array(
+        'emailAddresses' => array($raw_address),
+      ));
+  }
+
 }

src/applications/people/editor/PhabricatorUserEditor.php

@@ -89,6 +89,9 @@ public function createNewUser(
       $this->didVerifyEmail($user, $email);
     }
 
+    id(new DiffusionRepositoryIdentityEngine())
+      ->didUpdateEmailAddress($email->getAddress());
+
     return $this;
   }
 
@@ -202,11 +205,8 @@ public function addEmail(
       $user->endWriteLocking();
     $user->saveTransaction();
 
-    // Try and match this new address against unclaimed `RepositoryIdentity`s
-    PhabricatorWorker::scheduleTask(
-      'PhabricatorRepositoryIdentityChangeWorker',
-      array('userPHID' => $user->getPHID()),
-      array('objectPHID' => $user->getPHID()));
+    id(new DiffusionRepositoryIdentityEngine())
+      ->didUpdateEmailAddress($email->getAddress());
 
     return $this;
   }
@@ -241,7 +241,8 @@ public function removeEmail(
           throw new Exception(pht('Email not owned by user!'));
         }
 
-        id(new PhabricatorDestructionEngine())
+        $destruction_engine = id(new PhabricatorDestructionEngine())
+          ->setWaitToFinalizeDestruction(true)
           ->destroyObject($email);
 
         $log = PhabricatorUserLog::initializeNewLog(
@@ -255,6 +256,7 @@ public function removeEmail(
     $user->saveTransaction();
 
     $this->revokePasswordResetLinks($user);
+    $destruction_engine->finalizeDestruction();
 
     return $this;
   }
@@ -327,7 +329,6 @@ public function changePrimaryEmail(
     }
     $email->sendNewPrimaryEmail($user);
 
-
     $this->revokePasswordResetLinks($user);
 
     return $this;
@@ -441,6 +442,9 @@ public function reassignEmail(
 
       $user->endWriteLocking();
     $user->saveTransaction();
+
+    id(new DiffusionRepositoryIdentityEngine())
+      ->didUpdateEmailAddress($email->getAddress());
   }
 
 

src/applications/repository/worker/PhabricatorRepositoryIdentityChangeWorker.php

@@ -6,30 +6,54 @@ final class PhabricatorRepositoryIdentityChangeWorker
   protected function doWork() {
     $viewer = PhabricatorUser::getOmnipotentUser();
 
-    $task_data = $this->getTaskData();
-    $user_phid = idx($task_data, 'userPHID');
+    $related_phids = $this->getTaskDataValue('relatedPHIDs');
+    $email_addresses = $this->getTaskDataValue('emailAddresses');
+
+    // Retain backward compatibility with older tasks which may still be in
+    // queue. Previously, this worker accepted a single "userPHID". See
+    // T13444. This can be removed in some future version of Phabricator once
+    // these tasks have likely flushed out of queue.
+    $legacy_phid = $this->getTaskDataValue('userPHID');
+    if ($legacy_phid) {
+      if (!is_array($related_phids)) {
+        $related_phids = array();
+      }
+      $related_phids[] = $legacy_phid;
+    }
 
-    $user = id(new PhabricatorPeopleQuery())
-      ->withPHIDs(array($user_phid))
-      ->setViewer($viewer)
-      ->executeOne();
+    // Note that we may arrive in this worker after the associated objects
+    // have already been destroyed, so we can't (and shouldn't) verify that
+    // PHIDs correspond to real objects. If you "bin/remove destroy" a user,
+    // we'll end up here with a now-bogus user PHID that we should
+    // disassociate from identities.
 
-    $emails = id(new PhabricatorUserEmail())->loadAllWhere(
-      'userPHID = %s',
-      $user->getPHID());
+    $identity_map = array();
 
-    $identity_engine = id(new DiffusionRepositoryIdentityEngine())
-      ->setViewer($viewer);
+    if ($related_phids) {
+      $identities = id(new PhabricatorRepositoryIdentityQuery())
+        ->setViewer($viewer)
+        ->withRelatedPHIDs($related_phids)
+        ->execute();
+      $identity_map += mpull($identities, null, 'getPHID');
+    }
 
-    foreach ($emails as $email) {
+    if ($email_addresses) {
       $identities = id(new PhabricatorRepositoryIdentityQuery())
         ->setViewer($viewer)
-        ->withEmailAddresses(array($email->getAddress()))
+        ->withEmailAddresses($email_addresses)
         ->execute();
+      $identity_map += mpull($identities, null, 'getPHID');
+    }
 
-      foreach ($identities as $identity) {
-        $identity_engine->newUpdatedIdentity($identity);
-      }
+    // If we didn't find any related identities, we're all set.
+    if (!$identity_map) {
+      return;
+    }
+
+    $identity_engine = id(new DiffusionRepositoryIdentityEngine())
+      ->setViewer($viewer);
+    foreach ($identity_map as $identity) {
+      $identity_engine->newUpdatedIdentity($identity);
     }
   }
 

src/applications/system/engine/PhabricatorDestructionEngine.php

@@ -5,6 +5,9 @@ final class PhabricatorDestructionEngine extends Phobject {
   private $rootLogID;
   private $collectNotes;
   private $notes = array();
+  private $depth = 0;
+  private $destroyedObjects = array();
+  private $waitToFinalizeDestruction = false;
 
   public function setCollectNotes($collect_notes) {
     $this->collectNotes = $collect_notes;
@@ -19,9 +22,20 @@ public function getViewer() {
     return PhabricatorUser::getOmnipotentUser();
   }
 
+  public function setWaitToFinalizeDestruction($wait) {
+    $this->waitToFinalizeDestruction = $wait;
+    return $this;
+  }
+
+  public function getWaitToFinalizeDestruction() {
+    return $this->waitToFinalizeDestruction;
+  }
+
   public function destroyObject(PhabricatorDestructibleInterface $object) {
+    $this->depth++;
+
     $log = id(new PhabricatorSystemDestructionLog())
-      ->setEpoch(time())
+      ->setEpoch(PhabricatorTime::getNow())
       ->setObjectClass(get_class($object));
 
     if ($this->rootLogID) {
@@ -73,7 +87,42 @@ public function destroyObject(PhabricatorDestructibleInterface $object) {
       foreach ($extensions as $key => $extension) {
         $extension->destroyObject($this, $object);
       }
+
+      $this->destroyedObjects[] = $object;
+    }
+
+    $this->depth--;
+
+    // If this is a root-level invocation of "destroyObject()", flush the
+    // queue of destroyed objects and fire "didDestroyObject()" hooks. This
+    // hook allows extensions to do things like queue cache updates which
+    // might race if we fire them during object destruction.
+
+    if (!$this->depth) {
+      if (!$this->getWaitToFinalizeDestruction()) {
+        $this->finalizeDestruction();
+      }
+    }
+
+    return $this;
+  }
+
+  public function finalizeDestruction() {
+    $extensions = PhabricatorDestructionEngineExtension::getAllExtensions();
+
+    foreach ($this->destroyedObjects as $object) {
+      foreach ($extensions as $extension) {
+        if (!$extension->canDestroyObject($this, $object)) {
+          continue;
+        }
+
+        $extension->didDestroyObject($this, $object);
+      }
     }
+
+    $this->destroyedObjects = array();
+
+    return $this;
   }
 
   private function getObjectPHID($object) {

src/applications/system/engine/PhabricatorDestructionEngineExtension.php

@@ -14,9 +14,17 @@ public function canDestroyObject(
     return true;
   }
 
-  abstract public function destroyObject(
+  public function destroyObject(
     PhabricatorDestructionEngine $engine,
-    $object);
+    $object) {
+    return null;
+  }
+
+  public function didDestroyObject(
+    PhabricatorDestructionEngine $engine,
+    $object) {
+    return null;
+  }
 
   final public static function getAllExtensions() {
     return id(new PhutilClassMapQuery())