From ce706292bfe83617a887dad7a36cebe6de2e4e0c 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 | 3 +++ 3 files changed, 18 insertions(+), 3 deletions(-) diff --git a/lib/grouplib.php b/lib/grouplib.php index 63a65da6626d0..95758be7480c4 100644 --- a/lib/grouplib.php +++ b/lib/grouplib.php @@ -457,9 +457,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)) { @@ -471,7 +474,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'); @@ -483,7 +486,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 ea02fe88a858e..7045b2fb4ecea 100644 --- a/lib/tests/grouplib_test.php +++ b/lib/tests/grouplib_test.php @@ -2109,6 +2109,12 @@ public function test_get_user_groups_with_visibility() { // 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 @@ public function test_get_user_groups_with_visibility() { // 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 a5f2a492698a0..6c61dfcc491b7 100644 --- a/lib/upgrade.txt +++ b/lib/upgrade.txt @@ -55,6 +55,9 @@ information provided here is intended especially for developers. * The nocache option for format_text has been removed. It was deprecated in Moodle 2.3. * The set_heading() method has a new parameter, $clean, to define whether the heading should be cleaned or not when no formatting is applied. +* 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.3 === From 238ea3d1a39e08970ebbf3e4825dfc7341f2c1c7 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 b54d919a61589..ffab7b25c4f2d 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 @@ public function __construct($course, $visible, $availability) { $this->course = $course; $this->visible = (bool)$visible; $this->availability = $availability; + $this->groups = null; } /** @@ -800,4 +804,28 @@ public static function completion_value_used($course, $cmid) { } 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 fd247e26053df..feeae1c6ce3f3 100644 --- a/availability/condition/group/classes/condition.php +++ b/availability/condition/group/classes/condition.php @@ -71,7 +71,7 @@ public function is_available($not, \core_availability\info $info, $grabthelot, $ $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 57e4d4cc1a6d5..4d9e1a97c31b1 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 eb221ea923421..f20a3d9a554bf 100644 --- a/availability/condition/group/tests/condition_test.php +++ b/availability/condition/group/tests/condition_test.php @@ -49,6 +49,7 @@ public function test_usage() { $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 @@ public function test_usage() { // 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));