diff --git a/src/applications/conpherence/editor/ConpherenceEditor.php b/src/applications/conpherence/editor/ConpherenceEditor.php index 3d451e78d3..3b4ad09e11 100644 --- a/src/applications/conpherence/editor/ConpherenceEditor.php +++ b/src/applications/conpherence/editor/ConpherenceEditor.php @@ -146,49 +146,6 @@ final class ConpherenceEditor extends PhabricatorApplicationTransactionEditor { return $xactions; } - protected function requireCapabilities( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - - parent::requireCapabilities($object, $xaction); - - switch ($xaction->getTransactionType()) { - case ConpherenceThreadParticipantsTransaction::TRANSACTIONTYPE: - $old_map = array_fuse($xaction->getOldValue()); - $new_map = array_fuse($xaction->getNewValue()); - - $add = array_keys(array_diff_key($new_map, $old_map)); - $rem = array_keys(array_diff_key($old_map, $new_map)); - - $actor_phid = $this->getActingAsPHID(); - - $is_join = (($add === array($actor_phid)) && !$rem); - $is_leave = (($rem === array($actor_phid)) && !$add); - - if ($is_join) { - // Anyone can join a thread they can see. - } else if ($is_leave) { - // Anyone can leave a thread. - } else { - // You need CAN_EDIT to add or remove participants. For additional - // discussion, see D17696 and T4411. - PhabricatorPolicyFilter::requireCapability( - $this->requireActor(), - $object, - PhabricatorPolicyCapability::CAN_EDIT); - } - break; - - case ConpherenceThreadTitleTransaction::TRANSACTIONTYPE: - case ConpherenceThreadTopicTransaction::TRANSACTIONTYPE: - PhabricatorPolicyFilter::requireCapability( - $this->requireActor(), - $object, - PhabricatorPolicyCapability::CAN_EDIT); - break; - } - } - protected function shouldSendMail( PhabricatorLiskDAO $object, array $xactions) { diff --git a/src/applications/conpherence/xaction/ConpherenceThreadParticipantsTransaction.php b/src/applications/conpherence/xaction/ConpherenceThreadParticipantsTransaction.php index 681c6d639c..c0e4c7c501 100644 --- a/src/applications/conpherence/xaction/ConpherenceThreadParticipantsTransaction.php +++ b/src/applications/conpherence/xaction/ConpherenceThreadParticipantsTransaction.php @@ -114,4 +114,34 @@ final class ConpherenceThreadParticipantsTransaction return $errors; } + public function getRequiredCapabilities( + $object, + PhabricatorApplicationTransaction $xaction) { + + $old_map = array_fuse($xaction->getOldValue()); + $new_map = array_fuse($xaction->getNewValue()); + + $add = array_keys(array_diff_key($new_map, $old_map)); + $rem = array_keys(array_diff_key($old_map, $new_map)); + + $actor_phid = $this->getActingAsPHID(); + + $is_join = (($add === array($actor_phid)) && !$rem); + $is_leave = (($rem === array($actor_phid)) && !$add); + + if ($is_join) { + // Anyone can join a thread they can see. + return null; + } + + if ($is_leave) { + // Anyone can leave a thread. + return null; + } + + // You need CAN_EDIT to add or remove participants. For additional + // discussion, see D17696 and T4411. + return PhabricatorPolicyCapability::CAN_EDIT; + } + } diff --git a/src/applications/project/editor/PhabricatorProjectTransactionEditor.php b/src/applications/project/editor/PhabricatorProjectTransactionEditor.php index ef0cc65c8e..581ec23153 100644 --- a/src/applications/project/editor/PhabricatorProjectTransactionEditor.php +++ b/src/applications/project/editor/PhabricatorProjectTransactionEditor.php @@ -115,58 +115,6 @@ final class PhabricatorProjectTransactionEditor return $errors; } - protected function requireCapabilities( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - - switch ($xaction->getTransactionType()) { - case PhabricatorTransactions::TYPE_EDGE: - switch ($xaction->getMetadataValue('edge:type')) { - case PhabricatorProjectProjectHasMemberEdgeType::EDGECONST: - $old = $xaction->getOldValue(); - $new = $xaction->getNewValue(); - - $add = array_keys(array_diff_key($new, $old)); - $rem = array_keys(array_diff_key($old, $new)); - - $actor_phid = $this->requireActor()->getPHID(); - - $is_join = (($add === array($actor_phid)) && !$rem); - $is_leave = (($rem === array($actor_phid)) && !$add); - - if ($is_join) { - // You need CAN_JOIN to join a project. - PhabricatorPolicyFilter::requireCapability( - $this->requireActor(), - $object, - PhabricatorPolicyCapability::CAN_JOIN); - } else if ($is_leave) { - // You usually don't need any capabilities to leave a project. - if ($object->getIsMembershipLocked()) { - // you must be able to edit though to leave locked projects - PhabricatorPolicyFilter::requireCapability( - $this->requireActor(), - $object, - PhabricatorPolicyCapability::CAN_EDIT); - } - } else { - if (!$this->getIsNewObject()) { - // You need CAN_EDIT to change members other than yourself. - // (PHI193) Just skip this check if we're creating a project. - PhabricatorPolicyFilter::requireCapability( - $this->requireActor(), - $object, - PhabricatorPolicyCapability::CAN_EDIT); - } - } - return; - } - break; - } - - return parent::requireCapabilities($object, $xaction); - } - protected function willPublish(PhabricatorLiskDAO $object, array $xactions) { // NOTE: We're using the omnipotent user here because the original actor // may no longer have permission to view the object. diff --git a/src/applications/subscriptions/controller/PhabricatorSubscriptionsMuteController.php b/src/applications/subscriptions/controller/PhabricatorSubscriptionsMuteController.php index 1369643ffc..e29d5e3802 100644 --- a/src/applications/subscriptions/controller/PhabricatorSubscriptionsMuteController.php +++ b/src/applications/subscriptions/controller/PhabricatorSubscriptionsMuteController.php @@ -44,8 +44,6 @@ final class PhabricatorSubscriptionsMuteController ); } - $muted_type = PhabricatorMutedByEdgeType::EDGECONST; - $xaction = id($object->getApplicationTransactionTemplate()) ->setTransactionType(PhabricatorTransactions::TYPE_EDGE) ->setMetadataValue('edge:type', $muted_type) diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index 7e98ebb364..1873fcb6e4 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -976,6 +976,27 @@ abstract class PhabricatorApplicationTransactionEditor $this->adjustTransactionValues($object, $xaction); } + // Now that we've merged and combined transactions, check for required + // capabilities. Note that we're doing this before filtering + // transactions: if you try to apply an edit which you do not have + // permission to apply, we want to give you a permissions error even + // if the edit would have no effect. + $this->applyCapabilityChecks($object, $xactions); + + // See T13186. Fatal hard if this object has an older + // "requireCapabilities()" method. The code may rely on this method being + // called to apply policy checks, so err on the side of safety and fatal. + // TODO: Remove this check after some time has passed. + if (method_exists($this, 'requireCapabilities')) { + throw new Exception( + pht( + 'Editor (of class "%s") implements obsolete policy method '. + 'requireCapabilities(). The implementation for this Editor '. + 'MUST be updated. See <%s> for discussion.', + get_class($this), + 'https://secure.phabricator.com/T13186')); + } + $xactions = $this->filterTransactions($object, $xactions); // TODO: Once everything is on EditEngine, just use getIsNewObject() to @@ -994,12 +1015,6 @@ abstract class PhabricatorApplicationTransactionEditor } } - // 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); $file_phids = $this->extractFilePHIDs($object, $xactions); @@ -1459,34 +1474,151 @@ abstract class PhabricatorApplicationTransactionEditor } } - protected function requireCapabilities( + private function applyCapabilityChecks( PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { + array $xactions) { + assert_instances_of($xactions, 'PhabricatorApplicationTransaction'); + + $can_edit = PhabricatorPolicyCapability::CAN_EDIT; if ($this->getIsNewObject()) { - return; + // If we're creating a new object, we don't need any special capabilities + // on the object. The actor has already made it through creation checks, + // and objects which haven't been created yet often can not be + // meaningfully tested for capabilities anyway. + $required_capabilities = array(); + } else { + if (!$xactions && !$this->xactions) { + // If we aren't doing anything, require CAN_EDIT to improve consistency. + $required_capabilities = array($can_edit); + } else { + $required_capabilities = array(); + + foreach ($xactions as $xaction) { + $type = $xaction->getTransactionType(); + + $xtype = $this->getModularTransactionType($type); + if (!$xtype) { + $capabilities = $this->getLegacyRequiredCapabilities($xaction); + } else { + $capabilities = $xtype->getRequiredCapabilities($object, $xaction); + } + + // For convenience, we allow flexibility in the return types because + // it's very unusual that a transaction actually requires multiple + // capability checks. + if ($capabilities === null) { + $capabilities = array(); + } else { + $capabilities = (array)$capabilities; + } + + foreach ($capabilities as $capability) { + $required_capabilities[$capability] = $capability; + } + } + } } - $actor = $this->requireActor(); - switch ($xaction->getTransactionType()) { - case PhabricatorTransactions::TYPE_COMMENT: - PhabricatorPolicyFilter::requireCapability( - $actor, - $object, - PhabricatorPolicyCapability::CAN_VIEW); - break; - case PhabricatorTransactions::TYPE_VIEW_POLICY: - case PhabricatorTransactions::TYPE_EDIT_POLICY: - case PhabricatorTransactions::TYPE_JOIN_POLICY: - case PhabricatorTransactions::TYPE_SPACE: - PhabricatorPolicyFilter::requireCapability( - $actor, - $object, - PhabricatorPolicyCapability::CAN_EDIT); - break; + $required_capabilities = array_fuse($required_capabilities); + $actor = $this->getActor(); + + if ($required_capabilities) { + id(new PhabricatorPolicyFilter()) + ->setViewer($actor) + ->requireCapabilities($required_capabilities) + ->raisePolicyExceptions(true) + ->apply(array($object)); } } + private function getLegacyRequiredCapabilities( + PhabricatorApplicationTransaction $xaction) { + + $type = $xaction->getTransactionType(); + switch ($type) { + case PhabricatorTransactions::TYPE_COMMENT: + // TODO: Comments technically require CAN_INTERACT, but this is + // currently somewhat special and handled through EditEngine. For now, + // 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. + return null; + case PhabricatorTransactions::TYPE_TOKEN: + // TODO: This technically requires CAN_INTERACT, like comments. + return null; + case PhabricatorTransactions::TYPE_HISTORY: + // This is a special magic transaction which sends you history via + // email and is only partially supported in the upstream. You don't + // need any capabilities to apply it. + return null; + case PhabricatorTransactions::TYPE_EDGE: + return $this->getLegacyRequiredEdgeCapabilities($xaction); + default: + // For other older (non-modular) transactions, always require exactly + // CAN_EDIT. Transactions which do not need CAN_EDIT or need additional + // capabilities must move to ModularTransactions. + return PhabricatorPolicyCapability::CAN_EDIT; + } + } + + private function getLegacyRequiredEdgeCapabilities( + PhabricatorApplicationTransaction $xaction) { + + // You don't need to have edit permission on an object to mention it or + // otherwise add a relationship pointing toward it. + if ($this->getIsInverseEdgeEditor()) { + return null; + } + + $edge_type = $xaction->getMetadataValue('edge:type'); + switch ($edge_type) { + case PhabricatorMutedByEdgeType::EDGECONST: + // At time of writing, you can only write this edge for yourself, so + // you don't need permissions. If you can eventually mute an object + // for other users, this would need to be revisited. + return null; + case PhabricatorObjectMentionsObjectEdgeType::EDGECONST: + return null; + case PhabricatorProjectProjectHasMemberEdgeType::EDGECONST: + $old = $xaction->getOldValue(); + $new = $xaction->getNewValue(); + + $add = array_keys(array_diff_key($new, $old)); + $rem = array_keys(array_diff_key($old, $new)); + + $actor_phid = $this->requireActor()->getPHID(); + + $is_join = (($add === array($actor_phid)) && !$rem); + $is_leave = (($rem === array($actor_phid)) && !$add); + + if ($is_join) { + // You need CAN_JOIN to join a project. + return PhabricatorPolicyCapability::CAN_JOIN; + } + + if ($is_leave) { + $object = $this->object; + // You usually don't need any capabilities to leave a project... + if ($object->getIsMembershipLocked()) { + // ...you must be able to edit to leave locked projects, though. + return PhabricatorPolicyCapability::CAN_EDIT; + } else { + return null; + } + } + + // You need CAN_EDIT to change members other than yourself. + return PhabricatorPolicyCapability::CAN_EDIT; + default: + return PhabricatorPolicyCapability::CAN_EDIT; + } + } + + private function buildSubscribeTransaction( PhabricatorLiskDAO $object, array $xactions, diff --git a/src/applications/transactions/storage/PhabricatorModularTransactionType.php b/src/applications/transactions/storage/PhabricatorModularTransactionType.php index b0714aeccf..3d81e02c38 100644 --- a/src/applications/transactions/storage/PhabricatorModularTransactionType.php +++ b/src/applications/transactions/storage/PhabricatorModularTransactionType.php @@ -366,4 +366,30 @@ abstract class PhabricatorModularTransactionType $capability); } + /** + * Get a list of capabilities the actor must have on the object to apply + * a transaction to it. + * + * Usually, you should use this to reduce capability requirements when a + * transaction (like leaving a Conpherence thread) can be applied without + * having edit permission on the object. You can override this method to + * remove the CAN_EDIT requirement, or to replace it with a different + * requirement. + * + * If you are increasing capability requirements and need to add an + * additional capability or policy requirement above and beyond CAN_EDIT, it + * is usually better implemented as a validation check. + * + * @param object Object being edited. + * @param PhabricatorApplicationTransaction Transaction being applied. + * @return null|const|list A capability constant (or list of + * capability constants) which the actor must have on the object. You can + * return `null` as a shorthand for "no capabilities are required". + */ + public function getRequiredCapabilities( + $object, + PhabricatorApplicationTransaction $xaction) { + return PhabricatorPolicyCapability::CAN_EDIT; + } + }