From f16778fc18b7ca096093c6f3108b7ea31832fd63 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Sun, 8 Jan 2017 10:53:35 -0800
Subject: [PATCH] Fix excessively strict "Can Use Application" policy filtering

Summary:
Ref T9058. The stricter filtering is over-filtering Handles. For example, in the Phacility cluster, users can not see Almanac services.

So this filtering happens:

  - The AlmanacServiceQuery filters the service beacuse they can't see the application.
  - The HandleQuery generates a "you can't see this" handle.
  - But then the HandleQuery filters that handle! It has a "service" PHID and the user can't see Almanac.

This violates the assumption that all application code makes about handles: it's OK to query handles for objects you can't see, and you'll get something back.

Instead, don't do application filtering on handles.

Test Plan:
  - Added a failing test and made it pass.
  - As a user who can not see Almanac, viewed an Instances timeline.
    - Before patch: fatal on trying to load a handle for a Service.
    - After patch: smooth sailing.

Reviewers: chad

Maniphest Tasks: T9058

Differential Revision: https://secure.phabricator.com/D17152
---
 .../policy/filter/PhabricatorPolicyFilter.php | 13 ++++++++++++
 .../PhabricatorProjectCoreTestCase.php        | 20 +++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/src/applications/policy/filter/PhabricatorPolicyFilter.php b/src/applications/policy/filter/PhabricatorPolicyFilter.php
index 855149f6b3..e0c223cd2c 100644
--- a/src/applications/policy/filter/PhabricatorPolicyFilter.php
+++ b/src/applications/policy/filter/PhabricatorPolicyFilter.php
@@ -874,6 +874,19 @@ final class PhabricatorPolicyFilter extends Phobject {
     $viewer = $this->viewer;
 
     foreach ($objects as $key => $object) {
+      // Don't filter handles: users are allowed to see handles from an
+      // application they can't see even if they can not see objects from
+      // that application. Note that the application policies still apply to
+      // the underlying object, so these will be "Restricted Object" handles.
+
+      // If we don't let these through, PhabricatorHandleQuery will completely
+      // fail to load results for PHIDs that are part of applications which
+      // the viewer can not see, but a fundamental property of handles is that
+      // we always load something and they can safely be assumed to load.
+      if ($object instanceof PhabricatorObjectHandle) {
+        continue;
+      }
+
       $phid = $object->getPHID();
       if (!$phid) {
         continue;
diff --git a/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php b/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php
index cb977dd681..71161b031e 100644
--- a/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php
+++ b/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php
@@ -51,6 +51,13 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase {
         $proj,
         PhabricatorPolicyCapability::CAN_VIEW));
 
+    // This object is visible so its handle should load normally.
+    $handle = id(new PhabricatorHandleQuery())
+      ->setViewer($user)
+      ->withPHIDs(array($proj->getPHID()))
+      ->executeOne();
+    $this->assertEqual($proj->getPHID(), $handle->getPHID());
+
     // Change the "Can Use Application" policy for Projecs to "No One". This
     // should cause filtering checks to fail even when they are executed
     // directly rather than via a Query.
@@ -76,6 +83,19 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase {
         $proj,
         PhabricatorPolicyCapability::CAN_VIEW));
 
+    // We should still be able to load a handle for the project, even if we
+    // can not see the application.
+    $handle = id(new PhabricatorHandleQuery())
+      ->setViewer($user)
+      ->withPHIDs(array($proj->getPHID()))
+      ->executeOne();
+
+    // The handle should load...
+    $this->assertEqual($proj->getPHID(), $handle->getPHID());
+
+    // ...but be policy filtered.
+    $this->assertTrue($handle->getPolicyFiltered());
+
     unset($env);
   }