Skip to content

Fix some ldap related issues #30201

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.

Sign up for GitHub

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

Closed
CarlSchwan wants to merge 3 commits into from
Closed

Fix some ldap related issues #30201

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
da6705b
Add more type hinting
CarlSchwan Dec 10, 2021
d82c655
Fix detection of intermediates
CarlSchwan Dec 10, 2021
68fbe7b
Fix merging list with null
CarlSchwan Dec 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions apps/user_ldap/lib/Access.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down Expand Up @@ -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];
Expand Down
60 changes: 31 additions & 29 deletions apps/user_ldap/lib/Group_LDAP.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -240,18 +240,21 @@ public function getDynamicGroupMembers(string $dnGroup): array {
}

/**
* Get group members from dn.
* @psalm-param array<string, int|array|string> $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<string, string[]|bool> $rawMemberReads */
static $rawMemberReads = []; // runtime cache for intermediate ldap read results
$allMembers = [];

Expand Down Expand Up @@ -331,6 +334,7 @@ private function _groupMembers(string $dnGroup, ?array &$seen = null): array {
}

/**
* @return string[]
* @throws ServerNotAvailableException
*/
private function _getGroupDNsFromMemberOf(string $dn): array {
Expand All @@ -357,7 +361,8 @@ private function _getGroupDNsFromMemberOf(string $dn): array {
}

/**
* @param list<array{dn: list<string>}|string> $list
* @psalm-param list<array{dn: list<string>}|string> $list
* @psalm-param array<string, int|array|string> $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 {
Expand All @@ -372,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)) {
Expand All @@ -382,7 +387,8 @@ private function processListFromWalkingNestedGroups(array &$list, array &$seen,
}

/**
* @param list<array{dn: list<string>}|string> $list
* @psalm-param list<array{dn: list<string>}|string> $list
* @psalm-param array<string, int|array|string> $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 {
Expand All @@ -397,7 +403,9 @@ private function walkNestedGroupsReturnDNs(string $dn, Closure $fetcher, array $
}

/**
* @param list<array{dn: list<string>}> $list
* @psalm-param list<array{dn: list<string>}> $list
* @psalm-param array<string, int|array|string> $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 {
Expand All @@ -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
*/
Expand All @@ -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 {
Copy link
Member

@nickvergessen nickvergessen Dec 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apps/user_ldap/lib/Group_LDAP.php#L472

The declared return type 'null|string' for OCA\User_LDAP\Group_LDAP::getNameOfGroup does not allow false, but the function returns 'false|string'

$result = $this->access->searchGroups($filter, ['dn'], 1);
if (empty($result)) {
$this->access->connection->writeToCache($cacheKey, false);
Expand All @@ -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) {
Expand All @@ -478,17 +485,15 @@ private function getEntryGidNumber(string $dn, string $attribute) {
}

/**
* @return string|bool
* @return string|bool The group's gidNumber
* @throws ServerNotAvailableException
*/
public function getGroupGidNumber(string $dn) {
return $this->getEntryGidNumber($dn, 'gidNumber');
}

/**
* returns the user's gidNumber
*
* @return string|bool
* @return string|bool The user's gidNumber
* @throws ServerNotAvailableException
*/
public function getUserGidNumber(string $dn) {
Expand Down Expand Up @@ -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(
Expand All @@ -551,7 +555,7 @@ public function getUsersInGidNumber(

/**
* @throws ServerNotAvailableException
* @return bool
* @return false|string
*/
public function getUserGroupByGid(string $dn) {
$groupID = $this->getUserGidNumber($dn);
Expand All @@ -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
*/
Expand All @@ -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) {
Expand All @@ -607,15 +609,15 @@ 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) {
return $this->getEntryGroupID($dn, 'primaryGroupToken');
}

/**
* @return string|bool
* @return string|false
* @throws ServerNotAvailableException
*/
public function getUserPrimaryGroupIDs(string $dn) {
Expand Down Expand Up @@ -695,7 +697,7 @@ public function countUsersInPrimaryGroup(
}

/**
* @return string|bool
* @return string|false
* @throws ServerNotAvailableException
*/
public function getUserPrimaryGroup(string $dn) {
Expand Down Expand Up @@ -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 [];
}
Expand Down
2 changes: 1 addition & 1 deletion apps/user_ldap/lib/Wizard.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down