From 84d7c6ce6dcb7586b492c597ae34132377691e6d Mon Sep 17 00:00:00 2001 From: Mark Johnson Date: Mon, 7 Jul 2025 15:28:19 +0100 Subject: [PATCH 1/3] MDL-85754 question: Only use current context in question_edit_contexts Due to the changes in question sharing, it no longer makes sense to look at parent contexts when establishing permissions in the question bank. Questions now belong to a single activity module, and we should be able to inherit and override permisions at that level like we do with any other. In the longer term, we probably want to replace question_edit_contexts with something simpler, but for the time being this should apply the correct permission checks where it's used without requiring a big refactor of existing code. --- .upgradenotes/MDL-85754-2025070810511425.yml | 11 +++++++++++ .../classes/local/bank/question_edit_contexts.php | 2 +- .../local/bank/context_to_string_translator_test.php | 2 +- 3 files changed, 13 insertions(+), 2 deletions(-) create mode 100644 .upgradenotes/MDL-85754-2025070810511425.yml diff --git a/.upgradenotes/MDL-85754-2025070810511425.yml b/.upgradenotes/MDL-85754-2025070810511425.yml new file mode 100644 index 00000000000..50c107ece6e --- /dev/null +++ b/.upgradenotes/MDL-85754-2025070810511425.yml @@ -0,0 +1,11 @@ +issueNumber: MDL-85754 +notes: + core_question: + - message: >- + `question_edit_contexts` now only considers the provided context when + checking permissions, rather than all parent contexts as well. As + questions now exist only at the activity module context level, + permissions can be inherited or overridden as normal for each question + bank. The previous pattern of checking for a permission in any parent + context circumvented the override system, and no longer makes sense. + type: changed diff --git a/question/classes/local/bank/question_edit_contexts.php b/question/classes/local/bank/question_edit_contexts.php index c3776223c34..392fc015619 100644 --- a/question/classes/local/bank/question_edit_contexts.php +++ b/question/classes/local/bank/question_edit_contexts.php @@ -66,7 +66,7 @@ class question_edit_contexts { * @param \context $thiscontext the current context. */ public function __construct(\context $thiscontext) { - $this->allcontexts = array_values($thiscontext->get_parent_contexts(true)); + $this->allcontexts = [$thiscontext]; } /** diff --git a/question/tests/local/bank/context_to_string_translator_test.php b/question/tests/local/bank/context_to_string_translator_test.php index 1eef4667da6..c55a24c5640 100644 --- a/question/tests/local/bank/context_to_string_translator_test.php +++ b/question/tests/local/bank/context_to_string_translator_test.php @@ -50,7 +50,7 @@ final class context_to_string_translator_test extends \advanced_testcase { $quizcontext = context_module::instance($quiz->cmid); // Create the context_to_string_translator. - $translator = new context_to_string_translator((new question_edit_contexts($quizcontext))->all()); + $translator = new context_to_string_translator([$systemcontext, $categorycontext, $coursecontext, $quizcontext]); // Verify its behaviour. $this->assertEquals('module', $translator->context_to_string($quizcontext->id)); From 563280c99a4c2b246f189aee066edfc7b101a14f Mon Sep 17 00:00:00 2001 From: Mark Johnson Date: Mon, 7 Jul 2025 15:31:12 +0100 Subject: [PATCH 2/3] MDL-85754 question: Don't show banks where user has no permission The Switch Banks UI was listing all question banks on the course where the use had the managecategory permission, plus the other banks they had used recently. This conflicted with the check when it tried to load a selected bank, which checked the useall/usemine permissions. This could lead to errors if this permission had been removed. This applies consistent checks to both lists with the useall/usermine permissions instead. --- question/classes/local/bank/question_bank_helper.php | 4 ++++ question/classes/output/switch_question_bank.php | 5 +++-- .../tests/local/bank/question_bank_helper_test.php | 11 +++++++++++ 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/question/classes/local/bank/question_bank_helper.php b/question/classes/local/bank/question_bank_helper.php index eb193835c7f..417c2b9b65c 100644 --- a/question/classes/local/bank/question_bank_helper.php +++ b/question/classes/local/bank/question_bank_helper.php @@ -339,6 +339,7 @@ class question_bank_helper { int $userid, int $notincourseid = 0, ?context $filtercontext = null, + array $havingcap = [], ): array { $prefs = get_user_preferences(self::RECENTLY_VIEWED, null, $userid); $contextids = !empty($prefs) ? explode(',', $prefs) : []; @@ -360,6 +361,9 @@ class question_bank_helper { if (!empty($notincourseid) && $notincourseid == $cm->course) { continue; } + if (!empty($havingcap) && !(new question_edit_contexts($context))->have_one_cap($havingcap)) { + continue; + } $record = self::get_formatted_bank($cm, filtercontext: $filtercontext); $banks[] = $record; } diff --git a/question/classes/output/switch_question_bank.php b/question/classes/output/switch_question_bank.php index ab64b51994a..b958249be8b 100644 --- a/question/classes/output/switch_question_bank.php +++ b/question/classes/output/switch_question_bank.php @@ -62,12 +62,13 @@ class switch_question_bank implements \renderable, \templatable { [, $cm] = get_module_from_cmid($this->quizcmid); $cminfo = cm_info::create($cm); + $capabilities = ['moodle/question:useall', 'moodle/question:usemine']; $coursesharedbanks = question_bank_helper::get_activity_instances_with_shareable_questions( incourseids: [$this->courseid], - havingcap: ['moodle/question:managecategory'], + havingcap: $capabilities, filtercontext: $cminfo->context, ); - $recentlyviewedbanks = question_bank_helper::get_recently_used_open_banks($this->userid); + $recentlyviewedbanks = question_bank_helper::get_recently_used_open_banks($this->userid, havingcap: $capabilities); return [ 'quizname' => $cminfo->get_formatted_name(), diff --git a/question/tests/local/bank/question_bank_helper_test.php b/question/tests/local/bank/question_bank_helper_test.php index 36a4ec9d88c..0d9e02a29c1 100644 --- a/question/tests/local/bank/question_bank_helper_test.php +++ b/question/tests/local/bank/question_bank_helper_test.php @@ -292,6 +292,7 @@ final class question_bank_helper_test extends \advanced_testcase { $user = self::getDataGenerator()->create_user(); $course1 = self::getDataGenerator()->create_course(); $course2 = self::getDataGenerator()->create_course(); + self::getDataGenerator()->enrol_user($user->id, $course1->id, 'editingteacher'); $banks = []; $banks[] = self::getDataGenerator()->create_module('qbank', ['course' => $course1->id]); $banks[] = self::getDataGenerator()->create_module('qbank', ['course' => $course1->id]); @@ -313,6 +314,16 @@ final class question_bank_helper_test extends \advanced_testcase { // Check that the courseid filter works. $recentlyviewed = question_bank_helper::get_recently_used_open_banks($user->id, $course1->id); $this->assertCount(3, $recentlyviewed); + // We should have the viewed banks in course 2. + $courseviewed = array_slice($banks, 3, 3); + $this->assertEqualsCanonicalizing(array_column($recentlyviewed, 'modid'), array_column($courseviewed, 'cmid')); + + // Check that the capability filter works. + $recentlyviewed = question_bank_helper::get_recently_used_open_banks($user->id, havingcap: ['moodle/question:useall']); + $this->assertCount(2, $recentlyviewed); + // We should have the 2 most recently viewed banks in course 1. + $capabilityviewed = array_slice($banks, 1, 2); + $this->assertEqualsCanonicalizing(array_column($recentlyviewed, 'modid'), array_column($capabilityviewed, 'cmid')); $recentlyviewed = question_bank_helper::get_recently_used_open_banks($user->id); From d0a3348bf0b9038e9ad95e4e73801aab521b82c2 Mon Sep 17 00:00:00 2001 From: Mark Johnson Date: Mon, 7 Jul 2025 15:31:53 +0100 Subject: [PATCH 3/3] MDL-85754 quiz: Test for permission checks in "Add from question bank" --- .../editing_add_from_question_bank.feature | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/mod/quiz/tests/behat/editing_add_from_question_bank.feature b/mod/quiz/tests/behat/editing_add_from_question_bank.feature index 33fb35b7a48..e5c72df50cf 100644 --- a/mod/quiz/tests/behat/editing_add_from_question_bank.feature +++ b/mod/quiz/tests/behat/editing_add_from_question_bank.feature @@ -216,3 +216,32 @@ Feature: Adding questions to a quiz from the question bank When I am on the "Quiz 1" "mod_quiz > Edit" page logged in as teacher1 Then I should see "Question Bank A" in the "TF1" "list_item" And I should see "Question Bank B" in the "TF2" "list_item" + + Scenario: User doesn't see the option to switch to a bank they can't use + Given the "multilang" filter is "on" + And the "multilang" filter applies to "content and headings" + And the following "users" exist: + | username | firstname | lastname | email | + | teacher2 | Teacher | 2 | teacher2@example.com | + And the following "role" exists: + | shortname | noquestions | + | name | Cannot use questions | + | moodle/question:usemine | prohibit | + | moodle/question:useall | prohibit | + And the following "course enrolments" exist: + | user | course | role | + | teacher2 | C1 | editingteacher | + And I am on the "Quiz 1" "mod_quiz > Edit" page logged in as "teacher2" + And I open the "last" add to quiz menu + And I follow "from question bank" + And I click on "Switch bank" "button" + And I click on "Qbank 1 & < > \" ' &" "link" in the "Select question bank" "dialogue" + And I click on "Select" "checkbox" in the "question 03 name" "table_row" + And I click on "Add selected questions to the quiz" "button" + When the following "role assigns" exist: + | user | role | contextlevel | reference | + | teacher2 | noquestions | Activity module | qbank1 | + And I open the "Page 1" add to quiz menu + And I follow "from question bank" + And I click on "Switch bank" "button" + Then "Qbank 1 & < > \" ' &" "link" should not exist in the "Select question bank" "dialogue"