diff --git a/ChangeLog b/ChangeLog index 5fdbf6e37..5fded0d43 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 452faecd1..12b911445 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 * @@ -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( - '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 6fa1fb0ad..211ca5b4e 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 @@ -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); diff --git a/app/Core/Session/SessionStorage.php b/app/Core/Session/SessionStorage.php index 667d9253d..6e2f96603 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 8d653d4ff..7ca864f48 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 3606eddf5..b2921de96 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 cf3f01912..c4743922e 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 1090d6c9c..af88b3743 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 04efa7e77..9a31e4850 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 477f36557..c36233692 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 a32082e3a..8e5dd81f5 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 00e64876b..cedd60390 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 f2e1cafbc..f642a6c1f 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 8d279633c..3f105054c 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 873274620..93ceb03fb 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 aa8cc6548..b48eabd8f 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 be41c19c3..36b3db0b7 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 0e214cf48..465efb532 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 dc68eb34e..3c3fa1ee0 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 0d020dcb6..d06e347f8 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 ebed08cdd..050d1a9f5 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 4d2d20b42..1c3278877 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 1d93b3f3a..3cb3c6bbc 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 634f6f8c5..c7070a8df 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 4dcc63ad2..e4728d2d0 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 a81bef73c..1e2fb98ae 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 9a5380d23..6e8fae2f8 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 d7e45a895..decd49d8e 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 c68ae116a..5a9c0ac16 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'); }