From 304467feb2aee2af4991aad6d5245fec35441659 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 30 Apr 2020 07:41:50 -0700
Subject: [PATCH] Stabilize fatals when a build has a build plan the viewer
 can't see because of policy restrictions

Summary:
Ref T13526. Currently, if a build plan is restricted, viewers may fatal when trying to view related builds.

The old behavior allowed them to see the build even if they can not see the build plan. This is sort of incoherent, but try to stabilize things before fixing this.

Test Plan:
This is a muddy change.

  - Created a build with a build plan that Alice can't see.
  - As Alice, viewed the build page (restricted before, restricted after); the buildable page (fatal before, works after).
  - Also viewed a revision page (works before and after, but user-reported fatal).

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13526

Differential Revision: https://secure.phabricator.com/D21194
---
 .../storage/DifferentialRevision.php             | 15 ++++++++++++---
 .../storage/build/HarbormasterBuild.php          | 16 ++++++++++++++++
 2 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php
index 7baff4889d..2ab9d59821 100644
--- a/src/applications/differential/storage/DifferentialRevision.php
+++ b/src/applications/differential/storage/DifferentialRevision.php
@@ -870,10 +870,19 @@ final class DifferentialRevision extends DifferentialDAO
 
     foreach ($builds as $key => $build) {
       $plan = $build->getBuildPlan();
-      $hold_key = $behavior->getPlanOption($plan)->getKey();
 
-      $hold_never = ($hold_key === $key_never);
-      $hold_building = ($hold_key === $key_building);
+      // See T13526. If the viewer can't see the build plan, pretend it has
+      // generic options. This is often wrong, but "often wrong" is better than
+      // "fatal".
+      if ($plan) {
+        $hold_key = $behavior->getPlanOption($plan)->getKey();
+
+        $hold_never = ($hold_key === $key_never);
+        $hold_building = ($hold_key === $key_building);
+      } else {
+        $hold_never = false;
+        $hold_building = false;
+      }
 
       // If the build "Never" holds drafts from promoting, we don't care what
       // the status is.
diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuild.php b/src/applications/harbormaster/storage/build/HarbormasterBuild.php
index 70c26827ec..8243d2a577 100644
--- a/src/applications/harbormaster/storage/build/HarbormasterBuild.php
+++ b/src/applications/harbormaster/storage/build/HarbormasterBuild.php
@@ -235,6 +235,16 @@ final class HarbormasterBuild extends HarbormasterDAO
     $restartable = HarbormasterBuildPlanBehavior::BEHAVIOR_RESTARTABLE;
     $plan = $this->getBuildPlan();
 
+    // See T13526. Users who can't see the "BuildPlan" can end up here with
+    // no object. This is highly questionable.
+    if (!$plan) {
+      throw new HarbormasterRestartException(
+        pht('No Build Plan Permission'),
+        pht(
+          'You can not restart this build because you do not have '.
+          'permission to access the build plan.'));
+    }
+
     $option = HarbormasterBuildPlanBehavior::getBehavior($restartable)
       ->getPlanOption($plan);
     $option_key = $option->getKey();
@@ -389,6 +399,12 @@ final class HarbormasterBuild extends HarbormasterDAO
   public function assertCanIssueCommand(PhabricatorUser $viewer, $command) {
     $plan = $this->getBuildPlan();
 
+    // See T13526. Users without permission to access the build plan can
+    // currently end up here with no "BuildPlan" object.
+    if (!$plan) {
+      return false;
+    }
+
     $need_edit = true;
     switch ($command) {
       case HarbormasterBuildCommand::COMMAND_RESTART: