From bc7309ca1cb6bc93f05359c84acfe637d2ab25d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Fri, 16 May 2025 19:13:40 +0200 Subject: [PATCH] fix(user_ldap): Store the list of used configuration prefixed in appconfig MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This avoids getting all keys from appconfig, which was triggering loading of lazy configuration on all requests. Signed-off-by: Côme Chilliet --- apps/user_ldap/lib/Helper.php | 96 ++++++++++++++++++----------- apps/user_ldap/tests/HelperTest.php | 82 ++++++++++++++++++------ 2 files changed, 122 insertions(+), 56 deletions(-) diff --git a/apps/user_ldap/lib/Helper.php b/apps/user_ldap/lib/Helper.php index 27c19e6c4f5..243b2c3ade8 100644 --- a/apps/user_ldap/lib/Helper.php +++ b/apps/user_ldap/lib/Helper.php @@ -7,9 +7,9 @@ */ namespace OCA\User_LDAP; +use OCP\AppFramework\Services\IAppConfig; use OCP\Cache\CappedMemoryCache; use OCP\DB\QueryBuilder\IQueryBuilder; -use OCP\IConfig; use OCP\IDBConnection; use OCP\Server; @@ -18,7 +18,7 @@ class Helper { protected CappedMemoryCache $sanitizeDnCache; public function __construct( - private IConfig $config, + private IAppConfig $appConfig, private IDBConnection $connection, ) { $this->sanitizeDnCache = new CappedMemoryCache(10000); @@ -45,21 +45,37 @@ class Helper { * except the default (first) server shall be connected to. * */ - public function getServerConfigurationPrefixes($activeConfigurations = false): array { + public function getServerConfigurationPrefixes(bool $activeConfigurations = false): array { + $all = $this->getAllServerConfigurationPrefixes(); + if (!$activeConfigurations) { + return $all; + } + return array_values(array_filter( + $all, + fn (string $prefix): bool => ($this->appConfig->getAppValueString($prefix . 'ldap_configuration_active') === '1') + )); + } + + protected function getAllServerConfigurationPrefixes(): array { + $unfilled = ['UNFILLED']; + $prefixes = $this->appConfig->getAppValueArray('configuration_prefixes', $unfilled); + if ($prefixes !== $unfilled) { + return $prefixes; + } + + /* Fallback to browsing key for migration from Nextcloud<32 */ $referenceConfigkey = 'ldap_configuration_active'; $keys = $this->getServersConfig($referenceConfigkey); $prefixes = []; foreach ($keys as $key) { - if ($activeConfigurations && $this->config->getAppValue('user_ldap', $key, '0') !== '1') { - continue; - } - $len = strlen($key) - strlen($referenceConfigkey); $prefixes[] = substr($key, 0, $len); } - asort($prefixes); + sort($prefixes); + + $this->appConfig->setAppValueArray('configuration_prefixes', $prefixes); return $prefixes; } @@ -68,46 +84,45 @@ class Helper { * * determines the host for every configured connection * - * @return array an array with configprefix as keys + * @return array an array with configprefix as keys * */ - public function getServerConfigurationHosts() { - $referenceConfigkey = 'ldap_host'; - - $keys = $this->getServersConfig($referenceConfigkey); + public function getServerConfigurationHosts(): array { + $prefixes = $this->getServerConfigurationPrefixes(); + $referenceConfigkey = 'ldap_host'; $result = []; - foreach ($keys as $key) { - $len = strlen($key) - strlen($referenceConfigkey); - $prefix = substr($key, 0, $len); - $result[$prefix] = $this->config->getAppValue('user_ldap', $key); + foreach ($prefixes as $prefix) { + $result[$prefix] = $this->appConfig->getAppValueString($prefix . $referenceConfigkey); } return $result; } /** - * return the next available configuration prefix - * - * @return string + * return the next available configuration prefix and register it as used */ - public function getNextServerConfigurationPrefix() { - $serverConnections = $this->getServerConfigurationPrefixes(); - - if (count($serverConnections) === 0) { - return 's01'; + public function getNextServerConfigurationPrefix(): string { + $prefixes = $this->getServerConfigurationPrefixes(); + + if (count($prefixes) === 0) { + $prefix = 's01'; + } else { + sort($prefixes); + $lastKey = array_pop($prefixes); + $lastNumber = (int)str_replace('s', '', $lastKey); + $prefix = 's' . str_pad((string)($lastNumber + 1), 2, '0', STR_PAD_LEFT); } - sort($serverConnections); - $lastKey = array_pop($serverConnections); - $lastNumber = (int)str_replace('s', '', $lastKey); - return 's' . str_pad((string)($lastNumber + 1), 2, '0', STR_PAD_LEFT); + $prefixes[] = $prefix; + $this->appConfig->setAppValueArray('configuration_prefixes', $prefixes); + return $prefix; } private function getServersConfig(string $value): array { $regex = '/' . $value . '$/S'; - $keys = $this->config->getAppKeys('user_ldap'); + $keys = $this->appConfig->getAppKeys(); $result = []; foreach ($keys as $key) { if (preg_match($regex, $key) === 1) { @@ -125,7 +140,9 @@ class Helper { * @return bool true on success, false otherwise */ public function deleteServerConfiguration($prefix) { - if (!in_array($prefix, self::getServerConfigurationPrefixes())) { + $prefixes = $this->getServerConfigurationPrefixes(); + $index = array_search($prefix, $prefixes); + if ($index === false) { return false; } @@ -144,7 +161,11 @@ class Helper { $query->andWhere($query->expr()->notLike('configkey', $query->createNamedParameter('s%'))); } - $deletedRows = $query->execute(); + $deletedRows = $query->executeStatement(); + + unset($prefixes[$index]); + $this->appConfig->setAppValueArray('configuration_prefixes', array_values($prefixes)); + return $deletedRows !== 0; } @@ -152,10 +173,13 @@ class Helper { * checks whether there is one or more disabled LDAP configurations */ public function haveDisabledConfigurations(): bool { - $all = $this->getServerConfigurationPrefixes(false); - $active = $this->getServerConfigurationPrefixes(true); - - return count($all) !== count($active) || count($all) === 0; + $all = $this->getServerConfigurationPrefixes(); + foreach ($all as $prefix) { + if ($this->appConfig->getAppValueString($prefix . 'ldap_configuration_active') !== '1') { + return true; + } + } + return false; } /** diff --git a/apps/user_ldap/tests/HelperTest.php b/apps/user_ldap/tests/HelperTest.php index 470b67c5531..6cfa1a89450 100644 --- a/apps/user_ldap/tests/HelperTest.php +++ b/apps/user_ldap/tests/HelperTest.php @@ -8,7 +8,7 @@ declare(strict_types=1); namespace OCA\User_LDAP\Tests; use OCA\User_LDAP\Helper; -use OCP\IConfig; +use OCP\AppFramework\Services\IAppConfig; use OCP\IDBConnection; use OCP\Server; use PHPUnit\Framework\MockObject\MockObject; @@ -17,45 +17,51 @@ use PHPUnit\Framework\MockObject\MockObject; * @group DB */ class HelperTest extends \Test\TestCase { - private IConfig&MockObject $config; + private IAppConfig&MockObject $appConfig; private Helper $helper; protected function setUp(): void { parent::setUp(); - $this->config = $this->createMock(IConfig::class); - $this->helper = new Helper($this->config, Server::get(IDBConnection::class)); + $this->appConfig = $this->createMock(IAppConfig::class); + $this->helper = new Helper( + $this->appConfig, + Server::get(IDBConnection::class) + ); } public function testGetServerConfigurationPrefixes(): void { - $this->config->method('getAppKeys') - ->with($this->equalTo('user_ldap')) + $this->appConfig->method('getAppKeys') ->willReturn([ 'foo', 'ldap_configuration_active', 's1ldap_configuration_active', ]); + $this->appConfig->method('getAppValueArray') + ->with('configuration_prefixes') + -> willReturnArgument(1); + $result = $this->helper->getServerConfigurationPrefixes(false); $this->assertEquals(['', 's1'], $result); } public function testGetServerConfigurationPrefixesActive(): void { - $this->config->method('getAppKeys') - ->with($this->equalTo('user_ldap')) + $this->appConfig->method('getAppKeys') ->willReturn([ 'foo', 'ldap_configuration_active', 's1ldap_configuration_active', ]); - $this->config->method('getAppValue') - ->willReturnCallback(function ($app, $key, $default) { - if ($app !== 'user_ldap') { - $this->fail('wrong app'); - } + $this->appConfig->method('getAppValueArray') + ->with('configuration_prefixes') + -> willReturnArgument(1); + + $this->appConfig->method('getAppValueString') + ->willReturnCallback(function ($key, $default) { if ($key === 's1ldap_configuration_active') { return '1'; } @@ -67,21 +73,57 @@ class HelperTest extends \Test\TestCase { $this->assertEquals(['s1'], $result); } - public function testGetServerConfigurationHost(): void { - $this->config->method('getAppKeys') - ->with($this->equalTo('user_ldap')) + public function testGetServerConfigurationHostFromAppKeys(): void { + $this->appConfig->method('getAppKeys') ->willReturn([ 'foo', 'ldap_host', 's1ldap_host', 's02ldap_host', + 'ldap_configuration_active', + 's1ldap_configuration_active', + 's02ldap_configuration_active', ]); - $this->config->method('getAppValue') - ->willReturnCallback(function ($app, $key, $default) { - if ($app !== 'user_ldap') { - $this->fail('wrong app'); + $this->appConfig->method('getAppValueArray') + ->with('configuration_prefixes') + -> willReturnArgument(1); + + $this->appConfig->method('getAppValueString') + ->willReturnCallback(function ($key, $default) { + if ($key === 'ldap_host') { + return 'example.com'; } + if ($key === 's1ldap_host') { + return 'foo.bar.com'; + } + return $default; + }); + + $result = $this->helper->getServerConfigurationHosts(); + + $this->assertEquals([ + '' => 'example.com', + 's1' => 'foo.bar.com', + 's02' => '', + ], $result); + } + + public function testGetServerConfigurationHost(): void { + $this->appConfig + ->expects(self::never()) + ->method('getAppKeys'); + + $this->appConfig->method('getAppValueArray') + ->with('configuration_prefixes') + -> willReturn([ + '', + 's1', + 's02', + ]); + + $this->appConfig->method('getAppValueString') + ->willReturnCallback(function ($key, $default) { if ($key === 'ldap_host') { return 'example.com'; }