From eb4e36400e93b51c3bea28d5f61c1f6555194293 Mon Sep 17 00:00:00 2001 From: Sara Arjona Date: Wed, 14 Jul 2021 18:07:55 +0200 Subject: [PATCH 1/2] MDL-71956 core_h5p: Add more scenarios to can_edit_content The method can_edit_content() now supports more scenarios where the H5P content can be edited: - Instead of supporting only mod_h5pactivity, now it supports any mod or block when the user has the addinstance capability. - If the component implements the can_edit_content method in the h5p\canedit class and it returns true. For instance, the mod_forum implements it and return true when filearea is post, if the user can edit the post where the H5P is. --- h5p/classes/api.php | 27 +++- h5p/tests/api_test.php | 95 ++++++++++++- h5p/upgrade.txt | 3 + mod/forum/classes/h5p/canedit.php | 75 ++++++++++ mod/forum/tests/h5p_canedit_test.php | 204 +++++++++++++++++++++++++++ 5 files changed, 392 insertions(+), 12 deletions(-) create mode 100644 mod/forum/classes/h5p/canedit.php create mode 100644 mod/forum/tests/h5p_canedit_test.php diff --git a/h5p/classes/api.php b/h5p/classes/api.php index 6adacc8680b..eda8766641a 100644 --- a/h5p/classes/api.php +++ b/h5p/classes/api.php @@ -267,7 +267,8 @@ class api { * - The user is the author of the file. * - The component is different from user (i.e. private files). * - If the component is contentbank, the user can edit this file (calling the ContentBank API). - * - If the component is mod_h5pactivity, the user has the addinstance capability. + * - If the component is mod_xxx or block_xxx, the user has the addinstance capability. + * - If the component implements the can_edit_content in the h5p\canedit class and the callback to this method returns true. * * @param \stored_file $file The H5P file to check. * @@ -277,25 +278,37 @@ class api { public static function can_edit_content(\stored_file $file): bool { global $USER; + list($type, $component) = \core_component::normalize_component($file->get_component()); + // Private files. $currentuserisauthor = $file->get_userid() == $USER->id; - $isuserfile = $file->get_component() === 'user'; + $isuserfile = $component === 'user'; if ($currentuserisauthor && $isuserfile) { // The user can edit the content because it's a private user file and she is the owner. return true; } - // For mod_h5pactivity, check whether the user can add/edit them. - if ($file->get_component() === 'mod_h5pactivity') { + // Check if the plugin where the file belongs implements the custom can_edit_content method and call it if that's the case. + $classname = '\\' . $file->get_component() . '\\h5p\\canedit'; + $methodname = 'can_edit_content'; + if (method_exists($classname, $methodname)) { + return $classname::{$methodname}($file); + } + + // For mod/block files, check if the user has the addinstance capability of the component where the file belongs. + if ($type === 'mod' || $type === 'block') { + // For any other component, check whether the user can add/edit them. $context = \context::instance_by_id($file->get_contextid()); - if (has_capability("mod/h5pactivity:addinstance", $context)) { - // The user can edit the content because she has the capability for creating H5P activities where the file belongs. + $plugins = \core_component::get_plugin_list($type); + $isvalid = array_key_exists($component, $plugins); + if ($isvalid && has_capability("$type/$component:addinstance", $context)) { + // The user can edit the content because she has the capability for creating instances where the file belongs. return true; } } // For contentbank files, use the API to check if the user has access. - if ($file->get_component() == 'contentbank') { + if ($component == 'contentbank') { $cb = new \core_contentbank\contentbank(); $content = $cb->get_content_from_id($file->get_itemid()); $contenttype = $content->get_content_type_instance(); diff --git a/h5p/tests/api_test.php b/h5p/tests/api_test.php index 0ab510e4c37..007600021b4 100644 --- a/h5p/tests/api_test.php +++ b/h5p/tests/api_test.php @@ -27,6 +27,8 @@ declare(strict_types = 1); namespace core_h5p; +use stdClass; + defined('MOODLE_INTERNAL') || die(); /** @@ -444,11 +446,13 @@ class api_test extends \advanced_testcase { * @param string $fileauthor Author of the file to check. * @param string $filecomponent Component of the file to check. * @param bool $expected Expected result after calling the can_edit_content method. + * @param string $filearea Area of the file to check. * * @return void */ - public function test_can_edit_content(string $currentuser, string $fileauthor, string $filecomponent, bool $expected): void { - global $USER; + public function test_can_edit_content(string $currentuser, string $fileauthor, string $filecomponent, bool $expected, + $filearea = 'unittest'): void { + global $USER, $DB; $this->setRunTestInSeparateProcess(true); $this->resetAfterTest(); @@ -472,6 +476,20 @@ class api_test extends \advanced_testcase { $this->setUser($users[$currentuser]); } + $itemid = rand(); + if ($filearea === 'post') { + // Create a forum and add a discussion. + $forum = $this->getDataGenerator()->create_module('forum', ['course' => $course->id]); + + $record = new stdClass(); + $record->course = $course->id; + $record->userid = $users[$fileauthor]->id; + $record->forum = $forum->id; + $discussion = $this->getDataGenerator()->get_plugin_generator('mod_forum')->create_discussion($record); + $post = $DB->get_record('forum_posts', ['discussion' => $discussion->id]); + $itemid = $post->id; + } + // Create the file. $filename = 'greeting-card-887.h5p'; $path = __DIR__ . '/fixtures/' . $filename; @@ -491,8 +509,8 @@ class api_test extends \advanced_testcase { $filerecord = [ 'contextid' => $context->id, 'component' => $filecomponent, - 'filearea' => 'unittest', - 'itemid' => rand(), + 'filearea' => $filearea, + 'itemid' => $itemid, 'filepath' => '/', 'filename' => basename($path), 'userid' => $users[$fileauthor]->id, @@ -589,19 +607,80 @@ class api_test extends \advanced_testcase { 'expected' => false, ], + // Component = mod_book. + 'mod_book: Admin user is author' => [ + 'currentuser' => 'admin', + 'fileauthor' => 'admin', + 'filecomponent' => 'mod_book', + 'expected' => true, + ], + 'mod_book: Admin user, teacher is author' => [ + 'currentuser' => 'admin', + 'fileauthor' => 'teacher', + 'filecomponent' => 'mod_book', + 'expected' => true, + ], + // Component = mod_forum. 'mod_forum: Admin user is author' => [ 'currentuser' => 'admin', 'fileauthor' => 'admin', 'filecomponent' => 'mod_forum', - 'expected' => false, + 'expected' => true, ], 'mod_forum: Admin user, teacher is author' => [ 'currentuser' => 'admin', 'fileauthor' => 'teacher', 'filecomponent' => 'mod_forum', + 'expected' => true, + ], + 'mod_forum: Teacher user, admin is author' => [ + 'currentuser' => 'teacher', + 'fileauthor' => 'admin', + 'filecomponent' => 'mod_forum', + 'expected' => true, + ], + 'mod_forum: Student user, teacher is author' => [ + 'currentuser' => 'student', + 'fileauthor' => 'teacher', + 'filecomponent' => 'mod_forum', 'expected' => false, ], + 'mod_forum/post: Admin user is author' => [ + 'currentuser' => 'admin', + 'fileauthor' => 'admin', + 'filecomponent' => 'mod_forum', + 'expected' => true, + 'filearea' => 'post', + ], + 'mod_forum/post: Teacher user, admin is author' => [ + 'currentuser' => 'teacher', + 'fileauthor' => 'admin', + 'filecomponent' => 'mod_forum', + 'expected' => true, + 'filearea' => 'post', + ], + 'mod_forum/post: Student user, teacher is author' => [ + 'currentuser' => 'student', + 'fileauthor' => 'teacher', + 'filecomponent' => 'mod_forum', + 'expected' => false, + 'filearea' => 'post', + ], + + // Component = block_html. + 'block_html: Admin user is author' => [ + 'currentuser' => 'admin', + 'fileauthor' => 'admin', + 'filecomponent' => 'block_html', + 'expected' => true, + ], + 'block_html: Admin user, teacher is author' => [ + 'currentuser' => 'admin', + 'fileauthor' => 'teacher', + 'filecomponent' => 'block_html', + 'expected' => true, + ], // Component = contentbank. 'contentbank: Admin user is author' => [ @@ -654,6 +733,12 @@ class api_test extends \advanced_testcase { 'filecomponent' => 'mod_unexisting', 'expected' => false, ], + 'Unexisting block' => [ + 'currentuser' => 'admin', + 'fileauthor' => 'admin', + 'filecomponent' => 'block_unexisting', + 'expected' => false, + ], ]; } diff --git a/h5p/upgrade.txt b/h5p/upgrade.txt index 8e811526904..2e760313733 100644 --- a/h5p/upgrade.txt +++ b/h5p/upgrade.txt @@ -6,6 +6,9 @@ information provided here is intended especially for developers. * Added edit.php and editcontent_form class, for modifying H5P content given an H5P identifier (from the h5p table). * Added a new parameter to the player::display method, to define whether the edit button should be displayed below the H5P content or not. Default value for this parameter is false. +* H5P subsystem is allowed to act as an API (level 2) too. +* Plugins can now implement h5p\canedit::can_edit_content method to define, if required, any custom behaviour for deciding +whether an H5P content can be edited or not. The specific plugin check will completely override the generic check. === 3.11 === * Added $skipcapcheck parameter to H5P constructor, api::create_content_from_pluginfile_url() and diff --git a/mod/forum/classes/h5p/canedit.php b/mod/forum/classes/h5p/canedit.php new file mode 100644 index 00000000000..695df4ef1d8 --- /dev/null +++ b/mod/forum/classes/h5p/canedit.php @@ -0,0 +1,75 @@ +. + +namespace mod_forum\h5p; + +/** + * Class to check if the H5P content can be edited for this plugin. + * + * @package mod_forum + * @copyright 2021 Sara Arjona (sara@moodle.com) + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class canedit { + + /** + * Check if the user can edit an H5P file. In that case, this method will return true if the file belongs to mod_forum + * filearea is post and the user can edit the post where the H5P is. + * + * @param \stored_file $file The H5P file to check. + * + * @return boolean Whether the user can edit or not the given file. + * @since Moodle 4.0 + */ + public static function can_edit_content(\stored_file $file): bool { + global $USER; + + list($type, $component) = \core_component::normalize_component($file->get_component()); + + if ($type === 'mod' && $component === 'forum') { + // For mod_forum files in posts, check if the user can edit the post where the H5P is. + if ($file->get_filearea() === 'post') { + // Check if the user can edit the forum post. + $vaultfactory = \mod_forum\local\container::get_vault_factory(); + $forumvault = $vaultfactory->get_forum_vault(); + $discussionvault = $vaultfactory->get_discussion_vault(); + $postvault = $vaultfactory->get_post_vault(); + $postid = $file->get_itemid(); + $postentity = $postvault->get_from_id($postid); + if (!empty($postentity)) { + $discussionentity = $discussionvault->get_from_id($postentity->get_discussion_id()); + $managerfactory = \mod_forum\local\container::get_manager_factory(); + $forumentity = $forumvault->get_from_id($discussionentity->get_forum_id()); + $capabilitymanager = $managerfactory->get_capability_manager($forumentity); + if ($capabilitymanager->can_edit_post($USER, $discussionentity, $postentity)) { + return true; + } + } + } else { + // For any other fileare, check whether the user can add/edit them. + $context = \context::instance_by_id($file->get_contextid()); + $plugins = \core_component::get_plugin_list($type); + $isvalid = array_key_exists($component, $plugins); + if ($isvalid && has_capability("$type/$component:addinstance", $context)) { + // The user can edit the content because she has the capability for creating instances where the file belongs. + return true; + } + } + } + + return false; + } +} diff --git a/mod/forum/tests/h5p_canedit_test.php b/mod/forum/tests/h5p_canedit_test.php new file mode 100644 index 00000000000..64062a28018 --- /dev/null +++ b/mod/forum/tests/h5p_canedit_test.php @@ -0,0 +1,204 @@ +. + +declare(strict_types = 1); + +namespace mod_forum\h5p; + +use stdClass; + +/** + * Test class covering the H5P canedit class. + * + * @package mod_forum + * @copyright 2021 Sara Arjona + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @coversDefaultClass \mod_forum\h5p\canedit + */ +class h5p_canedit_test extends \advanced_testcase { + + /** + * Test the behaviour of can_edit_content(). + * + * @covers ::can_edit_content + * @dataProvider can_edit_content_provider + * + * @param string $currentuser User who will call the method. + * @param string $fileauthor Author of the file to check. + * @param string $filecomponent Component of the file to check. + * @param bool $expected Expected result after calling the can_edit_content method. + * @param string $filearea Area of the file to check. + * + * @return void + */ + public function test_can_edit_content(string $currentuser, string $fileauthor, string $filecomponent, bool $expected, + $filearea = 'unittest'): void { + global $USER, $DB; + + $this->setRunTestInSeparateProcess(true); + $this->resetAfterTest(); + + // Create course. + $course = $this->getDataGenerator()->create_course(); + $context = \context_course::instance($course->id); + + // Create some users. + $this->setAdminUser(); + $teacher = $this->getDataGenerator()->create_and_enrol($course, 'editingteacher'); + $student = $this->getDataGenerator()->create_and_enrol($course, 'student'); + $users = [ + 'admin' => $USER, + 'teacher' => $teacher, + 'student' => $student, + ]; + + // Set current user. + if ($currentuser !== 'admin') { + $this->setUser($users[$currentuser]); + } + + $itemid = rand(); + if ($filearea === 'post') { + // Create a forum and add a discussion. + $forum = $this->getDataGenerator()->create_module('forum', ['course' => $course->id]); + + $record = new stdClass(); + $record->course = $course->id; + $record->userid = $users[$fileauthor]->id; + $record->forum = $forum->id; + $discussion = $this->getDataGenerator()->get_plugin_generator('mod_forum')->create_discussion($record); + $post = $DB->get_record('forum_posts', ['discussion' => $discussion->id]); + $itemid = $post->id; + } + + // Create the file. + $filename = 'greeting-card-887.h5p'; + $path = __DIR__ . '/../../../h5p/tests/fixtures/' . $filename; + if ($filecomponent === 'contentbank') { + $generator = $this->getDataGenerator()->get_plugin_generator('core_contentbank'); + $contents = $generator->generate_contentbank_data( + 'contenttype_h5p', + 1, + (int)$users[$fileauthor]->id, + $context, + true, + $path + ); + $content = array_shift($contents); + $file = $content->get_file(); + } else { + $filerecord = [ + 'contextid' => $context->id, + 'component' => $filecomponent, + 'filearea' => $filearea, + 'itemid' => $itemid, + 'filepath' => '/', + 'filename' => basename($path), + 'userid' => $users[$fileauthor]->id, + ]; + $fs = get_file_storage(); + $file = $fs->create_file_from_pathname($filerecord, $path); + } + + // Check if the currentuser can edit the file. + $result = \mod_forum\h5p\canedit::can_edit_content($file); + $this->assertEquals($expected, $result); + } + + /** + * Data provider for test_can_edit_content(). + * + * @return array + */ + public function can_edit_content_provider(): array { + return [ + // Component = mod_forum. + 'mod_forum: Admin user is author' => [ + 'currentuser' => 'admin', + 'fileauthor' => 'admin', + 'filecomponent' => 'mod_forum', + 'expected' => true, + ], + 'mod_forum: Admin user, teacher is author' => [ + 'currentuser' => 'admin', + 'fileauthor' => 'teacher', + 'filecomponent' => 'mod_forum', + 'expected' => true, + ], + 'mod_forum: Teacher user, admin is author' => [ + 'currentuser' => 'teacher', + 'fileauthor' => 'admin', + 'filecomponent' => 'mod_forum', + 'expected' => true, + ], + 'mod_forum: Student user, teacher is author' => [ + 'currentuser' => 'student', + 'fileauthor' => 'teacher', + 'filecomponent' => 'mod_forum', + 'expected' => false, + ], + 'mod_forum/post: Admin user is author' => [ + 'currentuser' => 'admin', + 'fileauthor' => 'admin', + 'filecomponent' => 'mod_forum', + 'expected' => true, + 'filearea' => 'post', + ], + 'mod_forum/post: Teacher user, admin is author' => [ + 'currentuser' => 'teacher', + 'fileauthor' => 'admin', + 'filecomponent' => 'mod_forum', + 'expected' => true, + 'filearea' => 'post', + ], + 'mod_forum/post: Student user, teacher is author' => [ + 'currentuser' => 'student', + 'fileauthor' => 'teacher', + 'filecomponent' => 'mod_forum', + 'expected' => false, + 'filearea' => 'post', + ], + + // Component <> mod_forum. + 'mod_page: Admin user is author' => [ + 'currentuser' => 'admin', + 'fileauthor' => 'admin', + 'filecomponent' => 'mod_page', + 'expected' => false, + ], + + // Unexisting components. + 'Unexisting component' => [ + 'currentuser' => 'admin', + 'fileauthor' => 'admin', + 'filecomponent' => 'unexisting_component', + 'expected' => false, + ], + 'Unexisting module activity' => [ + 'currentuser' => 'admin', + 'fileauthor' => 'admin', + 'filecomponent' => 'mod_unexisting', + 'expected' => false, + ], + 'Unexisting block' => [ + 'currentuser' => 'admin', + 'fileauthor' => 'admin', + 'filecomponent' => 'block_unexisting', + 'expected' => false, + ], + ]; + } +} From 03af737f2160c484c5f5c1e3861a9d48e81e19ed Mon Sep 17 00:00:00 2001 From: Sara Arjona Date: Thu, 10 Jun 2021 17:10:01 +0200 Subject: [PATCH 2/2] MDL-71956 filter_h5p: Display the edit content button The H5P filter will display now the "Edit content" button if the user can edit the file. --- filter/displayh5p/filter.php | 66 ++++- .../behat/inline_editing_content.feature | 255 ++++++++++++++++++ .../behat/h5p_inline_editing_content.feature | 126 +++++++++ 3 files changed, 442 insertions(+), 5 deletions(-) create mode 100644 filter/displayh5p/tests/behat/inline_editing_content.feature create mode 100644 mod/forum/tests/behat/h5p_inline_editing_content.feature diff --git a/filter/displayh5p/filter.php b/filter/displayh5p/filter.php index f693b3c035b..9d38d2ea073 100644 --- a/filter/displayh5p/filter.php +++ b/filter/displayh5p/filter.php @@ -49,7 +49,7 @@ class filter_displayh5p extends moodle_text_filter { * @return string */ public function filter($text, array $options = array()) { - global $CFG; + global $CFG, $USER; if (!is_string($text) or empty($text)) { // Non string data can not be filtered anyway. @@ -83,7 +83,10 @@ class filter_displayh5p extends moodle_text_filter { // It is needed to add "/embed" at the end of URLs like https:://*.h5p.com/content/12345 (H5P.com). $params['urlmodifier'] = ''; - if (($source == $localsource)) { + // Local files may display a button below the content to modify it when editing mode is on. This button will appear + // only if the user has the proper capabilities. + $params['canbeedited'] = (!empty($USER->editing)) && ($source == $localsource); + if ($source == $localsource) { $params['tagbegin'] = '