diff --git a/src/applications/policy/__tests__/PhabricatorPolicyTestCase.php b/src/applications/policy/__tests__/PhabricatorPolicyTestCase.php index aff5ee7d6e..b8b0236b52 100644 --- a/src/applications/policy/__tests__/PhabricatorPolicyTestCase.php +++ b/src/applications/policy/__tests__/PhabricatorPolicyTestCase.php @@ -82,7 +82,6 @@ final class PhabricatorPolicyTestCase extends PhabricatorTestCase { 'admin' => false, ), 'No One Policy'); - } @@ -172,6 +171,46 @@ final class PhabricatorPolicyTestCase extends PhabricatorTestCase { } + /** + * Test that invalid policies reject viewers of all types. + */ + public function testRejectInvalidPolicy() { + $invalid_policy = "the duck goes quack"; + $object = $this->buildObject($invalid_policy); + + $this->expectVisibility( + $object = $this->buildObject($invalid_policy), + array( + 'public' => false, + 'user' => false, + 'admin' => false, + ), + 'Invalid Policy'); + } + + + /** + * An omnipotent user should be able to see even objects with invalid + * policies. + */ + public function testInvalidPolicyVisibleByOmnipotentUser() { + $invalid_policy = "the cow goes moo"; + $object = $this->buildObject($invalid_policy); + + $results = array( + $object, + ); + + $query = new PhabricatorPolicyAwareTestQuery(); + $query->setResults($results); + $query->setViewer(PhabricatorUser::getOmnipotentUser()); + + $this->assertEqual( + 1, + count($query->execute())); + } + + /** * Test an object for visibility across multiple user specifications. */ diff --git a/src/applications/policy/filter/PhabricatorPolicyFilter.php b/src/applications/policy/filter/PhabricatorPolicyFilter.php index b5907e6a7a..1d6bba9c0a 100644 --- a/src/applications/policy/filter/PhabricatorPolicyFilter.php +++ b/src/applications/policy/filter/PhabricatorPolicyFilter.php @@ -225,7 +225,8 @@ final class PhabricatorPolicyFilter { $this->rejectObject($object, $policy, $capability); } } else { - throw new Exception("Object has unknown policy '{$policy}'!"); + // Reject objects with unknown policies. + $this->rejectObject($object, false, $capability); } } @@ -241,11 +242,22 @@ final class PhabricatorPolicyFilter { return; } - // TODO: clean this up - $verb = $capability; + switch ($capability) { + case PhabricatorPolicyCapability::CAN_VIEW: + $message = pht("This object has an impossible view policy."); + break; + case PhabricatorPolicyCapability::CAN_EDIT: + $message = pht("This object has an impossible edit policy."); + break; + case PhabricatorPolicyCapability::CAN_JOIN: + $message = pht("This object has an impossible join policy."); + break; + default: + $message = pht("This object has an impossible policy."); + break; + } - throw new PhabricatorPolicyException( - "This object has an impossible {$verb} policy."); + throw new PhabricatorPolicyException($message); } public function rejectObject( @@ -350,9 +362,8 @@ final class PhabricatorPolicyFilter { $handle->getFullName()); break; } - } else { - $who = pht("This object has an unknown policy setting."); + $who = pht("This object has an unknown or invalid policy setting."); } break; }