From 7b0b820be1236fa4a13aa34e12d2bfe329c00c28 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 28 Mar 2016 12:01:18 -0700 Subject: [PATCH] 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 | 4 + .../bridge/DoorkeeperBridgeGitHubUser.php | 120 ++++++++++++++++++ .../phid/DoorkeeperExternalObjectPHIDType.php | 47 +++++++ .../query/DoorkeeperExternalObjectQuery.php | 34 ++--- .../storage/DoorkeeperExternalObject.php | 23 +++- .../nuance/item/NuanceGitHubEventItemType.php | 102 +++++++++++++-- .../phid/PhabricatorPHIDConstants.php | 2 - .../edges/constants/PhabricatorEdgeConfig.php | 1 - 8 files changed, 294 insertions(+), 39 deletions(-) create mode 100644 src/applications/doorkeeper/bridge/DoorkeeperBridgeGitHubUser.php create mode 100644 src/applications/doorkeeper/phid/DoorkeeperExternalObjectPHIDType.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 9a2a1a7e19..df9a2dd3d6 100644 --- a/src/__phutil_library_map__.php +++ b/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', diff --git a/src/applications/doorkeeper/bridge/DoorkeeperBridgeGitHubUser.php b/src/applications/doorkeeper/bridge/DoorkeeperBridgeGitHubUser.php new file mode 100644 index 0000000000..3470894e5c --- /dev/null +++ b/src/applications/doorkeeper/bridge/DoorkeeperBridgeGitHubUser.php @@ -0,0 +1,120 @@ +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'))); + } + +} diff --git a/src/applications/doorkeeper/phid/DoorkeeperExternalObjectPHIDType.php b/src/applications/doorkeeper/phid/DoorkeeperExternalObjectPHIDType.php new file mode 100644 index 0000000000..be9e32cf2a --- /dev/null +++ b/src/applications/doorkeeper/phid/DoorkeeperExternalObjectPHIDType.php @@ -0,0 +1,47 @@ +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); + } + } + +} diff --git a/src/applications/doorkeeper/query/DoorkeeperExternalObjectQuery.php b/src/applications/doorkeeper/query/DoorkeeperExternalObjectQuery.php index 770678a687..bc794cc629 100644 --- a/src/applications/doorkeeper/query/DoorkeeperExternalObjectQuery.php +++ b/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() { diff --git a/src/applications/doorkeeper/storage/DoorkeeperExternalObject.php b/src/applications/doorkeeper/storage/DoorkeeperExternalObject.php index 3050008594..9d8d3bc967 100644 --- a/src/applications/doorkeeper/storage/DoorkeeperExternalObject.php +++ b/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 )----------------------------------------- */ diff --git a/src/applications/nuance/item/NuanceGitHubEventItemType.php b/src/applications/nuance/item/NuanceGitHubEventItemType.php index 21b371c506..3fef8b3da7 100644 --- a/src/applications/nuance/item/NuanceGitHubEventItemType.php +++ b/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); } diff --git a/src/applications/phid/PhabricatorPHIDConstants.php b/src/applications/phid/PhabricatorPHIDConstants.php index 6e6dfca6dc..91330bf499 100644 --- a/src/applications/phid/PhabricatorPHIDConstants.php +++ b/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'; diff --git a/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php b/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php index 4e256dd5ca..0473f2426d 100644 --- a/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php +++ b/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);