From 8e8e983f28347261424ad33593555ef484ee4f09 Mon Sep 17 00:00:00 2001 From: Marcin Kurczewski Date: Sat, 6 Sep 2014 10:00:26 +0200 Subject: [PATCH] Refactored privilege system --- data/config.ini | 9 +-- public_html/js/Auth.js | 22 +++++- .../js/Presenters/TopNavigationPresenter.js | 2 +- .../js/Presenters/UserListPresenter.js | 1 + public_html/js/Presenters/UserPresenter.js | 6 +- src/Controllers/AuthController.php | 11 +-- src/Controllers/UserController.php | 21 +++--- src/Controllers/ViewProxies/UserViewProxy.php | 16 ++++ src/Helpers/EnumHelper.php | 19 +++++ src/Privilege.php | 7 +- src/Services/AuthService.php | 29 -------- src/Services/PrivilegeService.php | 60 +++++++++++++++ tests/PrivilegeTest.php | 33 +++++++++ tests/Services/AuthServiceTest.php | 3 - tests/Services/PrivilegeServiceTest.php | 74 +++++++++++++++++++ 15 files changed, 247 insertions(+), 66 deletions(-) create mode 100644 src/Helpers/EnumHelper.php create mode 100644 src/Services/PrivilegeService.php create mode 100644 tests/PrivilegeTest.php create mode 100644 tests/Services/PrivilegeServiceTest.php diff --git a/data/config.ini b/data/config.ini index 56393c8e..942ee782 100644 --- a/data/config.ini +++ b/data/config.ini @@ -8,11 +8,10 @@ secret = change minPasswordLength = 5 [security.privileges] -anonymous = register, viewUser -regularUser = listUsers, viewUser, deleteOwnAccount -powerUser = listUsers, viewUser, deleteOwnAccount -moderator = listUsers, viewUser, deleteOwnAccount -administrator = listUsers, viewUser, deleteOwnAccount, deleteUsers +register = anonymous +listUsers = regularUser, powerUser, moderator, administrator +deleteOwnAccount = regularUser, powerUser, moderator, administrator +deleteAllAccounts = administrator [users] minUserNameLength = 1 diff --git a/public_html/js/Auth.js b/public_html/js/Auth.js index 192934a3..4b2dfef4 100644 --- a/public_html/js/Auth.js +++ b/public_html/js/Auth.js @@ -2,6 +2,13 @@ var App = App || {}; App.Auth = function(jQuery, util, api, appState, promise) { + var privileges = { + register: 'register', + listUsers: 'listUsers', + deleteOwnAccount: 'deleteOwnAccount', + deleteAllAccounts: 'deleteAllAccounts', + }; + function loginFromCredentials(userName, password, remember) { return promise.make(function(resolve, reject) { promise.wait(api.post('/login', {userName: userName, password: password})) @@ -79,8 +86,14 @@ App.Auth = function(jQuery, util, api, appState, promise) { appState.set('loggedIn', response.json.user && !!response.json.user.id); } - function isLoggedIn() { - return appState.get('loggedIn'); + function isLoggedIn(userName) { + if (!appState.get('loggedIn')) + return false; + if (typeof(userName) != 'undefined') { + if (getCurrentUser().name != userName) + return false; + } + return true; } function getCurrentUser() { @@ -105,11 +118,14 @@ App.Auth = function(jQuery, util, api, appState, promise) { loginAnonymous: loginAnonymous, tryLoginFromCookie: tryLoginFromCookie, logout: logout, + + startObservingLoginChanges: startObservingLoginChanges, isLoggedIn: isLoggedIn, getCurrentUser: getCurrentUser, getCurrentPrivileges: getCurrentPrivileges, hasPrivilege: hasPrivilege, - startObservingLoginChanges: startObservingLoginChanges, + + privileges: privileges, }; }; diff --git a/public_html/js/Presenters/TopNavigationPresenter.js b/public_html/js/Presenters/TopNavigationPresenter.js index 6c3c011a..b8c35298 100644 --- a/public_html/js/Presenters/TopNavigationPresenter.js +++ b/public_html/js/Presenters/TopNavigationPresenter.js @@ -33,7 +33,7 @@ App.Presenters.TopNavigationPresenter = function( $el.html(template({ loggedIn: auth.isLoggedIn(), user: auth.getCurrentUser(), - canListUsers: auth.hasPrivilege('listUsers') + canListUsers: auth.hasPrivilege(auth.privileges.listUsers) })); $el.find('li.' + selectedElement).addClass('active'); }; diff --git a/public_html/js/Presenters/UserListPresenter.js b/public_html/js/Presenters/UserListPresenter.js index 756e88a7..5c299034 100644 --- a/public_html/js/Presenters/UserListPresenter.js +++ b/public_html/js/Presenters/UserListPresenter.js @@ -5,6 +5,7 @@ App.Presenters.UserListPresenter = function( jQuery, util, promise, + auth, router, pagedCollectionPresenter, topNavigationPresenter, diff --git a/public_html/js/Presenters/UserPresenter.js b/public_html/js/Presenters/UserPresenter.js index 987468a4..f6c4b38a 100644 --- a/public_html/js/Presenters/UserPresenter.js +++ b/public_html/js/Presenters/UserPresenter.js @@ -21,7 +21,7 @@ App.Presenters.UserPresenter = function( function init(args) { userName = args.userName; - topNavigationPresenter.select(auth.isLoggedIn() && auth.getCurrentUser().name == userName ? 'my-account' : 'users'); + topNavigationPresenter.select(auth.isLoggedIn(userName) ? 'my-account' : 'users'); promise.waitAll( util.promiseTemplate('user'), @@ -51,8 +51,8 @@ App.Presenters.UserPresenter = function( function render() { var context = { user: user, - canDeleteAccount: auth.hasPrivilege('deleteAccounts') || - (auth.hasPrivilege('deleteOwnAccount') && auth.getCurrentUser().name == userName), + canDeleteAccount: auth.hasPrivilege(auth.privileges.deleteAllAccounts) || + (auth.isLoggedIn(userName) && auth.hasPrivilege(auth.privileges.deleteOwnAccount)), }; $el.html(template(context)); $el.find('.browsing-settings').html(browsingSettingsTemplate(context)); diff --git a/src/Controllers/AuthController.php b/src/Controllers/AuthController.php index 9b753d04..85cd8ae5 100644 --- a/src/Controllers/AuthController.php +++ b/src/Controllers/AuthController.php @@ -4,23 +4,20 @@ namespace Szurubooru\Controllers; final class AuthController extends AbstractController { private $authService; - private $userService; - private $passwordService; + private $privilegeService; private $inputReader; private $userViewProxy; private $tokenViewProxy; public function __construct( \Szurubooru\Services\AuthService $authService, - \Szurubooru\Services\UserService $userService, - \Szurubooru\Services\PasswordService $passwordService, + \Szurubooru\Services\PrivilegeService $privilegeService, \Szurubooru\Helpers\InputReader $inputReader, \Szurubooru\Controllers\ViewProxies\UserViewProxy $userViewProxy, \Szurubooru\Controllers\ViewProxies\TokenViewProxy $tokenViewProxy) { $this->authService = $authService; - $this->userService = $userService; - $this->passwordService = $passwordService; + $this->privilegeService = $privilegeService; $this->inputReader = $inputReader; $this->userViewProxy = $userViewProxy; $this->tokenViewProxy = $tokenViewProxy; @@ -51,7 +48,7 @@ final class AuthController extends AbstractController [ 'token' => $this->tokenViewProxy->fromEntity($this->authService->getLoginToken()), 'user' => $this->userViewProxy->fromEntity($this->authService->getLoggedInUser()), - 'privileges' => $this->authService->getCurrentPrivileges(), + 'privileges' => $this->privilegeService->getCurrentPrivileges(), ]; } } diff --git a/src/Controllers/UserController.php b/src/Controllers/UserController.php index 782f0317..a8d466d3 100644 --- a/src/Controllers/UserController.php +++ b/src/Controllers/UserController.php @@ -3,18 +3,18 @@ namespace Szurubooru\Controllers; final class UserController extends AbstractController { - private $authService; + private $privilegeService; private $userService; private $inputReader; private $userViewProxy; public function __construct( - \Szurubooru\Services\AuthService $authService, + \Szurubooru\Services\PrivilegeService $privilegeService, \Szurubooru\Services\UserService $userService, \Szurubooru\Helpers\InputReader $inputReader, \Szurubooru\Controllers\ViewProxies\UserViewProxy $userViewProxy) { - $this->authService = $authService; + $this->privilegeService = $privilegeService; $this->userService = $userService; $this->inputReader = $inputReader; $this->userViewProxy = $userViewProxy; @@ -31,8 +31,6 @@ final class UserController extends AbstractController public function getByName($name) { - $this->authService->assertPrivilege(\Szurubooru\Privilege::PRIVILEGE_VIEW_USER); - $user = $this->userService->getByName($name); if (!$user) throw new \DomainException('User with name "' . $name . '" was not found.'); @@ -41,7 +39,7 @@ final class UserController extends AbstractController public function getFiltered() { - $this->authService->assertPrivilege(\Szurubooru\Privilege::PRIVILEGE_LIST_USERS); + $this->privilegeService->assertPrivilege(\Szurubooru\Privilege::PRIVILEGE_LIST_USERS); $searchFormData = new \Szurubooru\FormData\SearchFormData($this->inputReader); $searchResult = $this->userService->getFiltered($searchFormData); @@ -54,7 +52,7 @@ final class UserController extends AbstractController public function register() { - $this->authService->assertPrivilege(\Szurubooru\Privilege::PRIVILEGE_REGISTER); + $this->privilegeService->assertPrivilege(\Szurubooru\Privilege::PRIVILEGE_REGISTER); $input = new \Szurubooru\FormData\RegistrationFormData($this->inputReader); $user = $this->userService->register($input); @@ -68,10 +66,11 @@ final class UserController extends AbstractController public function delete($name) { - if ($name == $this->authService->getLoggedInUser()->name) - $this->authService->assertPrivilege(\Szurubooru\Privilege::PRIVILEGE_DELETE_OWN_ACCOUNT); - else - $this->authService->assertPrivilege(\Szurubooru\Privilege::PRIVILEGE_DELETE_ACCOUNTS); + $this->privilegeService->assertPrivilege( + $this->privilegeService->isLoggedIn($name) + ? \Szurubooru\Privilege::PRIVILEGE_DELETE_OWN_ACCOUNT + : \Szurubooru\Privilege::PRIVILEGE_DELETE_ACCOUNTS); + return $this->userService->deleteByName($name); } } diff --git a/src/Controllers/ViewProxies/UserViewProxy.php b/src/Controllers/ViewProxies/UserViewProxy.php index a1f21525..6aeb9e3c 100644 --- a/src/Controllers/ViewProxies/UserViewProxy.php +++ b/src/Controllers/ViewProxies/UserViewProxy.php @@ -3,6 +3,13 @@ namespace Szurubooru\Controllers\ViewProxies; class UserViewProxy extends AbstractViewProxy { + private $privilegeService; + + public function __construct(\Szurubooru\Services\PrivilegeService $privilegeService) + { + $this->privilegeService = $privilegeService; + } + public function fromEntity($user) { $result = new \StdClass; @@ -10,6 +17,15 @@ class UserViewProxy extends AbstractViewProxy { $result->id = $user->id; $result->name = $user->name; + $result->accessRank = \Szurubooru\Helpers\EnumHelper::accessRankToString($user->accessRank); + $result->registrationTime = $user->registrationTime; + $result->lastLoginTime = $user->lastLoginTime; + + if ($this->privilegeService->hasPrivilege(\Szurubooru\Privilege::PRIVILEGE_VIEW_ALL_EMAIL_ADDRESSES) or + $this->privilegeService->isLoggedIn($user)) + { + $result->email = $user->email; + } } return $result; } diff --git a/src/Helpers/EnumHelper.php b/src/Helpers/EnumHelper.php new file mode 100644 index 00000000..3e515200 --- /dev/null +++ b/src/Helpers/EnumHelper.php @@ -0,0 +1,19 @@ +loggedInUser = $this->getAnonymousUser(); $this->validator = $validator; - $this->config = $config; $this->passwordService = $passwordService; $this->timeService = $timeService; $this->tokenDao = $tokenDao; @@ -108,32 +105,6 @@ class AuthService $this->loginToken = null; } - public function getCurrentPrivileges() - { - switch ($this->getLoggedInUser()->accessRank) - { - case \Szurubooru\Entities\User::ACCESS_RANK_ANONYMOUS: $keyName = 'anonymous'; break; - case \Szurubooru\Entities\User::ACCESS_RANK_REGULAR_USER: $keyName = 'regularUser'; break; - case \Szurubooru\Entities\User::ACCESS_RANK_POWER_USER: $keyName = 'powerUser'; break; - case \Szurubooru\Entities\User::ACCESS_RANK_MODERATOR: $keyName = 'moderator'; break; - case \Szurubooru\Entities\User::ACCESS_RANK_ADMINISTRATOR: $keyName = 'administrator'; break; - default: - throw new \DomainException('Invalid access rank!'); - } - return array_filter(preg_split('/[;,\s]+/', $this->config->security->privileges[$keyName])); - } - - public function hasPrivilege($privilege) - { - return in_array($privilege, $this->getCurrentPrivileges()); - } - - public function assertPrivilege($privilege) - { - if (!$this->hasPrivilege($privilege)) - throw new \DomainException('Unprivileged operation'); - } - private function createAndSaveLoginToken(\Szurubooru\Entities\User $user) { $loginToken = new \Szurubooru\Entities\Token(); diff --git a/src/Services/PrivilegeService.php b/src/Services/PrivilegeService.php new file mode 100644 index 00000000..112591b3 --- /dev/null +++ b/src/Services/PrivilegeService.php @@ -0,0 +1,60 @@ +authService = $authService; + + if (isset($config->security->privileges)) + { + foreach ($config->security->privileges as $privilegeName => $allowedAccessRanks) + { + $allowedAccessRanks = array_filter(preg_split('/[;,\s]+/', $allowedAccessRanks)); + foreach ($allowedAccessRanks as $allowedAccessRank) + { + if (!isset($this->privilegeMap[$allowedAccessRank])) + $this->privilegeMap[$allowedAccessRank] = []; + $this->privilegeMap[$allowedAccessRank] []= $privilegeName; + } + } + } + } + + public function getCurrentPrivileges() + { + $currentAccessRank = $this->authService->getLoggedInUser()->accessRank; + $currentAccessRankName = \Szurubooru\Helpers\EnumHelper::accessRankToString($currentAccessRank); + if (!isset($this->privilegeMap[$currentAccessRankName])) + return []; + return $this->privilegeMap[$currentAccessRankName]; + } + + public function hasPrivilege($privilege) + { + return in_array($privilege, $this->getCurrentPrivileges()); + } + + public function assertPrivilege($privilege) + { + if (!$this->hasPrivilege($privilege)) + throw new \DomainException('Unprivileged operation'); + } + + public function isLoggedIn($userIdentifier) + { + $loggedInUser = $this->authService->getLoggedInUser(); + if ($userIdentifier instanceof \Szurubooru\Entities\User) + return $loggedInUser->name == $userIdentifier->name; + elseif (is_string($userIdentifier)) + return $loggedInUser->name == $userIdentifier; + else + throw new \InvalidArgumentException('Invalid user identifier.'); + } +} diff --git a/tests/PrivilegeTest.php b/tests/PrivilegeTest.php new file mode 100644 index 00000000..7c578782 --- /dev/null +++ b/tests/PrivilegeTest.php @@ -0,0 +1,33 @@ +getConstants() as $key => $value) + { + $value = strtoupper('privilege_' . ltrim(preg_replace('/[A-Z]/', '_\0', $value), '_')); + $this->assertEquals($key, $value); + } + } + + public function testConfigSectionNaming() + { + $refl = new \ReflectionClass(\Szurubooru\Privilege::class); + $constants = array_values($refl->getConstants()); + + $configPath = __DIR__ + . DIRECTORY_SEPARATOR . '..' + . DIRECTORY_SEPARATOR . 'data' + . DIRECTORY_SEPARATOR . 'config.ini'; + + $config = new \Szurubooru\Config(); + $config->loadFromIni($configPath); + foreach ($config->security->privileges as $key => $value) + { + $this->assertTrue(in_array($key, $constants), "$key not in constants"); + } + } +} diff --git a/tests/Services/AuthServiceTest.php b/tests/Services/AuthServiceTest.php index 5f3173ad..796a0130 100644 --- a/tests/Services/AuthServiceTest.php +++ b/tests/Services/AuthServiceTest.php @@ -4,7 +4,6 @@ namespace Szurubooru\Tests\Services; class AuthServiceTest extends \Szurubooru\Tests\AbstractTestCase { private $validatorMock; - private $configMock; private $passwordServiceMock; private $timeServiceMock; private $tokenDaoMock; @@ -13,7 +12,6 @@ class AuthServiceTest extends \Szurubooru\Tests\AbstractTestCase public function setUp() { $this->validatorMock = $this->mock(\Szurubooru\Validator::class); - $this->configMock = $this->mockConfig(); $this->passwordServiceMock = $this->mock(\Szurubooru\Services\PasswordService::class); $this->timeServiceMock = $this->mock(\Szurubooru\Services\TimeService::class); $this->tokenDaoMock = $this->mock(\Szurubooru\Dao\TokenDao::class); @@ -97,7 +95,6 @@ class AuthServiceTest extends \Szurubooru\Tests\AbstractTestCase { return new \Szurubooru\Services\AuthService( $this->validatorMock, - $this->configMock, $this->passwordServiceMock, $this->timeServiceMock, $this->tokenDaoMock, diff --git a/tests/Services/PrivilegeServiceTest.php b/tests/Services/PrivilegeServiceTest.php new file mode 100644 index 00000000..6caa02b3 --- /dev/null +++ b/tests/Services/PrivilegeServiceTest.php @@ -0,0 +1,74 @@ +configMock = $this->mockConfig(); + $this->authServiceMock = $this->mock(\Szurubooru\Services\AuthService::class); + } + + public function testReadingConfig() + { + $testUser = new \Szurubooru\Entities\User(); + $testUser->name = 'dummy'; + $testUser->accessRank = \Szurubooru\Entities\User::ACCESS_RANK_POWER_USER; + $this->authServiceMock->method('getLoggedInUser')->willReturn($testUser); + + $privilege = \Szurubooru\Privilege::PRIVILEGE_LIST_USERS; + $this->configMock->set('security/privileges/' . $privilege, 'powerUser'); + + $privilegeService = $this->getPrivilegeService(); + $this->assertEquals([$privilege], $privilegeService->getCurrentPrivileges()); + $this->assertTrue($privilegeService->hasPrivilege($privilege)); + } + + public function testIsLoggedInByString() + { + $testUser1 = new \Szurubooru\Entities\User(); + $testUser1->name = 'dummy'; + $testUser2 = new \Szurubooru\Entities\User(); + $testUser2->name = 'godzilla'; + $this->authServiceMock->method('getLoggedInUser')->willReturn($testUser1); + + $privilegeService = $this->getPrivilegeService(); + $this->assertTrue($privilegeService->isLoggedIn($testUser1->name)); + $this->assertFalse($privilegeService->isLoggedIn($testUser2->name)); + } + + public function testIsLoggedInByUser() + { + $testUser1 = new \Szurubooru\Entities\User(); + $testUser1->name = 'dummy'; + $testUser2 = new \Szurubooru\Entities\User(); + $testUser2->name = 'godzilla'; + $this->authServiceMock->method('getLoggedInUser')->willReturn($testUser1); + + $privilegeService = $this->getPrivilegeService(); + $this->assertTrue($privilegeService->isLoggedIn($testUser1)); + $this->assertFalse($privilegeService->isLoggedIn($testUser2)); + } + + public function testIsLoggedInByInvalidObject() + { + $testUser = new \Szurubooru\Entities\User(); + $testUser->name = 'dummy'; + $this->authServiceMock->method('getLoggedInUser')->willReturn($testUser); + + $rubbish = new \StdClass; + $privilegeService = $this->getPrivilegeService(); + $this->setExpectedException(\InvalidArgumentException::class); + $this->assertTrue($privilegeService->isLoggedIn($rubbish)); + } + + private function getPrivilegeService() + { + return new \Szurubooru\Services\PrivilegeService( + $this->configMock, + $this->authServiceMock); + } +}