diff --git a/src/applications/subscriptions/controller/PhabricatorSubscriptionsEditController.php b/src/applications/subscriptions/controller/PhabricatorSubscriptionsEditController.php --- a/src/applications/subscriptions/controller/PhabricatorSubscriptionsEditController.php +++ b/src/applications/subscriptions/controller/PhabricatorSubscriptionsEditController.php @@ -47,15 +47,6 @@ $handle->getURI()); } - if (!PhabricatorPolicyFilter::canInteract($viewer, $object)) { - $lock = PhabricatorEditEngineLock::newForObject($viewer, $object); - - $dialog = $this->newDialog() - ->addCancelButton($handle->getURI()); - - return $lock->willBlockUserInteractionWithDialog($dialog); - } - if ($object instanceof PhabricatorApplicationTransactionInterface) { if ($is_add) { $xaction_value = array( diff --git a/src/applications/subscriptions/events/PhabricatorSubscriptionsUIEventListener.php b/src/applications/subscriptions/events/PhabricatorSubscriptionsUIEventListener.php --- a/src/applications/subscriptions/events/PhabricatorSubscriptionsUIEventListener.php +++ b/src/applications/subscriptions/events/PhabricatorSubscriptionsUIEventListener.php @@ -73,24 +73,20 @@ ->setName(pht('Automatically Subscribed')) ->setIcon('fa-check-circle lightgreytext'); } else { - $can_interact = PhabricatorPolicyFilter::canInteract($user, $object); - if ($is_subscribed) { $sub_action = id(new PhabricatorActionView()) ->setWorkflow(true) ->setRenderAsForm(true) ->setHref('/subscriptions/delete/'.$object->getPHID().'/') ->setName(pht('Unsubscribe')) - ->setIcon('fa-minus-circle') - ->setDisabled(!$can_interact); + ->setIcon('fa-minus-circle'); } else { $sub_action = id(new PhabricatorActionView()) ->setWorkflow(true) ->setRenderAsForm(true) ->setHref('/subscriptions/add/'.$object->getPHID().'/') ->setName(pht('Subscribe')) - ->setIcon('fa-plus-circle') - ->setDisabled(!$can_interact); + ->setIcon('fa-plus-circle'); } if (!$user->isLoggedIn()) { diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -1648,9 +1648,48 @@ // don't enforce it here. return null; case PhabricatorTransactions::TYPE_SUBSCRIBERS: - // TODO: Removing subscribers other than yourself should probably - // require CAN_EDIT permission. You can do this via the API but - // generally can not via the web interface. + // Anyone can subscribe to or unsubscribe from anything they can view, + // with no other permissions. + + $old = array_fuse($xaction->getOldValue()); + $new = array_fuse($xaction->getNewValue()); + + // To remove users other than yourself, you must be able to edit the + // object. + $rem = array_diff_key($old, $new); + foreach ($rem as $phid) { + if ($phid !== $this->getActingAsPHID()) { + return PhabricatorPolicyCapability::CAN_EDIT; + } + } + + // To add users other than yourself, you must be able to interact. + // This allows "@mentioning" users to work as long as you can comment + // on objects. + + // If you can edit, we return that policy instead so that you can + // override a soft lock and still make edits. + + // TODO: This is a little bit hacky. We really want to be able to say + // "this requires either interact or edit", but there's currently no + // way to specify this kind of requirement. + + $can_edit = PhabricatorPolicyFilter::hasCapability( + $this->getActor(), + $this->object, + PhabricatorPolicyCapability::CAN_EDIT); + + $add = array_diff_key($new, $old); + foreach ($add as $phid) { + if ($phid !== $this->getActingAsPHID()) { + if ($can_edit) { + return PhabricatorPolicyCapability::CAN_EDIT; + } else { + return PhabricatorPolicyCapability::CAN_INTERACT; + } + } + } + return null; case PhabricatorTransactions::TYPE_TOKEN: // TODO: This technically requires CAN_INTERACT, like comments.