Restriction by course completion
availability_coursecompleted
Availability condition that restricts access to course modules and sections based on the completion status of the current course or another course. Provides a frontend form for choosing whether completion is required or must not yet have occurred and, optionally, which course's completion is referenced.
The plugin is small, well-scoped and follows Moodle conventions throughout. It uses $DB parameterised queries, escapes output with format_string/html_writer, ships a null_provider privacy implementation, has solid PHPUnit and Behat coverage, and contains no direct database connections, file-system bypasses, raw HTTP requests, or capability gaps. The findings are all low-severity code-quality and robustness concerns: reliance on the internal structure of the core coursecompletion cache, direct format_string use in get_description rather than the deferred description_format_string() helper, an unhandled exception path when a referenced course has been deleted (get_description and get_debug_string), an N+1 query pattern in allow_add, malformed HTML attribute quoting in the YUI form template, and a few minor documentation/lang-string hygiene issues. None expose user data or alter state in a way an attacker could exploit.
Overview
availability_coursecompleted is a small Moodle availability condition (~220 lines of production PHP plus a YUI form module) that lets editing teachers gate activities or sections behind completion of the current course or another course the user is enrolled in.
Security posture
No exploitable vulnerabilities were identified.
- SQL — The single hand-written query in
condition::filter_user_list()uses placeholders for the only user-derived value ($courseid) and interpolates only a hard-coded'NOT'/''literal based on a boolean property. The other database calls go through$DB->record_exists()/$DB->get_field()/$DB->insert_record()with array parameters. - XSS — Course names are passed through
format_string()before being sent to JavaScript and embedded in<option>content. The HTML link inget_descriptionis built withhtml_writer::link()andmoodle_url. - Access control — The plugin defines no entry-point scripts and never writes user data. All operations run inside the core
core_availabilityevaluator, which is responsible for the surrounding authentication and capability checks. The frontend'sallow_addis purely a UI gate. - External I/O — No filesystem, network, shell, or schema operations.
- Privacy — Declares a
null_providerand stores nothing of its own.
Code quality observations
The condition reads from the core coursecompletion cache directly and depends on the cache value being an array shaped like ['value' => $obj] (it dereferences with current($values)). This is the current internal layout in completion_completion::fetch() but is not part of any public contract. Because the fallback path already uses the same cache via completion_info::is_course_complete(), the manual lookup is a brittle micro-optimisation rather than a real performance win.
get_description() calls format_string($course->shortname|fullname) directly. Core's documentation on core_availability\condition::get_description recommends description_format_string() so that formatting is deferred until modinfo is fully resolved; the bundled availability_grouping is an example of the recommended pattern.
get_description() and get_debug_string() call get_course($this->courseid) without the try/catch used in is_available(). If a referenced course is deleted, these methods will throw dml_missing_record_exception instead of returning a graceful fallback.
frontend::allow_add() iterates every enrolled course and issues a per-course record_exists query. Replaced with a single JOIN, this would be cheaper for users enrolled in many courses.
The YUI form template emits title=' + tit + ' without quoting the attribute value; because the string is Course completed (with a space), the rendered HTML degrades to title="Course" completed. Not exploitable, but the title attribute does not function as intended.
The language file ships four unused requires_* strings, and README.md still states the minimum required Moodle version as 4.02 while version.php requires the 5.x line.
Tests and tooling
The plugin ships extensive PHPUnit tests (basic, advanced, backup, behat helper, privacy), four Behat feature files, mutation testing configuration, GitHub Actions workflows that lint, run code-checker, PHPUnit, and Behat against multiple Moodle branches and PHP versions. There are no bundled third-party libraries and no thirdpartylibs.xml is required.
Findings
condition::is_available() reads the core/coursecompletion cache directly and dereferences the returned value with current($values). This works only because completion_completion::fetch() happens to store entries as ['value' => $completion_or_false]. That layout is an internal implementation detail of core, not a documented contract.
If core ever changes the cache to store the completion_completion object (or any other non-array value) directly — which would be a backward-compatible change inside core — current() on a non-array would emit a PHP warning and the truthiness check would mis-classify completion state.
The fallback branch already calls completion_info::is_course_complete(), which internally goes through the same cache via completion_completion::fetch(), so the manual lookup is mostly a robustness liability with little real performance gain. Calling completion_completion::fetch(['userid' => $userid, 'course' => $course->id]) directly would give the same cache benefit through the public API.
Low risk. No data corruption or security implication. Worst-case effect of a future core cache layout change would be the condition silently mis-reporting completion state (or emitting a PHP warning) until the plugin is updated. The plugin's own test suite seeds the cache with arbitrary scalar values ('pseudo') to exercise this code path, suggesting the author is aware of the fragility.
The core/coursecompletion cache is populated by completion_completion::fetch() in /moodle/public/completion/completion_completion.php which stores each per-user/course entry as ['value' => $tocache] where $tocache is either a completion_completion object or false. The condition relies on current($values) returning that inner value, which is true today but not guaranteed.
$cache = \cache::make('core', 'coursecompletion');
try {
$course = $this->courseid === 0 ? $info->get_course() : get_course($this->courseid);
} catch (\exception) {
// Deleted courses alwas return false.
return false;
}
$values = $cache->get("{$userid}_{$course->id}");
if ($values && $value = current($values)) {
$allow = (bool)$value->timecompleted;
} else {
$completioninfo = new \completion_info($course);
$allow = $completioninfo->is_course_complete($userid);
unset($completioninfo);
}
Use the public API, which uses the cache internally and is not tied to the cache value's internal shape:
try {
$course = $this->courseid === 0 ? $info->get_course() : get_course($this->courseid);
} catch (\dml_missing_record_exception) {
return false;
}
$completion = \completion_completion::fetch(['userid' => $userid, 'course' => $course->id]);
$allow = ($completion instanceof \completion_completion) && $completion->is_complete();
The base class core_availability\condition::get_description() documents that callers should not invoke format_string() directly because formatting may depend on modinfo being fully resolved (for example during AJAX rebuilds or activity duplication). Conditions are expected to defer formatting via self::description_format_string() or self::description_callback(), and the rendering layer applies the format later when modinfo is safe to use. The core availability_grouping condition follows this pattern explicitly (/moodle/public/availability/condition/grouping/classes/condition.php line 144 — "Not safe to call format_string here; use the special function to call it later.").
This plugin calls format_string directly on the referenced course's shortname/fullname, then wraps it in an html_writer::link() call and feeds the resulting HTML to get_string().
Low risk. In practice, calling format_string against a different course than the current one tends to work because the formatter uses the calling context. However, deviating from the documented pattern can surface bugs in AJAX duplication and similar flows where modinfo has not been finalised, and aligns the plugin with the conventions used by core availability plugins.
Availability descriptions are rendered both for staff (the editing UI / availability info widget) and for students (when an activity is hidden). The info::format_info() pipeline post-processes the placeholder tokens emitted by description_format_string, so the recommended pattern is to defer formatting rather than call it inline.
$course = get_course($this->courseid);
$name = empty($CFG->navshowfullcoursenames) ? format_string($course->shortname) : format_string($course->fullname);
$url = \html_writer::link(new moodle_url('/course/view.php', ['id' => $this->courseid]), $name);
return get_string($allow ? 'getotherdescription' : 'getotherdescriptionnot', 'availability_coursecompleted', $url);
Defer the formatting using the helper provided by the base class. The link should be built around a placeholder that the formatting pipeline expands later:
$course = get_course($this->courseid);
$rawname = empty($CFG->navshowfullcoursenames) ? $course->shortname : $course->fullname;
$name = self::description_format_string($rawname);
$url = \html_writer::link(new moodle_url('/course/view.php', ['id' => $this->courseid]), $name);
return get_string($allow ? 'getotherdescription' : 'getotherdescriptionnot', 'availability_coursecompleted', $url);
condition::is_available() wraps the call to get_course($this->courseid) in try/catch so a deleted referenced course results in the condition simply evaluating to false. condition::get_description() and condition::get_debug_string() make the same call without protection. get_course() ultimately uses $DB->get_record('course', ..., MUST_EXIST) and throws dml_missing_record_exception when the course is missing.
Result: as long as the activity is restricted to completion of an external course, deleting that course will keep is_available consistent (always false) but break any page that needs to render the activity's restriction description, including the editing UI and the student's "Not available unless..." message.
Low risk. No security impact; the failure surfaces as an exception/debug page rather than data leakage. Affected functionality is limited to administrative UI for activities whose referenced course has been deleted, but the recovery path requires direct database surgery to clean up the orphan restriction.
The plugin already deals with the deleted-course case in is_available(), but the description and debug paths assume the referenced course exists. The asymmetry only manifests after a course referenced by an availability restriction is deleted, which can happen long after the restriction was created.
if ($this->courseid === 0) {
return get_string($allow ? 'getdescription' : 'getdescriptionnot', 'availability_coursecompleted');
}
$course = get_course($this->courseid);
$name = empty($CFG->navshowfullcoursenames) ? format_string($course->shortname) : format_string($course->fullname);
$url = \html_writer::link(new moodle_url('/course/view.php', ['id' => $this->courseid]), $name);
return get_string($allow ? 'getotherdescription' : 'getotherdescriptionnot', 'availability_coursecompleted', $url);
Mirror the defensive pattern from is_available() — return a sensible string (for example, get_string('missing', 'availability_coursecompleted')) when the referenced course no longer exists:
try {
$course = get_course($this->courseid);
} catch (\dml_missing_record_exception) {
return get_string('missing', 'availability_coursecompleted');
}
protected function get_debug_string() {
if ($this->courseid === 0) {
return get_string($this->completed ? 'true' : 'false', 'mod_quiz');
}
$name = format_string(get_course($this->courseid)->shortname);
return get_string($this->completed ? 'true' : 'false', 'mod_quiz') . ' ' . $name;
}
Guard against a deleted course:
protected function get_debug_string() {
if ($this->courseid === 0) {
return get_string($this->completed ? 'true' : 'false', 'mod_quiz');
}
try {
$shortname = format_string(get_course($this->courseid)->shortname);
} catch (\dml_missing_record_exception) {
$shortname = '(deleted course #' . $this->courseid . ')';
}
return get_string($this->completed ? 'true' : 'false', 'mod_quiz') . ' ' . $shortname;
}
frontend::allow_add() fetches every enrolled course for the current user with enrol_get_users_courses() and then issues $DB->record_exists('course_completion_criteria', ['course' => $course->id]) once per course inside the loop. For users enrolled in many courses (which is common for managers and teachers across faculties), this produces a query per course on every page that loads the activity restriction UI.
A single JOIN against course × course_completion_criteria returns the same answer in one round-trip.
Low risk. Functionality is correct; the cost only matters on installations with users enrolled in many completion-enabled courses. There is no DoS-class concern because the user must already be authenticated and have rights to edit the activity restriction UI.
allow_add() is invoked from core_availability\frontend::include_all_javascript() whenever the availability editor is rendered for a course or activity, so it runs on the activity-edit page for editing teachers and managers.
protected function allow_add($course, ?cm_info $cm = null, ?section_info $section = null) {
global $DB, $USER;
$courses = enrol_get_users_courses($USER->id, true, 'id, enablecompletion');
foreach ($courses as $course) {
if ($course->enablecompletion == 1) {
if ($DB->record_exists('course_completion_criteria', ['course' => $course->id])) {
return true;
}
}
}
return is_siteadmin();
}
Collect the candidate course ids first, then issue one query:
protected function allow_add($course, ?cm_info $cm = null, ?section_info $section = null) {
global $DB, $USER;
if (is_siteadmin()) {
return true;
}
$courses = enrol_get_users_courses($USER->id, true, 'id, enablecompletion');
$ids = array_keys(array_filter($courses, fn($c) => $c->enablecompletion == 1));
if (!$ids) {
return false;
}
[$insql, $params] = $DB->get_in_or_equal($ids);
return $DB->record_exists_select('course_completion_criteria', "course $insql", $params);
}
The first <select> rendered by getNode() has title=' + tit + ' without quotes around the attribute value. The translated string is Course completed (with a space), so the browser parses this as two attributes: title="Course" and a boolean attribute completed. The second select in the same function uses a literal value with quotes (title="courses") and is correct.
The practical effect is that the tooltip for the first select shows only the first word. There is no XSS exposure because the string comes from a hard-coded language identifier (not user input).
Informational. UI quality issue: the tooltip on the Yes/No select displays "Course" instead of "Course completed". No security implications.
The form template is built by string concatenation and parsed by Y.Node.create. Because tit is the literal output of M.util.get_string('title', 'availability_coursecompleted') — controlled by the language file, not by user input — there is no injection risk.
html += '<span class="availability-coursecompleted"><select class="form-select" name="id" title=' + tit + '>';
Quote the attribute value:
html += '<span class="availability-coursecompleted"><select class="form-select" name="id" title="' + tit + '">';
The regenerated YUI build files (yui/build/.../*.js and the minified variants) carry the same bug and need to be rebuilt after the source change.
html += '<span class="availability-coursecompleted"><select class="form-select" name="id" title=' + tit + '>';
Regenerate via grunt shifter after fixing yui/src/form/js/form.js.
Four language strings are defined but never consumed by the plugin or by core. condition::get_description() uses getdescription / getdescriptionnot / getotherdescription / getotherdescriptionnot, so the requires_* variants below appear to be legacy and translatable but orphaned. Carrying them forces translators to translate strings that never reach users.
Informational. Pure hygiene; no security or functional impact.
Verified by grepping the plugin source and the local Moodle 5.x core tree (/moodle/public/...); no caller references any of the four strings.
$string['requires_completed'] = 'You completed this course.';
$string['requires_notcompleted'] = 'You did <b>not</b> complete this course.';
$string['requires_othercompleted'] = 'You completed course: {$a}.';
$string['requires_othernotcompleted'] = 'You did <b>not</b> complete course: {$a}';
Remove the four unused requires_* strings, or — if they are intentionally kept for backwards compatibility — record that intent in a comment so future translators and maintainers know.
README.md states This plugin requires Moodle 4.02+, but version.php declares $plugin->requires = 2025100600; (Moodle 5.1) and $plugin->supported = [501, 502];. Users following the README will assume the plugin still works on 4.x.
Informational. Documentation inaccuracy only.
The CHANGES.md log additionally only goes up to v5.3.1 (2025-01-07) while version.php reports release v5.5.1 / version 2026050900 — the changelog has also fallen behind, but the README requirement claim is the more immediate user-facing problem.
## Requirements
This plugin requires Moodle 4.02+
Update the line to reflect the actual minimum supported branch, e.g. This plugin requires Moodle 5.1+.
Privacy: The plugin correctly declares \core_privacy\local\metadata\null_provider because it stores no user data of its own — it only reads from core's course_completions table. The privacy unit test covers this and the lang string privacy:metadata is present.
Test coverage: PHPUnit suites cover the constructor, JSON round-trip, debug string, description, user-list filtering and the deleted-course path; Behat features cover module, section, other-course and fast-completion scenarios. Infection mutation testing is wired up via .infection.json5.
Cache structure tests: tests/advanced_test.php deliberately seeds the core/coursecompletion cache with scalar values ('pseudo') to exercise the defensive current($values) path. This confirms the author considered cache poisoning of this entry; combined with finding #1, the safer fix is to drop the manual lookup in favour of completion_completion::fetch().