Tighten up some policy interactions in Herald

Summary:
Ref T603. Herald is a bit of a policy minefield right now, although I think pretty much everything has straightforward solutions. This change:

  - Introduces "create" and "create global" permisions for Herald.
    - Maybe "create" is sort of redundant since there's no reason to have access to the application if not creating rules, but I think this won't be the case for most applications, so having an explicit "create" permission is more consistent.
  - Add some application policy helper functions.
  - Improve rendering a bit -- I think we probably need to build some `PolicyType` class, similar to `PHIDType`, to really get this right.
  - Don't let users who can't use application X create Herald rules for application X.
  - Remove Maniphest/Pholio rules when those applications are not installed.

Test Plan:
  - Restricted access to Maniphest and uninstalled Pholio.
  - Verified Pholio rules no longer appear for anyone.
  - Verified Maniphest ruls no longer appear for restricted users.
  - Verified users without CREATE_GLOBAL can not create global ruls.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

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

src/applications/base/controller/PhabricatorController.php

@@ -350,4 +350,23 @@ abstract class PhabricatorController extends AphrontController {
     return $view;
   }
 
+  protected function hasApplicationCapability($capability) {
+    return PhabricatorPolicyFilter::hasCapability(
+      $this->getRequest()->getUser(),
+      $this->getCurrentApplication(),
+      $capability);
+  }
+
+  protected function requireApplicationCapability($capability) {
+    PhabricatorPolicyFilter::requireCapability(
+      $this->getRequest()->getUser(),
+      $this->getCurrentApplication(),
+      $capability);
+  }
+
+  protected function explainApplicationCapability($capability, $message) {
+    // TODO: Render a link to get more information.
+    return $message;
+  }
+
 }

src/applications/herald/adapter/HeraldAdapter.php

@@ -97,6 +97,17 @@ abstract class HeraldAdapter {
     return true;
   }
 
+  public function isAvailableToUser(PhabricatorUser $viewer) {
+    $applications = id(new PhabricatorApplicationQuery())
+      ->setViewer($viewer)
+      ->withInstalled(true)
+      ->withClasses(array($this->getAdapterApplicationClass()))
+      ->execute();
+
+    return !empty($applications);
+  }
+
+
   /**
    * NOTE: You generally should not override this; it exists to support legacy
    * adapters which had hard-coded content types.
@@ -106,6 +117,7 @@ abstract class HeraldAdapter {
   }
 
   abstract public function getAdapterContentName();
+  abstract public function getAdapterApplicationClass();
 
 
 /* -(  Fields  )------------------------------------------------------------- */
@@ -694,16 +706,6 @@ abstract class HeraldAdapter {
     return $adapters;
   }
 
-  public static function getAllEnabledAdapters() {
-    $adapters = self::getAllAdapters();
-    foreach ($adapters as $key => $adapter) {
-      if (!$adapter->isEnabled()) {
-        unset($adapters[$key]);
-      }
-    }
-    return $adapters;
-  }
-
   public static function getAdapterForContentType($content_type) {
     $adapters = self::getAllAdapters();
 
@@ -719,11 +721,14 @@ abstract class HeraldAdapter {
         $content_type));
   }
 
-  public static function getEnabledAdapterMap() {
+  public static function getEnabledAdapterMap(PhabricatorUser $viewer) {
     $map = array();
 
-    $adapters = HeraldAdapter::getAllEnabledAdapters();
+    $adapters = HeraldAdapter::getAllAdapters();
     foreach ($adapters as $adapter) {
+      if (!$adapter->isAvailableToUser($viewer)) {
+        continue;
+      }
       $type = $adapter->getAdapterContentType();
       $name = $adapter->getAdapterContentName();
       $map[$type] = $name;
@@ -733,7 +738,6 @@ abstract class HeraldAdapter {
     return $map;
   }
 
-
   public function renderRuleAsText(HeraldRule $rule, array $handles) {
     assert_instances_of($handles, 'PhabricatorObjectHandle');
 

src/applications/herald/adapter/HeraldCommitAdapter.php

@@ -27,9 +27,8 @@ final class HeraldCommitAdapter extends HeraldAdapter {
   protected $affectedPackages;
   protected $auditNeededPackages;
 
-  public function isEnabled() {
+  public function getAdapterApplicationClass() {
-    $app = 'PhabricatorApplicationDiffusion';
+    return 'PhabricatorApplicationDiffusion';
-    return PhabricatorApplication::isClassInstalled($app);
   }
 
   public function getAdapterContentType() {

src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php

@@ -20,9 +20,8 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter {
   protected $affectedPackages;
   protected $changesets;
 
-  public function isEnabled() {
+  public function getAdapterApplicationClass() {
-    $app = 'PhabricatorApplicationDifferential';
+    return 'PhabricatorApplicationDifferential';
-    return PhabricatorApplication::isClassInstalled($app);
   }
 
   public function getAdapterContentType() {

src/applications/herald/adapter/HeraldManiphestTaskAdapter.php

@@ -10,6 +10,10 @@ final class HeraldManiphestTaskAdapter extends HeraldAdapter {
   private $assignPHID;
   private $projectPHIDs = array();
 
+  public function getAdapterApplicationClass() {
+    return 'PhabricatorApplicationManiphest';
+  }
+
   public function setTask(ManiphestTask $task) {
     $this->task = $task;
     return $this;

src/applications/herald/adapter/HeraldPholioMockAdapter.php

@@ -8,6 +8,10 @@ final class HeraldPholioMockAdapter extends HeraldAdapter {
   private $mock;
   private $ccPHIDs = array();
 
+  public function getAdapterApplicationClass() {
+    return 'PhabricatorApplicationPholio';
+  }
+
   public function setMock(PholioMock $mock) {
     $this->mock = $mock;
     return $this;

src/applications/herald/application/PhabricatorApplicationHerald.php

@@ -2,6 +2,9 @@
 
 final class PhabricatorApplicationHerald extends PhabricatorApplication {
 
+  const CAN_CREATE_RULE = 'herald.create';
+  const CAN_CREATE_GLOBAL_RULE = 'herald.global';
+
   public function getBaseURI() {
     return '/herald/';
   }
@@ -48,4 +51,18 @@ final class PhabricatorApplicationHerald extends PhabricatorApplication {
     );
   }
 
+  protected function getCustomCapabilities() {
+    return array(
+      self::CAN_CREATE_RULE => array(
+        'label' => pht('Can Create Rules'),
+      ),
+      self::CAN_CREATE_GLOBAL_RULE => array(
+        'label' => pht('Can Create Global Rules'),
+        'caption' => pht('Global rules can bypass access controls.'),
+        'default' => PhabricatorPolicies::POLICY_ADMIN,
+      ),
+    );
+  }
+
+
 }

src/applications/herald/controller/HeraldController.php

@@ -23,11 +23,15 @@ abstract class HeraldController extends PhabricatorController {
   public function buildApplicationCrumbs() {
     $crumbs = parent::buildApplicationCrumbs();
 
+    $can_create = $this->hasApplicationCapability(
+      PhabricatorApplicationHerald::CAN_CREATE_RULE);
+
     $crumbs->addAction(
       id(new PHUIListItemView())
         ->setName(pht('Create Herald Rule'))
         ->setHref($this->getApplicationURI('new/'))
-        ->setIcon('create'));
+        ->setIcon('create')
+        ->setDisabled(!$can_create));
 
     return $crumbs;
   }

src/applications/herald/controller/HeraldNewController.php

@@ -11,11 +11,16 @@ final class HeraldNewController extends HeraldController {
   }
 
   public function processRequest() {
-
     $request = $this->getRequest();
     $user = $request->getUser();
 
-    $content_type_map = HeraldAdapter::getEnabledAdapterMap();
+    $this->requireApplicationCapability(
+      PhabricatorApplicationHerald::CAN_CREATE_RULE);
+
+    $can_global = $this->hasApplicationCapability(
+      PhabricatorApplicationHerald::CAN_CREATE_GLOBAL_RULE);
+
+    $content_type_map = HeraldAdapter::getEnabledAdapterMap($user);
     if (empty($content_type_map[$this->contentType])) {
       $this->contentType = head_key($content_type_map);
     }
@@ -32,14 +37,29 @@ final class HeraldNewController extends HeraldController {
         HeraldRuleTypeConfig::RULE_TYPE_PERSONAL,
       )) + $rule_type_map;
 
+    if (!$can_global) {
+      $global_link = $this->explainApplicationCapability(
+        PhabricatorApplicationHerald::CAN_CREATE_GLOBAL_RULE,
+        pht('You do not have permission to create or manage global rules.'));
+    } else {
+      $global_link = null;
+    }
+
     $captions = array(
       HeraldRuleTypeConfig::RULE_TYPE_PERSONAL =>
-        pht('Personal rules notify you about events. You own them, but '.
+        pht(
-        'they can only affect you.'),
+          'Personal rules notify you about events. You own them, but they can '.
+          'only affect you.'),
       HeraldRuleTypeConfig::RULE_TYPE_GLOBAL =>
-        pht('Global rules notify anyone about events. No one owns them, and '.
+        phutil_implode_html(
-        'anyone can edit them. Usually, Global rules are used to notify '.
+          phutil_tag('br'),
-        'mailing lists.'),
+          array_filter(
+            array(
+              pht(
+                'Global rules notify anyone about events. Global rules can '.
+                'bypass access control policies.'),
+              $global_link,
+            ))),
     );
 
     $radio = id(new AphrontFormRadioButtonControl())
@@ -48,10 +68,15 @@ final class HeraldNewController extends HeraldController {
       ->setValue($this->ruleType);
 
     foreach ($rule_type_map as $value => $name) {
+      $disabled = ($value == HeraldRuleTypeConfig::RULE_TYPE_GLOBAL) &&
+                  (!$can_global);
+
       $radio->addButton(
         $value,
         $name,
-        idx($captions, $value));
+        idx($captions, $value),
+        $disabled ? 'disabled' : null,
+        $disabled);
     }
 
     $form = id(new AphrontFormView())

src/applications/herald/controller/HeraldRuleController.php

@@ -14,7 +14,7 @@ final class HeraldRuleController extends HeraldController {
     $request = $this->getRequest();
     $user = $request->getUser();
 
-    $content_type_map = HeraldAdapter::getEnabledAdapterMap();
+    $content_type_map = HeraldAdapter::getEnabledAdapterMap($user);
     $rule_type_map = HeraldRuleTypeConfig::getRuleTypeMap();
 
     if ($this->id) {
@@ -42,11 +42,19 @@ final class HeraldRuleController extends HeraldController {
 
       $rule_type = $request->getStr('rule_type');
       if (!isset($rule_type_map[$rule_type])) {
-        $rule_type = HeraldRuleTypeConfig::RULE_TYPE_GLOBAL;
+        $rule_type = HeraldRuleTypeConfig::RULE_TYPE_PERSONAL;
       }
       $rule->setRuleType($rule_type);
 
       $cancel_uri = $this->getApplicationURI();
+
+      $this->requireApplicationCapability(
+        PhabricatorApplicationHerald::CAN_CREATE_RULE);
+    }
+
+    if ($rule->getRuleType() == HeraldRuleTypeConfig::RULE_TYPE_GLOBAL) {
+      $this->requireApplicationCapability(
+        PhabricatorApplicationHerald::CAN_CREATE_GLOBAL_RULE);
     }
 
     $adapter = HeraldAdapter::getAdapterForContentType($rule->getContentType());

src/applications/herald/controller/HeraldRuleListController.php

@@ -33,7 +33,7 @@ final class HeraldRuleListController extends HeraldController
     $phids = mpull($rules, 'getAuthorPHID');
     $this->loadHandles($phids);
 
-    $content_type_map = HeraldAdapter::getEnabledAdapterMap();
+    $content_type_map = HeraldAdapter::getEnabledAdapterMap($viewer);
 
     $list = id(new PHUIObjectItemListView())
       ->setUser($viewer);

src/applications/herald/controller/HeraldRuleViewController.php

@@ -96,7 +96,9 @@ final class HeraldRuleViewController extends HeraldController {
     if ($adapter) {
       $view->addProperty(
         pht('Applies To'),
-        idx(HeraldAdapter::getEnabledAdapterMap(), $rule->getContentType()));
+        idx(
+          HeraldAdapter::getEnabledAdapterMap($viewer),
+          $rule->getContentType()));
 
       $view->invokeWillRenderEvent();
 

src/applications/herald/query/HeraldRuleQuery.php

@@ -76,6 +76,16 @@ final class HeraldRuleQuery
   public function willFilterPage(array $rules) {
     $rule_ids = mpull($rules, 'getID');
 
+    // Filter out any rules that have invalid adapters, or have adapters the
+    // viewer isn't permitted to see or use (for example, Differential rules
+    // if the user can't use Differential or Differential is not installed).
+    $types = HeraldAdapter::getEnabledAdapterMap($this->getViewer());
+    foreach ($rules as $key => $rule) {
+      if (empty($types[$rule->getContentType()])) {
+        unset($rules[$key]);
+      }
+    }
+
     if ($this->needValidateAuthors) {
       $this->validateRuleAuthors($rules);
     }

src/applications/herald/query/HeraldRuleSearchEngine.php

@@ -110,11 +110,13 @@ final class HeraldRuleSearchEngine
   private function getContentTypeOptions() {
     return array(
       '' => pht('(All Content Types)'),
-    ) + HeraldAdapter::getEnabledAdapterMap();
+    ) + HeraldAdapter::getEnabledAdapterMap($this->requireViewer());
   }
 
   private function getContentTypeValues() {
-    return array_fuse(array_keys(HeraldAdapter::getEnabledAdapterMap()));
+    return array_fuse(
+      array_keys(
+        HeraldAdapter::getEnabledAdapterMap($this->requireViewer())));
   }
 
   private function getRuleTypeOptions() {

src/applications/policy/filter/PhabricatorPolicyFilter.php

@@ -281,6 +281,13 @@ final class PhabricatorPolicyFilter {
       case PhabricatorPolicyCapability::CAN_JOIN:
         $message = pht('You do not have permission to join this object.');
         break;
+      default:
+        // TODO: Farm these out to applications?
+        $message = pht(
+          'You do not have a required capability ("%s") to do whatever you '.
+          'are trying to do.',
+          $capability);
+        break;
     }
 
     switch ($policy) {
@@ -369,7 +376,7 @@ final class PhabricatorPolicyFilter {
     }
 
     $more = array_merge(
-      array($more),
+      array_filter(array($more)),
       array_filter((array)$object->describeAutomaticCapability($capability)));
 
     $exception = new PhabricatorPolicyException($message);