Policy - fix up DifferentialChangesetParser

Summary:
Ref T7094. We should do a policy query on the files IMO because there exists a scenario where the file gets locked down directly. This requires being a bit more disciplined about setting user, which in turn requires deciding whether or not to show edit / reply links as a separate piece of logic, not conditional on user presence.

This is not the best code but I don't think it gets worse with this and is just some other nuance in any larger cleanup we take on someday.

Test Plan: looked at a revision and noted inline comments rendered correctly with reply / edit actions. looked at a diff standalone and noted no reply / edit actions as expected. looked at a "details" link on a transaction and it rendered correctly. looked at a diff in phriction of page edits and it looked good. grepped around and verified the remaining callsite in diffusion already has the setUser call.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T7094

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

src/applications/differential/__tests__/DifferentialParseRenderTestCase.php

@@ -48,6 +48,7 @@ final class DifferentialParseRenderTestCase extends PhabricatorTestCase {
     $engine->setViewer(new PhabricatorUser());
 
     $cparser = new DifferentialChangesetParser();
+    $cparser->setUser(new PhabricatorUser());
     $cparser->setDisableCache(true);
     $cparser->setChangeset($changeset);
     $cparser->setMarkupEngine($engine);

src/applications/differential/controller/DifferentialChangesetViewController.php

@@ -207,11 +207,12 @@ final class DifferentialChangesetViewController extends DifferentialController {
 
     $engine->process();
     $parser->setMarkupEngine($engine);
+    $parser->setUser($request->getUser());
 
     if ($request->isAjax()) {
-      // TODO: This is sort of lazy, the effect is just to not render "Edit"
-      // and "Reply" links on the "standalone view".
-      $parser->setUser($request->getUser());
+      $parser->setShowEditAndReplyLinks(true);
+    } else {
+      $parser->setShowEditAndReplyLinks(false);
     }
 
     $output = $parser->render($range_s, $range_e, $mask);

src/applications/differential/parser/DifferentialChangesetParser.php

@@ -43,6 +43,15 @@ final class DifferentialChangesetParser {
   private $renderer;
   private $characterEncoding;
   private $highlightAs;
+  private $showEditAndReplyLinks = true;
+
+  public function setShowEditAndReplyLinks($bool) {
+    $this->showEditAndReplyLinks = $bool;
+    return $this;
+  }
+  public function getShowEditAndReplyLinks() {
+    return $this->showEditAndReplyLinks;
+  }
 
   public function setHighlightAs($highlight_as) {
     $this->highlightAs = $highlight_as;
@@ -62,7 +71,7 @@ final class DifferentialChangesetParser {
     return $this->characterEncoding;
   }
 
-  public function setRenderer($renderer) {
+  public function setRenderer(DifferentialChangesetRenderer $renderer) {
     $this->renderer = $renderer;
     return $this;
   }
@@ -252,6 +261,10 @@ final class DifferentialChangesetParser {
     return $this;
   }
 
+  public function getUser() {
+    return $this->user;
+  }
+
   public function setCoverage($coverage) {
     $this->coverage = $coverage;
     return $this;
@@ -739,6 +752,7 @@ final class DifferentialChangesetParser {
       count($this->new));
 
     $renderer = $this->getRenderer()
+      ->setUser($this->getUser())
       ->setChangeset($this->changeset)
       ->setRenderPropertyChangeHeader($render_pch)
       ->setIsTopLevel($this->isTopLevel)
@@ -755,11 +769,8 @@ final class DifferentialChangesetParser {
       ->setHandles($this->handles)
       ->setOldLines($this->old)
       ->setNewLines($this->new)
-      ->setOriginalCharacterEncoding($encoding);
-
-    if ($this->user) {
-      $renderer->setUser($this->user);
-    }
+      ->setOriginalCharacterEncoding($encoding)
+      ->setShowEditAndReplyLinks($this->getShowEditAndReplyLinks());
 
     $shield = null;
     if ($this->isTopLevel && !$this->comments) {
@@ -905,10 +916,10 @@ final class DifferentialChangesetParser {
             $file_phids[] = $new_phid;
           }
 
-          // TODO: (T603) Probably fine to use omnipotent viewer here?
-          $files = id(new PhabricatorFile())->loadAllWhere(
-            'phid IN (%Ls)',
-            $file_phids);
+          $files = id(new PhabricatorFileQuery())
+            ->setViewer($this->getUser())
+            ->withPHIDs($file_phids)
+            ->execute();
           foreach ($files as $file) {
             if (empty($file)) {
               continue;

src/applications/differential/render/DifferentialChangesetHTMLRenderer.php

@@ -443,8 +443,9 @@ abstract class DifferentialChangesetHTMLRenderer
     $user = $this->getUser();
     $edit = $user &&
             ($comment->getAuthorPHID() == $user->getPHID()) &&
-            ($comment->isDraft());
-    $allow_reply = (bool)$user;
+            ($comment->isDraft())
+            && $this->getShowEditAndReplyLinks();
+    $allow_reply = (bool)$user && $this->getShowEditAndReplyLinks();
 
     return id(new DifferentialInlineCommentView())
       ->setInlineComment($comment)

src/applications/differential/render/DifferentialChangesetRenderer.php

@@ -29,10 +29,19 @@ abstract class DifferentialChangesetRenderer {
   private $mask;
   private $depths;
   private $originalCharacterEncoding;
+  private $showEditAndReplyLinks;
 
   private $oldFile = false;
   private $newFile = false;
 
+  public function setShowEditAndReplyLinks($bool) {
+    $this->showEditAndReplyLinks = $bool;
+    return $this;
+  }
+  public function getShowEditAndReplyLinks() {
+    return $this->showEditAndReplyLinks;
+  }
+
   public function setOriginalCharacterEncoding($original_character_encoding) {
     $this->originalCharacterEncoding = $original_character_encoding;
     return $this;

src/applications/phriction/controller/PhrictionDiffController.php

@@ -71,6 +71,7 @@ final class PhrictionDiffController extends PhrictionController {
     $whitespace_mode = DifferentialChangesetParser::WHITESPACE_SHOW_ALL;
 
     $parser = new DifferentialChangesetParser();
+    $parser->setUser($user);
     $parser->setChangeset($changeset);
     $parser->setRenderingReference("{$l},{$r}");
     $parser->setWhitespaceMode($whitespace_mode);

src/applications/transactions/view/PhabricatorApplicationTransactionTextDiffDetailView.php

@@ -43,6 +43,7 @@ final class PhabricatorApplicationTransactionTextDiffDetailView
       $markup_engine->setViewer($this->getUser());
 
       $parser = new DifferentialChangesetParser();
+      $parser->setUser($this->getUser());
       $parser->setChangeset($changeset);
       $parser->setMarkupEngine($markup_engine);
       $parser->setWhitespaceMode($whitespace_mode);