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 <object>" 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

epriestley 9 years ago 0a0ac130 08b18ac5

+58

5 changed files

expand all

src

applications

differential

xaction

DifferentialRevisionActionTransaction.php

diffusion

xaction

DiffusionCommitActionTransaction.php

transactions

editengine

PhabricatorEditEngine.php

editfield

PhabricatorEditField.php

engineextension

PhabricatorCommentEditEngineExtension.php

src/applications/differential/xaction/DifferentialRevisionActionTransaction.php

··· 74 74 DifferentialRevision $revision, 75 75 PhabricatorUser $viewer) { 76 76 77 + // Actions in the "review" group, like "Accept Revision", do not require 78 + // that the actor be able to edit the revision. 79 + $group_review = DifferentialRevisionEditEngine::ACTIONGROUP_REVIEW; 80 + $is_review = ($this->getRevisionActionGroupKey() == $group_review); 81 + 77 82 $field = id(new PhabricatorApplyEditField()) 78 83 ->setKey($this->getRevisionActionKey()) 79 84 ->setTransactionType($this->getTransactionTypeConstant()) 85 + ->setCanApplyWithoutEditCapability($is_review) 80 86 ->setValue(true); 81 87 82 88 if ($this->isActionAvailable($revision, $viewer)) {

src/applications/diffusion/xaction/DiffusionCommitActionTransaction.php

··· 70 70 PhabricatorRepositoryCommit $commit, 71 71 PhabricatorUser $viewer) { 72 72 73 + // Actions in the "audit" group, like "Accept Commit", do not require 74 + // that the actor be able to edit the commit. 75 + $group_audit = DiffusionCommitEditEngine::ACTIONGROUP_AUDIT; 76 + $is_audit = ($this->getCommitActionGroupKey() == $group_audit); 77 + 73 78 $field = id(new PhabricatorApplyEditField()) 74 79 ->setKey($this->getCommitActionKey()) 75 80 ->setTransactionType($this->getTransactionTypeConstant()) 81 + ->setCanApplyWithoutEditCapability($is_audit) 76 82 ->setValue(true); 77 83 78 84 if ($this->isActionAvailable($commit, $viewer)) {

+31

src/applications/transactions/editengine/PhabricatorEditEngine.php

··· 1586 1586 1587 1587 $fields = $this->buildEditFields($object); 1588 1588 1589 + $can_edit = PhabricatorPolicyFilter::hasCapability( 1590 + $viewer, 1591 + $object, 1592 + PhabricatorPolicyCapability::CAN_EDIT); 1593 + 1589 1594 $comment_actions = array(); 1590 1595 foreach ($fields as $field) { 1591 1596 if (!$field->shouldGenerateTransactionsFromComment()) { 1592 1597 continue; 1598 + } 1599 + 1600 + if (!$can_edit) { 1601 + if (!$field->getCanApplyWithoutEditCapability()) { 1602 + continue; 1603 + } 1593 1604 } 1594 1605 1595 1606 $comment_action = $field->getCommentAction(); ··· 1812 1823 1813 1824 $xactions = array(); 1814 1825 1826 + $can_edit = PhabricatorPolicyFilter::hasCapability( 1827 + $viewer, 1828 + $object, 1829 + PhabricatorPolicyCapability::CAN_EDIT); 1830 + 1815 1831 if ($actions) { 1816 1832 $action_map = array(); 1817 1833 foreach ($actions as $action) { ··· 1832 1848 1833 1849 if (!$field->shouldGenerateTransactionsFromComment()) { 1834 1850 continue; 1851 + } 1852 + 1853 + // If you don't have edit permission on the object, you're limited in 1854 + // which actions you can take via the comment form. Most actions 1855 + // need edit permission, but some actions (like "Accept Revision") 1856 + // can be applied by anyone with view permission. 1857 + if (!$can_edit) { 1858 + if (!$field->getCanApplyWithoutEditCapability()) { 1859 + // We know the user doesn't have the capability, so this will 1860 + // raise a policy exception. 1861 + PhabricatorPolicyFilter::requireCapability( 1862 + $viewer, 1863 + $object, 1864 + PhabricatorPolicyCapability::CAN_EDIT); 1865 + } 1835 1866 } 1836 1867 1837 1868 if (array_key_exists('initialValue', $action)) {

+10

src/applications/transactions/editfield/PhabricatorEditField.php

··· 36 36 private $isEditDefaults; 37 37 private $isSubmittedForm; 38 38 private $controlError; 39 + private $canApplyWithoutEditCapability = false; 39 40 40 41 private $isReorderable = true; 41 42 private $isDefaultable = true; ··· 290 291 291 292 public function getControlInstructions() { 292 293 return $this->controlInstructions; 294 + } 295 + 296 + public function setCanApplyWithoutEditCapability($can_apply) { 297 + $this->canApplyWithoutEditCapability = $can_apply; 298 + return $this; 299 + } 300 + 301 + public function getCanApplyWithoutEditCapability() { 302 + return $this->canApplyWithoutEditCapability; 293 303 } 294 304 295 305 protected function newControl() {

src/applications/transactions/engineextension/PhabricatorCommentEditEngineExtension.php

··· 39 39 40 40 $comment_type = PhabricatorTransactions::TYPE_COMMENT; 41 41 42 + // Comments have a lot of special behavior which doesn't always check 43 + // this flag, but we set it for consistency. 44 + $is_interact = true; 45 + 42 46 $comment_field = id(new PhabricatorCommentEditField()) 43 47 ->setKey(self::EDITKEY) 44 48 ->setLabel(pht('Comments')) ··· 47 51 ->setIsReorderable(false) 48 52 ->setIsDefaultable(false) 49 53 ->setIsLockable(false) 54 + ->setCanApplyWithoutEditCapability($is_interact) 50 55 ->setTransactionType($comment_type) 51 56 ->setConduitDescription(pht('Make comments.')) 52 57 ->setConduitTypeDescription(

Configure Feed

Configure Feed