Skip to content

Commit bc7309c

Browse files
come-nccome-nc
committed
fix(user_ldap): Store the list of used configuration prefixed in appconfig
This avoids getting all keys from appconfig, which was triggering loading of lazy configuration on all requests. Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
1 parent eb9a621 commit bc7309c

2 files changed

Lines changed: 122 additions & 56 deletions

File tree

‎apps/user_ldap/lib/Helper.php‎

Lines changed: 60 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,9 @@
77
*/
88
namespace OCA\User_LDAP;
99

10+
use OCP\AppFramework\Services\IAppConfig;
1011
use OCP\Cache\CappedMemoryCache;
1112
use OCP\DB\QueryBuilder\IQueryBuilder;
12-
use OCP\IConfig;
1313
use OCP\IDBConnection;
1414
use OCP\Server;
1515

@@ -18,7 +18,7 @@ class Helper {
1818
protected CappedMemoryCache $sanitizeDnCache;
1919

2020
public function __construct(
21-
private IConfig $config,
21+
private IAppConfig $appConfig,
2222
private IDBConnection $connection,
2323
) {
2424
$this->sanitizeDnCache = new CappedMemoryCache(10000);
@@ -45,21 +45,37 @@ public function __construct(
4545
* except the default (first) server shall be connected to.
4646
*
4747
*/
48-
public function getServerConfigurationPrefixes($activeConfigurations = false): array {
48+
public function getServerConfigurationPrefixes(bool $activeConfigurations = false): array {
49+
$all = $this->getAllServerConfigurationPrefixes();
50+
if (!$activeConfigurations) {
51+
return $all;
52+
}
53+
return array_values(array_filter(
54+
$all,
55+
fn (string $prefix): bool => ($this->appConfig->getAppValueString($prefix . 'ldap_configuration_active') === '1')
56+
));
57+
}
58+
59+
protected function getAllServerConfigurationPrefixes(): array {
60+
$unfilled = ['UNFILLED'];
61+
$prefixes = $this->appConfig->getAppValueArray('configuration_prefixes', $unfilled);
62+
if ($prefixes !== $unfilled) {
63+
return $prefixes;
64+
}
65+
66+
/* Fallback to browsing key for migration from Nextcloud<32 */
4967
$referenceConfigkey = 'ldap_configuration_active';
5068

5169
$keys = $this->getServersConfig($referenceConfigkey);
5270

5371
$prefixes = [];
5472
foreach ($keys as $key) {
55-
if ($activeConfigurations && $this->config->getAppValue('user_ldap', $key, '0') !== '1') {
56-
continue;
57-
}
58-
5973
$len = strlen($key) - strlen($referenceConfigkey);
6074
$prefixes[] = substr($key, 0, $len);
6175
}
62-
asort($prefixes);
76+
sort($prefixes);
77+
78+
$this->appConfig->setAppValueArray('configuration_prefixes', $prefixes);
6379

6480
return $prefixes;
6581
}
@@ -68,46 +84,45 @@ public function getServerConfigurationPrefixes($activeConfigurations = false): a
6884
*
6985
* determines the host for every configured connection
7086
*
71-
* @return array an array with configprefix as keys
87+
* @return array<string,string> an array with configprefix as keys
7288
*
7389
*/
74-
public function getServerConfigurationHosts() {
75-
$referenceConfigkey = 'ldap_host';
76-
77-
$keys = $this->getServersConfig($referenceConfigkey);
90+
public function getServerConfigurationHosts(): array {
91+
$prefixes = $this->getServerConfigurationPrefixes();
7892

93+
$referenceConfigkey = 'ldap_host';
7994
$result = [];
80-
foreach ($keys as $key) {
81-
$len = strlen($key) - strlen($referenceConfigkey);
82-
$prefix = substr($key, 0, $len);
83-
$result[$prefix] = $this->config->getAppValue('user_ldap', $key);
95+
foreach ($prefixes as $prefix) {
96+
$result[$prefix] = $this->appConfig->getAppValueString($prefix . $referenceConfigkey);
8497
}
8598

8699
return $result;
87100
}
88101

89102
/**
90-
* return the next available configuration prefix
91-
*
92-
* @return string
103+
* return the next available configuration prefix and register it as used
93104
*/
94-
public function getNextServerConfigurationPrefix() {
95-
$serverConnections = $this->getServerConfigurationPrefixes();
96-
97-
if (count($serverConnections) === 0) {
98-
return 's01';
105+
public function getNextServerConfigurationPrefix(): string {
106+
$prefixes = $this->getServerConfigurationPrefixes();
107+
108+
if (count($prefixes) === 0) {
109+
$prefix = 's01';
110+
} else {
111+
sort($prefixes);
112+
$lastKey = array_pop($prefixes);
113+
$lastNumber = (int)str_replace('s', '', $lastKey);
114+
$prefix = 's' . str_pad((string)($lastNumber + 1), 2, '0', STR_PAD_LEFT);
99115
}
100116

101-
sort($serverConnections);
102-
$lastKey = array_pop($serverConnections);
103-
$lastNumber = (int)str_replace('s', '', $lastKey);
104-
return 's' . str_pad((string)($lastNumber + 1), 2, '0', STR_PAD_LEFT);
117+
$prefixes[] = $prefix;
118+
$this->appConfig->setAppValueArray('configuration_prefixes', $prefixes);
119+
return $prefix;
105120
}
106121

107122
private function getServersConfig(string $value): array {
108123
$regex = '/' . $value . '$/S';
109124

110-
$keys = $this->config->getAppKeys('user_ldap');
125+
$keys = $this->appConfig->getAppKeys();
111126
$result = [];
112127
foreach ($keys as $key) {
113128
if (preg_match($regex, $key) === 1) {
@@ -125,7 +140,9 @@ private function getServersConfig(string $value): array {
125140
* @return bool true on success, false otherwise
126141
*/
127142
public function deleteServerConfiguration($prefix) {
128-
if (!in_array($prefix, self::getServerConfigurationPrefixes())) {
143+
$prefixes = $this->getServerConfigurationPrefixes();
144+
$index = array_search($prefix, $prefixes);
145+
if ($index === false) {
129146
return false;
130147
}
131148

@@ -144,18 +161,25 @@ public function deleteServerConfiguration($prefix) {
144161
$query->andWhere($query->expr()->notLike('configkey', $query->createNamedParameter('s%')));
145162
}
146163

147-
$deletedRows = $query->execute();
164+
$deletedRows = $query->executeStatement();
165+
166+
unset($prefixes[$index]);
167+
$this->appConfig->setAppValueArray('configuration_prefixes', array_values($prefixes));
168+
148169
return $deletedRows !== 0;
149170
}
150171

151172
/**
152173
* checks whether there is one or more disabled LDAP configurations
153174
*/
154175
public function haveDisabledConfigurations(): bool {
155-
$all = $this->getServerConfigurationPrefixes(false);
156-
$active = $this->getServerConfigurationPrefixes(true);
157-
158-
return count($all) !== count($active) || count($all) === 0;
176+
$all = $this->getServerConfigurationPrefixes();
177+
foreach ($all as $prefix) {
178+
if ($this->appConfig->getAppValueString($prefix . 'ldap_configuration_active') !== '1') {
179+
return true;
180+
}
181+
}
182+
return false;
159183
}
160184

161185
/**

‎apps/user_ldap/tests/HelperTest.php‎

Lines changed: 62 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
namespace OCA\User_LDAP\Tests;
99

1010
use OCA\User_LDAP\Helper;
11-
use OCP\IConfig;
11+
use OCP\AppFramework\Services\IAppConfig;
1212
use OCP\IDBConnection;
1313
use OCP\Server;
1414
use PHPUnit\Framework\MockObject\MockObject;
@@ -17,45 +17,51 @@
1717
* @group DB
1818
*/
1919
class HelperTest extends \Test\TestCase {
20-
private IConfig&MockObject $config;
20+
private IAppConfig&MockObject $appConfig;
2121

2222
private Helper $helper;
2323

2424
protected function setUp(): void {
2525
parent::setUp();
2626

27-
$this->config = $this->createMock(IConfig::class);
28-
$this->helper = new Helper($this->config, Server::get(IDBConnection::class));
27+
$this->appConfig = $this->createMock(IAppConfig::class);
28+
$this->helper = new Helper(
29+
$this->appConfig,
30+
Server::get(IDBConnection::class)
31+
);
2932
}
3033

3134
public function testGetServerConfigurationPrefixes(): void {
32-
$this->config->method('getAppKeys')
33-
->with($this->equalTo('user_ldap'))
35+
$this->appConfig->method('getAppKeys')
3436
->willReturn([
3537
'foo',
3638
'ldap_configuration_active',
3739
's1ldap_configuration_active',
3840
]);
3941

42+
$this->appConfig->method('getAppValueArray')
43+
->with('configuration_prefixes')
44+
-> willReturnArgument(1);
45+
4046
$result = $this->helper->getServerConfigurationPrefixes(false);
4147

4248
$this->assertEquals(['', 's1'], $result);
4349
}
4450

4551
public function testGetServerConfigurationPrefixesActive(): void {
46-
$this->config->method('getAppKeys')
47-
->with($this->equalTo('user_ldap'))
52+
$this->appConfig->method('getAppKeys')
4853
->willReturn([
4954
'foo',
5055
'ldap_configuration_active',
5156
's1ldap_configuration_active',
5257
]);
5358

54-
$this->config->method('getAppValue')
55-
->willReturnCallback(function ($app, $key, $default) {
56-
if ($app !== 'user_ldap') {
57-
$this->fail('wrong app');
58-
}
59+
$this->appConfig->method('getAppValueArray')
60+
->with('configuration_prefixes')
61+
-> willReturnArgument(1);
62+
63+
$this->appConfig->method('getAppValueString')
64+
->willReturnCallback(function ($key, $default) {
5965
if ($key === 's1ldap_configuration_active') {
6066
return '1';
6167
}
@@ -67,21 +73,57 @@ public function testGetServerConfigurationPrefixesActive(): void {
6773
$this->assertEquals(['s1'], $result);
6874
}
6975

70-
public function testGetServerConfigurationHost(): void {
71-
$this->config->method('getAppKeys')
72-
->with($this->equalTo('user_ldap'))
76+
public function testGetServerConfigurationHostFromAppKeys(): void {
77+
$this->appConfig->method('getAppKeys')
7378
->willReturn([
7479
'foo',
7580
'ldap_host',
7681
's1ldap_host',
7782
's02ldap_host',
83+
'ldap_configuration_active',
84+
's1ldap_configuration_active',
85+
's02ldap_configuration_active',
7886
]);
7987

80-
$this->config->method('getAppValue')
81-
->willReturnCallback(function ($app, $key, $default) {
82-
if ($app !== 'user_ldap') {
83-
$this->fail('wrong app');
88+
$this->appConfig->method('getAppValueArray')
89+
->with('configuration_prefixes')
90+
-> willReturnArgument(1);
91+
92+
$this->appConfig->method('getAppValueString')
93+
->willReturnCallback(function ($key, $default) {
94+
if ($key === 'ldap_host') {
95+
return 'example.com';
8496
}
97+
if ($key === 's1ldap_host') {
98+
return 'foo.bar.com';
99+
}
100+
return $default;
101+
});
102+
103+
$result = $this->helper->getServerConfigurationHosts();
104+
105+
$this->assertEquals([
106+
'' => 'example.com',
107+
's1' => 'foo.bar.com',
108+
's02' => '',
109+
], $result);
110+
}
111+
112+
public function testGetServerConfigurationHost(): void {
113+
$this->appConfig
114+
->expects(self::never())
115+
->method('getAppKeys');
116+
117+
$this->appConfig->method('getAppValueArray')
118+
->with('configuration_prefixes')
119+
-> willReturn([
120+
'',
121+
's1',
122+
's02',
123+
]);
124+
125+
$this->appConfig->method('getAppValueString')
126+
->willReturnCallback(function ($key, $default) {
85127
if ($key === 'ldap_host') {
86128
return 'example.com';
87129
}

0 commit comments

Comments
 (0)