From 0a0ac1302f0234249707fcfb13b255fda574e5ba Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 2 Mar 2017 12:05:48 -0800 Subject: [PATCH] Prevent users from taking "edit"-like actions via comment forms if they don't have edit permission Summary: Ref T12335. Fixes T11207. Edit-like interactions which are not performed via "Edit " are a bit of a grey area, policy-wise. For example, you can correctly do these things to an object you can't edit: - Comment on it. - Award tokens. - Subscribe or unsubscribe. - Subscribe other users by mentioning them. - Perform review. - Perform audit. - (Maybe some other stuff.) These behaviors are all desirable and correct. But, particularly now that we offer stacked actions, you can do a bunch of other stuff which you shouldn't really be able to, like changing the status and priority of tasks you can't edit, as long as you submit the change via the comment form. (Before the advent of stacked actions there were fewer things you could do via the comment form, and more of them were very "grey area", especially since "Change Subscribers" was just "Add Subscribers", which you can do via mentions.) This isn't too much of a problem in practice because we won't //show// you those actions if the edit form you'd end up on doesn't have those fields. So on intalls like ours where we've created simple + advanced flows, users who shouldn't be changing task priorities generally don't see an option to do so, even though they technically could if they mucked with the HTML. Change this behavior to be more strict: unless an action explicitly says that it doesn't need edit permission (comment, review, audit) don't show it to users who don't have edit permission and don't let them take the action. Test Plan: - As a user who could not edit a task, tried to change status via comment form; received policy exception. - As a user who could not edit a task, viewed a comment form: no actions available (just "comment"). - As a user who could not edit a revision, viewed a revision form: only "review" actions available (accept, resign, etc). - Viewed a commit form but these are kind of moot because there's no separate edit permission. Reviewers: chad Reviewed By: chad Maniphest Tasks: T12335, T11207 Differential Revision: https://secure.phabricator.com/D17452 --- .../DifferentialRevisionActionTransaction.php | 6 ++++ .../DiffusionCommitActionTransaction.php | 6 ++++ .../editengine/PhabricatorEditEngine.php | 31 +++++++++++++++++++ .../editfield/PhabricatorEditField.php | 10 ++++++ .../PhabricatorCommentEditEngineExtension.php | 5 +++ 5 files changed, 58 insertions(+) diff --git a/src/applications/differential/xaction/DifferentialRevisionActionTransaction.php b/src/applications/differential/xaction/DifferentialRevisionActionTransaction.php index 5507d1309d..82198083f8 100644 --- a/src/applications/differential/xaction/DifferentialRevisionActionTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionActionTransaction.php @@ -74,9 +74,15 @@ abstract class DifferentialRevisionActionTransaction DifferentialRevision $revision, PhabricatorUser $viewer) { + // Actions in the "review" group, like "Accept Revision", do not require + // that the actor be able to edit the revision. + $group_review = DifferentialRevisionEditEngine::ACTIONGROUP_REVIEW; + $is_review = ($this->getRevisionActionGroupKey() == $group_review); + $field = id(new PhabricatorApplyEditField()) ->setKey($this->getRevisionActionKey()) ->setTransactionType($this->getTransactionTypeConstant()) + ->setCanApplyWithoutEditCapability($is_review) ->setValue(true); if ($this->isActionAvailable($revision, $viewer)) { diff --git a/src/applications/diffusion/xaction/DiffusionCommitActionTransaction.php b/src/applications/diffusion/xaction/DiffusionCommitActionTransaction.php index 4756f914e3..d2e5f17ccb 100644 --- a/src/applications/diffusion/xaction/DiffusionCommitActionTransaction.php +++ b/src/applications/diffusion/xaction/DiffusionCommitActionTransaction.php @@ -70,9 +70,15 @@ abstract class DiffusionCommitActionTransaction PhabricatorRepositoryCommit $commit, PhabricatorUser $viewer) { + // Actions in the "audit" group, like "Accept Commit", do not require + // that the actor be able to edit the commit. + $group_audit = DiffusionCommitEditEngine::ACTIONGROUP_AUDIT; + $is_audit = ($this->getCommitActionGroupKey() == $group_audit); + $field = id(new PhabricatorApplyEditField()) ->setKey($this->getCommitActionKey()) ->setTransactionType($this->getTransactionTypeConstant()) + ->setCanApplyWithoutEditCapability($is_audit) ->setValue(true); if ($this->isActionAvailable($commit, $viewer)) { diff --git a/src/applications/transactions/editengine/PhabricatorEditEngine.php b/src/applications/transactions/editengine/PhabricatorEditEngine.php index 4d1bdac921..9d769c0e66 100644 --- a/src/applications/transactions/editengine/PhabricatorEditEngine.php +++ b/src/applications/transactions/editengine/PhabricatorEditEngine.php @@ -1586,12 +1586,23 @@ abstract class PhabricatorEditEngine $fields = $this->buildEditFields($object); + $can_edit = PhabricatorPolicyFilter::hasCapability( + $viewer, + $object, + PhabricatorPolicyCapability::CAN_EDIT); + $comment_actions = array(); foreach ($fields as $field) { if (!$field->shouldGenerateTransactionsFromComment()) { continue; } + if (!$can_edit) { + if (!$field->getCanApplyWithoutEditCapability()) { + continue; + } + } + $comment_action = $field->getCommentAction(); if (!$comment_action) { continue; @@ -1812,6 +1823,11 @@ abstract class PhabricatorEditEngine $xactions = array(); + $can_edit = PhabricatorPolicyFilter::hasCapability( + $viewer, + $object, + PhabricatorPolicyCapability::CAN_EDIT); + if ($actions) { $action_map = array(); foreach ($actions as $action) { @@ -1834,6 +1850,21 @@ abstract class PhabricatorEditEngine continue; } + // If you don't have edit permission on the object, you're limited in + // which actions you can take via the comment form. Most actions + // need edit permission, but some actions (like "Accept Revision") + // can be applied by anyone with view permission. + if (!$can_edit) { + if (!$field->getCanApplyWithoutEditCapability()) { + // We know the user doesn't have the capability, so this will + // raise a policy exception. + PhabricatorPolicyFilter::requireCapability( + $viewer, + $object, + PhabricatorPolicyCapability::CAN_EDIT); + } + } + if (array_key_exists('initialValue', $action)) { $field->setInitialValue($action['initialValue']); } diff --git a/src/applications/transactions/editfield/PhabricatorEditField.php b/src/applications/transactions/editfield/PhabricatorEditField.php index 2b0c93568b..3cc6fe2fff 100644 --- a/src/applications/transactions/editfield/PhabricatorEditField.php +++ b/src/applications/transactions/editfield/PhabricatorEditField.php @@ -36,6 +36,7 @@ abstract class PhabricatorEditField extends Phobject { private $isEditDefaults; private $isSubmittedForm; private $controlError; + private $canApplyWithoutEditCapability = false; private $isReorderable = true; private $isDefaultable = true; @@ -292,6 +293,15 @@ abstract class PhabricatorEditField extends Phobject { return $this->controlInstructions; } + public function setCanApplyWithoutEditCapability($can_apply) { + $this->canApplyWithoutEditCapability = $can_apply; + return $this; + } + + public function getCanApplyWithoutEditCapability() { + return $this->canApplyWithoutEditCapability; + } + protected function newControl() { throw new PhutilMethodNotImplementedException(); } diff --git a/src/applications/transactions/engineextension/PhabricatorCommentEditEngineExtension.php b/src/applications/transactions/engineextension/PhabricatorCommentEditEngineExtension.php index 7eb2aee037..3a01756ea7 100644 --- a/src/applications/transactions/engineextension/PhabricatorCommentEditEngineExtension.php +++ b/src/applications/transactions/engineextension/PhabricatorCommentEditEngineExtension.php @@ -39,6 +39,10 @@ final class PhabricatorCommentEditEngineExtension $comment_type = PhabricatorTransactions::TYPE_COMMENT; + // Comments have a lot of special behavior which doesn't always check + // this flag, but we set it for consistency. + $is_interact = true; + $comment_field = id(new PhabricatorCommentEditField()) ->setKey(self::EDITKEY) ->setLabel(pht('Comments')) @@ -47,6 +51,7 @@ final class PhabricatorCommentEditEngineExtension ->setIsReorderable(false) ->setIsDefaultable(false) ->setIsLockable(false) + ->setCanApplyWithoutEditCapability($is_interact) ->setTransactionType($comment_type) ->setConduitDescription(pht('Make comments.')) ->setConduitTypeDescription(