From 92e10726f9876c4959957207e335eb5943116721 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Fri, 22 Jan 2021 14:34:02 +0100 Subject: [PATCH 1/6] Add integration tests for deleting federated shares MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Daniel Calviño Sánchez --- .../features/bootstrap/FederationContext.php | 14 +++ .../federation_features/federated.feature | 96 +++++++++++++++++-- 2 files changed, 102 insertions(+), 8 deletions(-) diff --git a/build/integration/features/bootstrap/FederationContext.php b/build/integration/features/bootstrap/FederationContext.php index 2754cf668e61f..d859afc610fa6 100644 --- a/build/integration/features/bootstrap/FederationContext.php +++ b/build/integration/features/bootstrap/FederationContext.php @@ -39,6 +39,9 @@ class FederationContext implements Context, SnippetAcceptingContext { use AppConfiguration; use CommandLine; + /** @var string */ + private $lastAcceptedRemoteShareId; + /** * @BeforeScenario */ @@ -109,6 +112,17 @@ public function acceptLastPendingShare($user, $server) { $this->theHTTPStatusCodeShouldBe('200'); $this->theOCSStatusCodeShouldBe('100'); $this->usingServer($previous); + + $this->lastAcceptedRemoteShareId = $share_id; + } + + /** + * @When /^user "([^"]*)" deletes last accepted remote share$/ + * @param string $user + */ + public function deleteLastAcceptedRemoteShare($user) { + $this->asAn($user); + $this->sendingToWith('DELETE', "/apps/files_sharing/api/v1/remote_shares/" . $this->lastAcceptedRemoteShareId, null); } protected function resetAppConfigs() { diff --git a/build/integration/federation_features/federated.feature b/build/integration/federation_features/federated.feature index 17ec6b4b43e22..46794e02ff8a2 100644 --- a/build/integration/federation_features/federated.feature +++ b/build/integration/federation_features/federated.feature @@ -278,13 +278,93 @@ Feature: federated + Scenario: Delete federated share with another server + 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 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 "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 Using server "REMOTE" + When As an "user1" + 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 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 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 + 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 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 "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 + 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 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 0 shares - - - - - - - - + Scenario: Delete federated share file from another server + 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 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 "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 + When User "user0" deletes file "/remote-share.txt" + Then the HTTP status code should be "204" + 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 0 shares From 60b8116f4b928f776ad3c62a8d10b6ef348f6aff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Thu, 8 Apr 2021 21:00:11 +0200 Subject: [PATCH 2/6] Add integration tests for listing federated shares MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Daniel Calviño Sánchez --- .../features/bootstrap/FederationContext.php | 32 ++++++++++++++ .../federation_features/federated.feature | 42 +++++++++++++++++++ 2 files changed, 74 insertions(+) diff --git a/build/integration/features/bootstrap/FederationContext.php b/build/integration/features/bootstrap/FederationContext.php index d859afc610fa6..a4472c54b1be3 100644 --- a/build/integration/features/bootstrap/FederationContext.php +++ b/build/integration/features/bootstrap/FederationContext.php @@ -28,6 +28,7 @@ */ use Behat\Behat\Context\Context; use Behat\Behat\Context\SnippetAcceptingContext; +use Behat\Gherkin\Node\TableNode; require __DIR__ . '/../../vendor/autoload.php'; @@ -96,6 +97,37 @@ public function federateGroupSharing($sharerUser, $sharerServer, $sharerPath, $s $this->usingServer($previous); } + /** + * @Then remote share :count is returned with + * + * @param int $number + * @param TableNode $body + */ + public function remoteShareXIsReturnedWith(int $number, TableNode $body) { + $this->theHTTPStatusCodeShouldBe('200'); + $this->theOCSStatusCodeShouldBe('100'); + + if (!($body instanceof TableNode)) { + return; + } + + $returnedShare = $this->getXmlResponse()->data[0]; + if ($returnedShare->element) { + $returnedShare = $returnedShare->element[$number]; + } + + $defaultExpectedFields = [ + 'id' => 'A_NUMBER', + 'remote_id' => 'A_NUMBER', + 'accepted' => '1', + ]; + $expectedFields = array_merge($defaultExpectedFields, $body->getRowsHash()); + + foreach ($expectedFields as $field => $value) { + $this->assertFieldIsInReturnedShare($field, $value, $returnedShare); + } + } + /** * @When /^User "([^"]*)" from server "(LOCAL|REMOTE)" accepts last pending share$/ * @param string $user diff --git a/build/integration/federation_features/federated.feature b/build/integration/federation_features/federated.feature index 46794e02ff8a2..a8af32387a08b 100644 --- a/build/integration/federation_features/federated.feature +++ b/build/integration/federation_features/federated.feature @@ -278,6 +278,48 @@ Feature: federated + Scenario: List federated share from another server not accepted yet + 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" + When As an "user0" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + Then the list of returned shares has 0 shares + + Scenario: List federated share from another server + 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 + When As an "user0" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + Then the list of returned shares has 1 shares + And remote share 0 is returned with + | remote | http://localhost:8180/ | + | name | /remote-share.txt | + | owner | user1 | + | user | user0 | + | mountpoint | /remote-share.txt | + | mimetype | text/plain | + | mtime | A_NUMBER | + | permissions | 27 | + | type | file | + | file_id | A_NUMBER | + + + Scenario: Delete federated share with another server Given Using server "LOCAL" And user "user0" exists From 6e45663018afea1f227c0999afe0f8d3d30ae93f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Fri, 22 Jan 2021 14:34:02 +0100 Subject: [PATCH 3/6] Add integration tests for federated shares from unavailable servers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The federated server needs to be stopped during the tests, so it is now stopped in the FederationContext for each scenario instead of just once in the run.sh script. Signed-off-by: Daniel Calviño Sánchez --- .../features/bootstrap/FederationContext.php | 33 +++++++ .../federation_features/federated.feature | 95 +++++++++++++++++++ build/integration/run.sh | 6 +- 3 files changed, 130 insertions(+), 4 deletions(-) diff --git a/build/integration/features/bootstrap/FederationContext.php b/build/integration/features/bootstrap/FederationContext.php index a4472c54b1be3..423708adc1053 100644 --- a/build/integration/features/bootstrap/FederationContext.php +++ b/build/integration/features/bootstrap/FederationContext.php @@ -40,9 +40,29 @@ class FederationContext implements Context, SnippetAcceptingContext { use AppConfiguration; use CommandLine; + /** @var string */ + private static $phpFederatedServerPid = ''; + /** @var string */ private $lastAcceptedRemoteShareId; + /** + * @BeforeScenario + * @AfterScenario + * + * The server is started also after the scenarios to ensure that it is + * properly cleaned up if stopped. + */ + public function startFederatedServer() { + if (self::$phpFederatedServerPid !== '') { + return; + } + + $port = getenv('PORT_FED'); + + self::$phpFederatedServerPid = exec('php -S localhost:' . $port . ' -t ../../ >/dev/null & echo $!'); + } + /** * @BeforeScenario */ @@ -157,6 +177,19 @@ public function deleteLastAcceptedRemoteShare($user) { $this->sendingToWith('DELETE', "/apps/files_sharing/api/v1/remote_shares/" . $this->lastAcceptedRemoteShareId, null); } + /** + * @When /^remote server is stopped$/ + */ + public function remoteServerIsStopped() { + if (self::$phpFederatedServerPid === '') { + return; + } + + exec('kill ' . self::$phpFederatedServerPid); + + self::$phpFederatedServerPid = ''; + } + protected function resetAppConfigs() { $this->deleteServerConfig('files_sharing', 'incoming_server2server_group_share_enabled'); $this->deleteServerConfig('files_sharing', 'outgoing_server2server_group_share_enabled'); diff --git a/build/integration/federation_features/federated.feature b/build/integration/federation_features/federated.feature index a8af32387a08b..8b627e55a429b 100644 --- a/build/integration/federation_features/federated.feature +++ b/build/integration/federation_features/federated.feature @@ -318,6 +318,54 @@ Feature: federated | type | file | | file_id | A_NUMBER | + Scenario: List federated share from 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 "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 remote server is stopped + When As an "user0" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + Then the list of returned shares has 1 shares + And remote share 0 is returned with + | remote | http://localhost:8180/ | + | name | /remote-share.txt | + | owner | user1 | + | user | user0 | + | mountpoint | /remote-share.txt | + + Scenario: List federated share from another server no longer reachable after caching the file entry + 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 + # Checking that the file exists caches the file entry, which causes an + # exception to be thrown when getting the file info if the remote server is + # unreachable. + And as "user0" the file "/remote-share.txt" exists + And remote server is stopped + When As an "user0" + And sending "GET" to "/apps/files_sharing/api/v1/remote_shares" + Then the list of returned shares has 1 shares + And remote share 0 is returned with + | remote | http://localhost:8180/ | + | name | /remote-share.txt | + | owner | user1 | + | user | user0 | + | mountpoint | /remote-share.txt | + Scenario: Delete federated share with another server @@ -382,6 +430,30 @@ 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 from 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 "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 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 + Scenario: Delete federated share file from another server Given Using server "LOCAL" And user "user0" exists @@ -410,3 +482,26 @@ Feature: federated And As an "user1" And sending "GET" to "/apps/files_sharing/api/v1/shares" And the list of returned shares has 0 shares + + Scenario: Delete federated share file from 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 "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 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 diff --git a/build/integration/run.sh b/build/integration/run.sh index 4808ab58ef5f9..a20398e8ee9ba 100755 --- a/build/integration/run.sh +++ b/build/integration/run.sh @@ -38,11 +38,10 @@ php -S localhost:$PORT -t ../.. & PHPPID=$! echo $PHPPID +# The federated server is started and stopped by the tests themselves PORT_FED=$((8180 + $EXECUTOR_NUMBER)) echo $PORT_FED -php -S localhost:$PORT_FED -t ../.. & -PHPPID_FED=$! -echo $PHPPID_FED +export PORT_FED export TEST_SERVER_URL="http://localhost:$PORT/ocs/" export TEST_SERVER_FED_URL="http://localhost:$PORT_FED/ocs/" @@ -65,7 +64,6 @@ vendor/bin/behat --strict -f junit -f pretty $TAGS $SCENARIO_TO_RUN RESULT=$? kill $PHPPID -kill $PHPPID_FED if [ "$INSTALLED" == "true" ]; then From 2421282497840545c3cc912bb268f4567a77a64e Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Thu, 21 Oct 2021 22:53:33 +0200 Subject: [PATCH 4/6] Return false in hasUpdated when storage is not available Technically, saying that a storage has no updates when it's not available is correct. This makes it possible to retrieve the cache entry for the mount point and also to list and remove unavailable federated shares. Signed-off-by: Vincent Petry --- lib/private/Files/Storage/Wrapper/Availability.php | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/lib/private/Files/Storage/Wrapper/Availability.php b/lib/private/Files/Storage/Wrapper/Availability.php index b6d1ba2178ba0..1b532e3ba04cc 100644 --- a/lib/private/Files/Storage/Wrapper/Availability.php +++ b/lib/private/Files/Storage/Wrapper/Availability.php @@ -379,11 +379,15 @@ public function getLocalFile($path) { /** {@inheritdoc} */ public function hasUpdated($path, $time) { - $this->checkAvailability(); + if (!$this->isAvailable()) { + return false; + } try { return parent::hasUpdated($path, $time); } catch (StorageNotAvailableException $e) { - $this->setUnavailable($e); + // set unavailable but don't rethrow + $this->setUnavailable(null); + return false; } } @@ -449,7 +453,7 @@ public function getMetaData($path) { /** * @throws StorageNotAvailableException */ - protected function setUnavailable(StorageNotAvailableException $e) { + protected function setUnavailable(?StorageNotAvailableException $e) { $delay = self::RECHECK_TTL_SEC; if ($e instanceof StorageAuthException) { $delay = max( @@ -459,7 +463,9 @@ protected function setUnavailable(StorageNotAvailableException $e) { ); } $this->getStorageCache()->setAvailability(false, $delay); - throw $e; + if ($e !== null) { + throw $e; + } } From 3f08a6da907c76440c04e6b59dbe794090ad920d Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 22 Oct 2021 08:31:14 +0200 Subject: [PATCH 5/6] Update psalm baseline Signed-off-by: Vincent Petry --- build/psalm-baseline.xml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml index e420ab4b734c5..22c2732bf2945 100644 --- a/build/psalm-baseline.xml +++ b/build/psalm-baseline.xml @@ -3884,7 +3884,7 @@ - + copy copyFromStorage file_exists @@ -3901,7 +3901,6 @@ getMimeType getOwner getPermissions - hasUpdated hash isCreatable isDeletable From 8206b38f00ab9fcc2f34623119cb633c25ef3254 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 5 Nov 2021 10:28:41 +0100 Subject: [PATCH 6/6] Update lib/private/Files/Storage/Wrapper/Availability.php add void Signed-off-by: Vincent Petry Co-authored-by: Carl Schwan --- lib/private/Files/Storage/Wrapper/Availability.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/Files/Storage/Wrapper/Availability.php b/lib/private/Files/Storage/Wrapper/Availability.php index 1b532e3ba04cc..910ea3697571a 100644 --- a/lib/private/Files/Storage/Wrapper/Availability.php +++ b/lib/private/Files/Storage/Wrapper/Availability.php @@ -453,7 +453,7 @@ public function getMetaData($path) { /** * @throws StorageNotAvailableException */ - protected function setUnavailable(?StorageNotAvailableException $e) { + protected function setUnavailable(?StorageNotAvailableException $e): void { $delay = self::RECHECK_TTL_SEC; if ($e instanceof StorageAuthException) { $delay = max(