From 1661204a6c27de6fdfbdc89809455417f72a42e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Fri, 14 Jul 2017 14:48:19 +0200 Subject: [PATCH 1/2] MDL-59544 forms: Implicit validation of the filemanager and filepicker The patch adds an extra validation step against accepted file types. Even if the repository checks are bypassed (as illustrated in the Behat), the invalid file is still caught by the element's validation rules. It turns out there is no way to test the filepicker element easily via Behat. Additionally, it provides the renaming features only with disabled javascript. So the Behat tests are provided for the filemanager only. AMOS BEGIN CPY [err_wrongfileextension,mod_workshop],[err_wrongfileextension,core_form] AMOS END --- lang/en/form.php | 1 + lib/form/filemanager.php | 41 ++++++++++++++++++++++++++ lib/form/filepicker.php | 41 ++++++++++++++++++++++++++ lib/form/tests/behat/filetypes.feature | 13 +++++++- lib/form/tests/fixtures/filetypes.php | 2 ++ 5 files changed, 97 insertions(+), 1 deletion(-) diff --git a/lang/en/form.php b/lang/en/form.php index e13d8b29086..2b9b10ab6c4 100644 --- a/lang/en/form.php +++ b/lang/en/form.php @@ -41,6 +41,7 @@ $string['err_nopunctuation'] = 'You must enter no punctuation characters here.'; $string['err_numeric'] = 'You must enter a number here.'; $string['err_rangelength'] = 'You must enter between {$a->format[0]} and {$a->format[1]} characters here.'; $string['err_required'] = 'You must supply a value here.'; +$string['err_wrongfileextension'] = 'Some files ({$a->wrongfiles}) cannot be uploaded. Only file types {$a->whitelist} are allowed.'; $string['filetypesany'] = 'All file types'; $string['filetypesnotall'] = 'It is not allowed to select \'All file types\' here'; $string['filetypesnotwhitelisted'] = 'These file types are not allowed here: {$a}'; diff --git a/lib/form/filemanager.php b/lib/form/filemanager.php index 1c9126fe0c0..c7a63f21ea0 100644 --- a/lib/form/filemanager.php +++ b/lib/form/filemanager.php @@ -308,6 +308,47 @@ class MoodleQuickForm_filemanager extends HTML_QuickForm_element implements temp $context['html'] = $this->toHtml(); return $context; } + + /** + * Check that all files have the allowed type. + * + * @param array $value Draft item id with the uploaded files. + * @return string|null Validation error message or null. + */ + public function validateSubmitValue($value) { + + $filetypesutil = new \core_form\filetypes_util(); + $whitelist = $filetypesutil->normalize_file_types($this->_options['accepted_types']); + + if (empty($whitelist) || $whitelist === ['*']) { + // Any file type is allowed, nothing to check here. + return; + } + + $draftfiles = file_get_drafarea_files($value); + $wrongfiles = array(); + + if (empty($draftfiles)) { + // No file uploaded, nothing to check here. + return; + } + + foreach ($draftfiles->list as $file) { + if (!$filetypesutil->is_allowed_file_type($file->filename, $whitelist)) { + $wrongfiles[] = $file->filename; + } + } + + if ($wrongfiles) { + $a = array( + 'whitelist' => implode(', ', $whitelist), + 'wrongfiles' => implode(', ', $wrongfiles), + ); + return get_string('err_wrongfileextension', 'core_form', $a); + } + + return; + } } /** diff --git a/lib/form/filepicker.php b/lib/form/filepicker.php index 985879fd29d..c1485b2d586 100644 --- a/lib/form/filepicker.php +++ b/lib/form/filepicker.php @@ -230,4 +230,45 @@ class MoodleQuickForm_filepicker extends HTML_QuickForm_input implements templat $context['html'] = $this->toHtml(); return $context; } + + /** + * Check that the file has the allowed type. + * + * @param array $value Draft item id with the uploaded files. + * @return string|null Validation error message or null. + */ + public function validateSubmitValue($value) { + + $filetypesutil = new \core_form\filetypes_util(); + $whitelist = $filetypesutil->normalize_file_types($this->_options['accepted_types']); + + if (empty($whitelist) || $whitelist === ['*']) { + // Any file type is allowed, nothing to check here. + return; + } + + $draftfiles = file_get_drafarea_files($value); + $wrongfiles = array(); + + if (empty($draftfiles)) { + // No file uploaded, nothing to check here. + return; + } + + foreach ($draftfiles->list as $file) { + if (!$filetypesutil->is_allowed_file_type($file->filename, $whitelist)) { + $wrongfiles[] = $file->filename; + } + } + + if ($wrongfiles) { + $a = array( + 'whitelist' => implode(', ', $whitelist), + 'wrongfiles' => implode(', ', $wrongfiles), + ); + return get_string('err_wrongfileextension', 'core_form', $a); + } + + return; + } } diff --git a/lib/form/tests/behat/filetypes.feature b/lib/form/tests/behat/filetypes.feature index cf75dd2b9b3..653e50d2b4d 100644 --- a/lib/form/tests/behat/filetypes.feature +++ b/lib/form/tests/behat/filetypes.feature @@ -34,4 +34,15 @@ Feature: There is a form element allowing to select filetypes Then I should see "Unknown file types: .doesnoexist" And I should see "These file types are not allowed here: .doc, .docx, .rtf" And I should see "It is not allowed to select 'All file types' here" - And I should not see "Unknown file types: .neverminditdoesnotexist" \ No newline at end of file + And I should not see "Unknown file types: .neverminditdoesnotexist" + + @javascript @_file_upload + Scenario: File manager element implicitly validates submitted files + # We can't directly upload the invalid file here as the upload repository would throw an exception. + # So instead we try to trick the filemanager, to be finally stopped by the implicit validation. + And I upload "lib/tests/fixtures/empty.txt" file to "Picky file manager" filemanager + And I follow "empty.txt" + And I set the field "Name" to "renamed.exe" + And I press "Update" + When I press "Save changes" + Then I should see "Some files (renamed.exe) cannot be uploaded. Only file types .txt are allowed." diff --git a/lib/form/tests/fixtures/filetypes.php b/lib/form/tests/fixtures/filetypes.php index 331fbfa7fe8..8477743c315 100644 --- a/lib/form/tests/fixtures/filetypes.php +++ b/lib/form/tests/fixtures/filetypes.php @@ -57,6 +57,8 @@ class test_form extends moodleform { $mform->addElement('filetypes', 'filetypes3', 'Unknown file types are allowed here', ['allowunknown' => true]); + $mform->addElement('filemanager', 'fileman1', 'Picky file manager', null, ['accepted_types' => '.txt']); + $this->add_action_buttons(false); } } From 0fa2b2ee8829608deb82f6414d1458855f7cedb5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Fri, 14 Jul 2017 14:54:19 +0200 Subject: [PATCH 2/2] MDL-59544 workshop: Drop the custom validation of allowed file types Now that the validation is provided directly by the filemanager, we do not need to do the same here. Note that existing Behat tests are left untouched to make sure there is no regression. --- mod/workshop/form/assessment_form.php | 37 --------------------------- mod/workshop/lang/en/deprecated.txt | 1 + mod/workshop/lang/en/workshop.php | 2 +- mod/workshop/submission_form.php | 23 ----------------- 4 files changed, 2 insertions(+), 61 deletions(-) diff --git a/mod/workshop/form/assessment_form.php b/mod/workshop/form/assessment_form.php index 7ecbcf6738c..56128dcf5b3 100644 --- a/mod/workshop/form/assessment_form.php +++ b/mod/workshop/form/assessment_form.php @@ -120,41 +120,4 @@ class workshop_assessment_form extends moodleform { public function is_editable() { return !$this->_form->isFrozen(); } - - /** - * Validate assessment form data. - * - * @param array $data - * @param array $files - * @return array - */ - public function validation($data, $files) { - - $errors = parent::validation($data, $files); - - if (isset($data['feedbackauthorattachment_filemanager']) and isset($this->workshop->overallfeedbackfiletypes)) { - $filetypesutil = new \core_form\filetypes_util(); - $whitelist = $filetypesutil->normalize_file_types($this->workshop->overallfeedbackfiletypes); - if ($whitelist) { - $draftfiles = file_get_drafarea_files($data['feedbackauthorattachment_filemanager']); - if ($draftfiles) { - $wrongfiles = array(); - foreach ($draftfiles->list as $file) { - if (!$filetypesutil->is_allowed_file_type($file->filename, $whitelist)) { - $wrongfiles[] = $file->filename; - } - } - if ($wrongfiles) { - $a = array( - 'whitelist' => implode(', ', $whitelist), - 'wrongfiles' => implode(', ', $wrongfiles), - ); - $errors['feedbackauthorattachment_filemanager'] = get_string('err_wrongfileextension', 'mod_workshop', $a); - } - } - } - } - - return $errors; - } } diff --git a/mod/workshop/lang/en/deprecated.txt b/mod/workshop/lang/en/deprecated.txt index 01fea12c442..dc549a1c974 100644 --- a/mod/workshop/lang/en/deprecated.txt +++ b/mod/workshop/lang/en/deprecated.txt @@ -1,2 +1,3 @@ err_unknownfileextension,mod_workshop +err_wrongfileextension,mod_workshop yourassessment,mod_workshop diff --git a/mod/workshop/lang/en/workshop.php b/mod/workshop/lang/en/workshop.php index 21df5a6d7c4..6af18ef1ef8 100644 --- a/mod/workshop/lang/en/workshop.php +++ b/mod/workshop/lang/en/workshop.php @@ -107,7 +107,6 @@ $string['editingsubmission'] = 'Editing submission'; $string['editsubmission'] = 'Edit submission'; $string['err_multiplesubmissions'] = 'While editing this form, another version of the submission has been saved. Multiple submissions per user are not allowed.'; $string['err_removegrademappings'] = 'Unable to remove the unused grade mappings'; -$string['err_wrongfileextension'] = 'Some files ({$a->wrongfiles}) cannot be uploaded. Only file types {$a->whitelist} are allowed.'; $string['evaluategradeswait'] = 'Please wait until the assessments are evaluated and the grades are calculated'; $string['evaluation'] = 'Grading evaluation'; $string['evaluationmethod'] = 'Grading evaluation method'; @@ -373,3 +372,4 @@ $string['yourassessment'] = 'Your assessment'; // Deprecated since Moodle 3.4. $string['err_unknownfileextension'] = 'Unknown file extension: {$a}'; +$string['err_wrongfileextension'] = 'Some files ({$a->wrongfiles}) cannot be uploaded. Only file types {$a->whitelist} are allowed.'; diff --git a/mod/workshop/submission_form.php b/mod/workshop/submission_form.php index 6b794dcd935..29980feb374 100644 --- a/mod/workshop/submission_form.php +++ b/mod/workshop/submission_form.php @@ -94,29 +94,6 @@ class workshop_submission_form extends moodleform { $errors['attachment_filemanager'] = get_string('submissionrequiredfile', 'mod_workshop'); } - if (isset($data['attachment_filemanager']) and isset($this->_customdata['workshop']->submissionfiletypes)) { - $filetypesutil = new \core_form\filetypes_util(); - $whitelist = $filetypesutil->normalize_file_types($this->_customdata['workshop']->submissionfiletypes); - if ($whitelist) { - $draftfiles = file_get_drafarea_files($data['attachment_filemanager']); - if ($draftfiles) { - $wrongfiles = array(); - foreach ($draftfiles->list as $file) { - if (!$filetypesutil->is_allowed_file_type($file->filename, $whitelist)) { - $wrongfiles[] = $file->filename; - } - } - if ($wrongfiles) { - $a = array( - 'whitelist' => implode(', ', $whitelist), - 'wrongfiles' => implode(', ', $wrongfiles), - ); - $errors['attachment_filemanager'] = get_string('err_wrongfileextension', 'mod_workshop', $a); - } - } - } - } - return $errors; } }