Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issues 633 634-MOODLE_402_STABLE #638

Closed
wants to merge 2 commits into from
Closed

Issues 633 634-MOODLE_402_STABLE #638

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d190dd3
Issue #633: Move preferexternal setting
petersistrom Sep 18, 2024
f988e00
Issue #634: Explicitly check if path is directory
petersistrom Sep 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions classes/local/store/object_file_system.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,14 +246,14 @@ public function is_file_readable_externally_by_storedfile(stored_file $file) {
* @return bool
*/
public function is_file_readable_externally_by_hash($contenthash) {
if ($contenthash === sha1('')) {
// Files with empty size are either directories or empty.
$path = $this->get_external_path_from_hash($contenthash, false);

if ($contenthash === sha1('') && is_dir($path)) {
// Explicitly check if it is a directory, handle empty files as normal.
// We handle these virtually.
return true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't it be better to simply say no this file isn't readable externally, so false here? get_external_path_from_hash is more expensive so I think this should come after the sha1 test like it was

}

$path = $this->get_external_path_from_hash($contenthash, false);

// Note - it is not possible to perform a content recovery safely from a hash alone.
return is_readable($path);
}
Expand Down
9 changes: 3 additions & 6 deletions settings.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,9 @@
new lang_string('settings:tokenexpirywarnperiod', 'tool_objectfs'),
'', 60 * DAYSECS, DAYSECS));

$settings->add(new admin_setting_configcheckbox('tool_objectfs/preferexternal',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why have the settings changed?

new lang_string('settings:preferexternal', 'tool_objectfs'), '', ''));

$settings->add(new admin_setting_heading('tool_objectfs/filetransfersettings',
new lang_string('settings:filetransferheader', 'tool_objectfs'), ''));

Expand Down Expand Up @@ -247,10 +250,4 @@
}
}
}

$settings->add(new admin_setting_heading('tool_objectfs/testsettings',
new lang_string('settings:testingheader', 'tool_objectfs'), ''));

$settings->add(new admin_setting_configcheckbox('tool_objectfs/preferexternal',
new lang_string('settings:preferexternal', 'tool_objectfs'), '', ''));
}
Loading