From ab4743b216f262487166a39dfbb2ee81db0532f9 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 3 Mar 2015 10:38:25 -0800 Subject: [PATCH] Improve Phortune policy behavior Summary: Currently, PhortuneAccounts have a very open default policy to allow merchants to see and interact with them. This has the undesirable side effect of leaking their names in too many places, because all users are allowed to load the handles for the accounts. Although this information is not super sensitive, we shouldn't expose it. I went through about 5 really messy diffs trying to fix this. It's very complicated because there are a lot of objects and many of them are related to PhortuneAccounts, but PhortuneAccounts are not bound to a specific merchant. This lead to a lot of threading viewers and merchants all over the place through the call stack and some really sketchy diffs with OmnipotentUsers that weren't going anywhere good. This is the cleanest approach I came up with, by far: - Introduce the concept of an "Authority", which gives a user more powers as a viewer. For now, since we only have one use case, this is pretty open-ended. - When a viewer is acting as a merchant, grant them authority through the merchant. - Have Accounts check if the viewer is acting with merchant authority. This lets us easily implement the rule "merchants can see this stuff" without being too broad. Then update the Subscription view to respect Merchant Authority. I partially updated the Cart views to respect it. I'll finish this up in a separate diff, but this seemed like a good checkpoint that introduced the concept without too much extra baggage. This feels pretty good/clean to me, overall, even ignoring the series of horrible messes I made on my way here. Test Plan: - Verified I can see everything I need to as a merchant (modulo un-updated Cart UIs). - Verified I can see nothing when acting as a normal user. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Differential Revision: https://secure.phabricator.com/D11945 --- .../people/storage/PhabricatorUser.php | 21 +++++++++++++++ .../PhabricatorPhortuneApplication.php | 6 +++++ .../controller/PhortuneCartListController.php | 1 + .../controller/PhortuneCartViewController.php | 11 +++++++- .../controller/PhortuneController.php | 26 +++++++++++++++++++ .../PhortuneSubscriptionListController.php | 1 + .../PhortuneSubscriptionViewController.php | 3 ++- .../phortune/query/PhortuneCartQuery.php | 8 ++++++ .../query/PhortuneCartSearchEngine.php | 13 ++++++++++ .../query/PhortunePaymentMethodQuery.php | 8 ++++++ .../phortune/storage/PhortuneAccount.php | 20 ++++++++++---- 11 files changed, 111 insertions(+), 7 deletions(-) diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php index 2276a032cb..a44cb08a56 100644 --- a/src/applications/people/storage/PhabricatorUser.php +++ b/src/applications/people/storage/PhabricatorUser.php @@ -50,6 +50,8 @@ final class PhabricatorUser private $alternateCSRFString = self::ATTACHABLE; private $session = self::ATTACHABLE; + private $authorities = array(); + protected function readField($field) { switch ($field) { case 'timezoneIdentifier': @@ -694,6 +696,25 @@ EOBODY; $email->getUserPHID()); } + + /** + * Grant a user a source of authority, to let them bypass policy checks they + * could not otherwise. + */ + public function grantAuthority($authority) { + $this->authorities[] = $authority; + return $this; + } + + + /** + * Get authorities granted to the user. + */ + public function getAuthorities() { + return $this->authorities; + } + + /* -( Multi-Factor Authentication )---------------------------------------- */ diff --git a/src/applications/phortune/application/PhabricatorPhortuneApplication.php b/src/applications/phortune/application/PhabricatorPhortuneApplication.php index ee573e2664..db3f52d61d 100644 --- a/src/applications/phortune/application/PhabricatorPhortuneApplication.php +++ b/src/applications/phortune/application/PhabricatorPhortuneApplication.php @@ -85,6 +85,12 @@ final class PhabricatorPhortuneApplication extends PhabricatorApplication { 'edit/(?:(?P\d+)/)?' => 'PhortuneMerchantEditController', 'orders/(?P\d+)/(?:query/(?P[^/]+)/)?' => 'PhortuneCartListController', + '(?P\d+)/cart/(?P\d+)/' => array( + '' => 'PhortuneCartViewController', + '(?Pcancel|refund)/' => 'PhortuneCartCancelController', + 'update/' => 'PhortuneCartUpdateController', + 'accept/' => 'PhortuneCartAcceptController', + ), '(?P\d+)/subscription/' => array( '(?:query/(?P[^/]+)/)?' => 'PhortuneSubscriptionListController', diff --git a/src/applications/phortune/controller/PhortuneCartListController.php b/src/applications/phortune/controller/PhortuneCartListController.php index bc73ecfd1a..f07fad3819 100644 --- a/src/applications/phortune/controller/PhortuneCartListController.php +++ b/src/applications/phortune/controller/PhortuneCartListController.php @@ -42,6 +42,7 @@ final class PhortuneCartListController return new Aphront404Response(); } $this->merchant = $merchant; + $viewer->grantAuthority($merchant); $engine->setMerchant($merchant); } else if ($account_id) { $account = id(new PhortuneAccountQuery()) diff --git a/src/applications/phortune/controller/PhortuneCartViewController.php b/src/applications/phortune/controller/PhortuneCartViewController.php index e5557ca8fd..b9f3d19b44 100644 --- a/src/applications/phortune/controller/PhortuneCartViewController.php +++ b/src/applications/phortune/controller/PhortuneCartViewController.php @@ -13,6 +13,11 @@ final class PhortuneCartViewController $request = $this->getRequest(); $viewer = $request->getUser(); + $authority = $this->loadMerchantAuthority(); + + // TODO: This (and the rest of the Cart controllers) need to be updated + // to use merchant URIs and merchant authority. + $cart = id(new PhortuneCartQuery()) ->setViewer($viewer) ->withIDs(array($this->id)) @@ -157,7 +162,11 @@ final class PhortuneCartViewController $account = $cart->getAccount(); $crumbs = $this->buildApplicationCrumbs(); - $this->addAccountCrumb($crumbs, $cart->getAccount()); + if ($authority) { + $this->addMerchantCrumb($crumbs, $authority); + } else { + $this->addAccountCrumb($crumbs, $cart->getAccount()); + } $crumbs->addTextCrumb(pht('Cart %d', $cart->getID())); $timeline = $this->buildTransactionTimeline( diff --git a/src/applications/phortune/controller/PhortuneController.php b/src/applications/phortune/controller/PhortuneController.php index 2900a3e850..655dcee4e1 100644 --- a/src/applications/phortune/controller/PhortuneController.php +++ b/src/applications/phortune/controller/PhortuneController.php @@ -84,4 +84,30 @@ abstract class PhortuneController extends PhabricatorController { return $providers; } + protected function loadMerchantAuthority() { + $request = $this->getRequest(); + $viewer = $this->getViewer(); + + $is_merchant = (bool)$request->getURIData('merchantID'); + if (!$is_merchant) { + return null; + } + + $merchant = id(new PhortuneMerchantQuery()) + ->setViewer($viewer) + ->withIDs(array($request->getURIData('merchantID'))) + ->requireCapabilities( + array( + PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, + )) + ->executeOne(); + if (!$merchant) { + return null; + } + + $viewer->grantAuthority($merchant); + return $merchant; + } + } diff --git a/src/applications/phortune/controller/PhortuneSubscriptionListController.php b/src/applications/phortune/controller/PhortuneSubscriptionListController.php index 06fb1115a4..4fbdb804c3 100644 --- a/src/applications/phortune/controller/PhortuneSubscriptionListController.php +++ b/src/applications/phortune/controller/PhortuneSubscriptionListController.php @@ -36,6 +36,7 @@ final class PhortuneSubscriptionListController return new Aphront404Response(); } $this->merchant = $merchant; + $viewer->grantAuthority($merchant); $engine->setMerchant($merchant); } else if ($this->accountID) { $account = id(new PhortuneAccountQuery()) diff --git a/src/applications/phortune/controller/PhortuneSubscriptionViewController.php b/src/applications/phortune/controller/PhortuneSubscriptionViewController.php index d9483271e0..e3c5d3e759 100644 --- a/src/applications/phortune/controller/PhortuneSubscriptionViewController.php +++ b/src/applications/phortune/controller/PhortuneSubscriptionViewController.php @@ -5,6 +5,8 @@ final class PhortuneSubscriptionViewController extends PhortuneController { public function handleRequest(AphrontRequest $request) { $viewer = $this->getViewer(); + $is_merchant = (bool)$this->loadMerchantAuthority(); + $subscription = id(new PhortuneSubscriptionQuery()) ->setViewer($viewer) ->withIDs(array($request->getURIData('id'))) @@ -19,7 +21,6 @@ final class PhortuneSubscriptionViewController extends PhortuneController { $subscription, PhabricatorPolicyCapability::CAN_EDIT); - $is_merchant = (bool)$request->getURIData('merchantID'); $merchant = $subscription->getMerchant(); $account = $subscription->getAccount(); diff --git a/src/applications/phortune/query/PhortuneCartQuery.php b/src/applications/phortune/query/PhortuneCartQuery.php index df2003949c..e324d3f7c0 100644 --- a/src/applications/phortune/query/PhortuneCartQuery.php +++ b/src/applications/phortune/query/PhortuneCartQuery.php @@ -91,6 +91,10 @@ final class PhortuneCartQuery $cart->attachAccount($account); } + if (!$carts) { + return array(); + } + $merchants = id(new PhortuneMerchantQuery()) ->setViewer($this->getViewer()) ->withPHIDs(mpull($carts, 'getMerchantPHID')) @@ -106,6 +110,10 @@ final class PhortuneCartQuery $cart->attachMerchant($merchant); } + if (!$carts) { + return array(); + } + $implementations = array(); $cart_map = mgroup($carts, 'getCartClass'); diff --git a/src/applications/phortune/query/PhortuneCartSearchEngine.php b/src/applications/phortune/query/PhortuneCartSearchEngine.php index 72f4fa3469..83ef32b526 100644 --- a/src/applications/phortune/query/PhortuneCartSearchEngine.php +++ b/src/applications/phortune/query/PhortuneCartSearchEngine.php @@ -166,8 +166,21 @@ final class PhortuneCartSearchEngine foreach ($carts as $cart) { $merchant = $cart->getMerchant(); + if ($this->getMerchant()) { + $href = $this->getApplicationURI( + 'merchant/'.$merchant->getID().'/cart/'.$cart->getID().'/'); + } else { + $href = $cart->getDetailURI(); + } + $rows[] = array( $cart->getID(), + phutil_tag( + 'a', + array( + 'href' => $href, + ), + $cart->getName()), $handles[$cart->getPHID()]->renderLink(), $handles[$merchant->getPHID()]->renderLink(), $handles[$cart->getAuthorPHID()]->renderLink(), diff --git a/src/applications/phortune/query/PhortunePaymentMethodQuery.php b/src/applications/phortune/query/PhortunePaymentMethodQuery.php index 87455e4bd9..c6b1fd878a 100644 --- a/src/applications/phortune/query/PhortunePaymentMethodQuery.php +++ b/src/applications/phortune/query/PhortunePaymentMethodQuery.php @@ -65,6 +65,10 @@ final class PhortunePaymentMethodQuery $method->attachAccount($account); } + if (!$methods) { + return $methods; + } + $merchants = id(new PhortuneMerchantQuery()) ->setViewer($this->getViewer()) ->withPHIDs(mpull($methods, 'getMerchantPHID')) @@ -80,6 +84,10 @@ final class PhortunePaymentMethodQuery $method->attachMerchant($merchant); } + if (!$methods) { + return $methods; + } + $provider_configs = id(new PhortunePaymentProviderConfigQuery()) ->setViewer($this->getViewer()) ->withPHIDs(mpull($methods, 'getProviderPHID')) diff --git a/src/applications/phortune/storage/PhortuneAccount.php b/src/applications/phortune/storage/PhortuneAccount.php index c53aa002e0..facb9d5089 100644 --- a/src/applications/phortune/storage/PhortuneAccount.php +++ b/src/applications/phortune/storage/PhortuneAccount.php @@ -133,10 +133,6 @@ final class PhortuneAccount extends PhortuneDAO public function getPolicy($capability) { switch ($capability) { case PhabricatorPolicyCapability::CAN_VIEW: - // Accounts are technically visible to all users, because merchant - // controllers need to be able to see accounts in order to process - // orders. We lock things down more tightly at the application level. - return PhabricatorPolicies::POLICY_USER; case PhabricatorPolicyCapability::CAN_EDIT: if ($this->getPHID() === null) { // Allow a user to create an account for themselves. @@ -149,7 +145,21 @@ final class PhortuneAccount extends PhortuneDAO public function hasAutomaticCapability($capability, PhabricatorUser $viewer) { $members = array_fuse($this->getMemberPHIDs()); - return isset($members[$viewer->getPHID()]); + if (isset($members[$viewer->getPHID()])) { + return true; + } + + // If the viewer is acting on behalf of a merchant, they can see + // payment accounts. + if ($capability == PhabricatorPolicyCapability::CAN_VIEW) { + foreach ($viewer->getAuthorities() as $authority) { + if ($authority instanceof PhortuneMerchant) { + return true; + } + } + } + + return false; } public function describeAutomaticCapability($capability) {