From c207964036ceea2f608b1aa9da17b348011c2083 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 19 Nov 2013 14:10:38 -0800
Subject: [PATCH] Never raise policy exceptions for the omnipotent viewer

Summary:
Fixes T4109. If a revision has a bad `repositoryPHID` (for example, because the repository was deleted), `DifferentialRevisionQuery` calls `didRejectResult()` on it, which raises a policy exception, even if the viewer is omnipotent. This aborts the `MessageParser`, because it does not expect policy exceptions to be raised for an omnipotent viewer.

Fix this in two ways:

  # Never raise a policy exception for an omnipotent viewer. I think this is the expected behavior and a reasonable rule.
  # In this case, load the revision for an omnipotent viewer.

This feels a little gross, but it's the only place where we do this in the codebase right now. We can clean this up later on once it's more clear what the circumstances of checks like these are.

Test Plan: Set a revision to have an invalid `repositoryPHID`, ran message parser on it, got a clean parse.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4109

Differential Revision: https://secure.phabricator.com/D7603
---
 .../differential/query/DifferentialRevisionQuery.php     | 7 +++++++
 .../policy/filter/PhabricatorPolicyFilter.php            | 9 +++++++++
 2 files changed, 16 insertions(+)

diff --git a/src/applications/differential/query/DifferentialRevisionQuery.php b/src/applications/differential/query/DifferentialRevisionQuery.php
index e124801d11..de52a5cf74 100644
--- a/src/applications/differential/query/DifferentialRevisionQuery.php
+++ b/src/applications/differential/query/DifferentialRevisionQuery.php
@@ -429,6 +429,13 @@ final class DifferentialRevisionQuery
         continue;
       }
 
+      if ($this->getViewer()->isOmnipotent()) {
+        // The viewer is omnipotent. Allow the revision to load even without
+        // a repository.
+        $revision->attachRepository(null);
+        continue;
+      }
+
       // The revision has an associated repository, and the viewer can't see
       // it, and the viewer has no special capabilities. Filter out this
       // revision.
diff --git a/src/applications/policy/filter/PhabricatorPolicyFilter.php b/src/applications/policy/filter/PhabricatorPolicyFilter.php
index cdaf8af816..969e167d30 100644
--- a/src/applications/policy/filter/PhabricatorPolicyFilter.php
+++ b/src/applications/policy/filter/PhabricatorPolicyFilter.php
@@ -259,6 +259,15 @@ final class PhabricatorPolicyFilter {
       return;
     }
 
+    if ($this->viewer->isOmnipotent()) {
+      // Never raise policy exceptions for the omnipotent viewer. Although we
+      // will never normally issue a policy rejection for the omnipotent
+      // viewer, we can end up here when queries blanket reject objects that
+      // have failed to load, without distinguishing between nonexistent and
+      // nonvisible objects.
+      return;
+    }
+
     $capobj = PhabricatorPolicyCapability::getCapabilityByKey($capability);
     $rejection = null;
     if ($capobj) {