Give Harbormaster Build Plans real policies

Summary:
Ref T9614. Currently, a lot of Build Plan behavior is covered by a global "can manage" policy.

One install in particular is experiencing difficulty with warring factions within engineering aborting one another's builds.

As a first step to remedy this, and also generally make Harbormaster more flexible and bring it in line with other applications in terms of policy power:

  - Give Build Plans normal view/edit policies.
  - Require "Can Edit" to run a plan manually.

Having "Can View" on plans may be a little weird in some cases (the status of a Buildable might be bad because of a build you can't see) but we can cross that bridge when we come to it.

Next change here will require "Can Edit" to abort a build. This will reasonably allow installs to reserve pause/abort for administrators/adults. (I might let anyone restart a plan, though?)

Test Plan:
  - Created a new build plan.
  - Verified defaults were inherited from application defaults (swapped them around, too).
  - Saved build plan.
  - Edited policies.
  - Verified autoplans get the right policies.
  - Verified old plans got migrated properly.
  - Tried to run a plan I couldn't edit (denied).
  - Ran a plan from CLI with `bin/harbormaster`.
  - Tried to create a plan with an unprivileged user.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9614

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

resources/sql/autopatches/20151023.harborpolicy.1.sql

@@ -0,0 +1,5 @@
+ALTER TABLE {$NAMESPACE}_harbormaster.harbormaster_buildplan
+  ADD viewPolicy VARBINARY(64) NOT NULL;
+
+ALTER TABLE {$NAMESPACE}_harbormaster.harbormaster_buildplan
+  ADD editPolicy VARBINARY(64) NOT NULL;

resources/sql/autopatches/20151023.harborpolicy.2.php

@@ -0,0 +1,21 @@
+<?php
+
+$table = new HarbormasterBuildPlan();
+$conn_w = $table->establishConnection('w');
+
+$view_policy = PhabricatorPolicies::getMostOpenPolicy();
+queryfx(
+  $conn_w,
+  'UPDATE %T SET viewPolicy = %s WHERE viewPolicy = %s',
+  $table->getTableName(),
+  $view_policy,
+  '');
+
+$edit_policy = id(new PhabricatorHarbormasterApplication())
+  ->getPolicy(HarbormasterCreatePlansCapability::CAPABILITY);
+queryfx(
+  $conn_w,
+  'UPDATE %T SET editPolicy = %s WHERE editPolicy = %s',
+  $table->getTableName(),
+  $edit_policy,
+  '');

src/__phutil_library_map__.php

@@ -991,6 +991,8 @@ phutil_register_library_map(array(
     'HarbormasterBuildPHIDType' => 'applications/harbormaster/phid/HarbormasterBuildPHIDType.php',
     'HarbormasterBuildPlan' => 'applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php',
     'HarbormasterBuildPlanDatasource' => 'applications/harbormaster/typeahead/HarbormasterBuildPlanDatasource.php',
+    'HarbormasterBuildPlanDefaultEditCapability' => 'applications/harbormaster/capability/HarbormasterBuildPlanDefaultEditCapability.php',
+    'HarbormasterBuildPlanDefaultViewCapability' => 'applications/harbormaster/capability/HarbormasterBuildPlanDefaultViewCapability.php',
     'HarbormasterBuildPlanEditor' => 'applications/harbormaster/editor/HarbormasterBuildPlanEditor.php',
     'HarbormasterBuildPlanPHIDType' => 'applications/harbormaster/phid/HarbormasterBuildPlanPHIDType.php',
     'HarbormasterBuildPlanQuery' => 'applications/harbormaster/query/HarbormasterBuildPlanQuery.php',
@@ -1036,6 +1038,7 @@ phutil_register_library_map(array(
     'HarbormasterConduitAPIMethod' => 'applications/harbormaster/conduit/HarbormasterConduitAPIMethod.php',
     'HarbormasterController' => 'applications/harbormaster/controller/HarbormasterController.php',
     'HarbormasterCreateArtifactConduitAPIMethod' => 'applications/harbormaster/conduit/HarbormasterCreateArtifactConduitAPIMethod.php',
+    'HarbormasterCreatePlansCapability' => 'applications/harbormaster/capability/HarbormasterCreatePlansCapability.php',
     'HarbormasterDAO' => 'applications/harbormaster/storage/HarbormasterDAO.php',
     'HarbormasterDrydockBuildStepGroup' => 'applications/harbormaster/stepgroup/HarbormasterDrydockBuildStepGroup.php',
     'HarbormasterDrydockCommandBuildStepImplementation' => 'applications/harbormaster/step/HarbormasterDrydockCommandBuildStepImplementation.php',
@@ -1049,7 +1052,6 @@ phutil_register_library_map(array(
     'HarbormasterLeaseWorkingCopyBuildStepImplementation' => 'applications/harbormaster/step/HarbormasterLeaseWorkingCopyBuildStepImplementation.php',
     'HarbormasterLintMessagesController' => 'applications/harbormaster/controller/HarbormasterLintMessagesController.php',
     'HarbormasterLintPropertyView' => 'applications/harbormaster/view/HarbormasterLintPropertyView.php',
-    'HarbormasterManagePlansCapability' => 'applications/harbormaster/capability/HarbormasterManagePlansCapability.php',
     'HarbormasterManagementBuildWorkflow' => 'applications/harbormaster/management/HarbormasterManagementBuildWorkflow.php',
     'HarbormasterManagementUpdateWorkflow' => 'applications/harbormaster/management/HarbormasterManagementUpdateWorkflow.php',
     'HarbormasterManagementWorkflow' => 'applications/harbormaster/management/HarbormasterManagementWorkflow.php',
@@ -4816,6 +4818,8 @@ phutil_register_library_map(array(
       'PhabricatorSubscribableInterface',
     ),
     'HarbormasterBuildPlanDatasource' => 'PhabricatorTypeaheadDatasource',
+    'HarbormasterBuildPlanDefaultEditCapability' => 'PhabricatorPolicyCapability',
+    'HarbormasterBuildPlanDefaultViewCapability' => 'PhabricatorPolicyCapability',
     'HarbormasterBuildPlanEditor' => 'PhabricatorApplicationTransactionEditor',
     'HarbormasterBuildPlanPHIDType' => 'PhabricatorPHIDType',
     'HarbormasterBuildPlanQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
@@ -4875,6 +4879,7 @@ phutil_register_library_map(array(
     'HarbormasterConduitAPIMethod' => 'ConduitAPIMethod',
     'HarbormasterController' => 'PhabricatorController',
     'HarbormasterCreateArtifactConduitAPIMethod' => 'HarbormasterConduitAPIMethod',
+    'HarbormasterCreatePlansCapability' => 'PhabricatorPolicyCapability',
     'HarbormasterDAO' => 'PhabricatorLiskDAO',
     'HarbormasterDrydockBuildStepGroup' => 'HarbormasterBuildStepGroup',
     'HarbormasterDrydockCommandBuildStepImplementation' => 'HarbormasterBuildStepImplementation',
@@ -4888,7 +4893,6 @@ phutil_register_library_map(array(
     'HarbormasterLeaseWorkingCopyBuildStepImplementation' => 'HarbormasterBuildStepImplementation',
     'HarbormasterLintMessagesController' => 'HarbormasterController',
     'HarbormasterLintPropertyView' => 'AphrontView',
-    'HarbormasterManagePlansCapability' => 'PhabricatorPolicyCapability',
     'HarbormasterManagementBuildWorkflow' => 'HarbormasterManagementWorkflow',
     'HarbormasterManagementUpdateWorkflow' => 'HarbormasterManagementWorkflow',
     'HarbormasterManagementWorkflow' => 'PhabricatorManagementWorkflow',

src/applications/harbormaster/application/PhabricatorHarbormasterApplication.php

@@ -95,8 +95,16 @@ final class PhabricatorHarbormasterApplication extends PhabricatorApplication {
 
   protected function getCustomCapabilities() {
     return array(
-      HarbormasterManagePlansCapability::CAPABILITY => array(
-        'caption' => pht('Can create and manage build plans.'),
+      HarbormasterCreatePlansCapability::CAPABILITY => array(
+        'default' => PhabricatorPolicies::POLICY_ADMIN,
+      ),
+      HarbormasterBuildPlanDefaultViewCapability::CAPABILITY => array(
+        'template' => HarbormasterBuildPlanPHIDType::TYPECONST,
+        'capability' => PhabricatorPolicyCapability::CAN_VIEW,
+      ),
+      HarbormasterBuildPlanDefaultEditCapability::CAPABILITY => array(
+        'template' => HarbormasterBuildPlanPHIDType::TYPECONST,
+        'capability' => PhabricatorPolicyCapability::CAN_EDIT,
         'default' => PhabricatorPolicies::POLICY_ADMIN,
       ),
     );

src/applications/harbormaster/capability/HarbormasterBuildPlanDefaultEditCapability.php

@@ -0,0 +1,12 @@
+<?php
+
+final class HarbormasterBuildPlanDefaultEditCapability
+  extends PhabricatorPolicyCapability {
+
+  const CAPABILITY = 'harbormaster.plan.default.edit';
+
+  public function getCapabilityName() {
+    return pht('Default Build Plan Edit Policy');
+  }
+
+}

src/applications/harbormaster/capability/HarbormasterBuildPlanDefaultViewCapability.php

@@ -0,0 +1,16 @@
+<?php
+
+final class HarbormasterBuildPlanDefaultViewCapability
+  extends PhabricatorPolicyCapability {
+
+  const CAPABILITY = 'harbomaster.plan.default.view';
+
+  public function getCapabilityName() {
+    return pht('Default Build Plan View Policy');
+  }
+
+  public function shouldAllowPublicPolicySetting() {
+    return true;
+  }
+
+}

src/applications/harbormaster/capability/HarbormasterCreatePlansCapability.php

@@ -1,17 +1,17 @@
 <?php
 
-final class HarbormasterManagePlansCapability
+final class HarbormasterCreatePlansCapability
   extends PhabricatorPolicyCapability {
 
   const CAPABILITY = 'harbormaster.plans';
 
   public function getCapabilityName() {
-    return pht('Can Manage Build Plans');
+    return pht('Can Create Build Plans');
   }
 
   public function describeCapabilityRejection() {
     return pht(
-      'You do not have permission to manage Harbormaster build plans.');
+      'You do not have permission to create Harbormaster build plans.');
   }
 
 }

src/applications/harbormaster/controller/HarbormasterPlanDisableController.php

@@ -6,9 +6,6 @@ final class HarbormasterPlanDisableController
   public function handleRequest(AphrontRequest $request) {
     $viewer = $this->getViewer();
 
-    $this->requireApplicationCapability(
-      HarbormasterManagePlansCapability::CAPABILITY);
-
     $plan = id(new HarbormasterBuildPlanQuery())
       ->setViewer($viewer)
       ->withIDs(array($request->getURIData('id')))

src/applications/harbormaster/controller/HarbormasterPlanEditController.php

@@ -5,9 +5,6 @@ final class HarbormasterPlanEditController extends HarbormasterPlanController {
   public function handleRequest(AphrontRequest $request) {
     $viewer = $this->getViewer();
 
-    $this->requireApplicationCapability(
-      HarbormasterManagePlansCapability::CAPABILITY);
-
     $id = $request->getURIData('id');
     if ($id) {
       $plan = id(new HarbormasterBuildPlanQuery())
@@ -23,23 +20,42 @@ final class HarbormasterPlanEditController extends HarbormasterPlanController {
         return new Aphront404Response();
       }
     } else {
+      $this->requireApplicationCapability(
+        HarbormasterCreatePlansCapability::CAPABILITY);
+
       $plan = HarbormasterBuildPlan::initializeNewBuildPlan($viewer);
     }
 
     $e_name = true;
     $v_name = $plan->getName();
+    $v_view = $plan->getViewPolicy();
+    $v_edit = $plan->getEditPolicy();
     $validation_exception = null;
     if ($request->isFormPost()) {
       $xactions = array();
 
       $v_name = $request->getStr('name');
+      $v_view = $request->getStr('viewPolicy');
+      $v_edit = $request->getStr('editPolicy');
+
       $e_name = null;
+
       $type_name = HarbormasterBuildPlanTransaction::TYPE_NAME;
+      $type_view = PhabricatorTransactions::TYPE_VIEW_POLICY;
+      $type_edit = PhabricatorTransactions::TYPE_EDIT_POLICY;
 
       $xactions[] = id(new HarbormasterBuildPlanTransaction())
         ->setTransactionType($type_name)
         ->setNewValue($v_name);
 
+      $xactions[] = id(new HarbormasterBuildPlanTransaction())
+        ->setTransactionType($type_view)
+        ->setNewValue($v_view);
+
+      $xactions[] = id(new HarbormasterBuildPlanTransaction())
+        ->setTransactionType($type_edit)
+        ->setNewValue($v_edit);
+
       $editor = id(new HarbormasterBuildPlanEditor())
         ->setActor($viewer)
         ->setContinueOnNoEffect(true)
@@ -71,19 +87,37 @@ final class HarbormasterPlanEditController extends HarbormasterPlanController {
       $save_button = pht('Save Build Plan');
     }
 
+    $policies = id(new PhabricatorPolicyQuery())
+      ->setViewer($viewer)
+      ->setObject($plan)
+      ->execute();
+
     $form = id(new AphrontFormView())
       ->setUser($viewer)
-      ->appendChild(
+      ->appendControl(
         id(new AphrontFormTextControl())
           ->setLabel(pht('Plan Name'))
           ->setName('name')
           ->setError($e_name)
-          ->setValue($v_name));
-
-    $form->appendChild(
-      id(new AphrontFormSubmitControl())
-        ->setValue($save_button)
-        ->addCancelButton($cancel_uri));
+          ->setValue($v_name))
+      ->appendControl(
+        id(new AphrontFormPolicyControl())
+          ->setCapability(PhabricatorPolicyCapability::CAN_VIEW)
+          ->setPolicyObject($plan)
+          ->setPolicies($policies)
+          ->setValue($v_view)
+          ->setName('viewPolicy'))
+      ->appendControl(
+        id(new AphrontFormPolicyControl())
+          ->setCapability(PhabricatorPolicyCapability::CAN_EDIT)
+          ->setPolicyObject($plan)
+          ->setPolicies($policies)
+          ->setValue($v_edit)
+          ->setName('editPolicy'))
+      ->appendControl(
+        id(new AphrontFormSubmitControl())
+          ->setValue($save_button)
+          ->addCancelButton($cancel_uri));
 
     $box = id(new PHUIObjectBoxView())
       ->setHeaderText($title)

src/applications/harbormaster/controller/HarbormasterPlanListController.php

@@ -42,7 +42,7 @@ final class HarbormasterPlanListController extends HarbormasterPlanController {
     $crumbs = parent::buildApplicationCrumbs();
 
     $can_create = $this->hasApplicationCapability(
-      HarbormasterManagePlansCapability::CAPABILITY);
+      HarbormasterCreatePlansCapability::CAPABILITY);
 
     $crumbs->addAction(
       id(new PHUIListItemView())

src/applications/harbormaster/controller/HarbormasterPlanRunController.php

@@ -4,19 +4,16 @@ final class HarbormasterPlanRunController extends HarbormasterController {
 
   public function handleRequest(AphrontRequest $request) {
     $viewer = $this->getViewer();
-
-    $this->requireApplicationCapability(
-      HarbormasterManagePlansCapability::CAPABILITY);
-
     $plan_id = $request->getURIData('id');
 
-    // NOTE: At least for now, this only requires the "Can Manage Plans"
-    // capability, not the "Can Edit" capability. Possibly it should have
-    // a more stringent requirement, though.
-
     $plan = id(new HarbormasterBuildPlanQuery())
       ->setViewer($viewer)
       ->withIDs(array($plan_id))
+      ->requireCapabilities(
+        array(
+          PhabricatorPolicyCapability::CAN_VIEW,
+          PhabricatorPolicyCapability::CAN_EDIT,
+        ))
       ->executeOne();
     if (!$plan) {
       return new Aphront404Response();

src/applications/harbormaster/controller/HarbormasterPlanViewController.php

@@ -3,7 +3,7 @@
 final class HarbormasterPlanViewController extends HarbormasterPlanController {
 
   public function handleRequest(AphrontRequest $request) {
-    $viewer = $this->getviewer();
+    $viewer = $this->getViewer();
     $id = $request->getURIData('id');
 
     $plan = id(new HarbormasterBuildPlanQuery())
@@ -81,16 +81,11 @@ final class HarbormasterPlanViewController extends HarbormasterPlanController {
       ->execute();
     $steps = mpull($steps, null, 'getPHID');
 
-    $has_manage = $this->hasApplicationCapability(
-      HarbormasterManagePlansCapability::CAPABILITY);
-
-    $has_edit = PhabricatorPolicyFilter::hasCapability(
+    $can_edit = PhabricatorPolicyFilter::hasCapability(
       $viewer,
       $plan,
       PhabricatorPolicyCapability::CAN_EDIT);
 
-    $can_edit = ($has_manage && $has_edit);
-
     $step_list = id(new PHUIObjectItemListView())
       ->setUser($viewer)
       ->setNoDataString(
@@ -252,16 +247,11 @@ final class HarbormasterPlanViewController extends HarbormasterPlanController {
       ->setObject($plan)
       ->setObjectURI($this->getApplicationURI("plan/{$id}/"));
 
-    $has_manage = $this->hasApplicationCapability(
-      HarbormasterManagePlansCapability::CAPABILITY);
-
-    $has_edit = PhabricatorPolicyFilter::hasCapability(
+    $can_edit = PhabricatorPolicyFilter::hasCapability(
       $viewer,
       $plan,
       PhabricatorPolicyCapability::CAN_EDIT);
 
-    $can_edit = ($has_manage && $has_edit);
-
     $list->addAction(
       id(new PhabricatorActionView())
         ->setName(pht('Edit Plan'))
@@ -288,7 +278,7 @@ final class HarbormasterPlanViewController extends HarbormasterPlanController {
           ->setIcon('fa-ban'));
     }
 
-    $can_run = ($has_manage && $plan->canRunManually());
+    $can_run = ($can_edit && $plan->canRunManually());
 
     $list->addAction(
       id(new PhabricatorActionView())

src/applications/harbormaster/controller/HarbormasterStepAddController.php

@@ -5,9 +5,6 @@ final class HarbormasterStepAddController extends HarbormasterController {
   public function handleRequest(AphrontRequest $request) {
     $viewer = $this->getViewer();
 
-    $this->requireApplicationCapability(
-      HarbormasterManagePlansCapability::CAPABILITY);
-
     $plan = id(new HarbormasterBuildPlanQuery())
       ->setViewer($viewer)
       ->withIDs(array($request->getURIData('id')))

src/applications/harbormaster/controller/HarbormasterStepDeleteController.php

@@ -5,9 +5,6 @@ final class HarbormasterStepDeleteController extends HarbormasterController {
   public function handleRequest(AphrontRequest $request) {
     $viewer = $this->getViewer();
 
-    $this->requireApplicationCapability(
-      HarbormasterManagePlansCapability::CAPABILITY);
-
     $id = $request->getURIData('id');
 
     $step = id(new HarbormasterBuildStepQuery())

src/applications/harbormaster/controller/HarbormasterStepEditController.php

@@ -6,9 +6,6 @@ final class HarbormasterStepEditController extends HarbormasterController {
     $viewer = $this->getViewer();
     $id = $request->getURIData('id');
 
-    $this->requireApplicationCapability(
-      HarbormasterManagePlansCapability::CAPABILITY);
-
     if ($id) {
       $step = id(new HarbormasterBuildStepQuery())
         ->setViewer($viewer)

src/applications/harbormaster/editor/HarbormasterBuildPlanEditor.php

@@ -15,7 +15,8 @@ final class HarbormasterBuildPlanEditor
     $types = parent::getTransactionTypes();
     $types[] = HarbormasterBuildPlanTransaction::TYPE_NAME;
     $types[] = HarbormasterBuildPlanTransaction::TYPE_STATUS;
-    $types[] = PhabricatorTransactions::TYPE_COMMENT;
+    $types[] = PhabricatorTransactions::TYPE_VIEW_POLICY;
+    $types[] = PhabricatorTransactions::TYPE_EDIT_POLICY;
     return $types;
   }
 

src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php

@@ -12,6 +12,8 @@ final class HarbormasterBuildPlan extends HarbormasterDAO
   protected $name;
   protected $planStatus;
   protected $planAutoKey;
+  protected $viewPolicy;
+  protected $editPolicy;
 
   const STATUS_ACTIVE   = 'active';
   const STATUS_DISABLED = 'disabled';
@@ -19,10 +21,22 @@ final class HarbormasterBuildPlan extends HarbormasterDAO
   private $buildSteps = self::ATTACHABLE;
 
   public static function initializeNewBuildPlan(PhabricatorUser $actor) {
+    $app = id(new PhabricatorApplicationQuery())
+      ->setViewer($actor)
+      ->withClasses(array('PhabricatorHarbormasterApplication'))
+      ->executeOne();
+
+    $view_policy = $app->getPolicy(
+      HarbormasterBuildPlanDefaultViewCapability::CAPABILITY);
+    $edit_policy = $app->getPolicy(
+      HarbormasterBuildPlanDefaultEditCapability::CAPABILITY);
+
     return id(new HarbormasterBuildPlan())
       ->setName('')
       ->setPlanStatus(self::STATUS_ACTIVE)
-      ->attachBuildSteps(array());
+      ->attachBuildSteps(array())
+      ->setViewPolicy($view_policy)
+      ->setEditPolicy($edit_policy);
   }
 
   protected function getConfiguration() {
@@ -156,16 +170,15 @@ final class HarbormasterBuildPlan extends HarbormasterDAO
   public function getPolicy($capability) {
     switch ($capability) {
       case PhabricatorPolicyCapability::CAN_VIEW:
-        return PhabricatorPolicies::getMostOpenPolicy();
+        if ($this->isAutoplan()) {
+          return PhabricatorPolicies::getMostOpenPolicy();
+        }
+        return $this->getViewPolicy();
       case PhabricatorPolicyCapability::CAN_EDIT:
-        // NOTE: In practice, this policy is always limited by the "Mangage
-        // Build Plans" policy.
-
         if ($this->isAutoplan()) {
           return PhabricatorPolicies::POLICY_NOONE;
         }
-
-        return PhabricatorPolicies::getMostOpenPolicy();
+        return $this->getEditPolicy();
     }
   }