Handle state in OAuth2 client

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

app/Controller/Oauth.php

@@ -2,6 +2,8 @@
 
 namespace Kanboard\Controller;
 
+use Kanboard\Core\Security\OAuthAuthenticationProviderInterface;
+
 /**
  * OAuth controller
  *
@@ -10,6 +12,72 @@ namespace Kanboard\Controller;
  */
 class Oauth extends Base
 {
+    /**
+     * Redirect to the provider if no code received
+     *
+     * @access private
+     * @param string $provider
+     */
+    protected function step1($provider)
+    {
+        $code = $this->request->getStringParam('code');
+        $state = $this->request->getStringParam('state');
+
+        if (! empty($code)) {
+            $this->step2($provider, $code, $state);
+        } else {
+            $this->response->redirect($this->authenticationManager->getProvider($provider)->getService()->getAuthorizationUrl());
+        }
+    }
+
+    /**
+     * Link or authenticate the user
+     *
+     * @access protected
+     * @param string $providerName
+     * @param string $code
+     * @param string $state
+     */
+    protected function step2($providerName, $code, $state)
+    {
+        $provider = $this->authenticationManager->getProvider($providerName);
+        $provider->setCode($code);
+        $hasValidState = $provider->getService()->isValidateState($state);
+
+        if ($this->userSession->isLogged()) {
+            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'));
+            }
+        }
+    }
+
+    /**
+     * Link the account
+     *
+     * @access protected
+     * @param  OAuthAuthenticationProviderInterface $provider
+     */
+    protected function link(OAuthAuthenticationProviderInterface $provider)
+    {
+        if (! $provider->authenticate()) {
+            $this->flash->failure(t('External authentication failed'));
+        } else {
+            $this->userProfile->assign($this->userSession->getId(), $provider->getUser());
+            $this->flash->success(t('Your external account is linked to your profile successfully.'));
+        }
+
+        $this->response->redirect($this->helper->url->to('user', 'external', array('user_id' => $this->userSession->getId())));
+    }
+
     /**
      * Unlink external account
      *
@@ -29,78 +97,34 @@ class Oauth extends Base
         $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
-     * @param string $provider
-     */
-    protected function step1($provider)
-    {
-        $code = $this->request->getStringParam('code');
-
-        if (! empty($code)) {
-            $this->step2($provider, $code);
-        } else {
-            $this->response->redirect($this->authenticationManager->getProvider($provider)->getService()->getAuthorizationUrl());
-        }
-    }
-
-    /**
-     * Link or authenticate the user
-     *
-     * @access protected
-     * @param string $provider
-     * @param string $code
-     */
-    protected function step2($provider, $code)
-    {
-        $this->authenticationManager->getProvider($provider)->setCode($code);
-
-        if ($this->userSession->isLogged()) {
-            $this->link($provider);
-        }
-
-        $this->authenticate($provider);
-    }
-
-    /**
-     * Link the account
-     *
-     * @access protected
-     * @param string $provider
-     */
-    protected function link($provider)
-    {
-        $authProvider = $this->authenticationManager->getProvider($provider);
-
-        if (! $authProvider->authenticate()) {
-            $this->flash->failure(t('External authentication failed'));
-        } else {
-            $this->userProfile->assign($this->userSession->getId(), $authProvider->getUser());
-            $this->flash->success(t('Your external account is linked to your profile successfully.'));
-        }
-
-        $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(
+            $this->authenticationFailure(t('External authentication failed'));
-                'errors' => array('login' => t('External authentication failed')),
-                'values' => array(),
-                'no_layout' => true,
-                'title' => t('Login')
-            )));
         }
     }
+
+    /**
+     * 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')
+        )));
+    }
 }

app/Core/Http/OAuth2.php

@@ -12,14 +12,14 @@ use Kanboard\Core\Base;
  */
 class OAuth2 extends Base
 {
-    private $clientId;
+    protected $clientId;
-    private $secret;
+    protected $secret;
-    private $callbackUrl;
+    protected $callbackUrl;
-    private $authUrl;
+    protected $authUrl;
-    private $tokenUrl;
+    protected $tokenUrl;
-    private $scopes;
+    protected $scopes;
-    private $tokenType;
+    protected $tokenType;
-    private $accessToken;
+    protected $accessToken;
 
     /**
      * Create OAuth2 service
@@ -45,6 +45,33 @@ class OAuth2 extends Base
         return $this;
     }
 
+    /**
+     * 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
      *
@@ -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);

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
 {

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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