Page MenuHomePhabricator
Files F15401537

D8508.id20185.diff
No OneTemporary
Actions

D8508.id20185.diff
View Options

diff --git a/src/applications/maniphest/editor/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/ManiphestTransactionEditor.php
--- a/src/applications/maniphest/editor/ManiphestTransactionEditor.php
+++ b/src/applications/maniphest/editor/ManiphestTransactionEditor.php
@@ -3,6 +3,8 @@
final class ManiphestTransactionEditor
extends PhabricatorApplicationTransactionEditor {
+ private $oldOwnerPHID;
+
public function getTransactionTypes() {
$types = parent::getTransactionTypes();
@@ -24,6 +26,91 @@
return $types;
}
+ /**
+ * We probably want to apply initial effects if there is an owner
+ * transaction. Note we will only really apply any initial effects
+ * if the transaction changes ownership.
+ *
+ * Overall, the goal is to set the Task owner phid correctly ASAP so
+ * policy checks all work correctly w.r.t. "owner can always do stuff"
+ * meta policy.
+ *
+ * Similarly, we also want to apply initial effects if there is a change
+ * to policy *AND* the object is new. This may change if / when we decide
+ * to have an explicit "can create" permission, in which case "can view"
+ * and "can edit" policies will need to be removed from the create path.
+ * For now though, since anyone can create tasks, we have to update the
+ * policy ASAP so e.g. if the default edit policy is no one people can
+ * still create tasks so long as they set the edit policy to something
+ * that lets them edit the Task.
+ */
+ protected function shouldApplyInitialEffects(
+ PhabricatorLiskDAO $object,
+ array $xactions) {
+ $should_apply = false;
+ foreach ($xactions as $xaction) {
+ switch ($xaction->getTransactionType()) {
+ case ManiphestTransaction::TYPE_OWNER:
+ $should_apply = true;
+ break 2;
+ case PhabricatorTransactions::TYPE_VIEW_POLICY:
+ case PhabricatorTransactions::TYPE_EDIT_POLICY:
+ if ($this->getIsNewObject()) {
+ $should_apply = true;
+ break 2;
+ }
+ continue;
+ default:
+ continue;
+ }
+ }
+ return $should_apply;
+ }
+
+ protected function applyInitialEffects(
+ PhabricatorLiskDAO $object,
+ array $xactions) {
+ $changing_owner = false;
+ $changing_edit_policy = false;
+ $changing_view_policy = false;
+ foreach ($xactions as $xaction) {
+ switch ($xaction->getTransactionType()) {
+ case ManiphestTransaction::TYPE_OWNER:
+ $this->oldOwnerPHID = nonempty($object->getOwnerPHID(), null);
+ $new_owner_phid = $xaction->getNewValue();
+ if ($new_owner_phid != $this->oldOwnerPHID) {
+ $object->setOwnerPHID($new_owner_phid);
+ $changing_owner = true;
+ }
+ break;
+ case PhabricatorTransactions::TYPE_VIEW_POLICY:
+ $view_policy = $xaction->getNewValue();
+ if ($this->getIsNewObject()) {
+ $object->setViewPolicy($view_policy);
+ } else if ($object->getViewPolicy() != $view_policy) {
+ $changing_view_policy = $view_policy;
+ }
+ break;
+ case PhabricatorTransactions::TYPE_EDIT_POLICY:
+ $edit_policy = $xaction->getNewValue();
+ if ($this->getIsNewObject()) {
+ $object->setEditPolicy($edit_policy);
+ } else if ($object->getEditPolicy() != $edit_policy) {
+ $changing_edit_policy = $edit_policy;
+ }
+ break;
+ default:
+ continue;
+ }
+ }
+ if ($changing_owner && $changing_view_policy) {
+ $object->setViewPolicy($changing_view_policy);
+ }
+ if ($changing_owner && $changing_edit_policy) {
+ $object->setEditPolicy($changing_edit_policy);
+ }
+ }
+
protected function getCustomTransactionOldValue(
PhabricatorLiskDAO $object,
PhabricatorApplicationTransaction $xaction) {
@@ -50,7 +137,7 @@
}
return $object->getDescription();
case ManiphestTransaction::TYPE_OWNER:
- return nonempty($object->getOwnerPHID(), null);
+ return $this->oldOwnerPHID;
case ManiphestTransaction::TYPE_CCS:
return array_values(array_unique($object->getCCPHIDs()));
case ManiphestTransaction::TYPE_PROJECTS:
@@ -396,7 +483,6 @@
$transaction_type = $xaction->getTransactionType();
$app_capability = idx($app_capability_map, $transaction_type);
-
if ($app_capability) {
$app = id(new PhabricatorApplicationQuery())
->setViewer($this->getActor())
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
@@ -178,10 +178,19 @@
case PhabricatorTransactions::TYPE_SUBSCRIBERS:
return array_values($this->subscribers);
case PhabricatorTransactions::TYPE_VIEW_POLICY:
+ if ($this->getIsNewObject()) {
+ return null;
+ }
return $object->getViewPolicy();
case PhabricatorTransactions::TYPE_EDIT_POLICY:
+ if ($this->getIsNewObject()) {
+ return null;
+ }
return $object->getEditPolicy();
case PhabricatorTransactions::TYPE_JOIN_POLICY:
+ if ($this->getIsNewObject()) {
+ return null;
+ }
return $object->getJoinPolicy();
case PhabricatorTransactions::TYPE_EDGE:
$edge_type = $xaction->getMetadataValue('edge:type');
@@ -511,6 +520,14 @@
$read_locking = false;
$transaction_open = false;
+ if ($this->shouldApplyInitialEffects($object, $xactions)) {
+ if (!$is_preview) {
+ $object->openTransaction();
+ $transaction_open = true;
+ }
+ $this->applyInitialEffects($object, $xactions);
+ }
+
if (!$is_preview) {
$errors = array();
$type_map = mgroup($xactions, 'getTransactionType');
@@ -529,6 +546,10 @@
}
if ($errors) {
+ if ($transaction_open) {
+ $object->killTransaction();
+ $transaction_open = false;
+ }
throw new PhabricatorApplicationTransactionValidationException($errors);
}
@@ -544,26 +565,17 @@
// from the state when it was originally loaded.
if ($this->shouldReadLock($object, $xaction)) {
- $object->openTransaction();
+ if (!$transaction_open) {
+ $object->openTransaction();
+ $transaction_open = true;
+ }
$object->beginReadLocking();
- $transaction_open = true;
$read_locking = true;
$object->reload();
break;
}
}
}
-
- if ($this->shouldApplyInitialEffects($object, $xactions)) {
- if (!$transaction_open) {
- $object->openTransaction();
- $transaction_open = true;
- }
- }
- }
-
- if ($this->shouldApplyInitialEffects($object, $xactions)) {
- $this->applyInitialEffects($object, $xactions);
}
foreach ($xactions as $xaction) {
@@ -587,7 +599,15 @@
// Now that we've merged, filtered, and combined transactions, check for
// required capabilities.
foreach ($xactions as $xaction) {
- $this->requireCapabilities($object, $xaction);
+ try {
+ $this->requireCapabilities($object, $xaction);
+ } catch (PhabricatorPolicyException $ex) {
+ if ($transaction_open) {
+ $object->killTransaction();
+ $transaction_open = false;
+ }
+ throw $ex;
+ }
}
$xactions = $this->sortTransactions($xactions);
@@ -897,33 +917,30 @@
get_class($this)));
}
}
-
- // The actor must have permission to view and edit the object.
-
- $actor = $this->requireActor();
-
- PhabricatorPolicyFilter::requireCapability(
- $actor,
- $object,
- PhabricatorPolicyCapability::CAN_VIEW);
}
protected function requireCapabilities(
PhabricatorLiskDAO $object,
PhabricatorApplicationTransaction $xaction) {
+ $capabilities = array(
+ PhabricatorPolicyCapability::CAN_EDIT => 1,
+ PhabricatorPolicyCapability::CAN_VIEW => 1);
+
switch ($xaction->getTransactionType()) {
case PhabricatorTransactions::TYPE_EDIT_POLICY:
// You must have the edit capability to alter the edit policy of an
- // object. For other default transaction types, we don't enforce
- // anything for the moment.
+ // object.
- PhabricatorPolicyFilter::requireCapability(
- $this->requireActor(),
- $object,
- PhabricatorPolicyCapability::CAN_EDIT);
+ $capabilities[PhabricatorPolicyCapability::CAN_EDIT] = 1;
break;
}
+
+ id(new PhabricatorPolicyFilter())
+ ->setViewer($this->requireActor())
+ ->requireCapabilities(array_keys($capabilities))
+ ->raisePolicyExceptions(true)
+ ->apply(array($object));
}
private function buildMentionTransaction(

File Metadata

Mime Type
text/plain
Expires
Mar 18 2025, 6:09 PM (5 w, 20 h ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7704797
Default Alt Text
D8508.id20185.diff (9 KB)

Event Timeline

Phabricator · Built by Phacility