From fa6cd200e87823a7fd05480015a80ced233d9cd3 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 16 Mar 2018 13:18:35 -0700
Subject: [PATCH] Reduce the severity of policy fatals when building the
 Harbormaster "build status" element

Summary:
See PHI430. Ref T13102. When the "Build Status" element raises a policy exception, we currently fatal the whole page rather than raising a normal policy error.

This is because the policy check happens very late in page construction, long after we've made the decision to show the page instead of a policy error, and gets treated as a rendering error.

In turn, this is because the rendering is event-based rather than using a more modern Engine + EngineExtension sort of construct, so some of the actual logic runs way later than it should.

Since unwinding all of this isn't trivial and the current behavior is materially bad, limit the damage here for now by just hiding the element. See T13088 for notes on handling this in a more nuanced way in the future.

Test Plan:
  - Created a revision visible to "Public".
  - Ran a build against it with a build plan visible to "All Users".
  - Viewed revision in an incognito window.
    - Before patch: Policy fatal with a red "rendering phase" error box.
    - After patch: Mostly-functional page with a missing "Build Status" element.
  - Viewed revision as a user with a normal session, saw the same UI before and after the change.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13102

Differential Revision: https://secure.phabricator.com/D19232
---
 .../event/HarbormasterUIEventListener.php     | 23 +++++++++++++------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/src/applications/harbormaster/event/HarbormasterUIEventListener.php b/src/applications/harbormaster/event/HarbormasterUIEventListener.php
index 1227fbbe31..f5e6aee3e7 100644
--- a/src/applications/harbormaster/event/HarbormasterUIEventListener.php
+++ b/src/applications/harbormaster/event/HarbormasterUIEventListener.php
@@ -51,13 +51,22 @@ final class HarbormasterUIEventListener
       return;
     }
 
-    $buildable = id(new HarbormasterBuildableQuery())
-      ->setViewer($viewer)
-      ->withManualBuildables(false)
-      ->withBuildablePHIDs(array($buildable_phid))
-      ->needBuilds(true)
-      ->needTargets(true)
-      ->executeOne();
+    try {
+      $buildable = id(new HarbormasterBuildableQuery())
+        ->setViewer($viewer)
+        ->withManualBuildables(false)
+        ->withBuildablePHIDs(array($buildable_phid))
+        ->needBuilds(true)
+        ->needTargets(true)
+        ->executeOne();
+    } catch (PhabricatorPolicyException $ex) {
+      // TODO: See PHI430. When this query raises a policy exception, it
+      // fatals the whole page because it happens very late in execution,
+      // during final page rendering. If the viewer can't see the buildable or
+      // some of the builds, just hide this element for now.
+      return;
+    }
+
     if (!$buildable) {
       return;
     }