diff --git a/contentbank/classes/form/upload_files.php b/contentbank/classes/form/upload_files.php index 91f8d3a29ca..bbd729c89ce 100644 --- a/contentbank/classes/form/upload_files.php +++ b/contentbank/classes/form/upload_files.php @@ -151,16 +151,35 @@ class upload_files extends \core_form\dynamic_form { if (!empty($files)) { $file = reset($files); $cb = new \core_contentbank\contentbank(); - if ($this->get_data()->id) { - $content = $cb->get_content_from_id($this->get_data()->id); - $contenttype = $content->get_content_type_instance(); - $content = $contenttype->replace_content($file, $content); - } else { - $content = $cb->create_content_from_file($this->get_context_for_dynamic_submission(), $USER->id, $file); + try { + if ($this->get_data()->id) { + $content = $cb->get_content_from_id($this->get_data()->id); + $contenttype = $content->get_content_type_instance(); + $content = $contenttype->replace_content($file, $content); + } else { + $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]; + $url = new \moodle_url('/contentbank/view.php', $params); + } 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'], + ); + } } - $params = ['id' => $content->get_id(), 'contextid' => $this->get_context_for_dynamic_submission()->id]; - $viewurl = new \moodle_url('/contentbank/view.php', $params); - return ['returnurl' => $viewurl->out(false)]; + + return ['returnurl' => $url->out(false)]; } return null; diff --git a/contentbank/contenttype/h5p/classes/content.php b/contentbank/contenttype/h5p/classes/content.php index 4670423dc98..ea41ffbbdd0 100644 --- a/contentbank/contenttype/h5p/classes/content.php +++ b/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); + } } diff --git a/contentbank/contenttype/h5p/classes/contenttype.php b/contentbank/contenttype/h5p/classes/contenttype.php index e16a30e6a12..eda31d90a2f 100644 --- a/contentbank/contenttype/h5p/classes/contenttype.php +++ b/contentbank/contenttype/h5p/classes/contenttype.php @@ -148,22 +148,25 @@ class contenttype extends \core_contentbank\contenttype { $types = []; $h5pfilestorage = new file_storage(); foreach ($h5pcontenttypes as $h5pcontenttype) { - $library = [ - 'name' => $h5pcontenttype->machine_name, - 'majorVersion' => $h5pcontenttype->major_version, - 'minorVersion' => $h5pcontenttype->minor_version, - ]; - $key = H5PCore::libraryToString($library); - $type = new stdClass(); - $type->key = $key; - $type->typename = $h5pcontenttype->title; - $type->typeeditorparams = 'library=' . $key; - $type->typeicon = $h5pfilestorage->get_icon_url( - $h5pcontenttype->id, - $h5pcontenttype->machine_name, - $h5pcontenttype->major_version, - $h5pcontenttype->minor_version); - $types[] = $type; + if ($h5pcontenttype->enabled) { + // Only enabled content-types will be displayed. + $library = [ + 'name' => $h5pcontenttype->machine_name, + 'majorVersion' => $h5pcontenttype->major_version, + 'minorVersion' => $h5pcontenttype->minor_version, + ]; + $key = H5PCore::libraryToString($library); + $type = new stdClass(); + $type->key = $key; + $type->typename = $h5pcontenttype->title; + $type->typeeditorparams = 'library=' . $key; + $type->typeicon = $h5pfilestorage->get_icon_url( + $h5pcontenttype->id, + $h5pcontenttype->machine_name, + $h5pcontenttype->major_version, + $h5pcontenttype->minor_version); + $types[] = $type; + } } return $types; diff --git a/contentbank/contenttype/h5p/tests/behat/disable_contenttypes.feature b/contentbank/contenttype/h5p/tests/behat/disable_contenttypes.feature new file mode 100644 index 00000000000..fa2e8b09664 --- /dev/null +++ b/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" diff --git a/contentbank/contenttype/h5p/tests/behat/teacher_upload_content.feature b/contentbank/contenttype/h5p/tests/behat/teacher_upload_content.feature index 48f6e222b2b..3ae0b6c19d1 100644 --- a/contentbank/contenttype/h5p/tests/behat/teacher_upload_content.feature +++ b/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 not see "Of which countries" - And I should see "missing-required-library" - And I switch to the main frame + Then I should see "Sorry, this file is not valid." + And I should not see "filltheblanks.h5p" And I log out And I log in as "admin" - And I navigate to "H5P > Manage H5P content types" in site administration - And I should not see "Fill in the Blanks" + And I am on "Course 1" course homepage + 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" - Then I should see "missing-required-library" + And I should see "missing-required-library" diff --git a/contentbank/contenttype/h5p/tests/content_h5p_test.php b/contentbank/contenttype/h5p/tests/content_h5p_test.php index 657a459d768..407a9bc8279 100644 --- a/contentbank/contenttype/h5p/tests/content_h5p_test.php +++ b/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], + ], + ]; + } } diff --git a/contentbank/tests/contentbank_test.php b/contentbank/tests/contentbank_test.php index 3ed5b6339b4..485717fcaf9 100644 --- a/contentbank/tests/contentbank_test.php +++ b/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(); - $dummyh5pfile = $fs->create_file_from_string($dummyh5p, 'Dummy H5Pcontent'); + $path = $CFG->dirroot . '/h5p/tests/fixtures/' . $name; + $dummyh5pfile = \core_h5p\helper::create_fake_stored_file_from_path($path); $cb = new contentbank(); $content = $cb->create_content_from_file($systemcontext, $USER->id, $dummyh5pfile); diff --git a/contentbank/view.php b/contentbank/view.php index 48519f4fc21..25e036883b4 100644 --- a/contentbank/view.php +++ b/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) { diff --git a/h5p/classes/api.php b/h5p/classes/api.php index f2d4a9ac083..5eb3ecbef63 100644 --- a/h5p/classes/api.php +++ b/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; + } } diff --git a/h5p/classes/editor_ajax.php b/h5p/classes/editor_ajax.php index 43e42480d73..ec349d787ce 100644 --- a/h5p/classes/editor_ajax.php +++ b/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 diff --git a/h5p/classes/helper.php b/h5p/classes/helper.php index 004757dab61..e88fe63bc55 100644 --- a/h5p/classes/helper.php +++ b/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(); - - $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; - } + if (api::is_valid_package($file, $onlyupdatelibs, $skipcontent, $factory, false)) { + $core = $factory->get_core(); + $h5pvalidator = $factory->get_validator(); $h5pstorage = $factory->get_storage(); $content = [ @@ -90,6 +70,7 @@ class helper { return $h5pstorage->contentId; } + return false; } diff --git a/h5p/classes/player.php b/h5p/classes/player.php index dfd6041cb59..bbd523e7bc2 100644 --- a/h5p/classes/player.php +++ b/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()); diff --git a/h5p/tests/api_test.php b/h5p/tests/api_test.php index 6872fde2f44..ec7f5e56ae9 100644 --- a/h5p/tests/api_test.php +++ b/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, + ], + ]; + } } diff --git a/h5p/upgrade.txt b/h5p/upgrade.txt index ee5271c44a9..8ebd917b0f3 100644 --- a/h5p/upgrade.txt +++ b/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) diff --git a/lang/en/contentbank.php b/lang/en/contentbank.php index ceeb7cb3d32..69c3c32a2f4 100644 --- a/lang/en/contentbank.php +++ b/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.'; diff --git a/lib/tests/event/contentbank_content_uploaded_test.php b/lib/tests/event/contentbank_content_uploaded_test.php index 8e1fe8da262..1f7af49ca2e 100644 --- a/lib/tests/event/contentbank_content_uploaded_test.php +++ b/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(); - $dummyh5pfile = $fs->create_file_from_string($dummyh5p, 'Dummy H5Pcontent'); + $path = $CFG->dirroot . '/h5p/tests/fixtures/greeting-card-887.h5p'; + $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(); diff --git a/repository/contentbank/tests/browser_test.php b/repository/contentbank/tests/browser_test.php index fec563eff1e..7de26e25975 100644 --- a/repository/contentbank/tests/browser_test.php +++ b/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. diff --git a/repository/contentbank/tests/search_test.php b/repository/contentbank/tests/search_test.php index 2ac4dc235a0..f4f0a646942 100644 --- a/repository/contentbank/tests/search_test.php +++ b/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);