From ee0ad4703ebdc47345ba220923ca42609ed7c3a3 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 28 Feb 2019 11:17:04 -0800 Subject: [PATCH] Make the new Build Plan "Runnable" behavior work Summary: Ref T13258. Fixes T11415. This makes "Runnable" actually do something: - With "Runnable" set to "If Editable" (default): to manually run, pause, resume, abort, or restart a build, you must normally be able to edit the associated build plan. - If you toggle "Runnable" to "If Viewable", anyone who can view the build plan may take these actions. This is pretty straightforward since T9614 already got us pretty close to this ruleset a while ago. Test Plan: - Created a Build Plan, set "Can Edit" to just me, toggled "Runnable" to "If Viewable"/"If Editable", tried to take actions as another user. - With "If Editable", unable to run, pause, resume, abort, or restart as another user. - With "If Viewable", those actions work. Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13258, T11415 Differential Revision: https://secure.phabricator.com/D20229 --- src/__phutil_library_map__.php | 3 ++ .../HarbormasterBuildPlanPolicyCodex.php | 38 ++++++++++++++++ .../HarbormasterPlanRunController.php | 7 +-- .../HarbormasterPlanViewController.php | 2 +- .../plan/HarbormasterBuildPlanBehavior.php | 28 +++++++++--- .../storage/build/HarbormasterBuild.php | 11 +++-- .../configuration/HarbormasterBuildPlan.php | 44 ++++++++++++++++++- 7 files changed, 115 insertions(+), 18 deletions(-) create mode 100644 src/applications/harbormaster/codex/HarbormasterBuildPlanPolicyCodex.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 15c7637e3d..4eedb44b22 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1340,6 +1340,7 @@ phutil_register_library_map(array( 'HarbormasterBuildPlanNameNgrams' => 'applications/harbormaster/storage/configuration/HarbormasterBuildPlanNameNgrams.php', 'HarbormasterBuildPlanNameTransaction' => 'applications/harbormaster/xaction/plan/HarbormasterBuildPlanNameTransaction.php', 'HarbormasterBuildPlanPHIDType' => 'applications/harbormaster/phid/HarbormasterBuildPlanPHIDType.php', + 'HarbormasterBuildPlanPolicyCodex' => 'applications/harbormaster/codex/HarbormasterBuildPlanPolicyCodex.php', 'HarbormasterBuildPlanQuery' => 'applications/harbormaster/query/HarbormasterBuildPlanQuery.php', 'HarbormasterBuildPlanSearchAPIMethod' => 'applications/harbormaster/conduit/HarbormasterBuildPlanSearchAPIMethod.php', 'HarbormasterBuildPlanSearchEngine' => 'applications/harbormaster/query/HarbormasterBuildPlanSearchEngine.php', @@ -6945,6 +6946,7 @@ phutil_register_library_map(array( 'PhabricatorNgramsInterface', 'PhabricatorConduitResultInterface', 'PhabricatorProjectInterface', + 'PhabricatorPolicyCodexInterface', ), 'HarbormasterBuildPlanBehavior' => 'Phobject', 'HarbormasterBuildPlanBehaviorOption' => 'Phobject', @@ -6958,6 +6960,7 @@ phutil_register_library_map(array( 'HarbormasterBuildPlanNameNgrams' => 'PhabricatorSearchNgrams', 'HarbormasterBuildPlanNameTransaction' => 'HarbormasterBuildPlanTransactionType', 'HarbormasterBuildPlanPHIDType' => 'PhabricatorPHIDType', + 'HarbormasterBuildPlanPolicyCodex' => 'PhabricatorPolicyCodex', 'HarbormasterBuildPlanQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'HarbormasterBuildPlanSearchAPIMethod' => 'PhabricatorSearchEngineAPIMethod', 'HarbormasterBuildPlanSearchEngine' => 'PhabricatorApplicationSearchEngine', diff --git a/src/applications/harbormaster/codex/HarbormasterBuildPlanPolicyCodex.php b/src/applications/harbormaster/codex/HarbormasterBuildPlanPolicyCodex.php new file mode 100644 index 0000000000..a17f2fb293 --- /dev/null +++ b/src/applications/harbormaster/codex/HarbormasterBuildPlanPolicyCodex.php @@ -0,0 +1,38 @@ +getObject(); + $run_with_view = $object->canRunWithoutEditCapability(); + + $rules = array(); + + $rules[] = $this->newRule() + ->setCapabilities( + array( + PhabricatorPolicyCapability::CAN_EDIT, + )) + ->setIsActive(!$run_with_view) + ->setDescription( + pht( + 'You must have edit permission on this build plan to pause, '. + 'abort, resume, or restart it.')); + + $rules[] = $this->newRule() + ->setCapabilities( + array( + PhabricatorPolicyCapability::CAN_EDIT, + )) + ->setIsActive(!$run_with_view) + ->setDescription( + pht( + 'You must have edit permission on this build plan to run it '. + 'manually.')); + + return $rules; + } + + +} diff --git a/src/applications/harbormaster/controller/HarbormasterPlanRunController.php b/src/applications/harbormaster/controller/HarbormasterPlanRunController.php index fd227ee554..5d80d421aa 100644 --- a/src/applications/harbormaster/controller/HarbormasterPlanRunController.php +++ b/src/applications/harbormaster/controller/HarbormasterPlanRunController.php @@ -9,16 +9,13 @@ final class HarbormasterPlanRunController extends HarbormasterPlanController { $plan = id(new HarbormasterBuildPlanQuery()) ->setViewer($viewer) ->withIDs(array($plan_id)) - ->requireCapabilities( - array( - PhabricatorPolicyCapability::CAN_VIEW, - PhabricatorPolicyCapability::CAN_EDIT, - )) ->executeOne(); if (!$plan) { return new Aphront404Response(); } + $plan->assertHasRunCapability($viewer); + $cancel_uri = $this->getApplicationURI("plan/{$plan_id}/"); if (!$plan->canRunManually()) { diff --git a/src/applications/harbormaster/controller/HarbormasterPlanViewController.php b/src/applications/harbormaster/controller/HarbormasterPlanViewController.php index 0ef6162aad..141f321caa 100644 --- a/src/applications/harbormaster/controller/HarbormasterPlanViewController.php +++ b/src/applications/harbormaster/controller/HarbormasterPlanViewController.php @@ -266,7 +266,7 @@ final class HarbormasterPlanViewController extends HarbormasterPlanController { ->setIcon('fa-ban')); } - $can_run = ($can_edit && $plan->canRunManually()); + $can_run = ($plan->hasRunCapability($viewer) && $plan->canRunManually()); $curtain->addAction( id(new PhabricatorActionView()) diff --git a/src/applications/harbormaster/plan/HarbormasterBuildPlanBehavior.php b/src/applications/harbormaster/plan/HarbormasterBuildPlanBehavior.php index b0f722a1ca..893d351065 100644 --- a/src/applications/harbormaster/plan/HarbormasterBuildPlanBehavior.php +++ b/src/applications/harbormaster/plan/HarbormasterBuildPlanBehavior.php @@ -9,6 +9,10 @@ final class HarbormasterBuildPlanBehavior private $defaultKey; private $editInstructions; + const BEHAVIOR_RUNNABLE = 'runnable'; + const RUNNABLE_IF_VIEWABLE = 'view'; + const RUNNABLE_IF_EDITABLE = 'edit'; + public function setKey($key) { $this->key = $key; return $this; @@ -114,6 +118,19 @@ final class HarbormasterBuildPlanBehavior return sprintf('behavior.%s', $behavior_key); } + public static function getBehavior($key) { + $behaviors = self::newPlanBehaviors(); + + if (!isset($behaviors[$key])) { + throw new Exception( + pht( + 'No build plan behavior with key "%s" exists.', + $key)); + } + + return $behaviors[$key]; + } + public static function newPlanBehaviors() { $draft_options = array( id(new HarbormasterBuildPlanBehaviorOption()) @@ -224,14 +241,14 @@ final class HarbormasterBuildPlanBehavior $run_options = array( id(new HarbormasterBuildPlanBehaviorOption()) - ->setKey('edit') + ->setKey(self::RUNNABLE_IF_EDITABLE) ->setIcon('fa-pencil green') ->setName(pht('If Editable')) ->setIsDefault(true) ->setDescription( pht('Only users who can edit the plan can run it manually.')), id(new HarbormasterBuildPlanBehaviorOption()) - ->setKey('view') + ->setKey(self::RUNNABLE_IF_VIEWABLE) ->setIcon('fa-exclamation-triangle yellow') ->setName(pht('If Viewable')) ->setDescription( @@ -315,7 +332,7 @@ final class HarbormasterBuildPlanBehavior ->setName(pht('Restartable')) ->setOptions($restart_options), id(new self()) - ->setKey('runnable') + ->setKey(self::BEHAVIOR_RUNNABLE) ->setEditInstructions( pht( 'To run a build manually, you normally must have permission to '. @@ -323,9 +340,8 @@ final class HarbormasterBuildPlanBehavior 'can see the build plan be able to run and restart the build, you '. 'can change the behavior here.'. "\n\n". - 'Note that this affects both {nav Run Plan Manually} and '. - '{nav Restart Build}, since the two actions are largely '. - 'equivalent.'. + 'Note that this controls access to all build management actions: '. + '"Run Plan Manually", "Restart", "Abort", "Pause", and "Resume".'. "\n\n". 'WARNING: This may be unsafe, particularly if the build has '. 'side effects like deployment.'. diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuild.php b/src/applications/harbormaster/storage/build/HarbormasterBuild.php index 84668b79f0..9b7b64d06b 100644 --- a/src/applications/harbormaster/storage/build/HarbormasterBuild.php +++ b/src/applications/harbormaster/storage/build/HarbormasterBuild.php @@ -334,14 +334,17 @@ final class HarbormasterBuild extends HarbormasterDAO } public function assertCanIssueCommand(PhabricatorUser $viewer, $command) { - $need_edit = false; + $plan = $this->getBuildPlan(); + + $need_edit = true; switch ($command) { case HarbormasterBuildCommand::COMMAND_RESTART: - break; case HarbormasterBuildCommand::COMMAND_PAUSE: case HarbormasterBuildCommand::COMMAND_RESUME: case HarbormasterBuildCommand::COMMAND_ABORT: - $need_edit = true; + if ($plan->canRunWithoutEditCapability()) { + $need_edit = false; + } break; default: throw new Exception( @@ -355,7 +358,7 @@ final class HarbormasterBuild extends HarbormasterDAO if ($need_edit) { PhabricatorPolicyFilter::requireCapability( $viewer, - $this->getBuildPlan(), + $plan, PhabricatorPolicyCapability::CAN_EDIT); } } diff --git a/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php b/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php index faecb79a3a..798201f490 100644 --- a/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php +++ b/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php @@ -10,7 +10,8 @@ final class HarbormasterBuildPlan extends HarbormasterDAO PhabricatorSubscribableInterface, PhabricatorNgramsInterface, PhabricatorConduitResultInterface, - PhabricatorProjectInterface { + PhabricatorProjectInterface, + PhabricatorPolicyCodexInterface { protected $name; protected $planStatus; @@ -133,7 +134,6 @@ final class HarbormasterBuildPlan extends HarbormasterDAO return true; } - public function getName() { $autoplan = $this->getAutoplan(); if ($autoplan) { @@ -143,6 +143,38 @@ final class HarbormasterBuildPlan extends HarbormasterDAO return parent::getName(); } + public function hasRunCapability(PhabricatorUser $viewer) { + try { + $this->assertHasRunCapability($viewer); + return true; + } catch (PhabricatorPolicyException $ex) { + return false; + } + } + + public function canRunWithoutEditCapability() { + $runnable = HarbormasterBuildPlanBehavior::BEHAVIOR_RUNNABLE; + $if_viewable = HarbormasterBuildPlanBehavior::RUNNABLE_IF_VIEWABLE; + + $option = HarbormasterBuildPlanBehavior::getBehavior($runnable) + ->getPlanOption($this); + + return ($option->getKey() === $if_viewable); + } + + public function assertHasRunCapability(PhabricatorUser $viewer) { + if ($this->canRunWithoutEditCapability()) { + $capability = PhabricatorPolicyCapability::CAN_VIEW; + } else { + $capability = PhabricatorPolicyCapability::CAN_EDIT; + } + + PhabricatorPolicyFilter::requireCapability( + $viewer, + $this, + $capability); + } + /* -( PhabricatorSubscribableInterface )----------------------------------- */ @@ -265,4 +297,12 @@ final class HarbormasterBuildPlan extends HarbormasterDAO return array(); } + +/* -( PhabricatorPolicyCodexInterface )------------------------------------ */ + + + public function newPolicyCodex() { + return new HarbormasterBuildPlanPolicyCodex(); + } + } -- 2.51.2