From 6b1c27eb0ead43ba6623b07d3a6e5ccb243d8888 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 30 Sep 2012 19:44:09 -0700 Subject: [PATCH] Make "public" pastes meaningfully visible to logged-out users Summary: - Introduce `shouldAllowPublic()`, indicating that logged-out users are OK in a controller if the install is configured to permit public policies. - Make Paste views and lists allow public users. - Make UI do sensible things with respect to disabling links, etc. - Improve behavior of "you need to login" with respect to policy exceptions and Ajax requests. Test Plan: Looked at "public" paste, saw all unavailable UI disabled, clicked it, got appropraite prompts. Reviewers: vrana, btrahan Reviewed By: vrana CC: aran Maniphest Tasks: T603 Differential Revision: https://secure.phabricator.com/D3502 --- src/aphront/AphrontController.php | 1 - ...AphrontDefaultApplicationConfiguration.php | 12 ++++++++++ .../controller/PhabricatorLoginController.php | 23 ++++++++++++++++++- .../base/controller/PhabricatorController.php | 12 ++++++++++ .../controller/PhabricatorPasteController.php | 13 +++++++++-- .../PhabricatorPasteListController.php | 4 ++++ .../PhabricatorPasteViewController.php | 8 +++++++ 7 files changed, 69 insertions(+), 4 deletions(-) diff --git a/src/aphront/AphrontController.php b/src/aphront/AphrontController.php index 231a580667..94f0071f18 100644 --- a/src/aphront/AphrontController.php +++ b/src/aphront/AphrontController.php @@ -50,7 +50,6 @@ abstract class AphrontController { return $controller->processRequest(); } - final public function setCurrentApplication( PhabricatorApplication $current_application) { diff --git a/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php b/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php index ef935fdf89..0770c5dc5f 100644 --- a/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php +++ b/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php @@ -336,6 +336,18 @@ class AphrontDefaultApplicationConfiguration } if ($ex instanceof PhabricatorPolicyException) { + + if (!$user->isLoggedIn()) { + // If the user isn't logged in, just give them a login form. This is + // probably a generally more useful response than a policy dialog that + // they have to click through to get a login form. + // + // Possibly we should add a header here like "you need to login to see + // the thing you are trying to look at". + $login_controller = new PhabricatorLoginController($request); + return $login_controller->processRequest(); + } + $content = '
'. phutil_escape_html($ex->getMessage()). diff --git a/src/applications/auth/controller/PhabricatorLoginController.php b/src/applications/auth/controller/PhabricatorLoginController.php index a21114eb87..2d71fef199 100644 --- a/src/applications/auth/controller/PhabricatorLoginController.php +++ b/src/applications/auth/controller/PhabricatorLoginController.php @@ -25,12 +25,33 @@ final class PhabricatorLoginController public function processRequest() { $request = $this->getRequest(); + $user = $request->getUser(); - if ($request->getUser()->getPHID()) { + if ($user->isLoggedIn()) { // Kick the user out if they're already logged in. return id(new AphrontRedirectResponse())->setURI('/'); } + if ($request->isAjax()) { + + // We end up here if the user clicks a workflow link that they need to + // login to use. We give them a dialog saying "You need to login..". + + if ($request->isDialogFormPost()) { + return id(new AphrontRedirectResponse())->setURI( + $request->getRequestURI()); + } + + $dialog = new AphrontDialogView(); + $dialog->setUser($user); + $dialog->setTitle('Login Required'); + $dialog->appendChild('

You must login to continue.

'); + $dialog->addSubmitButton('Login'); + $dialog->addCancelButton('/', 'Cancel'); + + return id(new AphrontDialogResponse())->setDialog($dialog); + } + if ($request->isConduit()) { // A common source of errors in Conduit client configuration is getting diff --git a/src/applications/base/controller/PhabricatorController.php b/src/applications/base/controller/PhabricatorController.php index 58641d3cf1..2c1148c545 100644 --- a/src/applications/base/controller/PhabricatorController.php +++ b/src/applications/base/controller/PhabricatorController.php @@ -21,6 +21,14 @@ abstract class PhabricatorController extends AphrontController { private $handles; public function shouldRequireLogin() { + + // If this install is configured to allow public resources and the + // controller works in public mode, allow the request through. + $is_public_allowed = PhabricatorEnv::getEnvConfig('policy.allow-public'); + if ($is_public_allowed && $this->shouldAllowPublic()) { + return false; + } + return true; } @@ -32,6 +40,10 @@ abstract class PhabricatorController extends AphrontController { return true; } + public function shouldAllowPublic() { + return false; + } + public function shouldRequireEmailVerification() { $need_verify = PhabricatorUserEmail::isEmailVerificationRequired(); $need_login = $this->shouldRequireLogin(); diff --git a/src/applications/paste/controller/PhabricatorPasteController.php b/src/applications/paste/controller/PhabricatorPasteController.php index 7aaa584fd0..af6a031709 100644 --- a/src/applications/paste/controller/PhabricatorPasteController.php +++ b/src/applications/paste/controller/PhabricatorPasteController.php @@ -19,6 +19,8 @@ abstract class PhabricatorPasteController extends PhabricatorController { public function buildSideNavView(PhabricatorPaste $paste = null) { + $user = $this->getRequest()->getUser(); + $nav = new AphrontSideNavFilterView(); $nav->setBaseURI(new PhutilURI($this->getApplicationURI('filter/'))); @@ -28,11 +30,18 @@ abstract class PhabricatorPasteController extends PhabricatorController { } $nav->addLabel('Create'); - $nav->addFilter('edit', 'New Paste', $this->getApplicationURI()); + $nav->addFilter( + 'edit', + 'New Paste', + $this->getApplicationURI(), + $relative = false, + $class = ($user->isLoggedIn() ? null : 'disabled')); $nav->addSpacer(); $nav->addLabel('Pastes'); - $nav->addFilter('my', 'My Pastes'); + if ($user->isLoggedIn()) { + $nav->addFilter('my', 'My Pastes'); + } $nav->addFilter('all', 'All Pastes'); return $nav; diff --git a/src/applications/paste/controller/PhabricatorPasteListController.php b/src/applications/paste/controller/PhabricatorPasteListController.php index 6f281bacfc..373f95b2ad 100644 --- a/src/applications/paste/controller/PhabricatorPasteListController.php +++ b/src/applications/paste/controller/PhabricatorPasteListController.php @@ -18,6 +18,10 @@ final class PhabricatorPasteListController extends PhabricatorPasteController { + public function shouldRequireLogin() { + return false; + } + private $filter; public function willProcessRequest(array $data) { diff --git a/src/applications/paste/controller/PhabricatorPasteViewController.php b/src/applications/paste/controller/PhabricatorPasteViewController.php index 83308d52b7..220e533280 100644 --- a/src/applications/paste/controller/PhabricatorPasteViewController.php +++ b/src/applications/paste/controller/PhabricatorPasteViewController.php @@ -18,6 +18,10 @@ final class PhabricatorPasteViewController extends PhabricatorPasteController { + public function shouldAllowPublic() { + return true; + } + private $id; private $handles; @@ -98,6 +102,8 @@ final class PhabricatorPasteViewController extends PhabricatorPasteController { $paste, PhabricatorPolicyCapability::CAN_EDIT); + $can_fork = $user->isLoggedIn(); + return id(new PhabricatorActionListView()) ->setUser($user) ->setObject($paste) @@ -105,6 +111,8 @@ final class PhabricatorPasteViewController extends PhabricatorPasteController { id(new PhabricatorActionView()) ->setName(pht('Fork This Paste')) ->setIcon('fork') + ->setDisabled(!$can_fork) + ->setWorkflow(!$can_fork) ->setHref($this->getApplicationURI('?parent='.$paste->getID()))) ->addAction( id(new PhabricatorActionView())