From 1e4ac22c946969088a2265730095775e8fc5c645 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Sun, 12 Dec 2021 14:09:16 +0100 Subject: Make it possible to return nested records whem walking over groups Signed-off-by: Arthur Schiwon --- apps/user_ldap/lib/Group_LDAP.php | 47 +++++++++++++++++++++++++++++++++++---- 1 file changed, 43 insertions(+), 4 deletions(-) (limited to 'apps') diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php index 5072f9d3f74..3db3c6c7664 100644 --- a/apps/user_ldap/lib/Group_LDAP.php +++ b/apps/user_ldap/lib/Group_LDAP.php @@ -360,7 +360,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I $nesting = (int)$this->access->connection->ldapNestedGroups; // depending on the input, we either have a list of DNs or a list of LDAP records // also, the output expects either DNs or records. Testing the first element should suffice. - $recordMode = is_array($list) && isset($list[0]) && is_array($list[0]) && isset($list[0]['dn'][0]); + $recordMode = isset($list[0]) && is_array($list[0]) && isset($list[0]['dn'][0]); if ($nesting !== 1) { if ($recordMode) { @@ -392,6 +392,36 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I return $recordMode ? array_filter($seen, 'is_array') : array_keys($seen); } + private function walkNestedGroupsReturnRecords(string $dn, Closure $fetcher, array $list, array &$seen = []): array { + $nesting = (int)$this->access->connection->ldapNestedGroups; + + if ($nesting !== 1) { + // the keys are numeric, but should hold the DN + return array_reduce($list, function ($transformed, $record) use ($dn) { + if ($record['dn'][0] != $dn) { + $transformed[$record['dn'][0]] = $record; + } + return $transformed; + }, []); + } + + while ($record = array_shift($list)) { + $recordDN = $record['dn'][0] ?? $record; + if ($recordDN === $dn || array_key_exists($recordDN, $seen)) { + // Prevent loops + continue; + } + $fetched = $fetcher($record); + $list = array_merge($list, $fetched); + if (!isset($seen[$recordDN]) || is_bool($seen[$recordDN]) && is_array($record)) { + $seen[$recordDN] = $record; + } + } + + // on record mode, filter out intermediate state + return array_filter($seen, 'is_array'); + } + /** * translates a gidNumber into an ownCloud internal name * @@ -848,6 +878,9 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I // avoid loops return []; } + if ($this->cachedGroupsByMember[$dn]) { + return $this->cachedGroupsByMember[$dn]; + } $allGroups = []; $seen[$dn] = true; $filter = $this->access->connection->ldapGroupMemberAssocAttr . '=' . $dn; @@ -875,9 +908,11 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I $dn = ""; } - $allGroups = $this->walkNestedGroups($dn, $fetcher, $groups, $seen); + $allGroups = $this->walkNestedGroupsReturnRecords($dn, $fetcher, $groups, $seen); $visibleGroups = $this->filterValidGroups($allGroups); - return array_intersect_key($allGroups, $visibleGroups); + $effectiveGroups = array_intersect_key($allGroups, $visibleGroups); + $this->cachedGroupsByMember[$dn] = $effectiveGroups; + return $effectiveGroups; } /** @@ -1183,7 +1218,11 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I $validGroupDNs = []; foreach ($listOfGroups as $key => $item) { $dn = is_string($item) ? $item : $item['dn'][0]; - $gid = $this->access->dn2groupname($dn); + if(is_array($item) && !isset($item[$this->access->connection->ldapGroupDisplayName][0])) { + continue; + } + $name = $item[$this->access->connection->ldapGroupDisplayName][0] ?? null; + $gid = $this->access->dn2groupname($dn, $name); if (!$gid) { continue; } -- cgit v1.2.3 From ad2fdbe3776dc2d25c646d37b9cff448d5fdf326 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Sun, 12 Dec 2021 14:11:31 +0100 Subject: Refactor code to split common loop Signed-off-by: Arthur Schiwon --- apps/user_ldap/lib/Group_LDAP.php | 52 +++++++++++---------------------------- 1 file changed, 15 insertions(+), 37 deletions(-) (limited to 'apps') diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php index 3db3c6c7664..b0d5909fd81 100644 --- a/apps/user_ldap/lib/Group_LDAP.php +++ b/apps/user_ldap/lib/Group_LDAP.php @@ -305,7 +305,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I $fetcher = function ($memberDN) use (&$seen) { return $this->_groupMembers($memberDN, $seen); }; - $allMembers = $this->walkNestedGroups($dnGroup, $fetcher, $members, $seen); + $allMembers = $this->walkNestedGroupsReturnDNs($dnGroup, $fetcher, $members, $seen); } $allMembers += $this->getDynamicGroupMembers($dnGroup); @@ -352,29 +352,11 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I return $nestedGroups; }; - $groups = $this->walkNestedGroups($dn, $fetcher, $groups); + $groups = $this->walkNestedGroupsReturnDNs($dn, $fetcher, $groups); return $this->filterValidGroups($groups); } - private function walkNestedGroups(string $dn, Closure $fetcher, array $list, array &$seen = []): array { - $nesting = (int)$this->access->connection->ldapNestedGroups; - // depending on the input, we either have a list of DNs or a list of LDAP records - // also, the output expects either DNs or records. Testing the first element should suffice. - $recordMode = isset($list[0]) && is_array($list[0]) && isset($list[0]['dn'][0]); - - if ($nesting !== 1) { - if ($recordMode) { - // the keys are numeric, but should hold the DN - return array_reduce($list, function ($transformed, $record) use ($dn) { - if ($record['dn'][0] != $dn) { - $transformed[$record['dn'][0]] = $record; - } - return $transformed; - }, []); - } - return $list; - } - + private function processListFromWalkingNestedGroups(array &$list, array &$seen, string $dn, Closure $fetcher): void { while ($record = array_shift($list)) { $recordDN = $record['dn'][0] ?? $record; if ($recordDN === $dn || array_key_exists($recordDN, $seen)) { @@ -387,9 +369,17 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I $seen[$recordDN] = $record; } } + } + + private function walkNestedGroupsReturnDNs(string $dn, Closure $fetcher, array $list, array &$seen = []): array { + $nesting = (int)$this->access->connection->ldapNestedGroups; + + if ($nesting !== 1) { + return $list; + } - // on record mode, filter out intermediate state - return $recordMode ? array_filter($seen, 'is_array') : array_keys($seen); + $this->processListFromWalkingNestedGroups($list, $seen, $dn, $fetcher); + return array_keys($seen); } private function walkNestedGroupsReturnRecords(string $dn, Closure $fetcher, array $list, array &$seen = []): array { @@ -405,20 +395,8 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I }, []); } - while ($record = array_shift($list)) { - $recordDN = $record['dn'][0] ?? $record; - if ($recordDN === $dn || array_key_exists($recordDN, $seen)) { - // Prevent loops - continue; - } - $fetched = $fetcher($record); - $list = array_merge($list, $fetched); - if (!isset($seen[$recordDN]) || is_bool($seen[$recordDN]) && is_array($record)) { - $seen[$recordDN] = $record; - } - } - - // on record mode, filter out intermediate state + $this->processListFromWalkingNestedGroups($list, $seen, $dn, $fetcher); + // filter out intermediate state return array_filter($seen, 'is_array'); } -- cgit v1.2.3 From 5647093319e6083a4571651f7491c5aa50df4a03 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Sun, 12 Dec 2021 14:14:16 +0100 Subject: Cache intermediates Signed-off-by: Arthur Schiwon Co-authored-by: Carl Schwan --- apps/user_ldap/lib/Access.php | 6 ++++++ apps/user_ldap/lib/Group_LDAP.php | 8 +++++++- 2 files changed, 13 insertions(+), 1 deletion(-) (limited to 'apps') diff --git a/apps/user_ldap/lib/Access.php b/apps/user_ldap/lib/Access.php index cec192721a5..7ad6552780c 100644 --- a/apps/user_ldap/lib/Access.php +++ b/apps/user_ldap/lib/Access.php @@ -511,6 +511,11 @@ class Access extends LDAPUtility { * @throws \Exception */ public function dn2ocname($fdn, $ldapName = null, $isUser = true, &$newlyMapped = null, array $record = null) { + static $intermediates = []; + if (isset($intermediates[($isUser ? 'user-' : 'group-') . $fdn])) { + return false; // is a known intermediate + } + $newlyMapped = false; if ($isUser) { $mapper = $this->getUserMapper(); @@ -546,6 +551,7 @@ class Access extends LDAPUtility { $ldapName = $this->readAttribute($fdn, $nameAttribute, $filter); if (!isset($ldapName[0]) || empty($ldapName[0])) { $this->logger->debug('No or empty name for ' . $fdn . ' with filter ' . $filter . '.', ['app' => 'user_ldap']); + $intermediates[($isUser ? 'user-' : 'group-') . $fdn] = true; return false; } $ldapName = $ldapName[0]; diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php index b0d5909fd81..aa4c6572869 100644 --- a/apps/user_ldap/lib/Group_LDAP.php +++ b/apps/user_ldap/lib/Group_LDAP.php @@ -363,7 +363,13 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I // Prevent loops continue; } - $fetched = $fetcher($record); + + $cacheKey = 'walkNestedGroups_' . $recordDN; + $fetched = $this->access->connection->getFromCache($cacheKey); + if ($fetched === null) { + $fetched = $fetcher($record); + $fetched = $this->access->connection->writeToCache($cacheKey, $fetched); + } $list = array_merge($list, $fetched); if (!isset($seen[$recordDN]) || is_bool($seen[$recordDN]) && is_array($record)) { $seen[$recordDN] = $record; -- cgit v1.2.3 From 0fd7a51e3c78fabc50505f4c8c3a27eaad46b00b Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Fri, 10 Dec 2021 16:36:14 +0100 Subject: Add more type hinting Signed-off-by: Carl Schwan --- apps/user_ldap/lib/Group_LDAP.php | 74 +++++++++++++++++++++++---------------- 1 file changed, 43 insertions(+), 31 deletions(-) (limited to 'apps') diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php index aa4c6572869..df34cd349ff 100644 --- a/apps/user_ldap/lib/Group_LDAP.php +++ b/apps/user_ldap/lib/Group_LDAP.php @@ -88,7 +88,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I } /** - * is user in group? + * Check if user is in group * * @param string $uid uid of the user * @param string $gid gid of the group @@ -240,18 +240,21 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I } /** + * Get group members from dn. + * @psalm-param array $seen List of DN that have already been processed. * @throws ServerNotAvailableException */ private function _groupMembers(string $dnGroup, ?array &$seen = null): array { if ($seen === null) { $seen = []; // the root entry has to be marked as processed to avoid infinite loops, - // but not included in the results laters on + // but not included in the results later on $excludeFromResult = $dnGroup; } // cache only base groups, otherwise groups get additional unwarranted members $shouldCacheResult = count($seen) === 0; + /** @psalm-var array $rawMemberReads */ static $rawMemberReads = []; // runtime cache for intermediate ldap read results $allMembers = []; @@ -331,6 +334,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I } /** + * @return string[] * @throws ServerNotAvailableException */ private function _getGroupDNsFromMemberOf(string $dn): array { @@ -356,6 +360,11 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I return $this->filterValidGroups($groups); } + /** + * @psalm-param list}|string> $list + * @psalm-param array $seen List of DN that have already been processed. + * @param Closure(string) $fetcher + */ private function processListFromWalkingNestedGroups(array &$list, array &$seen, string $dn, Closure $fetcher): void { while ($record = array_shift($list)) { $recordDN = $record['dn'][0] ?? $record; @@ -377,6 +386,11 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I } } + /** + * @psalm-param list}|string> $list + * @psalm-param array $seen List of DN that have already been processed. + * @param Closure(string) $fetcher + */ private function walkNestedGroupsReturnDNs(string $dn, Closure $fetcher, array $list, array &$seen = []): array { $nesting = (int)$this->access->connection->ldapNestedGroups; @@ -388,6 +402,12 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I return array_keys($seen); } + /** + * @psalm-param list}> $list + * @psalm-param array $seen List of DN that have already been processed. + * @return array[] An array of records + * @param Closure(string) $fetcher + */ private function walkNestedGroupsReturnRecords(string $dn, Closure $fetcher, array $list, array &$seen = []): array { $nesting = (int)$this->access->connection->ldapNestedGroups; @@ -407,9 +427,9 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I } /** - * translates a gidNumber into an ownCloud internal name + * Translates a gidNumber into the Nextcloud internal name. * - * @return string|bool + * @return string|false The nextcloud internal name. * @throws Exception * @throws ServerNotAvailableException */ @@ -430,6 +450,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I } /** + * @return string|null|false The name of the group * @throws ServerNotAvailableException * @throws Exception */ @@ -452,9 +473,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I } /** - * returns the entry's gidNumber - * - * @return string|bool + * @return string|bool The entry's gidNumber * @throws ServerNotAvailableException */ private function getEntryGidNumber(string $dn, string $attribute) { @@ -466,7 +485,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I } /** - * @return string|bool + * @return string|bool The group's gidNumber * @throws ServerNotAvailableException */ public function getGroupGidNumber(string $dn) { @@ -474,9 +493,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I } /** - * returns the user's gidNumber - * - * @return string|bool + * @return string|bool The user's gidNumber * @throws ServerNotAvailableException */ public function getUserGidNumber(string $dn) { @@ -511,8 +528,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I } /** - * returns 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( @@ -539,7 +555,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I /** * @throws ServerNotAvailableException - * @return bool + * @return false|string */ public function getUserGroupByGid(string $dn) { $groupID = $this->getUserGidNumber($dn); @@ -554,9 +570,9 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I } /** - * translates a primary group ID into an Nextcloud internal name + * Translates a primary group ID into an Nextcloud internal name * - * @return string|bool + * @return string|false * @throws Exception * @throws ServerNotAvailableException */ @@ -581,9 +597,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I } /** - * returns the entry's primary group ID - * - * @return string|bool + * @return string|false The entry's group Id * @throws ServerNotAvailableException */ private function getEntryGroupID(string $dn, string $attribute) { @@ -595,7 +609,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I } /** - * @return string|bool + * @return string|false The entry's primary group Id * @throws ServerNotAvailableException */ public function getGroupPrimaryGroupID(string $dn) { @@ -603,7 +617,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I } /** - * @return string|bool + * @return string|false * @throws ServerNotAvailableException */ public function getUserPrimaryGroupIDs(string $dn) { @@ -683,7 +697,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I } /** - * @return string|bool + * @return string|false * @throws ServerNotAvailableException */ public function getUserPrimaryGroup(string $dn) { @@ -776,14 +790,12 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I && $this->ldapGroupMemberAssocAttr !== 'memberuid' && $this->ldapGroupMemberAssocAttr !== 'zimbramailforwardingaddress') { $groupDNs = $this->_getGroupDNsFromMemberOf($userDN); - if (is_array($groupDNs)) { - foreach ($groupDNs as $dn) { - $groupName = $this->access->dn2groupname($dn); - if (is_string($groupName)) { - // be sure to never return false if the dn could not be - // resolved to a name, for whatever reason. - $groups[] = $groupName; - } + foreach ($groupDNs as $dn) { + $groupName = $this->access->dn2groupname($dn); + if (is_string($groupName)) { + // be sure to never return false if the dn could not be + // resolved to a name, for whatever reason. + $groups[] = $groupName; } } @@ -938,7 +950,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I $groupDN = $this->access->groupname2dn($gid); if (!$groupDN) { - // group couldn't be found, return empty resultset + // group couldn't be found, return empty result-set $this->access->connection->writeToCache($cacheKey, []); return []; } -- cgit v1.2.3 From 49aa352069f5c4703d01593cb51b2787d2a27aeb Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Thu, 9 Dec 2021 11:51:13 +0100 Subject: Unify a bit the types of the fetcher Now it will only accept a string as parameter instead of either a string (DN) or a array (complete record). Signed-off-by: Carl Schwan --- apps/user_ldap/lib/Group_LDAP.php | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-) (limited to 'apps') diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php index df34cd349ff..6b50c30ff0a 100644 --- a/apps/user_ldap/lib/Group_LDAP.php +++ b/apps/user_ldap/lib/Group_LDAP.php @@ -61,15 +61,13 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I protected CappedMemoryCache $cachedGroupsByMember; /** @var CappedMemoryCache $cachedNestedGroups array of groups with gid (DN) as key */ protected CappedMemoryCache $cachedNestedGroups; - /** @var GroupPluginManager */ - protected $groupPluginManager; - /** @var LoggerInterface */ - protected $logger; + protected GroupInterface $groupPluginManager; + protected LoggerInterface $logger; /** * @var string $ldapGroupMemberAssocAttr contains the LDAP setting (in lower case) with the same name */ - protected $ldapGroupMemberAssocAttr; + protected string $ldapGroupMemberAssocAttr; public function __construct(Access $access, GroupPluginManager $groupPluginManager) { parent::__construct($access); @@ -305,7 +303,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I $rawMemberReads[$dnGroup] = $members; } if (is_array($members)) { - $fetcher = function ($memberDN) use (&$seen) { + $fetcher = function (string $memberDN) use (&$seen) { return $this->_groupMembers($memberDN, $seen); }; $allMembers = $this->walkNestedGroupsReturnDNs($dnGroup, $fetcher, $members, $seen); @@ -343,7 +341,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I return []; } - $fetcher = function ($groupDN) { + $fetcher = function (string $groupDN) { if (isset($this->cachedNestedGroups[$groupDN])) { $nestedGroups = $this->cachedNestedGroups[$groupDN]; } else { @@ -363,7 +361,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I /** * @psalm-param list}|string> $list * @psalm-param array $seen List of DN that have already been processed. - * @param Closure(string) $fetcher + * @psalm-param Closure(string) $fetcher */ private function processListFromWalkingNestedGroups(array &$list, array &$seen, string $dn, Closure $fetcher): void { while ($record = array_shift($list)) { @@ -376,7 +374,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I $cacheKey = 'walkNestedGroups_' . $recordDN; $fetched = $this->access->connection->getFromCache($cacheKey); if ($fetched === null) { - $fetched = $fetcher($record); + $fetched = $fetcher($recordDN); $fetched = $this->access->connection->writeToCache($cacheKey, $fetched); } $list = array_merge($list, $fetched); @@ -389,7 +387,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I /** * @psalm-param list}|string> $list * @psalm-param array $seen List of DN that have already been processed. - * @param Closure(string) $fetcher + * @psalm-param Closure(string) $fetcher */ private function walkNestedGroupsReturnDNs(string $dn, Closure $fetcher, array $list, array &$seen = []): array { $nesting = (int)$this->access->connection->ldapNestedGroups; @@ -405,15 +403,15 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I /** * @psalm-param list}> $list * @psalm-param array $seen List of DN that have already been processed. + * @psalm-param Closure(string) $fetcher * @return array[] An array of records - * @param Closure(string) $fetcher */ private function walkNestedGroupsReturnRecords(string $dn, Closure $fetcher, array $list, array &$seen = []): array { $nesting = (int)$this->access->connection->ldapNestedGroups; if ($nesting !== 1) { // the keys are numeric, but should hold the DN - return array_reduce($list, function ($transformed, $record) use ($dn) { + return array_reduce($list, function (array $transformed, array $record) use ($dn) { if ($record['dn'][0] != $dn) { $transformed[$record['dn'][0]] = $record; } @@ -893,9 +891,12 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I $groups = $this->access->fetchListOfGroups($filter, [strtolower($this->access->connection->ldapGroupMemberAssocAttr), $this->access->connection->ldapGroupDisplayName, 'dn']); - $fetcher = function ($dn) use (&$seen) { - if (is_array($dn) && isset($dn['dn'][0])) { - $dn = $dn['dn'][0]; + $fetcher = function (string $dn) use (&$seen) { + return $this->getGroupsByMember($dn, $seen); + }; + + if (empty($dn)) { + $dn = ""; } return $this->getGroupsByMember($dn, $seen); }; @@ -1214,7 +1215,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I $validGroupDNs = []; foreach ($listOfGroups as $key => $item) { $dn = is_string($item) ? $item : $item['dn'][0]; - if(is_array($item) && !isset($item[$this->access->connection->ldapGroupDisplayName][0])) { + if (is_array($item) && !isset($item[$this->access->connection->ldapGroupDisplayName][0])) { continue; } $name = $item[$this->access->connection->ldapGroupDisplayName][0] ?? null; -- cgit v1.2.3 From 6522f8a6d98f81a10f68cd01294201997d51c3a0 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Fri, 10 Dec 2021 16:39:50 +0100 Subject: Fix merging list with null This fixes some cases observed with the debugger where we end up merging a non empty list with null. The result is then null and the looping over the items would then end. Signed-off-by: Carl Schwan --- apps/user_ldap/lib/Group_LDAP.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'apps') diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php index 6b50c30ff0a..c2259c42aba 100644 --- a/apps/user_ldap/lib/Group_LDAP.php +++ b/apps/user_ldap/lib/Group_LDAP.php @@ -24,6 +24,7 @@ * @author Victor Dubiniuk * @author Vinicius Cubas Brand * @author Xuanwo + * @author Carl Schwan * * @license AGPL-3.0 * @@ -375,7 +376,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I $fetched = $this->access->connection->getFromCache($cacheKey); if ($fetched === null) { $fetched = $fetcher($recordDN); - $fetched = $this->access->connection->writeToCache($cacheKey, $fetched); + $this->access->connection->writeToCache($cacheKey, $fetched); } $list = array_merge($list, $fetched); if (!isset($seen[$recordDN]) || is_bool($seen[$recordDN]) && is_array($record)) { -- cgit v1.2.3 From d07f43dc12addf21aaa76e6b330560b42d8f3ced Mon Sep 17 00:00:00 2001 From: Côme Chilliet Date: Mon, 13 Dec 2021 12:17:06 +0100 Subject: Refactor _groupMembers to correctly use cache on intermediate results 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 | 63 +++++++++++++++++---------------------- 1 file changed, 28 insertions(+), 35 deletions(-) (limited to 'apps') diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php index c2259c42aba..3962b064fde 100644 --- a/apps/user_ldap/lib/Group_LDAP.php +++ b/apps/user_ldap/lib/Group_LDAP.php @@ -240,26 +240,15 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I /** * Get group members from dn. - * @psalm-param array $seen List of DN that have already been processed. + * @psalm-param array $seen List of DN that have already been processed. * @throws ServerNotAvailableException */ - private function _groupMembers(string $dnGroup, ?array &$seen = null): array { - if ($seen === null) { - $seen = []; - // the root entry has to be marked as processed to avoid infinite loops, - // but not included in the results later on - $excludeFromResult = $dnGroup; - } - // cache only base groups, otherwise groups get additional unwarranted members - $shouldCacheResult = count($seen) === 0; - - /** @psalm-var array $rawMemberReads */ - static $rawMemberReads = []; // runtime cache for intermediate ldap read results - $allMembers = []; - - if (array_key_exists($dnGroup, $seen)) { + private function _groupMembers(string $dnGroup, array &$seen = []): array { + if (isset($seen[$dnGroup])) { return []; } + $seen[$dnGroup] = true; + // used extensively in cron job, caching makes sense for nested groups $cacheKey = '_groupMembers' . $dnGroup; $groupMembers = $this->access->connection->getFromCache($cacheKey); @@ -288,40 +277,43 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I return $carry; }, []); if ($this->access->connection->ldapMatchingRuleInChainState === Configuration::LDAP_SERVER_FEATURE_AVAILABLE) { + $this->access->connection->writeToCache($cacheKey, $result); return $result; } elseif (!empty($memberRecords)) { $this->access->connection->ldapMatchingRuleInChainState = Configuration::LDAP_SERVER_FEATURE_AVAILABLE; $this->access->connection->saveConfiguration(); + $this->access->connection->writeToCache($cacheKey, $result); return $result; } // when feature availability is unknown, and the result is empty, continue and test with original approach } - $seen[$dnGroup] = 1; - $members = $rawMemberReads[$dnGroup] ?? null; - if ($members === null) { - $members = $this->access->readAttribute($dnGroup, $this->access->connection->ldapGroupMemberAssocAttr); - $rawMemberReads[$dnGroup] = $members; - } + $allMembers = []; + $members = $this->access->readAttribute($dnGroup, $this->access->connection->ldapGroupMemberAssocAttr); if (is_array($members)) { - $fetcher = function (string $memberDN) use (&$seen) { - return $this->_groupMembers($memberDN, $seen); - }; - $allMembers = $this->walkNestedGroupsReturnDNs($dnGroup, $fetcher, $members, $seen); + if ((int)$this->access->connection->ldapNestedGroups === 1) { + while ($recordDn = array_shift($members)) { + $nestedMembers = $this->_groupMembers($recordDn, $seen); + $members = array_merge($members, $nestedMembers); + $allMembers[] = $recordDn; + } + } else { + $allMembers = $members; + } } $allMembers += $this->getDynamicGroupMembers($dnGroup); - if (isset($excludeFromResult)) { - $index = array_search($excludeFromResult, $allMembers, true); - if ($index !== false) { - unset($allMembers[$index]); - } - } - if ($shouldCacheResult) { - $this->access->connection->writeToCache($cacheKey, $allMembers); - unset($rawMemberReads[$dnGroup]); + $allMembers = array_unique($allMembers); + + // A group cannot be a member of itself + $index = array_search($dnGroup, $allMembers, true); + if ($index !== false) { + unset($allMembers[$index]); } + + $this->access->connection->writeToCache($cacheKey, $allMembers); + if (isset($attemptedLdapMatchingRuleInChain) && $this->access->connection->ldapMatchingRuleInChainState === Configuration::LDAP_SERVER_FEATURE_UNKNOWN && !empty($allMembers) @@ -329,6 +321,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I $this->access->connection->ldapMatchingRuleInChainState = Configuration::LDAP_SERVER_FEATURE_UNAVAILABLE; $this->access->connection->saveConfiguration(); } + return $allMembers; } -- cgit v1.2.3 From 8b19cfcd885451080a5114dc451a3c78a8dccc81 Mon Sep 17 00:00:00 2001 From: Côme Chilliet Date: Mon, 13 Dec 2021 16:02:05 +0100 Subject: Small optimisation of _groupMembers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This will not change the result as users are check to be existing afterwards but avoids this check when we know it’s a group. Signed-off-by: Côme Chilliet --- apps/user_ldap/lib/Group_LDAP.php | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) (limited to 'apps') diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php index 3962b064fde..a7a22ff46d9 100644 --- a/apps/user_ldap/lib/Group_LDAP.php +++ b/apps/user_ldap/lib/Group_LDAP.php @@ -294,8 +294,13 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I if ((int)$this->access->connection->ldapNestedGroups === 1) { while ($recordDn = array_shift($members)) { $nestedMembers = $this->_groupMembers($recordDn, $seen); - $members = array_merge($members, $nestedMembers); - $allMembers[] = $recordDn; + if (!empty($nestedMembers)) { + // Group, queue its members for processing + $members = array_merge($members, $nestedMembers); + } else { + // User (or empty group, or previously seen group), add it to the member list + $allMembers[] = $recordDn; + } } } else { $allMembers = $members; -- cgit v1.2.3 From 02ccce17f7ea86d0a8e0df31a954cfdbef8affa7 Mon Sep 17 00:00:00 2001 From: Côme Chilliet Date: Mon, 13 Dec 2021 16:02:59 +0100 Subject: Add tests for nested groups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- apps/user_ldap/tests/Group_LDAPTest.php | 53 ++++++++++++++++++++++++++++----- 1 file changed, 45 insertions(+), 8 deletions(-) (limited to 'apps') diff --git a/apps/user_ldap/tests/Group_LDAPTest.php b/apps/user_ldap/tests/Group_LDAPTest.php index 236dcee3bb7..fc94c475420 100644 --- a/apps/user_ldap/tests/Group_LDAPTest.php +++ b/apps/user_ldap/tests/Group_LDAPTest.php @@ -1274,33 +1274,70 @@ class Group_LDAPTest extends TestCase { public function groupMemberProvider() { $base = 'dc=species,dc=earth'; - $groups0 = [ + $birdsDn = [ 'uid=3723,' . $base, 'uid=8372,' . $base, 'uid=8427,' . $base, 'uid=2333,' . $base, 'uid=4754,' . $base, ]; - $groups1 = [ + $birdsUid = [ '3723', '8372', '8427', '2333', '4754', ]; - $groups2Nested = ['6642', '1424']; - $expGroups2 = array_merge($groups1, $groups2Nested); + $animalsDn = [ + 'uid=lion,' . $base, + 'uid=tiger,' . $base, + ]; + $plantsDn = [ + 'uid=flower,' . $base, + 'uid=tree,' . $base, + ]; + $thingsDn = [ + 'uid=thing1,' . $base, + 'uid=thing2,' . $base, + ]; return [ [ #0 – test DNs 'cn=Birds,' . $base, - $groups0, - ['cn=Birds,' . $base => $groups0] + $birdsDn, + ['cn=Birds,' . $base => $birdsDn] ], [ #1 – test uids 'cn=Birds,' . $base, - $groups1, - ['cn=Birds,' . $base => $groups1] + $birdsUid, + ['cn=Birds,' . $base => $birdsUid] + ], + [ #2 – test simple nested group + 'cn=Animals,' . $base, + array_merge($birdsDn, $animalsDn), + [ + 'cn=Animals,' . $base => array_merge(['cn=Birds,' . $base], $animalsDn), + 'cn=Birds,' . $base => $birdsDn, + ] + ], + [ #3 – test recursive nested group + 'cn=Animals,' . $base, + // Because of complicated nesting the group gets returned as a member + array_merge(['cn=Birds,' . $base], $birdsDn, $animalsDn), + [ + 'cn=Animals,' . $base => array_merge(['cn=Birds,' . $base,'cn=Birds,' . $base,'cn=Animals,' . $base], $animalsDn), + 'cn=Birds,' . $base => array_merge(['cn=Animals,' . $base,'cn=Birds,' . $base], $birdsDn), + ] + ], + [ #4 – Complicated nested group + 'cn=Things,' . $base, + array_merge($birdsDn, $animalsDn, $thingsDn, $plantsDn), + [ + 'cn=Animals,' . $base => array_merge(['cn=Birds,' . $base], $animalsDn), + 'cn=Birds,' . $base => $birdsDn, + 'cn=Plants,' . $base => $plantsDn, + 'cn=Things,' . $base => array_merge(['cn=Animals,' . $base,'cn=Plants,' . $base], $thingsDn), + ] ], ]; } -- cgit v1.2.3 From 6ed0d0b8b1661deb1bb0fe57ec2ca612bb06a0f7 Mon Sep 17 00:00:00 2001 From: Côme Chilliet Date: Tue, 14 Dec 2021 10:17:08 +0100 Subject: Refactor group membership listing for nested groups 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 | 214 ++++++++++---------------------- apps/user_ldap/tests/Group_LDAPTest.php | 4 +- 2 files changed, 70 insertions(+), 148 deletions(-) (limited to 'apps') diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php index a7a22ff46d9..96f1898c647 100644 --- a/apps/user_ldap/lib/Group_LDAP.php +++ b/apps/user_ldap/lib/Group_LDAP.php @@ -262,7 +262,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I && $this->access->connection->ldapMatchingRuleInChainState !== Configuration::LDAP_SERVER_FEATURE_UNAVAILABLE ) { $attemptedLdapMatchingRuleInChain = true; - // compatibility hack with servers supporting :1.2.840.113556.1.4.1941:, and others) + // Use matching rule 1.2.840.113556.1.4.1941 if available (LDAP_MATCHING_RULE_IN_CHAIN) $filter = $this->access->combineFilterWithAnd([ $this->access->connection->ldapUserFilter, $this->access->connection->ldapUserDisplayName . '=*', @@ -334,93 +334,33 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I * @return string[] * @throws ServerNotAvailableException */ - private function _getGroupDNsFromMemberOf(string $dn): array { - $groups = $this->access->readAttribute($dn, 'memberOf'); - if (!is_array($groups)) { + private function _getGroupDNsFromMemberOf(string $dn, array &$seen = []): array { + if (isset($seen[$dn])) { return []; } + $seen[$dn] = true; - $fetcher = function (string $groupDN) { - if (isset($this->cachedNestedGroups[$groupDN])) { - $nestedGroups = $this->cachedNestedGroups[$groupDN]; - } else { - $nestedGroups = $this->access->readAttribute($groupDN, 'memberOf'); - if (!is_array($nestedGroups)) { - $nestedGroups = []; - } - $this->cachedNestedGroups[$groupDN] = $nestedGroups; - } - return $nestedGroups; - }; - - $groups = $this->walkNestedGroupsReturnDNs($dn, $fetcher, $groups); - return $this->filterValidGroups($groups); - } - - /** - * @psalm-param list}|string> $list - * @psalm-param array $seen List of DN that have already been processed. - * @psalm-param Closure(string) $fetcher - */ - private function processListFromWalkingNestedGroups(array &$list, array &$seen, string $dn, Closure $fetcher): void { - while ($record = array_shift($list)) { - $recordDN = $record['dn'][0] ?? $record; - if ($recordDN === $dn || array_key_exists($recordDN, $seen)) { - // Prevent loops - continue; - } - - $cacheKey = 'walkNestedGroups_' . $recordDN; - $fetched = $this->access->connection->getFromCache($cacheKey); - if ($fetched === null) { - $fetched = $fetcher($recordDN); - $this->access->connection->writeToCache($cacheKey, $fetched); - } - $list = array_merge($list, $fetched); - if (!isset($seen[$recordDN]) || is_bool($seen[$recordDN]) && is_array($record)) { - $seen[$recordDN] = $record; - } - } - } - - /** - * @psalm-param list}|string> $list - * @psalm-param array $seen List of DN that have already been processed. - * @psalm-param Closure(string) $fetcher - */ - private function walkNestedGroupsReturnDNs(string $dn, Closure $fetcher, array $list, array &$seen = []): array { - $nesting = (int)$this->access->connection->ldapNestedGroups; - - if ($nesting !== 1) { - return $list; + if (isset($this->cachedNestedGroups[$dn])) { + return $this->cachedNestedGroups[$dn]; } - $this->processListFromWalkingNestedGroups($list, $seen, $dn, $fetcher); - return array_keys($seen); - } - - /** - * @psalm-param list}> $list - * @psalm-param array $seen List of DN that have already been processed. - * @psalm-param Closure(string) $fetcher - * @return array[] An array of records - */ - private function walkNestedGroupsReturnRecords(string $dn, Closure $fetcher, array $list, array &$seen = []): array { - $nesting = (int)$this->access->connection->ldapNestedGroups; - - if ($nesting !== 1) { - // the keys are numeric, but should hold the DN - return array_reduce($list, function (array $transformed, array $record) use ($dn) { - if ($record['dn'][0] != $dn) { - $transformed[$record['dn'][0]] = $record; + $allGroups = []; + $groups = $this->access->readAttribute($dn, 'memberOf'); + if (is_array($groups)) { + if ((int)$this->access->connection->ldapNestedGroups === 1) { + while ($recordDn = array_shift($groups)) { + $nestedParents = $this->_getGroupDNsFromMemberOf($recordDn, $seen); + $groups = array_merge($groups, $nestedParents); + $allGroups[] = $recordDn; } - return $transformed; - }, []); + } else { + $allGroups = $groups; + } } - $this->processListFromWalkingNestedGroups($list, $seen, $dn, $fetcher); - // filter out intermediate state - return array_filter($seen, 'is_array'); + // We do not perform array_unique here at it is done in getUserGroups later + $this->cachedNestedGroups[$dn] = $allGroups; + return $this->filterValidGroups($allGroups); } /** @@ -716,7 +656,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I * This function includes groups based on dynamic group membership. * * @param string $uid Name of the user - * @return array with group names + * @return string[] Group names * @throws Exception * @throws ServerNotAvailableException */ @@ -746,7 +686,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I $groupsToMatch = $this->access->fetchListOfGroups( $this->access->connection->ldapGroupFilter, ['dn', $dynamicGroupMemberURL]); foreach ($groupsToMatch as $dynamicGroup) { - if (!array_key_exists($dynamicGroupMemberURL, $dynamicGroup)) { + if (!isset($dynamicGroup[$dynamicGroupMemberURL][0])) { continue; } $pos = strpos($dynamicGroup[$dynamicGroupMemberURL][0], '('); @@ -795,54 +735,40 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I $groups[] = $groupName; } } + } else { + // uniqueMember takes DN, memberuid the uid, so we need to distinguish + switch ($this->ldapGroupMemberAssocAttr) { + case 'uniquemember': + case 'member': + $uid = $userDN; + break; - if ($primaryGroup !== false) { - $groups[] = $primaryGroup; - } - if ($gidGroupName !== false) { - $groups[] = $gidGroupName; - } - $this->access->connection->writeToCache($cacheKey, $groups); - return $groups; - } - - //uniqueMember takes DN, memberuid the uid, so we need to distinguish - switch ($this->ldapGroupMemberAssocAttr) { - case 'uniquemember': - case 'member': - $uid = $userDN; - break; - - case 'memberuid': - case 'zimbramailforwardingaddress': - $result = $this->access->readAttribute($userDN, 'uid'); - if ($result === false) { - $this->logger->debug('No uid attribute found for DN {dn} on {host}', - [ - 'app' => 'user_ldap', - 'dn' => $userDN, - 'host' => $this->access->connection->ldapHost, - ] - ); - $uid = false; - } else { - $uid = $result[0]; - } - break; + case 'memberuid': + case 'zimbramailforwardingaddress': + $result = $this->access->readAttribute($userDN, 'uid'); + if ($result === false) { + $this->logger->debug('No uid attribute found for DN {dn} on {host}', + [ + 'app' => 'user_ldap', + 'dn' => $userDN, + 'host' => $this->access->connection->ldapHost, + ] + ); + $uid = false; + } else { + $uid = $result[0]; + } + break; - default: - // just in case - $uid = $userDN; - break; - } + default: + // just in case + $uid = $userDN; + break; + } - if ($uid !== false) { - if (isset($this->cachedGroupsByMember[$uid])) { - $groups = array_merge($groups, $this->cachedGroupsByMember[$uid]); - } else { + if ($uid !== false) { $groupsByMember = array_values($this->getGroupsByMember($uid)); $groupsByMember = $this->access->nextcloudGroupNames($groupsByMember); - $this->cachedGroupsByMember[$uid] = $groupsByMember; $groups = array_merge($groups, $groupsByMember); } } @@ -863,19 +789,16 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I /** * @throws ServerNotAvailableException */ - private function getGroupsByMember(string $dn, array &$seen = null): array { - if ($seen === null) { - $seen = []; - } - if (array_key_exists($dn, $seen)) { - // avoid loops + private function getGroupsByMember(string $dn, array &$seen = []): array { + if (isset($seen[$dn])) { return []; } + $seen[$dn] = true; + if ($this->cachedGroupsByMember[$dn]) { return $this->cachedGroupsByMember[$dn]; } - $allGroups = []; - $seen[$dn] = true; + $filter = $this->access->connection->ldapGroupMemberAssocAttr . '=' . $dn; if ($this->ldapGroupMemberAssocAttr === 'zimbramailforwardingaddress') { @@ -888,27 +811,24 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I $filter = $this->access->combineFilterWithAnd([$filter, $this->access->connection->ldapGroupFilter]); } + $allGroups = []; $groups = $this->access->fetchListOfGroups($filter, [strtolower($this->access->connection->ldapGroupMemberAssocAttr), $this->access->connection->ldapGroupDisplayName, 'dn']); - $fetcher = function (string $dn) use (&$seen) { - return $this->getGroupsByMember($dn, $seen); - }; - if (empty($dn)) { - $dn = ""; + if ($nesting === 1) { + while ($record = array_shift($groups)) { + // Note: this has no effect when ldapGroupMemberAssocAttr is uid based + $nestedParents = $this->getGroupsByMember($record['dn'][0], $seen); + $groups = array_merge($groups, $nestedParents); + $allGroups[] = $record; } - return $this->getGroupsByMember($dn, $seen); - }; - - if (empty($dn)) { - $dn = ""; + } else { + $allGroups = $groups; } - $allGroups = $this->walkNestedGroupsReturnRecords($dn, $fetcher, $groups, $seen); $visibleGroups = $this->filterValidGroups($allGroups); - $effectiveGroups = array_intersect_key($allGroups, $visibleGroups); - $this->cachedGroupsByMember[$dn] = $effectiveGroups; - return $effectiveGroups; + $this->cachedGroupsByMember[$dn] = $visibleGroups; + return $visibleGroups; } /** diff --git a/apps/user_ldap/tests/Group_LDAPTest.php b/apps/user_ldap/tests/Group_LDAPTest.php index fc94c475420..d44007a078c 100644 --- a/apps/user_ldap/tests/Group_LDAPTest.php +++ b/apps/user_ldap/tests/Group_LDAPTest.php @@ -955,6 +955,8 @@ class Group_LDAPTest extends TestCase { return $groupFilter; case 'ldapBaseGroups': return []; + case 'ldapGroupDisplayName': + return 'cn'; } return 1; }); @@ -986,7 +988,7 @@ class Group_LDAPTest extends TestCase { 'dn' => ['cn=group2,ou=groups,dc=domain,dc=com'], ]; - $access->expects($this->once()) + $access->expects($this->any()) ->method('nextcloudGroupNames') ->with([$group1, $group2]) ->willReturn(['group1', 'group2']); -- cgit v1.2.3 From 7437673addfe8025072b9bb8e5f2b15a406bf1e9 Mon Sep 17 00:00:00 2001 From: Côme Chilliet Date: Tue, 14 Dec 2021 10:51:39 +0100 Subject: Add testing of nested group membership MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- apps/user_ldap/tests/Group_LDAPTest.php | 42 ++++++++++++++++++++++++--------- 1 file changed, 31 insertions(+), 11 deletions(-) (limited to 'apps') diff --git a/apps/user_ldap/tests/Group_LDAPTest.php b/apps/user_ldap/tests/Group_LDAPTest.php index d44007a078c..6a4960351e2 100644 --- a/apps/user_ldap/tests/Group_LDAPTest.php +++ b/apps/user_ldap/tests/Group_LDAPTest.php @@ -948,7 +948,7 @@ class Group_LDAPTest extends TestCase { case 'ldapDynamicGroupMemberURL': return ''; case 'ldapNestedGroups': - return $nestedGroups; + return (int)$nestedGroups; case 'ldapGroupMemberAssocAttr': return 'member'; case 'ldapGroupFilter': @@ -982,30 +982,47 @@ class Group_LDAPTest extends TestCase { $group1 = [ 'cn' => 'group1', 'dn' => ['cn=group1,ou=groups,dc=domain,dc=com'], + 'member' => [$dn], ]; $group2 = [ 'cn' => 'group2', 'dn' => ['cn=group2,ou=groups,dc=domain,dc=com'], + 'member' => [$dn], ]; + $group3 = [ + 'cn' => 'group3', + 'dn' => ['cn=group3,ou=groups,dc=domain,dc=com'], + 'member' => [$group2['dn'][0]], + ]; + + $expectedGroups = ($nestedGroups ? [$group1, $group2, $group3] : [$group1, $group2]); + $expectedGroupsNames = ($nestedGroups ? ['group1', 'group2', 'group3'] : ['group1', 'group2']); $access->expects($this->any()) ->method('nextcloudGroupNames') - ->with([$group1, $group2]) - ->willReturn(['group1', 'group2']); + ->with($expectedGroups) + ->willReturn($expectedGroupsNames); $access->expects($nestedGroups ? $this->atLeastOnce() : $this->once()) ->method('fetchListOfGroups') - ->willReturnCallback(function ($filter, $attr, $limit, $offset) use ($nestedGroups, $groupFilter, $group1, $group2) { + ->willReturnCallback(function ($filter, $attr, $limit, $offset) use ($nestedGroups, $groupFilter, $group1, $group2, $group3, $dn) { static $firstRun = true; if (!$nestedGroups) { // When nested groups are enabled, groups cannot be filtered early as it would // exclude intermediate groups. But we can, and should, when working with flat groups. $this->assertTrue(strpos($filter, $groupFilter) !== false); } - if ($firstRun) { - $firstRun = false; - return [$group1, $group2]; + [$memberFilter] = explode('&', $filter); + if ($memberFilter === 'member='.$dn) { + if ($firstRun) { + $firstRun = false; + return [$group1, $group2]; + } + return []; + } elseif ($memberFilter === 'member='.$group2['dn'][0]) { + return [$group3]; + } else { + return []; } - return []; }); $access->expects($this->any()) ->method('dn2groupname') @@ -1014,13 +1031,16 @@ class Group_LDAPTest extends TestCase { }); $access->expects($this->any()) ->method('groupname2dn') - ->willReturnCallback(function (string $gid) use ($group1, $group2) { + ->willReturnCallback(function (string $gid) use ($group1, $group2, $group3) { if ($gid === $group1['cn']) { return $group1['dn'][0]; } if ($gid === $group2['cn']) { return $group2['dn'][0]; } + if ($gid === $group3['cn']) { + return $group3['dn'][0]; + } }); $access->expects($this->any()) ->method('isDNPartOfBase') @@ -1028,10 +1048,10 @@ class Group_LDAPTest extends TestCase { $groupBackend = new GroupLDAP($access, $pluginManager); $groups = $groupBackend->getUserGroups('userX'); - $this->assertEquals(['group1', 'group2'], $groups); + $this->assertEquals($expectedGroupsNames, $groups); $groupsAgain = $groupBackend->getUserGroups('userX'); - $this->assertEquals(['group1', 'group2'], $groupsAgain); + $this->assertEquals($expectedGroupsNames, $groupsAgain); } public function testCreateGroupWithPlugin() { -- cgit v1.2.3 From 150e6adbc55df2d3003a82fca5fd3fc92bb16152 Mon Sep 17 00:00:00 2001 From: Côme Chilliet Date: Tue, 14 Dec 2021 11:13:46 +0100 Subject: Fix types in docblocks 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 | 1 + 1 file changed, 1 insertion(+) (limited to 'apps') diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php index 96f1898c647..950f8bd3e4f 100644 --- a/apps/user_ldap/lib/Group_LDAP.php +++ b/apps/user_ldap/lib/Group_LDAP.php @@ -787,6 +787,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I } /** + * @return array[] * @throws ServerNotAvailableException */ private function getGroupsByMember(string $dn, array &$seen = []): array { -- cgit v1.2.3 From 69f9e9f387d217692f7ee039a76183c3db3feb8d Mon Sep 17 00:00:00 2001 From: Côme Chilliet Date: Tue, 14 Dec 2021 13:51:18 +0100 Subject: Removed unused use declaration 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 | 1 - 1 file changed, 1 deletion(-) (limited to 'apps') diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php index 950f8bd3e4f..abe306974d8 100644 --- a/apps/user_ldap/lib/Group_LDAP.php +++ b/apps/user_ldap/lib/Group_LDAP.php @@ -43,7 +43,6 @@ */ namespace OCA\User_LDAP; -use Closure; use Exception; use OC; use OCP\Cache\CappedMemoryCache; -- cgit v1.2.3 From 604b5ace12844ca3da80cd68e3942d789d833e3c Mon Sep 17 00:00:00 2001 From: Côme Chilliet Date: Tue, 14 Dec 2021 14:30:50 +0100 Subject: Add missing copyright author in Group_LDAP 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 | 1 + 1 file changed, 1 insertion(+) (limited to 'apps') diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php index abe306974d8..70cc7a0107a 100644 --- a/apps/user_ldap/lib/Group_LDAP.php +++ b/apps/user_ldap/lib/Group_LDAP.php @@ -25,6 +25,7 @@ * @author Vinicius Cubas Brand * @author Xuanwo * @author Carl Schwan + * @author Côme Chilliet * * @license AGPL-3.0 * -- cgit v1.2.3 From 33be3f754a00d30021ede8a92aae15599b832f4a Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Mon, 18 Jul 2022 09:58:30 +0200 Subject: Only cache base inGroup search And not intermediate search for nested groups, this is causing issues othewise with nested groups Signed-off-by: Carl Schwan --- apps/user_ldap/lib/Group_LDAP.php | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) (limited to 'apps') diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php index 70cc7a0107a..272ceea5865 100644 --- a/apps/user_ldap/lib/Group_LDAP.php +++ b/apps/user_ldap/lib/Group_LDAP.php @@ -45,16 +45,15 @@ namespace OCA\User_LDAP; use Exception; -use OC; -use OCP\Cache\CappedMemoryCache; use OC\ServerNotAvailableException; +use OCP\Cache\CappedMemoryCache; use OCP\Group\Backend\IGetDisplayNameBackend; use OCP\Group\Backend\IDeleteGroupBackend; use OCP\GroupInterface; use Psr\Log\LoggerInterface; class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, IGetDisplayNameBackend, IDeleteGroupBackend { - protected $enabled = false; + protected bool $enabled = false; /** @var CappedMemoryCache $cachedGroupMembers array of users with gid as key */ protected CappedMemoryCache $cachedGroupMembers; @@ -62,7 +61,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I protected CappedMemoryCache $cachedGroupsByMember; /** @var CappedMemoryCache $cachedNestedGroups array of groups with gid (DN) as key */ protected CappedMemoryCache $cachedNestedGroups; - protected GroupInterface $groupPluginManager; + protected GroupPluginManager $groupPluginManager; protected LoggerInterface $logger; /** @@ -82,7 +81,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I $this->cachedGroupsByMember = new CappedMemoryCache(); $this->cachedNestedGroups = new CappedMemoryCache(); $this->groupPluginManager = $groupPluginManager; - $this->logger = OC::$server->get(LoggerInterface::class); + $this->logger = \OCP\Server::get(LoggerInterface::class); $this->ldapGroupMemberAssocAttr = strtolower((string)$gAssoc); } @@ -91,11 +90,10 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I * * @param string $uid uid of the user * @param string $gid gid of the group - * @return bool * @throws Exception * @throws ServerNotAvailableException */ - public function inGroup($uid, $gid) { + public function inGroup($uid, $gid): bool { if (!$this->enabled) { return false; } @@ -248,6 +246,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I return []; } $seen[$dnGroup] = true; + $shouldCacheResult = count($seen) === 0; // used extensively in cron job, caching makes sense for nested groups $cacheKey = '_groupMembers' . $dnGroup; @@ -317,7 +316,9 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I unset($allMembers[$index]); } - $this->access->connection->writeToCache($cacheKey, $allMembers); + if ($shouldCacheResult) { + $this->access->connection->writeToCache($cacheKey, $allMembers); + } if (isset($attemptedLdapMatchingRuleInChain) && $this->access->connection->ldapMatchingRuleInChainState === Configuration::LDAP_SERVER_FEATURE_UNKNOWN @@ -767,6 +768,12 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I } if ($uid !== false) { + // Clear cache between invocation of getGroupsByMember + // getGroupsByMember is a recursive method and the results stored in + // the cache depends on the already seen groups. This breaks when we + // have circular groups + $this->cachedGroupsByMember = new CappedMemoryCache(); + $groupsByMember = array_values($this->getGroupsByMember($uid)); $groupsByMember = $this->access->nextcloudGroupNames($groupsByMember); $groups = array_merge($groups, $groupsByMember); -- cgit v1.2.3 From e0fbd3984003164738ffc9d727314c718867b59e Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Mon, 18 Jul 2022 10:06:41 +0200 Subject: Add back runtime cache for intermediate ldap read results This is a small optimization that save a few LDAP queries Signed-off-by: Carl Schwan --- apps/user_ldap/lib/Group_LDAP.php | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) (limited to 'apps') diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php index 272ceea5865..5bec38e30ee 100644 --- a/apps/user_ldap/lib/Group_LDAP.php +++ b/apps/user_ldap/lib/Group_LDAP.php @@ -245,8 +245,8 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I if (isset($seen[$dnGroup])) { return []; } - $seen[$dnGroup] = true; $shouldCacheResult = count($seen) === 0; + $seen[$dnGroup] = true; // used extensively in cron job, caching makes sense for nested groups $cacheKey = '_groupMembers' . $dnGroup; @@ -288,7 +288,15 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I } $allMembers = []; - $members = $this->access->readAttribute($dnGroup, $this->access->connection->ldapGroupMemberAssocAttr); + + /** @psalm-var array $rawMemberReads */ + static $rawMemberReads = []; // runtime cache for intermediate ldap read results + $members = $rawMemberReads[$dnGroup] ?? null; + if ($members === null) { + $members = $this->access->readAttribute($dnGroup, $this->access->connection->ldapGroupMemberAssocAttr); + $rawMemberReads[$dnGroup] = $members; + } + if (is_array($members)) { if ((int)$this->access->connection->ldapNestedGroups === 1) { while ($recordDn = array_shift($members)) { -- cgit v1.2.3 From 1b12a08ec24ad4153642841c3ed9d67dc13cdc5c Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Thu, 21 Jul 2022 17:24:22 +0200 Subject: Fix user_ldap tests Signed-off-by: Carl Schwan --- apps/user_ldap/lib/Group_LDAP.php | 18 +++++++++--------- apps/user_ldap/lib/Mapping/AbstractMapping.php | 10 +++++----- apps/user_ldap/tests/Group_LDAPTest.php | 11 +++-------- 3 files changed, 17 insertions(+), 22 deletions(-) (limited to 'apps') diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php index 5bec38e30ee..2fe68318c87 100644 --- a/apps/user_ldap/lib/Group_LDAP.php +++ b/apps/user_ldap/lib/Group_LDAP.php @@ -64,6 +64,9 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I protected GroupPluginManager $groupPluginManager; protected LoggerInterface $logger; + /** @psalm-var array $rawMemberReads */ + protected $rawMemberReads = []; // runtime cache for intermediate ldap read results + /** * @var string $ldapGroupMemberAssocAttr contains the LDAP setting (in lower case) with the same name */ @@ -289,12 +292,10 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I $allMembers = []; - /** @psalm-var array $rawMemberReads */ - static $rawMemberReads = []; // runtime cache for intermediate ldap read results - $members = $rawMemberReads[$dnGroup] ?? null; + $members = $this->rawMemberReads[$dnGroup] ?? null; if ($members === null) { $members = $this->access->readAttribute($dnGroup, $this->access->connection->ldapGroupMemberAssocAttr); - $rawMemberReads[$dnGroup] = $members; + $this->rawMemberReads[$dnGroup] = $members; } if (is_array($members)) { @@ -780,8 +781,6 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I // getGroupsByMember is a recursive method and the results stored in // the cache depends on the already seen groups. This breaks when we // have circular groups - $this->cachedGroupsByMember = new CappedMemoryCache(); - $groupsByMember = array_values($this->getGroupsByMember($uid)); $groupsByMember = $this->access->nextcloudGroupNames($groupsByMember); $groups = array_merge($groups, $groupsByMember); @@ -809,10 +808,11 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I if (isset($seen[$dn])) { return []; } + $cacheKey = $dn . implode('-', array_keys($seen)); $seen[$dn] = true; - if ($this->cachedGroupsByMember[$dn]) { - return $this->cachedGroupsByMember[$dn]; + if ($this->cachedGroupsByMember[$cacheKey]) { + return $this->cachedGroupsByMember[$cacheKey]; } $filter = $this->access->connection->ldapGroupMemberAssocAttr . '=' . $dn; @@ -843,7 +843,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I } $visibleGroups = $this->filterValidGroups($allGroups); - $this->cachedGroupsByMember[$dn] = $visibleGroups; + $this->cachedGroupsByMember[$cacheKey] = $visibleGroups; return $visibleGroups; } diff --git a/apps/user_ldap/lib/Mapping/AbstractMapping.php b/apps/user_ldap/lib/Mapping/AbstractMapping.php index 9026b8cfb78..8fbad6aae3a 100644 --- a/apps/user_ldap/lib/Mapping/AbstractMapping.php +++ b/apps/user_ldap/lib/Mapping/AbstractMapping.php @@ -27,9 +27,9 @@ namespace OCA\User_LDAP\Mapping; use Doctrine\DBAL\Exception; -use OC\DB\QueryBuilder\QueryBuilder; use OCP\DB\IPreparedStatement; use OCP\DB\QueryBuilder\IQueryBuilder; +use Doctrine\DBAL\Platforms\SqlitePlatform; /** * Class AbstractMapping @@ -219,12 +219,12 @@ abstract class AbstractMapping { $qb = $this->dbc->getQueryBuilder(); $qb->select('owncloud_name', 'ldap_dn_hash', 'ldap_dn') ->from($this->getTableName(false)) - ->where($qb->expr()->in('ldap_dn_hash', $qb->createNamedParameter($hashList, QueryBuilder::PARAM_STR_ARRAY))); + ->where($qb->expr()->in('ldap_dn_hash', $qb->createNamedParameter($hashList, IQueryBuilder::PARAM_STR_ARRAY))); return $qb; } protected function collectResultsFromListOfIdsQuery(IQueryBuilder $qb, array &$results): void { - $stmt = $qb->execute(); + $stmt = $qb->executeQuery(); while ($entry = $stmt->fetch(\Doctrine\DBAL\FetchMode::ASSOCIATIVE)) { $results[$entry['ldap_dn']] = $entry['owncloud_name']; $this->cache[$entry['ldap_dn']] = $entry['owncloud_name']; @@ -239,7 +239,7 @@ abstract class AbstractMapping { public function getListOfIdsByDn(array $fdns): array { $totalDBParamLimit = 65000; $sliceSize = 1000; - $maxSlices = $totalDBParamLimit / $sliceSize; + $maxSlices = $this->dbc->getDatabasePlatform() instanceof SqlitePlatform ? 9 : $totalDBParamLimit / $sliceSize; $results = []; $slice = 1; @@ -261,7 +261,7 @@ abstract class AbstractMapping { } if (!empty($fdnsSlice)) { - $qb->orWhere($qb->expr()->in('ldap_dn_hash', $qb->createNamedParameter($fdnsSlice, QueryBuilder::PARAM_STR_ARRAY))); + $qb->orWhere($qb->expr()->in('ldap_dn_hash', $qb->createNamedParameter($fdnsSlice, IQueryBuilder::PARAM_STR_ARRAY))); } if ($slice % $maxSlices === 0) { diff --git a/apps/user_ldap/tests/Group_LDAPTest.php b/apps/user_ldap/tests/Group_LDAPTest.php index 6a4960351e2..d9bfe81e63c 100644 --- a/apps/user_ldap/tests/Group_LDAPTest.php +++ b/apps/user_ldap/tests/Group_LDAPTest.php @@ -941,7 +941,7 @@ class Group_LDAPTest extends TestCase { $access->connection = $this->createMock(Connection::class); $access->connection->expects($this->any()) ->method('__get') - ->willReturnCallback(function ($name) use ($nestedGroups, $groupFilter) { + ->willReturnCallback(function (string $name) use ($nestedGroups, $groupFilter) { switch ($name) { case 'useMemberOfToDetectMembership': return 0; @@ -1013,10 +1013,7 @@ class Group_LDAPTest extends TestCase { } [$memberFilter] = explode('&', $filter); if ($memberFilter === 'member='.$dn) { - if ($firstRun) { - $firstRun = false; return [$group1, $group2]; - } return []; } elseif ($memberFilter === 'member='.$group2['dn'][0]) { return [$group3]; @@ -1365,12 +1362,10 @@ class Group_LDAPTest extends TestCase { } /** - * @param string $groupDN * @param string[] $expectedMembers - * @param array $groupsInfo * @dataProvider groupMemberProvider */ - public function testGroupMembers($groupDN, $expectedMembers, $groupsInfo = null) { + public function testGroupMembers(string $groupDN, array $expectedMembers, $groupsInfo = null) { $access = $this->getAccessMock(); $access->expects($this->any()) ->method('readAttribute') @@ -1384,7 +1379,7 @@ class Group_LDAPTest extends TestCase { $access->connection = $this->createMock(Connection::class); $access->connection->expects($this->any()) ->method('__get') - ->willReturnCallback(function ($name) { + ->willReturnCallback(function (string $name) { if ($name === 'ldapNestedGroups') { return 1; } elseif ($name === 'ldapGroupMemberAssocAttr') { -- cgit v1.2.3 From be5338e57264b95a6e444a5ea16b07ef6553387d Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Thu, 21 Jul 2022 17:37:30 +0200 Subject: Revert Carl changes on apps/user_ldap/lib/Group_LDAP.php Signed-off-by: Carl Schwan --- apps/user_ldap/lib/Group_LDAP.php | 39 ++++++++++++--------------------------- 1 file changed, 12 insertions(+), 27 deletions(-) (limited to 'apps') diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php index 2fe68318c87..70cc7a0107a 100644 --- a/apps/user_ldap/lib/Group_LDAP.php +++ b/apps/user_ldap/lib/Group_LDAP.php @@ -45,15 +45,16 @@ namespace OCA\User_LDAP; use Exception; -use OC\ServerNotAvailableException; +use OC; use OCP\Cache\CappedMemoryCache; +use OC\ServerNotAvailableException; use OCP\Group\Backend\IGetDisplayNameBackend; use OCP\Group\Backend\IDeleteGroupBackend; use OCP\GroupInterface; use Psr\Log\LoggerInterface; class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, IGetDisplayNameBackend, IDeleteGroupBackend { - protected bool $enabled = false; + protected $enabled = false; /** @var CappedMemoryCache $cachedGroupMembers array of users with gid as key */ protected CappedMemoryCache $cachedGroupMembers; @@ -61,12 +62,9 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I protected CappedMemoryCache $cachedGroupsByMember; /** @var CappedMemoryCache $cachedNestedGroups array of groups with gid (DN) as key */ protected CappedMemoryCache $cachedNestedGroups; - protected GroupPluginManager $groupPluginManager; + protected GroupInterface $groupPluginManager; protected LoggerInterface $logger; - /** @psalm-var array $rawMemberReads */ - protected $rawMemberReads = []; // runtime cache for intermediate ldap read results - /** * @var string $ldapGroupMemberAssocAttr contains the LDAP setting (in lower case) with the same name */ @@ -84,7 +82,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I $this->cachedGroupsByMember = new CappedMemoryCache(); $this->cachedNestedGroups = new CappedMemoryCache(); $this->groupPluginManager = $groupPluginManager; - $this->logger = \OCP\Server::get(LoggerInterface::class); + $this->logger = OC::$server->get(LoggerInterface::class); $this->ldapGroupMemberAssocAttr = strtolower((string)$gAssoc); } @@ -93,10 +91,11 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I * * @param string $uid uid of the user * @param string $gid gid of the group + * @return bool * @throws Exception * @throws ServerNotAvailableException */ - public function inGroup($uid, $gid): bool { + public function inGroup($uid, $gid) { if (!$this->enabled) { return false; } @@ -248,7 +247,6 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I if (isset($seen[$dnGroup])) { return []; } - $shouldCacheResult = count($seen) === 0; $seen[$dnGroup] = true; // used extensively in cron job, caching makes sense for nested groups @@ -291,13 +289,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I } $allMembers = []; - - $members = $this->rawMemberReads[$dnGroup] ?? null; - if ($members === null) { - $members = $this->access->readAttribute($dnGroup, $this->access->connection->ldapGroupMemberAssocAttr); - $this->rawMemberReads[$dnGroup] = $members; - } - + $members = $this->access->readAttribute($dnGroup, $this->access->connection->ldapGroupMemberAssocAttr); if (is_array($members)) { if ((int)$this->access->connection->ldapNestedGroups === 1) { while ($recordDn = array_shift($members)) { @@ -325,9 +317,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I unset($allMembers[$index]); } - if ($shouldCacheResult) { - $this->access->connection->writeToCache($cacheKey, $allMembers); - } + $this->access->connection->writeToCache($cacheKey, $allMembers); if (isset($attemptedLdapMatchingRuleInChain) && $this->access->connection->ldapMatchingRuleInChainState === Configuration::LDAP_SERVER_FEATURE_UNKNOWN @@ -777,10 +767,6 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I } if ($uid !== false) { - // Clear cache between invocation of getGroupsByMember - // getGroupsByMember is a recursive method and the results stored in - // the cache depends on the already seen groups. This breaks when we - // have circular groups $groupsByMember = array_values($this->getGroupsByMember($uid)); $groupsByMember = $this->access->nextcloudGroupNames($groupsByMember); $groups = array_merge($groups, $groupsByMember); @@ -808,11 +794,10 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I if (isset($seen[$dn])) { return []; } - $cacheKey = $dn . implode('-', array_keys($seen)); $seen[$dn] = true; - if ($this->cachedGroupsByMember[$cacheKey]) { - return $this->cachedGroupsByMember[$cacheKey]; + if ($this->cachedGroupsByMember[$dn]) { + return $this->cachedGroupsByMember[$dn]; } $filter = $this->access->connection->ldapGroupMemberAssocAttr . '=' . $dn; @@ -843,7 +828,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I } $visibleGroups = $this->filterValidGroups($allGroups); - $this->cachedGroupsByMember[$cacheKey] = $visibleGroups; + $this->cachedGroupsByMember[$dn] = $visibleGroups; return $visibleGroups; } -- cgit v1.2.3 From 746a5fb7e07c806af7f0e9b0ebb3f72d823452a8 Mon Sep 17 00:00:00 2001 From: Côme Chilliet Date: Tue, 26 Jul 2022 09:39:48 +0200 Subject: Fix LDAP recursive nested group support 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 | 11 +++++++---- apps/user_ldap/tests/Group_LDAPTest.php | 29 ++++++++++++++--------------- 2 files changed, 21 insertions(+), 19 deletions(-) (limited to 'apps') diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php index 70cc7a0107a..d5d715d1b51 100644 --- a/apps/user_ldap/lib/Group_LDAP.php +++ b/apps/user_ldap/lib/Group_LDAP.php @@ -62,7 +62,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I protected CappedMemoryCache $cachedGroupsByMember; /** @var CappedMemoryCache $cachedNestedGroups array of groups with gid (DN) as key */ protected CappedMemoryCache $cachedNestedGroups; - protected GroupInterface $groupPluginManager; + protected GroupPluginManager $groupPluginManager; protected LoggerInterface $logger; /** @@ -243,8 +243,9 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I * @psalm-param array $seen List of DN that have already been processed. * @throws ServerNotAvailableException */ - private function _groupMembers(string $dnGroup, array &$seen = []): array { + private function _groupMembers(string $dnGroup, array $seen = [], bool &$recursive = false): array { if (isset($seen[$dnGroup])) { + $recursive = true; return []; } $seen[$dnGroup] = true; @@ -293,7 +294,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I if (is_array($members)) { if ((int)$this->access->connection->ldapNestedGroups === 1) { while ($recordDn = array_shift($members)) { - $nestedMembers = $this->_groupMembers($recordDn, $seen); + $nestedMembers = $this->_groupMembers($recordDn, $seen, $recursive); if (!empty($nestedMembers)) { // Group, queue its members for processing $members = array_merge($members, $nestedMembers); @@ -317,7 +318,9 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I unset($allMembers[$index]); } - $this->access->connection->writeToCache($cacheKey, $allMembers); + if (!$recursive) { + $this->access->connection->writeToCache($cacheKey, $allMembers); + } if (isset($attemptedLdapMatchingRuleInChain) && $this->access->connection->ldapMatchingRuleInChainState === Configuration::LDAP_SERVER_FEATURE_UNKNOWN diff --git a/apps/user_ldap/tests/Group_LDAPTest.php b/apps/user_ldap/tests/Group_LDAPTest.php index d9bfe81e63c..edcb4be8819 100644 --- a/apps/user_ldap/tests/Group_LDAPTest.php +++ b/apps/user_ldap/tests/Group_LDAPTest.php @@ -1322,35 +1322,32 @@ class Group_LDAPTest extends TestCase { return [ [ #0 – test DNs - 'cn=Birds,' . $base, - $birdsDn, + ['cn=Birds,' . $base => $birdsDn], ['cn=Birds,' . $base => $birdsDn] ], [ #1 – test uids - 'cn=Birds,' . $base, - $birdsUid, + ['cn=Birds,' . $base => $birdsUid], ['cn=Birds,' . $base => $birdsUid] ], [ #2 – test simple nested group - 'cn=Animals,' . $base, - array_merge($birdsDn, $animalsDn), + ['cn=Animals,' . $base => array_merge($birdsDn, $animalsDn)], [ 'cn=Animals,' . $base => array_merge(['cn=Birds,' . $base], $animalsDn), 'cn=Birds,' . $base => $birdsDn, ] ], [ #3 – test recursive nested group - 'cn=Animals,' . $base, - // Because of complicated nesting the group gets returned as a member - array_merge(['cn=Birds,' . $base], $birdsDn, $animalsDn), + [ + 'cn=Animals,' . $base => array_merge($birdsDn, $animalsDn), + 'cn=Birds,' . $base => array_merge($birdsDn, $animalsDn), + ], [ 'cn=Animals,' . $base => array_merge(['cn=Birds,' . $base,'cn=Birds,' . $base,'cn=Animals,' . $base], $animalsDn), 'cn=Birds,' . $base => array_merge(['cn=Animals,' . $base,'cn=Birds,' . $base], $birdsDn), ] ], [ #4 – Complicated nested group - 'cn=Things,' . $base, - array_merge($birdsDn, $animalsDn, $thingsDn, $plantsDn), + ['cn=Things,' . $base => array_merge($birdsDn, $animalsDn, $thingsDn, $plantsDn)], [ 'cn=Animals,' . $base => array_merge(['cn=Birds,' . $base], $animalsDn), 'cn=Birds,' . $base => $birdsDn, @@ -1365,11 +1362,11 @@ class Group_LDAPTest extends TestCase { * @param string[] $expectedMembers * @dataProvider groupMemberProvider */ - public function testGroupMembers(string $groupDN, array $expectedMembers, $groupsInfo = null) { + public function testGroupMembers(array $expectedResult, array $groupsInfo = null) { $access = $this->getAccessMock(); $access->expects($this->any()) ->method('readAttribute') - ->willReturnCallback(function ($group) use ($groupDN, $expectedMembers, $groupsInfo) { + ->willReturnCallback(function ($group) use ($groupsInfo) { if (isset($groupsInfo[$group])) { return $groupsInfo[$group]; } @@ -1392,9 +1389,11 @@ class Group_LDAPTest extends TestCase { $pluginManager = $this->createMock(GroupPluginManager::class); $ldap = new GroupLDAP($access, $pluginManager); - $resultingMembers = $this->invokePrivate($ldap, '_groupMembers', [$groupDN]); + foreach ($expectedResult as $groupDN => $expectedMembers) { + $resultingMembers = $this->invokePrivate($ldap, '_groupMembers', [$groupDN]); - $this->assertEqualsCanonicalizing($expectedMembers, $resultingMembers); + $this->assertEqualsCanonicalizing($expectedMembers, $resultingMembers); + } } public function displayNameProvider() { -- cgit v1.2.3 From 1a6a6c985a014a0759135c9cc694cbf2d430b650 Mon Sep 17 00:00:00 2001 From: Côme Chilliet Date: Tue, 30 Aug 2022 16:46:00 +0200 Subject: Bring back small fixes by Carl 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 | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) (limited to 'apps') diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php index d5d715d1b51..d5a1cab30fe 100644 --- a/apps/user_ldap/lib/Group_LDAP.php +++ b/apps/user_ldap/lib/Group_LDAP.php @@ -45,16 +45,15 @@ namespace OCA\User_LDAP; use Exception; -use OC; use OCP\Cache\CappedMemoryCache; -use OC\ServerNotAvailableException; -use OCP\Group\Backend\IGetDisplayNameBackend; -use OCP\Group\Backend\IDeleteGroupBackend; use OCP\GroupInterface; +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 { - protected $enabled = false; + protected bool $enabled = false; /** @var CappedMemoryCache $cachedGroupMembers array of users with gid as key */ protected CappedMemoryCache $cachedGroupMembers; @@ -82,7 +81,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I $this->cachedGroupsByMember = new CappedMemoryCache(); $this->cachedNestedGroups = new CappedMemoryCache(); $this->groupPluginManager = $groupPluginManager; - $this->logger = OC::$server->get(LoggerInterface::class); + $this->logger = \OCP\Server::get(LoggerInterface::class); $this->ldapGroupMemberAssocAttr = strtolower((string)$gAssoc); } @@ -91,11 +90,10 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I * * @param string $uid uid of the user * @param string $gid gid of the group - * @return bool * @throws Exception * @throws ServerNotAvailableException */ - public function inGroup($uid, $gid) { + public function inGroup($uid, $gid): bool { if (!$this->enabled) { return false; } -- cgit v1.2.3 From 60ec5e655c3450c8083f37aeccfbc2876b2a7d1a Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Thu, 20 Oct 2022 11:30:46 +0200 Subject: Check if cache is present with isset Otherwise we get false for empty array Signed-off-by: Carl Schwan --- apps/user_ldap/lib/Group_LDAP.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'apps') diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php index d5a1cab30fe..723b3d856ad 100644 --- a/apps/user_ldap/lib/Group_LDAP.php +++ b/apps/user_ldap/lib/Group_LDAP.php @@ -797,7 +797,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I } $seen[$dn] = true; - if ($this->cachedGroupsByMember[$dn]) { + if (isset($this->cachedGroupsByMember[$dn])) { return $this->cachedGroupsByMember[$dn]; } -- cgit v1.2.3 From 99a752922f2241fd4c7477fcf00a68e3ead52461 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Thu, 20 Oct 2022 12:01:19 +0200 Subject: Fix psalm Signed-off-by: Carl Schwan --- apps/user_ldap/lib/Group_LDAP.php | 5 ++++- build/psalm-baseline.xml | 12 +----------- 2 files changed, 5 insertions(+), 12 deletions(-) (limited to 'apps') diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php index 723b3d856ad..81cb30dd25b 100644 --- a/apps/user_ldap/lib/Group_LDAP.php +++ b/apps/user_ldap/lib/Group_LDAP.php @@ -57,7 +57,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I /** @var CappedMemoryCache $cachedGroupMembers array of users with gid as key */ protected CappedMemoryCache $cachedGroupMembers; - /** @var CappedMemoryCache $cachedGroupsByMember array of groups with uid as key */ + /** @var CappedMemoryCache $cachedGroupsByMember array of groups with uid as key */ protected CappedMemoryCache $cachedGroupsByMember; /** @var CappedMemoryCache $cachedNestedGroups array of groups with gid (DN) as key */ protected CappedMemoryCache $cachedNestedGroups; @@ -1129,6 +1129,9 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I } /** + * @template T + * @param array $listOfGroups + * @return array * @throws ServerNotAvailableException * @throws Exception */ diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml index dc2b8a8f10a..e672dee4c0e 100644 --- a/build/psalm-baseline.xml +++ b/build/psalm-baseline.xml @@ -1628,20 +1628,10 @@ - - $groupName - - - bool - - + $groupID $groupID - - is_array($groupDNs) - is_array($list) - -- cgit v1.2.3