Improve Policy options

Summary:
  - Add an "Administrators" policy.
  - Allow "Public" to be completely disabled in configuration.
  - Simplify unit tests, and cover the new policies.

Test Plan: Ran unit tests.

Reviewers: btrahan, vrana, jungejason

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

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

conf/default.conf.php

@@ -69,6 +69,18 @@ return array(
   'phabricator.custom.logo'   => null,
 
 
+// -- Access Policies ------------------------------------------------------- //
+
+  // Phabricator allows you to set the visibility of objects (like repositories
+  // and source code) to "Public", which means anyone on the internet can see
+  // them, even without being logged in. This is great for open source, but
+  // some installs may never want to make anything public, so this policy is
+  // disabled by default. You can enable it here, which will let you set the
+  // policy for objects to "Public". With this option disabled, the most open
+  // policy is "All Users", which means users must be logged in to view things.
+  'policy.allow-public'         => false,
+
+
 // -- DarkConsole ----------------------------------------------------------- //
 
   // DarkConsole is a administrative debugging/profiling tool built into

src/applications/policy/__tests__/PhabricatorPolicyTestCase.php

@@ -21,26 +21,37 @@ final class PhabricatorPolicyTestCase extends PhabricatorTestCase {
   /**
    * Verify that any user can view an object with POLICY_PUBLIC.
    */
-  public function testPublicPolicy() {
+  public function testPublicPolicyEnabled() {
-    $viewer = new PhabricatorUser();
+    $env = PhabricatorEnv::beginScopedEnv();
+    $env->overrideEnvConfig('policy.allow-public', true);
 
-    $object = new PhabricatorPolicyTestObject();
+    $this->expectVisibility(
-    $object->setCapabilities(
+      $this->buildObject(PhabricatorPolicies::POLICY_PUBLIC),
       array(
-        PhabricatorPolicyCapability::CAN_VIEW,
+        'public'  => true,
-      ));
+        'user'    => true,
-    $object->setPolicies(
+        'admin'   => true,
+      ),
+      'Public Policy (Enabled in Config)');
+  }
+
+
+  /**
+   * Verify that POLICY_PUBLIC is interpreted as POLICY_USER when public
+   * policies are disallowed.
+   */
+  public function testPublicPolicyDisabled() {
+    $env = PhabricatorEnv::beginScopedEnv();
+    $env->overrideEnvConfig('policy.allow-public', false);
+
+    $this->expectVisibility(
+      $this->buildObject(PhabricatorPolicies::POLICY_PUBLIC),
       array(
-        PhabricatorPolicyCapability::CAN_VIEW =>
+        'public'  => false,
-          PhabricatorPolicies::POLICY_PUBLIC,
+        'user'    => true,
-      ));
+        'admin'   => true,
-
+      ),
-    $query = new PhabricatorPolicyTestQuery();
+      'Public Policy (Disabled in Config)');
-    $query->setResults(array($object));
-    $query->setViewer($viewer);
-    $result = $query->executeOne();
-
-    $this->assertEqual($object, $result, 'Policy: Public');
   }
 
 
@@ -49,41 +60,29 @@ final class PhabricatorPolicyTestCase extends PhabricatorTestCase {
    * logged-out users can not.
    */
   public function testUsersPolicy() {
-    $viewer = new PhabricatorUser();
+    $this->expectVisibility(
-
+      $this->buildObject(PhabricatorPolicies::POLICY_USER),
-    $object = new PhabricatorPolicyTestObject();
-    $object->setCapabilities(
       array(
-        PhabricatorPolicyCapability::CAN_VIEW,
+        'public'  => false,
-      ));
+        'user'    => true,
-    $object->setPolicies(
+        'admin'   => true,
+      ),
+      'User Policy');
+  }
+
+
+  /**
+   * Verify that only administrators can view an object with POLICY_ADMIN.
+   */
+  public function testAdminPolicy() {
+    $this->expectVisibility(
+      $this->buildObject(PhabricatorPolicies::POLICY_ADMIN),
       array(
-        PhabricatorPolicyCapability::CAN_VIEW =>
+        'public'  => false,
-          PhabricatorPolicies::POLICY_USER,
+        'user'    => false,
-      ));
+        'admin'   => true,
-
+      ),
-    $query = new PhabricatorPolicyTestQuery();
+      'Admin Policy');
-    $query->setResults(array($object));
-    $query->setViewer($viewer);
-
-    $caught = null;
-    try {
-      $query->executeOne();
-    } catch (PhabricatorPolicyException $ex) {
-      $caught = $ex;
-    }
-
-    $this->assertEqual(
-      true,
-      ($caught instanceof PhabricatorPolicyException),
-      'Policy: Users rejects logged out users.');
-
-    $viewer->setPHID(1);
-    $result = $query->executeOne();
-    $this->assertEqual(
-      $object,
-      $result,
-      'Policy: Users');
   }
 
 
@@ -91,8 +90,59 @@ final class PhabricatorPolicyTestCase extends PhabricatorTestCase {
    * Verify that no one can view an object with POLICY_NOONE.
    */
   public function testNoOnePolicy() {
-     $viewer = new PhabricatorUser();
+    $this->expectVisibility(
+      $this->buildObject(PhabricatorPolicies::POLICY_NOONE),
+      array(
+        'public'  => false,
+        'user'    => false,
+        'admin'   => false,
+      ),
+      'No One Policy');
 
+  }
+
+
+  /**
+   * Test an object for visibility across multiple user specifications.
+   */
+  private function expectVisibility(
+    PhabricatorPolicyTestObject $object,
+    array $map,
+    $description) {
+
+    foreach ($map as $spec => $expect) {
+      $viewer = $this->buildUser($spec);
+
+      $query = new PhabricatorPolicyTestQuery();
+      $query->setResults(array($object));
+      $query->setViewer($viewer);
+
+      $caught = null;
+      try {
+        $result = $query->executeOne();
+      } catch (PhabricatorPolicyException $ex) {
+        $caught = $ex;
+      }
+
+      if ($expect) {
+        $this->assertEqual(
+          $object,
+          $result,
+          "{$description} with user {$spec} should succeed.");
+      } else {
+        $this->assertEqual(
+          true,
+          $caught instanceof PhabricatorPolicyException,
+          "{$description} with user {$spec} should fail.");
+      }
+    }
+  }
+
+
+  /**
+   * Build a test object to spec.
+   */
+  private function buildObject($policy) {
     $object = new PhabricatorPolicyTestObject();
     $object->setCapabilities(
       array(
@@ -100,39 +150,34 @@ final class PhabricatorPolicyTestCase extends PhabricatorTestCase {
       ));
     $object->setPolicies(
       array(
-        PhabricatorPolicyCapability::CAN_VIEW =>
+        PhabricatorPolicyCapability::CAN_VIEW => $policy,
-          PhabricatorPolicies::POLICY_NOONE,
       ));
 
-    $query = new PhabricatorPolicyTestQuery();
+    return $object;
-    $query->setResults(array($object));
+  }
-    $query->setViewer($viewer);
 
-    $caught = null;
+
-    try {
+  /**
-      $query->executeOne();
+   * Build a test user to spec.
-    } catch (PhabricatorPolicyException $ex) {
+   */
-      $caught = $ex;
+  private function buildUser($spec) {
+    $user = new PhabricatorUser();
+
+    switch ($spec) {
+      case 'public':
+        break;
+      case 'user':
+        $user->setPHID(1);
+        break;
+      case 'admin':
+        $user->setPHID(1);
+        $user->setIsAdmin(true);
+        break;
+      default:
+        throw new Exception("Unknown user spec '{$spec}'.");
     }
 
-    $this->assertEqual(
+    return $user;
-      true,
-      ($caught instanceof PhabricatorPolicyException),
-      'Policy: No One rejects logged out users.');
-
-    $viewer->setPHID(1);
-
-    $caught = null;
-    try {
-      $query->executeOne();
-    } catch (PhabricatorPolicyException $ex) {
-      $caught = $ex;
-    }
-
-    $this->assertEqual(
-      true,
-      ($caught instanceof PhabricatorPolicyException),
-      'Policy: No One rejects logged-in users.');
   }
 
 }

src/applications/policy/__tests__/__init__.php

@@ -10,6 +10,7 @@ phutil_require_module('phabricator', 'applications/people/storage/user');
 phutil_require_module('phabricator', 'applications/policy/constants/capability');
 phutil_require_module('phabricator', 'applications/policy/constants/policy');
 phutil_require_module('phabricator', 'applications/policy/interface/policy');
+phutil_require_module('phabricator', 'infrastructure/env');
 phutil_require_module('phabricator', 'infrastructure/query/policy/base');
 phutil_require_module('phabricator', 'infrastructure/testing/testcase');
 

src/applications/policy/constants/policy/PhabricatorPolicies.php

@@ -20,6 +20,7 @@ final class PhabricatorPolicies extends PhabricatorPolicyConstants {
 
   const POLICY_PUBLIC   = 'public';
   const POLICY_USER     = 'users';
+  const POLICY_ADMIN    = 'admin';
   const POLICY_NOONE    = 'no-one';
 
 }

src/applications/policy/filter/policy/PhabricatorPolicyFilter.php

@@ -67,6 +67,14 @@ final class PhabricatorPolicyFilter {
 
       $policy = $object->getPolicy($capability);
 
+      // If the object is set to "public" but that policy is disabled for this
+      // install, restrict the policy to "user".
+      if ($policy == PhabricatorPolicies::POLICY_PUBLIC) {
+        if (!PhabricatorEnv::getEnvConfig('policy.allow-public')) {
+          $policy = PhabricatorPolicies::POLICY_USER;
+        }
+      }
+
       switch ($policy) {
         case PhabricatorPolicies::POLICY_PUBLIC:
           $filtered[$key] = $object;
@@ -78,6 +86,13 @@ final class PhabricatorPolicyFilter {
             $this->rejectObject($object, $policy);
           }
           break;
+        case PhabricatorPolicies::POLICY_ADMIN:
+          if ($viewer->getIsAdmin()) {
+            $filtered[$key] = $object;
+          } else {
+            $this->rejectObject($object, $policy);
+          }
+          break;
         case PhabricatorPolicies::POLICY_NOONE:
           $this->rejectObject($object, $policy);
           break;
@@ -103,6 +118,9 @@ final class PhabricatorPolicyFilter {
       case PhabricatorPolicies::POLICY_USER:
         $who = "To view this object, you must be logged in.";
         break;
+      case PhabricatorPolicies::POLICY_ADMIN:
+        $who = "To view this object, you must be an administrator.";
+        break;
       case PhabricatorPolicies::POLICY_NOONE:
         $who = "No one can view this object.";
         break;

src/applications/policy/filter/policy/__init__.php

@@ -8,6 +8,7 @@
 
 phutil_require_module('phabricator', 'applications/policy/constants/policy');
 phutil_require_module('phabricator', 'applications/policy/exception/base');
+phutil_require_module('phabricator', 'infrastructure/env');
 
 phutil_require_module('phutil', 'utils');
 

src/view/form/control/policy/AphrontFormPolicyControl.php

@@ -32,9 +32,30 @@ final class AphrontFormPolicyControl extends AphrontFormControl {
   }
 
   private function getOptions() {
-    return array(
+    $show_public = PhabricatorEnv::getEnvConfig('policy.allow-public');
-      PhabricatorPolicies::POLICY_USER => 'All Users',
+
-    );
+    if ($this->getValue() == PhabricatorPolicies::POLICY_PUBLIC) {
+      // If the object already has a "public" policy, show the option in
+      // the dropdown even if it will be enforced as "users", so we don't
+      // change the policy just because the config is changing.
+      $show_public = true;
+    }
+
+    $options = array();
+
+    if ($show_public) {
+      $options[PhabricatorPolicies::POLICY_PUBLIC] = 'Public';
+    }
+
+    $options[PhabricatorPolicies::POLICY_USER] = 'All Users';
+
+    if ($this->user->getIsAdmin()) {
+      $options[PhabricatorPolicies::POLICY_ADMIN] = 'Administrators';
+    }
+
+    $options[PhabricatorPolicies::POLICY_NOONE] = 'No One';
+
+    return $options;
   }
 
   protected function renderInput() {

src/view/form/control/policy/__init__.php

@@ -7,6 +7,7 @@
 
 
 phutil_require_module('phabricator', 'applications/policy/constants/policy');
+phutil_require_module('phabricator', 'infrastructure/env');
 phutil_require_module('phabricator', 'view/form/control/base');
 phutil_require_module('phabricator', 'view/form/control/select');