From 637c75bca176d3ef7a06e8b4fa2d60fece1c89a7 Mon Sep 17 00:00:00 2001 From: Thomas Pulzer Date: Fri, 8 Jul 2016 13:22:34 +0200 Subject: [PATCH 01/18] Implemented visual feedback if a user is disabled in admin user menu. Implemented visuals for enabling/disabling user from admin user list. Added the controller functions for enabling/disabling a user. Added the route for changing user status (enabled/disabled) and added an additional route handler in the user controller. Finished the visuals to reflect current user status and changed user status respectively. Changed the single icon for enabling/disabling a user into a menu where deletion and state toggling of a user is selectable. Added displaying of disabled user count. Improved style of user action menu. Added proper counting of disabled users. Removed visual indicator for disabled users. Moved pseudo-group detection for disabled users from frontend to the controller. Changed units for newly introduced css values from em to px. Removed unnecessary png and optimized svg with scour. Changed the userlist template to display the user action menu with correct width. Style fixes for better readability and coding style conformity. Changed the icons for enabling, disabling and deleting a user in the action menu. --- core/img/actions/user-plus.svg | 12 ++ core/img/actions/user-times.svg | 12 ++ settings/Controller/UsersController.php | 154 ++++++++++++++++++-- settings/css/settings.css | 45 +++++- settings/js/users/groups.js | 17 ++- settings/js/users/users.js | 84 ++++++++--- settings/routes.php | 1 + settings/templates/users/part.grouplist.php | 9 ++ settings/templates/users/part.userlist.php | 18 ++- settings/users.php | 15 ++ 10 files changed, 325 insertions(+), 42 deletions(-) create mode 100644 core/img/actions/user-plus.svg create mode 100644 core/img/actions/user-times.svg diff --git a/core/img/actions/user-plus.svg b/core/img/actions/user-plus.svg new file mode 100644 index 0000000000..c236d9fc1d --- /dev/null +++ b/core/img/actions/user-plus.svg @@ -0,0 +1,12 @@ + + + + + +image/svg+xml + + + + + + diff --git a/core/img/actions/user-times.svg b/core/img/actions/user-times.svg new file mode 100644 index 0000000000..d99463b55c --- /dev/null +++ b/core/img/actions/user-times.svg @@ -0,0 +1,12 @@ + + + + + +image/svg+xml + + + + + + diff --git a/settings/Controller/UsersController.php b/settings/Controller/UsersController.php index 7f6602a510..5949d9c974 100644 --- a/settings/Controller/UsersController.php +++ b/settings/Controller/UsersController.php @@ -225,6 +225,7 @@ class UsersController extends Controller { 'email' => $displayName, 'isRestoreDisabled' => !$restorePossible, 'isAvatarAvailable' => $avatarAvailable, + 'isEnabled' => $user->isEnabled(), ]; } @@ -253,11 +254,6 @@ class UsersController extends Controller { * TODO: Tidy up and write unit tests - code is mainly static method calls */ public function index($offset = 0, $limit = 10, $gid = '', $pattern = '', $backend = '') { - // FIXME: The JS sends the group '_everyone' instead of no GID for the "all users" group. - if($gid === '_everyone') { - $gid = ''; - } - // Remove backends if(!empty($backend)) { $activeBackends = $this->userManager->getBackends(); @@ -272,15 +268,18 @@ class UsersController extends Controller { $users = []; if ($this->isAdmin) { - - if($gid !== '') { + if($gid !== '' && $gid !== 'disabledUsers') { $batch = $this->getUsersForUID($this->groupManager->displayNamesInGroup($gid, $pattern, $limit, $offset)); } else { $batch = $this->userManager->search($pattern, $limit, $offset); } foreach ($batch as $user) { - $users[] = $this->formatUserForIndex($user); + if( ($gid !== 'disabledUsers' && $user->isEnabled()) || + ($gid === 'disabledUsers' && !$user->isEnabled()) + ) { + $users[] = $this->formatUserForIndex($user); + } } } else { @@ -293,7 +292,7 @@ class UsersController extends Controller { $subAdminOfGroups = $gids; // Set the $gid parameter to an empty value if the subadmin has no rights to access a specific group - if($gid !== '' && !in_array($gid, $subAdminOfGroups)) { + if($gid !== '' && $gid !== 'disabledUsers' && !in_array($gid, $subAdminOfGroups)) { $gid = ''; } @@ -318,7 +317,11 @@ class UsersController extends Controller { $this->groupManager->getUserGroupIds($user), $subAdminOfGroups )); - $users[] = $this->formatUserForIndex($user, $userGroups); + if( ($gid !== 'disabledUsers' && $user->isEnabled()) || + ($gid === 'disabledUsers' && !$user->isEnabled()) + ) { + $users[] = $this->formatUserForIndex($user, $userGroups); + } } } @@ -513,6 +516,137 @@ class UsersController extends Controller { } /** + * @NoAdminRequired + * + * @param string $id + * @return DataResponse + */ + public function disable($id) { + $userId = $this->userSession->getUser()->getUID(); + $user = $this->userManager->get($id); + + if($userId === $id) { + return new DataResponse( + array( + 'status' => 'error', + 'data' => array( + 'message' => (string)$this->l10n->t('Unable to disable user.') + ) + ), + Http::STATUS_FORBIDDEN + ); + } + + if(!$this->isAdmin && !$this->groupManager->getSubAdmin()->isUserAccessible($this->userSession->getUser(), $user)) { + return new DataResponse( + array( + 'status' => 'error', + 'data' => array( + 'message' => (string)$this->l10n->t('Authentication error') + ) + ), + Http::STATUS_FORBIDDEN + ); + } + + if($user) { + $user->setEnabled(false); + return new DataResponse( + array( + 'status' => 'success', + 'data' => array( + 'username' => $id, + 'enabled' => 0 + ) + ) + ); + } else { + return new DataResponse( + array( + 'status' => 'error', + 'data' => array( + 'message' => (string)$this->l10n->t('Unable to disable user.') + ) + ) + ); + } + } + + /** + * @NoAdminRequired + * + * @param string $id + * @return DataResponse + */ + public function enable($id) { + $userId = $this->userSession->getUser()->getUID(); + $user = $this->userManager->get($id); + + if($userId === $id) { + return new DataResponse( + array( + 'status' => 'error', + 'data' => array( + 'message' => (string)$this->l10n->t('Unable to enable user.') + ) + ), + Http::STATUS_FORBIDDEN + ); + } + + if(!$this->isAdmin && !$this->groupManager->getSubAdmin()->isUserAccessible($this->userSession->getUser(), $user)) { + return new DataResponse( + array( + 'status' => 'error', + 'data' => array( + 'message' => (string)$this->l10n->t('Authentication error') + ) + ), + Http::STATUS_FORBIDDEN + ); + } + + if($user) { + $user->setEnabled(true); + return new DataResponse( + array( + 'status' => 'success', + 'data' => array( + 'username' => $id, + 'enabled' => 1 + ) + ) + ); + } else { + return new DataResponse( + array( + 'status' => 'error', + 'data' => array( + 'message' => (string)$this->l10n->t('Unable to enable user.') + ) + ) + ); + } + } + + /** + * @NoAdminRequired + * + * @param string $id + * @param int $enabled + * @return DataResponse + */ + public function setEnabled($id, $enabled) { + if((bool)$enabled) { + return $this->enable($id); + } else { + return $this->disable($id); + } + } + + /** + * Set the mail address of a user + * * @NoAdminRequired * @NoSubadminRequired * @PasswordConfirmationRequired diff --git a/settings/css/settings.css b/settings/css/settings.css index 5693224b94..4a245fcf52 100644 --- a/settings/css/settings.css +++ b/settings/css/settings.css @@ -458,15 +458,16 @@ table.grid th, table.grid td { font-weight: normal; } td.name, td.password { padding-left:.8em; } -td.password>img,td.displayName>img, td.remove>a, td.quota>img { visibility:hidden; vertical-align: middle;} +td.password>img,td.displayName>img, td.quota>img { visibility:hidden; } td.password, td.quota, td.displayName { width:12em; cursor:pointer; } -td.password>span, td.quota>span, rd.displayName>span { margin-right: 1.2em; color: #C7C7C7; } +td.password>span, td.quota>span, td.displayName>span { margin-right: 1.2em; color: #C7C7C7; } span.usersLastLoginTooltip { white-space: nowrap; } /* dropdowns will be relative to this element */ #userlist { position: relative; } + #userlist .mailAddress, #userlist .storageLocation, #userlist .userBackend, @@ -501,13 +502,41 @@ span.usersLastLoginTooltip { white-space: nowrap; } display: none; } -tr:hover>td.password>span, tr:hover>td.displayName>span { margin:0; cursor:pointer; } -tr:hover>td.remove>a, tr:hover>td.password>img,tr:hover>td.displayName>img, tr:hover>td.quota>img { visibility:visible; cursor:pointer; } -td.remove { - width: 25px; +.bubble { + z-index:1; + right: -5px; } -tr:hover>td.remove>a { - float: left; +.bubble:after { + right: 5px; +} + +.popovermenu a.menuitem { + padding: 10px; + margin: -5px -10px -5px -15px; + height: 20px; +} + +#userlist .popovermenu { + margin-top: 1px; +} + +#userlist .popovermenu>ul.userActionsMenu { + right: 10px; + padding: 0 10px 10px; +} + +#userlist .popovermenu>ul.userActionsMenu a span { + margin-left: 5px; +} + +#userlist .popovermenu { + display:none; +} + +tr:hover>td.password>span, tr:hover>td.displayName>span { margin:0; cursor:pointer; } +tr:hover>td.password>img,tr:hover>td.displayName>img, tr:hover>td.quota>img { visibility:visible; cursor:pointer; } +td.userActions { + width: 25px; } div.recoveryPassword { left:50em; display:block; position:absolute; top:-1px; } diff --git a/settings/js/users/groups.js b/settings/js/users/groups.js index aac1609bce..b54f26b4cc 100644 --- a/settings/js/users/groups.js +++ b/settings/js/users/groups.js @@ -48,7 +48,8 @@ GroupList = { }, getUserCount: function ($groupLiElement) { - return parseInt($groupLiElement.data('usercount'), 10); + var count = parseInt($groupLiElement.data('usercount'), 10); + return isNaN(count) ? 0 : count; }, modGroupCount: function(gid, diff) { @@ -208,10 +209,17 @@ GroupList = { }, + showDisabledUsers: function () { + UserList.empty(); + UserList.update('disabledUsers'); + $userGroupList.find('li').removeClass('active'); + GroupList.getGroupLI('disabledUsers').addClass('active'); + }, + showGroup: function (gid) { GroupList.activeGID = gid; UserList.empty(); - UserList.update(gid); + UserList.update(gid === '_everyone' ? '' : gid); $userGroupList.find('li').removeClass('active'); if (gid !== undefined) { //TODO: treat Everyone properly @@ -364,6 +372,11 @@ $(document).ready( function () { GroupList.showGroup(GroupList.getElementGID(this)); }); + // show disabled users + $userGroupList.on('click', '.disabledusers', function () { + GroupList.showDisabledUsers(); + }); + $('#newgroupname').on('input', function(){ GroupList.handleAddGroupInput(this.value); }); diff --git a/settings/js/users/users.js b/settings/js/users/users.js index 46f4e5977b..552d28ad50 100644 --- a/settings/js/users/users.js +++ b/settings/js/users/users.js @@ -48,11 +48,12 @@ var UserList = { * 'backend': 'LDAP', * 'email': 'username@example.org' * 'isRestoreDisabled':false + * 'isEnabled': true * } */ add: function (user) { - if (this.currentGid && this.currentGid !== '_everyone' && _.indexOf(user.groups, this.currentGid) < 0) { - return; + if (this.currentGid && this.currentGid !== '_everyone' && this.currentGid !== 'disabledUsers' && _.indexOf(user.groups, this.currentGid) < 0) { + return false; } var $tr = $userListBody.find('tr:first-child').clone(); @@ -77,6 +78,7 @@ var UserList = { $tr.data('displayname', user.displayname); $tr.data('mailAddress', user.email); $tr.data('restoreDisabled', user.isRestoreDisabled); + $tr.data('userEnabled', user.isEnabled); $tr.find('.name').text(user.name); $tr.find('td.displayName > span').text(user.displayname); $tr.find('td.mailAddress > span').text(user.email); @@ -97,18 +99,17 @@ var UserList = { $tdSubadmins.find('.action').tooltip({placement: 'top'}); /** - * remove action + * user actions menu */ - if ($tr.find('td.remove img').length === 0 && OC.currentUser !== user.name) { - var deleteImage = $('').attr({ - src: OC.imagePath('core', 'actions/delete') + if ($tr.find('td.userActions > span > img').length === 0 && OC.currentUser !== user.name) { + var menuImage = $('').attr({ + src: OC.imagePath('core', 'actions/more') }); - var deleteLink = $('') - .attr({ href: '#', 'original-title': t('settings', 'Delete')}) - .append(deleteImage); - $tr.find('td.remove').append(deleteLink); + var menuLink = $('') + .append(menuImage); + $tr.find('td.userActions > span').replaceWith(menuLink); } else if (OC.currentUser === user.name) { - $tr.find('td.remove a').remove(); + $tr.find('td.userActions').empty(); } /** @@ -160,14 +161,6 @@ var UserList = { * append generated row to user list */ $tr.appendTo($userList); - if(UserList.isEmpty === true) { - //when the list was emptied, one row was left, necessary to keep - //add working and the layout unbroken. We need to remove this item - $tr.show(); - $userListBody.find('tr:first').remove(); - UserList.isEmpty = false; - UserList.checkUsersToLoad(); - } $quotaSelect.on('change', UserList.onQuotaSelect); @@ -338,6 +331,9 @@ var UserList = { getRestoreDisabled: function(element) { return ($(element).closest('tr').data('restoreDisabled') || ''); }, + getUserEnabled: function(element) { + return ($(element).closest('tr').data('userEnabled') || ''); + }, initDeleteHandling: function() { //set up handler UserDeleteHandler = new DeleteHandler('/settings/users/users', 'username', @@ -351,7 +347,7 @@ var UserList = { UserList.undoRemove); //when to mark user for delete - $userListBody.on('click', '.delete', function () { + $userListBody.on('click', '.action-remove', function () { // Call function for handling delete/undo var uid = UserList.getUID(this); @@ -908,6 +904,54 @@ $(document).ready(function () { UserList._triggerGroupEdit($td, isSubadminSelect); }); + $userListBody.on('click', '.toggleUserActions', function (event) { + event.stopPropagation(); + var $td = $(this).closest('td'); + var $tr = $($td).closest('tr'); + var menudiv = $td.find('.popovermenu'); + + if(menudiv.is(':visible')) { + menudiv.fadeOut(100); + return; + } + menudiv.find('.action-togglestate').empty(); + if($tr.data('userEnabled')) { + $('.action-togglestate', $td).html(''+t('settings', 'Disable')+''); + } else { + $('.action-togglestate', $td).html(''+t('settings', 'Enable')+''); + } + menudiv.click(function() { menudiv.fadeOut(100); }); + menudiv.hover('', function() { menudiv.fadeOut(100); }); + menudiv.fadeIn(100); + }); + + $userListBody.on('click', '.action-togglestate', function (event) { + event.stopPropagation(); + var $td = $(this).closest('td'); + var $tr = $td.closest('tr'); + var uid = UserList.getUID($td); + var setEnabled = UserList.getUserEnabled($td) ? 0 : 1; + $.post( + OC.generateUrl('/settings/users/{id}/setEnabled', {id: uid}), + {username: uid, enabled: setEnabled}, + function (result) { + if (result && result.status==='success'){ + var count = GroupList.getUserCount(GroupList.getGroupLI('disabledUsers')); + $tr.remove(); + if(result.data.enabled == 1) { + $tr.data('userEnabled', true); + GroupList.setUserCount(GroupList.getGroupLI('disabledUsers'), count-1); + } else { + $tr.data('userEnabled', false); + GroupList.setUserCount(GroupList.getGroupLI('disabledUsers'), count+1); + } + } else { + OC.dialogs.alert(result.data.message, t('settings', 'Unable to change status of {user}', {user: uid})); + } + } + ); + }); + // init the quota field select box after it is shown the first time $('#app-settings').one('show', function() { $(this).find('#default_quota').singleSelect().on('change', UserList.onQuotaSelect); diff --git a/settings/routes.php b/settings/routes.php index ba0761856d..fb85b11f39 100644 --- a/settings/routes.php +++ b/settings/routes.php @@ -53,6 +53,7 @@ $application->registerRoutes($this, [ ['name' => 'Users#setEMailAddress', 'url' => '/settings/users/{id}/mailAddress', 'verb' => 'PUT'], ['name' => 'Users#setUserSettings', 'url' => '/settings/users/{username}/settings', 'verb' => 'PUT'], ['name' => 'Users#getVerificationCode', 'url' => '/settings/users/{account}/verify', 'verb' => 'GET'], + ['name' => 'Users#setEnabled', 'url' => '/settings/users/{id}/setEnabled', 'verb' => 'POST'], ['name' => 'Users#stats', 'url' => '/settings/users/stats', 'verb' => 'GET'], ['name' => 'LogSettings#setLogLevel', 'url' => '/settings/admin/log/level', 'verb' => 'POST'], ['name' => 'LogSettings#getEntries', 'url' => '/settings/admin/log/entries', 'verb' => 'GET'], diff --git a/settings/templates/users/part.grouplist.php b/settings/templates/users/part.grouplist.php index beb0f275a3..0b953c0298 100644 --- a/settings/templates/users/part.grouplist.php +++ b/settings/templates/users/part.grouplist.php @@ -37,6 +37,15 @@ + + +
  • + t('Disabled')); ?> + + 0) { p($disabledUsersGroup['usercount']); } ?> + +
    • +
  • diff --git a/settings/templates/users/part.userlist.php b/settings/templates/users/part.userlist.php index 7e7e1561e2..b908109ad2 100644 --- a/settings/templates/users/part.userlist.php +++ b/settings/templates/users/part.userlist.php @@ -14,7 +14,7 @@ t('Storage location')); ?> t('User backend')); ?> t('Last login')); ?> -   + @@ -63,7 +63,21 @@ - + +
    + +
    + diff --git a/settings/users.php b/settings/users.php index 1986592af7..c3b32d0473 100644 --- a/settings/users.php +++ b/settings/users.php @@ -59,12 +59,15 @@ $config = \OC::$server->getConfig(); $isAdmin = OC_User::isAdminUser(OC_User::getUser()); +$isDisabled = !OC_User::isEnabled(OC_User::getUser()); + $groupsInfo = new \OC\Group\MetaData( OC_User::getUser(), $isAdmin, $groupManager, \OC::$server->getUserSession() ); + $groupsInfo->setSorting($sortGroupsBy); list($adminGroup, $groups) = $groupsInfo->get(); @@ -92,6 +95,17 @@ if($isAdmin) { } $subAdmins = false; } +$disabledUsers = 0; +foreach (OC_User::getUsers() as $uid) { + if(!$userManager->get($uid)->isEnabled()) { + $disabledUsers++; + } +} +$disabledUsersGroup = array( + 'id' => 'disabledUsers', + 'name' => 'disabledUsers', + 'usercount' => $disabledUsers +); // load preset quotas $quotaPreset=$config->getAppValue('files', 'quota_preset', '1 GB, 5 GB, 10 GB'); @@ -111,6 +125,7 @@ $tmpl = new OC_Template("settings", "users/main", "user"); $tmpl->assign('groups', $groups); $tmpl->assign('sortGroups', $sortGroupsBy); $tmpl->assign('adminGroup', $adminGroup); +$tmpl->assign('disabledUsersGroup', $disabledUsersGroup); $tmpl->assign('isAdmin', (int)$isAdmin); $tmpl->assign('subadmins', $subAdmins); $tmpl->assign('numofgroups', count($groups) + count($adminGroup)); From 0ab85018e33175b1fb743a5d06eed84a86eaf59c Mon Sep 17 00:00:00 2001 From: Morris Jobke Date: Thu, 1 Sep 2016 16:54:12 +0200 Subject: [PATCH 02/18] improve layout --- settings/css/settings.css | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/settings/css/settings.css b/settings/css/settings.css index 4a245fcf52..8f4ab9039f 100644 --- a/settings/css/settings.css +++ b/settings/css/settings.css @@ -504,25 +504,26 @@ span.usersLastLoginTooltip { white-space: nowrap; } .bubble { z-index:1; - right: -5px; + right: -6px; } .bubble:after { right: 5px; } .popovermenu a.menuitem { - padding: 10px; - margin: -5px -10px -5px -15px; height: 20px; + margin: 0; + padding: 0; + line-height: initial; } #userlist .popovermenu { - margin-top: 1px; + margin-top: 4px; + border-top-right-radius: 3px; } #userlist .popovermenu>ul.userActionsMenu { right: 10px; - padding: 0 10px 10px; } #userlist .popovermenu>ul.userActionsMenu a span { @@ -537,6 +538,14 @@ tr:hover>td.password>span, tr:hover>td.displayName>span { margin:0; cursor:point tr:hover>td.password>img,tr:hover>td.displayName>img, tr:hover>td.quota>img { visibility:visible; cursor:pointer; } td.userActions { width: 25px; + text-align: center; +} +td.userActions .action { + position: relative; + top: 3px; +} +td.userActions .action:hover { + cursor: pointer; } div.recoveryPassword { left:50em; display:block; position:absolute; top:-1px; } From 84b4d448d03b3d1d5383270f9045369df9e297a2 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Fri, 9 Sep 2016 08:08:39 +0200 Subject: [PATCH 03/18] Fix unit tests --- .../Controller/UsersControllerTest.php | 60 ++++++++++++++++++- 1 file changed, 59 insertions(+), 1 deletion(-) diff --git a/tests/Settings/Controller/UsersControllerTest.php b/tests/Settings/Controller/UsersControllerTest.php index 589c5e97ee..2491d72190 100644 --- a/tests/Settings/Controller/UsersControllerTest.php +++ b/tests/Settings/Controller/UsersControllerTest.php @@ -211,6 +211,9 @@ class UsersControllerTest extends \Test\TestCase { ->expects($this->once()) ->method('getBackendClassName') ->will($this->returnValue('OC_User_Database')); + $foo->expects($this->any()) + ->method('isEnabled') + ->willReturn(true); $admin = $this->createMock(User::class); $admin ->expects($this->exactly(2)) @@ -240,6 +243,9 @@ class UsersControllerTest extends \Test\TestCase { ->expects($this->once()) ->method('getBackendClassName') ->willReturn(Dummy::class); + $admin->expects($this->any()) + ->method('isEnabled') + ->willReturn(true); $bar = $this->createMock(User::class); $bar ->expects($this->exactly(2)) @@ -267,6 +273,9 @@ class UsersControllerTest extends \Test\TestCase { ->expects($this->once()) ->method('getBackendClassName') ->willReturn(Dummy::class); + $bar->expects($this->any()) + ->method('isEnabled') + ->willReturn(true); $this->groupManager ->expects($this->once()) @@ -330,6 +339,7 @@ class UsersControllerTest extends \Test\TestCase { 'email' => 'foo@bar.com', 'isRestoreDisabled' => false, 'isAvatarAvailable' => true, + 'isEnabled' => true, ), 1 => array( 'name' => 'admin', @@ -343,6 +353,7 @@ class UsersControllerTest extends \Test\TestCase { 'email' => 'admin@bar.com', 'isRestoreDisabled' => false, 'isAvatarAvailable' => false, + 'isEnabled' => true, ), 2 => array( 'name' => 'bar', @@ -356,6 +367,7 @@ class UsersControllerTest extends \Test\TestCase { 'email' => 'bar@dummy.com', 'isRestoreDisabled' => false, 'isAvatarAvailable' => true, + 'isEnabled' => true, ), ) ); @@ -399,6 +411,9 @@ class UsersControllerTest extends \Test\TestCase { ->expects($this->once()) ->method('getBackendClassName') ->will($this->returnValue('OC_User_Database')); + $foo->expects($this->any()) + ->method('isEnabled') + ->willReturn(true); $admin = $this->createMock(User::class); $admin ->expects($this->exactly(2)) @@ -428,6 +443,9 @@ class UsersControllerTest extends \Test\TestCase { ->expects($this->once()) ->method('getBackendClassName') ->willReturn(Dummy::class); + $admin->expects($this->any()) + ->method('isEnabled') + ->willReturn(true); $bar = $this->createMock(User::class); $bar ->expects($this->exactly(2)) @@ -455,6 +473,9 @@ class UsersControllerTest extends \Test\TestCase { ->expects($this->once()) ->method('getBackendClassName') ->willReturn(Dummy::class); + $bar->expects($this->any()) + ->method('isEnabled') + ->willReturn(true); $this->groupManager ->expects($this->at(2)) @@ -532,6 +553,7 @@ class UsersControllerTest extends \Test\TestCase { 'email' => 'bar@dummy.com', 'isRestoreDisabled' => false, 'isAvatarAvailable' => true, + 'isEnabled' => true, ], 1=> [ 'name' => 'foo', @@ -545,6 +567,7 @@ class UsersControllerTest extends \Test\TestCase { 'email' => 'foo@bar.com', 'isRestoreDisabled' => false, 'isAvatarAvailable' => true, + 'isEnabled' => true, ], 2 => [ 'name' => 'admin', @@ -558,6 +581,7 @@ class UsersControllerTest extends \Test\TestCase { 'email' => 'admin@bar.com', 'isRestoreDisabled' => false, 'isAvatarAvailable' => false, + 'isEnabled' => true, ], ] ); @@ -600,6 +624,9 @@ class UsersControllerTest extends \Test\TestCase { ->expects($this->once()) ->method('getBackendClassName') ->will($this->returnValue('OC_User_Database')); + $foo->expects($this->any()) + ->method('isEnabled') + ->willReturn(true); $admin = $this->createMock(User::class); $admin ->expects($this->exactly(2)) @@ -629,6 +656,9 @@ class UsersControllerTest extends \Test\TestCase { ->expects($this->once()) ->method('getBackendClassName') ->willReturn(Dummy::class); + $admin->expects($this->any()) + ->method('isEnabled') + ->willReturn(true); $bar = $this->createMock(User::class); $bar ->expects($this->exactly(2)) @@ -656,6 +686,9 @@ class UsersControllerTest extends \Test\TestCase { ->expects($this->once()) ->method('getBackendClassName') ->willReturn(Dummy::class); + $bar->expects($this->any()) + ->method('isEnabled') + ->willReturn(true); $this->userManager ->expects($this->once()) @@ -692,6 +725,7 @@ class UsersControllerTest extends \Test\TestCase { 'email' => 'foo@bar.com', 'isRestoreDisabled' => false, 'isAvatarAvailable' => true, + 'isEnabled' => true, ), 1 => array( 'name' => 'admin', @@ -705,6 +739,7 @@ class UsersControllerTest extends \Test\TestCase { 'email' => 'admin@bar.com', 'isRestoreDisabled' => false, 'isAvatarAvailable' => false, + 'isEnabled' => true, ), 2 => array( 'name' => 'bar', @@ -718,6 +753,7 @@ class UsersControllerTest extends \Test\TestCase { 'email' => 'bar@dummy.com', 'isRestoreDisabled' => false, 'isAvatarAvailable' => true, + 'isEnabled' => true, ), ) ); @@ -755,6 +791,10 @@ class UsersControllerTest extends \Test\TestCase { ->expects($this->once()) ->method('getBackendClassName') ->will($this->returnValue('OC_User_Database')); + $user->expects($this->any()) + ->method('isEnabled') + ->willReturn(true); + $this->userManager ->expects($this->once()) ->method('getBackends') @@ -793,6 +833,7 @@ class UsersControllerTest extends \Test\TestCase { 'email' => null, 'isRestoreDisabled' => false, 'isAvatarAvailable' => true, + 'isEnabled' => true, ) ) ); @@ -832,6 +873,9 @@ class UsersControllerTest extends \Test\TestCase { ->expects($this->once()) ->method('getBackendClassName') ->will($this->returnValue('bar')); + $user->expects($this->any()) + ->method('isEnabled') + ->willReturn(true); $this->userManager ->expects($this->once()) @@ -864,6 +908,7 @@ class UsersControllerTest extends \Test\TestCase { 'email' => null, 'isRestoreDisabled' => false, 'isAvatarAvailable' => true, + 'isEnabled' => true, ), Http::STATUS_CREATED ); @@ -888,6 +933,9 @@ class UsersControllerTest extends \Test\TestCase { ->expects($this->once()) ->method('getBackendClassName') ->will($this->returnValue('bar')); + $user->expects($this->any()) + ->method('isEnabled') + ->willReturn(true); $existingGroup = $this->getMockBuilder('\OCP\IGroup') ->disableOriginalConstructor()->getMock(); $existingGroup @@ -946,6 +994,7 @@ class UsersControllerTest extends \Test\TestCase { 'email' => null, 'isRestoreDisabled' => false, 'isAvatarAvailable' => true, + 'isEnabled' => true, ), Http::STATUS_CREATED ); @@ -975,6 +1024,9 @@ class UsersControllerTest extends \Test\TestCase { ->method('getBackendClassName') ->will($this->returnValue('bar')); $subGroup1 = $this->createMock(IGroup::class); + $newUser->expects($this->any()) + ->method('isEnabled') + ->willReturn(true); $subGroup1 ->expects($this->any()) ->method('getGID') @@ -1034,6 +1086,7 @@ class UsersControllerTest extends \Test\TestCase { 'email' => null, 'isRestoreDisabled' => false, 'isAvatarAvailable' => true, + 'isEnabled' => true, ), Http::STATUS_CREATED ); @@ -1463,7 +1516,8 @@ class UsersControllerTest extends \Test\TestCase { } private function mockUser($userId = 'foo', $displayName = 'M. Foo', - $lastLogin = 500, $home = '/home/foo', $backend = 'OC_User_Database') { + $lastLogin = 500, $home = '/home/foo', + $backend = 'OC_User_Database', $enabled = true) { $user = $this->createMock(User::class); $user ->expects($this->any()) @@ -1483,6 +1537,9 @@ class UsersControllerTest extends \Test\TestCase { ->expects($this->once()) ->method('getBackendClassName') ->will($this->returnValue($backend)); + $user->expects($this->any()) + ->method('isEnabled') + ->willReturn($enabled); $result = [ 'name' => $userId, @@ -1496,6 +1553,7 @@ class UsersControllerTest extends \Test\TestCase { 'email' => null, 'isRestoreDisabled' => false, 'isAvatarAvailable' => true, + 'isEnabled' => $enabled, ]; return [$user, $result]; From d91b4044fc040d4ddc541812c1afb6497873031a Mon Sep 17 00:00:00 2001 From: Morris Jobke Date: Fri, 9 Sep 2016 14:01:34 +0200 Subject: [PATCH 04/18] optimize SVGs --- core/img/actions/user-plus.svg | 12 ++---------- core/img/actions/user-times.svg | 12 ++---------- 2 files changed, 4 insertions(+), 20 deletions(-) diff --git a/core/img/actions/user-plus.svg b/core/img/actions/user-plus.svg index c236d9fc1d..16c59261a7 100644 --- a/core/img/actions/user-plus.svg +++ b/core/img/actions/user-plus.svg @@ -1,12 +1,4 @@ - - - - -image/svg+xml - - - - - + + diff --git a/core/img/actions/user-times.svg b/core/img/actions/user-times.svg index d99463b55c..f853784efc 100644 --- a/core/img/actions/user-times.svg +++ b/core/img/actions/user-times.svg @@ -1,12 +1,4 @@ - - - - -image/svg+xml - - - - - + + From 7be031ae6de807862d7d9e3632c79f8b80cf73c9 Mon Sep 17 00:00:00 2001 From: Morris Jobke Date: Mon, 12 Sep 2016 16:16:20 +0200 Subject: [PATCH 05/18] change group id to _disabledUsers --- settings/Controller/UsersController.php | 12 ++++++------ settings/js/users/groups.js | 4 ++-- settings/js/users/users.js | 8 ++++---- settings/templates/users/part.grouplist.php | 2 +- settings/users.php | 4 ++-- 5 files changed, 15 insertions(+), 15 deletions(-) diff --git a/settings/Controller/UsersController.php b/settings/Controller/UsersController.php index 5949d9c974..293afe9e6f 100644 --- a/settings/Controller/UsersController.php +++ b/settings/Controller/UsersController.php @@ -268,15 +268,15 @@ class UsersController extends Controller { $users = []; if ($this->isAdmin) { - if($gid !== '' && $gid !== 'disabledUsers') { + if($gid !== '' && $gid !== '_disabledUsers') { $batch = $this->getUsersForUID($this->groupManager->displayNamesInGroup($gid, $pattern, $limit, $offset)); } else { $batch = $this->userManager->search($pattern, $limit, $offset); } foreach ($batch as $user) { - if( ($gid !== 'disabledUsers' && $user->isEnabled()) || - ($gid === 'disabledUsers' && !$user->isEnabled()) + if( ($gid !== '_disabledUsers' && $user->isEnabled()) || + ($gid === '_disabledUsers' && !$user->isEnabled()) ) { $users[] = $this->formatUserForIndex($user); } @@ -292,7 +292,7 @@ class UsersController extends Controller { $subAdminOfGroups = $gids; // Set the $gid parameter to an empty value if the subadmin has no rights to access a specific group - if($gid !== '' && $gid !== 'disabledUsers' && !in_array($gid, $subAdminOfGroups)) { + if($gid !== '' && $gid !== '_disabledUsers' && !in_array($gid, $subAdminOfGroups)) { $gid = ''; } @@ -317,8 +317,8 @@ class UsersController extends Controller { $this->groupManager->getUserGroupIds($user), $subAdminOfGroups )); - if( ($gid !== 'disabledUsers' && $user->isEnabled()) || - ($gid === 'disabledUsers' && !$user->isEnabled()) + if( ($gid !== '_disabledUsers' && $user->isEnabled()) || + ($gid === '_disabledUsers' && !$user->isEnabled()) ) { $users[] = $this->formatUserForIndex($user, $userGroups); } diff --git a/settings/js/users/groups.js b/settings/js/users/groups.js index b54f26b4cc..16621441a6 100644 --- a/settings/js/users/groups.js +++ b/settings/js/users/groups.js @@ -211,9 +211,9 @@ GroupList = { showDisabledUsers: function () { UserList.empty(); - UserList.update('disabledUsers'); + UserList.update('_disabledUsers'); $userGroupList.find('li').removeClass('active'); - GroupList.getGroupLI('disabledUsers').addClass('active'); + GroupList.getGroupLI('_disabledUsers').addClass('active'); }, showGroup: function (gid) { diff --git a/settings/js/users/users.js b/settings/js/users/users.js index 552d28ad50..117c61b7dc 100644 --- a/settings/js/users/users.js +++ b/settings/js/users/users.js @@ -52,7 +52,7 @@ var UserList = { * } */ add: function (user) { - if (this.currentGid && this.currentGid !== '_everyone' && this.currentGid !== 'disabledUsers' && _.indexOf(user.groups, this.currentGid) < 0) { + if (this.currentGid && this.currentGid !== '_everyone' && this.currentGid !== '_disabledUsers' && _.indexOf(user.groups, this.currentGid) < 0) { return false; } @@ -936,14 +936,14 @@ $(document).ready(function () { {username: uid, enabled: setEnabled}, function (result) { if (result && result.status==='success'){ - var count = GroupList.getUserCount(GroupList.getGroupLI('disabledUsers')); + var count = GroupList.getUserCount(GroupList.getGroupLI('_disabledUsers')); $tr.remove(); if(result.data.enabled == 1) { $tr.data('userEnabled', true); - GroupList.setUserCount(GroupList.getGroupLI('disabledUsers'), count-1); + GroupList.setUserCount(GroupList.getGroupLI('_disabledUsers'), count-1); } else { $tr.data('userEnabled', false); - GroupList.setUserCount(GroupList.getGroupLI('disabledUsers'), count+1); + GroupList.setUserCount(GroupList.getGroupLI('_disabledUsers'), count+1); } } else { OC.dialogs.alert(result.data.message, t('settings', 'Unable to change status of {user}', {user: uid})); diff --git a/settings/templates/users/part.grouplist.php b/settings/templates/users/part.grouplist.php index 0b953c0298..5dfd7836f6 100644 --- a/settings/templates/users/part.grouplist.php +++ b/settings/templates/users/part.grouplist.php @@ -39,7 +39,7 @@ -
  • +
  • t('Disabled')); ?> 0) { p($disabledUsersGroup['usercount']); } ?> diff --git a/settings/users.php b/settings/users.php index c3b32d0473..9f3433aa33 100644 --- a/settings/users.php +++ b/settings/users.php @@ -102,8 +102,8 @@ foreach (OC_User::getUsers() as $uid) { } } $disabledUsersGroup = array( - 'id' => 'disabledUsers', - 'name' => 'disabledUsers', + 'id' => '_disabledUsers', + 'name' => '_disabledUsers', 'usercount' => $disabledUsers ); From 79d74a1425e6765a30469422a9b9ff218c8e2ef2 Mon Sep 17 00:00:00 2001 From: Morris Jobke Date: Wed, 14 Sep 2016 14:30:55 +0200 Subject: [PATCH 06/18] adjust tests to have at least one disabled user --- tests/Settings/Controller/UsersControllerTest.php | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/tests/Settings/Controller/UsersControllerTest.php b/tests/Settings/Controller/UsersControllerTest.php index 2491d72190..14f43a9e8e 100644 --- a/tests/Settings/Controller/UsersControllerTest.php +++ b/tests/Settings/Controller/UsersControllerTest.php @@ -273,9 +273,15 @@ class UsersControllerTest extends \Test\TestCase { ->expects($this->once()) ->method('getBackendClassName') ->willReturn(Dummy::class); - $bar->expects($this->any()) + $bar->expects($this->at(0)) ->method('isEnabled') ->willReturn(true); + $bar->expects($this->at(1)) + ->method('isEnabled') + ->willReturn(true); + $bar->expects($this->at(2)) + ->method('isEnabled') + ->willReturn(false); $this->groupManager ->expects($this->once()) @@ -367,7 +373,7 @@ class UsersControllerTest extends \Test\TestCase { 'email' => 'bar@dummy.com', 'isRestoreDisabled' => false, 'isAvatarAvailable' => true, - 'isEnabled' => true, + 'isEnabled' => false, ), ) ); @@ -2217,6 +2223,11 @@ class UsersControllerTest extends \Test\TestCase { /** * @dataProvider setEmailAddressData * + * @param string $mailAddress + * @param bool $isValid + * @param bool $expectsUpdate + * @param bool $canChangeDisplayName + * @param int $responseCode */ public function testSetEMailAddress($mailAddress, $isValid, $expectsUpdate, $canChangeDisplayName, $responseCode) { $user = $this->createMock(User::class); From e521b6799fac5e4765c4e0a7fdf0d182f4dc015d Mon Sep 17 00:00:00 2001 From: Morris Jobke Date: Wed, 14 Sep 2016 15:05:10 +0200 Subject: [PATCH 07/18] add unit tests for disable method --- .../Controller/UsersControllerTest.php | 220 +++++++++++++++++- 1 file changed, 219 insertions(+), 1 deletion(-) diff --git a/tests/Settings/Controller/UsersControllerTest.php b/tests/Settings/Controller/UsersControllerTest.php index 14f43a9e8e..986f15e7a3 100644 --- a/tests/Settings/Controller/UsersControllerTest.php +++ b/tests/Settings/Controller/UsersControllerTest.php @@ -2438,7 +2438,225 @@ class UsersControllerTest extends \Test\TestCase { $this->userSession->expects($this->once())->method('getUser')->willReturn(null); $result = $controller->getVerificationCode('account', false); - $this->assertSame(Http::STATUS_BAD_REQUEST ,$result->getStatus()); + $this->assertSame(Http::STATUS_BAD_REQUEST, $result->getStatus()); + } + public function testDisableUserFailsDueSameUser() { + $user = $this->getMockBuilder('\OC\User\User') + ->disableOriginalConstructor()->getMock(); + $user->expects($this->once()) + ->method('getUID') + ->will($this->returnValue('abc')); + $this->userSession + ->expects($this->once()) + ->method('getUser') + ->will($this->returnValue($user)); + $expectedResponse = new DataResponse( + [ + 'status' => 'error', + 'data' => [ + 'message' => 'Unable to disable user.', + ], + ], + Http::STATUS_FORBIDDEN + ); + $controller = $this->getController(true); + $response = $controller->disable('abc'); + $this->assertEquals($expectedResponse, $response); + } + + public function testDisableUserFailsDueNoAdminAndNoSubadmin() { + $user = $this->getMockBuilder('\OC\User\User') + ->disableOriginalConstructor()->getMock(); + $user->expects($this->once()) + ->method('getUID') + ->will($this->returnValue('def')); + $this->userSession + ->expects($this->exactly(2)) + ->method('getUser') + ->will($this->returnValue($user)); + $user2 = $this->getMockBuilder('\OC\User\User') + ->disableOriginalConstructor()->getMock(); + $user2->expects($this->never()) + ->method('setEnabled'); + $this->userManager + ->expects($this->once()) + ->method('get') + ->with('abc') + ->willReturn($user2); + + $subadmin = $this->createMock('\OC\SubAdmin'); + $subadmin->expects($this->once()) + ->method('isUserAccessible') + ->will($this->returnValue(false)); + $this->groupManager + ->expects($this->once()) + ->method('getSubAdmin') + ->willReturn($subadmin); + + $expectedResponse = new DataResponse( + [ + 'status' => 'error', + 'data' => [ + 'message' => 'Authentication error', + ], + ], + Http::STATUS_FORBIDDEN + ); + $controller = $this->getController(false); + $response = $controller->disable('abc'); + $this->assertEquals($expectedResponse, $response); + } + + public function testDisableUserFailsDueNoUser() { + $user = $this->getMockBuilder('\OC\User\User') + ->disableOriginalConstructor()->getMock(); + $user->expects($this->once()) + ->method('getUID') + ->will($this->returnValue('def')); + $this->userSession + ->expects($this->exactly(1)) + ->method('getUser') + ->will($this->returnValue($user)); + $this->userManager + ->expects($this->once()) + ->method('get') + ->with('abc') + ->willReturn(null); + + $this->groupManager + ->expects($this->never()) + ->method('getSubAdmin'); + + $expectedResponse = new DataResponse( + [ + 'status' => 'error', + 'data' => [ + 'message' => 'Unable to disable user.', + ], + ] + ); + $controller = $this->getController(true); + $response = $controller->disable('abc'); + $this->assertEquals($expectedResponse, $response); + } + + public function testDisableUserFailsDueNoUserForSubAdmin() { + $user = $this->getMockBuilder('\OC\User\User') + ->disableOriginalConstructor()->getMock(); + $user->expects($this->once()) + ->method('getUID') + ->will($this->returnValue('def')); + $this->userSession + ->expects($this->exactly(2)) + ->method('getUser') + ->will($this->returnValue($user)); + $this->userManager + ->expects($this->once()) + ->method('get') + ->with('abc') + ->willReturn(null); + + $subadmin = $this->createMock('\OC\SubAdmin'); + $subadmin->expects($this->once()) + ->method('isUserAccessible') + ->will($this->returnValue(true)); + $this->groupManager + ->expects($this->once()) + ->method('getSubAdmin') + ->willReturn($subadmin); + + $expectedResponse = new DataResponse( + [ + 'status' => 'error', + 'data' => [ + 'message' => 'Unable to disable user.', + ], + ] + ); + $controller = $this->getController(false); + $response = $controller->disable('abc'); + $this->assertEquals($expectedResponse, $response); + } + + public function testDisableUserSuccessForAdmin() { + $user = $this->getMockBuilder('\OC\User\User') + ->disableOriginalConstructor()->getMock(); + $user->expects($this->once()) + ->method('getUID') + ->will($this->returnValue('def')); + $this->userSession + ->expects($this->exactly(1)) + ->method('getUser') + ->will($this->returnValue($user)); + $user2 = $this->getMockBuilder('\OC\User\User') + ->disableOriginalConstructor()->getMock(); + $user2->expects($this->once()) + ->method('setEnabled'); + $this->userManager + ->expects($this->once()) + ->method('get') + ->with('abc') + ->willReturn($user2); + + $this->groupManager + ->expects($this->never()) + ->method('getSubAdmin'); + + $expectedResponse = new DataResponse( + [ + 'status' => 'success', + 'data' => [ + 'username' => 'abc', + 'enabled' => 0, + ], + ] + ); + $controller = $this->getController(true); + $response = $controller->disable('abc'); + $this->assertEquals($expectedResponse, $response); + } + + public function testDisableUserSuccessForSubAdmin() { + $user = $this->getMockBuilder('\OC\User\User') + ->disableOriginalConstructor()->getMock(); + $user->expects($this->once()) + ->method('getUID') + ->will($this->returnValue('def')); + $this->userSession + ->expects($this->exactly(2)) + ->method('getUser') + ->will($this->returnValue($user)); + $user2 = $this->getMockBuilder('\OC\User\User') + ->disableOriginalConstructor()->getMock(); + $user2->expects($this->once()) + ->method('setEnabled'); + $this->userManager + ->expects($this->once()) + ->method('get') + ->with('abc') + ->willReturn($user2); + + $subadmin = $this->createMock('\OC\SubAdmin'); + $subadmin->expects($this->once()) + ->method('isUserAccessible') + ->will($this->returnValue(true)); + $this->groupManager + ->expects($this->once()) + ->method('getSubAdmin') + ->willReturn($subadmin); + + $expectedResponse = new DataResponse( + [ + 'status' => 'success', + 'data' => [ + 'username' => 'abc', + 'enabled' => 0, + ], + ] + ); + $controller = $this->getController(false); + $response = $controller->disable('abc'); + $this->assertEquals($expectedResponse, $response); } } From 72550377b437f801925e8573f9fe53eb5803379e Mon Sep 17 00:00:00 2001 From: Morris Jobke Date: Wed, 14 Sep 2016 15:09:07 +0200 Subject: [PATCH 08/18] add unit tests for enable method --- .../Controller/UsersControllerTest.php | 222 +++++++++++++++++- 1 file changed, 221 insertions(+), 1 deletion(-) diff --git a/tests/Settings/Controller/UsersControllerTest.php b/tests/Settings/Controller/UsersControllerTest.php index 986f15e7a3..378c98e733 100644 --- a/tests/Settings/Controller/UsersControllerTest.php +++ b/tests/Settings/Controller/UsersControllerTest.php @@ -2592,7 +2592,8 @@ class UsersControllerTest extends \Test\TestCase { $user2 = $this->getMockBuilder('\OC\User\User') ->disableOriginalConstructor()->getMock(); $user2->expects($this->once()) - ->method('setEnabled'); + ->method('setEnabled') + ->with(false); $this->userManager ->expects($this->once()) ->method('get') @@ -2659,4 +2660,223 @@ class UsersControllerTest extends \Test\TestCase { $response = $controller->disable('abc'); $this->assertEquals($expectedResponse, $response); } + + public function testEnableUserFailsDueSameUser() { + $user = $this->getMockBuilder('\OC\User\User') + ->disableOriginalConstructor()->getMock(); + $user->expects($this->once()) + ->method('getUID') + ->will($this->returnValue('abc')); + $this->userSession + ->expects($this->once()) + ->method('getUser') + ->will($this->returnValue($user)); + $expectedResponse = new DataResponse( + [ + 'status' => 'error', + 'data' => [ + 'message' => 'Unable to enable user.', + ], + ], + Http::STATUS_FORBIDDEN + ); + $controller = $this->getController(true); + $response = $controller->enable('abc'); + $this->assertEquals($expectedResponse, $response); + } + + public function testEnableUserFailsDueNoAdminAndNoSubadmin() { + $user = $this->getMockBuilder('\OC\User\User') + ->disableOriginalConstructor()->getMock(); + $user->expects($this->once()) + ->method('getUID') + ->will($this->returnValue('def')); + $this->userSession + ->expects($this->exactly(2)) + ->method('getUser') + ->will($this->returnValue($user)); + $user2 = $this->getMockBuilder('\OC\User\User') + ->disableOriginalConstructor()->getMock(); + $user2->expects($this->never()) + ->method('setEnabled'); + $this->userManager + ->expects($this->once()) + ->method('get') + ->with('abc') + ->willReturn($user2); + + $subadmin = $this->createMock('\OC\SubAdmin'); + $subadmin->expects($this->once()) + ->method('isUserAccessible') + ->will($this->returnValue(false)); + $this->groupManager + ->expects($this->once()) + ->method('getSubAdmin') + ->willReturn($subadmin); + + $expectedResponse = new DataResponse( + [ + 'status' => 'error', + 'data' => [ + 'message' => 'Authentication error', + ], + ], + Http::STATUS_FORBIDDEN + ); + $controller = $this->getController(false); + $response = $controller->enable('abc'); + $this->assertEquals($expectedResponse, $response); + } + + public function testEnableUserFailsDueNoUser() { + $user = $this->getMockBuilder('\OC\User\User') + ->disableOriginalConstructor()->getMock(); + $user->expects($this->once()) + ->method('getUID') + ->will($this->returnValue('def')); + $this->userSession + ->expects($this->exactly(1)) + ->method('getUser') + ->will($this->returnValue($user)); + $this->userManager + ->expects($this->once()) + ->method('get') + ->with('abc') + ->willReturn(null); + + $this->groupManager + ->expects($this->never()) + ->method('getSubAdmin'); + + $expectedResponse = new DataResponse( + [ + 'status' => 'error', + 'data' => [ + 'message' => 'Unable to enable user.', + ], + ] + ); + $controller = $this->getController(true); + $response = $controller->enable('abc'); + $this->assertEquals($expectedResponse, $response); + } + + public function testEnableUserFailsDueNoUserForSubAdmin() { + $user = $this->getMockBuilder('\OC\User\User') + ->disableOriginalConstructor()->getMock(); + $user->expects($this->once()) + ->method('getUID') + ->will($this->returnValue('def')); + $this->userSession + ->expects($this->exactly(2)) + ->method('getUser') + ->will($this->returnValue($user)); + $this->userManager + ->expects($this->once()) + ->method('get') + ->with('abc') + ->willReturn(null); + + $subadmin = $this->createMock('\OC\SubAdmin'); + $subadmin->expects($this->once()) + ->method('isUserAccessible') + ->will($this->returnValue(true)); + $this->groupManager + ->expects($this->once()) + ->method('getSubAdmin') + ->willReturn($subadmin); + + $expectedResponse = new DataResponse( + [ + 'status' => 'error', + 'data' => [ + 'message' => 'Unable to enable user.', + ], + ] + ); + $controller = $this->getController(false); + $response = $controller->enable('abc'); + $this->assertEquals($expectedResponse, $response); + } + + public function testEnableUserSuccessForAdmin() { + $user = $this->getMockBuilder('\OC\User\User') + ->disableOriginalConstructor()->getMock(); + $user->expects($this->once()) + ->method('getUID') + ->will($this->returnValue('def')); + $this->userSession + ->expects($this->exactly(1)) + ->method('getUser') + ->will($this->returnValue($user)); + $user2 = $this->getMockBuilder('\OC\User\User') + ->disableOriginalConstructor()->getMock(); + $user2->expects($this->once()) + ->method('setEnabled'); + $this->userManager + ->expects($this->once()) + ->method('get') + ->with('abc') + ->willReturn($user2); + + $this->groupManager + ->expects($this->never()) + ->method('getSubAdmin'); + + $expectedResponse = new DataResponse( + [ + 'status' => 'success', + 'data' => [ + 'username' => 'abc', + 'enabled' => 1, + ], + ] + ); + $controller = $this->getController(true); + $response = $controller->enable('abc'); + $this->assertEquals($expectedResponse, $response); + } + + public function testEnableUserSuccessForSubAdmin() { + $user = $this->getMockBuilder('\OC\User\User') + ->disableOriginalConstructor()->getMock(); + $user->expects($this->once()) + ->method('getUID') + ->will($this->returnValue('def')); + $this->userSession + ->expects($this->exactly(2)) + ->method('getUser') + ->will($this->returnValue($user)); + $user2 = $this->getMockBuilder('\OC\User\User') + ->disableOriginalConstructor()->getMock(); + $user2->expects($this->once()) + ->method('setEnabled') + ->with(true); + $this->userManager + ->expects($this->once()) + ->method('get') + ->with('abc') + ->willReturn($user2); + + $subadmin = $this->createMock('\OC\SubAdmin'); + $subadmin->expects($this->once()) + ->method('isUserAccessible') + ->will($this->returnValue(true)); + $this->groupManager + ->method('getSubAdmin') + ->willReturn($subadmin); + + $expectedResponse = new DataResponse( + [ + 'status' => 'success', + 'data' => [ + 'username' => 'abc', + 'enabled' => 1, + ], + ] + ); + $controller = $this->getController(false); + $response = $controller->enable('abc'); + $this->assertEquals($expectedResponse, $response); + } } From 74e50910134610a108e18a3807a791ef0b677468 Mon Sep 17 00:00:00 2001 From: Morris Jobke Date: Thu, 6 Oct 2016 15:24:22 +0200 Subject: [PATCH 09/18] check $user object before using it Signed-off-by: Morris Jobke --- settings/Controller/UsersController.php | 48 ++++++++++++------------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/settings/Controller/UsersController.php b/settings/Controller/UsersController.php index 293afe9e6f..7ce4355aa0 100644 --- a/settings/Controller/UsersController.php +++ b/settings/Controller/UsersController.php @@ -537,19 +537,19 @@ class UsersController extends Controller { ); } - if(!$this->isAdmin && !$this->groupManager->getSubAdmin()->isUserAccessible($this->userSession->getUser(), $user)) { - return new DataResponse( - array( - 'status' => 'error', - 'data' => array( - 'message' => (string)$this->l10n->t('Authentication error') - ) - ), - Http::STATUS_FORBIDDEN - ); - } - if($user) { + if(!$this->isAdmin && !$this->groupManager->getSubAdmin()->isUserAccessible($this->userSession->getUser(), $user)) { + return new DataResponse( + array( + 'status' => 'error', + 'data' => array( + 'message' => (string)$this->l10n->t('Authentication error') + ) + ), + Http::STATUS_FORBIDDEN + ); + } + $user->setEnabled(false); return new DataResponse( array( @@ -594,19 +594,19 @@ class UsersController extends Controller { ); } - if(!$this->isAdmin && !$this->groupManager->getSubAdmin()->isUserAccessible($this->userSession->getUser(), $user)) { - return new DataResponse( - array( - 'status' => 'error', - 'data' => array( - 'message' => (string)$this->l10n->t('Authentication error') - ) - ), - Http::STATUS_FORBIDDEN - ); - } - if($user) { + if(!$this->isAdmin && !$this->groupManager->getSubAdmin()->isUserAccessible($this->userSession->getUser(), $user)) { + return new DataResponse( + array( + 'status' => 'error', + 'data' => array( + 'message' => (string)$this->l10n->t('Authentication error') + ) + ), + Http::STATUS_FORBIDDEN + ); + } + $user->setEnabled(true); return new DataResponse( array( From a8457df064eb14d2d67b2efcfee3638f18ff1683 Mon Sep 17 00:00:00 2001 From: Morris Jobke Date: Mon, 10 Oct 2016 16:12:18 +0200 Subject: [PATCH 10/18] fix unit tests Signed-off-by: Morris Jobke --- .../Controller/UsersControllerTest.php | 37 +++++++------------ 1 file changed, 13 insertions(+), 24 deletions(-) diff --git a/tests/Settings/Controller/UsersControllerTest.php b/tests/Settings/Controller/UsersControllerTest.php index 378c98e733..5cae52ece7 100644 --- a/tests/Settings/Controller/UsersControllerTest.php +++ b/tests/Settings/Controller/UsersControllerTest.php @@ -2460,8 +2460,7 @@ class UsersControllerTest extends \Test\TestCase { ], Http::STATUS_FORBIDDEN ); - $controller = $this->getController(true); - $response = $controller->disable('abc'); + $response = $this->getController(true)->disable('abc'); $this->assertEquals($expectedResponse, $response); } @@ -2503,8 +2502,7 @@ class UsersControllerTest extends \Test\TestCase { ], Http::STATUS_FORBIDDEN ); - $controller = $this->getController(false); - $response = $controller->disable('abc'); + $response = $this->getController(false)->disable('abc'); $this->assertEquals($expectedResponse, $response); } @@ -2536,8 +2534,7 @@ class UsersControllerTest extends \Test\TestCase { ], ] ); - $controller = $this->getController(true); - $response = $controller->disable('abc'); + $response = $this->getController(true)->disable('abc'); $this->assertEquals($expectedResponse, $response); } @@ -2574,8 +2571,7 @@ class UsersControllerTest extends \Test\TestCase { ], ] ); - $controller = $this->getController(false); - $response = $controller->disable('abc'); + $response = $this->getController(false)->disable('abc'); $this->assertEquals($expectedResponse, $response); } @@ -2613,8 +2609,7 @@ class UsersControllerTest extends \Test\TestCase { ], ] ); - $controller = $this->getController(true); - $response = $controller->disable('abc'); + $response = $this->getController(true)->disable('abc'); $this->assertEquals($expectedResponse, $response); } @@ -2656,8 +2651,7 @@ class UsersControllerTest extends \Test\TestCase { ], ] ); - $controller = $this->getController(false); - $response = $controller->disable('abc'); + $response = $this->getController(false)->disable('abc'); $this->assertEquals($expectedResponse, $response); } @@ -2680,8 +2674,7 @@ class UsersControllerTest extends \Test\TestCase { ], Http::STATUS_FORBIDDEN ); - $controller = $this->getController(true); - $response = $controller->enable('abc'); + $response = $this->getController(true)->enable('abc'); $this->assertEquals($expectedResponse, $response); } @@ -2723,8 +2716,7 @@ class UsersControllerTest extends \Test\TestCase { ], Http::STATUS_FORBIDDEN ); - $controller = $this->getController(false); - $response = $controller->enable('abc'); + $response = $this->getController(false)->enable('abc'); $this->assertEquals($expectedResponse, $response); } @@ -2756,8 +2748,7 @@ class UsersControllerTest extends \Test\TestCase { ], ] ); - $controller = $this->getController(true); - $response = $controller->enable('abc'); + $response = $this->getController(true)->enable('abc'); $this->assertEquals($expectedResponse, $response); } @@ -2794,8 +2785,7 @@ class UsersControllerTest extends \Test\TestCase { ], ] ); - $controller = $this->getController(false); - $response = $controller->enable('abc'); + $response = $this->getController(false)->enable('abc'); $this->assertEquals($expectedResponse, $response); } @@ -2832,8 +2822,7 @@ class UsersControllerTest extends \Test\TestCase { ], ] ); - $controller = $this->getController(true); - $response = $controller->enable('abc'); + $response = $this->getController(true)->enable('abc'); $this->assertEquals($expectedResponse, $response); } @@ -2863,6 +2852,7 @@ class UsersControllerTest extends \Test\TestCase { ->method('isUserAccessible') ->will($this->returnValue(true)); $this->groupManager + ->expects($this->once()) ->method('getSubAdmin') ->willReturn($subadmin); @@ -2875,8 +2865,7 @@ class UsersControllerTest extends \Test\TestCase { ], ] ); - $controller = $this->getController(false); - $response = $controller->enable('abc'); + $response = $this->getController(false)->enable('abc'); $this->assertEquals($expectedResponse, $response); } } From 74802ca72f27f41beb577f0ba9c085a50ed224e2 Mon Sep 17 00:00:00 2001 From: Morris Jobke Date: Tue, 11 Oct 2016 11:52:06 +0200 Subject: [PATCH 11/18] fix greyed out display name Signed-off-by: Morris Jobke --- settings/css/settings.css | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/settings/css/settings.css b/settings/css/settings.css index 8f4ab9039f..7fb1933bc9 100644 --- a/settings/css/settings.css +++ b/settings/css/settings.css @@ -460,7 +460,7 @@ table.grid th, table.grid td { td.name, td.password { padding-left:.8em; } td.password>img,td.displayName>img, td.quota>img { visibility:hidden; } td.password, td.quota, td.displayName { width:12em; cursor:pointer; } -td.password>span, td.quota>span, td.displayName>span { margin-right: 1.2em; color: #C7C7C7; } +td.password>span, td.quota>span { margin-right: 1.2em; color: #C7C7C7; } span.usersLastLoginTooltip { white-space: nowrap; } /* dropdowns will be relative to this element */ From 2507e7459d42e47f0581aeb69b738fec6497c6b5 Mon Sep 17 00:00:00 2001 From: Morris Jobke Date: Tue, 11 Oct 2016 11:52:49 +0200 Subject: [PATCH 12/18] Improve wording of error messages Signed-off-by: Morris Jobke --- settings/Controller/UsersController.php | 8 ++++---- settings/js/users/users.js | 2 +- tests/Settings/Controller/UsersControllerTest.php | 12 ++++++------ 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/settings/Controller/UsersController.php b/settings/Controller/UsersController.php index 7ce4355aa0..411269a435 100644 --- a/settings/Controller/UsersController.php +++ b/settings/Controller/UsersController.php @@ -530,7 +530,7 @@ class UsersController extends Controller { array( 'status' => 'error', 'data' => array( - 'message' => (string)$this->l10n->t('Unable to disable user.') + 'message' => (string)$this->l10n->t('Error while disabling user.') ) ), Http::STATUS_FORBIDDEN @@ -565,7 +565,7 @@ class UsersController extends Controller { array( 'status' => 'error', 'data' => array( - 'message' => (string)$this->l10n->t('Unable to disable user.') + 'message' => (string)$this->l10n->t('Error while disabling user.') ) ) ); @@ -587,7 +587,7 @@ class UsersController extends Controller { array( 'status' => 'error', 'data' => array( - 'message' => (string)$this->l10n->t('Unable to enable user.') + 'message' => (string)$this->l10n->t('Error while enabling user.') ) ), Http::STATUS_FORBIDDEN @@ -622,7 +622,7 @@ class UsersController extends Controller { array( 'status' => 'error', 'data' => array( - 'message' => (string)$this->l10n->t('Unable to enable user.') + 'message' => (string)$this->l10n->t('Error while enabling user.') ) ) ); diff --git a/settings/js/users/users.js b/settings/js/users/users.js index 117c61b7dc..2a0b0c09a9 100644 --- a/settings/js/users/users.js +++ b/settings/js/users/users.js @@ -946,7 +946,7 @@ $(document).ready(function () { GroupList.setUserCount(GroupList.getGroupLI('_disabledUsers'), count+1); } } else { - OC.dialogs.alert(result.data.message, t('settings', 'Unable to change status of {user}', {user: uid})); + OC.dialogs.alert(result.data.message, t('settings', 'Error while changing status of {user}', {user: uid})); } } ); diff --git a/tests/Settings/Controller/UsersControllerTest.php b/tests/Settings/Controller/UsersControllerTest.php index 5cae52ece7..b6eba294d3 100644 --- a/tests/Settings/Controller/UsersControllerTest.php +++ b/tests/Settings/Controller/UsersControllerTest.php @@ -2455,7 +2455,7 @@ class UsersControllerTest extends \Test\TestCase { [ 'status' => 'error', 'data' => [ - 'message' => 'Unable to disable user.', + 'message' => 'Error while disabling user.', ], ], Http::STATUS_FORBIDDEN @@ -2530,7 +2530,7 @@ class UsersControllerTest extends \Test\TestCase { [ 'status' => 'error', 'data' => [ - 'message' => 'Unable to disable user.', + 'message' => 'Error while disabling user.', ], ] ); @@ -2567,7 +2567,7 @@ class UsersControllerTest extends \Test\TestCase { [ 'status' => 'error', 'data' => [ - 'message' => 'Unable to disable user.', + 'message' => 'Error while disabling user.', ], ] ); @@ -2669,7 +2669,7 @@ class UsersControllerTest extends \Test\TestCase { [ 'status' => 'error', 'data' => [ - 'message' => 'Unable to enable user.', + 'message' => 'Error while enabling user.', ], ], Http::STATUS_FORBIDDEN @@ -2744,7 +2744,7 @@ class UsersControllerTest extends \Test\TestCase { [ 'status' => 'error', 'data' => [ - 'message' => 'Unable to enable user.', + 'message' => 'Error while enabling user.', ], ] ); @@ -2781,7 +2781,7 @@ class UsersControllerTest extends \Test\TestCase { [ 'status' => 'error', 'data' => [ - 'message' => 'Unable to enable user.', + 'message' => 'Error while enabling user.', ], ] ); From 485d6d657761370dc200d9cdb023ad564c23c4e9 Mon Sep 17 00:00:00 2001 From: Morris Jobke Date: Tue, 11 Oct 2016 12:07:20 +0200 Subject: [PATCH 13/18] use proper return codes and handle failure cases Signed-off-by: Morris Jobke --- settings/Controller/UsersController.php | 6 ++++-- settings/js/users/users.js | 10 +++++++++- tests/Settings/Controller/UsersControllerTest.php | 12 ++++++++---- 3 files changed, 21 insertions(+), 7 deletions(-) diff --git a/settings/Controller/UsersController.php b/settings/Controller/UsersController.php index 411269a435..56944ced98 100644 --- a/settings/Controller/UsersController.php +++ b/settings/Controller/UsersController.php @@ -567,7 +567,8 @@ class UsersController extends Controller { 'data' => array( 'message' => (string)$this->l10n->t('Error while disabling user.') ) - ) + ), + Http::STATUS_FORBIDDEN ); } } @@ -624,7 +625,8 @@ class UsersController extends Controller { 'data' => array( 'message' => (string)$this->l10n->t('Error while enabling user.') ) - ) + ), + Http::STATUS_FORBIDDEN ); } } diff --git a/settings/js/users/users.js b/settings/js/users/users.js index 2a0b0c09a9..387709cd64 100644 --- a/settings/js/users/users.js +++ b/settings/js/users/users.js @@ -949,7 +949,15 @@ $(document).ready(function () { OC.dialogs.alert(result.data.message, t('settings', 'Error while changing status of {user}', {user: uid})); } } - ); + ).fail(function(result){ + var message = 'Unknown error'; + if( result.responseJSON && + result.responseJSON.data && + result.responseJSON.data.message) { + message = result.responseJSON.data.message; + } + OC.dialogs.alert(message, t('settings', 'Error while changing status of {user}', {user: uid})); + }); }); // init the quota field select box after it is shown the first time diff --git a/tests/Settings/Controller/UsersControllerTest.php b/tests/Settings/Controller/UsersControllerTest.php index b6eba294d3..1dd3b0c697 100644 --- a/tests/Settings/Controller/UsersControllerTest.php +++ b/tests/Settings/Controller/UsersControllerTest.php @@ -2532,7 +2532,8 @@ class UsersControllerTest extends \Test\TestCase { 'data' => [ 'message' => 'Error while disabling user.', ], - ] + ], + Http::STATUS_FORBIDDEN ); $response = $this->getController(true)->disable('abc'); $this->assertEquals($expectedResponse, $response); @@ -2569,7 +2570,8 @@ class UsersControllerTest extends \Test\TestCase { 'data' => [ 'message' => 'Error while disabling user.', ], - ] + ], + Http::STATUS_FORBIDDEN ); $response = $this->getController(false)->disable('abc'); $this->assertEquals($expectedResponse, $response); @@ -2746,7 +2748,8 @@ class UsersControllerTest extends \Test\TestCase { 'data' => [ 'message' => 'Error while enabling user.', ], - ] + ], + Http::STATUS_FORBIDDEN ); $response = $this->getController(true)->enable('abc'); $this->assertEquals($expectedResponse, $response); @@ -2783,7 +2786,8 @@ class UsersControllerTest extends \Test\TestCase { 'data' => [ 'message' => 'Error while enabling user.', ], - ] + ], + Http::STATUS_FORBIDDEN ); $response = $this->getController(false)->enable('abc'); $this->assertEquals($expectedResponse, $response); From 4c37c380511bfb5cd06c247cef543e2c5ce3119a Mon Sep 17 00:00:00 2001 From: Morris Jobke Date: Fri, 2 Dec 2016 00:23:33 +0100 Subject: [PATCH 14/18] fix unit tests Signed-off-by: Morris Jobke --- .../Controller/UsersControllerTest.php | 22 +++++-------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/tests/Settings/Controller/UsersControllerTest.php b/tests/Settings/Controller/UsersControllerTest.php index 1dd3b0c697..2401980049 100644 --- a/tests/Settings/Controller/UsersControllerTest.php +++ b/tests/Settings/Controller/UsersControllerTest.php @@ -2546,7 +2546,7 @@ class UsersControllerTest extends \Test\TestCase { ->method('getUID') ->will($this->returnValue('def')); $this->userSession - ->expects($this->exactly(2)) + ->expects($this->exactly(1)) ->method('getUser') ->will($this->returnValue($user)); $this->userManager @@ -2555,14 +2555,9 @@ class UsersControllerTest extends \Test\TestCase { ->with('abc') ->willReturn(null); - $subadmin = $this->createMock('\OC\SubAdmin'); - $subadmin->expects($this->once()) - ->method('isUserAccessible') - ->will($this->returnValue(true)); $this->groupManager - ->expects($this->once()) - ->method('getSubAdmin') - ->willReturn($subadmin); + ->expects($this->never()) + ->method('getSubAdmin'); $expectedResponse = new DataResponse( [ @@ -2762,7 +2757,7 @@ class UsersControllerTest extends \Test\TestCase { ->method('getUID') ->will($this->returnValue('def')); $this->userSession - ->expects($this->exactly(2)) + ->expects($this->exactly(1)) ->method('getUser') ->will($this->returnValue($user)); $this->userManager @@ -2771,14 +2766,9 @@ class UsersControllerTest extends \Test\TestCase { ->with('abc') ->willReturn(null); - $subadmin = $this->createMock('\OC\SubAdmin'); - $subadmin->expects($this->once()) - ->method('isUserAccessible') - ->will($this->returnValue(true)); $this->groupManager - ->expects($this->once()) - ->method('getSubAdmin') - ->willReturn($subadmin); + ->expects($this->never()) + ->method('getSubAdmin'); $expectedResponse = new DataResponse( [ From 8f5f26c88d53b13a9ea7e5d9fe531decf0356879 Mon Sep 17 00:00:00 2001 From: Christoph Wurst Date: Tue, 18 Apr 2017 08:56:06 +0200 Subject: [PATCH 15/18] Use short array syntax Signed-off-by: Christoph Wurst --- settings/Controller/UsersController.php | 223 ++++++++++++------------ 1 file changed, 111 insertions(+), 112 deletions(-) diff --git a/settings/Controller/UsersController.php b/settings/Controller/UsersController.php index 56944ced98..277263b375 100644 --- a/settings/Controller/UsersController.php +++ b/settings/Controller/UsersController.php @@ -338,12 +338,12 @@ class UsersController extends Controller { * @param string $email * @return DataResponse */ - public function create($username, $password, array $groups=array(), $email='') { + public function create($username, $password, array $groups=[], $email='') { if($email !== '' && !$this->mailer->validateMailAddress($email)) { return new DataResponse( - array( + [ 'message' => (string)$this->l10n->t('Invalid mail address') - ), + ], Http::STATUS_UNPROCESSABLE_ENTITY ); } @@ -367,9 +367,9 @@ class UsersController extends Controller { if (empty($groups)) { return new DataResponse( - array( + [ 'message' => $this->l10n->t('No valid group selected'), - ), + ], Http::STATUS_FORBIDDEN ); } @@ -377,9 +377,9 @@ class UsersController extends Controller { if ($this->userManager->userExists($username)) { return new DataResponse( - array( + [ 'message' => (string)$this->l10n->t('A user with that name already exists.') - ), + ], Http::STATUS_CONFLICT ); } @@ -388,9 +388,9 @@ class UsersController extends Controller { if ($password === '') { if ($email === '') { return new DataResponse( - array( + [ 'message' => (string)$this->l10n->t('To send a password link to the user an email address is required.') - ), + ], Http::STATUS_UNPROCESSABLE_ENTITY ); } @@ -407,9 +407,9 @@ class UsersController extends Controller { $message = $this->l10n->t('Unable to create user.'); } return new DataResponse( - array( + [ 'message' => (string) $message, - ), + ], Http::STATUS_FORBIDDEN ); } @@ -434,7 +434,7 @@ class UsersController extends Controller { $emailTemplate = $this->newUserMailHelper->generateTemplate($user, $generatePasswordResetToken); $this->newUserMailHelper->sendMail($user, $emailTemplate); } catch(\Exception $e) { - $this->log->error("Can't send new user mail to $email: " . $e->getMessage(), array('app' => 'settings')); + $this->log->error("Can't send new user mail to $email: " . $e->getMessage(), ['app' => 'settings']); } } // fetch users groups @@ -447,9 +447,9 @@ class UsersController extends Controller { } return new DataResponse( - array( - 'message' => (string)$this->l10n->t('Unable to create user.') - ), + [ + 'message' => (string) $this->l10n->t('Unable to create user.') + ], Http::STATUS_FORBIDDEN ); @@ -468,24 +468,24 @@ class UsersController extends Controller { if($userId === $id) { return new DataResponse( - array( + [ 'status' => 'error', - 'data' => array( - 'message' => (string)$this->l10n->t('Unable to delete user.') - ) - ), + 'data' => [ + 'message' => (string) $this->l10n->t('Unable to delete user.') + ] + ], Http::STATUS_FORBIDDEN ); } if(!$this->isAdmin && !$this->groupManager->getSubAdmin()->isUserAccessible($this->userSession->getUser(), $user)) { return new DataResponse( - array( + [ 'status' => 'error', - 'data' => array( + 'data' => [ 'message' => (string)$this->l10n->t('Authentication error') - ) - ), + ] + ], Http::STATUS_FORBIDDEN ); } @@ -493,24 +493,24 @@ class UsersController extends Controller { if($user) { if($user->delete()) { return new DataResponse( - array( + [ 'status' => 'success', - 'data' => array( + 'data' => [ 'username' => $id - ) - ), + ] + ], Http::STATUS_NO_CONTENT ); } } return new DataResponse( - array( + [ 'status' => 'error', - 'data' => array( + 'data' => [ 'message' => (string)$this->l10n->t('Unable to delete user.') - ) - ), + ] + ], Http::STATUS_FORBIDDEN ); } @@ -525,49 +525,48 @@ class UsersController extends Controller { $userId = $this->userSession->getUser()->getUID(); $user = $this->userManager->get($id); - if($userId === $id) { + if ($userId === $id) { return new DataResponse( - array( + [ 'status' => 'error', - 'data' => array( - 'message' => (string)$this->l10n->t('Error while disabling user.') - ) - ), - Http::STATUS_FORBIDDEN + 'data' => [ + 'message' => (string) $this->l10n->t('Error while disabling user.') + ] + ], Http::STATUS_FORBIDDEN ); } - if($user) { + if ($user) { if(!$this->isAdmin && !$this->groupManager->getSubAdmin()->isUserAccessible($this->userSession->getUser(), $user)) { return new DataResponse( - array( + [ 'status' => 'error', - 'data' => array( - 'message' => (string)$this->l10n->t('Authentication error') - ) - ), + 'data' => [ + 'message' => (string) $this->l10n->t('Authentication error') + ] + ], Http::STATUS_FORBIDDEN ); } $user->setEnabled(false); return new DataResponse( - array( + [ 'status' => 'success', - 'data' => array( + 'data' => [ 'username' => $id, 'enabled' => 0 - ) - ) + ] + ] ); } else { return new DataResponse( - array( + [ 'status' => 'error', - 'data' => array( - 'message' => (string)$this->l10n->t('Error while disabling user.') - ) - ), + 'data' => [ + 'message' => (string) $this->l10n->t('Error while disabling user.') + ] + ], Http::STATUS_FORBIDDEN ); } @@ -583,49 +582,49 @@ class UsersController extends Controller { $userId = $this->userSession->getUser()->getUID(); $user = $this->userManager->get($id); - if($userId === $id) { + if ($userId === $id) { return new DataResponse( - array( + [ 'status' => 'error', - 'data' => array( - 'message' => (string)$this->l10n->t('Error while enabling user.') - ) - ), + 'data' => [ + 'message' => (string) $this->l10n->t('Error while enabling user.') + ] + ], Http::STATUS_FORBIDDEN ); } if($user) { - if(!$this->isAdmin && !$this->groupManager->getSubAdmin()->isUserAccessible($this->userSession->getUser(), $user)) { + if (!$this->isAdmin && !$this->groupManager->getSubAdmin()->isUserAccessible($this->userSession->getUser(), $user)) { return new DataResponse( - array( + [ 'status' => 'error', - 'data' => array( - 'message' => (string)$this->l10n->t('Authentication error') - ) - ), + 'data' => [ + 'message' => (string) $this->l10n->t('Authentication error') + ] + ], Http::STATUS_FORBIDDEN ); } $user->setEnabled(true); return new DataResponse( - array( + [ 'status' => 'success', - 'data' => array( + 'data' => [ 'username' => $id, 'enabled' => 1 - ) - ) + ] + ] ); } else { return new DataResponse( - array( + [ 'status' => 'error', - 'data' => array( - 'message' => (string)$this->l10n->t('Error while enabling user.') - ) - ), + 'data' => [ + 'message' => (string) $this->l10n->t('Error while enabling user.') + ] + ], Http::STATUS_FORBIDDEN ); } @@ -639,7 +638,7 @@ class UsersController extends Controller { * @return DataResponse */ public function setEnabled($id, $enabled) { - if((bool)$enabled) { + if ((bool) $enabled) { return $this->enable($id); } else { return $this->disable($id); @@ -771,14 +770,14 @@ class UsersController extends Controller { $twitterScope ) { - if(!empty($email) && !$this->mailer->validateMailAddress($email)) { + if (!empty($email) && !$this->mailer->validateMailAddress($email)) { return new DataResponse( - array( + [ 'status' => 'error', - 'data' => array( - 'message' => (string)$this->l10n->t('Invalid mail address') - ) - ), + 'data' => [ + 'message' => (string) $this->l10n->t('Invalid mail address') + ] + ], Http::STATUS_UNPROCESSABLE_ENTITY ); } @@ -798,9 +797,9 @@ class UsersController extends Controller { try { $this->saveUserSettings($user, $data); return new DataResponse( - array( + [ 'status' => 'success', - 'data' => array( + 'data' => [ 'userId' => $user->getUID(), 'avatarScope' => $avatarScope, 'displayname' => $displayname, @@ -811,9 +810,9 @@ class UsersController extends Controller { 'websiteScope' => $websiteScope, 'address' => $address, 'addressScope' => $addressScope, - 'message' => (string)$this->l10n->t('Settings saved') - ) - ), + 'message' => (string) $this->l10n->t('Settings saved') + ] + ], Http::STATUS_OK ); } catch (ForbiddenException $e) { @@ -979,36 +978,36 @@ class UsersController extends Controller { && !$this->groupManager->getSubAdmin()->isUserAccessible($this->userSession->getUser(), $user) ) { return new DataResponse( - array( + [ 'status' => 'error', - 'data' => array( - 'message' => (string)$this->l10n->t('Forbidden') - ) - ), + 'data' => [ + 'message' => (string) $this->l10n->t('Forbidden') + ] + ], Http::STATUS_FORBIDDEN ); } if($mailAddress !== '' && !$this->mailer->validateMailAddress($mailAddress)) { return new DataResponse( - array( + [ 'status' => 'error', - 'data' => array( - 'message' => (string)$this->l10n->t('Invalid mail address') - ) - ), + 'data' => [ + 'message' => (string) $this->l10n->t('Invalid mail address') + ] + ], Http::STATUS_UNPROCESSABLE_ENTITY ); } if (!$user) { return new DataResponse( - array( + [ 'status' => 'error', - 'data' => array( - 'message' => (string)$this->l10n->t('Invalid user') - ) - ), + 'data' => [ + 'message' => (string) $this->l10n->t('Invalid user') + ] + ], Http::STATUS_UNPROCESSABLE_ENTITY ); } @@ -1016,12 +1015,12 @@ class UsersController extends Controller { // for the permission of setting a email address if (!$user->canChangeDisplayName()) { return new DataResponse( - array( + [ 'status' => 'error', - 'data' => array( - 'message' => (string)$this->l10n->t('Unable to change mail address') - ) - ), + 'data' => [ + 'message' => (string) $this->l10n->t('Unable to change mail address') + ] + ], Http::STATUS_FORBIDDEN ); } @@ -1032,14 +1031,14 @@ class UsersController extends Controller { try { $this->saveUserSettings($user, $userData); return new DataResponse( - array( + [ 'status' => 'success', - 'data' => array( + 'data' => [ 'username' => $id, 'mailAddress' => $mailAddress, - 'message' => (string)$this->l10n->t('Email saved') - ) - ), + 'message' => (string) $this->l10n->t('Email saved') + ] + ], Http::STATUS_OK ); } catch (ForbiddenException $e) { From 668fe7df51e097a762d9f03e0329a06d0751cd78 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Wed, 26 Apr 2017 12:37:48 +0200 Subject: [PATCH 16/18] UserManager can now count disabled users Users page takes advantage of that Signed-off-by: Arthur Schiwon --- lib/private/User/Manager.php | 22 ++++++++++++++++++++++ lib/public/IUserManager.php | 8 ++++++++ settings/users.php | 13 +++++-------- tests/lib/User/ManagerTest.php | 25 +++++++++++++++++++++++++ 4 files changed, 60 insertions(+), 8 deletions(-) diff --git a/lib/private/User/Manager.php b/lib/private/User/Manager.php index 4a87dc7161..0477f23e55 100644 --- a/lib/private/User/Manager.php +++ b/lib/private/User/Manager.php @@ -408,6 +408,28 @@ class Manager extends PublicEmitter implements IUserManager { } } + /** + * returns how many users have logged in once + * + * @return int + * @since 12.0.0 + */ + public function countDisabledUsers() { + $queryBuilder = \OC::$server->getDatabaseConnection()->getQueryBuilder(); + $queryBuilder->select($queryBuilder->createFunction('COUNT(*)')) + ->from('preferences') + ->where($queryBuilder->expr()->eq('appid', $queryBuilder->createNamedParameter('core'))) + ->andWhere($queryBuilder->expr()->eq('configkey', $queryBuilder->createNamedParameter('enabled'))) + ->andWhere($queryBuilder->expr()->eq('configvalue', $queryBuilder->createNamedParameter('false'))); + + $query = $queryBuilder->execute(); + + $result = (int)$query->fetchColumn(); + $query->closeCursor(); + + return $result; + } + /** * returns how many users have logged in once * diff --git a/lib/public/IUserManager.php b/lib/public/IUserManager.php index 1ec392dfd8..6c6724487d 100644 --- a/lib/public/IUserManager.php +++ b/lib/public/IUserManager.php @@ -154,6 +154,14 @@ interface IUserManager { */ public function callForAllUsers(\Closure $callback, $search = ''); + /** + * returns how many users have logged in once + * + * @return int + * @since 11.0.0 + */ + public function countDisabledUsers(); + /** * returns how many users have logged in once * diff --git a/settings/users.php b/settings/users.php index 9f3433aa33..4d214bf950 100644 --- a/settings/users.php +++ b/settings/users.php @@ -45,6 +45,7 @@ $groupManager = \OC::$server->getGroupManager(); // Set the sort option: SORT_USERCOUNT or SORT_GROUPNAME $sortGroupsBy = \OC\Group\MetaData::SORT_USERCOUNT; +$isLDAPUsed = false; if (\OC_App::isEnabled('user_ldap')) { $isLDAPUsed = $groupManager->isBackendUsed('\OCA\User_LDAP\Group_LDAP') @@ -95,17 +96,13 @@ if($isAdmin) { } $subAdmins = false; } -$disabledUsers = 0; -foreach (OC_User::getUsers() as $uid) { - if(!$userManager->get($uid)->isEnabled()) { - $disabledUsers++; - } -} -$disabledUsersGroup = array( + +$disabledUsers = $isLDAPUsed ? 0 : $userManager->countDisabledUsers(); +$disabledUsersGroup = [ 'id' => '_disabledUsers', 'name' => '_disabledUsers', 'usercount' => $disabledUsers -); +]; // load preset quotas $quotaPreset=$config->getAppValue('files', 'quota_preset', '1 GB, 5 GB, 10 GB'); diff --git a/tests/lib/User/ManagerTest.php b/tests/lib/User/ManagerTest.php index aaee64f186..cf725aae67 100644 --- a/tests/lib/User/ManagerTest.php +++ b/tests/lib/User/ManagerTest.php @@ -510,6 +510,31 @@ class ManagerTest extends TestCase { $this->assertEquals(7 + 16, $users); } + public function testCountUsersOnlyDisabled() { + $manager = \OC::$server->getUserManager(); + // count other users in the db before adding our own + $countBefore = $manager->countDisabledUsers(); + + //Add test users + $user1 = $manager->createUser('testdisabledcount1', 'testdisabledcount1'); + + $user2 = $manager->createUser('testdisabledcount2', 'testdisabledcount2'); + $user2->setEnabled(false); + + $user3 = $manager->createUser('testdisabledcount3', 'testdisabledcount3'); + + $user4 = $manager->createUser('testdisabledcount4', 'testdisabledcount4'); + $user4->setEnabled(false); + + $this->assertEquals($countBefore + 2, $manager->countDisabledUsers()); + + //cleanup + $user1->delete(); + $user2->delete(); + $user3->delete(); + $user4->delete(); + } + public function testCountUsersOnlySeen() { $manager = \OC::$server->getUserManager(); // count other users in the db before adding our own From 99e97f135de73de85550e167feb7ccc80f1fc13d Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Wed, 26 Apr 2017 12:45:23 +0200 Subject: [PATCH 17/18] consolidate setEnabled method and fix a unit test Signed-off-by: Arthur Schiwon --- settings/Controller/UsersController.php | 90 +++---------------- .../Controller/UsersControllerTest.php | 28 +++--- 2 files changed, 29 insertions(+), 89 deletions(-) diff --git a/settings/Controller/UsersController.php b/settings/Controller/UsersController.php index 277263b375..4fed265594 100644 --- a/settings/Controller/UsersController.php +++ b/settings/Controller/UsersController.php @@ -519,9 +519,17 @@ class UsersController extends Controller { * @NoAdminRequired * * @param string $id + * @param int $enabled * @return DataResponse */ - public function disable($id) { + public function setEnabled($id, $enabled) { + $enabled = (bool)$enabled; + if($enabled) { + $errorMsgGeneral = (string) $this->l10n->t('Error while enabling user.'); + } else { + $errorMsgGeneral = (string) $this->l10n->t('Error while disabling user.'); + } + $userId = $this->userSession->getUser()->getUID(); $user = $this->userManager->get($id); @@ -530,70 +538,12 @@ class UsersController extends Controller { [ 'status' => 'error', 'data' => [ - 'message' => (string) $this->l10n->t('Error while disabling user.') + 'message' => $errorMsgGeneral ] ], Http::STATUS_FORBIDDEN ); } - if ($user) { - if(!$this->isAdmin && !$this->groupManager->getSubAdmin()->isUserAccessible($this->userSession->getUser(), $user)) { - return new DataResponse( - [ - 'status' => 'error', - 'data' => [ - 'message' => (string) $this->l10n->t('Authentication error') - ] - ], - Http::STATUS_FORBIDDEN - ); - } - - $user->setEnabled(false); - return new DataResponse( - [ - 'status' => 'success', - 'data' => [ - 'username' => $id, - 'enabled' => 0 - ] - ] - ); - } else { - return new DataResponse( - [ - 'status' => 'error', - 'data' => [ - 'message' => (string) $this->l10n->t('Error while disabling user.') - ] - ], - Http::STATUS_FORBIDDEN - ); - } - } - - /** - * @NoAdminRequired - * - * @param string $id - * @return DataResponse - */ - public function enable($id) { - $userId = $this->userSession->getUser()->getUID(); - $user = $this->userManager->get($id); - - if ($userId === $id) { - return new DataResponse( - [ - 'status' => 'error', - 'data' => [ - 'message' => (string) $this->l10n->t('Error while enabling user.') - ] - ], - Http::STATUS_FORBIDDEN - ); - } - if($user) { if (!$this->isAdmin && !$this->groupManager->getSubAdmin()->isUserAccessible($this->userSession->getUser(), $user)) { return new DataResponse( @@ -607,13 +557,13 @@ class UsersController extends Controller { ); } - $user->setEnabled(true); + $user->setEnabled($enabled); return new DataResponse( [ 'status' => 'success', 'data' => [ 'username' => $id, - 'enabled' => 1 + 'enabled' => $enabled ] ] ); @@ -622,27 +572,13 @@ class UsersController extends Controller { [ 'status' => 'error', 'data' => [ - 'message' => (string) $this->l10n->t('Error while enabling user.') + 'message' => $errorMsgGeneral ] ], Http::STATUS_FORBIDDEN ); } - } - /** - * @NoAdminRequired - * - * @param string $id - * @param int $enabled - * @return DataResponse - */ - public function setEnabled($id, $enabled) { - if ((bool) $enabled) { - return $this->enable($id); - } else { - return $this->disable($id); - } } /** diff --git a/tests/Settings/Controller/UsersControllerTest.php b/tests/Settings/Controller/UsersControllerTest.php index 2401980049..5905023e96 100644 --- a/tests/Settings/Controller/UsersControllerTest.php +++ b/tests/Settings/Controller/UsersControllerTest.php @@ -2299,6 +2299,9 @@ class UsersControllerTest extends \Test\TestCase { ->expects($this->once()) ->method('getBackendClassName') ->willReturn('bar'); + $user + ->method('isEnabled') + ->willReturn(true); $this->userManager ->expects($this->once()) @@ -2348,6 +2351,7 @@ class UsersControllerTest extends \Test\TestCase { 'email' => 'abc@example.org', 'isRestoreDisabled' => false, 'isAvatarAvailable' => true, + 'isEnabled' => true, ], Http::STATUS_CREATED ); @@ -2460,7 +2464,7 @@ class UsersControllerTest extends \Test\TestCase { ], Http::STATUS_FORBIDDEN ); - $response = $this->getController(true)->disable('abc'); + $response = $this->getController(true)->setEnabled('abc', false); $this->assertEquals($expectedResponse, $response); } @@ -2502,7 +2506,7 @@ class UsersControllerTest extends \Test\TestCase { ], Http::STATUS_FORBIDDEN ); - $response = $this->getController(false)->disable('abc'); + $response = $this->getController(false)->setEnabled('abc', false); $this->assertEquals($expectedResponse, $response); } @@ -2535,7 +2539,7 @@ class UsersControllerTest extends \Test\TestCase { ], Http::STATUS_FORBIDDEN ); - $response = $this->getController(true)->disable('abc'); + $response = $this->getController(true)->setEnabled('abc', false); $this->assertEquals($expectedResponse, $response); } @@ -2568,7 +2572,7 @@ class UsersControllerTest extends \Test\TestCase { ], Http::STATUS_FORBIDDEN ); - $response = $this->getController(false)->disable('abc'); + $response = $this->getController(false)->setEnabled('abc', false); $this->assertEquals($expectedResponse, $response); } @@ -2606,7 +2610,7 @@ class UsersControllerTest extends \Test\TestCase { ], ] ); - $response = $this->getController(true)->disable('abc'); + $response = $this->getController(true)->setEnabled('abc', false); $this->assertEquals($expectedResponse, $response); } @@ -2648,7 +2652,7 @@ class UsersControllerTest extends \Test\TestCase { ], ] ); - $response = $this->getController(false)->disable('abc'); + $response = $this->getController(false)->setEnabled('abc', false); $this->assertEquals($expectedResponse, $response); } @@ -2671,7 +2675,7 @@ class UsersControllerTest extends \Test\TestCase { ], Http::STATUS_FORBIDDEN ); - $response = $this->getController(true)->enable('abc'); + $response = $this->getController(true)->setEnabled('abc', true); $this->assertEquals($expectedResponse, $response); } @@ -2713,7 +2717,7 @@ class UsersControllerTest extends \Test\TestCase { ], Http::STATUS_FORBIDDEN ); - $response = $this->getController(false)->enable('abc'); + $response = $this->getController(false)->setEnabled('abc', true); $this->assertEquals($expectedResponse, $response); } @@ -2746,7 +2750,7 @@ class UsersControllerTest extends \Test\TestCase { ], Http::STATUS_FORBIDDEN ); - $response = $this->getController(true)->enable('abc'); + $response = $this->getController(true)->setEnabled('abc', true); $this->assertEquals($expectedResponse, $response); } @@ -2779,7 +2783,7 @@ class UsersControllerTest extends \Test\TestCase { ], Http::STATUS_FORBIDDEN ); - $response = $this->getController(false)->enable('abc'); + $response = $this->getController(false)->setEnabled('abc', true); $this->assertEquals($expectedResponse, $response); } @@ -2816,7 +2820,7 @@ class UsersControllerTest extends \Test\TestCase { ], ] ); - $response = $this->getController(true)->enable('abc'); + $response = $this->getController(true)->setEnabled('abc', true); $this->assertEquals($expectedResponse, $response); } @@ -2859,7 +2863,7 @@ class UsersControllerTest extends \Test\TestCase { ], ] ); - $response = $this->getController(false)->enable('abc'); + $response = $this->getController(false)->setEnabled('abc', true); $this->assertEquals($expectedResponse, $response); } } From c0e4fd0605318f02929ce1366b0fb884f26681a3 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Fri, 28 Apr 2017 17:47:34 +0200 Subject: [PATCH 18/18] =?UTF-8?q?align=20popovermenu=20next=20to=20?= =?UTF-8?q?=E2=80=A6=20buttonthingy?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Arthur Schiwon --- settings/css/settings.css | 1 + 1 file changed, 1 insertion(+) diff --git a/settings/css/settings.css b/settings/css/settings.css index 7fb1933bc9..0777f7e4cf 100644 --- a/settings/css/settings.css +++ b/settings/css/settings.css @@ -505,6 +505,7 @@ span.usersLastLoginTooltip { white-space: nowrap; } .bubble { z-index:1; right: -6px; + top: auto; } .bubble:after { right: 5px;