From 7fedfacbca27c165cf193f286cef9306cf9762e2 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 21 Oct 2013 16:59:06 -0700 Subject: [PATCH] Add capabilities for editing task triage details (priority, assignee, etc) Summary: This is primarily a client request, and a little bit use-case specific, but policies seem to be holding up well and I'm getting more comfortable about maintaining this. Much if it can run through ApplicationTransactions. Allow the ability to edit status, policies, priorities, assignees and projects of a task to be restricted to some subset of users. Also allow bulk edit to be locked. This affects the editor itself and the edit, view and list interfaces. Test Plan: As a restricted user, created, edited and commented on tasks. Tried to drag them around. Reviewers: btrahan Reviewed By: btrahan CC: aran Differential Revision: https://secure.phabricator.com/D7357 --- src/__phutil_library_map__.php | 12 ++ .../PhabricatorApplicationManiphest.php | 14 +- .../ManiphestCapabilityBulkEdit.php | 20 +++ .../ManiphestCapabilityEditAssign.php | 20 +++ .../ManiphestCapabilityEditPolicies.php | 20 +++ .../ManiphestCapabilityEditPriority.php | 20 +++ .../ManiphestCapabilityEditProjects.php | 20 +++ .../ManiphestCapabilityEditStatus.php | 20 +++ .../ManiphestBatchEditController.php | 3 + .../ManiphestTaskDetailController.php | 21 +++ .../ManiphestTaskEditController.php | 169 +++++++++++------- .../ManiphestTaskListController.php | 21 ++- .../editor/ManiphestTransactionEditor.php | 39 +++- ...habricatorApplicationTransactionEditor.php | 10 +- 14 files changed, 333 insertions(+), 76 deletions(-) create mode 100644 src/applications/maniphest/capability/ManiphestCapabilityBulkEdit.php create mode 100644 src/applications/maniphest/capability/ManiphestCapabilityEditAssign.php create mode 100644 src/applications/maniphest/capability/ManiphestCapabilityEditPolicies.php create mode 100644 src/applications/maniphest/capability/ManiphestCapabilityEditPriority.php create mode 100644 src/applications/maniphest/capability/ManiphestCapabilityEditProjects.php create mode 100644 src/applications/maniphest/capability/ManiphestCapabilityEditStatus.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index ae713a56fc..50af7d8448 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -705,8 +705,14 @@ phutil_register_library_map(array( 'LiskMigrationIterator' => 'infrastructure/storage/lisk/LiskMigrationIterator.php', 'LiskRawMigrationIterator' => 'infrastructure/storage/lisk/LiskRawMigrationIterator.php', 'ManiphestBatchEditController' => 'applications/maniphest/controller/ManiphestBatchEditController.php', + 'ManiphestCapabilityBulkEdit' => 'applications/maniphest/capability/ManiphestCapabilityBulkEdit.php', 'ManiphestCapabilityDefaultEdit' => 'applications/maniphest/capability/ManiphestCapabilityDefaultEdit.php', 'ManiphestCapabilityDefaultView' => 'applications/maniphest/capability/ManiphestCapabilityDefaultView.php', + 'ManiphestCapabilityEditAssign' => 'applications/maniphest/capability/ManiphestCapabilityEditAssign.php', + 'ManiphestCapabilityEditPolicies' => 'applications/maniphest/capability/ManiphestCapabilityEditPolicies.php', + 'ManiphestCapabilityEditPriority' => 'applications/maniphest/capability/ManiphestCapabilityEditPriority.php', + 'ManiphestCapabilityEditProjects' => 'applications/maniphest/capability/ManiphestCapabilityEditProjects.php', + 'ManiphestCapabilityEditStatus' => 'applications/maniphest/capability/ManiphestCapabilityEditStatus.php', 'ManiphestConfiguredCustomField' => 'applications/maniphest/field/ManiphestConfiguredCustomField.php', 'ManiphestConstants' => 'applications/maniphest/constants/ManiphestConstants.php', 'ManiphestController' => 'applications/maniphest/controller/ManiphestController.php', @@ -2837,8 +2843,14 @@ phutil_register_library_map(array( 'LiskMigrationIterator' => 'PhutilBufferedIterator', 'LiskRawMigrationIterator' => 'PhutilBufferedIterator', 'ManiphestBatchEditController' => 'ManiphestController', + 'ManiphestCapabilityBulkEdit' => 'PhabricatorPolicyCapability', 'ManiphestCapabilityDefaultEdit' => 'PhabricatorPolicyCapability', 'ManiphestCapabilityDefaultView' => 'PhabricatorPolicyCapability', + 'ManiphestCapabilityEditAssign' => 'PhabricatorPolicyCapability', + 'ManiphestCapabilityEditPolicies' => 'PhabricatorPolicyCapability', + 'ManiphestCapabilityEditPriority' => 'PhabricatorPolicyCapability', + 'ManiphestCapabilityEditProjects' => 'PhabricatorPolicyCapability', + 'ManiphestCapabilityEditStatus' => 'PhabricatorPolicyCapability', 'ManiphestConfiguredCustomField' => array( 0 => 'ManiphestCustomField', diff --git a/src/applications/maniphest/application/PhabricatorApplicationManiphest.php b/src/applications/maniphest/application/PhabricatorApplicationManiphest.php index 0bcf54bfc2..744c5c5113 100644 --- a/src/applications/maniphest/application/PhabricatorApplicationManiphest.php +++ b/src/applications/maniphest/application/PhabricatorApplicationManiphest.php @@ -67,7 +67,7 @@ final class PhabricatorApplicationManiphest extends PhabricatorApplication { 'export/(?P[^/]+)/' => 'ManiphestExportController', 'subpriority/' => 'ManiphestSubpriorityController', 'subscribe/(?Padd|rem)/(?P[1-9]\d*)/' - => 'ManiphestSubscribeController', + => 'ManiphestSubscribeController', ), ); } @@ -100,6 +100,18 @@ final class PhabricatorApplicationManiphest extends PhabricatorApplication { 'caption' => pht( 'Default edit policy for newly created tasks.'), ), + ManiphestCapabilityEditStatus::CAPABILITY => array( + ), + ManiphestCapabilityEditAssign::CAPABILITY => array( + ), + ManiphestCapabilityEditPolicies::CAPABILITY => array( + ), + ManiphestCapabilityEditPriority::CAPABILITY => array( + ), + ManiphestCapabilityEditProjects::CAPABILITY => array( + ), + ManiphestCapabilityBulkEdit::CAPABILITY => array( + ), ); } diff --git a/src/applications/maniphest/capability/ManiphestCapabilityBulkEdit.php b/src/applications/maniphest/capability/ManiphestCapabilityBulkEdit.php new file mode 100644 index 0000000000..4a27ead299 --- /dev/null +++ b/src/applications/maniphest/capability/ManiphestCapabilityBulkEdit.php @@ -0,0 +1,20 @@ +requireApplicationCapability( + ManiphestCapabilityBulkEdit::CAPABILITY); + $request = $this->getRequest(); $user = $request->getUser(); diff --git a/src/applications/maniphest/controller/ManiphestTaskDetailController.php b/src/applications/maniphest/controller/ManiphestTaskDetailController.php index 5d4b539552..af09505c61 100644 --- a/src/applications/maniphest/controller/ManiphestTaskDetailController.php +++ b/src/applications/maniphest/controller/ManiphestTaskDetailController.php @@ -165,6 +165,27 @@ final class ManiphestTaskDetailController extends ManiphestController { ManiphestTransaction::TYPE_PROJECTS => pht('Associate Projects'), ); + // Remove actions the user doesn't have permission to take. + + $requires = array( + ManiphestTransaction::TYPE_OWNER => + ManiphestCapabilityEditAssign::CAPABILITY, + ManiphestTransaction::TYPE_PRIORITY => + ManiphestCapabilityEditPriority::CAPABILITY, + ManiphestTransaction::TYPE_PROJECTS => + ManiphestCapabilityEditProjects::CAPABILITY, + ManiphestTransaction::TYPE_STATUS => + ManiphestCapabilityEditStatus::CAPABILITY, + ); + + foreach ($transaction_types as $type => $name) { + if (isset($requires[$type])) { + if (!$this->hasApplicationCapability($requires[$type])) { + unset($transaction_types[$type]); + } + } + } + if ($task->getStatus() == ManiphestTaskStatus::STATUS_OPEN) { $resolution_types = array_select_keys( $resolution_types, diff --git a/src/applications/maniphest/controller/ManiphestTaskEditController.php b/src/applications/maniphest/controller/ManiphestTaskEditController.php index 82da43a8bc..1b01a66dee 100644 --- a/src/applications/maniphest/controller/ManiphestTaskEditController.php +++ b/src/applications/maniphest/controller/ManiphestTaskEditController.php @@ -1,8 +1,5 @@ getRequest(); $user = $request->getUser(); + $can_edit_assign = $this->hasApplicationCapability( + ManiphestCapabilityEditAssign::CAPABILITY); + $can_edit_policies = $this->hasApplicationCapability( + ManiphestCapabilityEditPolicies::CAPABILITY); + $can_edit_priority = $this->hasApplicationCapability( + ManiphestCapabilityEditPriority::CAPABILITY); + $can_edit_projects = $this->hasApplicationCapability( + ManiphestCapabilityEditProjects::CAPABILITY); + $can_edit_status = $this->hasApplicationCapability( + ManiphestCapabilityEditStatus::CAPABILITY); + $files = array(); $parent_task = null; $template_id = null; @@ -40,20 +48,24 @@ final class ManiphestTaskEditController extends ManiphestController { if (!$request->isFormPost()) { $task->setTitle($request->getStr('title')); - $default_projects = $request->getStr('projects'); - if ($default_projects) { - $task->setProjectPHIDs(explode(';', $default_projects)); + if ($can_edit_projects) { + $default_projects = $request->getStr('projects'); + if ($default_projects) { + $task->setProjectPHIDs(explode(';', $default_projects)); + } } $task->setDescription($request->getStr('description')); - $assign = $request->getStr('assign'); - if (strlen($assign)) { - $assign_user = id(new PhabricatorUser())->loadOneWhere( - 'username = %s', - $assign); - if ($assign_user) { - $task->setOwnerPHID($assign_user->getPHID()); + if ($can_edit_assign) { + $assign = $request->getStr('assign'); + if (strlen($assign)) { + $assign_user = id(new PhabricatorUser())->loadOneWhere( + 'username = %s', + $assign); + if ($assign_user) { + $task->setOwnerPHID($assign_user->getPHID()); + } } } } @@ -122,7 +134,9 @@ final class ManiphestTaskEditController extends ManiphestController { $changes[ManiphestTransaction::TYPE_TITLE] = $new_title; $changes[ManiphestTransaction::TYPE_DESCRIPTION] = $new_desc; - $changes[ManiphestTransaction::TYPE_STATUS] = $new_status; + if ($can_edit_status) { + $changes[ManiphestTransaction::TYPE_STATUS] = $new_status; + } $owner_tokenizer = $request->getArr('assigned_to'); $owner_phid = reset($owner_tokenizer); @@ -173,17 +187,27 @@ final class ManiphestTaskEditController extends ManiphestController { $task->setProjectPHIDs($request->getArr('projects')); } else { - $changes[ManiphestTransaction::TYPE_PRIORITY] = - $request->getInt('priority'); - $changes[ManiphestTransaction::TYPE_OWNER] = $owner_phid; - $changes[ManiphestTransaction::TYPE_CCS] = $request->getArr('cc'); - $changes[ManiphestTransaction::TYPE_PROJECTS] = - $request->getArr('projects'); + if ($can_edit_priority) { + $changes[ManiphestTransaction::TYPE_PRIORITY] = + $request->getInt('priority'); + } + if ($can_edit_assign) { + $changes[ManiphestTransaction::TYPE_OWNER] = $owner_phid; + } - $changes[PhabricatorTransactions::TYPE_VIEW_POLICY] = - $request->getStr('viewPolicy'); - $changes[PhabricatorTransactions::TYPE_EDIT_POLICY] = - $request->getStr('editPolicy'); + $changes[ManiphestTransaction::TYPE_CCS] = $request->getArr('cc'); + + if ($can_edit_projects) { + $changes[ManiphestTransaction::TYPE_PROJECTS] = + $request->getArr('projects'); + } + + if ($can_edit_policies) { + $changes[PhabricatorTransactions::TYPE_VIEW_POLICY] = + $request->getStr('viewPolicy'); + $changes[PhabricatorTransactions::TYPE_EDIT_POLICY] = + $request->getStr('editPolicy'); + } if ($files) { $file_map = mpull($files, 'getPHID'); @@ -418,7 +442,7 @@ final class ManiphestTaskEditController extends ManiphestController { ->setHeight(AphrontFormTextAreaControl::HEIGHT_VERY_SHORT) ->setValue($task->getTitle())); - if ($task->getID()) { + if ($task->getID() && $can_edit_status) { // Only show this in "edit" mode, not "create" mode, since creating a // non-open task is kind of silly and it would just clutter up the // "create" interface. @@ -436,58 +460,73 @@ final class ManiphestTaskEditController extends ManiphestController { ->setObject($task) ->execute(); - $form - ->appendChild( + if ($can_edit_assign) { + $form->appendChild( id(new AphrontFormTokenizerControl()) ->setLabel(pht('Assigned To')) ->setName('assigned_to') ->setValue($assigned_value) ->setUser($user) ->setDatasource('/typeahead/common/users/') - ->setLimit(1)) + ->setLimit(1)); + } + + $form ->appendChild( id(new AphrontFormTokenizerControl()) ->setLabel(pht('CC')) ->setName('cc') ->setValue($cc_value) ->setUser($user) - ->setDatasource('/typeahead/common/mailable/')) - ->appendChild( - id(new AphrontFormSelectControl()) - ->setLabel(pht('Priority')) - ->setName('priority') - ->setOptions($priority_map) - ->setValue($task->getPriority())) - ->appendChild( - id(new AphrontFormPolicyControl()) - ->setUser($user) - ->setCapability(PhabricatorPolicyCapability::CAN_VIEW) - ->setPolicyObject($task) - ->setPolicies($policies) - ->setName('viewPolicy')) - ->appendChild( - id(new AphrontFormPolicyControl()) - ->setUser($user) - ->setCapability(PhabricatorPolicyCapability::CAN_EDIT) - ->setPolicyObject($task) - ->setPolicies($policies) - ->setName('editPolicy')) - ->appendChild( - id(new AphrontFormTokenizerControl()) - ->setLabel(pht('Projects')) - ->setName('projects') - ->setValue($projects_value) - ->setID($project_tokenizer_id) - ->setCaption( - javelin_tag( - 'a', - array( - 'href' => '/project/create/', - 'mustcapture' => true, - 'sigil' => 'project-create', - ), - pht('Create New Project'))) - ->setDatasource('/typeahead/common/projects/')); + ->setDatasource('/typeahead/common/mailable/')); + + if ($can_edit_priority) { + $form + ->appendChild( + id(new AphrontFormSelectControl()) + ->setLabel(pht('Priority')) + ->setName('priority') + ->setOptions($priority_map) + ->setValue($task->getPriority())); + } + + if ($can_edit_policies) { + $form + ->appendChild( + id(new AphrontFormPolicyControl()) + ->setUser($user) + ->setCapability(PhabricatorPolicyCapability::CAN_VIEW) + ->setPolicyObject($task) + ->setPolicies($policies) + ->setName('viewPolicy')) + ->appendChild( + id(new AphrontFormPolicyControl()) + ->setUser($user) + ->setCapability(PhabricatorPolicyCapability::CAN_EDIT) + ->setPolicyObject($task) + ->setPolicies($policies) + ->setName('editPolicy')); + } + + if ($can_edit_projects) { + $form + ->appendChild( + id(new AphrontFormTokenizerControl()) + ->setLabel(pht('Projects')) + ->setName('projects') + ->setValue($projects_value) + ->setID($project_tokenizer_id) + ->setCaption( + javelin_tag( + 'a', + array( + 'href' => '/project/create/', + 'mustcapture' => true, + 'sigil' => 'project-create', + ), + pht('Create New Project'))) + ->setDatasource('/typeahead/common/projects/')); + } foreach ($aux_fields as $aux_field) { $aux_control = $aux_field->renderEditControl(); diff --git a/src/applications/maniphest/controller/ManiphestTaskListController.php b/src/applications/maniphest/controller/ManiphestTaskListController.php index c4790ed4c2..dc3e71f80f 100644 --- a/src/applications/maniphest/controller/ManiphestTaskListController.php +++ b/src/applications/maniphest/controller/ManiphestTaskListController.php @@ -46,7 +46,11 @@ final class ManiphestTaskListController $group_parameter, $handles); + $can_edit_priority = $this->hasApplicationCapability( + ManiphestCapabilityEditPriority::CAPABILITY); + $can_drag = ($order_parameter == 'priority') && + ($can_edit_priority) && ($group_parameter == 'none' || $group_parameter == 'priority'); if (!$viewer->isLoggedIn()) { @@ -91,11 +95,13 @@ final class ManiphestTaskListController )); } - Javelin::initBehavior( - 'maniphest-subpriority-editor', - array( - 'uri' => '/maniphest/subpriority/', - )); + if ($can_drag) { + Javelin::initBehavior( + 'maniphest-subpriority-editor', + array( + 'uri' => '/maniphest/subpriority/', + )); + } return phutil_tag( 'div', @@ -191,6 +197,11 @@ final class ManiphestTaskListController private function renderBatchEditor(PhabricatorSavedQuery $saved_query) { $user = $this->getRequest()->getUser(); + $batch_capability = ManiphestCapabilityBulkEdit::CAPABILITY; + if (!$this->hasApplicationCapability($batch_capability)) { + return null; + } + if (!$user->isLoggedIn()) { // Don't show the batch editor or excel export for logged-out users. // Technically we //could// let them export, but ehh. diff --git a/src/applications/maniphest/editor/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/ManiphestTransactionEditor.php index 16dcef309b..8c5666b838 100644 --- a/src/applications/maniphest/editor/ManiphestTransactionEditor.php +++ b/src/applications/maniphest/editor/ManiphestTransactionEditor.php @@ -81,9 +81,9 @@ final class ManiphestTransactionEditor case ManiphestTransaction::TYPE_EDGE: return $xaction->getNewValue(); } - } + protected function transactionHasEffect( PhabricatorLiskDAO $object, PhabricatorApplicationTransaction $xaction) { @@ -249,6 +249,43 @@ final class ManiphestTransactionEditor } } + protected function requireCapabilities( + PhabricatorLiskDAO $object, + PhabricatorApplicationTransaction $xaction) { + + parent::requireCapabilities($object, $xaction); + + $app_capability_map = array( + ManiphestTransaction::TYPE_PRIORITY => + ManiphestCapabilityEditPriority::CAPABILITY, + ManiphestTransaction::TYPE_STATUS => + ManiphestCapabilityEditStatus::CAPABILITY, + ManiphestTransaction::TYPE_PROJECTS => + ManiphestCapabilityEditProjects::CAPABILITY, + ManiphestTransaction::TYPE_OWNER => + ManiphestCapabilityEditAssign::CAPABILITY, + PhabricatorTransactions::TYPE_EDIT_POLICY => + ManiphestCapabilityEditPolicies::CAPABILITY, + PhabricatorTransactions::TYPE_VIEW_POLICY => + ManiphestCapabilityEditPolicies::CAPABILITY, + ); + + $transaction_type = $xaction->getTransactionType(); + $app_capability = idx($app_capability_map, $transaction_type); + + if ($app_capability) { + $app = id(new PhabricatorApplicationQuery()) + ->setViewer($this->getActor()) + ->withClasses(array('PhabricatorApplicationManiphest')) + ->executeOne(); + PhabricatorPolicyFilter::requireCapability( + $this->getActor(), + $app, + $app_capability); + } + } + + public static function getNextSubpriority($pri, $sub) { // TODO: T603 Figure out what the policies here should be once this gets diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index 15241aff2d..d1a6e2a1ea 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -460,6 +460,12 @@ abstract class PhabricatorApplicationTransactionEditor return array(); } + // Now that we've merged, filtered, and combined transactions, check for + // required capabilities. + foreach ($xactions as $xaction) { + $this->requireCapabilities($object, $xaction); + } + $xactions = $this->sortTransactions($xactions); if ($is_preview) { @@ -696,10 +702,6 @@ abstract class PhabricatorApplicationTransactionEditor $actor, $object, PhabricatorPolicyCapability::CAN_VIEW); - - foreach ($xactions as $xaction) { - $this->requireCapabilities($object, $xaction); - } } protected function requireCapabilities(