From 8acee4b53d79500cda7056e712a1ef43c6997950 Mon Sep 17 00:00:00 2001 From: =?utf8?q?David=20Mudr=C3=A1k?= Date: Wed, 7 Oct 2015 11:50:49 +0200 Subject: [PATCH] MDL-49329 admin: Improve the plugin package validator Previously, the validator was used for installation of the new plugins only. We now validate every incoming plugin package. As a result, we must no longer insists on the target location is empty. Instead, warning is raised. --- lang/en/plugin.php | 5 +- lib/classes/plugin_manager.php | 2 +- lib/classes/update/validator.php | 49 ++++++++++++------- .../fixtures/testable_update_validator.php | 7 +-- lib/tests/update_validator_test.php | 18 ++++++- 5 files changed, 52 insertions(+), 29 deletions(-) diff --git a/lang/en/plugin.php b/lang/en/plugin.php index 19fe70659a5..189d817c5a6 100644 --- a/lang/en/plugin.php +++ b/lang/en/plugin.php @@ -201,8 +201,9 @@ $string['validationmsg_rootdir'] = 'Name of the plugin to be installed'; $string['validationmsg_rootdir_help'] = 'The name of the root directory in the ZIP package forms the name of the plugin to be installed. If the name is not correct, you may wish to rename the root directory in the ZIP prior to installing the plugin.'; $string['validationmsg_rootdirinvalid'] = 'Invalid plugin name'; $string['validationmsg_rootdirinvalid_help'] = 'The name of the root directory in the ZIP package violates formal syntax requirements. Some ZIP packages, such as those generated by Github, may contain an incorrect root directory name. You need to fix the name of the root directory to match the plugin name.'; -$string['validationmsg_targetexists'] = 'Target location already exists'; -$string['validationmsg_targetexists_help'] = 'The directory that the plugin is to be installed to must not yet exist.'; +$string['validationmsg_targetexists'] = 'Target location already exists and will be removed'; +$string['validationmsg_targetexists_help'] = 'The plugin directory already exists and will be replaced by the plugin package contents.'; +$string['validationmsg_targetnotdir'] = 'Target location occupied by a file'; $string['validationmsg_unknowntype'] = 'Unknown plugin type'; $string['validationmsg_versionphpsyntax'] = 'Unsupported syntax detected in version.php file'; $string['validationmsglevel_debug'] = 'Debug'; diff --git a/lib/classes/plugin_manager.php b/lib/classes/plugin_manager.php index 860af8a3b22..7c4e09197ee 100644 --- a/lib/classes/plugin_manager.php +++ b/lib/classes/plugin_manager.php @@ -1807,7 +1807,7 @@ class core_plugin_manager { * @param string $fullpath * @return boolean */ - protected function is_directory_removable($fullpath) { + public function is_directory_removable($fullpath) { if (!is_writable($fullpath)) { return false; diff --git a/lib/classes/update/validator.php b/lib/classes/update/validator.php index 64c13cbd092..e4e2a94fee3 100644 --- a/lib/classes/update/validator.php +++ b/lib/classes/update/validator.php @@ -29,6 +29,7 @@ namespace core\update; use core_component; +use core_plugin_manager; use help_icon; use coding_exception; @@ -474,18 +475,31 @@ class validator { throw new coding_exception('Plugin type location does not exist!'); } - $target = $plugintypepath.'/'.$this->rootdir; - - if (file_exists($target)) { - $this->add_message(self::ERROR, 'targetexists', $target); + // Always check that the plugintype root is writable. + if (!is_writable($plugintypepath)) { + $this->add_message(self::ERROR, 'pathwritable', $plugintypepath); return false; + } else { + $this->add_message(self::INFO, 'pathwritable', $plugintypepath); } - if (is_writable($plugintypepath)) { - $this->add_message(self::INFO, 'pathwritable', $plugintypepath); - } else { - $this->add_message(self::ERROR, 'pathwritable', $plugintypepath); - return false; + // The target location itself may or may not exist. Even if installing an + // available update, the code could have been removed by accident (and + // be reported as missing) etc. So we just make sure that the code + // can be replaced if it already exists. + $target = $plugintypepath.'/'.$this->rootdir; + if (file_exists($target)) { + if (!is_dir($target)) { + $this->add_message(self::ERROR, 'targetnotdir', $target); + return false; + } + $this->add_message(self::WARNING, 'targetexists', $target); + if ($this->get_plugin_manager()->is_directory_removable($target)) { + $this->add_message(self::INFO, 'pathwritable', $target); + } else { + $this->add_message(self::ERROR, 'pathwritable', $target); + return false; + } } return true; @@ -599,16 +613,13 @@ class validator { * @return string|null */ public function get_plugintype_location($plugintype) { + return $this->get_plugin_manager()->get_plugintype_root($plugintype); + } - $plugintypepath = null; - - foreach (core_component::get_plugin_types() as $type => $fullpath) { - if ($type === $plugintype) { - $plugintypepath = $fullpath; - break; - } - } - - return $plugintypepath; + /** + * @return core_plugin_manager + */ + protected function get_plugin_manager() { + return core_plugin_manager::instance(); } } diff --git a/lib/tests/fixtures/testable_update_validator.php b/lib/tests/fixtures/testable_update_validator.php index 8d75a7774c1..5c8edcd384c 100644 --- a/lib/tests/fixtures/testable_update_validator.php +++ b/lib/tests/fixtures/testable_update_validator.php @@ -40,15 +40,10 @@ class testable_core_update_validator extends \core\update\validator { public function get_plugintype_location($plugintype) { $testableroot = make_temp_directory('testable_core_update_validator/plugintypes'); - if (!is_dir($testableroot.'/'.$plugintype)) { + if (!file_exists($testableroot.'/'.$plugintype)) { make_temp_directory('testable_core_update_validator/plugintypes/'.$plugintype); } - if ($plugintype === 'local') { - // We need the following for the test_validate_target_location() method - make_temp_directory('testable_core_update_validator/plugintypes/local/greenbar'); - } - return $testableroot.'/'.$plugintype; } } diff --git a/lib/tests/update_validator_test.php b/lib/tests/update_validator_test.php index dfb46d381d7..f8342a9be25 100644 --- a/lib/tests/update_validator_test.php +++ b/lib/tests/update_validator_test.php @@ -277,10 +277,26 @@ class core_update_validator_testcase extends advanced_testcase { 'greenbar/lang/en/local_greenbar.php' => true)); $validator->assert_plugin_type('local'); $validator->assert_moodle_version('2013031400.00'); + + $this->assertTrue($validator->execute()); + $this->assertFalse($this->has_message($validator->get_messages(), $validator::WARNING, 'targetexists', + $validator->get_plugintype_location('local').'/greenbar')); + + $typeroot = $validator->get_plugintype_location('local'); + make_writable_directory($typeroot.'/greenbar'); + $this->assertTrue($validator->execute()); + $this->assertTrue($this->has_message($validator->get_messages(), $validator::WARNING, 'targetexists', + $validator->get_plugintype_location('local').'/greenbar')); + + remove_dir($typeroot.'/greenbar'); + file_put_contents($typeroot.'/greenbar', 'This file occupies a place where a plugin should land.'); $this->assertFalse($validator->execute()); - $this->assertTrue($this->has_message($validator->get_messages(), $validator::ERROR, 'targetexists', + $this->assertTrue($this->has_message($validator->get_messages(), $validator::ERROR, 'targetnotdir', $validator->get_plugintype_location('local').'/greenbar')); + unlink($typeroot.'/greenbar'); + $this->assertTrue($validator->execute()); + $validator = testable_core_update_validator::instance($fixtures.'/plugindir', array( 'foobar/' => true, 'foobar/version.php' => true, -- 2.17.1