From 4915d64de8d0ee862f0fdc92fe9bf856f3e8cbe1 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Thu, 17 Jan 2019 11:59:05 +0100 Subject: [PATCH] ignore non existing users when retrieving details of group members Signed-off-by: Arthur Schiwon --- .../lib/Controller/AUserData.php | 13 ++- .../lib/Controller/GroupsController.php | 24 +++--- .../tests/Controller/GroupsControllerTest.php | 84 +++++++++++++++++-- 3 files changed, 102 insertions(+), 19 deletions(-) diff --git a/apps/provisioning_api/lib/Controller/AUserData.php b/apps/provisioning_api/lib/Controller/AUserData.php index 864ed65e69..780d4f7a76 100644 --- a/apps/provisioning_api/lib/Controller/AUserData.php +++ b/apps/provisioning_api/lib/Controller/AUserData.php @@ -23,6 +23,7 @@ namespace OCA\Provisioning_API\Controller; use OC\Accounts\AccountManager; use OC\User\Backend; +use OC\User\NoUserException; use OCP\AppFramework\OCS\OCSException; use OCP\AppFramework\OCS\OCSNotFoundException; use OCP\AppFramework\OCSController; @@ -79,7 +80,9 @@ abstract class AUserData extends OCSController { * * @param string $userId * @return array + * @throws NotFoundException * @throws OCSException + * @throws OCSNotFoundException */ protected function getUserData(string $userId): array { $currentLoggedInUser = $this->userSession->getUser(); @@ -111,9 +114,17 @@ abstract class AUserData extends OCSController { $gids[] = $group->getGID(); } + try { + # might be thrown by LDAP due to handling of users disappears + # from the external source (reasons unknown to us) + # cf. https://github.com/nextcloud/server/issues/12991 + $data['storageLocation'] = $targetUserObject->getHome(); + } catch (NoUserException $e) { + throw new OCSNotFoundException($e->getMessage(), $e); + } + // Find the data $data['id'] = $targetUserObject->getUID(); - $data['storageLocation'] = $targetUserObject->getHome(); $data['lastLogin'] = $targetUserObject->getLastLogin() * 1000; $data['backend'] = $targetUserObject->getBackendClassName(); $data['subadmin'] = $this->getUserSubAdminGroupsData($targetUserObject->getUID()); diff --git a/apps/provisioning_api/lib/Controller/GroupsController.php b/apps/provisioning_api/lib/Controller/GroupsController.php index e52929df9c..7c4447ac4e 100644 --- a/apps/provisioning_api/lib/Controller/GroupsController.php +++ b/apps/provisioning_api/lib/Controller/GroupsController.php @@ -203,16 +203,20 @@ class GroupsController extends AUserData { // Extract required number $usersDetails = []; foreach ($users as $user) { - /** @var IUser $user */ - $userId = (string) $user->getUID(); - $userData = $this->getUserData($userId); - // Do not insert empty entry - if(!empty($userData)) { - $usersDetails[$userId] = $userData; - } else { - // Logged user does not have permissions to see this user - // only showing its id - $usersDetails[$userId] = ['id' => $userId]; + try { + /** @var IUser $user */ + $userId = (string)$user->getUID(); + $userData = $this->getUserData($userId); + // Do not insert empty entry + if (!empty($userData)) { + $usersDetails[$userId] = $userData; + } else { + // Logged user does not have permissions to see this user + // only showing its id + $usersDetails[$userId] = ['id' => $userId]; + } + } catch(OCSNotFoundException $e) { + // continue if a users ceased to exist. } } return new DataResponse(['users' => $usersDetails]); diff --git a/apps/provisioning_api/tests/Controller/GroupsControllerTest.php b/apps/provisioning_api/tests/Controller/GroupsControllerTest.php index 2ed62a6784..b22953ca04 100644 --- a/apps/provisioning_api/tests/Controller/GroupsControllerTest.php +++ b/apps/provisioning_api/tests/Controller/GroupsControllerTest.php @@ -29,6 +29,7 @@ namespace OCA\Provisioning_API\Tests\Controller; use OC\Accounts\AccountManager; use OC\Group\Manager; use OC\SubAdmin; +use OC\User\NoUserException; use OCA\Provisioning_API\Controller\GroupsController; use OCP\IConfig; use OCP\ILogger; @@ -36,27 +37,31 @@ use OCP\IRequest; use OCP\IUser; use OCP\IUserManager; use OCP\IUserSession; +use OCP\UserInterface; class GroupsControllerTest extends \Test\TestCase { - /** @var IRequest|PHPUnit_Framework_MockObject_MockObject */ + /** @var IRequest|\PHPUnit_Framework_MockObject_MockObject */ protected $request; - /** @var IUserManager|PHPUnit_Framework_MockObject_MockObject */ + /** @var IUserManager|\PHPUnit_Framework_MockObject_MockObject */ protected $userManager; - /** @var IConfig|PHPUnit_Framework_MockObject_MockObject */ + /** @var IConfig|\PHPUnit_Framework_MockObject_MockObject */ protected $config; - /** @var Manager|PHPUnit_Framework_MockObject_MockObject */ + /** @var Manager|\PHPUnit_Framework_MockObject_MockObject */ protected $groupManager; - /** @var IUserSession|PHPUnit_Framework_MockObject_MockObject */ + /** @var IUserSession|\PHPUnit_Framework_MockObject_MockObject */ protected $userSession; - /** @var AccountManager|PHPUnit_Framework_MockObject_MockObject */ + /** @var AccountManager|\PHPUnit_Framework_MockObject_MockObject */ protected $accountManager; - /** @var ILogger|PHPUnit_Framework_MockObject_MockObject */ + /** @var ILogger|\PHPUnit_Framework_MockObject_MockObject */ protected $logger; + /** @var SubAdmin|\PHPUnit_Framework_MockObject_MockObject */ + protected $subAdminManager; - /** @var GroupsController|PHPUnit_Framework_MockObject_MockObject */ + /** @var GroupsController|\PHPUnit_Framework_MockObject_MockObject */ protected $api; + protected function setUp() { parent::setUp(); @@ -126,6 +131,10 @@ class GroupsControllerTest extends \Test\TestCase { $user ->method('getUID') ->willReturn($uid); + $backendMock = $this->createMock(UserInterface::class); + $user + ->method('getBackend') + ->willReturn($backendMock); return $user; } @@ -164,6 +173,19 @@ class GroupsControllerTest extends \Test\TestCase { })); } + private function useAccountManager() { + $this->accountManager->expects($this->any()) + ->method('getUser') + ->willReturnCallback(function(IUser $user) { + return [ + AccountManager::PROPERTY_PHONE => ['value' => '0800-call-' . $user->getUID()], + AccountManager::PROPERTY_ADDRESS => ['value' => 'Holzweg 99, 0601 Herrera, Panama'], + AccountManager::PROPERTY_WEBSITE => ['value' => 'https://' . $user->getUid() . '.pa'], + AccountManager::PROPERTY_TWITTER => ['value' => '@' . $user->getUID()], + ]; + }); + } + public function dataGetGroups() { return [ [null, 0, 0], @@ -454,4 +476,50 @@ class GroupsControllerTest extends \Test\TestCase { $this->api->deleteGroup('ExistingGroup'); } + + public function testGetGroupUsersDetails() { + $gid = 'ncg1'; + + $this->asAdmin(); + $this->useAccountManager(); + + $users = [ + 'ncu1' => $this->createUser('ncu1'), # regular + 'ncu2' => $this->createUser('ncu2'), # the zombie + ]; + $users['ncu2']->expects($this->atLeastOnce()) + ->method('getHome') + ->willThrowException(new NoUserException()); + + $this->userManager->expects($this->any()) + ->method('get') + ->willReturnCallback(function(string $uid) use ($users) { + return isset($users[$uid]) ? $users[$uid] : null; + }); + + $group = $this->createGroup($gid); + $group->expects($this->once()) + ->method('searchUsers') + ->with('', null, 0) + ->willReturn(array_values($users)); + + $this->groupManager + ->method('get') + ->with($gid) + ->willReturn($group); + $this->groupManager->expects($this->any()) + ->method('getUserGroups') + ->willReturn([$group]); + + /** @var \PHPUnit_Framework_MockObject_MockObject */ + $this->subAdminManager->expects($this->any()) + ->method('isSubAdminOfGroup') + ->willReturn(false); + $this->subAdminManager->expects($this->any()) + ->method('getSubAdminsGroups') + ->willReturn([]); + + + $this->api->getGroupUsersDetails($gid); + } }