From 6fb4a3b56e7aaf4d50ebc394f439846ce40964f1 Mon Sep 17 00:00:00 2001 From: Neill Magill Date: Thu, 28 Oct 2021 15:58:43 +0100 Subject: [PATCH] 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 | 21 +++++++++++++---- message/classes/helper.php | 14 +++++++++--- message/tests/helper_test.php | 37 ++++++++++++++++++++++++++++++ user/lib.php | 14 +++++++++--- user/tests/userlib_test.php | 43 +++++++++++++++++++++++++++++++++++ user/upgrade.txt | 9 ++++++++ 6 files changed, 128 insertions(+), 10 deletions(-) diff --git a/message/classes/api.php b/message/classes/api.php index 65432281fa0..de89146fe9c 100644 --- a/message/classes/api.php +++ b/message/classes/api.php @@ -203,6 +203,8 @@ class api { 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 @@ class api { } } + // 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 @@ class api { $params, $excludeparams, $userid, - $selfconversation + $selfconversation, + $namefields ) { global $DB, $CFG; @@ -258,8 +266,9 @@ class api { // 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 @@ class api { // 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. diff --git a/message/classes/helper.php b/message/classes/helper.php index 8174cf1d886..1396d8f38c9 100644 --- a/message/classes/helper.php +++ b/message/classes/helper.php @@ -653,21 +653,29 @@ class helper { * 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; } diff --git a/message/tests/helper_test.php b/message/tests/helper_test.php index 8ac309b555d..6d08f834c88 100644 --- a/message/tests/helper_test.php +++ b/message/tests/helper_test.php @@ -29,6 +29,7 @@ require_once($CFG->dirroot . '/message/tests/messagelib_test.php'); * @category test * @copyright 2018 Jake Dallimore * @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 @@ class helper_test extends \advanced_testcase { $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. */ diff --git a/user/lib.php b/user/lib.php index cd2ba14d380..020ccc11fa4 100644 --- a/user/lib.php +++ b/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); } } } diff --git a/user/tests/userlib_test.php b/user/tests/userlib_test.php index e0f50b0a8cb..35b05d00227 100644 --- a/user/tests/userlib_test.php +++ b/user/tests/userlib_test.php @@ -130,6 +130,49 @@ class userlib_test extends \advanced_testcase { $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. */ diff --git a/user/upgrade.txt b/user/upgrade.txt index 442bf5a16e2..c74770ef21c 100644 --- a/user/upgrade.txt +++ b/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