From 30237aaa4788155eb9b9f030a5a48688567061b1 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 17 Jun 2013 12:14:00 -0700 Subject: [PATCH] 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 --- resources/builtin/profile.png | Bin 0 -> 959 bytes src/__phutil_library_map__.php | 8 +- .../controller/PhabricatorAuthController.php | 15 +- .../query/PhabricatorExternalAccountQuery.php | 170 ++++++++++++++++++ .../auth/view/PhabricatorAuthAccountView.php | 14 +- .../storage/PhabricatorExternalAccount.php | 35 +++- ...abricatorSettingsPanelExternalAccounts.php | 9 +- 7 files changed, 230 insertions(+), 21 deletions(-) create mode 100644 resources/builtin/profile.png create mode 100644 src/applications/auth/query/PhabricatorExternalAccountQuery.php diff --git a/resources/builtin/profile.png b/resources/builtin/profile.png new file mode 100644 index 0000000000000000000000000000000000000000..755d19cd2ef2467e93a7fb1fee14978bf2ac866e GIT binary patch literal 959 zcmV;w13>(VP)QPui1bI*-?emZemqRR;kx zA0=Etj0ThSLfXlyscPm%G0cOlYKPtapc}`@T;>I3CaQ&FLBGx3{OMI_^%mkTpP zC1nb2$gNWx#>-d;#B`ygV>j8LZ&bZOge<9QZe{zNROq1xsTmXj6g!+IkrzV?Y&cmUNveC8D^FC4XcTG+=k40d?(2UTV3tED}j|;-u>(*Vm1W^uh zgt9Y>z(O~MVT2M#A{G|1--zE9fdzS0s3nqyRe7A|n^DgutTg&zq-krwu8Lp^P9l$1 zhf=x8+G=ENv;H*4^>o8RkB^oW;~EBClDwj^@Yg?|cvP_*nS#0P09T#UA^1q4jK)Hi ze|*WmCVn=IlSqxpf{2gkzDdO~p}}u<=ZUrDV-1%DF?pVMu`H6b#iBQI(hgyC!*GzY zREMgU3rO+2pv39uX?Y8-&YdY!cY0nq)_q2st1 zj?srPwJ}qFztY9Jt&Z^-^pONU%jXW_WBD61K{-Mp3$B(UBK|xSq3-!oMMJCUzS~sI z#8d~|KD$D*fI}VsmWw9&uX_h~-Q>@B_Z&U|4KXMy!2vA&z9-K^@)=jU{}L3Am2v&Y hO0s?d009600{~U%*wRS4HSz!e002ovPDHLkV1h=g&Gi5P literal 0 HcmV?d00001 diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 3211fffd0d..a56036d494 100644 --- a/src/__phutil_library_map__.php +++ b/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', diff --git a/src/applications/auth/controller/PhabricatorAuthController.php b/src/applications/auth/controller/PhabricatorAuthController.php index d37aed6912..182a210e4a 100644 --- a/src/applications/auth/controller/PhabricatorAuthController.php +++ b/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); diff --git a/src/applications/auth/query/PhabricatorExternalAccountQuery.php b/src/applications/auth/query/PhabricatorExternalAccountQuery.php new file mode 100644 index 0000000000..8d40963708 --- /dev/null +++ b/src/applications/auth/query/PhabricatorExternalAccountQuery.php @@ -0,0 +1,170 @@ +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); + } + +} diff --git a/src/applications/auth/view/PhabricatorAuthAccountView.php b/src/applications/auth/view/PhabricatorAuthAccountView.php index 9cf19e65df..916b7e3b24 100644 --- a/src/applications/auth/view/PhabricatorAuthAccountView.php +++ b/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', diff --git a/src/applications/people/storage/PhabricatorExternalAccount.php b/src/applications/people/storage/PhabricatorExternalAccount.php index 31cd25287e..558485a316 100644 --- a/src/applications/people/storage/PhabricatorExternalAccount.php +++ b/src/applications/people/storage/PhabricatorExternalAccount.php @@ -1,6 +1,7 @@ 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()); + } + } diff --git a/src/applications/settings/panel/PhabricatorSettingsPanelExternalAccounts.php b/src/applications/settings/panel/PhabricatorSettingsPanelExternalAccounts.php index c3d303c516..21c45e36b2 100644 --- a/src/applications/settings/panel/PhabricatorSettingsPanelExternalAccounts.php +++ b/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'));