Skip to content

Commit

Permalink
MDL-69050 lang: Rename and deprecate filetypes_util methods
Browse files Browse the repository at this point in the history
  • Loading branch information
@mudrd8mz
mudrd8mz committed Sep 24, 2020
1 parent 801ee23 commit 00b6772
Show file tree
Hide file tree
Showing 5 changed files with 139 additions and 79 deletions.
6 changes: 3 additions & 3 deletions lib/adminlib.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9071,7 +9071,7 @@ function any_new_admin_settings($node) {
*/
function db_should_replace($table, $column = ''): bool {

// TODO: this is horrible hack, we should do whitelisting and each plugin should be responsible for proper replacing...
// TODO: this is horrible hack, we should have a hook and each plugin should be responsible for proper replacing...
$skiptables = ['config', 'config_plugins', 'filter_config', 'sessions',
'events_queue', 'repository_instance_config', 'block_instances', 'files'];

Expand Down Expand Up @@ -11271,11 +11271,11 @@ public function validate($data) {

// No need to call parent's validation here as we are PARAM_RAW.

if ($this->util->is_whitelisted($data, $this->onlytypes)) {
if ($this->util->is_listed($data, $this->onlytypes)) {
return true;

} else {
$troublemakers = $this->util->get_not_whitelisted($data, $this->onlytypes);
$troublemakers = $this->util->get_not_listed($data, $this->onlytypes);
return get_string('filetypesnotallowed', 'core_form', implode(' ', $troublemakers));
}
}
Expand Down
82 changes: 59 additions & 23 deletions lib/form/classes/filetypes_util.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ public function data_for_browser($onlytypes=null, $allowall=true, $current=null)
$types = [];

foreach ($groupinfo->extensions as $extension) {
if ($onlytypes && !$this->is_whitelisted($extension, $onlytypes)) {
if ($onlytypes && !$this->is_listed($extension, $onlytypes)) {
$group->selectable = false;
$group->expanded = true;
$group->ext = '';
Expand Down Expand Up @@ -328,7 +328,7 @@ public function data_for_browser($onlytypes=null, $allowall=true, $current=null)
continue;
}
$extension = '.'.$extension;
if ($onlytypes && !$this->is_whitelisted($extension, $onlytypes)) {
if ($onlytypes && !$this->is_listed($extension, $onlytypes)) {
continue;
}
if (!isset($info['groups']) || empty($info['groups'])) {
Expand Down Expand Up @@ -416,35 +416,53 @@ public function expand($types, $keepgroups=false, $keepmimetypes=false) {
}

/**
* Should the given file type be considered as a part of the given whitelist.
* Should the file type be considered as a part of the given list.
*
* If multiple types are provided, all of them must be part of the list. Empty type is part of any list.
* Any type is part of an empty list.
*
* @param string|array $types File type or list of types to be checked.
* @param string|array $list An array or string listing the types to check against.
* @return boolean
*/
public function is_listed($types, $list) {
return empty($this->get_not_listed($types, $list));
}

/**
* Should the given file type be considered as a part of the given list.
*
* If multiple types are provided, all of them must be part of the
* whitelist. Empty type is part of any whitelist. Any type is part of an
* empty whitelist.
* list. Empty type is part of any list. Any type is part of an
* empty list.
*
* @param string|array $types File types to be checked
* @param string|array $whitelist An array or string of whitelisted types
* @deprecated since Moodle 3.10 MDL-69050 - please use {@see self::is_listed()} instead.
* @param string|array $types File type or list of types to be checked.
* @param string|array $list An array or string listing the types to check against.
* @return boolean
*/
public function is_whitelisted($types, $whitelist) {
return empty($this->get_not_whitelisted($types, $whitelist));
public function is_whitelisted($types, $list) {

debugging('filetypes_util::is_whitelisted() is deprecated. Please use filetypes_util::is_listed() instead.',
DEBUG_DEVELOPER);

return $this->is_listed($types, $list);
}

/**
* Returns all types that are not part of the give whitelist.
* Returns all types that are not part of the given list.
*
* This is similar check to the {@link self::is_whitelisted()} but this one
* actually returns the extra types.
* This is similar check to the {@see self::is_listed()} but this one actually returns the extra types.
*
* @param string|array $types File types to be checked
* @param string|array $whitelist An array or string of whitelisted types
* @return array Types not present in the whitelist
* @param string|array $types File type or list of types to be checked.
* @param string|array $list An array or string listing the types to check against.
* @return array Types not present in the list.
*/
public function get_not_whitelisted($types, $whitelist) {
public function get_not_listed($types, $list) {

$whitelistedtypes = $this->expand($whitelist, true, true);
$listedtypes = $this->expand($list, true, true);

if (empty($whitelistedtypes) || $whitelistedtypes == ['*']) {
if (empty($listedtypes) || $listedtypes == ['*']) {
return [];
}

Expand All @@ -454,22 +472,40 @@ public function get_not_whitelisted($types, $whitelist) {
return [];
}

return array_diff($giventypes, $whitelistedtypes);
return array_diff($giventypes, $listedtypes);
}

/**
* Returns all types that are not part of the given list.
*
* This is similar check to the {@see self::is_listed()} but this one actually returns the extra types.
*
* @deprecated since Moodle 3.10 MDL-69050 - please use {@see self::get_not_whitelisted()} instead.
* @param string|array $types File type or list of types to be checked.
* @param string|array $list An array or string listing the types to check against.
* @return array Types not present in the list.
*/
public function get_not_whitelisted($types, $list) {

debugging('filetypes_util::get_not_whitelisted() is deprecated. Please use filetypes_util::get_not_listed() instead.',
DEBUG_DEVELOPER);

return $this->get_not_listed($types, $list);
}

/**
* Is the given filename of an allowed file type?
*
* Empty whitelist is interpretted as "any file type is allowed" rather
* Empty allowlist is interpreted as "any file type is allowed" rather
* than "no file can be uploaded".
*
* @param string $filename the file name
* @param string|array $whitelist list of allowed file extensions
* @param string|array $allowlist list of allowed file extensions
* @return boolean True if the file type is allowed, false if not
*/
public function is_allowed_file_type($filename, $whitelist) {
public function is_allowed_file_type($filename, $allowlist) {

$allowedextensions = $this->expand($whitelist);
$allowedextensions = $this->expand($allowlist);

if (empty($allowedextensions) || $allowedextensions == ['*']) {
return true;
Expand Down
6 changes: 3 additions & 3 deletions lib/form/filetypes.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -242,10 +242,10 @@ public function validateSubmitValue($value) {

if ($this->onlytypes) {
// Assert that all file types are allowed here.
$notwhitelisted = $this->util->get_not_whitelisted($value['filetypes'], $this->onlytypes);
$notlisted = $this->util->get_not_listed($value['filetypes'], $this->onlytypes);

if ($notwhitelisted) {
return get_string('filetypesnotallowed', 'core_form', implode(', ', $notwhitelisted));
if ($notlisted) {
return get_string('filetypesnotallowed', 'core_form', implode(', ', $notlisted));
}
}

Expand Down
122 changes: 72 additions & 50 deletions lib/form/tests/filetypes_util_test.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -207,67 +207,67 @@ public function test_expand() {
/**
* Test checking that a type is among others.
*/
public function test_is_whitelisted() {
public function test_is_listed() {

$this->resetAfterTest(true);
$util = new filetypes_util();

// These should be intuitively true.
$this->assertTrue($util->is_whitelisted('txt', 'text/plain'));
$this->assertTrue($util->is_whitelisted('txt', 'doc txt rtf'));
$this->assertTrue($util->is_whitelisted('.txt', '.doc;.txt;.rtf'));
$this->assertTrue($util->is_whitelisted('audio', 'text/plain audio video'));
$this->assertTrue($util->is_whitelisted('text/plain', 'text/plain audio video'));
$this->assertTrue($util->is_whitelisted('jpg jpe jpeg', 'image/jpeg'));
$this->assertTrue($util->is_whitelisted(['jpg', 'jpe', '.png'], 'image'));
$this->assertTrue($util->is_listed('txt', 'text/plain'));
$this->assertTrue($util->is_listed('txt', 'doc txt rtf'));
$this->assertTrue($util->is_listed('.txt', '.doc;.txt;.rtf'));
$this->assertTrue($util->is_listed('audio', 'text/plain audio video'));
$this->assertTrue($util->is_listed('text/plain', 'text/plain audio video'));
$this->assertTrue($util->is_listed('jpg jpe jpeg', 'image/jpeg'));
$this->assertTrue($util->is_listed(['jpg', 'jpe', '.png'], 'image'));

// These should be intuitively false.
$this->assertFalse($util->is_whitelisted('.gif', 'text/plain'));
$this->assertFalse($util->is_listed('.gif', 'text/plain'));

// Not all text/plain formats are in the document group.
$this->assertFalse($util->is_whitelisted('text/plain', 'document'));
$this->assertFalse($util->is_listed('text/plain', 'document'));

// Not all documents (and also the group itself) is not a plain text.
$this->assertFalse($util->is_whitelisted('document', 'text/plain'));
$this->assertFalse($util->is_listed('document', 'text/plain'));

// This may look wrong at the first sight as you might expect that the
// mimetype should simply map to an extension ...
$this->assertFalse($util->is_whitelisted('image/jpeg', '.jpg'));
$this->assertFalse($util->is_listed('image/jpeg', '.jpg'));

// But it is principally same situation as this (there is no 1:1 mapping).
$this->assertFalse($util->is_whitelisted('.c', '.txt'));
$this->assertTrue($util->is_whitelisted('.txt .c', 'text/plain'));
$this->assertFalse($util->is_whitelisted('text/plain', '.c'));
$this->assertFalse($util->is_listed('.c', '.txt'));
$this->assertTrue($util->is_listed('.txt .c', 'text/plain'));
$this->assertFalse($util->is_listed('text/plain', '.c'));

// Any type is included if the filter is empty.
$this->assertTrue($util->is_whitelisted('txt', ''));
$this->assertTrue($util->is_whitelisted('txt', '*'));
$this->assertTrue($util->is_listed('txt', ''));
$this->assertTrue($util->is_listed('txt', '*'));

// Empty value is part of any whitelist.
$this->assertTrue($util->is_whitelisted('', '.txt'));
// Empty value is part of any list.
$this->assertTrue($util->is_listed('', '.txt'));
}

/**
* Test getting types not present in a whitelist.
* Test getting types not present in a list.
*/
public function test_get_not_whitelisted() {
public function test_get_not_listed() {

$this->resetAfterTest(true);
$util = new filetypes_util();

$this->assertEmpty($util->get_not_whitelisted('txt', 'text/plain'));
$this->assertEmpty($util->get_not_whitelisted('txt', '.doc .txt .rtf'));
$this->assertEmpty($util->get_not_whitelisted('txt', 'text/plain'));
$this->assertEmpty($util->get_not_whitelisted(['jpg', 'jpe', 'jpeg'], 'image/jpeg'));
$this->assertEmpty($util->get_not_whitelisted('', 'foo/bar'));
$this->assertEmpty($util->get_not_whitelisted('.foobar', ''));
$this->assertEmpty($util->get_not_whitelisted('.foobar', '*'));
$this->assertEmpty($util->get_not_listed('txt', 'text/plain'));
$this->assertEmpty($util->get_not_listed('txt', '.doc .txt .rtf'));
$this->assertEmpty($util->get_not_listed('txt', 'text/plain'));
$this->assertEmpty($util->get_not_listed(['jpg', 'jpe', 'jpeg'], 'image/jpeg'));
$this->assertEmpty($util->get_not_listed('', 'foo/bar'));
$this->assertEmpty($util->get_not_listed('.foobar', ''));
$this->assertEmpty($util->get_not_listed('.foobar', '*'));

// Returned list is normalized so extensions have the dot added.
$this->assertContains('.exe', $util->get_not_whitelisted('exe', '.c .h'));
$this->assertContains('.exe', $util->get_not_listed('exe', '.c .h'));

// If this looks wrong to you, see {@link test_is_whitelisted()} for more details on this behaviour.
$this->assertContains('image/jpeg', $util->get_not_whitelisted('image/jpeg', '.jpg .jpeg'));
// If this looks wrong to you, see {@see self::test_is_listed()} for more details on this behaviour.
$this->assertContains('image/jpeg', $util->get_not_listed('image/jpeg', '.jpg .jpeg'));
}

/**
Expand Down Expand Up @@ -361,44 +361,44 @@ public function test_data_for_browser() {
*/
public function is_allowed_file_type_provider() {
return [
'Filetype not in extension whitelist' => [
'Filetype not in extension list' => [
'filename' => 'test.xml',
'whitelist' => '.png .jpg',
'list' => '.png .jpg',
'expected' => false
],
'Filetype not in mimetype whitelist' => [
'Filetype not in mimetype list' => [
'filename' => 'test.xml',
'whitelist' => 'image/png',
'list' => 'image/png',
'expected' => false
],
'Filetype not in group whitelist' => [
'Filetype not in group list' => [
'filename' => 'test.xml',
'whitelist' => 'web_file',
'list' => 'web_file',
'expected' => false
],
'Filetype in whitelist as extension' => [
'Filetype in list as extension' => [
'filename' => 'test.xml',
'whitelist' => 'xml',
'list' => 'xml',
'expected' => true
],
'Empty whitelist should allow all' => [
'Empty list should allow all' => [
'filename' => 'test.xml',
'whitelist' => '',
'list' => '',
'expected' => true
],
'Filetype in whitelist but later on' => [
'Filetype in list but later on' => [
'filename' => 'test.xml',
'whitelist' => 'gif;jpeg,image/png xml xlsx',
'list' => 'gif;jpeg,image/png xml xlsx',
'expected' => true
],
'Filetype in whitelist as mimetype' => [
'Filetype in list as mimetype' => [
'filename' => 'test.xml',
'whitelist' => 'image/png application/xml',
'list' => 'image/png application/xml',
'expected' => true
],
'Filetype in whitelist as group' => [
'Filetype in list as group' => [
'filename' => 'test.html',
'whitelist' => 'video,web_file',
'list' => 'video,web_file',
'expected' => true
],
];
Expand All @@ -408,12 +408,12 @@ public function is_allowed_file_type_provider() {
* Test is_allowed_file_type().
* @dataProvider is_allowed_file_type_provider
* @param string $filename The filename to check
* @param string $whitelist The space , or ; separated list of types supported
* @param string $list The space , or ; separated list of types supported
* @param boolean $expected The expected result. True if the file is allowed, false if not.
*/
public function test_is_allowed_file_type($filename, $whitelist, $expected) {
public function test_is_allowed_file_type($filename, $list, $expected) {
$util = new filetypes_util();
$this->assertSame($expected, $util->is_allowed_file_type($filename, $whitelist));
$this->assertSame($expected, $util->is_allowed_file_type($filename, $list));
}

/**
Expand Down Expand Up @@ -484,4 +484,26 @@ public function test_get_unknown_file_types($filetypes, $expected) {
$util = new filetypes_util();
$this->assertSame($expected, $util->get_unknown_file_types($filetypes));
}

/**
* Test that a debugging noticed is displayed when calling is_whitelisted().
*/
public function test_deprecation_is_whitelisted() {

$util = new filetypes_util();
$this->assertTrue($util->is_whitelisted('txt', 'text/plain'));
$this->assertDebuggingCalled('filetypes_util::is_whitelisted() is deprecated. ' .
'Please use filetypes_util::is_listed() instead.', DEBUG_DEVELOPER);
}

/**
* Test that a debugging noticed is displayed when calling get_not_whitelisted().
*/
public function test_deprecation_get_not_whitelisted() {

$util = new filetypes_util();
$this->assertEmpty($util->get_not_whitelisted('txt', 'text/plain'));
$this->assertDebuggingCalled('filetypes_util::get_not_whitelisted() is deprecated. ' .
'Please use filetypes_util::get_not_listed() instead.', DEBUG_DEVELOPER);
}
}
2 changes: 2 additions & 0 deletions lib/upgrade.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ information provided here is intended especially for developers.
* The ZipStream-PHP library has been added to Moodle core in /lib/zipstream.
* The php-enum library has been added to Moodle core in /lib/php-enum.
* The http-message library has been added to Moodle core in /lib/http-message.
* Methods `filetypes_util::is_whitelisted()` and `filetypes_util::get_not_whitelisted()` have been deprecated and
renamed to `is_listed()` and `get_not_listed()` respectively.

=== 3.9 ===
* Following function has been deprecated, please use \core\task\manager::run_from_cli().
Expand Down

0 comments on commit 00b6772

Please sign in to comment.