diff --git a/.drone.yml b/.drone.yml index 54932162cd..be09d9b388 100644 --- a/.drone.yml +++ b/.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: diff --git a/apps/files_sharing/lib/Command/CleanupRemoteStorages.php b/apps/files_sharing/lib/Command/CleanupRemoteStorages.php index db18e7d249..cf0550aef7 100644 --- a/apps/files_sharing/lib/Command/CleanupRemoteStorages.php +++ b/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; diff --git a/apps/files_sharing/tests/Command/CleanupRemoteStoragesTest.php b/apps/files_sharing/tests/Command/CleanupRemoteStoragesTest.php index 3176163c44..ba4b1d0549 100644 --- a/apps/files_sharing/tests/Command/CleanupRemoteStoragesTest.php +++ b/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'])); diff --git a/build/integration/features/bootstrap/FederationContext.php b/build/integration/features/bootstrap/FederationContext.php index 41581110bd..359d3226b3 100644 --- a/build/integration/features/bootstrap/FederationContext.php +++ b/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)"$/ diff --git a/build/integration/federation_features/cleanup-remote-storage.feature b/build/integration/federation_features/cleanup-remote-storage.feature new file mode 100644 index 0000000000..c782987cac --- /dev/null +++ b/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"