Commit 6fb4a3b5 authored by Neill Magill's avatar Neill Magill
Browse files

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.
parent b81fb00f
......@@ -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.
......
......@@ -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;
}
......
......@@ -29,6 +29,7 @@ require_once($CFG->dirroot . '/message/tests/messagelib_test.php');
* @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 @@ 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.
*/
......
......@@ -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);
}
}
}
......
......@@ -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.
*/
......
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
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment