Moodle: students can view other users in "only see own membership" groups

@@ -123,23 +123,28 @@ public static function sql_group_visibility_where(int $userid,
      * @param string $groupsalias The SQL alias being used for the groups table.
      * @param string $groupsmembersalias The SQL alias being used for the groups_members table.
      * @param string $useralias The SQL alias being used for the user table.
+     * @param string $paramprefix Prefix for the parameter names.
      * @return array [$where, $params]
      */
-    public static function sql_member_visibility_where(string $groupsalias = 'g',
-            string $groupsmembersalias = 'gm', string $useralias = 'u'): array {
+    public static function sql_member_visibility_where(
+        string $groupsalias = 'g',
+        string $groupsmembersalias = 'gm',
+        string $useralias = 'u',
+        string $paramprefix = '',
+    ): array {
         global $USER;
 
-        list($memberssql, $membersparams) = self::sql_members_visibility_condition($groupsalias, $groupsmembersalias);
+        list($memberssql, $membersparams) = self::sql_members_visibility_condition($groupsalias, $groupsmembersalias, $paramprefix);
 
-        $where = " AND (
-                        {$groupsalias}.visibility = :all
-                        OR ($memberssql)
-                        OR ({$groupsalias}.visibility = :own AND {$useralias}.id = :currentuser2)
-                    )";
+        $where = "(
+            {$groupsalias}.visibility = :{$paramprefix}all
+            OR ($memberssql)
+            OR ({$groupsalias}.visibility = :{$paramprefix}own AND {$useralias}.id = :{$paramprefix}currentuser2)
+        )";
         $params = [
-            'all' => GROUPS_VISIBILITY_ALL,
-            'own' => GROUPS_VISIBILITY_OWN,
-            'currentuser2' => $USER->id,
+            "{$paramprefix}all" => GROUPS_VISIBILITY_ALL,
+            "{$paramprefix}own" => GROUPS_VISIBILITY_OWN,
+            "{$paramprefix}currentuser2" => $USER->id,
         ];
         $params = array_merge($params, $membersparams);
         return [$where, $params];
@@ -150,21 +155,25 @@ public static function sql_member_visibility_where(string $groupsalias = 'g',
      *
      * @param string $groupsalias The SQL alias being used for the groups table.
      * @param string $groupsmembersalias The SQL alias being used for the groups_members table.
+     * @param string $paramprefix Prefix for the parameter names.
      * @return array [$sql, $params]
      */
-    protected static function sql_members_visibility_condition(string $groupsalias = 'g',
-            string $groupsmembersalias = 'gm'): array {
+    protected static function sql_members_visibility_condition(
+        string $groupsalias = 'g',
+        string $groupsmembersalias = 'gm',
+        string $paramprefix = '',
+    ): array {
         global $USER;
-        $sql = "{$groupsalias}.visibility = :members
+        $sql = "{$groupsalias}.visibility = :{$paramprefix}members
                     AND (
                         SELECT gm2.id
                           FROM {groups_members} gm2
                          WHERE gm2.groupid = {$groupsmembersalias}.groupid
-                               AND gm2.userid = :currentuser
+                               AND gm2.userid = :{$paramprefix}currentuser
                     ) IS NOT NULL";
         $params = [
-            'members' => GROUPS_VISIBILITY_MEMBERS,
-            'currentuser' => $USER->id
+            "{$paramprefix}members" => GROUPS_VISIBILITY_MEMBERS,
+            "{$paramprefix}currentuser" => $USER->id
         ];
 
         return [$sql, $params];

@@ -54,9 +54,7 @@ Feature: Private groups
       | student8 | N     |
 
   Scenario: Participants in "Visible to everyone" groups see their membership and other members:
-    Given I log in as "student1"
-    And I am on "Course 1" course homepage
-    When I follow "Participants"
+    Given I am on the "C1" "enrolled users" page logged in as "student1"
     Then the following should exist in the "participants" table:
       | First name / Surname | Groups                                                         |
       | Student 1            | Visible to everyone/Non-Participation, Visible to everyone/Participation |
@@ -69,9 +67,7 @@ Feature: Private groups
       | Student 8            | No groups                                                      |
 
   Scenario: Participants in "Only visible to members" groups see their membership and other members, plus "Visible to everyone"
-    Given I log in as "student2"
-    And I am on "Course 1" course homepage
-    When I follow "Participants"
+    Given I am on the "C1" "enrolled users" page logged in as "student2"
     Then the following should exist in the "participants" table:
       | First name / Surname | Groups                                                                 |
       | Student 1            | Visible to everyone/Non-Participation, Visible to everyone/Participation         |
@@ -84,9 +80,7 @@ Feature: Private groups
       | Student 8            | No groups                                                              |
 
   Scenario: Participants in "Only see own membership" groups see their membership but not other members, plus "Visible to everyone"
-    Given I log in as "student3"
-    And I am on "Course 1" course homepage
-    When I follow "Participants"
+    Given I am on the "C1" "enrolled users" page logged in as "student3"
     Then the following should exist in the "participants" table:
       | First name / Surname | Groups                                                                 |
       | Student 1            | Visible to everyone/Non-Participation, Visible to everyone/Participation         |
@@ -99,9 +93,7 @@ Feature: Private groups
       | Student 8            | No groups                                                              |
 
   Scenario: Participants in "Not visible" groups do not see that group, do see "Visible to everyone"
-    Given I log in as "student4"
-    And I am on "Course 1" course homepage
-    When I follow "Participants"
+    Given I am on the "C1" "enrolled users" page logged in as "student4"
     Then the following should exist in the "participants" table:
       | First name / Surname | Groups                                                                 |
       | Student 1            | Visible to everyone/Non-Participation, Visible to everyone/Participation         |
@@ -114,9 +106,7 @@ Feature: Private groups
       | Student 8            | No groups                                                              |
 
   Scenario: View participants list as a teacher:
-    Given I log in as "teacher1"
-    And I am on "Course 1" course homepage
-    When I follow "Participants"
+    Given I am on the "C1" "enrolled users" page logged in as "teacher1"
     Then the following should exist in the "participants" table:
       | First name / Surname | Groups                                                                 |
       | Student 1            | Visible to everyone/Non-Participation, Visible to everyone/Participation         |
@@ -127,3 +117,60 @@ Feature: Private groups
       | Student 6            | Only visible to members/Non-Participation, Only visible to members/Participation |
       | Student 7            | Only see own membership                                                     |
       | Student 8            | Not visible                                                            |
+
+  @javascript
+  Scenario: Filtering by "Only see own membership" groups should not show other members.
+    Given I am on the "C1" "enrolled users" page logged in as "student3"
+    When I set the field "type" to "Groups"
+    And I set the field "Type or select..." to "Only see own membership"
+    And I click on "Apply filters" "button"
+    Then the following should exist in the "participants" table:
+      | First name / Surname | Groups                  |
+      | Student 3            | Only see own membership |
+    And the following should not exist in the "participants" table:
+      | First name / Surname | Groups                  |
+      | Student 7            | No groups               |
+
+  @javascript
+  Scenario: Filtering by "No group" should show all users whose memberships I cannot see
+    Given I am on the "C1" "enrolled users" page logged in as "student3"
+    When I set the field "type" to "Groups"
+    And I set the field "Type or select..." to "No group"
+    And I click on "Apply filters" "button"
+    Then the following should exist in the "participants" table:
+      | First name / Surname | Groups     |
+      | Student 2            | No groups  |
+      | Student 4            | No groups  |
+      | Student 6            | No groups  |
+      | Student 7            | No groups  |
+      | Student 8            | No groups  |
+
+  @javascript
+  Scenario: Filtering by not a member of "Only see own membership" groups I am a member of should show everyone except me
+    Given I am on the "C1" "enrolled users" page logged in as "student3"
+    When I set the field "Match" in the "Filter 1" "fieldset" to "None"
+    And I set the field "type" to "Groups"
+    And I set the field "Type or select..." to "Only see own membership"
+    And I click on "Apply filters" "button"
+    Then the following should exist in the "participants" table:
+      | First name / Surname | Groups                                                                   |
+      | Student 1            | Visible to everyone/Non-Participation, Visible to everyone/Participation |
+      | Student 2            | No groups                                                                |
+      | Student 4            | No groups                                                                |
+      | Student 5            | Visible to everyone/Non-Participation, Visible to everyone/Participation |
+      | Student 6            | No groups                                                                |
+      | Student 7            | No groups                                                                |
+      | Student 8            | No groups                                                                |
+
+  @javascript
+  Scenario: Filtering by not a member of "No group" should only show users whose memberships I can see
+    Given I am on the "C1" "enrolled users" page logged in as "student3"
+    When I set the field "Match" in the "Filter 1" "fieldset" to "None"
+    And I set the field "type" to "Groups"
+    And I set the field "Type or select..." to "No group"
+    And I click on "Apply filters" "button"
+    Then the following should exist in the "participants" table:
+      | First name / Surname | Groups                                                                   |
+      | Student 1            | Visible to everyone/Non-Participation, Visible to everyone/Participation |
+      | Student 3            | Only see own membership                                                  |
+      | Student 5            | Visible to everyone/Non-Participation, Visible to everyone/Participation |

@@ -18,6 +18,9 @@ information provided here is intended especially for developers.
            'group.svg'
        );
 * Added group/grouping custom fields.
+* groups_get_members_join() now includes visibility checks for group memberships.
+* \core_group\visibility::sql_member_visibility_where() no longer prefixes the returned WHERE statement with AND, to
+  give the calling code greater flexibility about how to use it.
 
 === 4.2 ===
 * `\core_group\visibility` class added to support new `visibility` field in group records. This holds the visibility constants

@@ -338,7 +338,7 @@ function groups_get_all_groups($courseid, $userid=0, $groupingid=0, $fields='g.*
             $visibilityfrom = "LEFT JOIN {groups_members} gm ON gm.groupid = g.id AND gm.userid = ?";
         }
         [$insql, $inparams] = $DB->get_in_or_equal([GROUPS_VISIBILITY_MEMBERS, GROUPS_VISIBILITY_OWN]);
-        $visibilitywhere = "AND (g.visibility = ? OR (g.visibility $insql AND gm.id IS NOT NULL))";
+        $visibilitywhere = " AND (g.visibility = ? OR (g.visibility $insql AND gm.id IS NOT NULL))";
         $params = array_merge(
             $userids,
             $params,
@@ -439,7 +439,7 @@ function groups_get_my_groups() {
     $visibilitywhere = '';
     if (!$viewhidden) {
         $params['novisibility'] = GROUPS_VISIBILITY_NONE;
-        $visibilitywhere = 'AND g.visibility != :novisibility';
+        $visibilitywhere = ' AND g.visibility != :novisibility';
     }
 
     return $DB->get_records_sql("SELECT *
@@ -672,7 +672,7 @@ function groups_get_members($groupid, $fields='u.*', $sort='lastname ASC') {
         // or visibility is OWN and this is their membership.
         list($visibilitywhere, $visibilityparams) = \core_group\visibility::sql_member_visibility_where();
         $params = array_merge($params, $visibilityparams);
-        $where .= $visibilitywhere;
+        $where .= ' AND ' . $visibilitywhere;
     }
 
     $sql = implode(PHP_EOL, [$select, $from, $where, $order]);
@@ -1261,7 +1261,7 @@ function groups_get_members_join($groupids, $useridcolumn, context $context = nu
         $groupids = $groupids ? [$groupids] : [];
     }
 
-    $join = '';
+    $joins = [];
     $where = '';
     $param = [];
 
@@ -1270,14 +1270,31 @@ function groups_get_members_join($groupids, $useridcolumn, context $context = nu
         // Throw an exception if $context is empty or invalid because it's needed to get the users without any group.
         throw new coding_exception('Missing or wrong $context parameter in an attempt to get members without any group');
     }
+    // Can we view hidden groups within a course?
+    [$ualias] = explode('.', $useridcolumn);
+    $viewhidden = false;
+    if (!empty($coursecontext)) {
+        $viewhidden = \core_group\visibility::can_view_all_groups($coursecontext->instanceid);
+    }
 
     // Handle cases where we need to include/exclude users not in any groups.
     if (($nogroupskey = array_search(USERSWITHOUTGROUP, $groupids)) !== false) {
-        // Get members without any group.
-        $join .= "LEFT JOIN (
+        $visibilityjoin = '';
+        $visibilitywhere = '';
+
+        if (!$viewhidden) {
+            $visibilityjoin = 'JOIN {user} u ON u.id = m.userid';
+            [$visibilitywhere, $visibilityparams] = \core_group\visibility::sql_member_visibility_where('g', 'm');
+            $param = array_merge($param, $visibilityparams);
+            $visibilitywhere = 'WHERE ' . $visibilitywhere;
+        }
+        // Get members without any group, or only in groups we cannot see membership of.
+        $joins[] = "LEFT JOIN (
                      SELECT g.courseid, m.groupid, m.userid
                        FROM {groups_members} m
                        JOIN {groups} g ON g.id = m.groupid
+                       {$visibilityjoin}
+                       {$visibilitywhere}
                   ) {$prefix}gm ON ({$prefix}gm.userid = {$useridcolumn} AND {$prefix}gm.courseid = :{$prefix}gcourseid)";
 
         // Join type 'None' when filtering by 'no groups' means match users in at least one group.
@@ -1288,7 +1305,7 @@ function groups_get_members_join($groupids, $useridcolumn, context $context = nu
             $where = "{$prefix}gm.userid IS NULL";
         }
 
-        $param = ["{$prefix}gcourseid" => $coursecontext->instanceid];
+        $param["{$prefix}gcourseid"] = $coursecontext->instanceid;
         unset($groupids[$nogroupskey]);
     }
 
@@ -1302,10 +1319,22 @@ function groups_get_members_join($groupids, $useridcolumn, context $context = nu
                 foreach ($groupids as $groupid) {
                     $gmalias = "{$prefix}gm{$aliaskey}";
                     $aliaskey++;
-                    $join .= "LEFT JOIN {groups_members} {$gmalias}
+                    $joins[] = "LEFT JOIN {groups_members} {$gmalias}
                                      ON ({$gmalias}.userid = {$useridcolumn} AND {$gmalias}.groupid = :{$gmalias}param)";
                     $joinallwheres[] = "{$gmalias}.userid IS NOT NULL";
                     $param["{$gmalias}param"] = $groupid;
+                    if (!$viewhidden) {
+                        $galias = "{$prefix}g{$aliaskey}";
+                        $joins[] = "LEFT JOIN {groups} {$galias} ON {$gmalias}.groupid = {$galias}.id";
+                        [$visibilitywhere, $visibilityparams] = \core_group\visibility::sql_member_visibility_where(
+                            $galias,
+                            $gmalias,
+                            $ualias,
+                            $prefix . $aliaskey . '_'
+                        );
+                        $joinallwheres[] = $visibilitywhere;
+                        $param = array_merge($param, $visibilityparams);
+                    }
                 }
 
                 // Members of all of the specified groups only.
@@ -1323,7 +1352,7 @@ function groups_get_members_join($groupids, $useridcolumn, context $context = nu
                 // Handle matching any of the provided groups (logical OR).
                 list($groupssql, $groupsparams) = $DB->get_in_or_equal($groupids, SQL_PARAMS_NAMED, $prefix);
 
-                $join .= "LEFT JOIN {groups_members} {$prefix}gm2
+                $joins[] = "LEFT JOIN {groups_members} {$prefix}gm2
                                  ON ({$prefix}gm2.userid = {$useridcolumn} AND {$prefix}gm2.groupid {$groupssql})";
                 $param = array_merge($param, $groupsparams);
 
@@ -1335,13 +1364,24 @@ function groups_get_members_join($groupids, $useridcolumn, context $context = nu
                     $where = "({$where} OR {$prefix}gm2.userid IS NOT NULL)";
                 }
 
+                if (!$viewhidden) {
+                    $joins[] = "LEFT JOIN {groups} {$prefix}g2 ON {$prefix}gm2.groupid = {$prefix}g2.id";
+                    [$visibilitywhere, $visibilityparams] = \core_group\visibility::sql_member_visibility_where(
+                        $prefix . 'g2',
+                        $prefix . 'gm2',
+                        $ualias
+                    );
+                    $where .= ' AND ' . $visibilitywhere;
+                    $param = array_merge($param, $visibilityparams);
+                }
+
                 break;
 
             case GROUPS_JOIN_NONE:
                 // Handle matching none of the provided groups (logical NOT).
                 list($groupssql, $groupsparams) = $DB->get_in_or_equal($groupids, SQL_PARAMS_NAMED, $prefix);
 
-                $join .= "LEFT JOIN {groups_members} {$prefix}gm2
+                $joins[] = "LEFT JOIN {groups_members} {$prefix}gm2
                                  ON ({$prefix}gm2.userid = {$useridcolumn} AND {$prefix}gm2.groupid {$groupssql})";
                 $param = array_merge($param, $groupsparams);
 
@@ -1353,11 +1393,22 @@ function groups_get_members_join($groupids, $useridcolumn, context $context = nu
                     $where = "({$where} AND {$prefix}gm2.userid IS NULL)";
                 }
 
+                if (!$viewhidden) {
+                    $joins[] = "LEFT JOIN {groups} {$prefix}g2 ON {$prefix}gm2.groupid = {$prefix}g2.id";
+                    [$visibilitywhere, $visibilityparams] = \core_group\visibility::sql_member_visibility_where(
+                        $prefix . 'g2',
+                        $prefix . 'gm2',
+                        $ualias
+                    );
+                    $where .= ' OR NOT ' . $visibilitywhere;
+                    $param = array_merge($param, $visibilityparams);
+                }
+
                 break;
         }
     }
 
-    return new \core\dml\sql_join($join, $where, $param);
+    return new \core\dml\sql_join(implode("\n", $joins), $where, $param);
 }
 
 /**
@@ -1542,9 +1593,11 @@ function groups_user_groups_visible($course, $userid, $cm = null) {
 function groups_get_groups_members($groupsids, $extrafields=null, $sort='lastname ASC') {
     global $DB;
 
+    $wheres = [];
     $userfieldsapi = \core_user\fields::for_userpic()->including(...($extrafields ?? []));
     $userfields = $userfieldsapi->get_sql('u', false, '', '', false)->selects;
     list($insql, $params) = $DB->get_in_or_equal($groupsids, SQL_PARAMS_NAMED);
+    $wheres[] = "gm.groupid $insql";
 
     $courseids = $DB->get_fieldset_sql("SELECT DISTINCT courseid FROM {groups} WHERE id $insql", $params);
 
@@ -1556,17 +1609,18 @@ function groups_get_groups_members($groupsids, $extrafields=null, $sort='lastnam
         $context = context_course::instance($courseid);
     }
 
-    $visibilitywhere = '';
     if (!has_capability('moodle/course:viewhiddengroups', $context)) {
         list($visibilitywhere, $visibilityparams) = \core_group\visibility::sql_member_visibility_where();
         $params = array_merge($params, $visibilityparams);
+        $wheres[] = $visibilitywhere;
     }
 
+    $where = implode(' AND ', $wheres);
     return $DB->get_records_sql("SELECT $userfields
                                    FROM {user} u
                                         JOIN {groups_members} gm ON u.id = gm.userid
                                         JOIN {groups} g ON g.id = gm.groupid
-                                  WHERE gm.groupid $insql $visibilitywhere
+                                  WHERE {$where}
                                GROUP BY $userfields
                                ORDER BY $sort", $params);
 }

@@ -2950,6 +2950,90 @@ public function test_get_enrolled_sql_userswithoutgroup() {
         get_enrolled_users($systemcontext, '', USERSWITHOUTGROUP);
     }
 
+    /**
+     * Test that enrolled users returns only users in those groups that are
+     * specified, and they are allowed to see members of.
+     *
+     * @covers ::get_enrolled_users
+     * @covers ::get_enrolled_sql
+     * @covers ::get_enrolled_with_capabilities_join
+     * @covers ::get_enrolled_join
+     * @covers ::get_with_capability_join
+     * @covers ::groups_get_members_join
+     * @covers ::get_suspended_userids
+     */
+    public function test_get_enrolled_sql_userswithhiddengroups() {
+        $this->resetAfterTest();
+
+        $course = $this->getDataGenerator()->create_course();
+        $coursecontext = context_course::instance($course->id);
+        $user1 = $this->getDataGenerator()->create_user();
+        $user2 = $this->getDataGenerator()->create_user();
+        $user3 = $this->getDataGenerator()->create_user();
+        $user4 = $this->getDataGenerator()->create_user();
+        $user5 = $this->getDataGenerator()->create_user();
+        $user6 = $this->getDataGenerator()->create_user();
+
+        $this->getDataGenerator()->enrol_user($user1->id, $course->id);
+        $this->getDataGenerator()->enrol_user($user2->id, $course->id);
+        $this->getDataGenerator()->enrol_user($user3->id, $course->id);
+        $this->getDataGenerator()->enrol_user($user4->id, $course->id);
+        $this->getDataGenerator()->enrol_user($user5->id, $course->id);
+        $this->getDataGenerator()->enrol_user($user6->id, $course->id);
+
+        $group1 = $this->getDataGenerator()->create_group([
+                'courseid' => $course->id,
+                'visibility' => GROUPS_VISIBILITY_ALL,
+        ]);
+        groups_add_member($group1, $user1);
+        $group2 = $this->getDataGenerator()->create_group([
+                'courseid' => $course->id,
+                'visibility' => GROUPS_VISIBILITY_MEMBERS,
+        ]);
+        groups_add_member($group2, $user2);
+        groups_add_member($group2, $user5);
+        $group3 = $this->getDataGenerator()->create_group([
+                'courseid' => $course->id,
+                'visibility' => GROUPS_VISIBILITY_OWN,
+        ]);
+        groups_add_member($group3, $user3);
+        groups_add_member($group3, $user6);
+        $group4 = $this->getDataGenerator()->create_group([
+                'courseid' => $course->id,
+                'visibility' => GROUPS_VISIBILITY_NONE,
+        ]);
+        groups_add_member($group4, $user4);
+
+        $groupids = [$group1->id, $group2->id, $group3->id, $group4->id];
+        // User 1 can only see members of Group 1.
+        $this->setUser($user1);
+        $user1groupusers = get_enrolled_users($coursecontext, '', $groupids);
+        $this->assertCount(1, $user1groupusers);
+        $this->assertArrayHasKey($user1->id, $user1groupusers);
+        $this->assertEquals(1, count_enrolled_users($coursecontext, '', $groupids));
+        // User 2 can see all members of Group 1 and Group 2.
+        $this->setUser($user2);
+        $user2groupusers = get_enrolled_users($coursecontext, '', $groupids);
+        $this->assertCount(3, $user2groupusers);
+        $this->assertArrayHasKey($user1->id, $user2groupusers);
+        $this->assertArrayHasKey($user2->id, $user2groupusers);
+        $this->assertArrayHasKey($user5->id, $user2groupusers);
+        $this->assertEquals(3, count_enrolled_users($coursecontext, '', $groupids));
+        // User 3 can see members of Group 1, and themselves in Group 3 but not other members.
+        $this->setUser($user3);
+        $user3groupusers = get_enrolled_users($coursecontext, '', $groupids);
+        $this->assertCount(2, $user3groupusers);
+        $this->assertArrayHasKey($user1->id, $user3groupusers);
+        $this->assertArrayHasKey($user3->id, $user3groupusers);
+        $this->assertEquals(2, count_enrolled_users($coursecontext, '', $groupids));
+        // User 4 can only see members of Group 1.
+        $this->setUser($user4);
+        $user4groupusers = get_enrolled_users($coursecontext, '', $groupids);
+        $this->assertCount(1, $user4groupusers);
+        $this->assertArrayHasKey($user1->id, $user4groupusers);
+        $this->assertEquals(1, count_enrolled_users($coursecontext, '', $groupids));
+    }
+
     public function get_enrolled_sql_provider() {
         return array(
             array(