launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/ppa-htpasswd-fixes into lp:launchpad.

Commit message:
Write P3A .htpasswd tempfiles to a separate temp directory, and ensure they're cleaned up afterwards.

Requested reviews:
  William Grant (wgrant): code
Related bugs:
  Bug #1386822 in Launchpad itself: "staging files for private ppa .htpasswd files are not properly  cleaned up"
  https://bugs.launchpad.net/launchpad/+bug/1386822

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/ppa-htpasswd-fixes/+merge/239943
-- 
https://code.launchpad.net/~wgrant/launchpad/ppa-htpasswd-fixes/+merge/239943
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

=== modified file 'lib/lp/archivepublisher/scripts/generate_ppa_htaccess.py'
--- lib/lp/archivepublisher/scripts/generate_ppa_htaccess.py	2014-08-29 01:34:04 +0000
+++ lib/lp/archivepublisher/scripts/generate_ppa_htaccess.py	2014-10-29 06:29:18 +0000
@@ -84,9 +84,9 @@
         """
         # Create a temporary file that will be a new .htpasswd.
         pub_config = getPubConfig(ppa)
-        if not os.path.exists(pub_config.archiveroot):
-            os.makedirs(pub_config.archiveroot)
-        fd, temp_filename = tempfile.mkstemp(dir=pub_config.archiveroot)
+        if not os.path.exists(pub_config.temproot):
+            os.makedirs(pub_config.temproot)
+        fd, temp_filename = tempfile.mkstemp(dir=pub_config.temproot)
         os.close(fd)
 
         write_htpasswd(
@@ -99,22 +99,29 @@
 
         :return: True if the file was replaced.
         """
-        if self.options.dryrun:
+        try:
+            if self.options.dryrun:
+                return False
+
+            # The publisher Config object does not have an
+            # interface, so we need to remove the security wrapper.
+            pub_config = getPubConfig(ppa)
+            if not os.path.exists(pub_config.archiveroot):
+                os.makedirs(pub_config.archiveroot)
+            htpasswd_filename = os.path.join(
+                pub_config.archiveroot, ".htpasswd")
+
+            if (not os.path.isfile(htpasswd_filename) or
+                not filecmp.cmp(htpasswd_filename, temp_htpasswd_file)):
+                # Atomically replace the old file or create a new file.
+                os.rename(temp_htpasswd_file, htpasswd_filename)
+                self.logger.debug("Replaced htpasswd for %s" % ppa.displayname)
+                return True
+
             return False
-
-        # The publisher Config object does not have an
-        # interface, so we need to remove the security wrapper.
-        pub_config = getPubConfig(ppa)
-        htpasswd_filename = os.path.join(pub_config.archiveroot, ".htpasswd")
-
-        if (not os.path.isfile(htpasswd_filename) or
-            not filecmp.cmp(htpasswd_filename, temp_htpasswd_file)):
-            # Atomically replace the old file or create a new file.
-            os.rename(temp_htpasswd_file, htpasswd_filename)
-            self.logger.debug("Replaced htpasswd for %s" % ppa.displayname)
-            return True
-
-        return False
+        finally:
+            if os.path.exists(temp_htpasswd_file):
+                os.unlink(temp_htpasswd_file)
 
     def sendCancellationEmail(self, token):
         """Send an email to the person whose subscription was cancelled."""

=== modified file 'lib/lp/archivepublisher/tests/test_generate_ppa_htaccess.py'
--- lib/lp/archivepublisher/tests/test_generate_ppa_htaccess.py	2014-07-07 08:23:51 +0000
+++ lib/lp/archivepublisher/tests/test_generate_ppa_htaccess.py	2014-10-29 06:29:18 +0000
@@ -133,11 +133,11 @@
         filename = script.generateHtpasswd(self.ppa)
         self.addCleanup(remove_if_exists, filename)
 
-        # It should be a temp file in the same directory as the intended
-        # target file when it's renamed, so that os.rename() won't
-        # complain about renaming across file systems.
+        # It should be a temp file on the same filesystem as the target
+        # file, so os.rename() won't explode. temproot is relied on
+        # elsewhere for this same purpose, so it should be safe.
         pub_config = getPubConfig(self.ppa)
-        self.assertEqual(pub_config.archiveroot, os.path.dirname(filename))
+        self.assertEqual(pub_config.temproot, os.path.dirname(filename))
 
         # Read it back in.
         file_contents = [
@@ -166,21 +166,29 @@
         write_file(filename, FILE_CONTENT)
 
         # Write the same contents in a temp file.
-        fd, temp_filename = tempfile.mkstemp(dir=pub_config.archiveroot)
-        file = os.fdopen(fd, "w")
-        file.write(FILE_CONTENT)
-        file.close()
+        def write_tempfile():
+            fd, temp_filename = tempfile.mkstemp(dir=pub_config.archiveroot)
+            file = os.fdopen(fd, "w")
+            file.write(FILE_CONTENT)
+            file.close()
+            return temp_filename
 
         # Replacement should not happen.
+        temp_filename = write_tempfile()
         script = self.getScript()
+        self.assertTrue(os.path.exists(temp_filename))
         self.assertFalse(
             script.replaceUpdatedHtpasswd(self.ppa, temp_filename))
+        self.assertFalse(os.path.exists(temp_filename))
 
         # Writing a different .htpasswd should see it get replaced.
         write_file(filename, "Come to me, son of Jor-El!")
 
+        temp_filename = write_tempfile()
+        self.assertTrue(os.path.exists(temp_filename))
         self.assertTrue(
             script.replaceUpdatedHtpasswd(self.ppa, temp_filename))
+        self.assertFalse(os.path.exists(temp_filename))
 
         os.remove(filename)