diff --git a/django/core/files/move.py b/django/core/files/move.py index 2d71e11885..95d69f9d94 100644 --- a/django/core/files/move.py +++ b/django/core/files/move.py @@ -5,9 +5,8 @@ Move a file in the safest way possible:: >>> file_move_safe("/tmp/old_file", "/tmp/new_file") """ -import errno import os -from shutil import copystat +from shutil import copymode, copystat from django.core.files import locks @@ -82,12 +81,15 @@ def file_move_safe( try: copystat(old_file_name, new_file_name) - except PermissionError as e: + except PermissionError: # Certain filesystems (e.g. CIFS) fail to copy the file's metadata if # the type of the destination filesystem isn't the same as the source - # filesystem; ignore that. - if e.errno != errno.EPERM: - raise + # filesystem. This also happens with some SELinux-enabled systems. + # Ignore that, but try to set basic permissions. + try: + copymode(old_file_name, new_file_name) + except PermissionError: + pass try: os.remove(old_file_name) diff --git a/tests/files/tests.py b/tests/files/tests.py index 9777d09e1c..2f9b5b92c1 100644 --- a/tests/files/tests.py +++ b/tests/files/tests.py @@ -419,35 +419,66 @@ class FileMoveSafeTests(unittest.TestCase): os.close(handle_a) os.close(handle_b) - def test_file_move_copystat_cifs(self): + def test_file_move_permissionerror(self): """ - file_move_safe() ignores a copystat() EPERM PermissionError. This - happens when the destination filesystem is CIFS, for example. + file_move_safe() ignores PermissionError thrown by copystat() and + by copymode(). + This is raised when the destination filesystem is CIFS, for + example. This also can be raised when relabel is disabled by + SELinux across filesystems. """ - copystat_EACCES_error = PermissionError(errno.EACCES, "msg") - copystat_EPERM_error = PermissionError(errno.EPERM, "msg") + permission_error = PermissionError(errno.EPERM, "msg") + os_error = OSError("msg") handle_a, self.file_a = tempfile.mkstemp() handle_b, self.file_b = tempfile.mkstemp() + handle_c, self.file_c = tempfile.mkstemp() try: # This exception is required to reach the copystat() call in # file_safe_move(). with mock.patch("django.core.files.move.os.rename", side_effect=OSError()): - # An error besides EPERM isn't ignored. + # An error besides PermissionError isn't ignored. with mock.patch( - "django.core.files.move.copystat", side_effect=copystat_EACCES_error + "django.core.files.move.copystat", side_effect=os_error ): - with self.assertRaises(PermissionError): + with self.assertRaises(OSError): file_move_safe(self.file_a, self.file_b, allow_overwrite=True) - # EPERM is ignored. + + # When copystat() throws PermissionError, copymode() error besides + # PermissionError isn't ignored. with mock.patch( - "django.core.files.move.copystat", side_effect=copystat_EPERM_error + "django.core.files.move.copystat", side_effect=permission_error + ): + with mock.patch( + "django.core.files.move.copymode", side_effect=os_error + ): + with self.assertRaises(OSError): + file_move_safe( + self.file_a, self.file_b, allow_overwrite=True + ) + + # PermissionError raised by copystat() is ignored. + with mock.patch( + "django.core.files.move.copystat", + side_effect=permission_error, ): self.assertIsNone( file_move_safe(self.file_a, self.file_b, allow_overwrite=True) ) + + # PermissionError raised by copymode() is ignored too. + with mock.patch( + "django.core.files.move.copymode", + side_effect=permission_error, + ): + self.assertIsNone( + file_move_safe( + self.file_c, self.file_b, allow_overwrite=True + ) + ) finally: os.close(handle_a) os.close(handle_b) + os.close(handle_c) class SpooledTempTests(unittest.TestCase):