From e0f99484ac91bba8f671d61b2df4299ab5b5c1ba Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 26 Sep 2013 18:45:04 -0700 Subject: [PATCH] Make Differential views capability-sensitive Summary: Ref T603. Make Differential behaviors for logged-out and underprivleged users more similar to other apps. I'm going to drop this "anonymous access" thing at some point, but `reviews.fb.net` actually looks like it's running semi-modern code, so leave it alive until we have a more compelling replacement in the upstream. Test Plan: As a logged out user, browsed Differential and clicked things and such. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T603 Differential Revision: https://secure.phabricator.com/D7148 --- .../DifferentialChangesetViewController.php | 21 +++- .../DifferentialRevisionListController.php | 5 +- .../DifferentialRevisionViewController.php | 96 +++++++++++-------- .../query/DifferentialDiffQuery.php | 26 +++++ .../differential/storage/DifferentialDiff.php | 18 ++++ ...bricatorHelpKeyboardShortcutController.php | 4 + 6 files changed, 130 insertions(+), 40 deletions(-) diff --git a/src/applications/differential/controller/DifferentialChangesetViewController.php b/src/applications/differential/controller/DifferentialChangesetViewController.php index a445020e67..e5c5fbbc83 100644 --- a/src/applications/differential/controller/DifferentialChangesetViewController.php +++ b/src/applications/differential/controller/DifferentialChangesetViewController.php @@ -3,7 +3,15 @@ final class DifferentialChangesetViewController extends DifferentialController { public function shouldRequireLogin() { - return !$this->allowsAnonymousAccess(); + if ($this->allowsAnonymousAccess()) { + return false; + } + + return parent::shouldRequireLogin(); + } + + public function shouldAllowPublic() { + return true; } public function processRequest() { @@ -28,6 +36,17 @@ final class DifferentialChangesetViewController extends DifferentialController { return new Aphront404Response(); } + // TODO: (T603) Make Changeset policy-aware. For now, just fake it + // by making sure we can see the diff. + $diff = id(new DifferentialDiffQuery()) + ->setViewer($request->getUser()) + ->withIDs(array($changeset->getDiffID())) + ->executeOne(); + if (!$diff) { + return new Aphront404Response(); + } + + $view = $request->getStr('view'); if ($view) { $changeset->attachHunks($changeset->loadHunks()); diff --git a/src/applications/differential/controller/DifferentialRevisionListController.php b/src/applications/differential/controller/DifferentialRevisionListController.php index f7f9931be7..8322f5615b 100644 --- a/src/applications/differential/controller/DifferentialRevisionListController.php +++ b/src/applications/differential/controller/DifferentialRevisionListController.php @@ -6,7 +6,10 @@ final class DifferentialRevisionListController extends DifferentialController private $queryKey; public function shouldRequireLogin() { - return !$this->allowsAnonymousAccess(); + if ($this->allowsAnonymousAccess()) { + return false; + } + return parent::shouldRequireLogin(); } public function shouldAllowPublic() { diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index 54750a094b..a1861e962f 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -5,7 +5,14 @@ final class DifferentialRevisionViewController extends DifferentialController { private $revisionID; public function shouldRequireLogin() { - return !$this->allowsAnonymousAccess(); + if ($this->allowsAnonymousAccess()) { + return false; + } + return parent::shouldRequireLogin(); + } + + public function shouldAllowPublic() { + return true; } public function willProcessRequest(array $data) { @@ -405,8 +412,15 @@ final class DifferentialRevisionViewController extends DifferentialController { )); if ($comment_form) { $page_pane->appendChild($comment_form->render()); + } else { + // TODO: For now, just use this to get "Login to Comment". + $page_pane->appendChild( + id(new PhabricatorApplicationTransactionCommentView()) + ->setUser($user) + ->setRequestURI($request->getRequestURI())); } + $object_id = 'D'.$revision->getID(); $top_anchor = id(new PhabricatorAnchorView()) @@ -493,58 +507,64 @@ final class DifferentialRevisionViewController extends DifferentialController { $viewer_is_owner = ($revision->getAuthorPHID() == $viewer_phid); $viewer_is_reviewer = in_array($viewer_phid, $revision->getReviewers()); $viewer_is_cc = in_array($viewer_phid, $revision->getCCPHIDs()); - $viewer_is_anonymous = !$this->getRequest()->getUser()->isLoggedIn(); + $logged_in = $this->getRequest()->getUser()->isLoggedIn(); $status = $revision->getStatus(); $revision_id = $revision->getID(); $revision_phid = $revision->getPHID(); $links = array(); - if ($viewer_is_owner) { + $can_edit = PhabricatorPolicyFilter::hasCapability( + $user, + $revision, + PhabricatorPolicyCapability::CAN_EDIT); + + $links[] = array( + 'icon' => 'edit', + 'href' => "/differential/revision/edit/{$revision_id}/", + 'name' => pht('Edit Revision'), + 'disabled' => !$can_edit, + 'sigil' => $can_edit ? null : 'workflow', + ); + + if (!$viewer_is_owner && !$viewer_is_reviewer) { + $action = $viewer_is_cc ? 'rem' : 'add'; $links[] = array( - 'icon' => 'edit', - 'href' => "/differential/revision/edit/{$revision_id}/", - 'name' => pht('Edit Revision'), + 'icon' => $viewer_is_cc ? 'disable' : 'check', + 'href' => "/differential/subscribe/{$action}/{$revision_id}/", + 'name' => $viewer_is_cc ? pht('Unsubscribe') : pht('Subscribe'), + 'instant' => $logged_in, + 'disabled' => !$logged_in, + 'sigil' => $can_edit ? null : 'workflow', + ); + } else { + $links[] = array( + 'icon' => 'enable', + 'name' => pht('Automatically Subscribed'), + 'disabled' => true, ); } - if (!$viewer_is_anonymous) { + require_celerity_resource('phabricator-object-selector-css'); + require_celerity_resource('javelin-behavior-phabricator-object-selector'); - if (!$viewer_is_owner && !$viewer_is_reviewer) { - $action = $viewer_is_cc ? 'rem' : 'add'; - $links[] = array( - 'icon' => $viewer_is_cc ? 'disable' : 'check', - 'href' => "/differential/subscribe/{$action}/{$revision_id}/", - 'name' => $viewer_is_cc ? pht('Unsubscribe') : pht('Subscribe'), - 'instant' => true, - ); - } else { - $links[] = array( - 'icon' => 'enable', - 'name' => pht('Automatically Subscribed'), - 'disabled' => true, - ); - } - - require_celerity_resource('phabricator-object-selector-css'); - require_celerity_resource('javelin-behavior-phabricator-object-selector'); + $links[] = array( + 'icon' => 'link', + 'name' => pht('Edit Dependencies'), + 'href' => "/search/attach/{$revision_phid}/DREV/dependencies/", + 'sigil' => 'workflow', + 'disabled' => !$can_edit, + ); + $maniphest = 'PhabricatorApplicationManiphest'; + if (PhabricatorApplication::isClassInstalled($maniphest)) { $links[] = array( - 'icon' => 'link', - 'name' => pht('Edit Dependencies'), - 'href' => "/search/attach/{$revision_phid}/DREV/dependencies/", + 'icon' => 'attach', + 'name' => pht('Edit Maniphest Tasks'), + 'href' => "/search/attach/{$revision_phid}/TASK/", 'sigil' => 'workflow', + 'disabled' => !$can_edit, ); - - $maniphest = 'PhabricatorApplicationManiphest'; - if (PhabricatorApplication::isClassInstalled($maniphest)) { - $links[] = array( - 'icon' => 'attach', - 'name' => pht('Edit Maniphest Tasks'), - 'href' => "/search/attach/{$revision_phid}/TASK/", - 'sigil' => 'workflow', - ); - } } $request_uri = $this->getRequest()->getRequestURI(); diff --git a/src/applications/differential/query/DifferentialDiffQuery.php b/src/applications/differential/query/DifferentialDiffQuery.php index d1344e4245..a608a5421e 100644 --- a/src/applications/differential/query/DifferentialDiffQuery.php +++ b/src/applications/differential/query/DifferentialDiffQuery.php @@ -44,6 +44,32 @@ final class DifferentialDiffQuery } public function willFilterPage(array $diffs) { + $revision_ids = array_filter(mpull($diffs, 'getRevisionID')); + + $revisions = array(); + if ($revision_ids) { + $revisions = id(new DifferentialRevisionQuery()) + ->setViewer($this->getViewer()) + ->withIDs($revision_ids) + ->execute(); + } + + foreach ($diffs as $key => $diff) { + if (!$diff->getRevisionID()) { + $diff->attachRevision(null); + continue; + } + + $revision = idx($revisions, $diff->getRevisionID()); + if ($revision) { + $diff->attachRevision($revision); + continue; + } + + unset($diffs[$key]); + } + + if ($this->needChangesets) { $this->loadChangesets($diffs); } diff --git a/src/applications/differential/storage/DifferentialDiff.php b/src/applications/differential/storage/DifferentialDiff.php index a7ba369dc9..32cded90fe 100644 --- a/src/applications/differential/storage/DifferentialDiff.php +++ b/src/applications/differential/storage/DifferentialDiff.php @@ -32,6 +32,7 @@ final class DifferentialDiff private $unsavedChangesets = array(); private $changesets = self::ATTACHABLE; private $arcanistProject = self::ATTACHABLE; + private $revision = self::ATTACHABLE; public function addUnsavedChangeset(DifferentialChangeset $changeset) { if ($this->changesets === null) { @@ -284,6 +285,15 @@ final class DifferentialDiff return $changes; } + public function getRevision() { + return $this->assertAttached($this->revision); + } + + public function attachRevision(DifferentialRevision $revision = null) { + $this->revision = $revision; + return $this; + } + /* -( PhabricatorPolicyInterface )----------------------------------------- */ @@ -295,10 +305,18 @@ final class DifferentialDiff } public function getPolicy($capability) { + if ($this->getRevision()) { + return $this->getRevision()->getPolicy($capability); + } + return PhabricatorPolicies::POLICY_USER; } public function hasAutomaticCapability($capability, PhabricatorUser $viewer) { + if ($this->getRevision()) { + return $this->getRevision()->hasAutomaticCapability($capability, $viewer); + } + return false; } diff --git a/src/applications/help/controller/PhabricatorHelpKeyboardShortcutController.php b/src/applications/help/controller/PhabricatorHelpKeyboardShortcutController.php index f745b6da3c..55321a9155 100644 --- a/src/applications/help/controller/PhabricatorHelpKeyboardShortcutController.php +++ b/src/applications/help/controller/PhabricatorHelpKeyboardShortcutController.php @@ -3,6 +3,10 @@ final class PhabricatorHelpKeyboardShortcutController extends PhabricatorHelpController { + public function shouldAllowPublic() { + return true; + } + public function processRequest() { $request = $this->getRequest(); $user = $request->getUser();