From 77eae81e1a5c34db3dbacf1468696928712c1ba6 Mon Sep 17 00:00:00 2001 From: Bob Trahan Date: Fri, 30 Jan 2015 11:17:34 -0800 Subject: [PATCH] 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 --- .../DifferentialParseRenderTestCase.php | 1 + .../DifferentialChangesetViewController.php | 7 +++-- .../parser/DifferentialChangesetParser.php | 31 +++++++++++++------ .../DifferentialChangesetHTMLRenderer.php | 5 +-- .../render/DifferentialChangesetRenderer.php | 9 ++++++ .../controller/PhrictionDiffController.php | 1 + ...plicationTransactionTextDiffDetailView.php | 1 + 7 files changed, 40 insertions(+), 15 deletions(-) diff --git a/src/applications/differential/__tests__/DifferentialParseRenderTestCase.php b/src/applications/differential/__tests__/DifferentialParseRenderTestCase.php index 9d9f47315d..7e06248340 100644 --- a/src/applications/differential/__tests__/DifferentialParseRenderTestCase.php +++ b/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); diff --git a/src/applications/differential/controller/DifferentialChangesetViewController.php b/src/applications/differential/controller/DifferentialChangesetViewController.php index 3d25524e79..48f009b27f 100644 --- a/src/applications/differential/controller/DifferentialChangesetViewController.php +++ b/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); diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php index a27f8c823b..8ff90a256f 100644 --- a/src/applications/differential/parser/DifferentialChangesetParser.php +++ b/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; diff --git a/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php b/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php index bddbe46d74..f3790e4b7c 100644 --- a/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php +++ b/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) diff --git a/src/applications/differential/render/DifferentialChangesetRenderer.php b/src/applications/differential/render/DifferentialChangesetRenderer.php index 8327a4103e..be440b47e7 100644 --- a/src/applications/differential/render/DifferentialChangesetRenderer.php +++ b/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; diff --git a/src/applications/phriction/controller/PhrictionDiffController.php b/src/applications/phriction/controller/PhrictionDiffController.php index 41f515dfbb..b979d7f9b9 100644 --- a/src/applications/phriction/controller/PhrictionDiffController.php +++ b/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); diff --git a/src/applications/transactions/view/PhabricatorApplicationTransactionTextDiffDetailView.php b/src/applications/transactions/view/PhabricatorApplicationTransactionTextDiffDetailView.php index 55fe586bc6..0e84652234 100644 --- a/src/applications/transactions/view/PhabricatorApplicationTransactionTextDiffDetailView.php +++ b/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);