From 544caced7966fc90859a4c56f7b60a076e23214f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20M=2E=20Gonz=C3=A1lez=20Mart=C3=ADn?= Date: Thu, 26 Apr 2018 13:03:35 +0100 Subject: [PATCH 01/17] fix docs fixes #570 thanks to @makamo for reporting --- Docs/Documentation/Configuration.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Docs/Documentation/Configuration.md b/Docs/Documentation/Configuration.md index db4f2887f..e4db903d1 100644 --- a/Docs/Documentation/Configuration.md +++ b/Docs/Documentation/Configuration.md @@ -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 From 30ea0afb6697ff6f27b48a060222ff91be5b31b1 Mon Sep 17 00:00:00 2001 From: Andrej Griniuk Date: Mon, 30 Apr 2018 21:26:11 +1000 Subject: [PATCH 02/17] CakeDC/Users.RememberMe -> CakeDC/Auth.RememberMe --- Docs/Documentation/Configuration.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Docs/Documentation/Configuration.md b/Docs/Documentation/Configuration.md index e4db903d1..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' => [ From 3cf87b4f5aa40659e1f9886857bbf40a6f284ec9 Mon Sep 17 00:00:00 2001 From: Marcelo Rocha Date: Fri, 8 Jun 2018 15:56:08 -0300 Subject: [PATCH 03/17] Save user in temp session for Two factor authentication --- src/Controller/Traits/LoginTrait.php | 14 ++-- .../Controller/Traits/LoginTraitTest.php | 66 +++++++++++-------- 2 files changed, 44 insertions(+), 36 deletions(-) diff --git a/src/Controller/Traits/LoginTrait.php b/src/Controller/Traits/LoginTrait.php index 28c30d588..8ac463207 100644 --- a/src/Controller/Traits/LoginTrait.php +++ b/src/Controller/Traits/LoginTrait.php @@ -218,14 +218,7 @@ 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'); - - if (!empty($temporarySession)) { - $this->request->getSession()->write('temporarySession', $temporarySession); - } + $temporarySession = $this->request->getSession()->read('temporarySession'); if (array_key_exists('secret', $temporarySession)) { $secret = $temporarySession['secret']; @@ -322,14 +315,17 @@ 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/Traits/LoginTraitTest.php b/tests/TestCase/Controller/Traits/LoginTraitTest.php index 47a3d29c5..26e739d38 100644 --- a/tests/TestCase/Controller/Traits/LoginTraitTest.php +++ b/tests/TestCase/Controller/Traits/LoginTraitTest.php @@ -391,24 +391,21 @@ 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(); } @@ -433,30 +430,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 +462,23 @@ public function testVerifyGetShowQR() ->with(['secretDataUri' => 'newDataUriGenerated']); $this->Trait->verify(); } + + /** + * Mock session and mock session attributes + * + * @return void + */ + 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); + } } From 03c6fb277d28ba43c4dd51bb6ad82f118338c52f Mon Sep 17 00:00:00 2001 From: Marcelo Rocha Date: Fri, 8 Jun 2018 16:10:48 -0300 Subject: [PATCH 04/17] Save user in temp session for Two factor authentication --- src/Controller/Traits/LoginTrait.php | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Controller/Traits/LoginTrait.php b/src/Controller/Traits/LoginTrait.php index 8ac463207..7c3d516af 100644 --- a/src/Controller/Traits/LoginTrait.php +++ b/src/Controller/Traits/LoginTrait.php @@ -315,7 +315,6 @@ protected function _checkReCaptcha() protected function _afterIdentifyUser($user, $socialLogin = false, $googleAuthenticatorLogin = false) { if (!empty($user)) { - if ($googleAuthenticatorLogin) { // storing user's session in the temporary one // until the GA verification is checked From c56d6fc1dcedf6327f0a211982021892908c1463 Mon Sep 17 00:00:00 2001 From: Marcelo Rocha Date: Fri, 8 Jun 2018 16:21:43 -0300 Subject: [PATCH 05/17] Two factor authentication verify should work when temporary session is present --- src/Controller/Traits/LoginTrait.php | 5 +++++ .../Controller/Traits/LoginTraitTest.php | 22 +++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/src/Controller/Traits/LoginTrait.php b/src/Controller/Traits/LoginTrait.php index 7c3d516af..796f561a9 100644 --- a/src/Controller/Traits/LoginTrait.php +++ b/src/Controller/Traits/LoginTrait.php @@ -219,6 +219,11 @@ public function verify() } $temporarySession = $this->request->getSession()->read('temporarySession'); + if (empty($temporarySession)) { + $this->Flash->error(__d('CakeDC/Users', 'Invalid request.'), 'default', [], 'auth'); + + return $this->redirect(Configure::read('Auth.loginAction')); + } if (array_key_exists('secret', $temporarySession)) { $secret = $temporarySession['secret']; diff --git a/tests/TestCase/Controller/Traits/LoginTraitTest.php b/tests/TestCase/Controller/Traits/LoginTraitTest.php index 26e739d38..81571fc5e 100644 --- a/tests/TestCase/Controller/Traits/LoginTraitTest.php +++ b/tests/TestCase/Controller/Traits/LoginTraitTest.php @@ -409,6 +409,28 @@ public function testVerifyHappy() $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(); + } + /** * testVerifyHappy * From c17ac3a0a29263b3aedfc8c7429c763fe794c310 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20M=2E=20Gonz=C3=A1lez=20Mart=C3=ADn?= Date: Mon, 11 Jun 2018 10:00:35 +0100 Subject: [PATCH 06/17] check for array when extracting temporarySession --- src/Controller/Traits/LoginTrait.php | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/Controller/Traits/LoginTrait.php b/src/Controller/Traits/LoginTrait.php index 796f561a9..471bc4b39 100644 --- a/src/Controller/Traits/LoginTrait.php +++ b/src/Controller/Traits/LoginTrait.php @@ -219,17 +219,14 @@ public function verify() } $temporarySession = $this->request->getSession()->read('temporarySession'); - if (empty($temporarySession)) { + if (!is_array($temporarySession) || empty($temporarySession)) { $this->Flash->error(__d('CakeDC/Users', 'Invalid request.'), 'default', [], 'auth'); return $this->redirect(Configure::read('Auth.loginAction')); } - if (array_key_exists('secret', $temporarySession)) { - $secret = $temporarySession['secret']; - } - - $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) { From d218b9a0a0070eed960ec89650aba5d91da21c00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20Gonz=C3=A1lez?= Date: Mon, 11 Jun 2018 11:07:07 +0100 Subject: [PATCH 07/17] refs #rochamarcelo-google-two-factor-auth fix deprecations --- .../Controller/Component/GoogleAuthenticatorComponentTest.php | 1 - tests/TestCase/Controller/Component/UsersAuthComponentTest.php | 1 - tests/TestCase/Controller/Traits/RegisterTraitTest.php | 2 -- tests/TestCase/Model/Table/UsersTableTest.php | 1 - tests/TestCase/View/Helper/UserHelperTest.php | 1 - 5 files changed, 6 deletions(-) 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/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(); From efe42973cd1d17780fd986301f2c8c40db38429e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20M=2E=20Gonz=C3=A1lez=20Mart=C3=ADn?= Date: Wed, 13 Jun 2018 14:07:49 +0100 Subject: [PATCH 08/17] Update .semver --- .semver | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.semver b/.semver index d42bf6c44..0a8899195 100644 --- a/.semver +++ b/.semver @@ -1,5 +1,5 @@ --- :major: 7 :minor: 0 -:patch: 0 +:patch: 1 :special: '' From aba4fd1806fa6900990c478e0080bdac2517aa38 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20Gonz=C3=A1lez?= Date: Wed, 13 Jun 2018 14:11:43 +0100 Subject: [PATCH 09/17] refs #692 update cakedc/auth --- composer.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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", From de13b7efd2ffa43dd16487d01927353dcc9bf391 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20M=2E=20Gonz=C3=A1lez=20Mart=C3=ADn?= Date: Wed, 13 Jun 2018 14:16:58 +0100 Subject: [PATCH 10/17] Update CHANGELOG.md --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ce6ea612..be1b47fca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,11 @@ Changelog Releases for CakePHP 3 ------------- +* 7.0.1 + * Fixed a security issue in 2 factor authentication, reported by @ndm + * 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` From b1e5dee784e81989a0edd7d8f23db22bf00ebfd5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20M=2E=20Gonz=C3=A1lez=20Mart=C3=ADn?= Date: Wed, 13 Jun 2018 14:21:22 +0100 Subject: [PATCH 11/17] fix typo --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index be1b47fca..05bfe9836 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ Releases for CakePHP 3 ------------- * 7.0.1 - * Fixed a security issue in 2 factor authentication, reported by @ndm + * Fixed a security issue in 2 factor authentication, reported by @ndm2 * Updated to cakedc/auth ^3.0 * Documentation fixes From 9b85908d4648fe13d5225f01b487d192208a644b Mon Sep 17 00:00:00 2001 From: ndm2 Date: Wed, 13 Jun 2018 21:51:55 +0200 Subject: [PATCH 12/17] Fix secrets being regenerated on every request. This patch ensures that a new secret is only generated in case the user record doesn't already has a secret set. --- src/Controller/Traits/LoginTrait.php | 2 + .../Controller/Traits/LoginTraitTest.php | 116 +++++++++++++++++- 2 files changed, 117 insertions(+), 1 deletion(-) diff --git a/src/Controller/Traits/LoginTrait.php b/src/Controller/Traits/LoginTrait.php index 471bc4b39..3e6fa32f0 100644 --- a/src/Controller/Traits/LoginTrait.php +++ b/src/Controller/Traits/LoginTrait.php @@ -240,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(); diff --git a/tests/TestCase/Controller/Traits/LoginTraitTest.php b/tests/TestCase/Controller/Traits/LoginTraitTest.php index 81571fc5e..1f1f24901 100644 --- a/tests/TestCase/Controller/Traits/LoginTraitTest.php +++ b/tests/TestCase/Controller/Traits/LoginTraitTest.php @@ -485,10 +485,122 @@ public function testVerifyGetShowQR() $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() + ); + } + /** * Mock session and mock session attributes * - * @return void + * @return \Cake\Http\Session */ protected function _mockSession($attributes) { @@ -502,5 +614,7 @@ protected function _mockSession($attributes) ->expects($this->any()) ->method('getSession') ->willReturn($session); + + return $session; } } From 0f26b675fde34bce05278614c817daafc2dc018c Mon Sep 17 00:00:00 2001 From: ndm2 Date: Wed, 13 Jun 2018 21:56:44 +0200 Subject: [PATCH 13/17] Use the Auth component to store user data. Ensures that the user data is persisted in the storage configured for the Auth component, and that the `secret_verified` flag is set properly. --- src/Controller/Traits/LoginTrait.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Controller/Traits/LoginTrait.php b/src/Controller/Traits/LoginTrait.php index 3e6fa32f0..1a50890d2 100644 --- a/src/Controller/Traits/LoginTrait.php +++ b/src/Controller/Traits/LoginTrait.php @@ -275,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); From c2f04910d55ad3f98c2f2af9d93f49b484d5a042 Mon Sep 17 00:00:00 2001 From: ndm2 Date: Wed, 13 Jun 2018 22:01:56 +0200 Subject: [PATCH 14/17] Add two-step auth code verification tests. --- .../Controller/Traits/LoginTraitTest.php | 175 ++++++++++++++++++ 1 file changed, 175 insertions(+) diff --git a/tests/TestCase/Controller/Traits/LoginTraitTest.php b/tests/TestCase/Controller/Traits/LoginTraitTest.php index 1f1f24901..fae331d7f 100644 --- a/tests/TestCase/Controller/Traits/LoginTraitTest.php +++ b/tests/TestCase/Controller/Traits/LoginTraitTest.php @@ -597,6 +597,181 @@ public function testVerifyGetDoesNotGenerateNewSecret() ); } + /** + * 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 * From 526d066320a915c7e64817392ad02335e2e8d3a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20M=2E=20Gonz=C3=A1lez=20Mart=C3=ADn?= Date: Wed, 20 Jun 2018 09:48:55 +0100 Subject: [PATCH 15/17] Update .semver --- .semver | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.semver b/.semver index 0a8899195..ae45234e5 100644 --- a/.semver +++ b/.semver @@ -1,5 +1,5 @@ --- :major: 7 :minor: 0 -:patch: 1 +:patch: 2 :special: '' From 2fc40f28c051950a3c9b44a26a2d3b837d954045 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20M=2E=20Gonz=C3=A1lez=20Mart=C3=ADn?= Date: Wed, 20 Jun 2018 09:49:28 +0100 Subject: [PATCH 16/17] Update CHANGELOG.md --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 05bfe9836..b96bd7d4f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,9 @@ 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 From 342d981e66097311d95c8619d69fe8b770a73992 Mon Sep 17 00:00:00 2001 From: Lartak Date: Sat, 28 Jul 2018 21:54:33 +0200 Subject: [PATCH 17/17] Change the old syntax of `$this->request->session()` to `$this->request->getSession()`. The new syntax. --- src/Controller/Traits/LinkSocialTrait.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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;