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

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(

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?</p>');
         $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;
   }

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;
+  }
+
 }

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(

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);
     }
   }
 

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');

webroot/index.php

@@ -83,10 +83,11 @@ $request = $application->buildRequest();
 $application->setRequest($request);
 list($controller, $uri_data) = $application->buildController();
 try {
-  $controller->willBeginExecution();
-
+  $response = $controller->willBeginExecution();
+  if (!$response) {
     $controller->willProcessRequest($uri_data);
     $response = $controller->processRequest();
+  }
 } catch (AphrontRedirectException $ex) {
   $response = id(new AphrontRedirectResponse())
     ->setURI($ex->getURI());