Centralize file permisisons changing into drupal_chmod() - http://drupal.org/node/203204 From: andrew morton --- includes/file.inc | 53 +++++++++++++++++++++------ modules/color/color.module | 4 +- modules/simpletest/drupal_web_test_case.php | 4 ++ modules/simpletest/tests/file.test | 40 ++++++++++++++++---- modules/upload/upload.test | 3 ++ 5 files changed, 81 insertions(+), 23 deletions(-) diff --git includes/file.inc includes/file.inc index 96f7f4e..b92c2a1 100644 --- includes/file.inc +++ includes/file.inc @@ -157,7 +157,7 @@ function file_check_directory(&$directory, $mode = 0, $form_item = NULL) { // Check if directory exists. if (!is_dir($directory)) { if (($mode & FILE_CREATE_DIRECTORY) && @mkdir($directory)) { - @chmod($directory, 0775); // Necessary for non-webserver users. + drupal_chmod($directory); } else { if ($form_item) { @@ -172,7 +172,7 @@ function file_check_directory(&$directory, $mode = 0, $form_item = NULL) { if (!is_writable($directory)) { // If not able to modify permissions, or if able to, but chmod // fails, return false. - if (!$mode || (($mode & FILE_MODIFY_PERMISSIONS) && !@chmod($directory, 0775))) { + if (!$mode || (($mode & FILE_MODIFY_PERMISSIONS) && !drupal_chmod($directory))) { if ($form_item) { form_set_error($form_item, t('The directory %directory is not writable', array('%directory' => $directory))); watchdog('file system', 'The directory %directory is not writable, because it does not have the correct permissions set.', array('%directory' => $directory), WATCHDOG_ERROR); @@ -183,9 +183,8 @@ function file_check_directory(&$directory, $mode = 0, $form_item = NULL) { if ((file_directory_path() == $directory || file_directory_temp() == $directory) && !is_file("$directory/.htaccess")) { $htaccess_lines = "SetHandler Drupal_Security_Do_Not_Remove_See_SA_2006_006\nOptions None\nOptions +FollowSymLinks"; - if (($fp = fopen("$directory/.htaccess", 'w')) && fputs($fp, $htaccess_lines)) { - fclose($fp); - chmod($directory . '/.htaccess', 0664); + if (file_put_contents("$directory/.htaccess", $htaccess_lines)) { + drupal_chmod("$directory/.htaccess"); } else { $variables = array('%directory' => $directory, '!htaccess' => '
' . nl2br(check_plain($htaccess_lines))); @@ -483,11 +482,8 @@ function file_unmanaged_copy($source, $destination = NULL, $replace = FILE_EXIST return FALSE; } - // Give everyone read access so that FTP'd users or - // non-webserver users can see/read these files, - // and give group write permissions so group members - // can alter files uploaded by the webserver. - @chmod($destination, 0664); + // Set the permissions on the new file. + drupal_chmod($destination); return $destination; } @@ -996,6 +992,9 @@ function file_save_upload($source, $validators = array(), $destination = FALSE, return FALSE; } + // Set the permissions on the new file. + drupal_chmod($file->filepath); + // If we are replacing an existing file re-use its database record. if ($replace == FILE_EXISTS_REPLACE) { $existing_files = file_load_multiple(array(), array('filepath' => $file->filepath)); @@ -1889,6 +1888,38 @@ function file_get_mimetype($filename, $mapping = NULL) { return 'application/octet-stream'; } + /** - * @} End of "defgroup file". + * Set the permissions on a file or directory. + * + * This function will use the 'file_chmod_directory' and 'file_chmod_file' + * variables for the default modes for directories and files. By default these + * will give everyone read access so that FTP'd users or non-webserver users + * can see/read these files, and give group write permissions so group members + * can alter files uploaded by the webserver. + * + * @param $path + * String containing the path to a file or directory. + * @param $mode + * Integer value for the permissions. Consult PHP chmod() documentation for + * more information. + * @return + * TRUE for success, FALSE in the event of an error. */ +function drupal_chmod($path, $mode = NULL) { + if (empty($mode)) { + if (is_dir($path)) { + $mode = variable_get('file_chmod_directory', 0775); + } + else { + $mode = variable_get('file_chmod_file', 0664); + } + } + if (@chmod($path, $mode)) { + return TRUE; + } + + watchdog('file', 'The file permissions could not be set on %path.', array('%path' => $path), WATCHDOG_ERROR); + return FALSE; +} + diff --git modules/color/color.module modules/color/color.module index dd26672..aed84bd 100644 --- modules/color/color.module +++ modules/color/color.module @@ -455,7 +455,7 @@ function _color_save_stylesheet($file, $style, &$paths) { $paths['files'][] = $filepath; // Set standard file permissions for webserver-generated files. - @chmod($file, 0664); + drupal_chmod($file); } /** @@ -513,7 +513,7 @@ function _color_render_images($theme, &$info, &$paths, $palette) { $paths['files'][] = $image; // Set standard file permissions for webserver-generated files - @chmod(realpath($image), 0664); + drupal_chmod($image); // Build before/after map of image paths. $paths['map'][$file] = $base; diff --git modules/simpletest/drupal_web_test_case.php modules/simpletest/drupal_web_test_case.php index d817ef7..d7eb7a4 100644 --- modules/simpletest/drupal_web_test_case.php +++ modules/simpletest/drupal_web_test_case.php @@ -861,7 +861,9 @@ class DrupalWebTestCase { $this->originalFileDirectory = file_directory_path(); variable_set('file_directory_path', file_directory_path() . '/' . $db_prefix); $directory = file_directory_path(); - file_check_directory($directory, FILE_CREATE_DIRECTORY); // Create the files directory. + // Create the files directory. + file_check_directory($directory, FILE_CREATE_DIRECTORY | FILE_MODIFY_PERMISSIONS); + set_time_limit($this->timeLimit); } diff --git modules/simpletest/tests/file.test modules/simpletest/tests/file.test index 988f045..7449d26 100644 --- modules/simpletest/tests/file.test +++ modules/simpletest/tests/file.test @@ -115,6 +115,30 @@ class FileTestCase extends DrupalWebTestCase { } /** + * Helper function to test the permissions of a directory. + * + * @param $directory + * String directory path. + * @param $expected_mode + * Octal integer like 0664 or 0777. + * @param $message + * Optional message. + */ + function assertDirectoryPermissions($directory, $expected_mode, $message = NULL) { + // Mask out all but the last three octets. + $actual_mode = fileperms($directory) & 511; + if (is_null($message)) { + if ($actual_mode == $expected_mode) { + $message = t('Directory permissions set correctly.'); + } + else { + $message = t('Expected directory permission to be %expected, actually were %actual.', array('%actual' => decoct($actual_mode), '%expected' => decoct($expected_mode))); + } + } + $this->assertEqual($actual_mode, $expected_mode, $message); + } + + /** * Create a directory and assert it exists. * * @param $path @@ -493,7 +517,7 @@ class FileUnmanagedSaveDataTest extends FileTestCase { $this->assertEqual(file_directory_path(), dirname($filepath), t("File was placed in Drupal's files directory.")); $this->assertEqual('asdf.txt', basename($filepath), t('File was named correctly.')); $this->assertEqual($contents, file_get_contents(realpath($filepath)), t('Contents of the file are correct.')); - $this->assertFilePermissions($filepath, 0664); + $this->assertFilePermissions($filepath, variable_get('file_chmod_file', 0664)); } } @@ -681,8 +705,8 @@ class FileDirectoryTest extends FileTestCase { // Test directory permission modification. $this->assertTrue(file_check_directory($directory, FILE_MODIFY_PERMISSIONS), t('No error reported when making directory writeable.'), 'File'); - // Verify directory actually is writeable. - $this->assertTrue(is_writeable($directory), t('Directory is writeable.'), 'File'); + // Test directory permission modification actually set correct permissions. + $this->assertDirectoryPermissions($directory, variable_get('file_chmod_directory', 0775)); // Remove .htaccess file to then test that it gets re-created. @unlink(file_directory_path() .'/.htaccess'); @@ -1077,7 +1101,7 @@ class FileUnmanagedMoveTest extends FileTestCase { $this->assertEqual($new_filepath, $desired_filepath, t('Returned expected filepath.')); $this->assertTrue(file_exists($new_filepath), t('File exists at the new location.')); $this->assertFalse(file_exists($file->filepath), t('No file remains at the old location.')); - $this->assertFilePermissions($new_filepath, 0664); + $this->assertFilePermissions($new_filepath, variable_get('file_chmod_file', 0664)); // Moving with rename. $desired_filepath = file_directory_path() . '/' . $this->randomName(); @@ -1088,7 +1112,7 @@ class FileUnmanagedMoveTest extends FileTestCase { $this->assertNotEqual($newer_filepath, $desired_filepath, t('Returned expected filepath.')); $this->assertTrue(file_exists($newer_filepath), t('File exists at the new location.')); $this->assertFalse(file_exists($new_filepath), t('No file remains at the old location.')); - $this->assertFilePermissions($newer_filepath, 0664); + $this->assertFilePermissions($newer_filepath, variable_get('file_chmod_file', 0664)); // TODO: test moving to a directory (rather than full directory/file path) } @@ -1149,7 +1173,7 @@ class FileUnmanagedCopyTest extends FileTestCase { $this->assertEqual($new_filepath, $desired_filepath, t('Returned expected filepath.')); $this->assertTrue(file_exists($file->filepath), t('Original file remains.')); $this->assertTrue(file_exists($new_filepath), t('New file exists.')); - $this->assertFilePermissions($new_filepath, 0664); + $this->assertFilePermissions($new_filepath, variable_get('file_chmod_file', 0664)); // Copying with rename. $desired_filepath = file_directory_path() . '/' . $this->randomName(); @@ -1159,7 +1183,7 @@ class FileUnmanagedCopyTest extends FileTestCase { $this->assertNotEqual($newer_filepath, $desired_filepath, t('Returned expected filepath.')); $this->assertTrue(file_exists($file->filepath), t('Original file remains.')); $this->assertTrue(file_exists($new_filepath), t('New file exists.')); - $this->assertFilePermissions($new_filepath, 0664); + $this->assertFilePermissions($new_filepath, variable_get('file_chmod_file', 0664)); // TODO: test copying to a directory (rather than full directory/file path) } @@ -1208,8 +1232,6 @@ class FileUnmanagedCopyTest extends FileTestCase { } } - - /** * Deletion related tests. */ diff --git modules/upload/upload.test modules/upload/upload.test index 218067f..d9ad017 100644 --- modules/upload/upload.test +++ modules/upload/upload.test @@ -199,6 +199,9 @@ class UploadTestCase extends DrupalWebTestCase { $this->drupalGet($base_url . '/' . file_directory_path() . '/' . $filename, array('external' => TRUE)); $this->assertResponse(array(200), 'Uploaded ' . $filename . ' is accessible.'); $this->assertEqual(file_get_contents($file), $this->drupalGetContent(), 'Uploaded contents of ' . $filename . ' verified.'); + // Verify file actually is readable and writeable by PHP. + $this->assertTrue(is_readable($file), t('Uploaded file is readable.')); + $this->assertTrue(is_writeable($file), t('Uploaded file is writeable.')); } /**