From faaaff0b6f604c204598003439036bb7c6388647 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 5 Dec 2013 17:00:53 -0800
Subject: [PATCH] Fix an error in the PolicyFilter algorithm

Summary:
`PhabricatorPolicyFilter` has a bug right now where it lets through objects incorrectly if:

  - the query requests two or more policies;
  - the object satisfies at least one of those policies; and
  - policy exceptions are not enabled.

This would be bad, but there's only one call in the codebase which satisfies all of these conditions, in the Maniphest batch editor. And it's moot anyway because edit operations get another policy check slightly later. So there is no policy/security impact from this flaw.

(The next diff relies on this behavior, which is how I caught it.)

Test Plan:
  - Added a failing unit test and made it pass.
  - Grepped the codebase for `requireCapabilities()` and verified that there is no security impact. Basically, 99% of callsites use `executeOne()`, which throws anyway and moots the filtering.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D7721
---
 .../__tests__/PhabricatorPolicyTestCase.php   | 29 +++++++++++++++++++
 .../policy/filter/PhabricatorPolicyFilter.php |  6 ++--
 .../policy/PhabricatorPolicyAwareQuery.php    |  2 +-
 3 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/src/applications/policy/__tests__/PhabricatorPolicyTestCase.php b/src/applications/policy/__tests__/PhabricatorPolicyTestCase.php
index edd41d9c6b..860ca52f69 100644
--- a/src/applications/policy/__tests__/PhabricatorPolicyTestCase.php
+++ b/src/applications/policy/__tests__/PhabricatorPolicyTestCase.php
@@ -227,6 +227,35 @@ final class PhabricatorPolicyTestCase extends PhabricatorTestCase {
     }
   }
 
+  public function testMultipleCapabilities() {
+    $object = new PhabricatorPolicyTestObject();
+    $object->setCapabilities(
+      array(
+        PhabricatorPolicyCapability::CAN_VIEW,
+        PhabricatorPolicyCapability::CAN_EDIT,
+      ));
+    $object->setPolicies(
+      array(
+        PhabricatorPolicyCapability::CAN_VIEW
+          => PhabricatorPolicies::POLICY_USER,
+        PhabricatorPolicyCapability::CAN_EDIT
+          => PhabricatorPolicies::POLICY_NOONE,
+      ));
+
+    $filter = new PhabricatorPolicyFilter();
+    $filter->requireCapabilities(
+      array(
+        PhabricatorPolicyCapability::CAN_VIEW,
+        PhabricatorPolicyCapability::CAN_EDIT,
+      ));
+    $filter->setViewer($this->buildUser('user'));
+
+    $result = $filter->apply(array($object));
+
+    $this->assertEqual(array(), $result);
+  }
+
+
   /**
    * Test an object for visibility across multiple user specifications.
    */
diff --git a/src/applications/policy/filter/PhabricatorPolicyFilter.php b/src/applications/policy/filter/PhabricatorPolicyFilter.php
index 969e167d30..f3cac68765 100644
--- a/src/applications/policy/filter/PhabricatorPolicyFilter.php
+++ b/src/applications/policy/filter/PhabricatorPolicyFilter.php
@@ -156,10 +156,10 @@ final class PhabricatorPolicyFilter {
           // If we're missing any capability, move on to the next object.
           continue 2;
         }
-
-        // If we make it here, we have all of the required capabilities.
-        $filtered[$key] = $object;
       }
+
+      // If we make it here, we have all of the required capabilities.
+      $filtered[$key] = $object;
     }
 
     return $filtered;
diff --git a/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php b/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php
index 55c6edc277..d2fd056e7f 100644
--- a/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php
+++ b/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php
@@ -109,7 +109,7 @@ abstract class PhabricatorPolicyAwareQuery extends PhabricatorOffsetPagedQuery {
    * @task config
    */
   final public function shouldRaisePolicyExceptions() {
-    return (bool) $this->raisePolicyExceptions;
+    return (bool)$this->raisePolicyExceptions;
   }