Merge branch 'MDL-59847-35' of https://github.com/snake/moodle into M…

…OODLE_35_STABLE

lib/moodlelib.php

@@ -3601,25 +3601,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;
 }
 

lib/tests/moodlelib_test.php

@@ -1465,16 +1465,14 @@ public function test_unset_user_preference_for_current_user() {
         $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 @@ public function test_get_extra_user_fields() {
         $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();

user/selector/lib.php

@@ -105,6 +105,7 @@ public function __construct($name, $options = array()) {
             $this->accesscontext = context_system::instance();
         }
 
+        // Populate the list of additional user identifiers to display.
         if (isset($options['extrafields'])) {
             $this->extrafields = $options['extrafields'];
         } else if (!empty($CFG->showuseridentity) &&
@@ -113,6 +114,27 @@ public function __construct($name, $options = array()) {
         } else {
             $this->extrafields = array();
         }
+
+        // Filter out hidden identifiers if the user can't see them.
+        $hiddenfields = array_filter(explode(',', $CFG->hiddenuserfields));
+        $hiddenidentifiers = array_intersect($this->extrafields, $hiddenfields);
+
+        if ($hiddenidentifiers) {
+            if ($this->accesscontext->get_course_context(false)) {
+                // We are somewhere inside a course.
+                $canviewhiddenuserfields = has_capability('moodle/course:viewhiddenuserfields', $this->accesscontext);
+
+            } else {
+                // We are not inside a course.
+                $canviewhiddenuserfields = has_capability('moodle/user:viewhiddendetails', $this->accesscontext);
+            }
+
+            if (!$canviewhiddenuserfields) {
+                // Remove hidden identifiers from the list.
+                $this->extrafields = array_diff($this->extrafields, $hiddenidentifiers);
+            }
+        }
+
         if (isset($options['exclude']) && is_array($options['exclude'])) {
             $this->exclude = $options['exclude'];
         }

user/tests/fixtures/testable_user_selector.php

@@ -0,0 +1,65 @@
+<?php
+// This file is part of Moodle - https://moodle.org/
+//
+// Moodle is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// Moodle is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
+
+/**
+ * Provides {@link testable_user_selector} class.
+ *
+ * @package     core_user
+ * @subpackage  fixtures
+ * @category    test
+ * @copyright   2018 David Mudrák <david@moodle.com>
+ * @license     http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+
+defined('MOODLE_INTERNAL') || die();
+
+/**
+ * Testable subclass of the user selector base class.
+ *
+ * @copyright 2018 David Mudrák <david@moodle.com>
+ * @license   http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+class testable_user_selector extends user_selector_base {
+
+    /**
+     * Basic implementation of the users finder.
+     *
+     * @param string $search
+     * @return array of (string)optgroupname => array of users
+     */
+    public function find_users($search) {
+        global $DB;
+
+        list($wherecondition, $whereparams) = $this->search_sql($search, 'u');
+        list($sort, $sortparams) = users_order_by_sql('u', $search, $this->accesscontext);
+        $params = array_merge($whereparams, $sortparams);
+        $fields = $this->required_fields_sql('u');
+
+        $sql = "SELECT $fields
+                  FROM {user} u
+                 WHERE $wherecondition
+              ORDER BY $sort";
+
+        $found = $DB->get_records_sql($sql, $params);
+
+        if (empty($found)) {
+            return [];
+        }
+
+        return [get_string('potusers', 'core_role') => $found];
+    }
+
+}