diff --git a/.semver b/.semver index d42bf6c44..ae45234e5 100644 --- a/.semver +++ b/.semver @@ -1,5 +1,5 @@ --- :major: 7 :minor: 0 -:patch: 0 +:patch: 2 :special: '' diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ce6ea612..b96bd7d4f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,14 @@ Changelog Releases for CakePHP 3 ------------- +* 7.0.1 + * Fixed an issue with 2FA only working on the second try + +* 7.0.1 + * Fixed a security issue in 2 factor authentication, reported by @ndm2 + * Updated to cakedc/auth ^3.0 + * Documentation fixes + * 7.0.0 * Removed deprecations for CakePHP 3.6 * Added a new `UsersAuthComponent::EVENT_AFTER_CHANGE_PASSWORD` diff --git a/Docs/Documentation/Configuration.md b/Docs/Documentation/Configuration.md index db4f2887f..f52015acb 100644 --- a/Docs/Documentation/Configuration.md +++ b/Docs/Documentation/Configuration.md @@ -120,7 +120,7 @@ NOTE: SOME keys were hidden in this doc page, please refer to `vendor/cakedc/use 'all' => [ 'finder' => 'active', ], - 'CakeDC/Users.RememberMe', + 'CakeDC/Auth.RememberMe', 'Form', ], 'authorize' => [ @@ -140,10 +140,10 @@ Using the UsersAuthComponent default initialization, the component will load the * 'Form' * 'CakeDC/Users.Social' check [SocialAuthenticate](SocialAuthenticate.md) for configuration options * 'CakeDC/Auth.RememberMe' check [RememberMeAuthenticate](https://github.com/CakeDC/auth/blob/master/src/RememberMeAuthenticate.php) for configuration options + * 'CakeDC/Auth.ApiKey' check [ApiKeyAuthenticate](https://github.com/CakeDC/auth/blob/master/Docs/Documentation/ApiKeyAuthenticate.md) for configuration options * Authorize * 'CakeDC/Auth.Superuser' check [SuperuserAuthorize](https://github.com/CakeDC/auth/blob/master/Docs/Documentation/SuperuserAuthorize.md) for configuration options * 'CakeDC/Auth.SimpleRbac' check [SimpleRbacAuthorize](https://github.com/CakeDC/auth/blob/master/Docs/Documentation/SimpleRbacAuthorize.md) for configuration options - * 'CakeDC/Auth.ApiKey' check [ApiKeyAuthenticate](https://github.com/CakeDC/auth/blob/master/Docs/Documentation/ApiKeyAuthenticate.md) for configuration options ## Using the user's email to login diff --git a/composer.json b/composer.json index 93d1a1dbd..618b9dba8 100644 --- a/composer.json +++ b/composer.json @@ -28,7 +28,7 @@ }, "require": { "cakephp/cakephp": "^3.6", - "cakedc/auth": "^2.0" + "cakedc/auth": "^3.0" }, "require-dev": { "phpunit/phpunit": "^5.0", diff --git a/src/Controller/Traits/LinkSocialTrait.php b/src/Controller/Traits/LinkSocialTrait.php index 9b10c960b..ff1b951fd 100644 --- a/src/Controller/Traits/LinkSocialTrait.php +++ b/src/Controller/Traits/LinkSocialTrait.php @@ -41,7 +41,7 @@ public function linkSocial($alias = null) } $authUrl = $provider->getAuthorizationUrl($temporaryCredentials); if (empty($temporaryCredentials)) { - $this->request->session()->write('SocialLink.oauth2state', $provider->getState()); + $this->request->getSession()->write('SocialLink.oauth2state', $provider->getState()); } return $this->redirect($authUrl); @@ -228,8 +228,8 @@ protected function _validateCallbackSocialLink() } $sessionKey = 'SocialLink.oauth2state'; - $oauth2state = $this->request->session()->read($sessionKey); - $this->request->session()->delete($sessionKey); + $oauth2state = $this->request->getSession()->read($sessionKey); + $this->request->getSession()->delete($sessionKey); $state = $queryParams['state']; return $oauth2state === $state; diff --git a/src/Controller/Traits/LoginTrait.php b/src/Controller/Traits/LoginTrait.php index 28c30d588..1a50890d2 100644 --- a/src/Controller/Traits/LoginTrait.php +++ b/src/Controller/Traits/LoginTrait.php @@ -218,20 +218,15 @@ public function verify() return $this->redirect(Configure::read('Auth.loginAction')); } - // storing user's session in the temporary one - // until the GA verification is checked - $temporarySession = $this->Auth->user(); - $this->request->getSession()->delete('Auth.User'); + $temporarySession = $this->request->getSession()->read('temporarySession'); + if (!is_array($temporarySession) || empty($temporarySession)) { + $this->Flash->error(__d('CakeDC/Users', 'Invalid request.'), 'default', [], 'auth'); - if (!empty($temporarySession)) { - $this->request->getSession()->write('temporarySession', $temporarySession); - } - - if (array_key_exists('secret', $temporarySession)) { - $secret = $temporarySession['secret']; + return $this->redirect(Configure::read('Auth.loginAction')); } - $secretVerified = Hash::get((array)$temporarySession, 'secret_verified'); + $secret = Hash::get($temporarySession, 'secret'); + $secretVerified = Hash::get($temporarySession, 'secret_verified'); // showing QR-code until shared secret is verified if (!$secretVerified) { @@ -245,6 +240,8 @@ public function verify() ->set(['secret' => $secret]) ->where(['id' => $temporarySession['id']]); $query->execute(); + + $this->request->getSession()->write('temporarySession.secret', $secret); } catch (\Exception $e) { $this->request->getSession()->destroy(); $message = $e->getMessage(); @@ -278,10 +275,12 @@ public function verify() ->set(['secret_verified' => true]) ->where(['id' => $user['id']]) ->execute(); + + $user['secret_verified'] = true; } $this->request->getSession()->delete('temporarySession'); - $this->request->getSession()->write('Auth.User', $user); + $this->Auth->setUser($user); $url = $this->Auth->redirectUrl(); return $this->redirect($url); @@ -322,14 +321,16 @@ protected function _checkReCaptcha() protected function _afterIdentifyUser($user, $socialLogin = false, $googleAuthenticatorLogin = false) { if (!empty($user)) { - $this->Auth->setUser($user); - if ($googleAuthenticatorLogin) { + // storing user's session in the temporary one + // until the GA verification is checked + $this->request->getSession()->write('temporarySession', $user); $url = Configure::read('GoogleAuthenticator.verifyAction'); return $this->redirect($url); } + $this->Auth->setUser($user); $event = $this->dispatchEvent(UsersAuthComponent::EVENT_AFTER_LOGIN, ['user' => $user]); if (is_array($event->result)) { return $this->redirect($event->result); diff --git a/tests/TestCase/Controller/Component/GoogleAuthenticatorComponentTest.php b/tests/TestCase/Controller/Component/GoogleAuthenticatorComponentTest.php index f708e7f6f..c1e665b55 100644 --- a/tests/TestCase/Controller/Component/GoogleAuthenticatorComponentTest.php +++ b/tests/TestCase/Controller/Component/GoogleAuthenticatorComponentTest.php @@ -44,7 +44,6 @@ public function setUp() $this->backupUsersConfig = Configure::read('Users'); Router::reload(); - Plugin::routes('CakeDC/Users'); Router::connect('/route/*', [ 'plugin' => 'CakeDC/Users', 'controller' => 'Users', diff --git a/tests/TestCase/Controller/Component/UsersAuthComponentTest.php b/tests/TestCase/Controller/Component/UsersAuthComponentTest.php index 922646bda..7860d3439 100644 --- a/tests/TestCase/Controller/Component/UsersAuthComponentTest.php +++ b/tests/TestCase/Controller/Component/UsersAuthComponentTest.php @@ -53,7 +53,6 @@ public function setUp() $this->backupUsersConfig = Configure::read('Users'); Router::reload(); - Plugin::routes('CakeDC/Users'); Router::connect('/route/*', [ 'plugin' => 'CakeDC/Users', 'controller' => 'Users', diff --git a/tests/TestCase/Controller/Traits/LoginTraitTest.php b/tests/TestCase/Controller/Traits/LoginTraitTest.php index 47a3d29c5..fae331d7f 100644 --- a/tests/TestCase/Controller/Traits/LoginTraitTest.php +++ b/tests/TestCase/Controller/Traits/LoginTraitTest.php @@ -391,24 +391,43 @@ public function testFailedSocialUserAccount() public function testVerifyHappy() { Configure::write('Users.GoogleAuthenticator.login', true); - $this->Trait->Auth = $this->getMockBuilder('Cake\Controller\Component\AuthComponent') - ->setMethods(['user', ]) - ->disableOriginalConstructor() - ->getMock(); - $user = [ - 'id' => 1, - 'secret_verified' => 1, - ]; - $this->Trait->Auth->expects($this->at(0)) - ->method('user') - ->will($this->returnValue($user)); + $this->Trait->request = $this->getMockBuilder('Cake\Network\Request') - ->setMethods(['is', 'getData', 'allow']) + ->setMethods(['is', 'getData', 'allow', 'getSession']) ->getMock(); - $this->Trait->request->expects($this->at(0)) + $this->Trait->request->expects($this->once()) ->method('is') ->with('post') ->will($this->returnValue(false)); + + $this->_mockSession([ + 'temporarySession' => [ + 'id' => 1, + 'secret_verified' => 1, + ] + ]); + $this->Trait->verify(); + } + + /** + * testVerifyNoUser + * + */ + public function testVerifyNoUser() + { + Configure::write('Users.GoogleAuthenticator.login', true); + + $this->Trait->request = $this->getMockBuilder('Cake\Network\Request') + ->setMethods(['is', 'getData', 'allow', 'getSession']) + ->getMock(); + $this->Trait->request->expects($this->never()) + ->method('is') + ->with('post'); + $this->_mockSession([]); + $this->_mockFlash(); + $this->Trait->Flash->expects($this->once()) + ->method('error') + ->with('Invalid request.'); $this->Trait->verify(); } @@ -433,30 +452,26 @@ public function testVerifyNotEnabled() public function testVerifyGetShowQR() { Configure::write('Users.GoogleAuthenticator.login', true); - $this->Trait->Auth = $this->getMockBuilder('Cake\Controller\Component\AuthComponent') - ->setMethods(['user', ]) - ->disableOriginalConstructor() - ->getMock(); - $user = [ - 'id' => '00000000-0000-0000-0000-000000000001', - 'email' => 'email@example.com', - 'secret_verified' => 0, - ]; - $this->Trait->Auth->expects($this->at(0)) - ->method('user') - ->will($this->returnValue($user)); + $this->Trait->GoogleAuthenticator = $this->getMockBuilder(GoogleAuthenticatorComponent::class) ->disableOriginalConstructor() ->setMethods(['createSecret', 'getQRCodeImageAsDataUri']) ->getMock(); $this->Trait->request = $this->getMockBuilder(ServerRequest::class) - ->setMethods(['is', 'getData', 'allow']) + ->setMethods(['is', 'getData', 'allow', 'getSession']) ->getMock(); - $this->Trait->request->expects($this->at(0)) + $this->Trait->request->expects($this->once()) ->method('is') ->with('post') ->will($this->returnValue(false)); + $this->_mockSession([ + 'temporarySession' => [ + 'id' => '00000000-0000-0000-0000-000000000001', + 'email' => 'email@example.com', + 'secret_verified' => 0, + ] + ]); $this->Trait->GoogleAuthenticator->expects($this->at(0)) ->method('createSecret') ->will($this->returnValue('newSecret')); @@ -469,4 +484,312 @@ public function testVerifyGetShowQR() ->with(['secretDataUri' => 'newDataUriGenerated']); $this->Trait->verify(); } + + /** + * Tests that a GET request causes a a new secret to be generated in case it's + * not already present in the session. + */ + public function testVerifyGetGeneratesNewSecret() + { + Configure::write('Users.GoogleAuthenticator.login', true); + + $this->Trait->GoogleAuthenticator = $this + ->getMockBuilder(GoogleAuthenticatorComponent::class) + ->disableOriginalConstructor() + ->setMethods(['createSecret', 'getQRCodeImageAsDataUri']) + ->getMock(); + + $this->Trait->request = $this + ->getMockBuilder(ServerRequest::class) + ->setMethods(['is', 'getData', 'allow', 'getSession']) + ->getMock(); + $this->Trait->request + ->expects($this->once()) + ->method('is') + ->with('post') + ->will($this->returnValue(false)); + + $this->Trait->GoogleAuthenticator + ->expects($this->at(0)) + ->method('createSecret') + ->will($this->returnValue('newSecret')); + $this->Trait->GoogleAuthenticator + ->expects($this->at(1)) + ->method('getQRCodeImageAsDataUri') + ->with('email@example.com', 'newSecret') + ->will($this->returnValue('newDataUriGenerated')); + + $session = $this->_mockSession([ + 'temporarySession' => [ + 'id' => '00000000-0000-0000-0000-000000000001', + 'email' => 'email@example.com', + 'secret_verified' => false, + ] + ]); + $this->Trait->verify(); + + $this->assertEquals( + [ + 'temporarySession' => [ + 'id' => '00000000-0000-0000-0000-000000000001', + 'email' => 'email@example.com', + 'secret_verified' => false, + 'secret' => 'newSecret' + ] + ], + $session->read() + ); + } + + /** + * Tests that a GET request does not cause a new secret to be generated in case + * it's already present in the session. + */ + public function testVerifyGetDoesNotGenerateNewSecret() + { + Configure::write('Users.GoogleAuthenticator.login', true); + + $this->Trait->GoogleAuthenticator = $this + ->getMockBuilder(GoogleAuthenticatorComponent::class) + ->disableOriginalConstructor() + ->setMethods(['createSecret', 'getQRCodeImageAsDataUri']) + ->getMock(); + + $this->Trait->request = $this + ->getMockBuilder(ServerRequest::class) + ->setMethods(['is', 'getData', 'allow', 'getSession']) + ->getMock(); + $this->Trait->request + ->expects($this->once()) + ->method('is') + ->with('post') + ->will($this->returnValue(false)); + + $this->Trait->GoogleAuthenticator + ->expects($this->never()) + ->method('createSecret'); + $this->Trait->GoogleAuthenticator + ->expects($this->at(0)) + ->method('getQRCodeImageAsDataUri') + ->with('email@example.com', 'alreadyPresentSecret') + ->will($this->returnValue('newDataUriGenerated')); + + $session = $this->_mockSession([ + 'temporarySession' => [ + 'id' => '00000000-0000-0000-0000-000000000001', + 'email' => 'email@example.com', + 'secret_verified' => false, + 'secret' => 'alreadyPresentSecret' + ] + ]); + $this->Trait->verify(); + + $this->assertEquals( + [ + 'temporarySession' => [ + 'id' => '00000000-0000-0000-0000-000000000001', + 'email' => 'email@example.com', + 'secret_verified' => false, + 'secret' => 'alreadyPresentSecret' + ] + ], + $session->read() + ); + } + + /** + * Tests that posting a valid code causes verification to succeed. + */ + public function testVerifyPostValidCode() + { + Configure::write('Users.GoogleAuthenticator.login', true); + + $this->Trait->GoogleAuthenticator = $this->getMockBuilder(GoogleAuthenticatorComponent::class) + ->disableOriginalConstructor() + ->setMethods(['createSecret', 'verifyCode', 'getQRCodeImageAsDataUri']) + ->getMock(); + + $this->Trait->Auth = $this->getMockBuilder('Cake\Controller\Component\AuthComponent') + ->setMethods(['setUser', 'redirectUrl']) + ->disableOriginalConstructor() + ->getMock(); + + $this->Trait->request = $this->getMockBuilder(ServerRequest::class) + ->setMethods(['is', 'getData', 'allow', 'getSession']) + ->getMock(); + $this->Trait->request->expects($this->once()) + ->method('is') + ->with('post') + ->will($this->returnValue(true)); + $this->Trait->request->expects($this->once()) + ->method('getData') + ->with('code') + ->will($this->returnValue('123456')); + + $this->Trait->GoogleAuthenticator + ->expects($this->never()) + ->method('createSecret'); + $this->Trait->GoogleAuthenticator + ->expects($this->at(0)) + ->method('getQRCodeImageAsDataUri') + ->with('email@example.com', 'yyy') + ->will($this->returnValue('newDataUriGenerated')); + $this->Trait->GoogleAuthenticator + ->expects($this->at(1)) + ->method('verifyCode') + ->with('yyy', '123456') + ->will($this->returnValue(true)); + + $this->Trait->Auth + ->expects($this->at(0)) + ->method('setUser') + ->with([ + 'id' => '00000000-0000-0000-0000-000000000001', + 'email' => 'email@example.com', + 'secret_verified' => true + ]); + $this->Trait->Auth + ->expects($this->at(1)) + ->method('redirectUrl') + ->will($this->returnValue('/')); + + $this->assertFalse($this->table->exists([ + 'id' => '00000000-0000-0000-0000-000000000001', + 'secret_verified' => true + ])); + + $session = $this->_mockSession([ + 'temporarySession' => [ + 'id' => '00000000-0000-0000-0000-000000000001', + 'email' => 'email@example.com', + 'secret_verified' => false, + 'secret' => 'yyy' + ] + ]); + $this->Trait->verify(); + + $this->assertTrue($this->table->exists([ + 'id' => '00000000-0000-0000-0000-000000000001', + 'secret_verified' => true + ])); + + $this->assertEmpty($session->read()); + } + + /** + * Tests that posting and invalid code causes verification to fail. + */ + public function testVerifyPostInvalidCode() + { + Configure::write('Users.GoogleAuthenticator.login', true); + + $this->Trait->GoogleAuthenticator = $this + ->getMockBuilder(GoogleAuthenticatorComponent::class) + ->disableOriginalConstructor() + ->setMethods(['createSecret', 'verifyCode', 'getQRCodeImageAsDataUri']) + ->getMock(); + + $this->Trait->Auth = $this + ->getMockBuilder('Cake\Controller\Component\AuthComponent') + ->setMethods(['setUser']) + ->disableOriginalConstructor() + ->getMock(); + + $this->Trait->Flash = $this + ->getMockBuilder('Cake\Controller\Component\FlashComponent') + ->setMethods(['error']) + ->disableOriginalConstructor() + ->getMock(); + + $this->Trait->request = $this + ->getMockBuilder(ServerRequest::class) + ->setMethods(['is', 'getData', 'allow', 'getSession']) + ->getMock(); + $this->Trait->request + ->expects($this->once()) + ->method('is') + ->with('post') + ->will($this->returnValue(true)); + $this->Trait->request + ->expects($this->once()) + ->method('getData') + ->with('code') + ->will($this->returnValue('invalid')); + + $this->Trait->GoogleAuthenticator + ->expects($this->never()) + ->method('createSecret'); + $this->Trait->GoogleAuthenticator + ->expects($this->at(0)) + ->method('getQRCodeImageAsDataUri') + ->with('email@example.com', 'yyy') + ->will($this->returnValue('newDataUriGenerated')); + $this->Trait->GoogleAuthenticator + ->expects($this->at(1)) + ->method('verifyCode') + ->with('yyy', 'invalid') + ->will($this->returnValue(false)); + + $this->Trait->Auth + ->expects($this->never()) + ->method('setUser'); + + $this->Trait->Flash + ->expects($this->once()) + ->method('error') + ->with('Verification code is invalid. Try again', 'default', [], 'auth'); + + $this->Trait + ->expects($this->once()) + ->method('redirect') + ->with([ + 'plugin' => 'CakeDC/Users', + 'controller' => 'Users', + 'action' => 'login', + 'prefix' => false + ]); + + $this->assertFalse($this->table->exists([ + 'id' => '00000000-0000-0000-0000-000000000001', + 'secret_verified' => true + ])); + + $session = $this->_mockSession([ + 'temporarySession' => [ + 'id' => '00000000-0000-0000-0000-000000000001', + 'email' => 'email@example.com', + 'secret_verified' => false, + 'secret' => 'yyy' + ] + ]); + $this->Trait->verify(); + + $this->assertFalse($this->table->exists([ + 'id' => '00000000-0000-0000-0000-000000000001', + 'secret_verified' => true + ])); + + $this->assertEmpty($session->read()); + } + + /** + * Mock session and mock session attributes + * + * @return \Cake\Http\Session + */ + protected function _mockSession($attributes) + { + $session = new \Cake\Http\Session(); + + foreach ($attributes as $field => $value) { + $session->write($field, $value); + } + + $this->Trait->request + ->expects($this->any()) + ->method('getSession') + ->willReturn($session); + + return $session; + } } diff --git a/tests/TestCase/Controller/Traits/RegisterTraitTest.php b/tests/TestCase/Controller/Traits/RegisterTraitTest.php index ec2a005d9..6050b9bda 100644 --- a/tests/TestCase/Controller/Traits/RegisterTraitTest.php +++ b/tests/TestCase/Controller/Traits/RegisterTraitTest.php @@ -31,8 +31,6 @@ public function setUp() $this->traitMockMethods = ['validate', 'dispatchEvent', 'set', 'validateReCaptcha', 'redirect']; $this->mockDefaultEmail = true; parent::setUp(); - - Plugin::routes('CakeDC/Users'); } /** diff --git a/tests/TestCase/Model/Table/UsersTableTest.php b/tests/TestCase/Model/Table/UsersTableTest.php index dcad61b07..03b91b37e 100644 --- a/tests/TestCase/Model/Table/UsersTableTest.php +++ b/tests/TestCase/Model/Table/UsersTableTest.php @@ -56,7 +56,6 @@ public function setUp() 'transport' => 'test', 'from' => 'cakedc@example.com' ]); - Plugin::routes('CakeDC/Users'); } /** diff --git a/tests/TestCase/View/Helper/UserHelperTest.php b/tests/TestCase/View/Helper/UserHelperTest.php index e20b30e77..f0bf717f0 100644 --- a/tests/TestCase/View/Helper/UserHelperTest.php +++ b/tests/TestCase/View/Helper/UserHelperTest.php @@ -57,7 +57,6 @@ public function setUp() } parent::setUp(); - Plugin::routes('CakeDC/Users'); $this->View = $this->getMockBuilder('Cake\View\View') ->setMethods(['append']) ->getMock();