From a100d97ed5cec0a1a78a1057b8b42032d810082a Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 7 Mar 2011 19:29:51 -0800 Subject: [PATCH] Preserve "next" URI by using OAuth 'state' parameter Summary: When a user clicks a link like /T32 and has to login, redirect them to the resource once they've authenticated if possible. OAuth has a param specifically for this, called 'state', so use it if possible. Facebook supports it but Github does not. Test Plan: logged in with facebook after viewing /D20 Reviewed By: aran Reviewers: aran CC: aran, epriestley Differential Revision: 61 --- .../controller/login/PhabricatorLoginController.php | 9 +++++++++ .../oauth/PhabricatorOAuthLoginController.php | 13 ++++++++++++- .../base/PhabricatorOAuthRegistrationController.php | 10 ++++++++++ ...habricatorOAuthDefaultRegistrationController.php | 1 + .../base/controller/base/PhabricatorController.php | 3 ++- src/applications/base/controller/base/__init__.php | 2 +- webroot/index.php | 9 +++++---- 7 files changed, 40 insertions(+), 7 deletions(-) diff --git a/src/applications/auth/controller/login/PhabricatorLoginController.php b/src/applications/auth/controller/login/PhabricatorLoginController.php index f55cc98662..c9cc43d098 100644 --- a/src/applications/auth/controller/login/PhabricatorLoginController.php +++ b/src/applications/auth/controller/login/PhabricatorLoginController.php @@ -30,6 +30,11 @@ class PhabricatorLoginController extends PhabricatorAuthController { return id(new AphrontRedirectResponse())->setURI('/'); } + $next_uri = $this->getRequest()->getPath(); + if ($next_uri == '/login/') { + $next_uri = null; + } + $password_auth = PhabricatorEnv::getEnvConfig('auth.password-auth-enabled'); $forms = array(); @@ -76,6 +81,7 @@ class PhabricatorLoginController extends PhabricatorAuthController { $form ->setUser($request->getUser()) ->setAction('/login/') + ->addHiddenInput('next', $next_uri) ->appendChild( id(new AphrontFormTextControl()) ->setLabel('Username/Email') @@ -97,6 +103,8 @@ class PhabricatorLoginController extends PhabricatorAuthController { $forms['Phabricator Login'] = $form; } + $oauth_state = $next_uri; + $providers = array( PhabricatorOAuthProvider::PROVIDER_FACEBOOK, PhabricatorOAuthProvider::PROVIDER_GITHUB, @@ -140,6 +148,7 @@ class PhabricatorLoginController extends PhabricatorAuthController { ->addHiddenInput('client_id', $client_id) ->addHiddenInput('redirect_uri', $redirect_uri) ->addHiddenInput('scope', $minimum_scope) + ->addHiddenInput('state', $oauth_state) ->setUser($request->getUser()) ->setMethod('GET') ->appendChild( diff --git a/src/applications/auth/controller/oauth/PhabricatorOAuthLoginController.php b/src/applications/auth/controller/oauth/PhabricatorOAuthLoginController.php index 87d6bb9668..6baf7fd6f4 100644 --- a/src/applications/auth/controller/oauth/PhabricatorOAuthLoginController.php +++ b/src/applications/auth/controller/oauth/PhabricatorOAuthLoginController.php @@ -23,6 +23,7 @@ class PhabricatorOAuthLoginController extends PhabricatorAuthController { private $accessToken; private $tokenExpires; + private $oauthState; public function shouldRequireLogin() { return false; @@ -120,6 +121,7 @@ class PhabricatorOAuthLoginController extends PhabricatorAuthController { 'account?

'); $dialog->addHiddenInput('token', $provider->getAccessToken()); $dialog->addHiddenInput('expires', $oauth_info->getTokenExpires()); + $dialog->addHiddenInput('state', $this->oauthState); $dialog->addSubmitButton('Link Accounts'); $dialog->addCancelButton('/settings/page/'.$provider_key.'/'); @@ -133,6 +135,12 @@ class PhabricatorOAuthLoginController extends PhabricatorAuthController { ->setURI('/settings/page/'.$provider_key.'/'); } + $next_uri = '/'; + if ($this->oauthState) { + // Make sure a blind redirect to evil.com is impossible. + $uri = new PhutilURI($this->oauthState); + $next_uri = $uri->getPath(); + } // Login with known auth. @@ -151,7 +159,7 @@ class PhabricatorOAuthLoginController extends PhabricatorAuthController { $request->setCookie('phusr', $known_user->getUsername()); $request->setCookie('phsid', $session_key); return id(new AphrontRedirectResponse()) - ->setURI('/'); + ->setURI($next_uri); } $oauth_email = $provider->retrieveUserEmail(); @@ -193,6 +201,7 @@ class PhabricatorOAuthLoginController extends PhabricatorAuthController { $controller->setOAuthProvider($provider); $controller->setOAuthInfo($oauth_info); + $controller->setOAuthState($this->oauthState); return $this->delegateToController($controller); } @@ -217,6 +226,7 @@ class PhabricatorOAuthLoginController extends PhabricatorAuthController { if ($token) { $this->tokenExpires = $request->getInt('expires'); $this->accessToken = $token; + $this->oauthState = $request->getStr('state'); return null; } @@ -274,6 +284,7 @@ class PhabricatorOAuthLoginController extends PhabricatorAuthController { } $this->accessToken = $token; + $this->oauthState = $request->getStr('state'); return null; } diff --git a/src/applications/auth/controller/oauthregistration/base/PhabricatorOAuthRegistrationController.php b/src/applications/auth/controller/oauthregistration/base/PhabricatorOAuthRegistrationController.php index addbe914ee..ec90b91ed0 100644 --- a/src/applications/auth/controller/oauthregistration/base/PhabricatorOAuthRegistrationController.php +++ b/src/applications/auth/controller/oauthregistration/base/PhabricatorOAuthRegistrationController.php @@ -21,6 +21,7 @@ abstract class PhabricatorOAuthRegistrationController private $oauthProvider; private $oauthInfo; + private $oauthState; final public function setOAuthInfo($info) { $this->oauthInfo = $info; @@ -40,4 +41,13 @@ abstract class PhabricatorOAuthRegistrationController return $this->oauthProvider; } + final public function setOAuthState($state) { + $this->oauthState = $state; + return $this; + } + + final public function getOAuthState() { + return $this->oauthState; + } + } diff --git a/src/applications/auth/controller/oauthregistration/default/PhabricatorOAuthDefaultRegistrationController.php b/src/applications/auth/controller/oauthregistration/default/PhabricatorOAuthDefaultRegistrationController.php index a72604cc7f..816092c6dd 100644 --- a/src/applications/auth/controller/oauthregistration/default/PhabricatorOAuthDefaultRegistrationController.php +++ b/src/applications/auth/controller/oauthregistration/default/PhabricatorOAuthDefaultRegistrationController.php @@ -125,6 +125,7 @@ class PhabricatorOAuthDefaultRegistrationController $form ->addHiddenInput('token', $provider->getAccessToken()) ->addHiddenInput('expires', $oauth_info->getTokenExpires()) + ->addHiddenInput('state', $this->getOAuthState()) ->setUser($request->getUser()) ->setAction($provider->getRedirectURI()) ->appendChild( diff --git a/src/applications/base/controller/base/PhabricatorController.php b/src/applications/base/controller/base/PhabricatorController.php index a05964173e..fdcc24e356 100644 --- a/src/applications/base/controller/base/PhabricatorController.php +++ b/src/applications/base/controller/base/PhabricatorController.php @@ -56,7 +56,8 @@ abstract class PhabricatorController extends AphrontController { } if ($this->shouldRequireLogin() && !$user->getPHID()) { - throw new AphrontRedirectException('/login/'); + $login_controller = new PhabricatorLoginController($request); + return $this->delegateToController($login_controller); } } diff --git a/src/applications/base/controller/base/__init__.php b/src/applications/base/controller/base/__init__.php index ebf030f26b..fa8ccfb02f 100644 --- a/src/applications/base/controller/base/__init__.php +++ b/src/applications/base/controller/base/__init__.php @@ -8,8 +8,8 @@ phutil_require_module('phabricator', 'aphront/console/core'); phutil_require_module('phabricator', 'aphront/controller'); -phutil_require_module('phabricator', 'aphront/exception/redirect'); phutil_require_module('phabricator', 'aphront/response/webpage'); +phutil_require_module('phabricator', 'applications/auth/controller/login'); phutil_require_module('phabricator', 'applications/people/storage/user'); phutil_require_module('phabricator', 'infrastructure/env'); phutil_require_module('phabricator', 'storage/queryfx'); diff --git a/webroot/index.php b/webroot/index.php index 9960f30c71..2c893c2060 100644 --- a/webroot/index.php +++ b/webroot/index.php @@ -83,10 +83,11 @@ $request = $application->buildRequest(); $application->setRequest($request); list($controller, $uri_data) = $application->buildController(); try { - $controller->willBeginExecution(); - - $controller->willProcessRequest($uri_data); - $response = $controller->processRequest(); + $response = $controller->willBeginExecution(); + if (!$response) { + $controller->willProcessRequest($uri_data); + $response = $controller->processRequest(); + } } catch (AphrontRedirectException $ex) { $response = id(new AphrontRedirectResponse()) ->setURI($ex->getURI());