From 35dc2235001bf61f07c78b50e74ca029bb9fc05d Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Mon, 13 Jun 2022 18:48:49 +0200 Subject: [PATCH 1/8] Optimize retrieving display name when searching for users in a group This is recurrent scenario that we are searching for users and then for each users we fetch the displayName. This is inefficient, so instead try to do one query to fetch everything (e.g. Database backend) or use the already existing DisplayNameCache helper. Signed-off-by: Carl Schwan --- apps/user_ldap/lib/Group_LDAP.php | 17 +++++++-- apps/user_ldap/lib/Group_Proxy.php | 4 ++ build/psalm-baseline-ocp.xml | 5 +++ lib/private/Group/Database.php | 52 +++++++++++++++++++++++++ lib/private/Group/Group.php | 17 +++------ lib/private/User/LazyUser.php | 20 ++++++++-- lib/private/User/Manager.php | 29 ++++++-------- lib/public/Group/Backend/ABackend.php | 15 ++++++++ lib/public/GroupInterface.php | 24 +++++++++++- lib/public/IGroup.php | 2 +- tests/lib/Group/GroupTest.php | 24 ++++++------ tests/lib/Group/ManagerTest.php | 55 ++++++--------------------- tests/lib/Util/Group/Dummy.php | 46 ++++++++++++++++++---- 13 files changed, 210 insertions(+), 100 deletions(-) diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php index b32e031175ff9..ca3a6d13e8fa4 100644 --- a/apps/user_ldap/lib/Group_LDAP.php +++ b/apps/user_ldap/lib/Group_LDAP.php @@ -45,14 +45,15 @@ namespace OCA\User_LDAP; use Exception; +use OC\ServerNotAvailableException; use OCP\Cache\CappedMemoryCache; use OCP\GroupInterface; +use OCP\Group\Backend\ABackend; use OCP\Group\Backend\IDeleteGroupBackend; use OCP\Group\Backend\IGetDisplayNameBackend; -use OC\ServerNotAvailableException; use Psr\Log\LoggerInterface; -class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, IGetDisplayNameBackend, IDeleteGroupBackend { +class Group_LDAP extends ABackend implements GroupInterface, IGroupLDAP, IGetDisplayNameBackend, IDeleteGroupBackend { protected bool $enabled = false; /** @var CappedMemoryCache $cachedGroupMembers array of users with gid as key */ @@ -63,6 +64,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I protected CappedMemoryCache $cachedNestedGroups; protected GroupPluginManager $groupPluginManager; protected LoggerInterface $logger; + protected Access $access; /** * @var string $ldapGroupMemberAssocAttr contains the LDAP setting (in lower case) with the same name @@ -70,7 +72,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I protected string $ldapGroupMemberAssocAttr; public function __construct(Access $access, GroupPluginManager $groupPluginManager) { - parent::__construct($access); + $this->access = $access; $filter = $this->access->connection->ldapGroupFilter; $gAssoc = $this->access->connection->ldapGroupMemberAssocAttr; if (!empty($filter) && !empty($gAssoc)) { @@ -1163,7 +1165,7 @@ protected function filterValidGroups(array $listOfGroups): array { * Returns the supported actions as int to be * compared with GroupInterface::CREATE_GROUP etc. */ - public function implementsActions($actions) { + public function implementsActions($actions): bool { return (bool)((GroupInterface::COUNT_USERS | GroupInterface::DELETE_GROUP | $this->groupPluginManager->getImplementedActions()) & $actions); @@ -1331,4 +1333,11 @@ public function getDisplayName(string $gid): string { $this->access->connection->writeToCache($cacheKey, $displayName); return $displayName; } + + public function searchInGroup(string $gid, string $search = '', int $limit = -1, int $offset = 0): array { + if (!$this->enabled) { + return []; + } + return parent::searchInGroup($gid, $search, $limit, $offset); + } } diff --git a/apps/user_ldap/lib/Group_Proxy.php b/apps/user_ldap/lib/Group_Proxy.php index c8c986318ec7e..74655840f479c 100644 --- a/apps/user_ldap/lib/Group_Proxy.php +++ b/apps/user_ldap/lib/Group_Proxy.php @@ -334,4 +334,8 @@ public function getDisplayName(string $gid): string { public function getBackendName(): string { return 'LDAP'; } + + public function searchInGroup(string $gid, string $search = '', int $limit = -1, int $offset = 0): array { + return $this->handleRequest($gid, 'searchInGroup', [$gid, $search, $limit, $offset]); + } } diff --git a/build/psalm-baseline-ocp.xml b/build/psalm-baseline-ocp.xml index e993e85e5a247..1613a6fe8fab9 100644 --- a/build/psalm-baseline-ocp.xml +++ b/build/psalm-baseline-ocp.xml @@ -5,6 +5,11 @@ OC + + + OC + + $this->request->server diff --git a/lib/private/Group/Database.php b/lib/private/Group/Database.php index 5f17477db772b..bfc95c574e2c5 100644 --- a/lib/private/Group/Database.php +++ b/lib/private/Group/Database.php @@ -43,6 +43,9 @@ use OCP\Group\Backend\ISetDisplayNameBackend; use OCP\Group\Backend\INamedBackend; use OCP\IDBConnection; +use OCP\IUserManager; +use OC\User\LazyUser; +use OC\User\DisplayNameCache; /** * Class for group management in a SQL Database (e.g. MySQL, SQLite) @@ -376,6 +379,55 @@ public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0) { return $users; } + public function searchInGroup(string $gid, string $search = '', int $limit = -1, int $offset = 0): array { + $this->fixDI(); + + $query = $this->dbConn->getQueryBuilder(); + $query->select('g.uid', 'u.displayname'); + + $query->from('group_user', 'g') + ->where($query->expr()->eq('gid', $query->createNamedParameter($gid))) + ->orderBy('g.uid', 'ASC'); + + $query->leftJoin('g', 'users', 'u', $query->expr()->eq('g.uid', 'u.uid')) + + if ($search !== '') { + $query->leftJoin('u', 'preferences', 'p', $query->expr()->andX( + $query->expr()->eq('p.userid', 'u.uid'), + $query->expr()->eq('p.appid', $query->expr()->literal('settings')), + $query->expr()->eq('p.configkey', $query->expr()->literal('email')) + )) + // sqlite doesn't like re-using a single named parameter here + ->andWhere( + $query->expr()->orX( + $query->expr()->ilike('g.uid', $query->createNamedParameter('%' . $this->dbConn->escapeLikeParameter($search) . '%')), + $query->expr()->ilike('u.displayname', $query->createNamedParameter('%' . $this->dbConn->escapeLikeParameter($search) . '%')), + $query->expr()->ilike('p.configvalue', $query->createNamedParameter('%' . $this->dbConn->escapeLikeParameter($search) . '%')) + ) + ) + ->orderBy('u.uid_lower', 'ASC'); + } + + if ($limit !== -1) { + $query->setMaxResults($limit); + } + if ($offset !== 0) { + $query->setFirstResult($offset); + } + + $result = $query->executeQuery(); + + $users = []; + $userManager = \OCP\Server::get(IUserManager::class); + $displayNameCache = \OCP\Server::get(DisplayNameCache::class); + while ($row = $result->fetch()) { + $users[$row['uid']] = new LazyUser($row['uid'], $displayNameCache, $userManager, $row['displayname'] ?? null); + } + $result->closeCursor(); + + return $users; + } + /** * get the number of all users matching the search string in a group * @param string $gid diff --git a/lib/private/Group/Group.php b/lib/private/Group/Group.php index cca179bfe1984..b2903bcdb26ea 100644 --- a/lib/private/Group/Group.php +++ b/lib/private/Group/Group.php @@ -242,18 +242,13 @@ public function removeUser($user) { } /** - * search for users in the group by userid - * - * @param string $search - * @param int $limit - * @param int $offset - * @return \OC\User\User[] + * Search for users in the group by userid or display name + * @return IUser[] */ - public function searchUsers($search, $limit = null, $offset = null) { + public function searchUsers(string $search, ?int $limit = null, ?int $offset = null): array { $users = []; foreach ($this->backends as $backend) { - $userIds = $backend->usersInGroup($this->gid, $search, $limit, $offset); - $users += $this->getVerifiedUsers($userIds); + $users = array_merge($users, $backend->searchInGroup($this->gid, $search, $limit ?? -1, $offset ?? 0)); if (!is_null($limit) and $limit <= 0) { return $users; } @@ -309,12 +304,12 @@ public function countDisabled() { * @param int $limit * @param int $offset * @return \OC\User\User[] + * @deprecated 25.0.0 Use searchUsers instead (same implementation) */ public function searchDisplayName($search, $limit = null, $offset = null) { $users = []; foreach ($this->backends as $backend) { - $userIds = $backend->usersInGroup($this->gid, $search, $limit, $offset); - $users = $this->getVerifiedUsers($userIds); + $users = array_merge($users, $backend->searchInGroup($this->gid, $search, $limit ?? -1, $offset ?? 0)); if (!is_null($limit) and $limit <= 0) { return array_values($users); } diff --git a/lib/private/User/LazyUser.php b/lib/private/User/LazyUser.php index 096578b8f375d..c36ff86eff4e7 100644 --- a/lib/private/User/LazyUser.php +++ b/lib/private/User/LazyUser.php @@ -30,16 +30,26 @@ class LazyUser implements IUser { private ?IUser $user = null; private string $uid; + private ?string $displayName; private IUserManager $userManager; + private ?UserInterface $backend; - public function __construct(string $uid, IUserManager $userManager) { + public function __construct(string $uid, IUserManager $userManager, ?string $displayName = null, ?UserInterface $backend = null) { $this->uid = $uid; $this->userManager = $userManager; + $this->displayName = $displayName; + $this->backend = $backend; } private function getUser(): IUser { if ($this->user === null) { - $this->user = $this->userManager->get($this->uid); + if ($this->backend) { + /** @var \OC\User\Manager $manager */ + $manager = $this->userManager; + $this->user = $manager->getUserObject($this->uid, $this->backend); + } else { + $this->user = $this->userManager->get($this->uid); + } } /** @var IUser */ $user = $this->user; @@ -51,7 +61,11 @@ public function getUID() { } public function getDisplayName() { - return $this->userManager->getDisplayName($this->uid) ?? $this->uid; + if ($this->displayName) { + return $this->displayName; + } + + return $this->userManager->getDisplayName($this->uid); } public function setDisplayName($displayName) { diff --git a/lib/private/User/Manager.php b/lib/private/User/Manager.php index 859ebd2a604a8..c3275934a5af9 100644 --- a/lib/private/User/Manager.php +++ b/lib/private/User/Manager.php @@ -202,7 +202,7 @@ public function getDisplayName(string $uid): ?string { * @param bool $cacheUser If false the newly created user object will not be cached * @return \OC\User\User */ - protected function getUserObject($uid, $backend, $cacheUser = true) { + public function getUserObject($uid, $backend, $cacheUser = true) { if ($backend instanceof IGetRealUIDBackend) { $uid = $backend->getRealUID($uid); } @@ -293,58 +293,53 @@ public function checkPasswordNoLogging($loginName, $password) { } /** - * search by user id + * Search by user id * * @param string $pattern * @param int $limit * @param int $offset - * @return \OC\User\User[] + * @return IUser[] + * @deprecated since 25.0.0, use searchDisplayName instead */ public function search($pattern, $limit = null, $offset = null) { $users = []; + $displayNameCache = \OCP\Server::get(DisplayNameCache::class); foreach ($this->backends as $backend) { $backendUsers = $backend->getUsers($pattern, $limit, $offset); if (is_array($backendUsers)) { foreach ($backendUsers as $uid) { - $users[$uid] = $this->getUserObject($uid, $backend); + $users[$uid] = new LazyUser($uid, $displayNameCache, $this, null, $backend); } } } - uasort($users, function ($a, $b) { - /** - * @var \OC\User\User $a - * @var \OC\User\User $b - */ + uasort($users, function (IUser $a, IUser $b) { return strcasecmp($a->getUID(), $b->getUID()); }); return $users; } /** - * search by displayName + * Search by displayName * * @param string $pattern * @param int $limit * @param int $offset - * @return \OC\User\User[] + * @return IUser[] */ public function searchDisplayName($pattern, $limit = null, $offset = null) { $users = []; + $displayNameCache = \OCP\Server::get(DisplayNameCache::class); foreach ($this->backends as $backend) { $backendUsers = $backend->getDisplayNames($pattern, $limit, $offset); if (is_array($backendUsers)) { foreach ($backendUsers as $uid => $displayName) { - $users[] = $this->getUserObject($uid, $backend); + $users[] = new LazyUser($uid, $displayNameCache, $this, $displayName, $backend); } } } - usort($users, function ($a, $b) { - /** - * @var \OC\User\User $a - * @var \OC\User\User $b - */ + usort($users, function (IUser $a, IUser $b) { return strcasecmp($a->getDisplayName(), $b->getDisplayName()); }); return $users; diff --git a/lib/public/Group/Backend/ABackend.php b/lib/public/Group/Backend/ABackend.php index 7f5cf732335f3..e76285dfda9b6 100644 --- a/lib/public/Group/Backend/ABackend.php +++ b/lib/public/Group/Backend/ABackend.php @@ -26,6 +26,10 @@ namespace OCP\Group\Backend; use OCP\GroupInterface; +use OCP\IUserManager; +use OCP\Server; +use OC\User\LazyUser; +use OC\User\DisplayNameCache; /** * @since 14.0.0 @@ -65,4 +69,15 @@ public function implementsActions($actions): bool { return (bool)($actions & $implements); } + + public function searchInGroup(string $gid, string $search = '', int $limit = -1, int $offset = 0): array { + // Default implementation for compatibility reasons + $displayNameCache = Server::get(DisplayNameCache::class); + $userManager = Server::get(IUserManager::class); + $users = []; + foreach ($this->usersInGroup($gid, $search, $limit, $offset) as $userId) { + $users[$userId] = new LazyUser($userId, $displayNameCache, $userManager); + } + return $users; + } } diff --git a/lib/public/GroupInterface.php b/lib/public/GroupInterface.php index b7c0713612612..07fd5cb50c467 100644 --- a/lib/public/GroupInterface.php +++ b/lib/public/GroupInterface.php @@ -106,13 +106,35 @@ public function getGroups($search = '', $limit = -1, $offset = 0); public function groupExists($gid); /** - * get a list of all users in a group + * @brief Get a list of user ids in a group matching the given search parameters. + * * @param string $gid * @param string $search * @param int $limit * @param int $offset * @return array an array of user ids * @since 4.5.0 + * @deprecated 25.0.0 Use searchInGroup instead, for performance reasons */ public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0); + + /** + * @brief Get a list of users matching the given search parameters. + * + * Implementations of this method should return lazy evaluated user objects and + * preload if possible the display name. + * + * + * $users = $groupBackend->searchInGroup('admin', 'John', 10, 0); + * + * + * @param string $gid The group id of the user we want to search + * @param string $search The part of the display name or user id of the users we + * want to search. This can be empty to get all the users. + * @param int $limit The limit of results + * @param int $offset The offset of the results + * @return IUser[] + * @since 25.0.0 + */ + public function searchInGroup(string $gid, string $search = '', int $limit = -1, int $offset = 0): array; } diff --git a/lib/public/IGroup.php b/lib/public/IGroup.php index ba13359c472d9..ec26cc55b69ac 100644 --- a/lib/public/IGroup.php +++ b/lib/public/IGroup.php @@ -99,7 +99,7 @@ public function removeUser($user); * @return \OCP\IUser[] * @since 8.0.0 */ - public function searchUsers($search, $limit = null, $offset = null); + public function searchUsers(string $search, ?int $limit = null, ?int $offset = null): array; /** * returns the number of users matching the search string diff --git a/tests/lib/Group/GroupTest.php b/tests/lib/Group/GroupTest.php index 60f15a6573206..993898803831b 100644 --- a/tests/lib/Group/GroupTest.php +++ b/tests/lib/Group/GroupTest.php @@ -303,9 +303,9 @@ public function testSearchUsers() { $group = new \OC\Group\Group('group1', [$backend], $this->dispatcher, $userManager); $backend->expects($this->once()) - ->method('usersInGroup') + ->method('searchInGroup') ->with('group1', '2') - ->willReturn(['user2']); + ->willReturn(['user2' => new \OC\User\User('user2', null, $this->dispatcher)]); $users = $group->searchUsers('2'); @@ -325,13 +325,13 @@ public function testSearchUsersMultipleBackends() { $group = new \OC\Group\Group('group1', [$backend1, $backend2], $this->dispatcher, $userManager); $backend1->expects($this->once()) - ->method('usersInGroup') + ->method('searchInGroup') ->with('group1', '2') - ->willReturn(['user2']); + ->willReturn(['user2' => new \OC\User\User('user2', null, $this->dispatcher)]); $backend2->expects($this->once()) - ->method('usersInGroup') + ->method('searchInGroup') ->with('group1', '2') - ->willReturn(['user2']); + ->willReturn(['user2' => new \OC\User\User('user2', null, $this->dispatcher)]); $users = $group->searchUsers('2'); @@ -348,9 +348,9 @@ public function testSearchUsersLimitAndOffset() { $group = new \OC\Group\Group('group1', [$backend], $this->dispatcher, $userManager); $backend->expects($this->once()) - ->method('usersInGroup') + ->method('searchInGroup') ->with('group1', 'user', 1, 1) - ->willReturn(['user2']); + ->willReturn(['user2' => new \OC\User\User('user2', null, $this->dispatcher)]); $users = $group->searchUsers('user', 1, 1); @@ -370,13 +370,13 @@ public function testSearchUsersMultipleBackendsLimitAndOffset() { $group = new \OC\Group\Group('group1', [$backend1, $backend2], $this->dispatcher, $userManager); $backend1->expects($this->once()) - ->method('usersInGroup') + ->method('searchInGroup') ->with('group1', 'user', 2, 1) - ->willReturn(['user2']); + ->willReturn(['user2' => new \OC\User\User('user2', null, $this->dispatcher)]); $backend2->expects($this->once()) - ->method('usersInGroup') + ->method('searchInGroup') ->with('group1', 'user', 2, 1) - ->willReturn(['user1']); + ->willReturn(['user1' => new \OC\User\User('user1', null, $this->dispatcher)]); $users = $group->searchUsers('user', 2, 1); diff --git a/tests/lib/Group/ManagerTest.php b/tests/lib/Group/ManagerTest.php index 5e29f974706b7..1bdb221934685 100644 --- a/tests/lib/Group/ManagerTest.php +++ b/tests/lib/Group/ManagerTest.php @@ -24,6 +24,7 @@ namespace Test\Group; use OC\Group\Database; +use OC\User\User; use OC\User\Manager; use OCP\GroupInterface; use OCP\ICacheFactory; @@ -91,6 +92,7 @@ private function getTestBackend($implementedActions = null) { 'createGroup', 'addToGroup', 'removeFromGroup', + 'searchInGroup', ]) ->getMock(); $backend->expects($this->any()) @@ -724,7 +726,7 @@ public function testDisplayNamesInGroupWithOneUserBackendWithLimitAndOffsetSpeci public function testDisplayNamesInGroupWithOneUserBackendAndSearchEmpty() { /** - * @var \PHPUnit\Framework\MockObject\MockObject | \OC\Group\Backend $backend + * @var \PHPUnit\Framework\MockObject\MockObject|\OC\Group\Backend $backend */ $backend = $this->getTestBackend(); $backend->expects($this->exactly(1)) @@ -733,22 +735,11 @@ public function testDisplayNamesInGroupWithOneUserBackendAndSearchEmpty() { ->willReturn(true); $backend->expects($this->once()) - ->method('usersInGroup') + ->method('searchInGroup') ->with('testgroup', '', -1, 0) - ->willReturn(['user2', 'user33']); + ->willReturn([$this->getTestUser('user2'), $this->getTestUser('user33')]); - $this->userManager->expects($this->any()) - ->method('get') - ->willReturnCallback(function ($uid) { - switch ($uid) { - case 'user1': return $this->getTestUser('user1'); - case 'user2': return $this->getTestUser('user2'); - case 'user3': return $this->getTestUser('user3'); - case 'user33': return $this->getTestUser('user33'); - default: - return null; - } - }); + $this->userManager->expects($this->never())->method('get'); $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); $manager->addBackend($backend); @@ -772,22 +763,11 @@ public function testDisplayNamesInGroupWithOneUserBackendAndSearchEmptyAndLimitS ->willReturn(true); $backend->expects($this->once()) - ->method('usersInGroup') + ->method('searchInGroup') ->with('testgroup', '', 1, 0) - ->willReturn(['user2']); + ->willReturn([new User('user2', null, $this->dispatcher)]); - $this->userManager->expects($this->any()) - ->method('get') - ->willReturnCallback(function ($uid) { - switch ($uid) { - case 'user1': return $this->getTestUser('user1'); - case 'user2': return $this->getTestUser('user2'); - case 'user3': return $this->getTestUser('user3'); - case 'user33': return $this->getTestUser('user33'); - default: - return null; - } - }); + $this->userManager->expects($this->never())->method('get'); $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); $manager->addBackend($backend); @@ -811,22 +791,11 @@ public function testDisplayNamesInGroupWithOneUserBackendAndSearchEmptyAndLimitA ->willReturn(true); $backend->expects($this->once()) - ->method('usersInGroup') + ->method('searchInGroup') ->with('testgroup', '', 1, 1) - ->willReturn(['user33']); + ->willReturn([$this->getTestUser('user33')]); - $this->userManager->expects($this->any()) - ->method('get') - ->willReturnCallback(function ($uid) { - switch ($uid) { - case 'user1': return $this->getTestUser('user1'); - case 'user2': return $this->getTestUser('user2'); - case 'user3': return $this->getTestUser('user3'); - case 'user33': return $this->getTestUser('user33'); - default: - return null; - } - }); + $this->userManager->expects($this->never())->method('get'); $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); $manager->addBackend($backend); diff --git a/tests/lib/Util/Group/Dummy.php b/tests/lib/Util/Group/Dummy.php index 3735a5e1167e8..2d86e45cfc164 100644 --- a/tests/lib/Util/Group/Dummy.php +++ b/tests/lib/Util/Group/Dummy.php @@ -29,12 +29,18 @@ namespace Test\Util\Group; -use OC\Group\Backend; +use Test\Util\User\Dummy as DummyUser; +use OCP\Group\Backend\ABackend; +use OCP\Group\Backend\IDeleteGroupBackend; +use OCP\Group\Backend\IAddToGroupBackend; +use OCP\Group\Backend\IRemoveFromGroupBackend; +use OCP\Group\Backend\ICreateGroupBackend; +use OCP\Group\Backend\ICountUsersBackend; /** - * dummy group backend, does not keep state, only for testing use + * Dummy group backend, does not keep state, only for testing use */ -class Dummy extends Backend { +class Dummy extends ABackend implements ICreateGroupBackend, IDeleteGroupBackend, IAddToGroupBackend, IRemoveFromGroupBackend, ICountUsersBackend { private $groups = []; /** * Try to create a new group @@ -44,7 +50,7 @@ class Dummy extends Backend { * Tries to create a new group. If the group name already exists, false will * be returned. */ - public function createGroup($gid) { + public function createGroup(string $gid): bool { if (!isset($this->groups[$gid])) { $this->groups[$gid] = []; return true; @@ -60,7 +66,7 @@ public function createGroup($gid) { * * Deletes a group and removes it from the group_user-table */ - public function deleteGroup($gid) { + public function deleteGroup(string $gid): bool { if (isset($this->groups[$gid])) { unset($this->groups[$gid]); return true; @@ -93,7 +99,7 @@ public function inGroup($uid, $gid) { * * Adds a user to a group. */ - public function addToGroup($uid, $gid) { + public function addToGroup(string $uid, string $gid): bool { if (isset($this->groups[$gid])) { if (array_search($uid, $this->groups[$gid]) === false) { $this->groups[$gid][] = $uid; @@ -114,7 +120,7 @@ public function addToGroup($uid, $gid) { * * removes the user from a group. */ - public function removeFromGroup($uid, $gid) { + public function removeFromGroup(string $uid, string $gid): bool { if (isset($this->groups[$gid])) { if (($index = array_search($uid, $this->groups[$gid])) !== false) { unset($this->groups[$gid][$index]); @@ -192,6 +198,25 @@ public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0) { } } + public function searchInGroup(string $gid, string $search = '', int $limit = -1, int $offset = 0): array { + if (isset($this->groups[$gid])) { + if (empty($search)) { + $length = $limit < 0 ? null : $limit; + $users = array_slice($this->groups[$gid], $offset, $length); + return array_map(fn ($user) => new DummyUser($user, '')); + } + $result = []; + foreach ($this->groups[$gid] as $user) { + if (stripos($user, $search) !== false) { + $result[] = new DummyUser($user, ''); + } + } + return $result; + } else { + return []; + } + } + /** * get the number of all users in a group * @param string $gid @@ -200,7 +225,7 @@ public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0) { * @param int $offset * @return int */ - public function countUsersInGroup($gid, $search = '', $limit = -1, $offset = 0) { + public function countUsersInGroup(string $gid, string $search = ''): int { if (isset($this->groups[$gid])) { if (empty($search)) { return count($this->groups[$gid]); @@ -213,5 +238,10 @@ public function countUsersInGroup($gid, $search = '', $limit = -1, $offset = 0) } return $count; } + return 0; + } + + public function groupExists($gid) { + return isset($this->groups[$gid]); } } From a4c599c1c91dcd6e041cbf65198e1f1200513be8 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Sun, 16 Oct 2022 19:57:31 +0200 Subject: [PATCH 2/8] Split new method in a new group backend interface Better for backward compatibility, also move new interfaces to nc 26 Signed-off-by: Carl Schwan --- lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + lib/private/Group/Database.php | 57 +++---------------- lib/private/Group/Group.php | 25 ++++---- lib/private/User/LazyUser.php | 2 +- lib/private/User/Manager.php | 6 +- lib/public/Group/Backend/ABackend.php | 6 +- .../Group/Backend/ISearchableGroupBackend.php | 52 +++++++++++++++++ lib/public/GroupInterface.php | 22 +------ 9 files changed, 82 insertions(+), 90 deletions(-) create mode 100644 lib/public/Group/Backend/ISearchableGroupBackend.php diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index eb2f3e463546d..292e9b7b3af14 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -412,6 +412,7 @@ 'OCP\\Group\\Backend\\IIsAdminBackend' => $baseDir . '/lib/public/Group/Backend/IIsAdminBackend.php', 'OCP\\Group\\Backend\\INamedBackend' => $baseDir . '/lib/public/Group/Backend/INamedBackend.php', 'OCP\\Group\\Backend\\IRemoveFromGroupBackend' => $baseDir . '/lib/public/Group/Backend/IRemoveFromGroupBackend.php', + 'OCP\\Group\\Backend\\ISearchableGroupBackend' => $baseDir . '/lib/public/Group/Backend/ISearchableGroupBackend.php', 'OCP\\Group\\Backend\\ISetDisplayNameBackend' => $baseDir . '/lib/public/Group/Backend/ISetDisplayNameBackend.php', 'OCP\\Group\\Events\\BeforeGroupChangedEvent' => $baseDir . '/lib/public/Group/Events/BeforeGroupChangedEvent.php', 'OCP\\Group\\Events\\BeforeGroupCreatedEvent' => $baseDir . '/lib/public/Group/Events/BeforeGroupCreatedEvent.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 9343231a55b56..270d3857ca18e 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -445,6 +445,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OCP\\Group\\Backend\\IIsAdminBackend' => __DIR__ . '/../../..' . '/lib/public/Group/Backend/IIsAdminBackend.php', 'OCP\\Group\\Backend\\INamedBackend' => __DIR__ . '/../../..' . '/lib/public/Group/Backend/INamedBackend.php', 'OCP\\Group\\Backend\\IRemoveFromGroupBackend' => __DIR__ . '/../../..' . '/lib/public/Group/Backend/IRemoveFromGroupBackend.php', + 'OCP\\Group\\Backend\\ISearchableGroupBackend' => __DIR__ . '/../../..' . '/lib/public/Group/Backend/ISearchableGroupBackend.php', 'OCP\\Group\\Backend\\ISetDisplayNameBackend' => __DIR__ . '/../../..' . '/lib/public/Group/Backend/ISetDisplayNameBackend.php', 'OCP\\Group\\Events\\BeforeGroupChangedEvent' => __DIR__ . '/../../..' . '/lib/public/Group/Events/BeforeGroupChangedEvent.php', 'OCP\\Group\\Events\\BeforeGroupCreatedEvent' => __DIR__ . '/../../..' . '/lib/public/Group/Events/BeforeGroupCreatedEvent.php', diff --git a/lib/private/Group/Database.php b/lib/private/Group/Database.php index bfc95c574e2c5..13e4906c29852 100644 --- a/lib/private/Group/Database.php +++ b/lib/private/Group/Database.php @@ -40,12 +40,12 @@ use OCP\Group\Backend\IGetDisplayNameBackend; use OCP\Group\Backend\IGroupDetailsBackend; use OCP\Group\Backend\IRemoveFromGroupBackend; +use OCP\Group\Backend\ISearchableGroupBackend; use OCP\Group\Backend\ISetDisplayNameBackend; use OCP\Group\Backend\INamedBackend; use OCP\IDBConnection; use OCP\IUserManager; use OC\User\LazyUser; -use OC\User\DisplayNameCache; /** * Class for group management in a SQL Database (e.g. MySQL, SQLite) @@ -60,6 +60,7 @@ class Database extends ABackend implements IGroupDetailsBackend, IRemoveFromGroupBackend, ISetDisplayNameBackend, + ISearchableGroupBackend, INamedBackend { /** @var string[] */ private $groupCache = []; @@ -327,56 +328,15 @@ public function groupExists($gid) { } /** - * get a list of all users in a group + * Get a list of all users in a group * @param string $gid * @param string $search * @param int $limit * @param int $offset - * @return array an array of user ids + * @return array an array of user ids */ - public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0) { - $this->fixDI(); - - $query = $this->dbConn->getQueryBuilder(); - $query->select('g.uid') - ->from('group_user', 'g') - ->where($query->expr()->eq('gid', $query->createNamedParameter($gid))) - ->orderBy('g.uid', 'ASC'); - - if ($search !== '') { - $query->leftJoin('g', 'users', 'u', $query->expr()->eq('g.uid', 'u.uid')) - ->leftJoin('u', 'preferences', 'p', $query->expr()->andX( - $query->expr()->eq('p.userid', 'u.uid'), - $query->expr()->eq('p.appid', $query->expr()->literal('settings')), - $query->expr()->eq('p.configkey', $query->expr()->literal('email'))) - ) - // sqlite doesn't like re-using a single named parameter here - ->andWhere( - $query->expr()->orX( - $query->expr()->ilike('g.uid', $query->createNamedParameter('%' . $this->dbConn->escapeLikeParameter($search) . '%')), - $query->expr()->ilike('u.displayname', $query->createNamedParameter('%' . $this->dbConn->escapeLikeParameter($search) . '%')), - $query->expr()->ilike('p.configvalue', $query->createNamedParameter('%' . $this->dbConn->escapeLikeParameter($search) . '%')) - ) - ) - ->orderBy('u.uid_lower', 'ASC'); - } - - if ($limit !== -1) { - $query->setMaxResults($limit); - } - if ($offset !== 0) { - $query->setFirstResult($offset); - } - - $result = $query->execute(); - - $users = []; - while ($row = $result->fetch()) { - $users[] = $row['uid']; - } - $result->closeCursor(); - - return $users; + public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0): array { + return array_map(fn ($user) => $user->getUid(), $this->searchInGroup($gid, $search, $limit, $offset)); } public function searchInGroup(string $gid, string $search = '', int $limit = -1, int $offset = 0): array { @@ -389,7 +349,7 @@ public function searchInGroup(string $gid, string $search = '', int $limit = -1, ->where($query->expr()->eq('gid', $query->createNamedParameter($gid))) ->orderBy('g.uid', 'ASC'); - $query->leftJoin('g', 'users', 'u', $query->expr()->eq('g.uid', 'u.uid')) + $query->leftJoin('g', 'users', 'u', $query->expr()->eq('g.uid', 'u.uid')); if ($search !== '') { $query->leftJoin('u', 'preferences', 'p', $query->expr()->andX( @@ -419,9 +379,8 @@ public function searchInGroup(string $gid, string $search = '', int $limit = -1, $users = []; $userManager = \OCP\Server::get(IUserManager::class); - $displayNameCache = \OCP\Server::get(DisplayNameCache::class); while ($row = $result->fetch()) { - $users[$row['uid']] = new LazyUser($row['uid'], $displayNameCache, $userManager, $row['displayname'] ?? null); + $users[$row['uid']] = new LazyUser($row['uid'], $userManager, $row['displayname'] ?? null); } $result->closeCursor(); diff --git a/lib/private/Group/Group.php b/lib/private/Group/Group.php index b2903bcdb26ea..bfe985b266e66 100644 --- a/lib/private/Group/Group.php +++ b/lib/private/Group/Group.php @@ -33,14 +33,16 @@ namespace OC\Group; use OC\Hooks\PublicEmitter; +use OC\User\LazyUser; +use OCP\GroupInterface; use OCP\Group\Backend\ICountDisabledInGroup; use OCP\Group\Backend\IGetDisplayNameBackend; use OCP\Group\Backend\IHideFromCollaborationBackend; use OCP\Group\Backend\INamedBackend; +use OCP\Group\Backend\ISearchableGroupBackend; use OCP\Group\Backend\ISetDisplayNameBackend; use OCP\Group\Events\BeforeGroupChangedEvent; use OCP\Group\Events\GroupChangedEvent; -use OCP\GroupInterface; use OCP\IGroup; use OCP\IUser; use OCP\IUserManager; @@ -248,7 +250,15 @@ public function removeUser($user) { public function searchUsers(string $search, ?int $limit = null, ?int $offset = null): array { $users = []; foreach ($this->backends as $backend) { - $users = array_merge($users, $backend->searchInGroup($this->gid, $search, $limit ?? -1, $offset ?? 0)); + if ($backend instanceof ISearchableGroupBackend) { + $users = array_merge($users, $backend->searchInGroup($this->gid, $search, $limit ?? -1, $offset ?? 0)); + } else { + $userIds = $backend->usersInGroup($this->gid, $search, $limit ?? -1, $offset ?? 0); + $userManager = \OCP\Server::get(IUserManager::class); + $users = array_merge($users, array_map(function (string $userId) use ($userManager): IUser { + return new LazyUser($userId, $userManager); + }, $userIds)); + } if (!is_null($limit) and $limit <= 0) { return $users; } @@ -303,18 +313,11 @@ public function countDisabled() { * @param string $search * @param int $limit * @param int $offset - * @return \OC\User\User[] + * @return IUser[] * @deprecated 25.0.0 Use searchUsers instead (same implementation) */ public function searchDisplayName($search, $limit = null, $offset = null) { - $users = []; - foreach ($this->backends as $backend) { - $users = array_merge($users, $backend->searchInGroup($this->gid, $search, $limit ?? -1, $offset ?? 0)); - if (!is_null($limit) and $limit <= 0) { - return array_values($users); - } - } - return array_values($users); + return $this->searchUsers($search, $limit, $offset); } /** diff --git a/lib/private/User/LazyUser.php b/lib/private/User/LazyUser.php index c36ff86eff4e7..5472cf6f2b48d 100644 --- a/lib/private/User/LazyUser.php +++ b/lib/private/User/LazyUser.php @@ -65,7 +65,7 @@ public function getDisplayName() { return $this->displayName; } - return $this->userManager->getDisplayName($this->uid); + return $this->userManager->getDisplayName($this->uid) ?? $this->uid; } public function setDisplayName($displayName) { diff --git a/lib/private/User/Manager.php b/lib/private/User/Manager.php index c3275934a5af9..dcf9bbab2f3a5 100644 --- a/lib/private/User/Manager.php +++ b/lib/private/User/Manager.php @@ -303,12 +303,11 @@ public function checkPasswordNoLogging($loginName, $password) { */ public function search($pattern, $limit = null, $offset = null) { $users = []; - $displayNameCache = \OCP\Server::get(DisplayNameCache::class); foreach ($this->backends as $backend) { $backendUsers = $backend->getUsers($pattern, $limit, $offset); if (is_array($backendUsers)) { foreach ($backendUsers as $uid) { - $users[$uid] = new LazyUser($uid, $displayNameCache, $this, null, $backend); + $users[$uid] = new LazyUser($uid, $this, null, $backend); } } } @@ -329,12 +328,11 @@ public function search($pattern, $limit = null, $offset = null) { */ public function searchDisplayName($pattern, $limit = null, $offset = null) { $users = []; - $displayNameCache = \OCP\Server::get(DisplayNameCache::class); foreach ($this->backends as $backend) { $backendUsers = $backend->getDisplayNames($pattern, $limit, $offset); if (is_array($backendUsers)) { foreach ($backendUsers as $uid => $displayName) { - $users[] = new LazyUser($uid, $displayNameCache, $this, $displayName, $backend); + $users[] = new LazyUser($uid, $this, $displayName, $backend); } } } diff --git a/lib/public/Group/Backend/ABackend.php b/lib/public/Group/Backend/ABackend.php index e76285dfda9b6..ee1b62c887bd7 100644 --- a/lib/public/Group/Backend/ABackend.php +++ b/lib/public/Group/Backend/ABackend.php @@ -29,12 +29,11 @@ use OCP\IUserManager; use OCP\Server; use OC\User\LazyUser; -use OC\User\DisplayNameCache; /** * @since 14.0.0 */ -abstract class ABackend implements GroupInterface { +abstract class ABackend implements GroupInterface, ISearchableGroupBackend { /** * @deprecated 14.0.0 * @since 14.0.0 @@ -72,11 +71,10 @@ public function implementsActions($actions): bool { public function searchInGroup(string $gid, string $search = '', int $limit = -1, int $offset = 0): array { // Default implementation for compatibility reasons - $displayNameCache = Server::get(DisplayNameCache::class); $userManager = Server::get(IUserManager::class); $users = []; foreach ($this->usersInGroup($gid, $search, $limit, $offset) as $userId) { - $users[$userId] = new LazyUser($userId, $displayNameCache, $userManager); + $users[$userId] = new LazyUser($userId, $userManager); } return $users; } diff --git a/lib/public/Group/Backend/ISearchableGroupBackend.php b/lib/public/Group/Backend/ISearchableGroupBackend.php new file mode 100644 index 0000000000000..4e03680f75a98 --- /dev/null +++ b/lib/public/Group/Backend/ISearchableGroupBackend.php @@ -0,0 +1,52 @@ + + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ +namespace OCP\Group\Backend; + +use OCP\IUser; + +/** + * @since 26.0.0 + */ +interface ISearchableGroupBackend { + + /** + * @brief Get a list of users matching the given search parameters. + * + * Implementations of this method should return lazy evaluated user objects and + * preload if possible the display name. + * + * + * $users = $groupBackend->searchInGroup('admin', 'John', 10, 0); + * + * + * @param string $gid The group id of the user we want to search + * @param string $search The part of the display name or user id of the users we + * want to search. This can be empty to get all the users. + * @param int $limit The limit of results + * @param int $offset The offset of the results + * @return IUser[] + * @since 26.0.0 + */ + public function searchInGroup(string $gid, string $search = '', int $limit = -1, int $offset = 0): array; +} diff --git a/lib/public/GroupInterface.php b/lib/public/GroupInterface.php index 07fd5cb50c467..7a3c396054170 100644 --- a/lib/public/GroupInterface.php +++ b/lib/public/GroupInterface.php @@ -114,27 +114,7 @@ public function groupExists($gid); * @param int $offset * @return array an array of user ids * @since 4.5.0 - * @deprecated 25.0.0 Use searchInGroup instead, for performance reasons + * @deprecated 26.0.0 Use searchInGroup instead, for performance reasons */ public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0); - - /** - * @brief Get a list of users matching the given search parameters. - * - * Implementations of this method should return lazy evaluated user objects and - * preload if possible the display name. - * - * - * $users = $groupBackend->searchInGroup('admin', 'John', 10, 0); - * - * - * @param string $gid The group id of the user we want to search - * @param string $search The part of the display name or user id of the users we - * want to search. This can be empty to get all the users. - * @param int $limit The limit of results - * @param int $offset The offset of the results - * @return IUser[] - * @since 25.0.0 - */ - public function searchInGroup(string $gid, string $search = '', int $limit = -1, int $offset = 0): array; } From 3c2b126eba063bd056458be50e96d3015d62b66a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Thu, 22 Dec 2022 12:16:08 +0100 Subject: [PATCH 3/8] Make code clearer and bump @ deprecated annotations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- lib/private/Group/Group.php | 10 ++++++---- lib/private/User/Manager.php | 2 +- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/private/Group/Group.php b/lib/private/Group/Group.php index bfe985b266e66..f70221de8bd4d 100644 --- a/lib/private/Group/Group.php +++ b/lib/private/Group/Group.php @@ -255,9 +255,11 @@ public function searchUsers(string $search, ?int $limit = null, ?int $offset = n } else { $userIds = $backend->usersInGroup($this->gid, $search, $limit ?? -1, $offset ?? 0); $userManager = \OCP\Server::get(IUserManager::class); - $users = array_merge($users, array_map(function (string $userId) use ($userManager): IUser { - return new LazyUser($userId, $userManager); - }, $userIds)); + $userObjects = array_map( + fn (string $userId): IUser => new LazyUser($userId, $userManager), + $userIds + ); + $users = array_merge($users, $userObjects); } if (!is_null($limit) and $limit <= 0) { return $users; @@ -314,7 +316,7 @@ public function countDisabled() { * @param int $limit * @param int $offset * @return IUser[] - * @deprecated 25.0.0 Use searchUsers instead (same implementation) + * @deprecated 26.0.0 Use searchUsers instead (same implementation) */ public function searchDisplayName($search, $limit = null, $offset = null) { return $this->searchUsers($search, $limit, $offset); diff --git a/lib/private/User/Manager.php b/lib/private/User/Manager.php index dcf9bbab2f3a5..e736a1981088b 100644 --- a/lib/private/User/Manager.php +++ b/lib/private/User/Manager.php @@ -299,7 +299,7 @@ public function checkPasswordNoLogging($loginName, $password) { * @param int $limit * @param int $offset * @return IUser[] - * @deprecated since 25.0.0, use searchDisplayName instead + * @deprecated since 26.0.0, use searchDisplayName instead */ public function search($pattern, $limit = null, $offset = null) { $users = []; From 6385a5af36957cac4e1beed531d941129eb3a5a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Thu, 22 Dec 2022 12:19:06 +0100 Subject: [PATCH 4/8] Let OC\Group\Group handle the fallback and remove default implementation from ABackend MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- apps/user_ldap/lib/Group_LDAP.php | 13 ++----------- build/psalm-baseline-ocp.xml | 5 ----- lib/public/Group/Backend/ABackend.php | 15 +-------------- 3 files changed, 3 insertions(+), 30 deletions(-) diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php index ca3a6d13e8fa4..fac7109366531 100644 --- a/apps/user_ldap/lib/Group_LDAP.php +++ b/apps/user_ldap/lib/Group_LDAP.php @@ -48,12 +48,11 @@ use OC\ServerNotAvailableException; use OCP\Cache\CappedMemoryCache; use OCP\GroupInterface; -use OCP\Group\Backend\ABackend; use OCP\Group\Backend\IDeleteGroupBackend; use OCP\Group\Backend\IGetDisplayNameBackend; use Psr\Log\LoggerInterface; -class Group_LDAP extends ABackend implements GroupInterface, IGroupLDAP, IGetDisplayNameBackend, IDeleteGroupBackend { +class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, IGetDisplayNameBackend, IDeleteGroupBackend { protected bool $enabled = false; /** @var CappedMemoryCache $cachedGroupMembers array of users with gid as key */ @@ -64,7 +63,6 @@ class Group_LDAP extends ABackend implements GroupInterface, IGroupLDAP, IGetDis protected CappedMemoryCache $cachedNestedGroups; protected GroupPluginManager $groupPluginManager; protected LoggerInterface $logger; - protected Access $access; /** * @var string $ldapGroupMemberAssocAttr contains the LDAP setting (in lower case) with the same name @@ -72,7 +70,7 @@ class Group_LDAP extends ABackend implements GroupInterface, IGroupLDAP, IGetDis protected string $ldapGroupMemberAssocAttr; public function __construct(Access $access, GroupPluginManager $groupPluginManager) { - $this->access = $access; + parent::__construct($access); $filter = $this->access->connection->ldapGroupFilter; $gAssoc = $this->access->connection->ldapGroupMemberAssocAttr; if (!empty($filter) && !empty($gAssoc)) { @@ -1333,11 +1331,4 @@ public function getDisplayName(string $gid): string { $this->access->connection->writeToCache($cacheKey, $displayName); return $displayName; } - - public function searchInGroup(string $gid, string $search = '', int $limit = -1, int $offset = 0): array { - if (!$this->enabled) { - return []; - } - return parent::searchInGroup($gid, $search, $limit, $offset); - } } diff --git a/build/psalm-baseline-ocp.xml b/build/psalm-baseline-ocp.xml index 1613a6fe8fab9..e993e85e5a247 100644 --- a/build/psalm-baseline-ocp.xml +++ b/build/psalm-baseline-ocp.xml @@ -5,11 +5,6 @@ OC - - - OC - - $this->request->server diff --git a/lib/public/Group/Backend/ABackend.php b/lib/public/Group/Backend/ABackend.php index ee1b62c887bd7..7f5cf732335f3 100644 --- a/lib/public/Group/Backend/ABackend.php +++ b/lib/public/Group/Backend/ABackend.php @@ -26,14 +26,11 @@ namespace OCP\Group\Backend; use OCP\GroupInterface; -use OCP\IUserManager; -use OCP\Server; -use OC\User\LazyUser; /** * @since 14.0.0 */ -abstract class ABackend implements GroupInterface, ISearchableGroupBackend { +abstract class ABackend implements GroupInterface { /** * @deprecated 14.0.0 * @since 14.0.0 @@ -68,14 +65,4 @@ public function implementsActions($actions): bool { return (bool)($actions & $implements); } - - public function searchInGroup(string $gid, string $search = '', int $limit = -1, int $offset = 0): array { - // Default implementation for compatibility reasons - $userManager = Server::get(IUserManager::class); - $users = []; - foreach ($this->usersInGroup($gid, $search, $limit, $offset) as $userId) { - $users[$userId] = new LazyUser($userId, $userManager); - } - return $users; - } } From b6c17c6ce76080df0e291f60a85c0e42afab8f39 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Mon, 2 Jan 2023 17:10:31 +0100 Subject: [PATCH 5/8] Clear up return types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit usersInGroup index by int for BC, searchInGroup index by uid (string). Signed-off-by: Côme Chilliet --- apps/user_ldap/lib/Access.php | 5 +++-- apps/user_ldap/lib/Group_LDAP.php | 11 ++++++++--- apps/user_ldap/lib/Group_Proxy.php | 2 +- lib/private/Group/Backend.php | 2 +- lib/private/Group/Database.php | 4 ++-- lib/private/Group/Group.php | 2 +- lib/public/Group/Backend/ISearchableGroupBackend.php | 3 +-- lib/public/GroupInterface.php | 2 +- tests/lib/Group/ManagerTest.php | 4 ++-- tests/lib/Util/Group/Dummy.php | 2 +- 10 files changed, 21 insertions(+), 16 deletions(-) diff --git a/apps/user_ldap/lib/Access.php b/apps/user_ldap/lib/Access.php index 9a97a28c376cc..3f120caefe6f1 100644 --- a/apps/user_ldap/lib/Access.php +++ b/apps/user_ldap/lib/Access.php @@ -631,7 +631,7 @@ public function mapAndAnnounceIfApplicable( * gives back the user names as they are used ownClod internally * * @param array $ldapUsers as returned by fetchList() - * @return array an array with the user names to use in Nextcloud + * @return array an array with the user names to use in Nextcloud * * gives back the user names as they are used ownClod internally * @throws \Exception @@ -644,7 +644,7 @@ public function nextcloudUserNames($ldapUsers) { * gives back the group names as they are used ownClod internally * * @param array $ldapGroups as returned by fetchList() - * @return array an array with the group names to use in Nextcloud + * @return array an array with the group names to use in Nextcloud * * gives back the group names as they are used ownClod internally * @throws \Exception @@ -655,6 +655,7 @@ public function nextcloudGroupNames($ldapGroups) { /** * @param array[] $ldapObjects as returned by fetchList() + * @return array * @throws \Exception */ private function ldap2NextcloudNames(array $ldapObjects, bool $isUsers): array { diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php index fac7109366531..84267171d3749 100644 --- a/apps/user_ldap/lib/Group_LDAP.php +++ b/apps/user_ldap/lib/Group_LDAP.php @@ -466,7 +466,7 @@ private function prepareFilterForUsersHasGidNumber(string $groupDN, string $sear } /** - * @return array A list of users that have the given group as gid number + * @return array A list of users that have the given group as gid number * @throws ServerNotAvailableException */ public function getUsersInGidNumber( @@ -591,6 +591,7 @@ private function prepareFilterForUsersInPrimaryGroup(string $groupDN, string $se /** * @throws ServerNotAvailableException + * @return array */ public function getUsersInPrimaryGroup( string $groupDN, @@ -840,7 +841,7 @@ private function getGroupsByMember(string $dn, array &$seen = []): array { * @param string $search * @param int $limit * @param int $offset - * @return array with user ids + * @return array user ids * @throws Exception * @throws ServerNotAvailableException */ @@ -909,7 +910,11 @@ public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0) { if (empty($ldap_users)) { break; } - $groupUsers[] = $this->access->dn2username($ldap_users[0]['dn'][0]); + $uid = $this->access->dn2username($ldap_users[0]['dn'][0]); + if (!$uid) { + break; + } + $groupUsers[] = $uid; break; default: //we got DNs, check if we need to filter by search or we can give back all of them diff --git a/apps/user_ldap/lib/Group_Proxy.php b/apps/user_ldap/lib/Group_Proxy.php index 74655840f479c..5f8d0562fd918 100644 --- a/apps/user_ldap/lib/Group_Proxy.php +++ b/apps/user_ldap/lib/Group_Proxy.php @@ -171,7 +171,7 @@ public function getUserGroups($uid) { /** * get a list of all users in a group * - * @return string[] with user ids + * @return array user ids */ public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0) { $this->setup(); diff --git a/lib/private/Group/Backend.php b/lib/private/Group/Backend.php index 037cdacf4453e..1e3e5ef1164a3 100644 --- a/lib/private/Group/Backend.php +++ b/lib/private/Group/Backend.php @@ -126,7 +126,7 @@ public function groupExists($gid) { * @param string $search * @param int $limit * @param int $offset - * @return array an array of user ids + * @return array an array of user ids */ public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0) { return []; diff --git a/lib/private/Group/Database.php b/lib/private/Group/Database.php index 13e4906c29852..569cfa5007f34 100644 --- a/lib/private/Group/Database.php +++ b/lib/private/Group/Database.php @@ -333,10 +333,10 @@ public function groupExists($gid) { * @param string $search * @param int $limit * @param int $offset - * @return array an array of user ids + * @return array an array of user ids */ public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0): array { - return array_map(fn ($user) => $user->getUid(), $this->searchInGroup($gid, $search, $limit, $offset)); + return array_values(array_map(fn ($user) => $user->getUid(), $this->searchInGroup($gid, $search, $limit, $offset))); } public function searchInGroup(string $gid, string $search = '', int $limit = -1, int $offset = 0): array { diff --git a/lib/private/Group/Group.php b/lib/private/Group/Group.php index f70221de8bd4d..c72a7826fd04d 100644 --- a/lib/private/Group/Group.php +++ b/lib/private/Group/Group.php @@ -251,7 +251,7 @@ public function searchUsers(string $search, ?int $limit = null, ?int $offset = n $users = []; foreach ($this->backends as $backend) { if ($backend instanceof ISearchableGroupBackend) { - $users = array_merge($users, $backend->searchInGroup($this->gid, $search, $limit ?? -1, $offset ?? 0)); + $users = array_merge($users, array_values($backend->searchInGroup($this->gid, $search, $limit ?? -1, $offset ?? 0))); } else { $userIds = $backend->usersInGroup($this->gid, $search, $limit ?? -1, $offset ?? 0); $userManager = \OCP\Server::get(IUserManager::class); diff --git a/lib/public/Group/Backend/ISearchableGroupBackend.php b/lib/public/Group/Backend/ISearchableGroupBackend.php index 4e03680f75a98..e5861d2814c2e 100644 --- a/lib/public/Group/Backend/ISearchableGroupBackend.php +++ b/lib/public/Group/Backend/ISearchableGroupBackend.php @@ -29,7 +29,6 @@ * @since 26.0.0 */ interface ISearchableGroupBackend { - /** * @brief Get a list of users matching the given search parameters. * @@ -45,7 +44,7 @@ interface ISearchableGroupBackend { * want to search. This can be empty to get all the users. * @param int $limit The limit of results * @param int $offset The offset of the results - * @return IUser[] + * @return array Users indexed by uid * @since 26.0.0 */ public function searchInGroup(string $gid, string $search = '', int $limit = -1, int $offset = 0): array; diff --git a/lib/public/GroupInterface.php b/lib/public/GroupInterface.php index 7a3c396054170..726921920af00 100644 --- a/lib/public/GroupInterface.php +++ b/lib/public/GroupInterface.php @@ -112,7 +112,7 @@ public function groupExists($gid); * @param string $search * @param int $limit * @param int $offset - * @return array an array of user ids + * @return array an array of user ids * @since 4.5.0 * @deprecated 26.0.0 Use searchInGroup instead, for performance reasons */ diff --git a/tests/lib/Group/ManagerTest.php b/tests/lib/Group/ManagerTest.php index 1bdb221934685..186f9d619a29a 100644 --- a/tests/lib/Group/ManagerTest.php +++ b/tests/lib/Group/ManagerTest.php @@ -737,7 +737,7 @@ public function testDisplayNamesInGroupWithOneUserBackendAndSearchEmpty() { $backend->expects($this->once()) ->method('searchInGroup') ->with('testgroup', '', -1, 0) - ->willReturn([$this->getTestUser('user2'), $this->getTestUser('user33')]); + ->willReturn(['user2' => $this->getTestUser('user2'), 'user33' => $this->getTestUser('user33')]); $this->userManager->expects($this->never())->method('get'); @@ -793,7 +793,7 @@ public function testDisplayNamesInGroupWithOneUserBackendAndSearchEmptyAndLimitA $backend->expects($this->once()) ->method('searchInGroup') ->with('testgroup', '', 1, 1) - ->willReturn([$this->getTestUser('user33')]); + ->willReturn(['user33' => $this->getTestUser('user33')]); $this->userManager->expects($this->never())->method('get'); diff --git a/tests/lib/Util/Group/Dummy.php b/tests/lib/Util/Group/Dummy.php index 2d86e45cfc164..a864c8ce9d992 100644 --- a/tests/lib/Util/Group/Dummy.php +++ b/tests/lib/Util/Group/Dummy.php @@ -208,7 +208,7 @@ public function searchInGroup(string $gid, string $search = '', int $limit = -1, $result = []; foreach ($this->groups[$gid] as $user) { if (stripos($user, $search) !== false) { - $result[] = new DummyUser($user, ''); + $result[$user] = new DummyUser($user, ''); } } return $result; From 346344c15371597ec825059abdf7564da9334b7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Thu, 27 Apr 2023 12:04:32 +0200 Subject: [PATCH 6/8] Update version number in since and deprecated annotations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- lib/private/Group/Group.php | 2 +- lib/private/User/Manager.php | 2 +- lib/public/Group/Backend/ISearchableGroupBackend.php | 4 ++-- lib/public/GroupInterface.php | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/private/Group/Group.php b/lib/private/Group/Group.php index c72a7826fd04d..7f04e45de80b3 100644 --- a/lib/private/Group/Group.php +++ b/lib/private/Group/Group.php @@ -316,7 +316,7 @@ public function countDisabled() { * @param int $limit * @param int $offset * @return IUser[] - * @deprecated 26.0.0 Use searchUsers instead (same implementation) + * @deprecated 27.0.0 Use searchUsers instead (same implementation) */ public function searchDisplayName($search, $limit = null, $offset = null) { return $this->searchUsers($search, $limit, $offset); diff --git a/lib/private/User/Manager.php b/lib/private/User/Manager.php index e736a1981088b..60059d5baddbe 100644 --- a/lib/private/User/Manager.php +++ b/lib/private/User/Manager.php @@ -299,7 +299,7 @@ public function checkPasswordNoLogging($loginName, $password) { * @param int $limit * @param int $offset * @return IUser[] - * @deprecated since 26.0.0, use searchDisplayName instead + * @deprecated since 27.0.0, use searchDisplayName instead */ public function search($pattern, $limit = null, $offset = null) { $users = []; diff --git a/lib/public/Group/Backend/ISearchableGroupBackend.php b/lib/public/Group/Backend/ISearchableGroupBackend.php index e5861d2814c2e..20753822258e4 100644 --- a/lib/public/Group/Backend/ISearchableGroupBackend.php +++ b/lib/public/Group/Backend/ISearchableGroupBackend.php @@ -26,7 +26,7 @@ use OCP\IUser; /** - * @since 26.0.0 + * @since 27.0.0 */ interface ISearchableGroupBackend { /** @@ -45,7 +45,7 @@ interface ISearchableGroupBackend { * @param int $limit The limit of results * @param int $offset The offset of the results * @return array Users indexed by uid - * @since 26.0.0 + * @since 27.0.0 */ public function searchInGroup(string $gid, string $search = '', int $limit = -1, int $offset = 0): array; } diff --git a/lib/public/GroupInterface.php b/lib/public/GroupInterface.php index 726921920af00..56863100c0504 100644 --- a/lib/public/GroupInterface.php +++ b/lib/public/GroupInterface.php @@ -114,7 +114,7 @@ public function groupExists($gid); * @param int $offset * @return array an array of user ids * @since 4.5.0 - * @deprecated 26.0.0 Use searchInGroup instead, for performance reasons + * @deprecated 27.0.0 Use searchInGroup instead, for performance reasons */ public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0); } From 876c162df831f70a9b4ed886f35c47c9c22df5ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Thu, 27 Apr 2023 15:25:13 +0200 Subject: [PATCH 7/8] Use the searchDisplayName recommended method in user:list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- core/Command/User/ListCommand.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/Command/User/ListCommand.php b/core/Command/User/ListCommand.php index c254a8a11cfa2..bf4bf7f030e33 100644 --- a/core/Command/User/ListCommand.php +++ b/core/Command/User/ListCommand.php @@ -74,7 +74,7 @@ protected function configure() { } protected function execute(InputInterface $input, OutputInterface $output): int { - $users = $this->userManager->search('', (int) $input->getOption('limit'), (int) $input->getOption('offset')); + $users = $this->userManager->searchDisplayName('', (int) $input->getOption('limit'), (int) $input->getOption('offset')); $this->writeArrayInOutputFormat($input, $output, $this->formatUsers($users, (bool)$input->getOption('info'))); return 0; From 10296ba7e50ede8eee0ed18f477eb3e67bfd5878 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Tue, 2 May 2023 11:35:41 +0200 Subject: [PATCH 8/8] Fix tests, and fix Group::searchUsers to avoid duplicates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Also went back to searchUsers indexing the array by uid as it was the previous behavior and the IGroup phpdoc does not say anything about the keys. Signed-off-by: Côme Chilliet --- lib/private/Group/Group.php | 17 +++++++---------- tests/lib/Group/GroupTest.php | 10 +++++----- tests/lib/Group/ManagerTest.php | 6 +++++- 3 files changed, 17 insertions(+), 16 deletions(-) diff --git a/lib/private/Group/Group.php b/lib/private/Group/Group.php index 7f04e45de80b3..efc21ad7c0dc0 100644 --- a/lib/private/Group/Group.php +++ b/lib/private/Group/Group.php @@ -251,15 +251,15 @@ public function searchUsers(string $search, ?int $limit = null, ?int $offset = n $users = []; foreach ($this->backends as $backend) { if ($backend instanceof ISearchableGroupBackend) { - $users = array_merge($users, array_values($backend->searchInGroup($this->gid, $search, $limit ?? -1, $offset ?? 0))); + $users += $backend->searchInGroup($this->gid, $search, $limit ?? -1, $offset ?? 0); } else { $userIds = $backend->usersInGroup($this->gid, $search, $limit ?? -1, $offset ?? 0); $userManager = \OCP\Server::get(IUserManager::class); - $userObjects = array_map( - fn (string $userId): IUser => new LazyUser($userId, $userManager), - $userIds - ); - $users = array_merge($users, $userObjects); + foreach ($userIds as $userId) { + if (!isset($users[$userId])) { + $users[$userId] = new LazyUser($userId, $userManager); + } + } } if (!is_null($limit) and $limit <= 0) { return $users; @@ -375,10 +375,7 @@ public function delete() { * @param string[] $userIds an array containing user IDs * @return \OC\User\User[] an Array with the userId as Key and \OC\User\User as value */ - private function getVerifiedUsers($userIds) { - if (!is_array($userIds)) { - return []; - } + private function getVerifiedUsers(array $userIds): array { $users = []; foreach ($userIds as $userId) { $user = $this->userManager->get($userId); diff --git a/tests/lib/Group/GroupTest.php b/tests/lib/Group/GroupTest.php index 993898803831b..ac648fd7b4bbf 100644 --- a/tests/lib/Group/GroupTest.php +++ b/tests/lib/Group/GroupTest.php @@ -310,7 +310,7 @@ public function testSearchUsers() { $users = $group->searchUsers('2'); $this->assertEquals(1, count($users)); - $user2 = $users['user2']; + $user2 = reset($users); $this->assertEquals('user2', $user2->getUID()); } @@ -336,7 +336,7 @@ public function testSearchUsersMultipleBackends() { $users = $group->searchUsers('2'); $this->assertEquals(1, count($users)); - $user2 = $users['user2']; + $user2 = reset($users); $this->assertEquals('user2', $user2->getUID()); } @@ -355,7 +355,7 @@ public function testSearchUsersLimitAndOffset() { $users = $group->searchUsers('user', 1, 1); $this->assertEquals(1, count($users)); - $user2 = $users['user2']; + $user2 = reset($users); $this->assertEquals('user2', $user2->getUID()); } @@ -381,8 +381,8 @@ public function testSearchUsersMultipleBackendsLimitAndOffset() { $users = $group->searchUsers('user', 2, 1); $this->assertEquals(2, count($users)); - $user2 = $users['user2']; - $user1 = $users['user1']; + $user2 = reset($users); + $user1 = next($users); $this->assertEquals('user2', $user2->getUID()); $this->assertEquals('user1', $user1->getUID()); } diff --git a/tests/lib/Group/ManagerTest.php b/tests/lib/Group/ManagerTest.php index 186f9d619a29a..710d3888d55ab 100644 --- a/tests/lib/Group/ManagerTest.php +++ b/tests/lib/Group/ManagerTest.php @@ -27,6 +27,7 @@ use OC\User\User; use OC\User\Manager; use OCP\GroupInterface; +use OCP\Group\Backend\ISearchableGroupBackend; use OCP\ICacheFactory; use OCP\IUser; use PHPUnit\Framework\MockObject\MockObject; @@ -34,6 +35,9 @@ use Symfony\Component\EventDispatcher\EventDispatcherInterface; use Test\TestCase; +interface ISearchableGroupInterface extends ISearchableGroupBackend, GroupInterface { +} + class ManagerTest extends TestCase { /** @var Manager|MockObject */ protected $userManager; @@ -79,7 +83,7 @@ private function getTestBackend($implementedActions = null) { } // need to declare it this way due to optional methods // thanks to the implementsActions logic - $backend = $this->getMockBuilder(GroupInterface::class) + $backend = $this->getMockBuilder(ISearchableGroupInterface::class) ->disableOriginalConstructor() ->setMethods([ 'getGroupDetails',