From da6705b5b2ac045ebcafb1502a97e2d4fbcdee76 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Fri, 10 Dec 2021 16:36:14 +0100 Subject: [PATCH 1/3] Add more type hinting Signed-off-by: Carl Schwan --- apps/user_ldap/lib/Group_LDAP.php | 58 ++++++++++++++++--------------- apps/user_ldap/lib/Wizard.php | 2 +- 2 files changed, 31 insertions(+), 29 deletions(-) diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php index 77294f9659d45..d01b2403f6d6c 100644 --- a/apps/user_ldap/lib/Group_LDAP.php +++ b/apps/user_ldap/lib/Group_LDAP.php @@ -88,7 +88,7 @@ public function __construct(Access $access, GroupPluginManager $groupPluginManag } /** - * 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 @@ public function getDynamicGroupMembers(string $dnGroup): array { } /** + * 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 avoind infinit loops, - // but not included in the results laters on + // 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 = []; @@ -331,6 +334,7 @@ private function _groupMembers(string $dnGroup, ?array &$seen = null): array { } /** + * @return string[] * @throws ServerNotAvailableException */ private function _getGroupDNsFromMemberOf(string $dn): array { @@ -357,7 +361,8 @@ private function _getGroupDNsFromMemberOf(string $dn): array { } /** - * @param list}|string> $list + * @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 { @@ -382,7 +387,8 @@ private function processListFromWalkingNestedGroups(array &$list, array &$seen, } /** - * @param list}|string> $list + * @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 { @@ -397,7 +403,9 @@ private function walkNestedGroupsReturnDNs(string $dn, Closure $fetcher, array $ } /** - * @param list}> $list + * @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 { @@ -419,9 +427,9 @@ private function walkNestedGroupsReturnRecords(string $dn, Closure $fetcher, arr } /** - * 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 */ @@ -442,10 +450,11 @@ public function gidNumber2Name(string $gid, string $dn) { } /** + * @return ?string The name of the group * @throws ServerNotAvailableException * @throws Exception */ - private function getNameOfGroup(string $filter, string $cacheKey) { + private function getNameOfGroup(string $filter, string $cacheKey): ?string { $result = $this->access->searchGroups($filter, ['dn'], 1); if (empty($result)) { $this->access->connection->writeToCache($cacheKey, false); @@ -464,9 +473,7 @@ private function getNameOfGroup(string $filter, string $cacheKey) { } /** - * returns the entry's gidNumber - * - * @return string|bool + * @return string|bool The entry's gidNumber * @throws ServerNotAvailableException */ private function getEntryGidNumber(string $dn, string $attribute) { @@ -478,7 +485,7 @@ private function getEntryGidNumber(string $dn, string $attribute) { } /** - * @return string|bool + * @return string|bool The group's gidNumber * @throws ServerNotAvailableException */ public function getGroupGidNumber(string $dn) { @@ -486,9 +493,7 @@ public function getGroupGidNumber(string $dn) { } /** - * returns the user's gidNumber - * - * @return string|bool + * @return string|bool The user's gidNumber * @throws ServerNotAvailableException */ public function getUserGidNumber(string $dn) { @@ -523,8 +528,7 @@ private function prepareFilterForUsersHasGidNumber(string $groupDN, string $sear } /** - * 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( @@ -551,7 +555,7 @@ public function getUsersInGidNumber( /** * @throws ServerNotAvailableException - * @return bool + * @return false|string */ public function getUserGroupByGid(string $dn) { $groupID = $this->getUserGidNumber($dn); @@ -566,9 +570,9 @@ public function getUserGroupByGid(string $dn) { } /** - * 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 */ @@ -593,9 +597,7 @@ public function primaryGroupID2Name(string $gid, string $dn) { } /** - * 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) { @@ -607,7 +609,7 @@ private function getEntryGroupID(string $dn, string $attribute) { } /** - * @return string|bool + * @return string|false The entry's primary group Id * @throws ServerNotAvailableException */ public function getGroupPrimaryGroupID(string $dn) { @@ -615,7 +617,7 @@ public function getGroupPrimaryGroupID(string $dn) { } /** - * @return string|bool + * @return string|false * @throws ServerNotAvailableException */ public function getUserPrimaryGroupIDs(string $dn) { @@ -695,7 +697,7 @@ public function countUsersInPrimaryGroup( } /** - * @return string|bool + * @return string|false * @throws ServerNotAvailableException */ public function getUserPrimaryGroup(string $dn) { @@ -949,7 +951,7 @@ public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0) { $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 []; } diff --git a/apps/user_ldap/lib/Wizard.php b/apps/user_ldap/lib/Wizard.php index 67a130555f2e9..90c9e9c4323fd 100644 --- a/apps/user_ldap/lib/Wizard.php +++ b/apps/user_ldap/lib/Wizard.php @@ -886,7 +886,7 @@ private function testMemberOf() { throw new \Exception('Could not connect to LDAP'); } $result = $this->access->countUsers('memberOf=*', ['memberOf'], 1); - if (is_int($result) && $result > 0) { + if (is_int($result) && $result > 0) { return true; } return false; From d82c655b00c49aa2dd7ff35a71ca964e8934acb8 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Fri, 10 Dec 2021 16:36:55 +0100 Subject: [PATCH 2/3] Fix detection of intermediates Currently the check if an object is in intermediates is done using group-{gid} for groups but only user- for users. This change the check for user to user-{uid}. This fix the listing of users in a group in the users settings. Signed-off-by: Carl Schwan --- apps/user_ldap/lib/Access.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/user_ldap/lib/Access.php b/apps/user_ldap/lib/Access.php index 972a7d4de66a2..59598274a21be 100644 --- a/apps/user_ldap/lib/Access.php +++ b/apps/user_ldap/lib/Access.php @@ -528,7 +528,7 @@ public function dn2username($fdn, $ldapName = null) { */ public function dn2ocname($fdn, $ldapName = null, $isUser = true, &$newlyMapped = null, array $record = null) { static $intermediates = []; - if (isset($intermediates[$isUser ? 'user-' : 'group-' . $fdn])) { + if (isset($intermediates[($isUser ? 'user-' : 'group-') . $fdn])) { return false; // is a known intermediate } @@ -567,7 +567,7 @@ public function dn2ocname($fdn, $ldapName = null, $isUser = true, &$newlyMapped $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; + $intermediates[($isUser ? 'user-' : 'group-') . $fdn] = true; return false; } $ldapName = $ldapName[0]; From 68fbe7bfd24195b474d2de77d4e7081e727ac7b4 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Fri, 10 Dec 2021 16:39:50 +0100 Subject: [PATCH 3/3] 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 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php index d01b2403f6d6c..8b21b9348821f 100644 --- a/apps/user_ldap/lib/Group_LDAP.php +++ b/apps/user_ldap/lib/Group_LDAP.php @@ -377,7 +377,7 @@ private function processListFromWalkingNestedGroups(array &$list, array &$seen, $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)) {