MDL-72339 core_availability: Unsafe to use format_string in description

The availability condition get_description method is called while
gathering data for the modinfo object. As such it is not safe to
call other functions which might rely on modinfo, such as format_string
(if using filters which access modinfo).

This change provides a mechanism to call format_string later, and also
a general callback to do other stuff later as well if needed. It uses
the same approach already taken to make activity names work correctly
in the availability_condition class.
This commit is contained in:
sam marshall 2021-09-01 14:14:19 +01:00
parent 1a9bee69e6
commit 4dd7f2ac54
16 changed files with 275 additions and 19 deletions

View File

@ -108,6 +108,64 @@ abstract class condition extends tree_node {
return preg_replace('~^availability_(.*?)\\\\condition$~', '$1', get_class($this));
}
/**
* Returns a marker indicating that an activity name should be placed in a description.
*
* Gets placeholder text which will be decoded by info::format_info later when we can safely
* display names.
*
* @param int $cmid Course-module id
* @return string Placeholder text
* @since Moodle 4.0
*/
public static function description_cm_name(int $cmid): string {
return '<AVAILABILITY_CMNAME_' . $cmid . '/>';
}
/**
* Returns a marker indicating that formatted text should be placed in a description.
*
* Gets placeholder text which will be decoded by info::format_info later when we can safely
* call format_string.
*
* @param string $str Text to be processed with format_string
* @return string Placeholder text
* @since Moodle 4.0
*/
public static function description_format_string(string $str): string {
return '<AVAILABILITY_FORMAT_STRING>' . htmlspecialchars($str, ENT_NOQUOTES) .
'</AVAILABILITY_FORMAT_STRING>';
}
/**
* Returns a marker indicating that some of the description text should be computed at display
* time.
*
* This will result in a call to the get_description_callback_value static function within
* the condition class.
*
* Gets placeholder text which will be decoded by info::format_info later when we can safely
* call most Moodle functions.
*
* @param string[] $params Array of arbitrary parameters
* @return string Placeholder text
* @since Moodle 4.0
*/
public function description_callback(array $params): string {
$out = '<AVAILABILITY_CALLBACK type="' . $this->get_type() . '">';
$first = true;
foreach ($params as $param) {
if ($first) {
$first = false;
} else {
$out .= '<P/>';
}
$out .= htmlspecialchars($param, ENT_NOQUOTES);
}
$out .= '</AVAILABILITY_CALLBACK>';
return $out;
}
/**
* Obtains a string describing this restriction (whether or not
* it actually applies). Used to obtain information that is displayed to
@ -119,11 +177,17 @@ abstract class condition extends tree_node {
* (when displaying only conditions they don't meet).
*
* If implementations require a course or modinfo, they should use
* the get methods in $info.
* the get methods in $info. They should not use any other functions that
* might rely on modinfo, such as format_string.
*
* The special string <AVAILABILITY_CMNAME_123/> can be returned, where
* 123 is any number. It will be replaced with the correctly-formatted
* name for that activity.
* To work around this limitation, use the functions:
*
* description_cm_name()
* description_format_string()
* description_callback()
*
* These return special markers which will be added to the string and processed
* later after modinfo is complete.
*
* @param bool $full Set true if this is the 'full information' view
* @param bool $not Set true if we are inverting the condition
@ -142,11 +206,17 @@ abstract class condition extends tree_node {
* the list, in front of the standard get_description call.
*
* If implementations require a course or modinfo, they should use
* the get methods in $info.
* the get methods in $info. They should not use any other functions that
* might rely on modinfo, such as format_string.
*
* The special string <AVAILABILITY_CMNAME_123/> can be returned, where
* 123 is any number. It will be replaced with the correctly-formatted
* name for that activity.
* To work around this limitation, use the functions:
*
* description_cm_name()
* description_format_string()
* description_callback()
*
* These return special markers which will be added to the string and processed
* later after modinfo is complete.
*
* @param bool $full Set true if this is the 'full information' view
* @param bool $not Set true if we are inverting the condition

View File

@ -746,6 +746,26 @@ abstract class info {
return format_string($cm->get_name(), true, ['context' => $context]);
}
}, $info);
$info = preg_replace_callback('~<AVAILABILITY_FORMAT_STRING>(.*?)</AVAILABILITY_FORMAT_STRING>~s',
function($matches) use ($context) {
$decoded = htmlspecialchars_decode($matches[1], ENT_NOQUOTES);
return format_string($decoded, true, ['context' => $context]);
}, $info);
$info = preg_replace_callback('~<AVAILABILITY_CALLBACK type="([a-z0-9_]+)">(.*?)</AVAILABILITY_CALLBACK>~s',
function($matches) use ($modinfo, $context) {
// Find the class, it must have already been loaded by now.
$fullclassname = 'availability_' . $matches[1] . '\condition';
if (!class_exists($fullclassname, false)) {
return '<!-- Error finding class ' . $fullclassname .' -->';
}
// Load the parameters.
$params = [];
$encodedparams = preg_split('~<P/>~', $matches[2], 0);
foreach ($encodedparams as $encodedparam) {
$params[] = htmlspecialchars_decode($encodedparam, ENT_NOQUOTES);
}
return $fullclassname::get_description_callback_value($modinfo, $context, $params);
}, $info);
return $info;
}

View File

@ -51,7 +51,7 @@ class info_module extends info {
// We cannot access $cm->name as a property at this point, because this
// code may itself run in response to the $cm->name property access, and
// PHP magic function properties do not allow recursion (because PHP).
return '<AVAILABILITY_CMNAME_' . $this->cm->id . '/>';
return condition::description_cm_name($this->cm->id);
}
protected function set_in_database($availability) {

View File

@ -354,7 +354,7 @@ class condition extends \core_availability\condition {
if (!array_key_exists($cmid, $modinfo->cms) || $modinfo->cms[$cmid]->deletioninprogress) {
$modname = get_string('missing', 'availability_completion');
} else {
$modname = '<AVAILABILITY_CMNAME_' . $modinfo->cms[$cmid]->id . '/>';
$modname = self::description_cm_name($modinfo->cms[$cmid]->id);
}
}

View File

@ -142,10 +142,29 @@ class condition extends \core_availability\condition {
$string = 'notgeneral';
}
}
$name = self::get_cached_grade_name($course->id, $this->gradeitemid);
// We cannot get the name at this point because it requires format_string which is not
// allowed here. Instead, get it later with the callback function below.
$name = $this->description_callback([$this->gradeitemid]);
return get_string('requires_' . $string, 'availability_grade', $name);
}
/**
* Gets the grade name at display time.
*
* @param \course_modinfo $modinfo Modinfo
* @param \context $context Context
* @param string[] $params Parameters (just grade item id)
* @return string Text value
*/
public static function get_description_callback_value(
\course_modinfo $modinfo, \context $context, array $params): string {
if (count($params) !== 1 || !is_number($params[0])) {
return '<!-- Invalid grade description callback -->';
}
$gradeitemid = (int)$params[0];
return self::get_cached_grade_name($modinfo->get_course_id(), $gradeitemid);
}
protected function get_debug_string() {
$out = '#' . $this->gradeitemid;
if (!is_null($this->min)) {

View File

@ -123,3 +123,39 @@ Feature: availability_grade
Then I should see "P2" in the "region-main" "region"
And I should see "P4" in the "region-main" "region"
And I should not see "P3" in the "region-main" "region"
@javascript
Scenario: Condition display with filters
# Teacher sets up a restriction on group G1, using multilang filter.
Given the following "activity" exists:
| activity | assign |
| name | <span lang="en" class="multilang">A-One</span><span lang="fr" class="multilang">A-Un</span> |
| intro | Test |
| course | C1 |
| idnumber | 0001 |
| section | 1 |
And the "multilang" filter is "on"
And the "multilang" filter applies to "content and headings"
# The activity names filter is enabled because it triggered a bug in older versions.
And the "activitynames" filter is "on"
And the "activitynames" filter applies to "content and headings"
And I am on the "C1" "Course" page logged in as "teacher1"
And I turn editing mode on
And I add a "Page" to section "1"
And I expand all fieldsets
And I set the following fields to these values:
| Name | P1 |
| Description | x |
| Page content | x |
And I click on "Add restriction..." "button"
And I click on "Grade" "button" in the "Add restriction..." "dialogue"
And I set the field "Grade" to "A-One"
And I click on "min" "checkbox" in the ".availability-item" "css_element"
And I set the field "Minimum grade percentage (inclusive)" to "10"
And I press "Save and return to course"
And I log out
# Student sees information about no access to group, with group name in correct language.
When I am on the "C1" "Course" page logged in as "student1"
Then I should see "Not available unless: You achieve a required score in A-One"
And I should not see "A-Un"

View File

@ -68,6 +68,7 @@ class availability_grade_condition_testcase extends advanced_testcase {
// Check if available (not available).
$this->assertFalse($cond->is_available(false, $info, true, $user->id));
$information = $cond->get_description(false, false, $info);
$information = \core_availability\info::format_info($information, $course);
$this->assertMatchesRegularExpression('~have a grade.*Test!~', $information);
$this->assertTrue($cond->is_available(true, $info, true, $user->id));
@ -76,6 +77,7 @@ class availability_grade_condition_testcase extends advanced_testcase {
$this->assertTrue($cond->is_available(false, $info, true, $user->id));
$this->assertFalse($cond->is_available(true, $info, true, $user->id));
$information = $cond->get_description(false, true, $info);
$information = \core_availability\info::format_info($information, $course);
$this->assertMatchesRegularExpression('~do not have a grade.*Test!~', $information);
// Construct directly and test remaining conditions; first, min grade (fail).
@ -84,6 +86,7 @@ class availability_grade_condition_testcase extends advanced_testcase {
$cond = new condition($structure);
$this->assertFalse($cond->is_available(false, $info, true, $user->id));
$information = $cond->get_description(false, false, $info);
$information = \core_availability\info::format_info($information, $course);
$this->assertMatchesRegularExpression('~achieve a required score.*Test!~', $information);
$this->assertTrue($cond->is_available(true, $info, true, $user->id));
@ -92,6 +95,7 @@ class availability_grade_condition_testcase extends advanced_testcase {
$this->assertTrue($cond->is_available(false, $info, true, $user->id));
$this->assertFalse($cond->is_available(true, $info, true, $user->id));
$information = $cond->get_description(false, true, $info);
$information = \core_availability\info::format_info($information, $course);
$this->assertMatchesRegularExpression('~do not get certain scores.*Test!~', $information);
// Max grade (fail).
@ -100,6 +104,7 @@ class availability_grade_condition_testcase extends advanced_testcase {
$cond = new condition($structure);
$this->assertFalse($cond->is_available(false, $info, true, $user->id));
$information = $cond->get_description(false, false, $info);
$information = \core_availability\info::format_info($information, $course);
$this->assertMatchesRegularExpression('~get an appropriate score.*Test!~', $information);
$this->assertTrue($cond->is_available(true, $info, true, $user->id));
@ -108,6 +113,7 @@ class availability_grade_condition_testcase extends advanced_testcase {
$this->assertTrue($cond->is_available(false, $info, true, $user->id));
$this->assertFalse($cond->is_available(true, $info, true, $user->id));
$information = $cond->get_description(false, true, $info);
$information = \core_availability\info::format_info($information, $course);
$this->assertMatchesRegularExpression('~do not get certain scores.*Test!~', $information);
// Max and min (fail).
@ -116,6 +122,7 @@ class availability_grade_condition_testcase extends advanced_testcase {
$cond = new condition($structure);
$this->assertFalse($cond->is_available(false, $info, true, $user->id));
$information = $cond->get_description(false, false, $info);
$information = \core_availability\info::format_info($information, $course);
$this->assertMatchesRegularExpression('~get a particular score.*Test!~', $information);
$this->assertTrue($cond->is_available(true, $info, true, $user->id));
@ -128,6 +135,7 @@ class availability_grade_condition_testcase extends advanced_testcase {
$this->assertTrue($cond->is_available(false, $info, true, $user->id));
$this->assertFalse($cond->is_available(true, $info, true, $user->id));
$information = $cond->get_description(false, true, $info);
$information = \core_availability\info::format_info($information, $course);
$this->assertMatchesRegularExpression('~do not get certain scores.*Test!~', $information);
// Success (bottom end).
@ -135,6 +143,7 @@ class availability_grade_condition_testcase extends advanced_testcase {
$this->assertTrue($cond->is_available(false, $info, true, $user->id));
$this->assertFalse($cond->is_available(true, $info, true, $user->id));
$information = $cond->get_description(false, true, $info);
$information = \core_availability\info::format_info($information, $course);
$this->assertMatchesRegularExpression('~do not get certain scores.*Test!~', $information);
}

View File

@ -110,9 +110,8 @@ class condition extends \core_availability\condition {
if (!array_key_exists($this->groupid, self::$groupnames)) {
$name = get_string('missing', 'availability_group');
} else {
$context = \context_course::instance($course->id);
$name = format_string(self::$groupnames[$this->groupid], true,
array('context' => $context));
// Not safe to call format_string here; use the special function to call it later.
$name = self::description_format_string(self::$groupnames[$this->groupid]);
}
} else {
return get_string($not ? 'requires_notanygroup' : 'requires_anygroup',

View File

@ -101,3 +101,33 @@ Feature: availability_group
Then I should see "P1" in the "region-main" "region"
And I should see "P2" in the "region-main" "region"
And I should not see "P3" in the "region-main" "region"
@javascript
Scenario: Condition display with filters
# Teacher sets up a restriction on group G1, using multilang filter.
Given the following "groups" exist:
| name | course | idnumber |
| <span lang="en" class="multilang">G-One</span><span lang="fr" class="multilang">G-Un</span> | C1 | GI1 |
And the "multilang" filter is "on"
And the "multilang" filter applies to "content and headings"
# The activity names filter is enabled because it triggered a bug in older versions.
And the "activitynames" filter is "on"
And the "activitynames" filter applies to "content and headings"
And I am on the "C1" "Course" page logged in as "teacher1"
And I turn editing mode on
And I add a "Page" to section "1"
And I expand all fieldsets
And I set the following fields to these values:
| Name | P1 |
| Description | x |
| Page content | x |
And I click on "Add restriction..." "button"
And I click on "Group" "button" in the "Add restriction..." "dialogue"
And I set the field "Group" to "G-One"
And I click on "Save and return to course" "button"
And I log out
# Student sees information about no access to group, with group name in correct language.
When I am on the "C1" "Course" page logged in as "student1"
Then I should see "Not available unless: You belong to G-One"
And I should not see "G-Un"

View File

@ -73,6 +73,7 @@ class availability_group_condition_testcase extends advanced_testcase {
// Check if available (when not available).
$this->assertFalse($cond->is_available(false, $info, true, $user->id));
$information = $cond->get_description(false, false, $info);
$information = \core_availability\info::format_info($information, $course);
$this->assertMatchesRegularExpression('~You belong to.*G1!~', $information);
$this->assertTrue($cond->is_available(true, $info, true, $user->id));
@ -85,6 +86,7 @@ class availability_group_condition_testcase extends advanced_testcase {
$this->assertTrue($cond->is_available(false, $info, true, $user->id));
$this->assertFalse($cond->is_available(true, $info, true, $user->id));
$information = $cond->get_description(false, true, $info);
$information = \core_availability\info::format_info($information, $course);
$this->assertMatchesRegularExpression('~do not belong to.*G1!~', $information);
// Check group 2 works also.
@ -96,6 +98,7 @@ class availability_group_condition_testcase extends advanced_testcase {
$this->assertTrue($cond->is_available(false, $info, true, $user->id));
$this->assertFalse($cond->is_available(true, $info, true, $user->id));
$information = $cond->get_description(false, true, $info);
$information = \core_availability\info::format_info($information, $course);
$this->assertMatchesRegularExpression('~do not belong to any~', $information);
// Admin user doesn't belong to a group, but they can access it
@ -108,6 +111,7 @@ class availability_group_condition_testcase extends advanced_testcase {
$cond = new condition((object)array('id' => $group2->id + 1000));
$this->assertFalse($cond->is_available(false, $info, true, $user->id));
$information = $cond->get_description(false, false, $info);
$information = \core_availability\info::format_info($information, $course);
$this->assertMatchesRegularExpression('~You belong to.*\(Missing group\)~', $information);
}

View File

@ -141,9 +141,8 @@ class condition extends \core_availability\condition {
if (!array_key_exists($groupingid, self::$groupingnames)) {
$name = get_string('missing', 'availability_grouping');
} else {
$context = \context_course::instance($course->id);
$name = format_string(self::$groupingnames[$groupingid], true,
array('context' => $context));
// Not safe to call format_string here; use the special function to call it later.
$name = self::description_format_string(self::$groupingnames[$groupingid]);
}
return get_string($not ? 'requires_notgrouping' : 'requires_grouping',

View File

@ -114,3 +114,31 @@ Feature: availability_grouping
And I press "Add group/grouping access restriction"
When I press "Save and return to course"
Then I should see "Not available unless: You belong to a group in Grouping A"
@javascript
Scenario: Condition display with filters
# Teacher sets up a restriction on group G1, using multilang filter.
Given the following "groupings" exist:
| name | course | idnumber |
| <span lang="en" class="multilang">Gr-One</span><span lang="fr" class="multilang">Gr-Un</span> | C1 | GA |
And the following "activities" exist:
| activity | name | intro | course | idnumber | groupmode | grouping |
| assign | Test assign | Assign description | C1 | assign1 | 1 | GA |
And the "multilang" filter is "on"
And the "multilang" filter applies to "content and headings"
# The activity names filter is enabled because it triggered a bug in older versions.
And the "activitynames" filter is "on"
And the "activitynames" filter applies to "content and headings"
And I am on the "C1" "Course" page logged in as "teacher1"
And I turn editing mode on
And I open "Test assign" actions menu
And I choose "Edit settings" in the open action menu
And I expand all fieldsets
And I press "Add group/grouping access restriction"
And I press "Save and return to course"
And I log out
# Student sees information about no access to group, with group name in correct language.
When I am on the "C1" "Course" page logged in as "student1"
Then I should see "Not available unless: You belong to a group in Gr-One"
And I should not see "Gr-Un"

View File

@ -199,9 +199,9 @@ class condition extends \core_availability\condition {
} else {
$translatedfieldname = \core_user\fields::get_display_name($this->standardfield);
}
$context = \context_course::instance($course->id);
$a = new \stdClass();
$a->field = format_string($translatedfieldname, true, array('context' => $context));
// Not safe to call format_string here; use the special function to call it later.
$a->field = self::description_format_string($translatedfieldname);
$a->value = s($this->value);
if ($not) {
// When doing NOT strings, we replace the operator with its inverse.

View File

@ -106,3 +106,35 @@ Feature: availability_profile
And I log in as "student1"
And I am on "Course 1" course homepage
Then I should see "P1" in the "region-main" "region"
@javascript
Scenario: Condition display with filters
# Teacher sets up a restriction on group G1, using multilang filter.
Given the following "custom profile fields" exist:
| datatype | shortname | name | param2 |
| text | frog | <span lang="en" class="multilang">F-One</span><span lang="fr" class="multilang">F-Un</span> | 100 |
And the "multilang" filter is "on"
And the "multilang" filter applies to "content and headings"
# The activity names filter is enabled because it triggered a bug in older versions.
And the "activitynames" filter is "on"
And the "activitynames" filter applies to "content and headings"
And I am on the "C1" "Course" page logged in as "teacher1"
And I turn editing mode on
And I add a "Page" to section "1"
And I expand all fieldsets
And I set the following fields to these values:
| Name | P1 |
| Description | x |
| Page content | x |
And I click on "Add restriction..." "button"
And I click on "User profile" "button" in the "Add restriction..." "dialogue"
And I set the following fields to these values:
| User profile field | F-One |
| Value to compare against | 111 |
And I click on "Save and return to course" "button"
And I log out
# Student sees information about no access to group, with group name in correct language.
When I am on the "C1" "Course" page logged in as "student1"
Then I should see "Not available unless: Your F-One is 111"
And I should not see "F-Un"

View File

@ -309,6 +309,7 @@ class availability_profile_condition_testcase extends advanced_testcase {
// Check the message (should be using lang string with capital, which
// is evidence that it called the right function to get the name).
$information = $cond->get_description(false, false, $info);
$information = \core_availability\info::format_info($information, $info->get_course());
$this->assertMatchesRegularExpression('~Department~', $information);
// Set the field to true for both users and retry.
@ -388,6 +389,7 @@ class availability_profile_condition_testcase extends advanced_testcase {
'Failed checking normal (positive) result');
if (!$yes) {
$information = $cond->get_description(false, false, $info);
$information = \core_availability\info::format_info($information, $info->get_course());
$this->assertMatchesRegularExpression($failpattern, $information);
}
@ -396,6 +398,7 @@ class availability_profile_condition_testcase extends advanced_testcase {
'Failed checking NOT (negative) result');
if ($yes) {
$information = $cond->get_description(false, true, $info);
$information = \core_availability\info::format_info($information, $info->get_course());
$this->assertMatchesRegularExpression($failpattern, $information);
}
}

View File

@ -2,6 +2,13 @@ This files describes API changes in /availability/*.
The information here is intended only for developers.
=== 4.0 ===
* There were existing restrictions on what condition plugins can do in the get_description
method (for example they mustn't call format_string), which were not well documented.
New functions description_cm_name(), description_format_string(), description_callback()
can be used so that condition plugins to behave correctly in all situations.
=== 3.2 ===
* Condition plugins must replace the CSS selector "#fitem_id_availabilityconditionsjson" with ".availability-field".