From a3213ab20be6127e3b3db73533b0fd3c26826ae1 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 15 Aug 2019 16:37:46 -0700 Subject: [PATCH] In Phortune, use actual merchant authority (not authority grants) to control account visibility Summary: Depends on D20715. Ref T13366. See that task for discussion. Replace the unreliable "grantAuthority()"-based check with an actual "can the viewer edit any merchant this account has a relationship with?" check. This makes these objects easier to use from a policy perspective and makes it so that the `Query` alone can fully enforce permissions properly with no setup, so general infrastructure (like handles and transactions) works properly with Phortune objects. Test Plan: Viewed merchants and accounts as users with no authority, direct authority on the account, and indirect authority via a merchant relationship. Maniphest Tasks: T13366 Differential Revision: https://secure.phabricator.com/D20716 --- ...hortuneMerchantInvoiceCreateController.php | 7 +- .../phortune/query/PhortuneMerchantQuery.php | 69 +++++++++++++++++-- .../phortune/storage/PhortuneAccount.php | 22 ++++-- 3 files changed, 84 insertions(+), 14 deletions(-) diff --git a/src/applications/phortune/controller/merchant/PhortuneMerchantInvoiceCreateController.php b/src/applications/phortune/controller/merchant/PhortuneMerchantInvoiceCreateController.php index 300525cdd2..8bd070853c 100644 --- a/src/applications/phortune/controller/merchant/PhortuneMerchantInvoiceCreateController.php +++ b/src/applications/phortune/controller/merchant/PhortuneMerchantInvoiceCreateController.php @@ -58,9 +58,10 @@ final class PhortuneMerchantInvoiceCreateController } if (!$target_account) { - $accounts = PhortuneAccountQuery::loadAccountsForUser( - $target_user, - PhabricatorContentSource::newFromRequest($request)); + $accounts = id(new PhortuneAccountQuery()) + ->setViewer($viewer) + ->withMemberPHIDs(array($target_user->getPHID())) + ->execute(); $form = id(new AphrontFormView()) ->setUser($viewer) diff --git a/src/applications/phortune/query/PhortuneMerchantQuery.php b/src/applications/phortune/query/PhortuneMerchantQuery.php index b6cab7dbc2..aef7d8aaf1 100644 --- a/src/applications/phortune/query/PhortuneMerchantQuery.php +++ b/src/applications/phortune/query/PhortuneMerchantQuery.php @@ -86,14 +86,14 @@ final class PhortuneMerchantQuery if ($this->ids !== null) { $where[] = qsprintf( $conn, - 'id IN (%Ld)', + 'merchant.id IN (%Ld)', $this->ids); } if ($this->phids !== null) { $where[] = qsprintf( $conn, - 'phid IN (%Ls)', + 'merchant.phid IN (%Ls)', $this->phids); } @@ -113,7 +113,7 @@ final class PhortuneMerchantQuery if ($this->memberPHIDs !== null) { $joins[] = qsprintf( $conn, - 'LEFT JOIN %T e ON m.phid = e.src AND e.type = %d', + 'LEFT JOIN %T e ON merchant.phid = e.src AND e.type = %d', PhabricatorEdgeConfig::TABLE_NAME_EDGE, PhortuneMerchantHasMemberEdgeType::EDGECONST); } @@ -126,7 +126,68 @@ final class PhortuneMerchantQuery } protected function getPrimaryTableAlias() { - return 'm'; + return 'merchant'; + } + + public static function canViewersEditMerchants( + array $viewer_phids, + array $merchant_phids) { + + // See T13366 for some discussion. This is an unusual caching construct to + // make policy filtering of Accounts easier. + + foreach ($viewer_phids as $key => $viewer_phid) { + if (!$viewer_phid) { + unset($viewer_phids[$key]); + } + } + + if (!$viewer_phids) { + return array(); + } + + $cache_key = 'phortune.merchant.can-edit'; + $cache = PhabricatorCaches::getRequestCache(); + + $cache_data = $cache->getKey($cache_key); + if (!$cache_data) { + $cache_data = array(); + } + + $load_phids = array(); + foreach ($viewer_phids as $viewer_phid) { + if (!isset($cache_data[$viewer_phid])) { + $load_phids[] = $viewer_phid; + } + } + + $did_write = false; + foreach ($load_phids as $load_phid) { + $merchants = id(new self()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withMemberPHIDs(array($load_phid)) + ->execute(); + foreach ($merchants as $merchant) { + $cache_data[$load_phid][$merchant->getPHID()] = true; + $did_write = true; + } + } + + if ($did_write) { + $cache->setKey($cache_key, $cache_data); + } + + $results = array(); + foreach ($viewer_phids as $viewer_phid) { + foreach ($merchant_phids as $merchant_phid) { + if (!isset($cache_data[$viewer_phid][$merchant_phid])) { + continue; + } + $results[$viewer_phid][$merchant_phid] = true; + } + } + + return $results; } } diff --git a/src/applications/phortune/storage/PhortuneAccount.php b/src/applications/phortune/storage/PhortuneAccount.php index 6b31805671..182c80f40f 100644 --- a/src/applications/phortune/storage/PhortuneAccount.php +++ b/src/applications/phortune/storage/PhortuneAccount.php @@ -179,13 +179,18 @@ final class PhortuneAccount extends PhortuneDAO return true; } - // If the viewer is acting on behalf of a merchant, they can see - // payment accounts. + // See T13366. If the viewer can edit any merchant that this payment + // account has a relationship with, they can see the payment account. if ($capability == PhabricatorPolicyCapability::CAN_VIEW) { - foreach ($viewer->getAuthorities() as $authority) { - if ($authority instanceof PhortuneMerchant) { - return true; - } + $viewer_phids = array($viewer->getPHID()); + $merchant_phids = $this->getMerchantPHIDs(); + + $any_edit = PhortuneMerchantQuery::canViewersEditMerchants( + $viewer_phids, + $merchant_phids); + + if ($any_edit) { + return true; } } @@ -193,7 +198,10 @@ final class PhortuneAccount extends PhortuneDAO } public function describeAutomaticCapability($capability) { - return pht('Members of an account can always view and edit it.'); + return array( + pht('Members of an account can always view and edit it.'), + pht('Merchants an account has established a relationship can view it.'), + ); }