diff --git a/apps/user_ldap/ajax/clearMappings.php b/apps/user_ldap/ajax/clearMappings.php index c3b64f08a16fd..eb2ac7f81f233 100644 --- a/apps/user_ldap/ajax/clearMappings.php +++ b/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 @@ function (string $uid) use ($dispatcher): void { } ); } elseif ($subject === 'group') { - $mapping = new GroupMapping(Server::get(IDBConnection::class)); + $mapping = Server::get(GroupMapping::class); $result = $mapping->clear(); } diff --git a/apps/user_ldap/lib/Mapping/AbstractMapping.php b/apps/user_ldap/lib/Mapping/AbstractMapping.php index fa10312a915f4..fdcc15535ead8 100644 --- a/apps/user_ldap/lib/Mapping/AbstractMapping.php +++ b/apps/user_ldap/lib/Mapping/AbstractMapping.php @@ -10,6 +10,9 @@ 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 @@ protected function getXbyY($fetchCol, $compareCol, $search) { //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 @@ protected function modify(IPreparedStatement $statement, $parameters) { /** * 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 @@ public function setDNbyUUID($fdn, $uuid) { '); $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 @@ public function map($fdn, $name, $uuid) { $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 @@ public function unmap($name) { if ($dn !== false) { unset($this->cache[$dn]); } + $this->localNameToDnCache?->remove($name); return $this->modify($statement, [$name]); } @@ -389,6 +467,7 @@ public function clear() { ->getTruncateTableSQL('`' . $this->getTableName() . '`'); try { $this->dbc->executeQuery($sql); + $this->localNameToDnCache?->clear(); return true; } catch (Exception $e) { diff --git a/apps/user_ldap/lib/Mapping/UserMapping.php b/apps/user_ldap/lib/Mapping/UserMapping.php index a030cd0ab52c6..b2c97df3f9c8b 100644 --- a/apps/user_ldap/lib/Mapping/UserMapping.php +++ b/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); } /** diff --git a/apps/user_ldap/lib/User_LDAP.php b/apps/user_ldap/lib/User_LDAP.php index c3f56f5ff9b47..708aeb037fd2b 100644 --- a/apps/user_ldap/lib/User_LDAP.php +++ b/apps/user_ldap/lib/User_LDAP.php @@ -70,8 +70,9 @@ public function canChangeAvatar($uid) { 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 @@ public function checkPassword($uid, $password) { 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) { diff --git a/apps/user_ldap/tests/Mapping/AbstractMappingTestCase.php b/apps/user_ldap/tests/Mapping/AbstractMappingTestCase.php index 8efee4e20859b..8a0ec8a0f9eff 100644 --- a/apps/user_ldap/tests/Mapping/AbstractMappingTestCase.php +++ b/apps/user_ldap/tests/Mapping/AbstractMappingTestCase.php @@ -9,18 +9,31 @@ 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 @@ protected function mapEntries(AbstractMapping $mapper, array $data): void { */ 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(); diff --git a/apps/user_ldap/tests/Mapping/GroupMappingTest.php b/apps/user_ldap/tests/Mapping/GroupMappingTest.php index 5729058d10e20..df64149577f95 100644 --- a/apps/user_ldap/tests/Mapping/GroupMappingTest.php +++ b/apps/user_ldap/tests/Mapping/GroupMappingTest.php @@ -9,6 +9,8 @@ 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 @@ * @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); } } diff --git a/apps/user_ldap/tests/Mapping/UserMappingTest.php b/apps/user_ldap/tests/Mapping/UserMappingTest.php index 4346fe1d23f77..c79a5fa77e9bc 100644 --- a/apps/user_ldap/tests/Mapping/UserMappingTest.php +++ b/apps/user_ldap/tests/Mapping/UserMappingTest.php @@ -9,6 +9,8 @@ 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 @@ * @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)); } }