From 4b3308bf3fd4c6fe572ee1658a7809bba20c7339 Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Thu, 20 Dec 2018 11:11:04 +0100 Subject: [PATCH] fix can change password check in case of encryption is enabled Admin should _not_ be able to change password when: - if an encryption module is loaded and it uses per-user keys - if encryption is enabled but no encryption modules are loaded Admin should be able to change the password when: - no encryption module is loaded and encryption is disabled - encryption module is loaded but it doesn't require per user keys Signed-off-by: Bjoern Schiessle --- settings/Controller/UsersController.php | 68 +++++++++++++------ .../Controller/UsersControllerTest.php | 48 ++++++++++++- 2 files changed, 95 insertions(+), 21 deletions(-) diff --git a/settings/Controller/UsersController.php b/settings/Controller/UsersController.php index 12c3e47dfe..55ef267d8b 100644 --- a/settings/Controller/UsersController.php +++ b/settings/Controller/UsersController.php @@ -41,6 +41,7 @@ namespace OC\Settings\Controller; use OC\Accounts\AccountManager; use OC\AppFramework\Http; +use OC\Encryption\Exceptions\ModuleDoesNotExistsException; use OC\ForbiddenException; use OC\Security\IdentityProof\Manager; use OCA\User_LDAP\User_Proxy; @@ -128,9 +129,9 @@ class UsersController extends Controller { /** * @NoCSRFRequired * @NoAdminRequired - * + * * Display users list template - * + * * @return TemplateResponse */ public function usersListByGroup() { @@ -140,9 +141,9 @@ class UsersController extends Controller { /** * @NoCSRFRequired * @NoAdminRequired - * + * * Display users list template - * + * * @return TemplateResponse */ public function usersList() { @@ -150,7 +151,7 @@ class UsersController extends Controller { $uid = $user->getUID(); \OC::$server->getNavigationManager()->setActiveEntry('core_users'); - + /* SORT OPTION: SORT_USERCOUNT or SORT_GROUPNAME */ $sortGroupsBy = \OC\Group\MetaData::SORT_USERCOUNT; $isLDAPUsed = false; @@ -166,22 +167,17 @@ class UsersController extends Controller { } } } - - /* ENCRYPTION CONFIG */ - $isEncryptionEnabled = $this->encryptionManager->isEnabled(); - $useMasterKey = $this->config->getAppValue('encryption', 'useMasterKey', true); - // If masterKey enabled, then you can change password. This is to avoid data loss! - $canChangePassword = ($isEncryptionEnabled && $useMasterKey) || $useMasterKey; - - - /* GROUPS */ + + $canChangePassword = $this->canAdminChangeUserPasswords(); + + /* GROUPS */ $groupsInfo = new \OC\Group\MetaData( $uid, $this->isAdmin, $this->groupManager, $this->userSession ); - + $groupsInfo->setSorting($sortGroupsBy); list($adminGroup, $groups) = $groupsInfo->get(); @@ -190,7 +186,7 @@ class UsersController extends Controller { return $ldapFound || $backend instanceof User_Proxy; }); } - + if ($this->isAdmin) { $disabledUsers = $isLDAPUsed ? -1 : $this->userManager->countDisabledUsers(); $userCount = $isLDAPUsed ? 0 : array_reduce($this->userManager->countUsers(), function($v, $w) { @@ -221,7 +217,7 @@ class UsersController extends Controller { 'name' => 'Disabled users', 'usercount' => $disabledUsers ]; - + /* QUOTAS PRESETS */ $quotaPreset = $this->config->getAppValue('files', 'quota_preset', '1 GB, 5 GB, 10 GB'); $quotaPreset = explode(',', $quotaPreset); @@ -230,12 +226,12 @@ class UsersController extends Controller { } $quotaPreset = array_diff($quotaPreset, array('default', 'none')); $defaultQuota = $this->config->getAppValue('files', 'default_quota', 'none'); - + \OC::$server->getEventDispatcher()->dispatch('OC\Settings\Users::loadAdditionalScripts'); - + /* LANGUAGES */ $languages = $this->l10nFactory->getLanguages(); - + /* FINAL DATA */ $serverData = array(); // groups @@ -254,6 +250,38 @@ class UsersController extends Controller { return new TemplateResponse('settings', 'settings-vue', ['serverData' => $serverData]); } + /** + * check if the admin can change the users password + * + * The admin can change the passwords if: + * + * - no encryption module is loaded and encryption is disabled + * - encryption module is loaded but it doesn't require per user keys + * + * The admin can not change the passwords if: + * + * - an encryption module is loaded and it uses per-user keys + * - encryption is enabled but no encryption modules are loaded + * + * @return bool + */ + protected function canAdminChangeUserPasswords() { + $isEncryptionEnabled = $this->encryptionManager->isEnabled(); + try { + $noUserSpecificEncryptionKeys =!$this->encryptionManager->getEncryptionModule()->needDetailedAccessList(); + $isEncryptionModuleLoaded = true; + } catch (ModuleDoesNotExistsException $e) { + $noUserSpecificEncryptionKeys = true; + $isEncryptionModuleLoaded = false; + } + + $canChangePassword = ($isEncryptionEnabled && $isEncryptionModuleLoaded && $noUserSpecificEncryptionKeys) + || (!$isEncryptionEnabled && !$isEncryptionModuleLoaded) + || (!$isEncryptionEnabled && $isEncryptionModuleLoaded && $noUserSpecificEncryptionKeys); + + return $canChangePassword; + } + /** * @NoAdminRequired * @NoSubadminRequired diff --git a/tests/Settings/Controller/UsersControllerTest.php b/tests/Settings/Controller/UsersControllerTest.php index 74ac990be9..8294514fa5 100644 --- a/tests/Settings/Controller/UsersControllerTest.php +++ b/tests/Settings/Controller/UsersControllerTest.php @@ -11,6 +11,7 @@ namespace Tests\Settings\Controller; use OC\Accounts\AccountManager; +use OC\Encryption\Exceptions\ModuleDoesNotExistsException; use OC\Group\Group; use OC\Group\Manager; use OC\Settings\Controller\UsersController; @@ -98,7 +99,7 @@ class UsersControllerTest extends \Test\TestCase { $this->securityManager = $this->getMockBuilder(\OC\Security\IdentityProof\Manager::class)->disableOriginalConstructor()->getMock(); $this->jobList = $this->createMock(IJobList::class); $this->encryptionManager = $this->createMock(IManager::class); - + $this->l->method('t') ->will($this->returnCallback(function ($text, $parameters = []) { return vsprintf($text, $parameters); @@ -513,4 +514,49 @@ class UsersControllerTest extends \Test\TestCase { $this->assertSame(Http::STATUS_BAD_REQUEST, $result->getStatus()); } + /** + * @dataProvider dataTestCanAdminChangeUserPasswords + * + * @param bool $encryptionEnabled + * @param bool $encryptionModuleLoaded + * @param bool $masterKeyEnabled + * @param bool $expected + */ + public function testCanAdminChangeUserPasswords($encryptionEnabled, + $encryptionModuleLoaded, + $masterKeyEnabled, + $expected) { + $controller = $this->getController(); + + $this->encryptionManager->expects($this->any()) + ->method('isEnabled') + ->willReturn($encryptionEnabled); + $this->encryptionManager->expects($this->any()) + ->method('getEncryptionModule') + ->willReturnCallback(function() use ($encryptionModuleLoaded) { + if ($encryptionModuleLoaded) return $this->encryptionModule; + else throw new ModuleDoesNotExistsException(); + }); + $this->encryptionModule->expects($this->any()) + ->method('needDetailedAccessList') + ->willReturn(!$masterKeyEnabled); + + $result = $this->invokePrivate($controller, 'canAdminChangeUserPasswords', []); + $this->assertSame($expected, $result); + } + + public function dataTestCanAdminChangeUserPasswords() { + return [ + // encryptionEnabled, encryptionModuleLoaded, masterKeyEnabled, expectedResult + [true, true, true, true], + [false, true, true, true], + [true, false, true, false], + [false, false, true, true], + [true, true, false, false], + [false, true, false, false], + [true, false, false, false], + [false, false, false, true], + ]; + } + }