From 78dddf39ba7200cb71303b167278161747fb6d1a Mon Sep 17 00:00:00 2001 From: Bob Trahan Date: Mon, 18 May 2015 13:07:05 -0700 Subject: [PATCH] Maniphest - prevent uneditable tasks from being able to be closed as duplicates Summary: Fixes T7923. Prevent the user from finding tasks that they can't edit in merge workflows. Also ensure that we query properly on final merge action just in case. Test Plan: Tried to find a task I couldn't edit in various searches under the "merge" dialogue and couldn't find the task. Removed this big of code and tried to merge in a task and after hitting "merge" observed the page reloaded with no task merged in. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin, epriestley Maniphest Tasks: T7923 Differential Revision: https://secure.phabricator.com/D12872 --- .../phid/query/PhabricatorHandleQuery.php | 14 +++++ .../PhabricatorSearchApplication.php | 2 +- .../PhabricatorSearchAttachController.php | 59 ++++++++----------- .../PhabricatorSearchBaseController.php | 6 ++ .../PhabricatorSearchSelectController.php | 35 ++++++----- .../query/PhabricatorSearchDocumentQuery.php | 14 +++++ 6 files changed, 80 insertions(+), 50 deletions(-) diff --git a/src/applications/phid/query/PhabricatorHandleQuery.php b/src/applications/phid/query/PhabricatorHandleQuery.php index 296116312b..2635b84298 100644 --- a/src/applications/phid/query/PhabricatorHandleQuery.php +++ b/src/applications/phid/query/PhabricatorHandleQuery.php @@ -3,6 +3,7 @@ final class PhabricatorHandleQuery extends PhabricatorCursorPagedPolicyAwareQuery { + private $objectCapabilities; private $phids = array(); public function withPHIDs(array $phids) { @@ -10,6 +11,18 @@ final class PhabricatorHandleQuery return $this; } + public function requireObjectCapabilities(array $capabilities) { + $this->objectCapabilities = $capabilities; + return $this; + } + + protected function getRequiredObjectCapabilities() { + if ($this->objectCapabilities) { + return $this->objectCapabilities; + } + return $this->getRequiredCapabilities(); + } + protected function loadPage() { $types = PhabricatorPHIDType::getAllTypes(); @@ -20,6 +33,7 @@ final class PhabricatorHandleQuery $object_query = id(new PhabricatorObjectQuery()) ->withPHIDs($phids) + ->requireCapabilities($this->getRequiredObjectCapabilities()) ->setViewer($this->getViewer()); $objects = $object_query->execute(); diff --git a/src/applications/search/application/PhabricatorSearchApplication.php b/src/applications/search/application/PhabricatorSearchApplication.php index 51d5e9780c..f350438616 100644 --- a/src/applications/search/application/PhabricatorSearchApplication.php +++ b/src/applications/search/application/PhabricatorSearchApplication.php @@ -32,7 +32,7 @@ final class PhabricatorSearchApplication extends PhabricatorApplication { '(?:query/(?P[^/]+)/)?' => 'PhabricatorSearchController', 'attach/(?P[^/]+)/(?P\w+)/(?:(?P\w+)/)?' => 'PhabricatorSearchAttachController', - 'select/(?P\w+)/' + 'select/(?P\w+)/(?:(?P\w+)/)?' => 'PhabricatorSearchSelectController', 'index/(?P[^/]+)/' => 'PhabricatorSearchIndexController', 'hovercard/(?Pretrieve|test)/' diff --git a/src/applications/search/controller/PhabricatorSearchAttachController.php b/src/applications/search/controller/PhabricatorSearchAttachController.php index e08fd7734c..b2e0a92b9d 100644 --- a/src/applications/search/controller/PhabricatorSearchAttachController.php +++ b/src/applications/search/controller/PhabricatorSearchAttachController.php @@ -3,38 +3,22 @@ final class PhabricatorSearchAttachController extends PhabricatorSearchBaseController { - private $phid; - private $type; - private $action; - - const ACTION_ATTACH = 'attach'; - const ACTION_MERGE = 'merge'; - const ACTION_DEPENDENCIES = 'dependencies'; - const ACTION_BLOCKS = 'blocks'; - const ACTION_EDGE = 'edge'; - - public function willProcessRequest(array $data) { - $this->phid = $data['phid']; - $this->type = $data['type']; - $this->action = idx($data, 'action', self::ACTION_ATTACH); - } - - public function processRequest() { - - $request = $this->getRequest(); - $user = $request->getUser(); + public function handleRequest(AphrontRequest $request) { + $user = $request->getUser(); + $phid = $request->getURIData('phid'); + $attach_type = $request->getURIData('type'); + $action = $request->getURIData('action', self::ACTION_ATTACH); $handle = id(new PhabricatorHandleQuery()) ->setViewer($user) - ->withPHIDs(array($this->phid)) + ->withPHIDs(array($phid)) ->executeOne(); $object_type = $handle->getType(); - $attach_type = $this->type; $object = id(new PhabricatorObjectQuery()) ->setViewer($user) - ->withPHIDs(array($this->phid)) + ->withPHIDs(array($phid)) ->executeOne(); if (!$object) { @@ -42,7 +26,7 @@ final class PhabricatorSearchAttachController } $edge_type = null; - switch ($this->action) { + switch ($action) { case self::ACTION_EDGE: case self::ACTION_DEPENDENCIES: case self::ACTION_BLOCKS: @@ -66,7 +50,7 @@ final class PhabricatorSearchAttachController } $old_phids = PhabricatorEdgeQuery::loadDestinationPHIDs( - $this->phid, + $phid, $edge_type); $add_phids = $phids; $rem_phids = array_diff($old_phids, $add_phids); @@ -100,7 +84,7 @@ final class PhabricatorSearchAttachController } else { if ($edge_type) { $phids = PhabricatorEdgeQuery::loadDestinationPHIDs( - $this->phid, + $phid, $edge_type); } else { // This is a merge. @@ -108,7 +92,7 @@ final class PhabricatorSearchAttachController } } - $strings = $this->getStrings(); + $strings = $this->getStrings($attach_type, $action); $handles = $this->loadViewerHandles($phids); @@ -116,11 +100,11 @@ final class PhabricatorSearchAttachController $obj_dialog ->setUser($user) ->setHandles($handles) - ->setFilters($this->getFilters($strings)) + ->setFilters($this->getFilters($strings, $attach_type)) ->setSelectedFilter($strings['selected']) - ->setExcluded($this->phid) + ->setExcluded($phid) ->setCancelURI($handle->getURI()) - ->setSearchURI('/search/select/'.$attach_type.'/') + ->setSearchURI('/search/select/'.$attach_type.'/'.$action.'/') ->setTitle($strings['title']) ->setHeader($strings['header']) ->setButtonText($strings['button']) @@ -148,6 +132,11 @@ final class PhabricatorSearchAttachController $targets = id(new ManiphestTaskQuery()) ->setViewer($user) + ->requireCapabilities( + array( + PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, + )) ->withPHIDs(array_keys($phids)) ->needSubscriberPHIDs(true) ->needProjectPHIDs(true) @@ -208,8 +197,8 @@ final class PhabricatorSearchAttachController return $response; } - private function getStrings() { - switch ($this->type) { + private function getStrings($attach_type, $action) { + switch ($attach_type) { case DifferentialRevisionPHIDType::TYPECONST: $noun = 'Revisions'; $selected = 'created'; @@ -228,7 +217,7 @@ final class PhabricatorSearchAttachController break; } - switch ($this->action) { + switch ($action) { case self::ACTION_EDGE: case self::ACTION_ATTACH: $dialog_title = "Manage Attached {$noun}"; @@ -268,8 +257,8 @@ final class PhabricatorSearchAttachController ); } - private function getFilters(array $strings) { - if ($this->type == PholioMockPHIDType::TYPECONST) { + private function getFilters(array $strings, $attach_type) { + if ($attach_type == PholioMockPHIDType::TYPECONST) { $filters = array( 'created' => 'Created By Me', 'all' => 'All '.$strings['target_plural_noun'], diff --git a/src/applications/search/controller/PhabricatorSearchBaseController.php b/src/applications/search/controller/PhabricatorSearchBaseController.php index 446406dfea..86573b38bd 100644 --- a/src/applications/search/controller/PhabricatorSearchBaseController.php +++ b/src/applications/search/controller/PhabricatorSearchBaseController.php @@ -2,6 +2,12 @@ abstract class PhabricatorSearchBaseController extends PhabricatorController { + const ACTION_ATTACH = 'attach'; + const ACTION_MERGE = 'merge'; + const ACTION_DEPENDENCIES = 'dependencies'; + const ACTION_BLOCKS = 'blocks'; + const ACTION_EDGE = 'edge'; + public function buildStandardPageResponse($view, array $data) { $page = $this->buildStandardPageView(); diff --git a/src/applications/search/controller/PhabricatorSearchSelectController.php b/src/applications/search/controller/PhabricatorSearchSelectController.php index 03d3ca4c69..f663cd03d7 100644 --- a/src/applications/search/controller/PhabricatorSearchSelectController.php +++ b/src/applications/search/controller/PhabricatorSearchSelectController.php @@ -3,22 +3,17 @@ final class PhabricatorSearchSelectController extends PhabricatorSearchBaseController { - private $type; - - public function willProcessRequest(array $data) { - $this->type = $data['type']; - } - - public function processRequest() { - $request = $this->getRequest(); + public function handleRequest(AphrontRequest $request) { $user = $request->getUser(); + $type = $request->getURIData('type'); + $action = $request->getURIData('action'); $query = new PhabricatorSavedQuery(); $query_str = $request->getStr('query'); $query->setEngineClassName('PhabricatorSearchApplicationSearchEngine'); $query->setParameter('query', $query_str); - $query->setParameter('types', array($this->type)); + $query->setParameter('types', array($type)); $status_open = PhabricatorSearchRelationship::RELATIONSHIP_OPEN; @@ -31,7 +26,7 @@ final class PhabricatorSearchSelectController $query->setParameter('authorPHIDs', array($user->getPHID())); // TODO - if / when we allow pholio mocks to be archived, etc // update this - if ($this->type != PholioMockPHIDType::TYPECONST) { + if ($type != PholioMockPHIDType::TYPECONST) { $query->setParameter('statuses', array($status_open)); } break; @@ -42,15 +37,25 @@ final class PhabricatorSearchSelectController $query->setParameter('excludePHIDs', array($request->getStr('exclude'))); + $capabilities = array(PhabricatorPolicyCapability::CAN_VIEW); + switch ($action) { + case self::ACTION_MERGE: + $capabilities[] = PhabricatorPolicyCapability::CAN_EDIT; + break; + default: + break; + } + $results = id(new PhabricatorSearchDocumentQuery()) ->setViewer($user) + ->requireObjectCapabilities($capabilities) ->withSavedQuery($query) ->setOffset(0) ->setLimit(100) ->execute(); $phids = array_fill_keys(mpull($results, 'getPHID'), true); - $phids += $this->queryObjectNames($query_str); + $phids += $this->queryObjectNames($query_str, $capabilities); $phids = array_keys($phids); $handles = $this->loadViewerHandles($phids); @@ -64,12 +69,14 @@ final class PhabricatorSearchSelectController return id(new AphrontAjaxResponse())->setContent($data); } - private function queryObjectNames($query) { - $viewer = $this->getRequest()->getUser(); + private function queryObjectNames($query, $capabilities) { + $request = $this->getRequest(); + $viewer = $request->getUser(); $objects = id(new PhabricatorObjectQuery()) ->setViewer($viewer) - ->withTypes(array($this->type)) + ->requireCapabilities($capabilities) + ->withTypes(array($request->getURIData('type'))) ->withNames(array($query)) ->execute(); diff --git a/src/applications/search/query/PhabricatorSearchDocumentQuery.php b/src/applications/search/query/PhabricatorSearchDocumentQuery.php index b5591d8eca..903d40dd5a 100644 --- a/src/applications/search/query/PhabricatorSearchDocumentQuery.php +++ b/src/applications/search/query/PhabricatorSearchDocumentQuery.php @@ -4,17 +4,31 @@ final class PhabricatorSearchDocumentQuery extends PhabricatorCursorPagedPolicyAwareQuery { private $savedQuery; + private $objectCapabilities; public function withSavedQuery(PhabricatorSavedQuery $query) { $this->savedQuery = $query; return $this; } + public function requireObjectCapabilities(array $capabilities) { + $this->objectCapabilities = $capabilities; + return $this; + } + + protected function getRequiredObjectCapabilities() { + if ($this->objectCapabilities) { + return $this->objectCapabilities; + } + return $this->getRequiredCapabilities(); + } + protected function loadPage() { $phids = $this->loadDocumentPHIDsWithoutPolicyChecks(); $handles = id(new PhabricatorHandleQuery()) ->setViewer($this->getViewer()) + ->requireObjectCapabilities($this->getRequiredObjectCapabilities()) ->withPHIDs($phids) ->execute();