Page MenuHomePhabricator
Files F13180903

D19586.id46817.diff
No OneTemporary
Actions

D19586.id46817.diff
View Options

diff --git a/src/applications/conpherence/editor/ConpherenceEditor.php b/src/applications/conpherence/editor/ConpherenceEditor.php
--- a/src/applications/conpherence/editor/ConpherenceEditor.php
+++ b/src/applications/conpherence/editor/ConpherenceEditor.php
@@ -146,49 +146,6 @@
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
--- a/src/applications/conpherence/xaction/ConpherenceThreadParticipantsTransaction.php
+++ b/src/applications/conpherence/xaction/ConpherenceThreadParticipantsTransaction.php
@@ -114,4 +114,34 @@
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
--- a/src/applications/project/editor/PhabricatorProjectTransactionEditor.php
+++ b/src/applications/project/editor/PhabricatorProjectTransactionEditor.php
@@ -115,58 +115,6 @@
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/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
@@ -976,6 +976,13 @@
$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);
+
$xactions = $this->filterTransactions($object, $xactions);
// TODO: Once everything is on EditEngine, just use getIsNewObject() to
@@ -994,12 +1001,6 @@
}
}
- // 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 +1460,139 @@
}
}
- 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 don't 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.
+ // If we
+ $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()) {
+ $required_capabilities = array_fuse($required_capabilities);
+ $actor = $this->getActor();
+
+ if ($required_capabilities) {
+ id(new PhabricatorPolicyFilter())
+ ->setViewer($this->getActor())
+ ->requireCapabilities($required_capabilities)
+ ->raisePolicyExceptions(true)
+ ->apply(array($object));
+ }
+ }
+
+ private function getLegacyRequiredCapabilities(
+ PhabricatorApplicationTransaction $xaction) {
+
+ $type = $xaction->getTransactionType();
+ switch ($type) {
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;
+ // 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) {
+
+ $edge_type = $xaction->getMetadataValue('edge:type');
+ switch ($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.
+ 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
--- a/src/applications/transactions/storage/PhabricatorModularTransactionType.php
+++ b/src/applications/transactions/storage/PhabricatorModularTransactionType.php
@@ -366,4 +366,29 @@
$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 project) 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<const> 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;
+ }
+
}

File Metadata

Mime Type
text/plain
Expires
Fri, May 10, 5:24 AM (3 w, 1 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6281637
Default Alt Text
D19586.id46817.diff (14 KB)

Event Timeline

Phabricator · Built by Phacility