Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
MDL-50794 workshop: Improve the file type restricting implementation
This is basically a clean up and what I think improved version of the
original Mahmoud's patch.

The actual checking for allowed file extensions has been re-implemented
and is now covered by unit tests. The list of allowed extensions is now
also assed to the filemanager element's accepted_types option to prevent
picking other files (we still need the in-place validation though). The
form validation is simplified a bit. The custom validation of file size
introduced in the previous patch has been removed as not related to this
issue (also I believe it should not be done at this level).
  • Loading branch information
mudrd8mz committed Feb 25, 2016
1 parent 1a28221 commit 996f7e8
Show file tree
Hide file tree
Showing 10 changed files with 324 additions and 232 deletions.
9 changes: 3 additions & 6 deletions mod/workshop/db/upgrade.php
Expand Up @@ -49,27 +49,24 @@ function xmldb_workshop_upgrade($oldversion) {
$dbman = $DB->get_manager();

if ($oldversion < 2016022200) {

// Define field submissionfiletypes to be added to workshop.
// Add field submissionfiletypes to the table workshop.
$table = new xmldb_table('workshop');
$field = new xmldb_field('submissionfiletypes', XMLDB_TYPE_CHAR, '255', null, null, null, null, 'nattachments');

// Conditionally launch add field submissionfiletypes.
if (!$dbman->field_exists($table, $field)) {
$dbman->add_field($table, $field);
}

// Define field overallfeedbackfiletypes to be added to workshop.
// Add field overallfeedbackfiletypes to the table workshop.
$field = new xmldb_field('overallfeedbackfiletypes',
XMLDB_TYPE_CHAR, '255', null, null, null, null, 'overallfeedbackfiles');

// Conditionally launch add field overallfeedbackfiletypes.
if (!$dbman->field_exists($table, $field)) {
$dbman->add_field($table, $field);
}

// Workshop savepoint reached.
upgrade_mod_savepoint(true, 2016022200, 'workshop');
}

return true;
}
27 changes: 8 additions & 19 deletions mod/workshop/exsubmission.php
Expand Up @@ -115,25 +115,14 @@
if ($edit and $canmanage) {
require_once(dirname(__FILE__).'/submission_form.php');

$maxfiles = $workshop->nattachments;
$maxbytes = $workshop->maxbytes;
$contentopts = array(
'trusttext' => true,
'subdirs' => false,
'maxfiles' => $maxfiles,
'filetypes' => $workshop->submissionfiletypes,
'maxbytes' => $maxbytes,
'context' => $workshop->context
);

$attachmentopts = array('subdirs' => true, 'maxfiles' => $maxfiles, 'maxbytes' => $maxbytes);
$example = file_prepare_standard_editor($example, 'content', $contentopts, $workshop->context,
'mod_workshop', 'submission_content', $example->id);
$example = file_prepare_standard_filemanager($example, 'attachment', $attachmentopts, $workshop->context,
'mod_workshop', 'submission_attachment', $example->id);

$mform = new workshop_submission_form($PAGE->url, array('current' => $example, 'workshop' => $workshop,
'contentopts' => $contentopts, 'attachmentopts' => $attachmentopts));
$example = file_prepare_standard_editor($example, 'content', $workshop->submission_content_options(),
$workshop->context, 'mod_workshop', 'submission_content', $example->id);

$example = file_prepare_standard_filemanager($example, 'attachment', $workshop->submission_attachment_options(),
$workshop->context, 'mod_workshop', 'submission_attachment', $example->id);

$mform = new workshop_submission_form($PAGE->url, array('current' => $example, 'workshop' => $workshop,
'contentopts' => $workshop->submission_content_options(), 'attachmentopts' => $workshop->submission_attachment_options()));

if ($mform->is_cancelled()) {
redirect($workshop->view_url());
Expand Down
50 changes: 19 additions & 31 deletions mod/workshop/form/assessment_form.php
Expand Up @@ -122,50 +122,38 @@ public function is_editable() {
}

/**
* Validate incoming data.
* 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'])) {
$draftitemid = $data['feedbackauthorattachment_filemanager'];

// If we have draft files, then make sure they are the correct ones.
if ($draftfiles = file_get_drafarea_files($draftitemid)) {
$errors = parent::validation($data, $files);

if (!$validfileextensions = workshop::get_array_of_file_extensions($this->workshop->overallfeedbackfiletypes)) {
return $errors;
}
$wrongfileextensions = null;
$bigfiles = null;

// Check the size of each file.
foreach ($draftfiles->list as $file) {
$a = new stdClass();
$a->maxbytes = $this->workshop->overallfeedbackmaxbytes;
$a->currentbytes = $file->size;
$a->filename = $file->filename;
$a->validfileextensions = implode(',', $validfileextensions);

// Check whether the extension of uploaded file is in the list.
$thisextension = substr(strrchr($file->filename, '.'), 1);
if (!in_array($thisextension, $validfileextensions)) {
$wrongfileextensions .= get_string('err_wrongfileextension', 'workshop', $a) . '<br/>';
if (isset($data['feedbackauthorattachment_filemanager']) and isset($this->workshop->overallfeedbackfiletypes)) {
$whitelist = workshop::normalize_file_extensions($this->workshop->overallfeedbackfiletypes);
if ($whitelist) {
$draftfiles = file_get_drafarea_files($data['feedbackauthorattachment_filemanager']);
if ($draftfiles) {
$wrongfiles = array();
foreach ($draftfiles->list as $file) {
if (!workshop::is_allowed_file_type($file->filename, $whitelist)) {
$wrongfiles[] = $file->filename;
}
}
if ($file->size > $this->workshop->overallfeedbackmaxbytes) {
$bigfiles .= get_string('err_maxbytes', 'workshop', $a) . '<br/>';
if ($wrongfiles) {
$a = array(
'whitelist' => workshop::clean_file_extensions($whitelist),
'wrongfiles' => implode(', ', $wrongfiles),
);
$errors['feedbackauthorattachment_filemanager'] = get_string('err_wrongfileextension', 'mod_workshop', $a);
}
}
if ($bigfiles || $wrongfileextensions) {
$errors['feedbackauthorattachment_filemanager'] = $bigfiles . $wrongfileextensions;
}
}
}

return $errors;
}

}
15 changes: 10 additions & 5 deletions mod/workshop/lang/en/workshop.php
Expand Up @@ -31,9 +31,15 @@
$string['allocationerror'] = 'Allocation error';
$string['allocationconfigured'] = 'Allocation configured';
$string['allowedfiletypesforoverallfeedback'] = 'Feedback attachment allowed file types';
$string['allowedfiletypesforoverallfeedback_help'] = 'Feedback attachment allowed file types can be restricted by entering a comma-separated list of file extensions, for example \'mp4, mp3, jpg, jpeg\'. If the field is left empty, then all file types are allowed.';
$string['allowedfiletypesforoverallfeedback_help'] = 'Feedback attachment allowed file types can be restricted by entering a comma-separated list of file extensions, for example \'png, jpg, jpeg, gif\'. If the field is left empty, then all file types are allowed.
Additional supported file extensions can be configured in the server administration';
$string['allowedfiletypesforoverallfeedback_link'] = 'admin/tool/filetypes/index';
$string['allowedfiletypesforsubmission'] = 'Submission attachment allowed file types';
$string['allowedfiletypesforsubmission_help'] = 'Submission attachment allowed file types can be restricted by entering a comma-separated list of file extensions, for example \'mp4, mp3, jpg, jpeg\'. If the field is left empty, then all file types are allowed.';
$string['allowedfiletypesforsubmission_help'] = 'Submission attachment allowed file types can be restricted by entering a comma-separated list of file extensions, for example `png, jpg, jpeg, gif`. If the field is left empty, then all file types are allowed.
Additional supported file extensions can be configured in the server administration';
$string['allowedfiletypesforsubmission_link'] = 'admin/tool/filetypes/index';
$string['allsubmissions'] = 'All submissions ({$a})';
$string['alreadygraded'] = 'Already graded';
$string['areaconclusion'] = 'Conclusion text';
Expand Down Expand Up @@ -104,9 +110,8 @@
$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_maxbytes'] = 'The attachment "{$a->filename} ({$a->currentbytes} bytes)" exeeds the maximum allowed file size ({$a->maxbytes} bytes)';
$string['err_notallowedfiletype'] = 'The file extension "{$a}" is not allowed';
$string['err_wrongfileextension'] = 'The file "{$a->filename}" cannot be uploaded, only file types "{$a->validfileextensions}" are allowed';
$string['err_unknownfileextension'] = 'Unknown file extension: {$a}';
$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';
Expand Down
16 changes: 16 additions & 0 deletions mod/workshop/lib.php
Expand Up @@ -81,6 +81,14 @@ function workshop_add_instance(stdclass $workshop) {
$workshop->phaseswitchassessment = (int)!empty($workshop->phaseswitchassessment);
$workshop->evaluation = 'best';

if (isset($workshop->submissionfiletypes)) {
$workshop->submissionfiletypes = workshop::clean_file_extensions($workshop->submissionfiletypes);
}

if (isset($workshop->overallfeedbackfiletypes)) {
$workshop->overallfeedbackfiletypes = workshop::clean_file_extensions($workshop->overallfeedbackfiletypes);
}

// insert the new record so we get the id
$workshop->id = $DB->insert_record('workshop', $workshop);

Expand Down Expand Up @@ -141,6 +149,14 @@ function workshop_update_instance(stdclass $workshop) {
$workshop->latesubmissions = (int)!empty($workshop->latesubmissions);
$workshop->phaseswitchassessment = (int)!empty($workshop->phaseswitchassessment);

if (isset($workshop->submissionfiletypes)) {
$workshop->submissionfiletypes = workshop::clean_file_extensions($workshop->submissionfiletypes);
}

if (isset($workshop->overallfeedbackfiletypes)) {
$workshop->overallfeedbackfiletypes = workshop::clean_file_extensions($workshop->overallfeedbackfiletypes);
}

// todo - if the grading strategy is being changed, we may want to replace all aggregated peer grades with nulls

$DB->update_record('workshop', $workshop);
Expand Down

0 comments on commit 996f7e8

Please sign in to comment.