upload.module hard-codes 'upload files' permission check
| Project: | Drupal |
| Version: | 6.x-dev |
| Component: | upload.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
The short version: _upload_form() should be using a form #access property to set its requirement for upload files permission, and not hard-code it in the module logic. Because it does not do this, the only way to extend this form from a contributed module is to hack core.
The long version: I was asked by a client how to extend comment_upload.module so that it provided a permission "upload files to comments" that could be given out to certain low-level users, who don't have "upload files" permissions, since that's reserved for a more powerful role. Sure, you're thinking, throw in a hook_perm(), and modify the permission check in comment_upload.module. Bing, bang, done, right? WRONG. ;)
Why? Because comment_upload.module is calling _upload_form() to place the upload form in the comment. This is because it would be silly to duplicate all of that function's code in comment_upload.module, both because it's annoying, and also because if there were ever important security fixes to this part of code, comment_upload.module's version might fall out of date. Not good.
Unfortunately, _upload_form() has this dandy piece of code:
<?php
if (user_access('upload files')) {
// Make the form here.
}
?>which results in absolutely nada being returned back to comment_upload.module's form.
If, however, we did $form['#access'] = user_access('upload files'), /then/ comment_upload.module could use hook_form_alter() to have it check "upload files to comments" instead of the more generic "upload files."
Whew!
Anyway, patch forthcoming.
#1
Patch for 6.x and 7.x.
#2
And here's one for 5.x.
#3
Oopsie.
#4
I know your just moving this line around, but what's with:
($limits['resolution'] ? t('Images are larger than %resolution will be resized. ', array('%resolution' => $limits['resolution'])) : '') . t('The maximum upload size is %filesize. Only files with the following extensions may be uploaded: %extensions. ', array('%extensions' => $limits['extensions'], '%filesize' => format_size($limits['file_size']))),
Doesn't "Images are larger than %resolution will be resized." have an extra "are" in it or something?
#5
Here is the test for Simpletest module for the issue.
My first attempt was create a test module for this which called _upload_form() and put its own perm for uploads.
But it's better to isolate a test as far as possible.
There are 2 checks: for 'upload files' permission and for 'upload files to comments' (as an example).
#6
@webchick: patch looks good on short review.
@keith.smith: yeah, you are right
@vladimir: you'd sill need a custom form alter hook to alter the form and its #access check to the permission you'd like to check
#7
@Gábor Hojtsy: It seems I have to generate a module in the test. Simpletest module can't do that itself, but I can.
#8
Here's a patch for 6.x/7.x that fixes the string bug found by Keith Smith. Thanks, and thanks a LOT for the test-writing, Vladimir!! :) You rock!
5.x doesn't have the sloppy string so old patch still applies.
#9
Here is improved test for the issue.
The main idea: create simple module with hook_perm() and hook_form_alter() and alter a form according permissions.
After drupal_prepare_form() check for permissions the form has.
Test is too big because of a module creation stuff here.
<?php
class TestCase226853 extends DrupalTestCase {
function get_info() {
return array(
'name' => t('[226853] upload.module hard-codes \'upload files\' permission check'),
'desc' => t('The short version: _upload_form() should be using a form #access property to set its requirement for upload files permission, and not hard-code it in the module logic. Because it does not do this, the only way to extend this form from a contributed module is to hack core.'),
'group' => t('Drupal 7 Tests'),
);
}
function testIssue() {
$this->drupalModuleEnable('upload');
// A module makes new own permission and implements hook_form_alter()
$module_name = $this->randomName();
$module_text = <<< EOF
<?php
function {$module_name}_perm() {
return array(
'upload files to {$module_name}' => t('Attach files to content to something {$module_name} related.'),
);
}
function {$module_name}_form_alter(&\$form, \$form_state, \$form_id) {
if (\$form_id == '{$module_name}_form') {
\$obj->files = array();
\$form['attachments'] = array(
'#type' => 'fieldset',
'#title' => t('File attachments'),
'#access' => user_access('upload files to {$module_name}') || user_access('upload files'),
'#collapsible' => TRUE,
'#collapsed' => empty(\$obj->files),
'#description' => t('Changes made to the attachments are not permanent until you save this post. The first "listed" file will be included in RSS feeds.'),
'#prefix' => '<div class="attachments">',
'#suffix' => '</div>',
'#weight' => 10,
);
// Wrapper for fieldset contents (used by ahah.js).
\$form['attachments']['wrapper'] = array(
'#prefix' => '<div id="attach-wrapper">',
'#suffix' => '</div>',
);
\$form['attachments']['wrapper'] += _upload_form(\$obj);
if (isset(\$form['attachments']['wrapper']['#access'])) {
\$form['attachments']['wrapper']['#access'] |= user_access('upload files to {$module_name}');
}
\$form['#attributes']['enctype'] = 'multipart/form-data';
}
}
EOF;
$this->createModule($module_name, $module_text);
// check for a user has only 'upload files' permission
$this->checkPowerfulRole($module_name);
// check for a user has only module's permission
$this->checkModuleRole($module_name);
// check for a user has other permission
$this->checkRandomRole($module_name);
$this->deleteModule($module_name);
}
function createModule($module_name, $module_text, $module_core = '7.x') {
$module_dir = "sites/all/modules/$module_name";
$module_file = "$module_dir/$module_name.module";
$module_file_info = "$module_dir/$module_name.info";
if (file_check_directory($module_dir, FILE_CREATE_DIRECTORY)) {
$info = <<< EOF
name = "$module_name module"
description = "$module_name module for test. Delete it when you see it."
package = Temporary
core = $module_core
EOF;
if ($fp = @fopen($module_file_info, 'w')) {
fwrite($fp, $info);
fclose($fp);
}
if ($fp = @fopen($module_file, 'w')) {
fwrite($fp, $module_text);
fclose($fp);
}
$this->drupalModuleEnable($module_name);
}
}
function deleteModule($module_name) {
$module_dir = "sites/all/modules/$module_name";
$module_file = "$module_dir/$module_name.module";
$module_file_info = "$module_dir/$module_name.info";
@unlink($module_file);
@unlink($module_file_info);
@rmdir($module_dir);
}
// Check 'upload files' permission.
// Should pass so 'upload files' is very general
function checkPowerfulRole($module_name) {
// function _upload_form() uses a global $user
global $user;
$orig_user = $user;
$form = array();
// create a user for form's preparing
$user = $this->drupalCreateUserRolePerm(array("upload files"));
$this->drupalLoginUser($user);
// prepare the form (executing any hook_form_alter)
drupal_prepare_form("{$module_name}_form", $form, $form_state);
$this->assertTrue($this->checkAccess($form), "check generic 'upload files'");
$this->drupalGet('logout');
$user = $orig_user; // restore the original user
}
// Check permission for upload by module's permission.
// Should pass so module's permission checked in hook_form_alter()
// It checks the issue: the user don't have 'upload files' permission,
// but he have module's permission though.
function checkModuleRole($module_name) {
global $user;
$orig_user = $user;
$form = array();
$user = $this->drupalCreateUserRolePerm(array("upload files to {$module_name}"));
$this->drupalLoginUser($user);
drupal_prepare_form("{$module_name}_form", $form, $form_state);
$this->assertTrue($this->checkAccess($form), "check module's 'upload files to {$module_name}'");
$this->drupalGet('logout');
$user = $orig_user;
}
// Check random permission
// Should pass (assertFalse) so random permission does not allow file upload
function checkRandomRole($module_name) {
global $user;
$orig_user = $user;
$form = array();
$user = $this->drupalCreateUserRolePerm(array($this->randomName()));
$this->drupalLoginUser($user);
drupal_prepare_form("{$module_name}_form", $form, $form_state);
$this->assertFalse($this->checkAccess($form), "Check other roles");
$this->drupalGet('logout');
$user = $orig_user;
}
function checkAccess($form) {
return !empty($form['attachments']['#access']) && // access to attachments
isset($form['attachments']['wrapper']['new']); // is 'new' form exists?
!empty($form['attachments']['wrapper']['#access']); // access to wrapper
}
}
?>
#10
wanted to point to a similar issue #247095: Upload.module hard-codes 'view uploaded files' permission check, i'll try to test this out this afternoon.
#11
here's a re-roll with vladimir.dolgopolov's test included. i will say that this test freaks me the flip out. we now got the ability to have hidden modules so we don't need to be creating them dynamically but i'm a bit unsure where the module should live.
#12
I think I would rather fix this in 7.x with #563000: Remove upload module and provide upgrade path to file. So, assuming that patch makes it in, this is a 6.x-only bug.