Merge lp:~leonardr/launchpadlib/retry-on-invalid-token into lp:launchpadlib

[Leonard Richardson]

This branch moves almost all of the code having to do with OAuth tokens--getting the request token, exchanging it for an access token, storing it in the keyring and retrieving it from the keyring--into the RequestTokenAuthorizationEngine. Previously that class only the code to get the end-user to authorize the request token.

Why did I do this? Because of bug 670886. Up to this point, we've been assuming that once you exchange a request token for an access token, or retrieve an access token from local store, you can make HTTP requests to the web service and never have to think about tokens anymore. But that's not true. Since you last used that cached token, it might have expired, or you might have cancelled it. Worse, it might expire thirty minutes into your launchpadlib session. When that happens, launchpadlib simply crashes.

To fix this bug, I had to take the mechanism used to get new access tokens and make it portable, Make it so all the information about an access token--the application name, the allowable access levels, whether to store the access token in the keyring or somewhere else, whether to authorize the request token using a web browser open or some other mechanism--was all kept in a single object, which could be stored with the Launchpad instance and brought out of retirement if Launchpad ever found itself using an expired or invalid token.

The main non-refactoring code is the new LaunchpadOAuthAwareRestfulHttp class, which detects certain kinds of 401 failures from Launchpad and takes the opportunity to obtain a new access token. I talked with developer Martin Owens about this and he's fine with this behavior so long as he has the opportunity to plug in his own authorization engine.

I got all the existing tests to work and added several new tests to clarify the behavior of Launchpad.login_with(), which ignores different arguments in different circumstances. For instance, consumer_name overrides application_name, and authorization_engine overrides both of them plus credential_save_failed and allow_access_levels.

Problems with the branch, which I'm working on:

1. I haven't checked that the launchpadlib integration tests within Launchpad still work.

2. I don't have a test for the new behavior itself. We can't test Launchpad's HTTP behavior in general--that has to be done manually or within Launchpad--but I'm thinking of writing a fake Http subclass that just gives out 401 errors. That should be enough to write a test.

3. The code that checks the 401 is a little brittle: it looks at the entity-body to distinguish between a 401 caused by a bad token and a 401 caused by accessing data you don't have access to. We might decide to change or localize this one day. I'd like to do a Launchpad branch that puts this information in an X- HTTP header, and do a follow-up launchpadlib branch that looks at the HTTP header instead of the entity-body.

(I'm not sure whether "private data" gives you a 401 or 403. If that's a 403, it should make sense to get a new token every time a signed request yields a 401. That would simplify the code.)

Since this is a rather large branch I'm happy to write a guide to the code--just let me know.

[Benji York (benji)]

I've read through the entire diff and feel like I have a solid
understanding of the changes being made. The architectural
transformation not only accomplishes the desired effect, but makes the
responsibilities of the code better organized.

I have a few questions about particular bits of the code below. I have
run out of time today so I'm posting what I have and will finish first
thing tomorrow.

src/launchpadlib/credentials.py line 29:

    I was surprised to see "import" and "from...import" lines intermixed (I
    expected to see "from" first and then "import"). I don't see anything in
    the style guide about them though, so this may be the preferred
    style.

src/launchpadlib/credentials.py line 215:

    The RequestTokenAuthorizationEngine interface has changed in what appear
    to be backward-incompatible ways. Since this class is exported in
    __all__, I assume it is a public interface that others may depend on. Is
    this the case?

    The same goes for AuthorizeRequestTokenWithBrowser and other subclasses.

src/launchpadlib/credentials.py line 231:

    Mutable default arguments ("allow_access_levels=[]") are a bug magnet. I
    suggest using an empty tuple instead.

src/launchpadlib/credentials.py line 231:

    Is there a reason why some of the __init__ parameters are documented but
    others (like consumer_name) are not?

src/launchpadlib/credentials.py line 257:

    Should it be an error to provide a value for allow_access_levels and not a
    value for consumer_name because that will result in the caller-provided
    allow_access_levels value to be discarded.

src/launchpadlib/credentials.py line 264:

    Similar to the above, should it be an error to provide a value for
    application_name and value for consumer_name because that will result in
    the caller-provided application_name value to be discarded.

src/launchpadlib/credentials.py line 259:

    The "consumer_name = consumer.key" line seems to be dead code, the value
    of consumer_name isn't used after that line.

src/launchpadlib/credentials.py line 343:

    Minor: The new store_credentials_locally() and
    retrieve_credentials_from_local_store() method names might could be more
    symmetric. Perhaps "store_credentials_to_local_store" (I have to
    admit that I don't like that name much either).

src/launchpadlib/credentials.py line 389:

    It isn't part of your changes, but the WAITING_FOR_USER definition is one
    very long line.

[Benji York (benji)]

Here's the rest of my review:

src/launchpadlib/launchpad.py line 156:

    I know people most often use .login_with() to create instances, do they
    ever use the constructor?

    If so and they are passing service_root as a positional argument, then
    insertion of the authorization_engine parameter before service_root will
    cause their code to generate an error.

src/launchpadlib/launchpad.py line 204:

    allow_access_levels has a mutable default.

src/launchpadlib/launchpad.py line 204:

    Since max_failed_attempts is ignored, maybe None would be a better default
    value.

src/launchpadlib/launchpad.py line 374:

    This note on the allow_access_levels parameter

        This argument is used to construct the default `authorization_engine`,
        so if you pass in your own `authorization_engine` any value for this
        argument will be ignored. This argument will also be ignored unless
        you also specify `consumer_name`.

    ...is sufficiently complex that I think programmatic enforcement of the
    given rules is warranted. Unfortunately for backward compatibility
    reasons I suspect we con only enforce the first rule (passing
    authorization_engine makes allow_access_levels irrelevant).

src/launchpadlib/launchpad.py line 375:

    A line over 78 characters wide.

src/launchpadlib/launchpad.py line 396:

    A line over 78 characters wide.

None of these are major, and I'm sure they'll all be addressed forthwith (even if some don't spur changes), so approving.

Since I'm doing mentored reviews, Edwin will have to review my review before final approval. I'll let him know to look at this review once you're response is in.

review: Approve (code*)

[lp:~leonardr/launchpadlib/retry-on-invalid-token]

114. By Leonard Richardson on 2010-12-16

Added tests that require simulating HTTP responses.

115. By Leonard Richardson on 2010-12-16

Basic tests of the mock-HTTP-response code itself.

116. By Leonard Richardson on 2010-12-16

Track how many access tokens were obtained, not whether some have been obtained.

[Leonard Richardson (leonardr)]

Here's an incremental diff: http://pastebin.ubuntu.com/544534/

I've written a framework that lets me send fake HTTP responses to launchpadlib, allowing me to finally do a (sort of) end-to-end test of login_with(), as well as demonstrate what happens if Launchpad suddenly starts sending 401 responses to launchpadlib's requests. I had to do a tiny bit of refactoring of the LaunchpadOauthAwareHttp._request method, and I moved several helper classes from test_launchpad into testing/helpers.py (because they're now used by more than one test module), but almost all of this code is new tests.

[Benji York (benji)]

On Thu, Dec 16, 2010 at 1:17 PM, Leonard Richardson
<email address hidden> wrote:
> Here's an incremental diff: http://pastebin.ubuntu.com/544534/

I don't see any problems introduced in the diff.
--
Benji York

[lp:~leonardr/launchpadlib/retry-on-invalid-token]

117. By Leonard Richardson on 2010-12-16

Response to feedback, focusing on docstrings and default arguments.

118. By Leonard Richardson on 2010-12-16

Added checks for incompatible information passed into login_with. Made application_name optional, but started enforcing the precense of *some* way of identifying the application.

119. By Leonard Richardson on 2010-12-16

Application name and consumer name are not the same thing, so don't allow the creation of an authorization engine that specifies the same value for both.

120. By Leonard Richardson on 2010-12-16

Minor cleanup.

121. By Leonard Richardson on 2010-12-20

Merge with trunk.

[Edwin Grubbs (edwin-grubbs)]

Hi Leonard,

This is a nice branch, and I think benji did an excellent review,
however, since the branch is so big, I found a few more items to improve.

-Edwin

>=== modified file 'src/launchpadlib/launchpad.py'
>--- src/launchpadlib/launchpad.py 2010-11-01 19:48:27 +0000
>+++ src/launchpadlib/launchpad.py 2010-12-20 15:26:37 +0000
>@@ -124,6 +107,40 @@
> collection_of = 'distribution'
>
>
>+class LaunchpadOAuthAwareHttp(RestfulHttp):
>+ """Detects expired/invalid OAuth tokens and tries to get a new token."""
>+
>+ def __init__(self, launchpad, authorization_engine, *args):
>+ self.launchpad = launchpad
>+ self.authorization_engine = authorization_engine
>+ super(LaunchpadOAuthAwareHttp, self).__init__(*args)
>+
>+ def _bad_oauth_token(self, response, content):
>+ """Helper method to detect an error caused by a bad OAuth token."""
>+ return (response.status == 401 and
>+ (content.startswith("Expired token")
>+ or content.startswith("Invalid token")))
>+
>+ def _request(self, *args):
>+ response, content = super(
>+ LaunchpadOAuthAwareHttp, self)._request(*args)
>+ return self.retry_on_bad_token(response, content, *args)

If there is a bug that causes continual 401 errors, it will
eventually reach the max recursion depth and give a ginormous traceback.
It seems like each request should only need to be retried once or twice
before raising an exception.

>+
>+ def retry_on_bad_token(self, response, content, *args):
>+ """If the response indicates a bad token, get a new token and retry.
>+
>+ Otherwise, just return the response.
>+ """
>+ if (self._bad_oauth_token(response, content)
>+ and self.authorization_engine is not None):
>+ # This access token is bad. Scrap it and create a new one.
>+ self.launchpad.credentials.access_token = None
>+ self.authorization_engine(self.launchpad.credentials)
>+ # Retry the request with the new credentials.
>+ return self._request(*args)
>+ return response, content
>+
>+
> class Launchpad(ServiceRoot):
> """Root Launchpad API class.
>
>=== added file 'src/launchpadlib/tests/test_http.py'
>--- src/launchpadlib/tests/test_http.py 1970-01-01 00:00:00 +0000
>+++ src/launchpadlib/tests/test_http.py 2010-12-20 15:26:37 +0000
>+class TestAbilityToParseData(SimulatedResponsesTestCase):
>+ """Test launchpadlib's ability to handle the sample data.
>+
>+ To create a Launchpad object, two HTTP requests must succeed and
>+ return usable data: the requests for the WADL and JSON
>+ representations of the service root. This test shows that the
>+ minimal data in SIMPLE_WADL and SIMPLE_JSON is good enough to
>+ create a Launchpad object.
>+ """
>+
>+ def test_minimal_data(self):
>+ """Make sure that launchpadlib can use the minimal data."""
>+ launchpad = self.launchpad_with_responses(
>+ Response(200, SIMPLE_WADL),
>+ Response(200, SIMPLE_JSON))
>+
>+ def test_bad_data(self):
>+ """Show that bad WADL causes an exception."""
>+ sel...

review: Approve (code)

[lp:~leonardr/launchpadlib/retry-on-invalid-token]

122. By Leonard Richardson on 2010-12-20

Split out tests at Edwin's request.

[Leonard Richardson (leonardr)]

> >+ def _request(self, *args):
> >+ response, content = super(
> >+ LaunchpadOAuthAwareHttp, self)._request(*args)
> >+ return self.retry_on_bad_token(response, content, *args)
>
>
>
> If there is a bug that causes continual 401 errors, it will
> eventually reach the max recursion depth and give a ginormous traceback.
> It seems like each request should only need to be retried once or twice
> before raising an exception.

Long before this happens, the user will get tired of browser opens and hit Ctrl-C. I looked into making this change, but I couldn't find a way to do it since I can't really add arguments to _request (it's an httplib2 method). Given the low impact of the change, I'm going to leave it out. I've made the other changes you suggest.

=== modified file 'src/launchpadlib/NEWS.txt'
--- src/launchpadlib/NEWS.txt	2010-12-02 14:58:42 +0000
+++ src/launchpadlib/NEWS.txt	2010-12-20 17:49:10 +0000
@@ -2,9 +2,13 @@
 NEWS for launchpadlib
 =====================
 
-1.8.1 (unreleased)
+1.9.0 (Unreleased)
 ==================
 
+- When an authorization token expires or becomes invalid, attempt to
+  acquire a new one, even in the middle of a session, rather than
+  crashing.
+
 - The HTML generated by wadl-to-refhtml.xsl now validates.
 
 1.8.0 (2010-11-15)

=== modified file 'src/launchpadlib/credentials.py'
--- src/launchpadlib/credentials.py	2010-10-28 20:48:12 +0000
+++ src/launchpadlib/credentials.py	2010-12-20 17:49:10 +0000
@@ -20,6 +20,7 @@
 __all__ = [
     'AccessToken',
     'AnonymousAccessToken',
+    'AuthorizeRequestTokenWithBrowser',
     'RequestTokenAuthorizationEngine',
     'Consumer',
     'Credentials',
@@ -27,6 +28,7 @@
 
 import base64
 import cgi
+from cStringIO import StringIO
 import httplib2
 import sys
 import textwrap
@@ -35,6 +37,7 @@
 from urlparse import urljoin
 import webbrowser
 
+import keyring
 import simplejson
 
 from lazr.restfulclient.errors import HTTPError
@@ -52,6 +55,8 @@
 authorize_token_page = '+authorize-token'
 access_token_poll_time = 1
 
+EXPLOSIVE_ERRORS = (MemoryError, KeyboardInterrupt, SystemExit)
+
 
 class Credentials(OAuthAuthorizer):
     """Standard credentials storage and usage class.
@@ -66,6 +71,25 @@
     URI_TOKEN_FORMAT = "uri"
     DICT_TOKEN_FORMAT = "dict"
 
+    def serialize(self):
+        """Turn this object into a string.
+
+        This should probably be moved into OAuthAuthorizer.
+        """
+        sio = StringIO()
+        self.save(sio)
+        return sio.getvalue()
+
+    @classmethod
+    def from_string(cls, value):
+        """Create a `Credentials` object from a serialized string.
+
+        This should probably be moved into OAuthAuthorizer.
+        """
+        credentials = cls()
+        credentials.load(StringIO(value))
+        return credentials
+
     def get_request_token(self, context=None, web_root=uris.STAGING_WEB_ROOT,
                           token_format=URI_TOKEN_FORMAT):
         """Request an OAuth token to Launchpad.
@@ -189,45 +213,224 @@
 
 
 class RequestTokenAuthorizationEngine(object):
+    """The superclass of all request token authorizers.
+
+    This base class does not implement request token authorization,
+    since that varies depending on how you want the end-user to
+    authorize a request token. You'll need to subclass this class and
+    implement `make_end_user_authorize_token`.
+
+    This class does implement a default strategy for storing
+    authorized tokens in the GNOME keyring or KDE Wallet, but you can
+    override this by implementing `store_credentials_locally` and
+    `retrieve_credentials_from_local_store`.
+    """
 
     UNAUTHORIZED_ACCESS_LEVEL = "UNAUTHORIZED"
 
-    def __init__(self, web_root, consumer_name, request_token,
-                 allow_access_levels=[]):
-        self.web_root = uris.lookup_web_root(web_root)
-        self.consumer_name = consumer_name
-        self.request_token = request_token
-        self.allow_access_levels = allow_access_levels
-
-    def __call__(self):
+    def __init__(self, service_root, application_name=None,
+                 consumer_name=None, credential_save_failed=None,
+                 allow_access_levels=None):
+        """Base class initialization.
+
+        :param service_root: The root of the Launchpad instance being
+            used.
+
+        :param application_name: The name of the application that
+            wants to use launchpadlib. This is used in conjunction
+            with a desktop-wide integration.
+
+            If you specify this argument, your values for
+            consumer_name and allow_access_levels are ignored.
+
+        :param consumer_name: The OAuth consumer name, for an
+            application that wants its own point of integration into
+            Launchpad. In almost all cases, you want to specify
+            application_name instead and do a desktop-wide
+            integration. The exception is when you're integrating a
+            third-party website into Launchpad.
+
+        :param credential_save_failed: A callback method to be invoked
+            if the credentials cannot be stored locally.
+
+            You should not invoke this method yourself; instead, you
+            should raise an exception in `store_credentials_locally()`.
+
+        :param allow_access_levels: A list of the Launchpad access
+            levels to present to the user. ('READ_PUBLIC' and so on.)
+            Your value for this argument will be ignored during a
+            desktop-wide integration.
+        :type allow_access_levels: A list of strings.
+        """
+        self.service_root = uris.lookup_service_root(service_root)
+        self.web_root = uris.web_root_for_service_root(service_root)
+
+        if application_name is None and consumer_name is None:
+            raise ValueError(
+                "You must provide either application_name or consumer_name.")
+
+        if application_name is not None and consumer_name is not None:
+            raise ValueError(
+                "You must provide only one of application_name and "
+                "consumer_name.")
+
+        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)
+        else:
+            # Application-specific integration. Use the provided
+            # consumer name to create a consumer automatically.
+            consumer = Consumer(consumer_name)
+            application_name = consumer_name
+
+        self.consumer = consumer
+        self.application_name = application_name
+
+        self.allow_access_levels = allow_access_levels or []
+        self.credential_save_failed = credential_save_failed
+
+    def authorization_url(self, request_token):
+        """Return the authorization URL for a request token.
+
+        This is the URL the end-user must visit to authorize the
+        token. How exactly does this happen? That depends on the
+        subclass implementation.
+        """
+        page = "%s?oauth_token=%s" % (authorize_token_page, request_token)
+        allow_permission = "&allow_permission="
+        if len(self.allow_access_levels) > 0:
+            page += (
+                allow_permission
+                + allow_permission.join(self.allow_access_levels))
+        return urljoin(self.web_root, page)
+
+    def __call__(self, credentials):
+        """Authorize a token and associate it with the given credentials.
+
+        The `credential_save_failed` callback will be invoked if
+        there's a problem storing the credentials locally. It will not
+        be invoked if there's a problem authorizing the credentials.
+
+        :param credentials: A `Credentials` object. If the end-user
+            authorizes these credentials, this object will have its
+            .access_token property set.
+
+        :return: If the credentials are successfully authorized, the
+            return value is the `Credentials` object originally passed
+            in. Otherwise the return value is None.
+        """
+        request_token_string = self.get_request_token(credentials)
+        # Hand off control to the end-user.
+        self.make_end_user_authorize_token(credentials, request_token_string)
+        if credentials.access_token is None:
+            # The end-user refused to authorize the application.
+            return None
+        try:
+            self.store_credentials_locally(credentials)
+        except EXPLOSIVE_ERRORS:
+            raise
+        except:
+            if self.credential_save_failed is not None:
+                self.credential_save_failed()
+        return credentials
+
+    def get_request_token(self, credentials):
+        """Get a new request token from the server.
+
+        :param return: The request token.
+        """
+        authorization_json = credentials.get_request_token(
+            web_root=self.web_root,
+            token_format=Credentials.DICT_TOKEN_FORMAT)
+        return authorization_json['oauth_token']
+
+    def make_end_user_authorize_token(self, credentials, request_token):
+        """Authorize the given request token using the given credentials.
+
+        Your subclass must implement this method: it has no default
+        implementation.
+
+        Because an access token may expire or be revoked in the middle
+        of a session, this method may be called at arbitrary points in
+        a launchpadlib session, or even multiple times during a single
+        session (with a different request token each time).
+
+        In most cases, however, this method will be called at the
+        beginning of a launchpadlib session, or not at all.
+        """
         raise NotImplementedError()
 
+    def store_credentials_locally(self, credentials):
+        """Store newly-authorized credentials for later use.
+
+        By default, credentials are stored in the GNOME keyring or KDE
+        Wallet. This is a good solution for desktop applications and
+        local scripts, but if you are integrating a third-party
+        website into Launchpad you'll need to implement something else
+        (such as storing the credentials in a database).
+        """
+        keyring.set_password(
+            'launchpadlib',
+            (credentials.consumer.key + '@' + self.service_root),
+            credentials.serialize())
+
+    def retrieve_credentials_from_local_store(self):
+        """Retrieve credentials from a local store.
+
+        This method is the inverse of `store_credentials_locally`. The
+        default implementation looks for credentials stored in the
+        GNOME keyring or KDE Wallet.
+
+        :return: A `Credentials` object if one is found in the local
+            store, and None otherise.
+        """
+        credential_string = keyring.get_password(
+            'launchpadlib', self.consumer.key + '@' + self.service_root)
+        if credential_string is not None:
+            credentials = Credentials.from_string(credential_string)
+            # 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 = self.application_name
+            return credentials
+        return None
+
 
 class AuthorizeRequestTokenWithBrowser(RequestTokenAuthorizationEngine):
-    """The simplest (and, right now, only) request token authorizer.
+    """The simplest (and, right now, the only) request token authorizer.
 
     This authorizer simply opens up the end-user's web browser to a
     Launchpad URL and lets the end-user authorize the request token
     themselves.
-
-    :param max_failed_attempts: Unused by this token authorization
-    mechanism. Any value passed in will be ignored.
     """
 
     WAITING_FOR_USER = "The authorization page:\n (%s)\nshould be opening in your browser. Use your browser to authorize\nthis program to access Launchpad on your behalf. \n\nWaiting to hear from Launchpad about your decision..."
 
-    def __init__(self, web_root, consumer_name, request_token,
-                 allow_access_levels=[], max_failed_attempts=None):
-        web_root = uris.lookup_web_root(web_root)
-        page = "+authorize-token?oauth_token=%s" % request_token
-        if len(allow_access_levels) > 0:
-            page += ("&allow_permission=" +
-                     "&allow_permission=".join(allow_access_levels))
-        self.authorization_url = urljoin(web_root, page)
+    def __init__(self, service_root, application_name, consumer_name=None,
+                 credential_save_failed=None, allow_access_levels=None):
+        """Constructor.
 
+        :param service_root: See `RequestTokenAuthorizationEngine`.
+        :param application_name: See `RequestTokenAuthorizationEngine`.
+        :param consumer_name: The value of this argument is
+            ignored. If we have the capability to open the end-user's
+            web browser, we must be running on the end-user's computer,
+            so we should do a full desktop integration.
+        :param credential_save_failed: See `RequestTokenAuthorizationEngine`.
+        :param allow_access_levels: The value of this argument is
+            ignored, for the same reason as consumer_name.
+        """
+        # It doesn't look like we're doing anything here, but we
+        # are discarding the passed-in values for consumer_name and
+        # allow_access_levels.
         super(AuthorizeRequestTokenWithBrowser, self).__init__(
-            web_root, consumer_name, request_token,
-            allow_access_levels)
+              service_root, application_name, None,
+              credential_save_failed)
 
     def output(self, message):
         """Display a message.
@@ -238,13 +441,17 @@
         """
         print message
 
-    def __call__(self, credentials, web_root):
-        webbrowser.open(self.authorization_url)
-        self.output(self.WAITING_FOR_USER % self.authorization_url)
+    def make_end_user_authorize_token(self, credentials, request_token):
+        """Have the end-user authorize the token in their browser."""
+
+        authorization_url = self.authorization_url(request_token)
+        webbrowser.open(authorization_url)
+        self.output(self.WAITING_FOR_USER % authorization_url)
         while credentials.access_token is None:
             time.sleep(access_token_poll_time)
             try:
-                credentials.exchange_request_token_for_access_token(web_root)
+                credentials.exchange_request_token_for_access_token(
+                    self.web_root)
                 break
             except HTTPError, e:
                 if e.response.status == 403:
@@ -259,6 +466,7 @@
                     print "Unexpected response from Launchpad:"
                     print e
 
+
 class TokenAuthorizationException(Exception):
     pass
 

=== modified file 'src/launchpadlib/launchpad.py'
--- src/launchpadlib/launchpad.py	2010-11-01 19:48:27 +0000
+++ src/launchpadlib/launchpad.py	2010-12-20 17:49:10 +0000
@@ -25,16 +25,14 @@
 import socket
 import stat
 import urlparse
-from cStringIO import StringIO
 
-import keyring
-from lazr.uri import URI
 from lazr.restfulclient.resource import (
     CollectionWithKeyBasedLookup,
     HostedFile,
     ScalarValue,
     ServiceRoot,
     )
+from lazr.restfulclient._browser import RestfulHttp
 from launchpadlib.credentials import (
     AccessToken,
     AnonymousAccessToken,
@@ -52,21 +50,6 @@
 from launchpadlib.uris import EDGE_SERVICE_ROOT, STAGING_SERVICE_ROOT
 OAUTH_REALM = 'https://api.launchpad.net'
 
-EXPLOSIVE_ERRORS = (MemoryError, KeyboardInterrupt, SystemExit)
-
-def serialize_credentials(credentials):
-    # Get the credentials as a string.
-    sio = StringIO()
-    credentials.save(sio)
-    return sio.getvalue()
-
-
-def unserialize_credentials(value):
-    # Get the credentials as a string.
-    credentials = Credentials()
-    credentials.load(StringIO(value))
-    return credentials
-
 
 class PersonSet(CollectionWithKeyBasedLookup):
     """A custom subclass capable of person lookup by username."""
@@ -124,6 +107,40 @@
     collection_of = 'distribution'
 
 
+class LaunchpadOAuthAwareHttp(RestfulHttp):
+    """Detects expired/invalid OAuth tokens and tries to get a new token."""
+
+    def __init__(self, launchpad, authorization_engine, *args):
+        self.launchpad = launchpad
+        self.authorization_engine = authorization_engine
+        super(LaunchpadOAuthAwareHttp, self).__init__(*args)
+
+    def _bad_oauth_token(self, response, content):
+        """Helper method to detect an error caused by a bad OAuth token."""
+        return (response.status == 401 and
+                (content.startswith("Expired token")
+                 or content.startswith("Invalid token")))
+
+    def _request(self, *args):
+        response, content = super(
+            LaunchpadOAuthAwareHttp, self)._request(*args)
+        return self.retry_on_bad_token(response, content, *args)
+
+    def retry_on_bad_token(self, response, content, *args):
+        """If the response indicates a bad token, get a new token and retry.
+
+        Otherwise, just return the response.
+        """
+        if (self._bad_oauth_token(response, content)
+            and self.authorization_engine is not None):
+            # This access token is bad. Scrap it and create a new one.
+            self.launchpad.credentials.access_token = None
+            self.authorization_engine(self.launchpad.credentials)
+            # Retry the request with the new credentials.
+            return self._request(*args)
+        return response, content
+
+
 class Launchpad(ServiceRoot):
     """Root Launchpad API class.
 
@@ -142,13 +159,20 @@
             }
     RESOURCE_TYPE_CLASSES.update(ServiceRoot.RESOURCE_TYPE_CLASSES)
 
-    def __init__(self, credentials, service_root=uris.STAGING_SERVICE_ROOT,
+    def __init__(self, credentials, authorization_engine,
+                 service_root=uris.STAGING_SERVICE_ROOT,
                  cache=None, timeout=None, proxy_info=None,
                  version=DEFAULT_VERSION):
         """Root access to the Launchpad API.
 
         :param credentials: The credentials used to access Launchpad.
         :type credentials: `Credentials`
+        :param authorization_engine: The object used to get end-user input
+            for authorizing OAuth request tokens. Used when an OAuth
+            access token expires or becomes invalid during a
+            session, or is discovered to be invalid once launchpadlib
+            starts up.
+        :type authorization_engine: `RequestTokenAuthorizationEngine`
         :param service_root: The URL to the root of the web service.
         :type service_root: string
         """
@@ -162,15 +186,31 @@
                      "the version name from the root URI." % version)
             raise ValueError(error)
 
+        # We already have an access token, but it might expire or
+        # become invalid during use. Store the authorization engine in
+        # case we need to authorize a new token during use.
+        self.authorization_engine = authorization_engine
+
         super(Launchpad, self).__init__(
             credentials, service_root, cache, timeout, proxy_info, version)
 
+    def httpFactory(self, credentials, cache, timeout, proxy_info):
+        return LaunchpadOAuthAwareHttp(
+            self, self.authorization_engine, credentials, cache, timeout,
+            proxy_info)
+
+    @classmethod
+    def authorization_engine_factory(cls, *args):
+        return AuthorizeRequestTokenWithBrowser(*args)
+
     @classmethod
     def login(cls, consumer_name, token_string, access_secret,
               service_root=uris.STAGING_SERVICE_ROOT,
               cache=None, timeout=None, proxy_info=None,
+              authorization_engine=None, allow_access_levels=None,
+              max_failed_attempts=None, credential_save_failed=None,
               version=DEFAULT_VERSION):
-        """Convenience for setting up access credentials.
+        """Convenience method for setting up access credentials.
 
         When all three pieces of credential information (the consumer
         name, the access token and the access secret) are available, this
@@ -186,70 +226,80 @@
         :type access_secret: string
         :param service_root: The URL to the root of the web service.
         :type service_root: string
+        :param authorization_engine: See `Launchpad.__init__`. If you don't
+            provide an authorization engine, a default engine will be
+            constructed using your values for `service_root` and
+            `credential_save_failed`.
+        :param allow_access_levels: This argument is ignored, and only
+            present to preserve backwards compatibility.
+        :param max_failed_attempts: This argument is ignored, and only
+            present to preserve backwards compatibility.
         :return: The web service root
         :rtype: `Launchpad`
         """
         access_token = AccessToken(token_string, access_secret)
         credentials = Credentials(
             consumer_name=consumer_name, access_token=access_token)
-        return cls(credentials, service_root, cache, timeout, proxy_info,
-                   version)
+        if authorization_engine is None:
+            authorization_engine = cls.authorization_engine_factory(
+                service_root, None, consumer_name, allow_access_levels,
+                credential_save_failed)
+        return cls(credentials, authorization_engine, service_root, cache,
+                   timeout, proxy_info, version)
 
     @classmethod
     def get_token_and_login(cls, consumer_name,
                             service_root=uris.STAGING_SERVICE_ROOT,
                             cache=None, timeout=None, proxy_info=None,
-                            authorizer_class=AuthorizeRequestTokenWithBrowser,
-                            allow_access_levels=[], max_failed_attempts=3,
+                            authorization_engine=None, allow_access_levels=[],
+                            max_failed_attempts=None,
+                            credential_save_failed=None,
                             version=DEFAULT_VERSION):
         """Get credentials from Launchpad and log into the service root.
 
-        This is a convenience method which will open up the user's preferred
-        web browser and thus should not be used by most applications.
-        Applications should, instead, use Credentials.get_request_token() to
-        obtain the authorization URL and
-        Credentials.exchange_request_token_for_access_token() to obtain the
-        actual OAuth access token.
-
-        This method will negotiate an OAuth access token with the service
-        provider, but to complete it we will need the user to log into
-        Launchpad and authorize us, so we'll open the authorization page in
-        a web browser and ask the user to come back here and tell us when they
-        finished the authorization process.
+        This method is deprecated as of launchpadlib version 1.9.0. A
+        launchpadlib application running on the end-user's computer
+        should use `Launchpad.login_with()`. A launchpadlib
+        application running on a web server, integrating Launchpad
+        into some other website, should use
+        `Credentials.get_request_token()` to obtain the authorization
+        URL and
+        `Credentials.exchange_request_token_for_access_token()` to
+        obtain the actual OAuth access token. Then you can call
+        `Launchpad.login()`.
 
         :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
+        :param authorization_engine: See `Launchpad.__init__`. If you don't
+            provide an authorization engine, a default engine will be
+            constructed using your values for `service_root` and
+            `credential_save_failed`.
+        :param allow_access_levels: This argument is ignored, and only
+            present to preserve backwards compatibility.
+        :param max_failed_attempts: This argument is ignored, and only
+            present to preserve backwards compatibility.
         :return: The web service root
         :rtype: `Launchpad`
         """
         if isinstance(consumer_name, Consumer):
-            # Create the authorizer with no Consumer, then set the Consumer
-            # object directly.
+            # Create the credentials with no Consumer, then set its .consumer
+            # property 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 = ""
-        web_root_uri.host = web_root_uri.host.replace("api.", "", 1)
-        web_root = str(web_root_uri.ensureSlash())
-        authorization_json = credentials.get_request_token(
-            web_root=web_root, token_format=Credentials.DICT_TOKEN_FORMAT)
-        authorizer = authorizer_class(
-            web_root, authorization_json['oauth_token_consumer'],
-            authorization_json['oauth_token'], allow_access_levels,
-            max_failed_attempts)
-        authorizer(credentials, web_root)
-        if credentials.access_token is None:
-            # The end-user refused to authorize the application.
-            return None
-        return cls(credentials, service_root, cache, timeout, proxy_info,
-                   version)
+            # Have the Credentials constructor create the Consumer
+            # automatically.
+            credentials = Credentials(consumer_name)
+        if authorization_engine is None:
+            authorization_engine = cls.authorization_engine_factory(
+                service_root, None, consumer_name, allow_access_levels,
+                credential_save_failed)
+        credentials = authorization_engine(credentials)
+        return cls(credentials, authorization_engine, service_root, cache,
+                   timeout, proxy_info, version)
 
     @classmethod
     def login_anonymously(
@@ -261,32 +311,34 @@
          service_root_dir) = cls._get_paths(service_root, launchpadlib_dir)
         token = AnonymousAccessToken()
         credentials = Credentials(consumer_name, access_token=token)
-        return cls(credentials, service_root=service_root, cache=cache_path,
-                   timeout=timeout, proxy_info=proxy_info, version=version)
+        return cls(credentials, None, service_root=service_root,
+                   cache=cache_path, timeout=timeout, proxy_info=proxy_info,
+                   version=version)
 
     @classmethod
-    def login_with(cls, application_name,
+    def login_with(cls, application_name=None,
                    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,
-                   consumer_name=None, credential_save_failed=None):
+                   authorization_engine=None, allow_access_levels=None,
+                   max_failed_attempts=None, credentials_file=None,
+                   version=DEFAULT_VERSION, 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
-        credentials, or re-using existing ones. When a login token is
-        generated using this method, the resulting credentials will be stored
-        in a system keyring if available or on disk if not.
-
-        Subsequent calls to this method will load the credentials from one of
-        the aformentioned locations in the same priority order.
-
-        `launchpadlib_dir` is also used for caching fetched objects. The cache
-        is per service root, and shared by all consumers.
-
-        See `Launchpad.get_token_and_login()` for more information about
-        how new tokens are generated.
+        Use this method to get a `Launchpad` object if you are writing
+        a desktop application or a script. If the end-user has no
+        cached Launchpad credential, their browser will open and
+        they'll be asked to log in and authorize a desktop
+        integration. The authorized Launchpad credential will be
+        stored, so that the next time your program (or any other
+        program run by that user on the same computer) needs a
+        Launchpad credential, it will be retrieved from local storage.
+
+        By subclassing `RequestTokenAuthorizationEngine` and passing
+        in an instance of the subclass as `authorization_engine`, you
+        can change what happens when the end-user needs to authorize
+        the Launchpad credential, and you can change how the
+        authorized credential is stored and retrieved locally.
 
         :param application_name: The application name. This is *not*
             the OAuth consumer name. Unless a consumer_name is also
@@ -294,89 +346,128 @@
             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
-            `Consumer` constructor
-        :type consumer_name: string
         :param service_root: The URL to the root of the web service.
         :type service_root: string.  Can either be the full URL to a service
             or one of the short service names.
-        :param launchpadlib_dir: The directory where the cache and
-            credentials are stored.
+
+        :param launchpadlib_dir: The directory used to store cached
+           data obtained from Launchpad. The cache is shared by all
+           consumers, and each Launchpad service root has its own
+           cache.
         :type launchpadlib_dir: string
+
+        :param authorization_engine: A strategy for getting the end-user to
+            authorize an OAuth request token, for exchanging the
+            request token for an access token, and for storing the
+            access token locally so that it can be reused.
+        :type authorization_engine: `RequestTokenAuthorizationEngine`
+
+        :param credential_save_failed: a callback that is called upon
+           a failure to save the credentials locally. This argument is
+           used to construct the default `authorization_engine`, so if
+           you pass in your own `authorization_engine` any value for
+           this argument will be ignored.
+        :type credential_save_failed: A callable
+
+        :param consumer_name: The consumer name, as appropriate for
+            the `Consumer` constructor. You probably don't want to
+            provide this, since providing it will prevent you from
+            taking advantage of desktop-wide integration.
+        :type consumer_name: 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.
+            this application.
+
+            This argument is used to construct the default
+            `authorization_engine`, so if you pass in your own
+            `authorization_engine` any value for this argument will be
+            ignored. This argument will also be ignored unless you
+            also specify `consumer_name`.
+
         :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
+        :return: A web service root authorized as the end-user.
         :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 = 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 (application_name is None and consumer_name is None and
+            authorization_engine is None):
+            raise ValueError(
+                "At least one of application_name, consumer_name, or "
+                "authorization_engine must be provided.")
+
+        if authorization_engine is None:
+            authorization_engine = cls.authorization_engine_factory(
+                service_root, application_name, consumer_name,
+                credential_save_failed, allow_access_levels)
+        else:
+            # An authorization engine was passed in, so we won't be
+            # using any provided values for application_name,
+            # consumer_name, or allow_access_levels. But at least make
+            # sure we weren't given conflicting values, since that
+            # makes the calling code look confusing.
+            cls._assert_login_with_argument_consistency(
+                "application_name", application_name,
+                authorization_engine.application_name)
+
+            cls._assert_login_with_argument_consistency(
+                "consumer_name", consumer_name,
+                authorization_engine.consumer.key)
+
+            cls._assert_login_with_argument_consistency(
+                "allow_access_levels", allow_access_levels,
+                authorization_engine.allow_access_levels)
+
+        credentials = authorization_engine.retrieve_credentials_from_local_store()
 
         if credentials is None:
+            # Credentials were not found in the local store. Go
+            # through the authorization process.
             launchpad = cls.get_token_and_login(
-                consumer, service_root=service_root, cache=cache_path,
-                timeout=timeout, proxy_info=proxy_info,
-                authorizer_class=authorizer_class,
-                allow_access_levels=allow_access_levels,
-                max_failed_attempts=max_failed_attempts, version=version)
-            # New credentials were created, so save them for future use.
-            if launchpad is not None:
-                try:
-                    keyring.set_password(
-                        'launchpadlib', consumer_name + '@' + service_root,
-                        serialize_credentials(launchpad.credentials))
-                except EXPLOSIVE_ERRORS:
-                    raise
-                except:
-                    if credential_save_failed is not None:
-                        credential_save_failed()
+                authorization_engine.consumer, service_root=service_root,
+                cache=cache_path, timeout=timeout, proxy_info=proxy_info,
+                authorization_engine=authorization_engine,
+                version=version)
         else:
+            # Credentials were found in the local store. Create a
+            # Launchpad object.
             launchpad = cls(
-                credentials, service_root=service_root, cache=cache_path,
-                timeout=timeout, proxy_info=proxy_info, version=version)
+                credentials, authorization_engine, service_root=service_root,
+                cache=cache_path, timeout=timeout, proxy_info=proxy_info,
+                version=version)
         return launchpad
 
     @classmethod
+    def _assert_login_with_argument_consistency(
+        cls, argument_name, argument_value, authorization_engine_value):
+        """Helper to find conflicting values passed into login_with.
+
+        Many of the arguments to login_with are used to build an
+        authorization engine. If an authorization engine is passed in,
+        many of the arguments become redundant. We'll allow redundant
+        arguments through, but if a login_with argument *conflicts* with
+        the argument in the provided authorization engine, we raise an
+        error.
+        """
+        inconsistent_value_message = (
+            "Inconsistent values given for %s: "
+            "(%r passed in to login_with(), versus %r in authorization "
+            "engine). You don't need to pass in %s to login_with() if you "
+            "pass in an authorization engine, so just omit that argument.")
+        if (argument_value is not None
+            and argument_value != authorization_engine_value):
+            raise ValueError(inconsistent_value_message % (
+                argument_name, argument_value, authorization_engine_value,
+                argument_name))
+
+
+    @classmethod
     def _get_paths(cls, service_root, launchpadlib_dir=None):
         """Locate launchpadlib-related user paths and ensure they exist.
 

=== modified file 'src/launchpadlib/testing/helpers.py'
--- src/launchpadlib/testing/helpers.py	2010-10-20 13:45:30 +0000
+++ src/launchpadlib/testing/helpers.py	2010-12-20 17:49:10 +0000
@@ -21,6 +21,8 @@
 
 __metaclass__ = type
 __all__ = [
+    'NoNetworkAuthorizationEngine',
+    'NoNetworkLaunchpad',
     'TestableLaunchpad',
     'nopriv_read_nonprivate',
     'salgado_read_nonprivate',
@@ -31,8 +33,10 @@
 
 from launchpadlib.launchpad import Launchpad
 from launchpadlib.credentials import (
+    AccessToken,
     AuthorizeRequestTokenWithBrowser,
     Credentials,
+    RequestTokenAuthorizationEngine,
     )
 
 
@@ -47,6 +51,59 @@
             version=version)
 
 
+class NoNetworkAuthorizationEngine(RequestTokenAuthorizationEngine):
+    """An authorization engine that doesn't open a web browser.
+
+    You can use this to test the creation of Launchpad objects and the
+    storing of credentials. You can't use it to interact with the web
+    service, since it only pretends to authorize its OAuth request tokens.
+    """
+    ACCESS_TOKEN_KEY = "access_key:84"
+
+    def __init__(self, *args, **kwargs):
+        super(NoNetworkAuthorizationEngine, self).__init__(*args, **kwargs)
+        # Set up some instrumentation.
+        self.request_tokens_obtained = 0
+        self.access_tokens_obtained = 0
+
+    def get_request_token(self, credentials):
+        """Pretend to get a request token from the server.
+
+        We do this by simply returning a static token ID.
+        """
+        self.request_tokens_obtained += 1
+        return "request_token:42"
+
+    def make_end_user_authorize_token(self, credentials, request_token):
+        """Pretend to exchange a request token for an access token.
+
+        We do this by simply setting the access_token property.
+        """
+        credentials.access_token = AccessToken(
+            self.ACCESS_TOKEN_KEY, 'access_secret:168')
+        self.access_tokens_obtained += 1
+
+
+class NoNetworkLaunchpad(Launchpad):
+    """A Launchpad instance for tests with no network access.
+
+    It's only useful for making sure that certain methods were called.
+    It can't be used to interact with the API.
+    """
+
+    def __init__(self, credentials, authorization_engine, service_root,
+                 cache, timeout, proxy_info, version):
+        self.credentials = credentials
+        self.authorization_engine = authorization_engine
+        self.passed_in_args = dict(
+            service_root=service_root, cache=cache, timeout=timeout,
+            proxy_info=proxy_info, version=version)
+
+    @classmethod
+    def authorization_engine_factory(cls, *args):
+        return NoNetworkAuthorizationEngine(*args)
+
+
 class KnownTokens:
     """Known access token/secret combinations."""
 

=== added file 'src/launchpadlib/tests/test_http.py'
--- src/launchpadlib/tests/test_http.py	1970-01-01 00:00:00 +0000
+++ src/launchpadlib/tests/test_http.py	2010-12-20 17:49:10 +0000
@@ -0,0 +1,228 @@
+# Copyright 2010 Canonical Ltd.
+
+# This file is part of launchpadlib.
+#
+# launchpadlib is free software: you can redistribute it and/or modify it
+# under the terms of the GNU Lesser General Public License as published by the
+# Free Software Foundation, version 3 of the License.
+#
+# launchpadlib is distributed in the hope that it will be useful, but WITHOUT
+# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+# FITNESS FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License
+# for more details.
+#
+# You should have received a copy of the GNU Lesser General Public License
+# along with launchpadlib. If not, see <http://www.gnu.org/licenses/>.
+
+"""Tests for the LaunchpadOAuthAwareHTTP class."""
+
+from collections import deque
+import unittest
+
+from simplejson import dumps, JSONDecodeError
+
+from launchpadlib.errors import Unauthorized
+from launchpadlib.launchpad import (
+    Launchpad,
+    LaunchpadOAuthAwareHttp,
+    )
+from launchpadlib.testing.helpers import (
+    NoNetworkAuthorizationEngine,
+    NoNetworkLaunchpad,
+    )
+
+
+# The simplest WADL that looks like a representation of the service root.
+SIMPLE_WADL = '''<?xml version="1.0"?>
+<application xmlns="http://research.sun.com/wadl/2006/10">
+  <resources base="http://www.example.com/">
+    <resource path="" type="#service-root"/>
+  </resources>
+
+  <resource_type id="service-root">
+    <method name="GET" id="service-root-get">
+      <response>
+        <representation href="#service-root-json"/>
+      </response>
+    </method>
+  </resource_type>
+
+  <representation id="service-root-json" mediaType="application/json"/>
+</application>
+'''
+
+# The simplest JSON that looks like a representation of the service root.
+SIMPLE_JSON = dumps({})
+
+
+class Response:
+    """A fake HTTP response object."""
+    def __init__(self, status, content):
+        self.status = status
+        self.content = content
+
+
+class SimulatedResponsesHttp(LaunchpadOAuthAwareHttp):
+    """Responds to HTTP requests by shifting responses off a stack."""
+
+    def __init__(self, responses, *args):
+        """Constructor.
+
+        :param responses: A list of HttpResponse objects to use
+            in response to requests.
+        """
+        self.sent_responses = []
+        self.unsent_responses = responses
+        super(SimulatedResponsesHttp, self).__init__(*args)
+
+    def _request(self, *args):
+        response = self.unsent_responses.popleft()
+        self.sent_responses.append(response)
+        return self.retry_on_bad_token(response, response.content)
+
+
+class SimulatedResponsesLaunchpad(Launchpad):
+
+    # Every Http object generated by this class will return these
+    # responses, in order.
+    responses = []
+
+    def httpFactory(self, *args):
+        return SimulatedResponsesHttp(
+            deque(self.responses), self, self.authorization_engine, *args)
+
+
+class SimulatedResponsesTestCase(unittest.TestCase):
+    """Test cases that give fake responses to launchpad's HTTP requests."""
+
+    def setUp(self):
+        """Clear out the list of simulated responses."""
+        SimulatedResponsesLaunchpad.responses = []
+        self.engine = NoNetworkAuthorizationEngine(
+            'http://api.example.com/', 'application name')
+
+    def launchpad_with_responses(self, *responses):
+        """Use simulated HTTP responses to get a Launchpad object.
+
+        The given Response objects will be sent, in order, in response
+        to launchpadlib's requests.
+
+        :param responses: Some number of Response objects.
+        :return: The Launchpad object, assuming that errors in the
+            simulated requests didn't prevent one from being created.
+        """
+        SimulatedResponsesLaunchpad.responses = responses
+        return SimulatedResponsesLaunchpad.login_with(
+            'application name', authorization_engine=self.engine)
+
+
+class TestAbilityToParseData(SimulatedResponsesTestCase):
+    """Test launchpadlib's ability to handle the sample data.
+
+    To create a Launchpad object, two HTTP requests must succeed and
+    return usable data: the requests for the WADL and JSON
+    representations of the service root. This test shows that the
+    minimal data in SIMPLE_WADL and SIMPLE_JSON is good enough to
+    create a Launchpad object.
+    """
+
+    def test_minimal_data(self):
+        """Make sure that launchpadlib can use the minimal data."""
+        launchpad = self.launchpad_with_responses(
+            Response(200, SIMPLE_WADL),
+            Response(200, SIMPLE_JSON))
+
+    def test_bad_wadl(self):
+        """Show that bad WADL causes an exception."""
+        self.assertRaises(
+            SyntaxError, self.launchpad_with_responses,
+            Response(200, "This is not WADL."),
+            Response(200, SIMPLE_JSON))
+
+    def test_bad_json(self):
+        """Show that bad JSON causes an exception."""
+        self.assertRaises(
+            JSONDecodeError, self.launchpad_with_responses,
+            Response(200, SIMPLE_WADL),
+            Response(200, "This is not JSON."))
+
+
+class TestTokenFailureDuringRequest(SimulatedResponsesTestCase):
+    """Test access token failures during a request.
+
+    launchpadlib makes two HTTP requests on startup, to get the WADL
+    and JSON representations of the service root. If Launchpad
+    receives a 401 error during this process, it will acquire a fresh
+    access token and try again.
+    """
+
+    def test_good_token(self):
+        """If our token is good, we never get another one."""
+        SimulatedResponsesLaunchpad.responses = [
+            Response(200, SIMPLE_WADL),
+            Response(200, SIMPLE_JSON)]
+
+        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)
+
+    def test_bad_token(self):
+        """If our token is bad, we get another one."""
+        SimulatedResponsesLaunchpad.responses = [
+            Response(401, "Invalid token."),
+            Response(200, SIMPLE_WADL),
+            Response(200, SIMPLE_JSON)]
+
+        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)
+
+    def test_expired_token(self):
+        """If our token is expired, we get another one."""
+
+        SimulatedResponsesLaunchpad.responses = [
+            Response(401, "Expired token."),
+            Response(200, SIMPLE_WADL),
+            Response(200, SIMPLE_JSON)]
+
+        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)
+
+    def test_delayed_error(self):
+        """We get another token no matter when the error happens."""
+        SimulatedResponsesLaunchpad.responses = [
+            Response(200, SIMPLE_WADL),
+            Response(401, "Expired token."),
+            Response(200, SIMPLE_JSON)]
+
+        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)
+
+    def test_many_errors(self):
+        """We'll keep getting new tokens as long as tokens are the problem."""
+        SimulatedResponsesLaunchpad.responses = [
+            Response(401, "Invalid token."),
+            Response(200, SIMPLE_WADL),
+            Response(401, "Expired token."),
+            Response(401, "Invalid token."),
+            Response(200, SIMPLE_JSON)]
+        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)
+
+    def test_other_unauthorized(self):
+        """If the token is not at fault, a 401 error raises an exception."""
+
+        SimulatedResponsesLaunchpad.responses = [
+            Response(401, "Some other error.")]
+
+        self.assertRaises(
+            Unauthorized, SimulatedResponsesLaunchpad.login_with,
+            'application name', authorization_engine=self.engine)

=== modified file 'src/launchpadlib/tests/test_launchpad.py'
--- src/launchpadlib/tests/test_launchpad.py	2010-11-01 20:18:48 +0000
+++ src/launchpadlib/tests/test_launchpad.py	2010-12-20 17:49:10 +0000
@@ -31,44 +31,19 @@
 
 from launchpadlib.credentials import (
     AccessToken,
-    AuthorizeRequestTokenWithBrowser,
     Credentials,
-    SystemWideConsumer,
     )
-from launchpadlib.launchpad import Launchpad
+
 from launchpadlib import uris
 import launchpadlib.launchpad
-
-class NoNetworkLaunchpad(Launchpad):
-    """A Launchpad instance for tests with no network access.
-
-    It's only useful for making sure that certain methods were called.
-    It can't be used to interact with the API.
-    """
-
-    consumer_name = None
-    passed_in_kwargs = None
-    credentials = None
-    get_token_and_login_called = False
-
-    def __init__(self, credentials, **kw):
-        self.credentials = credentials
-        self.passed_in_kwargs = kw
-
-    @classmethod
-    def get_token_and_login(cls, consumer, **kw):
-        """Create fake credentials and record that we were called."""
-        credentials = Credentials(
-            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 = consumer
-        launchpad.passed_in_kwargs = kw
-        return launchpad
-
+from launchpadlib.launchpad import Launchpad
+from launchpadlib.testing.helpers import (
+    NoNetworkAuthorizationEngine,
+    NoNetworkLaunchpad,
+    )
+
+# A dummy service root for use in tests
+SERVICE_ROOT = "http://api.example.com/"
 
 class TestResourceTypeClasses(unittest.TestCase):
     """launchpadlib must know about restfulclient's resource types."""
@@ -137,7 +112,7 @@
         version = "version-foo"
         root = uris.service_roots['staging'] + version
         try:
-            Launchpad(None, root, version=version)
+            Launchpad(None, None, service_root=root, version=version)
         except ValueError, e:
             self.assertTrue(str(e).startswith(
                 "It looks like you're using a service root that incorporates "
@@ -149,13 +124,15 @@
         # Make sure the problematic URL is caught even if it has a
         # slash on the end.
         root += '/'
-        self.assertRaises(ValueError, Launchpad, None, root, version=version)
+        self.assertRaises(ValueError, Launchpad, None, None,
+                          service_root=root, version=version)
 
         # Test that the default version has the same problem
         # when no explicit version is specified
         default_version = NoNetworkLaunchpad.DEFAULT_VERSION
         root = uris.service_roots['staging'] + default_version + '/'
-        self.assertRaises(ValueError, Launchpad, None, root)
+        self.assertRaises(ValueError, Launchpad, None, None,
+                          service_root=root)
 
 
 class InMemoryKeyring:
@@ -171,6 +148,32 @@
         return self.data.get((service, username))
 
 
+class TestRequestTokenAuthorizationEngine(unittest.TestCase):
+    """Tests for the RequestTokenAuthorizationEngine class."""
+
+    def test_app_must_be_identified(self):
+        self.assertRaises(
+            ValueError, NoNetworkAuthorizationEngine, SERVICE_ROOT)
+
+    def test_application_name_identifies_app(self):
+        NoNetworkAuthorizationEngine(SERVICE_ROOT, application_name='name')
+
+    def test_consumer_name_identifies_app(self):
+        NoNetworkAuthorizationEngine(SERVICE_ROOT, consumer_name='name')
+
+    def test_conflicting_app_identification(self):
+        # You can't specify both application_name and consumer_name.
+        self.assertRaises(
+            ValueError, NoNetworkAuthorizationEngine,
+            SERVICE_ROOT, application_name='name1', consumer_name='name2')
+
+        # This holds true even if you specify the same value for
+        # both. They're not the same thing.
+        self.assertRaises(
+            ValueError, NoNetworkAuthorizationEngine,
+            SERVICE_ROOT, application_name='name', consumer_name='name')
+
+
 class TestLaunchpadLoginWith(unittest.TestCase):
     """Tests for Launchpad.login_with()."""
 
@@ -178,21 +181,33 @@
         # 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.launchpad.keyring
-        launchpadlib.launchpad.keyring = InMemoryKeyring()
+        self._saved_keyring = launchpadlib.credentials.keyring
+        launchpadlib.credentials.keyring = InMemoryKeyring()
         self.temp_dir = tempfile.mkdtemp()
 
     def tearDown(self):
         # Restore the gnomekeyring module that we disabled in setUp.
-        launchpadlib.launchpad.keyring = self._saved_keyring
+        launchpadlib.credentials.keyring = self._saved_keyring
         shutil.rmtree(self.temp_dir)
 
+    def test_max_failed_attempts_accepted(self):
+        # You can pass in a value for the 'max_failed_attempts'
+        # argument, even though that argument doesn't do anything.
+        launchpad = NoNetworkLaunchpad.login_with(
+            'not important', max_failed_attempts=5)
+
+    def test_credentials_file_accepted(self):
+        # You can pass in a value for the 'credentials_file'
+        # argument, even though that argument doesn't do anything.
+        launchpad = NoNetworkLaunchpad.login_with(
+            'not important', credentials_file='foo')
+
     def test_dirs_created(self):
         # The path we pass into login_with() is the directory where
         # cache and credentials for all service roots are stored.
         launchpadlib_dir = os.path.join(self.temp_dir, 'launchpadlib')
         launchpad = NoNetworkLaunchpad.login_with(
-            'not important', service_root='http://api.example.com/',
+            'not important', service_root=SERVICE_ROOT,
             launchpadlib_dir=launchpadlib_dir)
         # The 'launchpadlib' dir got created.
         self.assertTrue(os.path.isdir(launchpadlib_dir))
@@ -219,7 +234,7 @@
         mode = stat.S_IMODE(statinfo.st_mode)
         self.assertNotEqual(mode, stat.S_IWRITE | stat.S_IREAD | stat.S_IEXEC)
         launchpad = NoNetworkLaunchpad.login_with(
-            'not important', service_root='http://api.example.com/',
+            'not important', service_root=SERVICE_ROOT,
             launchpadlib_dir=launchpadlib_dir)
         # Verify the mode has been changed to 0700
         statinfo = os.stat(launchpadlib_dir)
@@ -229,7 +244,7 @@
     def test_dirs_created_are_secure(self):
         launchpadlib_dir = os.path.join(self.temp_dir, 'launchpadlib')
         launchpad = NoNetworkLaunchpad.login_with(
-            'not important', service_root='http://api.example.com/',
+            'not important', service_root=SERVICE_ROOT,
             launchpadlib_dir=launchpadlib_dir)
         self.assertTrue(os.path.isdir(launchpadlib_dir))
         # Verify the mode is safe
@@ -243,18 +258,18 @@
         # credentials will be cached to disk.
         launchpadlib_dir = os.path.join(self.temp_dir, 'launchpadlib')
         launchpad = NoNetworkLaunchpad.login_with(
-            'not important', service_root='http://api.example.com/',
+            'not important', service_root=SERVICE_ROOT,
             launchpadlib_dir=launchpadlib_dir, version="foo")
-        self.assertEquals(launchpad.passed_in_kwargs['version'], 'foo')
+        self.assertEquals(launchpad.passed_in_args['version'], 'foo')
 
         # 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 'version' argument.
         launchpad = NoNetworkLaunchpad.login_with(
-            'not important', service_root='http://api.example.com/',
+            'not important', service_root=SERVICE_ROOT,
             launchpadlib_dir=launchpadlib_dir, version="bar")
-        self.assertEquals(launchpad.passed_in_kwargs['version'], 'bar')
+        self.assertEquals(launchpad.passed_in_args['version'], 'bar')
 
     def test_application_name_is_propagated(self):
         # Create a Launchpad instance for a given application name.
@@ -263,7 +278,7 @@
         # 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/',
+            'very important', service_root=SERVICE_ROOT,
             launchpadlib_dir=launchpadlib_dir)
         self.assertEquals(
             launchpad.credentials.consumer.application_name, 'very important')
@@ -274,36 +289,112 @@
         # the application name, instead of picking an empty one from
         # disk.
         launchpad = NoNetworkLaunchpad.login_with(
-            'very important', service_root='http://api.example.com/',
+            'very important', service_root=SERVICE_ROOT,
             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/'
+    def test_authorization_engine_is_propagated(self):
+        # You can pass in a custom authorization engine, which will be
+        # used to get a request token and exchange it for an access
+        # token.
+        engine = NoNetworkAuthorizationEngine(
+            SERVICE_ROOT, 'application name')
+        launchpad = NoNetworkLaunchpad.login_with(
+            authorization_engine=engine)
+        self.assertEquals(engine.request_tokens_obtained, 1)
+        self.assertEquals(engine.access_tokens_obtained, 1)
+
+    def test_login_with_must_identify_application(self):
+        # If you call login_with without identifying your application
+        # you'll get an error.
+        self.assertRaises(ValueError, NoNetworkLaunchpad.login_with)
+
+    def test_application_name_identifies_app(self):
+        NoNetworkLaunchpad.login_with(application_name="name")
+
+    def test_consumer_name_identifies_app(self):
+        NoNetworkLaunchpad.login_with(consumer_name="name")
+
+    def test_inconsistent_application_name_rejected(self):
+        """Catch an attempt to specify inconsistent application_names."""
+        engine = NoNetworkAuthorizationEngine(
+            SERVICE_ROOT, 'application name1')
+        self.assertRaises(ValueError, NoNetworkLaunchpad.login_with,
+                          "application name2",
+                          authorization_engine=engine)
+
+    def test_inconsistent_consumer_name_rejected(self):
+        """Catch an attempt to specify inconsistent application_names."""
+        engine = NoNetworkAuthorizationEngine(
+            SERVICE_ROOT, None, consumer_name="consumer_name1")
+
+        self.assertRaises(ValueError, NoNetworkLaunchpad.login_with,
+                          "consumer_name2",
+                          authorization_engine=engine)
+
+    def test_inconsistent_allow_access_levels_rejected(self):
+        """Catch an attempt to specify inconsistent allow_access_levels."""
+        engine = NoNetworkAuthorizationEngine(
+            SERVICE_ROOT, consumer_name="consumer",
+            allow_access_levels=['FOO'])
+
+        self.assertRaises(ValueError, NoNetworkLaunchpad.login_with,
+                          None, consumer_name="consumer",
+                          allow_access_levels=['BAR'],
+                          authorization_engine=engine)
+
+    def test_non_desktop_integration(self):
+        # When doing a non-desktop integration, you must specify a
+        # consumer_name. You can pass a list of allowable access
+        # levels into login_with().
+        launchpad = NoNetworkLaunchpad.login_with(
+            consumer_name="consumer", allow_access_levels=['FOO'])
+        self.assertEquals(launchpad.credentials.consumer.key, "consumer")
+        self.assertEquals(launchpad.credentials.consumer.application_name,
+                          None)
+        self.assertEquals(launchpad.authorization_engine.allow_access_levels,
+                          ['FOO'])
+
+    def test_desktop_integration_doesnt_happen_without_consumer_name(self):
+        # The only way to do a non-desktop integration is to specify a
+        # consumer_name. If you specify application_name instead, your
+        # value for allow_access_levels is ignored, and a desktop
+        # integration is performed.
+        launchpad = NoNetworkLaunchpad.login_with(
+            'application name', allow_access_levels=['FOO'])
+        self.assertEquals(launchpad.authorization_engine.allow_access_levels,
+                          ['DESKTOP_INTEGRATION'])
+
+    def test_no_credentials_creates_new_credential(self):
+        # If no credentials are found, a desktop-wide credential is created.
         timeout = object()
         proxy_info = object()
         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.application_name, 'app name')
+            service_root=SERVICE_ROOT, timeout=timeout, proxy_info=proxy_info)
+        # Here's the new credential.
+        self.assertEqual(launchpad.credentials.access_token.key,
+                         NoNetworkAuthorizationEngine.ACCESS_TOKEN_KEY)
+        self.assertEqual(launchpad.credentials.consumer.application_name,
+                         'app name')
+        self.assertEquals(launchpad.authorization_engine.allow_access_levels,
+                          ['DESKTOP_INTEGRATION'])
+        # The expected arguments were passed in to the Launchpad
+        # constructor.
         expected_arguments = dict(
-            allow_access_levels=['DESKTOP_INTEGRATION'],
-            authorizer_class=AuthorizeRequestTokenWithBrowser,
-            max_failed_attempts=3,
-            service_root=service_root,
-            timeout=timeout,
-            proxy_info=proxy_info,
+            service_root=SERVICE_ROOT,
             cache=os.path.join(self.temp_dir, 'api.example.com', 'cache'),
+            timeout=timeout,
+            proxy_info=proxy_info,
             version=NoNetworkLaunchpad.DEFAULT_VERSION)
-        self.assertEqual(launchpad.passed_in_kwargs, expected_arguments)
+        self.assertEqual(launchpad.passed_in_args, expected_arguments)
 
     def test_anonymous_login(self):
         """Test the anonymous login helper function."""
         launchpad = NoNetworkLaunchpad.login_anonymously(
             'anonymous access', launchpadlib_dir=self.temp_dir,
-            service_root='http://api.example.com/')
+            service_root=SERVICE_ROOT)
         self.assertEqual(launchpad.credentials.access_token.key, '')
         self.assertEqual(launchpad.credentials.access_token.secret, '')
 
@@ -325,22 +416,21 @@
             access_token=AccessToken('access_key:84', 'access_secret:168'))
         credentials.save_to_path(credentials_file_path)
 
-        service_root = 'http://api.example.com/'
         timeout = object()
         proxy_info = object()
         version = "foo"
         launchpad = NoNetworkLaunchpad.login_with(
             'app name', launchpadlib_dir=self.temp_dir,
-            service_root=service_root, timeout=timeout, proxy_info=proxy_info,
+            service_root=SERVICE_ROOT, timeout=timeout, proxy_info=proxy_info,
             version=version)
         expected_arguments = dict(
-            service_root=service_root,
+            service_root=SERVICE_ROOT,
             timeout=timeout,
             proxy_info=proxy_info,
             version=version,
             cache=os.path.join(self.temp_dir, 'api.example.com', 'cache'))
         for key, expected in expected_arguments.items():
-            actual = launchpad.passed_in_kwargs[key]
+            actual = launchpad.passed_in_args[key]
             self.assertEqual(actual, expected)
 
     def test_None_launchpadlib_dir(self):
@@ -349,11 +439,11 @@
         old_home = os.environ['HOME']
         os.environ['HOME'] = self.temp_dir
         launchpad = NoNetworkLaunchpad.login_with(
-            'app name', service_root='http://api.example.com/')
+            'app name', service_root=SERVICE_ROOT)
         # Reset the environment to the old value.
         os.environ['HOME'] = old_home
 
-        cache_dir = launchpad.passed_in_kwargs['cache']
+        cache_dir = launchpad.passed_in_args['cache']
         launchpadlib_dir = os.path.abspath(
             os.path.join(cache_dir, '..', '..'))
         self.assertEqual(
@@ -365,14 +455,14 @@
         # A short service name is converted to the full service root URL.
         launchpad = NoNetworkLaunchpad.login_with('app name', 'staging')
         self.assertEqual(
-            launchpad.passed_in_kwargs['service_root'],
+            launchpad.passed_in_args['service_root'],
             'https://api.staging.launchpad.net/')
 
         # A full URL as the service name is left alone.
         launchpad = NoNetworkLaunchpad.login_with(
             'app name', uris.service_roots['staging'])
         self.assertEqual(
-            launchpad.passed_in_kwargs['service_root'],
+            launchpad.passed_in_args['service_root'],
             uris.service_roots['staging'])
 
         # A short service name that does not match one of the
@@ -402,10 +492,10 @@
 
 @contextmanager
 def fake_keyring(fake):
-    original_keyring = launchpadlib.launchpad.keyring
-    launchpadlib.launchpad.keyring = fake
+    original_keyring = launchpadlib.credentials.keyring
+    launchpadlib.credentials.keyring = fake
     yield
-    launchpadlib.launchpad.keyring = original_keyring
+    launchpadlib.credentials.keyring = original_keyring
 
 
 class TestCredenitialSaveFailedCallback(unittest.TestCase):
@@ -434,9 +524,10 @@
             callback_called.append(None)
 
         launchpadlib_dir = os.path.join(self.temp_dir, 'launchpadlib')
+        service_root = "http://api.example.com/"
         with fake_keyring(BadSaveKeyring()):
             NoNetworkLaunchpad.login_with(
-                'not important', service_root='http://api.example.com/',
+                'not important', service_root=service_root,
                 launchpadlib_dir=launchpadlib_dir,
                 credential_save_failed=callback)
             self.assertEquals(len(callback_called), 1)

=== modified file 'src/launchpadlib/uris.py'
--- src/launchpadlib/uris.py	2010-10-27 21:22:29 +0000
+++ src/launchpadlib/uris.py	2010-12-20 17:49:10 +0000
@@ -24,9 +24,11 @@
 __all__ = [
     'lookup_service_root',
     'lookup_web_root',
+    'web_root_for_service_root',
     ]
 
 from urlparse import urlparse
+from lazr.uri import URI
 
 LPNET_SERVICE_ROOT = 'https://api.launchpad.net/'
 EDGE_SERVICE_ROOT = 'https://api.edge.launchpad.net/'
@@ -101,3 +103,15 @@
     """
     return _dereference_alias(web_root, web_roots)
 
+
+def web_root_for_service_root(service_root):
+    """Turn a service root URL into a web root URL.
+
+    This is done heuristically, not with a lookup.
+    """
+    service_root = lookup_service_root(service_root)
+    web_root_uri = URI(service_root)
+    web_root_uri.path = ""
+    web_root_uri.host = web_root_uri.host.replace("api.", "", 1)
+    web_root = str(web_root_uri.ensureSlash())
+    return web_root