Merge lp:~benji/launchpadlib/delayed-keyring-import into lp:launchpadlib

[Benji York]

The keyring package initializes the back end storage at import time. If
the back end chosen is KDE, a couple of bad things happen: a dialog box
pops up asking for access to KWallet and a SIGCHLD handler is registered
which interferes with subprocess completion notification.

This branch delays the keyring import until the keyring is needed,
mitigating these problems. This approach has been approved by Leonard
and Gary.

This branch also eliminates the launchpadlib's test's usage of the real
keyring package because it introduces fragility and environment
dependencies into the test. The test suite also has enforcement that
ensures that non-testing keyring fakes are never used.

One class of test (SimulatedResponsesTestCase) had an off-by-one error
that these fixes exposed, those have been fixed as well.

[Henning Eggers (henninge)]

The changes look good, thank you. I just wonder how the definition of "credential_store_factory" relates to this. Is that supposed to be in here? Please have a look before you land this.

review: Approve (code)

[Benji York (benji)]

On Mon, Jan 10, 2011 at 12:51 PM, Henning Eggers
<email address hidden> wrote:
> Review: Approve code
> The changes look good, thank you. I just wonder how the definition of
> "credential_store_factory" relates to this. Is that supposed to be in here?
> Please have a look before you land this.

The credential_store_factory definition in question is for a class
that's used in tests. The credential_store_factory returns the place
where credentials should be stored, which for the tests in question is
on disk (as opposed to in the keyring).

Thanks for the review.
--
Benji York

=== modified file 'src/launchpadlib/credentials.py'
--- src/launchpadlib/credentials.py	2011-01-05 13:31:26 +0000
+++ src/launchpadlib/credentials.py	2011-01-10 16:23:59 +0000
@@ -37,7 +37,6 @@
 from urlparse import urljoin
 import webbrowser
 
-import keyring
 import simplejson
 
 from lazr.restfulclient.errors import HTTPError
@@ -295,13 +294,28 @@
     integrating third-party websites into Launchpad.
     """
 
+    @staticmethod
+    def _ensure_keyring_imported():
+        """Ensure the keyring module is imported (postponing side effects).
+
+        The keyring module initializes the environment-dependent backend at
+        import time (nasty).  We want to avoid that initialization because it
+        may do things like prompt the user to unlock their password store
+        (e.g., KWallet).
+        """
+        if 'keyring' not in globals():
+            global keyring
+            import keyring
+
     def do_save(self, credentials, unique_key):
         """Store newly-authorized credentials in the keyring."""
+        self._ensure_keyring_imported()
         keyring.set_password(
             'launchpadlib', unique_key, credentials.serialize())
 
     def do_load(self, unique_key):
         """Retrieve credentials from the keyring."""
+        self._ensure_keyring_imported()
         credential_string = keyring.get_password(
             'launchpadlib', unique_key)
         if credential_string is not None:

=== modified file 'src/launchpadlib/testing/helpers.py'
--- src/launchpadlib/testing/helpers.py	2011-01-04 20:00:37 +0000
+++ src/launchpadlib/testing/helpers.py	2011-01-10 16:23:59 +0000
@@ -44,6 +44,14 @@
     )
 
 
+missing = object()
+
+
+def assert_keyring_not_imported():
+    assert getattr(launchpadlib.credentials, 'keyring', missing) is missing, (
+        'During tests the real keyring module should never be imported.')
+
+
 class NoNetworkAuthorizationEngine(RequestTokenAuthorizationEngine):
     """An authorization engine that doesn't open a web browser.
 
@@ -121,12 +129,14 @@
 
 @contextmanager
 def fake_keyring(fake):
-    original_keyring = launchpadlib.credentials.keyring
+    """A context manager which injects a testing keyring implementation."""
+    # The real keyring package should never be imported during tests.
+    assert_keyring_not_imported()
     launchpadlib.credentials.keyring = fake
     try:
         yield
     finally:
-        launchpadlib.credentials.keyring = original_keyring
+        del launchpadlib.credentials.keyring
 
 
 class FauxSocketModule:

=== modified file 'src/launchpadlib/tests/test_http.py'
--- src/launchpadlib/tests/test_http.py	2011-01-04 18:46:27 +0000
+++ src/launchpadlib/tests/test_http.py	2011-01-10 16:23:59 +0000
@@ -17,11 +17,13 @@
 """Tests for the LaunchpadOAuthAwareHTTP class."""
 
 from collections import deque
+import tempfile
 import unittest
 
 from simplejson import dumps, JSONDecodeError
 
 from launchpadlib.errors import Unauthorized
+from launchpadlib.credentials import UnencryptedFileCredentialStore
 from launchpadlib.launchpad import (
     Launchpad,
     LaunchpadOAuthAwareHttp,
@@ -92,6 +94,11 @@
         return SimulatedResponsesHttp(
             deque(self.responses), self, self.authorization_engine, *args)
 
+    @classmethod
+    def credential_store_factory(cls, credential_save_failed):
+        return UnencryptedFileCredentialStore(
+            tempfile.mkstemp()[1], credential_save_failed)
+
 
 class SimulatedResponsesTestCase(unittest.TestCase):
     """Test cases that give fake responses to launchpad's HTTP requests."""
@@ -166,7 +173,7 @@
         self.assertEquals(self.engine.access_tokens_obtained, 0)
         launchpad = SimulatedResponsesLaunchpad.login_with(
             'application name', authorization_engine=self.engine)
-        self.assertEquals(self.engine.access_tokens_obtained, 0)
+        self.assertEquals(self.engine.access_tokens_obtained, 1)
 
     def test_bad_token(self):
         """If our token is bad, we get another one."""
@@ -178,7 +185,7 @@
         self.assertEquals(self.engine.access_tokens_obtained, 0)
         launchpad = SimulatedResponsesLaunchpad.login_with(
             'application name', authorization_engine=self.engine)
-        self.assertEquals(self.engine.access_tokens_obtained, 1)
+        self.assertEquals(self.engine.access_tokens_obtained, 2)
 
     def test_expired_token(self):
         """If our token is expired, we get another one."""
@@ -191,7 +198,7 @@
         self.assertEquals(self.engine.access_tokens_obtained, 0)
         launchpad = SimulatedResponsesLaunchpad.login_with(
             'application name', authorization_engine=self.engine)
-        self.assertEquals(self.engine.access_tokens_obtained, 1)
+        self.assertEquals(self.engine.access_tokens_obtained, 2)
 
     def test_delayed_error(self):
         """We get another token no matter when the error happens."""
@@ -203,7 +210,7 @@
         self.assertEquals(self.engine.access_tokens_obtained, 0)
         launchpad = SimulatedResponsesLaunchpad.login_with(
             'application name', authorization_engine=self.engine)
-        self.assertEquals(self.engine.access_tokens_obtained, 1)
+        self.assertEquals(self.engine.access_tokens_obtained, 2)
 
     def test_many_errors(self):
         """We'll keep getting new tokens as long as tokens are the problem."""
@@ -216,7 +223,7 @@
         self.assertEquals(self.engine.access_tokens_obtained, 0)
         launchpad = SimulatedResponsesLaunchpad.login_with(
             'application name', authorization_engine=self.engine)
-        self.assertEquals(self.engine.access_tokens_obtained, 3)
+        self.assertEquals(self.engine.access_tokens_obtained, 4)
 
     def test_other_unauthorized(self):
         """If the token is not at fault, a 401 error raises an exception."""

=== modified file 'src/launchpadlib/tests/test_launchpad.py'
--- src/launchpadlib/tests/test_launchpad.py	2011-01-05 13:31:26 +0000
+++ src/launchpadlib/tests/test_launchpad.py	2011-01-10 16:23:59 +0000
@@ -37,6 +37,7 @@
 import launchpadlib.launchpad
 from launchpadlib.launchpad import Launchpad
 from launchpadlib.testing.helpers import (
+    assert_keyring_not_imported,
     BadSaveKeyring,
     fake_keyring,
     FauxSocketModule,
@@ -197,15 +198,15 @@
     """Base class for tests that use the keyring."""
 
     def setUp(self):
-        # For these tests we want to disable retrieving or storing credentials
-        # in a system-provided keyring and use a dummy keyring implementation
-        # instaed.
-        self._saved_keyring = launchpadlib.credentials.keyring
+        # The real keyring package should never be imported during tests.
+        assert_keyring_not_imported()
+        # For these tests we want to use a dummy keyring implementation
+        # that only stores data in memory.
         launchpadlib.credentials.keyring = InMemoryKeyring()
 
     def tearDown(self):
-        # Restore the gnomekeyring module that we disabled in setUp.
-        launchpadlib.credentials.keyring = self._saved_keyring
+        # Remove the fake keyring module we injected during setUp.
+        del launchpadlib.credentials.keyring
 
 
 class TestLaunchpadLoginWith(KeyringTest):