moodle/moodle
3
0
Fork 0
mirror of https://github.com/moodle/moodle.git synced 2025-08-07 09:56:38 +02:00
Code Issues Projects Releases Packages Wiki Activity Actions

MDL-79213 groups: Add visibility checks in groups_get_members_join()

Browse source 
Group visibility was not taken into account when
generating SQL for getting enrolled users restricted
to a list of groups. This may have allowed users to
infer membership of groups they were not allowed to
see members of.
This commit is contained in:
Mark Johnson 2023-08-30 16:14:20 +01:00 • committed by Jenkins
parent f5f6ce375e
commit b0bb97ee3b
5 changed files with 242 additions and 45 deletions
Download patch file Download diff file Expand all files Collapse all files

39
group/classes/visibility.php
View file

@ -123,23 +123,28 @@ class visibility {
* @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
$where = "(
{$groupsalias}.visibility = :{$paramprefix}all
OR ($memberssql)
OR ({$groupsalias}.visibility = :own AND {$useralias}.id = :currentuser2)
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 @@ class visibility {
*
* @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];

77
group/tests/behat/private_groups.feature
View file

@ -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 |

3
group/upgrade.txt
View file

@ -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

80
lib/grouplib.php
View file

@ -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);
}

84
lib/tests/accesslib_test.php
View file

@ -2950,6 +2950,90 @@ class accesslib_test extends advanced_testcase {
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(