From 2d51b3dd977368bc6165ea7856e1afccc24d5f02 Mon Sep 17 00:00:00 2001 From: Mark Johnson Date: Wed, 21 Feb 2024 14:12:05 +0000 Subject: [PATCH 1/2] MDL-79174 groups: Add includehidden parameter This allows groups_get_user_groups to return groups with GROUP_VISIBILITY_NONE that the specified user belongs to. The parameter is false by default, and should be use with care as membership of these groups should not be revealed to the user. --- lib/grouplib.php | 9 ++++++--- lib/tests/grouplib_test.php | 9 +++++++++ lib/upgrade.txt | 5 +++++ 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/lib/grouplib.php b/lib/grouplib.php index 1c251f3f5df..d4c4ade52a6 100644 --- a/lib/grouplib.php +++ b/lib/grouplib.php @@ -447,9 +447,12 @@ function groups_get_my_groups() { * @category group * @param int $courseid * @param int $userid $USER if not specified + * @param bool $includehidden Include groups with GROUP_VISIBILITY_NONE that the user is a member of, but is not allowed to see + * themselves. Use this parameter with care - it is the responsibility of the calling code to ensure these groups are not exposed + * to the user, as this could have privacy implications. * @return array Array[groupingid][groupid] including grouping id 0 which means all groups */ -function groups_get_user_groups($courseid, $userid=0) { +function groups_get_user_groups(int $courseid, int $userid = 0, bool $includehidden = false): array { global $USER, $DB; if (empty($courseid)) { @@ -461,7 +464,7 @@ function groups_get_user_groups($courseid, $userid=0) { } $usergroups = false; - $viewhidden = has_capability('moodle/course:viewhiddengroups', context_course::instance($courseid)); + $viewhidden = $includehidden || has_capability('moodle/course:viewhiddengroups', context_course::instance($courseid)); $viewall = \core_group\visibility::can_view_all_groups($courseid); $cache = cache::make('core', 'user_group_groupings'); @@ -473,7 +476,7 @@ function groups_get_user_groups($courseid, $userid=0) { if ($usergroups === false) { - $sql = "SELECT g.id, g.courseid, gg.groupingid + $sql = "SELECT g.id, g.courseid, gg.groupingid, g.visibility FROM {groups} g JOIN {groups_members} gm ON gm.groupid = g.id LEFT JOIN {groupings_groups} gg ON gg.groupid = g.id diff --git a/lib/tests/grouplib_test.php b/lib/tests/grouplib_test.php index ea02fe88a85..7045b2fb4ec 100644 --- a/lib/tests/grouplib_test.php +++ b/lib/tests/grouplib_test.php @@ -2109,6 +2109,12 @@ class grouplib_test extends \advanced_testcase { // Own groups - should see all groups except group with visibility::NONE. $usergroups1 = groups_get_user_groups($course->id, $users[1]->id); $this->assertEquals([$groups['all']->id, $groups['members']->id, $groups['own']->id], $usergroups1[0]); + // Own groups including hidden - should see all groups. + $usergroups1hidden = groups_get_user_groups($course->id, $users[1]->id, true); + $this->assertEquals( + [$groups['all']->id, $groups['members']->id, $groups['own']->id, $groups['none']->id], + $usergroups1hidden[0] + ); // Fellow member of a group with visiblity::MEMBERS. Should see that group. $usergroups2 = groups_get_user_groups($course->id, $users[2]->id); $this->assertEquals([$groups['members']->id], $usergroups2[0]); @@ -2118,6 +2124,9 @@ class grouplib_test extends \advanced_testcase { // Fellow member of a group with visiblity::NONE. Should not see that group. $usergroups4 = groups_get_user_groups($course->id, $users[4]->id); $this->assertEmpty($usergroups4[0]); + // Fellow member of a group with visiblity::NONE including hidden. Should see that group. + $usergroups4hidden = groups_get_user_groups($course->id, $users[4]->id, true); + $this->assertEquals([$groups['none']->id], $usergroups4hidden[0]); // Run as a user with viewhiddengroups. Should see all group memberships for each member. $this->setUser($users[5]); diff --git a/lib/upgrade.txt b/lib/upgrade.txt index 5080744ea45..fbda37d6886 100644 --- a/lib/upgrade.txt +++ b/lib/upgrade.txt @@ -1,6 +1,11 @@ This files describes API changes in core libraries and APIs, information provided here is intended especially for developers. +=== 4.2.7 === +* groups_get_user_groups() in grouplib has a new $includehidden paramater, which will return groups for user even if they have + GROUP_VISIBILITY_NONE. This is false by default, and should be used with care. The calling code must ensure that these groups + are not exposed to the user as this may be a privacy issue. + === 4.2.6 === * The `html_writer::select_time` method has a `$timezone` parameter to use when formatting the time parameter From b47dcbb0cd00d1d83a1242561394f296eb477eb1 Mon Sep 17 00:00:00 2001 From: Mark Johnson Date: Wed, 21 Feb 2024 14:16:40 +0000 Subject: [PATCH 2/2] MDL-79174 availability: Allow group condition to use hidden groups Previously, groups with GROUP_VISIBILITY_NONE could be selected for an availability condition, but as the group API did not return a user's own groups with this visibility, the condition's is_available check always failed. This change uses the new $includehidden parameter to get all of a user's groups from groups_get_user_groups when evaluating the condition, so these groups work as expected. Due to the enforced hiding of conditions containing these groups, they will never be seen by the user. --- availability/classes/info.php | 28 +++++++++++ .../condition/group/classes/condition.php | 2 +- .../tests/behat/availability_group.feature | 46 +++++++++++++++++++ .../condition/group/tests/condition_test.php | 3 +- 4 files changed, 77 insertions(+), 2 deletions(-) diff --git a/availability/classes/info.php b/availability/classes/info.php index b54d919a615..ffab7b25c4f 100644 --- a/availability/classes/info.php +++ b/availability/classes/info.php @@ -49,6 +49,9 @@ abstract class info { /** @var tree Availability configuration, decoded from JSON; null if unset */ protected $availabilitytree; + /** @var array The groups the current user belongs to. */ + protected $groups; + /** @var array|null Array of information about current restore if any */ protected static $restoreinfo = null; @@ -65,6 +68,7 @@ abstract class info { $this->course = $course; $this->visible = (bool)$visible; $this->availability = $availability; + $this->groups = null; } /** @@ -800,4 +804,28 @@ abstract class info { } return false; } + + /** + * Returns groups that the current user belongs to on the course. Note: If not already + * available, this may make a database query. + * + * This will include groups the user is not allowed to see themselves, so check visibility + * before displaying groups to the user. + * + * @param int $groupingid Grouping ID or 0 (default) for all groups + * @return int[] Array of int (group id) => int (same group id again); empty array if none + */ + public function get_groups(int $groupingid = 0): array { + global $USER; + if (is_null($this->groups)) { + $allgroups = groups_get_user_groups($this->course->id, $USER->id, true); + $this->groups = $allgroups; + } else { + $allgroups = $this->groups; + } + if (!isset($allgroups[$groupingid])) { + return []; + } + return $allgroups[$groupingid]; + } } diff --git a/availability/condition/group/classes/condition.php b/availability/condition/group/classes/condition.php index fd247e26053..feeae1c6ce3 100644 --- a/availability/condition/group/classes/condition.php +++ b/availability/condition/group/classes/condition.php @@ -71,7 +71,7 @@ class condition extends \core_availability\condition { $allow = true; if (!has_capability('moodle/site:accessallgroups', $context, $userid)) { // Get all groups the user belongs to. - $groups = $info->get_modinfo()->get_groups(); + $groups = $info->get_groups(); if ($this->groupid) { $allow = in_array($this->groupid, $groups); } else { diff --git a/availability/condition/group/tests/behat/availability_group.feature b/availability/condition/group/tests/behat/availability_group.feature index 57e4d4cc1a6..4d9e1a97c31 100644 --- a/availability/condition/group/tests/behat/availability_group.feature +++ b/availability/condition/group/tests/behat/availability_group.feature @@ -108,3 +108,49 @@ Feature: availability_group When I am on the "C1" "Course" page logged in as "student1" Then I should see "Not available unless: You belong to G-One" And I should not see "G-Un" + + @javascript + Scenario: Condition using a hidden group + And the following "groups" exist: + | name | course | idnumber | visibility | + | Hidden Group | C1 | GA | 3 | + And I log in as "teacher1" + And I add a page activity to course "Course 1" section "1" + And I expand all fieldsets + + # Page P1 any group. + And I am on the "P1" "page activity editing" page + And I expand all fieldsets + And I click on "Add restriction..." "button" + Then "Group" "button" should exist in the "Add restriction..." "dialogue" + Given I click on "Group" "button" in the "Add restriction..." "dialogue" + And I set the field "Group" to "(Any group)" + And I click on ".availability-item .availability-eye img" "css_element" + And I click on "Save and return to course" "button" + + # Page P2 with hidden group. + And I am on the "P2" "page activity editing" page + And I expand all fieldsets + And I click on "Add restriction..." "button" + And I click on "Group" "button" in the "Add restriction..." "dialogue" + And I set the field "Group" to "Hidden Group" + And I click on "Save and return to course" "button" + + # Log back in as student. + When I am on the "Course 1" "course" page logged in as "student1" + + # No pages should appear yet. + Then I should not see "P1" in the "region-main" "region" + And I should not see "P2" in the "region-main" "region" + And I should not see "Hidden Group" + + # Add to groups and log out/in again. + Given the following "group members" exist: + | user | group | + | student1 | GA | + And I am on "Course 1" course homepage + + # P1 (any groups) and P2 should show. The user should not see the hidden group mentioned anywhere. + Then I should see "P1" in the "region-main" "region" + And I should see "P2" in the "region-main" "region" + And I should not see "Hidden Group" diff --git a/availability/condition/group/tests/condition_test.php b/availability/condition/group/tests/condition_test.php index eb221ea9234..f20a3d9a554 100644 --- a/availability/condition/group/tests/condition_test.php +++ b/availability/condition/group/tests/condition_test.php @@ -49,6 +49,7 @@ class condition_test extends \advanced_testcase { $course = $generator->create_course(); $user = $generator->create_user(); $generator->enrol_user($user->id, $course->id); + $this->setUser($user); $info = new \core_availability\mock_info($course, $user->id); // Make 2 test groups, one in a grouping and one not. @@ -70,7 +71,7 @@ class condition_test extends \advanced_testcase { // Add user to groups and refresh cache. groups_add_member($group1, $user); groups_add_member($group2, $user); - get_fast_modinfo($course->id, 0, true); + $info = new \core_availability\mock_info($course, $user->id); // Recheck. $this->assertTrue($cond->is_available(false, $info, true, $user->id));