From 383fde6da3453b50cb9445dbde6ef57a9782dbe0 Mon Sep 17 00:00:00 2001 From: Xymanek Date: Mon, 8 Aug 2016 08:07:17 +0300 Subject: [PATCH 01/29] Improvements to SimpleRbac --- src/Auth/SimpleRbacAuthorize.php | 121 ++++++++++++++++++++----------- 1 file changed, 79 insertions(+), 42 deletions(-) diff --git a/src/Auth/SimpleRbacAuthorize.php b/src/Auth/SimpleRbacAuthorize.php index d7e022e9f..f7a00f036 100644 --- a/src/Auth/SimpleRbacAuthorize.php +++ b/src/Auth/SimpleRbacAuthorize.php @@ -196,69 +196,106 @@ protected function _checkRules(array $user, $role, Request $request) /** * Match the rule for current permission * - * @param array $permission configuration - * @param array $user current user - * @param string $role effective user role - * @param Request $request request - * @return bool if rule matched, null if rule not matched + * @param array $permission The permission configuration + * @param array $user Current user data + * @param string $role Effective user's role + * @param \Cake\Network\Request $request Current request + * + * @return bool */ - protected function _matchRule($permission, $user, $role, $request) + protected function _matchRule (array $permission, array $user, $role, Request $request) { - $plugin = $request->plugin; - $controller = $request->controller; - $action = $request->action; - $prefix = null; - $extension = null; - if (!empty($request->params['prefix'])) { - $prefix = $request->params['prefix']; - } - if (!empty($request->params['_ext'])) { - $extension = $request->params['_ext']; + if (!isset($permission['controller'], $permission['action'])) { + $this->log( + __d('CakeDC/Users', "Cannot evaluate permission when 'controller' and/or 'action' keys are absent"), + LogLevel::DEBUG + ); + + return false; } - if ($this->_matchOrAsterisk($permission, 'role', $role) && - $this->_matchOrAsterisk($permission, 'prefix', $prefix, true) && - $this->_matchOrAsterisk($permission, 'plugin', $plugin, true) && - $this->_matchOrAsterisk($permission, 'extension', $extension, true) && - $this->_matchOrAsterisk($permission, 'controller', $controller) && - $this->_matchOrAsterisk($permission, 'action', $action)) { - $allowed = Hash::get($permission, 'allowed'); - if ($allowed === null) { - //allowed will be true by default - return true; - } elseif (is_callable($allowed)) { - return (bool)call_user_func($allowed, $user, $role, $request); - } elseif ($allowed instanceof Rule) { - return (bool)$allowed->allowed($user, $role, $request); + $permission += ['allowed' => true]; + $user_arr = ['user' => $user]; + $reserved = [ + 'prefix' => $request->params['prefix'], + 'plugin' => $request->plugin, + 'extension' => $request->params['_ext'], + 'controller' => $request->controller, + 'action' => $request->action, + ]; + + foreach ($permission as $key => $value) { + $inverse = $this->_startsWith($key, '*'); + if ($inverse) { + $key = ltrim($key, '*'); + } + + if (is_callable($value)) { + $return = (bool) call_user_func($value, $user, $role, $request); + } elseif ($value instanceof Rule) { + $return = (bool) $value->allowed($user, $role, $request); + } elseif ($key === 'allowed') { + $return = (bool) $value; + } elseif (array_key_exists($key, $reserved)) { + $return = $this->_matchOrAsterisk($value, $reserved[$key], true); } else { - return (bool)$allowed; + if (!$this->_startsWith($key, 'user')) { + $key = 'user.' . $key; + } + + $return = $this->_matchOrAsterisk($value, Hash::get($user_arr, $key)); + } + + if ($inverse) { + $return = !$return; + } + if (!$return) { + return false; } } - return null; + return true; } /** * Check if rule matched or '*' present in rule matching anything * - * @param string $permission permission configuration - * @param string $key key to retrieve and check in permissions configuration - * @param string $value value to check with (coming from the request) We'll check the DASHERIZED value too - * @param bool $allowEmpty true if we allow + * @param string|array $possibleValues Values that are accepted (from permission config) + * @param string|mixed|null $value Value to check with (coming from the request) We'll check the + * DASHERIZED value too + * @param bool $allowEmpty If true and $value is null, the rule will pass + * * @return bool */ - protected function _matchOrAsterisk($permission, $key, $value, $allowEmpty = false) + protected function _matchOrAsterisk ($possibleValues, $value, $allowEmpty = false) { - $possibleValues = (array)Hash::get($permission, $key); - if ($allowEmpty && empty($possibleValues) && $value === null) { + $possibleArray = (array) $possibleValues; + + if ($allowEmpty && empty($possibleArray) && $value === null) { return true; } - if (Hash::get($permission, $key) === '*' || - in_array($value, $possibleValues) || - in_array(Inflector::camelize($value, '-'), $possibleValues)) { + + if ( + $possibleValues === '*' || + in_array($value, $possibleArray) || + in_array(Inflector::camelize($value, '-'), $possibleArray) + ) { return true; } return false; } + + /** + * Checks if $heystack begins with $needle + * @see http://stackoverflow.com/a/7168986/2588539 + * + * @param string $haystack The whole string + * @param string $needle The beginning to check + * + * @return bool + */ + protected function _startsWith($haystack, $needle) { + return substr($haystack, 0, strlen($needle)) === $needle; + } } From 43b95ed0d6e179375be73da46467d8e854941211 Mon Sep 17 00:00:00 2001 From: Xymanek Date: Mon, 8 Aug 2016 09:59:22 +0300 Subject: [PATCH 02/29] (Should) fix failing tests --- src/Auth/SimpleRbacAuthorize.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Auth/SimpleRbacAuthorize.php b/src/Auth/SimpleRbacAuthorize.php index f7a00f036..c28dcaa56 100644 --- a/src/Auth/SimpleRbacAuthorize.php +++ b/src/Auth/SimpleRbacAuthorize.php @@ -217,9 +217,9 @@ protected function _matchRule (array $permission, array $user, $role, Request $r $permission += ['allowed' => true]; $user_arr = ['user' => $user]; $reserved = [ - 'prefix' => $request->params['prefix'], + 'prefix' => isset($request->params['prefix']) ? $request->params['prefix'] : null, 'plugin' => $request->plugin, - 'extension' => $request->params['_ext'], + 'extension' => isset($request->params['_ext']) ? $request->params['_ext'] : null, 'controller' => $request->controller, 'action' => $request->action, ]; From 13470b22595407ca17c99843081850c1a45874e6 Mon Sep 17 00:00:00 2001 From: Xymanek Date: Tue, 9 Aug 2016 15:39:39 +0300 Subject: [PATCH 03/29] Fixed CS errors as well as added `role` to reserved keys list --- src/Auth/SimpleRbacAuthorize.php | 40 +++++++++++++++++--------------- 1 file changed, 21 insertions(+), 19 deletions(-) diff --git a/src/Auth/SimpleRbacAuthorize.php b/src/Auth/SimpleRbacAuthorize.php index c28dcaa56..7929f241e 100644 --- a/src/Auth/SimpleRbacAuthorize.php +++ b/src/Auth/SimpleRbacAuthorize.php @@ -196,14 +196,14 @@ protected function _checkRules(array $user, $role, Request $request) /** * Match the rule for current permission * - * @param array $permission The permission configuration - * @param array $user Current user data - * @param string $role Effective user's role - * @param \Cake\Network\Request $request Current request + * @param array $permission The permission configuration + * @param array $user Current user data + * @param string $role Effective user's role + * @param \Cake\Network\Request $request Current request * * @return bool */ - protected function _matchRule (array $permission, array $user, $role, Request $request) + protected function _matchRule(array $permission, array $user, $role, Request $request) { if (!isset($permission['controller'], $permission['action'])) { $this->log( @@ -215,13 +215,14 @@ protected function _matchRule (array $permission, array $user, $role, Request $r } $permission += ['allowed' => true]; - $user_arr = ['user' => $user]; + $userArr = ['user' => $user]; $reserved = [ - 'prefix' => isset($request->params['prefix']) ? $request->params['prefix'] : null, + 'prefix' => Hash::get($request->params, 'prefix'), 'plugin' => $request->plugin, - 'extension' => isset($request->params['_ext']) ? $request->params['_ext'] : null, + 'extension' => Hash::get($request->params, '_ext'), 'controller' => $request->controller, 'action' => $request->action, + 'role' => $role ]; foreach ($permission as $key => $value) { @@ -231,11 +232,11 @@ protected function _matchRule (array $permission, array $user, $role, Request $r } if (is_callable($value)) { - $return = (bool) call_user_func($value, $user, $role, $request); + $return = (bool)call_user_func($value, $user, $role, $request); } elseif ($value instanceof Rule) { - $return = (bool) $value->allowed($user, $role, $request); + $return = (bool)$value->allowed($user, $role, $request); } elseif ($key === 'allowed') { - $return = (bool) $value; + $return = (bool)$value; } elseif (array_key_exists($key, $reserved)) { $return = $this->_matchOrAsterisk($value, $reserved[$key], true); } else { @@ -243,7 +244,7 @@ protected function _matchRule (array $permission, array $user, $role, Request $r $key = 'user.' . $key; } - $return = $this->_matchOrAsterisk($value, Hash::get($user_arr, $key)); + $return = $this->_matchOrAsterisk($value, Hash::get($userArr, $key)); } if ($inverse) { @@ -260,16 +261,15 @@ protected function _matchRule (array $permission, array $user, $role, Request $r /** * Check if rule matched or '*' present in rule matching anything * - * @param string|array $possibleValues Values that are accepted (from permission config) - * @param string|mixed|null $value Value to check with (coming from the request) We'll check the - * DASHERIZED value too - * @param bool $allowEmpty If true and $value is null, the rule will pass + * @param string|array $possibleValues Values that are accepted (from permission config) + * @param string|mixed|null $value Value to check with. We'll check the DASHERIZED value too + * @param bool $allowEmpty If true and $value is null, the rule will pass * * @return bool */ - protected function _matchOrAsterisk ($possibleValues, $value, $allowEmpty = false) + protected function _matchOrAsterisk($possibleValues, $value, $allowEmpty = false) { - $possibleArray = (array) $possibleValues; + $possibleArray = (array)$possibleValues; if ($allowEmpty && empty($possibleArray) && $value === null) { return true; @@ -288,6 +288,7 @@ protected function _matchOrAsterisk ($possibleValues, $value, $allowEmpty = fals /** * Checks if $heystack begins with $needle + * * @see http://stackoverflow.com/a/7168986/2588539 * * @param string $haystack The whole string @@ -295,7 +296,8 @@ protected function _matchOrAsterisk ($possibleValues, $value, $allowEmpty = fals * * @return bool */ - protected function _startsWith($haystack, $needle) { + protected function _startsWith($haystack, $needle) + { return substr($haystack, 0, strlen($needle)) === $needle; } } From cbef4be424460909e901f0528fd6e8835206691f Mon Sep 17 00:00:00 2001 From: Xymanek Date: Sat, 13 Aug 2016 13:21:41 +0300 Subject: [PATCH 04/29] Fixed some erroneous behavior in SimpleRbac --- src/Auth/SimpleRbacAuthorize.php | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/Auth/SimpleRbacAuthorize.php b/src/Auth/SimpleRbacAuthorize.php index 7929f241e..9397c8f79 100644 --- a/src/Auth/SimpleRbacAuthorize.php +++ b/src/Auth/SimpleRbacAuthorize.php @@ -205,7 +205,11 @@ protected function _checkRules(array $user, $role, Request $request) */ protected function _matchRule(array $permission, array $user, $role, Request $request) { - if (!isset($permission['controller'], $permission['action'])) { + $issetController = isset($permission['controller']) || isset($permission['*controller']); + $issetAction = isset($permission['action']) || isset($permission['*action']); + $issetUser = isset($permission['user']) || isset($permission['*user']); + + if (!$issetController || !$issetAction) { $this->log( __d('CakeDC/Users', "Cannot evaluate permission when 'controller' and/or 'action' keys are absent"), LogLevel::DEBUG @@ -213,6 +217,14 @@ protected function _matchRule(array $permission, array $user, $role, Request $re return false; } + if ($issetUser) { + $this->log( + __d('CakeDC/Users', "Permission key 'user' is illegal, cannot evaluate the permission"), + LogLevel::DEBUG + ); + + return false; + } $permission += ['allowed' => true]; $userArr = ['user' => $user]; @@ -240,7 +252,7 @@ protected function _matchRule(array $permission, array $user, $role, Request $re } elseif (array_key_exists($key, $reserved)) { $return = $this->_matchOrAsterisk($value, $reserved[$key], true); } else { - if (!$this->_startsWith($key, 'user')) { + if (!$this->_startsWith($key, 'user.')) { $key = 'user.' . $key; } From 6ad5d1b7287e131158e742a1e9b94302c152f876 Mon Sep 17 00:00:00 2001 From: Xymanek Date: Sat, 13 Aug 2016 13:23:43 +0300 Subject: [PATCH 05/29] Added tests, fixed some old ones, removed unused import --- .../TestCase/Auth/SimpleRbacAuthorizeTest.php | 377 +++++++++++++++--- 1 file changed, 329 insertions(+), 48 deletions(-) diff --git a/tests/TestCase/Auth/SimpleRbacAuthorizeTest.php b/tests/TestCase/Auth/SimpleRbacAuthorizeTest.php index 205064230..b36c25725 100644 --- a/tests/TestCase/Auth/SimpleRbacAuthorizeTest.php +++ b/tests/TestCase/Auth/SimpleRbacAuthorizeTest.php @@ -14,12 +14,11 @@ use CakeDC\Users\Auth\Rules\Rule; use CakeDC\Users\Auth\SimpleRbacAuthorize; use Cake\Controller\ComponentRegistry; -use Cake\Controller\Controller; -use Cake\Event\EventManager; use Cake\Network\Request; use Cake\Network\Response; use Cake\TestSuite\TestCase; use Cake\Utility\Hash; +use Psr\Log\LogLevel; use ReflectionClass; class SimpleRbacAuthorizeTest extends TestCase @@ -171,19 +170,7 @@ protected function preparePermissions($permissions) public function testAuthorize($permissions, $user, $requestParams, $expected, $msg = null) { $this->simpleRbacAuthorize = $this->preparePermissions($permissions); - $request = new Request(); - $request->plugin = Hash::get($requestParams, 'plugin'); - $request->controller = $requestParams['controller']; - $request->action = $requestParams['action']; - $prefix = Hash::get($requestParams, 'prefix'); - $request->params = []; - if ($prefix) { - $request->params['prefix'] = $prefix; - } - $extension = Hash::get($requestParams, '_ext'); - if ($extension) { - $request->params['_ext'] = $extension; - } + $request = $this->_requestFromArray($requestParams); $result = $this->simpleRbacAuthorize->authorize($user, $request); $this->assertSame($expected, $result, $msg); @@ -199,6 +186,139 @@ public function providerAuthorize() ->willReturn(true); return [ + 'star-invert' => [ + //permissions + [[ + '*plugin' => 'Tests', + '*role' => 'test', + '*controller' => 'Tests', + '*action' => 'test', + ]], + //user + [ + 'id' => 1, + 'username' => 'luke', + 'role' => 'something', + ], + //request + [ + 'plugin' => 'something', + 'controller' => 'something', + 'action' => 'something' + ], + //expected + true + ], + 'star-invert-deny' => [ + //permissions + [[ + '*plugin' => 'Tests', + '*role' => 'test', + '*controller' => 'Tests', + '*action' => 'test', + ]], + //user + [ + 'id' => 1, + 'username' => 'luke', + 'role' => 'something', + ], + //request + [ + 'plugin' => 'something', + 'controller' => 'something', + 'action' => 'test' + ], + //expected + false + ], + 'user-arr' => [ + //permissions + [ + [ + 'username' => 'luke', + 'user.id' => 1, + 'profile.id' => 256, + 'user.profile.signature' => "Hi I'm luke", + 'user.allowed' => false, + 'controller' => 'Tests', + 'action' => 'one' + ], + ], + //user + [ + 'id' => 1, + 'username' => 'luke', + 'role' => 'test', + 'profile' => [ + 'id' => 256, + 'signature' => "Hi I'm luke" + ], + 'allowed' => false + ], + //request + [ + 'controller' => 'Tests', + 'action' => 'one' + ], + //expected + true + ], + 'evaluate-order' => [ + //permissions + [ + [ + 'allowed' => false, + function () { + throw new \Exception(); + }, + 'controller' => 'Tests', + 'action' => 'one' + ], + ], + //user + [ + 'id' => 1, + 'username' => 'luke', + 'role' => 'test', + ], + //request + [ + 'controller' => 'Tests', + 'action' => 'one' + ], + //expected + false + ], + 'multiple-callables' => [ + //permissions + [ + [ + function () { + return true; + }, + clone $trueRuleMock, + function () { + return true; + }, + 'controller' => 'Tests', + 'action' => 'one' + ], + ], + //user + [ + 'id' => 1, + 'username' => 'luke', + 'role' => 'test', + ], + //request + [ + 'controller' => 'Tests', + 'action' => 'one' + ], + //expected + true + ], 'happy-strict-all' => [ //permissions [[ @@ -501,7 +621,7 @@ public function providerAuthorize() //expected true ], - 'happy-array' => [ + 'happy-array-deny' => [ //permissions [[ 'plugin' => ['Tests'], @@ -667,23 +787,15 @@ public function providerAuthorize() //expected true ], - 'array-prefix' => [ + 'array-prefix-deny' => [ //permissions - [ - [ - 'role' => ['test'], - 'prefix' => ['one', 'admin'], - 'controller' => '*', - 'action' => 'one', - 'allowed' => false, - ], - [ - 'role' => ['test'], - 'prefix' => ['one', 'admin'], - 'controller' => '*', - 'action' => '*', - ], - ], + [[ + 'role' => ['test'], + 'prefix' => ['one', 'admin'], + 'controller' => '*', + 'action' => 'one', + 'allowed' => false, + ],], //user [ 'id' => 1, @@ -796,23 +908,15 @@ public function providerAuthorize() //expected true ], - 'array-ext' => [ + 'array-ext-deny' => [ //permissions - [ - [ - 'role' => ['test'], - 'extension' => ['csv', 'docx'], - 'controller' => '*', - 'action' => 'one', - 'allowed' => false, - ], - [ - 'role' => ['test'], - 'extension' => ['csv', 'docx'], - 'controller' => '*', - 'action' => '*', - ], - ], + [[ + 'role' => ['test'], + 'extension' => ['csv', 'docx'], + 'controller' => '*', + 'action' => 'one', + 'allowed' => false, + ]], //user [ 'id' => 1, @@ -854,4 +958,181 @@ public function providerAuthorize() ], ]; } + + /** + * @dataProvider badPermissionProvider + * + * @param array $permissions + * @param array $user + * @param array $requestParams + * @param string $expectedMsg + */ + public function testBadPermission($permissions, $user, $requestParams, $expectedMsg) + { + /** @var \PHPUnit_Framework_MockObject_MockObject|SimpleRbacAuthorize $simpleRbacAuthorize */ + $simpleRbacAuthorize = $this->getMockBuilder(SimpleRbacAuthorize::class) + ->setMethods(['_loadPermissions', 'log']) + ->disableOriginalConstructor() + ->getMock(); + $simpleRbacAuthorize + ->expects($this->once()) + ->method('log') + ->with($expectedMsg, LogLevel::DEBUG); + + $simpleRbacAuthorize->config('permissions', $permissions); + $request = $this->_requestFromArray($requestParams); + + $simpleRbacAuthorize->authorize($user, $request); + } + + public function badPermissionProvider() + { + return [ + 'no-controller' => [ + //permissions + [[ + 'plugin' => 'Tests', + 'role' => 'test', + //'controller' => 'Tests', + 'action' => 'test', + 'allowed' => true, + ]], + //user + [ + 'id' => 1, + 'username' => 'luke', + 'role' => 'test', + ], + //request + [ + 'plugin' => 'Tests', + 'controller' => 'Tests', + 'action' => 'test' + ], + //expected + __d('CakeDC/Users', "Cannot evaluate permission when 'controller' and/or 'action' keys are absent"), + ], + 'no-action' => [ + //permissions + [[ + 'plugin' => 'Tests', + 'role' => 'test', + 'controller' => 'Tests', + //'action' => 'test', + 'allowed' => true, + ]], + //user + [ + 'id' => 1, + 'username' => 'luke', + 'role' => 'test', + ], + //request + [ + 'plugin' => 'Tests', + 'controller' => 'Tests', + 'action' => 'test' + ], + //expected + __d('CakeDC/Users', "Cannot evaluate permission when 'controller' and/or 'action' keys are absent"), + ], + 'no-controller-and-action' => [ + //permissions + [[ + 'plugin' => 'Tests', + 'role' => 'test', + //'controller' => 'Tests', + //'action' => 'test', + 'allowed' => true, + ]], + //user + [ + 'id' => 1, + 'username' => 'luke', + 'role' => 'test', + ], + //request + [ + 'plugin' => 'Tests', + 'controller' => 'Tests', + 'action' => 'test' + ], + //expected + __d('CakeDC/Users', "Cannot evaluate permission when 'controller' and/or 'action' keys are absent"), + ], + 'no-controller and user-key' => [ + //permissions + [[ + 'plugin' => 'Tests', + 'role' => 'test', + //'controller' => 'Tests', + 'action' => 'test', + 'allowed' => true, + 'user' => 'something', + ]], + //user + [ + 'id' => 1, + 'username' => 'luke', + 'role' => 'test', + ], + //request + [ + 'plugin' => 'Tests', + 'controller' => 'Tests', + 'action' => 'test' + ], + //expected + __d('CakeDC/Users', "Cannot evaluate permission when 'controller' and/or 'action' keys are absent"), + ], + 'user-key' => [ + //permissions + [[ + 'plugin' => 'Tests', + 'role' => 'test', + 'controller' => 'Tests', + 'action' => 'test', + 'allowed' => true, + 'user' => 'something', + ]], + //user + [ + 'id' => 1, + 'username' => 'luke', + 'role' => 'test', + ], + //request + [ + 'plugin' => 'Tests', + 'controller' => 'Tests', + 'action' => 'test' + ], + //expected + __d('CakeDC/Users', "Permission key 'user' is illegal, cannot evaluate the permission"), + ], + ]; + } + + /** + * @param array $params + * @return \Cake\Network\Request + */ + protected function _requestFromArray ($params) + { + $request = new Request(); + $request->plugin = Hash::get($params, 'plugin'); + $request->controller = $params['controller']; + $request->action = $params['action']; + $prefix = Hash::get($params, 'prefix'); + $request->params = []; + if ($prefix) { + $request->params['prefix'] = $prefix; + } + $extension = Hash::get($params, '_ext'); + if ($extension) { + $request->params['_ext'] = $extension; + } + + return $request; + } } From d4e7ae85e6d394199b308e35d13c74f704dd1955 Mon Sep 17 00:00:00 2001 From: Xymanek Date: Wed, 24 Aug 2016 15:55:13 +0300 Subject: [PATCH 06/29] CS fix --- src/Auth/SimpleRbacAuthorize.php | 3 +-- tests/TestCase/Auth/SimpleRbacAuthorizeTest.php | 5 ++--- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/Auth/SimpleRbacAuthorize.php b/src/Auth/SimpleRbacAuthorize.php index 9397c8f79..769dcd3a4 100644 --- a/src/Auth/SimpleRbacAuthorize.php +++ b/src/Auth/SimpleRbacAuthorize.php @@ -287,8 +287,7 @@ protected function _matchOrAsterisk($possibleValues, $value, $allowEmpty = false return true; } - if ( - $possibleValues === '*' || + if ($possibleValues === '*' || in_array($value, $possibleArray) || in_array(Inflector::camelize($value, '-'), $possibleArray) ) { diff --git a/tests/TestCase/Auth/SimpleRbacAuthorizeTest.php b/tests/TestCase/Auth/SimpleRbacAuthorizeTest.php index b36c25725..7b19e0abf 100644 --- a/tests/TestCase/Auth/SimpleRbacAuthorizeTest.php +++ b/tests/TestCase/Auth/SimpleRbacAuthorizeTest.php @@ -795,7 +795,7 @@ function () { 'controller' => '*', 'action' => 'one', 'allowed' => false, - ],], + ]], //user [ 'id' => 1, @@ -969,7 +969,6 @@ function () { */ public function testBadPermission($permissions, $user, $requestParams, $expectedMsg) { - /** @var \PHPUnit_Framework_MockObject_MockObject|SimpleRbacAuthorize $simpleRbacAuthorize */ $simpleRbacAuthorize = $this->getMockBuilder(SimpleRbacAuthorize::class) ->setMethods(['_loadPermissions', 'log']) ->disableOriginalConstructor() @@ -1117,7 +1116,7 @@ public function badPermissionProvider() * @param array $params * @return \Cake\Network\Request */ - protected function _requestFromArray ($params) + protected function _requestFromArray($params) { $request = new Request(); $request->plugin = Hash::get($params, 'plugin'); From 34924f0ae8e01680a7ef6fbc469a12be9e78bcc0 Mon Sep 17 00:00:00 2001 From: Xymanek Date: Sun, 4 Sep 2016 13:28:51 +0300 Subject: [PATCH 07/29] Fixed small bug, updated SimpleRbac documentation, added more tests and updated imports --- Docs/Documentation/SimpleRbacAuthorize.md | 95 ++++++++++----- src/Auth/Rules/AbstractRule.php | 9 +- src/Auth/Rules/Rule.php | 1 - src/Auth/SimpleRbacAuthorize.php | 19 +-- .../TestCase/Auth/SimpleRbacAuthorizeTest.php | 108 +++++++++++++++--- 5 files changed, 175 insertions(+), 57 deletions(-) diff --git a/Docs/Documentation/SimpleRbacAuthorize.md b/Docs/Documentation/SimpleRbacAuthorize.md index fbcda06e1..8a45eac61 100644 --- a/Docs/Documentation/SimpleRbacAuthorize.md +++ b/Docs/Documentation/SimpleRbacAuthorize.md @@ -52,25 +52,42 @@ The ```default_role``` will be used to set the role of the registered users by d Permission rules syntax ----------------- -* Rules are evaluated top-down, first matching rule will apply -* Each rule is defined: -```php -[ - 'role' => 'REQUIRED_NAME_OF_THE_ROLE_OR_[]_OR_*', - 'prefix' => 'OPTIONAL_PREFIX_USED_OR_[]_OR_*_DEFAULT_NULL', - 'extension' => 'OPTIONAL_PREFIX_USED_OR_[]_OR_*_DEFAULT_NULL', - 'plugin' => 'OPTIONAL_NAME_OF_THE_PLUGIN_OR_[]_OR_*_DEFAULT_NULL', - 'controller' => 'REQUIRED_NAME_OF_THE_CONTROLLER_OR_[]_OR_*' - 'action' => 'REQUIRED_NAME_OF_ACTION_OR_[]_OR_*', - 'allowed' => 'OPTIONAL_BOOLEAN_OR_CALLABLE_OR_INSTANCE_OF_RULE_DEFAULT_TRUE' -] -``` -* If no rule allowed = true is matched for a given user role and url, default return value will be false -* Note for Superadmin access (permission to access ALL THE THINGS in your app) there is a specific Authorize Object provided +* Permissions are evaluated top-down, first matching permission will apply +* Each permission is an associative array of rules with following structure: `'value_to_check' => 'expected_value'` +* `value_to_check` can be any key from user array or one of special keys: + * Routing params: + * `prefix` + * `plugin` + * `extension` + * `controller` + * `action` + * `role` - Alias/shortcut to field defined in `role_field` config value + * `allowed` - see below +* If you have a user field that overlaps with special keys (eg. `$user->allowed`) you can prepend `user.` to key to force matching from user array (eg. `user.allowed`) +* The keys can be placed in any order with exception of `allowed` which must be last one (see below) +* `value_to_check` can be prepended with `*` to match everything except `expected_value` +* `expected_value` can be one of following things: + * `*` will match absolutely everything + * A _string_/_integer_/_boolean_/etc - will match only the specified value + * An _array_ of strings/integers/booleans/etc (can be mixed). The rule will match if real value is equal to any of expected ones + * A callable/object (see below) +* If any of rules fail, the permission is discarded and the next one is evaluated +* A very special key `allowed` exists which has completely different behaviour: + * If `expected_value` is a callable/object then it's executed and the result is casted to boolean + * If `expected_value` is **not** a callable/object then it's simply casted to boolean + * The `*` is checked and if found the result is inverted + * The final boolean value is **the result of permission** checker. This means if it is `false` then no other permissions are checked and the user is denied access. + For this reason the `allowed` key must be placed at the end of permission since no other rules are executed after it -Permission Callbacks ------------------ -You could use a callback in your 'allowed' to process complex authentication, like +**Notes**: + +* For Superadmin access (permission to access ALL THE THINGS in your app) there is a specific Authorize Object provided +* Permissions that do not have `controller` and/or `action` keys (or the inverted versions) are automatically discarded in order to prevent errors. +If you need to match all controllers/actions you can explicitly do `'contoller' => '*'` +* Key `user` (or the inverted version) is illegal (as it's impossible to match an array) and any permission containing it will be discarded +* If the permission is discarded for the reasons stated above, a debug message will be logged + +**Permission Callbacks**: you could use a callback in your 'allowed' to process complex authentication, like - ownership - permissions stored in your database - permission based on an external service API call @@ -78,10 +95,10 @@ You could use a callback in your 'allowed' to process complex authentication, li Example *ownership* callback, to allow users to edit their own Posts: ```php - 'allowed' => function (array $user, $role, Request $request) { - $postId = Hash::get($request->params, 'pass.0'); - $post = TableRegistry::get('Posts')->get($postId); - $userId = Hash::get($user, 'id'); + 'allowed' => function (array $user, $role, \Cake\Network\Request $request) { + $postId = \Cake\Utility\Hash::get($request->params, 'pass.0'); + $post = \Cake\ORM\TableRegistry::get('Posts')->get($postId); + $userId = $user['id']; if (!empty($post->user_id) && !empty($userId)) { return $post->user_id === $userId; } @@ -89,12 +106,36 @@ Example *ownership* callback, to allow users to edit their own Posts: } ``` -Permission Rules ----------------- -You could use an instance of the \CakeDC\Users\Auth\Rules\Rule interface to reuse your custom rules -Examples: +**Permission Rules**: If you see that you are duplicating logic in your callables, you can create rule class to re-use the logic. +For example, the above ownership callback is included in CakeDC\Users as `Owner` rule ```php -'allowed' => new Owner() //will pick by default the post id from the first pass param +'allowed' => new \CakeDC\Users\Auth\Rules\Owner() //will pick by default the post id from the first pass param ``` Check the [Owner Rule](OwnerRule.md) documentation for more details +Creating rule classes +--------------------- + +The only requirement is to implement `\CakeDC\Users\Auth\Rules\Rule` interface which has one method: + +```php +class YourRule implements \CakeDC\Users\Auth\Rules\Rule +{ + /** + * Check the current entity is owned by the logged in user + * + * @param array $user Auth array with the logged in data + * @param string $role role of the user + * @param Request $request current request, used to get a default table if not provided + * @return bool + */ + public function allowed(array $user, $role, Request $request) + { + // Your logic here + } +} +``` + +This logic can be anything: database, external auth, etc. + +Also, if you are using DB, you can choose to extend `\CakeDC\Users\Auth\Rules\AbstractRule` since it provides convenience methods for reading from DB \ No newline at end of file diff --git a/src/Auth/Rules/AbstractRule.php b/src/Auth/Rules/AbstractRule.php index 17dacc8a5..eac837ea4 100644 --- a/src/Auth/Rules/AbstractRule.php +++ b/src/Auth/Rules/AbstractRule.php @@ -10,7 +10,10 @@ */ namespace CakeDC\Users\Auth\Rules; +use Cake\Core\InstanceConfigTrait; +use Cake\Datasource\ModelAwareTrait; use Cake\Network\Request; +use Cake\ORM\Locator\LocatorAwareTrait; use Cake\ORM\Table; use Cake\ORM\TableRegistry; use Cake\Utility\Hash; @@ -22,9 +25,9 @@ */ abstract class AbstractRule implements Rule { - use \Cake\Core\InstanceConfigTrait; - use \Cake\Datasource\ModelAwareTrait; - use \Cake\ORM\Locator\LocatorAwareTrait; + use InstanceConfigTrait; + use ModelAwareTrait; + use LocatorAwareTrait; /** * @var array default config diff --git a/src/Auth/Rules/Rule.php b/src/Auth/Rules/Rule.php index 7ba6c438d..9acd999d3 100644 --- a/src/Auth/Rules/Rule.php +++ b/src/Auth/Rules/Rule.php @@ -21,7 +21,6 @@ interface Rule * @param string $role role of the user * @param Request $request current request, used to get a default table if not provided * @return bool - * @throws \OutOfBoundsException if table is not found or it doesn't have the expected fields */ public function allowed(array $user, $role, Request $request); } diff --git a/src/Auth/SimpleRbacAuthorize.php b/src/Auth/SimpleRbacAuthorize.php index 769dcd3a4..120bdeaa7 100644 --- a/src/Auth/SimpleRbacAuthorize.php +++ b/src/Auth/SimpleRbacAuthorize.php @@ -120,7 +120,7 @@ public function __construct(ComponentRegistry $registry, array $config = []) parent::__construct($registry, $config); $autoload = $this->config('autoload_config'); if ($autoload) { - $loadedPermissions = $this->_loadPermissions($autoload, 'default'); + $loadedPermissions = $this->_loadPermissions($autoload); $this->config('permissions', $loadedPermissions); } } @@ -166,7 +166,7 @@ public function authorize($user, Request $request) $role = Hash::get($user, $roleField); } - $allowed = $this->_checkRules($user, $role, $request); + $allowed = $this->_checkPermissions($user, $role, $request); return $allowed; } @@ -180,11 +180,11 @@ public function authorize($user, Request $request) * @param Request $request request * @return bool true if there is a match in permissions */ - protected function _checkRules(array $user, $role, Request $request) + protected function _checkPermissions(array $user, $role, Request $request) { $permissions = $this->config('permissions'); foreach ($permissions as $permission) { - $allowed = $this->_matchRule($permission, $user, $role, $request); + $allowed = $this->_matchPermission($permission, $user, $role, $request); if ($allowed !== null) { return $allowed; } @@ -201,9 +201,9 @@ protected function _checkRules(array $user, $role, Request $request) * @param string $role Effective user's role * @param \Cake\Network\Request $request Current request * - * @return bool + * @return null|bool Null if permission is discarded, boolean if a final result is produced */ - protected function _matchRule(array $permission, array $user, $role, Request $request) + protected function _matchPermission(array $permission, array $user, $role, Request $request) { $issetController = isset($permission['controller']) || isset($permission['*controller']); $issetAction = isset($permission['action']) || isset($permission['*action']); @@ -262,12 +262,15 @@ protected function _matchRule(array $permission, array $user, $role, Request $re if ($inverse) { $return = !$return; } + if ($key === 'allowed') { + return $return; + } if (!$return) { - return false; + break; } } - return true; + return null; } /** diff --git a/tests/TestCase/Auth/SimpleRbacAuthorizeTest.php b/tests/TestCase/Auth/SimpleRbacAuthorizeTest.php index 7b19e0abf..8b93261f5 100644 --- a/tests/TestCase/Auth/SimpleRbacAuthorizeTest.php +++ b/tests/TestCase/Auth/SimpleRbacAuthorizeTest.php @@ -11,13 +11,13 @@ namespace CakeDC\Users\Test\TestCase\Auth; -use CakeDC\Users\Auth\Rules\Rule; -use CakeDC\Users\Auth\SimpleRbacAuthorize; use Cake\Controller\ComponentRegistry; use Cake\Network\Request; use Cake\Network\Response; use Cake\TestSuite\TestCase; use Cake\Utility\Hash; +use CakeDC\Users\Auth\Rules\Rule; +use CakeDC\Users\Auth\SimpleRbacAuthorize; use Psr\Log\LogLevel; use ReflectionClass; @@ -97,8 +97,8 @@ public function testConstruct() public function testLoadPermissions() { $this->simpleRbacAuthorize = $this->getMockBuilder('CakeDC\Users\Auth\SimpleRbacAuthorize') - ->disableOriginalConstructor() - ->getMock(); + ->disableOriginalConstructor() + ->getMock(); $reflectedClass = new ReflectionClass($this->simpleRbacAuthorize); $loadPermissions = $reflectedClass->getMethod('_loadPermissions'); $loadPermissions->setAccessible(true); @@ -135,20 +135,22 @@ protected function assertConstructorPermissions($instance, $config, $permissions */ public function testConstructPermissionsFileHappy() { - $permissions = [[ - 'controller' => 'Test', - 'action' => 'test' - ]]; + $permissions = [ + [ + 'controller' => 'Test', + 'action' => 'test' + ] + ]; $className = 'CakeDC\Users\Auth\SimpleRbacAuthorize'; $this->simpleRbacAuthorize = $this->getMockBuilder($className) - ->setMethods(['_loadPermissions']) - ->disableOriginalConstructor() - ->getMock(); + ->setMethods(['_loadPermissions']) + ->disableOriginalConstructor() + ->getMock(); $this->simpleRbacAuthorize - ->expects($this->once()) - ->method('_loadPermissions') - ->with('permissions-happy') - ->will($this->returnValue($permissions)); + ->expects($this->once()) + ->method('_loadPermissions') + ->with('permissions-happy') + ->will($this->returnValue($permissions)); $this->assertConstructorPermissions($className, ['autoload_config' => 'permissions-happy'], $permissions); } @@ -156,9 +158,9 @@ protected function preparePermissions($permissions) { $className = 'CakeDC\Users\Auth\SimpleRbacAuthorize'; $simpleRbacAuthorize = $this->getMockBuilder($className) - ->setMethods(['_loadPermissions']) - ->disableOriginalConstructor() - ->getMock(); + ->setMethods(['_loadPermissions']) + ->disableOriginalConstructor() + ->getMock(); $simpleRbacAuthorize->config('permissions', $permissions); return $simpleRbacAuthorize; @@ -186,6 +188,76 @@ public function providerAuthorize() ->willReturn(true); return [ + 'discard-first' => [ + //permissions + [ + [ + 'role' => 'test', + 'controller' => 'Tests', + 'action' => 'three', // Discard here + function () { + throw new \Exception(); + } + ], + [ + 'plugin' => ['Tests'], + 'role' => ['test'], + 'controller' => ['Tests'], + 'action' => ['one', 'two'], + ], + ], + //user + [ + 'id' => 1, + 'username' => 'luke', + 'role' => 'test', + ], + //request + [ + 'plugin' => 'Tests', + 'controller' => 'Tests', + 'action' => 'one' + ], + //expected + true + ], + 'deny-first-discard-after' => [ + //permissions + [ + [ + 'role' => 'test', + 'controller' => 'Tests', + 'action' => 'one', + 'allowed' => function () { + return false; // Deny here since under 'allowed' key + } + ], + [ + // This permission isn't evaluated + function () { + throw new \Exception(); + }, + 'plugin' => ['Tests'], + 'role' => ['test'], + 'controller' => ['Tests'], + 'action' => ['one', 'two'], + ], + ], + //user + [ + 'id' => 1, + 'username' => 'luke', + 'role' => 'test', + ], + //request + [ + 'plugin' => 'Tests', + 'controller' => 'Tests', + 'action' => 'one' + ], + //expected + false + ], 'star-invert' => [ //permissions [[ From 92d73612044fb19b2cbf38993a61ae3e0b4d8a3d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20Gonz=C3=A1lez?= Date: Wed, 21 Sep 2016 09:38:48 +0100 Subject: [PATCH 08/29] refs #helper-allowed-fix add allowed to manage the AuthLinkHelper when the action is allowed in Auth --- src/View/Helper/AuthLinkHelper.php | 16 +++++--- .../View/Helper/AuthLinkHelperTest.php | 41 +++++++++++++++++++ 2 files changed, 52 insertions(+), 5 deletions(-) diff --git a/src/View/Helper/AuthLinkHelper.php b/src/View/Helper/AuthLinkHelper.php index 648e64423..2b02c3fcb 100644 --- a/src/View/Helper/AuthLinkHelper.php +++ b/src/View/Helper/AuthLinkHelper.php @@ -18,15 +18,21 @@ class AuthLinkHelper extends HtmlHelper * * @param string $title link's title. * @param string|array|null $url url that the user is making request. - * @param array $options Array with option data. - * @return string + * @param array $options Array with option data. Extra options include + * 'before' and 'after' to quickly inject some html code in the link, like icons etc + * 'allowed' to manage if the link should be displayed, default is null to check isAuthorized + * @return string|bool */ public function link($title, $url = null, array $options = []) { - if ($this->isAuthorized($url)) { - $linkOptions = $options; - unset($linkOptions['before'], $linkOptions['after']); + $linkOptions = $options; + unset($linkOptions['before'], $linkOptions['after'], $linkOptions['allowed']); + $allowed = Hash::get($options, 'allowed'); + if ($allowed === false) { + return false; + } + if ($allowed === true || $this->isAuthorized($url)) { return Hash::get($options, 'before') . parent::link($title, $url, $linkOptions) . Hash::get($options, 'after'); } diff --git a/tests/TestCase/View/Helper/AuthLinkHelperTest.php b/tests/TestCase/View/Helper/AuthLinkHelperTest.php index 165423aec..426f2f7f3 100644 --- a/tests/TestCase/View/Helper/AuthLinkHelperTest.php +++ b/tests/TestCase/View/Helper/AuthLinkHelperTest.php @@ -78,6 +78,47 @@ public function testLinkAuthorized() $this->assertSame('before_title_after', $link); } + /** + * Test link + * + * @return void + */ + public function testLinkAuthorizedAllowedTrue() + { + $view = new View(); + $eventManagerMock = $this->getMockBuilder('Cake\Event\EventManager') + ->setMethods(['dispatch']) + ->getMock(); + $view->eventManager($eventManagerMock); + $this->AuthLink = new AuthLinkHelper($view); + $result = new Event('dispatch-result'); + $result->result = true; + $eventManagerMock->expects($this->never()) + ->method('dispatch'); + + $link = $this->AuthLink->link('title', '/', ['allowed' => true, 'before' => 'before_', 'after' => '_after', 'class' => 'link-class']); + $this->assertSame('before_title_after', $link); + } + + /** + * Test link + * + * @return void + */ + public function testLinkAuthorizedAllowedFalse() + { + $view = new View(); + $eventManagerMock = $this->getMockBuilder('Cake\Event\EventManager') + ->setMethods(['dispatch']) + ->getMock(); + $view->eventManager($eventManagerMock); + $this->AuthLink = new AuthLinkHelper($view); + $result = new Event('dispatch-result'); + $eventManagerMock->expects($this->never()) + ->method('dispatch'); + $link = $this->AuthLink->link('title', '/', ['allowed' => false, 'before' => 'before_', 'after' => '_after', 'class' => 'link-class']); + $this->assertFalse($link); + } /** * Test isAuthorized From a402d374afc2b5511ec6099b46763e7852b9649e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20Gonz=C3=A1lez?= Date: Wed, 21 Sep 2016 21:12:52 +0100 Subject: [PATCH 09/29] fix cs --- src/Auth/Rules/AbstractRule.php | 8 ++++---- tests/TestCase/Auth/SimpleRbacAuthorizeTest.php | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/Auth/Rules/AbstractRule.php b/src/Auth/Rules/AbstractRule.php index eac837ea4..0517e3d71 100644 --- a/src/Auth/Rules/AbstractRule.php +++ b/src/Auth/Rules/AbstractRule.php @@ -26,8 +26,8 @@ abstract class AbstractRule implements Rule { use InstanceConfigTrait; - use ModelAwareTrait; use LocatorAwareTrait; + use ModelAwareTrait; /** * @var array default config @@ -49,7 +49,7 @@ public function __construct($config = []) * @param Request $request request * @param mixed $table table * @return \Cake\ORM\Table - * @throw OutOfBoundsException if table alias is empty + * @throws \OutOfBoundsException if table alias is empty */ protected function _getTable(Request $request, $table = null) { @@ -68,7 +68,7 @@ protected function _getTable(Request $request, $table = null) * * @param Request $request request * @return Table - * @throws OutOfBoundsException if table alias can't be extracted from request + * @throws \OutOfBoundsException if table alias can't be extracted from request */ protected function _getTableFromRequest(Request $request) { @@ -91,7 +91,7 @@ protected function _getTableFromRequest(Request $request) * @param string $role role of the user * @param Request $request current request, used to get a default table if not provided * @return bool - * @throws OutOfBoundsException if table is not found or it doesn't have the expected fields + * @throws \OutOfBoundsException if table is not found or it doesn't have the expected fields */ abstract public function allowed(array $user, $role, Request $request); } diff --git a/tests/TestCase/Auth/SimpleRbacAuthorizeTest.php b/tests/TestCase/Auth/SimpleRbacAuthorizeTest.php index 8b93261f5..0d4eb865b 100644 --- a/tests/TestCase/Auth/SimpleRbacAuthorizeTest.php +++ b/tests/TestCase/Auth/SimpleRbacAuthorizeTest.php @@ -11,13 +11,13 @@ namespace CakeDC\Users\Test\TestCase\Auth; +use CakeDC\Users\Auth\Rules\Rule; +use CakeDC\Users\Auth\SimpleRbacAuthorize; use Cake\Controller\ComponentRegistry; use Cake\Network\Request; use Cake\Network\Response; use Cake\TestSuite\TestCase; use Cake\Utility\Hash; -use CakeDC\Users\Auth\Rules\Rule; -use CakeDC\Users\Auth\SimpleRbacAuthorize; use Psr\Log\LogLevel; use ReflectionClass; From 1823bb00ee4730c0fb48e30504dc87cdb2a6efde Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20M=2E=20Gonz=C3=A1lez=20Mart=C3=ADn?= Date: Thu, 20 Oct 2016 19:07:20 +0100 Subject: [PATCH 10/29] Update login action to use prefix=false --- config/users.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/users.php b/config/users.php index 7d2662ec1..dd2cfa232 100644 --- a/config/users.php +++ b/config/users.php @@ -101,7 +101,7 @@ 'plugin' => 'CakeDC/Users', 'controller' => 'Users', 'action' => 'login', - 'prefix' => null + 'prefix' => false ], 'authenticate' => [ 'all' => [ From a731da2063b48fb00a0b43f6474e87bad7842be5 Mon Sep 17 00:00:00 2001 From: Andrey Vystavkin Date: Sat, 29 Oct 2016 15:45:11 +0300 Subject: [PATCH 11/29] integrating two-step authorization into CakeDC users plugin --- composer.json | 6 +- config/routes.php | 4 +- config/users.php | 25 ++++++ .../GoogleAuthenticatorComponent.php | 47 ++++++++++ .../Component/UsersAuthComponent.php | 9 ++ src/Controller/Traits/LoginTrait.php | 88 +++++++++++++++++-- src/Controller/UsersController.php | 11 +++ src/Template/Users/verify.ctp | 31 +++++++ 8 files changed, 213 insertions(+), 8 deletions(-) create mode 100644 src/Controller/Component/GoogleAuthenticatorComponent.php create mode 100644 src/Template/Users/verify.ctp diff --git a/composer.json b/composer.json index 362d13241..29414f06d 100644 --- a/composer.json +++ b/composer.json @@ -36,7 +36,8 @@ "league/oauth2-instagram": "@stable", "league/oauth2-google": "@stable", "league/oauth2-linkedin": "@stable", - "google/recaptcha": "@stable" + "google/recaptcha": "@stable", + "robthree/twofactorauth": "^1.5" }, "suggest": { "league/oauth1-client": "Provides Social Authentication with Twitter", @@ -44,7 +45,8 @@ "league/oauth2-instagram": "Provides Social Authentication with Instagram", "league/oauth2-google": "Provides Social Authentication with Google+", "league/oauth2-linkedin": "Provides Social Authentication with LinkedIn", - "google/recaptcha": "Provides reCAPTCHA validation for registration form" + "google/recaptcha": "Provides reCAPTCHA validation for registration form", + "robthree/twofactorauth": "Provides Google Authenticator functionality" }, "autoload": { "psr-4": { diff --git a/config/routes.php b/config/routes.php index 6c4331798..7b63e7ce9 100644 --- a/config/routes.php +++ b/config/routes.php @@ -28,4 +28,6 @@ ]); 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']); \ No newline at end of file +Router::connect('/logout', ['plugin' => 'CakeDC/Users', 'controller' => 'Users', 'action' => 'logout']); + +Router::connect('/verify', ['plugin' => 'CakeDC/Users', 'controller' => 'Users', 'action' => 'verify']); diff --git a/config/users.php b/config/users.php index 7d2662ec1..cb2c17f99 100644 --- a/config/users.php +++ b/config/users.php @@ -56,6 +56,23 @@ //enable social login 'login' => false, ], + 'GoogleAuthenticator' => [ + //enable Google Authenticator + 'login' => false, + 'issuer' => null, + // The number of digits the resulting codes will be + 'digits' => 6, + // The number of seconds a code will be valid + 'period' => 30, + // The algorithm used + 'algorithm' => 'sha1', + // QR-code provider (more on this later) + 'qrcodeprovider' => null, + // Random Number Generator provider (more on this later) + 'rngprovider' => null, + // Key used for encrypting the user credentials, leave this false to use Security.salt + 'encryptionKey' => false + ], 'Profile' => [ //Allow view other users profiles 'viewOthers' => true, @@ -95,6 +112,14 @@ ] ], ], + 'GoogleAuthenticator' => [ + 'verifyAction' => [ + 'plugin' => 'CakeDC/Users', + 'controller' => 'Users', + 'action' => 'verify', + 'prefix' => null, + ], + ], //default configuration used to auto-load the Auth Component, override to change the way Auth works 'Auth' => [ 'loginAction' => [ diff --git a/src/Controller/Component/GoogleAuthenticatorComponent.php b/src/Controller/Component/GoogleAuthenticatorComponent.php new file mode 100644 index 000000000..2f9e3ea90 --- /dev/null +++ b/src/Controller/Component/GoogleAuthenticatorComponent.php @@ -0,0 +1,47 @@ +tfa = new TwoFactorAuth( + Configure::read('Users.GoogleAuthenticator.issuer'), + Configure::read('Users.GoogleAuthenticator.digits'), + Configure::read('Users.GoogleAuthenticator.period'), + Configure::read('Users.GoogleAuthenticator.algorithm'), + Configure::read('Users.GoogleAuthenticator.qrcodeprovider'), + Configure::read('Users.GoogleAuthenticator.rngprovider'), + Configure::read('Users.GoogleAuthenticator.encryptionKey') + ); + } + } + + public function verifyCode($secret, $code) + { + return $this->tfa->verifyCode($secret, $code); + } + + public function getQRCodeImageAsDataUri($issuer, $secret) + { + return $this->tfa->getQRCodeImageAsDataUri($issuer, $secret); + } +} + diff --git a/src/Controller/Component/UsersAuthComponent.php b/src/Controller/Component/UsersAuthComponent.php index 3257e607d..27eac54dc 100644 --- a/src/Controller/Component/UsersAuthComponent.php +++ b/src/Controller/Component/UsersAuthComponent.php @@ -32,6 +32,7 @@ class UsersAuthComponent extends Component const EVENT_BEFORE_LOGOUT = 'Users.Component.UsersAuth.beforeLogout'; const EVENT_AFTER_LOGOUT = 'Users.Component.UsersAuth.afterLogout'; + /** * Initialize method, setup Auth if not already done passing the $config provided and * setup the default table to Users.Users if not provided @@ -52,9 +53,17 @@ public function initialize(array $config) $this->_loadRememberMe(); } + if (Configure::read('Users.GoogleAuthenticator.login')) { + $this->_loadGoogleAuthenticator(); + } $this->_attachPermissionChecker(); } + protected function _loadGoogleAuthenticator() + { + $this->_registry->getController()->loadComponent('CakeDC/Users.GoogleAuthenticator'); + } + /** * Load Social Auth object * diff --git a/src/Controller/Traits/LoginTrait.php b/src/Controller/Traits/LoginTrait.php index 1aa16833d..55adcc890 100644 --- a/src/Controller/Traits/LoginTrait.php +++ b/src/Controller/Traits/LoginTrait.php @@ -19,6 +19,7 @@ use Cake\Core\Configure; use Cake\Event\Event; use Cake\Network\Exception\NotFoundException; +use Cake\ORM\TableRegistry; use League\OAuth1\Client\Server\Twitter; /** @@ -155,8 +156,9 @@ public function login() } $socialLogin = $this->_isSocialLogin(); + $googleAuthenticatorLogin = $this->_isGoogleAuthenticator(); - if ($this->request->is('post')) { + if ($this->request->is('post')) { if (!$this->_checkReCaptcha()) { $this->Flash->error(__d('CakeDC/Users', 'Invalid reCaptcha')); @@ -164,9 +166,10 @@ public function login() } $user = $this->Auth->identify(); - return $this->_afterIdentifyUser($user, $socialLogin); + return $this->_afterIdentifyUser($user, $socialLogin, $googleAuthenticatorLogin); } - if (!$this->request->is('post') && !$socialLogin) { + + if (!$this->request->is('post') && !$socialLogin) { if ($this->Auth->user()) { $msg = __d('CakeDC/Users', 'You are already logged in'); $this->Flash->error($msg); @@ -177,6 +180,66 @@ public function login() } } + /** + * verify for Google Authenticator codes + */ + public function verify() + { + if (!Configure::read('Users.GoogleAuthenticator.login')) { + $message = __d('CakeDC/Users', 'Google Authenticator is disabled'); + $this->Flash->error($message, 'default', [], 'auth'); + + $this->redirect(Configure::read('Auth.loginAction')); + } + + $user = $this->Auth->user(); + $secret = $user['secret']; + if ( empty($secret) ) { + $secret = $this->GoogleAuthenticator->tfa->createSecret(); + + $users = TableRegistry::get('Users'); + $query = $users->query(); + $query->update() + ->set(['secret' => $secret]) + ->where(['id' => $user['id']]) + ->execute(); + + $this->request->session()->write('Auth.User.secret', $secret); + $this->set('secretDataUri', $this->GoogleAuthenticator->getQRCodeImageAsDataUri($user['email'], $secret)); + } + + if ($this->request->is('post')) { + $verificationCode = $this->request->data['code']; + $user = $this->Auth->user(); + + $verified = $this->GoogleAuthenticator->verifyCode($secret, $verificationCode) ); + + if (!$verified) { + $message = __d('CakeDC/Users', 'Verification code is invalid. Try again'); + $this->Flash->error($message, 'default', [], 'auth'); + + //prevent the user accessing the authorized area + //with unverified two-step, thus destroying the session + $this->request->session()->destroy(); + + $this->redirect(Configure::read('Auth.loginAction')); + } else { + $url = $this->Auth->redirectUrl(); + return $this->redirect($url); + } + } + } + + /** + * verify for Google Authenticator codes + */ + + $this->redirect($this->Auth->redirectUrl()); + } + } + } + + /** * Check reCaptcha if enabled for login * @@ -200,15 +263,21 @@ protected function _checkReCaptcha() * @param bool $socialLogin is social login * @return array */ - protected function _afterIdentifyUser($user, $socialLogin = false) + protected function _afterIdentifyUser($user, $socialLogin = false, $googleAuthenticatorLogin = false) { if (!empty($user)) { $this->Auth->setUser($user); + if ($googleAuthenticatorLogin) { + $url = Configure::read('GoogleAuthenticator.verifyAction'); + return $this->redirect($url); + } + $event = $this->dispatchEvent(UsersAuthComponent::EVENT_AFTER_LOGIN, ['user' => $user]); - if (is_array($event->result)) { + if (is_array($event->result)) { return $this->redirect($event->result); } + $url = $this->Auth->redirectUrl(); return $this->redirect($url); @@ -256,4 +325,13 @@ protected function _isSocialLogin() return Configure::read('Users.Social.login') && $this->request->session()->check(Configure::read('Users.Key.Session.social')); } + + /** + * Check if we doing Google Authenticator Two Factor auth + * @return bool true if Google Authenticator is enabled + */ + protected function _isGoogleAuthenticator() + { + return Configure::read('Users.GoogleAuthenticator.login'); + } } diff --git a/src/Controller/UsersController.php b/src/Controller/UsersController.php index 996398a3c..5cc8a1a3c 100644 --- a/src/Controller/UsersController.php +++ b/src/Controller/UsersController.php @@ -21,6 +21,7 @@ use CakeDC\Users\Controller\Traits\SocialTrait; use CakeDC\Users\Model\Table\UsersTable; use Cake\Core\Configure; +use Cake\Event\Event; use Cake\ORM\Table; /** @@ -36,4 +37,14 @@ class UsersController extends AppController use RegisterTrait; use SimpleCrudTrait; use SocialTrait; + + public function beforeFilter(Event $event) + { + parent::beforeFilter($event); + + // let us verify the code received from Google Authenticator + if (Configure::read('Users.GoogleAuthenticator.login')) { + // $this->Auth->allow(['verify']); + } + } } diff --git a/src/Template/Users/verify.ctp b/src/Template/Users/verify.ctp new file mode 100644 index 000000000..5cebeded2 --- /dev/null +++ b/src/Template/Users/verify.ctp @@ -0,0 +1,31 @@ +layout = 'QoboAdminPanel.plain'; +?> +
+
+
+
+ Form->create() ?> +

+ elementExists('QoboAdminPanel.logo')) { + echo $this->element('QoboAdminPanel.logo'); + } + ?> +

+ Flash->render('auth') ?> + Flash->render() ?> +
+ +

+ + Form->input('code', ['required' => true,'label' => 'Verification Code']) ?> +
+ Form->button(__d('Users', ' Verify'), ['class' => 'btn btn-primary']); ?> + Form->end() ?> +
+
+
+
From 2fd6e8aa81ccc3a6e69b1805c3d0bc8ea0831849 Mon Sep 17 00:00:00 2001 From: Andrey Vystavkin Date: Sat, 29 Oct 2016 15:48:05 +0300 Subject: [PATCH 12/29] fixed typos --- src/Controller/Traits/LoginTrait.php | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/src/Controller/Traits/LoginTrait.php b/src/Controller/Traits/LoginTrait.php index 55adcc890..075f0766b 100644 --- a/src/Controller/Traits/LoginTrait.php +++ b/src/Controller/Traits/LoginTrait.php @@ -212,7 +212,7 @@ public function verify() $verificationCode = $this->request->data['code']; $user = $this->Auth->user(); - $verified = $this->GoogleAuthenticator->verifyCode($secret, $verificationCode) ); + $verified = $this->GoogleAuthenticator->verifyCode($secret, $verificationCode); if (!$verified) { $message = __d('CakeDC/Users', 'Verification code is invalid. Try again'); @@ -230,16 +230,6 @@ public function verify() } } - /** - * verify for Google Authenticator codes - */ - - $this->redirect($this->Auth->redirectUrl()); - } - } - } - - /** * Check reCaptcha if enabled for login * From 55f6daef3ea7d222ee7f6b74825891b2ded2e706 Mon Sep 17 00:00:00 2001 From: Andrey Vystavkin Date: Sun, 30 Oct 2016 11:45:30 +0200 Subject: [PATCH 13/29] moving tfa methods inside GoogleAuthenticator --- .../GoogleAuthenticatorComponent.php | 35 ++++++++++++++++--- src/Controller/Traits/LoginTrait.php | 2 +- 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/src/Controller/Component/GoogleAuthenticatorComponent.php b/src/Controller/Component/GoogleAuthenticatorComponent.php index 2f9e3ea90..09d0596b4 100644 --- a/src/Controller/Component/GoogleAuthenticatorComponent.php +++ b/src/Controller/Component/GoogleAuthenticatorComponent.php @@ -1,6 +1,7 @@ tfa = new TwoFactorAuth( Configure::read('Users.GoogleAuthenticator.issuer'), @@ -34,11 +41,31 @@ public function initialize(array $config) } } + /** + * createSecret + * @return base32 shared secret stored in users table + */ + public function createSecret() + { + return $this->tfa->createSecret(); + } + + /** + * verifyCode + * Verifying tfa code with shared secret + * @param string $secret of the user + * @param string $code from verification form + * @return boolean + */ public function verifyCode($secret, $code) { return $this->tfa->verifyCode($secret, $code); } + /** + * getQRCodeImageAsDataUri + * @return string base64 string containing QR code for shared secret + */ public function getQRCodeImageAsDataUri($issuer, $secret) { return $this->tfa->getQRCodeImageAsDataUri($issuer, $secret); diff --git a/src/Controller/Traits/LoginTrait.php b/src/Controller/Traits/LoginTrait.php index 075f0766b..ffe711d2d 100644 --- a/src/Controller/Traits/LoginTrait.php +++ b/src/Controller/Traits/LoginTrait.php @@ -195,7 +195,7 @@ public function verify() $user = $this->Auth->user(); $secret = $user['secret']; if ( empty($secret) ) { - $secret = $this->GoogleAuthenticator->tfa->createSecret(); + $secret = $this->GoogleAuthenticator->createSecret(); $users = TableRegistry::get('Users'); $query = $users->query(); From f2ee568d41bccba10eb6c1e15288954c22198364 Mon Sep 17 00:00:00 2001 From: Andrey Vystavkin Date: Sun, 30 Oct 2016 11:46:07 +0200 Subject: [PATCH 14/29] removing unnecessary beforeFilter --- src/Controller/UsersController.php | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/src/Controller/UsersController.php b/src/Controller/UsersController.php index 5cc8a1a3c..0a35f3def 100644 --- a/src/Controller/UsersController.php +++ b/src/Controller/UsersController.php @@ -37,14 +37,4 @@ class UsersController extends AppController use RegisterTrait; use SimpleCrudTrait; use SocialTrait; - - public function beforeFilter(Event $event) - { - parent::beforeFilter($event); - - // let us verify the code received from Google Authenticator - if (Configure::read('Users.GoogleAuthenticator.login')) { - // $this->Auth->allow(['verify']); - } - } } From 735f2e94441657ab6e565a11b228c3f4de21f8f7 Mon Sep 17 00:00:00 2001 From: Andrey Vystavkin Date: Sun, 30 Oct 2016 11:52:51 +0200 Subject: [PATCH 15/29] fixed coding style --- .../GoogleAuthenticatorComponent.php | 14 ++++------ .../Component/UsersAuthComponent.php | 5 ++++ src/Controller/Traits/LoginTrait.php | 28 ++++++++++--------- 3 files changed, 26 insertions(+), 21 deletions(-) diff --git a/src/Controller/Component/GoogleAuthenticatorComponent.php b/src/Controller/Component/GoogleAuthenticatorComponent.php index 09d0596b4..c9c9034a9 100644 --- a/src/Controller/Component/GoogleAuthenticatorComponent.php +++ b/src/Controller/Component/GoogleAuthenticatorComponent.php @@ -1,7 +1,6 @@ tfa = new TwoFactorAuth( Configure::read('Users.GoogleAuthenticator.issuer'), Configure::read('Users.GoogleAuthenticator.digits'), @@ -39,7 +38,7 @@ public function initialize(array $config) Configure::read('Users.GoogleAuthenticator.encryptionKey') ); } - } + } /** * createSecret @@ -55,7 +54,7 @@ public function createSecret() * Verifying tfa code with shared secret * @param string $secret of the user * @param string $code from verification form - * @return boolean + * @return bool */ public function verifyCode($secret, $code) { @@ -71,4 +70,3 @@ public function getQRCodeImageAsDataUri($issuer, $secret) return $this->tfa->getQRCodeImageAsDataUri($issuer, $secret); } } - diff --git a/src/Controller/Component/UsersAuthComponent.php b/src/Controller/Component/UsersAuthComponent.php index 27eac54dc..6ac53d2e7 100644 --- a/src/Controller/Component/UsersAuthComponent.php +++ b/src/Controller/Component/UsersAuthComponent.php @@ -59,6 +59,11 @@ public function initialize(array $config) $this->_attachPermissionChecker(); } + /** + * Load GoogleAuthenticator object + * + * @return void + */ protected function _loadGoogleAuthenticator() { $this->_registry->getController()->loadComponent('CakeDC/Users.GoogleAuthenticator'); diff --git a/src/Controller/Traits/LoginTrait.php b/src/Controller/Traits/LoginTrait.php index ffe711d2d..4d266baa8 100644 --- a/src/Controller/Traits/LoginTrait.php +++ b/src/Controller/Traits/LoginTrait.php @@ -156,9 +156,9 @@ public function login() } $socialLogin = $this->_isSocialLogin(); - $googleAuthenticatorLogin = $this->_isGoogleAuthenticator(); + $googleAuthenticatorLogin = $this->_isGoogleAuthenticator(); - if ($this->request->is('post')) { + if ($this->request->is('post')) { if (!$this->_checkReCaptcha()) { $this->Flash->error(__d('CakeDC/Users', 'Invalid reCaptcha')); @@ -169,7 +169,7 @@ public function login() return $this->_afterIdentifyUser($user, $socialLogin, $googleAuthenticatorLogin); } - if (!$this->request->is('post') && !$socialLogin) { + if (!$this->request->is('post') && !$socialLogin) { if ($this->Auth->user()) { $msg = __d('CakeDC/Users', 'You are already logged in'); $this->Flash->error($msg); @@ -194,7 +194,7 @@ public function verify() $user = $this->Auth->user(); $secret = $user['secret']; - if ( empty($secret) ) { + if (empty($secret)) { $secret = $this->GoogleAuthenticator->createSecret(); $users = TableRegistry::get('Users'); @@ -225,6 +225,7 @@ public function verify() $this->redirect(Configure::read('Auth.loginAction')); } else { $url = $this->Auth->redirectUrl(); + return $this->redirect($url); } } @@ -260,11 +261,12 @@ protected function _afterIdentifyUser($user, $socialLogin = false, $googleAuthen if ($googleAuthenticatorLogin) { $url = Configure::read('GoogleAuthenticator.verifyAction'); + return $this->redirect($url); } $event = $this->dispatchEvent(UsersAuthComponent::EVENT_AFTER_LOGIN, ['user' => $user]); - if (is_array($event->result)) { + if (is_array($event->result)) { return $this->redirect($event->result); } @@ -316,12 +318,12 @@ protected function _isSocialLogin() $this->request->session()->check(Configure::read('Users.Key.Session.social')); } - /** - * Check if we doing Google Authenticator Two Factor auth - * @return bool true if Google Authenticator is enabled - */ - protected function _isGoogleAuthenticator() - { - return Configure::read('Users.GoogleAuthenticator.login'); - } + /** + * Check if we doing Google Authenticator Two Factor auth + * @return bool true if Google Authenticator is enabled + */ + protected function _isGoogleAuthenticator() + { + return Configure::read('Users.GoogleAuthenticator.login'); + } } From c6e7d1d36c045b73de17447f8d88c2e688be8a3a Mon Sep 17 00:00:00 2001 From: Andrey Vystavkin Date: Sun, 30 Oct 2016 12:24:02 +0200 Subject: [PATCH 16/29] unsetting secret key from the session once verified --- src/Controller/Traits/LoginTrait.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/Controller/Traits/LoginTrait.php b/src/Controller/Traits/LoginTrait.php index 4d266baa8..c7c39fdd5 100644 --- a/src/Controller/Traits/LoginTrait.php +++ b/src/Controller/Traits/LoginTrait.php @@ -224,6 +224,8 @@ public function verify() $this->redirect(Configure::read('Auth.loginAction')); } else { + //removing secret key from the session, once verified + $this->request->session()->delete('Auth.User.secret'); $url = $this->Auth->redirectUrl(); return $this->redirect($url); From 6d5a7415ecb266652f09f59c01f180c87db8a674 Mon Sep 17 00:00:00 2001 From: Andrey Vystavkin Date: Sun, 30 Oct 2016 12:34:08 +0200 Subject: [PATCH 17/29] removing obsolete code for the core plugin --- src/Controller/UsersController.php | 1 - src/Template/Users/verify.ctp | 2 -- 2 files changed, 3 deletions(-) diff --git a/src/Controller/UsersController.php b/src/Controller/UsersController.php index 0a35f3def..996398a3c 100644 --- a/src/Controller/UsersController.php +++ b/src/Controller/UsersController.php @@ -21,7 +21,6 @@ use CakeDC\Users\Controller\Traits\SocialTrait; use CakeDC\Users\Model\Table\UsersTable; use Cake\Core\Configure; -use Cake\Event\Event; use Cake\ORM\Table; /** diff --git a/src/Template/Users/verify.ctp b/src/Template/Users/verify.ctp index 5cebeded2..4906b23ba 100644 --- a/src/Template/Users/verify.ctp +++ b/src/Template/Users/verify.ctp @@ -1,7 +1,5 @@ layout = 'QoboAdminPanel.plain'; ?>
From 43ded2f0ebdd54d59554cf04fea7a1d40d1703e2 Mon Sep 17 00:00:00 2001 From: Andrey Vystavkin Date: Sun, 30 Oct 2016 12:37:56 +0200 Subject: [PATCH 18/29] removed Qobo customizations --- src/Template/Users/verify.ctp | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/src/Template/Users/verify.ctp b/src/Template/Users/verify.ctp index 4906b23ba..b93df3247 100644 --- a/src/Template/Users/verify.ctp +++ b/src/Template/Users/verify.ctp @@ -1,24 +1,15 @@ -
Form->create() ?> -

- elementExists('QoboAdminPanel.logo')) { - echo $this->element('QoboAdminPanel.logo'); - } - ?> -

+ Flash->render('auth') ?> Flash->render() ?>
- -

- + +

+ Form->input('code', ['required' => true,'label' => 'Verification Code']) ?>
Form->button(__d('Users', ' Verify'), ['class' => 'btn btn-primary']); ?> From b5bc8d757d86b5d5537f6488e7a38396cd815592 Mon Sep 17 00:00:00 2001 From: Andrey Vystavkin Date: Sun, 30 Oct 2016 13:03:09 +0200 Subject: [PATCH 19/29] moving tfa into require section --- composer.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/composer.json b/composer.json index 29414f06d..00f645695 100644 --- a/composer.json +++ b/composer.json @@ -27,7 +27,8 @@ "source": "https://github.com/CakeDC/users" }, "require": { - "cakephp/cakephp": ">=3.2.9 <4.0.0" + "cakephp/cakephp": ">=3.2.9 <4.0.0", + "robthree/twofactorauth": "^1.5" }, "require-dev": { "phpunit/phpunit": "@stable", From 8152f367a2e0bf6be753bc912f4906282e97b597 Mon Sep 17 00:00:00 2001 From: Andrey Vystavkin Date: Mon, 31 Oct 2016 09:14:34 +0200 Subject: [PATCH 20/29] changing deps for psr-4 compatible version --- composer.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/composer.json b/composer.json index 00f645695..fffdba59b 100644 --- a/composer.json +++ b/composer.json @@ -28,7 +28,7 @@ }, "require": { "cakephp/cakephp": ">=3.2.9 <4.0.0", - "robthree/twofactorauth": "^1.5" + "robthree/twofactorauth": "^1.5.2" }, "require-dev": { "phpunit/phpunit": "@stable", @@ -38,7 +38,7 @@ "league/oauth2-google": "@stable", "league/oauth2-linkedin": "@stable", "google/recaptcha": "@stable", - "robthree/twofactorauth": "^1.5" + "robthree/twofactorauth": "^1.5.2" }, "suggest": { "league/oauth1-client": "Provides Social Authentication with Twitter", From 50740054a1ac6a57b7cba597aa5c2d0f96edc542 Mon Sep 17 00:00:00 2001 From: Andrey Vystavkin Date: Mon, 31 Oct 2016 11:54:06 +0200 Subject: [PATCH 21/29] making sure the Auth session will not be accidently verified --- .../Component/UsersAuthComponent.php | 4 +- src/Controller/Traits/LoginTrait.php | 73 ++++++++++++------- 2 files changed, 49 insertions(+), 28 deletions(-) diff --git a/src/Controller/Component/UsersAuthComponent.php b/src/Controller/Component/UsersAuthComponent.php index 6ac53d2e7..551bdb414 100644 --- a/src/Controller/Component/UsersAuthComponent.php +++ b/src/Controller/Component/UsersAuthComponent.php @@ -56,6 +56,7 @@ public function initialize(array $config) if (Configure::read('Users.GoogleAuthenticator.login')) { $this->_loadGoogleAuthenticator(); } + $this->_attachPermissionChecker(); } @@ -124,7 +125,8 @@ protected function _initAuth() 'requestResetPassword', 'changePassword', 'endpoint', - 'authenticated' + 'authenticated', + 'verify' ]); } diff --git a/src/Controller/Traits/LoginTrait.php b/src/Controller/Traits/LoginTrait.php index c7c39fdd5..2ec8d77e1 100644 --- a/src/Controller/Traits/LoginTrait.php +++ b/src/Controller/Traits/LoginTrait.php @@ -19,7 +19,6 @@ use Cake\Core\Configure; use Cake\Event\Event; use Cake\Network\Exception\NotFoundException; -use Cake\ORM\TableRegistry; use League\OAuth1\Client\Server\Twitter; /** @@ -182,53 +181,73 @@ public function login() /** * verify for Google Authenticator codes + * @return void */ public function verify() { if (!Configure::read('Users.GoogleAuthenticator.login')) { - $message = __d('CakeDC/Users', 'Google Authenticator is disabled'); + $message = __d('CakeDC/Users', 'Please enable Google Authenticator first.'); $this->Flash->error($message, 'default', [], 'auth'); $this->redirect(Configure::read('Auth.loginAction')); } - $user = $this->Auth->user(); - $secret = $user['secret']; - if (empty($secret)) { - $secret = $this->GoogleAuthenticator->createSecret(); - - $users = TableRegistry::get('Users'); - $query = $users->query(); - $query->update() - ->set(['secret' => $secret]) - ->where(['id' => $user['id']]) - ->execute(); - - $this->request->session()->write('Auth.User.secret', $secret); - $this->set('secretDataUri', $this->GoogleAuthenticator->getQRCodeImageAsDataUri($user['email'], $secret)); + // storing user's session in the temporary one + // until the GA verification is checked + $temporarySession = $this->Auth->user(); + $this->request->session()->delete('Auth.User'); + + if (!empty($temporarySession)) { + $this->request->session()->write('temporarySession', $temporarySession); + } + + $secret = $temporarySession['secret']; + $secretVerified = $temporarySession['secret_verified']; + + // showing QR-code until shared secret is verified + if (!$secretVerified) { + if (empty($secret)) { + $secret = $this->GoogleAuthenticator->createSecret(); + + $query = $this->getUsersTable()->query(); + $query->update() + ->set(['secret' => $secret]) + ->where(['id' => $temporarySession['id']]) + ->execute(); + } + + $this->set('secretDataUri', $this->GoogleAuthenticator->getQRCodeImageAsDataUri($temporarySession['email'], $secret)); } if ($this->request->is('post')) { $verificationCode = $this->request->data['code']; - $user = $this->Auth->user(); + $user = $this->request->session()->read('temporarySession'); + + $codeVerified = $this->GoogleAuthenticator->verifyCode($user['secret'], $verificationCode); - $verified = $this->GoogleAuthenticator->verifyCode($secret, $verificationCode); + if($codeVerified) { + unset($user['secret']); - if (!$verified) { + if (!$user['secret_verified']) { + $this->getUsersTable()->query()->update() + ->set(['secret_verified' => true]) + ->where(['id' => $user['id']]) + ->execute(); + } + + $this->request->session()->delete('temporarySession'); + $this->request->session()->write('Auth.User', $user); + + $url = $this->Auth->redirectUrl(); + + $this->redirect($url); + } else { $message = __d('CakeDC/Users', 'Verification code is invalid. Try again'); $this->Flash->error($message, 'default', [], 'auth'); - //prevent the user accessing the authorized area - //with unverified two-step, thus destroying the session $this->request->session()->destroy(); $this->redirect(Configure::read('Auth.loginAction')); - } else { - //removing secret key from the session, once verified - $this->request->session()->delete('Auth.User.secret'); - $url = $this->Auth->redirectUrl(); - - return $this->redirect($url); } } } From 9b3f5d097e3ef1326adcf4e321063f7358ddd42b Mon Sep 17 00:00:00 2001 From: Andrey Vystavkin Date: Mon, 31 Oct 2016 12:22:08 +0200 Subject: [PATCH 22/29] adding GoogleAuthenticator migration for secret fields --- .../20161031101316_AddSecretToUsers.php | 29 +++++++++++++++++++ src/Controller/Traits/LoginTrait.php | 9 ++++-- 2 files changed, 36 insertions(+), 2 deletions(-) create mode 100644 config/Migrations/20161031101316_AddSecretToUsers.php diff --git a/config/Migrations/20161031101316_AddSecretToUsers.php b/config/Migrations/20161031101316_AddSecretToUsers.php new file mode 100644 index 000000000..7f3ea2dc4 --- /dev/null +++ b/config/Migrations/20161031101316_AddSecretToUsers.php @@ -0,0 +1,29 @@ +table('users'); + $table->addColumn('secret', 'string', [ + 'after' => 'activation_date', + 'default' => null, + 'limit' => 255, + 'null' => true, + ]); + $table->addColumn('secret_verified', 'boolean', [ + 'after' => 'secret', + 'default' => null, + 'null' => true, + ]); + $table->update(); + } +} diff --git a/src/Controller/Traits/LoginTrait.php b/src/Controller/Traits/LoginTrait.php index 2ec8d77e1..d95af39f8 100644 --- a/src/Controller/Traits/LoginTrait.php +++ b/src/Controller/Traits/LoginTrait.php @@ -180,7 +180,12 @@ public function login() } /** - * verify for Google Authenticator codes + * Verify for Google Authenticator + * If Google Authenticator's enabled we need to verify + * authenticated user. To avoid accidental access to + * other URL's we store auth'ed used into temporary session + * to perform code verification. + * * @return void */ public function verify() @@ -225,7 +230,7 @@ public function verify() $codeVerified = $this->GoogleAuthenticator->verifyCode($user['secret'], $verificationCode); - if($codeVerified) { + if ($codeVerified) { unset($user['secret']); if (!$user['secret_verified']) { From ffebc26deb7a10a93136a8b191304ff9fda15bd3 Mon Sep 17 00:00:00 2001 From: Andrey Vystavkin Date: Mon, 31 Oct 2016 13:32:21 +0200 Subject: [PATCH 23/29] added modified fixtures for Google Authenticator support --- tests/Fixture/UsersFixture.php | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/tests/Fixture/UsersFixture.php b/tests/Fixture/UsersFixture.php index 1577a0783..bcbc85458 100644 --- a/tests/Fixture/UsersFixture.php +++ b/tests/Fixture/UsersFixture.php @@ -37,6 +37,8 @@ class UsersFixture extends TestFixture 'token_expires' => ['type' => 'datetime', 'length' => null, 'null' => true, 'default' => null, 'comment' => '', 'precision' => null], 'api_token' => ['type' => 'string', 'length' => 255, 'null' => true, 'default' => null, 'comment' => '', 'precision' => null, 'fixed' => null], 'activation_date' => ['type' => 'datetime', 'length' => null, 'null' => true, 'default' => null, 'comment' => '', 'precision' => null], + 'secret' => ['type' => 'string', 'length' => 255, 'null' => true, 'default' => null, 'comment' => '', 'precision' => null, 'fixed' => null], + 'secret_verified' => ['type' => 'boolean', 'length' => null, 'null' => true, 'default' => false, 'comment' => '', 'precision' => null], 'tos_date' => ['type' => 'datetime', 'length' => null, 'null' => true, 'default' => null, 'comment' => '', 'precision' => null], 'active' => ['type' => 'boolean', 'length' => null, 'null' => false, 'default' => true, 'comment' => '', 'precision' => null], 'is_superuser' => ['type' => 'boolean', 'length' => null, 'unsigned' => false, 'null' => false, 'default' => false, 'comment' => '', 'precision' => null, 'autoIncrement' => null], @@ -70,6 +72,8 @@ class UsersFixture extends TestFixture 'token_expires' => '2035-06-24 17:33:54', 'api_token' => 'yyy', 'activation_date' => '2015-06-24 17:33:54', + 'secret' => 'yyy', + 'secret_verified' => false, 'tos_date' => '2015-06-24 17:33:54', 'active' => false, 'is_superuser' => true, @@ -88,6 +92,8 @@ class UsersFixture extends TestFixture 'token_expires' => '2015-06-24 17:33:54', 'api_token' => 'xxx', 'activation_date' => '2015-06-24 17:33:54', + 'secret' => 'xxx', + 'secret_verified' => false, 'tos_date' => '2015-06-24 17:33:54', 'active' => true, 'is_superuser' => true, @@ -106,6 +112,8 @@ class UsersFixture extends TestFixture 'token_expires' => '2030-06-20 17:33:54', 'api_token' => 'xxx', 'activation_date' => '2015-06-24 17:33:54', + 'secret' => 'xxx', + 'is_superuser' => true, 'tos_date' => '2015-06-24 17:33:54', 'active' => false, 'is_superuser' => true, @@ -124,6 +132,8 @@ class UsersFixture extends TestFixture 'token_expires' => '2030-06-24 17:33:54', 'api_token' => 'Lorem ipsum dolor sit amet', 'activation_date' => '2015-06-24 17:33:54', + 'secret' => 'Lorem ipsum dolor sit amet', + 'is_superuser' => false, 'tos_date' => '2015-06-24 17:33:54', 'active' => true, 'is_superuser' => false, @@ -142,6 +152,8 @@ class UsersFixture extends TestFixture 'token_expires' => '2015-06-24 17:33:54', 'api_token' => '', 'activation_date' => '2015-06-24 17:33:54', + 'secret' => '', + 'secret_verified' => false, 'tos_date' => '2015-06-24 17:33:54', 'active' => true, 'is_superuser' => false, @@ -160,6 +172,8 @@ class UsersFixture extends TestFixture 'token_expires' => '2015-06-24 17:33:54', 'api_token' => '', 'activation_date' => '2015-06-24 17:33:54', + 'secret' => '', + 'secret_verified' => false, 'tos_date' => '2015-06-24 17:33:54', 'active' => true, 'is_superuser' => false, @@ -178,6 +192,8 @@ class UsersFixture extends TestFixture 'token_expires' => '2015-06-24 17:33:54', 'api_token' => 'Lorem ipsum dolor sit amet', 'activation_date' => '2015-06-24 17:33:54', + 'secret' => 'Lorem ipsum dolor sit amet', + 'secret_verified' => false, 'tos_date' => '2015-06-24 17:33:54', 'active' => true, 'is_superuser' => false, @@ -196,6 +212,8 @@ class UsersFixture extends TestFixture 'token_expires' => '2015-06-24 17:33:54', 'api_token' => 'Lorem ipsum dolor sit amet', 'activation_date' => '2015-06-24 17:33:54', + 'secret' => 'Lorem ipsum dolor sit amet', + 'secret_verified' => false, 'tos_date' => '2015-06-24 17:33:54', 'active' => true, 'is_superuser' => false, @@ -214,6 +232,8 @@ class UsersFixture extends TestFixture 'token_expires' => '2015-06-24 17:33:54', 'api_token' => 'Lorem ipsum dolor sit amet', 'activation_date' => '2015-06-24 17:33:54', + 'secret' => 'Lorem ipsum dolor sit amet', + 'secret_verified' => false, 'tos_date' => '2015-06-24 17:33:54', 'active' => true, 'is_superuser' => false, @@ -232,6 +252,8 @@ class UsersFixture extends TestFixture 'token_expires' => '2015-06-24 17:33:54', 'api_token' => 'Lorem ipsum dolor sit amet', 'activation_date' => '2015-06-24 17:33:54', + 'secret' => 'Lorem ipsum dolor sit amet', + 'secret_verified' => false, 'tos_date' => '2015-06-24 17:33:54', 'active' => true, 'is_superuser' => false, From fd2fa2b3ec15e1623ee87421bc06b37c9ec980db Mon Sep 17 00:00:00 2001 From: Andrey Vystavkin Date: Mon, 31 Oct 2016 15:19:38 +0200 Subject: [PATCH 24/29] covering GoogleAuthenticator component with tests --- .../GoogleAuthenticatorComponentTest.php | 128 ++++++++++++++++++ 1 file changed, 128 insertions(+) create mode 100644 tests/TestCase/Controller/Component/GoogleAuthenticatorComponentTest.php diff --git a/tests/TestCase/Controller/Component/GoogleAuthenticatorComponentTest.php b/tests/TestCase/Controller/Component/GoogleAuthenticatorComponentTest.php new file mode 100644 index 000000000..5a0d094d4 --- /dev/null +++ b/tests/TestCase/Controller/Component/GoogleAuthenticatorComponentTest.php @@ -0,0 +1,128 @@ +backupUsersConfig = Configure::read('Users'); + + Router::reload(); + Plugin::routes('CakeDC/Users'); + Router::connect('/route/*', [ + 'plugin' => 'CakeDC/Users', + 'controller' => 'Users', + 'action' => 'requestResetPassword' + ]); + Router::connect('/notAllowed/*', [ + 'plugin' => 'CakeDC/Users', + 'controller' => 'Users', + 'action' => 'edit' + ]); + + Security::salt('YJfIxfs2guVoUubWDYhG93b0qyJfIxfs2guwvniR2G0FgaC9mi'); + Configure::write('App.namespace', 'Users'); + Configure::write('Users.GoogleAuthenticator.login', true); + + $this->request = $this->getMockBuilder('Cake\Network\Request') + ->setMethods(['is', 'method']) + ->getMock(); + $this->request->expects($this->any())->method('is')->will($this->returnValue(true)); + $this->response = $this->getMockBuilder('Cake\Network\Response') + ->setMethods(['stop']) + ->getMock(); + $this->Controller = new Controller($this->request, $this->response); + $this->Registry = $this->Controller->components(); + $this->Controller->GoogleAuthenticator = new GoogleAuthenticatorComponent($this->Registry); + } + + /** + * tearDown method + * + * @return void + */ + public function tearDown() + { + parent::tearDown(); + + $_SESSION = []; + unset($this->Controller, $this->GoogleAuthenticator); + Configure::write('Users', $this->backupUsersConfig); + Configure::write('Users.GoogleAuthenticator.login', false); + } + + /** + * Test initialize + * + */ + public function testInitialize() + { + $this->Registry->unload('GoogleAuthenticator'); + $this->Controller->GoogleAuthenticator = new GoogleAuthenticatorComponent($this->Registry); + $this->assertInstanceOf('CakeDC\Users\Controller\Component\GoogleAuthenticatorComponent', $this->Controller->GoogleAuthenticator); + } + + /** + * test base64 qr-code returned from component + * @return void + */ + public function testgetQRCodeImageAsDataUri() + { + $this->Controller->GoogleAuthenticator->initialize([]); + $result = $this->Controller->GoogleAuthenticator->getQRCodeImageAsDataUri('test@localhost.com', '123123'); + + $this->assertContains('data:image/png;base64', $result); + } + + /** + * Making sure we return secret + * @return void + */ + public function testCreateSecret() + { + $this->Controller->GoogleAuthenticator->initialize([]); + $result = $this->Controller->GoogleAuthenticator->createSecret(); + $this->assertNotEmpty($result); + } + + /** + * Testing code verification in the component + * @return void + */ + public function testVerifyCode() + { + $this->Controller->GoogleAuthenticator->initialize([]); + $secret = $this->Controller->GoogleAuthenticator->createSecret(); + $verificationCode = $this->Controller->GoogleAuthenticator->tfa->getCode($secret); + + $verified = $this->Controller->GoogleAuthenticator->verifyCode($secret, $verificationCode); + $this->assertTrue($verified); + } +} From 5584c2929ba84f4dba680d271a021dc835bf9cff Mon Sep 17 00:00:00 2001 From: Andrey Vystavkin Date: Tue, 1 Nov 2016 16:32:33 +0200 Subject: [PATCH 25/29] fixed the code issues reported from #439 --- composer.json | 6 +-- .../20161031101316_AddSecretToUsers.php | 6 ++- config/users.php | 2 +- .../Component/UsersAuthComponent.php | 1 - src/Controller/Traits/LoginTrait.php | 39 ++++++++++++------- src/Template/Users/verify.ctp | 2 +- 6 files changed, 35 insertions(+), 21 deletions(-) diff --git a/composer.json b/composer.json index fffdba59b..560a36307 100644 --- a/composer.json +++ b/composer.json @@ -27,8 +27,7 @@ "source": "https://github.com/CakeDC/users" }, "require": { - "cakephp/cakephp": ">=3.2.9 <4.0.0", - "robthree/twofactorauth": "^1.5.2" + "cakephp/cakephp": ">=3.2.9 <4.0.0" }, "require-dev": { "phpunit/phpunit": "@stable", @@ -37,8 +36,7 @@ "league/oauth2-instagram": "@stable", "league/oauth2-google": "@stable", "league/oauth2-linkedin": "@stable", - "google/recaptcha": "@stable", - "robthree/twofactorauth": "^1.5.2" + "google/recaptcha": "@stable" }, "suggest": { "league/oauth1-client": "Provides Social Authentication with Twitter", diff --git a/config/Migrations/20161031101316_AddSecretToUsers.php b/config/Migrations/20161031101316_AddSecretToUsers.php index 7f3ea2dc4..82f9f44f4 100644 --- a/config/Migrations/20161031101316_AddSecretToUsers.php +++ b/config/Migrations/20161031101316_AddSecretToUsers.php @@ -13,10 +13,14 @@ class AddSecretToUsers extends AbstractMigration public function change() { $table = $this->table('users'); + /** + * Limiting secret field to 32 chars + * @see https://en.wikipedia.org/wiki/Google_Authenticator#Technical_description + */ $table->addColumn('secret', 'string', [ 'after' => 'activation_date', 'default' => null, - 'limit' => 255, + 'limit' => 32, 'null' => true, ]); $table->addColumn('secret_verified', 'boolean', [ diff --git a/config/users.php b/config/users.php index cb2c17f99..d59d2244d 100644 --- a/config/users.php +++ b/config/users.php @@ -117,7 +117,7 @@ 'plugin' => 'CakeDC/Users', 'controller' => 'Users', 'action' => 'verify', - 'prefix' => null, + 'prefix' => false, ], ], //default configuration used to auto-load the Auth Component, override to change the way Auth works diff --git a/src/Controller/Component/UsersAuthComponent.php b/src/Controller/Component/UsersAuthComponent.php index 551bdb414..b3ee322fe 100644 --- a/src/Controller/Component/UsersAuthComponent.php +++ b/src/Controller/Component/UsersAuthComponent.php @@ -32,7 +32,6 @@ class UsersAuthComponent extends Component const EVENT_BEFORE_LOGOUT = 'Users.Component.UsersAuth.beforeLogout'; const EVENT_AFTER_LOGOUT = 'Users.Component.UsersAuth.afterLogout'; - /** * Initialize method, setup Auth if not already done passing the $config provided and * setup the default table to Users.Users if not provided diff --git a/src/Controller/Traits/LoginTrait.php b/src/Controller/Traits/LoginTrait.php index d95af39f8..a39e58e4e 100644 --- a/src/Controller/Traits/LoginTrait.php +++ b/src/Controller/Traits/LoginTrait.php @@ -17,6 +17,7 @@ use CakeDC\Users\Exception\UserNotActiveException; use CakeDC\Users\Model\Table\SocialAccountsTable; use Cake\Core\Configure; +use Cake\Core\Exception\Exception; use Cake\Event\Event; use Cake\Network\Exception\NotFoundException; use League\OAuth1\Client\Server\Twitter; @@ -206,7 +207,10 @@ public function verify() $this->request->session()->write('temporarySession', $temporarySession); } - $secret = $temporarySession['secret']; + if (array_key_exists('secret', $temporarySession)) { + $secret = $temporarySession['secret']; + } + $secretVerified = $temporarySession['secret_verified']; // showing QR-code until shared secret is verified @@ -214,21 +218,32 @@ public function verify() if (empty($secret)) { $secret = $this->GoogleAuthenticator->createSecret(); - $query = $this->getUsersTable()->query(); - $query->update() - ->set(['secret' => $secret]) - ->where(['id' => $temporarySession['id']]) - ->execute(); + // catching sql exception in case of any sql inconsistencies + try { + $query = $this->getUsersTable()->query(); + $query->update() + ->set(['secret' => $secret]) + ->where(['id' => $temporarySession['id']]); + $executed = $query->execute(); + } catch (\Exception $e) { + $this->request->session()->destroy(); + $message = __d('CakeDC/Users', $e->getMessage()); + $this->Flash->error($message, 'default', [], 'auth'); + + return $this->redirect(Configure::read('Auth.loginAction')); + } } $this->set('secretDataUri', $this->GoogleAuthenticator->getQRCodeImageAsDataUri($temporarySession['email'], $secret)); } if ($this->request->is('post')) { - $verificationCode = $this->request->data['code']; + $verificationCode = $this->request->data('code'); $user = $this->request->session()->read('temporarySession'); - $codeVerified = $this->GoogleAuthenticator->verifyCode($user['secret'], $verificationCode); + if (array_key_exists('secret', $user)) { + $codeVerified = $this->GoogleAuthenticator->verifyCode($user['secret'], $verificationCode); + } if ($codeVerified) { unset($user['secret']); @@ -242,17 +257,15 @@ public function verify() $this->request->session()->delete('temporarySession'); $this->request->session()->write('Auth.User', $user); - $url = $this->Auth->redirectUrl(); - $this->redirect($url); + return $this->redirect($url); } else { + $this->request->session()->destroy(); $message = __d('CakeDC/Users', 'Verification code is invalid. Try again'); $this->Flash->error($message, 'default', [], 'auth'); - $this->request->session()->destroy(); - - $this->redirect(Configure::read('Auth.loginAction')); + return $this->redirect(Configure::read('Auth.loginAction')); } } } diff --git a/src/Template/Users/verify.ctp b/src/Template/Users/verify.ctp index b93df3247..b0dfeeef9 100644 --- a/src/Template/Users/verify.ctp +++ b/src/Template/Users/verify.ctp @@ -10,7 +10,7 @@

- Form->input('code', ['required' => true,'label' => 'Verification Code']) ?> + Form->input('code', ['required' => true,'label' => __d('Verification Code')]) ?> Form->button(__d('Users', ' Verify'), ['class' => 'btn btn-primary']); ?> Form->end() ?> From b1a00570243b5c75752bae2b7466df5f1e6ef3ad Mon Sep 17 00:00:00 2001 From: Andrey Vystavkin Date: Tue, 1 Nov 2016 16:46:26 +0200 Subject: [PATCH 26/29] Restoring 2factor lib in require-dev for test purposes Otherwise the tests won't run --- composer.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/composer.json b/composer.json index 560a36307..0372c618d 100644 --- a/composer.json +++ b/composer.json @@ -36,7 +36,8 @@ "league/oauth2-instagram": "@stable", "league/oauth2-google": "@stable", "league/oauth2-linkedin": "@stable", - "google/recaptcha": "@stable" + "google/recaptcha": "@stable", + "robthree/twofactorauth": "^1.5.2" }, "suggest": { "league/oauth1-client": "Provides Social Authentication with Twitter", From a90f72208206bef925fe6d861e48809d1c232f04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20M=2E=20Gonz=C3=A1lez=20Mart=C3=ADn?= Date: Wed, 2 Nov 2016 12:51:00 +0000 Subject: [PATCH 27/29] fix typo in domain name --- src/Template/Users/verify.ctp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Template/Users/verify.ctp b/src/Template/Users/verify.ctp index b0dfeeef9..087fdaa6e 100644 --- a/src/Template/Users/verify.ctp +++ b/src/Template/Users/verify.ctp @@ -10,9 +10,9 @@

- Form->input('code', ['required' => true,'label' => __d('Verification Code')]) ?> + Form->input('code', ['required' => true, 'label' => __d('CakeDC/Users', 'Verification Code')]) ?> - Form->button(__d('Users', ' Verify'), ['class' => 'btn btn-primary']); ?> + Form->button(__d('CakeDC/Users', ' Verify'), ['class' => 'btn btn-primary']); ?> Form->end() ?>
From 505ba44d8f6e2d2c00ee59facfb3417d0cf32f5a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20Gonz=C3=A1lez?= Date: Fri, 2 Dec 2016 21:57:48 +0000 Subject: [PATCH 28/29] refs #383 add option to configure the api table and finder --- src/Auth/ApiKeyAuthenticate.php | 8 ++- .../TestCase/Auth/ApiKeyAuthenticateTest.php | 54 +++++++++++++++++-- 2 files changed, 56 insertions(+), 6 deletions(-) diff --git a/src/Auth/ApiKeyAuthenticate.php b/src/Auth/ApiKeyAuthenticate.php index 2a7d358bf..68ed80fbf 100644 --- a/src/Auth/ApiKeyAuthenticate.php +++ b/src/Auth/ApiKeyAuthenticate.php @@ -38,6 +38,10 @@ class ApiKeyAuthenticate extends BaseAuthenticate 'field' => 'api_token', //require SSL to pass the token. You should always require SSL to use tokens for Auth 'require_ssl' => true, + //set a specific table for API auth, set as null to use Users.table + 'table' => null, + //set a specific finder for API auth, set as null to use Auth.authenticate.all.finder + 'finder' => null, ]; /** @@ -87,8 +91,8 @@ public function getUser(Request $request) } $this->_config['fields']['username'] = $this->config('field'); - $this->_config['userModel'] = Configure::read('Users.table'); - $this->_config['finder'] = 'all'; + $this->_config['userModel'] = $this->config('table') ?: Configure::read('Users.table'); + $this->_config['finder'] = $this->config('finder') ?: Configure::read('Auth.authenticate.all.finder') ?: 'all'; $result = $this->_query($apiKey)->first(); if (empty($result)) { diff --git a/tests/TestCase/Auth/ApiKeyAuthenticateTest.php b/tests/TestCase/Auth/ApiKeyAuthenticateTest.php index 90a95c28e..e3ab0ec59 100644 --- a/tests/TestCase/Auth/ApiKeyAuthenticateTest.php +++ b/tests/TestCase/Auth/ApiKeyAuthenticateTest.php @@ -13,6 +13,7 @@ use CakeDC\Users\Auth\ApiKeyAuthenticate; use Cake\Controller\ComponentRegistry; +use Cake\Core\Configure; use Cake\Network\Request; use Cake\Network\Response; use Cake\TestSuite\TestCase; @@ -62,9 +63,9 @@ public function tearDown() */ public function testAuthenticateHappy() { - $request = new Request('/?api_key=yyy'); + $request = new Request('/?api_key=xxx'); $result = $this->apiKey->authenticate($request, new Response()); - $this->assertEquals('user-1', $result['username']); + $this->assertEquals('user-2', $result['username']); } /** @@ -85,6 +86,10 @@ public function testAuthenticateFail() $request = new Request('/?api_key='); $result = $this->apiKey->authenticate($request, new Response()); $this->assertFalse($result); + + $request = new Request('/?api_key=yyy'); + $result = $this->apiKey->authenticate($request, new Response()); + $this->assertFalse($result); } /** @@ -140,10 +145,10 @@ public function testHeaderHappy() $request->expects($this->once()) ->method('header') ->with('api_key') - ->will($this->returnValue('yyy')); + ->will($this->returnValue('xxx')); $this->apiKey->config('type', 'header'); $result = $this->apiKey->authenticate($request, new Response()); - $this->assertEquals('user-1', $result['username']); + $this->assertEquals('user-2', $result['username']); } /** @@ -164,4 +169,45 @@ public function testAuthenticateHeaderFail() $result = $this->apiKey->authenticate($request, new Response()); $this->assertFalse($result); } + + /** + * test + * + * @return void + * @expectedException \BadMethodCallException + * @expectedExceptionMessage Unknown finder method "undefinedInConfig" + */ + public function testAuthenticateFinderConfig() + { + $this->apiKey->config('finder', 'undefinedInConfig'); + $request = new Request('/?api_key=xxx'); + $result = $this->apiKey->authenticate($request, new Response()); + } + + /** + * test + * + * @return void + * @expectedException \BadMethodCallException + * @expectedExceptionMessage Unknown finder method "undefinedFinderInAuth" + */ + public function testAuthenticateFinderAuthConfig() + { + Configure::write('Auth.authenticate.all.finder', 'undefinedFinderInAuth'); + $request = new Request('/?api_key=xxx'); + $result = $this->apiKey->authenticate($request, new Response()); + } + + /** + * test + * + * @return void + */ + public function testAuthenticateDefaultAllFinder() + { + Configure::write('Auth.authenticate.all.finder', null); + $request = new Request('/?api_key=yyy'); + $result = $this->apiKey->authenticate($request, new Response()); + $this->assertEquals('user-1', $result['username']); + } } From 964fb747d0c64254730d919c45d3381ad38f6b06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20Gonz=C3=A1lez?= Date: Mon, 19 Dec 2016 13:20:43 +0000 Subject: [PATCH 29/29] fix changelog and installation docs --- .semver | 6 +++--- CHANGELOG.md | 6 ++++++ Docs/Documentation/Installation.md | 12 ++++++++++++ 3 files changed, 21 insertions(+), 3 deletions(-) diff --git a/.semver b/.semver index 7048659d7..2b479677e 100644 --- a/.semver +++ b/.semver @@ -1,5 +1,5 @@ --- -:major: 3 -:minor: 2 -:patch: 5 +:major: 4 +:minor: 0 +:patch: 0 :special: '' diff --git a/CHANGELOG.md b/CHANGELOG.md index 4aaae87b8..cb47b90f1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,12 @@ Changelog Releases for CakePHP 3 ------------- +* 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 + * Add `allowed` to manage the AuthLinkHelper when action is allowed + * Add option to configure the api table and finder in ApiKeyAuthenticate + * 3.2.5 * Fixed RegisterBehavior api, make getRegisterValidators public. diff --git a/Docs/Documentation/Installation.md b/Docs/Documentation/Installation.md index 35a6b8967..d303fc9dd 100644 --- a/Docs/Documentation/Installation.md +++ b/Docs/Documentation/Installation.md @@ -34,6 +34,18 @@ composer require google/recaptcha:@stable NOTE: you'll need to configure the reCaptcha key and secret, check the [Configuration](Configuration.md) page for more details. +If you want to use Google Authenticator features... + +``` +composer require robthree/twofactorauth:"^1.5.2" +``` + +NOTE: you'll need to enable `Users.GoogleAuthenticator.login` + +``` +Configure::write('Users.GoogleAuthenticator.login', true); +``` + Creating Required Tables ------------------------ If you want to use the Users tables to store your users and social accounts: