From ea3f8fa0cbe5b077d74dcc741735ee9d4d93062f Mon Sep 17 00:00:00 2001 From: John Okely Date: Mon, 3 Apr 2017 14:38:52 +0800 Subject: [PATCH] MDL-39913 assignsubmission_file: Add validation --- .../file/lang/en/assignsubmission_file.php | 1 + mod/assign/submission/file/locallib.php | 71 ++++++++++++++++--- .../tests/behat/file_type_restriction.feature | 14 ++-- .../submission/file/tests/locallib_test.php | 64 +++++++++++++++++ 4 files changed, 138 insertions(+), 12 deletions(-) diff --git a/mod/assign/submission/file/lang/en/assignsubmission_file.php b/mod/assign/submission/file/lang/en/assignsubmission_file.php index 9c83b5824a8..381adb28b0f 100644 --- a/mod/assign/submission/file/lang/en/assignsubmission_file.php +++ b/mod/assign/submission/file/lang/en/assignsubmission_file.php @@ -42,6 +42,7 @@ $string['maxfilessubmission'] = 'Maximum number of uploaded files'; $string['maxfilessubmission_help'] = 'If file submissions are enabled, each student will be able to upload up to this number of files for their submission.'; $string['maximumsubmissionsize'] = 'Maximum submission size'; $string['maximumsubmissionsize_help'] = 'Files uploaded by students may be up to this size.'; +$string['nonexistentfiletypes'] = 'The following file types were not recognised: {$a}'; $string['numfilesforlog'] = 'The number of file(s) : {$a} file(s).'; $string['pluginname'] = 'File submissions'; $string['siteuploadlimit'] = 'Site upload limit'; diff --git a/mod/assign/submission/file/locallib.php b/mod/assign/submission/file/locallib.php index a30dd1f5098..dccdfe3fc73 100644 --- a/mod/assign/submission/file/locallib.php +++ b/mod/assign/submission/file/locallib.php @@ -113,6 +113,18 @@ class assign_submission_file extends assign_submission_plugin { $mform->setType('assignsubmission_file_filetypes', PARAM_RAW); $mform->setDefault('assignsubmission_file_filetypes', $defaultfiletypes); $mform->disabledIf('assignsubmission_file_filetypes', 'assignsubmission_file_enabled', 'notchecked'); + $mform->addFormRule(function ($values, $files) { + if (empty($values['assignsubmission_file_filetypes'])) { + return true; + } + $nonexistent = $this->get_nonexistent_file_types($values['assignsubmission_file_filetypes']); + if (empty($nonexistent)) { + return true; + } else { + $a = join(' ', $nonexistent); + return ["assignsubmission_file_filetypes" => get_string('nonexistentfiletypes', 'assignsubmission_file', $a)]; + } + }); } /** @@ -185,13 +197,21 @@ class assign_submission_file extends assign_submission_plugin { $typesets = $this->get_configured_typesets(); foreach ($typesets as $type) { $a = new stdClass(); - if (strpos($type, '/') !== false) { - $a->name = get_mimetype_description($type); - } else { - $a->name = get_string("group:$type", 'mimetypes'); + $extensions = file_get_typegroup('extension', $type); + $typetext = html_writer::tag('li', $type); + // Only bother checking if it's a mimetype or group if it has extensions in the group. + if (!empty($extensions)) { + if (strpos($type, '/') !== false) { + $a->name = get_mimetype_description($type); + $a->extlist = implode(' ', $extensions); + $typetext = html_writer::tag('li', get_string('filetypewithexts', 'assignsubmission_file', $a)); + } else if (get_string_manager()->string_exists("group:$type", 'mimetypes')) { + $a->name = get_string("group:$type", 'mimetypes'); + $a->extlist = implode(' ', $extensions); + $typetext = html_writer::tag('li', get_string('filetypewithexts', 'assignsubmission_file', $a)); + } } - $a->extlist = implode(' ', file_get_typegroup('extension', $type)); - $text .= html_writer::tag('li', get_string('filetypewithexts', 'assignsubmission_file', $a)); + $text .= $typetext; } $text .= html_writer::end_tag('ul'); @@ -615,9 +635,21 @@ class assign_submission_file extends assign_submission_plugin { private function get_configured_typesets() { $typeslist = (string)$this->get_config('filetypeslist'); + $sets = $this->get_typesets($typeslist); + + return $sets; + } + + /** + * Get the type sets passed. + * + * @param string $types The space , ; separated list of types + * @return array('groupname', 'mime/type', ...) + */ + private function get_typesets($types) { $sets = array(); - if (!empty($typeslist)) { - $sets = explode(';', $typeslist); + if (!empty($types)) { + $sets = preg_split('/[\s,;:"\']+/', $types, null, PREG_SPLIT_NO_EMPTY); } return $sets; } @@ -637,4 +669,27 @@ class assign_submission_file extends assign_submission_plugin { return '*'; } + + /** + * List the nonexistent file types that need to be removed. + * + * @param string $types space , or ; separated types + * @return array A list of the nonexistent file types. + */ + private function get_nonexistent_file_types($types) { + $nonexistent = []; + foreach ($this->get_typesets($types) as $type) { + $coretypes = core_filetypes::get_types(); + // We can allow any extension, but validate groups & mimetypes. + if (strpos($type, '.') === false) { + // If there's no dot, check if it's a group. + $extensions = file_get_typegroup('extension', [$type]); + if (empty($extensions)) { + // If there's no extensions under that group, it doesn't exist. + $nonexistent[$type] = true; + } + } + } + return array_keys($nonexistent); + } } diff --git a/mod/assign/submission/file/tests/behat/file_type_restriction.feature b/mod/assign/submission/file/tests/behat/file_type_restriction.feature index ed765b70805..4990daece70 100644 --- a/mod/assign/submission/file/tests/behat/file_type_restriction.feature +++ b/mod/assign/submission/file/tests/behat/file_type_restriction.feature @@ -20,7 +20,7 @@ Feature: In an assignment, limit submittable file types | filetypes | image/png;spreadsheet | assignsubmission_file | @javascript - Scenario: Configuring permitted file types for an assignment + Scenario: File types validation for an assignment Given the following "activities" exist: | activity | course | idnumber | name | intro | duedate | assignsubmission_onlinetext_enabled | assignsubmission_file_enabled | assignsubmission_file_maxfiles | assignsubmission_file_maxsizebytes | | assign | C1 | assign1 | Test assignment name | Test assignment description | 1388534400 | 0 | 1 | 1 | 0 | @@ -28,7 +28,10 @@ Feature: In an assignment, limit submittable file types And I am on "Course 1" course homepage And I follow "Test assignment name" And I navigate to "Edit settings" in current page administration - When I set the field "Accepted file types" to "image/png;spreadsheet" + When I set the field "Accepted file types" to "image/png;doesntexist;.anything;unreal/mimetype;nodot" + And I press "Save and display" + And I should see "The following file types were not recognised: doesntexist unreal/mimetype nodot" + And I set the field "Accepted file types" to "image/png;spreadsheet" And I press "Save and display" And I navigate to "Edit settings" in current page administration Then the field "Accepted file types" matches value "image/png;spreadsheet" @@ -37,19 +40,22 @@ Feature: In an assignment, limit submittable file types Scenario: Uploading permitted file types for an assignment Given the following "activities" exist: | activity | course | idnumber | name | intro | duedate | assignsubmission_onlinetext_enabled | assignsubmission_file_enabled | assignsubmission_file_maxfiles | assignsubmission_file_maxsizebytes | assignsubmission_file_filetypes | - | assign | C1 | assign1 | Test assignment name | Test assignment description | 1388534400 | 0 | 1 | 2 | 0 | image/png;spreadsheet | + | assign | C1 | assign1 | Test assignment name | Test assignment description | 1388534400 | 0 | 1 | 3 | 0 | image/png;spreadsheet;.xml;.txt | And I log in as "student1" And I am on "Course 1" course homepage And I follow "Test assignment name" When I press "Add submission" And I should see "Files of these types may be added to the submission" And I should see "Image (PNG) — .png" - And I should see "Spreadsheet files — .csv .ods .ots .xls .xlsx .xlsm" + And I should see "Spreadsheet files — .csv .gsheet .ods .ots .xls .xlsx .xlsm" + And I should see ".txt" And I upload "lib/tests/fixtures/gd-logo.png" file to "File submissions" filemanager And I upload "lib/tests/fixtures/tabfile.csv" file to "File submissions" filemanager + And I upload "lib/tests/fixtures/empty.txt" file to "File submissions" filemanager And I press "Save changes" Then "gd-logo.png" "link" should exist And "tabfile.csv" "link" should exist + And "empty.txt" "link" should exist @javascript @_file_upload Scenario: No filetypes allows all diff --git a/mod/assign/submission/file/tests/locallib_test.php b/mod/assign/submission/file/tests/locallib_test.php index 8df613a58da..c8b1082bdaf 100644 --- a/mod/assign/submission/file/tests/locallib_test.php +++ b/mod/assign/submission/file/tests/locallib_test.php @@ -137,5 +137,69 @@ class assignsubmission_file_locallib_testcase extends advanced_testcase { ]; } + /** + * Data provider for testing test_get_nonexistent_file_types. + * + * @return array + */ + public function get_nonexistent_file_types_provider() { + return [ + 'Nonexistent extensions are allowed' => [ + 'filetypes' => '.rat', + 'expected' => [] + ], + 'Multiple nonexistent extensions are allowed' => [ + 'filetypes' => '.ricefield .rat', + 'expected' => [] + ], + 'Existent extension is allowed' => [ + 'filetypes' => '.xml', + 'expected' => [] + ], + 'Existent group is allowed' => [ + 'filetypes' => 'web_file', + 'expected' => [] + ], + 'Nonexistent group is not allowed' => [ + 'filetypes' => '©ç√√ß∂å√©åß©√', + 'expected' => ['©ç√√ß∂å√©åß©√'] + ], + 'Existent mimetype is allowed' => [ + 'filetypes' => 'application/xml', + 'expected' => [] + ], + 'Nonexistent mimetype is not allowed' => [ + 'filetypes' => 'ricefield/rat', + 'expected' => ['ricefield/rat'] + ], + 'Multiple nonexistent mimetypes are not allowed' => [ + 'filetypes' => 'ricefield/rat cam/ball', + 'expected' => ['ricefield/rat', 'cam/ball'] + ], + 'Missing dot in extension is not allowed' => [ + 'filetypes' => 'png', + 'expected' => ['png'] + ], + 'Some existent some not' => [ + 'filetypes' => '.txt application/xml web_file ©ç√√ß∂å√©åß©√ .png ricefield/rat document png', + 'expected' => ['©ç√√ß∂å√©åß©√', 'ricefield/rat', 'png'] + ] + ]; + } + + /** + * Test get_nonexistent_file_types(). + * @dataProvider get_nonexistent_file_types_provider + * @param string $filetypes The filetypes to check + * @param array $expected The expected result. The list of non existent file types. + */ + public function test_get_nonexistent_file_types($filetypes, $expected) { + $this->resetAfterTest(); + $method = new ReflectionMethod(assign_submission_file::class, 'get_nonexistent_file_types'); + $method->setAccessible(true); + $plugin = $this->assign->get_submission_plugin_by_type('file'); + $nonexistentfiletypes = $method->invokeArgs($plugin, [$filetypes]); + $this->assertSame($expected, $nonexistentfiletypes); + } }