From cf1ccc995e19fbfee92967921110dc955ea2a604 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Sun, 1 Jan 2017 11:03:16 -0800
Subject: [PATCH] Apply application visibility checks during normal object
 filtering

Summary:
Fixes T9058. Normally, "Query" classes apply an application check and just don't load anything if it fails.

However, in some cases (like email recipient filtering) we run policy checks without having run a Query check first. In that case, one user (the actor) loads the object, then we filter it against other users (the recipeints).

Explicitly apply the application check during normal filtering.

Test Plan: Added a failing test case and made it pass.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9058

Differential Revision: https://secure.phabricator.com/D17127
---
 .../base/PhabricatorApplication.php           |  4 +-
 .../policy/filter/PhabricatorPolicyFilter.php | 51 +++++++++++++++++++
 .../PhabricatorProjectCoreTestCase.php        | 40 +++++++++++++++
 3 files changed, 93 insertions(+), 2 deletions(-)

diff --git a/src/applications/base/PhabricatorApplication.php b/src/applications/base/PhabricatorApplication.php
index 23a759d2aa..95045623d0 100644
--- a/src/applications/base/PhabricatorApplication.php
+++ b/src/applications/base/PhabricatorApplication.php
@@ -429,8 +429,8 @@ abstract class PhabricatorApplication
     }
 
     $cache = PhabricatorCaches::getRequestCache();
-    $viewer_phid = $viewer->getPHID();
-    $key = 'app.'.$class.'.installed.'.$viewer_phid;
+    $viewer_fragment = $viewer->getCacheFragment();
+    $key = 'app.'.$class.'.installed.'.$viewer_fragment;
 
     $result = $cache->getKey($key);
     if ($result === null) {
diff --git a/src/applications/policy/filter/PhabricatorPolicyFilter.php b/src/applications/policy/filter/PhabricatorPolicyFilter.php
index 1466d4e569..855149f6b3 100644
--- a/src/applications/policy/filter/PhabricatorPolicyFilter.php
+++ b/src/applications/policy/filter/PhabricatorPolicyFilter.php
@@ -123,6 +123,12 @@ final class PhabricatorPolicyFilter extends Phobject {
       return $objects;
     }
 
+    // Before doing any actual object checks, make sure the viewer can see
+    // the applications that these objects belong to. This is normally enforced
+    // in the Query layer before we reach object filtering, but execution
+    // sometimes reaches policy filtering without running application checks.
+    $objects = $this->applyApplicationChecks($objects);
+
     $filtered = array();
     $viewer_phid = $viewer->getPHID();
 
@@ -864,4 +870,49 @@ final class PhabricatorPolicyFilter extends Phobject {
     throw $exception;
   }
 
+  private function applyApplicationChecks(array $objects) {
+    $viewer = $this->viewer;
+
+    foreach ($objects as $key => $object) {
+      $phid = $object->getPHID();
+      if (!$phid) {
+        continue;
+      }
+
+      $application_class = $this->getApplicationForPHID($phid);
+      if ($application_class === null) {
+        continue;
+      }
+
+      $can_see = PhabricatorApplication::isClassInstalledForViewer(
+        $application_class,
+        $viewer);
+      if ($can_see) {
+        continue;
+      }
+
+      unset($objects[$key]);
+
+      $application = newv($application_class, array());
+      $this->rejectObject(
+        $application,
+        $application->getPolicy(PhabricatorPolicyCapability::CAN_VIEW),
+        PhabricatorPolicyCapability::CAN_VIEW);
+    }
+
+    return $objects;
+  }
+
+  private function getApplicationForPHID($phid) {
+    $phid_type = phid_get_type($phid);
+
+    $type_objects = PhabricatorPHIDType::getTypes(array($phid_type));
+    $type_object = idx($type_objects, $phid_type);
+    if (!$type_object) {
+      return null;
+    }
+
+    return $type_object->getPHIDTypeApplicationClass();
+  }
+
 }
diff --git a/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php b/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php
index b6d3aeebcf..cb977dd681 100644
--- a/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php
+++ b/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php
@@ -39,6 +39,46 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase {
     $this->assertFalse((bool)$this->refreshProject($proj, $user2));
   }
 
+  public function testApplicationPolicy() {
+    $user = $this->createUser()
+      ->save();
+
+    $proj = $this->createProject($user);
+
+    $this->assertTrue(
+      PhabricatorPolicyFilter::hasCapability(
+        $user,
+        $proj,
+        PhabricatorPolicyCapability::CAN_VIEW));
+
+    // 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.
+    $env = PhabricatorEnv::beginScopedEnv();
+    $env->overrideEnvConfig(
+      'phabricator.application-settings',
+      array(
+        'PHID-APPS-PhabricatorProjectApplication' => array(
+          'policy' => array(
+            'view' => PhabricatorPolicies::POLICY_NOONE,
+          ),
+        ),
+      ));
+
+    // Application visibility is cached because it does not normally change
+    // over the course of a single request. Drop the cache so the next filter
+    // test uses the new visibility.
+    PhabricatorCaches::destroyRequestCache();
+
+    $this->assertFalse(
+      PhabricatorPolicyFilter::hasCapability(
+        $user,
+        $proj,
+        PhabricatorPolicyCapability::CAN_VIEW));
+
+    unset($env);
+  }
+
   public function testIsViewerMemberOrWatcher() {
     $user1 = $this->createUser()
       ->save();