From b6d12864141b0280f4dd4f54ffa31e7892bd90e2 Mon Sep 17 00:00:00 2001
From: Alexandru Elisei <alexandru.elisei@gmail.com>
Date: Fri, 4 Mar 2016 15:43:36 +0200
Subject: [PATCH] MDL-52574 accesslib: Prevent get_role_users() fail in certain
 cases

The function get_role_users() requires the caller to include the $sort fields in
the $fields argument. On PostgreSQL this will cause the function to fail when
the default $sort fields aren't part of the requested fields. The behavior of
the function is augmented to add the $sort fields to $fields if they are not
already present.
---
 lib/accesslib.php            | 19 +++++++++++++++++--
 lib/tests/accesslib_test.php | 10 ++++++++++
 lib/upgrade.txt              |  3 +++
 3 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/lib/accesslib.php b/lib/accesslib.php
index f50c23fa71a..ae3936d1e78 100644
--- a/lib/accesslib.php
+++ b/lib/accesslib.php
@@ -4110,8 +4110,7 @@ function sort_by_roleassignment_authority($users, context $context, $roles = arr
  * system is more flexible. If you really need, you can to use this
  * function but consider has_capability() as a possible substitute.
  *
- * The caller function is responsible for including all the
- * $sort fields in $fields param.
+ * All $sort fields are added into $fields if not present there yet.
  *
  * If $roleid is an array or is empty (all roles) you need to set $fields
  * (and $sort by extension) params according to it, as the first field
@@ -4209,6 +4208,22 @@ function get_role_users($roleid, context $context, $parent = false, $fields = ''
         $params = array_merge($params, $sortparams);
     }
 
+    // Adding the fields from $sort that are not present in $fields.
+    $sortarray = preg_split('/,\s*/', $sort);
+    $fieldsarray = preg_split('/,\s*/', $fields);
+    $addedfields = array();
+    foreach ($sortarray as $sortfield) {
+        if (!in_array($sortfield, $fieldsarray)) {
+            $fieldsarray[] = $sortfield;
+            $addedfields[] = $sortfield;
+        }
+    }
+    $fields = implode(', ', $fieldsarray);
+    if (!empty($addedfields)) {
+        $addedfields = implode(', ', $addedfields);
+        debugging('get_role_users() adding '.$addedfields.' to the query result because they were required by $sort but missing in $fields');
+    }
+
     if ($all === null) {
         // Previously null was used to indicate that parameter was not used.
         $all = true;
diff --git a/lib/tests/accesslib_test.php b/lib/tests/accesslib_test.php
index 4534dde1fd6..e3a2f995d2c 100644
--- a/lib/tests/accesslib_test.php
+++ b/lib/tests/accesslib_test.php
@@ -1443,6 +1443,16 @@ class core_accesslib_testcase extends advanced_testcase {
         $this->assertArrayHasKey($user1->id, $users);
         $this->assertArrayHasKey($user3->id, $users);
 
+        $users = get_role_users($teacherrole->id, $coursecontext, false, 'u.id, u.email');
+        $this->assertDebuggingCalled('get_role_users() adding u.lastname, u.firstname to the query result because they were required by $sort but missing in $fields');
+        $this->assertCount(2, $users);
+        $this->assertArrayHasKey($user1->id, $users);
+        $this->assertObjectHasAttribute('lastname', $users[$user1->id]);
+        $this->assertObjectHasAttribute('firstname', $users[$user1->id]);
+        $this->assertArrayHasKey($user3->id, $users);
+        $this->assertObjectHasAttribute('lastname', $users[$user3->id]);
+        $this->assertObjectHasAttribute('firstname', $users[$user3->id]);
+
         $users = get_role_users($teacherrole->id, $coursecontext, false, 'u.id, u.email, u.idnumber', 'u.idnumber', null, $group->id);
         $this->assertCount(1, $users);
         $this->assertArrayHasKey($user3->id, $users);
diff --git a/lib/upgrade.txt b/lib/upgrade.txt
index 72e47f20d7f..a88c602c401 100644
--- a/lib/upgrade.txt
+++ b/lib/upgrade.txt
@@ -3,6 +3,9 @@ information provided here is intended especially for developers.
 
 === 3.1 ===
 
+* The get_role_users() function will now add the $sort fields that are not part
+  of the requested fields to the query result and will throw a debugging message
+  with the added fields when that happens.
 * The core_user::fill_properties_cache() static method has been introduced to be a reference
   and allow standard user fields data validation. Right now only type validation is supported
   checking it against the parameter (PARAM_*) type of the target user field. MDL-52781 is