From c1ebec7cbc280ca795214cafe34bb82e3a1d2354 Mon Sep 17 00:00:00 2001 From: Ilya Tregubov Date: Fri, 25 Feb 2022 10:10:43 +0200 Subject: [PATCH] MDL-73981 tool_uploadcourse: Validate enrolment role from csv. --- admin/tool/uploadcourse/classes/course.php | 146 +++++++++++++----- admin/tool/uploadcourse/classes/helper.php | 5 + admin/tool/uploadcourse/classes/processor.php | 4 + .../uploadcourse/tests/behat/create.feature | 25 +++ admin/tool/uploadcourse/tests/course_test.php | 88 +++++++++++ .../tests/fixtures/enrolment_role.csv | 4 + lang/en/role.php | 1 + 7 files changed, 235 insertions(+), 38 deletions(-) create mode 100644 admin/tool/uploadcourse/tests/fixtures/enrolment_role.csv diff --git a/admin/tool/uploadcourse/classes/course.php b/admin/tool/uploadcourse/classes/course.php index 6d6433c44eb..dc648df8216 100644 --- a/admin/tool/uploadcourse/classes/course.php +++ b/admin/tool/uploadcourse/classes/course.php @@ -44,6 +44,12 @@ class tool_uploadcourse_course { /** Outcome of the process: deleting the course */ const DO_DELETE = 3; + /** @var array assignable roles. */ + protected $assignableroles = []; + + /** @var array Roles context levels. */ + protected $contextlevels = []; + /** @var array final import data. */ protected $data = array(); @@ -794,16 +800,15 @@ class tool_uploadcourse_course { // Get enrolment data. Where the course already exists, we can also perform validation. $this->enrolmentdata = tool_uploadcourse_helper::get_enrolment_data($this->rawdata); - if ($exists) { - $errors = $this->validate_enrolment_data($coursedata['id'], $this->enrolmentdata); + $courseid = $coursedata['id'] ?? 0; + $errors = $this->validate_enrolment_data($courseid, $this->enrolmentdata); - if (!empty($errors)) { - foreach ($errors as $key => $message) { - $this->error($key, $message); - } - - return false; + if (!empty($errors)) { + foreach ($errors as $key => $message) { + $this->error($key, $message); } + + return false; } if (isset($this->rawdata['tags']) && strval($this->rawdata['tags']) !== '') { @@ -913,6 +918,8 @@ class tool_uploadcourse_course { * @return lang_string[] Errors keyed on error code */ protected function validate_enrolment_data(int $courseid, array $enrolmentdata): array { + global $DB; + // Nothing to validate. if (empty($enrolmentdata)) { return []; @@ -924,46 +931,67 @@ class tool_uploadcourse_course { $instances = enrol_get_instances($courseid, false); foreach ($enrolmentdata as $method => $options) { - $plugin = $enrolmentplugins[$method]; - // Find matching instances by enrolment method. - $methodinstances = array_filter($instances, static function(stdClass $instance) use ($method) { - return (strcmp($instance->enrol, $method) == 0); - }); + if (isset($options['role'])) { + $role = $options['role']; + if ($courseid) { + if (!$this->validate_role_context($courseid, $role)) { + $errors['contextrolenotallowed'] = new lang_string('contextrolenotallowed', 'core_role', $role); - if (!empty($options['delete'])) { - // Ensure user is able to delete the instances. - foreach ($methodinstances as $methodinstance) { - if (!$plugin->can_delete_instance($methodinstance)) { - $errors['errorcannotdeleteenrolment'] = new lang_string('errorcannotdeleteenrolment', 'tool_uploadcourse', - $plugin->get_instance_name($methodinstance)); + break; + } + } else { + // We can at least check that context level is correct while actual context not exist. + $roleid = $DB->get_field('role', 'id', ['shortname' => $role], MUST_EXIST); + if (!$this->validate_role_context_level($roleid)) { + $errors['contextrolenotallowed'] = new lang_string('contextrolenotallowed', 'core_role', $role); break; } } - } else if (!empty($options['disable'])) { - // Ensure user is able to toggle instance statuses. - foreach ($methodinstances as $methodinstance) { - if (!$plugin->can_hide_show_instance($methodinstance)) { - $errors['errorcannotdisableenrolment'] = - new lang_string('errorcannotdisableenrolment', 'tool_uploadcourse', + } + + if ($courseid) { + $plugin = $enrolmentplugins[$method]; + + // Find matching instances by enrolment method. + $methodinstances = array_filter($instances, static function (stdClass $instance) use ($method) { + return (strcmp($instance->enrol, $method) == 0); + }); + + if (!empty($options['delete'])) { + // Ensure user is able to delete the instances. + foreach ($methodinstances as $methodinstance) { + if (!$plugin->can_delete_instance($methodinstance)) { + $errors['errorcannotdeleteenrolment'] = new lang_string('errorcannotdeleteenrolment', + 'tool_uploadcourse', $plugin->get_instance_name($methodinstance)); + break; + } + } + } else if (!empty($options['disable'])) { + // Ensure user is able to toggle instance statuses. + foreach ($methodinstances as $methodinstance) { + if (!$plugin->can_hide_show_instance($methodinstance)) { + $errors['errorcannotdisableenrolment'] = + new lang_string('errorcannotdisableenrolment', 'tool_uploadcourse', + $plugin->get_instance_name($methodinstance)); + + break; + } + } + } else { + // Ensure user is able to create/update instance. + $methodinstance = empty($methodinstances) ? null : reset($methodinstances); + if ((empty($methodinstance) && !$plugin->can_add_instance($courseid)) || + (!empty($methodinstance) && !$plugin->can_edit_instance($methodinstance))) { + + $errors['errorcannotcreateorupdateenrolment'] = + new lang_string('errorcannotcreateorupdateenrolment', 'tool_uploadcourse', $plugin->get_instance_name($methodinstance)); break; } } - } else { - // Ensure user is able to create/update instance. - $methodinstance = empty($methodinstances) ? null : reset($methodinstances); - if ((empty($methodinstance) && !$plugin->can_add_instance($courseid)) || - (!empty($methodinstance) && !$plugin->can_edit_instance($methodinstance))) { - - $errors['errorcannotcreateorupdateenrolment'] = - new lang_string('errorcannotcreateorupdateenrolment', 'tool_uploadcourse', - $plugin->get_instance_name($methodinstance)); - - break; - } } } @@ -1079,8 +1107,15 @@ class tool_uploadcourse_course { $instance->enrolenddate = $instance->enrolstartdate; } - // Sort out the given role. This does not filter the roles allowed in the course. + // Sort out the given role. if (isset($method['role'])) { + $role = $method['role']; + if (!$this->validate_role_context($course->id, $role)) { + $this->error('contextrolenotallowed', + new lang_string('contextrolenotallowed', 'core_role', $role)); + break; + } + $roleids = tool_uploadcourse_helper::get_role_ids(); if (isset($roleids[$method['role']])) { $instance->roleid = $roleids[$method['role']]; @@ -1093,6 +1128,41 @@ class tool_uploadcourse_course { } } + /** + * Check if role is allowed in course context + * + * @param int $courseid course context. + * @param string $role Role. + * @return bool + */ + protected function validate_role_context(int $courseid, string $role) : bool { + if (empty($this->assignableroles[$courseid])) { + $coursecontext = \context_course::instance($courseid); + $this->assignableroles[$courseid] = get_assignable_roles($coursecontext, ROLENAME_SHORT); + } + if (!in_array($role, $this->assignableroles[$courseid])) { + return false; + } + return true; + } + + /** + * Check if role is allowed at this context level. + * + * @param int $roleid Role ID. + * @return bool + */ + protected function validate_role_context_level(int $roleid) : bool { + if (empty($this->contextlevels[$roleid])) { + $this->contextlevels[$roleid] = get_role_contextlevels($roleid); + } + + if (!in_array(CONTEXT_COURSE, $this->contextlevels[$roleid])) { + return false; + } + return true; + } + /** * Reset the current course. * diff --git a/admin/tool/uploadcourse/classes/helper.php b/admin/tool/uploadcourse/classes/helper.php index a91e7909df3..8d26c7590e1 100644 --- a/admin/tool/uploadcourse/classes/helper.php +++ b/admin/tool/uploadcourse/classes/helper.php @@ -325,6 +325,11 @@ class tool_uploadcourse_helper { continue; } $rolenames['role_' . $rolesids[$matches[1]]] = $value; + } else if (preg_match('/^(.+)?_role$/', $field, $matches)) { + if (!isset($rolesids[$value])) { + $invalidroles[] = $value; + break; + } } } diff --git a/admin/tool/uploadcourse/classes/processor.php b/admin/tool/uploadcourse/classes/processor.php index 8f55eef78b2..53e0f1a6dcc 100644 --- a/admin/tool/uploadcourse/classes/processor.php +++ b/admin/tool/uploadcourse/classes/processor.php @@ -219,6 +219,10 @@ class tool_uploadcourse_processor { $data = array_merge($data, $course->get_data(), array('id' => $course->get_id())); $tracker->output($this->linenb, true, $status, $data); + if ($course->has_errors()) { + $errors++; + $tracker->output($this->linenb, false, $course->get_errors(), $data); + } } else { $errors++; $tracker->output($this->linenb, false, $course->get_errors(), $data); diff --git a/admin/tool/uploadcourse/tests/behat/create.feature b/admin/tool/uploadcourse/tests/behat/create.feature index 75ae6cd675c..b42e3e949c6 100644 --- a/admin/tool/uploadcourse/tests/behat/create.feature +++ b/admin/tool/uploadcourse/tests/behat/create.feature @@ -105,3 +105,28 @@ Feature: An admin can create courses using a CSV file And I should see "Field 3: b" And I should see "Field 4: Hello" And I should see "Field 5: Some text" + + @javascript + Scenario: Validation of role for uploaded courses + Given I navigate to "Users > Permissions > Define roles" in site administration + And I click on "Add a new role" "button" + And I click on "Continue" "button" + And I set the following fields to these values: + | Short name | notallowed | + | Custom full name | notallowed | + | contextlevel80 | 1 | + And I click on "Create this role" "button" + And I navigate to "Courses > Upload courses" in site administration + And I upload "admin/tool/uploadcourse/tests/fixtures/enrolment_role.csv" file to "File" filemanager + And I click on "Preview" "button" + And I should see "Invalid role names: notexist" + And I should see "Role notallowed not allowed in this context." + When I click on "Upload courses" "button" + And I should see "Course created" + And I should see "Courses total: 3" + And I should see "Courses created: 1" + And I should see "Courses errors: 2" + And I should see "Invalid role names: notexist" + And I should see "Role notallowed not allowed in this context." + And I am on site homepage + And I should see "coursez" diff --git a/admin/tool/uploadcourse/tests/course_test.php b/admin/tool/uploadcourse/tests/course_test.php index cf77b2762af..d664621904c 100644 --- a/admin/tool/uploadcourse/tests/course_test.php +++ b/admin/tool/uploadcourse/tests/course_test.php @@ -1561,6 +1561,94 @@ class course_test extends \advanced_testcase { $this->assertArrayHasKey('cannotrenameshortnamealreadyinuse', $co->get_errors()); } + /** + * Test when role doesn't exist. + * + * @covers \tool_uploadcourse_course::prepare + */ + public function test_role_not_exist() { + $this->resetAfterTest(); + $this->setAdminUser(); + + $mode = tool_uploadcourse_processor::MODE_CREATE_NEW; + $updatemode = tool_uploadcourse_processor::UPDATE_NOTHING; + + $upload = new tool_uploadcourse_course($mode, $updatemode, [ + 'category' => 1, + 'fullname' => 'Testing', + 'shortname' => 'T101', + 'enrolment_1' => 'manual', + 'enrolment_1_role' => 'notexist' + ]); + + $this->assertFalse($upload->prepare()); + $this->assertArrayHasKey('invalidroles', $upload->get_errors()); + } + + /** + * Test when role not allowed in course context. + * + * @covers \tool_uploadcourse_course::proceed + */ + public function test_role_not_allowed() { + $this->resetAfterTest(); + $this->setAdminUser(); + + $roleid = create_role('New student role', 'student2', 'New student description', 'student'); + set_role_contextlevels($roleid, [CONTEXT_BLOCK]); + + $mode = tool_uploadcourse_processor::MODE_CREATE_NEW; + $updatemode = tool_uploadcourse_processor::UPDATE_NOTHING; + + $upload = new tool_uploadcourse_course($mode, $updatemode, [ + 'category' => 1, + 'fullname' => 'Testing', + 'shortname' => 'T101', + 'enrolment_1' => 'manual', + 'enrolment_1_role' => 'student2' + ]); + + $this->assertFalse($upload->prepare()); + $this->assertArrayHasKey('contextrolenotallowed', $upload->get_errors()); + } + + /** + * Test when role is allowed. + * + * @covers \tool_uploadcourse_course::proceed + */ + public function test_role_allowed() { + global $DB; + + $this->resetAfterTest(); + $this->setAdminUser(); + + $mode = tool_uploadcourse_processor::MODE_UPDATE_ONLY; + $updatemode = tool_uploadcourse_processor::UPDATE_MISSING_WITH_DATA_OR_DEFAUTLS; + + $course = $this->getDataGenerator()->create_course([ + 'shortname' => 'c1', + ]); + + $instances = enrol_get_instances($course->id, true); + $studentrole = $DB->get_record('role', ['shortname' => 'student']); + $teacherrole = $DB->get_record('role', ['shortname' => 'teacher']); + $instance = reset($instances); + $this->assertEquals($studentrole->id, $instance->roleid); + + $upload = new tool_uploadcourse_course($mode, $updatemode, [ + 'shortname' => 'c1', + 'enrolment_1' => 'manual', + 'enrolment_1_role' => 'teacher' + ]); + + $this->assertTrue($upload->prepare()); + $upload->proceed(); + $instances = enrol_get_instances($course->id, true); + $instance = reset($instances); + $this->assertEquals($teacherrole->id, $instance->roleid); + } + /** * Get custom field plugin generator * diff --git a/admin/tool/uploadcourse/tests/fixtures/enrolment_role.csv b/admin/tool/uploadcourse/tests/fixtures/enrolment_role.csv new file mode 100644 index 00000000000..1190431d7eb --- /dev/null +++ b/admin/tool/uploadcourse/tests/fixtures/enrolment_role.csv @@ -0,0 +1,4 @@ +shortname,fullname,category,enrolment_1,enrolment_1_role +CX,coursex,1,manual,notexist +CY,coursey,1,manual,notallowed +CZ,coursez,1,manual,student \ No newline at end of file diff --git a/lang/en/role.php b/lang/en/role.php index 06e2c058f3f..3d840671759 100644 --- a/lang/en/role.php +++ b/lang/en/role.php @@ -160,6 +160,7 @@ $string['contentbank:viewunlistedcontent'] = 'View unlisted content from the con $string['contentbank:upload'] = 'Upload new content to the content bank'; $string['contentbank:useeditor'] = 'Create or edit content using a content type editor'; $string['context'] = 'Context'; +$string['contextrolenotallowed'] = 'Role {$a} not allowed in this context.'; $string['course:activityvisibility'] = 'Hide/show activities'; $string['course:bulkmessaging'] = 'Send a message to many people'; $string['course:create'] = 'Create courses';