From 53b9acfb7d58ac5f33424b84df35757a5c68aba1 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 28 May 2019 10:11:09 -0700
Subject: [PATCH] Test for "CAN_INTERACT" on comment edits in a way that
 survives objects which only implement "CAN_VIEW"

Summary:
Ref T13289. See D20551. In D20551, I implemented some "CAN_INTERACT" checks against certain edits, but these checks end up testing "CAN_INTERACT" against objects like Conpherence threads which do not support a distinct "CAN_INTERACT" permission. I misrembered how the "CAN_INTERACT" fallback to "CAN_VIEW" actually works: it's not fully automatic, and needs some explicit "interact, or view if interact is not available" checks.

Use the "interact" wrappers to test these policies so they fall back to "CAN_VIEW" if an object does not support "CAN_INTERACT". Generally, objects which have a "locked" state have a separate "CAN_INTERACT" permission; objects which don't have a "locked" state do not.

Test Plan: Created and edited comments in Conpherence (or most applications other than Maniphest).

Reviewers: amckinley

Maniphest Tasks: T13289

Differential Revision: https://secure.phabricator.com/D20558
---
 .../policy/filter/PhabricatorPolicyFilter.php | 31 ++++++++++++++-----
 ...cationTransactionCommentEditController.php |  5 ++-
 ...torApplicationTransactionCommentEditor.php |  6 ++--
 .../PhabricatorApplicationTransactionView.php |  5 ++-
 4 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/src/applications/policy/filter/PhabricatorPolicyFilter.php b/src/applications/policy/filter/PhabricatorPolicyFilter.php
index a5c9f356f4..4ea7ce1549 100644
--- a/src/applications/policy/filter/PhabricatorPolicyFilter.php
+++ b/src/applications/policy/filter/PhabricatorPolicyFilter.php
@@ -90,6 +90,29 @@ final class PhabricatorPolicyFilter extends Phobject {
     PhabricatorUser $user,
     PhabricatorPolicyInterface $object) {
 
+    $capabilities = self::getRequiredInteractCapabilities($object);
+
+    foreach ($capabilities as $capability) {
+      if (!self::hasCapability($user, $object, $capability)) {
+        return false;
+      }
+    }
+
+    return true;
+  }
+
+  public static function requireCanInteract(
+    PhabricatorUser $user,
+    PhabricatorPolicyInterface $object) {
+
+    $capabilities = self::getRequiredInteractCapabilities($object);
+    foreach ($capabilities as $capability) {
+      self::requireCapability($user, $object, $capability);
+    }
+  }
+
+  private static function getRequiredInteractCapabilities(
+    PhabricatorPolicyInterface $object) {
     $capabilities = $object->getCapabilities();
     $capabilities = array_fuse($capabilities);
 
@@ -107,13 +130,7 @@ final class PhabricatorPolicyFilter extends Phobject {
       $require[] = $can_interact;
     }
 
-    foreach ($require as $capability) {
-      if (!self::hasCapability($user, $object, $capability)) {
-        return false;
-      }
-    }
-
-    return true;
+    return $require;
   }
 
   public function setViewer(PhabricatorUser $user) {
diff --git a/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentEditController.php b/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentEditController.php
index 4529704c2f..84fcecfa6f 100644
--- a/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentEditController.php
+++ b/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentEditController.php
@@ -36,10 +36,9 @@ final class PhabricatorApplicationTransactionCommentEditController
     // auditing, and editing comments serves neither goal.
 
     $object = $xaction->getObject();
-    $can_interact = PhabricatorPolicyFilter::hasCapability(
+    $can_interact = PhabricatorPolicyFilter::canInteract(
       $viewer,
-      $object,
-      PhabricatorPolicyCapability::CAN_INTERACT);
+      $object);
     if (!$can_interact) {
       return $this->newDialog()
         ->setTitle(pht('Conversation Locked'))
diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionCommentEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionCommentEditor.php
index 22acb3312f..b2405d90c4 100644
--- a/src/applications/transactions/editor/PhabricatorApplicationTransactionCommentEditor.php
+++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionCommentEditor.php
@@ -189,10 +189,10 @@ final class PhabricatorApplicationTransactionCommentEditor
         $actor,
         $xaction,
         PhabricatorPolicyCapability::CAN_EDIT);
-      PhabricatorPolicyFilter::requireCapability(
+
+      PhabricatorPolicyFilter::requireCanInteract(
         $actor,
-        $xaction->getObject(),
-        PhabricatorPolicyCapability::CAN_INTERACT);
+        $xaction->getObject());
     }
   }
 
diff --git a/src/applications/transactions/view/PhabricatorApplicationTransactionView.php b/src/applications/transactions/view/PhabricatorApplicationTransactionView.php
index 7a24bf8ff8..209b6baf64 100644
--- a/src/applications/transactions/view/PhabricatorApplicationTransactionView.php
+++ b/src/applications/transactions/view/PhabricatorApplicationTransactionView.php
@@ -513,10 +513,9 @@ class PhabricatorApplicationTransactionView extends AphrontView {
         }
       }
 
-      $can_interact = PhabricatorPolicyFilter::hasCapability(
+      $can_interact = PhabricatorPolicyFilter::canInteract(
         $viewer,
-        $xaction->getObject(),
-        PhabricatorPolicyCapability::CAN_INTERACT);
+        $xaction->getObject());
       $event->setCanInteract($can_interact);
     }