Merge branch 'MDL-77665-main' of https://github.com/laurentdavid/moodle

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())) {
+        $fileuser = core_user::get_user($userid);
-            return true;
+        if (empty($fileuser) || $fileuser->deleted) {
+            $userid = null;
         }
-
+        return has_capability('moodle/h5p:deploy', $context, $userid);
-        return false;
     }
 
     /**
      * 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())) {
+        $fileuser = core_user::get_user($userid);
-            return true;
+        if (empty($fileuser) || $fileuser->deleted) {
+            $userid = null;
         }
 
-        return false;
+        return has_capability('moodle/h5p:updatelibraries', $context, $userid);
     }
 
     /**

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"

h5p/tests/helper_test.php

@@ -312,6 +312,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.
     }
 
     /**
@@ -340,6 +350,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.
     }
 
     /**

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 {

mod/h5pactivity/tests/behat/h5pactivity_deployment.feature

@@ -0,0 +1,51 @@
+@mod @mod_h5pactivity @core_h5p @_file_upload @_switch_iframe
+Feature: Undeployed H5P activities packages should be available only to any user 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       |           |
+    # Now create the activity as teacher1.
+    And the following "activities" exist:
+      | activity    | course | name          | username | packagefilepath                      |
+      | h5pactivity | C1     | Music history | teacher1 | h5p/tests/fixtures/filltheblanks.h5p |
+    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: In an H5P activity, as student I should not be able to deploy the package if not deployed by the teacher
+  beforehand. Then if a second teacher deploys the package, I can see it.
+    Given I am on the "Music history" "h5pactivity activity" page logged in as student1
+    And I switch to "h5p-player" 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.
+    And I am on the "Music history" "h5pactivity activity" page logged in as teacher2
+    And I switch to "h5p-player" class iframe
+    When 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 "Music history" "h5pactivity activity" page logged in as student1
+    And I switch to "h5p-player" class iframe
+    When I switch to "h5p-iframe" class iframe
+    Then I should see "Of which countries are Berlin"

mod/h5pactivity/tests/generator/lib.php

@@ -76,7 +76,11 @@ class mod_h5pactivity_generator extends testing_module_generator {
         if (!isset($record->reviewmode)) {
             $record->reviewmode = manager::REVIEWCOMPLETION;
         }
-
+        $globaluser = $USER;
+        if (!empty($record->username)) {
+            $user = core_user::get_user_by_username($record->username);
+            $this->set_user($user);
+        }
         // The 'packagefile' value corresponds to the draft file area ID. If not specified, create from packagefilepath.
         if (empty($record->packagefile)) {
             if (!isloggedin() || isguestuser()) {
@@ -85,21 +89,28 @@ class mod_h5pactivity_generator extends testing_module_generator {
             if (!file_exists($record->packagefilepath)) {
                 throw new coding_exception("File {$record->packagefilepath} does not exist");
             }
+
             $usercontext = context_user::instance($USER->id);
 
             // Pick a random context id for specified user.
             $record->packagefile = file_get_unused_draft_itemid();
 
             // Add actual file there.
-            $filerecord = ['component' => 'user', 'filearea' => 'draft',
+            $filerecord = [
-                    'contextid' => $usercontext->id, 'itemid' => $record->packagefile,
+                'component' => 'user',
-                    'filename' => basename($record->packagefilepath), 'filepath' => '/'];
+                'filearea' => 'draft',
+                'contextid' => $usercontext->id,
+                'itemid' => $record->packagefile,
+                'filename' => basename($record->packagefilepath),
+                'filepath' => '/',
+                'userid' => $USER->id,
+            ];
             $fs = get_file_storage();
             $fs->create_file_from_pathname($filerecord, $record->packagefilepath);
         }
-
+        $instance = parent::create_instance($record, (array)$options);
-        // Do work to actually add the instance.
+        $this->set_user($globaluser);
-        return parent::create_instance($record, (array)$options);
+        return $instance;
     }
 
     /**