Skip to content

Commit 29d5976

Browse files
blizzzblizzz
committed
feat: locally cache frequently requested LDAP mapping data
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
1 parent e0a21e5 commit 29d5976

File tree

6 files changed

+114
-20
lines changed

6 files changed

+114
-20
lines changed

‎apps/user_ldap/ajax/clearMappings.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ function (string $uid) use ($dispatcher): void {
4040
}
4141
);
4242
} elseif ($subject === 'group') {
43-
$mapping = new GroupMapping(Server::get(IDBConnection::class));
43+
$mapping = Server::get(GroupMapping::class);
4444
$result = $mapping->clear();
4545
}
4646

‎apps/user_ldap/lib/Mapping/AbstractMapping.php‎

Lines changed: 83 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,13 @@
1010
use Doctrine\DBAL\Exception;
1111
use OCP\DB\IPreparedStatement;
1212
use OCP\DB\QueryBuilder\IQueryBuilder;
13+
use OCP\IAppConfig;
14+
use OCP\ICache;
15+
use OCP\ICacheFactory;
1316
use OCP\IDBConnection;
1417
use OCP\Server;
1518
use Psr\Log\LoggerInterface;
19+
use function intdiv;
1620

1721
/**
1822
* Class AbstractMapping
@@ -27,16 +31,76 @@ abstract class AbstractMapping {
2731
*/
2832
abstract protected function getTableName(bool $includePrefix = true);
2933

34+
/**
35+
* A month worth of cache time for as good as never changing mapping data.
36+
* Implemented when it was found that name-to-DN lookups are quite frequent.
37+
*/
38+
protected const LOCAL_CACHE_TTL = 2592000;
39+
40+
/**
41+
* By default, the local cache is only used up to a certain amount of objects.
42+
* This constant holds this number. The amount of entries would amount up to
43+
* 1 MiB (worst case) per mappings table.
44+
* Setting `use_local_mapping_cache` for `user_ldap` to `yes` or `no`
45+
* deliberately enables or disables this mechanism.
46+
*/
47+
protected const LOCAL_CACHE_OBJECT_THRESHOLD = 2000;
48+
49+
protected ?ICache $localNameToDnCache = null;
50+
51+
/** @var array caches Names (value) by DN (key) */
52+
protected array $cache = [];
53+
3054
/**
3155
* @param IDBConnection $dbc
3256
*/
3357
public function __construct(
3458
protected IDBConnection $dbc,
59+
protected ICacheFactory $cacheFactory,
60+
protected IAppConfig $config,
61+
protected bool $isCLI,
3562
) {
63+
$this->initLocalCache();
3664
}
3765

38-
/** @var array caches Names (value) by DN (key) */
39-
protected $cache = [];
66+
protected function initLocalCache(): void {
67+
if ($this->isCLI || !$this->cacheFactory->isLocalCacheAvailable()) {
68+
return;
69+
}
70+
71+
$useLocalCache = $this->config->getValueString('user_ldap', 'use_local_mapping_cache', 'auto', false);
72+
if ($useLocalCache !== 'yes' && $useLocalCache !== 'auto') {
73+
return;
74+
}
75+
76+
$section = \str_contains($this->getTableName(), 'user') ? 'u/' : 'g/';
77+
$this->localNameToDnCache = $this->cacheFactory->createLocal('ldap/map/' . $section);
78+
79+
// We use the cache as well to store whether it shall be used. If the
80+
// answer was no, we unset it again.
81+
if ($useLocalCache === 'auto' && !$this->useCacheByUserCount()) {
82+
$this->localNameToDnCache = null;
83+
}
84+
}
85+
86+
protected function useCacheByUserCount(): bool {
87+
$use = $this->localNameToDnCache->get('use');
88+
if ($use !== null) {
89+
return $use;
90+
}
91+
92+
$qb = $this->dbc->getQueryBuilder();
93+
$q = $qb->selectAlias($qb->createFunction('COUNT(owncloud_name)'), 'count')
94+
->from($this->getTableName());
95+
$q->setMaxResults(self::LOCAL_CACHE_OBJECT_THRESHOLD + 1);
96+
$result = $q->executeQuery();
97+
$row = $result->fetch();
98+
$result->closeCursor();
99+
100+
$use = (int)$row['count'] <= self::LOCAL_CACHE_OBJECT_THRESHOLD;
101+
$this->localNameToDnCache->set('use', $use, intdiv(self::LOCAL_CACHE_TTL, 4));
102+
return $use;
103+
}
40104

41105
/**
42106
* checks whether a provided string represents an existing table col
@@ -107,17 +171,22 @@ protected function modify(IPreparedStatement $statement, $parameters) {
107171

108172
/**
109173
* Gets the LDAP DN based on the provided name.
110-
* Replaces Access::ocname2dn
111-
*
112-
* @param string $name
113-
* @return string|false
114174
*/
115-
public function getDNByName($name) {
116-
$dn = array_search($name, $this->cache);
117-
if ($dn === false && ($dn = $this->getXbyY('ldap_dn', 'owncloud_name', $name)) !== false) {
118-
$this->cache[$dn] = $name;
175+
public function getDNByName(string $name): string|false {
176+
$dn = array_search($name, $this->cache, true);
177+
if ($dn === false) {
178+
$dn = $this->localNameToDnCache?->get($name);
179+
if ($dn === null || $dn === false) {
180+
$dn = $this->getXbyY('ldap_dn', 'owncloud_name', $name);
181+
if ($dn === false) {
182+
return false;
183+
}
184+
185+
$this->cache[$dn] = $name;
186+
$this->localNameToDnCache?->set($name, $dn, self::LOCAL_CACHE_TTL);
187+
}
119188
}
120-
return $dn;
189+
return $dn ?? false;
121190
}
122191

123192
/**
@@ -128,6 +197,7 @@ public function getDNByName($name) {
128197
* @return bool
129198
*/
130199
public function setDNbyUUID($fdn, $uuid) {
200+
// FIXME: need to update local cache by name and broadcast this information
131201
$oldDn = $this->getDnByUUID($uuid);
132202
$statement = $this->dbc->prepare('
133203
UPDATE `' . $this->getTableName() . '`
@@ -374,6 +444,7 @@ public function unmap($name) {
374444
if ($dn !== false) {
375445
unset($this->cache[$dn]);
376446
}
447+
$this->localNameToDnCache?->remove($name);
377448

378449
return $this->modify($statement, [$name]);
379450
}
@@ -389,6 +460,7 @@ public function clear() {
389460
->getTruncateTableSQL('`' . $this->getTableName() . '`');
390461
try {
391462
$this->dbc->executeQuery($sql);
463+
$this->localNameToDnCache?->clear();
392464

393465
return true;
394466
} catch (Exception $e) {

‎apps/user_ldap/lib/Mapping/UserMapping.php‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@
88
namespace OCA\User_LDAP\Mapping;
99

1010
use OCP\HintException;
11+
use OCP\IAppConfig;
12+
use OCP\ICacheFactory;
1113
use OCP\IDBConnection;
1214
use OCP\IRequest;
1315
use OCP\Server;
@@ -24,9 +26,12 @@ class UserMapping extends AbstractMapping {
2426

2527
public function __construct(
2628
IDBConnection $dbc,
29+
ICacheFactory $cacheFactory,
30+
IAppConfig $config,
31+
bool $isCLI,
2732
private IAssertion $assertion,
2833
) {
29-
parent::__construct($dbc);
34+
parent::__construct($dbc, $cacheFactory, $config, $isCLI);
3035
}
3136

3237
/**

‎apps/user_ldap/tests/Mapping/AbstractMappingTestCase.php‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,18 +9,31 @@
99
namespace OCA\User_LDAP\Tests\Mapping;
1010

1111
use OCA\User_LDAP\Mapping\AbstractMapping;
12+
use OCP\IAppConfig;
13+
use OCP\ICacheFactory;
1214
use OCP\IDBConnection;
1315
use OCP\Server;
16+
use PHPUnit\Framework\MockObject\MockObject;
1417

1518
abstract class AbstractMappingTestCase extends \Test\TestCase {
16-
abstract public function getMapper(IDBConnection $dbMock);
19+
20+
private ICacheFactory|MockObject $cacheFactoryMock;
21+
22+
private IAppConfig|MockObject $configMock;
23+
24+
abstract public function getMapper(IDBConnection $dbMock, ICacheFactory $cacheFactory, IAppConfig $appConfig): AbstractMapping;
25+
26+
protected function setUp(): void {
27+
$this->cacheFactoryMock = $this->createMock(ICacheFactory::class);
28+
$this->configMock = $this->createMock(IAppConfig::class);
29+
}
1730

1831
/**
1932
* kiss test on isColNameValid
2033
*/
2134
public function testIsColNameValid(): void {
2235
$dbMock = $this->createMock(IDBConnection::class);
23-
$mapper = $this->getMapper($dbMock);
36+
$mapper = $this->getMapper($dbMock, $this->cacheFactoryMock, $this->configMock);
2437

2538
$this->assertTrue($mapper->isColNameValid('ldap_dn'));
2639
$this->assertFalse($mapper->isColNameValid('foobar'));
@@ -71,7 +84,7 @@ protected function mapEntries(AbstractMapping $mapper, array $data): void {
7184
*/
7285
private function initTest(): array {
7386
$dbc = Server::get(IDBConnection::class);
74-
$mapper = $this->getMapper($dbc);
87+
$mapper = $this->getMapper($dbc, $this->cacheFactoryMock, $this->configMock);
7588
$data = $this->getTestData();
7689
// make sure DB is pristine, then fill it with test entries
7790
$mapper->clear();

‎apps/user_ldap/tests/Mapping/GroupMappingTest.php‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@
99
namespace OCA\User_LDAP\Tests\Mapping;
1010

1111
use OCA\User_LDAP\Mapping\GroupMapping;
12+
use OCP\IAppConfig;
13+
use OCP\ICacheFactory;
1214
use OCP\IDBConnection;
1315

1416
/**
@@ -19,7 +21,7 @@
1921
* @package OCA\User_LDAP\Tests\Mapping
2022
*/
2123
class GroupMappingTest extends AbstractMappingTestCase {
22-
public function getMapper(IDBConnection $dbMock) {
23-
return new GroupMapping($dbMock);
24+
public function getMapper(IDBConnection $dbMock, ICacheFactory $cacheFactory, IAppConfig $appConfig): GroupMapping {
25+
return new GroupMapping($dbMock, $cacheFactory, $appConfig, true);
2426
}
2527
}

‎apps/user_ldap/tests/Mapping/UserMappingTest.php‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@
99
namespace OCA\User_LDAP\Tests\Mapping;
1010

1111
use OCA\User_LDAP\Mapping\UserMapping;
12+
use OCP\IAppConfig;
13+
use OCP\ICacheFactory;
1214
use OCP\IDBConnection;
1315
use OCP\Support\Subscription\IAssertion;
1416

@@ -20,7 +22,7 @@
2022
* @package OCA\User_LDAP\Tests\Mapping
2123
*/
2224
class UserMappingTest extends AbstractMappingTestCase {
23-
public function getMapper(IDBConnection $dbMock) {
24-
return new UserMapping($dbMock, $this->createMock(IAssertion::class));
25+
public function getMapper(IDBConnection $dbMock, ICacheFactory $cacheFactory, IAppConfig $appConfig): UserMapping {
26+
return new UserMapping($dbMock, $cacheFactory, $appConfig, true, $this->createMock(IAssertion::class));
2527
}
2628
}

0 commit comments

Comments
 (0)