Make feed stories properly respect object policies

Summary:
  - When a feed story's primary object is a Policy object, use its visibility policy to control story visibility. Leave an exception for
  - Augment PhabricatorPolicyAwareQuery so queries may do pre-policy filtering without the need to handle their own buffering/cursor code. (We could slightly improve this: if a query returns less than a page of pre-filtered results we could keep getting pre-filtered results until we had at least a page's worth and then filter them all at once.)
  - Load and attach "required objects" to feed stories. We need this for policies anyway, and it will let us simplify story implementations by sourcing data directly from the object when we don't have some need to denormalize it (e.g., "title was changed from X to Y" needs to save the values of X and Y from when we published the story, but "user asked question X" can reflect the current version of the question).

Test Plan: Loaded main feed, project feed, notification menu / dropdown, notificaiton list, paginated things.

Reviewers: btrahan, vrana

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D3783

src/applications/feed/PhabricatorFeedQuery.php

@@ -45,7 +45,7 @@ final class PhabricatorFeedQuery
   }
 
   protected function willFilterPage(array $data) {
-    return PhabricatorFeedStory::loadAllFromRows($data);
+    return PhabricatorFeedStory::loadAllFromRows($data, $this->getViewer());
   }
 
   private function buildJoinClause(AphrontDatabaseConnection $conn_r) {

src/applications/feed/builder/PhabricatorFeedBuilder.php

@@ -44,21 +44,12 @@ final class PhabricatorFeedBuilder {
     $user = $this->user;
     $stories = $this->stories;
 
-    $handles = array();
-    if ($stories) {
-      $handle_phids = array_mergev(mpull($stories, 'getRequiredHandlePHIDs'));
-      $object_phids = array_mergev(mpull($stories, 'getRequiredObjectPHIDs'));
-      $handles = id(new PhabricatorObjectHandleData($handle_phids))
-        ->loadHandles();
-    }
-
     $null_view = new AphrontNullView();
 
     require_celerity_resource('phabricator-feed-css');
 
     $last_date = null;
     foreach ($stories as $story) {
-      $story->setHandles($handles);
       $story->setFramed($this->framed);
 
       $date = ucfirst(phabricator_relative_date($story->getEpoch(), $user));

src/applications/feed/constants/PhabricatorFeedStoryTypeConstants.php

@@ -19,7 +19,6 @@
 final class PhabricatorFeedStoryTypeConstants
   extends PhabricatorFeedConstants {
 
-  const STORY_UNKNOWN       = 'PhabricatorFeedStoryUnknown';
   const STORY_STATUS        = 'PhabricatorFeedStoryStatus';
   const STORY_DIFFERENTIAL  = 'PhabricatorFeedStoryDifferential';
   const STORY_PHRICTION     = 'PhabricatorFeedStoryPhriction';

src/applications/feed/story/PhabricatorFeedStory.php

@@ -21,15 +21,17 @@
  * user adding a comment) which may be represented in different forms on
  * different channels (like feed, notifications and realtime alerts).
  *
- * @task load Loading Stories
+ * @task load     Loading Stories
+ * @task policy   Policy Implementation
  */
 abstract class PhabricatorFeedStory implements PhabricatorPolicyInterface {
 
   private $data;
   private $hasViewed;
-  private $handles;
   private $framed;
-  private $primaryObjectPHID;
+
+  private $handles  = array();
+  private $objects  = array();
 
 
 /* -(  Loading Stories  )---------------------------------------------------- */
@@ -46,7 +48,7 @@ abstract class PhabricatorFeedStory implements PhabricatorPolicyInterface {
    *                                      objects.
    * @task load
    */
-  public static function loadAllFromRows(array $rows) {
+  public static function loadAllFromRows(array $rows, PhabricatorUser $viewer) {
     $stories = array();
 
     $data = id(new PhabricatorFeedStoryData())->loadAllFromArray($rows);
@@ -62,42 +64,99 @@ abstract class PhabricatorFeedStory implements PhabricatorPolicyInterface {
       }
 
       // If the story type isn't a valid class or isn't a subclass of
-      // PhabricatorFeedStory, load it as PhabricatorFeedStoryUnknown.
-
+      // PhabricatorFeedStory, decline to load it.
       if (!$ok) {
-        $class = 'PhabricatorFeedStoryUnknown';
+        continue;
       }
 
       $key = $story_data->getChronologicalKey();
       $stories[$key] = newv($class, array($story_data));
     }
 
+    $object_phids = array();
+    $key_phids = array();
+    foreach ($stories as $key => $story) {
+      $phids = array();
+      foreach ($story->getRequiredObjectPHIDs() as $phid) {
+        $phids[$phid] = true;
+      }
+      if ($story->getPrimaryObjectPHID()) {
+        $phids[$story->getPrimaryObjectPHID()] = true;
+      }
+      $key_phids[$key] = $phids;
+      $object_phids += $phids;
+    }
+
+    $objects = id(new PhabricatorObjectHandleData(array_keys($object_phids)))
+      ->setViewer($viewer)
+      ->loadObjects();
+
+    foreach ($key_phids as $key => $phids) {
+      if (!$phids) {
+        continue;
+      }
+      $story_objects = array_select_keys($objects, array_keys($phids));
+      if (count($story_objects) != count($phids)) {
+        // An object this story requires either does not exist or is not visible
+        // to the user. Decline to render the story.
+        unset($stories[$key]);
+        unset($key_phids[$key]);
+        continue;
+      }
+
+      $stories[$key]->setObjects($story_objects);
+    }
+
+    $handle_phids = array();
+    foreach ($stories as $key => $story) {
+      foreach ($story->getRequiredHandlePHIDs() as $phid) {
+        $key_phids[$key][$phid] = true;
+      }
+      if ($story->getAuthorPHID()) {
+        $key_phids[$key][$story->getAuthorPHID()] = true;
+      }
+      $handle_phids += $key_phids[$key];
+    }
+
+    $handles = id(new PhabricatorObjectHandleData(array_keys($handle_phids)))
+      ->setViewer($viewer)
+      ->loadHandles();
+
+    foreach ($key_phids as $key => $phids) {
+      if (!$phids) {
+        continue;
+      }
+      $story_handles = array_select_keys($handles, array_keys($phids));
+      $stories[$key]->setHandles($story_handles);
+    }
+
     return $stories;
   }
 
-  public function getCapabilities() {
-    return array(
-      PhabricatorPolicyCapability::CAN_VIEW,
-    );
-  }
-
-  public function getPolicy($capability) {
-    return PhabricatorEnv::getEnvConfig('feed.public')
-      ? PhabricatorPolicies::POLICY_PUBLIC
-      : PhabricatorPolicies::POLICY_USER;
-  }
-
-  public function hasAutomaticCapability($capability, PhabricatorUser $viewer) {
-    return false;
-  }
-
-  public function setPrimaryObjectPHID($primary_object_phid) {
-    $this->primaryObjectPHID = $primary_object_phid;
+  public function setObjects(array $objects) {
+    $this->objects = $objects;
     return $this;
   }
 
+  public function getObject($phid) {
+    $object = idx($this->objects, $phid);
+    if (!$object) {
+      throw new Exception(
+        "Story is asking for an object it did not request ('{$phid}')!");
+    }
+    return $object;
+  }
+
+  public function getPrimaryObject() {
+    $phid = $this->getPrimaryObjectPHID();
+    if (!$phid) {
+      throw new Exception("Story has no primary object!");
+    }
+    return $this->getObject($phid);
+  }
+
   public function getPrimaryObjectPHID() {
-    return $this->primaryObjectPHID;
+    return null;
   }
 
   final public function __construct(PhabricatorFeedStoryData $data) {
@@ -116,6 +175,10 @@ abstract class PhabricatorFeedStory implements PhabricatorPolicyInterface {
     return array();
   }
 
+  public function getRequiredObjectPHIDs() {
+    return array();
+  }
+
   public function setHasViewed($has_viewed) {
     $this->hasViewed = $has_viewed;
     return $this;
@@ -125,10 +188,6 @@ abstract class PhabricatorFeedStory implements PhabricatorPolicyInterface {
     return $this->hasViewed;
   }
 
-  public function getRequiredObjectPHIDs() {
-    return array();
-  }
-
   final public function setFramed($framed) {
     $this->framed = $framed;
     return $this;
@@ -140,6 +199,10 @@ abstract class PhabricatorFeedStory implements PhabricatorPolicyInterface {
     return $this;
   }
 
+  final protected function getObjects() {
+    return $this->objects;
+  }
+
   final protected function getHandles() {
     return $this->handles;
   }
@@ -170,6 +233,14 @@ abstract class PhabricatorFeedStory implements PhabricatorPolicyInterface {
     return $this->getStoryData()->getChronologicalKey();
   }
 
+  final public function getValue($key, $default = null) {
+    return $this->getStoryData()->getValue($key, $default);
+  }
+
+  final public function getAuthorPHID() {
+    return $this->getStoryData()->getAuthorPHID();
+  }
+
   final protected function renderHandleList(array $phids) {
     $list = array();
     foreach ($phids as $phid) {
@@ -210,4 +281,48 @@ abstract class PhabricatorFeedStory implements PhabricatorPolicyInterface {
     return array();
   }
 
+
+/* -(  PhabricatorPolicyInterface Implementation  )-------------------------- */
+
+
+  /**
+   * @task policy
+   */
+  public function getCapabilities() {
+    return array(
+      PhabricatorPolicyCapability::CAN_VIEW,
+    );
+  }
+
+
+  /**
+   * @task policy
+   */
+  public function getPolicy($capability) {
+    // If this story's primary object is a policy-aware object, use its policy
+    // to control story visiblity.
+
+    $primary_phid = $this->getPrimaryObjectPHID();
+    if (isset($this->objects[$primary_phid])) {
+      $object = $this->objects[$primary_phid];
+      if ($object instanceof PhabricatorPolicyInterface) {
+        return $object->getPolicy($capability);
+      }
+    }
+
+    // TODO: Remove this once all objects are policy-aware. For now, keep
+    // respecting the `feed.public` setting.
+    return PhabricatorEnv::getEnvConfig('feed.public')
+      ? PhabricatorPolicies::POLICY_PUBLIC
+      : PhabricatorPolicies::POLICY_USER;
+  }
+
+
+  /**
+   * @task policy
+   */
+  public function hasAutomaticCapability($capability, PhabricatorUser $viewer) {
+    return false;
+  }
+
 }

src/applications/feed/story/PhabricatorFeedStoryAggregate.php

@@ -24,6 +24,10 @@ abstract class PhabricatorFeedStoryAggregate extends PhabricatorFeedStory {
     return head($this->getAggregateStories())->getHasViewed();
   }
 
+  public function getPrimaryObjectPHID() {
+    return head($this->getAggregateStories())->getPrimaryObjectPHID();
+  }
+
   public function getRequiredHandlePHIDs() {
     $phids = array();
     foreach ($this->getAggregateStories() as $story) {
@@ -59,6 +63,18 @@ abstract class PhabricatorFeedStoryAggregate extends PhabricatorFeedStory {
   final public function setAggregateStories(array $aggregate_stories) {
     assert_instances_of($aggregate_stories, 'PhabricatorFeedStory');
     $this->aggregateStories = $aggregate_stories;
+
+    $objects = array();
+    $handles = array();
+
+    foreach ($this->aggregateStories as $story) {
+      $objects += $story->getObjects();
+      $handles += $story->getHandles();
+    }
+
+    $this->setObjects($objects);
+    $this->setHandles($handles);
+
     return $this;
   }
 

src/applications/feed/story/PhabricatorFeedStoryAudit.php

@@ -18,26 +18,17 @@
 
 final class PhabricatorFeedStoryAudit extends PhabricatorFeedStory {
 
-  public function getRequiredHandlePHIDs() {
-    return array(
-      $this->getStoryData()->getAuthorPHID(),
-      $this->getStoryData()->getValue('commitPHID'),
-    );
-  }
-
-  public function getRequiredObjectPHIDs() {
-    return array();
+  public function getPrimaryObjectPHID() {
+    return $this->getStoryData()->getValue('commitPHID');
   }
 
   public function renderView() {
-    $data = $this->getStoryData();
-
-    $author_phid = $data->getAuthorPHID();
-    $commit_phid = $data->getValue('commitPHID');
+    $author_phid = $this->getAuthorPHID();
+    $commit_phid = $this->getPrimaryObjectPHID();
 
     $view = new PhabricatorFeedStoryView();
 
-    $action = $data->getValue('action');
+    $action = $this->getValue('action');
     $verb = PhabricatorAuditActionConstants::getActionPastTenseVerb($action);
 
     $view->setTitle(
@@ -46,9 +37,9 @@ final class PhabricatorFeedStoryAudit extends PhabricatorFeedStory {
       $this->linkTo($commit_phid).
       ".");
 
-    $view->setEpoch($data->getEpoch());
+    $view->setEpoch($this->getEpoch());
 
-    $comments = $data->getValue('content');
+    $comments = $this->getValue('content');
     if ($comments) {
       $full_size = true;
     } else {
@@ -57,7 +48,7 @@ final class PhabricatorFeedStoryAudit extends PhabricatorFeedStory {
 
     if ($full_size) {
       $view->setImage($this->getHandle($author_phid)->getImageURI());
-      $content = $this->renderSummary($data->getValue('content'));
+      $content = $this->renderSummary($this->getValue('content'));
       $view->appendChild($content);
     } else {
       $view->setOneLineStory(true);

src/applications/feed/story/PhabricatorFeedStoryCommit.php

@@ -18,15 +18,14 @@
 
 final class PhabricatorFeedStoryCommit extends PhabricatorFeedStory {
 
-  public function getRequiredHandlePHIDs() {
-    $data = $this->getStoryData();
+  public function getPrimaryObjectPHID() {
+    return $this->getValue('commitPHID');
+  }
 
-    return array_filter(
-      array(
-        $data->getValue('commitPHID'),
-        $data->getValue('authorPHID'),
-        $data->getValue('committerPHID'),
-      ));
+  public function getRequiredHandlePHIDs() {
+    return array(
+      $this->getValue('committerPHID'),
+    );
   }
 
   public function renderView() {

src/applications/feed/story/PhabricatorFeedStoryDifferential.php

@@ -18,19 +18,8 @@
 
 final class PhabricatorFeedStoryDifferential extends PhabricatorFeedStory {
 
-  public function getRequiredHandlePHIDs() {
-    $data = $this->getStoryData();
-    return array(
-      $this->getStoryData()->getAuthorPHID(),
-      $data->getValue('revision_phid'),
-      $data->getValue('revision_author_phid'),
-    );
-  }
-
-  public function getRequiredObjectPHIDs() {
-    return array(
-      $this->getStoryData()->getAuthorPHID(),
-    );
+  public function getPrimaryObjectPHID() {
+    return $this->getValue('revision_phid');
   }
 
   public function renderView() {
@@ -78,13 +67,11 @@ final class PhabricatorFeedStoryDifferential extends PhabricatorFeedStory {
 
   private function getLineForData($data) {
     $actor_phid = $data->getAuthorPHID();
-    $owner_phid = $data->getValue('revision_author_phid');
     $revision_phid = $data->getValue('revision_phid');
     $action = $data->getValue('action');
 
     $actor_link = $this->linkTo($actor_phid);
     $revision_link = $this->linkTo($revision_phid);
-    $owner_link = $this->linkTo($owner_phid);
 
     $verb = DifferentialAction::getActionPastTenseVerb($action);
 

src/applications/feed/story/PhabricatorFeedStoryManiphest.php

@@ -19,19 +19,13 @@
 final class PhabricatorFeedStoryManiphest
   extends PhabricatorFeedStory {
 
-  public function getRequiredHandlePHIDs() {
-    $data = $this->getStoryData();
-    return array_filter(
-      array(
-        $this->getStoryData()->getAuthorPHID(),
-        $data->getValue('taskPHID'),
-        $data->getValue('ownerPHID'),
-      ));
+  public function getPrimaryObjectPHID() {
+    return $this->getValue('taskPHID');
   }
 
-  public function getRequiredObjectPHIDs() {
+  public function getRequiredHandlePHIDs() {
     return array(
-      $this->getStoryData()->getAuthorPHID(),
+      $this->getValue('ownerPHID'),
     );
   }
 

src/applications/feed/story/PhabricatorFeedStoryPhriction.php

@@ -18,17 +18,8 @@
 
 final class PhabricatorFeedStoryPhriction extends PhabricatorFeedStory {
 
-  public function getRequiredHandlePHIDs() {
-    return array(
-      $this->getStoryData()->getAuthorPHID(),
-      $this->getStoryData()->getValue('phid'),
-    );
-  }
-
-  public function getRequiredObjectPHIDs() {
-    return array(
-      $this->getStoryData()->getAuthorPHID(),
-    );
+  public function getPrimaryObjectPHID() {
+    return $this->getValue('phid');
   }
 
   public function renderView() {

src/applications/feed/story/PhabricatorFeedStoryProject.php

@@ -18,17 +18,8 @@
 
 final class PhabricatorFeedStoryProject extends PhabricatorFeedStory {
 
-  public function getRequiredHandlePHIDs() {
-    return array(
-      $this->getStoryData()->getAuthorPHID(),
-      $this->getStoryData()->getValue('projectPHID'),
-    );
-  }
-
-  public function getRequiredObjectPHIDs() {
-    return array(
-      $this->getStoryData()->getAuthorPHID(),
-    );
+  public function getPrimaryObjectPHID() {
+    return $this->getValue('projectPHID');
   }
 
   public function renderView() {

src/applications/feed/story/PhabricatorFeedStoryStatus.php

@@ -18,16 +18,8 @@
 
 final class PhabricatorFeedStoryStatus extends PhabricatorFeedStory {
 
-  public function getRequiredHandlePHIDs() {
-    return array(
-      $this->getStoryData()->getAuthorPHID(),
-    );
-  }
-
-  public function getRequiredObjectPHIDs() {
-    return array(
-      $this->getStoryData()->getAuthorPHID(),
-    );
+  public function getPrimaryObjectPHID() {
+    return $this->getAuthorPHID();
   }
 
   public function renderView() {

src/applications/notification/PhabricatorNotificationQuery.php

@@ -20,7 +20,8 @@
  * @task config Configuring the Query
  * @task exec   Query Execution
  */
-final class PhabricatorNotificationQuery extends PhabricatorOffsetPagedQuery {
+final class PhabricatorNotificationQuery
+  extends PhabricatorCursorPagedPolicyAwareQuery {
 
   private $userPHID;
   private $keys;
@@ -60,7 +61,7 @@ final class PhabricatorNotificationQuery extends PhabricatorOffsetPagedQuery {
 /* -(  Query Execution  )---------------------------------------------------- */
 
 
-  public function execute() {
+  public function loadPage() {
     if (!$this->userPHID) {
       throw new Exception("Call setUser() before executing the query");
     }
@@ -72,7 +73,7 @@ final class PhabricatorNotificationQuery extends PhabricatorOffsetPagedQuery {
 
     $data = queryfx_all(
       $conn,
-      "SELECT story.*, notif.primaryObjectPHID, notif.hasViewed FROM %T notif
+      "SELECT story.*, notif.hasViewed FROM %T notif
          JOIN %T story ON notif.chronologicalKey = story.chronologicalKey
          %Q
          ORDER BY notif.chronologicalKey DESC
@@ -83,12 +84,13 @@ final class PhabricatorNotificationQuery extends PhabricatorOffsetPagedQuery {
       $this->buildLimitClause($conn));
 
     $viewed_map = ipull($data, 'hasViewed', 'chronologicalKey');
-    $primary_map = ipull($data, 'primaryObjectPHID', 'chronologicalKey');
 
-    $stories = PhabricatorFeedStory::loadAllFromRows($data);
+    $stories = PhabricatorFeedStory::loadAllFromRows(
+      $data,
+      $this->getViewer());
+
     foreach ($stories as $key => $story) {
       $story->setHasViewed($viewed_map[$key]);
-      $story->setPrimaryObjectPHID($primary_map[$key]);
     }
 
     return $stories;

src/applications/notification/builder/PhabricatorNotificationBuilder.php

@@ -136,19 +136,9 @@ final class PhabricatorNotificationBuilder {
     $stories = mpull($stories, null, 'getChronologicalKey');
     krsort($stories);
 
-    $handles = array();
-    $objects = array();
-
-    if ($stories) {
-      $handle_phids = array_mergev(mpull($stories, 'getRequiredHandlePHIDs'));
-      $handles = id(new PhabricatorObjectHandleData($handle_phids))
-        ->loadHandles();
-    }
-
     $null_view = new AphrontNullView();
 
     foreach ($stories as $story) {
-      $story->setHandles($handles);
       $view = $story->renderNotificationView();
       $null_view->appendChild($view);
     }

src/applications/notification/controller/PhabricatorNotificationIndividualController.php

@@ -24,6 +24,7 @@ final class PhabricatorNotificationIndividualController
     $user = $request->getUser();
 
     $stories = id(new PhabricatorNotificationQuery())
+      ->setViewer($user)
       ->setUserPHID($user->getPHID())
       ->withKeys(array($request->getStr('key')))
       ->execute();

src/applications/notification/controller/PhabricatorNotificationListController.php

@@ -40,6 +40,7 @@ final class PhabricatorNotificationListController
     $pager->setOffset($request->getInt('offset'));
 
     $query = new PhabricatorNotificationQuery();
+    $query->setViewer($user);
     $query->setUserPHID($user->getPHID());
 
     switch ($filter) {

src/applications/notification/controller/PhabricatorNotificationPanelController.php

@@ -25,6 +25,7 @@ final class PhabricatorNotificationPanelController
     $user = $request->getUser();
 
     $query = new PhabricatorNotificationQuery();
+    $query->setViewer($user);
     $query->setUserPHID($user->getPHID());
     $query->setLimit(15);