Merge lp:~leonardr/launchpadlib/bug-712808 into lp:launchpadlib

[Leonard Richardson]

This branch fixes bug 712808 by changing launchpadlib's behavior when consumer_name is specified as a string. The previous behavior was to treat consumer_name as an OAuth consumer, but to try to do a desktop-wide integration. This is internally inconsistent--either the "consumer_name" should be treated as an application name and we should perform a desktop-wide integration, or we should try to do an application-specific integration. Since we've already decided to do desktop-wide integration in ordinary cases, I chose to treat a string consumer_name as the application name for a desktop-wide credential.

This code can't be tested normally, but it's easy to test manually, by running this code when you have no active credential on staging.launchpad.net:

Launchpad.get_token_and_login("my-app")

[lp:~leonardr/launchpadlib/bug-712808]

114. By Leonard Richardson on 2011-02-07

Moved the code to check the credential store into _authorize_token_and_login, so that it's used by all code paths.

[Leonard Richardson (leonardr)]

I noticed another problem: get_token_and_login() works, and it stores credentials in the keyring, but it doesn't _read_ credentials from the keyring the second time you run the code, because the code to look in the keyring was in login_with(). I moved that code to _authorize_token_and_login, which is called by all code paths, and now get_token_and_login() works just as well as login_with().

[lp:~leonardr/launchpadlib/bug-712808]

115. By Leonard Richardson on 2011-02-07

Don't overwrite the credentials variable after going through so much trouble to create it.

[Graham Binns (gmb)]

review: Approve (code)

=== modified file 'src/launchpadlib/NEWS.txt'
--- src/launchpadlib/NEWS.txt	2011-01-18 21:57:09 +0000
+++ src/launchpadlib/NEWS.txt	2011-02-07 20:13:43 +0000
@@ -2,6 +2,13 @@
 NEWS for launchpadlib
 =====================
 
+1.9.5 (2010-02-07)
+==================
+
+- Fixed a bug that prevented the deprecated get_token_and_login code
+  from working, and required that users of get_token_and_login get a
+  new token on every usage.
+
 1.9.4 (2010-01-18)
 ==================
 
 
=== modified file 'src/launchpadlib/__init__.py'
--- src/launchpadlib/__init__.py	2011-01-18 22:33:53 +0000
+++ src/launchpadlib/__init__.py	2011-02-07 20:13:43 +0000
@@ -14,4 +14,4 @@
 # You should have received a copy of the GNU Lesser General Public License
 # along with launchpadlib. If not, see <http://www.gnu.org/licenses/>.
 
-__version__ = '1.9.4'
+__version__ = '1.9.5'
 
=== modified file 'src/launchpadlib/launchpad.py'
--- src/launchpadlib/launchpad.py	2011-01-18 21:57:09 +0000
+++ src/launchpadlib/launchpad.py	2011-02-07 20:13:43 +0000
@@ -31,6 +31,7 @@
     ScalarValue,          # Re-import for client convenience
     ServiceRoot,
     )
+from lazr.restfulclient.authorize.oauth import SystemWideConsumer
 from lazr.restfulclient._browser import RestfulHttp
 from launchpadlib.credentials import (
     AccessToken,
@@ -309,16 +310,18 @@
         get_token_and_login() is removed, this code can be streamlined
         and moved into its other call site, login_with().
         """
-
         if isinstance(consumer_name, Consumer):
-            # Create the credentials with no Consumer, then set its .consumer
-            # property directly.
-            credentials = Credentials(None)
-            credentials.consumer = consumer_name
+            consumer = consumer_name
         else:
-            # Have the Credentials constructor create the Consumer
-            # automatically.
-            credentials = Credentials(consumer_name)
+            # Create a system-wide consumer. lazr.restfulclient won't
+            # do this automatically, but launchpadlib's default is to
+            # do a desktop-wide integration.
+            consumer = SystemWideConsumer(consumer_name)
+
+        # Create the credentials with no Consumer, then set its .consumer
+        # property directly.
+        credentials = Credentials(None)
+        credentials.consumer = consumer
         if authorization_engine is None:
             authorization_engine = cls.authorization_engine_factory(
                 service_root, consumer_name, None, allow_access_levels)
@@ -335,7 +338,24 @@
                 credential_store.credential_save_failed,
                 "credential_store")
 
-        credentials = authorization_engine(credentials, credential_store)
+        # Try to get the credentials out of the credential store.
+        cached_credentials = credential_store.load(
+            authorization_engine.unique_consumer_id)
+        if cached_credentials is None:
+            # They're not there. Acquire new credentials using the
+            # authorization engine.
+            credentials = authorization_engine(credentials, credential_store)
+        else:
+            # We acquired credentials. But, the application name
+            # wasn't stored along with the credentials, because in a
+            # desktop integration scenario, a single set of
+            # credentials may be shared by many applications. We need
+            # to set the application name for this specific instance
+            # of the credentials.
+            credentials = cached_credentials
+            credentials.consumer.application_name = (
+                authorization_engine.application_name)
+
         return cls(credentials, authorization_engine, credential_store,
                    service_root, cache, timeout, proxy_info, version)
 
@@ -513,33 +533,11 @@
                 "allow_access_levels", allow_access_levels,
                 authorization_engine.allow_access_levels)
 
-        credentials = credential_store.load(
-            authorization_engine.unique_consumer_id)
-
-        if credentials is None:
-            # Credentials were not found in the local store. Go
-            # through the authorization process.
-            launchpad = cls._authorize_token_and_login(
-                authorization_engine.consumer, service_root,
-                cache_path, timeout, proxy_info, authorization_engine,
-                allow_access_levels, credential_store,
-                credential_save_failed, version)
-        else:
-            # The application name wasn't stored locally, because in a
-            # desktop integration scenario, a single set of
-            # credentials may be shared by many applications. We need
-            # to set the application name for this specific instance
-            # of the credentials.
-            credentials.consumer.application_name = (
-                authorization_engine.application_name)
-
-            # Credentials were found in the local store. Create a
-            # Launchpad object.
-            launchpad = cls(
-                credentials, authorization_engine, credential_store,
-                service_root=service_root, cache=cache_path, timeout=timeout,
-                proxy_info=proxy_info, version=version)
-        return launchpad
+        return cls._authorize_token_and_login(
+            authorization_engine.consumer, service_root,
+            cache_path, timeout, proxy_info, authorization_engine,
+            allow_access_levels, credential_store,
+            credential_save_failed, version)
 
     @classmethod
     def _warn_of_deprecated_login_method(cls, name):