Tweak application and maniphest editors to handle policy corner cases better

Summary:
Fixes T4362. If you have a default edit + view policy of "no one" things get weird when you try to create a task - basically its impossible.

Ergo, re-jigger how we do policy checks just a bit.

- if its a new object, don't bother with the "can the $actor edit this thing by virtue of having can see / can edit priveleges?" That makes no sense on create.
- add a hook so when doing the "will $actor still be able to edit this thing after all the edits" checks the object can be updated to its ultimate state. This matters for Maniphest as being the owner lets you do all sorts of stuff.

Test Plan:
- made a task with no one policy and assigned to no one - exception
- made a task with no one policy and assigned to me - success
- made a comment on the task - success
- reassigned the task to another user - exception
- reassigned the task to another user and updated policies to "users" - success

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: aran, epriestley, Korvin

Maniphest Tasks: T4362

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

Bob Trahan 12 years ago e6118bcb a9f38e55

+117 -35

2 changed files

expand all

src

applications

maniphest

editor

ManiphestTransactionEditor.php

transactions

editor

PhabricatorApplicationTransactionEditor.php

+17

src/applications/maniphest/editor/ManiphestTransactionEditor.php

··· 409 409 } 410 410 } 411 411 412 + protected function adjustObjectForPolicyChecks( 413 + PhabricatorLiskDAO $object, 414 + array $xactions) { 415 + 416 + $copy = parent::adjustObjectForPolicyChecks($object, $xactions); 417 + foreach ($xactions as $xaction) { 418 + switch ($xaction->getTransactionType()) { 419 + case ManiphestTransaction::TYPE_OWNER: 420 + $copy->setOwnerPHID($xaction->getNewValue()); 421 + break; 422 + default: 423 + continue; 424 + } 425 + } 426 + 427 + return $copy; 428 + } 412 429 413 430 private function getNextSubpriority($pri, $sub) { 414 431

+100 -35

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

··· 897 897 get_class($this))); 898 898 } 899 899 } 900 - 901 - // The actor must have permission to view and edit the object. 902 - 903 - $actor = $this->requireActor(); 904 - 905 - PhabricatorPolicyFilter::requireCapability( 906 - $actor, 907 - $object, 908 - PhabricatorPolicyCapability::CAN_VIEW); 909 900 } 910 901 911 902 protected function requireCapabilities( 912 903 PhabricatorLiskDAO $object, 913 904 PhabricatorApplicationTransaction $xaction) { 914 905 906 + if ($this->getIsNewObject()) { 907 + return; 908 + } 909 + 910 + $actor = $this->requireActor(); 915 911 switch ($xaction->getTransactionType()) { 912 + case PhabricatorTransactions::TYPE_COMMENT: 913 + PhabricatorPolicyFilter::requireCapability( 914 + $actor, 915 + $object, 916 + PhabricatorPolicyCapability::CAN_VIEW); 917 + break; 918 + case PhabricatorTransactions::TYPE_VIEW_POLICY: 919 + PhabricatorPolicyFilter::requireCapability( 920 + $actor, 921 + $object, 922 + PhabricatorPolicyCapability::CAN_EDIT); 923 + break; 916 924 case PhabricatorTransactions::TYPE_EDIT_POLICY: 917 - // You must have the edit capability to alter the edit policy of an 918 - // object. For other default transaction types, we don't enforce 919 - // anything for the moment. 920 - 925 + PhabricatorPolicyFilter::requireCapability( 926 + $actor, 927 + $object, 928 + PhabricatorPolicyCapability::CAN_EDIT); 929 + break; 930 + case PhabricatorTransactions::TYPE_JOIN_POLICY: 921 931 PhabricatorPolicyFilter::requireCapability( 922 - $this->requireActor(), 932 + $actor, 923 933 $object, 924 934 PhabricatorPolicyCapability::CAN_EDIT); 925 935 break; ··· 1464 1474 1465 1475 $errors = array(); 1466 1476 switch ($type) { 1477 + case PhabricatorTransactions::TYPE_VIEW_POLICY: 1478 + $errors[] = $this->validatePolicyTransaction( 1479 + $object, 1480 + $xactions, 1481 + $type, 1482 + PhabricatorPolicyCapability::CAN_VIEW); 1483 + break; 1467 1484 case PhabricatorTransactions::TYPE_EDIT_POLICY: 1468 - // Make sure the user isn't editing away their ability to edit this 1469 - // object. 1470 - foreach ($xactions as $xaction) { 1471 - try { 1472 - PhabricatorPolicyFilter::requireCapabilityWithForcedPolicy( 1473 - $this->requireActor(), 1474 - $object, 1475 - PhabricatorPolicyCapability::CAN_EDIT, 1476 - $xaction->getNewValue()); 1477 - } catch (PhabricatorPolicyException $ex) { 1478 - $errors[] = array( 1479 - new PhabricatorApplicationTransactionValidationError( 1480 - $type, 1481 - pht('Invalid'), 1482 - pht( 1483 - 'You can not select this edit policy, because you would '. 1484 - 'no longer be able to edit the object.'), 1485 - $xaction), 1486 - ); 1487 - } 1488 - } 1485 + $errors[] = $this->validatePolicyTransaction( 1486 + $object, 1487 + $xactions, 1488 + $type, 1489 + PhabricatorPolicyCapability::CAN_EDIT); 1489 1490 break; 1490 1491 case PhabricatorTransactions::TYPE_CUSTOMFIELD: 1491 1492 $groups = array(); ··· 1514 1515 return array_mergev($errors); 1515 1516 } 1516 1517 1518 + private function validatePolicyTransaction( 1519 + PhabricatorLiskDAO $object, 1520 + array $xactions, 1521 + $transaction_type, 1522 + $capability) { 1523 + 1524 + $actor = $this->requireActor(); 1525 + $errors = array(); 1526 + // Note $this->xactions is necessary; $xactions is $this->xactions of 1527 + // $transaction_type 1528 + $policy_object = $this->adjustObjectForPolicyChecks( 1529 + $object, 1530 + $this->xactions); 1531 + 1532 + // Make sure the user isn't editing away their ability to $capability this 1533 + // object. 1534 + foreach ($xactions as $xaction) { 1535 + try { 1536 + PhabricatorPolicyFilter::requireCapabilityWithForcedPolicy( 1537 + $actor, 1538 + $policy_object, 1539 + $capability, 1540 + $xaction->getNewValue()); 1541 + } catch (PhabricatorPolicyException $ex) { 1542 + $errors[] = new PhabricatorApplicationTransactionValidationError( 1543 + $transaction_type, 1544 + pht('Invalid'), 1545 + pht( 1546 + 'You can not select this %s policy, because you would no longer '. 1547 + 'be able to %s the object.', 1548 + $capability, 1549 + $capability), 1550 + $xaction); 1551 + } 1552 + } 1553 + 1554 + if ($this->getIsNewObject()) { 1555 + if (!$xactions) { 1556 + $has_capability = PhabricatorPolicyFilter::hasCapability( 1557 + $actor, 1558 + $policy_object, 1559 + $capability); 1560 + if (!$has_capability) { 1561 + $errors[] = new PhabricatorApplicationTransactionValidationError( 1562 + $transaction_type, 1563 + pht('Invalid'), 1564 + pht('The selected %s policy excludes you. Choose a %s policy '. 1565 + 'which allows you to %s the object.', 1566 + $capability, 1567 + $capability, 1568 + $capability)); 1569 + } 1570 + } 1571 + } 1572 + 1573 + return $errors; 1574 + } 1575 + 1576 + protected function adjustObjectForPolicyChecks( 1577 + PhabricatorLiskDAO $object, 1578 + array $xactions) { 1579 + 1580 + return clone $object; 1581 + } 1517 1582 1518 1583 /** 1519 1584 * Check for a missing text field.

Configure Feed

Configure Feed