From a9256b4b30aae49ab4a708c9a300f6f6ed4044c6 Mon Sep 17 00:00:00 2001 From: Andrey Vystavkin Date: Thu, 22 Dec 2016 16:18:25 +0200 Subject: [PATCH 1/7] Adding Reset for Google Authenticator Token --- config/routes.php | 1 - src/Controller/Traits/LoginTrait.php | 6 +- .../Traits/PasswordManagementTrait.php | 50 ++++++++++++++ src/Template/Users/edit.ctp | 16 +++++ tests/Fixture/UsersFixture.php | 2 + .../Traits/PasswordManagementTraitTest.php | 65 +++++++++++++++++++ 6 files changed, 137 insertions(+), 3 deletions(-) diff --git a/config/routes.php b/config/routes.php index 97a7aa111..389f53290 100644 --- a/config/routes.php +++ b/config/routes.php @@ -29,4 +29,3 @@ Router::connect('/login', ['plugin' => 'CakeDC/Users', 'controller' => 'Users', 'action' => 'login']); Router::connect('/logout', ['plugin' => 'CakeDC/Users', 'controller' => 'Users', 'action' => 'logout']); Router::connect('/verify', ['plugin' => 'CakeDC/Users', 'controller' => 'Users', 'action' => 'verify']); - diff --git a/src/Controller/Traits/LoginTrait.php b/src/Controller/Traits/LoginTrait.php index a39e58e4e..57842b7ac 100644 --- a/src/Controller/Traits/LoginTrait.php +++ b/src/Controller/Traits/LoginTrait.php @@ -238,11 +238,13 @@ public function verify() } if ($this->request->is('post')) { + $codeVerified = false; $verificationCode = $this->request->data('code'); $user = $this->request->session()->read('temporarySession'); + $entity = $this->getUsersTable()->get($user['id']); - if (array_key_exists('secret', $user)) { - $codeVerified = $this->GoogleAuthenticator->verifyCode($user['secret'], $verificationCode); + if (!empty($entity['secret'])) { + $codeVerified = $this->GoogleAuthenticator->verifyCode($entity['secret'], $verificationCode); } if ($codeVerified) { diff --git a/src/Controller/Traits/PasswordManagementTrait.php b/src/Controller/Traits/PasswordManagementTrait.php index baa28ad93..0d8a37c9f 100644 --- a/src/Controller/Traits/PasswordManagementTrait.php +++ b/src/Controller/Traits/PasswordManagementTrait.php @@ -133,4 +133,54 @@ public function requestResetPassword() $this->Flash->error(__d('CakeDC/Users', 'Token could not be reset')); } } + + /** + * resetGoogleAuthenticator + * + * Resets Google Authenticator token by setting secret_verified + * to false. + * Currently allowed to be done either by superuser or user himself. + * + * @param mixed $id of the user record. + * @return mixed. + */ + public function resetGoogleAuthenticator($id = null) + { + $allowed = false; + $currentUser = !empty($this->Auth->user()) ? $this->Auth->user() : false; + + if (!$currentUser) { + $message = __d('CakeDC/User', 'Please login to the system'); + $this->Flash->error($message, 'auth'); + + return $this->redirect($this->Auth->config('loginAction')); + } + + if (true == $currentUser['is_superuser'] || $id == $currentUser['id']) { + $allowed = true; + } + + if (!$allowed) { + $message = __d('CakeDC/Users', 'You are not allowed to reset users Google Authenticator token'); + $this->Flash->error($message, 'default'); + } + + if ($this->request->is('post') && $allowed) { + try { + $query = $this->getUsersTable()->query(); + $query->update() + ->set(['secret_verified' => false, 'secret' => null]) + ->where(['id' => $id]); + $executed = $query->execute(); + + $message = __d('CakeDC/Users', 'Google Authenticator token was successfully reset'); + $this->Flash->success($message, 'default'); + } catch (\Exception $e) { + $message = __d('CakeDC/Users', $e->getMessage()); + $this->Flash->error($message, 'default'); + } + } + + return $this->redirect($this->request->referer()); + } } diff --git a/src/Template/Users/edit.ctp b/src/Template/Users/edit.ctp index b789d73c5..4b9891c23 100644 --- a/src/Template/Users/edit.ctp +++ b/src/Template/Users/edit.ctp @@ -8,6 +8,7 @@ * @copyright Copyright 2010 - 2015, Cake Development Corporation (http://cakedc.com) * @license MIT License (http://www.opensource.org/licenses/mit-license.php) */ +use Cake\Core\Configure; $Users = ${$tableAlias}; ?> @@ -55,4 +56,19 @@ $Users = ${$tableAlias}; Form->button(__d('CakeDC/Users', 'Submit')) ?> Form->end() ?> + +
+ Reset Google Authenticator + Form->postLink( + __d('CakeDC/Users', 'Reset Google Authenticator Token'), [ + 'plugin' => 'CakeDC/Users', + 'controller' => 'Users', + 'action' => '/resetGoogleAuthenticator', $Users->id + ], [ + 'class' => 'btn btn-danger', + 'confirm' => __d('CakeDC/Users', 'Are you sure you want to reset token for user "{0}"?', $Users->username) + ]); + ?> +
+ diff --git a/tests/Fixture/UsersFixture.php b/tests/Fixture/UsersFixture.php index bcbc85458..ee6e4aa7b 100644 --- a/tests/Fixture/UsersFixture.php +++ b/tests/Fixture/UsersFixture.php @@ -113,6 +113,7 @@ class UsersFixture extends TestFixture 'api_token' => 'xxx', 'activation_date' => '2015-06-24 17:33:54', 'secret' => 'xxx', + 'secret_verified' => true, 'is_superuser' => true, 'tos_date' => '2015-06-24 17:33:54', 'active' => false, @@ -133,6 +134,7 @@ class UsersFixture extends TestFixture 'api_token' => 'Lorem ipsum dolor sit amet', 'activation_date' => '2015-06-24 17:33:54', 'secret' => 'Lorem ipsum dolor sit amet', + 'secret_verified' => true, 'is_superuser' => false, 'tos_date' => '2015-06-24 17:33:54', 'active' => true, diff --git a/tests/TestCase/Controller/Traits/PasswordManagementTraitTest.php b/tests/TestCase/Controller/Traits/PasswordManagementTraitTest.php index b681227be..e5d6a86c1 100644 --- a/tests/TestCase/Controller/Traits/PasswordManagementTraitTest.php +++ b/tests/TestCase/Controller/Traits/PasswordManagementTraitTest.php @@ -383,4 +383,69 @@ public function ensureUserActiveForResetPasswordFeature() [$defaultBehavior] ]; } + + public function testRequestGoogleAuthTokenResetWithoutSession() + { + $this->_mockRequestGet(true); + $this->_mockAuth(); + $this->_mockFlash(); + + $this->Trait->Auth->expects($this->any()) + ->method('user') + ->will($this->returnValue(null)); + + $this->Trait->Flash->expects($this->once()) + ->method('error') + ->with('Please login to the system'); + $this->Trait->resetGoogleAuthenticator(); + } + + /** + * @dataProvider ensureGoogleAuthenticatorResets + * + * @return void + */ + public function testRequestGoogleAuthTokenResetWithValidUser($userId, $entityId, $method, $msg) + { + $this->_mockRequestPost(); + $this->_mockFlash(); + + $user = $this->table->get($userId); + + $this->Trait->Auth = $this->getMockBuilder('Cake\Controller\Component\AuthComponent') + ->setMethods(['user', 'config']) + ->disableOriginalConstructor() + ->getMock(); + + $this->Trait->Auth->expects($this->any()) + ->method('user') + ->will($this->returnValue($user)); + + $this->Trait->Flash->expects($this->any()) + ->method($method) + ->with($msg); + + $this->Trait->resetGoogleAuthenticator($entityId); + } + + public function ensureGoogleAuthenticatorResets() + { + $error = 'error'; + $success = 'success'; + $errorMsg = 'You are not allowed to reset users Google Authenticator token'; + $successMsg = 'Google Authenticator token was successfully reset'; + + return [ + //is_superuser = true. + ['00000000-0000-0000-0000-000000000003', null, $success, $successMsg], + //is_superuser = true. + ['00000000-0000-0000-0000-000000000001', null, $success, $successMsg], + //is_superuser = false, and not his profile. + ['00000000-0000-0000-0000-000000000004', '00000000-0000-0000-0000-000000000001', $error, $errorMsg], + //is_superuser = false, editing own record. + ['00000000-0000-0000-0000-000000000004', '00000000-0000-0000-0000-000000000004', $success, $successMsg], + //is_superuser = false, and no entity-id given. + ['00000000-0000-0000-0000-000000000004', null, $error, $errorMsg], + ]; + } } From 52a28fe764c425addce88641c229da9a833a7acd Mon Sep 17 00:00:00 2001 From: Andrey Vystavkin Date: Mon, 9 Jan 2017 14:32:17 +0200 Subject: [PATCH 2/7] Removed checks for permissions and added minor fixes on urls --- config/routes.php | 13 ++++++++++++- src/Controller/Traits/PasswordManagementTrait.php | 12 +----------- src/Template/Users/edit.ctp | 2 +- 3 files changed, 14 insertions(+), 13 deletions(-) diff --git a/config/routes.php b/config/routes.php index 389f53290..4968e02fc 100644 --- a/config/routes.php +++ b/config/routes.php @@ -8,6 +8,7 @@ * @copyright Copyright 2010 - 2015, Cake Development Corporation (http://cakedc.com) * @license MIT License (http://www.opensource.org/licenses/mit-license.php) */ +use Cake\Core\Configure; use Cake\Routing\Router; Router::plugin('CakeDC/Users', ['path' => '/users'], function ($routes) { @@ -25,7 +26,17 @@ 'controller' => 'SocialAccounts', 'action' => 'validate' ]); +// Google Authenticator related routes +if (Configure::read('Users.GoogleAuthenticator.login')) { + Router::connect('/verify', ['plugin' => 'CakeDC/Users', 'controller' => 'Users', 'action' => 'verify']); + + Router::connect('/resetGoogleAuthenticator', [ + 'plugin' => 'CakeDC/Users', + 'controller' => 'Users', + 'action' => 'resetGoogleAuthenticator' + ]); +} + Router::connect('/profile/*', ['plugin' => 'CakeDC/Users', 'controller' => 'Users', 'action' => 'profile']); Router::connect('/login', ['plugin' => 'CakeDC/Users', 'controller' => 'Users', 'action' => 'login']); Router::connect('/logout', ['plugin' => 'CakeDC/Users', 'controller' => 'Users', 'action' => 'logout']); -Router::connect('/verify', ['plugin' => 'CakeDC/Users', 'controller' => 'Users', 'action' => 'verify']); diff --git a/src/Controller/Traits/PasswordManagementTrait.php b/src/Controller/Traits/PasswordManagementTrait.php index 0d8a37c9f..f6318da99 100644 --- a/src/Controller/Traits/PasswordManagementTrait.php +++ b/src/Controller/Traits/PasswordManagementTrait.php @@ -146,7 +146,6 @@ public function requestResetPassword() */ public function resetGoogleAuthenticator($id = null) { - $allowed = false; $currentUser = !empty($this->Auth->user()) ? $this->Auth->user() : false; if (!$currentUser) { @@ -156,16 +155,7 @@ public function resetGoogleAuthenticator($id = null) return $this->redirect($this->Auth->config('loginAction')); } - if (true == $currentUser['is_superuser'] || $id == $currentUser['id']) { - $allowed = true; - } - - if (!$allowed) { - $message = __d('CakeDC/Users', 'You are not allowed to reset users Google Authenticator token'); - $this->Flash->error($message, 'default'); - } - - if ($this->request->is('post') && $allowed) { + if ($this->request->is('post')) { try { $query = $this->getUsersTable()->query(); $query->update() diff --git a/src/Template/Users/edit.ctp b/src/Template/Users/edit.ctp index 4b9891c23..e85c53d36 100644 --- a/src/Template/Users/edit.ctp +++ b/src/Template/Users/edit.ctp @@ -63,7 +63,7 @@ $Users = ${$tableAlias}; __d('CakeDC/Users', 'Reset Google Authenticator Token'), [ 'plugin' => 'CakeDC/Users', 'controller' => 'Users', - 'action' => '/resetGoogleAuthenticator', $Users->id + 'action' => 'resetGoogleAuthenticator', $Users->id ], [ 'class' => 'btn btn-danger', 'confirm' => __d('CakeDC/Users', 'Are you sure you want to reset token for user "{0}"?', $Users->username) From ffe93368b9dd4ed5f2e48419cc52e02cba5761d9 Mon Sep 17 00:00:00 2001 From: Andrey Vystavkin Date: Mon, 9 Jan 2017 14:48:06 +0200 Subject: [PATCH 3/7] not logged in user will be redirected to login automagically --- src/Controller/Traits/PasswordManagementTrait.php | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/src/Controller/Traits/PasswordManagementTrait.php b/src/Controller/Traits/PasswordManagementTrait.php index f6318da99..450862c10 100644 --- a/src/Controller/Traits/PasswordManagementTrait.php +++ b/src/Controller/Traits/PasswordManagementTrait.php @@ -139,22 +139,12 @@ public function requestResetPassword() * * Resets Google Authenticator token by setting secret_verified * to false. - * Currently allowed to be done either by superuser or user himself. * * @param mixed $id of the user record. * @return mixed. */ public function resetGoogleAuthenticator($id = null) { - $currentUser = !empty($this->Auth->user()) ? $this->Auth->user() : false; - - if (!$currentUser) { - $message = __d('CakeDC/User', 'Please login to the system'); - $this->Flash->error($message, 'auth'); - - return $this->redirect($this->Auth->config('loginAction')); - } - if ($this->request->is('post')) { try { $query = $this->getUsersTable()->query(); From 6e0e0f2a4375f5b8481163e1de741642ed747041 Mon Sep 17 00:00:00 2001 From: Andrey Vystavkin Date: Mon, 9 Jan 2017 17:45:57 +0200 Subject: [PATCH 4/7] Adding permissions check for the Owners Token reset --- config/permissions.php | 15 +++++++++++++++ .../Traits/PasswordManagementTraitTest.php | 16 ---------------- 2 files changed, 15 insertions(+), 16 deletions(-) diff --git a/config/permissions.php b/config/permissions.php index 0f8451259..ee6b207c1 100644 --- a/config/permissions.php +++ b/config/permissions.php @@ -63,6 +63,21 @@ 'controller' => 'Users', 'action' => ['register', 'edit', 'view'], ], + [ + 'role' => '*', + 'plugin' => 'CakeDC/Users', + 'controller' => 'Users', + 'action' => 'resetGoogleAuthenticator', + 'allowed' => function (array $user, $role, \Cake\Network\Request $request) { + $userId = \Cake\Utility\Hash::get($request->params, 'pass.0'); + $userRecord = \Cake\ORM\TableRegistry::get('Users')->get($userId); + if (!empty($userId) && !empty($userRecord)) { + return $userRecord->id === $user['id']; + } + + return false; + } + ], [ 'role' => 'user', 'plugin' => 'CakeDC/Users', diff --git a/tests/TestCase/Controller/Traits/PasswordManagementTraitTest.php b/tests/TestCase/Controller/Traits/PasswordManagementTraitTest.php index e5d6a86c1..681c4ef33 100644 --- a/tests/TestCase/Controller/Traits/PasswordManagementTraitTest.php +++ b/tests/TestCase/Controller/Traits/PasswordManagementTraitTest.php @@ -384,22 +384,6 @@ public function ensureUserActiveForResetPasswordFeature() ]; } - public function testRequestGoogleAuthTokenResetWithoutSession() - { - $this->_mockRequestGet(true); - $this->_mockAuth(); - $this->_mockFlash(); - - $this->Trait->Auth->expects($this->any()) - ->method('user') - ->will($this->returnValue(null)); - - $this->Trait->Flash->expects($this->once()) - ->method('error') - ->with('Please login to the system'); - $this->Trait->resetGoogleAuthenticator(); - } - /** * @dataProvider ensureGoogleAuthenticatorResets * From 56e0d0f4df39b467ca1f9b19d051b3134b3017b1 Mon Sep 17 00:00:00 2001 From: Andrey Vystavkin Date: Mon, 9 Jan 2017 18:40:22 +0200 Subject: [PATCH 5/7] Removing unnecessary user checks on the user record ownership --- config/permissions.php | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/config/permissions.php b/config/permissions.php index ee6b207c1..a0525fa5b 100644 --- a/config/permissions.php +++ b/config/permissions.php @@ -70,9 +70,8 @@ 'action' => 'resetGoogleAuthenticator', 'allowed' => function (array $user, $role, \Cake\Network\Request $request) { $userId = \Cake\Utility\Hash::get($request->params, 'pass.0'); - $userRecord = \Cake\ORM\TableRegistry::get('Users')->get($userId); - if (!empty($userId) && !empty($userRecord)) { - return $userRecord->id === $user['id']; + if (!empty($userId) && !empty($user)) { + return $userId === $user['id']; } return false; From 7c40d80c525eee16d81cfec12f7a2e233dd510e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20M=2E=20Gonz=C3=A1lez=20Mart=C3=ADn?= Date: Mon, 9 Jan 2017 16:59:13 +0000 Subject: [PATCH 6/7] Update .semver --- .semver | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.semver b/.semver index 2b479677e..0845a9533 100644 --- a/.semver +++ b/.semver @@ -1,5 +1,5 @@ --- :major: 4 -:minor: 0 +:minor: 1 :patch: 0 :special: '' From bca6c370d5663419f25ceb50300eb78a93b7390f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20M=2E=20Gonz=C3=A1lez=20Mart=C3=ADn?= Date: Mon, 9 Jan 2017 16:59:57 +0000 Subject: [PATCH 7/7] Update CHANGELOG.md --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index cb47b90f1..97c024cef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,9 @@ Changelog Releases for CakePHP 3 ------------- +* 4.1.0 + * Add reset action for Google Authenticator + * 4.0.0 * Add Google Authenticator * Add improvements to SimpleRbac, like star to invert rules and `user.` prefix to match values from the user array