From 9b85908d4648fe13d5225f01b487d192208a644b Mon Sep 17 00:00:00 2001 From: ndm2 Date: Wed, 13 Jun 2018 21:51:55 +0200 Subject: [PATCH 1/3] 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 2/3] 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 3/3] 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 *