From 5058cfb9726f6f4d9b93e2da3275fb415edbde69 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 25 Jan 2018 11:24:21 -0800
Subject: [PATCH] Pass a real viewer to HeraldAdapter when doing test console
 runs

Summary:
Depends on D18932. Ref T13048. See PHI276. In the cluster, we don't have device keys on `web` nodes. This is generally good, since they don't need them, and it means that we aren't putting more credentials than we need on those hosts.

However, it means that when we pull diff content to test "Commit" rules via the Herald test console, we use the omnipotent user and try to use device credentials, and this fails since we don't have any.

Instead, pass the real viewer in this case so we just sign the request as them, like we do for normal Diffusion requests.

Test Plan:
Wrote and ran a commit content rule locally, no issues.

This isn't completely convincing since my local setup does have device credentials, but I'll double-check in production once this deploys.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13048

Differential Revision: https://secure.phabricator.com/D18933
---
 .../diffusion/herald/HeraldCommitAdapter.php  |  2 +-
 .../herald/adapter/HeraldAdapter.php          | 20 +++++++++++++++++++
 .../HeraldTestConsoleController.php           |  4 +++-
 3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/src/applications/diffusion/herald/HeraldCommitAdapter.php b/src/applications/diffusion/herald/HeraldCommitAdapter.php
index bfcd8e3ccb..e0e15352b6 100644
--- a/src/applications/diffusion/herald/HeraldCommitAdapter.php
+++ b/src/applications/diffusion/herald/HeraldCommitAdapter.php
@@ -209,7 +209,7 @@ final class HeraldCommitAdapter
   }
 
   private function loadCommitDiff() {
-    $viewer = PhabricatorUser::getOmnipotentUser();
+    $viewer = $this->getViewer();
 
     $byte_limit = self::getEnormousByteLimit();
     $time_limit = self::getEnormousTimeLimit();
diff --git a/src/applications/herald/adapter/HeraldAdapter.php b/src/applications/herald/adapter/HeraldAdapter.php
index d67c69f584..9d56f474ff 100644
--- a/src/applications/herald/adapter/HeraldAdapter.php
+++ b/src/applications/herald/adapter/HeraldAdapter.php
@@ -38,6 +38,7 @@ abstract class HeraldAdapter extends Phobject {
   private $actionMap;
   private $edgeCache = array();
   private $forbiddenActions = array();
+  private $viewer;
 
   public function getEmailPHIDs() {
     return array_values($this->emailPHIDs);
@@ -55,10 +56,29 @@ abstract class HeraldAdapter extends Phobject {
     return $this;
   }
 
+  public function setViewer(PhabricatorUser $viewer) {
+    $this->viewer = $viewer;
+    return $this;
+  }
+
+  public function getViewer() {
+    // See PHI276. Normally, Herald runs without regard for policy checks.
+    // However, we use a real viewer during test console runs: this makes
+    // intracluster calls to Diffusion APIs work even if web nodes don't
+    // have privileged credentials.
+
+    if ($this->viewer) {
+      return $this->viewer;
+    }
+
+    return PhabricatorUser::getOmnipotentUser();
+  }
+
   public function setContentSource(PhabricatorContentSource $content_source) {
     $this->contentSource = $content_source;
     return $this;
   }
+
   public function getContentSource() {
     return $this->contentSource;
   }
diff --git a/src/applications/herald/controller/HeraldTestConsoleController.php b/src/applications/herald/controller/HeraldTestConsoleController.php
index 21bedcd848..8a7a94963d 100644
--- a/src/applications/herald/controller/HeraldTestConsoleController.php
+++ b/src/applications/herald/controller/HeraldTestConsoleController.php
@@ -39,7 +39,9 @@ final class HeraldTestConsoleController extends HeraldController {
     $object = $this->getTestObject();
     $adapter = $this->getTestAdapter();
 
-    $adapter->setIsNewObject(false);
+    $adapter
+      ->setIsNewObject(false)
+      ->setViewer($viewer);
 
     $rules = id(new HeraldRuleQuery())
       ->setViewer($viewer)