Training credits
tool_mutrain
Training credits plugin (tool_mutrain) for Moodle LMS. Provides a framework system for tracking and aggregating training credits earned through course completions and program completions. Supports multiple credit frameworks with configurable requirements, context restrictions, date restrictions, and public/private access. Integrates with the MuTMS plugin suite (tool_mulib, customfield_mutrain, tool_muprog). Features include management UI for frameworks and fields, user-facing credit reports, event-driven completion syncing, scheduled cron task for cache maintenance, and report builder integration.
The plugin demonstrates strong security practices throughout: all pages require authentication via require_login(), authorization is enforced via require_capability() before any privileged operations, all forms use the Moodle form API (which handles sesskey validation automatically), database queries use parameterized placeholders via the $DB API, and output escaping uses format_string(), s(), and format_text() consistently.
Two medium-severity findings drive the grade:
-
Privacy API compliance gap — the plugin implements both
null_provider(declaring it stores no personal data) andmetadata\provider(declaring metadata for two tables that store user IDs). This contradiction means Moodle's privacy subsystem will skip this plugin during GDPR export/deletion requests, leaving user completion and credit cache records unprocessed. While the data is derivative (cached from course/program completions), the privacy implementation should handle cleanup rather than relying solely on cron. -
Missing space in SQL concatenation in
frameworks_user.phpthat would produce a SQL syntax error when multi-tenancy is active, breaking the user credits report for tenancy users.
Additional low-severity findings include a capability check that uses system context rather than the framework's actual context (UI bug only, not a security issue), and direct SQL value interpolation where the report builder API supports parameterized queries.
No security vulnerabilities were identified. The code is well-structured, uses transactions where appropriate, has comprehensive test coverage, and follows Moodle coding patterns consistently.
Plugin Overview
tool_mutrain is a training credits management plugin for Moodle, part of the MuTMS plugin suite. It provides:
- Credit frameworks — configurable containers that aggregate training credits from custom fields on courses and programs
- Completion tracking — event-driven and cron-based synchronization of course/program completions into a cached credit system
- Management UI — admin pages for creating, updating, archiving, moving, and deleting frameworks; adding/removing custom fields
- User reports — user-facing pages showing earned credits across frameworks, with drill-down to individual completions
- Report builder integration — system reports using Moodle's report builder API with proper entity definitions
Security Assessment
The plugin follows Moodle security best practices:
- All pages call
require_login()andrequire_capability()with appropriate capability checks - Forms extend
tool_mulib\local\ajax_form(Moodle form base class), providing automatic sesskey/CSRF protection - Database operations use the
$DBAPI with parameterized queries throughout - Output is properly escaped with
format_string(),s(), andformat_text() - No raw HTTP requests, no filesystem bypasses, no shell execution, no eval usage
- No hardcoded credentials or sensitive data in source
Key Findings
The review identified:
- 2 medium findings — Privacy API compliance gap and a SQL concatenation bug
- 2 low findings — incorrect capability context check and SQL interpolation patterns
- No critical or high findings
- No security vulnerabilities
Findings
The plugin implements both core_privacy\local\metadata\null_provider and core_privacy\local\metadata\provider. These interfaces are contradictory:
null_providerdeclares that the plugin stores no personal data and requires no export/deletion handlingproviderdeclares metadata for two tables (tool_mutrain_completionandtool_mutrain_credit) that both containuseridfields
Because null_provider is implemented, Moodle's privacy subsystem will skip this plugin entirely during GDPR data subject access requests and deletion requests. User completion and credit records will not be exported or deleted.
The plugin should:
- Remove the
null_providerinterface - Implement
core_privacy\local\request\plugin\providerto handle data export and deletion - Or, if the data is truly considered transient cache, implement
core_privacy\local\request\userlist_providerwith appropriate cleanup
Medium risk. This is a GDPR/privacy compliance gap. When a data subject requests data export or deletion, this plugin's records will be skipped. The data is derivative (cached from course/program completions), and the cron task will eventually clean up orphaned records if the source data is deleted. However, relying on eventual cron cleanup is not compliant with GDPR requirements for timely data deletion, and the data would not appear in privacy exports at all.
The plugin stores personal data in two tables:
tool_mutrain_completion— records which user completed which course/program, with timestampstool_mutrain_credit— records aggregated credit totals per user per framework, including when required credits were reached
Both tables have userid foreign keys to the user table. The plugin's own get_metadata() method correctly declares these tables and their fields, but the null_provider interface overrides this by telling the privacy subsystem to ignore the plugin.
class provider implements
\core_privacy\local\metadata\null_provider,
\core_privacy\local\metadata\provider {
Remove null_provider and implement the request handler interface:
class provider implements
\core_privacy\local\metadata\provider,
\core_privacy\local\request\plugin\provider {
Then implement the required methods: get_contexts_for_userid(), export_user_data(), delete_data_for_all_users_in_context(), and delete_data_for_user().
In the frameworks_user system report, when multi-tenancy is active, a SQL condition is appended without a leading space. This produces invalid SQL like ...archived = 0AND (...), causing a database error that prevents the user credits report from loading for tenancy users.
Medium risk. This is a functional bug, not a security vulnerability. It only manifests when multi-tenancy (tool_mutenancy) is active and the user belongs to a tenant. The user credits report page will throw a database exception, preventing users from viewing their training credits. Sites not using multi-tenancy are unaffected.
The $basewhere string is built in two steps: line 62 sets the base condition ending with archived = 0, and line 67 conditionally appends a tenancy filter. The concatenation operator .= joins the strings directly without any whitespace separator.
if (\tool_mulib\local\mulib::is_mutenancy_active()) {
if ($usercontext->tenantid) {
$basewhere .= "AND ({$contextalias}.tenantid IS NULL OR {$contextalias}.tenantid = $usercontext->tenantid)";
}
}
Add a leading space before AND:
$basewhere .= " AND ({$contextalias}.tenantid IS NULL OR {$contextalias}.tenantid = $usercontext->tenantid)";
In the fields table, the "remove field" action button checks tool/mutrain:manageframeworks capability against context_system rather than the framework's actual context. This means:
- Users with
manageframeworksin a category context (but not system) will not see the remove button, even though they can access the removal page directly - The actual
field_remove.phppage correctly checks the capability against the framework's context
This is a UI inconsistency, not a security issue, since the page-level check is correct.
Low risk. This is a UI/UX bug only. Users with category-level management capability will not see the remove button in the table, but they can still navigate to the removal page directly (which performs the correct authorization check). No security bypass is possible.
The fields table class is used on the management/framework.php page, which already requires tool/mutrain:viewframeworks in the framework's context. The table displays custom fields associated with a framework and optionally shows a remove button. The framework record is stored as $this->framework in the constructor.
if (!$this->framework->archived && has_capability('tool/mutrain:manageframeworks', \context_system::instance())) {
Check capability against the framework's context instead of system context:
$context = \context::instance_by_id($this->framework->contextid);
if (!$this->framework->archived && has_capability('tool/mutrain:manageframeworks', $context)) {
Several system reports interpolate PHP variables directly into SQL strings passed to add_base_condition_sql(), which accepts an optional $params array. While the interpolated values are all integers from trusted sources (context instance IDs, database record IDs, timestamps), using parameterized queries is the recommended Moodle practice for defense in depth.
Low risk. No actual SQL injection vulnerability exists because all interpolated values are integers from the Moodle database or context system. However, using parameterized queries is a best practice that protects against future code changes that might introduce non-integer values, and is the pattern recommended by the Moodle report builder API.
The add_base_condition_sql() method in Moodle's report builder API accepts a second $params array argument for parameterized queries. The interpolated values in this plugin all come from $usercontext->instanceid (integer user ID), $this->framework->id (integer DB primary key), and $this->framework->restrictafter (integer timestamp) — all from trusted database sources.
$basewhere = "{$this->creditalias}.userid = $userid AND {$this->frameworkalias}.publicaccess = 1 AND {$this->frameworkalias}.archived = 0";
Use parameterized queries with database::generate_param_name():
$useridparam = \core_reportbuilder\local\helpers\database::generate_param_name();
$basewhere = "{$this->creditalias}.userid = :{$useridparam} AND {$this->frameworkalias}.publicaccess = 1 AND {$this->frameworkalias}.archived = 0";
$params[$useridparam] = $userid;
Then pass $params to add_base_condition_sql().
$basewhere[] = "{$this->completionalias}.userid = {$usercontext->instanceid}";
$basewhere[] = "{$this->frameworkalias}.id = {$this->framework->id}";
Use parameterized queries similarly to the suggested fix above.
$basewhere[] = "{$this->completionalias}.timecompleted >= {$this->framework->restrictafter}";
Use a parameterized query for the restrictafter value.
The plugin has good test coverage with PHPUnit tests for core business logic (framework operations, area completions, utilities, events) and Behat generator support. This is a positive indicator of code quality and maintainability.
The database schema upgrade path in db/upgrade.php is well-structured, using proper XMLDB operations with $DB->get_manager() (the correct location for DDL in upgrade files), savepoints, and conditional field existence checks.
The plugin's SQL handling in framework::sync_credits() correctly provides separate MySQL and PostgreSQL UPDATE syntax for the credit aggregation query. This is a valid approach to handling cross-database UPDATE...FROM differences that Moodle's DB API does not abstract away.
All management pages use the tool_mulib\local\ajax_form base class (extending Moodle's moodleform), which provides automatic sesskey/CSRF validation. No manual sesskey handling is needed or missing.
The plugin depends on tool_mulib and customfield_mutrain as declared in version.php. Several classes extend base classes from tool_mulib (e.g., ajax_form, form_autocomplete\base, sql). The review evaluated the plugin's own code in isolation; the security of the base classes in tool_mulib is outside scope.