diff --git a/src/applications/policy/__tests__/PhabricatorPolicyTestCase.php b/src/applications/policy/__tests__/PhabricatorPolicyTestCase.php --- a/src/applications/policy/__tests__/PhabricatorPolicyTestCase.php +++ b/src/applications/policy/__tests__/PhabricatorPolicyTestCase.php @@ -285,6 +285,76 @@ pht('Extended Policy with Cycle')); } + + /** + * Test bulk checks of extended policies. + * + * This is testing an issue with extended policy filtering which allowed + * unusual inputs to slip objects through the filter. See D14993. + */ + public function testBulkExtendedPolicies() { + $object1 = $this->buildObject(PhabricatorPolicies::POLICY_USER) + ->setPHID('PHID-TEST-1'); + $object2 = $this->buildObject(PhabricatorPolicies::POLICY_USER) + ->setPHID('PHID-TEST-2'); + $object3 = $this->buildObject(PhabricatorPolicies::POLICY_USER) + ->setPHID('PHID-TEST-3'); + + $extended = $this->buildObject(PhabricatorPolicies::POLICY_ADMIN) + ->setPHID('PHID-TEST-999'); + + $object1->setExtendedPolicies( + array( + PhabricatorPolicyCapability::CAN_VIEW => array( + array( + $extended, + array( + PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, + ), + ), + ), + )); + + $object2->setExtendedPolicies( + array( + PhabricatorPolicyCapability::CAN_VIEW => array( + array($extended, PhabricatorPolicyCapability::CAN_VIEW), + ), + )); + + $object3->setExtendedPolicies( + array( + PhabricatorPolicyCapability::CAN_VIEW => array( + array( + $extended, + array( + PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, + ), + ), + ), + )); + + $user = $this->buildUser('user'); + + $visible = id(new PhabricatorPolicyFilter()) + ->setViewer($user) + ->requireCapabilities( + array( + PhabricatorPolicyCapability::CAN_VIEW, + )) + ->apply( + array( + $object1, + $object2, + $object3, + )); + + $this->assertEqual(array(), $visible); + } + + /** * An omnipotent user should be able to see even objects with invalid * policies. @@ -430,10 +500,12 @@ $object->setCapabilities( array( PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, )); $object->setPolicies( array( PhabricatorPolicyCapability::CAN_VIEW => $policy, + PhabricatorPolicyCapability::CAN_EDIT => $policy, )); return $object; diff --git a/src/applications/policy/filter/PhabricatorPolicyFilter.php b/src/applications/policy/filter/PhabricatorPolicyFilter.php --- a/src/applications/policy/filter/PhabricatorPolicyFilter.php +++ b/src/applications/policy/filter/PhabricatorPolicyFilter.php @@ -321,7 +321,7 @@ $objects_in = array(); foreach ($structs as $struct) { $extended_key = $struct['key']; - if (empty($extended_objects[$key])) { + if (empty($extended_objects[$extended_key])) { // If this object has already been rejected by an earlier filtering // pass, we don't need to do any tests on it. continue; @@ -335,8 +335,8 @@ // We weren't able to load the corresponding object, so just // reject this result outright. - $reject = $extended_objects[$key]; - unset($extended_objects[$key]); + $reject = $extended_objects[$extended_key]; + unset($extended_objects[$extended_key]); // TODO: This could be friendlier. $this->rejectObject($reject, false, '');