MDL-32239 question bank: wrong cap checks editing/viewing quesitions
authorTim Hunt <T.J.Hunt@open.ac.uk>
Wed, 28 Mar 2012 11:09:12 +0000 (12:09 +0100)
committerTim Hunt <T.J.Hunt@open.ac.uk>
Thu, 29 Mar 2012 13:14:10 +0000 (14:14 +0100)
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
question/question.php
question/type/edit_question_form.php

index 06ba39f..0a4a64b 100644 (file)
@@ -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));
         }
     }
index 896a3d1..fecc772 100644 (file)
@@ -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
index f01e2d5..68a9d03 100644 (file)
@@ -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;