recaptime.dev / phorge
@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.) hq.recaptime.dev/wiki/Phorge
phorge phabricator
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

Make "Subscribe/Unsubscribe" require only "CAN_VIEW", not "CAN_INTERACT"

Summary:
Ref T13249. See PHI1059. Currently, Subscribe/Unsubscribe require CAN_INTERACT via the web UI and no permissions (i.e., effectively CAN_VIEW) via the API.

Weaken the requirements from the web UI so that you do not need "CAN_INTERACT". This is a product change to the effect that it's okay to subscribe/unsubscribe from anything you can see, even hard-locked tasks. This generally seems reasonable.

Increase the requirements for the actual transaction, which mostly applies to API changes:

- To remove subscribers other than yourself, require CAN_EDIT.
- To add subscribers other than yourself, require CAN_EDIT or CAN_INTERACT. You may have CAN_EDIT but not CAN_INTERACT on "soft locked" tasks. It's okay to click "Edit" on these, click "Yes, override lock", then remove subscribers other than yourself.

This technically plugs some weird, mostly theoretical holes in the API where "attackers" could sometimes make more subscription changes than they should have been able to. Now that we send you email when you're unsubscribed this could only really be used to be mildly mischievous, but no harm in making the policy enforcement more correct.

Test Plan: Against normal, soft-locked, and hard-locked tasks: subscribed, unsubscribed, added and removed subscribers, overrode locks, edited via API. Everything worked like it should and I couldn't find any combination of lock state, policy state, and edit pathway that did anything suspicious.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13249

Differential Revision: https://secure.phabricator.com/D20174

epriestley e44b40ca 8cf6c68c

+44 -18
-9
src/applications/subscriptions/controller/PhabricatorSubscriptionsEditController.php
··· 47 47 $handle->getURI()); 48 48 } 49 49 50 - if (!PhabricatorPolicyFilter::canInteract($viewer, $object)) { 51 - $lock = PhabricatorEditEngineLock::newForObject($viewer, $object); 52 - 53 - $dialog = $this->newDialog() 54 - ->addCancelButton($handle->getURI()); 55 - 56 - return $lock->willBlockUserInteractionWithDialog($dialog); 57 - } 58 - 59 50 if ($object instanceof PhabricatorApplicationTransactionInterface) { 60 51 if ($is_add) { 61 52 $xaction_value = array(
+2 -6
src/applications/subscriptions/events/PhabricatorSubscriptionsUIEventListener.php
··· 73 73 ->setName(pht('Automatically Subscribed')) 74 74 ->setIcon('fa-check-circle lightgreytext'); 75 75 } else { 76 - $can_interact = PhabricatorPolicyFilter::canInteract($user, $object); 77 - 78 76 if ($is_subscribed) { 79 77 $sub_action = id(new PhabricatorActionView()) 80 78 ->setWorkflow(true) 81 79 ->setRenderAsForm(true) 82 80 ->setHref('/subscriptions/delete/'.$object->getPHID().'/') 83 81 ->setName(pht('Unsubscribe')) 84 - ->setIcon('fa-minus-circle') 85 - ->setDisabled(!$can_interact); 82 + ->setIcon('fa-minus-circle'); 86 83 } else { 87 84 $sub_action = id(new PhabricatorActionView()) 88 85 ->setWorkflow(true) 89 86 ->setRenderAsForm(true) 90 87 ->setHref('/subscriptions/add/'.$object->getPHID().'/') 91 88 ->setName(pht('Subscribe')) 92 - ->setIcon('fa-plus-circle') 93 - ->setDisabled(!$can_interact); 89 + ->setIcon('fa-plus-circle'); 94 90 } 95 91 96 92 if (!$user->isLoggedIn()) {
+42 -3
src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
··· 1648 1648 // don't enforce it here. 1649 1649 return null; 1650 1650 case PhabricatorTransactions::TYPE_SUBSCRIBERS: 1651 - // TODO: Removing subscribers other than yourself should probably 1652 - // require CAN_EDIT permission. You can do this via the API but 1653 - // generally can not via the web interface. 1651 + // Anyone can subscribe to or unsubscribe from anything they can view, 1652 + // with no other permissions. 1653 + 1654 + $old = array_fuse($xaction->getOldValue()); 1655 + $new = array_fuse($xaction->getNewValue()); 1656 + 1657 + // To remove users other than yourself, you must be able to edit the 1658 + // object. 1659 + $rem = array_diff_key($old, $new); 1660 + foreach ($rem as $phid) { 1661 + if ($phid !== $this->getActingAsPHID()) { 1662 + return PhabricatorPolicyCapability::CAN_EDIT; 1663 + } 1664 + } 1665 + 1666 + // To add users other than yourself, you must be able to interact. 1667 + // This allows "@mentioning" users to work as long as you can comment 1668 + // on objects. 1669 + 1670 + // If you can edit, we return that policy instead so that you can 1671 + // override a soft lock and still make edits. 1672 + 1673 + // TODO: This is a little bit hacky. We really want to be able to say 1674 + // "this requires either interact or edit", but there's currently no 1675 + // way to specify this kind of requirement. 1676 + 1677 + $can_edit = PhabricatorPolicyFilter::hasCapability( 1678 + $this->getActor(), 1679 + $this->object, 1680 + PhabricatorPolicyCapability::CAN_EDIT); 1681 + 1682 + $add = array_diff_key($new, $old); 1683 + foreach ($add as $phid) { 1684 + if ($phid !== $this->getActingAsPHID()) { 1685 + if ($can_edit) { 1686 + return PhabricatorPolicyCapability::CAN_EDIT; 1687 + } else { 1688 + return PhabricatorPolicyCapability::CAN_INTERACT; 1689 + } 1690 + } 1691 + } 1692 + 1654 1693 return null; 1655 1694 case PhabricatorTransactions::TYPE_TOKEN: 1656 1695 // TODO: This technically requires CAN_INTERACT, like comments.