upload.module hard-codes 'upload files' permission check

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?

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).

@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

@Gábor Hojtsy: It seems I have to generate a module in the test. Simpletest module can't do that itself, but I can.

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.

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
  }

}

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.

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.