Merge pull request #25331 from nextcloud/fix-valid-storages-removed-when-cleaning-remote-storages

Fix valid storages removed when cleaning remote storages

.drone.yml

@@ -794,7 +794,7 @@ steps:
     - bash tests/drone-run-integration-tests.sh || exit 0
     - ./occ maintenance:install --admin-pass=admin
     - cd build/integration
-    - ./run.sh federation_features/federated.feature
+    - ./run.sh federation_features/
 
 trigger:
   branch:

apps/files_sharing/lib/Command/CleanupRemoteStorages.php

@@ -25,6 +25,7 @@
 namespace OCA\Files_Sharing\Command;
 
 use OCP\DB\QueryBuilder\IQueryBuilder;
+use OCP\Federation\ICloudIdManager;
 use OCP\IDBConnection;
 use Symfony\Component\Console\Command\Command;
 use Symfony\Component\Console\Input\InputInterface;
@@ -42,8 +43,14 @@ class CleanupRemoteStorages extends Command {
 	 */
 	protected $connection;
 
-	public function __construct(IDBConnection $connection) {
+	/**
+	 * @var ICloudIdManager
+	 */
+	private $cloudIdManager;
+
+	public function __construct(IDBConnection $connection, ICloudIdManager $cloudIdManager) {
 		$this->connection = $connection;
+		$this->cloudIdManager = $cloudIdManager;
 		parent::__construct();
 	}
 
@@ -166,14 +173,17 @@ class CleanupRemoteStorages extends Command {
 
 	public function getRemoteShareIds() {
 		$queryBuilder = $this->connection->getQueryBuilder();
-		$queryBuilder->select(['id', 'share_token', 'remote'])
+		$queryBuilder->select(['id', 'share_token', 'owner', 'remote'])
 			->from('share_external');
 		$query = $queryBuilder->execute();
 
 		$remoteShareIds = [];
 
 		while ($row = $query->fetch()) {
-			$remoteShareIds[$row['id']] = 'shared::' . md5($row['share_token'] . '@' . $row['remote']);
+			$cloudId = $this->cloudIdManager->getCloudId($row['owner'], $row['remote']);
+			$remote = $cloudId->getRemote();
+
+			$remoteShareIds[$row['id']] = 'shared::' . md5($row['share_token'] . '@' . $remote);
 		}
 
 		return $remoteShareIds;

apps/files_sharing/tests/Command/CleanupRemoteStoragesTest.php

@@ -26,6 +26,8 @@
 namespace OCA\Files_Sharing\Tests\Command;
 
 use OCA\Files_Sharing\Command\CleanupRemoteStorages;
+use OCP\Federation\ICloudId;
+use OCP\Federation\ICloudIdManager;
 use Symfony\Component\Console\Input\InputInterface;
 use Symfony\Component\Console\Output\OutputInterface;
 use Test\TestCase;
@@ -49,6 +51,11 @@ class CleanupRemoteStoragesTest extends TestCase {
 	 */
 	private $connection;
 
+	/**
+	 * @var ICloudIdManager|\PHPUnit\Framework\MockObject\MockObject
+	 */
+	private $cloudIdManager;
+
 	private $storages = [
 		['id' => 'shared::7b4a322b22f9d0047c38d77d471ce3cf', 'share_token' => 'f2c69dad1dc0649f26976fd210fc62e1', 'remote' => 'https://hostname.tld/owncloud1', 'user' => 'user1'],
 		['id' => 'shared::efe3b456112c3780da6155d3a9b9141c', 'share_token' => 'f2c69dad1dc0649f26976fd210fc62e2', 'remote' => 'https://hostname.tld/owncloud2', 'user' => 'user2'],
@@ -109,7 +116,9 @@ class CleanupRemoteStoragesTest extends TestCase {
 			}
 		}
 
-		$this->command = new CleanupRemoteStorages($this->connection);
+		$this->cloudIdManager = $this->createMock(ICloudIdManager::class);
+
+		$this->command = new CleanupRemoteStorages($this->connection, $this->cloudIdManager);
 	}
 
 	protected function tearDown(): void {
@@ -191,6 +200,22 @@ class CleanupRemoteStoragesTest extends TestCase {
 			->method('writeln')
 			->with('5 remote share(s) exist');
 
+		$this->cloudIdManager
+			->expects($this->any())
+			->method('getCloudId')
+			->will($this->returnCallback(function (string $user, string $remote) {
+				$cloudIdMock = $this->createMock(ICloudId::class);
+
+				// The remotes are already sanitized in the original data, so
+				// they can be directly returned.
+				$cloudIdMock
+					->expects($this->any())
+					->method('getRemote')
+					->willReturn($remote);
+
+				return $cloudIdMock;
+			}));
+
 		$this->command->execute($input, $output);
 
 		$this->assertTrue($this->doesStorageExist($this->storages[0]['numeric_id']));

build/integration/features/bootstrap/FederationContext.php

@@ -37,6 +37,20 @@ require __DIR__ . '/../../vendor/autoload.php';
 class FederationContext implements Context, SnippetAcceptingContext {
 	use WebDav;
 	use AppConfiguration;
+	use CommandLine;
+
+	/**
+	 * @BeforeScenario
+	 */
+	public function cleanupRemoteStorages() {
+		// 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
+		// cleaned after each scenario, as this hook is executed before the hook
+		// 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']);
+	}
 
 	/**
 	 * @Given /^User "([^"]*)" from server "(LOCAL|REMOTE)" shares "([^"]*)" with user "([^"]*)" from server "(LOCAL|REMOTE)"$/

build/integration/federation_features/cleanup-remote-storage.feature

@@ -0,0 +1,53 @@
+Feature: cleanup-remote-storage
+  Background:
+    Given using api version "1"
+
+  Scenario: cleanup remote storage with active storages
+    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"
+    # Accept and download the file to ensure that a storage is created for the
+    # federated share
+    And User "user0" from server "LOCAL" accepts last pending share
+    And As an "user0"
+    And Downloading file "/remote-share.txt"
+    And the HTTP status code should be "200"
+    When invoking occ with "sharing:cleanup-remote-storage"
+    Then the command was successful
+    And the command output contains the text "1 remote storage(s) need(s) to be checked"
+    And the command output contains the text "1 remote share(s) exist"
+    And the command output contains the text "no storages deleted"
+
+  Scenario: cleanup remote storage with inactive storages
+    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"
+    # Accept and download the file to ensure that a storage is created for the
+    # federated share
+    And User "user0" from server "LOCAL" accepts last pending share
+    And As an "user0"
+    And Downloading file "/remote-share.txt"
+    And the HTTP status code should be "200"
+    And Using server "REMOTE"
+    And As an "user1"
+    And Deleting last share
+    And the OCS status code should be "100"
+    And the HTTP status code should be "200"
+    When Using server "LOCAL"
+    And invoking occ with "sharing:cleanup-remote-storage"
+    Then the command was successful
+    And the command output contains the text "1 remote storage(s) need(s) to be checked"
+    And the command output contains the text "0 remote share(s) exist"
+    And the command output contains the text "deleted 1 storage"