Bridge GitHub users into Phabricator and attribute actions to them

Summary:
Ref T10538. Ref T10537. This creates PHIDs which represent GitHub users, and uses them as the actors for synchronized comments.

I've just made them Doorkeeper objects. There are three major kinds of objects they //could// possibly be:

  - Nuance requestor objects.
  - External account objects.
  - Doorkeeper objects.

I don't think we actually need distinct nuance requestor objects. These don't really do anything right now, and were originally created before Doorkeeper. I think Doorkeeper is a superset of nuance requestor functionality, and better developed and more flexible.

Likewise, doorkeeper objects are much more flexible than external account objects, and it's nice to imagine that we can import from Twootfeed or whatever without needing to build full OAuth for it. I also like less stuff touching auth code, when possible.

Making these separate from external accounts does make it a bit harder to reconcile external users with internal users, but I think that's OK, and that it's generally desirable to show the real source of a piece of content. That is, if I wrote a comment on GitHub but also have a Phabricator account, I think it's good to show "epriestley (GitHub)" (the GitHub user) as the author, not "epriestley" (the Phabricator user). I think this is generally less confusing overall, and we can add more linkage later to make it clearer.

Test Plan:
{F1194104}

{F1194105}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10537, T10538

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

src/__phutil_library_map__.php

@@ -843,12 +843,14 @@ phutil_register_library_map(array(
     'DoorkeeperBridgeAsana' => 'applications/doorkeeper/bridge/DoorkeeperBridgeAsana.php',
     'DoorkeeperBridgeGitHub' => 'applications/doorkeeper/bridge/DoorkeeperBridgeGitHub.php',
     'DoorkeeperBridgeGitHubIssue' => 'applications/doorkeeper/bridge/DoorkeeperBridgeGitHubIssue.php',
+    'DoorkeeperBridgeGitHubUser' => 'applications/doorkeeper/bridge/DoorkeeperBridgeGitHubUser.php',
     'DoorkeeperBridgeJIRA' => 'applications/doorkeeper/bridge/DoorkeeperBridgeJIRA.php',
     'DoorkeeperBridgeJIRATestCase' => 'applications/doorkeeper/bridge/__tests__/DoorkeeperBridgeJIRATestCase.php',
     'DoorkeeperBridgedObjectCurtainExtension' => 'applications/doorkeeper/engineextension/DoorkeeperBridgedObjectCurtainExtension.php',
     'DoorkeeperBridgedObjectInterface' => 'applications/doorkeeper/bridge/DoorkeeperBridgedObjectInterface.php',
     'DoorkeeperDAO' => 'applications/doorkeeper/storage/DoorkeeperDAO.php',
     'DoorkeeperExternalObject' => 'applications/doorkeeper/storage/DoorkeeperExternalObject.php',
+    'DoorkeeperExternalObjectPHIDType' => 'applications/doorkeeper/phid/DoorkeeperExternalObjectPHIDType.php',
     'DoorkeeperExternalObjectQuery' => 'applications/doorkeeper/query/DoorkeeperExternalObjectQuery.php',
     'DoorkeeperFeedStoryPublisher' => 'applications/doorkeeper/engine/DoorkeeperFeedStoryPublisher.php',
     'DoorkeeperFeedWorker' => 'applications/doorkeeper/worker/DoorkeeperFeedWorker.php',
@@ -5017,6 +5019,7 @@ phutil_register_library_map(array(
     'DoorkeeperBridgeAsana' => 'DoorkeeperBridge',
     'DoorkeeperBridgeGitHub' => 'DoorkeeperBridge',
     'DoorkeeperBridgeGitHubIssue' => 'DoorkeeperBridgeGitHub',
+    'DoorkeeperBridgeGitHubUser' => 'DoorkeeperBridgeGitHub',
     'DoorkeeperBridgeJIRA' => 'DoorkeeperBridge',
     'DoorkeeperBridgeJIRATestCase' => 'PhabricatorTestCase',
     'DoorkeeperBridgedObjectCurtainExtension' => 'PHUICurtainExtension',
@@ -5025,6 +5028,7 @@ phutil_register_library_map(array(
       'DoorkeeperDAO',
       'PhabricatorPolicyInterface',
     ),
+    'DoorkeeperExternalObjectPHIDType' => 'PhabricatorPHIDType',
     'DoorkeeperExternalObjectQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'DoorkeeperFeedStoryPublisher' => 'Phobject',
     'DoorkeeperFeedWorker' => 'FeedPushWorker',

src/applications/doorkeeper/bridge/DoorkeeperBridgeGitHubUser.php

@@ -0,0 +1,120 @@
+<?php
+
+final class DoorkeeperBridgeGitHubUser
+  extends DoorkeeperBridgeGitHub {
+
+  const OBJTYPE_GITHUB_USER = 'github.user';
+
+  public function canPullRef(DoorkeeperObjectRef $ref) {
+    if (!parent::canPullRef($ref)) {
+      return false;
+    }
+
+    if ($ref->getObjectType() !== self::OBJTYPE_GITHUB_USER) {
+      return false;
+    }
+
+    return true;
+  }
+
+  public function pullRefs(array $refs) {
+    $token = $this->getGitHubAccessToken();
+    if (!strlen($token)) {
+      return null;
+    }
+
+    $template = id(new PhutilGitHubFuture())
+      ->setAccessToken($token);
+
+    $futures = array();
+    $id_map = mpull($refs, 'getObjectID', 'getObjectKey');
+    foreach ($id_map as $key => $id) {
+      // GitHub doesn't provide a way to query for users by ID directly, but we
+      // can list all users, ordered by ID, starting at some particular ID,
+      // with a page size of one, which will achieve the desired effect.
+      $one_less = ($id - 1);
+      $uri = "/users?since={$one_less}&per_page=1";
+
+      $data = array();
+      $futures[$key] = id(clone $template)
+        ->setRawGitHubQuery($uri, $data);
+    }
+
+    $results = array();
+    $failed = array();
+    foreach (new FutureIterator($futures) as $key => $future) {
+      try {
+        $results[$key] = $future->resolve();
+      } catch (Exception $ex) {
+        if (($ex instanceof HTTPFutureResponseStatus) &&
+            ($ex->getStatusCode() == 404)) {
+          // TODO: Do we end up here for deleted objects and invisible
+          // objects?
+        } else {
+          phlog($ex);
+          $failed[$key] = $ex;
+        }
+      }
+    }
+
+    $viewer = $this->getViewer();
+
+    foreach ($refs as $ref) {
+      $ref->setAttribute('name', pht('GitHub User %s', $ref->getObjectID()));
+
+      $did_fail = idx($failed, $ref->getObjectKey());
+      if ($did_fail) {
+        $ref->setSyncFailed(true);
+        continue;
+      }
+
+      $result = idx($results, $ref->getObjectKey());
+      if (!$result) {
+        continue;
+      }
+
+      $body = $result->getBody();
+      if (!is_array($body) || !count($body)) {
+        $ref->setSyncFailed(true);
+        continue;
+      }
+
+      $spec = head($body);
+      if (!is_array($spec)) {
+        $ref->setSyncFailed(true);
+        continue;
+      }
+
+      // Because we're using a paging query to load each user, if a user (say,
+      // user ID 123) does not exist for some reason, we might get the next
+      // user (say, user ID 124) back. Make sure the user we got back is really
+      // the user we expect.
+      $id = idx($spec, 'id');
+      if ($id !== $ref->getObjectID()) {
+        $ref->setSyncFailed(true);
+        continue;
+      }
+
+      $ref->setIsVisible(true);
+      $ref->setAttribute('api.raw', $spec);
+      $ref->setAttribute('name', $spec['login']);
+
+      $obj = $ref->getExternalObject();
+      $this->fillObjectFromData($obj, $spec);
+
+      $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
+        $obj->save();
+      unset($unguarded);
+    }
+  }
+
+  public function fillObjectFromData(DoorkeeperExternalObject $obj, $spec) {
+    $uri = $spec['html_url'];
+    $obj->setObjectURI($uri);
+
+    $login = $spec['login'];
+
+    $obj->setDisplayName(pht('%s <%s>', $login, pht('GitHub')));
+  }
+
+}

src/applications/doorkeeper/phid/DoorkeeperExternalObjectPHIDType.php

@@ -0,0 +1,47 @@
+<?php
+
+final class DoorkeeperExternalObjectPHIDType
+  extends PhabricatorPHIDType {
+
+  const TYPECONST = 'XOBJ';
+
+  public function getTypeName() {
+    return pht('External Object');
+  }
+
+  public function newObject() {
+    return new DoorkeeperExternalObject();
+  }
+
+  public function getPHIDTypeApplicationClass() {
+    return 'PhabricatorDoorkeeperApplication';
+  }
+
+  protected function buildQueryForObjects(
+    PhabricatorObjectQuery $query,
+    array $phids) {
+
+    return id(new DoorkeeperExternalObjectQuery())
+      ->withPHIDs($phids);
+  }
+
+  public function loadHandles(
+    PhabricatorHandleQuery $query,
+    array $handles,
+    array $objects) {
+
+    foreach ($handles as $phid => $handle) {
+      $xobj = $objects[$phid];
+
+      $uri = $xobj->getObjectURI();
+      $name = $xobj->getDisplayName();
+      $full_name = $xobj->getDisplayFullName();
+
+      $handle
+        ->setURI($uri)
+        ->setName($name)
+        ->setFullName($full_name);
+    }
+  }
+
+}

src/applications/doorkeeper/query/DoorkeeperExternalObjectQuery.php

@@ -16,40 +16,32 @@ final class DoorkeeperExternalObjectQuery
     return $this;
   }
 
-  protected function loadPage() {
-    $table = new DoorkeeperExternalObject();
-    $conn_r = $table->establishConnection('r');
-
-    $data = queryfx_all(
-      $conn_r,
-      'SELECT * FROM %T %Q %Q %Q',
-      $table->getTableName(),
-      $this->buildWhereClause($conn_r),
-      $this->buildOrderClause($conn_r),
-      $this->buildLimitClause($conn_r));
-
-    return $table->loadAllFromArray($data);
+  public function newResultObject() {
+    return new DoorkeeperExternalObject();
   }
 
-  protected function buildWhereClause(AphrontDatabaseConnection $conn_r) {
-    $where = array();
+  protected function loadPage() {
+    return $this->loadStandardPage($this->newResultObject());
+  }
 
-    if ($this->phids) {
+  protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) {
+    $where = parent::buildWhereClauseParts($conn);
+
+    if ($this->phids !== null) {
       $where[] = qsprintf(
-        $conn_r,
+        $conn,
         'phid IN (%Ls)',
         $this->phids);
     }
 
-    if ($this->objectKeys) {
+    if ($this->objectKeys !== null) {
       $where[] = qsprintf(
-        $conn_r,
+        $conn,
         'objectKey IN (%Ls)',
         $this->objectKeys);
     }
 
-    $where[] = $this->buildPagingClause($conn_r);
-    return $this->formatWhereClause($where);
+    return $where;
   }
 
   public function getQueryApplicationClass() {

src/applications/doorkeeper/storage/DoorkeeperExternalObject.php

@@ -47,7 +47,7 @@ final class DoorkeeperExternalObject extends DoorkeeperDAO
 
   public function generatePHID() {
     return PhabricatorPHID::generateNewPHID(
-      PhabricatorPHIDConstants::PHID_TYPE_XOBJ);
+      DoorkeeperExternalObjectPHIDType::TYPECONST);
   }
 
   public function getProperty($key, $default = null) {
@@ -83,6 +83,27 @@ final class DoorkeeperExternalObject extends DoorkeeperDAO
     return parent::save();
   }
 
+  public function setDisplayName($display_name) {
+    return $this->setProperty('xobj.name.display', $display_name);
+  }
+
+  public function getDisplayName() {
+    return $this->getProperty('xobj.name.display', pht('External Object'));
+  }
+
+  public function setDisplayFullName($full_name) {
+    return $this->setProperty('xobj.name.display-full', $full_name);
+  }
+
+  public function getDisplayFullName() {
+    $full_name = $this->getProperty('xobj.name.display-full');
+
+    if ($full_name !== null) {
+      return $full_name;
+    }
+
+    return $this->getDisplayName();
+  }
 
 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */
 

src/applications/nuance/item/NuanceGitHubEventItemType.php

@@ -6,6 +6,7 @@ final class NuanceGitHubEventItemType
   const ITEMTYPE = 'github.event';
 
   private $externalObject;
+  private $externalActor;
 
   public function getItemTypeDisplayName() {
     return pht('GitHub Event');
@@ -27,17 +28,18 @@ final class NuanceGitHubEventItemType
     $viewer = $this->getViewer();
     $is_dirty = false;
 
-    // TODO: Link up the requestor, etc.
-
-    $is_dirty = false;
-
     $xobj = $this->reloadExternalObject($item);
-
     if ($xobj) {
       $item->setItemProperty('doorkeeper.xobj.phid', $xobj->getPHID());
       $is_dirty = true;
     }
 
+    $actor = $this->reloadExternalActor($item);
+    if ($actor) {
+      $item->setRequestorPHID($actor->getPHID());
+      $is_dirty = true;
+    }
+
     if ($item->getStatus() == NuanceItem::STATUS_IMPORTING) {
       $item->setStatus(NuanceItem::STATUS_ROUTING);
       $is_dirty = true;
@@ -48,6 +50,21 @@ final class NuanceGitHubEventItemType
     }
   }
 
+  private function getDoorkeeperActorRef(NuanceItem $item) {
+    $raw = $this->newRawEvent($item);
+
+    $user_id = $raw->getActorGitHubUserID();
+    if (!$user_id) {
+      return null;
+    }
+
+    $ref_type = DoorkeeperBridgeGitHubUser::OBJTYPE_GITHUB_USER;
+
+    return $this->newDoorkeeperRef()
+      ->setObjectType($ref_type)
+      ->setObjectID($user_id);
+  }
+
   private function getDoorkeeperRef(NuanceItem $item) {
     $raw = $this->newRawEvent($item);
 
@@ -64,19 +81,52 @@ final class NuanceGitHubEventItemType
       return null;
     }
 
-    return id(new DoorkeeperObjectRef())
-      ->setApplicationType(DoorkeeperBridgeGitHub::APPTYPE_GITHUB)
-      ->setApplicationDomain(DoorkeeperBridgeGitHub::APPDOMAIN_GITHUB)
+    return $this->newDoorkeeperRef()
       ->setObjectType($ref_type)
       ->setObjectID($full_ref);
   }
 
+  private function newDoorkeeperRef() {
+    return id(new DoorkeeperObjectRef())
+      ->setApplicationType(DoorkeeperBridgeGitHub::APPTYPE_GITHUB)
+      ->setApplicationDomain(DoorkeeperBridgeGitHub::APPDOMAIN_GITHUB);
+  }
+
   private function reloadExternalObject(NuanceItem $item, $local = false) {
     $ref = $this->getDoorkeeperRef($item);
     if (!$ref) {
       return null;
     }
 
+    $xobj = $this->reloadExternalRef($item, $ref, $local);
+
+    if ($xobj) {
+      $this->externalObject = $xobj;
+    }
+
+    return $xobj;
+  }
+
+  private function reloadExternalActor(NuanceItem $item, $local = false) {
+    $ref = $this->getDoorkeeperActorRef($item);
+    if (!$ref) {
+      return null;
+    }
+
+    $xobj = $this->reloadExternalRef($item, $ref, $local);
+
+    if ($xobj) {
+      $this->externalActor = $xobj;
+    }
+
+    return $xobj;
+  }
+
+  private function reloadExternalRef(
+    NuanceItem $item,
+    DoorkeeperObjectRef $ref,
+    $local) {
+
     $source = $item->getSource();
     $token = $source->getSourceProperty('github.token');
     $token = new PhutilOpaqueEnvelope($token);
@@ -97,10 +147,6 @@ final class NuanceGitHubEventItemType
       $xobj = $ref->getExternalObject();
     }
 
-    if ($xobj) {
-      $this->externalObject = $xobj;
-    }
-
     return $xobj;
   }
 
@@ -121,6 +167,23 @@ final class NuanceGitHubEventItemType
     return null;
   }
 
+  private function getExternalActor(NuanceItem $item) {
+    if ($this->externalActor === null) {
+      $xobj = $this->reloadExternalActor($item, $local = true);
+      if ($xobj) {
+        $this->externalActor = $xobj;
+      } else {
+        $this->externalActor = false;
+      }
+    }
+
+    if ($this->externalActor) {
+      return $this->externalActor;
+    }
+
+    return null;
+  }
+
   private function newRawEvent(NuanceItem $item) {
     $type = $item->getItemProperty('api.type');
     $raw = $item->getItemProperty('api.raw', array());
@@ -174,6 +237,13 @@ final class NuanceGitHubEventItemType
       }
     }
 
+    $xactor = $this->getExternalActor($item);
+    if ($xactor) {
+      $panels[] = $this->newCurtainPanel($item)
+        ->setHeaderText(pht('GitHub Actor'))
+        ->appendChild($viewer->renderHandle($xactor->getPHID()));
+    }
+
     return $panels;
   }
 
@@ -363,8 +433,12 @@ final class NuanceGitHubEventItemType
   }
 
   protected function getActingAsPHID(NuanceItem $item) {
-    // TODO: This should be an external account PHID representing the original
-    // GitHub user.
+    $actor_phid = $item->getRequestorPHID();
+
+    if ($actor_phid) {
+      return $actor_phid;
+    }
+
     return parent::getActingAsPHID($item);
   }
 

src/applications/phid/PhabricatorPHIDConstants.php

@@ -10,8 +10,6 @@ final class PhabricatorPHIDConstants extends Phobject {
 
   const PHID_TYPE_XCMT    = 'XCMT';
 
-  const PHID_TYPE_XOBJ    = 'XOBJ';
-
   const PHID_TYPE_VOID    = 'VOID';
   const PHID_VOID         = 'PHID-VOID-00000000000000000000';
 

src/infrastructure/edges/constants/PhabricatorEdgeConfig.php

@@ -17,7 +17,6 @@ final class PhabricatorEdgeConfig extends PhabricatorEdgeConstants {
 
     static $class_map = array(
       PhabricatorPHIDConstants::PHID_TYPE_TOBJ  => 'HarbormasterObject',
-      PhabricatorPHIDConstants::PHID_TYPE_XOBJ  => 'DoorkeeperExternalObject',
     );
 
     $class = idx($class_map, $phid_type);