diff --git a/lib/myprofilelib.php b/lib/myprofilelib.php index 57a90741849..eb6baaa394e 100644 --- a/lib/myprofilelib.php +++ b/lib/myprofilelib.php @@ -299,7 +299,8 @@ function core_myprofile_navigation(core_user\output\myprofile\tree $tree, $user, $groupstr = ''; foreach ($usergroups as $group) { if ($course->groupmode == SEPARATEGROUPS and !$accessallgroups and $user->id != $USER->id) { - if (!groups_is_member($group->id, $user->id)) { + // In separate groups mode, I only have to see the groups shared between both users. + if (!groups_is_member($group->id, $USER->id)) { continue; } } diff --git a/user/lib.php b/user/lib.php index 020ccc11fa4..463228b277a 100644 --- a/user/lib.php +++ b/user/lib.php @@ -400,21 +400,21 @@ function user_get_user_details($user, $course = null, array $userfields = array( // Hidden user field. if ($canviewhiddenuserfields) { $hiddenfields = array(); - // Address, phone1 and phone2 not appears in hidden fields list but require viewhiddenfields capability - // according to user/profile.php. - if (!empty($user->address) && in_array('address', $userfields)) { - $userdetails['address'] = $user->address; - } } else { $hiddenfields = array_flip(explode(',', $CFG->hiddenuserfields)); } - if (!empty($user->phone1) && in_array('phone1', $userfields) && - (in_array('phone1', $showuseridentityfields) or $canviewhiddenuserfields)) { + + if (!empty($user->address) && (in_array('address', $userfields) + && in_array('address', $showuseridentityfields) || $isadmin)) { + $userdetails['address'] = $user->address; + } + if (!empty($user->phone1) && (in_array('phone1', $userfields) + && in_array('phone1', $showuseridentityfields) || $isadmin)) { $userdetails['phone1'] = $user->phone1; } - if (!empty($user->phone2) && in_array('phone2', $userfields) && - (in_array('phone2', $showuseridentityfields) or $canviewhiddenuserfields)) { + if (!empty($user->phone2) && (in_array('phone2', $userfields) + && in_array('phone2', $showuseridentityfields) || $isadmin)) { $userdetails['phone2'] = $user->phone2; } @@ -436,6 +436,10 @@ function user_get_user_details($user, $course = null, array $userfields = array( $userdetails['city'] = $user->city; } + if (in_array('timezone', $userfields) && (!isset($hiddenfields['timezone']) || $isadmin) && $user->timezone) { + $userdetails['timezone'] = $user->timezone; + } + if (in_array('suspended', $userfields) && (!isset($hiddenfields['suspended']) or $isadmin)) { $userdetails['suspended'] = (bool)$user->suspended; } @@ -518,17 +522,25 @@ function user_get_user_details($user, $course = null, array $userfields = array( } } - // If groups are in use and enforced throughout the course, then make sure we can meet in at least one course level group. - if (in_array('groups', $userfields) && !empty($course) && $canaccessallgroups) { - $usergroups = groups_get_all_groups($course->id, $user->id, $course->defaultgroupingid, - 'g.id, g.name,g.description,g.descriptionformat'); - $userdetails['groups'] = array(); - foreach ($usergroups as $group) { - list($group->description, $group->descriptionformat) = - external_format_text($group->description, $group->descriptionformat, - $context->id, 'group', 'description', $group->id); - $userdetails['groups'][] = array('id' => $group->id, 'name' => $group->name, - 'description' => $group->description, 'descriptionformat' => $group->descriptionformat); + // Return user groups. + if (in_array('groups', $userfields) && !empty($course)) { + if ($usergroups = groups_get_all_groups($course->id, $user->id)) { + $userdetails['groups'] = []; + foreach ($usergroups as $group) { + if ($course->groupmode == SEPARATEGROUPS && !$canaccessallgroups && $user->id != $USER->id) { + // In separate groups, I only have to see the groups shared between both users. + if (!groups_is_member($group->id, $USER->id)) { + continue; + } + } + + $userdetails['groups'][] = [ + 'id' => $group->id, + 'name' => format_string($group->name), + 'description' => format_text($group->description, $group->descriptionformat, ['context' => $context]), + 'descriptionformat' => $group->descriptionformat + ]; + } } } // List of courses where the user is enrolled. @@ -560,7 +572,7 @@ function user_get_user_details($user, $course = null, array $userfields = array( } if ($currentuser or has_capability('moodle/user:viewalldetails', $context)) { - $extrafields = ['auth', 'confirmed', 'lang', 'theme', 'timezone', 'mailformat']; + $extrafields = ['auth', 'confirmed', 'lang', 'theme', 'mailformat']; foreach ($extrafields as $extrafield) { if (in_array($extrafield, $userfields) && isset($user->$extrafield)) { $userdetails[$extrafield] = $user->$extrafield; diff --git a/user/tests/behat/view_full_profile.feature b/user/tests/behat/view_full_profile.feature index 5a0071f0922..75e6220d096 100644 --- a/user/tests/behat/view_full_profile.feature +++ b/user/tests/behat/view_full_profile.feature @@ -9,17 +9,19 @@ Feature: Access to full profiles of users | username | firstname | lastname | email | | student1 | Student | 1 | student1@example.com | | student2 | Student | 2 | student2@example.com | - | student3 | Student | 3 | student2@example.com | + | student3 | Student | 3 | student3@example.com | | teacher1 | Teacher | 1 | teacher1@example.com | And the following "courses" exist: - | fullname | shortname | format | - | Course 1 | C1 | topics | - | Course 2 | C2 | topics | + | fullname | shortname | format | groupmode | + | Course 1 | C1 | topics | 0 | + | Course 2 | C2 | topics | 1 | And the following "course enrolments" exist: | user | course | role | | student1 | C1 | student | | student2 | C1 | student | | teacher1 | C1 | editingteacher | + | teacher1 | C2 | editingteacher | + | student1 | C2 | student | | student3 | C2 | student | And the following config values are set as admin: | messaging | 1 | @@ -79,6 +81,42 @@ Feature: Access to full profiles of users When I follow "Profile" in the user menu Then I should see "First access to site" + Scenario: View only shared groups in a course with separate groups forced + Given the following "groups" exist: + | name | course | idnumber | + | Group 1 | C2 | G1 | + | Group 2 | C2 | G2 | + And the following "group members" exist: + | user | group | + | student1 | G1 | + | student3 | G2 | + | teacher1 | G1 | + | teacher1 | G2 | + When I log in as "student3" + And I am on "Course 2" course homepage + And I navigate to course participants + And I follow "Teacher 1" + Then I should see "Group 2" + And I should not see "Group 1" + + Scenario: View all groups in a course with visible groups + Given the following "groups" exist: + | name | course | idnumber | + | Group 1 | C1 | G1 | + | Group 2 | C1 | G2 | + And the following "group members" exist: + | user | group | + | student1 | G1 | + | student2 | G2 | + | teacher1 | G1 | + | teacher1 | G2 | + When I log in as "student1" + And I am on "Course 1" course homepage + And I navigate to course participants + And I follow "Teacher 1" + Then I should see "Group 1" + And I should see "Group 2" + @javascript Scenario: Viewing full profiles of someone with the course contact role Given I log in as "admin" @@ -96,6 +134,11 @@ Feature: Access to full profiles of users When I log in as "student1" And I open messaging And I search for "Student 3" in messaging + Then I should see "Student 3" + And I log out + When I log in as "student2" + And I open messaging + And I search for "Student 3" in messaging Then I should see "No results" @javascript diff --git a/user/tests/externallib_test.php b/user/tests/externallib_test.php index ba50e073324..df55cc4c18f 100644 --- a/user/tests/externallib_test.php +++ b/user/tests/externallib_test.php @@ -356,6 +356,12 @@ class externallib_test extends externallib_advanced_testcase { $this->getDataGenerator()->enrol_user($return->user2->id, $return->course->id, $return->roleid, 'manual'); $this->getDataGenerator()->enrol_user($USER->id, $return->course->id, $return->roleid, 'manual'); + $group1 = $this->getDataGenerator()->create_group(['courseid' => $return->course->id, 'name' => 'G1']); + $group2 = $this->getDataGenerator()->create_group(['courseid' => $return->course->id, 'name' => 'G2']); + + groups_add_member($group1->id, $return->user1->id); + groups_add_member($group2->id, $return->user2->id); + return $return; } @@ -399,6 +405,9 @@ class externallib_test extends externallib_advanced_testcase { // We need to execute the return values cleaning process to simulate the web service server. $enrolledusers = \external_api::clean_returnvalue(core_user_external::get_course_user_profiles_returns(), $enrolledusers); + // Check we get the requested user and that is in a group. + $this->assertCount(1, $enrolledusers); + $this->assertCount(1, $enrolledusers[0]['groups']); foreach($enrolledusers as $enrolleduser) { if ($enrolleduser['username'] == $data->user1->username) { diff --git a/user/tests/userlib_test.php b/user/tests/userlib_test.php index 35b05d00227..67969ff99d6 100644 --- a/user/tests/userlib_test.php +++ b/user/tests/userlib_test.php @@ -912,4 +912,143 @@ class userlib_test extends \advanced_testcase { self::assertSame('5', $got['timezone']); self::assertSame('0', $got['mailformat']); } + + /** + * Test user_get_user_details_permissions. + * @covers ::user_get_user_details + */ + public function test_user_get_user_details_permissions() { + global $CFG; + + $this->resetAfterTest(); + + // Create user and modify user profile. + $teacher = $this->getDataGenerator()->create_user(); + $student1 = $this->getDataGenerator()->create_user(['idnumber' => 'user1id', 'city' => 'Barcelona', 'address' => 'BCN 1B']); + $student2 = $this->getDataGenerator()->create_user(); + $student1fullname = fullname($student1); + + $course = $this->getDataGenerator()->create_course(); + $coursecontext = \context_course::instance($course->id); + $this->getDataGenerator()->enrol_user($teacher->id, $course->id); + $this->getDataGenerator()->enrol_user($student1->id, $course->id); + $this->getDataGenerator()->enrol_user($student2->id, $course->id); + $this->getDataGenerator()->role_assign('teacher', $teacher->id, $coursecontext->id); + $this->getDataGenerator()->role_assign('student', $student1->id, $coursecontext->id); + $this->getDataGenerator()->role_assign('student', $student2->id, $coursecontext->id); + + accesslib_clear_all_caches_for_unit_testing(); + + // Get student details as a user with super system capabilities. + $result = user_get_user_details($student1, $course); + $this->assertEquals($student1->id, $result['id']); + $this->assertEquals($student1fullname, $result['fullname']); + $this->assertEquals($course->id, $result['enrolledcourses'][0]['id']); + + $this->setUser($student2); + + // Get student details with required fields. + $result = user_get_user_details($student1, $course, array('id', 'fullname', 'timezone', 'city', 'address', 'idnumber')); + $this->assertCount(4, $result); // Ensure address (never returned), idnumber (identity field) are not returned here. + $this->assertEquals($student1->id, $result['id']); + $this->assertEquals($student1fullname, $result['fullname']); + $this->assertEquals($student1->timezone, $result['timezone']); + $this->assertEquals($student1->city, $result['city']); + + // Set new identity fields and hidden fields and try to retrieve them without permission. + $CFG->showuseridentity = $CFG->showuseridentity . ',idnumber'; + $CFG->hiddenuserfields = 'city'; + $result = user_get_user_details($student1, $course, array('id', 'fullname', 'timezone', 'city', 'address', 'idnumber')); + $this->assertCount(3, $result); // Ensure address, city and idnumber are not returned here. + $this->assertEquals($student1->id, $result['id']); + $this->assertEquals($student1fullname, $result['fullname']); + $this->assertEquals($student1->timezone, $result['timezone']); + + // Now, teacher should have permission to see the idnumber and city fields. + $this->setUser($teacher); + $result = user_get_user_details($student1, $course, array('id', 'fullname', 'timezone', 'city', 'address', 'idnumber')); + $this->assertCount(5, $result); // Ensure address is not returned here. + $this->assertEquals($student1->id, $result['id']); + $this->assertEquals($student1fullname, $result['fullname']); + $this->assertEquals($student1->timezone, $result['timezone']); + $this->assertEquals($student1->idnumber, $result['idnumber']); + $this->assertEquals($student1->city, $result['city']); + + // And admins can see anything. + $this->setAdminUser(); + $result = user_get_user_details($student1, $course, array('id', 'fullname', 'timezone', 'city', 'address', 'idnumber')); + $this->assertCount(6, $result); + $this->assertEquals($student1->id, $result['id']); + $this->assertEquals($student1fullname, $result['fullname']); + $this->assertEquals($student1->timezone, $result['timezone']); + $this->assertEquals($student1->idnumber, $result['idnumber']); + $this->assertEquals($student1->city, $result['city']); + $this->assertEquals($student1->address, $result['address']); + } + + /** + * Test user_get_user_details_groups. + * @covers ::user_get_user_details + */ + public function test_user_get_user_details_groups() { + $this->resetAfterTest(); + + // Create user and modify user profile. + $teacher = $this->getDataGenerator()->create_user(); + $student1 = $this->getDataGenerator()->create_user(['idnumber' => 'user1id', 'city' => 'Barcelona', 'address' => 'BCN 1B']); + $student2 = $this->getDataGenerator()->create_user(); + + $course = $this->getDataGenerator()->create_course(); + $coursecontext = \context_course::instance($course->id); + $this->getDataGenerator()->enrol_user($teacher->id, $course->id); + $this->getDataGenerator()->enrol_user($student1->id, $course->id); + $this->getDataGenerator()->enrol_user($student2->id, $course->id); + $this->getDataGenerator()->role_assign('teacher', $teacher->id, $coursecontext->id); + $this->getDataGenerator()->role_assign('student', $student1->id, $coursecontext->id); + $this->getDataGenerator()->role_assign('student', $student2->id, $coursecontext->id); + + $group1 = $this->getDataGenerator()->create_group(['courseid' => $course->id, 'name' => 'G1']); + $group2 = $this->getDataGenerator()->create_group(['courseid' => $course->id, 'name' => 'G2']); + + // Each student in one group but teacher in two. + groups_add_member($group1->id, $student1->id); + groups_add_member($group1->id, $teacher->id); + groups_add_member($group2->id, $student2->id); + groups_add_member($group2->id, $teacher->id); + + accesslib_clear_all_caches_for_unit_testing(); + + // A student can see other users groups when separate groups are not forced. + $this->setUser($student2); + + // Get student details with groups. + $result = user_get_user_details($student1, $course, array('id', 'fullname', 'groups')); + $this->assertCount(3, $result); + $this->assertEquals($group1->id, $result['groups'][0]['id']); + + // Teacher is in two different groups. + $result = user_get_user_details($teacher, $course, array('id', 'fullname', 'groups')); + + // Order by group id. + usort($result['groups'], function($a, $b) { + return $a['id'] - $b['id']; + }); + + $this->assertCount(3, $result); + $this->assertCount(2, $result['groups']); + $this->assertEquals($group1->id, $result['groups'][0]['id']); + $this->assertEquals($group2->id, $result['groups'][1]['id']); + + // Change to separate groups. + $course->groupmode = SEPARATEGROUPS; + $course->groupmodeforce = true; + update_course($course); + + // Teacher is in two groups but I can only see the one shared with me. + $result = user_get_user_details($teacher, $course, array('id', 'fullname', 'groups')); + + $this->assertCount(3, $result); + $this->assertCount(1, $result['groups']); + $this->assertEquals($group2->id, $result['groups'][0]['id']); + } }