From 4dfdd0d3167d4402dfe92a06fba42e8daa051476 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 1 Oct 2013 11:25:30 -0700
Subject: [PATCH] Treat invalid policies as broadly similar to "no one"

Summary:
Ref T3903. Ref T603. We currently overreact to invalid policies. Instead:

  - For non-omnipotent users, just reject the viewer.
  - For omnipotent users, we already shortcircuit and permit the viewer.
  - Formalize and add test coverage for these behaviors.

Also clean up some strings.

The practical effect of this is that setting an object to an invalid policy (either intentionally or accidentally) doesn't break callers who are querying it.

Test Plan:
  - Created a Legalpad document and set view policy to "asldkfnaslkdfna".
  - Verified this policy behaved as though it were "no one".
  - Added, executed unit tests.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603, T3903

Differential Revision: https://secure.phabricator.com/D7185
---
 .../__tests__/PhabricatorPolicyTestCase.php   | 41 ++++++++++++++++++-
 .../policy/filter/PhabricatorPolicyFilter.php | 25 +++++++----
 2 files changed, 58 insertions(+), 8 deletions(-)

diff --git a/src/applications/policy/__tests__/PhabricatorPolicyTestCase.php b/src/applications/policy/__tests__/PhabricatorPolicyTestCase.php
index aff5ee7d6e..b8b0236b52 100644
--- a/src/applications/policy/__tests__/PhabricatorPolicyTestCase.php
+++ b/src/applications/policy/__tests__/PhabricatorPolicyTestCase.php
@@ -82,7 +82,6 @@ final class PhabricatorPolicyTestCase extends PhabricatorTestCase {
         'admin'   => false,
       ),
       'No One Policy');
-
   }
 
 
@@ -172,6 +171,46 @@ final class PhabricatorPolicyTestCase extends PhabricatorTestCase {
   }
 
 
+  /**
+   * Test that invalid policies reject viewers of all types.
+   */
+  public function testRejectInvalidPolicy() {
+    $invalid_policy = "the duck goes quack";
+    $object = $this->buildObject($invalid_policy);
+
+    $this->expectVisibility(
+      $object = $this->buildObject($invalid_policy),
+      array(
+        'public'  => false,
+        'user'    => false,
+        'admin'   => false,
+      ),
+      'Invalid Policy');
+  }
+
+
+  /**
+   * An omnipotent user should be able to see even objects with invalid
+   * policies.
+   */
+  public function testInvalidPolicyVisibleByOmnipotentUser() {
+    $invalid_policy = "the cow goes moo";
+    $object = $this->buildObject($invalid_policy);
+
+    $results = array(
+      $object,
+    );
+
+    $query = new PhabricatorPolicyAwareTestQuery();
+    $query->setResults($results);
+    $query->setViewer(PhabricatorUser::getOmnipotentUser());
+
+    $this->assertEqual(
+      1,
+      count($query->execute()));
+  }
+
+
   /**
    * 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 b5907e6a7a..1d6bba9c0a 100644
--- a/src/applications/policy/filter/PhabricatorPolicyFilter.php
+++ b/src/applications/policy/filter/PhabricatorPolicyFilter.php
@@ -225,7 +225,8 @@ final class PhabricatorPolicyFilter {
             $this->rejectObject($object, $policy, $capability);
           }
         } else {
-          throw new Exception("Object has unknown policy '{$policy}'!");
+          // Reject objects with unknown policies.
+          $this->rejectObject($object, false, $capability);
         }
     }
 
@@ -241,11 +242,22 @@ final class PhabricatorPolicyFilter {
       return;
     }
 
-    // TODO: clean this up
-    $verb = $capability;
+    switch ($capability) {
+      case PhabricatorPolicyCapability::CAN_VIEW:
+        $message = pht("This object has an impossible view policy.");
+        break;
+      case PhabricatorPolicyCapability::CAN_EDIT:
+        $message = pht("This object has an impossible edit policy.");
+        break;
+      case PhabricatorPolicyCapability::CAN_JOIN:
+        $message = pht("This object has an impossible join policy.");
+        break;
+      default:
+        $message = pht("This object has an impossible policy.");
+        break;
+    }
 
-    throw new PhabricatorPolicyException(
-      "This object has an impossible {$verb} policy.");
+    throw new PhabricatorPolicyException($message);
   }
 
   public function rejectObject(
@@ -350,9 +362,8 @@ final class PhabricatorPolicyFilter {
                 $handle->getFullName());
               break;
           }
-
         } else {
-          $who = pht("This object has an unknown policy setting.");
+          $who = pht("This object has an unknown or invalid policy setting.");
         }
         break;
     }