Merge pull request #52916 from nextcloud/fix/cache-ldap-configuration-prefixes

fix(user_ldap): Store the list of used configuration prefixed in appconfig

apps/user_ldap/ajax/deleteConfiguration.php

@@ -1,8 +1,6 @@
 <?php
 
 use OCA\User_LDAP\Helper;
-use OCP\IConfig;
-use OCP\IDBConnection;
 use OCP\Server;
 use OCP\Util;
 
@@ -17,7 +15,7 @@ use OCP\Util;
 \OC_JSON::callCheck();
 
 $prefix = (string)$_POST['ldap_serverconfig_chooser'];
-$helper = new Helper(Server::get(IConfig::class), Server::get(IDBConnection::class));
+$helper = Server::get(Helper::class);
 if ($helper->deleteServerConfiguration($prefix)) {
 	\OC_JSON::success();
 } else {

apps/user_ldap/ajax/getNewServerConfigPrefix.php

@@ -2,8 +2,6 @@
 
 use OCA\User_LDAP\Configuration;
 use OCA\User_LDAP\Helper;
-use OCP\IConfig;
-use OCP\IDBConnection;
 use OCP\Server;
 
 /**
@@ -16,7 +14,7 @@ use OCP\Server;
 \OC_JSON::checkAppEnabled('user_ldap');
 \OC_JSON::callCheck();
 
-$helper = new Helper(Server::get(IConfig::class), Server::get(IDBConnection::class));
+$helper = Server::get(Helper::class);
 $serverConnections = $helper->getServerConfigurationPrefixes();
 sort($serverConnections);
 $lk = array_pop($serverConnections);

apps/user_ldap/lib/Command/Search.php

@@ -12,7 +12,6 @@ use OCA\User_LDAP\Helper;
 use OCA\User_LDAP\LDAP;
 use OCA\User_LDAP\User_Proxy;
 use OCP\IConfig;
-use OCP\IDBConnection;
 use OCP\Server;
 
 use Symfony\Component\Console\Command\Command;
@@ -83,7 +82,7 @@ class Search extends Command {
 	}
 
 	protected function execute(InputInterface $input, OutputInterface $output): int {
-		$helper = new Helper($this->ocConfig, Server::get(IDBConnection::class));
+		$helper = Server::get(Helper::class);
 		$configPrefixes = $helper->getServerConfigurationPrefixes(true);
 		$ldapWrapper = new LDAP();
 

apps/user_ldap/lib/Command/SetConfig.php

@@ -11,8 +11,6 @@ use OCA\User_LDAP\Configuration;
 use OCA\User_LDAP\ConnectionFactory;
 use OCA\User_LDAP\Helper;
 use OCA\User_LDAP\LDAP;
-use OCP\IConfig;
-use OCP\IDBConnection;
 use OCP\Server;
 use Symfony\Component\Console\Command\Command;
 use Symfony\Component\Console\Input\InputArgument;
@@ -43,7 +41,7 @@ class SetConfig extends Command {
 	}
 
 	protected function execute(InputInterface $input, OutputInterface $output): int {
-		$helper = new Helper(Server::get(IConfig::class), Server::get(IDBConnection::class));
+		$helper = Server::get(Helper::class);
 		$availableConfigs = $helper->getServerConfigurationPrefixes();
 		$configID = $input->getArgument('configID');
 		if (!in_array($configID, $availableConfigs)) {

apps/user_ldap/lib/Connection.php

@@ -11,8 +11,6 @@ use OC\ServerNotAvailableException;
 use OCA\User_LDAP\Exceptions\ConfigurationIssueException;
 use OCP\ICache;
 use OCP\ICacheFactory;
-use OCP\IConfig;
-use OCP\IDBConnection;
 use OCP\IL10N;
 use OCP\Server;
 use OCP\Util;
@@ -156,7 +154,7 @@ class Connection extends LDAPUtility {
 		if ($memcache->isAvailable()) {
 			$this->cache = $memcache->createDistributed();
 		}
-		$helper = new Helper(Server::get(IConfig::class), Server::get(IDBConnection::class));
+		$helper = Server::get(Helper::class);
 		$this->doNotValidate = !in_array($this->configPrefix,
 			$helper->getServerConfigurationPrefixes());
 		$this->logger = Server::get(LoggerInterface::class);

apps/user_ldap/lib/Helper.php

@@ -9,7 +9,7 @@ namespace OCA\User_LDAP;
 
 use OCP\Cache\CappedMemoryCache;
 use OCP\DB\QueryBuilder\IQueryBuilder;
-use OCP\IConfig;
+use OCP\IAppConfig;
 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->getValueString('user_ldap', $prefix . 'ldap_configuration_active') === '1')
+		));
+	}
+
+	protected function getAllServerConfigurationPrefixes(): array {
+		$unfilled = ['UNFILLED'];
+		$prefixes = $this->appConfig->getValueArray('user_ldap', '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->setValueArray('user_ldap', '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<string,string> 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->getValueString('user_ldap', $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->setValueArray('user_ldap', 'configuration_prefixes', $prefixes);
+		return $prefix;
 	}
 
 	private function getServersConfig(string $value): array {
 		$regex = '/' . $value . '$/S';
 
-		$keys = $this->config->getAppKeys('user_ldap');
+		$keys = $this->appConfig->getKeys('user_ldap');
 		$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->setValueArray('user_ldap', '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->getValueString('user_ldap', $prefix . 'ldap_configuration_active') !== '1') {
+				return true;
+			}
+		}
+		return false;
 	}
 
 	/**

apps/user_ldap/lib/Jobs/CleanUp.php

@@ -67,7 +67,7 @@ class CleanUp extends TimedJob {
 		if (isset($arguments['helper'])) {
 			$this->ldapHelper = $arguments['helper'];
 		} else {
-			$this->ldapHelper = new Helper(Server::get(IConfig::class), Server::get(IDBConnection::class));
+			$this->ldapHelper = Server::get(Helper::class);
 		}
 
 		if (isset($arguments['ocConfig'])) {

apps/user_ldap/lib/Settings/Admin.php

@@ -8,8 +8,6 @@ namespace OCA\User_LDAP\Settings;
 use OCA\User_LDAP\Configuration;
 use OCA\User_LDAP\Helper;
 use OCP\AppFramework\Http\TemplateResponse;
-use OCP\IConfig;
-use OCP\IDBConnection;
 use OCP\IL10N;
 use OCP\Server;
 use OCP\Settings\IDelegatedSettings;
@@ -26,7 +24,7 @@ class Admin implements IDelegatedSettings {
 	 * @return TemplateResponse
 	 */
 	public function getForm() {
-		$helper = new Helper(Server::get(IConfig::class), Server::get(IDBConnection::class));
+		$helper = Server::get(Helper::class);
 		$prefixes = $helper->getServerConfigurationPrefixes();
 		if (count($prefixes) === 0) {
 			$newPrefix = $helper->getNextServerConfigurationPrefix();

apps/user_ldap/tests/AccessTest.php

@@ -25,7 +25,6 @@ use OCP\HintException;
 use OCP\IAppConfig;
 use OCP\IAvatarManager;
 use OCP\IConfig;
-use OCP\IDBConnection;
 use OCP\Image;
 use OCP\IUserManager;
 use OCP\Notification\IManager as INotificationManager;
@@ -110,7 +109,7 @@ class AccessTest extends TestCase {
 				$this->createMock(INotificationManager::class),
 				$this->shareManager])
 			->getMock();
-		$helper = new Helper(Server::get(IConfig::class), Server::get(IDBConnection::class));
+		$helper = Server::get(Helper::class);
 
 		return [$lw, $connector, $um, $helper];
 	}

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\IAppConfig;
 use OCP\IDBConnection;
 use OCP\Server;
 use PHPUnit\Framework\MockObject\MockObject;
@@ -17,45 +17,53 @@ 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('getKeys')
+			->with('user_ldap')
 			->willReturn([
 				'foo',
 				'ldap_configuration_active',
 				's1ldap_configuration_active',
 			]);
 
+		$this->appConfig->method('getValueArray')
+			->with('user_ldap', 'configuration_prefixes')
+			-> willReturnArgument(2);
+
 		$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('getKeys')
+			->with('user_ldap')
 			->willReturn([
 				'foo',
 				'ldap_configuration_active',
 				's1ldap_configuration_active',
 			]);
 
-		$this->config->method('getAppValue')
+		$this->appConfig->method('getValueArray')
+			->with('user_ldap', 'configuration_prefixes')
+			-> willReturnArgument(2);
+
+		$this->appConfig->method('getValueString')
 			->willReturnCallback(function ($app, $key, $default) {
-				if ($app !== 'user_ldap') {
-					$this->fail('wrong app');
-				}
 				if ($key === 's1ldap_configuration_active') {
 					return '1';
 				}
@@ -67,21 +75,58 @@ 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('getKeys')
+			->with('user_ldap')
 			->willReturn([
 				'foo',
 				'ldap_host',
 				's1ldap_host',
 				's02ldap_host',
+				'ldap_configuration_active',
+				's1ldap_configuration_active',
+				's02ldap_configuration_active',
 			]);
 
-		$this->config->method('getAppValue')
+		$this->appConfig->method('getValueArray')
+			->with('user_ldap', 'configuration_prefixes')
+			-> willReturnArgument(2);
+
+		$this->appConfig->method('getValueString')
 			->willReturnCallback(function ($app, $key, $default) {
-				if ($app !== 'user_ldap') {
-					$this->fail('wrong app');
+				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('getKeys');
+
+		$this->appConfig->method('getValueArray')
+			->with('user_ldap', 'configuration_prefixes')
+			-> willReturn([
+				'',
+				's1',
+				's02',
+			]);
+
+		$this->appConfig->method('getValueString')
+			->willReturnCallback(function ($app, $key, $default) {
 				if ($key === 'ldap_host') {
 					return 'example.com';
 				}

apps/user_ldap/tests/Integration/AbstractIntegrationTest.php

@@ -16,7 +16,6 @@ use OCA\User_LDAP\User\Manager;
 use OCA\User_LDAP\UserPluginManager;
 use OCP\IAvatarManager;
 use OCP\IConfig;
-use OCP\IDBConnection;
 use OCP\Image;
 use OCP\IUserManager;
 use OCP\Server;
@@ -125,7 +124,7 @@ abstract class AbstractIntegrationTest {
 	 * initializes the test Helper
 	 */
 	protected function initHelper() {
-		$this->helper = new Helper(Server::get(IConfig::class), Server::get(IDBConnection::class));
+		$this->helper = Server::get(Helper::class);
 	}
 
 	/**

apps/user_ldap/tests/LDAPProviderTest.php

@@ -21,7 +21,6 @@ use OCA\User_LDAP\User_LDAP;
 use OCP\EventDispatcher\IEventDispatcher;
 use OCP\ICacheFactory;
 use OCP\IConfig;
-use OCP\IDBConnection;
 use OCP\IServerContainer;
 use OCP\Server;
 use Psr\Log\LoggerInterface;
@@ -199,7 +198,7 @@ class LDAPProviderTest extends \Test\TestCase {
 
 		$server = $this->getServerMock($userBackend, $this->getDefaultGroupBackendMock());
 
-		$helper = new Helper(Server::get(IConfig::class), Server::get(IDBConnection::class));
+		$helper = Server::get(Helper::class);
 
 		$ldapProvider = $this->getLDAPProvider($server);
 		$this->assertEquals(
@@ -212,7 +211,7 @@ class LDAPProviderTest extends \Test\TestCase {
 
 		$server = $this->getServerMock($userBackend, $this->getDefaultGroupBackendMock());
 
-		$helper = new Helper(Server::get(IConfig::class), Server::get(IDBConnection::class));
+		$helper = Server::get(Helper::class);
 
 		$ldapProvider = $this->getLDAPProvider($server);
 		$this->assertEquals(

build/psalm-baseline.xml

@@ -2597,14 +2597,6 @@
       <code><![CDATA[$gid]]></code>
     </ParamNameMismatch>
   </file>
-  <file src="apps/user_ldap/lib/Helper.php">
-    <DeprecatedMethod>
-      <code><![CDATA[execute]]></code>
-      <code><![CDATA[getAppKeys]]></code>
-      <code><![CDATA[getAppValue]]></code>
-      <code><![CDATA[getAppValue]]></code>
-    </DeprecatedMethod>
-  </file>
   <file src="apps/user_ldap/lib/Jobs/CleanUp.php">
     <DeprecatedMethod>
       <code><![CDATA[getAppValue]]></code>