diff --git a/comment/classes/external.php b/comment/classes/external.php index 41a8b4108bc..1eb3d8e07e1 100644 --- a/comment/classes/external.php +++ b/comment/classes/external.php @@ -337,7 +337,7 @@ class core_comment_external extends external_api { $args->area = $commentrecord->commentarea; $manager = new comment($args); - if ($commentrecord->userid != $USER->id && !$manager->can_delete($commentrecord->id)) { + if (!$manager->can_delete($commentrecord)) { throw new comment_exception('nopermissiontodelentry'); } diff --git a/comment/comment_ajax.php b/comment/comment_ajax.php index c1fe37c00bd..50caa94be42 100644 --- a/comment/comment_ajax.php +++ b/comment/comment_ajax.php @@ -91,8 +91,8 @@ switch ($action) { } break; case 'delete': - $comment_record = $DB->get_record('comments', array('id'=>$commentid)); - if ($manager->can_delete($commentid) || $comment_record->userid == $USER->id) { + $comment = $DB->get_record('comments', ['id' => $commentid]); + if ($manager->can_delete($comment)) { if ($manager->delete($commentid)) { $result = array( 'client_id' => $client_id, diff --git a/comment/lib.php b/comment/lib.php index 58f8151d380..65d81e30cbd 100644 --- a/comment/lib.php +++ b/comment/lib.php @@ -589,8 +589,7 @@ class comment { $c->avatar = $OUTPUT->user_picture($u, array('size'=>18)); $c->userid = $u->id; - $candelete = $this->can_delete($c->id); - if (($USER->id == $u->id) || !empty($candelete)) { + if ($this->can_delete($c)) { $c->delete = true; } $comments[] = $c; @@ -800,16 +799,22 @@ class comment { /** * Delete a comment * - * @param int $commentid + * @param int|stdClass $comment The id of a comment, or a comment record. * @return bool */ - public function delete($commentid) { - global $DB, $USER; - $candelete = has_capability('moodle/comment:delete', $this->context); - if (!$comment = $DB->get_record('comments', array('id'=>$commentid))) { + public function delete($comment) { + global $DB; + if (is_object($comment)) { + $commentid = $comment->id; + } else { + $commentid = $comment; + $comment = $DB->get_record('comments', ['id' => $commentid]); + } + + if (!$comment) { throw new comment_exception('dbupdatefailed'); } - if (!($USER->id == $comment->userid || !empty($candelete))) { + if (!$this->can_delete($comment)) { throw new comment_exception('nopermissiontocomment'); } $DB->delete_records('comments', array('id'=>$commentid)); @@ -976,13 +981,35 @@ class comment { } /** - * Returns true if the user can delete this comment - * @param int $commentid + * Returns true if the user can delete this comment. + * + * The user can delete comments if it is one they posted and they can still make posts, + * or they have the capability to delete comments. + * + * A database call is avoided if a comment record is passed. + * + * @param int|stdClass $comment The id of a comment, or a comment record. * @return bool */ - public function can_delete($commentid) { + public function can_delete($comment) { + global $USER, $DB; + if (is_object($comment)) { + $commentid = $comment->id; + } else { + $commentid = $comment; + } + $this->validate(array('commentid'=>$commentid)); - return has_capability('moodle/comment:delete', $this->context); + + if (!is_object($comment)) { + // Get the comment record from the database. + $comment = $DB->get_record('comments', array('id' => $commentid), 'id, userid', MUST_EXIST); + } + + $hascapability = has_capability('moodle/comment:delete', $this->context); + $owncomment = $USER->id == $comment->userid; + + return ($hascapability || ($owncomment && $this->can_post())); } /** diff --git a/comment/tests/context_freeze_test.php b/comment/tests/context_freeze_test.php new file mode 100644 index 00000000000..36f1d600bca --- /dev/null +++ b/comment/tests/context_freeze_test.php @@ -0,0 +1,166 @@ +. + +/** + * Tests for comments when the context is frozen. + * + * @package core_comment + * @copyright 2019 University of Nottingham + * @author Neill Magill + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +defined('MOODLE_INTERNAL') || die(); + +/** + * Tests for comments when the context is frozen. + * + * @package core_comment + * @copyright 2019 University of Nottingham + * @author Neill Magill + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class comment_context_freeze_testcase extends advanced_testcase { + /** + * Creates a comment by a student. + * + * Returns: + * - The comment object + * - The sudent that wrote the comment + * - The arguments used to create the comment + * + * @param stdClass $course Moodle course from the datagenerator + * @return array + */ + protected function create_student_comment_and_freeze_course($course): array { + set_config('contextlocking', 1); + + $context = context_course::instance($course->id); + $student = $this->getDataGenerator()->create_and_enrol($course, 'student'); + + $args = new stdClass; + $args->context = $context; + $args->course = $course; + $args->area = 'page_comments'; + $args->itemid = 0; + $args->component = 'block_comments'; + $args->linktext = get_string('showcomments'); + $args->notoggle = true; + $args->autostart = true; + $args->displaycancel = false; + + // Create a comment by the student. + $this->setUser($student); + $comment = new comment($args); + $newcomment = $comment->add('New comment'); + + // Freeze the context. + $this->setAdminUser(); + $context->set_locked(true); + + return [$newcomment, $student, $args]; + } + + /** + * Test that a student cannot delete their own comments in frozen contexts via the external service. + */ + public function test_delete_student_external() { + global $CFG; + require_once($CFG->dirroot . '/comment/lib.php'); + + $this->resetAfterTest(); + + $course = $this->getDataGenerator()->create_course(); + + list($newcomment, $student, $args) = $this->create_student_comment_and_freeze_course($course); + + // Check that a student cannot delete their own comment. + $this->setUser($student); + $studentcomment = new comment($args); + $this->assertFalse($studentcomment->can_delete($newcomment->id)); + $this->assertFalse($studentcomment->can_post()); + $this->expectException(comment_exception::class); + $this->expectExceptionMessage(get_string('nopermissiontodelentry', 'error')); + core_comment_external::delete_comments([$newcomment->id]); + } + + /** + * Test that a student cannot delete their own comments in frozen contexts. + */ + public function test_delete_student() { + global $CFG; + require_once($CFG->dirroot . '/comment/lib.php'); + + $this->resetAfterTest(); + + $course = $this->getDataGenerator()->create_course(); + + list($newcomment, $student, $args) = $this->create_student_comment_and_freeze_course($course); + + // Check that a student cannot delete their own comment. + $this->setUser($student); + $studentcomment = new comment($args); + $this->assertFalse($studentcomment->can_delete($newcomment->id)); + $this->assertFalse($studentcomment->can_post()); + $this->expectException(comment_exception::class); + $this->expectExceptionMessage(get_string('nopermissiontocomment', 'error')); + $studentcomment->delete($newcomment->id); + } + + /** + * Test that an admin cannot delete comments in frozen contexts via the external service. + */ + public function test_delete_admin_external() { + global $CFG; + require_once($CFG->dirroot . '/comment/lib.php'); + + $this->resetAfterTest(); + + $course = $this->getDataGenerator()->create_course(); + + list($newcomment, $student, $args) = $this->create_student_comment_and_freeze_course($course); + + // Check that the admin user cannot delete the comment. + $admincomment = new comment($args); + $this->assertFalse($admincomment->can_delete($newcomment->id)); + $this->assertFalse($admincomment->can_post()); + $this->expectException(comment_exception::class); + $this->expectExceptionMessage(get_string('nopermissiontodelentry', 'error')); + core_comment_external::delete_comments([$newcomment->id]); + } + + /** + * Test that an admin cannot delete comments in frozen contexts. + */ + public function test_delete_admin() { + global $CFG; + require_once($CFG->dirroot . '/comment/lib.php'); + + $this->resetAfterTest(); + + $course = $this->getDataGenerator()->create_course(); + + list($newcomment, $student, $args) = $this->create_student_comment_and_freeze_course($course); + + // Check that the admin user cannot delete the comment. + $admincomment = new comment($args); + $this->assertFalse($admincomment->can_delete($newcomment->id)); + $this->assertFalse($admincomment->can_post()); + $this->expectException(comment_exception::class); + $this->expectExceptionMessage(get_string('nopermissiontocomment', 'error')); + $admincomment->delete($newcomment->id); + } +} diff --git a/lang/en/error.php b/lang/en/error.php index 6a728796b47..828a2f52cc5 100644 --- a/lang/en/error.php +++ b/lang/en/error.php @@ -440,7 +440,7 @@ $string['noparticipants'] = 'No participants found for this course'; $string['noparticipatorycms'] = 'Sorry, but you have no participatory course modules to report on'; $string['nopermissions'] = 'Sorry, but you do not currently have permissions to do that ({$a}).'; $string['nopermissiontocomment'] = 'You can\'t add comments'; -$string['nopermissiontodelentry'] = 'You can\'t delete other people\'s entries!'; +$string['nopermissiontodelentry'] = 'You can\'t delete this comment!'; $string['nopermissiontoeditcomment'] = 'You can\'t edit other people\'s comments!'; $string['nopermissiontohide'] = 'No permission to hide!'; $string['nopermissiontoimportact'] = 'You do not have the required permissions to import activities to this course';