Handle state in OAuth2 client

29 files changed, 133 insertions, 51 deletions

diff --git a/ChangeLog b/ChangeLog
index 5fdbf6e3..5fded0d4 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -9,6 +9,7 @@ New features:
 
 Improvements:
 
+* Handle state in OAuth2 client
 * Allow to use the original template in overridden templates
 * Unification of the project header
 * Refactoring of Javascript code
diff --git a/app/Controller/Oauth.php b/app/Controller/Oauth.php
index 452faecd..12b91144 100644
--- a/app/Controller/Oauth.php
+++ b/app/Controller/Oauth.php
@@ -2,6 +2,8 @@
 
 namespace Kanboard\Controller;
 
+use Kanboard\Core\Security\OAuthAuthenticationProviderInterface;
+
 /**
  * OAuth controller
  *
@@ -11,25 +13,6 @@ namespace Kanboard\Controller;
 class Oauth extends Base
 {
     /**
-     * Unlink external account
-     *
-     * @access public
-     */
-    public function unlink()
-    {
-        $backend = $this->request->getStringParam('backend');
-        $this->checkCSRFParam();
-
-        if ($this->authenticationManager->getProvider($backend)->unlink($this->userSession->getId())) {
-            $this->flash->success(t('Your external account is not linked anymore to your profile.'));
-        } else {
-            $this->flash->failure(t('Unable to unlink your external account.'));
-        }
-
-        $this->response->redirect($this->helper->url->to('user', 'external', array('user_id' => $this->userSession->getId())));
-    }
-
-    /**
      * Redirect to the provider if no code received
      *
      * @access private
@@ -38,9 +21,10 @@ class Oauth extends Base
     protected function step1($provider)
     {
         $code = $this->request->getStringParam('code');
+        $state = $this->request->getStringParam('state');
 
         if (! empty($code)) {
-            $this->step2($provider, $code);
+            $this->step2($provider, $code, $state);
         } else {
             $this->response->redirect($this->authenticationManager->getProvider($provider)->getService()->getAuthorizationUrl());
         }
@@ -50,34 +34,44 @@ class Oauth extends Base
      * Link or authenticate the user
      *
      * @access protected
-     * @param string $provider
+     * @param string $providerName
      * @param string $code
+     * @param string $state
      */
-    protected function step2($provider, $code)
+    protected function step2($providerName, $code, $state)
     {
-        $this->authenticationManager->getProvider($provider)->setCode($code);
+        $provider = $this->authenticationManager->getProvider($providerName);
+        $provider->setCode($code);
+        $hasValidState = $provider->getService()->isValidateState($state);
 
         if ($this->userSession->isLogged()) {
-            $this->link($provider);
+            if ($hasValidState) {
+                $this->link($provider);
+            } else {
+                $this->flash->failure(t('The OAuth2 state parameter is invalid'));
+                $this->response->redirect($this->helper->url->to('user', 'external', array('user_id' => $this->userSession->getId())));
+            }
+        } else {
+            if ($hasValidState) {
+                $this->authenticate($providerName);
+            } else {
+                $this->authenticationFailure(t('The OAuth2 state parameter is invalid'));
+            }
         }
-
-        $this->authenticate($provider);
     }
 
     /**
      * Link the account
      *
      * @access protected
-     * @param string $provider
+     * @param  OAuthAuthenticationProviderInterface $provider
      */
-    protected function link($provider)
+    protected function link(OAuthAuthenticationProviderInterface $provider)
     {
-        $authProvider = $this->authenticationManager->getProvider($provider);
-
-        if (! $authProvider->authenticate()) {
+        if (! $provider->authenticate()) {
             $this->flash->failure(t('External authentication failed'));
         } else {
-            $this->userProfile->assign($this->userSession->getId(), $authProvider->getUser());
+            $this->userProfile->assign($this->userSession->getId(), $provider->getUser());
             $this->flash->success(t('Your external account is linked to your profile successfully.'));
         }
 
@@ -85,22 +79,52 @@ class Oauth extends Base
     }
 
     /**
+     * Unlink external account
+     *
+     * @access public
+     */
+    public function unlink()
+    {
+        $backend = $this->request->getStringParam('backend');
+        $this->checkCSRFParam();
+
+        if ($this->authenticationManager->getProvider($backend)->unlink($this->userSession->getId())) {
+            $this->flash->success(t('Your external account is not linked anymore to your profile.'));
+        } else {
+            $this->flash->failure(t('Unable to unlink your external account.'));
+        }
+
+        $this->response->redirect($this->helper->url->to('user', 'external', array('user_id' => $this->userSession->getId())));
+    }
+
+    /**
      * Authenticate the account
      *
      * @access protected
-     * @param string $provider
+     * @param string $providerName
      */
-    protected function authenticate($provider)
+    protected function authenticate($providerName)
     {
-        if ($this->authenticationManager->oauthAuthentication($provider)) {
+        if ($this->authenticationManager->oauthAuthentication($providerName)) {
             $this->response->redirect($this->helper->url->to('app', 'index'));
         } else {
-            $this->response->html($this->helper->layout->app('auth/index', array(
-                'errors' => array('login' => t('External authentication failed')),
-                'values' => array(),
-                'no_layout' => true,
-                'title' => t('Login')
-            )));
+            $this->authenticationFailure(t('External authentication failed'));
         }
     }
+
+    /**
+     * Show login failure page
+     *
+     * @access protected
+     * @param  string $message
+     */
+    protected function authenticationFailure($message)
+    {
+        $this->response->html($this->helper->layout->app('auth/index', array(
+            'errors' => array('login' => $message),
+            'values' => array(),
+            'no_layout' => true,
+            'title' => t('Login')
+        )));
+    }
 }
diff --git a/app/Core/Http/OAuth2.php b/app/Core/Http/OAuth2.php
index 6fa1fb0a..211ca5b4 100644
--- a/app/Core/Http/OAuth2.php
+++ b/app/Core/Http/OAuth2.php
@@ -12,14 +12,14 @@ use Kanboard\Core\Base;
  */
 class OAuth2 extends Base
 {
-    private $clientId;
-    private $secret;
-    private $callbackUrl;
-    private $authUrl;
-    private $tokenUrl;
-    private $scopes;
-    private $tokenType;
-    private $accessToken;
+    protected $clientId;
+    protected $secret;
+    protected $callbackUrl;
+    protected $authUrl;
+    protected $tokenUrl;
+    protected $scopes;
+    protected $tokenType;
+    protected $accessToken;
 
     /**
      * Create OAuth2 service
@@ -46,6 +46,33 @@ class OAuth2 extends Base
     }
 
     /**
+     * Generate OAuth2 state and return the token value
+     *
+     * @access public
+     * @return string
+     */
+    public function getState()
+    {
+        if (! isset($this->sessionStorage->oauthState) || empty($this->sessionStorage->oauthState)) {
+            $this->sessionStorage->oauthState = $this->token->getToken();
+        }
+
+        return $this->sessionStorage->oauthState;
+    }
+
+    /**
+     * Check the validity of the state (CSRF token)
+     *
+     * @access public
+     * @param  string $state
+     * @return bool
+     */
+    public function isValidateState($state)
+    {
+        return $state === $this->getState();
+    }
+
+    /**
      * Get authorization url
      *
      * @access public
@@ -58,6 +85,7 @@ class OAuth2 extends Base
             'client_id' => $this->clientId,
             'redirect_uri' => $this->callbackUrl,
             'scope' => implode(' ', $this->scopes),
+            'state' => $this->getState(),
         );
 
         return $this->authUrl.'?'.http_build_query($params);
@@ -94,6 +122,7 @@ class OAuth2 extends Base
                 'client_secret' => $this->secret,
                 'redirect_uri' => $this->callbackUrl,
                 'grant_type' => 'authorization_code',
+                'state' => $this->getState(),
             );
 
             $response = json_decode($this->httpClient->postForm($this->tokenUrl, $params, array('Accept: application/json')), true);
diff --git a/app/Core/Session/SessionStorage.php b/app/Core/Session/SessionStorage.php
index 667d9253..6e2f9660 100644
--- a/app/Core/Session/SessionStorage.php
+++ b/app/Core/Session/SessionStorage.php
@@ -21,6 +21,7 @@ namespace Kanboard\Core\Session;
  * @property bool   $boardCollapsed
  * @property bool   $twoFactorBeforeCodeCalled
  * @property string $twoFactorSecret
+ * @property string $oauthState
  */
 class SessionStorage
 {
diff --git a/app/Locale/bs_BA/translations.php b/app/Locale/bs_BA/translations.php
index 8d653d4f..7ca864f4 100644
--- a/app/Locale/bs_BA/translations.php
+++ b/app/Locale/bs_BA/translations.php
@@ -1152,4 +1152,5 @@ return array(
     // 'Avatar' => '',
     // 'Upload my avatar image' => '',
     // 'Remove my image' => '',
+    // 'The OAuth2 state parameter is invalid' => '',
 );
diff --git a/app/Locale/cs_CZ/translations.php b/app/Locale/cs_CZ/translations.php
index 3606eddf..b2921de9 100644
--- a/app/Locale/cs_CZ/translations.php
+++ b/app/Locale/cs_CZ/translations.php
@@ -1152,4 +1152,5 @@ return array(
     // 'Avatar' => '',
     // 'Upload my avatar image' => '',
     // 'Remove my image' => '',
+    // 'The OAuth2 state parameter is invalid' => '',
 );
diff --git a/app/Locale/da_DK/translations.php b/app/Locale/da_DK/translations.php
index cf3f0191..c4743922 100644
--- a/app/Locale/da_DK/translations.php
+++ b/app/Locale/da_DK/translations.php
@@ -1152,4 +1152,5 @@ return array(
     // 'Avatar' => '',
     // 'Upload my avatar image' => '',
     // 'Remove my image' => '',
+    // 'The OAuth2 state parameter is invalid' => '',
 );
diff --git a/app/Locale/de_DE/translations.php b/app/Locale/de_DE/translations.php
index 1090d6c9..af88b374 100644
--- a/app/Locale/de_DE/translations.php
+++ b/app/Locale/de_DE/translations.php
@@ -1152,4 +1152,5 @@ return array(
     // 'Avatar' => '',
     // 'Upload my avatar image' => '',
     // 'Remove my image' => '',
+    // 'The OAuth2 state parameter is invalid' => '',
 );
diff --git a/app/Locale/el_GR/translations.php b/app/Locale/el_GR/translations.php
index 04efa7e7..9a31e485 100644
--- a/app/Locale/el_GR/translations.php
+++ b/app/Locale/el_GR/translations.php
@@ -1152,4 +1152,5 @@ return array(
     // 'Avatar' => '',
     // 'Upload my avatar image' => '',
     // 'Remove my image' => '',
+    // 'The OAuth2 state parameter is invalid' => '',
 );
diff --git a/app/Locale/es_ES/translations.php b/app/Locale/es_ES/translations.php
index 477f3655..c3623369 100644
--- a/app/Locale/es_ES/translations.php
+++ b/app/Locale/es_ES/translations.php
@@ -1152,4 +1152,5 @@ return array(
     // 'Avatar' => '',
     // 'Upload my avatar image' => '',
     // 'Remove my image' => '',
+    // 'The OAuth2 state parameter is invalid' => '',
 );
diff --git a/app/Locale/fi_FI/translations.php b/app/Locale/fi_FI/translations.php
index a32082e3..8e5dd81f 100644
--- a/app/Locale/fi_FI/translations.php
+++ b/app/Locale/fi_FI/translations.php
@@ -1152,4 +1152,5 @@ return array(
     // 'Avatar' => '',
     // 'Upload my avatar image' => '',
     // 'Remove my image' => '',
+    // 'The OAuth2 state parameter is invalid' => '',
 );
diff --git a/app/Locale/fr_FR/translations.php b/app/Locale/fr_FR/translations.php
index 00e64876..cedd6039 100644
--- a/app/Locale/fr_FR/translations.php
+++ b/app/Locale/fr_FR/translations.php
@@ -1152,4 +1152,5 @@ return array(
     'Avatar' => 'Avatar',
     'Upload my avatar image' => 'Uploader mon image d\'avatar',
     'Remove my image' => 'Supprimer mon image',
+    'The OAuth2 state parameter is invalid' => 'Le paramètre "state" de OAuth2 est invalide',
 );
diff --git a/app/Locale/hu_HU/translations.php b/app/Locale/hu_HU/translations.php
index f2e1cafb..f642a6c1 100644
--- a/app/Locale/hu_HU/translations.php
+++ b/app/Locale/hu_HU/translations.php
@@ -1152,4 +1152,5 @@ return array(
     // 'Avatar' => '',
     // 'Upload my avatar image' => '',
     // 'Remove my image' => '',
+    // 'The OAuth2 state parameter is invalid' => '',
 );
diff --git a/app/Locale/id_ID/translations.php b/app/Locale/id_ID/translations.php
index 8d279633..3f105054 100644
--- a/app/Locale/id_ID/translations.php
+++ b/app/Locale/id_ID/translations.php
@@ -1152,4 +1152,5 @@ return array(
     // 'Avatar' => '',
     // 'Upload my avatar image' => '',
     // 'Remove my image' => '',
+    // 'The OAuth2 state parameter is invalid' => '',
 );
diff --git a/app/Locale/it_IT/translations.php b/app/Locale/it_IT/translations.php
index 87327462..93ceb03f 100644
--- a/app/Locale/it_IT/translations.php
+++ b/app/Locale/it_IT/translations.php
@@ -1152,4 +1152,5 @@ return array(
     // 'Avatar' => '',
     // 'Upload my avatar image' => '',
     // 'Remove my image' => '',
+    // 'The OAuth2 state parameter is invalid' => '',
 );
diff --git a/app/Locale/ja_JP/translations.php b/app/Locale/ja_JP/translations.php
index aa8cc654..b48eabd8 100644
--- a/app/Locale/ja_JP/translations.php
+++ b/app/Locale/ja_JP/translations.php
@@ -1152,4 +1152,5 @@ return array(
     // 'Avatar' => '',
     // 'Upload my avatar image' => '',
     // 'Remove my image' => '',
+    // 'The OAuth2 state parameter is invalid' => '',
 );
diff --git a/app/Locale/my_MY/translations.php b/app/Locale/my_MY/translations.php
index be41c19c..36b3db0b 100644
--- a/app/Locale/my_MY/translations.php
+++ b/app/Locale/my_MY/translations.php
@@ -1152,4 +1152,5 @@ return array(
     // 'Avatar' => '',
     // 'Upload my avatar image' => '',
     // 'Remove my image' => '',
+    // 'The OAuth2 state parameter is invalid' => '',
 );
diff --git a/app/Locale/nb_NO/translations.php b/app/Locale/nb_NO/translations.php
index 0e214cf4..465efb53 100644
--- a/app/Locale/nb_NO/translations.php
+++ b/app/Locale/nb_NO/translations.php
@@ -1152,4 +1152,5 @@ return array(
     // 'Avatar' => '',
     // 'Upload my avatar image' => '',
     // 'Remove my image' => '',
+    // 'The OAuth2 state parameter is invalid' => '',
 );
diff --git a/app/Locale/nl_NL/translations.php b/app/Locale/nl_NL/translations.php
index dc68eb34..3c3fa1ee 100644
--- a/app/Locale/nl_NL/translations.php
+++ b/app/Locale/nl_NL/translations.php
@@ -1152,4 +1152,5 @@ return array(
     // 'Avatar' => '',
     // 'Upload my avatar image' => '',
     // 'Remove my image' => '',
+    // 'The OAuth2 state parameter is invalid' => '',
 );
diff --git a/app/Locale/pl_PL/translations.php b/app/Locale/pl_PL/translations.php
index 0d020dcb..d06e347f 100644
--- a/app/Locale/pl_PL/translations.php
+++ b/app/Locale/pl_PL/translations.php
@@ -1152,4 +1152,5 @@ return array(
     // 'Avatar' => '',
     // 'Upload my avatar image' => '',
     // 'Remove my image' => '',
+    // 'The OAuth2 state parameter is invalid' => '',
 );
diff --git a/app/Locale/pt_BR/translations.php b/app/Locale/pt_BR/translations.php
index ebed08cd..050d1a9f 100644
--- a/app/Locale/pt_BR/translations.php
+++ b/app/Locale/pt_BR/translations.php
@@ -1152,4 +1152,5 @@ return array(
     // 'Avatar' => '',
     // 'Upload my avatar image' => '',
     // 'Remove my image' => '',
+    // 'The OAuth2 state parameter is invalid' => '',
 );
diff --git a/app/Locale/pt_PT/translations.php b/app/Locale/pt_PT/translations.php
index 4d2d20b4..1c327887 100644
--- a/app/Locale/pt_PT/translations.php
+++ b/app/Locale/pt_PT/translations.php
@@ -1152,4 +1152,5 @@ return array(
     // 'Avatar' => '',
     // 'Upload my avatar image' => '',
     // 'Remove my image' => '',
+    // 'The OAuth2 state parameter is invalid' => '',
 );
diff --git a/app/Locale/ru_RU/translations.php b/app/Locale/ru_RU/translations.php
index 1d93b3f3..3cb3c6bb 100644
--- a/app/Locale/ru_RU/translations.php
+++ b/app/Locale/ru_RU/translations.php
@@ -1152,4 +1152,5 @@ return array(
     // 'Avatar' => '',
     // 'Upload my avatar image' => '',
     // 'Remove my image' => '',
+    // 'The OAuth2 state parameter is invalid' => '',
 );
diff --git a/app/Locale/sr_Latn_RS/translations.php b/app/Locale/sr_Latn_RS/translations.php
index 634f6f8c..c7070a8d 100644
--- a/app/Locale/sr_Latn_RS/translations.php
+++ b/app/Locale/sr_Latn_RS/translations.php
@@ -1152,4 +1152,5 @@ return array(
     // 'Avatar' => '',
     // 'Upload my avatar image' => '',
     // 'Remove my image' => '',
+    // 'The OAuth2 state parameter is invalid' => '',
 );
diff --git a/app/Locale/sv_SE/translations.php b/app/Locale/sv_SE/translations.php
index 4dcc63ad..e4728d2d 100644
--- a/app/Locale/sv_SE/translations.php
+++ b/app/Locale/sv_SE/translations.php
@@ -1152,4 +1152,5 @@ return array(
     // 'Avatar' => '',
     // 'Upload my avatar image' => '',
     // 'Remove my image' => '',
+    // 'The OAuth2 state parameter is invalid' => '',
 );
diff --git a/app/Locale/th_TH/translations.php b/app/Locale/th_TH/translations.php
index a81bef73..1e2fb98a 100644
--- a/app/Locale/th_TH/translations.php
+++ b/app/Locale/th_TH/translations.php
@@ -1152,4 +1152,5 @@ return array(
     // 'Avatar' => '',
     // 'Upload my avatar image' => '',
     // 'Remove my image' => '',
+    // 'The OAuth2 state parameter is invalid' => '',
 );
diff --git a/app/Locale/tr_TR/translations.php b/app/Locale/tr_TR/translations.php
index 9a5380d2..6e8fae2f 100644
--- a/app/Locale/tr_TR/translations.php
+++ b/app/Locale/tr_TR/translations.php
@@ -1152,4 +1152,5 @@ return array(
     // 'Avatar' => '',
     // 'Upload my avatar image' => '',
     // 'Remove my image' => '',
+    // 'The OAuth2 state parameter is invalid' => '',
 );
diff --git a/app/Locale/zh_CN/translations.php b/app/Locale/zh_CN/translations.php
index d7e45a89..decd49d8 100644
--- a/app/Locale/zh_CN/translations.php
+++ b/app/Locale/zh_CN/translations.php
@@ -1152,4 +1152,5 @@ return array(
     // 'Avatar' => '',
     // 'Upload my avatar image' => '',
     // 'Remove my image' => '',
+    // 'The OAuth2 state parameter is invalid' => '',
 );
diff --git a/tests/units/Core/Http/OAuth2Test.php b/tests/units/Core/Http/OAuth2Test.php
index c68ae116..5a9c0ac1 100644
--- a/tests/units/Core/Http/OAuth2Test.php
+++ b/tests/units/Core/Http/OAuth2Test.php
@@ -10,7 +10,8 @@ class OAuth2Test extends Base
     {
         $oauth = new OAuth2($this->container);
         $oauth->createService('A', 'B', 'C', 'D', 'E', array('f', 'g'));
-        $this->assertEquals('D?response_type=code&client_id=A&redirect_uri=C&scope=f+g', $oauth->getAuthorizationUrl());
+        $state = $oauth->getState();
+        $this->assertEquals('D?response_type=code&client_id=A&redirect_uri=C&scope=f+g&state='.$state, $oauth->getAuthorizationUrl());
     }
 
     public function testAuthHeader()
@@ -27,12 +28,15 @@ class OAuth2Test extends Base
 
     public function testAccessToken()
     {
+        $oauth = new OAuth2($this->container);
+
         $params = array(
             'code' => 'something',
             'client_id' => 'A',
             'client_secret' => 'B',
             'redirect_uri' => 'C',
             'grant_type' => 'authorization_code',
+            'state' => $oauth->getState(),
         );
 
         $response = json_encode(array(
@@ -46,7 +50,6 @@ class OAuth2Test extends Base
             ->with('E', $params, array('Accept: application/json'))
             ->will($this->returnValue($response));
 
-        $oauth = new OAuth2($this->container);
         $oauth->createService('A', 'B', 'C', 'D', 'E', array('f', 'g'));
         $oauth->getAccessToken('something');
     }