From ccbe85b2452c12fc1778af7163048716dddfa303 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Thu, 21 Mar 2024 13:12:44 +0100 Subject: [PATCH 01/12] refactor: Switch local and remote server roles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This will be needed to add tests with unavailable servers, as the local server is started by the script that runs the tests rather than from the test features themselves, so only the remote server can be stopped and started again. Signed-off-by: Daniel Calviño Sánchez --- .../federation_features/federated.feature | 27 ++++++++++--------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/build/integration/federation_features/federated.feature b/build/integration/federation_features/federated.feature index fc7cc4c4bf40f..b25442fd12e9b 100644 --- a/build/integration/federation_features/federated.feature +++ b/build/integration/federation_features/federated.feature @@ -375,28 +375,29 @@ Feature: federated And user "user1" exists # Rename file so it has a unique name in the target server (as the target # server may have its own /textfile0.txt" file) - And User "user1" copies file "/textfile0.txt" to "/remote-share.txt" - And User "user1" from server "REMOTE" shares "/remote-share.txt" with user "user0" from server "LOCAL" - And As an "user1" - And sending "GET" to "/apps/files_sharing/api/v1/shares" - And the list of returned shares has 1 shares + And User "user0" copies file "/textfile0.txt" to "/remote-share.txt" + And User "user0" from server "LOCAL" shares "/remote-share.txt" with user "user1" from server "REMOTE" And Using server "LOCAL" - And User "user0" from server "LOCAL" accepts last pending share - And as "user0" the file "/remote-share.txt" exists And As an "user0" - And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And sending "GET" to "/apps/files_sharing/api/v1/shares" And the list of returned shares has 1 shares And Using server "REMOTE" - When As an "user1" + And User "user1" from server "REMOTE" accepts last pending share + And as "user1" the file "/remote-share.txt" exists + And As an "user1" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 1 shares + And Using server "LOCAL" + When As an "user0" And Deleting last share Then the OCS status code should be "100" And the HTTP status code should be "200" - And As an "user1" + And As an "user0" And sending "GET" to "/apps/files_sharing/api/v1/shares" And the list of returned shares has 0 shares - And Using server "LOCAL" - And as "user0" the file "/remote-share.txt" does not exist - And As an "user0" + And Using server "REMOTE" + And as "user1" the file "/remote-share.txt" does not exist + And As an "user1" And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" And the list of returned shares has 0 shares From 3d9242c816061aa734d9b487cc077a2872399117 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Thu, 21 Mar 2024 13:14:32 +0100 Subject: [PATCH 02/12] test: Add integration test for federated share with unavailable server MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Daniel Calviño Sánchez --- .../federation_features/federated.feature | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/build/integration/federation_features/federated.feature b/build/integration/federation_features/federated.feature index b25442fd12e9b..d2314215acfb5 100644 --- a/build/integration/federation_features/federated.feature +++ b/build/integration/federation_features/federated.feature @@ -401,6 +401,35 @@ Feature: federated And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" And the list of returned shares has 0 shares + Scenario: Delete federated share with another server no longer reachable + Given Using server "LOCAL" + And user "user0" exists + Given Using server "REMOTE" + And user "user1" exists + # Rename file so it has a unique name in the target server (as the target + # server may have its own /textfile0.txt" file) + And User "user0" copies file "/textfile0.txt" to "/remote-share.txt" + And User "user0" from server "LOCAL" shares "/remote-share.txt" with user "user1" from server "REMOTE" + And Using server "LOCAL" + And As an "user0" + And sending "GET" to "/apps/files_sharing/api/v1/shares" + And the list of returned shares has 1 shares + And Using server "REMOTE" + And User "user1" from server "REMOTE" accepts last pending share + And as "user1" the file "/remote-share.txt" exists + And As an "user1" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 1 shares + And remote server is stopped + And Using server "LOCAL" + When As an "user0" + And Deleting last share + Then the OCS status code should be "100" + And the HTTP status code should be "200" + And As an "user0" + And sending "GET" to "/apps/files_sharing/api/v1/shares" + And the list of returned shares has 0 shares + Scenario: Delete federated share from another server Given Using server "LOCAL" And user "user0" exists From 7437fd6ca1d8d031470f551f23014af532aab4b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Thu, 21 Mar 2024 13:16:27 +0100 Subject: [PATCH 03/12] test: Add integration tests for federated shares with temporary down server MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Once the receiver server is up again trying to access the federated share should cause it to be removed in the receiver server as well, as it will be no longer found in the sharer server. Signed-off-by: Daniel Calviño Sánchez --- .../features/bootstrap/FederationContext.php | 40 +++++++++++++++++++ .../federation_features/federated.feature | 35 ++++++++++++++++ 2 files changed, 75 insertions(+) diff --git a/build/integration/features/bootstrap/FederationContext.php b/build/integration/features/bootstrap/FederationContext.php index 423708adc1053..3ff83de7b91e7 100644 --- a/build/integration/features/bootstrap/FederationContext.php +++ b/build/integration/features/bootstrap/FederationContext.php @@ -29,6 +29,10 @@ use Behat\Behat\Context\Context; use Behat\Behat\Context\SnippetAcceptingContext; use Behat\Gherkin\Node\TableNode; +use GuzzleHttp\Client; +use GuzzleHttp\Exception\ClientException; +use GuzzleHttp\Exception\ConnectException; +use PHPUnit\Framework\Assert; require __DIR__ . '/../../vendor/autoload.php'; @@ -177,6 +181,42 @@ public function deleteLastAcceptedRemoteShare($user) { $this->sendingToWith('DELETE', "/apps/files_sharing/api/v1/remote_shares/" . $this->lastAcceptedRemoteShareId, null); } + /** + * @When /^remote server is started$/ + */ + public function remoteServerIsStarted() { + $this->startFederatedServer(); + + $retryCount = 10; + + while (!$this->isRemoteServerReady()) { + if ($retryCount > 0) { + sleep(1); + + $retryCount--; + } else { + Assert::fail("Remote server not ready yet after 10 seconds"); + } + } + } + + private function isRemoteServerReady() { + $port = getenv('PORT_FED'); + $remoteServerUrl = 'http://localhost:' . $port; + + $client = new Client(); + + try { + $client->request('GET', $remoteServerUrl); + } catch (ClientException $exception) { + return false; + } catch (ConnectException $exception) { + return false; + } + + return true; + } + /** * @When /^remote server is stopped$/ */ diff --git a/build/integration/federation_features/federated.feature b/build/integration/federation_features/federated.feature index d2314215acfb5..5eccbd6f03023 100644 --- a/build/integration/federation_features/federated.feature +++ b/build/integration/federation_features/federated.feature @@ -430,6 +430,41 @@ Feature: federated And sending "GET" to "/apps/files_sharing/api/v1/shares" And the list of returned shares has 0 shares + Scenario: Delete federated share with another server when temporary unreachable + Given Using server "LOCAL" + And user "user0" exists + Given Using server "REMOTE" + And user "user1" exists + # Rename file so it has a unique name in the target server (as the target + # server may have its own /textfile0.txt" file) + And User "user0" copies file "/textfile0.txt" to "/remote-share.txt" + And User "user0" from server "LOCAL" shares "/remote-share.txt" with user "user1" from server "REMOTE" + And Using server "LOCAL" + And As an "user0" + And sending "GET" to "/apps/files_sharing/api/v1/shares" + And the list of returned shares has 1 shares + And Using server "REMOTE" + And User "user1" from server "REMOTE" accepts last pending share + And as "user1" the file "/remote-share.txt" exists + And As an "user1" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 1 shares + And remote server is stopped + And Using server "LOCAL" + When As an "user0" + And Deleting last share + Then the OCS status code should be "100" + And the HTTP status code should be "200" + And As an "user0" + And sending "GET" to "/apps/files_sharing/api/v1/shares" + And the list of returned shares has 0 shares + And remote server is started + And Using server "REMOTE" + And as "user1" the file "/remote-share.txt" does not exist + And As an "user1" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 0 shares + Scenario: Delete federated share from another server Given Using server "LOCAL" And user "user0" exists From 2b1cc5524749445fb9753d3fe36cbf1e9335727f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Thu, 21 Mar 2024 13:19:03 +0100 Subject: [PATCH 04/12] test: Add integration tests for federated shares from temporary down server MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Once the sharer server is up again the federated share will be still there, as the sharer server missed the notification from the receiver server about deleting the share. Signed-off-by: Daniel Calviño Sánchez --- .../federation_features/federated.feature | 57 +++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/build/integration/federation_features/federated.feature b/build/integration/federation_features/federated.feature index 5eccbd6f03023..3e39f3d484737 100644 --- a/build/integration/federation_features/federated.feature +++ b/build/integration/federation_features/federated.feature @@ -519,6 +519,35 @@ Feature: federated And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" And the list of returned shares has 0 shares + Scenario: Delete federated share from another server when temporary unreachable + Given Using server "LOCAL" + And user "user0" exists + Given Using server "REMOTE" + And user "user1" exists + # Rename file so it has a unique name in the target server (as the target + # server may have its own /textfile0.txt" file) + And User "user1" copies file "/textfile0.txt" to "/remote-share.txt" + And User "user1" from server "REMOTE" shares "/remote-share.txt" with user "user0" from server "LOCAL" + And Using server "LOCAL" + And User "user0" from server "LOCAL" accepts last pending share + And as "user0" the file "/remote-share.txt" exists + And As an "user0" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 1 shares + And remote server is stopped + When user "user0" deletes last accepted remote share + Then the OCS status code should be "100" + And the HTTP status code should be "200" + And remote server is started + And as "user0" the file "/remote-share.txt" does not exist + And As an "user0" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 0 shares + And Using server "REMOTE" + And As an "user1" + And sending "GET" to "/apps/files_sharing/api/v1/shares" + And the list of returned shares has 1 shares + Scenario: Delete federated share file from another server Given Using server "LOCAL" And user "user0" exists @@ -570,3 +599,31 @@ Feature: federated And As an "user0" And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" And the list of returned shares has 0 shares + + Scenario: Delete federated share file from another server when temporary unreachable + Given Using server "LOCAL" + And user "user0" exists + Given Using server "REMOTE" + And user "user1" exists + # Rename file so it has a unique name in the target server (as the target + # server may have its own /textfile0.txt" file) + And User "user1" copies file "/textfile0.txt" to "/remote-share.txt" + And User "user1" from server "REMOTE" shares "/remote-share.txt" with user "user0" from server "LOCAL" + And Using server "LOCAL" + And User "user0" from server "LOCAL" accepts last pending share + And as "user0" the file "/remote-share.txt" exists + And As an "user0" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 1 shares + And remote server is stopped + When User "user0" deletes file "/remote-share.txt" + Then the HTTP status code should be "204" + And remote server is started + And as "user0" the file "/remote-share.txt" does not exist + And As an "user0" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 0 shares + And Using server "REMOTE" + And As an "user1" + And sending "GET" to "/apps/files_sharing/api/v1/shares" + And the list of returned shares has 1 shares From b15de3105ec1733297bdc1ceaafa6a0ee70b1dc1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Wed, 27 Mar 2024 04:55:10 +0100 Subject: [PATCH 05/12] fix: Clear remote shares between scenarios MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Although groups are removed after each scenario it seems that their remote group shares are not, so there might be dangling remote group shares that could interfere with the following scenarios. To solve that a new endpoint is added to the "testing" app to explicitly clear the remote shares. Note that any dangling remote storage was already removed with the OCC command, so only the "share_external" table is cleared. Signed-off-by: Daniel Calviño Sánchez --- apps/testing/appinfo/routes.php | 5 ++ .../composer/composer/autoload_classmap.php | 1 + .../composer/composer/autoload_static.php | 1 + .../lib/Controller/RemoteSharesController.php | 64 +++++++++++++++++++ .../features/bootstrap/FederationContext.php | 18 +++++- 5 files changed, 88 insertions(+), 1 deletion(-) create mode 100644 apps/testing/lib/Controller/RemoteSharesController.php diff --git a/apps/testing/appinfo/routes.php b/apps/testing/appinfo/routes.php index 1ab36a4e82612..a461bd98b663e 100644 --- a/apps/testing/appinfo/routes.php +++ b/apps/testing/appinfo/routes.php @@ -80,5 +80,10 @@ 'type' => null ] ], + [ + 'name' => 'RemoteShares#delete', + 'url' => '/api/v1/remote_shares', + 'verb' => 'DELETE', + ], ], ]; diff --git a/apps/testing/composer/composer/autoload_classmap.php b/apps/testing/composer/composer/autoload_classmap.php index 079f887788100..7898144672b94 100644 --- a/apps/testing/composer/composer/autoload_classmap.php +++ b/apps/testing/composer/composer/autoload_classmap.php @@ -12,6 +12,7 @@ 'OCA\\Testing\\Controller\\ConfigController' => $baseDir . '/../lib/Controller/ConfigController.php', 'OCA\\Testing\\Controller\\LockingController' => $baseDir . '/../lib/Controller/LockingController.php', 'OCA\\Testing\\Controller\\RateLimitTestController' => $baseDir . '/../lib/Controller/RateLimitTestController.php', + 'OCA\\Testing\\Controller\\RemoteSharesController' => $baseDir . '/../lib/Controller/RemoteSharesController.php', 'OCA\\Testing\\Listener\\GetDeclarativeSettingsValueListener' => $baseDir . '/../lib/Listener/GetDeclarativeSettingsValueListener.php', 'OCA\\Testing\\Listener\\RegisterDeclarativeSettingsListener' => $baseDir . '/../lib/Listener/RegisterDeclarativeSettingsListener.php', 'OCA\\Testing\\Listener\\SetDeclarativeSettingsValueListener' => $baseDir . '/../lib/Listener/SetDeclarativeSettingsValueListener.php', diff --git a/apps/testing/composer/composer/autoload_static.php b/apps/testing/composer/composer/autoload_static.php index 2332da70da97f..e2819e61b82e9 100644 --- a/apps/testing/composer/composer/autoload_static.php +++ b/apps/testing/composer/composer/autoload_static.php @@ -27,6 +27,7 @@ class ComposerStaticInitTesting 'OCA\\Testing\\Controller\\ConfigController' => __DIR__ . '/..' . '/../lib/Controller/ConfigController.php', 'OCA\\Testing\\Controller\\LockingController' => __DIR__ . '/..' . '/../lib/Controller/LockingController.php', 'OCA\\Testing\\Controller\\RateLimitTestController' => __DIR__ . '/..' . '/../lib/Controller/RateLimitTestController.php', + 'OCA\\Testing\\Controller\\RemoteSharesController' => __DIR__ . '/..' . '/../lib/Controller/RemoteSharesController.php', 'OCA\\Testing\\Listener\\GetDeclarativeSettingsValueListener' => __DIR__ . '/..' . '/../lib/Listener/GetDeclarativeSettingsValueListener.php', 'OCA\\Testing\\Listener\\RegisterDeclarativeSettingsListener' => __DIR__ . '/..' . '/../lib/Listener/RegisterDeclarativeSettingsListener.php', 'OCA\\Testing\\Listener\\SetDeclarativeSettingsValueListener' => __DIR__ . '/..' . '/../lib/Listener/SetDeclarativeSettingsValueListener.php', diff --git a/apps/testing/lib/Controller/RemoteSharesController.php b/apps/testing/lib/Controller/RemoteSharesController.php new file mode 100644 index 0000000000000..90fe4dd229ef7 --- /dev/null +++ b/apps/testing/lib/Controller/RemoteSharesController.php @@ -0,0 +1,64 @@ + + * + * @author Daniel Calviño Sánchez + * + * @license AGPL-3.0 + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License, version 3, + * along with this program. If not, see + * + */ +namespace OCA\Testing\Controller; + +use OCP\AppFramework\Http\DataResponse; +use OCP\AppFramework\OCSController; +use OCP\IDBConnection; +use OCP\IRequest; + +class RemoteSharesController extends OCSController { + + /** @var IDBConnection */ + protected $connection; + + /** + * @param string $appName + * @param IRequest $request + */ + public function __construct($appName, + IRequest $request, + IDBConnection $connection) { + parent::__construct($appName, $request); + $this->connection = $connection; + } + + /** + * Deletes all remote shares. + * + * Note that neither storages nor mountpoints are deleted. This is meant to + * be used to get rid of dangling group shares after the users and groups + * were deleted. + * + * @return DataResponse + */ + public function delete() { + // For simplicity the database table is cleared directly in the + // controller :see-no-evil: + $query = $this->connection->getQueryBuilder(); + $query->delete('share_external'); + + $query->executeStatement(); + + return new DataResponse(); + } +} diff --git a/build/integration/features/bootstrap/FederationContext.php b/build/integration/features/bootstrap/FederationContext.php index 3ff83de7b91e7..409c5f3ca1e32 100644 --- a/build/integration/features/bootstrap/FederationContext.php +++ b/build/integration/features/bootstrap/FederationContext.php @@ -70,7 +70,7 @@ public function startFederatedServer() { /** * @BeforeScenario */ - public function cleanupRemoteStorages() { + public function cleanupRemoteStoragesAndShares() { // Ensure that dangling remote storages from previous tests will not // interfere with the current scenario. // The storages must be cleaned before each scenario; they can not be @@ -78,6 +78,22 @@ public function cleanupRemoteStorages() { // that removes the users, so the shares would be still valid and thus // the storages would not be dangling yet. $this->runOcc(['sharing:cleanup-remote-storages']); + + // Even if the groups are removed after each scenario there might be + // dangling remote group shares that could interfere with the current + // scenario, so the remote shares need to be explicitly cleared. + $this->runOcc(['app:enable', 'testing']); + + $user = $this->currentUser; + $this->currentUser = 'admin'; + + $this->sendingTo('DELETE', "/apps/testing/api/v1/remote_shares"); + $this->theHTTPStatusCodeShouldBe('200'); + if ($this->apiVersion === 1) { + $this->theOCSStatusCodeShouldBe('100'); + } + + $this->currentUser = $user; } /** From 19f71346a7f3abb9c335ec4fb07bc3a3788e5ad6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Fri, 22 Mar 2024 14:12:33 +0100 Subject: [PATCH 06/12] test: Add integration tests for deleting federated group shares from remote MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a member of the group that received a federated group share removes it the share is just removed for that specific user, but it does not affect other users in the group. Due to that the share is simply marked again as pending for that user. This even applies when the remote server is no longer reachable, as it is not possible to know if the server is just temporarily down or gone forever. Signed-off-by: Daniel Calviño Sánchez --- .../features/bootstrap/FederationContext.php | 32 +++ .../federation_features/federated.feature | 265 ++++++++++++++++++ 2 files changed, 297 insertions(+) diff --git a/build/integration/features/bootstrap/FederationContext.php b/build/integration/features/bootstrap/FederationContext.php index 409c5f3ca1e32..b37976987991e 100644 --- a/build/integration/features/bootstrap/FederationContext.php +++ b/build/integration/features/bootstrap/FederationContext.php @@ -197,6 +197,38 @@ public function deleteLastAcceptedRemoteShare($user) { $this->sendingToWith('DELETE', "/apps/files_sharing/api/v1/remote_shares/" . $this->lastAcceptedRemoteShareId, null); } + /** + * @When /^user "([^"]*)" deletes last accepted remote group share$/ + * @param string $user + */ + public function deleteLastAcceptedRemoteGroupShare($user) { + $this->asAn($user); + + // Accepting the group share creates an additional share exclusive for + // the user which needs to be got from the list of remote shares. + $this->sendingToWith('DELETE', "/apps/files_sharing/api/v1/remote_shares/" . $this->getRemoteShareWithParentId($this->lastAcceptedRemoteShareId), null); + } + + private function getRemoteShareWithParentId($parentId) { + // Ensure that the id is a string rather than a SimpleXMLElement. + $parentId = (string)$parentId; + + $this->sendingToWith('GET', "/apps/files_sharing/api/v1/remote_shares", null); + + $returnedShare = $this->getXmlResponse()->data[0]; + if ($returnedShare->element) { + for ($i = 0; $i < count($returnedShare->element); $i++) { + if (((string)$returnedShare->element[$i]->parent) === $parentId) { + return (string)$returnedShare->element[$i]->id; + } + } + } elseif (((string)$returnedShare->parent) === $parentId) { + return (string)$returnedShare->id; + } + + Assert::fail("No remote share found with parent id $parentId"); + } + /** * @When /^remote server is started$/ */ diff --git a/build/integration/federation_features/federated.feature b/build/integration/federation_features/federated.feature index 3e39f3d484737..2552b79b458fc 100644 --- a/build/integration/federation_features/federated.feature +++ b/build/integration/federation_features/federated.feature @@ -495,6 +495,53 @@ Feature: federated And sending "GET" to "/apps/files_sharing/api/v1/shares" And the list of returned shares has 0 shares + Scenario: Delete federated group share from another server + Given Using server "LOCAL" + And parameter "incoming_server2server_group_share_enabled" of app "files_sharing" is set to "yes" + And user "group0-user0" exists + And user "group0-user1" exists + And group "group0" exists + And user "group0-user0" belongs to group "group0" + And user "group0-user1" belongs to group "group0" + Given Using server "REMOTE" + And parameter "outgoing_server2server_group_share_enabled" of app "files_sharing" is set to "yes" + And user "user1" exists + # Rename file so it has a unique name in the target server (as the target + # server may have its own /textfile0.txt" file) + And User "user1" copies file "/textfile0.txt" to "/remote-share.txt" + And User "user1" from server "REMOTE" shares "/remote-share.txt" with group "group0" from server "LOCAL" + And As an "user1" + And sending "GET" to "/apps/files_sharing/api/v1/shares" + And the list of returned shares has 1 shares + And Using server "LOCAL" + And User "group0-user1" from server "LOCAL" accepts last pending share + And as "group0-user1" the file "/remote-share.txt" exists + And As an "group0-user1" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 1 shares + And User "group0-user0" from server "LOCAL" accepts last pending share + And as "group0-user0" the file "/remote-share.txt" exists + And As an "group0-user0" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 1 shares + When user "group0-user0" deletes last accepted remote group share + Then the OCS status code should be "100" + And the HTTP status code should be "200" + And as "group0-user0" the file "/remote-share.txt" does not exist + And As an "group0-user0" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 0 shares + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares/pending" + And the list of returned shares has 1 shares + And as "group0-user1" the file "/remote-share.txt" exists + And As an "group0-user1" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 1 shares + And Using server "REMOTE" + And As an "user1" + And sending "GET" to "/apps/files_sharing/api/v1/shares" + And the list of returned shares has 1 shares + Scenario: Delete federated share from another server no longer reachable Given Using server "LOCAL" And user "user0" exists @@ -519,6 +566,47 @@ Feature: federated And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" And the list of returned shares has 0 shares + Scenario: Delete federated group share from another server no longer reachable + Given Using server "LOCAL" + And parameter "incoming_server2server_group_share_enabled" of app "files_sharing" is set to "yes" + And user "group0-user0" exists + And user "group0-user1" exists + And group "group0" exists + And user "group0-user0" belongs to group "group0" + And user "group0-user1" belongs to group "group0" + Given Using server "REMOTE" + And parameter "outgoing_server2server_group_share_enabled" of app "files_sharing" is set to "yes" + And user "user1" exists + # Rename file so it has a unique name in the target server (as the target + # server may have its own /textfile0.txt" file) + And User "user1" copies file "/textfile0.txt" to "/remote-share.txt" + And User "user1" from server "REMOTE" shares "/remote-share.txt" with group "group0" from server "LOCAL" + And Using server "LOCAL" + And User "group0-user1" from server "LOCAL" accepts last pending share + And as "group0-user1" the file "/remote-share.txt" exists + And As an "group0-user1" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 1 shares + And User "group0-user0" from server "LOCAL" accepts last pending share + And as "group0-user0" the file "/remote-share.txt" exists + And As an "group0-user0" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 1 shares + And remote server is stopped + When user "group0-user0" deletes last accepted remote group share + Then the OCS status code should be "100" + And the HTTP status code should be "200" + And as "group0-user0" the file "/remote-share.txt" does not exist + And As an "group0-user0" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 0 shares + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares/pending" + And the list of returned shares has 1 shares + And as "group0-user1" the file "/remote-share.txt" exists + And As an "group0-user1" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 1 shares + Scenario: Delete federated share from another server when temporary unreachable Given Using server "LOCAL" And user "user0" exists @@ -548,6 +636,52 @@ Feature: federated And sending "GET" to "/apps/files_sharing/api/v1/shares" And the list of returned shares has 1 shares + Scenario: Delete federated group share from another server when temporary unreachable + Given Using server "LOCAL" + And parameter "incoming_server2server_group_share_enabled" of app "files_sharing" is set to "yes" + And user "group0-user0" exists + And user "group0-user1" exists + And group "group0" exists + And user "group0-user0" belongs to group "group0" + And user "group0-user1" belongs to group "group0" + Given Using server "REMOTE" + And parameter "outgoing_server2server_group_share_enabled" of app "files_sharing" is set to "yes" + And user "user1" exists + # Rename file so it has a unique name in the target server (as the target + # server may have its own /textfile0.txt" file) + And User "user1" copies file "/textfile0.txt" to "/remote-share.txt" + And User "user1" from server "REMOTE" shares "/remote-share.txt" with group "group0" from server "LOCAL" + And Using server "LOCAL" + And User "group0-user1" from server "LOCAL" accepts last pending share + And as "group0-user1" the file "/remote-share.txt" exists + And As an "group0-user1" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 1 shares + And User "group0-user0" from server "LOCAL" accepts last pending share + And as "group0-user0" the file "/remote-share.txt" exists + And As an "group0-user0" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 1 shares + And remote server is stopped + When user "group0-user0" deletes last accepted remote group share + Then the OCS status code should be "100" + And the HTTP status code should be "200" + And remote server is started + And as "group0-user0" the file "/remote-share.txt" does not exist + And As an "group0-user0" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 0 shares + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares/pending" + And the list of returned shares has 1 shares + And as "group0-user1" the file "/remote-share.txt" exists + And As an "group0-user1" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 1 shares + And Using server "REMOTE" + And As an "user1" + And sending "GET" to "/apps/files_sharing/api/v1/shares" + And the list of returned shares has 1 shares + Scenario: Delete federated share file from another server Given Using server "LOCAL" And user "user0" exists @@ -577,6 +711,52 @@ Feature: federated And sending "GET" to "/apps/files_sharing/api/v1/shares" And the list of returned shares has 0 shares + Scenario: Delete federated group share file from another server + Given Using server "LOCAL" + And parameter "incoming_server2server_group_share_enabled" of app "files_sharing" is set to "yes" + And user "group0-user0" exists + And user "group0-user1" exists + And group "group0" exists + And user "group0-user0" belongs to group "group0" + And user "group0-user1" belongs to group "group0" + Given Using server "REMOTE" + And parameter "outgoing_server2server_group_share_enabled" of app "files_sharing" is set to "yes" + And user "user1" exists + # Rename file so it has a unique name in the target server (as the target + # server may have its own /textfile0.txt" file) + And User "user1" copies file "/textfile0.txt" to "/remote-share.txt" + And User "user1" from server "REMOTE" shares "/remote-share.txt" with group "group0" from server "LOCAL" + And As an "user1" + And sending "GET" to "/apps/files_sharing/api/v1/shares" + And the list of returned shares has 1 shares + And Using server "LOCAL" + And User "group0-user1" from server "LOCAL" accepts last pending share + And as "group0-user1" the file "/remote-share.txt" exists + And As an "group0-user1" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 1 shares + And User "group0-user0" from server "LOCAL" accepts last pending share + And as "group0-user0" the file "/remote-share.txt" exists + And As an "group0-user0" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 1 shares + When User "group0-user0" deletes file "/remote-share.txt" + Then the HTTP status code should be "204" + And as "group0-user0" the file "/remote-share.txt" does not exist + And As an "group0-user0" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 0 shares + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares/pending" + And the list of returned shares has 1 shares + And as "group0-user1" the file "/remote-share.txt" exists + And As an "group0-user1" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 1 shares + And Using server "REMOTE" + And As an "user1" + And sending "GET" to "/apps/files_sharing/api/v1/shares" + And the list of returned shares has 1 shares + Scenario: Delete federated share file from another server no longer reachable Given Using server "LOCAL" And user "user0" exists @@ -600,6 +780,46 @@ Feature: federated And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" And the list of returned shares has 0 shares + Scenario: Delete federated group share file from another server no longer reachable + Given Using server "LOCAL" + And parameter "incoming_server2server_group_share_enabled" of app "files_sharing" is set to "yes" + And user "group0-user0" exists + And user "group0-user1" exists + And group "group0" exists + And user "group0-user0" belongs to group "group0" + And user "group0-user1" belongs to group "group0" + Given Using server "REMOTE" + And parameter "outgoing_server2server_group_share_enabled" of app "files_sharing" is set to "yes" + And user "user1" exists + # Rename file so it has a unique name in the target server (as the target + # server may have its own /textfile0.txt" file) + And User "user1" copies file "/textfile0.txt" to "/remote-share.txt" + And User "user1" from server "REMOTE" shares "/remote-share.txt" with group "group0" from server "LOCAL" + And Using server "LOCAL" + And User "group0-user1" from server "LOCAL" accepts last pending share + And as "group0-user1" the file "/remote-share.txt" exists + And As an "group0-user1" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 1 shares + And User "group0-user0" from server "LOCAL" accepts last pending share + And as "group0-user0" the file "/remote-share.txt" exists + And As an "group0-user0" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 1 shares + And remote server is stopped + When User "group0-user0" deletes file "/remote-share.txt" + Then the HTTP status code should be "204" + And as "group0-user0" the file "/remote-share.txt" does not exist + And As an "group0-user0" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 0 shares + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares/pending" + And the list of returned shares has 1 shares + And as "group0-user1" the file "/remote-share.txt" exists + And As an "group0-user1" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 1 shares + Scenario: Delete federated share file from another server when temporary unreachable Given Using server "LOCAL" And user "user0" exists @@ -627,3 +847,48 @@ Feature: federated And As an "user1" And sending "GET" to "/apps/files_sharing/api/v1/shares" And the list of returned shares has 1 shares + + Scenario: Delete federated group share file from another server when temporary unreachable + Given Using server "LOCAL" + And parameter "incoming_server2server_group_share_enabled" of app "files_sharing" is set to "yes" + And user "group0-user0" exists + And user "group0-user1" exists + And group "group0" exists + And user "group0-user0" belongs to group "group0" + And user "group0-user1" belongs to group "group0" + Given Using server "REMOTE" + And parameter "outgoing_server2server_group_share_enabled" of app "files_sharing" is set to "yes" + And user "user1" exists + # Rename file so it has a unique name in the target server (as the target + # server may have its own /textfile0.txt" file) + And User "user1" copies file "/textfile0.txt" to "/remote-share.txt" + And User "user1" from server "REMOTE" shares "/remote-share.txt" with group "group0" from server "LOCAL" + And Using server "LOCAL" + And User "group0-user1" from server "LOCAL" accepts last pending share + And as "group0-user1" the file "/remote-share.txt" exists + And As an "group0-user1" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 1 shares + And User "group0-user0" from server "LOCAL" accepts last pending share + And as "group0-user0" the file "/remote-share.txt" exists + And As an "group0-user0" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 1 shares + And remote server is stopped + When User "group0-user0" deletes file "/remote-share.txt" + Then the HTTP status code should be "204" + And remote server is started + And as "group0-user0" the file "/remote-share.txt" does not exist + And As an "group0-user0" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 0 shares + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares/pending" + And the list of returned shares has 1 shares + And as "group0-user1" the file "/remote-share.txt" exists + And As an "group0-user1" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 1 shares + And Using server "REMOTE" + And As an "user1" + And sending "GET" to "/apps/files_sharing/api/v1/shares" + And the list of returned shares has 1 shares From 514ce8658d40030f54beb02c61bf851a65a2f2fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Fri, 22 Mar 2024 14:14:06 +0100 Subject: [PATCH 07/12] test: Add integration tests for deleting federated group shares with remote MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the sharer removes a federated group share it is removed for all the members of the group that received it. Due to that, if the remote server was temporary down when the share was removed but once it is running again a user tries to access the share should be removed for all of them, rather than being marked as pending for that user (which is not the current behaviour and will need to be fixed in a follow up commit). Signed-off-by: Daniel Calviño Sánchez --- .../federation_features/federated.feature | 146 ++++++++++++++++++ 1 file changed, 146 insertions(+) diff --git a/build/integration/federation_features/federated.feature b/build/integration/federation_features/federated.feature index 2552b79b458fc..033eeecdef1f8 100644 --- a/build/integration/federation_features/federated.feature +++ b/build/integration/federation_features/federated.feature @@ -401,6 +401,58 @@ Feature: federated And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" And the list of returned shares has 0 shares + Scenario: Delete federated group share with another server + Given Using server "LOCAL" + And parameter "outgoing_server2server_group_share_enabled" of app "files_sharing" is set to "yes" + And user "user0" exists + Given Using server "REMOTE" + And parameter "incoming_server2server_group_share_enabled" of app "files_sharing" is set to "yes" + And user "group1-user0" exists + And user "group1-user1" exists + And group "group1" exists + And user "group1-user0" belongs to group "group1" + And user "group1-user1" belongs to group "group1" + # Rename file so it has a unique name in the target server (as the target + # server may have its own /textfile0.txt" file) + And User "user0" copies file "/textfile0.txt" to "/remote-share.txt" + And User "user0" from server "LOCAL" shares "/remote-share.txt" with group "group1" from server "REMOTE" + And Using server "LOCAL" + And As an "user0" + And sending "GET" to "/apps/files_sharing/api/v1/shares" + And the list of returned shares has 1 shares + And Using server "REMOTE" + And User "group1-user0" from server "REMOTE" accepts last pending share + And as "group1-user0" the file "/remote-share.txt" exists + And As an "group1-user0" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 1 shares + And User "group1-user1" from server "REMOTE" accepts last pending share + And as "group1-user1" the file "/remote-share.txt" exists + And As an "group1-user1" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 1 shares + And Using server "LOCAL" + When As an "user0" + And Deleting last share + Then the OCS status code should be "100" + And the HTTP status code should be "200" + And As an "user0" + And sending "GET" to "/apps/files_sharing/api/v1/shares" + And the list of returned shares has 0 shares + And Using server "REMOTE" + And as "group1-user0" the file "/remote-share.txt" does not exist + And As an "group1-user0" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 0 shares + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares/pending" + And the list of returned shares has 0 shares + And as "group1-user1" the file "/remote-share.txt" does not exist + And As an "group1-user1" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 0 shares + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares/pending" + And the list of returned shares has 0 shares + Scenario: Delete federated share with another server no longer reachable Given Using server "LOCAL" And user "user0" exists @@ -430,6 +482,46 @@ Feature: federated And sending "GET" to "/apps/files_sharing/api/v1/shares" And the list of returned shares has 0 shares + Scenario: Delete federated group share with another server no longer reachable + Given Using server "LOCAL" + And parameter "outgoing_server2server_group_share_enabled" of app "files_sharing" is set to "yes" + And user "user0" exists + Given Using server "REMOTE" + And parameter "incoming_server2server_group_share_enabled" of app "files_sharing" is set to "yes" + And user "group1-user0" exists + And user "group1-user1" exists + And group "group1" exists + And user "group1-user0" belongs to group "group1" + And user "group1-user1" belongs to group "group1" + # Rename file so it has a unique name in the target server (as the target + # server may have its own /textfile0.txt" file) + And User "user0" copies file "/textfile0.txt" to "/remote-share.txt" + And User "user0" from server "LOCAL" shares "/remote-share.txt" with group "group1" from server "REMOTE" + And Using server "LOCAL" + And As an "user0" + And sending "GET" to "/apps/files_sharing/api/v1/shares" + And the list of returned shares has 1 shares + And Using server "REMOTE" + And User "group1-user0" from server "REMOTE" accepts last pending share + And as "group1-user0" the file "/remote-share.txt" exists + And As an "group1-user0" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 1 shares + And User "group1-user1" from server "REMOTE" accepts last pending share + And as "group1-user1" the file "/remote-share.txt" exists + And As an "group1-user1" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 1 shares + And remote server is stopped + And Using server "LOCAL" + When As an "user0" + And Deleting last share + Then the OCS status code should be "100" + And the HTTP status code should be "200" + And As an "user0" + And sending "GET" to "/apps/files_sharing/api/v1/shares" + And the list of returned shares has 0 shares + Scenario: Delete federated share with another server when temporary unreachable Given Using server "LOCAL" And user "user0" exists @@ -465,6 +557,60 @@ Feature: federated And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" And the list of returned shares has 0 shares + Scenario: Delete federated group share with another server when temporary unreachable + Given Using server "LOCAL" + And parameter "outgoing_server2server_group_share_enabled" of app "files_sharing" is set to "yes" + And user "user0" exists + Given Using server "REMOTE" + And parameter "incoming_server2server_group_share_enabled" of app "files_sharing" is set to "yes" + And user "group1-user0" exists + And user "group1-user1" exists + And group "group1" exists + And user "group1-user0" belongs to group "group1" + And user "group1-user1" belongs to group "group1" + # Rename file so it has a unique name in the target server (as the target + # server may have its own /textfile0.txt" file) + And User "user0" copies file "/textfile0.txt" to "/remote-share.txt" + And User "user0" from server "LOCAL" shares "/remote-share.txt" with group "group1" from server "REMOTE" + And Using server "LOCAL" + And As an "user0" + And sending "GET" to "/apps/files_sharing/api/v1/shares" + And the list of returned shares has 1 shares + And Using server "REMOTE" + And User "group1-user0" from server "REMOTE" accepts last pending share + And as "group1-user0" the file "/remote-share.txt" exists + And As an "group1-user0" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 1 shares + And User "group1-user1" from server "REMOTE" accepts last pending share + And as "group1-user1" the file "/remote-share.txt" exists + And As an "group1-user1" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 1 shares + And remote server is stopped + And Using server "LOCAL" + When As an "user0" + And Deleting last share + Then the OCS status code should be "100" + And the HTTP status code should be "200" + And As an "user0" + And sending "GET" to "/apps/files_sharing/api/v1/shares" + And the list of returned shares has 0 shares + And remote server is started + And Using server "REMOTE" + And as "group1-user0" the file "/remote-share.txt" does not exist + And As an "group1-user0" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 0 shares + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares/pending" + And the list of returned shares has 0 shares + And as "group1-user1" the file "/remote-share.txt" does not exist + And As an "group1-user1" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + And the list of returned shares has 0 shares + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares/pending" + And the list of returned shares has 0 shares + Scenario: Delete federated share from another server Given Using server "LOCAL" And user "user0" exists From 86947cb96e16e372b7821ec30e6ffc2323dc010e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Fri, 22 Mar 2024 14:15:57 +0100 Subject: [PATCH 08/12] fix: Fix federated group shares when no longer found in remote server MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a federated group share with another server is deleted a notification is sent to the remote server to delete the share. If that notification is not received then when the remote server tries to access the share again it will not be found in the sharer server, so it will be locally unshared in the remote server. This works fine for user shares, as in that case the share is deleted in the remote server, but locally unsharing a received federated group share only marks it as pending again for the user that tried to access it. Due to that the received federated group share is left dangling in the remote server, and it is kept forever in the list of pending shares for the users in the group. To solve that now the received federated group share is fully removed instead of simply unshared when no longer available in the sharer server. Note that all of the above applies only when the sharer server is still available and it returns a "not found" (or "forbidden") error when the remote server tries to access the share. If the sharer server is not available accessing the received share does not fully remove the group share, as in that case it is not possible to know if the sharer server is temporarily unavailable or gone forever. Signed-off-by: Daniel Calviño Sánchez --- apps/files_sharing/lib/External/Manager.php | 32 +++++++++++++++++++-- apps/files_sharing/lib/External/Storage.php | 4 +-- 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/apps/files_sharing/lib/External/Manager.php b/apps/files_sharing/lib/External/Manager.php index f18d8346dc44d..77f5c6bd17266 100644 --- a/apps/files_sharing/lib/External/Manager.php +++ b/apps/files_sharing/lib/External/Manager.php @@ -599,7 +599,18 @@ public function setMountPoint($source, $target) { return $result; } - public function removeShare($mountPoint): bool { + /** + * Remove a share based on its mountpoint. + * + * User shares are always fully removed; group shares by default are just + * marked again as pending for the current user, unless explicitly forced + * to be fully removed (parent group share and all its sub-shares). + * + * @param bool $force True to fully removed a group share, false to mark it + * as pending for the current user + * @return bool True if the share could be removed, false otherwise + */ + public function removeShare(string $mountPoint, bool $force = false): bool { try { $mountPointObj = $this->mountManager->find($mountPoint); } catch (NotFoundException $e) { @@ -617,7 +628,7 @@ public function removeShare($mountPoint): bool { try { $getShare = $this->connection->prepare(' - SELECT `remote`, `share_token`, `remote_id`, `share_type`, `id` + SELECT `remote`, `share_token`, `remote_id`, `share_type`, `id`, `parent` FROM `*PREFIX*share_external` WHERE `mountpoint_hash` = ? AND `user` = ?'); $result = $getShare->execute([$hash, $this->uid]); @@ -638,7 +649,22 @@ public function removeShare($mountPoint): bool { $deleteResult = $query->execute([(int)$share['id']]); $deleteResult->closeCursor(); } elseif ($share !== false && (int)$share['share_type'] === IShare::TYPE_GROUP) { - $this->updateAccepted((int)$share['id'], false); + if ($force) { + $qb = $this->connection->getQueryBuilder(); + // delete group share entry and matching sub-entries + $qb->delete('share_external') + ->where( + $qb->expr()->orX( + $qb->expr()->eq('id', $qb->createParameter('share_parent_id')), + $qb->expr()->eq('parent', $qb->createParameter('share_parent_id')) + ) + ); + + $qb->setParameter('share_parent_id', $share['parent']); + $qb->executeStatement(); + } else { + $this->updateAccepted((int)$share['id'], false); + } } $this->removeReShares($id); diff --git a/apps/files_sharing/lib/External/Storage.php b/apps/files_sharing/lib/External/Storage.php index 7b64690d53e17..425cf178c9bfe 100644 --- a/apps/files_sharing/lib/External/Storage.php +++ b/apps/files_sharing/lib/External/Storage.php @@ -239,7 +239,7 @@ public function checkStorageAvailability() { // valid Nextcloud instance means that the public share no longer exists // since this is permanent (re-sharing the file will create a new token) // we remove the invalid storage - $this->manager->removeShare($this->mountPoint); + $this->manager->removeShare($this->mountPoint, true); $this->manager->getMountManager()->removeMount($this->mountPoint); throw new StorageInvalidException("Remote share not found", 0, $e); } else { @@ -248,7 +248,7 @@ public function checkStorageAvailability() { } } catch (ForbiddenException $e) { // auth error, remove share for now (provide a dialog in the future) - $this->manager->removeShare($this->mountPoint); + $this->manager->removeShare($this->mountPoint, true); $this->manager->getMountManager()->removeMount($this->mountPoint); throw new StorageInvalidException("Auth error when getting remote share"); } catch (\GuzzleHttp\Exception\ConnectException $e) { From ad659f3ddc98f1391415e551e6dfa76d4eed1eb7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Sun, 31 Mar 2024 03:19:23 +0200 Subject: [PATCH 09/12] refactor: Add another user and group to the test case MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This will be needed to test group shares with several users, as the external share managers returned by "createManagerForUser" use the group manager stored in the test case attribute. Signed-off-by: Daniel Calviño Sánchez --- .../tests/External/ManagerTest.php | 45 +++++++++++++++---- 1 file changed, 36 insertions(+), 9 deletions(-) diff --git a/apps/files_sharing/tests/External/ManagerTest.php b/apps/files_sharing/tests/External/ManagerTest.php index 0e80479eafed5..fcf88b1b7ae6d 100644 --- a/apps/files_sharing/tests/External/ManagerTest.php +++ b/apps/files_sharing/tests/External/ManagerTest.php @@ -99,6 +99,14 @@ class ManagerTest extends TestCase { * @var \OCP\IUser */ private $user; + + private $uid2; + + /** + * @var \OCP\IUser + */ + private $user2; + private $testMountProvider; /** @var IEventDispatcher|\PHPUnit\Framework\MockObject\MockObject */ private $eventDispatcher; @@ -108,6 +116,8 @@ protected function setUp(): void { $this->uid = $this->getUniqueID('user'); $this->user = $this->createUser($this->uid, ''); + $this->uid2 = $this->getUniqueID('user2'); + $this->user2 = $this->createUser($this->uid2, ''); $this->mountManager = new \OC\Files\Mount\Manager($this->createMock(SetupManagerFactory::class)); $this->clientService = $this->getMockBuilder(IClientService::class) ->disableOriginalConstructor()->getMock(); @@ -140,18 +150,35 @@ protected function setUp(): void { $group1 = $this->createMock(IGroup::class); $group1->expects($this->any())->method('getGID')->willReturn('group1'); - $group1->expects($this->any())->method('inGroup')->with($this->user)->willReturn(true); + $group1->expects($this->any())->method('inGroup') + ->will($this->returnValueMap([ + [$this->user, true], + [$this->user2, true], + ])); $group2 = $this->createMock(IGroup::class); $group2->expects($this->any())->method('getGID')->willReturn('group2'); $group2->expects($this->any())->method('inGroup')->with($this->user)->willReturn(true); - $this->userManager->expects($this->any())->method('get')->willReturn($this->user); - $this->groupManager->expects($this->any())->method(('getUserGroups'))->willReturn([$group1, $group2]); + $group3 = $this->createMock(IGroup::class); + $group3->expects($this->any())->method('getGID')->willReturn('group3'); + $group3->expects($this->any())->method('inGroup')->with($this->user2)->willReturn(true); + + $this->userManager->expects($this->any())->method('get') + ->will($this->returnValueMap([ + [$this->uid, $this->user], + [$this->uid2, $this->user2], + ])); + $this->groupManager->expects($this->any())->method(('getUserGroups')) + ->will($this->returnValueMap([ + [$this->user, [$group1, $group2]], + [$this->user2, [$group1, $group3]], + ])); $this->groupManager->expects($this->any())->method(('get')) ->will($this->returnValueMap([ ['group1', $group1], ['group2', $group2], + ['group3', $group3], ])); } @@ -656,7 +683,7 @@ public function testDeleteUserShares() { $this->assertTrue($this->manager->acceptShare($groupShare['id'])); // user 2 shares - $manager2 = $this->createManagerForUser('user2'); + $manager2 = $this->createManagerForUser($this->uid2); $shareData2 = [ 'remote' => 'http://localhost', 'token' => 'token1', @@ -665,7 +692,7 @@ public function testDeleteUserShares() { 'owner' => 'foobar', 'shareType' => IShare::TYPE_USER, 'accepted' => false, - 'user' => 'user2', + 'user' => $this->uid2, 'remoteId' => '2342' ]; $this->assertSame(null, call_user_func_array([$manager2, 'addShare'], $shareData2)); @@ -687,7 +714,7 @@ public function testDeleteUserShares() { $this->assertEquals($user2Shares[0]['share_type'], IShare::TYPE_GROUP); $this->assertEquals($user2Shares[0]['user'], 'group1'); $this->assertEquals($user2Shares[1]['share_type'], IShare::TYPE_USER); - $this->assertEquals($user2Shares[1]['user'], 'user2'); + $this->assertEquals($user2Shares[1]['user'], $this->uid2); } public function testDeleteGroupShares() { @@ -701,7 +728,7 @@ public function testDeleteGroupShares() { $this->assertTrue($this->manager->acceptShare($groupShare['id'])); // user 2 shares - $manager2 = $this->createManagerForUser('user2'); + $manager2 = $this->createManagerForUser($this->uid2); $shareData2 = [ 'remote' => 'http://localhost', 'token' => 'token1', @@ -710,7 +737,7 @@ public function testDeleteGroupShares() { 'owner' => 'foobar', 'shareType' => IShare::TYPE_USER, 'accepted' => false, - 'user' => 'user2', + 'user' => $this->uid2, 'remoteId' => '2342' ]; $this->assertSame(null, call_user_func_array([$manager2, 'addShare'], $shareData2)); @@ -730,7 +757,7 @@ public function testDeleteGroupShares() { $user2Shares = $manager2->getOpenShares(); $this->assertCount(1, $user2Shares); $this->assertEquals($user2Shares[0]['share_type'], IShare::TYPE_USER); - $this->assertEquals($user2Shares[0]['user'], 'user2'); + $this->assertEquals($user2Shares[0]['user'], $this->uid2); } /** From 74e8524ae1e03286f364034e3cc8dd0bf0523af2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Sun, 31 Mar 2024 03:20:12 +0200 Subject: [PATCH 10/12] refactor: Make test group share parameters configurable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Daniel Calviño Sánchez --- apps/files_sharing/tests/External/ManagerTest.php | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/apps/files_sharing/tests/External/ManagerTest.php b/apps/files_sharing/tests/External/ManagerTest.php index fcf88b1b7ae6d..3facf29042f7c 100644 --- a/apps/files_sharing/tests/External/ManagerTest.php +++ b/apps/files_sharing/tests/External/ManagerTest.php @@ -498,24 +498,24 @@ private function createTestUserShare($userId = 'user1') { return $shareData; } - private function createTestGroupShare($groupId = 'group1') { + private function createTestGroupShare($groupId = 'group1', $token = 'token1', $name = '/SharedFolder', $remoteId = '2342') { $shareData = [ 'remote' => 'http://localhost', - 'token' => 'token1', + 'token' => $token, 'password' => '', - 'name' => '/SharedFolder', + 'name' => $name, 'owner' => 'foobar', 'shareType' => IShare::TYPE_GROUP, 'accepted' => false, 'user' => $groupId, - 'remoteId' => '2342' + 'remoteId' => $remoteId ]; $this->assertSame(null, call_user_func_array([$this->manager, 'addShare'], $shareData)); $allShares = self::invokePrivate($this->manager, 'getShares', [null]); foreach ($allShares as $share) { - if ($share['user'] === $groupId) { + if ($share['user'] === $groupId && $share['share_token'] == $token && $share['name'] == $name && $share['remote_id'] == $remoteId) { // this will hold the main group entry $groupShare = $share; break; From 3749b31f4e5f03f775cf997f3daea75d0e165dbd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Sun, 31 Mar 2024 03:21:11 +0200 Subject: [PATCH 11/12] refactor: Make external share manager for test group share configurable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Daniel Calviño Sánchez --- apps/files_sharing/tests/External/ManagerTest.php | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/apps/files_sharing/tests/External/ManagerTest.php b/apps/files_sharing/tests/External/ManagerTest.php index 3facf29042f7c..349c24f77c8c1 100644 --- a/apps/files_sharing/tests/External/ManagerTest.php +++ b/apps/files_sharing/tests/External/ManagerTest.php @@ -498,7 +498,9 @@ private function createTestUserShare($userId = 'user1') { return $shareData; } - private function createTestGroupShare($groupId = 'group1', $token = 'token1', $name = '/SharedFolder', $remoteId = '2342') { + private function createTestGroupShare($groupId = 'group1', $token = 'token1', $name = '/SharedFolder', $remoteId = '2342', $manager = null) { + $manager ??= $this->manager; + $shareData = [ 'remote' => 'http://localhost', 'token' => $token, @@ -511,9 +513,9 @@ private function createTestGroupShare($groupId = 'group1', $token = 'token1', $n 'remoteId' => $remoteId ]; - $this->assertSame(null, call_user_func_array([$this->manager, 'addShare'], $shareData)); + $this->assertSame(null, call_user_func_array([$manager, 'addShare'], $shareData)); - $allShares = self::invokePrivate($this->manager, 'getShares', [null]); + $allShares = self::invokePrivate($manager, 'getShares', [null]); foreach ($allShares as $share) { if ($share['user'] === $groupId && $share['share_token'] == $token && $share['name'] == $name && $share['remote_id'] == $remoteId) { // this will hold the main group entry From 718560a4a4e16b0af9cfc7edce80fe23364fb2f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Sun, 31 Mar 2024 03:21:42 +0200 Subject: [PATCH 12/12] test: Add unit tests for removing federated group shares MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Daniel Calviño Sánchez --- .../tests/External/ManagerTest.php | 207 ++++++++++++++++++ 1 file changed, 207 insertions(+) diff --git a/apps/files_sharing/tests/External/ManagerTest.php b/apps/files_sharing/tests/External/ManagerTest.php index 349c24f77c8c1..da8f336014c24 100644 --- a/apps/files_sharing/tests/External/ManagerTest.php +++ b/apps/files_sharing/tests/External/ManagerTest.php @@ -672,6 +672,213 @@ public function testDeclineThenAcceptGroupShareAgainThroughSubShare() { $this->verifyAcceptedGroupShare($shareData); } + public function testRemoveGroupShare() { + [$shareData, $groupShare] = $this->createTestGroupShare(); + $this->assertTrue($this->manager->acceptShare($groupShare['id'])); + + // Setup mounts to remove share through mountpoint + $this->setupMounts(); + + $this->manager->removeShare($this->uid . '/files/' . $groupShare['name']); + + // Refresh mounts after removing share + $this->setupMounts(); + + $this->assertNotMount($groupShare['name']); + + $openShares = $this->manager->getOpenShares(); + $this->assertCount(1, $openShares); + // After removing/declining a group share the sub-share of the user + // rather than the group share is listed, so the mountpoint is no longer + // a temporary one, even if it was removed. + $this->assertExternalShareEntry($shareData, $openShares[0], 1, $shareData['name'], $this->uid); + + $acceptedShares = $this->manager->getAcceptedShares(); + $this->assertCount(0, $acceptedShares); + } + + public function testRemoveGroupShareWhenAcceptedByOtherUser() { + $manager2 = $this->createManagerForUser($this->uid2); + + [$shareData, $groupShare] = $this->createTestGroupShare(); + $this->assertTrue($this->manager->acceptShare($groupShare['id'])); + $this->assertTrue($manager2->acceptShare($groupShare['id'])); + + // Setup mounts to remove share through mountpoint + $this->setupMounts(); + + $this->manager->removeShare($this->uid . '/files/' . $groupShare['name']); + + // Refresh mounts after removing share + $this->setupMounts(); + + $openShares = $this->manager->getOpenShares(); + $this->assertCount(1, $openShares); + // After removing/declining a group share the sub-share of the user + // rather than the group share is listed, so the mountpoint is no longer + // a temporary one, even if it was removed. + $this->assertExternalShareEntry($shareData, $openShares[0], 1, $shareData['name'], $this->uid); + + $acceptedShares = $this->manager->getAcceptedShares(); + $this->assertCount(0, $acceptedShares); + + $user2OpenShares = $manager2->getOpenShares(); + $this->assertCount(0, $user2OpenShares); + + $user2ShareData = $shareData; + $user2ShareData['accepted'] = true; + + $user2AcceptedShares = $manager2->getAcceptedShares(); + $this->assertCount(1, $user2AcceptedShares); + $this->assertExternalShareEntry($user2ShareData, $user2AcceptedShares[0], 1, $shareData['name'], $this->uid2); + } + + public function testRemoveGroupShareWhenThereAreSeveralShares() { + $manager2 = $this->createManagerForUser($this->uid2); + + [$shareData1, $groupShare1] = $this->createTestGroupShare('group1', 'token1', '/SharedFolder', '2341'); + $this->assertTrue($this->manager->acceptShare($groupShare1['id'])); + [$shareData2, $groupShare2] = $this->createTestGroupShare('group1', 'token2', '/SharedFolder2', '2342'); + $this->assertTrue($this->manager->acceptShare($groupShare2['id'])); + [$shareData3, $groupShare3] = $this->createTestGroupShare('group1', 'token3', '/SharedFolder3', '2343'); + [$shareData4, $groupShare4] = $this->createTestGroupShare('group2', 'token4', '/SharedFolder4', '2344'); + [$shareData5, $groupShare5] = $this->createTestGroupShare('group3', 'token1', '/SharedFolder', '2341', $manager2); + $this->assertTrue($manager2->acceptShare($groupShare5['id'])); + + // Setup mounts to remove share through mountpoint + $this->setupMounts(); + + $this->manager->removeShare($this->uid . '/files/' . $groupShare2['name']); + + // Refresh mounts after removing share + $this->setupMounts(); + + $this->assertNotMount($groupShare2['name']); + + $openShares = $this->manager->getOpenShares(); + $this->assertCount(3, $openShares); + // After removing/declining a group share the sub-share of the user + // rather than the group share is listed, so the mountpoint is no longer + // a temporary one, even if it was removed. + $this->assertExternalShareEntry($shareData2, $openShares[0], 2, $shareData2['name'], $this->uid); + $this->assertExternalShareEntry($shareData3, $openShares[1], 3, '{{TemporaryMountPointName#' . $shareData3['name'] . '}}', 'group1'); + $this->assertExternalShareEntry($shareData4, $openShares[2], 4, '{{TemporaryMountPointName#' . $shareData4['name'] . '}}', 'group2'); + + $user1ShareData1 = $shareData1; + $user1ShareData1['accepted'] = true; + + $acceptedShares = $this->manager->getAcceptedShares(); + $this->assertCount(1, $acceptedShares); + $this->assertExternalShareEntry($user1ShareData1, $acceptedShares[0], 1, $user1ShareData1['name'], $this->uid); + + $user2OpenShares = $manager2->getOpenShares(); + $this->assertCount(3, $user2OpenShares); + $this->assertExternalShareEntry($shareData1, $user2OpenShares[0], 1, '{{TemporaryMountPointName#' . $shareData1['name'] . '}}', 'group1'); + $this->assertExternalShareEntry($shareData2, $user2OpenShares[1], 2, '{{TemporaryMountPointName#' . $shareData2['name'] . '}}', 'group1'); + $this->assertExternalShareEntry($shareData3, $user2OpenShares[2], 3, '{{TemporaryMountPointName#' . $shareData3['name'] . '}}', 'group1'); + + $shareData5['accepted'] = true; + + $user2AcceptedShares = $manager2->getAcceptedShares(); + $this->assertCount(1, $user2AcceptedShares); + $this->assertExternalShareEntry($shareData5, $user2AcceptedShares[0], 5, $shareData1['name'], $this->uid2); + } + + public function testForceRemoveGroupShare() { + [$shareData, $groupShare] = $this->createTestGroupShare(); + $this->assertTrue($this->manager->acceptShare($groupShare['id'])); + + // Setup mounts to remove share through mountpoint + $this->setupMounts(); + + $this->manager->removeShare($this->uid . '/files/' . $groupShare['name'], true); + + // Refresh mounts after removing share + $this->setupMounts(); + + $this->assertNotMount($groupShare['name']); + + $openShares = $this->manager->getOpenShares(); + $this->assertCount(0, $openShares); + + $acceptedShares = $this->manager->getAcceptedShares(); + $this->assertCount(0, $acceptedShares); + } + + public function testForceRemoveGroupShareWhenAcceptedByOtherUser() { + $manager2 = $this->createManagerForUser($this->uid2); + + [$shareData, $groupShare] = $this->createTestGroupShare(); + $this->assertTrue($this->manager->acceptShare($groupShare['id'])); + $this->assertTrue($manager2->acceptShare($groupShare['id'])); + + // Setup mounts to remove share through mountpoint + $this->setupMounts(); + + $this->manager->removeShare($this->uid . '/files/' . $groupShare['name'], true); + + // Refresh mounts after removing share + $this->setupMounts(); + + $openShares = $this->manager->getOpenShares(); + $this->assertCount(0, $openShares); + + $acceptedShares = $this->manager->getAcceptedShares(); + $this->assertCount(0, $acceptedShares); + + $user2OpenShares = $manager2->getOpenShares(); + $this->assertCount(0, $openShares); + + $user2AcceptedShares = $manager2->getAcceptedShares(); + $this->assertCount(0, $user2AcceptedShares); + } + + public function testForceRemoveGroupShareWhenThereAreSeveralShares() { + $manager2 = $this->createManagerForUser($this->uid2); + + [$shareData1, $groupShare1] = $this->createTestGroupShare('group1', 'token1', '/SharedFolder', '2341'); + $this->assertTrue($this->manager->acceptShare($groupShare1['id'])); + [$shareData2, $groupShare2] = $this->createTestGroupShare('group1', 'token2', '/SharedFolder2', '2342'); + $this->assertTrue($this->manager->acceptShare($groupShare2['id'])); + [$shareData3, $groupShare3] = $this->createTestGroupShare('group1', 'token3', '/SharedFolder3', '2343'); + [$shareData4, $groupShare4] = $this->createTestGroupShare('group2', 'token4', '/SharedFolder4', '2344'); + [$shareData5, $groupShare5] = $this->createTestGroupShare('group3', 'token1', '/SharedFolder', '2341', $manager2); + $this->assertTrue($manager2->acceptShare($groupShare5['id'])); + + // Setup mounts to remove share through mountpoint + $this->setupMounts(); + + $this->manager->removeShare($this->uid . '/files/' . $groupShare2['name'], true); + + // Refresh mounts after removing share + $this->setupMounts(); + + $this->assertNotMount($groupShare2['name']); + + $openShares = $this->manager->getOpenShares(); + $this->assertCount(2, $openShares); + $this->assertExternalShareEntry($shareData3, $openShares[0], 3, '{{TemporaryMountPointName#' . $shareData3['name'] . '}}', 'group1'); + $this->assertExternalShareEntry($shareData4, $openShares[1], 4, '{{TemporaryMountPointName#' . $shareData4['name'] . '}}', 'group2'); + + $user1ShareData1 = $shareData1; + $user1ShareData1['accepted'] = true; + + $acceptedShares = $this->manager->getAcceptedShares(); + $this->assertCount(1, $acceptedShares); + $this->assertExternalShareEntry($user1ShareData1, $acceptedShares[0], 1, $user1ShareData1['name'], $this->uid); + + $user2OpenShares = $manager2->getOpenShares(); + $this->assertCount(2, $user2OpenShares); + $this->assertExternalShareEntry($shareData1, $user2OpenShares[0], 1, '{{TemporaryMountPointName#' . $shareData1['name'] . '}}', 'group1'); + $this->assertExternalShareEntry($shareData3, $user2OpenShares[1], 3, '{{TemporaryMountPointName#' . $shareData3['name'] . '}}', 'group1'); + + $shareData5['accepted'] = true; + + $user2AcceptedShares = $manager2->getAcceptedShares(); + $this->assertCount(1, $user2AcceptedShares); + $this->assertExternalShareEntry($shareData5, $user2AcceptedShares[0], 5, $shareData1['name'], $this->uid2); + } + public function testDeleteUserShares() { // user 1 shares