Merge lp:~ubuntudotcom1/maas/bug-1443346-atomic-write-copy-ownership into lp:~maas-committers/maas/trunk

[ubuntudotcom1]

Make utils.fs.atomic_write() preserve file ownership details if file previously exists.

Make utils.fs.atomic_write() preserve file ownership details if file previously exists.

[Mike Pontillo (mpontillo)]

Looks good, but one question below.

review: Approve

[Gavin Panella (allenap)]

Looks good. Lots of comments, most of which we've discussed in person.

review: Needs Fixing

[ubuntudotcom1 (ubuntudotcom1)]

Address comments.

[Gavin Panella (allenap)]

This looks great. Some more comments, but it's basically good to land.

review: Approve

=== modified file 'src/provisioningserver/utils/fs.py'
--- src/provisioningserver/utils/fs.py	2015-04-03 16:35:20 +0000
+++ src/provisioningserver/utils/fs.py	2015-04-15 12:05:23 +0000
@@ -29,7 +29,12 @@
 import hashlib
 from itertools import count
 import os
-from os import environ
+from os import (
+    environ,
+    rename,
+    stat,
+    chown,
+)
 from os.path import isdir
 from random import randint
 from shutil import rmtree
@@ -101,15 +106,25 @@
     """
     temp_file = _write_temp_file(content, filename)
     os.chmod(temp_file, mode)
+
+    # Copy over ownership attributes if file exists
+    try:
+        prev_stats = stat(filename)
+    except OSError as error:
+        if error.errno != errno.ENOENT:
+            raise  # Something's seriously wrong.
+    else:
+        chown(temp_file, prev_stats.st_uid, prev_stats.st_gid)
+
     try:
         if overwrite:
-            os.rename(temp_file, filename)
+            rename(temp_file, filename)
         else:
             lock = FileLock(filename)
             lock.acquire()
             try:
                 if not os.path.isfile(filename):
-                    os.rename(temp_file, filename)
+                    rename(temp_file, filename)
             finally:
                 lock.release()
     finally:

=== modified file 'src/provisioningserver/utils/tests/test_fs.py'
--- src/provisioningserver/utils/tests/test_fs.py	2015-04-03 15:48:28 +0000
+++ src/provisioningserver/utils/tests/test_fs.py	2015-04-15 12:05:23 +0000
@@ -32,7 +32,11 @@
 from maastesting.fakemethod import FakeMethod
 from maastesting.matchers import MockCalledOnceWith
 from maastesting.testcase import MAASTestCase
-from mock import Mock
+from mock import (
+    ANY,
+    Mock,
+    sentinel,
+)
 from provisioningserver.utils.fs import (
     atomic_symlink,
     atomic_write,
@@ -94,7 +98,7 @@
     def test_atomic_write_does_not_leak_temp_file_on_failure(self):
         # If the overwrite fails, atomic_write does not leak its
         # temporary file.
-        self.patch(os, 'rename', Mock(side_effect=OSError()))
+        self.patch(fs_module, 'rename', Mock(side_effect=OSError()))
         filename = self.make_file()
         with ExpectedException(OSError):
             atomic_write(factory.make_string(), filename)
@@ -119,7 +123,7 @@
             """Stub for os.rename: get source file's access mode."""
             recorded_modes.append(os.stat(source).st_mode)
 
-        self.patch(os, 'rename', Mock(side_effect=record_mode))
+        self.patch(fs_module, 'rename', Mock(side_effect=record_mode))
         playground = self.make_dir()
         atomic_file = os.path.join(playground, factory.make_name('atomic'))
         mode = 0o323
@@ -127,6 +131,25 @@
         [recorded_mode] = recorded_modes
         self.assertEqual(mode, stat.S_IMODE(recorded_mode))
 
+    def test_atomic_write_preserves_ownership_before_moving_into_place(self):
+        atomic_file = self.make_file('atomic')
+
+        self.patch(fs_module, 'isfile').return_value = True
+        self.patch(fs_module, 'chown')
+        self.patch(fs_module, 'rename')
+        self.patch(fs_module, 'stat')
+
+        ret_stat = fs_module.stat.return_value
+        ret_stat.st_uid = sentinel.uid
+        ret_stat.st_gid = sentinel.gid
+        ret_stat.st_mode = stat.S_IFREG
+
+        atomic_write(factory.make_string(), atomic_file)
+
+        self.assertThat(fs_module.stat, MockCalledOnceWith(atomic_file))
+        self.assertThat(fs_module.chown, MockCalledOnceWith(
+            ANY, sentinel.uid, sentinel.gid))
+
     def test_atomic_write_sets_OSError_filename_if_undefined(self):
         # When the filename attribute of an OSError is undefined when
         # attempting to create a temporary file, atomic_write fills it in with