Skip to content

Commit 8787c75

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

10 files changed

Lines changed: 71 additions & 54 deletions

File tree

‎apps/federatedfilesharing/lib/FederatedShareProvider.php‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -998,7 +998,10 @@ public function isLookupServerQueriesEnabled(): bool {
998998
if ($this->gsConfig->isGlobalScaleEnabled()) {
999999
return true;
10001000
}
1001-
$result = $this->config->getAppValue('files_sharing', 'lookupServerEnabled', 'no');
1001+
$result = $this->config->getAppValue('files_sharing', 'lookupServerEnabled', 'no') === 'yes';
1002+
// TODO: Reenable if lookup server is used again
1003+
// return $result;
1004+
return false;
10021005
}
10031006

10041007

@@ -1010,8 +1013,10 @@ public function isLookupServerUploadEnabled(): bool {
10101013
if ($this->gsConfig->isGlobalScaleEnabled()) {
10111014
return false;
10121015
}
1013-
$result = $this->config->getAppValue('files_sharing', 'lookupServerUploadEnabled', 'no');
1014-
return $result === 'yes';
1016+
$result = $this->config->getAppValue('files_sharing', 'lookupServerUploadEnabled', 'no') === 'yes';
1017+
// TODO: Reenable if lookup server is used again
1018+
// return $result;
1019+
return false;
10151020
}
10161021

10171022
/**

‎apps/federatedfilesharing/src/components/AdminSettings.vue‎

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -32,17 +32,23 @@
3232
{{ t('federatedfilesharing', 'Allow people on this server to receive group shares from other servers') }}
3333
</NcCheckboxRadioSwitch>
3434

35-
<NcCheckboxRadioSwitch type="switch"
36-
:checked.sync="lookupServerEnabled"
37-
@update:checked="update('lookupServerEnabled', lookupServerEnabled)">
38-
{{ t('federatedfilesharing', 'Search global and public address book for people') }}
39-
</NcCheckboxRadioSwitch>
35+
<fieldset>
36+
<legend>{{ t('federatedfilesharing', 'The lookup server is only available for global scale.') }}</legend>
4037

41-
<NcCheckboxRadioSwitch type="switch"
42-
:checked.sync="lookupServerUploadEnabled"
43-
@update:checked="update('lookupServerUploadEnabled', lookupServerUploadEnabled)">
44-
{{ t('federatedfilesharing', 'Allow people to publish their data to a global and public address book') }}
45-
</NcCheckboxRadioSwitch>
38+
<NcCheckboxRadioSwitch type="switch"
39+
:checked.sync="lookupServerEnabled"
40+
disabled
41+
@update:checked="update('lookupServerEnabled', lookupServerEnabled)">
42+
{{ t('federatedfilesharing', 'Search global and public address book for people') }}
43+
</NcCheckboxRadioSwitch>
44+
45+
<NcCheckboxRadioSwitch type="switch"
46+
:checked.sync="lookupServerUploadEnabled"
47+
disabled
48+
@update:checked="update('lookupServerUploadEnabled', lookupServerUploadEnabled)">
49+
{{ t('federatedfilesharing', 'Allow people to publish their data to a global and public address book') }}
50+
</NcCheckboxRadioSwitch>
51+
</fieldset>
4652

4753
<!-- Trusted server handling -->
4854
<div class="settings-subsection">

‎apps/federatedfilesharing/tests/FederatedShareProviderTest.php‎

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

837837
public function dataTestIsLookupServerQueriesEnabled() {
838838
return [
839-
[false, 'yes', true],
840-
[false, 'no', false],
841839
[true, 'yes', true],
842840
[true, 'no', true],
841+
// TODO: reenable if we use the lookup server for non-global scale
842+
// [false, 'yes', true],
843+
// [false, 'no', false],
844+
[false, 'no', false],
845+
[false, 'yes', false],
843846
];
844847
}
845848

@@ -863,10 +866,13 @@ public function testIsLookupServerUploadEnabled($gsEnabled, $isEnabled, $expecte
863866

864867
public function dataTestIsLookupServerUploadEnabled() {
865868
return [
866-
[false, 'yes', true],
867-
[false, 'no', false],
868869
[true, 'yes', false],
869870
[true, 'no', false],
871+
// TODO: reenable if we use the lookup server again
872+
// [false, 'yes', true],
873+
// [false, 'no', false],
874+
[false, 'yes', false],
875+
[false, 'no', false],
870876
];
871877
}
872878

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

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
use OCP\Collaboration\Collaborators\ISearchResult;
2323
use OCP\Collaboration\Collaborators\SearchResultType;
2424
use OCP\Constants;
25+
use OCP\GlobalScale\IConfig as GlobalScaleIConfig;
2526
use OCP\IConfig;
2627
use OCP\IRequest;
2728
use OCP\IURLGenerator;
@@ -175,12 +176,11 @@ public function search(string $search = '', ?string $itemType = null, int $page
175176
$this->limit = $perPage;
176177
$this->offset = $perPage * ($page - 1);
177178

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

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

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

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

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -84,10 +84,11 @@ public function start(IJobList $jobList): void {
8484
* - max retries are reached (set to 5)
8585
*/
8686
protected function shouldRemoveBackgroundJob(): bool {
87-
return $this->config->getSystemValueBool('has_internet_connection', true) === false ||
88-
$this->config->getSystemValueString('lookup_server', 'https://lookup.nextcloud.com') === '' ||
89-
$this->config->getAppValue('files_sharing', 'lookupServerUploadEnabled', 'no') !== 'yes' ||
90-
$this->retries >= 5;
87+
// TODO: Remove global scale condition once lookup server is used for non-global scale federation
88+
return !$this->config->getSystemValueBool('gs.enabled', false)
89+
|| $this->config->getSystemValueBool('has_internet_connection', true) === false
90+
|| $this->config->getSystemValueString('lookup_server', 'https://lookup.nextcloud.com') === ''
91+
|| $this->retries >= 5;
9192
}
9293

9394
protected function shouldRun(): bool {
@@ -149,7 +150,7 @@ protected function run($argument): void {
149150
$user->getUID(),
150151
'lookup_server_connector',
151152
'update_retries',
152-
$this->retries + 1
153+
(string)($this->retries + 1),
153154
);
154155
}
155156
}

‎apps/lookup_server_connector/lib/UpdateLookupServer.php‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -56,8 +56,9 @@ public function userUpdated(IUser $user): void {
5656
* @return bool
5757
*/
5858
private function shouldUpdateLookupServer(): bool {
59-
return $this->config->getSystemValueBool('has_internet_connection', true) === true &&
60-
$this->config->getAppValue('files_sharing', 'lookupServerUploadEnabled', 'no') === 'yes' &&
61-
$this->config->getSystemValueString('lookup_server', 'https://lookup.nextcloud.com') !== '';
59+
// TODO: Consider reenable for non-global-scale setups by checking "'files_sharing', 'lookupServerUploadEnabled'" instead of "gs.enabled"
60+
return $this->config->getSystemValueBool('gs.enabled', false)
61+
&& $this->config->getSystemValueBool('has_internet_connection', true)
62+
&& $this->config->getSystemValueString('lookup_server', 'https://lookup.nextcloud.com') !== '';
6263
}
6364
}

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

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

122122
protected function verifyViaLookupServer(array $argument, string $dataType): bool {
123-
if (empty($this->lookupServerUrl) ||
124-
$this->config->getAppValue('files_sharing', 'lookupServerUploadEnabled', 'no') !== 'yes' ||
125-
$this->config->getSystemValue('has_internet_connection', true) === false) {
123+
// TODO: Consider to enable for non-global-scale setups by checking 'files_sharing', 'lookupServerUploadEnabled'
124+
if (!$this->config->getSystemValueBool('gs.enabled', false)
125+
|| empty($this->lookupServerUrl)
126+
|| $this->config->getSystemValue('has_internet_connection', true) === false
127+
) {
126128
return true;
127129
}
128130

‎build/psalm-baseline.xml‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -983,11 +983,6 @@
983983
<code><![CDATA[$storage1->instanceOfStorage('\OC\Files\ObjectStore\ObjectStoreStorage')]]></code>
984984
</RedundantCondition>
985985
</file>
986-
<file src="apps/lookup_server_connector/lib/BackgroundJobs/RetryJob.php">
987-
<InvalidArgument>
988-
<code><![CDATA[$this->retries + 1]]></code>
989-
</InvalidArgument>
990-
</file>
991986
<file src="apps/oauth2/lib/Controller/OauthApiController.php">
992987
<NoInterfaceProperties>
993988
<code><![CDATA[$this->request->server]]></code>

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,10 @@ public function search($search, $limit, $offset, ISearchResult $searchResult): b
3535
$isLookupServerEnabled = $this->config->getAppValue('files_sharing', 'lookupServerEnabled', 'no') === 'yes';
3636
$hasInternetConnection = $this->config->getSystemValueBool('has_internet_connection', true);
3737

38-
// if case of Global Scale we always search the lookup server
39-
if (!$isGlobalScaleEnabled && (!$isLookupServerEnabled || !$hasInternetConnection)) {
38+
// If case of Global Scale we always search the lookup server
39+
// TODO: Reconsider using the lookup server for non-global scale
40+
// if (!$isGlobalScaleEnabled && (!$isLookupServerEnabled || !$hasInternetConnection || $disableLookupServer)) {
41+
if (!$isGlobalScaleEnabled) {
4042
return false;
4143
}
4244

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

Lines changed: 13 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ public function testSearchNoLookupServerURI(): void {
8080
['gs.enabled', false],
8181
['has_internet_connection', true],
8282
)->willReturnOnConsecutiveCalls(
83-
false,
83+
true,
8484
true,
8585
);
8686

@@ -145,7 +145,7 @@ public function testSearch(array $searchParams): void {
145145
['gs.enabled', false],
146146
['has_internet_connection', true],
147147
)->willReturnOnConsecutiveCalls(
148-
false,
148+
true,
149149
true,
150150
);
151151

@@ -199,7 +199,7 @@ public function testSearchEnableDisableLookupServer(array $searchParams, $GSEnab
199199
->method('getAppValue')
200200
->with('files_sharing', 'lookupServerEnabled', 'no')
201201
->willReturn($LookupEnabled ? 'yes' : 'no');
202-
if ($GSEnabled || $LookupEnabled) {
202+
if ($GSEnabled) {
203203
$searchResult->expects($this->once())
204204
->method('addResultSet')
205205
->with($type, $searchParams['expectedResult'], []);
@@ -258,11 +258,13 @@ public function testSearchEnableDisableLookupServer(array $searchParams, $GSEnab
258258
}
259259

260260

261-
public function testSearchLookupServerDisabled(): void {
262-
$this->config->expects($this->once())
263-
->method('getAppValue')
264-
->with('files_sharing', 'lookupServerEnabled', 'yes')
265-
->willReturn('no');
261+
public function testSearchGSDisabled(): void {
262+
$this->config->expects($this->atLeastOnce())
263+
->method('getSystemValueBool')
264+
->willReturnMap([
265+
['has_internet_connection', true, true],
266+
['gs.enabled', false, false],
267+
]);
266268

267269
/** @var ISearchResult|MockObject $searchResult */
268270
$searchResult = $this->createMock(ISearchResult::class);
@@ -381,7 +383,6 @@ public function dataSearchEnableDisableLookupServer() {
381383
'label' => $fedIDs[0],
382384
'value' => [
383385
'shareType' => IShare::TYPE_REMOTE,
384-
'globalScale' => false,
385386
'shareWith' => $fedIDs[0]
386387
],
387388
'extra' => ['federationId' => $fedIDs[0]],
@@ -390,7 +391,6 @@ public function dataSearchEnableDisableLookupServer() {
390391
'label' => $fedIDs[1],
391392
'value' => [
392393
'shareType' => IShare::TYPE_REMOTE,
393-
'globalScale' => false,
394394
'shareWith' => $fedIDs[1]
395395
],
396396
'extra' => ['federationId' => $fedIDs[1]],
@@ -399,7 +399,6 @@ public function dataSearchEnableDisableLookupServer() {
399399
'label' => $fedIDs[2],
400400
'value' => [
401401
'shareType' => IShare::TYPE_REMOTE,
402-
'globalScale' => false,
403402
'shareWith' => $fedIDs[2]
404403
],
405404
'extra' => ['federationId' => $fedIDs[2]],
@@ -474,7 +473,7 @@ public function searchDataProvider() {
474473
'label' => $fedIDs[0],
475474
'value' => [
476475
'shareType' => IShare::TYPE_REMOTE,
477-
'globalScale' => false,
476+
'globalScale' => true,
478477
'shareWith' => $fedIDs[0]
479478
],
480479
'extra' => ['federationId' => $fedIDs[0]],
@@ -483,7 +482,7 @@ public function searchDataProvider() {
483482
'label' => $fedIDs[1],
484483
'value' => [
485484
'shareType' => IShare::TYPE_REMOTE,
486-
'globalScale' => false,
485+
'globalScale' => true,
487486
'shareWith' => $fedIDs[1]
488487
],
489488
'extra' => ['federationId' => $fedIDs[1]],
@@ -492,7 +491,7 @@ public function searchDataProvider() {
492491
'label' => $fedIDs[2],
493492
'value' => [
494493
'shareType' => IShare::TYPE_REMOTE,
495-
'globalScale' => false,
494+
'globalScale' => true,
496495
'shareWith' => $fedIDs[2]
497496
],
498497
'extra' => ['federationId' => $fedIDs[2]],

0 commit comments

Comments
 (0)