Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix performance problem when searching for groups in the groupfolder app #32201

Closed
wants to merge 12 commits into from
19 changes: 11 additions & 8 deletions apps/user_ldap/lib/GroupPluginManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ public function deleteGroup(string $gid): bool {
*
* Adds a user to a group.
*/
public function addToGroup($uid, $gid) {
public function addToGroup(string $uid, string $gid): bool {
$plugin = $this->which[GroupInterface::ADD_TO_GROUP];

if ($plugin) {
Expand All @@ -144,7 +144,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 {
$plugin = $this->which[GroupInterface::REMOVE_FROM_GROUP];

if ($plugin) {
Expand All @@ -157,29 +157,32 @@ public function removeFromGroup($uid, $gid) {
* get the number of all users matching the search string in a group
* @param string $gid ID of the group
* @param string $search query string
* @return int|false
* @throws \Exception
*/
public function countUsersInGroup($gid, $search = '') {
public function countUsersInGroup(string $gid, string $search = ''): int {
$plugin = $this->which[GroupInterface::COUNT_USERS];

if ($plugin) {
return $plugin->countUsersInGroup($gid,$search);
return (int)$plugin->countUsersInGroup($gid, $search);
}
throw new \Exception('No plugin implements countUsersInGroup in this LDAP Backend.');
}

/**
* get an array with group details
* @param string $gid
* @return array|false
* @return array
* @throws \Exception
*/
public function getGroupDetails($gid) {
public function getGroupDetails($gid): array {
$plugin = $this->which[GroupInterface::GROUP_DETAILS];

if ($plugin) {
return $plugin->getGroupDetails($gid);
$details = $plugin->getGroupDetails($gid);
if ($details === false) {
return [];
CarlSchwan marked this conversation as resolved.
Show resolved Hide resolved
}
return $details;
}
throw new \Exception('No plugin implements getGroupDetails in this LDAP Backend.');
}
Expand Down
52 changes: 31 additions & 21 deletions apps/user_ldap/lib/Group_LDAP.php
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,19 @@
use OC;
use OC\Cache\CappedMemoryCache;
use OC\ServerNotAvailableException;
use OCP\Group\Backend\ABackend;
use OCP\Group\Backend\IAddToGroupBackend;
use OCP\Group\Backend\ICountUsersBackend;
use OCP\Group\Backend\IGetDisplayNameBackend;
use OCP\Group\Backend\IDeleteGroupBackend;
use OCP\Group\Backend\IGroupDetailsBackend;
use OCP\Group\Backend\IRemoveFromGroupBackend;
use OCP\GroupInterface;
use Psr\Log\LoggerInterface;

class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, IGetDisplayNameBackend, IDeleteGroupBackend {
class Group_LDAP extends ABackend implements GroupInterface, IGroupLDAP,
IGetDisplayNameBackend, IDeleteGroupBackend, ICountUsersBackend, IAddToGroupBackend,
IRemoveFromGroupBackend {
protected $enabled = false;

/** @var CappedMemoryCache<string[]> $cachedGroupMembers array of users with gid as key */
Expand All @@ -61,18 +68,17 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I
protected CappedMemoryCache $cachedGroupsByMember;
/** @var CappedMemoryCache<string[]> $cachedNestedGroups array of groups with gid (DN) as key */
protected CappedMemoryCache $cachedNestedGroups;
/** @var GroupPluginManager */
protected $groupPluginManager;
/** @var LoggerInterface */
protected $logger;
protected GroupPluginManager $groupPluginManager;
protected LoggerInterface $logger;
protected Access $access;

/**
* @var string $ldapGroupMemberAssocAttr contains the LDAP setting (in lower case) with the same name
*/
protected $ldapGroupMemberAssocAttr;

public function __construct(Access $access, GroupPluginManager $groupPluginManager) {
parent::__construct($access);
CarlSchwan marked this conversation as resolved.
Show resolved Hide resolved
$this->access = $access;
$filter = $this->access->connection->ldapGroupFilter;
$gAssoc = $this->access->connection->ldapGroupMemberAssocAttr;
if (!empty($filter) && !empty($gAssoc)) {
Expand Down Expand Up @@ -1000,22 +1006,19 @@ public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0) {
}

/**
* returns the number of users in a group, who match the search term
* Returns the number of users in a group, who match the search term
*
* @param string $gid the internal group name
* @param string $search optional, a search string
* @return int|bool
* @throws Exception
* @throws ServerNotAvailableException
*/
public function countUsersInGroup($gid, $search = '') {
public function countUsersInGroup(string $gid, string $search = ''): int {
CarlSchwan marked this conversation as resolved.
Show resolved Hide resolved
if ($this->groupPluginManager->implementsActions(GroupInterface::COUNT_USERS)) {
return $this->groupPluginManager->countUsersInGroup($gid, $search);
}

$cacheKey = 'countUsersInGroup-' . $gid . '-' . $search;
if (!$this->enabled || !$this->groupExists($gid)) {
return false;
return 0;
}
$groupUsers = $this->access->connection->getFromCache($cacheKey);
if (!is_null($groupUsers)) {
Expand All @@ -1025,16 +1028,16 @@ public function countUsersInGroup($gid, $search = '') {
$groupDN = $this->access->groupname2dn($gid);
if (!$groupDN) {
// group couldn't be found, return empty result set
$this->access->connection->writeToCache($cacheKey, false);
return false;
$this->access->connection->writeToCache($cacheKey, 0);
return 0;
}

$members = $this->_groupMembers($groupDN);
$primaryUserCount = $this->countUsersInPrimaryGroup($groupDN, '');
if (!$members && $primaryUserCount === 0) {
//in case users could not be retrieved, return empty result set
$this->access->connection->writeToCache($cacheKey, false);
return false;
$this->access->connection->writeToCache($cacheKey, 0);
return 0;
}

if ($search === '') {
Expand Down Expand Up @@ -1203,7 +1206,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);
Expand Down Expand Up @@ -1287,7 +1290,7 @@ public function deleteGroup(string $gid): bool {
* @return bool
* @throws Exception
*/
public function addToGroup($uid, $gid) {
public function addToGroup(string $uid, string $gid): bool {
if ($this->groupPluginManager->implementsActions(GroupInterface::ADD_TO_GROUP)) {
if ($ret = $this->groupPluginManager->addToGroup($uid, $gid)) {
$this->access->connection->clearCache();
Expand All @@ -1306,7 +1309,7 @@ public function addToGroup($uid, $gid) {
* @return bool
* @throws Exception
*/
public function removeFromGroup($uid, $gid) {
public function removeFromGroup(string $uid, string $gid): bool {
if ($this->groupPluginManager->implementsActions(GroupInterface::REMOVE_FROM_GROUP)) {
if ($ret = $this->groupPluginManager->removeFromGroup($uid, $gid)) {
$this->access->connection->clearCache();
Expand All @@ -1321,10 +1324,10 @@ public function removeFromGroup($uid, $gid) {
* Gets group details
*
* @param string $gid Name of the group
* @return array|false
* @return array{displayName?: string}
* @throws Exception
*/
public function getGroupDetails($gid) {
public function getGroupDetails(string $gid): array {
if ($this->groupPluginManager->implementsActions(GroupInterface::GROUP_DETAILS)) {
return $this->groupPluginManager->getGroupDetails($gid);
}
Expand Down Expand Up @@ -1370,4 +1373,11 @@ public function getDisplayName(string $gid): string {

return '';
}

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);
}
}
51 changes: 41 additions & 10 deletions apps/user_ldap/lib/Group_Proxy.php
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,19 @@
*/
namespace OCA\User_LDAP;

use OCP\Group\Backend\ABackend;
use OCP\Group\Backend\IDeleteGroupBackend;
use OCP\Group\Backend\IGetDisplayNameBackend;
use OCP\Group\Backend\IGroupDetailsBackend;
use OCP\Group\Backend\INamedBackend;
use OCP\Group\Backend\IAddToGroupBackend;
use OCP\Group\Backend\ICountUsersBackend;
use OCP\Group\Backend\IRemoveFromGroupBackend;
use OCP\GroupInterface;

class Group_Proxy extends Proxy implements \OCP\GroupInterface, IGroupLDAP, IGetDisplayNameBackend, INamedBackend, IDeleteGroupBackend {
class Group_Proxy extends Proxy implements GroupInterface, IGroupLDAP,
IGetDisplayNameBackend, IDeleteGroupBackend, ICountUsersBackend, IAddToGroupBackend,
IRemoveFromGroupBackend {
private $backends = [];
private $refBackend = null;

Expand Down Expand Up @@ -187,7 +195,7 @@ public function deleteGroup(string $gid): bool {
*
* Adds a user to a group.
*/
public function addToGroup($uid, $gid) {
public function addToGroup(string $uid, string $gid): bool {
return $this->handleRequest(
$gid, 'addToGroup', [$uid, $gid]);
}
Expand All @@ -201,7 +209,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 {
return $this->handleRequest(
$gid, 'removeFromGroup', [$uid, $gid]);
}
Expand All @@ -211,24 +219,32 @@ public function removeFromGroup($uid, $gid) {
*
* @param string $gid the internal group name
* @param string $search optional, a search string
* @return int|bool
*/
public function countUsersInGroup($gid, $search = '') {
public function countUsersInGroup(string $gid, string $search = ''): int {
CarlSchwan marked this conversation as resolved.
Show resolved Hide resolved
return $this->handleRequest(
$gid, 'countUsersInGroup', [$gid, $search]);
}

/**
* get an array with group details
*
* @param string $gid
* @return array|false
* Get an array with group details
*/
public function getGroupDetails($gid) {
public function getGroupDetails(string $gid): array {
return $this->handleRequest(
$gid, 'getGroupDetails', [$gid]);
}

public function getGroupsDetails(array $gids): array {
if (!($this instanceof IGroupDetailsBackend || $this->implementsActions(GroupInterface::GROUP_DETAILS))) {
throw new \Exception("Should not have been called");
}

$groupData = [];
foreach ($gids as $gid) {
$groupData[$gid] = $this->handleRequest($gid, 'getGroupDetails', [$gid]);
}
return $groupData;
}

/**
* get a list of all groups
*
Expand Down Expand Up @@ -259,6 +275,17 @@ public function groupExists($gid) {
return $this->handleRequest($gid, 'groupExists', [$gid]);
}

public function groupsExists(array $gids): array {
$existingGroups = [];
foreach ($gids as $gid) {
$exits = $this->handleRequest($gid, 'groupExists', [$gid]);
if ($exits) {
$existingGroups[] = $gid;
}
}
return $existingGroups;
}

/**
* Check if backend implements actions
*
Expand Down Expand Up @@ -306,4 +333,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]);
}
}
10 changes: 9 additions & 1 deletion apps/user_ldap/lib/LDAP.php
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,15 @@ private function preFunctionCall(string $functionName, array $args): void {
$this->curArgs = $args;

if ($this->dataCollector !== null) {
$args = array_map(fn ($item) => (!$this->isResource($item) ? $item : '(resource)'), $this->curArgs);
$args = array_map(function ($item) {
if ($this->isResource($item)) {
return '(resource)';
}
if (isset($item[0]['value']['cookie'])) {
$item[0]['value']['cookie'] = "";
}
return $item;
}, $this->curArgs);

$this->dataCollector->startLdapRequest($this->curFunc, $args);
}
Expand Down
2 changes: 1 addition & 1 deletion apps/user_ldap/lib/Proxy.php
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ abstract protected function walkBackends($id, $method, $parameters);
* @param string $id
* @return Access
*/
abstract public function getLDAPAccess($id);
abstract public function getLDAPAccess($gid);

Check notice

Code scanning / Psalm

MissingParamType

Parameter $gid has no provided type

abstract protected function activeBackends(): int;

Expand Down
16 changes: 8 additions & 8 deletions apps/user_ldap/tests/LDAPGroupPluginDummy.php
Original file line number Diff line number Diff line change
Expand Up @@ -37,19 +37,19 @@ public function deleteGroup($gid) {
return null;
}

public function addToGroup($uid, $gid) {
return null;
public function addToGroup($uid, $gid): bool {
return false;
}

public function removeFromGroup($uid, $gid) {
return null;
public function removeFromGroup($uid, $gid): bool {
return false;
}

public function countUsersInGroup($gid, $search = '') {
return null;
public function countUsersInGroup($gid, $search = ''): int {
return 0;
}

public function getGroupDetails($gid) {
return null;
public function getGroupDetails($gid): array {
return [];
}
}
Loading