From 3cf87b4f5aa40659e1f9886857bbf40a6f284ec9 Mon Sep 17 00:00:00 2001 From: Marcelo Rocha Date: Fri, 8 Jun 2018 15:56:08 -0300 Subject: [PATCH 1/4] 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 2/4] 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 3/4] 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 4/4] 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) {