From 51c5e6057c67687f5d872f8a228cfea275abf576 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Wed, 28 Mar 2012 12:09:12 +0100 Subject: [PATCH] MDL-32239 question bank: wrong cap checks editing/viewing quesitions None of these problems affect the default roles. They only occur when the permissions have been edited to allow restricted subsets of the question capabilities. In some cases, things that the restriced subset of capabilities should have allowed were blocked by errors. In other cases, users were allowed to do more than they should bave been due to failures to check the right capabilities in the right places. For full details, see the bug report. Two of the bugs were previously reported separately as MDL-26395 and MDL-27232. --- question/editlib.php | 8 ++++---- question/question.php | 28 +++++++++++++++++++--------- question/type/edit_question_form.php | 8 +++----- 3 files changed, 26 insertions(+), 18 deletions(-) diff --git a/question/editlib.php b/question/editlib.php index 06ba39fbda3..0a4a64b2e6b 100644 --- a/question/editlib.php +++ b/question/editlib.php @@ -605,7 +605,8 @@ abstract class question_bank_action_column_base extends question_bank_column_bas } public function get_required_fields() { - return array('q.id'); + // createdby is required for permission checks. + return array('q.id, q.createdby'); } } @@ -631,10 +632,9 @@ class question_bank_edit_action_column extends question_bank_action_column_base } protected function display_content($question, $rowclasses) { - if (question_has_capability_on($question, 'edit') || - question_has_capability_on($question, 'move')) { + if (question_has_capability_on($question, 'edit')) { $this->print_icon('t/edit', $this->stredit, $this->qbank->edit_question_url($question->id)); - } else { + } else if (question_has_capability_on($question, 'view')) { $this->print_icon('i/info', $this->strview, $this->qbank->edit_question_url($question->id)); } } diff --git a/question/question.php b/question/question.php index 896a3d1739b..fecc772f94c 100644 --- a/question/question.php +++ b/question/question.php @@ -126,6 +126,7 @@ if ($id) { $question = new stdClass(); $question->category = $categoryid; $question->qtype = $qtype; + $question->createdby = $USER->id; // Check that users are allowed to create this question type at the moment. if (!question_bank::qtype_enabled($qtype)) { @@ -157,7 +158,6 @@ $categorycontext = get_context_instance_by_id($category->contextid); $addpermission = has_capability('moodle/question:add', $categorycontext); if ($id) { - $canview = question_has_capability_on($question, 'view'); if ($movecontext){ $question->formoptions->canedit = false; $question->formoptions->canmove = (question_has_capability_on($question, 'move') && $contexts->have_cap('moodle/question:add')); @@ -165,26 +165,28 @@ if ($id) { $question->formoptions->repeatelements = false; $question->formoptions->movecontext = true; $formeditable = true; - question_require_capability_on($question, 'view'); + question_require_capability_on($question, 'move'); } else { $question->formoptions->canedit = question_has_capability_on($question, 'edit'); - $question->formoptions->canmove = (question_has_capability_on($question, 'move') && $addpermission); - $question->formoptions->cansaveasnew = (($canview ||question_has_capability_on($question, 'edit')) && $addpermission); - $question->formoptions->repeatelements = ($question->formoptions->canedit || $question->formoptions->cansaveasnew); + $question->formoptions->canmove = $addpermission && question_has_capability_on($question, 'move'); + $question->formoptions->cansaveasnew = $addpermission && + (question_has_capability_on($question, 'view') || $question->formoptions->canedit); + $question->formoptions->repeatelements = $question->formoptions->canedit || $question->formoptions->cansaveasnew; $formeditable = $question->formoptions->canedit || $question->formoptions->cansaveasnew || $question->formoptions->canmove; $question->formoptions->movecontext = false; - if (!$formeditable){ + if (!$formeditable) { question_require_capability_on($question, 'view'); } } } else { // creating a new question - require_capability('moodle/question:add', $categorycontext); - $formeditable = true; $question->formoptions->canedit = question_has_capability_on($question, 'edit'); $question->formoptions->canmove = (question_has_capability_on($question, 'move') && $addpermission); + $question->formoptions->cansaveasnew = false; $question->formoptions->repeatelements = true; $question->formoptions->movecontext = false; + $formeditable = true; + require_capability('moodle/question:add', $categorycontext); } // Validate the question type. @@ -245,7 +247,7 @@ if ($mform->is_cancelled()) { /// If we are moving a question, check we have permission to move it from /// whence it came. (Where we are moving to is validated by the form.) - list($newcatid) = explode(',', $fromform->category); + list($newcatid, $newcontextid) = explode(',', $fromform->category); if (!empty($question->id) && $newcatid != $question->category) { question_require_capability_on($question, 'move'); } @@ -261,6 +263,14 @@ if ($mform->is_cancelled()) { } else { // We are acutally saving the question. + if (!empty($question->id)) { + question_require_capability_on($question, 'edit'); + } else { + require_capability('moodle/question:add', get_context_instance_by_id($newcontextid)); + if (!empty($fromform->makecopy) && !$question->formoptions->cansaveasnew) { + print_error('nopermissions', '', '', 'edit'); + } + } $question = $qtypeobj->save_question($question, $fromform); if (!empty($CFG->usetags) && isset($fromform->tags)) { // A wizardpage from multipe pages questiontype like calculated may not diff --git a/question/type/edit_question_form.php b/question/type/edit_question_form.php index f01e2d5673b..68a9d0328ce 100644 --- a/question/type/edit_question_form.php +++ b/question/type/edit_question_form.php @@ -237,9 +237,7 @@ abstract class question_edit_form extends question_wizard_form { if ($this->question->formoptions->movecontext) { $buttonarray[] = $mform->createElement('submit', 'submitbutton', get_string('moveq', 'question')); - } else if ($this->question->formoptions->canedit || - $this->question->formoptions->canmove || - $this->question->formoptions->movecontext) { + } else if ($this->question->formoptions->canedit) { $buttonarray[] = $mform->createElement('submit', 'submitbutton', get_string('savechanges')); } @@ -641,10 +639,10 @@ abstract class question_edit_form extends question_wizard_form { public function validation($fromform, $files) { $errors = parent::validation($fromform, $files); - if (empty($fromform->makecopy) && isset($this->question->id) + if (empty($fromform['makecopy']) && isset($this->question->id) && ($this->question->formoptions->canedit || $this->question->formoptions->cansaveasnew) - && empty($fromform->usecurrentcat) && !$this->question->formoptions->canmove) { + && empty($fromform['usecurrentcat']) && !$this->question->formoptions->canmove) { $errors['currentgrp'] = get_string('nopermissionmove', 'question'); } return $errors;