From 8787c75db588f23621504735c8a3a953006665bd Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Mon, 10 Mar 2025 16:24:26 +0100 Subject: [PATCH] fix(lookup-server): disable lookup server for non-global scale setups Signed-off-by: Ferdinand Thiessen --- .../lib/FederatedShareProvider.php | 11 +++++--- .../src/components/AdminSettings.vue | 26 +++++++++++------- .../tests/FederatedShareProviderTest.php | 14 +++++++--- .../lib/Controller/ShareesAPIController.php | 10 +++---- .../lib/BackgroundJobs/RetryJob.php | 11 ++++---- .../lib/UpdateLookupServer.php | 7 ++--- .../lib/BackgroundJobs/VerifyUserData.php | 8 +++--- build/psalm-baseline.xml | 5 ---- .../Collaborators/LookupPlugin.php | 6 +++-- .../Collaborators/LookupPluginTest.php | 27 +++++++++---------- 10 files changed, 71 insertions(+), 54 deletions(-) diff --git a/apps/federatedfilesharing/lib/FederatedShareProvider.php b/apps/federatedfilesharing/lib/FederatedShareProvider.php index 251eadab38634..58eaca0361e2c 100644 --- a/apps/federatedfilesharing/lib/FederatedShareProvider.php +++ b/apps/federatedfilesharing/lib/FederatedShareProvider.php @@ -998,7 +998,10 @@ public function isLookupServerQueriesEnabled(): bool { if ($this->gsConfig->isGlobalScaleEnabled()) { return true; } - $result = $this->config->getAppValue('files_sharing', 'lookupServerEnabled', 'no'); + $result = $this->config->getAppValue('files_sharing', 'lookupServerEnabled', 'no') === 'yes'; + // TODO: Reenable if lookup server is used again + // return $result; + return false; } @@ -1010,8 +1013,10 @@ public function isLookupServerUploadEnabled(): bool { if ($this->gsConfig->isGlobalScaleEnabled()) { return false; } - $result = $this->config->getAppValue('files_sharing', 'lookupServerUploadEnabled', 'no'); - return $result === 'yes'; + $result = $this->config->getAppValue('files_sharing', 'lookupServerUploadEnabled', 'no') === 'yes'; + // TODO: Reenable if lookup server is used again + // return $result; + return false; } /** diff --git a/apps/federatedfilesharing/src/components/AdminSettings.vue b/apps/federatedfilesharing/src/components/AdminSettings.vue index 50ca46ae2b430..9f565aea3f695 100644 --- a/apps/federatedfilesharing/src/components/AdminSettings.vue +++ b/apps/federatedfilesharing/src/components/AdminSettings.vue @@ -32,17 +32,23 @@ {{ t('federatedfilesharing', 'Allow people on this server to receive group shares from other servers') }} - - {{ t('federatedfilesharing', 'Search global and public address book for people') }} - +
+ {{ t('federatedfilesharing', 'The lookup server is only available for global scale.') }} - - {{ t('federatedfilesharing', 'Allow people to publish their data to a global and public address book') }} - + + {{ t('federatedfilesharing', 'Search global and public address book for people') }} + + + + {{ t('federatedfilesharing', 'Allow people to publish their data to a global and public address book') }} + +
diff --git a/apps/federatedfilesharing/tests/FederatedShareProviderTest.php b/apps/federatedfilesharing/tests/FederatedShareProviderTest.php index a9738a7551921..002a8bac37406 100644 --- a/apps/federatedfilesharing/tests/FederatedShareProviderTest.php +++ b/apps/federatedfilesharing/tests/FederatedShareProviderTest.php @@ -836,10 +836,13 @@ public function testIsLookupServerQueriesEnabled($gsEnabled, $isEnabled, $expect public function dataTestIsLookupServerQueriesEnabled() { return [ - [false, 'yes', true], - [false, 'no', false], [true, 'yes', true], [true, 'no', true], + // TODO: reenable if we use the lookup server for non-global scale + // [false, 'yes', true], + // [false, 'no', false], + [false, 'no', false], + [false, 'yes', false], ]; } @@ -863,10 +866,13 @@ public function testIsLookupServerUploadEnabled($gsEnabled, $isEnabled, $expecte public function dataTestIsLookupServerUploadEnabled() { return [ - [false, 'yes', true], - [false, 'no', false], [true, 'yes', false], [true, 'no', false], + // TODO: reenable if we use the lookup server again + // [false, 'yes', true], + // [false, 'no', false], + [false, 'yes', false], + [false, 'no', false], ]; } diff --git a/apps/files_sharing/lib/Controller/ShareesAPIController.php b/apps/files_sharing/lib/Controller/ShareesAPIController.php index 3390c896532b2..0c458ce9662de 100644 --- a/apps/files_sharing/lib/Controller/ShareesAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareesAPIController.php @@ -22,6 +22,7 @@ use OCP\Collaboration\Collaborators\ISearchResult; use OCP\Collaboration\Collaborators\SearchResultType; use OCP\Constants; +use OCP\GlobalScale\IConfig as GlobalScaleIConfig; use OCP\IConfig; use OCP\IRequest; use OCP\IURLGenerator; @@ -175,12 +176,11 @@ public function search(string $search = '', ?string $itemType = null, int $page $this->limit = $perPage; $this->offset = $perPage * ($page - 1); - // In global scale mode we always search the loogup server - $lookup = $this->config->getSystemValueBool('gs.enabled', false) - || $this->config->getAppValue('files_sharing', 'lookupServerEnabled', 'no') === 'yes'; - $this->result['lookupEnabled'] = $lookup; + // In global scale mode we always search the lookup server + $this->result['lookupEnabled'] = Server::get(GlobalScaleIConfig::class)->isGlobalScaleEnabled(); + // TODO: Reconsider using lookup server for non-global-scale federation - [$result, $hasMoreResults] = $this->collaboratorSearch->search($search, $shareTypes, $lookup, $this->limit, $this->offset); + [$result, $hasMoreResults] = $this->collaboratorSearch->search($search, $shareTypes, $this->result['lookupEnabled'], $this->limit, $this->offset); // extra treatment for 'exact' subarray, with a single merge expected keys might be lost if (isset($result['exact'])) { diff --git a/apps/lookup_server_connector/lib/BackgroundJobs/RetryJob.php b/apps/lookup_server_connector/lib/BackgroundJobs/RetryJob.php index b5ce8823903db..c6801a46a3add 100644 --- a/apps/lookup_server_connector/lib/BackgroundJobs/RetryJob.php +++ b/apps/lookup_server_connector/lib/BackgroundJobs/RetryJob.php @@ -84,10 +84,11 @@ public function start(IJobList $jobList): void { * - max retries are reached (set to 5) */ protected function shouldRemoveBackgroundJob(): bool { - return $this->config->getSystemValueBool('has_internet_connection', true) === false || - $this->config->getSystemValueString('lookup_server', 'https://lookup.nextcloud.com') === '' || - $this->config->getAppValue('files_sharing', 'lookupServerUploadEnabled', 'no') !== 'yes' || - $this->retries >= 5; + // TODO: Remove global scale condition once lookup server is used for non-global scale federation + return !$this->config->getSystemValueBool('gs.enabled', false) + || $this->config->getSystemValueBool('has_internet_connection', true) === false + || $this->config->getSystemValueString('lookup_server', 'https://lookup.nextcloud.com') === '' + || $this->retries >= 5; } protected function shouldRun(): bool { @@ -149,7 +150,7 @@ protected function run($argument): void { $user->getUID(), 'lookup_server_connector', 'update_retries', - $this->retries + 1 + (string)($this->retries + 1), ); } } diff --git a/apps/lookup_server_connector/lib/UpdateLookupServer.php b/apps/lookup_server_connector/lib/UpdateLookupServer.php index eb390bacba7b5..dac3e8a80fae3 100644 --- a/apps/lookup_server_connector/lib/UpdateLookupServer.php +++ b/apps/lookup_server_connector/lib/UpdateLookupServer.php @@ -56,8 +56,9 @@ public function userUpdated(IUser $user): void { * @return bool */ private function shouldUpdateLookupServer(): bool { - return $this->config->getSystemValueBool('has_internet_connection', true) === true && - $this->config->getAppValue('files_sharing', 'lookupServerUploadEnabled', 'no') === 'yes' && - $this->config->getSystemValueString('lookup_server', 'https://lookup.nextcloud.com') !== ''; + // TODO: Consider reenable for non-global-scale setups by checking "'files_sharing', 'lookupServerUploadEnabled'" instead of "gs.enabled" + return $this->config->getSystemValueBool('gs.enabled', false) + && $this->config->getSystemValueBool('has_internet_connection', true) + && $this->config->getSystemValueString('lookup_server', 'https://lookup.nextcloud.com') !== ''; } } diff --git a/apps/settings/lib/BackgroundJobs/VerifyUserData.php b/apps/settings/lib/BackgroundJobs/VerifyUserData.php index 38021eae00ff0..eb66644ad9181 100644 --- a/apps/settings/lib/BackgroundJobs/VerifyUserData.php +++ b/apps/settings/lib/BackgroundJobs/VerifyUserData.php @@ -120,9 +120,11 @@ protected function verifyWebsite(array $argument) { } protected function verifyViaLookupServer(array $argument, string $dataType): bool { - if (empty($this->lookupServerUrl) || - $this->config->getAppValue('files_sharing', 'lookupServerUploadEnabled', 'no') !== 'yes' || - $this->config->getSystemValue('has_internet_connection', true) === false) { + // TODO: Consider to enable for non-global-scale setups by checking 'files_sharing', 'lookupServerUploadEnabled' + if (!$this->config->getSystemValueBool('gs.enabled', false) + || empty($this->lookupServerUrl) + || $this->config->getSystemValue('has_internet_connection', true) === false + ) { return true; } diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml index cecea953d0099..528bdcf5665d3 100644 --- a/build/psalm-baseline.xml +++ b/build/psalm-baseline.xml @@ -983,11 +983,6 @@ instanceOfStorage('\OC\Files\ObjectStore\ObjectStoreStorage')]]> - - - retries + 1]]> - - request->server]]> diff --git a/lib/private/Collaboration/Collaborators/LookupPlugin.php b/lib/private/Collaboration/Collaborators/LookupPlugin.php index cc40b2b6abdc5..2c8dc8454582f 100644 --- a/lib/private/Collaboration/Collaborators/LookupPlugin.php +++ b/lib/private/Collaboration/Collaborators/LookupPlugin.php @@ -35,8 +35,10 @@ public function search($search, $limit, $offset, ISearchResult $searchResult): b $isLookupServerEnabled = $this->config->getAppValue('files_sharing', 'lookupServerEnabled', 'no') === 'yes'; $hasInternetConnection = $this->config->getSystemValueBool('has_internet_connection', true); - // if case of Global Scale we always search the lookup server - if (!$isGlobalScaleEnabled && (!$isLookupServerEnabled || !$hasInternetConnection)) { + // If case of Global Scale we always search the lookup server + // TODO: Reconsider using the lookup server for non-global scale + // if (!$isGlobalScaleEnabled && (!$isLookupServerEnabled || !$hasInternetConnection || $disableLookupServer)) { + if (!$isGlobalScaleEnabled) { return false; } diff --git a/tests/lib/Collaboration/Collaborators/LookupPluginTest.php b/tests/lib/Collaboration/Collaborators/LookupPluginTest.php index 1ca40d160aa23..7ff01831e1823 100644 --- a/tests/lib/Collaboration/Collaborators/LookupPluginTest.php +++ b/tests/lib/Collaboration/Collaborators/LookupPluginTest.php @@ -80,7 +80,7 @@ public function testSearchNoLookupServerURI(): void { ['gs.enabled', false], ['has_internet_connection', true], )->willReturnOnConsecutiveCalls( - false, + true, true, ); @@ -145,7 +145,7 @@ public function testSearch(array $searchParams): void { ['gs.enabled', false], ['has_internet_connection', true], )->willReturnOnConsecutiveCalls( - false, + true, true, ); @@ -199,7 +199,7 @@ public function testSearchEnableDisableLookupServer(array $searchParams, $GSEnab ->method('getAppValue') ->with('files_sharing', 'lookupServerEnabled', 'no') ->willReturn($LookupEnabled ? 'yes' : 'no'); - if ($GSEnabled || $LookupEnabled) { + if ($GSEnabled) { $searchResult->expects($this->once()) ->method('addResultSet') ->with($type, $searchParams['expectedResult'], []); @@ -258,11 +258,13 @@ public function testSearchEnableDisableLookupServer(array $searchParams, $GSEnab } - public function testSearchLookupServerDisabled(): void { - $this->config->expects($this->once()) - ->method('getAppValue') - ->with('files_sharing', 'lookupServerEnabled', 'yes') - ->willReturn('no'); + public function testSearchGSDisabled(): void { + $this->config->expects($this->atLeastOnce()) + ->method('getSystemValueBool') + ->willReturnMap([ + ['has_internet_connection', true, true], + ['gs.enabled', false, false], + ]); /** @var ISearchResult|MockObject $searchResult */ $searchResult = $this->createMock(ISearchResult::class); @@ -381,7 +383,6 @@ public function dataSearchEnableDisableLookupServer() { 'label' => $fedIDs[0], 'value' => [ 'shareType' => IShare::TYPE_REMOTE, - 'globalScale' => false, 'shareWith' => $fedIDs[0] ], 'extra' => ['federationId' => $fedIDs[0]], @@ -390,7 +391,6 @@ public function dataSearchEnableDisableLookupServer() { 'label' => $fedIDs[1], 'value' => [ 'shareType' => IShare::TYPE_REMOTE, - 'globalScale' => false, 'shareWith' => $fedIDs[1] ], 'extra' => ['federationId' => $fedIDs[1]], @@ -399,7 +399,6 @@ public function dataSearchEnableDisableLookupServer() { 'label' => $fedIDs[2], 'value' => [ 'shareType' => IShare::TYPE_REMOTE, - 'globalScale' => false, 'shareWith' => $fedIDs[2] ], 'extra' => ['federationId' => $fedIDs[2]], @@ -474,7 +473,7 @@ public function searchDataProvider() { 'label' => $fedIDs[0], 'value' => [ 'shareType' => IShare::TYPE_REMOTE, - 'globalScale' => false, + 'globalScale' => true, 'shareWith' => $fedIDs[0] ], 'extra' => ['federationId' => $fedIDs[0]], @@ -483,7 +482,7 @@ public function searchDataProvider() { 'label' => $fedIDs[1], 'value' => [ 'shareType' => IShare::TYPE_REMOTE, - 'globalScale' => false, + 'globalScale' => true, 'shareWith' => $fedIDs[1] ], 'extra' => ['federationId' => $fedIDs[1]], @@ -492,7 +491,7 @@ public function searchDataProvider() { 'label' => $fedIDs[2], 'value' => [ 'shareType' => IShare::TYPE_REMOTE, - 'globalScale' => false, + 'globalScale' => true, 'shareWith' => $fedIDs[2] ], 'extra' => ['federationId' => $fedIDs[2]],