From a9b8cf5645279ff70a060f2b2414b3ee6c2fd2b3 Mon Sep 17 00:00:00 2001 From: Phil Davis Date: Wed, 10 Jun 2020 18:51:42 +0545 Subject: [PATCH] Fix files_external:export so the output can be successfully imported --- apps/files_external/lib/Command/Export.php | 1 + .../lib/Command/ListCommand.php | 43 +++-- .../tests/Command/ListCommandTest.php | 73 ++++++++ changelog/unreleased/37054 | 7 + .../features/bootstrap/OccContext.php | 162 +++++++++++++----- .../exportLocalStorage.feature | 39 ++++- .../importLocalStorage.feature | 54 +++++- 7 files changed, 309 insertions(+), 70 deletions(-) create mode 100644 changelog/unreleased/37054 diff --git a/apps/files_external/lib/Command/Export.php b/apps/files_external/lib/Command/Export.php index a93dc27962a3..f07294b18ab0 100644 --- a/apps/files_external/lib/Command/Export.php +++ b/apps/files_external/lib/Command/Export.php @@ -52,6 +52,7 @@ protected function execute(InputInterface $input, OutputInterface $output) { $listInput->setOption('output', 'json_pretty'); $listInput->setOption('show-password', true); $listInput->setOption('full', true); + $listInput->setOption('importable-format', true); $listCommand->execute($listInput, $output); } } diff --git a/apps/files_external/lib/Command/ListCommand.php b/apps/files_external/lib/Command/ListCommand.php index 654ffa53661e..54c285b7f228 100644 --- a/apps/files_external/lib/Command/ListCommand.php +++ b/apps/files_external/lib/Command/ListCommand.php @@ -101,6 +101,11 @@ protected function configure() { 's', InputOption::VALUE_NONE, 'Show only a reduced mount info' + )->addOption( + 'importable-format', + 'i', + InputOption::VALUE_NONE, + 'Provide output values in a format compatible with files_external:import' ); parent::configure(); } @@ -129,7 +134,9 @@ protected function execute(InputInterface $input, OutputInterface $output) { */ public function listMounts($userId, array $mounts, InputInterface $input, OutputInterface $output) { $outputType = $input->getOption('output'); + $outputIsJson = ($outputType === self::OUTPUT_FORMAT_JSON) || ($outputType === self::OUTPUT_FORMAT_JSON_PRETTY); $shortView = $input->getOption('short'); + $importableView = $input->getOption('importable-format'); // check if there are any mounts present if (\count($mounts) === 0) { @@ -191,7 +198,7 @@ public function listMounts($userId, array $mounts, InputInterface $input, Output $countInvalid = 0; // In case adding array elements, add them only after the first two (Mount ID / Mount Point) // and before the last one entry (Type). Necessary for option -s - $rows = \array_map(function (IStorageConfig $config) use ($shortView, $userId, $defaultMountOptions, $full, $showMountOptions, &$countInvalid) { + $rows = \array_map(function (IStorageConfig $config) use ($outputIsJson, $shortView, $importableView, $userId, $defaultMountOptions, $full, $showMountOptions, &$countInvalid) { if ($config->getBackend() instanceof InvalidBackend || $config->getAuthMechanism() instanceof InvalidAuth) { $countInvalid++; } @@ -239,22 +246,38 @@ public function listMounts($userId, array $mounts, InputInterface $input, Output $config->getMountPoint() ]; } else { + $configValue = $configString; + if ($importableView) { + $storageValue = $config->getBackend()->getStorageClass(); + $authValue = $config->getAuthMechanism()->getIdentifier(); + if ($outputIsJson) { + $configValue = $storageConfig; + } + } else { + $storageValue = $config->getBackend()->getText(); + $authValue = $config->getAuthMechanism()->getText(); + } $values = [ $config->getId(), $config->getMountPoint(), - $config->getBackend()->getText(), - $config->getAuthMechanism()->getText(), - $configString, + $storageValue, + $authValue, + $configValue, $optionsString ]; } - // output independent on option shortview + // output independent of option shortview if (!$userId || $userId === self::ALL) { - $applicableUsers = \implode(', ', $config->getApplicableUsers()); - $applicableGroups = \implode(', ', $config->getApplicableGroups()); - if ($applicableUsers === '' && $applicableGroups === '') { - $applicableUsers = 'All'; + if ($importableView && $outputIsJson) { + $applicableUsers = $config->getApplicableUsers(); + $applicableGroups = $config->getApplicableGroups(); + } else { + $applicableUsers = \implode(', ', $config->getApplicableUsers()); + $applicableGroups = \implode(', ', $config->getApplicableGroups()); + if ($applicableUsers === '' && $applicableGroups === '') { + $applicableUsers = 'All'; + } } $values[] = $applicableUsers; $values[] = $applicableGroups; @@ -274,7 +297,7 @@ public function listMounts($userId, array $mounts, InputInterface $input, Output return $values; }, $mounts); - if ($outputType === self::OUTPUT_FORMAT_JSON || $outputType === self::OUTPUT_FORMAT_JSON_PRETTY) { + if ($outputIsJson) { $keys = \array_map(function ($header) { return \strtolower(\str_replace(' ', '_', $header)); }, $headers); diff --git a/apps/files_external/tests/Command/ListCommandTest.php b/apps/files_external/tests/Command/ListCommandTest.php index 6d5ff691530d..79ffdd0c0dc1 100644 --- a/apps/files_external/tests/Command/ListCommandTest.php +++ b/apps/files_external/tests/Command/ListCommandTest.php @@ -228,6 +228,79 @@ public function testLongView($options, $expectedResult, $mountOptions) { $input = $this->getInput($instance, ['user_id' => 'user1'], $options); $output = new BufferedOutput(); + $instance->listMounts('user1', [$mount1, $mount2], $input, $output); + $output = $output->fetch(); + if (isset($options['output']) && ($options['output'] === 'json')) { + $results = \json_decode($output, true); + $countResults = \count($results); + + for ($i = 0; $i < $countResults; $i++) { + $this->assertEquals($expectedResult[$i]['mount_id'], $results[$i]['mount_id']); + $this->assertEquals($expectedResult[$i]['mount_point'], $results[$i]['mount_point']); + $this->assertEquals($expectedResult[$i]['authentication_type'], $results[$i]['authentication_type']); + } + } else { + $this->assertEquals($expectedResult, $output); + } + } + public function providesImportableFormat() { + return [ + [ + ['importable-format' => true, 'short' => false, 'output' => 'json'], + [ + ['mount_id' => 1, 'mount_point' => '/ownCloud', 'storage' => 'Local', 'authentication_type' => 'password::password'], + ['mount_id' => 2, 'mount_point' => '/SFTP', 'storage' => 'Local', 'authentication_type' => 'password::sessioncredentials'], + ], + [ + ['mount_id' => 1, 'mount_point' => '/ownCloud'], + ['mount_id' => 2, 'mount_point' => '/SFTP'], + ] + ], + [ + ['importable-format' => true, 'short' => false], + << 1, 'mount_point' => '/ownCloud', 'storage' => 'Local'], + ['mount_id' => 2, 'mount_point' => '/SFTP', 'storage' => 'Local'], + ] + ], + ]; + } + + /** + * @dataProvider providesImportableFormat + * @param $options + * @param $expectedResult + * @param $mountOptions + */ + public function testImportableFormat($options, $expectedResult, $mountOptions) { + $l10n = $this->createMock('\OCP\IL10N', null, [], '', false); + $session = $this->createMock('\OCP\ISession'); + $crypto = $this->createMock('\OCP\Security\ICrypto'); + $instance = $this->getInstance(); + // FIXME: use mock of IStorageConfig + $mount1 = new StorageConfig(); + $mount1->setId($mountOptions[0]['mount_id']); + $mount1->setMountPoint($mountOptions[0]['mount_point']); + $mount1->setAuthMechanism(new Password()); + $mount1->setBackend(new Local($l10n, new NullMechanism())); + $mount2 = new StorageConfig(); + $mount2->setId($mountOptions[1]['mount_id']); + $mount2->setMountPoint($mountOptions[1]['mount_point']); + $mount2->setAuthMechanism(new SessionCredentials($session, $crypto)); + $mount2->setBackend(new Local($l10n, new NullMechanism())); + $input = $this->getInput($instance, ['user_id' => 'user1'], $options); + $output = new BufferedOutput(); + $instance->listMounts('user1', [$mount1, $mount2], $input, $output); $output = $output->fetch(); if (isset($options['output']) && ($options['output'] === 'json')) { diff --git a/changelog/unreleased/37054 b/changelog/unreleased/37054 new file mode 100644 index 000000000000..5b413e2c372e --- /dev/null +++ b/changelog/unreleased/37054 @@ -0,0 +1,7 @@ +Bugfix: Correct files_external:export output so it can be imported + +The output of files_external:export was not suitable to be used as input to +files_external:import. This has been corrected. + +https://github.com/owncloud/core/issues/37054 +https://github.com/owncloud/core/pull/37513 diff --git a/tests/acceptance/features/bootstrap/OccContext.php b/tests/acceptance/features/bootstrap/OccContext.php index 0466a091fc5e..8f54d314909d 100644 --- a/tests/acceptance/features/bootstrap/OccContext.php +++ b/tests/acceptance/features/bootstrap/OccContext.php @@ -1720,12 +1720,29 @@ public function theFollowingLocalStorageShouldBeListed(TableNode $table) { ); } if (isset($expectedStorageEntry['Configuration'])) { - Assert::assertStringStartsWith( - $expectedStorageEntry['Configuration'], - $listedStorageEntry->configuration, - "Configuration column does not start with the expected value for storage " - . $expectedStorageEntry['MountPoint'] - ); + if ($expectedStorageEntry['Configuration'] === '') { + Assert::assertEquals( + '', + $listedStorageEntry->configuration, + 'Configuration column should be empty but is ' + . $listedStorageEntry->configuration + ); + } else { + if (\is_string($listedStorageEntry->configuration)) { + Assert::assertStringStartsWith( + $expectedStorageEntry['Configuration'], + $listedStorageEntry->configuration, + "Configuration column does not start with the expected value for storage " + . $expectedStorageEntry['Configuration'] + ); + } else { + $item = \strtok($expectedStorageEntry['Configuration'], ':'); + Assert::assertTrue( + \property_exists($listedStorageEntry->configuration, $item), + "$item was not found in the Configuration column" + ); + } + } } if (isset($expectedStorageEntry['Options'])) { Assert::assertEquals( @@ -1736,32 +1753,65 @@ public function theFollowingLocalStorageShouldBeListed(TableNode $table) { ); } if (isset($expectedStorageEntry['ApplicableUsers'])) { - $listedApplicableUsers = \explode(', ', $listedStorageEntry->applicable_users); - $expectedApplicableUsers = \explode(', ', $expectedStorageEntry['ApplicableUsers']); - foreach ($expectedApplicableUsers as $expectedApplicableUserEntry) { - $expectedApplicableUserEntry = $this->featureContext->getActualUsername($expectedApplicableUserEntry); - Assert::assertContains( - $expectedApplicableUserEntry, + if (\is_string($listedStorageEntry->applicable_users)) { + if ($listedStorageEntry->applicable_users === '') { + $listedApplicableUsers = []; + } else { + $listedApplicableUsers = \explode(', ', $listedStorageEntry->applicable_users); + } + } else { + $listedApplicableUsers = $listedStorageEntry->applicable_users; + } + if ($expectedStorageEntry['ApplicableUsers'] === '') { + Assert::assertEquals( + [], $listedApplicableUsers, - __METHOD__ - . " '$expectedApplicableUserEntry' is not listed in '" - . \implode(', ', $listedApplicableUsers) - . "'" + "ApplicableUsers was expected to be an empty array but was not empty" ); + } else { + $expectedApplicableUsers = \explode(', ', $expectedStorageEntry['ApplicableUsers']); + foreach ($expectedApplicableUsers as $expectedApplicableUserEntry) { + $expectedApplicableUserEntry = $this->featureContext->getActualUsername($expectedApplicableUserEntry); + Assert::assertContains( + $expectedApplicableUserEntry, + $listedApplicableUsers, + __METHOD__ + . " '$expectedApplicableUserEntry' is not listed in '" + . \implode(', ', $listedApplicableUsers) + . "'" + ); + } } } if (isset($expectedStorageEntry['ApplicableGroups'])) { - $listedApplicableGroups = \explode(', ', $listedStorageEntry->applicable_groups); - $expectedApplicableGroups = \explode(', ', $expectedStorageEntry['ApplicableGroups']); - foreach ($expectedApplicableGroups as $expectedApplicableGroupEntry) { - Assert::assertContains( - $expectedApplicableGroupEntry, + if (\is_string($listedStorageEntry->applicable_groups)) { + if ($listedStorageEntry->applicable_groups === '') { + $listedApplicableGroups = []; + } else { + $listedApplicableGroups = \explode(', ', $listedStorageEntry->applicable_groups); + } + } else { + $listedApplicableGroups = $listedStorageEntry->applicable_groups; + } + if ($expectedStorageEntry['ApplicableGroups'] === '') { + Assert::assertEquals( + [], $listedApplicableGroups, - __METHOD__ - . " '$expectedApplicableGroupEntry' is not listed in '" - . \implode(', ', $listedApplicableGroups) - . "'" + "ApplicableGroups was expected to be an empty array but was not empty" ); + Assert::assertEquals([], $listedApplicableGroups); + } else { + $expectedApplicableGroups = \explode(', ', $expectedStorageEntry['ApplicableGroups']); + foreach ($expectedApplicableGroups as $expectedApplicableGroupEntry) { + Assert::assertContains( + $expectedApplicableGroupEntry, + $listedApplicableGroups, + __METHOD__ + . " '$expectedApplicableGroupEntry' is not listed in '" + . \implode(', ', $listedApplicableGroups) + . "'" + ); + } } } if (isset($expectedStorageEntry['Type'])) { @@ -1965,37 +2015,35 @@ public function theFollowingShouldNotBeIncludedInTheOptionsOfLocalStorage($local * * @param string $folder * - * @return integer + * @return integer|boolean * @throws Exception */ public function administratorDeletesFolder($folder) { - $createdLocalStorage = []; - $this->listLocalStorageMount(); - $commandOutput = \json_decode($this->featureContext->getStdOutOfOccCommand()); - foreach ($commandOutput as $i) { - $createdLocalStorage[$i->mount_id] = \ltrim($i->mount_point, '/'); - } - foreach ($createdLocalStorage as $key => $value) { - if ($value === $folder) { - $mount_id = $key; - } - } - if (!isset($mount_id)) { - throw new Exception("Id not found for folder to be deleted"); - } - $this->invokingTheCommand('files_external:delete --yes ' . $mount_id); - return (int) $mount_id; + return $this->deleteLocalStorageFolderUsingTheOccCommand($folder); } /** - * @When the administrator has deleted local storage :folder using the occ command + * @Given the administrator has deleted local storage :folder using the occ command + * + * @param string $folder * + * @return integer|boolean + * @throws Exception + */ + public function administratorHasDeletedLocalStorageFolderUsingTheOccCommand($folder) { + $mount_id = $this->deleteLocalStorageFolderUsingTheOccCommand($folder); + $this->theCommandShouldHaveBeenSuccessful(); + return $mount_id; + } + + /** * @param string $folder + * @param bool $mustExist * - * @return integer + * @return integer|boolean * @throws Exception */ - public function administratorHasDeletedFolder($folder) { + public function deleteLocalStorageFolderUsingTheOccCommand($folder, $mustExist = true) { $createdLocalStorage = []; $this->listLocalStorageMount(); $commandOutput = \json_decode($this->featureContext->getStdOutOfOccCommand()); @@ -2008,10 +2056,12 @@ public function administratorHasDeletedFolder($folder) { } } if (!isset($mount_id)) { - throw new Exception("Id not found for folder to be deleted"); + if ($mustExist) { + throw new Exception("Id not found for folder to be deleted"); + } + return false; } $this->invokingTheCommand('files_external:delete --yes ' . $mount_id); - $this->theCommandShouldHaveBeenSuccessful(); return (int) $mount_id; } @@ -2822,6 +2872,24 @@ public function resetDAVTechPreview() { } } + /** + * This will run after EVERY scenario. + * Some local_storage tests import storage from an export file. In that case + * we have not explicitly created the storage, and so we do not explicitly + * know to delete it. So delete the local storage that is known to be used + * in tests. + * + * @AfterScenario @local_storage + * + * @return void + * @throws Exception + */ + public function removeLocalStorageIfExists() { + $this->deleteLocalStorageFolderUsingTheOccCommand('local_storage', false); + $this->deleteLocalStorageFolderUsingTheOccCommand('local_storage2', false); + $this->deleteLocalStorageFolderUsingTheOccCommand('local_storage3', false); + } + /** * This will run before EVERY scenario. * It will set the properties for this object. diff --git a/tests/acceptance/features/cliLocalStorage/exportLocalStorage.feature b/tests/acceptance/features/cliLocalStorage/exportLocalStorage.feature index df02f9af947e..e0a74bed99ee 100644 --- a/tests/acceptance/features/cliLocalStorage/exportLocalStorage.feature +++ b/tests/acceptance/features/cliLocalStorage/exportLocalStorage.feature @@ -13,17 +13,38 @@ Feature: export created local storage mounts from the command line Scenario: export the created mounts When the administrator exports the local storage mounts using the occ command Then the following local storage should be listed: - | MountPoint | Storage | AuthenticationType | Configuration | Options | ApplicableUsers | ApplicableGroups | - | /local_storage2 | Local | None | datadir: | | All | | - | /new_local_storage | Local | None | datadir: | | All | | - | /local_storage | Local | None | datadir: | enable_sharing: true | All | | + | MountPoint | Storage | AuthenticationType | Configuration | Options | ApplicableUsers | ApplicableGroups | + | /local_storage2 | \OC\Files\Storage\Local | null::null | datadir: | | | | + | /new_local_storage | \OC\Files\Storage\Local | null::null | datadir: | | | | + | /local_storage | \OC\Files\Storage\Local | null::null | datadir: | enable_sharing: true | | | - @issue-37054 Scenario: export the created mounts when the system language is "de" Given the administrator has set the system language to "de" When the administrator exports the local storage mounts using the occ command Then the following local storage should be listed: - | MountPoint | Storage | AuthenticationType | Configuration | Options | ApplicableUsers | ApplicableGroups | - | /local_storage2 | Lokal | Keine | datadir: | | All | | - | /new_local_storage | Lokal | Keine | datadir: | | All | | - | /local_storage | Lokal | Keine | datadir: | enable_sharing: true | All | | \ No newline at end of file + | MountPoint | Storage | AuthenticationType | Configuration | Options | ApplicableUsers | ApplicableGroups | + | /local_storage2 | \OC\Files\Storage\Local | null::null | datadir: | | | | + | /new_local_storage | \OC\Files\Storage\Local | null::null | datadir: | | | | + | /local_storage | \OC\Files\Storage\Local | null::null | datadir: | enable_sharing: true | | | + + Scenario: export the created mounts with various user and group settings + Given these users have been created with default attributes and without skeleton files: + | username | + | Alice | + | Brian | + | Carol | + And group "grp1" has been created + And group "grp2" has been created + And group "grp3" has been created + And the administrator has added user "Alice" as the applicable user for local storage mount "local_storage2" + And the administrator has added user "Brian" as the applicable user for local storage mount "new_local_storage" + And the administrator has added user "Carol" as the applicable user for local storage mount "new_local_storage" + And the administrator has added group "grp1" as the applicable group for local storage mount "local_storage2" + And the administrator has added group "grp2" as the applicable group for local storage mount "new_local_storage" + And the administrator has added group "grp3" as the applicable group for local storage mount "new_local_storage" + When the administrator exports the local storage mounts using the occ command + Then the following local storage should be listed: + | MountPoint | Storage | AuthenticationType | Configuration | Options | ApplicableUsers | ApplicableGroups | + | /local_storage2 | \OC\Files\Storage\Local | null::null | datadir: | | Alice | grp1 | + | /new_local_storage | \OC\Files\Storage\Local | null::null | datadir: | | Brian, Carol | grp2, grp3 | + | /local_storage | \OC\Files\Storage\Local | null::null | datadir: | enable_sharing: true | | | diff --git a/tests/acceptance/features/cliLocalStorage/importLocalStorage.feature b/tests/acceptance/features/cliLocalStorage/importLocalStorage.feature index ca035c6a7216..11fef0f0dda9 100644 --- a/tests/acceptance/features/cliLocalStorage/importLocalStorage.feature +++ b/tests/acceptance/features/cliLocalStorage/importLocalStorage.feature @@ -4,13 +4,59 @@ Feature: import exported local storage mounts from the command line I want to import exported local storage mounts from the command line So that I can create previously exported local storage mounts - @issue-37054 + Background: + Given these users have been created with default attributes and without skeleton files: + | username | + | Alice | + Scenario: import local storage mounts from a file Given the administrator has created the local storage mount "local_storage2" - And the administrator has uploaded file with content "this is a file in local storage" to "/local_storage2/file-in-local-storage.txt" + And the administrator has uploaded file with content "this is a file in local storage2" to "/local_storage2/file-in-local-storage2.txt" + And the administrator has exported the local storage mounts using the occ command + And the administrator has created a file "data/exportedMounts.json" with the last exported content using the testing API + And the administrator has deleted local storage "local_storage" using the occ command + And the administrator has deleted local storage "local_storage2" using the occ command + When the administrator imports the local storage mount from file "data/exportedMounts.json" using the occ command + And the administrator lists the local storage using the occ command + Then the following local storage should be listed: + | MountPoint | Storage | AuthenticationType | Configuration | Options | ApplicableUsers | ApplicableGroups | + | /local_storage2 | Local | None | datadir: | | All | | + And as "Alice" folder "/local_storage2" should exist + And the content of file "/local_storage2/file-in-local-storage2.txt" for user "Alice" should be "this is a file in local storage2" + + Scenario: import local storage mounts from a file with various user and group settings + Given these users have been created with default attributes and without skeleton files: + | username | + | Brian | + | Carol | + And group "grp1" has been created + And group "grp2" has been created + And group "grp3" has been created + And the administrator has created the local storage mount "local_storage2" + And the administrator has created the local storage mount "local_storage3" + And the administrator has uploaded file with content "this is a file in local storage2" to "/local_storage2/file-in-local-storage2.txt" + And the administrator has uploaded file with content "this is a file in local storage3" to "/local_storage3/file-in-local-storage3.txt" + And the administrator has added user "Alice" as the applicable user for local storage mount "local_storage2" + And the administrator has added user "Brian" as the applicable user for local storage mount "local_storage3" + And the administrator has added user "Carol" as the applicable user for local storage mount "local_storage3" + And the administrator has added group "grp1" as the applicable group for local storage mount "local_storage2" + And the administrator has added group "grp2" as the applicable group for local storage mount "local_storage3" + And the administrator has added group "grp3" as the applicable group for local storage mount "local_storage3" And the administrator has exported the local storage mounts using the occ command And the administrator has created a file "data/exportedMounts.json" with the last exported content using the testing API + And the administrator has deleted local storage "local_storage" using the occ command And the administrator has deleted local storage "local_storage2" using the occ command + And the administrator has deleted local storage "local_storage3" using the occ command When the administrator imports the local storage mount from file "data/exportedMounts.json" using the occ command - # change the then step once the issue is fixed, and the import works well - Then the command output should contain the text "An unhandled exception has been thrown:" \ No newline at end of file + And the administrator lists the local storage using the occ command + Then the following local storage should be listed: + | MountPoint | Storage | AuthenticationType | Configuration | Options | ApplicableUsers | ApplicableGroups | + | /local_storage2 | Local | None | datadir: | | Alice | grp1 | + | /local_storage3 | Local | None | datadir: | | Brian, Carol | grp2, grp3 | + | /local_storage | Local | None | datadir: | | All | | + And as "Alice" folder "/local_storage2" should exist + And the content of file "/local_storage2/file-in-local-storage2.txt" for user "Alice" should be "this is a file in local storage2" + And as "Brian" folder "/local_storage3" should exist + And the content of file "/local_storage3/file-in-local-storage3.txt" for user "Brian" should be "this is a file in local storage3" + But as "Alice" folder "/local_storage3" should not exist + And as "Brian" folder "/local_storage2" should not exist