From 6ff0525a3f13eebab426f388dc9c8b89eb9a123e Mon Sep 17 00:00:00 2001 From: Eric Merrill Date: Fri, 31 Jul 2020 19:06:49 -0400 Subject: [PATCH] MDL-69112 assign: Improve parsing of uploaded feedback names --- mod/assign/feedback/file/importziplib.php | 68 +++++--- .../feedback/file/tests/importziplib_test.php | 148 ++++++++++++++++++ 2 files changed, 192 insertions(+), 24 deletions(-) create mode 100644 mod/assign/feedback/file/tests/importziplib_test.php diff --git a/mod/assign/feedback/file/importziplib.php b/mod/assign/feedback/file/importziplib.php index 01d9a3346d6..3cbe9f79c1e 100644 --- a/mod/assign/feedback/file/importziplib.php +++ b/mod/assign/feedback/file/importziplib.php @@ -59,33 +59,53 @@ class assignfeedback_file_zip_importer { return false; } - $info = explode('_', $fileinfo->get_filepath() . $fileinfo->get_filename(), 5); + // Break the full path-name into path parts. + $pathparts = explode('/', $fileinfo->get_filepath() . $fileinfo->get_filename()); - if (count($info) < 5) { - return false; + while (!empty($pathparts)) { + // Get the next path part and break it up by underscores. + $pathpart = array_shift($pathparts); + $info = explode('_', $pathpart, 5); + + if (count($info) < 5) { + continue; + } + + // Check the participant id. + $participantid = $info[1]; + + if (!is_numeric($participantid)) { + continue; + } + + // Convert to int. + $participantid += 0; + + if (empty($participants[$participantid])) { + continue; + } + + // Set user, which is by reference, so is used by the calling script. + $user = $participants[$participantid]; + + // Set the plugin. This by reference, and is used by the calling script. + $plugin = $assignment->get_plugin_by_type($info[2], $info[3]); + + if (!$plugin) { + continue; + } + + // Take any remaining text in this part and put it back in the path parts array. + array_unshift($pathparts, $info[4]); + + // Combine the remaining parts and set it as the filename. + // Note that filename is a 'by reference' variable, so we need to set it before returning. + $filename = implode('/', $pathparts); + + return true; } - $participantid = $info[1]; - $filename = $info[4]; - $plugin = $assignment->get_plugin_by_type($info[2], $info[3]); - - if (!is_numeric($participantid)) { - return false; - } - - if (!$plugin) { - return false; - } - - // Convert to int. - $participantid += 0; - - if (empty($participants[$participantid])) { - return false; - } - - $user = $participants[$participantid]; - return true; + return false; } /** diff --git a/mod/assign/feedback/file/tests/importziplib_test.php b/mod/assign/feedback/file/tests/importziplib_test.php new file mode 100644 index 00000000000..009579b893b --- /dev/null +++ b/mod/assign/feedback/file/tests/importziplib_test.php @@ -0,0 +1,148 @@ +. + +/** + * Unit tests for importziplib. + * + * @package assignfeedback_file + * @copyright 2020 Eric Merrill + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +defined('MOODLE_INTERNAL') || die(); + +global $CFG; +require_once($CFG->dirroot . '/mod/assign/tests/generator.php'); +require_once($CFG->dirroot . '/mod/assign/feedback/file/importziplib.php'); + +/** + * Unit tests for importziplib. + * + * @copyright 2020 Eric Merrill + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class assignfeedback_importziplib_testcase extends advanced_testcase { + + // Use the generator helper. + use mod_assign_test_generator; + + /** + * Test the assignfeedback_file_zip_importer->is_valid_filename_for_import() method. + */ + public function test_is_valid_filename_for_import() { + // Do the initial assign setup. + $this->resetAfterTest(); + $course = $this->getDataGenerator()->create_course(); + $teacher = $this->getDataGenerator()->create_and_enrol($course, 'teacher'); + $student = $this->getDataGenerator()->create_and_enrol($course, 'student'); + + $assign = $this->create_instance($course, [ + 'assignsubmission_onlinetext_enabled' => 1, + 'assignfeedback_file_enabled' => 1, + ]); + + // Create an online text submission. + $this->add_submission($student, $assign); + + // Now onto the file work. + $fs = get_file_storage(); + + // Setup a basic file we will work with. We will keep renaming and repathing it. + $record = new stdClass; + $record->contextid = $assign->get_context()->id; + $record->component = 'assignfeedback_file'; + $record->filearea = ASSIGNFEEDBACK_FILE_FILEAREA; + $record->itemid = $assign->get_user_grade($student->id, true)->id; + $record->filepath = '/'; + $record->filename = '1.txt'; + $record->source = 'test'; + $file = $fs->create_file_from_string($record, 'file content'); + + // The importer we will use. + $importer = new assignfeedback_file_zip_importer(); + + // Setup some variable we use. + $user = null; + $plugin = null; + $filename = ''; + + $allusers = $assign->list_participants(0, false); + $participants = array(); + foreach ($allusers as $user) { + $participants[$assign->get_uniqueid_for_user($user->id)] = $user; + } + + $file->rename('/import/', '.hiddenfile'); + $result = $importer->is_valid_filename_for_import($assign, $file, $participants, $user, $plugin, $filename); + $this->assertFalse($result); + + $file->rename('/import/', '~hiddenfile'); + $result = $importer->is_valid_filename_for_import($assign, $file, $participants, $user, $plugin, $filename); + $this->assertFalse($result); + + $file->rename('/import/some_path_here/', 'RandomFile.txt'); + $result = $importer->is_valid_filename_for_import($assign, $file, $participants, $user, $plugin, $filename); + $this->assertFalse($result); + + $file->rename('/import/', '~hiddenfile'); + $result = $importer->is_valid_filename_for_import($assign, $file, $participants, $user, $plugin, $filename); + $this->assertFalse($result); + + // Get the students assign id. + $studentid = $assign->get_uniqueid_for_user($student->id); + + // Submissions are identified with the format: + // StudentName_StudentID_PluginType_Plugin_FilePathAndName. + + // Test a string student id. + $badname = 'Student Name_StringID_assignsubmission_file_My_cool_filename.txt'; + $file->rename('/import/', $badname); + $result = $importer->is_valid_filename_for_import($assign, $file, $participants, $user, $plugin, $filename); + $this->assertFalse($result); + + // Test an invalid student id. + $badname = 'Student Name_' . ($studentid + 100) . '_assignsubmission_file_My_cool_filename.txt'; + $file->rename('/import/', $badname); + $result = $importer->is_valid_filename_for_import($assign, $file, $participants, $user, $plugin, $filename); + $this->assertFalse($result); + + // Test an invalid submission plugin. + $badname = 'Student Name_' . $studentid . '_assignsubmission_noplugin_My_cool_filename.txt'; + $file->rename('/import/', $badname); + $result = $importer->is_valid_filename_for_import($assign, $file, $participants, $user, $plugin, $filename); + $this->assertFalse($result); + + // Test a basic, good file. + $goodbase = 'Student Name_' . $studentid . '_assignsubmission_file_'; + $file->rename('/import/', $goodbase . "My_cool_filename.txt"); + $result = $importer->is_valid_filename_for_import($assign, $file, $participants, $user, $plugin, $filename); + $this->assertTrue($result); + $this->assertEquals($participants[$studentid], $user); + $this->assertEquals('My_cool_filename.txt', $filename); + $this->assertInstanceOf(assign_submission_file::class, $plugin); + + // Test another good file, with some additional path and underscores. + $user = null; + $plugin = null; + $filename = ''; + $file->rename('/import/some_path_here/' . $goodbase . '/some_path/', 'My File.txt'); + $result = $importer->is_valid_filename_for_import($assign, $file, $participants, $user, $plugin, $filename); + $this->assertTrue($result); + $this->assertEquals($participants[$studentid], $user); + $this->assertEquals('/some_path/My File.txt', $filename); + $this->assertInstanceOf(assign_submission_file::class, $plugin); + } +}