diff --git a/group/classes/visibility.php b/group/classes/visibility.php index bd032bf80a0e6..fe3e43c66e13e 100644 --- a/group/classes/visibility.php +++ b/group/classes/visibility.php @@ -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]; diff --git a/group/tests/behat/private_groups.feature b/group/tests/behat/private_groups.feature index 3a7e4f246836b..604012fe93a84 100644 --- a/group/tests/behat/private_groups.feature +++ b/group/tests/behat/private_groups.feature @@ -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 | diff --git a/group/upgrade.txt b/group/upgrade.txt index 32ca37a7409bb..f176eb310c811 100644 --- a/group/upgrade.txt +++ b/group/upgrade.txt @@ -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 diff --git a/lib/grouplib.php b/lib/grouplib.php index 181b508fb4368..577f73f2d623d 100644 --- a/lib/grouplib.php +++ b/lib/grouplib.php @@ -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); } diff --git a/lib/tests/accesslib_test.php b/lib/tests/accesslib_test.php index 9d23ca8c4fa56..c610d8e58fe40 100644 --- a/lib/tests/accesslib_test.php +++ b/lib/tests/accesslib_test.php @@ -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(