Skip to content

Commit 6b85a3a

Browse files
blizzzblizzz
authored
Merge pull request #46114 from nextcloud/enh/improve-ldap-group-members-listing-performances
fix(user_ldap): Avoid extra LDAP request when mapping a user for the first time
2 parents 45ba2f7 + 36479df commit 6b85a3a

File tree

6 files changed

+219
-47
lines changed

6 files changed

+219
-47
lines changed

‎apps/user_ldap/lib/Access.php‎

Lines changed: 96 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,56 @@ public function getConnection() {
116116
return $this->connection;
117117
}
118118

119+
/**
120+
* Reads several attributes for an LDAP record identified by a DN and a filter
121+
* No support for ranged attributes.
122+
*
123+
* @param string $dn the record in question
124+
* @param array $attrs the attributes that shall be retrieved
125+
* if empty, just check the record's existence
126+
* @param string $filter
127+
* @return array|false an array of values on success or an empty
128+
* array if $attr is empty, false otherwise
129+
* @throws ServerNotAvailableException
130+
*/
131+
public function readAttributes(string $dn, array $attrs, string $filter = 'objectClass=*'): array|false {
132+
if (!$this->checkConnection()) {
133+
$this->logger->warning(
134+
'No LDAP Connector assigned, access impossible for readAttribute.',
135+
['app' => 'user_ldap']
136+
);
137+
return false;
138+
}
139+
$cr = $this->connection->getConnectionResource();
140+
$attrs = array_map(
141+
fn (string $attr): string => mb_strtolower($attr, 'UTF-8'),
142+
$attrs,
143+
);
144+
145+
$values = [];
146+
$record = $this->executeRead($dn, $attrs, $filter);
147+
if (is_bool($record)) {
148+
// when an exists request was run and it was successful, an empty
149+
// array must be returned
150+
return $record ? [] : false;
151+
}
152+
153+
$result = [];
154+
foreach ($attrs as $attr) {
155+
$values = $this->extractAttributeValuesFromResult($record, $attr);
156+
if (!empty($values)) {
157+
$result[$attr] = $values;
158+
}
159+
}
160+
161+
if (!empty($result)) {
162+
return $result;
163+
}
164+
165+
$this->logger->debug('Requested attributes {attrs} not found for ' . $dn, ['app' => 'user_ldap', 'attrs' => $attrs]);
166+
return false;
167+
}
168+
119169
/**
120170
* reads a given attribute for an LDAP record identified by a DN
121171
*
@@ -191,9 +241,9 @@ public function readAttribute(string $dn, string $attr, string $filter = 'object
191241
* returned data on a successful usual operation
192242
* @throws ServerNotAvailableException
193243
*/
194-
public function executeRead(string $dn, string $attribute, string $filter) {
244+
public function executeRead(string $dn, string|array $attribute, string $filter) {
195245
$dn = $this->helper->DNasBaseParameter($dn);
196-
$rr = @$this->invokeLDAPMethod('read', $dn, $filter, [$attribute]);
246+
$rr = @$this->invokeLDAPMethod('read', $dn, $filter, (is_string($attribute) ? [$attribute] : $attribute));
197247
if (!$this->ldap->isResource($rr)) {
198248
if ($attribute !== '') {
199249
//do not throw this message on userExists check, irritates
@@ -410,15 +460,15 @@ public function dn2groupname($fdn, $ldapName = null) {
410460
* @return string|false with with the name to use in Nextcloud
411461
* @throws \Exception
412462
*/
413-
public function dn2username($fdn, $ldapName = null) {
463+
public function dn2username($fdn) {
414464
//To avoid bypassing the base DN settings under certain circumstances
415465
//with the group support, check whether the provided DN matches one of
416466
//the given Bases
417467
if (!$this->isDNPartOfBase($fdn, $this->connection->ldapBaseUsers)) {
418468
return false;
419469
}
420470

421-
return $this->dn2ocname($fdn, $ldapName, true);
471+
return $this->dn2ocname($fdn, null, true);
422472
}
423473

424474
/**
@@ -441,12 +491,8 @@ public function dn2ocname($fdn, $ldapName = null, $isUser = true, &$newlyMapped
441491
$newlyMapped = false;
442492
if ($isUser) {
443493
$mapper = $this->getUserMapper();
444-
$nameAttribute = $this->connection->ldapUserDisplayName;
445-
$filter = $this->connection->ldapUserFilter;
446494
} else {
447495
$mapper = $this->getGroupMapper();
448-
$nameAttribute = $this->connection->ldapGroupDisplayName;
449-
$filter = $this->connection->ldapGroupFilter;
450496
}
451497

452498
//let's try to retrieve the Nextcloud name from the mappings table
@@ -455,6 +501,36 @@ public function dn2ocname($fdn, $ldapName = null, $isUser = true, &$newlyMapped
455501
return $ncName;
456502
}
457503

504+
if ($isUser) {
505+
$nameAttribute = strtolower($this->connection->ldapUserDisplayName);
506+
$filter = $this->connection->ldapUserFilter;
507+
$uuidAttr = 'ldapUuidUserAttribute';
508+
$uuidOverride = $this->connection->ldapExpertUUIDUserAttr;
509+
$usernameAttribute = strtolower($this->connection->ldapExpertUsernameAttr);
510+
$attributesToRead = [$nameAttribute,$usernameAttribute];
511+
// TODO fetch also display name attributes and cache them if the user is mapped
512+
} else {
513+
$nameAttribute = strtolower($this->connection->ldapGroupDisplayName);
514+
$filter = $this->connection->ldapGroupFilter;
515+
$uuidAttr = 'ldapUuidGroupAttribute';
516+
$uuidOverride = $this->connection->ldapExpertUUIDGroupAttr;
517+
$attributesToRead = [$nameAttribute];
518+
}
519+
520+
if ($this->detectUuidAttribute($fdn, $isUser, false, $record)) {
521+
$attributesToRead[] = $this->connection->$uuidAttr;
522+
}
523+
524+
if ($record === null) {
525+
/* No record was passed, fetch it */
526+
$record = $this->readAttributes($fdn, $attributesToRead, $filter);
527+
if ($record === false) {
528+
$this->logger->debug('Cannot read attributes for ' . $fdn . '. Skipping.', ['filter' => $filter]);
529+
$intermediates[($isUser ? 'user-' : 'group-') . $fdn] = true;
530+
return false;
531+
}
532+
}
533+
458534
//second try: get the UUID and check if it is known. Then, update the DN and return the name.
459535
$uuid = $this->getUUID($fdn, $isUser, $record);
460536
if (is_string($uuid)) {
@@ -469,20 +545,9 @@ public function dn2ocname($fdn, $ldapName = null, $isUser = true, &$newlyMapped
469545
return false;
470546
}
471547

472-
if (is_null($ldapName)) {
473-
$ldapName = $this->readAttribute($fdn, $nameAttribute, $filter);
474-
if (!isset($ldapName[0]) || empty($ldapName[0])) {
475-
$this->logger->debug('No or empty name for ' . $fdn . ' with filter ' . $filter . '.', ['app' => 'user_ldap']);
476-
$intermediates[($isUser ? 'user-' : 'group-') . $fdn] = true;
477-
return false;
478-
}
479-
$ldapName = $ldapName[0];
480-
}
481-
482548
if ($isUser) {
483-
$usernameAttribute = (string)$this->connection->ldapExpertUsernameAttr;
484549
if ($usernameAttribute !== '') {
485-
$username = $this->readAttribute($fdn, $usernameAttribute);
550+
$username = $record[$usernameAttribute];
486551
if (!isset($username[0]) || empty($username[0])) {
487552
$this->logger->debug('No or empty username (' . $usernameAttribute . ') for ' . $fdn . '.', ['app' => 'user_ldap']);
488553
return false;
@@ -504,6 +569,15 @@ public function dn2ocname($fdn, $ldapName = null, $isUser = true, &$newlyMapped
504569
return false;
505570
}
506571
} else {
572+
if (is_null($ldapName)) {
573+
$ldapName = $record[$nameAttribute];
574+
if (!isset($ldapName[0]) || empty($ldapName[0])) {
575+
$this->logger->debug('No or empty name for ' . $fdn . ' with filter ' . $filter . '.', ['app' => 'user_ldap']);
576+
$intermediates['group-' . $fdn] = true;
577+
return false;
578+
}
579+
$ldapName = $ldapName[0];
580+
}
507581
$intName = $this->sanitizeGroupIDCandidate($ldapName);
508582
}
509583

@@ -521,6 +595,7 @@ public function dn2ocname($fdn, $ldapName = null, $isUser = true, &$newlyMapped
521595
$this->connection->setConfiguration(['ldapCacheTTL' => $originalTTL]);
522596
$newlyMapped = $this->mapAndAnnounceIfApplicable($mapper, $fdn, $intName, $uuid, $isUser);
523597
if ($newlyMapped) {
598+
$this->logger->debug('Mapped {fdn} as {name}', ['fdn' => $fdn,'name' => $intName]);
524599
return $intName;
525600
}
526601
}
@@ -535,7 +610,6 @@ public function dn2ocname($fdn, $ldapName = null, $isUser = true, &$newlyMapped
535610
'fdn' => $fdn,
536611
'altName' => $altName,
537612
'intName' => $intName,
538-
'app' => 'user_ldap',
539613
]
540614
);
541615
$newlyMapped = true;
@@ -660,6 +734,7 @@ public function cacheUserHome(string $ocName, $home): void {
660734
*/
661735
public function cacheUserExists(string $ocName): void {
662736
$this->connection->writeToCache('userExists' . $ocName, true);
737+
$this->connection->writeToCache('userExistsOnLDAP' . $ocName, true);
663738
}
664739

665740
/**

‎apps/user_ldap/lib/Configuration.php‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -294,6 +294,27 @@ public function readConfiguration(): void {
294294
break;
295295
case 'ldapUserDisplayName2':
296296
case 'ldapGroupDisplayName':
297+
case 'ldapGidNumber':
298+
case 'ldapGroupMemberAssocAttr':
299+
case 'ldapQuotaAttribute':
300+
case 'ldapEmailAttribute':
301+
case 'ldapUuidUserAttribute':
302+
case 'ldapUuidGroupAttribute':
303+
case 'ldapExpertUsernameAttr':
304+
case 'ldapExpertUUIDUserAttr':
305+
case 'ldapExpertUUIDGroupAttr':
306+
case 'ldapExtStorageHomeAttribute':
307+
case 'ldapAttributePhone':
308+
case 'ldapAttributeWebsite':
309+
case 'ldapAttributeAddress':
310+
case 'ldapAttributeTwitter':
311+
case 'ldapAttributeFediverse':
312+
case 'ldapAttributeOrganisation':
313+
case 'ldapAttributeRole':
314+
case 'ldapAttributeHeadline':
315+
case 'ldapAttributeBiography':
316+
case 'ldapAttributeBirthDate':
317+
case 'ldapAttributeAnniversaryDate':
297318
$readMethod = 'getLcValue';
298319
break;
299320
case 'ldapUserDisplayName':

‎apps/user_ldap/lib/User/Manager.php‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,7 @@ public function invalidate($uid) {
106106
/**
107107
* @brief checks whether the Access instance has been set
108108
* @throws \Exception if Access has not been set
109+
* @psalm-assert !null $this->access
109110
* @return null
110111
*/
111112
private function checkAccess() {
@@ -237,4 +238,37 @@ public function get($id) {
237238

238239
return $this->createInstancyByUserName($id);
239240
}
241+
242+
/**
243+
* @brief Checks whether a User object by its DN or Nextcloud username exists
244+
* @param string $id the DN or username of the user
245+
* @throws \Exception when connection could not be established
246+
*/
247+
public function exists($id): bool {
248+
$this->checkAccess();
249+
$this->logger->debug('Checking if {id} exists', ['id' => $id]);
250+
if (isset($this->usersByDN[$id])) {
251+
return true;
252+
} elseif (isset($this->usersByUid[$id])) {
253+
return true;
254+
}
255+
256+
if ($this->access->stringResemblesDN($id)) {
257+
$this->logger->debug('{id} looks like a dn', ['id' => $id]);
258+
$uid = $this->access->dn2username($id);
259+
if ($uid !== false) {
260+
return true;
261+
}
262+
}
263+
264+
// Most likely a uid. Check whether it is a deleted user
265+
if ($this->isDeletedUser($id)) {
266+
return true;
267+
}
268+
$dn = $this->access->username2dn($id);
269+
if ($dn !== false) {
270+
return true;
271+
}
272+
return false;
273+
}
240274
}

‎apps/user_ldap/lib/User_LDAP.php‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -322,10 +322,9 @@ public function userExists($uid) {
322322
if (!is_null($userExists)) {
323323
return (bool)$userExists;
324324
}
325-
//getting dn, if false the user does not exist. If dn, he may be mapped only, requires more checking.
326-
$user = $this->access->userManager->get($uid);
325+
$userExists = $this->access->userManager->exists($uid);
327326

328-
if (is_null($user)) {
327+
if (!$userExists) {
329328
$this->logger->debug(
330329
'No DN found for '.$uid.' on '.$this->access->connection->ldapHost,
331330
['app' => 'user_ldap']
@@ -464,7 +463,6 @@ public function getDisplayName($uid) {
464463
$this->access->connection->writeToCache($cacheKey, $displayName);
465464
}
466465
if ($user instanceof OfflineUser) {
467-
/** @var OfflineUser $user */
468466
$displayName = $user->getDisplayName();
469467
}
470468
return $displayName;
@@ -611,7 +609,6 @@ public function createUser($username, $password) {
611609
$uuid,
612610
true
613611
);
614-
$this->access->cacheUserExists($username);
615612
} else {
616613
$this->logger->warning(
617614
'Failed to map created LDAP user with userid {userid}, because UUID could not be determined',

‎apps/user_ldap/tests/AccessTest.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -612,7 +612,8 @@ public function testFetchListOfUsers() {
612612

613613
$this->prepareMocksForSearchTests($base, $fakeConnection, $fakeSearchResultResource, $fakeLdapEntries);
614614

615-
$this->connection->expects($this->exactly($fakeLdapEntries['count']))
615+
// Called twice per user, for userExists and userExistsOnLdap
616+
$this->connection->expects($this->exactly(2 * $fakeLdapEntries['count']))
616617
->method('writeToCache')
617618
->with($this->stringStartsWith('userExists'), true);
618619

0 commit comments

Comments
 (0)