From 1e7e6c900c51ae833f1f2e45cbb026706da63bfb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Fri, 22 Jan 2021 12:40:59 +0100 Subject: [PATCH 1/2] Add integration tests for "sharing:cleanup-remote-storages" OCC command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Daniel Calviño Sánchez --- .drone.yml | 2 +- .../features/bootstrap/FederationContext.php | 14 +++++ .../cleanup-remote-storage.feature | 53 +++++++++++++++++++ 3 files changed, 68 insertions(+), 1 deletion(-) create mode 100644 build/integration/federation_features/cleanup-remote-storage.feature diff --git a/.drone.yml b/.drone.yml index 21084be6df..94edd8e4ce 100644 --- a/.drone.yml +++ b/.drone.yml @@ -833,7 +833,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/build/integration/features/bootstrap/FederationContext.php b/build/integration/features/bootstrap/FederationContext.php index e119f950a4..25a5b02eab 100644 --- a/build/integration/features/bootstrap/FederationContext.php +++ b/build/integration/features/bootstrap/FederationContext.php @@ -36,6 +36,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" From 05cbdddb0d6296a5f3145b1d37c3125476dd57a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Tue, 26 Jan 2021 10:26:10 +0100 Subject: [PATCH 2/2] Fix valid storages removed when cleaning remote storages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The remote URL of a share is always stored in the database with a trailing slash. However, when a cloud ID is generated trailing slashes are removed. The ID of a remote storage is generated from the cloud ID, but the "cleanup-remote-storage" command directly used the remote URL stored in the database. Due to this, even if the remote storage was valid, its ID did not match the ID of the remote share generated by the command and ended being removed. Now the command generates the ID of remote shares using the cloud ID instead, just like done by the remote storage, so there is no longer a mismatch. Signed-off-by: Daniel Calviño Sánchez --- .../lib/Command/CleanupRemoteStorages.php | 16 ++++++++--- .../Command/CleanupRemoteStoragesTest.php | 27 ++++++++++++++++++- 2 files changed, 39 insertions(+), 4 deletions(-) diff --git a/apps/files_sharing/lib/Command/CleanupRemoteStorages.php b/apps/files_sharing/lib/Command/CleanupRemoteStorages.php index 2eb79f8776..e25c02aa48 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(); } @@ -165,14 +172,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 dd03ffc666..2ac3702c2d 100644 --- a/apps/files_sharing/tests/Command/CleanupRemoteStoragesTest.php +++ b/apps/files_sharing/tests/Command/CleanupRemoteStoragesTest.php @@ -25,6 +25,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; @@ -48,6 +50,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'], @@ -108,7 +115,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 { @@ -184,6 +193,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']));