Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[stable21] Fix valid storages removed when cleaning remote storages #26192

Merged
merged 2 commits into from
Mar 18, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .drone.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
16 changes: 13 additions & 3 deletions apps/files_sharing/lib/Command/CleanupRemoteStorages.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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();
}

Expand Down Expand Up @@ -166,14 +173,17 @@ public function getRemoteStorages() {

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;
Expand Down
27 changes: 26 additions & 1 deletion apps/files_sharing/tests/Command/CleanupRemoteStoragesTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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'],
Expand Down Expand Up @@ -109,7 +116,9 @@ protected function setUp(): void {
}
}

$this->command = new CleanupRemoteStorages($this->connection);
$this->cloudIdManager = $this->createMock(ICloudIdManager::class);

$this->command = new CleanupRemoteStorages($this->connection, $this->cloudIdManager);
}

protected function tearDown(): void {
Expand Down Expand Up @@ -191,6 +200,22 @@ public function testCleanup() {
->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']));
Expand Down
14 changes: 14 additions & 0 deletions build/integration/features/bootstrap/FederationContext.php
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,20 @@
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)"$/
Expand Down
Original file line number Diff line number Diff line change
@@ -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"