Permalink
Browse files

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.
  • Loading branch information...
1 parent c54172b commit 0f83dd10a1d013e77906c7be4560126bb14c6b5c @timhunt timhunt committed Mar 28, 2012
Showing with 26 additions and 18 deletions.
  1. +4 −4 question/editlib.php
  2. +19 −9 question/question.php
  3. +3 −5 question/type/edit_question_form.php
View
@@ -605,7 +605,8 @@ protected function print_icon($icon, $title, $url) {
}
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 @@ public function get_name() {
}
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));
}
}
View
@@ -126,6 +126,7 @@
$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,34 +158,35 @@
$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'));
$question->formoptions->cansaveasnew = false;
$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 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 @@
} 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
@@ -237,9 +237,7 @@ protected function definition() {
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'));
}
@@ -639,10 +637,10 @@ protected function data_preprocessing_hints($question, $withclearwrong = false,
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;

0 comments on commit 0f83dd1

Please sign in to comment.