Skip to content

Commit e798deb

Browse files
susnuxsusnux
committed
fix(lookup-server): disable lookup server for non-global scale setups
Signed-off-by: Ferdinand Thiessen <[email protected]>
1 parent b0f0a45 commit e798deb

File tree

9 files changed

+90
-83
lines changed

9 files changed

+90
-83
lines changed

‎apps/federatedfilesharing/lib/FederatedShareProvider.php‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1065,8 +1065,10 @@ public function isLookupServerQueriesEnabled() {
10651065
if ($this->gsConfig->isGlobalScaleEnabled()) {
10661066
return true;
10671067
}
1068-
$result = $this->config->getAppValue('files_sharing', 'lookupServerEnabled', 'no');
1069-
return $result === 'yes';
1068+
$result = $this->config->getAppValue('files_sharing', 'lookupServerEnabled', 'no') === 'yes';
1069+
// TODO: Reenable if lookup server is used again
1070+
// return $result;
1071+
return false;
10701072
}
10711073

10721074

@@ -1080,8 +1082,10 @@ public function isLookupServerUploadEnabled() {
10801082
if ($this->gsConfig->isGlobalScaleEnabled()) {
10811083
return false;
10821084
}
1083-
$result = $this->config->getAppValue('files_sharing', 'lookupServerUploadEnabled', 'no');
1084-
return $result === 'yes';
1085+
$result = $this->config->getAppValue('files_sharing', 'lookupServerUploadEnabled', 'no') === 'yes';
1086+
// TODO: Reenable if lookup server is used again
1087+
// return $result;
1088+
return false;
10851089
}
10861090

10871091
/**

‎apps/federatedfilesharing/tests/FederatedShareProviderTest.php‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -856,10 +856,13 @@ public function testIsLookupServerQueriesEnabled($gsEnabled, $isEnabled, $expect
856856

857857
public function dataTestIsLookupServerQueriesEnabled() {
858858
return [
859-
[false, 'yes', true],
860-
[false, 'no', false],
861859
[true, 'yes', true],
862860
[true, 'no', true],
861+
// TODO: reenable if we use the lookup server for non-global scale
862+
// [false, 'yes', true],
863+
// [false, 'no', false],
864+
[false, 'no', false],
865+
[false, 'yes', false],
863866
];
864867
}
865868

@@ -883,10 +886,13 @@ public function testIsLookupServerUploadEnabled($gsEnabled, $isEnabled, $expecte
883886

884887
public function dataTestIsLookupServerUploadEnabled() {
885888
return [
886-
[false, 'yes', true],
887-
[false, 'no', false],
888889
[true, 'yes', false],
889890
[true, 'no', false],
891+
// TODO: reenable if we use the lookup server again
892+
// [false, 'yes', true],
893+
// [false, 'no', false],
894+
[false, 'yes', false],
895+
[false, 'no', false],
890896
];
891897
}
892898

‎apps/files_sharing/lib/Controller/ShareesAPIController.php‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,6 @@
3737
*/
3838
namespace OCA\Files_Sharing\Controller;
3939

40-
use OCP\Constants;
4140
use function array_slice;
4241
use function array_values;
4342
use Generator;
@@ -48,6 +47,8 @@
4847
use OCP\Collaboration\Collaborators\ISearch;
4948
use OCP\Collaboration\Collaborators\ISearchResult;
5049
use OCP\Collaboration\Collaborators\SearchResultType;
50+
use OCP\Constants;
51+
use OCP\GlobalScale\IConfig as GlobalScaleIConfig;
5152
use OCP\IConfig;
5253
use OCP\IRequest;
5354
use OCP\IURLGenerator;
@@ -217,12 +218,11 @@ public function search(string $search = '', string $itemType = null, int $page =
217218
$this->limit = $perPage;
218219
$this->offset = $perPage * ($page - 1);
219220

220-
// In global scale mode we always search the loogup server
221-
$lookup = $this->config->getSystemValueBool('gs.enabled', false)
222-
|| $this->config->getAppValue('files_sharing', 'lookupServerEnabled', 'no') === 'yes';
223-
$this->result['lookupEnabled'] = $lookup;
221+
// In global scale mode we always search the lookup server
222+
$this->result['lookupEnabled'] = \OCP\Server::get(GlobalScaleIConfig::class)->isGlobalScaleEnabled();
223+
// TODO: Reconsider using lookup server for non-global-scale federation
224224

225-
[$result, $hasMoreResults] = $this->collaboratorSearch->search($search, $shareTypes, $lookup, $this->limit, $this->offset);
225+
[$result, $hasMoreResults] = $this->collaboratorSearch->search($search, $shareTypes, $this->result['lookupEnabled'], $this->limit, $this->offset);
226226

227227
// extra treatment for 'exact' subarray, with a single merge expected keys might be lost
228228
if (isset($result['exact'])) {

‎apps/files_sharing/tests/Controller/ShareesAPIControllerTest.php‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
use OCP\AppFramework\Http;
3636
use OCP\AppFramework\OCS\OCSBadRequestException;
3737
use OCP\Collaboration\Collaborators\ISearch;
38+
use OCP\GlobalScale\IConfig as GlobalScaleIConfig;
3839
use OCP\IConfig;
3940
use OCP\IRequest;
4041
use OCP\IURLGenerator;
@@ -240,14 +241,14 @@ public function testSearch(array $getData, string $apiSetting, string $enumSetti
240241
$perPage = $getData['perPage'] ?? 200;
241242
$shareType = $getData['shareType'] ?? null;
242243

244+
$globalConfig = $this->createMock(GlobalScaleIConfig::class);
245+
$globalConfig->expects(self::once())
246+
->method('isGlobalScaleEnabled')
247+
->willReturn(true);
248+
$this->overwriteService(GlobalScaleIConfig::class, $globalConfig);
249+
243250
/** @var IConfig|MockObject $config */
244251
$config = $this->createMock(IConfig::class);
245-
$config->expects($this->exactly(1))
246-
->method('getAppValue')
247-
->with($this->anything(), $this->anything(), $this->anything())
248-
->willReturnMap([
249-
['files_sharing', 'lookupServerEnabled', 'no', 'yes'],
250-
]);
251252

252253
$this->shareManager->expects($this->once())
253254
->method('allowGroupSharing')

‎apps/lookup_server_connector/lib/BackgroundJobs/RetryJob.php‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -128,10 +128,12 @@ public function execute(IJobList $jobList, ILogger $logger = null): void {
128128
* @return bool
129129
*/
130130
protected function shouldRemoveBackgroundJob(): bool {
131-
return $this->config->getSystemValueBool('has_internet_connection', true) === false ||
132-
$this->config->getSystemValueString('lookup_server', 'https://lookup.nextcloud.com') === '' ||
133-
$this->config->getAppValue('files_sharing', 'lookupServerUploadEnabled', 'no') !== 'yes' ||
134-
$this->retries >= 5;
131+
// TODO: Remove global scale condition once lookup server is used for non-global scale federation
132+
// return $this->config->getAppValue('files_sharing', 'lookupServerUploadEnabled', 'no') !== 'yes'
133+
return !$this->config->getSystemValueBool('gs.enabled', false)
134+
|| $this->config->getSystemValueBool('has_internet_connection', true) === false
135+
|| $this->config->getSystemValueString('lookup_server', 'https://lookup.nextcloud.com') === ''
136+
|| $this->retries >= 5;
135137
}
136138

137139
protected function shouldRun(): bool {
@@ -193,7 +195,7 @@ protected function run($argument): void {
193195
$user->getUID(),
194196
'lookup_server_connector',
195197
'update_retries',
196-
$this->retries + 1
198+
(string)($this->retries + 1),
197199
);
198200
}
199201
}

‎apps/lookup_server_connector/lib/UpdateLookupServer.php‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -82,8 +82,9 @@ public function userUpdated(IUser $user): void {
8282
* @return bool
8383
*/
8484
private function shouldUpdateLookupServer(): bool {
85-
return $this->config->getSystemValueBool('has_internet_connection', true) === true &&
86-
$this->config->getAppValue('files_sharing', 'lookupServerUploadEnabled', 'no') === 'yes' &&
87-
$this->config->getSystemValueString('lookup_server', 'https://lookup.nextcloud.com') !== '';
85+
// TODO: Consider reenable for non-global-scale setups by checking "'files_sharing', 'lookupServerUploadEnabled'" instead of "gs.enabled"
86+
return $this->config->getSystemValueBool('gs.enabled', false)
87+
&& $this->config->getSystemValueBool('has_internet_connection', true)
88+
&& $this->config->getSystemValueString('lookup_server', 'https://lookup.nextcloud.com') !== '';
8889
}
8990
}

‎apps/settings/lib/BackgroundJobs/VerifyUserData.php‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -169,9 +169,11 @@ protected function verifyWebsite(array $argument) {
169169
}
170170

171171
protected function verifyViaLookupServer(array $argument, string $dataType): bool {
172-
if (empty($this->lookupServerUrl) ||
173-
$this->config->getAppValue('files_sharing', 'lookupServerUploadEnabled', 'no') !== 'yes' ||
174-
$this->config->getSystemValue('has_internet_connection', true) === false) {
172+
// TODO: Consider to enable for non-global-scale setups by checking 'files_sharing', 'lookupServerUploadEnabled'
173+
if (!$this->config->getSystemValueBool('gs.enabled', false)
174+
|| empty($this->lookupServerUrl)
175+
|| $this->config->getSystemValue('has_internet_connection', true) === false
176+
) {
175177
return true;
176178
}
177179

‎lib/private/Collaboration/Collaborators/LookupPlugin.php‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -64,12 +64,14 @@ public function __construct(IConfig $config,
6464
}
6565

6666
public function search($search, $limit, $offset, ISearchResult $searchResult) {
67-
$isGlobalScaleEnabled = $this->config->getSystemValue('gs.enabled', false);
67+
$isGlobalScaleEnabled = $this->config->getSystemValueBool('gs.enabled', false);
6868
$isLookupServerEnabled = $this->config->getAppValue('files_sharing', 'lookupServerEnabled', 'no') === 'yes';
6969
$hasInternetConnection = $this->config->getSystemValueBool('has_internet_connection', true);
7070

71-
// if case of Global Scale we always search the lookup server
72-
if (!$isGlobalScaleEnabled && (!$isLookupServerEnabled || !$hasInternetConnection)) {
71+
// If case of Global Scale we always search the lookup server
72+
// TODO: Reconsider using the lookup server for non-global scale
73+
// if (!$isGlobalScaleEnabled && (!$isLookupServerEnabled || !$hasInternetConnection || $disableLookupServer)) {
74+
if (!$isGlobalScaleEnabled) {
7375
return false;
7476
}
7577

‎tests/lib/Collaboration/Collaborators/LookupPluginTest.php‎

Lines changed: 38 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -88,20 +88,17 @@ protected function setUp(): void {
8888
}
8989

9090
public function testSearchNoLookupServerURI() {
91-
$this->config->expects($this->once())
91+
$this->config->expects($this->atMost(1))
9292
->method('getAppValue')
9393
->with('files_sharing', 'lookupServerEnabled', 'no')
9494
->willReturn('yes');
95-
$this->config->expects($this->at(0))
96-
->method('getSystemValue')
97-
->with('gs.enabled', false)
98-
->willReturn(false);
99-
100-
$this->config->expects($this->at(2))
95+
$this->config->expects($this->exactly(2))
10196
->method('getSystemValueBool')
102-
->with('has_internet_connection', true)
103-
->willReturn(true);
104-
$this->config->expects($this->at(3))
97+
->willReturnMap([
98+
['gs.enabled', false, true],
99+
['has_internet_connection', true, true],
100+
]);
101+
$this->config->expects($this->once())
105102
->method('getSystemValue')
106103
->with('lookup_server', 'https://lookup.nextcloud.com')
107104
->willReturn('');
@@ -120,15 +117,13 @@ public function testSearchNoInternet() {
120117
->method('getAppValue')
121118
->with('files_sharing', 'lookupServerEnabled', 'no')
122119
->willReturn('yes');
123-
$this->config->expects($this->at(0))
124-
->method('getSystemValue')
125-
->with('gs.enabled', false)
126-
->willReturn(false);
127120

128-
$this->config->expects($this->at(2))
121+
$this->config->expects($this->exactly(2))
129122
->method('getSystemValueBool')
130-
->with('has_internet_connection', true)
131-
->willReturn(false);
123+
->willReturnMap([
124+
['has_internet_connection', true, false],
125+
['gs.enabled', false, true],
126+
]);
132127

133128
$this->clientService->expects($this->never())
134129
->method('newClient');
@@ -156,19 +151,16 @@ public function testSearch(array $searchParams) {
156151
->method('getAppValue')
157152
->with('files_sharing', 'lookupServerEnabled', 'no')
158153
->willReturn('yes');
159-
$this->config->expects($this->at(0))
160-
->method('getSystemValue')
161-
->with('gs.enabled', false)
162-
->willReturn(false);
163-
164-
$this->config->expects($this->at(2))
165-
->method('getSystemValueBool')
166-
->with('has_internet_connection', true)
167-
->willReturn(true);
168-
$this->config->expects($this->at(3))
154+
$this->config->expects($this->once())
169155
->method('getSystemValue')
170156
->with('lookup_server', 'https://lookup.nextcloud.com')
171157
->willReturn($searchParams['server']);
158+
$this->config->expects($this->exactly(2))
159+
->method('getSystemValueBool')
160+
->willReturnMap([
161+
['gs.enabled', false, true],
162+
['has_internet_connection', true, true],
163+
]);
172164

173165
$response = $this->createMock(IResponse::class);
174166
$response->expects($this->once())
@@ -215,20 +207,19 @@ public function testSearchEnableDisableLookupServer(array $searchParams, $GSEnab
215207
->method('getAppValue')
216208
->with('files_sharing', 'lookupServerEnabled', 'no')
217209
->willReturn($LookupEnabled ? 'yes' : 'no');
218-
$this->config->expects($this->at(0))
219-
->method('getSystemValue')
220-
->with('gs.enabled', false)
221-
->willReturn($GSEnabled);
222-
if ($GSEnabled || $LookupEnabled) {
210+
211+
$this->config->expects($this->exactly(2))
212+
->method('getSystemValueBool')
213+
->willReturnMap([
214+
['has_internet_connection', true, true],
215+
['gs.enabled', false, $GSEnabled],
216+
]);
217+
if ($GSEnabled) {
223218
$searchResult->expects($this->once())
224219
->method('addResultSet')
225220
->with($type, $searchParams['expectedResult'], []);
226221

227-
$this->config->expects($this->at(2))
228-
->method('getSystemValueBool')
229-
->with('has_internet_connection', true)
230-
->willReturn(true);
231-
$this->config->expects($this->at(3))
222+
$this->config->expects($this->once())
232223
->method('getSystemValue')
233224
->with('lookup_server', 'https://lookup.nextcloud.com')
234225
->willReturn($searchParams['server']);
@@ -263,12 +254,13 @@ public function testSearchEnableDisableLookupServer(array $searchParams, $GSEnab
263254
$this->assertFalse($moreResults);
264255
}
265256

266-
267-
public function testSearchLookupServerDisabled() {
268-
$this->config->expects($this->once())
269-
->method('getAppValue')
270-
->with('files_sharing', 'lookupServerEnabled', 'yes')
271-
->willReturn('no');
257+
public function testSearchGSDisabled(): void {
258+
$this->config->expects($this->atLeastOnce())
259+
->method('getSystemValueBool')
260+
->willReturnMap([
261+
['has_internet_connection', true, true],
262+
['gs.enabled', false, false],
263+
]);
272264

273265
/** @var ISearchResult|MockObject $searchResult */
274266
$searchResult = $this->createMock(ISearchResult::class);
@@ -387,7 +379,6 @@ public function dataSearchEnableDisableLookupServer() {
387379
'label' => $fedIDs[0],
388380
'value' => [
389381
'shareType' => IShare::TYPE_REMOTE,
390-
'globalScale' => false,
391382
'shareWith' => $fedIDs[0]
392383
],
393384
'extra' => ['federationId' => $fedIDs[0]],
@@ -396,7 +387,6 @@ public function dataSearchEnableDisableLookupServer() {
396387
'label' => $fedIDs[1],
397388
'value' => [
398389
'shareType' => IShare::TYPE_REMOTE,
399-
'globalScale' => false,
400390
'shareWith' => $fedIDs[1]
401391
],
402392
'extra' => ['federationId' => $fedIDs[1]],
@@ -405,7 +395,6 @@ public function dataSearchEnableDisableLookupServer() {
405395
'label' => $fedIDs[2],
406396
'value' => [
407397
'shareType' => IShare::TYPE_REMOTE,
408-
'globalScale' => false,
409398
'shareWith' => $fedIDs[2]
410399
],
411400
'extra' => ['federationId' => $fedIDs[2]],
@@ -480,7 +469,7 @@ public function searchDataProvider() {
480469
'label' => $fedIDs[0],
481470
'value' => [
482471
'shareType' => IShare::TYPE_REMOTE,
483-
'globalScale' => false,
472+
'globalScale' => true,
484473
'shareWith' => $fedIDs[0]
485474
],
486475
'extra' => ['federationId' => $fedIDs[0]],
@@ -489,7 +478,7 @@ public function searchDataProvider() {
489478
'label' => $fedIDs[1],
490479
'value' => [
491480
'shareType' => IShare::TYPE_REMOTE,
492-
'globalScale' => false,
481+
'globalScale' => true,
493482
'shareWith' => $fedIDs[1]
494483
],
495484
'extra' => ['federationId' => $fedIDs[1]],
@@ -498,7 +487,7 @@ public function searchDataProvider() {
498487
'label' => $fedIDs[2],
499488
'value' => [
500489
'shareType' => IShare::TYPE_REMOTE,
501-
'globalScale' => false,
490+
'globalScale' => true,
502491
'shareWith' => $fedIDs[2]
503492
],
504493
'extra' => ['federationId' => $fedIDs[2]],

0 commit comments

Comments
 (0)