Merge lp:~leonardr/launchpadlib/ask-for-desktop-integration into lp:launchpadlib

[Leonard Richardson]

This branch changes the default behavior to make launchpadlib ask Launchpad for a desktop-wide integration token instead of an application-specific token. The former "consumer name" is now an "application name" (concept defined in lazr.restfulclient), which shows up in the User-Agent but has no effect on any of the OAuth stuff.

I also removed some code left over from Benji's keyring branch: we were still creating a 'credentials' directory inside .launchpadlib, even though we don't store credentials in that directory anymore.

[Aaron Bentley (abentley)]

Looks good. I think there's an unnecessary blank line at 287.

review: Approve

=== modified file 'src/launchpadlib/NEWS.txt'
--- src/launchpadlib/NEWS.txt	2010-11-01 20:02:03 +0000
+++ src/launchpadlib/NEWS.txt	2010-11-01 20:02:03 +0000
@@ -9,6 +9,10 @@
   available.  The credentials_file parameter of Launchpad.login_with() is now
   ignored.
 
+- By default, Launchpad.login_with() now asks Launchpad for
+  desktop-wide integration. This removes the need for each individual
+  application to get its own OAuth token.
+
 1.7.0 (2010-09-23)
 ==================
 

=== modified file 'src/launchpadlib/credentials.py'
--- src/launchpadlib/credentials.py	2010-10-26 17:23:35 +0000
+++ src/launchpadlib/credentials.py	2010-11-01 20:02:03 +0000
@@ -39,7 +39,11 @@
 
 from lazr.restfulclient.errors import HTTPError
 from lazr.restfulclient.authorize.oauth import (
-    AccessToken as _AccessToken, Consumer, OAuthAuthorizer)
+    AccessToken as _AccessToken,
+    Consumer,
+    OAuthAuthorizer,
+    SystemWideConsumer # Not used directly, just re-imported into here.
+    )
 
 from launchpadlib import uris
 

=== modified file 'src/launchpadlib/launchpad.py'
--- src/launchpadlib/launchpad.py	2010-11-01 20:02:03 +0000
+++ src/launchpadlib/launchpad.py	2010-11-01 20:02:03 +0000
@@ -30,10 +30,19 @@
 import keyring
 from lazr.uri import URI
 from lazr.restfulclient.resource import (
-    CollectionWithKeyBasedLookup, HostedFile, ScalarValue, ServiceRoot)
+    CollectionWithKeyBasedLookup,
+    HostedFile,
+    ScalarValue,
+    ServiceRoot,
+    )
 from launchpadlib.credentials import (
-    AccessToken, AnonymousAccessToken, Credentials,
-    AuthorizeRequestTokenWithBrowser)
+    AccessToken,
+    AnonymousAccessToken,
+    AuthorizeRequestTokenWithBrowser,
+    Consumer,
+    Credentials,
+    SystemWideConsumer,
+    )
 from launchpadlib import uris
 
 
@@ -167,8 +176,7 @@
         name, the access token and the access secret) are available, this
         method can be used to quickly log into the service root.
 
-        :param consumer_name: the consumer name, as appropriate for the
-            `Consumer` constructor
+        :param consumer_name: the application name.
         :type consumer_name: string
         :param token_string: the access token, as appropriate for the
             `AccessToken` constructor
@@ -209,15 +217,22 @@
         a web browser and ask the user to come back here and tell us when they
         finished the authorization process.
 
-        :param consumer_name: The consumer name, as appropriate for the
-            `Consumer` constructor
+        :param consumer_name: Either a consumer name, as appropriate for
+            the `Consumer` constructor, or a premade Consumer object.
         :type consumer_name: string
         :param service_root: The URL to the root of the web service.
         :type service_root: string
         :return: The web service root
         :rtype: `Launchpad`
         """
-        credentials = Credentials(consumer_name)
+        if isinstance(consumer_name, Consumer):
+            # Create the authorizer with no Consumer, then set the Consumer
+            # object directly.
+            credentials = Credentials(None)
+            credentials.consumer = consumer_name
+        else:
+            # Have the authorizer create the Consumer itself.
+            credentials = Credentials(consumer_name_or_consumer)
         service_root = uris.lookup_service_root(service_root)
         web_root_uri = URI(service_root)
         web_root_uri.path = ""
@@ -250,13 +265,13 @@
                    timeout=timeout, proxy_info=proxy_info, version=version)
 
     @classmethod
-    def login_with(cls, consumer_name,
+    def login_with(cls, application_name,
                    service_root=uris.STAGING_SERVICE_ROOT,
                    launchpadlib_dir=None, timeout=None, proxy_info=None,
                    authorizer_class=AuthorizeRequestTokenWithBrowser,
                    allow_access_levels=[], max_failed_attempts=3,
                    credentials_file=None, version=DEFAULT_VERSION,
-                   credential_save_failed=None):
+                   consumer_name=None, credential_save_failed=None):
         """Log in to Launchpad with possibly cached credentials.
 
         This is a convenience method for either setting up new login
@@ -273,7 +288,13 @@
         See `Launchpad.get_token_and_login()` for more information about
         how new tokens are generated.
 
-        `credential_save_failed` is a callback that is called if saving the
+        :param application_name: The application name. This is *not*
+            the OAuth consumer name. Unless a consumer_name is also
+            provided, the OAuth consumer will be a system-wide
+            consumer representing the end-user's computer as a whole.
+        :type application_name: string
+
+        :param credential_save_failed: a callback that is called if saving the
         credentials in the auto-detected back-end (keyring or file) failed.
 
         :param consumer_name: The consumer name, as appropriate for the
@@ -285,27 +306,55 @@
         :param launchpadlib_dir: The directory where the cache and
             credentials are stored.
         :type launchpadlib_dir: string
+        :param allow_access_levels: The acceptable access levels for
+            this application. This is ignored unless you specify a
+            consumer_name. All applications using the default
+            (desktop-wide) consumer name will ask for "desktop integration"
+            access, which gives read-write access to public and private data.
+        :type allow_access_levels: list of strings
         :param credentials_file: ignored, only here for backward compatability
         :type credentials_file: string
+
+        :param consumer_name: The OAuth consumer name. In most cases,
+            this is not necessary. You should only use this if you
+            don't want to take advantage of desktop-wide integration.
+        :type consumer_name: string
+
         :return: The web service root
         :rtype: `Launchpad`
 
         """
+        if consumer_name is None:
+            # System-wide integration. Create a system-wide consumer
+            # and identify the application using a separate
+            # application name.
+            allow_access_levels = ["DESKTOP_INTEGRATION"]
+            consumer = SystemWideConsumer(application_name)
+            consumer_name = consumer.key
+        else:
+            # Application-specific integration. Use the provided
+            # consumer name to create a consumer automatically.
+            consumer = consumer_name
+
         (service_root, launchpadlib_dir, cache_path,
          service_root_dir) = cls._get_paths(service_root, launchpadlib_dir)
-        credentials_path = os.path.join(service_root_dir, 'credentials')
-
-        if not os.path.exists(credentials_path):
-            os.makedirs(credentials_path, 0700)
 
         credentials = keyring.get_password(
             'launchpadlib', consumer_name + '@' + service_root)
         if credentials is not None:
             credentials = unserialize_credentials(credentials)
 
+        if credentials is not None:
+            # We loaded credentials from a file, but the application
+            # name wasn't stored in the file, because 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 = application_name
+
         if credentials is None:
             launchpad = cls.get_token_and_login(
-                consumer_name, service_root=service_root, cache=cache_path,
+                consumer, service_root=service_root, cache=cache_path,
                 timeout=timeout, proxy_info=proxy_info,
                 authorizer_class=authorizer_class,
                 allow_access_levels=allow_access_levels,

=== modified file 'src/launchpadlib/tests/test_launchpad.py'
--- src/launchpadlib/tests/test_launchpad.py	2010-11-01 20:02:03 +0000
+++ src/launchpadlib/tests/test_launchpad.py	2010-11-01 20:02:03 +0000
@@ -33,6 +33,7 @@
     AccessToken,
     AuthorizeRequestTokenWithBrowser,
     Credentials,
+    SystemWideConsumer,
     )
 from launchpadlib.launchpad import Launchpad
 from launchpadlib import uris
@@ -55,14 +56,16 @@
         self.passed_in_kwargs = kw
 
     @classmethod
-    def get_token_and_login(cls, consumer_name, **kw):
+    def get_token_and_login(cls, consumer, **kw):
         """Create fake credentials and record that we were called."""
         credentials = Credentials(
-            consumer_name, consumer_secret='consumer_secret:42',
-            access_token=AccessToken('access_key:84', 'access_secret:168'))
+            consumer.key, consumer_secret='consumer_secret:42',
+            access_token=AccessToken('access_key:84', 'access_secret:168'),
+            application_name=consumer.application_name)
         launchpad = cls(credentials, **kw)
+
         launchpad.get_token_and_login_called = True
-        launchpad.consumer_name = consumer_name
+        launchpad.consumer = consumer
         launchpad.passed_in_kwargs = kw
         return launchpad
 
@@ -196,12 +199,15 @@
         # A directory for the passed in service root was created.
         service_path = os.path.join(launchpadlib_dir, 'api.example.com')
         self.assertTrue(os.path.isdir(service_path))
-        # Inside the service root directory, there is a 'cache' and a
-        # 'credentials' directory.
+        # Inside the service root directory, there is a 'cache'
+        # directory.
         self.assertTrue(
             os.path.isdir(os.path.join(service_path, 'cache')))
+
+        # In older versions there was also a 'credentials' directory,
+        # but no longer.
         credentials_path = os.path.join(service_path, 'credentials')
-        self.assertTrue(os.path.isdir(credentials_path))
+        self.assertFalse(os.path.isdir(credentials_path))
 
     def test_dirs_created_are_changed_to_secure(self):
         launchpadlib_dir = os.path.join(self.temp_dir, 'launchpadlib')
@@ -250,6 +256,29 @@
             launchpadlib_dir=launchpadlib_dir, version="bar")
         self.assertEquals(launchpad.passed_in_kwargs['version'], 'bar')
 
+    def test_application_name_is_propagated(self):
+        # Create a Launchpad instance for a given application name.
+        # Credentials are stored, but they don't include the
+        # application name, since multiple applications may share a
+        # single system-wide credential.
+        launchpadlib_dir = os.path.join(self.temp_dir, 'launchpadlib')
+        launchpad = NoNetworkLaunchpad.login_with(
+            'very important', service_root='http://api.example.com/',
+            launchpadlib_dir=launchpadlib_dir)
+        self.assertEquals(
+            launchpad.credentials.consumer.application_name, 'very important')
+
+        # Now execute the same test a second time. This time, the
+        # credentials are loaded from disk and a different code path
+        # is executed. We want to make sure this code path propagates
+        # the application name, instead of picking an empty one from
+        # disk.
+        launchpad = NoNetworkLaunchpad.login_with(
+            'very important', service_root='http://api.example.com/',
+            launchpadlib_dir=launchpadlib_dir)
+        self.assertEquals(
+            launchpad.credentials.consumer.application_name, 'very important')
+
     def test_no_credentials_calls_get_token_and_login(self):
         # If no credentials are found, get_token_and_login() is called.
         service_root = 'http://api.example.com/'
@@ -258,9 +287,9 @@
         launchpad = NoNetworkLaunchpad.login_with(
             'app name', launchpadlib_dir=self.temp_dir,
             service_root=service_root, timeout=timeout, proxy_info=proxy_info)
-        self.assertEqual(launchpad.consumer_name, 'app name')
+        self.assertEqual(launchpad.consumer.application_name, 'app name')
         expected_arguments = dict(
-            allow_access_levels=[],
+            allow_access_levels=['DESKTOP_INTEGRATION'],
             authorizer_class=AuthorizeRequestTokenWithBrowser,
             max_failed_attempts=3,
             service_root=service_root,
@@ -331,8 +360,7 @@
             launchpadlib_dir, os.path.join(self.temp_dir, '.launchpadlib'))
         self.assertTrue(os.path.exists(
             os.path.join(launchpadlib_dir, 'api.example.com', 'cache')))
-        self.assertTrue(os.path.exists(
-            os.path.join(launchpadlib_dir, 'api.example.com', 'credentials')))
+
 
     def test_short_service_name(self):
         # A short service name is converted to the full service root URL.
@@ -435,11 +463,12 @@
         launchpadlib_dir = os.path.join(self.temp_dir, 'launchpadlib')
         keyring = InMemoryKeyring()
         service_root = 'http://api.example.com/'
-        consumer_name = 'Super App 3000'
+        application_name = 'Super App 3000'
         with fake_keyring(keyring):
-            NoNetworkLaunchpad.login_with(
-                consumer_name, service_root=service_root,
+            launchpad = NoNetworkLaunchpad.login_with(
+                application_name, service_root=service_root,
                 launchpadlib_dir=launchpadlib_dir)
+            consumer_name = launchpad.credentials.consumer.key
 
         application_key = keyring.data.keys()[0][1]