Skip to content

Commit

Permalink
MDL-77230 mod_feedback: Validate feedback access
Browse files Browse the repository at this point in the history 
The get_items() and get_page_items() external methods should return
items only when the user has access. Otherwise, empty array for items
will be returned, with the exact error in the warnings parameter.
  • Loading branch information
@sarjona
sarjona committed Feb 17, 2023
1 parent cb2fa7a commit a47f28e
Show file tree
Hide file tree
Showing 3 changed files with 279 additions and 20 deletions.
104 changes: 84 additions & 20 deletions mod/feedback/classes/external.php
View file
Expand Up @@ -452,19 +452,48 @@ public static function get_items($feedbackid, $courseid = 0) {
$params = array('feedbackid' => $feedbackid, 'courseid' => $courseid);
$params = self::validate_parameters(self::get_items_parameters(), $params);
$warnings = array();
$returneditems = array();

list($feedback, $course, $cm, $context, $completioncourse) = self::validate_feedback($params['feedbackid'],
$params['courseid']);

$feedbackstructure = new mod_feedback_structure($feedback, $cm, $completioncourse->id);
$returneditems = array();
if ($items = $feedbackstructure->get_items()) {
foreach ($items as $item) {
$itemnumber = empty($item->itemnr) ? null : $item->itemnr;
unset($item->itemnr); // Added by the function, not part of the record.
$exporter = new feedback_item_exporter($item, array('context' => $context, 'itemnumber' => $itemnumber));
$returneditems[] = $exporter->export($PAGE->get_renderer('core'));
$userhasaccess = true;
try {
// Check the user has access to the feedback.
self::validate_feedback_access($feedback, $completioncourse, $cm, $context, true);
} catch (moodle_exception $e) {
$userhasaccess = false;
$warnings[] = [
'item' => $feedback->id,
'warningcode' => clean_param($e->errorcode, PARAM_ALPHANUM),
'message' => $e->getMessage(),
];
}

// For consistency with the web behaviour, the items should be returned only when the user can edit or view reports (to
// include non-editing teachers too).
$capabilities = [
'mod/feedback:edititems',
'mod/feedback:viewreports',
];
if ($userhasaccess || has_any_capability($capabilities, $context)) {
// Remove previous warnings because, although the user might not have access, they have the proper capability.
$warnings = [];
$feedbackstructure = new mod_feedback_structure($feedback, $cm, $completioncourse->id);
if ($items = $feedbackstructure->get_items()) {
foreach ($items as $item) {
$itemnumber = empty($item->itemnr) ? null : $item->itemnr;
unset($item->itemnr); // Added by the function, not part of the record.
$exporter = new feedback_item_exporter($item, array('context' => $context, 'itemnumber' => $itemnumber));
$returneditems[] = $exporter->export($PAGE->get_renderer('core'));
}
}
} else if ($userhasaccess) {
$warnings[] = [
'item' => $feedback->id,
'warningcode' => 'nopermission',
'message' => 'nopermission',
];
}

$result = array(
Expand Down Expand Up @@ -586,24 +615,59 @@ public static function get_page_items($feedbackid, $page, $courseid = 0) {
$params = array('feedbackid' => $feedbackid, 'page' => $page, 'courseid' => $courseid);
$params = self::validate_parameters(self::get_page_items_parameters(), $params);
$warnings = array();
$returneditems = array();
$hasprevpage = false;
$hasnextpage = false;

list($feedback, $course, $cm, $context, $completioncourse) = self::validate_feedback($params['feedbackid'],
$params['courseid']);

$feedbackcompletion = new mod_feedback_completion($feedback, $cm, $completioncourse->id);
$userhasaccess = true;
$feedbackcompletion = null;
try {
// Check the user has access to the feedback.
$feedbackcompletion = self::validate_feedback_access($feedback, $completioncourse, $cm, $context, true);
} catch (moodle_exception $e) {
$userhasaccess = false;
$warnings[] = [
'item' => $feedback->id,
'warningcode' => str_replace('_', '', $e->errorcode),
'message' => $e->getMessage(),
];
}

$page = $params['page'];
$pages = $feedbackcompletion->get_pages();
$pageitems = $pages[$page];
$hasnextpage = $page < count($pages) - 1; // Until we complete this page we can not trust get_next_page().
$hasprevpage = $page && ($feedbackcompletion->get_previous_page($page, false) !== null);
// For consistency with the web behaviour, the items should be returned only when the user can edit or view reports (to
// include non-editing teachers too).
$capabilities = [
'mod/feedback:edititems',
'mod/feedback:viewreports',
];
if ($userhasaccess || has_any_capability($capabilities, $context)) {
// Remove previous warnings because, although the user might not have access, they have the proper capability.
$warnings = [];

if ($feedbackcompletion == null) {
$feedbackcompletion = new mod_feedback_completion($feedback, $cm, $completioncourse->id);
}

$returneditems = array();
foreach ($pageitems as $item) {
$itemnumber = empty($item->itemnr) ? null : $item->itemnr;
unset($item->itemnr); // Added by the function, not part of the record.
$exporter = new feedback_item_exporter($item, array('context' => $context, 'itemnumber' => $itemnumber));
$returneditems[] = $exporter->export($PAGE->get_renderer('core'));
$page = $params['page'];
$pages = $feedbackcompletion->get_pages();
$pageitems = $pages[$page];
$hasnextpage = $page < count($pages) - 1; // Until we complete this page we can not trust get_next_page().
$hasprevpage = $page && ($feedbackcompletion->get_previous_page($page, false) !== null);

foreach ($pageitems as $item) {
$itemnumber = empty($item->itemnr) ? null : $item->itemnr;
unset($item->itemnr); // Added by the function, not part of the record.
$exporter = new feedback_item_exporter($item, array('context' => $context, 'itemnumber' => $itemnumber));
$returneditems[] = $exporter->export($PAGE->get_renderer('core'));
}
} else if ($userhasaccess) {
$warnings[] = [
'item' => $feedback->id,
'warningcode' => 'nopermission',
'message' => get_string('nopermission', 'mod_feedback'),
];
}

$result = array(
Expand Down
191 changes: 191 additions & 0 deletions mod/feedback/tests/external/external_test.php
View file
Expand Up @@ -29,6 +29,8 @@
use externallib_advanced_testcase;
use feedback_item_multichoice;
use mod_feedback_external;
use moodle_exception;
use required_capability_exception;

defined('MOODLE_INTERNAL') || die();

Expand All @@ -45,6 +47,7 @@
* @copyright 2017 Juan Leyva <juan@moodle.com>
* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
* @since Moodle 3.3
* @covers \mod_feedback_external
*/
class external_test extends externallib_advanced_testcase {

Expand Down Expand Up @@ -373,6 +376,130 @@ public function test_get_items() {
}
}

/**
* Test get_items, to confirm validation is done too.
*
* @dataProvider items_provider
* @param string $role Whether the current user should be a student or a teacher.
* @param array $info Settings to create the feedback.
* @param string|null $warning The warning message to display or null if warnings result is empty.
*/
public function test_get_items_validation(string $role, array $info, ?string $warning): void {
global $DB;

// Test user with full capabilities.
if ($role === 'teacher') {
$this->setUser($this->teacher);
} else {
$this->setUser($this->student);
}

// Create the feedback.
$data = ['course' => $this->course->id];
if (array_key_exists('closed', $info) && $info['closed']) {
$data['timeopen'] = time() + DAYSECS;
}
$feedback = $this->getDataGenerator()->create_module('feedback', $data);

$empty = true;
if (!array_key_exists('empty', $info) || !$info['empty']) {
$empty = false;
/** @var \mod_feedback_generator $feedbackgenerator */
$feedbackgenerator = $this->getDataGenerator()->get_plugin_generator('mod_feedback');
// Add,at least, one item to the feedback.
$feedbackgenerator->create_item_label($feedback);
}

if (array_key_exists('complete', $info) && !$info['complete']) {
$studentrole = $DB->get_record('role', array('shortname' => 'student'));
$coursecontext = \context_course::instance($this->course->id);
assign_capability('mod/feedback:complete', CAP_PROHIBIT, $studentrole->id, $coursecontext->id);
// Empty all the caches that may be affected by this change.
accesslib_clear_all_caches_for_unit_testing();
\course_modinfo::clear_instance_cache();
}

$result = mod_feedback_external::get_items($feedback->id);
$result = \external_api::clean_returnvalue(mod_feedback_external::get_items_returns(), $result);
if ($warning) {
$this->assertEmpty($result['items']);
$this->assertCount(1, $result['warnings']);
$resultwarning = reset($result['warnings']);
if ($warning == 'required_capability_exception') {
$this->assertStringContainsString('error/nopermission', $resultwarning['message']);
} else {
$this->assertEquals($warning, $resultwarning['message']);
}
} else {
if ($empty) {
$this->assertEmpty($result['items']);
} else {
$this->assertCount(1, $result['items']);
}
$this->assertEmpty($result['warnings']);
}
}

/**
* Data provider for test_get_items_validation() and test_get_page_items_validation().
*
* @return array
*/
public function items_provider(): array {
return [
'Valid feedback (as student)' => [
'role' => 'student',
'info' => [],
'warning' => null,
],
'Closed feedback (as student)' => [
'role' => 'student',
'info' => ['closed' => true],
'warning' => get_string('feedback_is_not_open', 'feedback'),
],
'Empty feedback (as student)' => [
'role' => 'student',
'info' => ['empty' => true],
'warning' => get_string('no_items_available_yet', 'feedback'),
],
'Closed feedback (as student)' => [
'role' => 'student',
'info' => ['closed' => true],
'warning' => get_string('feedback_is_not_open', 'feedback'),
],
'Cannot complete feedback (as student)' => [
'role' => 'student',
'info' => ['complete' => false],
'warning' => 'required_capability_exception',
],
'Valid feedback (as teacher)' => [
'role' => 'teacher',
'info' => [],
'warning' => null,
],
'Closed feedback (as teacher)' => [
'role' => 'teacher',
'info' => ['closed' => true],
'warning' => null,
],
'Empty feedback (as teacher)' => [
'role' => 'teacher',
'info' => ['empty' => true],
'warning' => null,
],
'Closed feedback (as teacher)' => [
'role' => 'teacher',
'info' => ['closed' => true],
'warning' => null,
],
'Cannot complete feedback (as teacher)' => [
'role' => 'teacher',
'info' => ['complete' => false],
'warning' => null,
],
];
}

/**
* Test launch_feedback.
*/
Expand Down Expand Up @@ -453,6 +580,70 @@ public function test_get_page_items() {
$this->assertTrue($result['hasprevpage']);
}

/**
* Test get_page_items, to confirm validation is done too.
*
* @dataProvider items_provider
* @param string $role Whether the current user should be a student or a teacher.
* @param array $info Settings to create the feedback.
* @param string|null $warning The warning message to display or null if warnings result is empty.
*/
public function test_get_page_items_validation(string $role, array $info, ?string $warning): void {
global $DB;

// Test user with full capabilities.
if ($role === 'teacher') {
$this->setUser($this->teacher);
} else {
$this->setUser($this->student);
}

// Create the feedback.
$data = ['course' => $this->course->id];
if (array_key_exists('closed', $info) && $info['closed']) {
$data['timeopen'] = time() + DAYSECS;
}
$feedback = $this->getDataGenerator()->create_module('feedback', $data);

$empty = true;
if (!array_key_exists('empty', $info) || !$info['empty']) {
$empty = false;
/** @var \mod_feedback_generator $feedbackgenerator */
$feedbackgenerator = $this->getDataGenerator()->get_plugin_generator('mod_feedback');
// Add,at least, one item to the feedback.
$feedbackgenerator->create_item_label($feedback);
}

if (array_key_exists('complete', $info) && !$info['complete']) {
$studentrole = $DB->get_record('role', array('shortname' => 'student'));
$coursecontext = \context_course::instance($this->course->id);
assign_capability('mod/feedback:complete', CAP_PROHIBIT, $studentrole->id, $coursecontext->id);
// Empty all the caches that may be affected by this change.
accesslib_clear_all_caches_for_unit_testing();
\course_modinfo::clear_instance_cache();
}

$result = mod_feedback_external::get_page_items($feedback->id, 0);
$result = \external_api::clean_returnvalue(mod_feedback_external::get_items_returns(), $result);
if ($warning) {
$this->assertEmpty($result['items']);
$this->assertCount(1, $result['warnings']);
$resultwarning = reset($result['warnings']);
if ($warning == 'required_capability_exception') {
$this->assertStringContainsString('error/nopermission', $resultwarning['message']);
} else {
$this->assertEquals($warning, $resultwarning['message']);
}
} else {
if ($empty) {
$this->assertEmpty($result['items']);
} else {
$this->assertCount(1, $result['items']);
}
$this->assertEmpty($result['warnings']);
}
}

/**
* Test process_page.
*/
Expand Down
4 changes: 4 additions & 0 deletions mod/feedback/upgrade.txt
View file
@@ -1,3 +1,7 @@
=== 4.0.7 ===
* The external methods get_items() and get_page_items() now only return the items when the user can access the feedback; if the
user can't see them, a warning message will be returned, with the reasons.

=== 4.0 ===
* The following files and classes within them have been deprecated in favour of dynamic forms:
* edit_form.php
Expand Down

0 comments on commit a47f28e

Please sign in to comment.