Skip to content

Commit

Permalink
MDL-79213 groups: Add visibility checks in groups_get_members_join()
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@marxjohnson
marxjohnson authored and Jenkins committed Oct 4, 2023
1 parent f5f6ce3 commit b0bb97e
Show file tree
Hide file tree
Showing 5 changed files with 242 additions and 45 deletions.
43 changes: 26 additions & 17 deletions group/classes/visibility.php
View file
Expand Up @@ -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];
Expand All @@ -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];
Expand Down
77 changes: 62 additions & 15 deletions group/tests/behat/private_groups.feature
View file
Expand Up @@ -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 |
Expand All @@ -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 |
Expand All @@ -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 |
Expand All @@ -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 |
Expand All @@ -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 |
Expand All @@ -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 changes: 3 additions & 0 deletions group/upgrade.txt
View file
Expand Up @@ -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
Expand Down

0 comments on commit b0bb97e

Please sign in to comment.