From 9fbeaf0fd9f3ba4cd01aa566553cb2373dde8cb2 Mon Sep 17 00:00:00 2001 From: Morris Jobke Date: Fri, 23 Jan 2015 17:45:45 +0100 Subject: Add value if restore of data is possible for a user * reason: nice to know before password change in user management * restore is possible: * encryption is disabled * encryption is enabled, admin and user has checked the restore option * if not possible: * highlight users row in red once the admin wants to change the password * show also a little tipsy --- settings/application.php | 3 +- settings/controller/userscontroller.php | 53 +++++- settings/css/settings.css | 5 +- settings/js/users/users.js | 17 +- tests/settings/controller/userscontrollertest.php | 193 +++++++++++++++++++--- 5 files changed, 235 insertions(+), 36 deletions(-) diff --git a/settings/application.php b/settings/application.php index f7ba72f3bfc..d5516a1eefd 100644 --- a/settings/application.php +++ b/settings/application.php @@ -90,7 +90,8 @@ class Application extends App { $c->query('Defaults'), $c->query('Mail'), $c->query('DefaultMailAddress'), - $c->query('URLGenerator') + $c->query('URLGenerator'), + $c->query('OCP\\App\\IAppManager') ); }); $container->registerService('LogSettingsController', function(IContainer $c) { diff --git a/settings/controller/userscontroller.php b/settings/controller/userscontroller.php index 1be2f4db9b9..be1b26f86ad 100644 --- a/settings/controller/userscontroller.php +++ b/settings/controller/userscontroller.php @@ -11,9 +11,9 @@ namespace OC\Settings\Controller; use OC\AppFramework\Http; -use OC\User\Manager; use OC\User\User; -use \OCP\AppFramework\Controller; +use OCP\App\IAppManager; +use OCP\AppFramework\Controller; use OCP\AppFramework\Http\DataResponse; use OCP\AppFramework\Http\TemplateResponse; use OCP\IConfig; @@ -52,6 +52,10 @@ class UsersController extends Controller { private $fromMailAddress; /** @var IURLGenerator */ private $urlGenerator; + /** @var bool contains the state of the encryption app */ + private $isEncryptionAppEnabled; + /** @var bool contains the state of the admin recovery setting */ + private $isRestoreEnabled = false; /** * @param string $appName @@ -66,6 +70,7 @@ class UsersController extends Controller { * @param \OC_Defaults $defaults * @param \OC_Mail $mail * @param string $fromMailAddress + * @param IAppManager $appManager */ public function __construct($appName, IRequest $request, @@ -79,7 +84,8 @@ class UsersController extends Controller { \OC_Defaults $defaults, \OC_Mail $mail, $fromMailAddress, - IURLGenerator $urlGenerator) { + IURLGenerator $urlGenerator, + IAppManager $appManager) { parent::__construct($appName, $request); $this->userManager = $userManager; $this->groupManager = $groupManager; @@ -92,6 +98,14 @@ class UsersController extends Controller { $this->mail = $mail; $this->fromMailAddress = $fromMailAddress; $this->urlGenerator = $urlGenerator; + + // check for encryption state - TODO see formatUserForIndex + $this->isEncryptionAppEnabled = $appManager->isEnabledForUser('files_encryption'); + if($this->isEncryptionAppEnabled) { + // putting this directly in empty is possible in PHP 5.5+ + $result = $config->getAppValue('files_encryption', 'recoveryAdminEnabled', 0); + $this->isRestoreEnabled = !empty($result); + } } /** @@ -100,7 +114,33 @@ class UsersController extends Controller { * @return array */ private function formatUserForIndex(IUser $user, array $userGroups = null) { - return array( + + // TODO: eliminate this encryption specific code below and somehow + // hook in additional user info from other apps + + // recovery isn't possible if admin or user has it disabled and encryption + // is enabled - so we eliminate the else paths in the conditional tree + // below + $restorePossible = false; + + if ($this->isEncryptionAppEnabled) { + if ($this->isRestoreEnabled) { + // check for the users recovery setting + $recoveryMode = $this->config->getUserValue($user->getUID(), 'files_encryption', 'recovery_enabled', '0'); + // method call inside empty is possible with PHP 5.5+ + $recoveryModeEnabled = !empty($recoveryMode); + if ($recoveryModeEnabled) { + // user also has recovery mode enabled + $restorePossible = true; + } + } + } else { + // recovery is possible if encryption is disabled (plain files are + // available) + $restorePossible = true; + } + + return [ 'name' => $user->getUID(), 'displayname' => $user->getDisplayName(), 'groups' => (empty($userGroups)) ? $this->groupManager->getUserGroupIds($user) : $userGroups, @@ -109,8 +149,9 @@ class UsersController extends Controller { 'storageLocation' => $user->getHome(), 'lastLogin' => $user->getLastLogin(), 'backend' => $user->getBackendClassName(), - 'email' => $this->config->getUserValue($user->getUID(), 'settings', 'email', '') - ); + 'email' => $this->config->getUserValue($user->getUID(), 'settings', 'email', ''), + 'isRestoreDisabled' => !$restorePossible, + ]; } /** diff --git a/settings/css/settings.css b/settings/css/settings.css index 55367e716c2..57edc18bd9a 100644 --- a/settings/css/settings.css +++ b/settings/css/settings.css @@ -132,7 +132,10 @@ input.userFilter {width: 200px;} .ie8 table.hascontrols{border-collapse:collapse;width: 100%;} .ie8 table.hascontrols tbody tr{border-collapse:collapse;border: 1px solid #ddd !important;} - +/* used to highlight a user row in red */ +#userlist tr.row-warning { + background-color: #FDD; +} /* APPS */ diff --git a/settings/js/users/users.js b/settings/js/users/users.js index f21c660b41f..1a755ab7b25 100644 --- a/settings/js/users/users.js +++ b/settings/js/users/users.js @@ -42,6 +42,7 @@ var UserList = { * 'lastLogin': '1418632333' * 'backend': 'LDAP', * 'email': 'username@example.org' + * 'isRestoreDisabled':false * } * @param sort * @returns table row created for this user @@ -63,11 +64,12 @@ var UserList = { } /** - * add username and displayname to row (in data and visible markup + * add username and displayname to row (in data and visible markup) */ $tr.data('uid', user.name); $tr.data('displayname', user.displayname); $tr.data('mailAddress', user.email); + $tr.data('restoreDisabled', user.isRestoreDisabled); $tr.find('td.name').text(user.name); $tr.find('td.displayName > span').text(user.displayname); $tr.find('td.mailAddress > span').text(user.email); @@ -352,6 +354,9 @@ var UserList = { getMailAddress: function(element) { return ($(element).closest('tr').data('mailAddress') || '').toString(); }, + getRestoreDisabled: function(element) { + return ($(element).closest('tr').data('restoreDisabled') || ''); + }, initDeleteHandling: function() { //set up handler UserDeleteHandler = new DeleteHandler('/settings/users/users', 'username', @@ -627,8 +632,16 @@ $(document).ready(function () { event.stopPropagation(); var $td = $(this).closest('td'); + var $tr = $(this).closest('tr'); var uid = UserList.getUID($td); var $input = $(''); + var isRestoreDisabled = UserList.getRestoreDisabled($td) === true; + if(isRestoreDisabled) { + $tr.addClass('row-warning'); + // add tipsy if the password change could cause data loss - no recovery enabled + $input.tipsy({gravity:'s', fade:false}); + $input.attr('title', t('settings', 'Changing the password will result in data loss, because data recovery is not available for this user')); + } $td.find('img').hide(); $td.children('span').replaceWith($input); $input @@ -655,6 +668,8 @@ $(document).ready(function () { .blur(function () { $(this).replaceWith($('●●●●●●●')); $td.find('img').show(); + // remove highlight class from users without recovery ability + $tr.removeClass('row-warning'); }); }); $('input:password[id="recoveryPassword"]').keyup(function() { diff --git a/tests/settings/controller/userscontrollertest.php b/tests/settings/controller/userscontrollertest.php index c6506ee440b..7dc2d066a5c 100644 --- a/tests/settings/controller/userscontrollertest.php +++ b/tests/settings/controller/userscontrollertest.php @@ -21,9 +21,6 @@ class UsersControllerTest extends \Test\TestCase { /** @var \OCP\AppFramework\IAppContainer */ private $container; - /** @var UsersController */ - private $usersController; - protected function setUp() { $app = new Application(); $this->container = $app->getContainer(); @@ -54,9 +51,8 @@ class UsersControllerTest extends \Test\TestCase { ->disableOriginalConstructor()->getMock(); $this->container['URLGenerator'] = $this->getMockBuilder('\OCP\IURLGenerator') ->disableOriginalConstructor()->getMock(); - - $this->usersController = $this->container['UsersController']; - + $this->container['OCP\\App\\IAppManager'] = $this->getMockBuilder('OCP\\App\\IAppManager') + ->disableOriginalConstructor()->getMock(); } /** @@ -169,7 +165,8 @@ class UsersControllerTest extends \Test\TestCase { 'storageLocation' => '/home/foo', 'lastLogin' => 500, 'backend' => 'OC_User_Database', - 'email' => 'foo@bar.com' + 'email' => 'foo@bar.com', + 'isRestoreDisabled' => false, ), 1 => array( 'name' => 'admin', @@ -180,7 +177,8 @@ class UsersControllerTest extends \Test\TestCase { 'storageLocation' => '/home/admin', 'lastLogin' => 12, 'backend' => 'OC_User_Dummy', - 'email' => 'admin@bar.com' + 'email' => 'admin@bar.com', + 'isRestoreDisabled' => false, ), 2 => array( 'name' => 'bar', @@ -191,11 +189,12 @@ class UsersControllerTest extends \Test\TestCase { 'storageLocation' => '/home/bar', 'lastLogin' => 3999, 'backend' => 'OC_User_Dummy', - 'email' => 'bar@dummy.com' + 'email' => 'bar@dummy.com', + 'isRestoreDisabled' => false, ), ) ); - $response = $this->usersController->index(0, 10, 'gid', 'pattern'); + $response = $this->container['UsersController']->index(0, 10, 'gid', 'pattern'); $this->assertEquals($expectedResponse, $response); } @@ -294,7 +293,8 @@ class UsersControllerTest extends \Test\TestCase { 'storageLocation' => '/home/foo', 'lastLogin' => 500, 'backend' => 'OC_User_Database', - 'email' => 'foo@bar.com' + 'email' => 'foo@bar.com', + 'isRestoreDisabled' => false, ), 1 => array( 'name' => 'admin', @@ -305,7 +305,8 @@ class UsersControllerTest extends \Test\TestCase { 'storageLocation' => '/home/admin', 'lastLogin' => 12, 'backend' => 'OC_User_Dummy', - 'email' => 'admin@bar.com' + 'email' => 'admin@bar.com', + 'isRestoreDisabled' => false, ), 2 => array( 'name' => 'bar', @@ -316,11 +317,12 @@ class UsersControllerTest extends \Test\TestCase { 'storageLocation' => '/home/bar', 'lastLogin' => 3999, 'backend' => 'OC_User_Dummy', - 'email' => 'bar@dummy.com' + 'email' => 'bar@dummy.com', + 'isRestoreDisabled' => false, ), ) ); - $response = $this->usersController->index(0, 10, '', 'pattern'); + $response = $this->container['UsersController']->index(0, 10, '', 'pattern'); $this->assertEquals($expectedResponse, $response); } @@ -374,11 +376,12 @@ class UsersControllerTest extends \Test\TestCase { 'storageLocation' => '/home/foo', 'lastLogin' => 500, 'backend' => 'OC_User_Database', - 'email' => null + 'email' => null, + 'isRestoreDisabled' => false, ) ) ); - $response = $this->usersController->index(0, 10, '','', 'OC_User_Dummy'); + $response = $this->container['UsersController']->index(0, 10, '','', 'OC_User_Dummy'); $this->assertEquals($expectedResponse, $response); } @@ -394,7 +397,7 @@ class UsersControllerTest extends \Test\TestCase { ->will($this->returnValue([])); $expectedResponse = new DataResponse([]); - $response = $this->usersController->index(0, 10, '','', 'OC_User_Dummy'); + $response = $this->container['UsersController']->index(0, 10, '','', 'OC_User_Dummy'); $this->assertEquals($expectedResponse, $response); } @@ -432,11 +435,12 @@ class UsersControllerTest extends \Test\TestCase { 'displayname' => null, 'quota' => null, 'subadmin' => array(), - 'email' => null + 'email' => null, + 'isRestoreDisabled' => false, ), Http::STATUS_CREATED ); - $response = $this->usersController->create('foo', 'password', array()); + $response = $this->container['UsersController']->create('foo', 'password', array()); $this->assertEquals($expectedResponse, $response); } @@ -502,11 +506,12 @@ class UsersControllerTest extends \Test\TestCase { 'displayname' => null, 'quota' => null, 'subadmin' => array(), - 'email' => null + 'email' => null, + 'isRestoreDisabled' => false, ), Http::STATUS_CREATED ); - $response = $this->usersController->create('foo', 'password', array('NewGroup', 'ExistingGroup')); + $response = $this->container['UsersController']->create('foo', 'password', array('NewGroup', 'ExistingGroup')); $this->assertEquals($expectedResponse, $response); } @@ -525,7 +530,7 @@ class UsersControllerTest extends \Test\TestCase { ), Http::STATUS_FORBIDDEN ); - $response = $this->usersController->create('foo', 'password', array()); + $response = $this->container['UsersController']->create('foo', 'password', array()); $this->assertEquals($expectedResponse, $response); } @@ -553,7 +558,7 @@ class UsersControllerTest extends \Test\TestCase { ), Http::STATUS_FORBIDDEN ); - $response = $this->usersController->destroy('myself'); + $response = $this->container['UsersController']->destroy('myself'); $this->assertEquals($expectedResponse, $response); } @@ -591,7 +596,7 @@ class UsersControllerTest extends \Test\TestCase { ), Http::STATUS_NO_CONTENT ); - $response = $this->usersController->destroy('UserToDelete'); + $response = $this->container['UsersController']->destroy('UserToDelete'); $this->assertEquals($expectedResponse, $response); } /** @@ -628,7 +633,7 @@ class UsersControllerTest extends \Test\TestCase { ), Http::STATUS_FORBIDDEN ); - $response = $this->usersController->destroy('UserToDelete'); + $response = $this->container['UsersController']->destroy('UserToDelete'); $this->assertEquals($expectedResponse, $response); } @@ -653,7 +658,7 @@ class UsersControllerTest extends \Test\TestCase { ), Http::STATUS_UNPROCESSABLE_ENTITY ); - $response = $this->usersController->create('foo', 'password', array(), 'invalidMailAdress'); + $response = $this->container['UsersController']->create('foo', 'password', array(), 'invalidMailAdress'); $this->assertEquals($expectedResponse, $response); } @@ -688,8 +693,142 @@ class UsersControllerTest extends \Test\TestCase { ->expects($this->never()) ->method('error'); - $response = $this->usersController->create('foo', 'password', array(), 'validMail@Adre.ss'); + $response = $this->container['UsersController']->create('foo', 'password', array(), 'validMail@Adre.ss'); $this->assertEquals(Http::STATUS_CREATED, $response->getStatus()); } + private function mockUser($userId = 'foo', $displayName = 'M. Foo', + $lastLogin = 500, $home = '/home/foo', $backend = 'OC_User_Database') { + $user = $this->getMockBuilder('\OC\User\User') + ->disableOriginalConstructor()->getMock(); + $user + ->expects($this->any()) + ->method('getUID') + ->will($this->returnValue($userId)); + $user + ->expects($this->once()) + ->method('getDisplayName') + ->will($this->returnValue($displayName)); + $user + ->method('getLastLogin') + ->will($this->returnValue($lastLogin)); + $user + ->method('getHome') + ->will($this->returnValue($home)); + $user + ->expects($this->once()) + ->method('getBackendClassName') + ->will($this->returnValue($backend)); + + $result = [ + 'name' => $userId, + 'displayname' => $displayName, + 'groups' => null, + 'subadmin' => array(), + 'quota' => null, + 'storageLocation' => $home, + 'lastLogin' => $lastLogin, + 'backend' => $backend, + 'email' => null, + 'isRestoreDisabled' => false, + ]; + + return [$user, $result]; + } + + public function testRestorePossibleWithoutEncryption() { + list($user, $expectedResult) = $this->mockUser(); + + $result = \Test_Helper::invokePrivate($this->container['UsersController'], 'formatUserForIndex', [$user]); + $this->assertEquals($expectedResult, $result); + } + + public function testRestorePossibleWithAdminAndUserRestore() { + list($user, $expectedResult) = $this->mockUser(); + + $this->container['OCP\\App\\IAppManager'] + ->expects($this->once()) + ->method('isEnabledForUser') + ->with( + $this->equalTo('files_encryption') + ) + ->will($this->returnValue(true)); + $this->container['Config'] + ->expects($this->once()) + ->method('getAppValue') + ->with( + $this->equalTo('files_encryption'), + $this->equalTo('recoveryAdminEnabled'), + $this->anything() + ) + ->will($this->returnValue('1')); + + $this->container['Config'] + ->expects($this->at(1)) + ->method('getUserValue') + ->with( + $this->anything(), + $this->equalTo('files_encryption'), + $this->equalTo('recovery_enabled'), + $this->anything() + ) + ->will($this->returnValue('1')); + + $result = \Test_Helper::invokePrivate($this->container['UsersController'], 'formatUserForIndex', [$user]); + $this->assertEquals($expectedResult, $result); + } + + public function testRestoreNotPossibleWithoutAdminRestore() { + list($user, $expectedResult) = $this->mockUser(); + + $this->container['OCP\\App\\IAppManager'] + ->method('isEnabledForUser') + ->with( + $this->equalTo('files_encryption') + ) + ->will($this->returnValue(true)); + + $expectedResult['isRestoreDisabled'] = true; + + $result = \Test_Helper::invokePrivate($this->container['UsersController'], 'formatUserForIndex', [$user]); + $this->assertEquals($expectedResult, $result); + } + + public function testRestoreNotPossibleWithoutUserRestore() { + list($user, $expectedResult) = $this->mockUser(); + + $this->container['OCP\\App\\IAppManager'] + ->expects($this->once()) + ->method('isEnabledForUser') + ->with( + $this->equalTo('files_encryption') + ) + ->will($this->returnValue(true)); + $this->container['Config'] + ->expects($this->once()) + ->method('getAppValue') + ->with( + $this->equalTo('files_encryption'), + $this->equalTo('recoveryAdminEnabled'), + $this->anything() + ) + ->will($this->returnValue('1')); + + $this->container['Config'] + ->expects($this->at(1)) + ->method('getUserValue') + ->with( + $this->anything(), + $this->equalTo('files_encryption'), + $this->equalTo('recovery_enabled'), + $this->anything() + ) + ->will($this->returnValue('0')); + + $expectedResult['isRestoreDisabled'] = true; + + $result = \Test_Helper::invokePrivate($this->container['UsersController'], 'formatUserForIndex', [$user]); + $this->assertEquals($expectedResult, $result); + } + } -- cgit v1.2.3