diff --git a/lang/en/role.php b/lang/en/role.php index 915f240d8c371..0a7d0408c20a5 100644 --- a/lang/en/role.php +++ b/lang/en/role.php @@ -495,6 +495,7 @@ $string['user:viewdetails'] = 'View user profiles'; $string['user:viewhiddendetails'] = 'View hidden details of users'; $string['user:viewlastip'] = 'View user last ip address'; +$string['user:viewprofilepics'] = 'View user profile pictures (if force login enabled)'; $string['user:viewuseractivitiesreport'] = 'See user activity reports'; $string['user:viewusergrades'] = 'View user grades'; $string['roleresetdefaults'] = 'Defaults'; diff --git a/lib/classes/output/user_picture.php b/lib/classes/output/user_picture.php index 8da6537c5814a..597bb5dd4cd37 100644 --- a/lib/classes/output/user_picture.php +++ b/lib/classes/output/user_picture.php @@ -212,6 +212,45 @@ public static function unalias( return $return; } + /** + * Checks if the current user is permitted to view user profile images. + * + * This is based on the forcelogin and forceloginforprofileimage config settings, and the + * moodle/user:viewprofilepics capability. + * + * Logged-in users are allowed to view their own profile image regardless of capability. + * + * @param int $imageuserid User id of profile image being viewed + * @return bool True if current user can view profile images + */ + public static function allow_view(int $imageuserid): bool { + global $CFG, $USER; + + // Not allowed to view profile images if forcelogin is enabled and not logged in (guest + // allowed), or forceloginforprofileimage is enabled and not logged in or guest. + if ( + (!empty($CFG->forcelogin) && !isloggedin()) || + (!empty($CFG->forceloginforprofileimage) && (!isloggedin() || isguestuser())) + ) { + return false; + } + + // Unless one of the forcelogin options is enabled, users can download profile pics + // without login, so the capability should not be checked as it might lead to a + // false sense of security (i.e. you log in as a test user, the HTML page doesn't + // show the picture, but they can still access it if they just log out). + // When the capability is checked, use system context for performance (if we check at + // user level, pages that show a lot of user pictures will individually load a lot of + // user contexts). + if ((!empty($CFG->forcelogin) || !empty($CFG->forceloginforprofileimage)) && + $USER->id != $imageuserid && + !has_capability('moodle/user:viewprofilepics', \context_system::instance())) { + return false; + } + + return true; + } + /** * Works out the URL for the users picture. * @@ -253,13 +292,7 @@ public function get_url( $defaulturl = $renderer->image_url('u/' . $filename); // Default image. - if ( - (!empty($CFG->forcelogin) && !isloggedin()) || - (!empty($CFG->forceloginforprofileimage) && (!isloggedin() || isguestuser())) - ) { - // Protect images if login required and not logged in; - // also if login is required for profile images and is not logged in or guest - // do not use require_login() because it is expensive and not suitable here anyway. + if (!self::allow_view($this->user->id)) { return $defaulturl; } diff --git a/lib/db/access.php b/lib/db/access.php index eda798dd5b1ed..3284a9a7d6f6c 100644 --- a/lib/db/access.php +++ b/lib/db/access.php @@ -529,6 +529,15 @@ ) ), + 'moodle/user:viewprofilepics' => [ + 'captype' => 'read', + 'contextlevel' => CONTEXT_SYSTEM, + 'archetypes' => [ + 'guest' => CAP_ALLOW, + 'user' => CAP_ALLOW, + ], + ], + 'moodle/user:viewalldetails' => array( 'riskbitmask' => RISK_PERSONAL, 'captype' => 'read', diff --git a/lib/filelib.php b/lib/filelib.php index 3a4eb1577f26e..5aebbe6ecc555 100644 --- a/lib/filelib.php +++ b/lib/filelib.php @@ -4734,8 +4734,7 @@ function file_pluginfile($relativepath, $forcedownload, $preview = null, $offlin $filename = 'f1'; } - if ((!empty($CFG->forcelogin) and !isloggedin()) || - (!empty($CFG->forceloginforprofileimage) && (!isloggedin() || isguestuser()))) { + if (!\core\output\user_picture::allow_view($context->instanceid)) { // protect images if login required and not logged in; // also if login is required for profile images and is not logged in or guest // do not use require_login() because it is expensive and not suitable here anyway diff --git a/lib/tests/behat/profile_pic_access.feature b/lib/tests/behat/profile_pic_access.feature new file mode 100644 index 0000000000000..56e5719c892d8 --- /dev/null +++ b/lib/tests/behat/profile_pic_access.feature @@ -0,0 +1,72 @@ +@core @_file_upload +Feature: Profile picture access + In order to enable precise security control and meet legal requirements + As site administrators + We should be able to prevent certain users from viewing profile pictures + + Background: + Given the following "users" exist: + | username | firstname | lastname | + | student1 | Alice | in Wonderland | + | student2 | Bob | a Job Week | + And the following "courses" exist: + | shortname | + | C1 | + And the following "course enrolments" exist: + | user | course | role | + | student1 | C1 | student | + | student2 | C1 | student | + And the following "activity" exists: + | course | C1 | + | activity | forum | + | name | TestForum | + | idnumber | forum1 | + And the following "mod_forum > discussions" exist: + | user | forum | name | message | timemodified | + | student1 | forum1 | Post1 | This is the first post | ##now -1 second## | + And the following "roles" exist: + | shortname | + | dangerous | + And the following "role capability" exists: + | role | dangerous | + | moodle/user:viewprofilepics | prohibit | + And I am on the "Profile editing" page logged in as "student1" + And I upload "/course/tests/fixtures/image.jpg" file to "New picture" filemanager + And I set the field "Picture description" to "MyPic" + And I press "Update profile" + + @javascript + Scenario: Users can view pictures on forum page when permitted + When I am on the "forum1" "forum activity" page logged in as "student2" + # By default you can see user pics. + And ".discussion-list img.userpicture[src*='user/icon']" "css_element" should be visible + # Even if you don't have the capability, you can still see them... + And the following "system role assigns" exist: + | user | role | contextlevel | + | student2 | dangerous | System | + And I reload the page + And ".discussion-list img.userpicture[src*='user/icon']" "css_element" should be visible + # ...unless forcelogin is on, when the system kicks in and hides it. + And the following config values are set as admin: + | forcelogin | 1 | + And I reload the page + Then ".discussion-list img.userpicture[src*='user/icon']" "css_element" should not exist + + @javascript + Scenario: Users can view pictures on profile page when permitted + When I am on the "forum1" "forum activity" page logged in as "student2" + And I follow "Post1" + And I follow "Alice in Wonderland" + # By default you can see user pics. + And ".page-header-image img.userpicture[src*='user/icon']" "css_element" should be visible + # Even if you don't have the capability, you can still see them... + And the following "system role assigns" exist: + | user | role | contextlevel | + | student2 | dangerous | System | + And I reload the page + And ".page-header-image img.userpicture[src*='user/icon']" "css_element" should be visible + # ...unless forcelogin is on, when the system kicks in and hides it. + And the following config values are set as admin: + | forcelogin | 1 | + And I reload the page + Then ".page-header-image img.userpicture[src*='user/icon']" "css_element" should not exist diff --git a/lib/tests/output/user_picture_test.php b/lib/tests/output/user_picture_test.php new file mode 100644 index 0000000000000..1ce6c64d62353 --- /dev/null +++ b/lib/tests/output/user_picture_test.php @@ -0,0 +1,116 @@ +. + +namespace core\output; + +/** + * Unit tests for the user_picture class. + * + * @package core + * @copyright 2024 The Open University + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @covers \core\output\user_picture + */ +final class user_picture_test extends \advanced_testcase { + /** + * Tests {@see user_picture::allow_view()} for a not-logged-in request. + */ + public function test_allow_view_not_logged_in(): void { + global $DB; + + $this->resetAfterTest(); + + $adminid = $DB->get_field('user', 'id', ['username' => 'admin'], MUST_EXIST); + + // Default config allows user pictures when not logged in. + $this->assertTrue(user_picture::allow_view($adminid)); + + // Not allowed with either or both forcelogin options. + set_config('forcelogin', 1); + $this->assertFalse(user_picture::allow_view($adminid)); + set_config('forcelogin', 0); + set_config('forceloginforprofileimage', 1); + $this->assertFalse(user_picture::allow_view($adminid)); + set_config('forcelogin', 1); + $this->assertFalse(user_picture::allow_view($adminid)); + } + + /** + * Tests {@see user_picture::allow_view()} for a guest user request. + */ + public function test_allow_view_guest(): void { + global $DB; + + $this->resetAfterTest(); + $this->setGuestUser(); + + $adminid = $DB->get_field('user', 'id', ['username' => 'admin'], MUST_EXIST); + + // Default config allows user pictures for guests. + $this->assertTrue(user_picture::allow_view($adminid)); + + // Not allowed with forceloginforprofileimage. + set_config('forceloginforprofileimage', 1); + $this->assertFalse(user_picture::allow_view($adminid)); + + // Allowed by default with just forcelogin. + set_config('forceloginforprofileimage', 0); + set_config('forcelogin', 1); + $this->assertTrue(user_picture::allow_view($adminid)); + + // But would not be allowed if we change guest role to remove capability. + $guestroleid = $DB->get_field('role', 'id', ['shortname' => 'guest'], MUST_EXIST); + assign_capability('moodle/user:viewprofilepics', CAP_INHERIT, $guestroleid, + \context_system::instance()->id, true); + $this->assertFalse(user_picture::allow_view($adminid)); + } + + /** + * Tests {@see user_picture::allow_view()} for a logged in user. + */ + public function test_allow_view_user(): void { + global $DB; + + $this->resetAfterTest(); + + $adminid = $DB->get_field('user', 'id', ['username' => 'admin'], MUST_EXIST); + + $generator = self::getDataGenerator(); + $user = $generator->create_user(); + $this->setUser($user); + + // Default config allows user pictures. + $this->assertTrue(user_picture::allow_view($adminid)); + + // Also allowed with either or both forcelogin option, because they are logged in. + set_config('forcelogin', 1); + $this->assertTrue(user_picture::allow_view($adminid)); + set_config('forcelogin', 0); + set_config('forceloginforprofileimage', 1); + $this->assertTrue(user_picture::allow_view($adminid)); + set_config('forcelogin', 1); + $this->assertTrue(user_picture::allow_view($adminid)); + + // But would not be allowed if we change user role to remove capability. + $userroleid = $DB->get_field('role', 'id', ['shortname' => 'user'], MUST_EXIST); + assign_capability('moodle/user:viewprofilepics', CAP_INHERIT, $userroleid, + \context_system::instance()->id, true); + $this->assertFalse(user_picture::allow_view($adminid)); + + // Except you are still allowed to view your own user picture. + $this->assertTrue(user_picture::allow_view($user->id)); + } +}