launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/poppy-sftp-gpgconf into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #757248 in Launchpad itself: "poppy-sftp's signature checking relies on long-term survival of a directory in /tmp"
  https://bugs.launchpad.net/launchpad/+bug/757248

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/poppy-sftp-gpgconf/+merge/59154

See bug 757248. A reaper script deletes the gpghandler config firectory from /tmp.

== Implementation ==

A few approaches were mooted in the bug report. After discussing with Curtis, we decided the most robust, lowest risk approach was to simply touch the config directory every 12 hours.

I used a twisted LoopingCall instance to call os.utime. The interval is hard coded to 12 hours. It is overkill to make this configurable given the circumstances of its usage. 

I had to package the functionality to start the job into a new method _scheduleTouchHomeDirectoryJob. This was to allow testing - the test needed to be able to reschedule the job to run over a much shorter interval than 12 hours to check the home dire modification time was being updated.

== Tests ==

A new test was added to TestImportKeyRing:
  testHomeDirectoryJob

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/canonical/launchpad/utilities/gpghandler.py
  lib/canonical/launchpad/utilities/ftests/test_gpghandler.py

./lib/canonical/launchpad/utilities/ftests/test_gpghandler.py
     182: E301 expected 1 blank line, found 2

-- 
https://code.launchpad.net/~wallyworld/launchpad/poppy-sftp-gpgconf/+merge/59154
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/poppy-sftp-gpgconf into lp:launchpad.

=== modified file 'lib/canonical/launchpad/utilities/ftests/test_gpghandler.py'
--- lib/canonical/launchpad/utilities/ftests/test_gpghandler.py	2010-10-04 19:50:45 +0000
+++ lib/canonical/launchpad/utilities/ftests/test_gpghandler.py	2011-04-27 01:58:27 +0000
@@ -4,7 +4,10 @@
 import unittest
 
 import gpgme
+import os
+import time
 from zope.component import getUtility
+from zope.security.proxy import removeSecurityProxy
 
 from canonical.launchpad.ftests import (
     ANONYMOUS,
@@ -176,11 +179,17 @@
         self.assertEqual(self.gpg_handler.checkTrustDb(), 0)
 
 
-def test_suite():
-    return unittest.TestLoader().loadTestsFromName(__name__)
-
-
-if __name__ == "__main__":
-    unittest.main(defaultTest=test_suite())
-
-
+    def testHomeDirectoryJob(self):
+        """Does the job to touch the home work."""
+        naked_gpgjandler = removeSecurityProxy(self.gpg_handler)
+        self.assertTrue(naked_gpgjandler._touch_home_call.running)
+        # It should be initially scheduled for every 12 hours.
+        self.assertEqual(12*3600, naked_gpgjandler._touch_home_call.interval)
+
+        first_time_modified = os.path.getmtime(naked_gpgjandler.home)
+        # Reschedule the job to run every 2 seconds
+        naked_gpgjandler._scheduleTouchHomeDirectoryJob(2)
+        # Wait and re-check last modified time
+        time.sleep(3)
+        second_time_modified = os.path.getmtime(naked_gpgjandler.home)
+        self.assertTrue(first_time_modified+2<second_time_modified)

=== modified file 'lib/canonical/launchpad/utilities/gpghandler.py'
--- lib/canonical/launchpad/utilities/gpghandler.py	2011-02-23 20:26:53 +0000
+++ lib/canonical/launchpad/utilities/gpghandler.py	2011-04-27 01:58:27 +0000
@@ -24,6 +24,7 @@
 
 import gpgme
 from gpgme import editutil as gpgme_editutil
+from twisted.internet import task
 from zope.interface import implements
 
 from canonical.config import config
@@ -63,6 +64,9 @@
 
     implements(IGPGHandler)
 
+    # The twisted job to touch the home directory.
+    _touch_home_call = None
+
     def __init__(self):
         """Initialize environment variable."""
         self._setNewHome()
@@ -72,9 +76,10 @@
         """Create a new directory containing the required configuration.
 
         This method is called inside the class constructor and genereates
-        a new directory (name ramdomly generated with the 'gpg-' prefix)
+        a new directory (name randomly generated with the 'gpg-' prefix)
         containing the proper file configuration and options.
 
+        Also touches the config directory every 12 hours.
         Also installs an atexit handler to remove the directory on normal
         process termination.
         """
@@ -89,15 +94,32 @@
                    'keyserver-options auto-key-retrieve\n'
                    'no-auto-check-trustdb\n' % config.gpghandler.host)
         conf.close()
+
+        # 1 hour = 3600 seconds
+        self._scheduleTouchHomeDirectoryJob(12*3600)
+
         # create a local atexit handler to remove the configuration directory
         # on normal termination.
         def removeHome(home):
-            """Remove GNUPGHOME directory."""
+            """Remove GNUPGHOME directory and stop the twisted job."""
+            self._touch_home_call.stop()
             if os.path.exists(home):
                 shutil.rmtree(home)
 
         atexit.register(removeHome, self.home)
 
+    def _scheduleTouchHomeDirectoryJob(self, touch_interval):
+        # Create a job to touch the home directory every 12 hours so that it
+        # does not get cleaned up by any reaper scripts which look at
+        # time last modified. This is done outside of _setHome to allow a
+        # test to be written which sets the touch interval to a value short
+        # enough for a viable test.
+
+        if self._touch_home_call:
+            self._touch_home_call.stop()
+        self._touch_home_call = task.LoopingCall(os.utime, *(self.home, None))
+        self._touch_home_call.start(touch_interval)
+
     def sanitizeFingerprint(self, fingerprint):
         """See IGPGHandler."""
         # remove whitespaces, truncate to max of 40 (as per v4 keys) and