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

apps/user_ldap/lib/Access.php

@@ -116,6 +116,56 @@ class Access extends LDAPUtility {
 		return $this->connection;
 	}
 
+	/**
+	 * Reads several attributes for an LDAP record identified by a DN and a filter
+	 * No support for ranged attributes.
+	 *
+	 * @param string $dn the record in question
+	 * @param array $attrs the attributes that shall be retrieved
+	 *                     if empty, just check the record's existence
+	 * @param string $filter
+	 * @return array|false an array of values on success or an empty
+	 *                     array if $attr is empty, false otherwise
+	 * @throws ServerNotAvailableException
+	 */
+	public function readAttributes(string $dn, array $attrs, string $filter = 'objectClass=*'): array|false {
+		if (!$this->checkConnection()) {
+			$this->logger->warning(
+				'No LDAP Connector assigned, access impossible for readAttribute.',
+				['app' => 'user_ldap']
+			);
+			return false;
+		}
+		$cr = $this->connection->getConnectionResource();
+		$attrs = array_map(
+			fn (string $attr): string => mb_strtolower($attr, 'UTF-8'),
+			$attrs,
+		);
+
+		$values = [];
+		$record = $this->executeRead($dn, $attrs, $filter);
+		if (is_bool($record)) {
+			// when an exists request was run and it was successful, an empty
+			// array must be returned
+			return $record ? [] : false;
+		}
+
+		$result = [];
+		foreach ($attrs as $attr) {
+			$values = $this->extractAttributeValuesFromResult($record, $attr);
+			if (!empty($values)) {
+				$result[$attr] = $values;
+			}
+		}
+
+		if (!empty($result)) {
+			return $result;
+		}
+
+		$this->logger->debug('Requested attributes {attrs} not found for ' . $dn, ['app' => 'user_ldap', 'attrs' => $attrs]);
+		return false;
+	}
+
 	/**
 	 * reads a given attribute for an LDAP record identified by a DN
 	 *
@@ -191,9 +241,9 @@ class Access extends LDAPUtility {
 	 *                    returned data on a successful usual operation
 	 * @throws ServerNotAvailableException
 	 */
-	public function executeRead(string $dn, string $attribute, string $filter) {
+	public function executeRead(string $dn, string|array $attribute, string $filter) {
 		$dn = $this->helper->DNasBaseParameter($dn);
-		$rr = @$this->invokeLDAPMethod('read', $dn, $filter, [$attribute]);
+		$rr = @$this->invokeLDAPMethod('read', $dn, $filter, (is_string($attribute) ? [$attribute] : $attribute));
 		if (!$this->ldap->isResource($rr)) {
 			if ($attribute !== '') {
 				//do not throw this message on userExists check, irritates
@@ -410,7 +460,7 @@ class Access extends LDAPUtility {
 	 * @return string|false with with the name to use in Nextcloud
 	 * @throws \Exception
 	 */
-	public function dn2username($fdn, $ldapName = null) {
+	public function dn2username($fdn) {
 		//To avoid bypassing the base DN settings under certain circumstances
 		//with the group support, check whether the provided DN matches one of
 		//the given Bases
@@ -418,7 +468,7 @@ class Access extends LDAPUtility {
 			return false;
 		}
 
-		return $this->dn2ocname($fdn, $ldapName, true);
+		return $this->dn2ocname($fdn, null, true);
 	}
 
 	/**
@@ -441,12 +491,8 @@ class Access extends LDAPUtility {
 		$newlyMapped = false;
 		if ($isUser) {
 			$mapper = $this->getUserMapper();
-			$nameAttribute = $this->connection->ldapUserDisplayName;
-			$filter = $this->connection->ldapUserFilter;
 		} else {
 			$mapper = $this->getGroupMapper();
-			$nameAttribute = $this->connection->ldapGroupDisplayName;
-			$filter = $this->connection->ldapGroupFilter;
 		}
 
 		//let's try to retrieve the Nextcloud name from the mappings table
@@ -455,6 +501,36 @@ class Access extends LDAPUtility {
 			return $ncName;
 		}
 
+		if ($isUser) {
+			$nameAttribute = strtolower($this->connection->ldapUserDisplayName);
+			$filter = $this->connection->ldapUserFilter;
+			$uuidAttr = 'ldapUuidUserAttribute';
+			$uuidOverride = $this->connection->ldapExpertUUIDUserAttr;
+			$usernameAttribute = strtolower($this->connection->ldapExpertUsernameAttr);
+			$attributesToRead = [$nameAttribute,$usernameAttribute];
+			// TODO fetch also display name attributes and cache them if the user is mapped
+		} else {
+			$nameAttribute = strtolower($this->connection->ldapGroupDisplayName);
+			$filter = $this->connection->ldapGroupFilter;
+			$uuidAttr = 'ldapUuidGroupAttribute';
+			$uuidOverride = $this->connection->ldapExpertUUIDGroupAttr;
+			$attributesToRead = [$nameAttribute];
+		}
+
+		if ($this->detectUuidAttribute($fdn, $isUser, false, $record)) {
+			$attributesToRead[] = $this->connection->$uuidAttr;
+		}
+
+		if ($record === null) {
+			/* No record was passed, fetch it */
+			$record = $this->readAttributes($fdn, $attributesToRead, $filter);
+			if ($record === false) {
+				$this->logger->debug('Cannot read attributes for ' . $fdn . '. Skipping.', ['filter' => $filter]);
+				$intermediates[($isUser ? 'user-' : 'group-') . $fdn] = true;
+				return false;
+			}
+		}
+
 		//second try: get the UUID and check if it is known. Then, update the DN and return the name.
 		$uuid = $this->getUUID($fdn, $isUser, $record);
 		if (is_string($uuid)) {
@@ -469,20 +545,9 @@ class Access extends LDAPUtility {
 			return false;
 		}
 
-		if (is_null($ldapName)) {
-			$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];
-		}
-
 		if ($isUser) {
-			$usernameAttribute = (string)$this->connection->ldapExpertUsernameAttr;
 			if ($usernameAttribute !== '') {
-				$username = $this->readAttribute($fdn, $usernameAttribute);
+				$username = $record[$usernameAttribute];
 				if (!isset($username[0]) || empty($username[0])) {
 					$this->logger->debug('No or empty username (' . $usernameAttribute . ') for ' . $fdn . '.', ['app' => 'user_ldap']);
 					return false;
@@ -504,6 +569,15 @@ class Access extends LDAPUtility {
 				return false;
 			}
 		} else {
+			if (is_null($ldapName)) {
+				$ldapName = $record[$nameAttribute];
+				if (!isset($ldapName[0]) || empty($ldapName[0])) {
+					$this->logger->debug('No or empty name for ' . $fdn . ' with filter ' . $filter . '.', ['app' => 'user_ldap']);
+					$intermediates['group-' . $fdn] = true;
+					return false;
+				}
+				$ldapName = $ldapName[0];
+			}
 			$intName = $this->sanitizeGroupIDCandidate($ldapName);
 		}
 
@@ -521,6 +595,7 @@ class Access extends LDAPUtility {
 			$this->connection->setConfiguration(['ldapCacheTTL' => $originalTTL]);
 			$newlyMapped = $this->mapAndAnnounceIfApplicable($mapper, $fdn, $intName, $uuid, $isUser);
 			if ($newlyMapped) {
+				$this->logger->debug('Mapped {fdn} as {name}', ['fdn' => $fdn,'name' => $intName]);
 				return $intName;
 			}
 		}
@@ -535,7 +610,6 @@ class Access extends LDAPUtility {
 						'fdn' => $fdn,
 						'altName' => $altName,
 						'intName' => $intName,
-						'app' => 'user_ldap',
 					]
 				);
 				$newlyMapped = true;
@@ -660,6 +734,7 @@ class Access extends LDAPUtility {
 	 */
 	public function cacheUserExists(string $ocName): void {
 		$this->connection->writeToCache('userExists' . $ocName, true);
+		$this->connection->writeToCache('userExistsOnLDAP' . $ocName, true);
 	}
 
 	/**

apps/user_ldap/lib/Configuration.php

@@ -294,6 +294,27 @@ class Configuration {
 						break;
 					case 'ldapUserDisplayName2':
 					case 'ldapGroupDisplayName':
+					case 'ldapGidNumber':
+					case 'ldapGroupMemberAssocAttr':
+					case 'ldapQuotaAttribute':
+					case 'ldapEmailAttribute':
+					case 'ldapUuidUserAttribute':
+					case 'ldapUuidGroupAttribute':
+					case 'ldapExpertUsernameAttr':
+					case 'ldapExpertUUIDUserAttr':
+					case 'ldapExpertUUIDGroupAttr':
+					case 'ldapExtStorageHomeAttribute':
+					case 'ldapAttributePhone':
+					case 'ldapAttributeWebsite':
+					case 'ldapAttributeAddress':
+					case 'ldapAttributeTwitter':
+					case 'ldapAttributeFediverse':
+					case 'ldapAttributeOrganisation':
+					case 'ldapAttributeRole':
+					case 'ldapAttributeHeadline':
+					case 'ldapAttributeBiography':
+					case 'ldapAttributeBirthDate':
+					case 'ldapAttributeAnniversaryDate':
 						$readMethod = 'getLcValue';
 						break;
 					case 'ldapUserDisplayName':

apps/user_ldap/lib/User/Manager.php

@@ -106,6 +106,7 @@ class Manager {
 	/**
 	 * @brief checks whether the Access instance has been set
 	 * @throws \Exception if Access has not been set
+	 * @psalm-assert !null $this->access
 	 * @return null
 	 */
 	private function checkAccess() {
@@ -237,4 +238,37 @@ class Manager {
 
 		return $this->createInstancyByUserName($id);
 	}
+
+	/**
+	 * @brief Checks whether a User object by its DN or Nextcloud username exists
+	 * @param string $id the DN or username of the user
+	 * @throws \Exception when connection could not be established
+	 */
+	public function exists($id): bool {
+		$this->checkAccess();
+		$this->logger->debug('Checking if {id} exists', ['id' => $id]);
+		if (isset($this->usersByDN[$id])) {
+			return true;
+		} elseif (isset($this->usersByUid[$id])) {
+			return true;
+		}
+
+		if ($this->access->stringResemblesDN($id)) {
+			$this->logger->debug('{id} looks like a dn', ['id' => $id]);
+			$uid = $this->access->dn2username($id);
+			if ($uid !== false) {
+				return true;
+			}
+		}
+
+		// Most likely a uid. Check whether it is a deleted user
+		if ($this->isDeletedUser($id)) {
+			return true;
+		}
+		$dn = $this->access->username2dn($id);
+		if ($dn !== false) {
+			return true;
+		}
+		return false;
+	}
 }

apps/user_ldap/lib/User_LDAP.php

@@ -322,10 +322,9 @@ class User_LDAP extends BackendUtility implements IUserBackend, UserInterface, I
 		if (!is_null($userExists)) {
 			return (bool)$userExists;
 		}
-		//getting dn, if false the user does not exist. If dn, he may be mapped only, requires more checking.
-		$user = $this->access->userManager->get($uid);
+		$userExists = $this->access->userManager->exists($uid);
 
-		if (is_null($user)) {
+		if (!$userExists) {
 			$this->logger->debug(
 				'No DN found for '.$uid.' on '.$this->access->connection->ldapHost,
 				['app' => 'user_ldap']
@@ -464,7 +463,6 @@ class User_LDAP extends BackendUtility implements IUserBackend, UserInterface, I
 				$this->access->connection->writeToCache($cacheKey, $displayName);
 			}
 			if ($user instanceof OfflineUser) {
-				/** @var OfflineUser $user */
 				$displayName = $user->getDisplayName();
 			}
 			return $displayName;
@@ -611,7 +609,6 @@ class User_LDAP extends BackendUtility implements IUserBackend, UserInterface, I
 							$uuid,
 							true
 						);
-						$this->access->cacheUserExists($username);
 					} else {
 						$this->logger->warning(
 							'Failed to map created LDAP user with userid {userid}, because UUID could not be determined',

apps/user_ldap/tests/AccessTest.php

@@ -612,7 +612,8 @@ class AccessTest extends TestCase {
 
 		$this->prepareMocksForSearchTests($base, $fakeConnection, $fakeSearchResultResource, $fakeLdapEntries);
 
-		$this->connection->expects($this->exactly($fakeLdapEntries['count']))
+		// Called twice per user, for userExists and userExistsOnLdap
+		$this->connection->expects($this->exactly(2 * $fakeLdapEntries['count']))
 			->method('writeToCache')
 			->with($this->stringStartsWith('userExists'), true);
 

apps/user_ldap/tests/User_LDAPTest.php

@@ -303,6 +303,10 @@ class User_LDAPTest extends TestCase {
 		$this->userManager->expects($this->atLeastOnce())
 			->method('get')
 			->willReturn($offlineUser);
+		$this->userManager->expects($this->once())
+			->method('exists')
+			->with($uid)
+			->willReturn(true);
 
 		$backend = new UserLDAP($this->access, $this->notificationManager, $this->pluginManager, $this->logger, $this->deletedUsersIndex);
 
@@ -490,9 +494,12 @@ class User_LDAPTest extends TestCase {
 
 		$user = $this->createMock(User::class);
 
-		$this->userManager->expects($this->atLeastOnce())
-			->method('get')
-			->willReturn($user);
+		$this->userManager->expects($this->never())
+			->method('get');
+		$this->userManager->expects($this->once())
+			->method('exists')
+			->with('gunslinger')
+			->willReturn(true);
 		$this->access->expects($this->any())
 			->method('getUserMapper')
 			->willReturn($this->createMock(UserMapping::class));
@@ -517,11 +524,12 @@ class User_LDAPTest extends TestCase {
 			->method('getUserMapper')
 			->willReturn($mapper);
 
-		$user = $this->createMock(User::class);
-
-		$this->userManager->expects($this->atLeastOnce())
-			->method('get')
-			->willReturn($user);
+		$this->userManager->expects($this->never())
+			->method('get');
+		$this->userManager->expects($this->once())
+			->method('exists')
+			->with('formerUser')
+			->willReturn(true);
 
 		//test for deleted user – always returns true as long as we have the user in DB
 		$this->assertTrue($backend->userExists('formerUser'));
@@ -564,9 +572,12 @@ class User_LDAPTest extends TestCase {
 				}
 				return false;
 			});
-		$this->userManager->expects($this->atLeastOnce())
-			->method('get')
-			->willReturn($user);
+		$this->userManager->expects($this->never())
+			->method('get');
+		$this->userManager->expects($this->once())
+			->method('exists')
+			->with('gunslinger')
+			->willReturn(true);
 		$this->access->expects($this->any())
 			->method('getUserMapper')
 			->willReturn($this->createMock(UserMapping::class));
@@ -625,7 +636,12 @@ class User_LDAPTest extends TestCase {
 
 		$this->userManager->expects($this->atLeastOnce())
 			->method('get')
+			->with('gunslinger')
 			->willReturn($user);
+		$this->userManager->expects($this->once())
+			->method('exists')
+			->with('gunslinger')
+			->willReturn(true);
 
 		//absolute path
 		/** @noinspection PhpUnhandledExceptionInspection */
@@ -678,6 +694,10 @@ class User_LDAPTest extends TestCase {
 		$this->userManager->expects($this->atLeastOnce())
 			->method('get')
 			->willReturn($user);
+		$this->userManager->expects($this->once())
+			->method('exists')
+			->with('ladyofshadows')
+			->willReturn(true);
 
 		/** @noinspection PhpUnhandledExceptionInspection */
 		$result = $backend->getHome('ladyofshadows');
@@ -707,14 +727,6 @@ class User_LDAPTest extends TestCase {
 						return false;
 				}
 			});
-		$this->access->connection->expects($this->any())
-			->method('getFromCache')
-			->willReturnCallback(function ($key) {
-				if ($key === 'userExistsnewyorker') {
-					return true;
-				}
-				return null;
-			});
 
 		$user = $this->createMock(User::class);
 		$user->expects($this->any())
@@ -726,7 +738,12 @@ class User_LDAPTest extends TestCase {
 
 		$this->userManager->expects($this->atLeastOnce())
 			->method('get')
+			->with('newyorker')
 			->willReturn($user);
+		$this->userManager->expects($this->once())
+			->method('exists')
+			->with('newyorker')
+			->willReturn(true);
 
 		//no path at all – triggers OC default behaviour
 		$result = $backend->getHome('newyorker');
@@ -765,7 +782,12 @@ class User_LDAPTest extends TestCase {
 
 		$this->userManager->expects($this->atLeastOnce())
 			->method('get')
+			->with($uid)
 			->willReturn($offlineUser);
+		$this->userManager->expects($this->once())
+			->method('exists')
+			->with($uid)
+			->willReturn(true);
 
 		$result = $backend->getHome($uid);
 		$this->assertFalse($result);
@@ -865,6 +887,16 @@ class User_LDAPTest extends TestCase {
 				}
 				return null;
 			});
+		$this->userManager->expects($this->any())
+			->method('exists')
+			->willReturnCallback(function ($uid) use ($user1, $user2) {
+				if ($uid === 'gunslinger') {
+					return true;
+				} elseif ($uid === 'newyorker') {
+					return true;
+				}
+				return false;
+			});
 		$this->access->expects($this->any())
 			->method('getUserMapper')
 			->willReturn($mapper);
@@ -948,6 +980,16 @@ class User_LDAPTest extends TestCase {
 				}
 				return null;
 			});
+		$this->userManager->expects($this->any())
+			->method('exists')
+			->willReturnCallback(function ($uid) use ($user1, $user2) {
+				if ($uid === 'gunslinger') {
+					return true;
+				} elseif ($uid === 'newyorker') {
+					return true;
+				}
+				return false;
+			});
 		$this->access->expects($this->any())
 			->method('getUserMapper')
 			->willReturn($mapper);
@@ -958,7 +1000,7 @@ class User_LDAPTest extends TestCase {
 			});
 
 		//with displayName
-		$result = \OC::$server->getUserManager()->get('gunslinger')->getDisplayName();
+		$result = \OC::$server->getUserManager()->get('gunslinger')?->getDisplayName();
 		$this->assertEquals('Roland Deschain', $result);
 
 		//empty displayname retrieved
@@ -1052,6 +1094,8 @@ class User_LDAPTest extends TestCase {
 			->method('get')
 			->with($dn)
 			->willReturn($user);
+		$this->userManager->expects($this->never())
+			->method('exists');
 		$this->userManager->expects($this->any())
 			->method('getAttributes')
 			->willReturn(['dn', 'uid', 'mail', 'displayname']);