diff --git a/h5p/classes/helper.php b/h5p/classes/helper.php index 8afb63b9507..0dca112db73 100644 --- a/h5p/classes/helper.php +++ b/h5p/classes/helper.php @@ -26,6 +26,7 @@ namespace core_h5p; use context_system; use core_h5p\local\library\autoloader; +use core_user; defined('MOODLE_INTERNAL') || die(); @@ -176,46 +177,50 @@ class helper { /** * Checks if the author of the .h5p file is "trustable". If the file hasn't been uploaded by a user with the - * required capability, the content won't be deployed. + * required capability, the content won't be deployed, unless the user has been deleted, in this + * case we check the capability against current user. * * @param stored_file $file The .h5p file to be deployed * @return bool Returns true if the file can be deployed, false otherwise. */ public static function can_deploy_package(\stored_file $file): bool { - if (null === $file->get_userid()) { + $userid = $file->get_userid(); + if (null === $userid) { // If there is no userid, it is owned by the system. return true; } $context = \context::instance_by_id($file->get_contextid()); - if (has_capability('moodle/h5p:deploy', $context, $file->get_userid())) { - return true; + $fileuser = core_user::get_user($userid); + if (empty($fileuser) || $fileuser->deleted) { + $userid = null; } - - return false; + return has_capability('moodle/h5p:deploy', $context, $userid); } /** * Checks if the content-type libraries can be upgraded. * The H5P content-type libraries can only be upgraded if the author of the .h5p file can manage content-types or if all the - * content-types exist, to avoid users without the required capability to upload malicious content. + * content-types exist, to avoid users without the required capability to upload malicious content. If user has been deleted + * we check against current user. * * @param stored_file $file The .h5p file to be deployed * @return bool Returns true if the content-type libraries can be created/updated, false otherwise. */ public static function can_update_library(\stored_file $file): bool { - if (null === $file->get_userid()) { + $userid = $file->get_userid(); + if (null === $userid) { // If there is no userid, it is owned by the system. return true; } - // Check if the owner of the .h5p file has the capability to manage content-types. $context = \context::instance_by_id($file->get_contextid()); - if (has_capability('moodle/h5p:updatelibraries', $context, $file->get_userid())) { - return true; + $fileuser = core_user::get_user($userid); + if (empty($fileuser) || $fileuser->deleted) { + $userid = null; } - return false; + return has_capability('moodle/h5p:updatelibraries', $context, $userid); } /** diff --git a/h5p/tests/behat/h5p_deployment.feature b/h5p/tests/behat/h5p_deployment.feature new file mode 100644 index 00000000000..be88ace9d95 --- /dev/null +++ b/h5p/tests/behat/h5p_deployment.feature @@ -0,0 +1,64 @@ +@core_h5p @_file_upload @_switch_iframe @editor_tiny +Feature: Undeployed H5P content should be only available to users that can deploy packages. + + Background: + Given the following "users" exist: + | username | firstname | lastname | email | + | teacher1 | Teacher | 1 | teacher1@example.com | + | teacher2 | Teacher | 2 | teacher2@example.com | + | student1 | Student | 1 | student1@example.com | + And the following "courses" exist: + | fullname | shortname | category | + | Course 1 | C1 | 0 | + And the following "course enrolments" exist: + | user | course | role | + | teacher1 | C1 | editingteacher | + | teacher2 | C1 | editingteacher | + | student1 | C1 | student | + # Make sure that the teacher2 can update libraries so it show the right info when. + And the following "permission overrides" exist: + | capability | permission | role | contextlevel | reference | + | moodle/h5p:updatelibraries | Allow | editingteacher | System | | + And the following "activities" exist: + | activity | name | intro | introformat | course | content | contentformat | idnumber | + | page | H5PPage | PageDesc1 | 1 | C1 | H5Ptest | 1 | 1 | + And I am on the H5PPage "page activity editing" page logged in as teacher1 + And the following "contentbank content" exist: + | contextlevel | reference | contenttype | user | contentname | filepath | + | Course | C1 | contenttype_h5p | teacher1 | filltheblanks.h5p | /h5p/tests/fixtures/filltheblanks.h5p | + And I click on the "Configure H5P content" button for the "Page content" TinyMCE editor + And I click on "Browse repositories..." "button" in the "Insert H5P content" "dialogue" + And I click on "Content bank" "link" in the ".fp-repo-area" "css_element" + And I click on "filltheblanks.h5p" "link" + And I click on "Select this file" "button" + And I click on "Insert H5P content" "button" in the "Insert H5P content" "dialogue" + # This is important here not to do Save and display as if not this will be deployed and the student will see it in the first step. + And I click on "Save and return to course" "button" + And I log out + And I log in as "admin" + And I navigate to "Users > Accounts > Browse list of users" in site administration + And I press "Delete" action in the "Teacher 1" report row + And I click on "Delete" "button" in the "Delete user" "dialogue" + And I should see "Deleted user Teacher 1" + + @javascript + Scenario: A student I should not be able to see a package that has been deployed by a deleted user. Then if another user deploys the package, I can see it. + Given I am on the "H5PPage" "page activity" page logged in as student1 + And I switch to "h5p-iframe" class iframe + And I should see "This file can't be displayed" + And I switch to the main frame + And I log out + # Then teacher2 will be allowed to deploy the package. + When I am on the "H5PPage" "page activity" page logged in as teacher2 + # Note the double switch to iframe is needed because the first iframe is the one that contains the H5P package and + # the second iframe is the one that contains the H5P content. + And I switch to "h5p-iframe" class iframe + And I switch to "h5p-iframe" class iframe + Then I should see "Of which countries are Berlin" + And I switch to the main frame + And I log out + # Now student1 should be able to see the package. + And I am on the "H5PPage" "page activity" page logged in as student1 + And I switch to "h5p-iframe" class iframe + And I switch to "h5p-iframe" class iframe + And I should see "Of which countries are Berlin" diff --git a/h5p/tests/helper_test.php b/h5p/tests/helper_test.php index 66be51cd030..a257d04e246 100644 --- a/h5p/tests/helper_test.php +++ b/h5p/tests/helper_test.php @@ -311,6 +311,16 @@ class helper_test extends \advanced_testcase { $factory->get_framework()->set_file($file); $candeploy = helper::can_deploy_package($file); $this->assertTrue($candeploy); + + $usertobedeleted = $this->getDataGenerator()->create_user(); + $this->setUser($usertobedeleted); + $file = helper::create_fake_stored_file_from_path($path, (int)$usertobedeleted->id); + $factory->get_framework()->set_file($file); + // Then we delete this user. + $this->setAdminUser(); + delete_user($usertobedeleted); + $candeploy = helper::can_deploy_package($file); + $this->assertTrue($candeploy); // We can update as admin. } /** @@ -339,6 +349,16 @@ class helper_test extends \advanced_testcase { $factory->get_framework()->set_file($file); $candeploy = helper::can_update_library($file); $this->assertTrue($candeploy); + + $usertobedeleted = $this->getDataGenerator()->create_user(); + $this->setUser($usertobedeleted); + $file = helper::create_fake_stored_file_from_path($path, (int)$usertobedeleted->id); + $factory->get_framework()->set_file($file); + // Then we delete this user. + $this->setAdminUser(); + delete_user($usertobedeleted); + $canupdate = helper::can_update_library($file); + $this->assertTrue($canupdate); // We can update as admin. } /** diff --git a/lib/behat/classes/behat_core_generator.php b/lib/behat/classes/behat_core_generator.php index 9275712babb..bc1e0807912 100644 --- a/lib/behat/classes/behat_core_generator.php +++ b/lib/behat/classes/behat_core_generator.php @@ -1052,15 +1052,15 @@ class behat_core_generator extends behat_generator_base { if (!empty($data['filepath'])) { $filename = basename($data['filepath']); $fs = get_file_storage(); - $filerecord = array( + $filerecord = [ 'component' => 'contentbank', 'filearea' => 'public', 'contextid' => $context->id, 'userid' => $data['userid'], 'itemid' => $content->get_id(), 'filename' => $filename, - 'filepath' => '/' - ); + 'filepath' => '/', + ]; $fs->create_file_from_pathname($filerecord, $CFG->dirroot . $data['filepath']); } } else {