diff --git a/lib/moodlelib.php b/lib/moodlelib.php index 928ec9f63ce..27349b3d35e 100644 --- a/lib/moodlelib.php +++ b/lib/moodlelib.php @@ -3636,25 +3636,38 @@ function get_extra_user_fields($context, $already = array()) { return array(); } - // Split showuseridentity on comma. - if (empty($CFG->showuseridentity)) { - // Explode gives wrong result with empty string. - $extra = array(); - } else { - $extra = explode(',', $CFG->showuseridentity); - } - $renumber = false; + // Split showuseridentity on comma (filter needed in case the showuseridentity is empty). + $extra = array_filter(explode(',', $CFG->showuseridentity)); + foreach ($extra as $key => $field) { if (in_array($field, $already)) { unset($extra[$key]); - $renumber = true; } } - if ($renumber) { - // For consistency, if entries are removed from array, renumber it - // so they are numbered as you would expect. - $extra = array_merge($extra); + + // If the identity fields are also among hidden fields, make sure the user can see them. + $hiddenfields = array_filter(explode(',', $CFG->hiddenuserfields)); + $hiddenidentifiers = array_intersect($extra, $hiddenfields); + + if ($hiddenidentifiers) { + if ($context->get_course_context(false)) { + // We are somewhere inside a course. + $canviewhiddenuserfields = has_capability('moodle/course:viewhiddenuserfields', $context); + + } else { + // We are not inside a course. + $canviewhiddenuserfields = has_capability('moodle/user:viewhiddendetails', $context); + } + + if (!$canviewhiddenuserfields) { + // Remove hidden identifiers from the list. + $extra = array_diff($extra, $hiddenidentifiers); + } } + + // Re-index the entries. + $extra = array_values($extra); + return $extra; } diff --git a/lib/tests/moodlelib_test.php b/lib/tests/moodlelib_test.php index 80dc799168d..c429fb8f34d 100644 --- a/lib/tests/moodlelib_test.php +++ b/lib/tests/moodlelib_test.php @@ -1465,16 +1465,14 @@ class core_moodlelib_testcase extends advanced_testcase { $this->assertNull(get_user_preferences('test_pref')); } - public function test_get_extra_user_fields() { + /** + * Test essential features implementation of {@link get_extra_user_fields()} as the admin user with all capabilities. + */ + public function test_get_extra_user_fields_essentials() { global $CFG, $USER, $DB; $this->resetAfterTest(); $this->setAdminUser(); - - // It would be really nice if there were a way to 'mock' has_capability - // checks (either to return true or false) but as there is not, this - // test doesn't test the capability check. Presumably, anyone running - // unit tests will have the capability. $context = context_system::instance(); // No fields. @@ -1502,6 +1500,121 @@ class core_moodlelib_testcase extends advanced_testcase { $this->assertEquals(array('zombie'), get_extra_user_fields($context, array('frog'))); } + /** + * Prepare environment for couple of tests related to permission checks in {@link get_extra_user_fields()}. + * + * @return stdClass + */ + protected function environment_for_get_extra_user_fields_tests() { + global $CFG, $DB; + + $CFG->showuseridentity = 'idnumber,country,city'; + $CFG->hiddenuserfields = 'country,city'; + + $env = new stdClass(); + + $env->course = $this->getDataGenerator()->create_course(); + $env->coursecontext = context_course::instance($env->course->id); + + $env->teacherrole = $DB->get_record('role', array('shortname' => 'teacher')); + $env->studentrole = $DB->get_record('role', array('shortname' => 'student')); + $env->managerrole = $DB->get_record('role', array('shortname' => 'manager')); + + $env->student = $this->getDataGenerator()->create_user(); + $env->teacher = $this->getDataGenerator()->create_user(); + $env->manager = $this->getDataGenerator()->create_user(); + + role_assign($env->studentrole->id, $env->student->id, $env->coursecontext->id); + role_assign($env->teacherrole->id, $env->teacher->id, $env->coursecontext->id); + role_assign($env->managerrole->id, $env->manager->id, SYSCONTEXTID); + + return $env; + } + + /** + * No identity fields shown to student user (no permission to view identity fields). + */ + public function test_get_extra_user_fields_no_access() { + + $this->resetAfterTest(); + $env = $this->environment_for_get_extra_user_fields_tests(); + $this->setUser($env->student); + + $this->assertEquals(array(), get_extra_user_fields($env->coursecontext)); + $this->assertEquals(array(), get_extra_user_fields(context_system::instance())); + } + + /** + * Teacher can see students' identity fields only within the course. + */ + public function test_get_extra_user_fields_course_only_access() { + + $this->resetAfterTest(); + $env = $this->environment_for_get_extra_user_fields_tests(); + $this->setUser($env->teacher); + + $this->assertEquals(array('idnumber', 'country', 'city'), get_extra_user_fields($env->coursecontext)); + $this->assertEquals(array(), get_extra_user_fields(context_system::instance())); + } + + /** + * Teacher can be prevented from seeing students' identity fields even within the course. + */ + public function test_get_extra_user_fields_course_prevented_access() { + + $this->resetAfterTest(); + $env = $this->environment_for_get_extra_user_fields_tests(); + $this->setUser($env->teacher); + + assign_capability('moodle/course:viewhiddenuserfields', CAP_PREVENT, $env->teacherrole->id, $env->coursecontext->id); + $this->assertEquals(array('idnumber'), get_extra_user_fields($env->coursecontext)); + } + + /** + * Manager can see students' identity fields anywhere. + */ + public function test_get_extra_user_fields_anywhere_access() { + + $this->resetAfterTest(); + $env = $this->environment_for_get_extra_user_fields_tests(); + $this->setUser($env->manager); + + $this->assertEquals(array('idnumber', 'country', 'city'), get_extra_user_fields($env->coursecontext)); + $this->assertEquals(array('idnumber', 'country', 'city'), get_extra_user_fields(context_system::instance())); + } + + /** + * Manager can be prevented from seeing hidden fields outside the course. + */ + public function test_get_extra_user_fields_schismatic_access() { + + $this->resetAfterTest(); + $env = $this->environment_for_get_extra_user_fields_tests(); + $this->setUser($env->manager); + + assign_capability('moodle/user:viewhiddendetails', CAP_PREVENT, $env->managerrole->id, SYSCONTEXTID, true); + $this->assertEquals(array('idnumber'), get_extra_user_fields(context_system::instance())); + // Note that inside the course, the manager can still see the hidden identifiers as this is currently + // controlled by a separate capability for legacy reasons. + $this->assertEquals(array('idnumber', 'country', 'city'), get_extra_user_fields($env->coursecontext)); + } + + /** + * Two capabilities must be currently set to prevent manager from seeing hidden fields. + */ + public function test_get_extra_user_fields_hard_to_prevent_access() { + + $this->resetAfterTest(); + $env = $this->environment_for_get_extra_user_fields_tests(); + $this->setUser($env->manager); + + assign_capability('moodle/user:viewhiddendetails', CAP_PREVENT, $env->managerrole->id, SYSCONTEXTID, true); + assign_capability('moodle/course:viewhiddenuserfields', CAP_PREVENT, $env->managerrole->id, SYSCONTEXTID, true); + + $this->assertEquals(array('idnumber'), get_extra_user_fields(context_system::instance())); + $this->assertEquals(array('idnumber'), get_extra_user_fields($env->coursecontext)); + } + public function test_get_extra_user_fields_sql() { global $CFG, $USER, $DB; $this->resetAfterTest();