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 | 43 +++++- 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, 324 insertions(+), 41 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 0000000000000..c236d9fc1df0d --- /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 0000000000000..d99463b55c499 --- /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 7f6602a510c11..5949d9c974618 100644 --- a/settings/Controller/UsersController.php +++ b/settings/Controller/UsersController.php @@ -225,6 +225,7 @@ private function formatUserForIndex(IUser $user, array $userGroups = null) { 'email' => $displayName, 'isRestoreDisabled' => !$restorePossible, 'isAvatarAvailable' => $avatarAvailable, + 'isEnabled' => $user->isEnabled(), ]; } @@ -253,11 +254,6 @@ private function getUsersForUID(array $userIDs) { * 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 @@ public function index($offset = 0, $limit = 10, $gid = '', $pattern = '', $backe $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 @@ public function index($offset = 0, $limit = 10, $gid = '', $pattern = '', $backe $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 @@ public function index($offset = 0, $limit = 10, $gid = '', $pattern = '', $backe $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 @@ public function destroy($id) { } /** + * @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 5693224b94cc9..4a245fcf52e9c 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,14 +502,42 @@ span.usersLastLoginTooltip { white-space: nowrap; } display: none; } +.bubble { + z-index:1; + right: -5px; +} +.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.remove>a, tr:hover>td.password>img,tr:hover>td.displayName>img, tr:hover>td.quota>img { visibility:visible; cursor:pointer; } -td.remove { +tr:hover>td.password>img,tr:hover>td.displayName>img, tr:hover>td.quota>img { visibility:visible; cursor:pointer; } +td.userActions { width: 25px; } -tr:hover>td.remove>a { - float: left; -} div.recoveryPassword { left:50em; display:block; position:absolute; top:-1px; } input#recoveryPassword {width:15em;} diff --git a/settings/js/users/groups.js b/settings/js/users/groups.js index aac1609bce7a1..b54f26b4ccbbf 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 46f4e5977bef0..552d28ad5058c 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 ba0761856d4a1..fb85b11f390e6 100644 --- a/settings/routes.php +++ b/settings/routes.php @@ -53,6 +53,7 @@ ['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 beb0f275a33de..0b953c0298f87 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 7e7e1561e2fc6..b908109ad2d9f 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 1986592af757b..c3b32d0473e2f 100644 --- a/settings/users.php +++ b/settings/users.php @@ -59,12 +59,15 @@ $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 @@ } $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->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 4a245fcf52e9c..8f4ab9039f892 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 589c5e97ee0d9..2491d72190090 100644 --- a/tests/Settings/Controller/UsersControllerTest.php +++ b/tests/Settings/Controller/UsersControllerTest.php @@ -211,6 +211,9 @@ public function testIndexAdmin() { ->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 @@ public function testIndexAdmin() { ->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 @@ public function testIndexAdmin() { ->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 @@ public function testIndexAdmin() { 'email' => 'foo@bar.com', 'isRestoreDisabled' => false, 'isAvatarAvailable' => true, + 'isEnabled' => true, ), 1 => array( 'name' => 'admin', @@ -343,6 +353,7 @@ public function testIndexAdmin() { 'email' => 'admin@bar.com', 'isRestoreDisabled' => false, 'isAvatarAvailable' => false, + 'isEnabled' => true, ), 2 => array( 'name' => 'bar', @@ -356,6 +367,7 @@ public function testIndexAdmin() { 'email' => 'bar@dummy.com', 'isRestoreDisabled' => false, 'isAvatarAvailable' => true, + 'isEnabled' => true, ), ) ); @@ -399,6 +411,9 @@ public function testIndexSubAdmin() { ->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 @@ public function testIndexSubAdmin() { ->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 @@ public function testIndexSubAdmin() { ->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 @@ public function testIndexSubAdmin() { 'email' => 'bar@dummy.com', 'isRestoreDisabled' => false, 'isAvatarAvailable' => true, + 'isEnabled' => true, ], 1=> [ 'name' => 'foo', @@ -545,6 +567,7 @@ public function testIndexSubAdmin() { 'email' => 'foo@bar.com', 'isRestoreDisabled' => false, 'isAvatarAvailable' => true, + 'isEnabled' => true, ], 2 => [ 'name' => 'admin', @@ -558,6 +581,7 @@ public function testIndexSubAdmin() { 'email' => 'admin@bar.com', 'isRestoreDisabled' => false, 'isAvatarAvailable' => false, + 'isEnabled' => true, ], ] ); @@ -600,6 +624,9 @@ public function testIndexWithSearch() { ->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 @@ public function testIndexWithSearch() { ->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 @@ public function testIndexWithSearch() { ->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 @@ public function testIndexWithSearch() { 'email' => 'foo@bar.com', 'isRestoreDisabled' => false, 'isAvatarAvailable' => true, + 'isEnabled' => true, ), 1 => array( 'name' => 'admin', @@ -705,6 +739,7 @@ public function testIndexWithSearch() { 'email' => 'admin@bar.com', 'isRestoreDisabled' => false, 'isAvatarAvailable' => false, + 'isEnabled' => true, ), 2 => array( 'name' => 'bar', @@ -718,6 +753,7 @@ public function testIndexWithSearch() { 'email' => 'bar@dummy.com', 'isRestoreDisabled' => false, 'isAvatarAvailable' => true, + 'isEnabled' => true, ), ) ); @@ -755,6 +791,10 @@ public function testIndexWithBackend() { ->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 @@ public function testIndexWithBackend() { 'email' => null, 'isRestoreDisabled' => false, 'isAvatarAvailable' => true, + 'isEnabled' => true, ) ) ); @@ -832,6 +873,9 @@ public function testCreateSuccessfulWithoutGroupAdmin() { ->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 @@ public function testCreateSuccessfulWithoutGroupAdmin() { 'email' => null, 'isRestoreDisabled' => false, 'isAvatarAvailable' => true, + 'isEnabled' => true, ), Http::STATUS_CREATED ); @@ -888,6 +933,9 @@ public function testCreateSuccessfulWithGroupAdmin() { ->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 @@ public function testCreateSuccessfulWithGroupAdmin() { 'email' => null, 'isRestoreDisabled' => false, 'isAvatarAvailable' => true, + 'isEnabled' => true, ), Http::STATUS_CREATED ); @@ -975,6 +1024,9 @@ public function testCreateSuccessfulWithGroupSubAdmin() { ->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 @@ public function testCreateSuccessfulWithGroupSubAdmin() { 'email' => null, 'isRestoreDisabled' => false, 'isAvatarAvailable' => true, + 'isEnabled' => true, ), Http::STATUS_CREATED ); @@ -1463,7 +1516,8 @@ public function testCreateSuccessfulWithValidEmailAdmin() { } 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 @@ private function mockUser($userId = 'foo', $displayName = 'M. Foo', ->expects($this->once()) ->method('getBackendClassName') ->will($this->returnValue($backend)); + $user->expects($this->any()) + ->method('isEnabled') + ->willReturn($enabled); $result = [ 'name' => $userId, @@ -1496,6 +1553,7 @@ private function mockUser($userId = 'foo', $displayName = 'M. Foo', '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 c236d9fc1df0d..16c59261a7334 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 d99463b55c499..f853784efc284 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 5949d9c974618..293afe9e6ff20 100644 --- a/settings/Controller/UsersController.php +++ b/settings/Controller/UsersController.php @@ -268,15 +268,15 @@ public function index($offset = 0, $limit = 10, $gid = '', $pattern = '', $backe $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 @@ public function index($offset = 0, $limit = 10, $gid = '', $pattern = '', $backe $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 @@ public function index($offset = 0, $limit = 10, $gid = '', $pattern = '', $backe $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 b54f26b4ccbbf..16621441a640a 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 552d28ad5058c..117c61b7dcbce 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 0b953c0298f87..5dfd7836f6a74 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 c3b32d0473e2f..9f3433aa33a84 100644 --- a/settings/users.php +++ b/settings/users.php @@ -102,8 +102,8 @@ } } $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 2491d72190090..14f43a9e8e357 100644 --- a/tests/Settings/Controller/UsersControllerTest.php +++ b/tests/Settings/Controller/UsersControllerTest.php @@ -273,9 +273,15 @@ public function testIndexAdmin() { ->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 @@ public function testIndexAdmin() { 'email' => 'bar@dummy.com', 'isRestoreDisabled' => false, 'isAvatarAvailable' => true, - 'isEnabled' => true, + 'isEnabled' => false, ), ) ); @@ -2217,6 +2223,11 @@ public function setEmailAddressData() { /** * @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 14f43a9e8e357..986f15e7a3211 100644 --- a/tests/Settings/Controller/UsersControllerTest.php +++ b/tests/Settings/Controller/UsersControllerTest.php @@ -2438,7 +2438,225 @@ public function testGetVerificationCodeInvalidUser() { $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 986f15e7a3211..378c98e7335af 100644 --- a/tests/Settings/Controller/UsersControllerTest.php +++ b/tests/Settings/Controller/UsersControllerTest.php @@ -2592,7 +2592,8 @@ public function testDisableUserSuccessForAdmin() { $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 @@ public function testDisableUserSuccessForSubAdmin() { $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 293afe9e6ff20..7ce4355aa0b57 100644 --- a/settings/Controller/UsersController.php +++ b/settings/Controller/UsersController.php @@ -537,19 +537,19 @@ public function disable($id) { ); } - 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 @@ public function enable($id) { ); } - 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 378c98e7335af..5cae52ece70ed 100644 --- a/tests/Settings/Controller/UsersControllerTest.php +++ b/tests/Settings/Controller/UsersControllerTest.php @@ -2460,8 +2460,7 @@ public function testDisableUserFailsDueSameUser() { ], 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 @@ public function testDisableUserFailsDueNoAdminAndNoSubadmin() { ], 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 @@ public function testDisableUserFailsDueNoUser() { ], ] ); - $controller = $this->getController(true); - $response = $controller->disable('abc'); + $response = $this->getController(true)->disable('abc'); $this->assertEquals($expectedResponse, $response); } @@ -2574,8 +2571,7 @@ public function testDisableUserFailsDueNoUserForSubAdmin() { ], ] ); - $controller = $this->getController(false); - $response = $controller->disable('abc'); + $response = $this->getController(false)->disable('abc'); $this->assertEquals($expectedResponse, $response); } @@ -2613,8 +2609,7 @@ public function testDisableUserSuccessForAdmin() { ], ] ); - $controller = $this->getController(true); - $response = $controller->disable('abc'); + $response = $this->getController(true)->disable('abc'); $this->assertEquals($expectedResponse, $response); } @@ -2656,8 +2651,7 @@ public function testDisableUserSuccessForSubAdmin() { ], ] ); - $controller = $this->getController(false); - $response = $controller->disable('abc'); + $response = $this->getController(false)->disable('abc'); $this->assertEquals($expectedResponse, $response); } @@ -2680,8 +2674,7 @@ public function testEnableUserFailsDueSameUser() { ], 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 @@ public function testEnableUserFailsDueNoAdminAndNoSubadmin() { ], 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 @@ public function testEnableUserFailsDueNoUser() { ], ] ); - $controller = $this->getController(true); - $response = $controller->enable('abc'); + $response = $this->getController(true)->enable('abc'); $this->assertEquals($expectedResponse, $response); } @@ -2794,8 +2785,7 @@ public function testEnableUserFailsDueNoUserForSubAdmin() { ], ] ); - $controller = $this->getController(false); - $response = $controller->enable('abc'); + $response = $this->getController(false)->enable('abc'); $this->assertEquals($expectedResponse, $response); } @@ -2832,8 +2822,7 @@ public function testEnableUserSuccessForAdmin() { ], ] ); - $controller = $this->getController(true); - $response = $controller->enable('abc'); + $response = $this->getController(true)->enable('abc'); $this->assertEquals($expectedResponse, $response); } @@ -2863,6 +2852,7 @@ public function testEnableUserSuccessForSubAdmin() { ->method('isUserAccessible') ->will($this->returnValue(true)); $this->groupManager + ->expects($this->once()) ->method('getSubAdmin') ->willReturn($subadmin); @@ -2875,8 +2865,7 @@ public function testEnableUserSuccessForSubAdmin() { ], ] ); - $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 8f4ab9039f892..7fb1933bc9af0 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 7ce4355aa0b57..411269a435104 100644 --- a/settings/Controller/UsersController.php +++ b/settings/Controller/UsersController.php @@ -530,7 +530,7 @@ public function disable($id) { 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 @@ public function disable($id) { 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 @@ public function enable($id) { 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 @@ public function enable($id) { 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 117c61b7dcbce..2a0b0c09a92b6 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 5cae52ece70ed..b6eba294d3629 100644 --- a/tests/Settings/Controller/UsersControllerTest.php +++ b/tests/Settings/Controller/UsersControllerTest.php @@ -2455,7 +2455,7 @@ public function testDisableUserFailsDueSameUser() { [ 'status' => 'error', 'data' => [ - 'message' => 'Unable to disable user.', + 'message' => 'Error while disabling user.', ], ], Http::STATUS_FORBIDDEN @@ -2530,7 +2530,7 @@ public function testDisableUserFailsDueNoUser() { [ 'status' => 'error', 'data' => [ - 'message' => 'Unable to disable user.', + 'message' => 'Error while disabling user.', ], ] ); @@ -2567,7 +2567,7 @@ public function testDisableUserFailsDueNoUserForSubAdmin() { [ 'status' => 'error', 'data' => [ - 'message' => 'Unable to disable user.', + 'message' => 'Error while disabling user.', ], ] ); @@ -2669,7 +2669,7 @@ public function testEnableUserFailsDueSameUser() { [ 'status' => 'error', 'data' => [ - 'message' => 'Unable to enable user.', + 'message' => 'Error while enabling user.', ], ], Http::STATUS_FORBIDDEN @@ -2744,7 +2744,7 @@ public function testEnableUserFailsDueNoUser() { [ 'status' => 'error', 'data' => [ - 'message' => 'Unable to enable user.', + 'message' => 'Error while enabling user.', ], ] ); @@ -2781,7 +2781,7 @@ public function testEnableUserFailsDueNoUserForSubAdmin() { [ '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 411269a435104..56944ced98440 100644 --- a/settings/Controller/UsersController.php +++ b/settings/Controller/UsersController.php @@ -567,7 +567,8 @@ public function disable($id) { 'data' => array( 'message' => (string)$this->l10n->t('Error while disabling user.') ) - ) + ), + Http::STATUS_FORBIDDEN ); } } @@ -624,7 +625,8 @@ public function enable($id) { '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 2a0b0c09a92b6..387709cd64cce 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 b6eba294d3629..1dd3b0c697929 100644 --- a/tests/Settings/Controller/UsersControllerTest.php +++ b/tests/Settings/Controller/UsersControllerTest.php @@ -2532,7 +2532,8 @@ public function testDisableUserFailsDueNoUser() { 'data' => [ 'message' => 'Error while disabling user.', ], - ] + ], + Http::STATUS_FORBIDDEN ); $response = $this->getController(true)->disable('abc'); $this->assertEquals($expectedResponse, $response); @@ -2569,7 +2570,8 @@ public function testDisableUserFailsDueNoUserForSubAdmin() { 'data' => [ 'message' => 'Error while disabling user.', ], - ] + ], + Http::STATUS_FORBIDDEN ); $response = $this->getController(false)->disable('abc'); $this->assertEquals($expectedResponse, $response); @@ -2746,7 +2748,8 @@ public function testEnableUserFailsDueNoUser() { 'data' => [ 'message' => 'Error while enabling user.', ], - ] + ], + Http::STATUS_FORBIDDEN ); $response = $this->getController(true)->enable('abc'); $this->assertEquals($expectedResponse, $response); @@ -2783,7 +2786,8 @@ public function testEnableUserFailsDueNoUserForSubAdmin() { '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 1dd3b0c697929..24019800490b3 100644 --- a/tests/Settings/Controller/UsersControllerTest.php +++ b/tests/Settings/Controller/UsersControllerTest.php @@ -2546,7 +2546,7 @@ public function testDisableUserFailsDueNoUserForSubAdmin() { ->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 @@ public function testDisableUserFailsDueNoUserForSubAdmin() { ->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 @@ public function testEnableUserFailsDueNoUserForSubAdmin() { ->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 @@ public function testEnableUserFailsDueNoUserForSubAdmin() { ->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 56944ced98440..277263b37536e 100644 --- a/settings/Controller/UsersController.php +++ b/settings/Controller/UsersController.php @@ -338,12 +338,12 @@ public function index($offset = 0, $limit = 10, $gid = '', $pattern = '', $backe * @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 @@ public function create($username, $password, array $groups=array(), $email='') { if (empty($groups)) { return new DataResponse( - array( + [ 'message' => $this->l10n->t('No valid group selected'), - ), + ], Http::STATUS_FORBIDDEN ); } @@ -377,9 +377,9 @@ public function create($username, $password, array $groups=array(), $email='') { 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 @@ public function create($username, $password, array $groups=array(), $email='') { 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 @@ public function create($username, $password, array $groups=array(), $email='') { $message = $this->l10n->t('Unable to create user.'); } return new DataResponse( - array( + [ 'message' => (string) $message, - ), + ], Http::STATUS_FORBIDDEN ); } @@ -434,7 +434,7 @@ public function create($username, $password, array $groups=array(), $email='') { $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 @@ public function create($username, $password, array $groups=array(), $email='') { } 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 @@ public function destroy($id) { 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 @@ public function destroy($id) { 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 @@ public function disable($id) { $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 @@ public function enable($id) { $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 @@ public function enable($id) { * @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 @@ public function setUserSettings($avatarScope, $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 @@ public function setUserSettings($avatarScope, 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 @@ public function setUserSettings($avatarScope, '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 @@ public function setEMailAddress($id, $mailAddress) { && !$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 @@ public function setEMailAddress($id, $mailAddress) { // 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 @@ public function setEMailAddress($id, $mailAddress) { 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 4a87dc7161c32..0477f23e552e0 100644 --- a/lib/private/User/Manager.php +++ b/lib/private/User/Manager.php @@ -408,6 +408,28 @@ public function callForAllUsers(\Closure $callback, $search = '', $onlySeen = fa } } + /** + * 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 1ec392dfd8264..6c6724487de3e 100644 --- a/lib/public/IUserManager.php +++ b/lib/public/IUserManager.php @@ -154,6 +154,14 @@ public function countUsers(); */ 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 9f3433aa33a84..4d214bf950226 100644 --- a/settings/users.php +++ b/settings/users.php @@ -45,6 +45,7 @@ // 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 @@ } $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 aaee64f186382..cf725aae6719b 100644 --- a/tests/lib/User/ManagerTest.php +++ b/tests/lib/User/ManagerTest.php @@ -510,6 +510,31 @@ public function testCountUsersTwoBackends() { $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 | 88 +++---------------- .../Controller/UsersControllerTest.php | 28 +++--- 2 files changed, 28 insertions(+), 88 deletions(-) diff --git a/settings/Controller/UsersController.php b/settings/Controller/UsersController.php index 277263b37536e..4fed265594044 100644 --- a/settings/Controller/UsersController.php +++ b/settings/Controller/UsersController.php @@ -519,66 +519,17 @@ public function destroy($id) { * @NoAdminRequired * * @param string $id + * @param int $enabled * @return DataResponse */ - public function disable($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 disabling user.') - ] - ], 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 - ] - ] - ); + public function setEnabled($id, $enabled) { + $enabled = (bool)$enabled; + if($enabled) { + $errorMsgGeneral = (string) $this->l10n->t('Error while enabling user.'); } else { - return new DataResponse( - [ - 'status' => 'error', - 'data' => [ - 'message' => (string) $this->l10n->t('Error while disabling user.') - ] - ], - Http::STATUS_FORBIDDEN - ); + $errorMsgGeneral = (string) $this->l10n->t('Error while disabling user.'); } - } - /** - * @NoAdminRequired - * - * @param string $id - * @return DataResponse - */ - public function enable($id) { $userId = $this->userSession->getUser()->getUID(); $user = $this->userManager->get($id); @@ -587,10 +538,9 @@ public function enable($id) { [ 'status' => 'error', 'data' => [ - 'message' => (string) $this->l10n->t('Error while enabling user.') - ] - ], - Http::STATUS_FORBIDDEN + 'message' => $errorMsgGeneral + ] + ], Http::STATUS_FORBIDDEN ); } @@ -607,13 +557,13 @@ public function enable($id) { ); } - $user->setEnabled(true); + $user->setEnabled($enabled); return new DataResponse( [ 'status' => 'success', 'data' => [ 'username' => $id, - 'enabled' => 1 + 'enabled' => $enabled ] ] ); @@ -622,27 +572,13 @@ public function enable($id) { [ '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 24019800490b3..5905023e96095 100644 --- a/tests/Settings/Controller/UsersControllerTest.php +++ b/tests/Settings/Controller/UsersControllerTest.php @@ -2299,6 +2299,9 @@ public function testCreateSuccessfulWithoutPasswordAndWithEmail() { ->expects($this->once()) ->method('getBackendClassName') ->willReturn('bar'); + $user + ->method('isEnabled') + ->willReturn(true); $this->userManager ->expects($this->once()) @@ -2348,6 +2351,7 @@ public function testCreateSuccessfulWithoutPasswordAndWithEmail() { 'email' => 'abc@example.org', 'isRestoreDisabled' => false, 'isAvatarAvailable' => true, + 'isEnabled' => true, ], Http::STATUS_CREATED ); @@ -2460,7 +2464,7 @@ public function testDisableUserFailsDueSameUser() { ], Http::STATUS_FORBIDDEN ); - $response = $this->getController(true)->disable('abc'); + $response = $this->getController(true)->setEnabled('abc', false); $this->assertEquals($expectedResponse, $response); } @@ -2502,7 +2506,7 @@ public function testDisableUserFailsDueNoAdminAndNoSubadmin() { ], Http::STATUS_FORBIDDEN ); - $response = $this->getController(false)->disable('abc'); + $response = $this->getController(false)->setEnabled('abc', false); $this->assertEquals($expectedResponse, $response); } @@ -2535,7 +2539,7 @@ public function testDisableUserFailsDueNoUser() { ], Http::STATUS_FORBIDDEN ); - $response = $this->getController(true)->disable('abc'); + $response = $this->getController(true)->setEnabled('abc', false); $this->assertEquals($expectedResponse, $response); } @@ -2568,7 +2572,7 @@ public function testDisableUserFailsDueNoUserForSubAdmin() { ], Http::STATUS_FORBIDDEN ); - $response = $this->getController(false)->disable('abc'); + $response = $this->getController(false)->setEnabled('abc', false); $this->assertEquals($expectedResponse, $response); } @@ -2606,7 +2610,7 @@ public function testDisableUserSuccessForAdmin() { ], ] ); - $response = $this->getController(true)->disable('abc'); + $response = $this->getController(true)->setEnabled('abc', false); $this->assertEquals($expectedResponse, $response); } @@ -2648,7 +2652,7 @@ public function testDisableUserSuccessForSubAdmin() { ], ] ); - $response = $this->getController(false)->disable('abc'); + $response = $this->getController(false)->setEnabled('abc', false); $this->assertEquals($expectedResponse, $response); } @@ -2671,7 +2675,7 @@ public function testEnableUserFailsDueSameUser() { ], Http::STATUS_FORBIDDEN ); - $response = $this->getController(true)->enable('abc'); + $response = $this->getController(true)->setEnabled('abc', true); $this->assertEquals($expectedResponse, $response); } @@ -2713,7 +2717,7 @@ public function testEnableUserFailsDueNoAdminAndNoSubadmin() { ], Http::STATUS_FORBIDDEN ); - $response = $this->getController(false)->enable('abc'); + $response = $this->getController(false)->setEnabled('abc', true); $this->assertEquals($expectedResponse, $response); } @@ -2746,7 +2750,7 @@ public function testEnableUserFailsDueNoUser() { ], Http::STATUS_FORBIDDEN ); - $response = $this->getController(true)->enable('abc'); + $response = $this->getController(true)->setEnabled('abc', true); $this->assertEquals($expectedResponse, $response); } @@ -2779,7 +2783,7 @@ public function testEnableUserFailsDueNoUserForSubAdmin() { ], Http::STATUS_FORBIDDEN ); - $response = $this->getController(false)->enable('abc'); + $response = $this->getController(false)->setEnabled('abc', true); $this->assertEquals($expectedResponse, $response); } @@ -2816,7 +2820,7 @@ public function testEnableUserSuccessForAdmin() { ], ] ); - $response = $this->getController(true)->enable('abc'); + $response = $this->getController(true)->setEnabled('abc', true); $this->assertEquals($expectedResponse, $response); } @@ -2859,7 +2863,7 @@ public function testEnableUserSuccessForSubAdmin() { ], ] ); - $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 7fb1933bc9af0..0777f7e4cf4ac 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;