MDL-72948 messaging: Minimise fields used in query

Most of the fields in the query are not needed and are discarded
soon after visibility checks are made.

message/classes/api.php

@@ -203,6 +203,8 @@ public static function message_search_users(int $userid, string $search, int $li
             throw new \moodle_exception('disabled', 'message');
         }
 
+        require_once($CFG->dirroot . '/user/lib.php');
+
         // Used to search for contacts.
         $fullname = $DB->sql_fullname();
 
@@ -236,6 +238,11 @@ public static function message_search_users(int $userid, string $search, int $li
             }
         }
 
+        // We need to get all the user details for a fullname in the visibility checks.
+        $namefields = \core_user\fields::for_name()
+            // Required by the visibility checks.
+            ->including('deleted');
+
         // Let's get those non-contacts.
         // Because we can't achieve all the required visibility checks in SQL, we'll iterate through the non-contact records
         // and stop once we have enough matching the 'visible' criteria.
@@ -247,7 +254,8 @@ public static function message_search_users(int $userid, string $search, int $li
             $params,
             $excludeparams,
             $userid,
-            $selfconversation
+            $selfconversation,
+            $namefields
         ) {
             global $DB, $CFG;
 
@@ -258,8 +266,9 @@ public static function message_search_users(int $userid, string $search, int $li
 
             // Since we want to order a UNION we need to list out all the user fields individually this will
             // allow us to reference the fullname correctly.
-            $userfields = implode(', u.', get_user_fieldnames());
-            $select = "u.id, " . $DB->sql_fullname() ." AS sortingname, u." . $userfields;
+            $userfields = $namefields->get_sql('u')->selects;
+
+            $select = "u.id, " . $DB->sql_fullname() . " AS sortingname" . $userfields;
 
             // When messageallusers is false valid non-contacts must be enrolled on one of the users courses.
             if (empty($CFG->messagingallusers)) {
@@ -313,13 +322,17 @@ public static function message_search_users(int $userid, string $search, int $li
         // See MDL-63983 dealing with performance improvements to this area of code.
         $noofvalidseenrecords = 0;
         $returnedusers = [];
+
+        // Only fields that are also part of user_get_default_fields() are valid when passed into user_get_user_details().
+        $fields = array_intersect($namefields->get_required_fields(), user_get_default_fields());
+
         foreach ($getnoncontactusers(0, $batchlimit) as $users) {
             foreach ($users as $id => $user) {
                 // User visibility checks: only return users who are visible to the user performing the search.
                 // Which visibility check to use depends on the 'messagingallusers' (site wide messaging) setting:
                 // - If enabled, return matched users whose profiles are visible to the current user anywhere (site or course).
                 // - If disabled, only return matched users whose course profiles are visible to the current user.
-                $userdetails = \core_message\helper::search_get_user_details($user);
+                $userdetails = \core_message\helper::search_get_user_details($user, $fields);
 
                 // Return the user only if the searched field is returned.
                 // Otherwise it means that the $USER was not allowed to search the returned user.

message/classes/helper.php

@@ -653,21 +653,29 @@ public static function render_messaging_widget(
      * If disabled, visibility requires that the user be sharing a course with the searching user, and have a visible profile there.
      * The current user is always returned.
      *
+     * You can use the $userfields parameter to reduce the amount of a user record that is required by the method.
+     * The minimum user fields are:
+     *  * id
+     *  * deleted
+     *  * all potential fullname fields
+     *
      * @param \stdClass $user
+     * @param array $userfields An array of userfields to be returned, the values must be a
+     *                          subset of user_get_default_fields (optional)
      * @return array the array of userdetails, if visible, or an empty array otherwise.
      */
-    public static function search_get_user_details(\stdClass $user) : array {
+    public static function search_get_user_details(\stdClass $user, array $userfields = []) : array {
         global $CFG, $USER;
         require_once($CFG->dirroot . '/user/lib.php');
 
         if ($CFG->messagingallusers || $user->id == $USER->id) {
-            return \user_get_user_details_courses($user) ?? []; // This checks visibility of site and course profiles.
+            return \user_get_user_details_courses($user, $userfields) ?? []; // This checks visibility of site and course profiles.
         } else {
             // Messaging specific: user must share a course with the searching user AND have a visible profile there.
             $sharedcourses = enrol_get_shared_courses($USER, $user);
             foreach ($sharedcourses as $course) {
                 if (user_can_view_profile($user, $course)) {
-                    $userdetails = user_get_user_details($user, $course);
+                    $userdetails = user_get_user_details($user, $course, $userfields);
                     if (!is_null($userdetails)) {
                         return $userdetails;
                     }

message/tests/helper_test.php

@@ -29,6 +29,7 @@
  * @category test
  * @copyright 2018 Jake Dallimore <jrhdallimore@gmail.com>
  * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ * @covers \core_message\helper
  */
 class helper_test extends \advanced_testcase {
 
@@ -116,6 +117,42 @@ public function test_search_get_user_details_sitewide_disabled() {
         $this->assertEmpty(\core_message\helper::search_get_user_details($user7)); // Teacher (course contact) in another course.
     }
 
+    /**
+     * Test search_get_user_details returns the correct profile data we limit the data we wish to be returned.
+     */
+    public function test_search_get_user_details_limited_data() {
+        set_config('messagingallusers', false);
+
+        // Two students sharing course 1, visible profile within course (no groups).
+        $user1 = $this->getDataGenerator()->create_user();
+        $user2 = $this->getDataGenerator()->create_user();
+        $course1 = $this->getDataGenerator()->create_course((object) ['groupmode' => 0]);
+        $this->getDataGenerator()->enrol_user($user1->id, $course1->id);
+        $this->getDataGenerator()->enrol_user($user2->id, $course1->id);
+
+        // Calculate the minimum fields that can be returned.
+        $namefields = \core_user\fields::for_name()->get_required_fields();
+        $fields = array_intersect($namefields, user_get_default_fields());
+
+        $minimaluser = (object) [
+            'id' => $user2->id,
+            'deleted' => $user2->deleted,
+        ];
+
+        foreach ($namefields as $field) {
+            $minimaluser->$field = $user2->$field;
+        }
+
+        // Test that less data is returned using the filter.
+        $this->setUser($user1);
+        $fulldetails = helper::search_get_user_details($user2);
+        $limiteddetails = helper::search_get_user_details($minimaluser, $fields);
+        $fullcount = count($fulldetails);
+        $limitedcount = count($limiteddetails);
+        $this->assertLessThan($fullcount, $limitedcount);
+        $this->assertNotEquals($fulldetails, $limiteddetails);
+    }
+
     /**
      * Test search_get_user_details returns the correct profile data when $CFG->messagingallusers is enabled.
      */

user/lib.php

@@ -583,10 +583,18 @@ function user_get_user_details($user, $course = null, array $userfields = array(
  * Tries to obtain user details, either recurring directly to the user's system profile
  * or through one of the user's course enrollments (course profile).
  *
+ * You can use the $userfields parameter to reduce the amount of a user record that is required by the method.
+ * The minimum user fields are:
+ *  * id
+ *  * deleted
+ *  * all potential fullname fields
+ *
  * @param stdClass $user The user.
+ * @param array $userfields An array of userfields to be returned, the values must be a
+ *                          subset of user_get_default_fields (optional)
  * @return array if unsuccessful or the allowed user details.
  */
-function user_get_user_details_courses($user) {
+function user_get_user_details_courses($user, array $userfields = []) {
     global $USER;
     $userdetails = null;
 
@@ -597,14 +605,14 @@ function user_get_user_details_courses($user) {
 
     // Try using system profile.
     if ($systemprofile) {
-        $userdetails = user_get_user_details($user, null);
+        $userdetails = user_get_user_details($user, null, $userfields);
     } else {
         // Try through course profile.
         // Get the courses that the user is enrolled in (only active).
         $courses = enrol_get_users_courses($user->id, true);
         foreach ($courses as $course) {
             if (user_can_view_profile($user, $course)) {
-                $userdetails = user_get_user_details($user, $course);
+                $userdetails = user_get_user_details($user, $course, $userfields);
             }
         }
     }

user/tests/userlib_test.php

@@ -130,6 +130,49 @@ public function test_user_get_user_details_courses_groupmode_visible() {
         $this->assertEquals($user2->id, $userdetails['id']);
     }
 
+    /**
+     * Tests that the user fields returned by the method can be limited.
+     *
+     * @covers ::user_get_user_details_courses
+     */
+    public function test_user_get_user_details_courses_limit_return() {
+        $this->resetAfterTest();
+
+        // Setup some data.
+        $user1 = $this->getDataGenerator()->create_user();
+        $user2 = $this->getDataGenerator()->create_user();
+        $course = $this->getDataGenerator()->create_course();
+        $this->getDataGenerator()->enrol_user($user1->id, $course->id);
+        $this->getDataGenerator()->enrol_user($user2->id, $course->id);
+
+        // Calculate the minimum fields that can be returned.
+        $namefields = \core_user\fields::for_name()->get_required_fields();
+        $fields = array_intersect($namefields, user_get_default_fields());
+
+        $minimaluser = (object) [
+            'id' => $user2->id,
+            'deleted' => $user2->deleted,
+        ];
+
+        foreach ($namefields as $field) {
+            $minimaluser->$field = $user2->$field;
+        }
+
+        $this->setUser($user1);
+        $fulldetails = user_get_user_details_courses($user2);
+        $limiteddetails = user_get_user_details_courses($minimaluser, $fields);
+        $this->assertIsArray($fulldetails);
+        $this->assertIsArray($limiteddetails);
+        $this->assertEquals($user2->id, $fulldetails['id']);
+        $this->assertEquals($user2->id, $limiteddetails['id']);
+
+        // Test that less data was returned when using a filter.
+        $fullcount = count($fulldetails);
+        $limitedcount = count($limiteddetails);
+        $this->assertLessThan($fullcount, $limitedcount);
+        $this->assertNotEquals($fulldetails, $limiteddetails);
+    }
+
     /**
      * Test user_update_user.
      */

user/upgrade.txt

@@ -1,5 +1,14 @@
 This files describes API changes for code that uses the user API.
 
+=== 4.1 ===
+* user_get_user_details_courses() now accepts an optional second parameter, an array of userfields that should be
+  returned. The values passed into the $userfields parameter must all be included in the return from
+  user_get_default_fields().
+  It also allows you to reduce how much of a user record is required by the method. The minimum user record fields are:
+    * id
+    * deleted
+    * all potential fullname fields
+
 === 4.0 ===
 
 * External function core_user_external::update_users() will now fail on a per user basis. Previously if one user