MDL-69331 core_contentbank: Hide disabled H5P content-types

If a H5P content-type is disabled:
- The content bank won't display existing contents having it as a
main library.
- The content bank won't allow to create new contents using it.

contentbank/classes/form/upload_files.php

@@ -151,6 +151,7 @@ class upload_files extends \core_form\dynamic_form {
         if (!empty($files)) {
             $file = reset($files);
             $cb = new \core_contentbank\contentbank();
+            try {
                 if ($this->get_data()->id) {
                     $content = $cb->get_content_from_id($this->get_data()->id);
                     $contenttype = $content->get_content_type_instance();
@@ -159,8 +160,26 @@ class upload_files extends \core_form\dynamic_form {
                     $content = $cb->create_content_from_file($this->get_context_for_dynamic_submission(), $USER->id, $file);
                 }
                 $params = ['id' => $content->get_id(), 'contextid' => $this->get_context_for_dynamic_submission()->id];
-            $viewurl = new \moodle_url('/contentbank/view.php', $params);
+                $url = new \moodle_url('/contentbank/view.php', $params);
-            return ['returnurl' => $viewurl->out(false)];
+            } catch (\Exception $e) {
+                // Redirect to the right page (depending on if content is new or existing) and display an error.
+                if ($this->get_data()->id) {
+                    $content = $cb->get_content_from_id($this->get_data()->id);
+                    $params = [
+                        'id' => $content->get_id(),
+                        'contextid' => $this->get_context_for_dynamic_submission()->id,
+                        'errormsg' => 'notvalidpackage',
+                    ];
+                    $url = new \moodle_url('/contentbank/view.php', $params);
+                } else {
+                    $url = new \moodle_url('/contentbank/index.php', [
+                        'contextid' => $this->get_context_for_dynamic_submission()->id,
+                        'errormsg' => 'notvalidpackage'],
+                    );
+                }
+            }
+
+            return ['returnurl' => $url->out(false)];
         }
 
         return null;

contentbank/contenttype/h5p/classes/content.php

@@ -24,9 +24,6 @@
 
 namespace contenttype_h5p;
 
-use stdClass;
-use html_writer;
-
 /**
  * H5P Content manager class
  *
@@ -36,4 +33,59 @@ use html_writer;
  */
 class content extends \core_contentbank\content {
 
+    /**
+     * Returns user has access permission for the content itself.
+     * If the H5P content-type library is disabled, the user won't have access to it.
+     *
+     * @return bool     True if content could be accessed. False otherwise.
+     */
+    public function is_view_allowed(): bool {
+        // Force H5P content to be deployed.
+        $fileurl = $this->get_file_url();
+        // Skip capability check when creating the H5P content (because it has been created by trusted users).
+        $h5pplayer = new \core_h5p\player($fileurl, new \stdClass(), true, '', true);
+        // Flush error messages.
+        $h5pplayer->get_messages();
+
+        // Check if the H5P entry has been created and if the main library is enabled.
+        $file = $this->get_file();
+        if (!empty($file)) {
+            $h5p = \core_h5p\api::get_content_from_pathnamehash($file->get_pathnamehash());
+            if (empty($h5p)) {
+                // If there is no H5P entry for this content, it won't be displayed unless the user has the manageanycontent
+                // capability. Reasons for contents without a proper H5P entry in DB:
+                // - Invalid H5P package (it won't be never deployed).
+                // - Disabled content-type library (it can't be deployed so there is no way to know the mainlibraryid).
+                $context = \context::instance_by_id($this->content->contextid);
+                if (!has_capability('moodle/contentbank:manageanycontent', $context)) {
+                    return false;
+                }
+            } else if (!\core_h5p\api::is_library_enabled((object) ['id' => $h5p->mainlibraryid])) {
+                // If the main library is disabled, it won't be displayed.
+                return false;
+            }
+        }
+
+        return parent::is_view_allowed();
+    }
+
+    /**
+     * Import a file as a valid content.
+     * Before importing the file, this method will check if the file is a valid H5P package. If it's not valid, it will thrown
+     * an exception.
+     *
+     * @throws \file_exception If file operations fail
+     * @param \stored_file $file File to store in the content file area.
+     * @return \stored_file|null the stored content file or null if the file is discarted.
+     */
+    public function import_file(\stored_file $file): ?\stored_file {
+        // The H5P content can be only deployed if the author of the .h5p file can update libraries or if all the
+        // content-type libraries exist, to avoid users without the h5p:updatelibraries capability upload malicious content.
+        $onlyupdatelibs = !\core_h5p\helper::can_update_library($file);
+
+        if (!\core_h5p\api::is_valid_package($file, $onlyupdatelibs)) {
+            throw new \file_exception('invalidpackage');
+        }
+        return parent::import_file($file);
+    }
 }

contentbank/contenttype/h5p/classes/contenttype.php

@@ -148,6 +148,8 @@ class contenttype extends \core_contentbank\contenttype {
         $types = [];
         $h5pfilestorage = new file_storage();
         foreach ($h5pcontenttypes as $h5pcontenttype) {
+            if ($h5pcontenttype->enabled) {
+                // Only enabled content-types will be displayed.
                 $library = [
                     'name' => $h5pcontenttype->machine_name,
                     'majorVersion' => $h5pcontenttype->major_version,
@@ -165,6 +167,7 @@ class contenttype extends \core_contentbank\contenttype {
                     $h5pcontenttype->minor_version);
                 $types[] = $type;
             }
+        }
 
         return $types;
     }

contentbank/contenttype/h5p/tests/behat/disable_contenttypes.feature

@@ -0,0 +1,101 @@
+@core @core_contentbank @core_h5p @contenttype_h5p @_file_upload @javascript
+Feature: Disable H5P content-types from the content bank
+  In order to disable H5P content-types
+  As an admin
+  I need to be able to check they are not displayed in the content bank
+
+  Background:
+    Given the following "users" exist:
+      | username | firstname | lastname | email                |
+      | teacher1 | Teacher   | 1        | teacher1@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 |
+    And the following "contentbank contents" exist:
+      | contextlevel | reference | contenttype     | user     | contentname       | filepath                              |
+      | Course       | C1        | contenttype_h5p | admin    | filltheblanks     | /h5p/tests/fixtures/filltheblanks.h5p |
+      | Course       | C1        | contenttype_h5p | admin    | accordion         | /h5p/tests/fixtures/ipsums.h5p        |
+      | Course       | C1        | contenttype_h5p | admin    | invalidh5p        | /h5p/tests/fixtures/h5ptest.zip       |
+    And I log in as "admin"
+    And I am on "Course 1" course homepage with editing mode on
+    And I add the "Navigation" block if not present
+    And I log out
+
+  Scenario: Teachers cannot view disabled or invalid content-types
+    Given I log in as "teacher1"
+    And I am on "Course 1" course homepage
+    And I expand "Site pages" node
+    And I click on "Content bank" "link"
+    And I should see "accordion"
+    And I should see "filltheblanks"
+    And I should not see "invalidh5p"
+    And I log out
+    And I log in as "admin"
+    And I navigate to "H5P > Manage H5P content types" in site administration
+    And I click on "Disable" "link" in the "Accordion" "table_row"
+    And I log out
+    When I log in as "teacher1"
+    And I am on "Course 1" course homepage
+    And I expand "Site pages" node
+    And I click on "Content bank" "link"
+    Then I should not see "accordion"
+    And I should see "filltheblanks"
+    And I should not see "invalidh5p"
+
+  Scenario: Admins cannot view disabled content-types
+    Given I log in as "admin"
+    And I am on "Course 1" course homepage
+    And I expand "Site pages" node
+    And I click on "Content bank" "link"
+    And I should see "accordion"
+    And I should see "filltheblanks"
+    And I should see "invalidh5p"
+    And I navigate to "H5P > Manage H5P content types" in site administration
+    And I click on "Disable" "link" in the "Accordion" "table_row"
+    When I am on "Course 1" course homepage
+    And I expand "Site pages" node
+    And I click on "Content bank" "link"
+    Then I should not see "accordion"
+    And I should see "filltheblanks"
+    And I should see "invalidh5p"
+
+  Scenario: Teachers cannot create disabled content-types
+    Given I log in as "teacher1"
+    And I am on "Course 1" course homepage
+    And I expand "Site pages" node
+    And I click on "Content bank" "link"
+    And I click on "[data-action=Add-content]" "css_element"
+    And I should see "Accordion"
+    And I should see "Fill in the Blanks"
+    And I log out
+    And I log in as "admin"
+    And I navigate to "H5P > Manage H5P content types" in site administration
+    And I click on "Disable" "link" in the "Accordion" "table_row"
+    And I log out
+    When I log in as "teacher1"
+    And I am on "Course 1" course homepage
+    And I expand "Site pages" node
+    And I click on "Content bank" "link"
+    And I click on "[data-action=Add-content]" "css_element"
+    Then I should not see "Accordion"
+    And I should see "Fill in the Blanks"
+
+  Scenario: Admins cannot create disabled content-types
+    Given I log in as "admin"
+    And I am on "Course 1" course homepage
+    And I expand "Site pages" node
+    And I click on "Content bank" "link"
+    And I click on "[data-action=Add-content]" "css_element"
+    And I should see "Accordion"
+    And I should see "Fill in the Blanks"
+    And I navigate to "H5P > Manage H5P content types" in site administration
+    And I click on "Disable" "link" in the "Accordion" "table_row"
+    When I am on "Course 1" course homepage
+    And I expand "Site pages" node
+    And I click on "Content bank" "link"
+    And I click on "[data-action=Add-content]" "css_element"
+    Then I should not see "Accordion"
+    And I should see "Fill in the Blanks"

contentbank/contenttype/h5p/tests/behat/teacher_upload_content.feature

@@ -72,7 +72,7 @@ Feature: H5P file upload to content bank for non admins
     And I click on "Content bank" "link"
     Then I should see "filltheblanks.h5p"
 
-  Scenario: Teachers can not upload and deployed content types when libraries are not installed
+  Scenario: Teachers can not upload and deploy content types when libraries are not installed
     Given I log out
     And I log in as "admin"
     And I navigate to "H5P > Manage H5P content types" in site administration
@@ -89,14 +89,14 @@ Feature: H5P file upload to content bank for non admins
     And I click on "filltheblanks.h5p" "link"
     And I click on "Select this file" "button"
     And I click on "Save changes" "button"
-    And I switch to "h5p-player" class iframe
+    Then I should see "Sorry, this file is not valid."
-    Then I should not see "Of which countries"
+    And I should not see "filltheblanks.h5p"
-    And I should see "missing-required-library"
-    And I switch to the main frame
     And I log out
     And I log in as "admin"
-    And I navigate to "H5P > Manage H5P content types" in site administration
+    And I am on "Course 1" course homepage
-    And I should not see "Fill in the Blanks"
+    And I expand "Site pages" node
+    And I click on "Content bank" "link"
+    And I should not see "filltheblanks.h5p"
 
   Scenario: Teachers can not see existing contents when libraries are not installed
     Given I log out
@@ -138,8 +138,13 @@ Feature: H5P file upload to content bank for non admins
     Given I am on "Course 1" course homepage
     When I expand "Site pages" node
     And I click on "Content bank" "link"
+    Then I should not see "filltheblanks.h5p"
+    And I log out
+    And I log in as "admin"
+    And I am on "Course 1" course homepage
+    And I expand "Site pages" node
+    And I click on "Content bank" "link"
     And I should see "filltheblanks.h5p"
     And I click on "filltheblanks.h5p" "link"
     And I switch to "h5p-player" class iframe
-    Then I should not see "Of which countries"
+    And I should see "missing-required-library"
-    Then I should see "missing-required-library"

contentbank/contenttype/h5p/tests/content_h5p_test.php

@@ -66,4 +66,132 @@ class contenttype_h5p_content_plugin_testcase extends advanced_testcase {
         $this->assertInstanceOf(\stored_file::class, $file);
         $this->assertEquals($filename, $file->get_filename());
     }
+
+    /**
+     * Tests for is view allowed content.
+     *
+     * @covers ::is_view_allowed
+     * @dataProvider is_view_allowed_provider
+     *
+     * @param string $role User role to use for create and view contents.
+     * @param array $disabledlibraries Library names to disable.
+     * @param array $expected Array with the expected values for the contents in the following order:
+     *     ['H5P.Blanks deployed', 'H5P.Accordion deployed', 'H5P.Accordion undeployed', 'Invalid content'].
+     */
+    public function test_is_view_allowed(string $role, array $disabledlibraries, array $expected): void {
+        global $CFG, $USER, $DB;
+
+        $this->resetAfterTest();
+
+        // Create a course.
+        $course = $this->getDataGenerator()->create_course();
+        $coursecontext = \context_course::instance($course->id);
+
+        // Set user.
+        if ($role == 'admin') {
+            $this->setAdminUser();
+        } else {
+            // Enrol user to the course.
+            $user = $this->getDataGenerator()->create_and_enrol($course, $role);
+            $this->setUser($user);
+        }
+
+        // Add contents to the content bank.
+        $generator = $this->getDataGenerator()->get_plugin_generator('core_contentbank');
+        $filepath = $CFG->dirroot . '/h5p/tests/fixtures/filltheblanks.h5p';
+        $contents = $generator->generate_contentbank_data('contenttype_h5p', 1, $USER->id, $coursecontext, true, $filepath);
+        $filltheblanks = array_shift($contents);
+        $filepath = $CFG->dirroot . '/h5p/tests/fixtures/ipsums.h5p';
+        $contents = $generator->generate_contentbank_data('contenttype_h5p', 2, $USER->id, $coursecontext, true, $filepath);
+        $accordion1 = array_shift($contents);
+        $accordion2 = array_shift($contents);
+        $filepath = $CFG->dirroot . '/h5p/tests/fixtures/invalid.zip';
+        $contents = $generator->generate_contentbank_data('contenttype_h5p', 1, $USER->id, $coursecontext, true, $filepath);
+        $invalid = array_shift($contents);
+
+        // Load some of these H5P files though the player to create the H5P DB entries.
+        $h5pplayer = new \core_h5p\player($filltheblanks->get_file_url(), new \stdClass(), true);
+        $h5pplayer = new \core_h5p\player($accordion1->get_file_url(), new \stdClass(), true);
+
+        // Check the expected H5P content has been created.
+        $this->assertEquals(2, $DB->count_records('h5p'));
+        $this->assertEquals(4, $DB->count_records('contentbank_content'));
+
+        // Disable libraries.
+        foreach ($disabledlibraries as $libraryname) {
+            $libraryid = $DB->get_field('h5p_libraries', 'id', ['machinename' => $libraryname]);
+            \core_h5p\api::set_library_enabled((int) $libraryid, false);
+        }
+
+        $this->assertEquals($expected[0], $filltheblanks->is_view_allowed());
+        $this->assertEquals($expected[1], $accordion1->is_view_allowed());
+        $this->assertEquals($expected[2], $accordion2->is_view_allowed());
+        $this->assertEquals($expected[3], $invalid->is_view_allowed());
+
+        // Check that after enabling libraries again, all the content return true (but the invalid package).
+        foreach ($disabledlibraries as $libraryname) {
+            $libraryid = $DB->get_field('h5p_libraries', 'id', ['machinename' => $libraryname]);
+            \core_h5p\api::set_library_enabled((int) $libraryid, true);
+        }
+
+        $this->assertEquals(true, $filltheblanks->is_view_allowed());
+        $this->assertEquals(true, $accordion1->is_view_allowed());
+        $this->assertEquals(true, $accordion2->is_view_allowed()); // It will be deployed, so now it will always return true.
+        $this->assertEquals($expected[3], $invalid->is_view_allowed());
+    }
+
+    /**
+     * Data provider for test_is_view_allowed.
+     *
+     * @return array
+     */
+    public function is_view_allowed_provider(): array {
+        return [
+            'Editing teacher with all libraries enabled' => [
+                'role' => 'editingteacher',
+                'disabledlibraries' => [],
+                'expected' => [true, true, true, false],
+            ],
+            'Manager with all libraries enabled' => [
+                'role' => 'manager',
+                'disabledlibraries' => [],
+                'expected' => [true, true, true, true],
+            ],
+            'Admin with all libraries enabled' => [
+                'role' => 'admin',
+                'disabledlibraries' => [],
+                'expected' => [true, true, true, true],
+            ],
+            'Editing teacher with H5P.Accordion disabled' => [
+                'role' => 'editingteacher',
+                'disabledlibraries' => ['H5P.Accordion'],
+                'expected' => [true, false, false, false],
+            ],
+            'Manager with H5P.Accordion disabled' => [
+                'role' => 'manager',
+                'disabledlibraries' => ['H5P.Accordion'],
+                'expected' => [true, false, true, true],
+            ],
+            'Admin with H5P.Accordion disabled' => [
+                'role' => 'admin',
+                'disabledlibraries' => ['H5P.Accordion'],
+                'expected' => [true, false, true, true],
+            ],
+            'Editing teacher with all libraries disabled' => [
+                'role' => 'editingteacher',
+                'disabledlibraries' => ['H5P.Accordion', 'H5P.Blanks'],
+                'expected' => [false, false, false, false],
+            ],
+            'Manager with all libraries disabled' => [
+                'role' => 'manager',
+                'disabledlibraries' => ['H5P.Accordion', 'H5P.Blanks'],
+                'expected' => [false, false, true, true],
+            ],
+            'Admin with all libraries disabled' => [
+                'role' => 'admin',
+                'disabledlibraries' => ['H5P.Accordion', 'H5P.Blanks'],
+                'expected' => [false, false, true, true],
+            ],
+        ];
+    }
 }

contentbank/tests/contentbank_test.php

@@ -46,7 +46,7 @@ require_once($CFG->dirroot . '/contentbank/tests/fixtures/testable_content.php')
  * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  * @coversDefaultClass \core_contentbank\contentbank
  */
-class core_contentbank_testcase extends advanced_testcase {
+class contentbank_test extends advanced_testcase {
 
     /**
      * Setup to ensure that fixtures are loaded.
@@ -206,9 +206,10 @@ class core_contentbank_testcase extends advanced_testcase {
      */
     public function test_search_contents(?string $search, string $where, int $expectedresult, array $contexts = [],
             array $contenttypes = null): void {
-        global $DB;
+        global $DB, $CFG;
 
         $this->resetAfterTest();
+        $this->setAdminUser();
 
         // Create users.
         $managerroleid = $DB->get_field('role', 'id', ['shortname' => 'manager']);
@@ -230,11 +231,12 @@ class core_contentbank_testcase extends advanced_testcase {
         }
 
         // Add some content to the content bank.
+        $filepath = $CFG->dirroot . '/h5p/tests/fixtures/filltheblanks.h5p';
         $generator = $this->getDataGenerator()->get_plugin_generator('core_contentbank');
         foreach ($contexts as $context) {
             $contextinstance = $existingcontexts[$context];
             $records = $generator->generate_contentbank_data('contenttype_h5p', 3,
-                $manager->id, $contextinstance, false);
+                $manager->id, $contextinstance, false, $filepath);
         }
 
         // Search for some content.
@@ -357,12 +359,12 @@ class core_contentbank_testcase extends advanced_testcase {
      * @covers ::create_content_from_file
      */
     public function test_create_content_from_file() {
-        global $USER;
+        global $USER, $CFG;
 
         $this->resetAfterTest();
         $this->setAdminUser();
         $systemcontext = \context_system::instance();
-        $name = 'dummy_h5p.h5p';
+        $name = 'greeting-card-887.h5p';
 
         // Create a dummy H5P file.
         $dummyh5p = array(
@@ -374,8 +376,8 @@ class core_contentbank_testcase extends advanced_testcase {
             'filename' => $name,
             'userid' => $USER->id
         );
-        $fs = get_file_storage();
+        $path = $CFG->dirroot . '/h5p/tests/fixtures/' . $name;
-        $dummyh5pfile = $fs->create_file_from_string($dummyh5p, 'Dummy H5Pcontent');
+        $dummyh5pfile = \core_h5p\helper::create_fake_stored_file_from_path($path);
 
         $cb = new contentbank();
         $content = $cb->create_content_from_file($systemcontext, $USER->id, $dummyh5pfile);

contentbank/view.php

@@ -58,7 +58,8 @@ $contenttype = $content->get_content_type_instance();
 $pageheading = $record->name;
 
 if (!$content->is_view_allowed()) {
-    print_error('notavailable', 'contentbank');
+    $cburl = new \moodle_url('/contentbank/index.php', ['contextid' => $context->id, 'errormsg' => 'notavailable']);
+    redirect($cburl);
 }
 
 if ($content->get_visibility() == content::VISIBILITY_UNLISTED) {

h5p/classes/api.php

@@ -185,16 +185,20 @@ class api {
      *
      * @param string $url H5P pluginfile URL.
      * @param bool $preventredirect Set to true in scripts that can not redirect (CLI, RSS feeds, etc.), throws exceptions
+     * @param bool $skipcapcheck Whether capabilities should be checked or not to get the pluginfile URL because sometimes they
+     *     might be controlled before calling this method.
      *
      * @return array of [file, stdClass|false]:
      *             - file local file for this $url.
      *             - stdClass is an H5P object or false if there isn't any H5P with this URL.
      */
-    public static function get_content_from_pluginfile_url(string $url, bool $preventredirect = true): array {
+    public static function get_content_from_pluginfile_url(string $url, bool $preventredirect = true,
+        bool $skipcapcheck = false): array {
+
         global $DB;
 
         // Deconstruct the URL and get the pathname associated.
-        if (self::can_access_pluginfile_hash($url, $preventredirect)) {
+        if ($skipcapcheck || self::can_access_pluginfile_hash($url, $preventredirect)) {
             $pathnamehash = self::get_pluginfile_hash($url);
         }
 
@@ -223,17 +227,19 @@ class api {
      * @param factory $factory The \core_h5p\factory object
      * @param stdClass $messages The error, exception and info messages, raised while preparing and running an H5P content.
      * @param bool $preventredirect Set to true in scripts that can not redirect (CLI, RSS feeds, etc.), throws exceptions
+     * @param bool $skipcapcheck Whether capabilities should be checked or not to get the pluginfile URL because sometimes they
+     *     might be controlled before calling this method.
      *
      * @return array of [file, h5pid]:
      *             - file local file for this $url.
      *             - h5pid is the H5P identifier or false if there isn't any H5P with this URL.
      */
     public static function create_content_from_pluginfile_url(string $url, \stdClass $config, factory $factory,
-        \stdClass &$messages, bool $preventredirect = true): array {
+        \stdClass &$messages, bool $preventredirect = true, bool $skipcapcheck = false): array {
         global $USER;
 
         $core = $factory->get_core();
-        list($file, $h5p) = self::get_content_from_pluginfile_url($url, $preventredirect);
+        list($file, $h5p) = self::get_content_from_pluginfile_url($url, $preventredirect, $skipcapcheck);
 
         if (!$file) {
             $core->h5pF->setErrorMessage(get_string('h5pfilenotfound', 'core_h5p'));
@@ -657,4 +663,60 @@ class api {
 
         return true;
     }
+
+    /**
+     * Check whether an H5P package is valid or not.
+     *
+     * @param \stored_file $file The file with the H5P content.
+     * @param bool $onlyupdatelibs Whether new libraries can be installed or only the existing ones can be updated
+     * @param bool $skipcontent Should the content be skipped (so only the libraries will be saved)?
+     * @param factory|null $factory The \core_h5p\factory object
+     * @param bool $deletefiletree Should the temporary files be deleted before returning?
+     * @return bool True if the H5P file is valid (expected format, valid libraries...); false otherwise.
+     */
+    public static function is_valid_package(\stored_file $file, bool $onlyupdatelibs, bool $skipcontent = false,
+            ?factory $factory = null, bool $deletefiletree = true): bool {
+
+        // This may take a long time.
+        \core_php_time_limit::raise();
+
+        $isvalid = false;
+
+        if (empty($factory)) {
+            $factory = new factory();
+        }
+        $core = $factory->get_core();
+        $h5pvalidator = $factory->get_validator();
+
+        // Set the H5P file path.
+        $core->h5pF->set_file($file);
+        $path = $core->fs->getTmpPath();
+        $core->h5pF->getUploadedH5pFolderPath($path);
+        // Add manually the extension to the file to avoid the validation fails.
+        $path .= '.h5p';
+        $core->h5pF->getUploadedH5pPath($path);
+        // Copy the .h5p file to the temporary folder.
+        $file->copy_content_to($path);
+
+        if ($h5pvalidator->isValidPackage($skipcontent, $onlyupdatelibs)) {
+            if ($skipcontent) {
+                $isvalid = true;
+            } else if (!empty($h5pvalidator->h5pC->mainJsonData['mainLibrary'])) {
+                $mainlibrary = (object) ['machinename' => $h5pvalidator->h5pC->mainJsonData['mainLibrary']];
+                if (self::is_library_enabled($mainlibrary)) {
+                    $isvalid = true;
+                } else {
+                    // If the main library of the package is disabled, the H5P content will be considered invalid.
+                    $core->h5pF->setErrorMessage(get_string('mainlibrarydisabled', 'core_h5p'));
+                }
+            }
+        }
+
+        if ($deletefiletree) {
+            // Remove temp content folder.
+            \H5PCore::deleteFileTree($path);
+        }
+
+        return $isvalid;
+    }
 }

h5p/classes/editor_ajax.php

@@ -51,7 +51,8 @@ class editor_ajax implements H5PEditorAjaxInterface {
         global $DB;
 
         $sql = "SELECT hl2.id, hl2.machinename as machine_name, hl2.title, hl2.majorversion as major_version,
-                       hl2.minorversion AS minor_version, hl2.patchversion as patch_version, '' as has_icon, 0 as restricted
+                       hl2.minorversion AS minor_version, hl2.patchversion as patch_version, '' as has_icon, 0 as restricted,
+                       hl2.enabled
                   FROM {h5p_libraries} hl2
              LEFT JOIN {h5p_libraries} hl1
                         ON hl1.machinename = hl2.machinename

h5p/classes/helper.php

@@ -50,30 +50,10 @@ class helper {
      */
     public static function save_h5p(factory $factory, \stored_file $file, \stdClass $config, bool $onlyupdatelibs = false,
             bool $skipcontent = false) {
-        // This may take a long time.
-        \core_php_time_limit::raise();
 
+        if (api::is_valid_package($file, $onlyupdatelibs, $skipcontent, $factory, false)) {
             $core = $factory->get_core();
-        $core->h5pF->set_file($file);
-        $path = $core->fs->getTmpPath();
-        $core->h5pF->getUploadedH5pFolderPath($path);
-        // Add manually the extension to the file to avoid the validation fails.
-        $path .= '.h5p';
-        $core->h5pF->getUploadedH5pPath($path);
-
-        // Copy the .h5p file to the temporary folder.
-        $file->copy_content_to($path);
-
-        // Check if the h5p file is valid before saving it.
             $h5pvalidator = $factory->get_validator();
-        if ($h5pvalidator->isValidPackage($skipcontent, $onlyupdatelibs)) {
-            // If the main library of the package is disabled, the H5P content won't be saved.
-            $mainlibrary = (object) ['machinename' => $h5pvalidator->h5pC->mainJsonData['mainLibrary']];
-            if (!api::is_library_enabled($mainlibrary)) {
-                $core->h5pF->setErrorMessage(get_string('mainlibrarydisabled', 'core_h5p'));
-                return false;
-            }
-
             $h5pstorage = $factory->get_storage();
 
             $content = [
@@ -90,6 +70,7 @@ class helper {
 
             return $h5pstorage->contentId;
         }
+
         return false;
     }
 

h5p/classes/player.php

@@ -105,8 +105,11 @@ class player {
      * @param stdClass $config Configuration for H5P buttons.
      * @param bool $preventredirect Set to true in scripts that can not redirect (CLI, RSS feeds, etc.), throws exceptions
      * @param string $component optional moodle component to sent xAPI tracking
+     * @param bool $skipcapcheck Whether capabilities should be checked or not to get the pluginfile URL because sometimes they
+     *     might be controlled before calling this method.
      */
-    public function __construct(string $url, \stdClass $config, bool $preventredirect = true, string $component = '') {
+    public function __construct(string $url, \stdClass $config, bool $preventredirect = true, string $component = '',
+            bool $skipcapcheck = false) {
         if (empty($url)) {
             throw new \moodle_exception('h5pinvalidurl', 'core_h5p');
         }
@@ -128,7 +131,8 @@ class player {
             $config,
             $this->factory,
             $this->messages,
-            $this->preventredirect
+            $this->preventredirect,
+            $skipcapcheck
         );
         if ($file) {
             $this->context = \context::instance_by_id($file->get_contextid());

h5p/tests/api_test.php

@@ -37,7 +37,7 @@ defined('MOODLE_INTERNAL') || die();
  * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  * @coversDefaultClass \core_h5p\api
  */
-class api_testcase extends \advanced_testcase {
+class api_test extends \advanced_testcase {
 
     /**
      * Test the behaviour of delete_library().
@@ -742,4 +742,101 @@ class api_testcase extends \advanced_testcase {
             ],
         ];
     }
+
+    /**
+     * Test the behaviour of is_valid_package().
+     * @runInSeparateProcess
+     *
+     * @covers ::is_valid_package
+     * @dataProvider is_valid_package_provider
+     *
+     * @param string $filename The H5P content to validate.
+     * @param bool $expected Expected result after calling the method.
+     * @param bool $isadmin Whether the user calling the method will be admin or not.
+     * @param bool $onlyupdatelibs Whether new libraries can be installed or only the existing ones can be updated.
+     * @param bool $skipcontent Should the content be skipped (so only the libraries will be saved)?
+     */
+    public function test_is_valid_package(string $filename, bool $expected, bool $isadmin = false, bool $onlyupdatelibs = false,
+            bool $skipcontent = false): void {
+        global $USER;
+
+        $this->resetAfterTest();
+
+        if ($isadmin) {
+            $this->setAdminUser();
+            $user = $USER;
+        } else {
+            // Create a user.
+            $user = $this->getDataGenerator()->create_user();
+            $this->setUser($user);
+        }
+
+        // Prepare the file.
+        $path = __DIR__ . $filename;
+        $file = helper::create_fake_stored_file_from_path($path, (int)$user->id);
+
+        // Check if the H5P content is valid or not.
+        $result = api::is_valid_package($file, $onlyupdatelibs, $skipcontent);
+        $this->assertEquals($expected, $result);
+    }
+
+    /**
+     * Data provider for test_is_valid_package().
+     *
+     * @return array
+     */
+    public function is_valid_package_provider(): array {
+        return [
+            'Valid H5P file (as admin)' => [
+                'filename' => '/fixtures/greeting-card-887.h5p',
+                'expected' => true,
+                'isadmin' => true,
+            ],
+            'Valid H5P file (as user) without library update and checking content' => [
+                'filename' => '/fixtures/greeting-card-887.h5p',
+                'expected' => false, // Libraries are missing and user hasn't the right permissions to upload them.
+                'isadmin' => false,
+                'onlyupdatelibs' => false,
+                'skipcontent' => false,
+            ],
+            'Valid H5P file (as user) with library update and checking content' => [
+                'filename' => '/fixtures/greeting-card-887.h5p',
+                'expected' => false, // Libraries are missing and user hasn't the right permissions to upload them.
+                'isadmin' => false,
+                'onlyupdatelibs' => true,
+                'skipcontent' => false,
+            ],
+            'Valid H5P file (as user) without library update and skipping content' => [
+                'filename' => '/fixtures/greeting-card-887.h5p',
+                'expected' => true, // Content check is skipped so the package will be considered valid.
+                'isadmin' => false,
+                'onlyupdatelibs' => false,
+                'skipcontent' => true,
+            ],
+            'Valid H5P file (as user) with library update and skipping content' => [
+                'filename' => '/fixtures/greeting-card-887.h5p',
+                'expected' => true, // Content check is skipped so the package will be considered valid.
+                'isadmin' => false,
+                'onlyupdatelibs' => true,
+                'skipcontent' => true,
+            ],
+            'Invalid H5P file (as admin)' => [
+                'filename' => '/fixtures/h5ptest.zip',
+                'expected' => false,
+                'isadmin' => true,
+            ],
+            'Invalid H5P file (as user)' => [
+                'filename' => '/fixtures/h5ptest.zip',
+                'expected' => false,
+                'isadmin' => false,
+            ],
+            'Invalid H5P file (as user) skipping content' => [
+                'filename' => '/fixtures/h5ptest.zip',
+                'expected' => true, // Content check is skipped so the package will be considered valid.
+                'isadmin' => false,
+                'onlyupdatelibs' => false,
+                'skipcontent' => true,
+            ],
+        ];
+    }
 }

h5p/upgrade.txt

@@ -1,6 +1,17 @@
 This files describes API changes in core libraries and APIs,
 information provided here is intended especially for developers.
 
+=== 3.11 ===
+* Added $skipcapcheck parameter to H5P constructor, api::create_content_from_pluginfile_url() and
+api::get_content_from_pluginfile_url() to let skip capabilities check to get the pluginfile URL.
+* Added new field "enabled" to h5p_libraries to let define if a content type is enabled (1) or not (0).
+For now, only runnable content-types can be disabled/enabled. When a content-type is disabled, their
+contents are not displayed and no new contents using it can be created/uploaded.
+Some extra methods have been added to the api too in order to support this field:
+- set_library_enabled
+- is_library_enabled
+- is_valid_package
+
 === 3.10 ===
 * Added a new cache for h5p_library_files (MDL-69207)
 

lang/en/contentbank.php

@@ -61,6 +61,7 @@ $string['nocontenttypes'] = 'No content types available';
 $string['notavailable'] = 'Sorry, this content is not available.';
 $string['nopermissiontodelete'] = 'You do not have permission to delete content.';
 $string['nopermissiontomanage'] = 'You do not have permission to manage content.';
+$string['notvalidpackage'] = 'Sorry, this file is not valid.';
 $string['privacy:metadata:content:contenttype'] = 'The contenttype plugin of the content in the content bank.';
 $string['privacy:metadata:content:name'] = 'Name of the content in the content bank.';
 $string['privacy:metadata:content:timecreated'] = 'The time when the content was created.';

lib/tests/event/contentbank_content_uploaded_test.php

@@ -36,7 +36,7 @@ use core_contentbank\contentbank;
  * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  * @coversDefaultClass \core\event\contentbank_content_uploaded
  */
-class contentbank_content_uploaded_testcase extends \advanced_testcase {
+class contentbank_content_uploaded_test extends \advanced_testcase {
 
     /**
      * Setup to ensure that fixtures are loaded.
@@ -54,7 +54,7 @@ class contentbank_content_uploaded_testcase extends \advanced_testcase {
      * @covers ::create_from_record
      */
     public function test_content_created() {
-        global $USER;
+        global $USER, $CFG;
 
         $this->resetAfterTest();
         $this->setAdminUser();
@@ -69,8 +69,8 @@ class contentbank_content_uploaded_testcase extends \advanced_testcase {
             'filepath' => '/',
             'filename' => 'dummy_h5p.h5p'
         );
-        $fs = get_file_storage();
+        $path = $CFG->dirroot . '/h5p/tests/fixtures/greeting-card-887.h5p';
-        $dummyh5pfile = $fs->create_file_from_string($dummyh5p, 'Dummy H5Pcontent');
+        $dummyh5pfile = \core_h5p\helper::create_fake_stored_file_from_path($path);
 
         // Trigger and capture the event when creating content from a file.
         $sink = $this->redirectEvents();

repository/contentbank/tests/browser_test.php

@@ -43,7 +43,7 @@ class repository_contentbank_browser_testcase extends advanced_testcase {
      * the system context.
      */
     public function test_get_content_system_context_user_has_capabilities() {
-        global $DB;
+        global $DB, $CFG;
 
         $this->resetAfterTest(true);
 
@@ -66,8 +66,9 @@ class repository_contentbank_browser_testcase extends advanced_testcase {
         // Add some content to the content bank.
         $generator = $this->getDataGenerator()->get_plugin_generator('core_contentbank');
         // Add some content bank files in the system context.
+        $filepath = $CFG->dirroot . '/h5p/tests/fixtures/filltheblanks.h5p';
         $contentbankcontents = $generator->generate_contentbank_data('contenttype_h5p', 3, $admin->id,
-            $systemcontext, true);
+            $systemcontext, true, $filepath);
 
         // Log in as admin.
         $this->setUser($admin);
@@ -156,6 +157,8 @@ class repository_contentbank_browser_testcase extends advanced_testcase {
      * any category course should be able to access/view the content in the course category context.
      */
     public function test_get_content_course_category_context_user_has_capabilities() {
+        global $CFG;
+
         $this->resetAfterTest(true);
 
         // Create a course category.
@@ -175,8 +178,9 @@ class repository_contentbank_browser_testcase extends advanced_testcase {
         // Add some content to the content bank.
         $generator = $this->getDataGenerator()->get_plugin_generator('core_contentbank');
         // Add some content bank files in the course category context.
+        $filepath = $CFG->dirroot . '/h5p/tests/fixtures/filltheblanks.h5p';
         $contentbankcontents = $generator->generate_contentbank_data('contenttype_h5p', 3, $admin->id,
-            $coursecatcontext, true);
+            $coursecatcontext, true, $filepath);
 
         $this->setUser($admin);
         // Get the content bank nodes displayed to the admin in the course category context.
@@ -277,6 +281,8 @@ class repository_contentbank_browser_testcase extends advanced_testcase {
      * in the course should be able to access/view the content.
      */
     public function test_get_content_course_context_user_has_capabilities() {
+        global $CFG;
+
         $this->resetAfterTest(true);
 
         // Create course1.
@@ -290,8 +296,9 @@ class repository_contentbank_browser_testcase extends advanced_testcase {
         // Add some content to the content bank.
         $generator = $this->getDataGenerator()->get_plugin_generator('core_contentbank');
         // Add some content bank files in the course context.
+        $filepath = $CFG->dirroot . '/h5p/tests/fixtures/filltheblanks.h5p';
         $contentbankcontents = $generator->generate_contentbank_data('contenttype_h5p', 3, $admin->id,
-            $coursecontext, true);
+            $coursecontext, true, $filepath);
 
         $this->setUser($admin);
         // Get the content bank nodes displayed to the admin in the course context.

repository/contentbank/tests/search_test.php

@@ -182,6 +182,8 @@ class repository_contentbank_search_testcase extends advanced_testcase {
      * and system content. Other authenticated users should be able to access only the system content.
      */
     public function test_get_search_contents_user_can_access_certain_content() {
+        global $CFG;
+
         $this->resetAfterTest(true);
 
         $systemcontext = \context_system::instance();
@@ -201,16 +203,17 @@ class repository_contentbank_search_testcase extends advanced_testcase {
         // Add some content to the content bank in different contexts.
         $generator = $this->getDataGenerator()->get_plugin_generator('core_contentbank');
         // Add a content bank file in the system context.
+        $filepath = $CFG->dirroot . '/h5p/tests/fixtures/ipsums.h5p';
         $systemcontents = $generator->generate_contentbank_data('contenttype_h5p', 1, $admin->id,
-            $systemcontext, true, 'file.h5p', 'systemcontentfile');
+            $systemcontext, true, $filepath, 'systemcontentfile');
         $systemcontent = reset($systemcontents);
         // Add a content bank file in the course1 context.
         $course1contents = $generator->generate_contentbank_data('contenttype_h5p', 1, $admin->id,
-            $course1context, true, 'file.h5p', 'coursecontentfile1');
+            $course1context, true, $filepath, 'coursecontentfile1');
         $course1content = reset($course1contents);
         // Add a content bank file in the course2 context.
         $generator->generate_contentbank_data('contenttype_h5p', 1, $admin->id,
-            $course2context, true, 'file.h5p', 'coursecontentfile2');
+            $course2context, true, $filepath, 'coursecontentfile2');
 
         // Log in as an editing teacher.
         $this->setUser($editingteacher);