Merge pull request #55823 from nextcloud/backport/54429/stable32

apps/user_ldap/ajax/clearMappings.php

@@ -8,7 +8,6 @@
 use OCA\User_LDAP\Mapping\GroupMapping;
 use OCA\User_LDAP\Mapping\UserMapping;
 use OCP\EventDispatcher\IEventDispatcher;
-use OCP\IDBConnection;
 use OCP\IUserManager;
 use OCP\Server;
 use OCP\User\Events\BeforeUserIdUnassignedEvent;
@@ -40,7 +39,7 @@ try {
 			}
 		);
 	} elseif ($subject === 'group') {
-		$mapping = new GroupMapping(Server::get(IDBConnection::class));
+		$mapping = Server::get(GroupMapping::class);
 		$result = $mapping->clear();
 	}
 

apps/user_ldap/lib/Mapping/AbstractMapping.php

@@ -10,6 +10,9 @@ namespace OCA\User_LDAP\Mapping;
 use Doctrine\DBAL\Exception;
 use OCP\DB\IPreparedStatement;
 use OCP\DB\QueryBuilder\IQueryBuilder;
+use OCP\IAppConfig;
+use OCP\ICache;
+use OCP\ICacheFactory;
 use OCP\IDBConnection;
 use OCP\Server;
 use Psr\Log\LoggerInterface;
@@ -27,16 +30,81 @@ abstract class AbstractMapping {
 	 */
 	abstract protected function getTableName(bool $includePrefix = true);
 
+	/**
+	 * A month worth of cache time for as good as never changing mapping data.
+	 * Implemented when it was found that name-to-DN lookups are quite frequent.
+	 */
+	protected const LOCAL_CACHE_TTL = 2592000;
+
+	/**
+	 * A week worth of cache time for rarely changing user count data.
+	 */
+	protected const LOCAL_USER_COUNT_TTL = 604800;
+
+	/**
+	 * By default, the local cache is only used up to a certain amount of objects.
+	 * This constant holds this number. The amount of entries would amount up to
+	 * 1 MiB (worst case) per mappings table.
+	 * Setting `use_local_mapping_cache` for `user_ldap` to `yes` or `no`
+	 * deliberately enables or disables this mechanism.
+	 */
+	protected const LOCAL_CACHE_OBJECT_THRESHOLD = 2000;
+
+	protected ?ICache $localNameToDnCache = null;
+
+	/** @var array caches Names (value) by DN (key) */
+	protected array $cache = [];
+
 	/**
 	 * @param IDBConnection $dbc
 	 */
 	public function __construct(
 		protected IDBConnection $dbc,
+		protected ICacheFactory $cacheFactory,
+		protected IAppConfig $config,
+		protected bool $isCLI,
 	) {
+		$this->initLocalCache();
 	}
 
-	/** @var array caches Names (value) by DN (key) */
-	protected $cache = [];
+	protected function initLocalCache(): void {
+		if ($this->isCLI || !$this->cacheFactory->isLocalCacheAvailable()) {
+			return;
+		}
+
+		$useLocalCache = $this->config->getValueString('user_ldap', 'use_local_mapping_cache', 'auto', false);
+		if ($useLocalCache !== 'yes' && $useLocalCache !== 'auto') {
+			return;
+		}
+
+		$section = \str_contains($this->getTableName(), 'user') ? 'u/' : 'g/';
+		$this->localNameToDnCache = $this->cacheFactory->createLocal('ldap/map/' . $section);
+
+		// We use the cache as well to store whether it shall be used. If the
+		// answer was no, we unset it again.
+		if ($useLocalCache === 'auto' && !$this->useCacheByUserCount()) {
+			$this->localNameToDnCache = null;
+		}
+	}
+
+	protected function useCacheByUserCount(): bool {
+		$use = $this->localNameToDnCache->get('use');
+		if ($use !== null) {
+			return $use;
+		}
+
+		$qb = $this->dbc->getQueryBuilder();
+		$q = $qb->selectAlias($qb->createFunction('COUNT(owncloud_name)'), 'count')
+			->from($this->getTableName());
+		$q->setMaxResults(self::LOCAL_CACHE_OBJECT_THRESHOLD + 1);
+		$result = $q->executeQuery();
+		$row = $result->fetch();
+		$result->closeCursor();
+
+		$use = (int)$row['count'] <= self::LOCAL_CACHE_OBJECT_THRESHOLD;
+		$this->localNameToDnCache->set('use', $use, self::LOCAL_USER_COUNT_TTL);
+		return $use;
+	}
 
 	/**
 	 * checks whether a provided string represents an existing table col
@@ -71,14 +139,13 @@ abstract class AbstractMapping {
 			//having SQL injection at all.
 			throw new \Exception('Invalid Column Name');
 		}
-		$query = $this->dbc->prepare('
-			SELECT `' . $fetchCol . '`
-			FROM `' . $this->getTableName() . '`
-			WHERE `' . $compareCol . '` = ?
-		');
+		$qb = $this->dbc->getQueryBuilder();
+		$qb->select($fetchCol)
+			->from($this->getTableName())
+			->where($qb->expr()->eq($compareCol, $qb->createNamedParameter($search)));
 
 		try {
-			$res = $query->execute([$search]);
+			$res = $qb->executeQuery();
 			$data = $res->fetchOne();
 			$res->closeCursor();
 			return $data;
@@ -107,17 +174,20 @@ abstract class AbstractMapping {
 
 	/**
 	 * Gets the LDAP DN based on the provided name.
-	 * Replaces Access::ocname2dn
-	 *
-	 * @param string $name
-	 * @return string|false
 	 */
-	public function getDNByName($name) {
-		$dn = array_search($name, $this->cache);
-		if ($dn === false && ($dn = $this->getXbyY('ldap_dn', 'owncloud_name', $name)) !== false) {
-			$this->cache[$dn] = $name;
+	public function getDNByName(string $name): string|false {
+		$dn = array_search($name, $this->cache, true);
+		if ($dn === false) {
+			$dn = $this->localNameToDnCache?->get($name);
+			if ($dn === null) {
+				$dn = $this->getXbyY('ldap_dn', 'owncloud_name', $name);
+				if ($dn !== false) {
+					$this->cache[$dn] = $name;
+				}
+				$this->localNameToDnCache?->set($name, $dn, self::LOCAL_CACHE_TTL);
+			}
 		}
-		return $dn;
+		return $dn ?? false;
 	}
 
 	/**
@@ -136,9 +206,15 @@ abstract class AbstractMapping {
 		');
 
 		$r = $this->modify($statement, [$this->getDNHash($fdn), $fdn, $uuid]);
-
-		if ($r && is_string($oldDn) && isset($this->cache[$oldDn])) {
-			$this->cache[$fdn] = $this->cache[$oldDn];
+		if ($r) {
+			if (is_string($oldDn) && isset($this->cache[$oldDn])) {
+				$userId = $this->cache[$oldDn];
+			}
+			$userId = $userId ?? $this->getNameByUUID($uuid);
+			if ($userId) {
+				$this->cache[$fdn] = $userId;
+				$this->localNameToDnCache?->set($userId, $fdn, self::LOCAL_CACHE_TTL);
+			}
 			unset($this->cache[$oldDn]);
 		}
 
@@ -351,6 +427,7 @@ abstract class AbstractMapping {
 			$result = $this->dbc->insertIfNotExist($this->getTableName(), $row);
 			if ((bool)$result === true) {
 				$this->cache[$fdn] = $name;
+				$this->localNameToDnCache?->set($name, $fdn, self::LOCAL_CACHE_TTL);
 			}
 			// insertIfNotExist returns values as int
 			return (bool)$result;
@@ -374,6 +451,7 @@ abstract class AbstractMapping {
 		if ($dn !== false) {
 			unset($this->cache[$dn]);
 		}
+		$this->localNameToDnCache?->remove($name);
 
 		return $this->modify($statement, [$name]);
 	}
@@ -389,6 +467,7 @@ abstract class AbstractMapping {
 			->getTruncateTableSQL('`' . $this->getTableName() . '`');
 		try {
 			$this->dbc->executeQuery($sql);
+			$this->localNameToDnCache?->clear();
 
 			return true;
 		} catch (Exception $e) {

apps/user_ldap/lib/Mapping/UserMapping.php

@@ -8,6 +8,8 @@
 namespace OCA\User_LDAP\Mapping;
 
 use OCP\HintException;
+use OCP\IAppConfig;
+use OCP\ICacheFactory;
 use OCP\IDBConnection;
 use OCP\IRequest;
 use OCP\Server;
@@ -24,9 +26,12 @@ class UserMapping extends AbstractMapping {
 
 	public function __construct(
 		IDBConnection $dbc,
+		ICacheFactory $cacheFactory,
+		IAppConfig $config,
+		bool $isCLI,
 		private IAssertion $assertion,
 	) {
-		parent::__construct($dbc);
+		parent::__construct($dbc, $cacheFactory, $config, $isCLI);
 	}
 
 	/**

apps/user_ldap/lib/User_LDAP.php

@@ -70,8 +70,9 @@ class User_LDAP extends BackendUtility implements IUserBackend, UserInterface, I
 	public function loginName2UserName($loginName, bool $forceLdapRefetch = false) {
 		$cacheKey = 'loginName2UserName-' . $loginName;
 		$username = $this->access->connection->getFromCache($cacheKey);
+		$knownDn = $username ? $this->access->username2dn($username) : false;
 
-		$ignoreCache = ($username === false && $forceLdapRefetch);
+		$ignoreCache = (($username === false || $knownDn === false) && $forceLdapRefetch);
 		if ($username !== null && !$ignoreCache) {
 			return $username;
 		}
@@ -138,6 +139,16 @@ class User_LDAP extends BackendUtility implements IUserBackend, UserInterface, I
 			return false;
 		}
 		$dn = $this->access->username2dn($username);
+		if ($dn === false) {
+			$this->logger->warning(
+				'LDAP Login: Found user {user}, but no assigned DN',
+				[
+					'app' => 'user_ldap',
+					'user' => $username,
+				]
+			);
+			return false;
+		}
 		$user = $this->access->userManager->get($dn);
 
 		if (!$user instanceof User) {

apps/user_ldap/tests/Mapping/AbstractMappingTestCase.php

@@ -9,18 +9,31 @@ declare(strict_types=1);
 namespace OCA\User_LDAP\Tests\Mapping;
 
 use OCA\User_LDAP\Mapping\AbstractMapping;
+use OCP\IAppConfig;
+use OCP\ICacheFactory;
 use OCP\IDBConnection;
 use OCP\Server;
+use PHPUnit\Framework\MockObject\MockObject;
 
 abstract class AbstractMappingTestCase extends \Test\TestCase {
-	abstract public function getMapper(IDBConnection $dbMock);
+
+	private ICacheFactory&MockObject $cacheFactoryMock;
+
+	private IAppConfig&MockObject $configMock;
+
+	abstract public function getMapper(IDBConnection $dbMock, ICacheFactory $cacheFactory, IAppConfig $appConfig): AbstractMapping;
+
+	protected function setUp(): void {
+		$this->cacheFactoryMock = $this->createMock(ICacheFactory::class);
+		$this->configMock = $this->createMock(IAppConfig::class);
+	}
 
 	/**
 	 * kiss test on isColNameValid
 	 */
 	public function testIsColNameValid(): void {
 		$dbMock = $this->createMock(IDBConnection::class);
-		$mapper = $this->getMapper($dbMock);
+		$mapper = $this->getMapper($dbMock, $this->cacheFactoryMock, $this->configMock);
 
 		$this->assertTrue($mapper->isColNameValid('ldap_dn'));
 		$this->assertFalse($mapper->isColNameValid('foobar'));
@@ -71,7 +84,7 @@ abstract class AbstractMappingTestCase extends \Test\TestCase {
 	 */
 	private function initTest(): array {
 		$dbc = Server::get(IDBConnection::class);
-		$mapper = $this->getMapper($dbc);
+		$mapper = $this->getMapper($dbc, $this->cacheFactoryMock, $this->configMock);
 		$data = $this->getTestData();
 		// make sure DB is pristine, then fill it with test entries
 		$mapper->clear();

apps/user_ldap/tests/Mapping/GroupMappingTest.php

@@ -9,6 +9,8 @@ declare(strict_types=1);
 namespace OCA\User_LDAP\Tests\Mapping;
 
 use OCA\User_LDAP\Mapping\GroupMapping;
+use OCP\IAppConfig;
+use OCP\ICacheFactory;
 use OCP\IDBConnection;
 
 /**
@@ -19,7 +21,7 @@ use OCP\IDBConnection;
  * @package OCA\User_LDAP\Tests\Mapping
  */
 class GroupMappingTest extends AbstractMappingTestCase {
-	public function getMapper(IDBConnection $dbMock) {
-		return new GroupMapping($dbMock);
+	public function getMapper(IDBConnection $dbMock, ICacheFactory $cacheFactory, IAppConfig $appConfig): GroupMapping {
+		return new GroupMapping($dbMock, $cacheFactory, $appConfig, true);
 	}
 }

apps/user_ldap/tests/Mapping/UserMappingTest.php

@@ -9,6 +9,8 @@ declare(strict_types=1);
 namespace OCA\User_LDAP\Tests\Mapping;
 
 use OCA\User_LDAP\Mapping\UserMapping;
+use OCP\IAppConfig;
+use OCP\ICacheFactory;
 use OCP\IDBConnection;
 use OCP\Support\Subscription\IAssertion;
 
@@ -20,7 +22,7 @@ use OCP\Support\Subscription\IAssertion;
  * @package OCA\User_LDAP\Tests\Mapping
  */
 class UserMappingTest extends AbstractMappingTestCase {
-	public function getMapper(IDBConnection $dbMock) {
-		return new UserMapping($dbMock, $this->createMock(IAssertion::class));
+	public function getMapper(IDBConnection $dbMock, ICacheFactory $cacheFactory, IAppConfig $appConfig): UserMapping {
+		return new UserMapping($dbMock, $cacheFactory, $appConfig, true, $this->createMock(IAssertion::class));
 	}
 }