From c07f31ae2c1d0ce9f1c0d8aaf191dced60ac3bd6 Mon Sep 17 00:00:00 2001 From: Sara Arjona Date: Wed, 30 Oct 2019 09:07:51 +0100 Subject: [PATCH] MDL-67057 core_h5p: Add capability to update content-type libraries --- h5p/classes/framework.php | 28 ++++++++- h5p/classes/player.php | 108 +++++++++++++++++++---------------- h5p/tests/framework_test.php | 101 +++++++++++++++++++++++++++++++- lang/en/h5p.php | 2 + lang/en/role.php | 3 +- lib/db/access.php | 10 ++++ version.php | 2 +- 7 files changed, 199 insertions(+), 55 deletions(-) diff --git a/h5p/classes/framework.php b/h5p/classes/framework.php index 3831f274dc0..e5293cf2c3b 100644 --- a/h5p/classes/framework.php +++ b/h5p/classes/framework.php @@ -41,6 +41,9 @@ class framework implements \H5PFrameworkInterface { /** @var string The path to the last uploaded h5p file */ private $lastuploadedfile; + /** @var stored_file The .h5p file */ + private $file; + /** * Returns info for the current platform. * Implements getPlatformInfo. @@ -634,8 +637,29 @@ class framework implements \H5PFrameworkInterface { * FALSE if the user is not allowed to update libraries. */ public function mayUpdateLibraries() { - // Currently, capabilities are not being set/used, so everyone can update libraries. - return true; + return helper::can_update_library($this->get_file()); + } + + /** + * Get the .h5p file. + * + * @return stored_file The .h5p file. + */ + public function get_file(): \stored_file { + if (!isset($this->file)) { + throw new \coding_exception('Using get_file() before file is set'); + } + + return $this->file; + } + + /** + * Set the .h5p file. + * + * @param stored_file $file The .h5p file. + */ + public function set_file(\stored_file $file): void { + $this->file = $file; } /** diff --git a/h5p/classes/player.php b/h5p/classes/player.php index 5605a972481..224e0bf411c 100644 --- a/h5p/classes/player.php +++ b/h5p/classes/player.php @@ -76,10 +76,15 @@ class player { private $context; /** - * @var context The \core_h5p\factory object. + * @var factory The \core_h5p\factory object. */ private $factory; + /** + * @var stdClass The error, exception and info messages, raised while preparing and running the player. + */ + private $messages; + /** * Inits the H5P player for rendering the content. * @@ -94,6 +99,8 @@ class player { $this->factory = new \core_h5p\factory(); + $this->messages = new \stdClass(); + // Create \core_h5p\core instance. $this->core = $this->factory->get_core(); @@ -113,13 +120,20 @@ class player { * @return stdClass with framework error messages. */ public function get_messages() : \stdClass { - $messages = new \stdClass(); - $messages->error = $this->core->h5pF->getMessages('error'); - - if (empty($messages->error)) { - $messages->error = false; + // Check if there are some errors and store them in $messages. + if (empty($this->messages->error)) { + $this->messages->error = $this->core->h5pF->getMessages('error') ?: false; + } else { + $this->messages->error = array_merge($this->messages->error, $this->core->h5pF->getMessages('error')); } - return $messages; + + if (empty($this->messages->info)) { + $this->messages->info = $this->core->h5pF->getMessages('info') ?: false; + } else { + $this->messages->info = array_merge($this->messages->info, $this->core->h5pF->getMessages('info')); + } + + return $this->messages; } /** @@ -214,7 +228,7 @@ class player { * @return int|false H5P DB identifier. */ private function get_h5p_id(string $url, \stdClass $config) { - global $DB; + global $DB, $USER; $fs = get_file_storage(); @@ -255,13 +269,48 @@ class player { // Check if the user uploading the H5P content is "trustable". If the file hasn't been uploaded by a user with this // capability, the content won't be deployed and an error message will be displayed. - if (!has_capability('moodle/h5p:deploy', $this->context, $file->get_userid())) { + if (!helper::can_deploy_package($file)) { $this->core->h5pF->setErrorMessage(get_string('nopermissiontodeploy', 'core_h5p')); return false; } + // 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 = !helper::can_update_library($file); + + // Set the .h5p file, in order to check later the permissions to update libraries. + $this->core->h5pF->set_file($file); + // Validate and store the H5P content before displaying it. - return $this->save_h5p($file, $config); + $h5pid = helper::save_h5p($this->factory, $file, $config, $onlyupdatelibs, false); + if (!$h5pid && $file->get_userid() != $USER->id && has_capability('moodle/h5p:updatelibraries', $this->context)) { + // The user has permission to update libraries but the package has been uploaded by a different + // user without this permission. Check if there is some missing required library error. + $missingliberror = false; + $messages = $this->get_messages(); + if (!empty($messages->error)) { + foreach ($messages->error as $error) { + if ($error->code == 'missing-required-library') { + $missingliberror = true; + break; + } + } + } + if ($missingliberror) { + // The message about the permissions to upload libraries should be removed. + $infomsg = "Note that the libraries may exist in the file you uploaded, but you're not allowed to upload " . + "new libraries. Contact the site administrator about this."; + if (($key = array_search($infomsg, $messages->info)) !== false) { + unset($messages->info[$key]); + } + + // No library will be installed and an error will be displayed, because this content is not trustable. + $this->core->h5pF->setInfoMessage(get_string('notrustablefile', 'core_h5p')); + } + return false; + + } + return $h5pid; } } @@ -362,45 +411,6 @@ class player { return $fs->get_pathname_hash($contextid, $component, $filearea, $itemid, $filepath, $filename); } - /** - * Store an H5P file - * - * @param stored_file $file Moodle file instance - * @param stdClass $config Button options config. - * - * @return int|false The H5P identifier or false if it's not a valid H5P package. - */ - private function save_h5p($file, \stdClass $config) : int { - // This may take a long time. - \core_php_time_limit::raise(); - - $path = $this->core->fs->getTmpPath(); - $this->core->h5pF->getUploadedH5pFolderPath($path); - // Add manually the extension to the file to avoid the validation fails. - $path .= '.h5p'; - $this->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 = $this->factory->get_validator(); - if ($h5pvalidator->isValidPackage(false, false)) { - $h5pstorage = $this->factory->get_storage(); - - $options = ['disable' => $this->get_display_options($config)]; - $content = [ - 'pathnamehash' => $file->get_pathnamehash(), - 'contenthash' => $file->get_contenthash(), - ]; - - $h5pstorage->savePackage($content, null, false, $options); - return $h5pstorage->contentId; - } - - return false; - } - /** * Get the representation of display options as int. * @param stdClass $config Button options config. diff --git a/h5p/tests/framework_test.php b/h5p/tests/framework_test.php index 8cef8457b45..555dbba02cf 100644 --- a/h5p/tests/framework_test.php +++ b/h5p/tests/framework_test.php @@ -702,10 +702,107 @@ class framework_testcase extends \advanced_testcase { /** * Test the behaviour of mayUpdateLibraries(). */ - public function test_mayUpdateLibraries() { - $mayupdatelib = $this->framework->mayUpdateLibraries(); + public function test_mayUpdateLibraries(): void { + global $DB; + $this->resetAfterTest(); + + // Create some users. + $contextsys = \context_system::instance(); + $user = $this->getDataGenerator()->create_user(); + $admin = get_admin(); + $managerrole = $DB->get_record('role', ['shortname' => 'manager'], '*', MUST_EXIST); + $studentrole = $DB->get_record('role', ['shortname' => 'student'], '*', MUST_EXIST); + $manager = $this->getDataGenerator()->create_user(); + role_assign($managerrole->id, $manager->id, $contextsys); + + // Create a course with a label and enrol the user. + $course = $this->getDataGenerator()->create_course(); + $label = $this->getDataGenerator()->create_module('label', ['course' => $course->id]); + list(, $labelcm) = get_course_and_cm_from_instance($label->id, 'label'); + $contextlabel = \context_module::instance($labelcm->id); + $this->getDataGenerator()->enrol_user($user->id, $course->id, 'student'); + + // Create the .h5p file. + $path = __DIR__ . '/fixtures/h5ptest.zip'; + + // Admin and manager should have permission to update libraries. + $file = helper::create_fake_stored_file_from_path($path, $admin->id, $contextsys); + $this->framework->set_file($file); + $mayupdatelib = $this->framework->mayUpdateLibraries(); $this->assertTrue($mayupdatelib); + + $file = helper::create_fake_stored_file_from_path($path, $manager->id, $contextsys); + $this->framework->set_file($file); + $mayupdatelib = $this->framework->mayUpdateLibraries(); + $this->assertTrue($mayupdatelib); + + // By default, normal user hasn't permission to update libraries (in both contexts, system and module label). + $file = helper::create_fake_stored_file_from_path($path, $user->id, $contextsys); + $this->framework->set_file($file); + $mayupdatelib = $this->framework->mayUpdateLibraries(); + $this->assertFalse($mayupdatelib); + + $file = helper::create_fake_stored_file_from_path($path, $user->id, $contextlabel); + $this->framework->set_file($file); + $mayupdatelib = $this->framework->mayUpdateLibraries(); + $this->assertFalse($mayupdatelib); + + // If the current user (admin) can update libraries, the method should return true (even if the file userid hasn't the + // required capabilility in the file context). + $file = helper::create_fake_stored_file_from_path($path, $admin->id, $contextlabel); + $this->framework->set_file($file); + $mayupdatelib = $this->framework->mayUpdateLibraries(); + $this->assertTrue($mayupdatelib); + + // If the update capability is assigned to the user, they should be able to update the libraries (only in the context + // where the capability has been assigned). + $file = helper::create_fake_stored_file_from_path($path, $user->id, $contextlabel); + $this->framework->set_file($file); + $mayupdatelib = $this->framework->mayUpdateLibraries(); + $this->assertFalse($mayupdatelib); + assign_capability('moodle/h5p:updatelibraries', CAP_ALLOW, $studentrole->id, $contextlabel); + $mayupdatelib = $this->framework->mayUpdateLibraries(); + $this->assertTrue($mayupdatelib); + $file = helper::create_fake_stored_file_from_path($path, $user->id, $contextsys); + $this->framework->set_file($file); + $mayupdatelib = $this->framework->mayUpdateLibraries(); + $this->assertFalse($mayupdatelib); + } + + /** + * Test the behaviour of get_file() and set_file(). + */ + public function test_get_file(): void { + $this->resetAfterTest(); + + // Create some users. + $contextsys = \context_system::instance(); + $user = $this->getDataGenerator()->create_user(); + + // The H5P file. + $path = __DIR__ . '/fixtures/h5ptest.zip'; + + // An error should be raised when it's called before initialitzing it. + $this->expectException('coding_exception'); + $this->expectExceptionMessage('Using get_file() before file is set'); + $this->framework->get_file(); + + // Check the value when only path and user are set. + $file = helper::create_fake_stored_file_from_path($path, $user->id); + $this->framework->set_file($file); + $file = $this->framework->get_file(); + $this->assertEquals($user->id, $$file->get_userid()); + $this->assertEquals($contextsys->id, $file->get_contextid()); + + // Check the value when also the context is set. + $course = $this->getDataGenerator()->create_course(); + $contextcourse = \context_course::instance($course->id); + $file = helper::create_fake_stored_file_from_path($path, $user->id, $contextcourse); + $this->framework->set_file($file); + $file = $this->framework->get_file(); + $this->assertEquals($user->id, $$file->get_userid()); + $this->assertEquals($contextcourse->id, $file->get_contextid()); } /** diff --git a/lang/en/h5p.php b/lang/en/h5p.php index a65724f2e55..d873142aeaf 100644 --- a/lang/en/h5p.php +++ b/lang/en/h5p.php @@ -125,6 +125,8 @@ $string['nocopyright'] = 'No copyright information available for this content.'; $string['noextension'] = 'The file you uploaded is not a valid HTML5 Package (It does not have the .h5p file extension)'; $string['nopermissiontodeploy'] = 'This file can\'t be displayed because it has been uploaded by a user without the required capability to deploy H5P content.'; $string['nojson'] = 'The main h5p.json file is not valid'; +$string['notrustablefile'] = 'This file can\'t be displayed because it has been uploaded by a user without the capability to update H5P content types. + Please contact your administrator to ask for the content type to be installed.'; $string['nounzip'] = 'The file you uploaded is not a valid HTML5 Package (We are unable to unzip it)'; $string['offlineDialogBody'] = 'We were unable to send information about your completion of this task. Please check your internet connection.'; $string['offlineDialogHeader'] = 'Your connection to the server was lost'; diff --git a/lang/en/role.php b/lang/en/role.php index d9ec365eb29..72edb507613 100644 --- a/lang/en/role.php +++ b/lang/en/role.php @@ -261,6 +261,7 @@ $string['grade:view'] = 'View own grades'; $string['grade:viewall'] = 'View grades of other users'; $string['grade:viewhidden'] = 'View hidden grades for owner'; $string['h5p:deploy'] = 'Deploy H5P content'; +$string['h5p:updatelibraries'] = 'Manage H5P content types'; $string['h5p:setdisplayoptions'] = 'Set H5P display options'; $string['highlightedcellsshowdefault'] = 'The permissions highlighted in the table below are the defaults for the role archetype currently selected above.'; $string['highlightedcellsshowinherit'] = 'The highlighted cells in the table below show the permission (if any) that will be inherited. Apart from the capabilities whose permission you actually want to alter, you should leave everything set to Inherit.'; @@ -496,4 +497,4 @@ $string['privacy:metadata:role_cohortroles'] = 'Roles to cohort'; $string['course:togglecompletion'] = 'Manually mark activities as complete'; // Deprecated since Moodle 3.8. -$string['eventrolecapabilitiesupdated'] = 'Role capabilities updated'; \ No newline at end of file +$string['eventrolecapabilitiesupdated'] = 'Role capabilities updated'; diff --git a/lib/db/access.php b/lib/db/access.php index 9b7668e7c8d..ee6ee0906bc 100644 --- a/lib/db/access.php +++ b/lib/db/access.php @@ -2450,4 +2450,14 @@ $capabilities = array( 'editingteacher' => CAP_ALLOW, ) ), + + // Allow to update H5P content-type libraries. + 'moodle/h5p:updatelibraries' => [ + 'riskbitmask' => RISK_XSS, + 'captype' => 'write', + 'contextlevel' => CONTEXT_MODULE, + 'archetypes' => [ + 'manager' => CAP_ALLOW, + ] + ], ); diff --git a/version.php b/version.php index 38541a31ef4..3177b09c3fb 100644 --- a/version.php +++ b/version.php @@ -29,7 +29,7 @@ defined('MOODLE_INTERNAL') || die(); -$version = 2019110500.00; // YYYYMMDD = weekly release date of this DEV branch. +$version = 2019110500.01; // YYYYMMDD = weekly release date of this DEV branch. // RR = release increments - 00 in DEV branches. // .XX = incremental changes.