Merge pull request #29565 from nextcloud/backport/25332/stable22

[stable22] Fix removing remote shares when the remote server is unreachable

5 files changed, 309 insertions, 10 deletions

diff --git a/build/integration/features/bootstrap/FederationContext.php b/build/integration/features/bootstrap/FederationContext.php
index 2754cf668e6..423708adc10 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';
 
@@ -39,6 +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
 	 */
@@ -94,6 +118,37 @@ class FederationContext implements Context, SnippetAcceptingContext {
 	}
 
 	/**
+	 * @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
 	 * @param string $server
@@ -109,6 +164,30 @@ class FederationContext implements Context, SnippetAcceptingContext {
 		$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);
+	}
+
+	/**
+	 * @When /^remote server is stopped$/
+	 */
+	public function remoteServerIsStopped() {
+		if (self::$phpFederatedServerPid === '') {
+			return;
+		}
+
+		exec('kill ' . self::$phpFederatedServerPid);
+
+		self::$phpFederatedServerPid = '';
 	}
 
 	protected function resetAppConfigs() {
diff --git a/build/integration/federation_features/federated.feature b/build/integration/federation_features/federated.feature
index 17ec6b4b43e..8b627e55a42 100644
--- a/build/integration/federation_features/federated.feature
+++ b/build/integration/federation_features/federated.feature
@@ -278,13 +278,230 @@ 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: 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
+		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 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
+		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
 
+	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 4808ab58ef5..a20398e8ee9 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
 
diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml
index 0b816e7f1ea..0d4328ae716 100644
--- a/build/psalm-baseline.xml
+++ b/build/psalm-baseline.xml
@@ -3943,7 +3943,7 @@
     </NoInterfaceProperties>
   </file>
   <file src="lib/private/Files/Storage/Wrapper/Availability.php">
-    <InvalidNullableReturnType occurrences="34">
+    <InvalidNullableReturnType occurrences="33">
       <code>copy</code>
       <code>copyFromStorage</code>
       <code>file_exists</code>
@@ -3960,7 +3960,6 @@
       <code>getMimeType</code>
       <code>getOwner</code>
       <code>getPermissions</code>
-      <code>hasUpdated</code>
       <code>hash</code>
       <code>isCreatable</code>
       <code>isDeletable</code>
diff --git a/lib/private/Files/Storage/Wrapper/Availability.php b/lib/private/Files/Storage/Wrapper/Availability.php
index b6d1ba2178b..910ea369757 100644
--- a/lib/private/Files/Storage/Wrapper/Availability.php
+++ b/lib/private/Files/Storage/Wrapper/Availability.php
@@ -379,11 +379,15 @@ class Availability extends Wrapper {
 
 	/** {@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 @@ class Availability extends Wrapper {
 	/**
 	 * @throws StorageNotAvailableException
 	 */
-	protected function setUnavailable(StorageNotAvailableException $e) {
+	protected function setUnavailable(?StorageNotAvailableException $e): void {
 		$delay = self::RECHECK_TTL_SEC;
 		if ($e instanceof StorageAuthException) {
 			$delay = max(
@@ -459,7 +463,9 @@ class Availability extends Wrapper {
 			);
 		}
 		$this->getStorageCache()->setAvailability(false, $delay);
-		throw $e;
+		if ($e !== null) {
+			throw $e;
+		}
 	}