From 2785fd193d57782363ef9c32bd7e811be2fb2756 Mon Sep 17 00:00:00 2001 From: =?utf8?q?David=20Mudr=C3=A1k?= Date: Fri, 1 Jul 2016 00:18:10 +0200 Subject: [PATCH] MDL-55020 admin: Fix renaming of the plugin package root folder There was a problem with core\update\code_manager::unzip_plugin_file() if it was used to extract a plugin package into a non-empty target directory and the plugin package root folder was being renamed at the same time. The problem was caused by the underlying helper method rename_extracted_rootdir() that worked only for ZIPs extracted to an empty temporary location. When the plugin was extracted to the actual dirroot with other existing plugin folders present, the method failed badly. The solution in the patch is to always extract the ZIP into a temporary empty location, perform the eventual root renaming there, and only then move the extracted contents to the final destination. Additionally we are changing the behaviour of the rename_extracted_rootdir() method so that now it throws exception if the plugin package contains multiple root folders (it should not happen in normal situations as such a plugin would not pass the pre-install validation). Unit tests did not catch this bug before because in the tests, the target directory had been empty. Now we are adding a new directory "aaa_another" to the target location to test in more realistic environment. Tests for the new behaviour of the renaming method are added, too. p.s. I noticed that moodle_exception class was not imported into the namespace and this is fixed now too (and covered with unit tests). --- lib/classes/update/code_manager.php | 56 +++++++++++++++--- .../update_validator/zips/multidir.zip | Bin 0 -> 672 bytes lib/tests/update_code_manager_test.php | 13 ++++ 3 files changed, 60 insertions(+), 9 deletions(-) create mode 100644 lib/tests/fixtures/update_validator/zips/multidir.zip diff --git a/lib/classes/update/code_manager.php b/lib/classes/update/code_manager.php index 12719cbf446..9eac43bf937 100644 --- a/lib/classes/update/code_manager.php +++ b/lib/classes/update/code_manager.php @@ -26,6 +26,7 @@ namespace core\update; use core_component; use coding_exception; +use moodle_exception; use SplFileInfo; use RecursiveDirectoryIterator; use RecursiveIteratorIterator; @@ -159,15 +160,18 @@ class code_manager { */ public function unzip_plugin_file($zipfilepath, $targetdir, $rootdir = '') { + // Extract the package into a temporary location. $fp = get_file_packer('application/zip'); - $files = $fp->extract_to_pathname($zipfilepath, $targetdir); + $tempdir = make_request_directory(); + $files = $fp->extract_to_pathname($zipfilepath, $tempdir); if (!$files) { return array(); } + // If requested, rename the root directory of the plugin. if (!empty($rootdir)) { - $files = $this->rename_extracted_rootdir($targetdir, $rootdir, $files); + $files = $this->rename_extracted_rootdir($tempdir, $rootdir, $files); } // Sometimes zip may not contain all parent directories, add them to make it consistent. @@ -187,6 +191,9 @@ class code_manager { } } + // Move the extracted files into the target location. + $this->move_extracted_plugin_files($tempdir, $targetdir, $files); + // Set the permissions of extracted subdirs and files. $this->set_plugin_files_permissions($targetdir, $files); @@ -443,12 +450,10 @@ class code_manager { /** * Renames the root directory of the extracted ZIP package. * - * This method does not validate the presence of the single root directory - * (it is the validator's duty). It just searches for the first directory - * under the given location and renames it. - * - * The method will not rename the root if the requested location already - * exists. + * This internal helper method assumes that the plugin ZIP package has been + * extracted into a temporary empty directory so the plugin folder is the + * only folder there. The ZIP package is supposed to be validated so that + * it contains just a single root folder. * * @param string $dirname fullpath location of the extracted ZIP package * @param string $rootdir the requested name of the root directory @@ -473,8 +478,11 @@ class code_manager { continue; } if (is_dir($dirname.'/'.$item)) { + if ($found !== null and $found !== $item) { + // Multiple directories found. + throw new moodle_exception('unexpected_archive_structure', 'core_plugin'); + } $found = $item; - break; } } @@ -520,4 +528,34 @@ class code_manager { } } } + + /** + * Moves the extracted contents of the plugin ZIP into the target location. + * + * @param string $sourcedir full path to the directory the ZIP file was extracted to + * @param mixed $targetdir full path to the directory where the files should be moved to + * @param array $files list of extracted files + */ + protected function move_extracted_plugin_files($sourcedir, $targetdir, array $files) { + global $CFG; + + foreach ($files as $file => $status) { + if ($status !== true) { + throw new moodle_exception('corrupted_archive_structure', 'core_plugin', '', $file, $status); + } + + $source = $sourcedir.'/'.$file; + $target = $targetdir.'/'.$file; + + if (is_dir($source)) { + continue; + + } else { + if (!is_dir(dirname($target))) { + mkdir(dirname($target), $CFG->directorypermissions, true); + } + rename($source, $target); + } + } + } } diff --git a/lib/tests/fixtures/update_validator/zips/multidir.zip b/lib/tests/fixtures/update_validator/zips/multidir.zip new file mode 100644 index 0000000000000000000000000000000000000000..a350015b319a0b7a64d7e88075f2025ccfee063a GIT binary patch literal 672 zcmWIWW@h1H0D;}nDjr}4lwe_yVaU%*)ejBfWME$C*%g+2CN#XXf}4Sny%1*N_u?HGrZZ%n#LFmReMtnV+XukWqloARBw20xp$;oYM5nJYBow{M-Vd-FYPn zwhHPw`N@en@j$M+H5aOdP?v%!kaJ7Q^GR@Sd<&O|1W+#s^Feh7xjMS|y6Tlwl;Cr% ze_l?d0?!@YfMULjso-61W%vi)s>T z&_YaN1e&p>(GF@7N literal 0 HcmV?d00001 diff --git a/lib/tests/update_code_manager_test.php b/lib/tests/update_code_manager_test.php index 6166152d3a5..cbf4717cdee 100644 --- a/lib/tests/update_code_manager_test.php +++ b/lib/tests/update_code_manager_test.php @@ -73,6 +73,7 @@ class core_update_code_manager_testcase extends advanced_testcase { $codeman = new \core\update\testable_code_manager(); $zipfilepath = __DIR__.'/fixtures/update_validator/zips/invalidroot.zip'; $targetdir = make_request_directory(); + mkdir($targetdir.'/aaa_another'); $files = $codeman->unzip_plugin_file($zipfilepath, $targetdir); @@ -110,6 +111,15 @@ class core_update_code_manager_testcase extends advanced_testcase { $files = $codeman->unzip_plugin_file($zipfilepath, $targetdir, 'bar'); } + public function test_unzip_plugin_file_multidir() { + $codeman = new \core\update\testable_code_manager(); + $zipfilepath = __DIR__.'/fixtures/update_validator/zips/multidir.zip'; + $targetdir = make_request_directory(); + // Attempting to rename the root folder if there are multiple ones should lead to exception. + $this->setExpectedException('moodle_exception'); + $files = $codeman->unzip_plugin_file($zipfilepath, $targetdir, 'foo'); + } + public function test_get_plugin_zip_root_dir() { $codeman = new \core\update\testable_code_manager(); @@ -118,6 +128,9 @@ class core_update_code_manager_testcase extends advanced_testcase { $zipfilepath = __DIR__.'/fixtures/update_validator/zips/bar.zip'; $this->assertEquals('bar', $codeman->get_plugin_zip_root_dir($zipfilepath)); + + $zipfilepath = __DIR__.'/fixtures/update_validator/zips/multidir.zip'; + $this->assertSame(false, $codeman->get_plugin_zip_root_dir($zipfilepath)); } public function test_list_plugin_folder_files() { -- 2.17.1