Clean up image loading for ExternalAccounts

Summary: Ref T1536. This gets the single queries out of the View and builds a propery Query class for ExternalAccount.

Test Plan: Linked/unlinked accounts, logged out, logged in.

Reviewers: chad, btrahan

Reviewed By: chad

CC: aran

Maniphest Tasks: T1536

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

src/__phutil_library_map__.php

@@ -1009,6 +1009,7 @@ phutil_register_library_map(array(
     'PhabricatorExampleEventListener' => 'infrastructure/events/PhabricatorExampleEventListener.php',
     'PhabricatorExtendingPhabricatorConfigOptions' => 'applications/config/option/PhabricatorExtendingPhabricatorConfigOptions.php',
     'PhabricatorExternalAccount' => 'applications/people/storage/PhabricatorExternalAccount.php',
+    'PhabricatorExternalAccountQuery' => 'applications/auth/query/PhabricatorExternalAccountQuery.php',
     'PhabricatorFacebookConfigOptions' => 'applications/config/option/PhabricatorFacebookConfigOptions.php',
     'PhabricatorFactAggregate' => 'applications/fact/storage/PhabricatorFactAggregate.php',
     'PhabricatorFactChartController' => 'applications/fact/controller/PhabricatorFactChartController.php',
@@ -2892,7 +2893,12 @@ phutil_register_library_map(array(
     'PhabricatorEventType' => 'PhutilEventType',
     'PhabricatorExampleEventListener' => 'PhutilEventListener',
     'PhabricatorExtendingPhabricatorConfigOptions' => 'PhabricatorApplicationConfigOptions',
-    'PhabricatorExternalAccount' => 'PhabricatorUserDAO',
+    'PhabricatorExternalAccount' =>
+    array(
+      0 => 'PhabricatorUserDAO',
+      1 => 'PhabricatorPolicyInterface',
+    ),
+    'PhabricatorExternalAccountQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'PhabricatorFacebookConfigOptions' => 'PhabricatorApplicationConfigOptions',
     'PhabricatorFactAggregate' => 'PhabricatorFactDAO',
     'PhabricatorFactChartController' => 'PhabricatorFactController',

src/applications/auth/controller/PhabricatorAuthController.php

@@ -114,9 +114,18 @@ abstract class PhabricatorAuthController extends PhabricatorController {
       return array($account, $provider, $response);
     }
 
-    $account = id(new PhabricatorExternalAccount())->loadOneWhere(
-      'accountSecret = %s',
-      $account_key);
+    // NOTE: We're using the omnipotent user because the actual user may not
+    // be logged in yet, and because we want to tailor an error message to
+    // distinguish between "not usable" and "does not exist". We do explicit
+    // checks later on to make sure this account is valid for the intended
+    // operation.
+
+    $account = id(new PhabricatorExternalAccountQuery())
+      ->setViewer(PhabricatorUser::getOmnipotentUser())
+      ->withAccountSecrets(array($account_key))
+      ->needImages(true)
+      ->executeOne();
+
     if (!$account) {
       $response = $this->renderError(pht('No valid linkable account.'));
       return array($account, $provider, $response);

src/applications/auth/query/PhabricatorExternalAccountQuery.php

@@ -0,0 +1,170 @@
+<?php
+
+final class PhabricatorExternalAccountQuery
+  extends PhabricatorCursorPagedPolicyAwareQuery {
+
+  private $ids;
+  private $phids;
+  private $accountTypes;
+  private $accountDomains;
+  private $accountIDs;
+  private $userPHIDs;
+  private $needImages;
+  private $accountSecrets;
+
+  public function withUserPHIDs(array $user_phids) {
+    $this->userPHIDs = $user_phids;
+    return $this;
+  }
+
+  public function withAccountIDs(array $account_ids) {
+    $this->accountIDs = $account_ids;
+    return $this;
+  }
+
+  public function withAccountDomains(array $account_domains) {
+    $this->accountDomains = $account_domains;
+    return $this;
+  }
+
+  public function withAccountTypes(array $account_types) {
+    $this->accountTypes = $account_types;
+    return $this;
+  }
+
+  public function withPHIDs(array $phids) {
+    $this->phids = $phids;
+    return $this;
+  }
+
+  public function withIDs($ids) {
+    $this->ids = $ids;
+    return $this;
+  }
+
+  public function withAccountSecrets(array $secrets) {
+    $this->accountSecrets = $secrets;
+    return $this;
+  }
+
+  public function needImages($need) {
+    $this->needImages = $need;
+    return $this;
+  }
+
+  public function loadPage() {
+    $table = new PhabricatorExternalAccount();
+    $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 willFilterPage(array $accounts) {
+    if (!$accounts) {
+      return $accounts;
+    }
+
+    if ($this->needImages) {
+      $file_phids = mpull($accounts, 'getProfileImagePHID');
+      $file_phids = array_filter($file_phids);
+
+      if ($file_phids) {
+        // NOTE: We use the omnipotent viewer here because these files are
+        // usually created during registration and can't be associated with
+        // the correct policies, since the relevant user account does not exist
+        // yet. In effect, if you can see an ExternalAccount, you can see its
+        // profile image.
+        $files = id(new PhabricatorFileQuery())
+          ->setViewer(PhabricatorUser::getOmnipotentUser())
+          ->withPHIDs($file_phids)
+          ->execute();
+        $files = mpull($files, null, 'getPHID');
+      } else {
+        $files = array();
+      }
+
+      $default_file = null;
+      foreach ($accounts as $account) {
+        $image_phid = $account->getProfileImagePHID();
+        if ($image_phid && isset($files[$image_phid])) {
+          $account->attachProfileImageFile($files[$image_phid]);
+        } else {
+          if ($default_file === null) {
+            $default_file = PhabricatorFile::loadBuiltin(
+              $this->getViewer(),
+              'profile.png');
+          }
+          $account->attachProfileImageFile($default_file);
+        }
+      }
+    }
+
+    return $accounts;
+  }
+
+  protected function buildWhereClause(AphrontDatabaseConnection $conn_r) {
+    $where = array();
+
+    $where[] = $this->buildPagingClause($conn_r);
+
+    if ($this->ids) {
+      $where[] = qsprintf(
+        $conn_r,
+        'id IN (%Ld)',
+        $this->ids);
+    }
+
+    if ($this->phids) {
+      $where[] = qsprintf(
+        $conn_r,
+        'phid IN (%Ls)',
+        $this->phids);
+    }
+
+    if ($this->accountTypes) {
+      $where[] = qsprintf(
+        $conn_r,
+        'accountType IN (%Ls)',
+        $this->accountTypes);
+    }
+
+    if ($this->accountDomains) {
+      $where[] = qsprintf(
+        $conn_r,
+        'accountDomain IN (%Ls)',
+        $this->accountDomains);
+    }
+
+    if ($this->accountIDs) {
+      $where[] = qsprintf(
+        $conn_r,
+        'accountID IN (%Ls)',
+        $this->accountIDs);
+    }
+
+    if ($this->userPHIDs) {
+      $where[] = qsprintf(
+        $conn_r,
+        'userPHID IN (%Ls)',
+        $this->userPHIDs);
+    }
+
+    if ($this->accountSecrets) {
+      $where[] = qsprintf(
+        $conn_r,
+        'accountSecret IN (%Ls)',
+        $this->accountSecrets);
+    }
+
+    return $this->formatWhereClause($where);
+  }
+
+}

src/applications/auth/view/PhabricatorAuthAccountView.php

@@ -89,19 +89,7 @@ final class PhabricatorAuthAccountView extends AphrontView {
         $account_uri);
     }
 
-    // TODO: This fetch is sketchy. We should probably build
-    // ExternalAccountQuery and move the logic there.
-
-    $image_uri = PhabricatorUser::getDefaultProfileImageURI();
-    if ($account->getProfileImagePHID()) {
-      $image = id(new PhabricatorFileQuery())
-        ->setViewer(PhabricatorUser::getOmnipotentUser())
-        ->withPHIDs(array($account->getProfileImagePHID()))
-        ->executeOne();
-      if ($image) {
-        $image_uri = $image->getProfileThumbURI();
-      }
-    }
+    $image_uri = $account->getProfileImageFile()->getProfileThumbURI();
 
     return phutil_tag(
       'div',

src/applications/people/storage/PhabricatorExternalAccount.php

@@ -1,6 +1,7 @@
 <?php
 
-final class PhabricatorExternalAccount extends PhabricatorUserDAO {
+final class PhabricatorExternalAccount extends PhabricatorUserDAO
+  implements PhabricatorPolicyInterface {
 
   protected $userPHID;
   protected $accountType;
@@ -16,6 +17,20 @@ final class PhabricatorExternalAccount extends PhabricatorUserDAO {
   protected $profileImagePHID;
   protected $properties = array();
 
+  private $profileImageFile;
+
+  public function getProfileImageFile() {
+    if ($this->profileImageFile === null) {
+      throw new Exception("Call attachProfileImageFile() first!");
+    }
+    return $this->profileImageFile;
+  }
+
+  public function attachProfileImageFile(PhabricatorFile $file) {
+    $this->profileImageFile = $file;
+    return $this;
+  }
+
   public function generatePHID() {
     return PhabricatorPHID::generateNewPHID(
       PhabricatorPHIDConstants::PHID_TYPE_XUSR);
@@ -72,4 +87,22 @@ final class PhabricatorExternalAccount extends PhabricatorUserDAO {
     return true;
   }
 
+
+/* -(  PhabricatorPolicyInterface  )----------------------------------------- */
+
+
+  public function getCapabilities() {
+    return array(
+      PhabricatorPolicyCapability::CAN_VIEW,
+    );
+  }
+
+  public function getPolicy($capability) {
+    return PhabricatorPolicies::POLICY_NOONE;
+  }
+
+  public function hasAutomaticCapability($capability, PhabricatorUser $viewer) {
+    return ($viewer->getPHID() == $this->getUserPHID());
+  }
+
 }

src/applications/settings/panel/PhabricatorSettingsPanelExternalAccounts.php

@@ -23,9 +23,12 @@ final class PhabricatorSettingsPanelExternalAccounts
     $viewer = $request->getUser();
 
     $providers = PhabricatorAuthProvider::getAllProviders();
-    $accounts = id(new PhabricatorExternalAccount())->loadAllWhere(
-      'userPHID = %s',
-      $viewer->getPHID());
+
+    $accounts = id(new PhabricatorExternalAccountQuery())
+      ->setViewer($viewer)
+      ->withUserPHIDs(array($viewer->getPHID()))
+      ->needImages(true)
+      ->execute();
 
     $linked_head = id(new PhabricatorHeaderView())
       ->setHeader(pht('Linked Accounts and Authentication'));