Merge pull request #5 from ucl-isd/CTP-3559-Coursework-review-permiss…

…ions-context-level-usage CTP-3559 course mod permissions
ucl-isd · Aug 28, 2024 · f2dbc3d · f2dbc3d
2 parents 3024b55 + 882d41e
commit f2dbc3d
Show file tree

Hide file tree

Showing 9 changed files with 86 additions and 60 deletions.
diff --git a/actions/finalgrade.php b/actions/finalgrade.php
@@ -42,17 +42,17 @@
 // Determines whether the current user is the owner of the grade.
 $gradeowner = true;
 
-$course_module = get_coursemodule_from_id('coursework', $cmid, 0, false, MUST_EXIST);
-$course = $DB->get_record('course', array('id' => $course_module->course), '*', MUST_EXIST);
-require_login($course, false, $course_module);
+$coursemodule = get_coursemodule_from_id('coursework', $cmid, 0, false, MUST_EXIST);
+$course = $DB->get_record('course', array('id' => $coursemodule->course), '*', MUST_EXIST);
+require_login($course, false, $coursemodule);
 
-$coursework = mod_coursework\models\coursework::find($course_module->instance);
+$coursework = mod_coursework\models\coursework::find($coursemodule->instance);
 $submission = submission::find($submission_id);
 $teacherfeedback = $DB->get_record('coursework_feedbacks', array('id' => $feedbackid));
 
 // This is where stuff used to construct the dynamic form is fed in.
 
-// Can the user final grade in this course?
+// Can the user final grade for this course module?
 $canfinalgrade = has_capability('mod/coursework:addagreedgrade', $PAGE->context);
 
 // TODO shift into custom data and set via somewhere else.

diff --git a/actions/general_feedback.php b/actions/general_feedback.php
@@ -26,43 +26,42 @@
 
 global $CFG, $PAGE, $DB, $OUTPUT;
 
-$course_module_id = required_param('cmid', PARAM_INT);
-$id = optional_param('id', 0, PARAM_INT);
+$cmid = required_param('cmid', PARAM_INT);
 $ajax = optional_param('ajax', false, PARAM_BOOL);
 
-$coursework = $DB->get_record('coursework', array('id' => $id));
-$course = $DB->get_record('course', array('id' => $coursework->course));
+$cm = get_coursemodule_from_instance('coursework', $cmid);
+$course = $DB->get_record('course', array('id' => $cm->course));
+require_login($course, false, $cm);
 
-require_login($course);
+$coursework = $DB->get_record('coursework', array('id' => $cm->instance));
 
 if (!has_capability('mod/coursework:addinitialgrade', $PAGE->context)) {
-    print_error('Can\'t grade here - permission denied.');
-    die();
+    throw new \moodle_exception('access_denied', 'coursework');
 }
 
 $url = '/mod/coursework/actions/general_feedback.php';
-$link = new moodle_url($url, array('cmid' => $course_module_id, 'id' => $id));
+$link = new moodle_url($url, array('cmid' => $cmid));
 $PAGE->set_url($link);
 $title = get_string('generalfeedback', 'mod_coursework');
 $PAGE->set_title($title);
 
-$custom_data = new stdClass();
-$custom_data->ajax = $ajax;
-$custom_data->id = $id;
-$custom_data->cmid = $course_module_id;
+$customdata = new stdClass();
+$customdata->ajax = $ajax;
+$customdata->id = $coursework->id;
+$customdata->cmid = $cmid;
 
-$grading_form = new general_feedback_form(null, $custom_data);
+$gradingform = new general_feedback_form(null, $customdata);
 
-$returned_data = $grading_form->get_data();
+$returneddata = $gradingform->get_data();
 
-if ($grading_form->is_cancelled()) {
-    redirect(new moodle_url('/mod/coursework/view.php', array('id' => $course_module_id)));
-} else if ($returned_data) {
-    $grading_form->process_data($returned_data);
+if ($gradingform->is_cancelled()) {
+    redirect(new moodle_url('/mod/coursework/view.php', array('id' => $cmid)));
+} else if ($returneddata) {
+    $gradingform->process_data($returneddata);
     // TODO should not echo before header.
     echo 'General feedback updated..';
     if (!$ajax) {
-        redirect(new moodle_url('/mod/coursework/view.php', array('id' => $course_module_id)),
+        redirect(new moodle_url('/mod/coursework/view.php', array('id' => $cmid)),
                                 get_string('changessaved'));
     }
 } else {
@@ -72,9 +71,9 @@
         echo $OUTPUT->header();
         echo $OUTPUT->heading('General Feedback');
     }
-    $custom_data->feedbackcomment_editor['text'] = $coursework->feedbackcomment;
-    $grading_form->set_data($custom_data);
-    $grading_form->display();
+    $customdata->feedbackcomment_editor['text'] = $coursework->feedbackcomment;
+    $gradingform->set_data($customdata);
+    $gradingform->display();
 
     if (!$ajax) {
         echo $OUTPUT->footer();

diff --git a/classes/controllers/deadline_extensions_controller.php b/classes/controllers/deadline_extensions_controller.php
@@ -184,7 +184,10 @@ public function ajax_submit_mitigation($data_params) {
         $extended_deadline = false;
         $response = [];
         $this->coursework = coursework::find(['id' => $this->params['courseworkid']]);
-        require_login($this->coursework->course);
+        $cm = get_coursemodule_from_instance(
+            'coursework', $this->coursework->id, 0, false, MUST_EXIST
+        );
+        require_login($this->coursework->course, false, $cm);
         $params = $this->set_default_current_deadline();
         $ability = new ability(user::find($USER), $this->coursework);
         $errors = $this->validation($data_params);
@@ -278,7 +281,10 @@ public function ajax_edit_mitigation($data_params) {
         $response = [];
         if ($data_params['id'] > 0) {
             $this->coursework = coursework::find(['id' => $this->params['courseworkid']]);
-            require_login($this->coursework->course);
+            $cm = get_coursemodule_from_instance(
+                'coursework', $this->coursework->id, 0, false, MUST_EXIST
+            );
+            require_login($this->coursework->course, false, $cm);
 
             $ability = new ability(user::find($USER), $this->coursework);
             $deadline_extension = deadline_extension::find(['id' => $data_params['id']]);
@@ -332,7 +338,10 @@ public function ajax_new_mitigation($data_params) {
         global $USER, $DB;
         $response = [];
         $this->coursework = coursework::find(['id' => $this->params['courseworkid']]);
-        require_login($this->coursework->course);
+        $cm = get_coursemodule_from_instance(
+            'coursework', $this->coursework->id, 0, false, MUST_EXIST
+        );
+        require_login($this->coursework->course, false, $cm);
 
         $params = array(
             'allocatableid' => $this->params['allocatableid'],

diff --git a/classes/controllers/grading_controller.php b/classes/controllers/grading_controller.php
@@ -36,9 +36,9 @@ public function get_remain_rows_grading_table($options) {
         $coursework_record = $DB->get_record('coursework', array('id' => $options['courseworkid']), '*', MUST_EXIST);
         //$coursework = mod_coursework\models\coursework::find($coursework_record);
         $coursework = coursework::find($coursework_record, false);
-        require_login($coursework->course);
 
         $coursework->coursemodule = get_coursemodule_from_instance('coursework', $coursework->id, $coursework->course, false, MUST_EXIST);
+        require_login($coursework->course, false, $coursework->coursemodule);
         $grading_report = $coursework->renderable_grading_report_factory($options);
 
         $tablerows = $grading_report->get_table_rows_for_page();

diff --git a/classes/controllers/personal_deadlines_controller.php b/classes/controllers/personal_deadlines_controller.php
@@ -183,7 +183,10 @@ public function insert_update($time) {
             ];
         }
         $this->coursework = coursework::find(['id' => $this->params['courseworkid']]);
-        require_login($this->coursework->course);
+        $cm = get_coursemodule_from_instance(
+            'coursework', $this->coursework->id, 0, false, MUST_EXIST
+        );
+        require_login($this->coursework->course, false, $cm);
         $params = $this->set_default_current_deadline();
 
         $ability = new ability(user::find($USER), $this->coursework);

diff --git a/classes/export/csv/cells/assessorfeedback_cell.php b/classes/export/csv/cells/assessorfeedback_cell.php
@@ -77,13 +77,22 @@ public function get_header($stage) {
     public function validate_cell($value, $submissionid, $stage_identifier='', $uploadedgradecells  = []) {
         global $DB, $PAGE, $USER;
 
+        $modulecontext = $PAGE->context;
+        if ($modulecontext->contextlevel !== CONTEXT_MODULE) {
+            // CTP-3559 sometimes require_login() is being called without passing a module context.
+            // This means that $PAGE->context will be course context & capability checks may be wrong so check it here.
+            throw new \moodle_exception(
+                "accesserror", 'coursework', "", null,
+                "Invalid context level " . $modulecontext->contextlevel
+            );
+        }
         $agreedgradecap = array('mod/coursework:addagreedgrade', 'mod/coursework:editagreedgrade');
         $initialgradecap = array('mod/coursework:addinitialgrade', 'mod/coursework:editinitialgrade');
 
         $subdbrecord = $DB->get_record('coursework_submissions', array('id' => $submissionid));
         $submission = \mod_coursework\models\submission::find($subdbrecord);
-        if (has_any_capability($agreedgradecap, $PAGE->context) && has_any_capability($initialgradecap, $PAGE->context)
-            || has_capability('mod/coursework:administergrades', $PAGE->context)) {
+        if (has_any_capability($agreedgradecap, $modulecontext) && has_any_capability($initialgradecap, $modulecontext)
+            || has_capability('mod/coursework:administergrades', $modulecontext)) {
 
             // Is the submission in question ready to grade?
             if (!$submission->ready_to_grade()) return get_string('submissionnotreadytograde', 'coursework');
@@ -92,7 +101,7 @@ public function validate_cell($value, $submissionid, $stage_identifier='', $uplo
             if ($submission->get_state() >= submission::PUBLISHED)  return $submission->get_status_text();
 
             // If you have administer grades you can grade anything
-            if (has_capability('mod/coursework:administergrades', $PAGE->context)) return true;
+            if (has_capability('mod/coursework:administergrades', $modulecontext)) return true;
 
             // Has this submission been graded if yes then check if the current user graded it (only if allocation is not enabled).
             $feedback_params = array(
@@ -106,18 +115,18 @@ public function validate_cell($value, $submissionid, $stage_identifier='', $uplo
             //does a feedback exist for this stage
             if (!empty($feedback)) {
                 // This is a new feedback check it against the new ability checks
-                if (!has_capability('mod/coursework:administergrades', $PAGE->context) && !$ability->can('new', $feedback))   return get_string('nopermissiontoeditgrade', 'coursework');
+                if (!has_capability('mod/coursework:administergrades', $modulecontext) && !$ability->can('new', $feedback))   return get_string('nopermissiontoeditgrade', 'coursework');
 
             } else {
 
                 // This is a new feedback check it against the edit ability checks
-                if (!has_capability('mod/coursework:administergrades', $PAGE->context) && !$ability->can('edit', $feedback))   return get_string('nopermissiontoeditgrade', 'coursework');
+                if (!has_capability('mod/coursework:administergrades', $modulecontext) && !$ability->can('edit', $feedback))   return get_string('nopermissiontoeditgrade', 'coursework');
 
             }
 
             if (!$this->coursework->allocation_enabled() && !empty($feedback)) {
                // Was this user the one who last graded this submission if not then user cannot grade
-                if ($feedback->assessorid != $USER->id || !has_capability('mod/coursework:editinitialgrade', $PAGE->context) )
+                if ($feedback->assessorid != $USER->id || !has_capability('mod/coursework:editinitialgrade', $modulecontext) )
                     return get_string('nopermissiontogradesubmission', 'coursework');
 
             }
@@ -131,12 +140,12 @@ public function validate_cell($value, $submissionid, $stage_identifier='', $uplo
                     'stage_identifier' => $stage_identifier
                 );
 
-                if (!has_capability('mod/coursework:administergrades', $PAGE->context)
+                if (!has_capability('mod/coursework:administergrades', $modulecontext)
                     && !$DB->get_record('coursework_allocation_pairs', $allocation_params)) return get_string('nopermissiontogradesubmission', 'coursework');
             }
 
             // Check for coursework without allocations - with/without samplings
-            if (has_capability('mod/coursework:addinitialgrade', $PAGE->context) && !has_capability('mod/coursework:editinitialgrade', $PAGE->context)
+            if (has_capability('mod/coursework:addinitialgrade', $modulecontext) && !has_capability('mod/coursework:editinitialgrade', $modulecontext)
                 && $this->coursework->get_max_markers() > 1 && !$this->coursework->allocation_enabled()) {
 
                 // check how many feedbacks for this submission
@@ -153,7 +162,7 @@ public function validate_cell($value, $submissionid, $stage_identifier='', $uplo
                 if ($assessors == $feedbacks) return get_string('gradealreadyexists', 'coursework');
             }
 
-        } else if (has_any_capability($agreedgradecap, $PAGE->context)) {
+        } else if (has_any_capability($agreedgradecap, $modulecontext)) {
 
             // If you have the add agreed or edit agreed grades capabilities then you may have the grades on your export sheet
             // We will return true as we will ignore them

diff --git a/classes/stages/base.php b/classes/stages/base.php
@@ -299,19 +299,19 @@ public function get_teachers() {
         // There is a chance that when the teachers were initially cached the dataset was empty
         // So check again
         if (empty($serialised_teachers) || empty(unserialize($serialised_teachers))) {
-            $teachers = get_enrolled_users($this->coursework->get_context(), $this->assessor_capability());
+            $users = get_enrolled_users($this->coursework->get_context());
             $teacher_users = [];
-            foreach ($teachers as $teacher) {
-                $teacher_users[] = user::build($teacher);
+            $modcontext = $this->coursework->get_context();
+            foreach ($users as $user) {
+                if (has_capability($this->assessor_capability(), $modcontext, $user)) {
+                    $teacher_users[] = user::build($user);
+                }
             }
-
             $cache->set($this->coursework->id()."_teachers", serialize($teacher_users));
         } else {
             $teacher_users = unserialize($serialised_teachers);
         }
-
         return $teacher_users;
-
     }
 
     /**
@@ -563,9 +563,11 @@ public function remove_allocatable_from_sampling($allocatable) {
      */
     public function user_is_assessor($assessor) {
         if (!isset(self::$self_cache['user_is_assessor'][$this->coursework->id][$assessor->id])) {
-            $enrolled = is_enrolled($this->coursework->get_course_context(), $assessor, $this->assessor_capability());
-            $res = $enrolled || is_primary_admin($assessor->id);
-            self::$self_cache['user_is_assessor'][$this->coursework->id][$assessor->id] = $res;
+            $enrolled = is_enrolled($this->coursework->get_course_context(), $assessor);
+            $hasmoduleassessorcapability =
+                ($enrolled && has_capability($this->assessor_capability(), $this->coursework->get_context(), $assessor))
+                || is_primary_admin($assessor->id);
+            self::$self_cache['user_is_assessor'][$this->coursework->id][$assessor->id] = $hasmoduleassessorcapability;
         }
         return self::$self_cache['user_is_assessor'][$this->coursework->id][$assessor->id];
     }
@@ -954,14 +956,18 @@ public function get_assessor_from_moodle_course_group($allocatable) {
 
         if ($groupid) {
             // find 1st assessor in the group
-            $first_group_assessor = get_enrolled_users($this->coursework->get_context(), $this->assessor_capability(),
-                $groupid, 'u.*', 'id ASC', 0, 1);
-
-            $assessor = array_column($first_group_assessor, 'id');
-
-            if ($assessor) {
-                $assessorid = $assessor[0];
-                $assessor = user::get_object($assessorid);
+            $modcontext = $this->coursework->get_context();
+            $users = get_enrolled_users($modcontext, '', $groupid, 'u.*', 'id ASC');
+
+            foreach ($users as $user) {
+                if (has_capability($this->assessor_capability(), $modcontext, $user)) {
+                    $assessor = array_column($user, 'id');
+                    if ($assessor) {
+                        $assessorid = $assessor[0];
+                        $assessor = user::get_object($assessorid);
+                        break;
+                    }
+                }
             }
         }
 

diff --git a/lang/en/coursework.php b/lang/en/coursework.php
@@ -27,6 +27,7 @@
 global $CFG;
 
 $string['access_denied'] = 'Access denied';
+$string['accesserror'] = 'Access error';
 $string['all'] = 'ALL';
 
 $string['aday'] = 'A day';

diff --git a/renderers/object_renderer.php b/renderers/object_renderer.php
@@ -536,8 +536,7 @@ protected function render_mod_coursework_coursework(mod_coursework_coursework $c
                 $class = ($coursework->feedbackcomment) ? 'edit-btn' : 'add-general_feedback-btn';
                 $out .= html_writer::tag('p', '', array('id' => 'feedback_text'));
                 $link = new moodle_url('/mod/coursework/actions/general_feedback.php',
-                                       array('cmid' => $coursework->get_coursemodule_id(),
-                                             'id' => $coursework->id));
+                                       array('cmid' => $coursework->get_coursemodule_id()));
                 $out .= html_writer::link($link,
                                           $title,
                                           array('class' => $class));