Openbook resource folder files block
block_openbook
A Moodle block that surfaces files from an Openbook resource folder activity (the third-party mod_openbook module) inside a quiz, giving students quick access to permitted resources during an open-book quiz. It lists three file groups — teacher files, shared student files, and the viewing user's own files — as pluginfile.php download links, optionally opening PDFs in mod_openbook's bundled PDF.js viewer. The block can only be placed on quiz pages and is configured per instance to point at one Openbook resource folder in the current course.
The plugin is generally well engineered. Every database query uses parameterised $DB methods, so there is no SQL injection despite several dynamically assembled WHERE clauses. All user-influenced output (file names, the activity name) passes through format_string() (which applies clean_text()) and html_writer/Mustache escaping, so there is no XSS. There is no direct filesystem access, no raw HTTP, no shell execution, and the configuration form correctly scopes selectable activities to the current course.
The issues that remain are about how widely the file listings are scoped. The most significant is a group-separation fail-open: the block only applies its same-group filter when groups_get_activity_group() returns a non-empty value, but core returns 0 — and explicitly warns authors to add extra checks — for a user who has no accessible group in a separate groups activity. Such a user is shown every group's file listing rather than none. This is a genuine access-control gap, but it is well mitigated: it requires the narrow combination of a separate-groups activity plus an unassigned (or wrong-grouping) participant, and actual file downloads are served by mod_openbook's own pluginfile callback, so the block-level exposure is limited to file metadata.
The remaining findings are low severity: the block lists a folder's files without confirming the activity is visible/available to the viewer, there is no backup/restore support to remap the stored activity reference on course restore, and the renderable contains substantial duplicated code. No high or critical vulnerabilities were found — no unauthenticated exposure, no cross-user data modification, and no privilege escalation.
The block_openbook plugin is a Moodle 5.x block that surfaces files from an Openbook resource folder activity (the third-party mod_openbook module) inside a quiz, so students can reach permitted resources during an open-book quiz. It renders three groups of files — teacher files, shared student files, and the viewing user's own files — as pluginfile.php download links, optionally opening PDFs in mod_openbook's bundled PDF.js viewer.
Overall the plugin follows Moodle standards well:
- No SQL injection — all queries use parameterised
$DBmethods; dynamicWHEREfragments are either hardcoded literals or built with$DB->get_in_or_equal(). - No XSS — file names and the activity name go through
format_string()(verified in core to applyclean_text()/escaping), and links are built withhtml_writerand emitted through Mustache. - No dangerous primitives — no raw filesystem, database, or HTTP access, no
eval/exec, no hardcoded secrets, no schema changes outsidedb/. - The configuration form restricts selectable activities to the current course and is a standard
moodleform(sesskey handled by core).
The findings concern access-control scoping of the file listings:
- Group separation fails open (medium, mitigated). When
groups_get_activity_group()returns0— which core returns for a user with no accessible group in a separate groups activity — the block applies no group filter and lists every group's files. - Activity visibility not enforced (low). The block lists a folder's files even when the
mod_openbookactivity is hidden or unavailable to the viewer. - No backup/restore support (low). The stored activity id is not remapped on course restore, leaving broken or cross-course references.
- Code duplication (low). The file-link building logic is repeated three times in
export_for_template().
Actual file downloads are served by mod_openbook's own pluginfile callback (outside this review), so the confirmed impact of findings 1 and 2 is limited to file metadata (names and paths). Grade: B+.
Findings
The block is meant to restrict teacher and shared-student file listings to members of the current user's group when the Openbook activity uses separate groups. It decides whether to apply that restriction with:
$grouprestricted = !empty(groups_get_activity_group($openbookcm));
The same-group f.userid IN (...) filter is then only added when $grouprestricted is true.
The problem is the value returned by groups_get_activity_group(). Verified in core (lib/grouplib.php), that function returns 0 for a user who is not assigned to any group in a separate-groups activity — the core code even carries the comment "this happen when user not assigned into group in SEPARATEGROUPS mode ... mod authors must add extra checks for this when SEPARATEGROUPS mode used". It also returns 0 when the user's groups fall outside the activity's configured grouping.
Because the plugin treats 0 as "no restriction needed", a user in exactly that situation is shown every group's teacher-file and shared-file listing, which is the opposite of the intended separate-groups behaviour (they should see nothing, or only their own material). The plugin does not add the extra check that core explicitly tells module authors to add.
Medium risk. This is a real fail-open in the plugin's own access-control logic, in precisely the scenario core documents as requiring extra handling. Blast radius is constrained: it only triggers for a course configured with a separate groups Openbook folder and a participant who has no accessible group in that activity; participants who are correctly assigned to a group are filtered correctly. The confirmed exposure is file metadata (names and paths) of other groups' material, because the actual downloads are served by mod_openbook's pluginfile callback, which is outside this plugin and this review — if that callback does not itself enforce separate groups (many module file callbacks check context/capability but not group membership), the links could also yield file content, which would raise the practical impact. Because of the narrow precondition and the metadata-level confirmed impact, this is medium rather than high.
export_for_template() builds three file listings (teacher, shared, own) by querying the {files} table for the mod_openbook activity's contexts. The group logic at lines 86-127 is intended to append AND f.userid IN (<members of the current user's groups>) to those queries when the activity is groupwise. $andgroupmembers is only populated inside the if ($grouprestricted && !$canviewallgroups) block, so when $grouprestricted is false the queries run unfiltered across all uploaders.
- Create an Openbook resource folder with Group mode = Separate groups and upload teacher/shared files under two groups (A and B).
- Enrol a student in the course but do not add them to any group (a common transient state, or a student outside the activity's grouping).
- Place the
block_openbookblock on a quiz and point it at that folder. - As the unassigned student, attempt the quiz.
Expected (separate groups): the student sees no other users' files. Actual: groups_get_activity_group() returns 0, $grouprestricted is false, no f.userid IN (...) filter is applied, and the block lists the teacher and shared-student files of all groups.
$groupwiseactivity = groups_get_activity_group($openbookcm);
$grouprestricted = !empty($groupwiseactivity);
Do not use the truthiness of groups_get_activity_group() as the sole gate. Decide on the group mode explicitly and, in separate-groups mode without accessallgroups, always restrict — and fail closed when the user has no accessible group:
$groupmode = groups_get_activity_groupmode($openbookcm);
$cmcontext = \context_module::instance($openbookcm->id);
$canviewallgroups = has_capability('moodle/site:accessallgroups', $cmcontext);
if ($groupmode == SEPARATEGROUPS && !$canviewallgroups) {
$allmembers = [];
$currentusergroups = groups_get_user_groups($openbookcm->course, $USER->id);
foreach ($currentusergroups as $coursegroup) {
foreach ($coursegroup as $groupid) {
$allmembers = array_merge($allmembers, groups_get_members($groupid, 'u.id'));
}
}
// Fail closed: with no accessible group, only expose the user's own id.
$memberids = $allmembers ? array_column($allmembers, 'id') : [$USER->id];
[$insql, $insqlparams] = $DB->get_in_or_equal($memberids, SQL_PARAMS_NAMED, 'user');
$andgroupmembers = ' AND f.userid ' . $insql;
$groupparams = $insqlparams;
}
if ($grouprestricted && !$canviewallgroups) {
This guard should be driven by the activity's group mode (as above), not by whether an active group happens to be selected, so that the member filter is applied whenever separate groups are in effect.
get_content() renders the file listings whenever get_openbook_cm() returns a non-null course module. get_openbook_cm(), however, returns a course module even when the activity is not visible/available to the current user.
When the activity is present in the current course's modinfo but not uservisible (hidden, or blocked by an availability restriction such as a date or grouping condition), the first branch does not return, and execution falls through to a raw SQL fallback that aliases the raw cm.visible column as uservisible and returns that record regardless of the real per-user accessibility:
- The fallback query selects
cm.visible AS uservisible, ignoring\core_availabilityrestrictions and the user's actual access. get_content()then only checks$openbookcm !== null— never$openbookcm->uservisible— before instantiating the renderable.
As a result, the file names and paths of an activity that has been hidden from, or made unavailable to, the student are still listed in the block.
Low risk. The disclosed data is file metadata for an activity in the same course as the quiz the user can already access; the underlying downloads are gated by mod_openbook's pluginfile callback (a well-behaved module callback calls require_login($course, true, $cm), which would deny a hidden/unavailable module). The scenario also depends on a teacher hiding or restricting the folder while leaving the block active, which is close to a misconfiguration. It is worth fixing because the block explicitly retrieves a uservisible-shaped value and then ignores it, but the practical impact is minor.
The block is only placed on quiz pages, so viewers are course participants who have passed the quiz's own require_login. The exposure is therefore not a course-access bypass; it is that the block does not honour the activity-level visibility/availability of the referenced Openbook folder before listing its contents. Teacher files and approved shared files are intended to be visible to quiz-takers anyway, so the extra disclosure only arises where the folder has been deliberately hidden or restricted.
As a teacher, configure the block to point at an Openbook folder, then hide that folder (or add an availability restriction that excludes some students). A restricted student attempting the quiz still sees the block list the folder's file names and download links, even though the activity itself is not accessible to them.
$cm = $DB->get_record_sql(
"SELECT cm.id, cm.visible AS uservisible
FROM {course_modules} cm
JOIN {modules} md ON md.id = cm.module
JOIN {openbook} o ON o.id = cm.instance
WHERE o.id = :instance AND md.name = :modulename",
['instance' => $this->config->openbook, 'modulename' => 'openbook']
);
Do not treat the raw cm.visible flag as uservisible. Prefer resolving the module through get_fast_modinfo() and honouring the real uservisible property, and only proceed when the activity is genuinely accessible to the user.
$openbookcm = $this->get_openbook_cm();
if (isset($this->config->openbook) && $openbookcm !== null) {
Gate rendering on the activity actually being visible/available to the current user, e.g.:
$openbookcm = $this->get_openbook_cm();
if (isset($this->config->openbook) && $openbookcm !== null
&& !empty($openbookcm->uservisible)) {
// ... render ...
}
The block stores a foreign reference — the selected Openbook activity instance id — in its instance configuration ($this->config->openbook), but the plugin ships no backup/moodle2/ classes to remap that id during course backup and restore.
Moodle's generic block backup preserves the serialised configdata verbatim, so after a course is restored (or imported) the stored openbook value still refers to the id from the source system:
- If no activity with that id exists in the target, the reference is dangling and the block silently clears itself.
- If an activity with that id happens to exist elsewhere (for example the original course on the same site),
get_openbook_cm()'s fallback query looks up the activity by global id with no course constraint and will resolve to that foreign activity, causing the restored block to list another course's file names.
Low risk. This is primarily a correctness/robustness gap: after restore the block either self-clears or points at the wrong folder. The cross-course angle is a metadata disclosure only (file names; downloads remain gated by mod_openbook, and target-course users are typically not enrolled in the source course), and it requires a privileged backup/restore or import operation to arise. Implementing backup_openbook_block_task/restore_openbook_block_task with restore_dbops::get_backup_ids_record() / get_mappingid('openbook', ...) to remap the stored id would resolve it.
get_openbook_cm() (in block_openbook.php) resolves the configured id with SELECT ... FROM {course_modules} cm JOIN {openbook} o ON o.id = cm.instance WHERE o.id = :instance, which is not restricted to the current course. Combined with the absence of restore remapping, a stale id can therefore resolve to an activity in a different course.
export_for_template() builds the download link for each file with the same ~25-line block of code three times — once for teacher files, once for shared files, and once for own files. Each copy repeats the same PDF-viewer-URL-versus-plain-pluginfile-URL branch and the same make_pluginfile_url() argument lists.
This duplication makes the method long and error-prone: a change to URL construction (as the changelog shows already happened for the PDF.js path) must be made in three places, and the copies have already drifted slightly (the plain-URL branch passes $f->component/$f->filearea while the PDF branch hardcodes 'mod_openbook' and the file area).
Additionally, at the top of the method the current course module is re-fetched from the database with get_coursemodule_from_instance(... MUST_EXIST) even though the resolved course module was already passed into the renderable as $this->openbookcm, and get_content() calls get_openbook_cm() twice for the same instance.
Low risk. No security impact. The value is maintainability: consolidating the three copies into one helper reduces the chance of the copies drifting further and removes a redundant per-render database query.
This is internal rendering code executed each time the block is displayed on a quiz page. The concern is maintainability and a small amount of redundant database work per render, not security.
foreach ($records as $f) {
if ($openpdffilesinpdfjs && $f->mimetype === 'application/pdf') {
if ($uselegacyviewer) {
$pdfviewer = 'pdfjs-legacy-dist';
} else {
$pdfviewer = 'pdfjs-dist';
}
Extract a single private helper that maps a set of file records to link objects, e.g. private function build_file_links(array $records, string $filearea, bool $openpdffilesinpdfjs, bool $uselegacyviewer): array, and call it for the teacher, shared, and own file sets. This removes the three near-identical blocks and prevents drift.
foreach ($records as $f) {
if ($openpdffilesinpdfjs && $f->mimetype === 'application/pdf') {
Replace with a call to the shared helper described above.
foreach ($records as $f) {
if ($openpdffilesinpdfjs && $f->mimetype === 'application/pdf') {
Replace with a call to the shared helper described above.
$openbookcm = get_coursemodule_from_instance(
'openbook',
$this->openbookid,
0,
false,
MUST_EXIST
);
Reuse the course module already provided as $this->openbookcm (a cm_info/stdClass with id and course) instead of issuing another get_coursemodule_from_instance() query per render.
Tight coupling to mod_openbook internals. export_for_template() reads the {openbook} and {openbook_file} tables and the commonteacherfiles/attachment file areas of the mod_openbook module directly, and constructs mod_openbook PDF.js viewer URLs by hand. This is largely unavoidable given the hard mod_openbook dependency and the absence of a public API, but it is brittle: a schema or file-area change in mod_openbook (columns such as openpdffilesinpdfjs, uselegacyviewer, filesarepersonal, obtainteacherapproval, obtainstudentapproval, or the PDF.js path layout) will silently break this block. Consider requesting a small read API from mod_openbook to depend on.
Teacher files can disappear under separate groups. Because the same-group filter matches on f.userid (the uploader), a teacher who is not a member of a student's group will have their commonteacherfiles filtered out for that student in separate-groups mode. The behat coverage assumes teachers are group members (teacher1 in Group A, teacher2 in Group B); in courses where teachers are not added to student groups, students may see no teacher files. This is a functional edge case rather than a security issue, but worth documenting or handling explicitly.
Privacy provider is appropriate. The block implements null_provider and stores only its own configuration (title and the selected activity id) in block_instances, not personal data — the file data it displays is owned and stored by mod_openbook. The null provider is the correct choice here.
Minor documentation/robustness nits. get_owning_activity() is documented as returning "the activity record" (an stdClass) but actually returns a course module from get_coursemodule_from_id(), and declares an unused global $DB. In export_for_template(), $DB->get_record('openbook', ..., IGNORE_MISSING) is dereferenced immediately afterward; although get_openbook_cm() should guarantee the row exists, using MUST_EXIST (or a null check) would be more defensive than IGNORE_MISSING followed by an unchecked property read.