Centralize file permisisons changing into drupal_chmod() - http://drupal.org/node/203204 From: andrew morton --- includes/file.inc | 58 ++++++++++++++++++++++----- includes/image.inc | 3 - modules/color/color.module | 4 +- modules/simpletest/drupal_web_test_case.php | 4 +- modules/simpletest/tests/file.test | 56 ++++++++++++++++++-------- modules/upload/upload.test | 3 + 6 files changed, 94 insertions(+), 34 deletions(-) diff --git includes/file.inc includes/file.inc index b9f4ed9..8b848be 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,43 @@ 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 uploaded/generated files. + * By default these will give everyone read access so that users accessing the + * files with a user account without the webserver group (e.g. via FTP) can read + * these files, and give group write permissions so webserver group members + * (e.g. a vhost account) can alter files uploaded and owned 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 (!isset($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; +} + +/** + * @} End of "defgroup file". + */ \ No newline at end of file diff --git includes/image.inc includes/image.inc index 89d2e51..dd82df8 100644 --- includes/image.inc +++ includes/image.inc @@ -373,10 +373,9 @@ function image_save(stdClass $image, $destination = NULL) { clearstatcache(); $image->info = image_get_info($destination); - if (@chmod($destination, 0664)) { + if (drupal_chmod($destination)) { return $return; } - watchdog('image', 'Could not set permissions on destination file: %file', array('%file' => $destination)); } 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 31e7096..69bde7b 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 bea3be3..6e7a99a 100644 --- modules/simpletest/tests/file.test +++ modules/simpletest/tests/file.test @@ -101,15 +101,35 @@ class FileTestCase extends DrupalWebTestCase { * Optional message. */ function assertFilePermissions($filepath, $expected_mode, $message = NULL) { + // Clear out PHP's file stat cache to be sure we see the current value. + clearstatcache(); + // Mask out all but the last three octets. $actual_mode = fileperms($filepath) & 511; - if (is_null($message)) { - if ($actual_mode == $expected_mode) { - $message = t('File permissions set correctly.'); - } - else { - $message = t('Expected file permission to be %expected, actually were %actual.', array('%actual' => decoct($actual_mode), '%expected' => decoct($expected_mode))); - } + if (!isset($message)) { + $message = t('Expected file permission to be %expected, actually were %actual.', array('%actual' => decoct($actual_mode), '%expected' => decoct($expected_mode))); + } + $this->assertEqual($actual_mode, $expected_mode, $message); + } + + /** + * 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) { + // Clear out PHP's file stat cache to be sure we see the current value. + clearstatcache(); + + // Mask out all but the last three octets. + $actual_mode = fileperms($directory) & 511; + if (!isset($message)) { + $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); } @@ -493,7 +513,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 +701,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 +1097,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 +1108,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,17 +1169,17 @@ 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(); $this->assertTrue(file_put_contents($desired_filepath, ' '), t('Created a file so a rename will have to happen.')); - $newer_filepath = file_unmanaged_copy($new_filepath, $desired_filepath, FILE_EXISTS_RENAME); + $newer_filepath = file_unmanaged_copy($file->filepath, $desired_filepath, FILE_EXISTS_RENAME); $this->assertTrue($newer_filepath, t('Copy was successful.')); $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->assertTrue(file_exists($newer_filepath), t('New file exists.')); + $this->assertFilePermissions($newer_filepath, variable_get('file_chmod_file', 0664)); // TODO: test copying to a directory (rather than full directory/file path) } @@ -1188,6 +1208,7 @@ class FileUnmanagedCopyTest extends FileTestCase { $this->assertNotEqual($new_filepath, $file->filepath, t('Copied file has a new name.')); $this->assertTrue(file_exists($file->filepath), t('Original file exists after copying onto itself.')); $this->assertTrue(file_exists($new_filepath), t('Copied file exists after copying onto itself.')); + $this->assertFilePermissions($new_filepath, variable_get('file_chmod_file', 0664)); // Copy the file onto itself without renaming fails. $new_filepath = file_unmanaged_copy($file->filepath, $file->filepath, FILE_EXISTS_ERROR); @@ -1205,11 +1226,10 @@ class FileUnmanagedCopyTest extends FileTestCase { $this->assertNotEqual($new_filepath, $file->filepath, t('Copied file has a new name.')); $this->assertTrue(file_exists($file->filepath), t('Original file exists after copying onto itself.')); $this->assertTrue(file_exists($new_filepath), t('Copied file exists after copying onto itself.')); + $this->assertFilePermissions($new_filepath, variable_get('file_chmod_file', 0664)); } } - - /** * Deletion related tests. */ diff --git modules/upload/upload.test modules/upload/upload.test index ac8c2eb..f7c839b 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.')); } /**