Merge lp:~leonardr/launchpadlib/separate-login-for-cronjob into lp:launchpadlib

[Leonard Richardson]

This branch has three interrelated purposes:

1. Currently the "request token authorization engine" object includes two strategies: a strategy for authorizing an OAuth request token, and a strategy for storing the resulting launchpadlib credentials locally. This branch decouples those two strategies. The strategy for storing launchpadlib credentials is now kept in a separate object, the "credential store". The code from RequestTokenAuthorizationEngine that stores credentials in the GNOME keyring is moved into the KeyringCredentialStore.

2. Currently there is no way to store launchpadlib credentials unencrypted in a file on disk. We deliberately got rid of this feature for security reasons, but there is a legitimate case for this: see bug 686690. This branch introduces a new credential store, the UnencryptedFileCredentialStore, which has the old behavior.

3. The difference between storing launchpadlib credentials unencrypted on disk vs. in the keyring is a big, big difference. We can't treat it as the difference between passing in a filename to login_with() and not passing in a filename. So I've split login_with() into two methods: login_securely() and login_insecurely(). You use login_insecurely() when you must: when your script must run without any user interaction whatsoever. You use login_securely() when you can.

I've taken this opportunity to deprecate most of the old login helpers: login_with(), login(), and get_token_and_login(). These methods arose over time to make the login code easier to write, and they weren't planned out. This is a good opportunity to start phasing them out. There are now three login helpers, one for each approach to credential security: login_insecurely(), login_securely(), and login_anonymously() (which doesn't get a credential at all).

Since I'm defining new login methods, I've also taken this opportunity to reorder the arguments they accept. Over the past couple of years we've added lots of arguments to the end of the login_with() signature, and it got very disorganized.

4. Other notes:

I moved fake_keyring, FauxSocketModule, BadSaveKeyring, and InMemoryKeyring from test_launchpad.py to testing/helpers.py.

I will need to do a follow-up Launchpad branch to get it to use the new methods, and a follow-up launchpadlib branch to stop KnownTokens from using the deprecated login() method. (I put an XXX in this part of the code).

[lp:~leonardr/launchpadlib/separate-login-for-cronjob]

128. By Leonard Richardson on 2010-12-23

Merge from trunk.

[Robert Collins (lifeless)]

Reading the cover, login_insecurely and login_securely seem like value
judgements rather than api changes.

Isn't the real difference
login-and-can-prompt-for-credentials
login-using-cached-credentials-only
?

After all, disk files in an encrypted per-account system are
approximately as secure as those in the gnome keyring (both need only
compromise the account to access the credentials).

[Leonard Richardson (leonardr)]

I'm not attached to the current method names at all. That said, the difference is not quite "login-and-can-prompt-for-credentials" versus "login-using-cached-credentials-only". The difference is which credential store is used to load *and save* the credential.

Even though login_insecurely is "the one you should use for cron scripts", if there is no credential it will still cause a browser open and require user input. So we can't use my original ideas for names, "login_interactive" and "login_non_interactive", nor anything like "login_using_cached_credentials_only".

My principles:

1. The difference between the methods is "look in the keyring" versus "look in an unencrypted (as far as launchpadlib knows) file".
2, I would like to guide people towards the keyring method. The unencrypted file method should be for people who have problems with the keyring method.

Related thoughts on encrypted filesystems:

I've got an encrypted home directory. I set up a cronjob to run a launchpadlib script. I use login_insecurely(), but I store my credential inside my home directory. Because I have an encrypted home directory, it's not really 'insecure'--it's about as secure as stuff in my keyring.

But the cronjob will only run when my home directory is unencrypted, ie. when I'm logged in. When I'm not logged in, the credential will be unreadable and the cronjob will fail. If I want the cronjob to run all the time, I need to store the credential in an unencrypted filesystem. I think that's the common cron scenario, and I'd say that is 'insecure', relatively speaking.

So, I don't think the existing names are terribly misleading. You can use login_insecurely() in a secure way, if you'll be logged in whenever the script runs. But if you're already logged in, login_securely() will work just as well. And the common case for login_securely() *is* more secure than the common case for login_insecurely().

[Gavin Panella (allenap)]

The new CredentialStore stuff is cool.

I've got lots of comments, but they're all fairly minor.

[1]

+ * Launchpad.login_securely, for programs that are run with user
+ interaction.
+ * Launchpad.login_insecurely, for programs that must run with no
+ user interaction.

These names worry me. I don't want to debate these names if they have
already been discussed. I just want to say: if I am the only person to
have seen them then I think they need some discussion, though not a
lot.

[2]

+
+
+ def do_load(self, unique_key):

Dangerous extra blank line.

[3]

Some lint:

credentials.py:29: 'base64' imported but unused
credentials.py:34: 'sys' imported but unused
credentials.py:35: 'textwrap' imported but unused
credentials.py:37: 'quote' imported but unused
launchpad.py:25: 'socket' imported but unused
launchpad.py:26: 'stat' imported but unused
launchpad.py:29: 'HostedFile' imported but unused
launchpad.py:29: 'ScalarValue' imported but unused
launchpad.py:36: 'SystemWideConsumer' imported but unused
launchpad.py:52: 'STAGING_SERVICE_ROOT' imported but unused
launchpad.py:52: 'EDGE_SERVICE_ROOT' imported but unused
testing/helpers.py:37: 'simplejson' imported but unused
testing/helpers.py:41: 'AuthorizeRequestTokenWithBrowser' imported but unused
testing/helpers.py:41: 'Credentials' imported but unused
tests/test_launchpad.py:21: 'StringIO' imported but unused
tests/test_launchpad.py:27: 'textwrap' imported but unused
tests/test_launchpad.py:226: local variable 'launchpad' is assigned to but never used
tests/test_launchpad.py:253: local variable 'launchpad' is assigned to but never used
tests/test_launchpad.py:263: local variable 'launchpad' is assigned to but never used
tests/test_launchpad.py:320: local variable 'launchpad' is assigned to but never used
tests/test_launchpad.py:534: local variable 'launchpad' is assigned to but never used
tests/test_launchpad.py:618: local variable 'service_root' is assigned to but never used

[4]

+ def __init__(self, file, credential_save_failed=None):
+ super(UnencryptedFileCredentialStore, self).__init__(
+ credential_save_failed)
+ self.file = file

Because file() is a builtin function/type consider using filename or
filepath instead, or something like that.

[5]

+ This method is deprecated as of launchpadlib version
+ 1.9.0. You should use Launchpad.login_anonymously() for
+ anonymous access, Launchpad.login_insecurely() for scripts
+ that need to run without any user interaction, and
+ Launchpad.login_securely() for all other purposes, possibly
+ specifying a custom authorization_engine or credential_store.

Perhaps it's worth putting some or all of this into a deprecation
warning, warnings.warn("...", DeprecationWarning)?

[6]

+@contextmanager
+def fake_keyring(fake):
+ original_keyring = launchpadlib.credentials.keyring
+ launchpadlib.credentials.keyring = fake
+ yield
+ launchpadlib.credentials.keyring = original_keyring

This will break when a test fails because the yield can raise an
exception. Something like the following would work better I think:

@contextmanager
def fake_keyring(fake):
    orig...

review: Approve

[Robert Collins (lifeless)]

So, these names then are essentially aliases to (pseudocode)
login(... store=keyringstore)
login(... store=filestore)

why not expose that instead.
login(... store=None)
"""
store: a store to request credentials from. If None, GnomeKeyringStore
will be used, as the most reliable and secure store we have available
today. Using a different store (see <...> for a list of stores) is
needed when running scripts outside of a desktop context such as from
cron.
"""

?

[Leonard Richardson (leonardr)]

> So, these names then are essentially aliases to (pseudocode)
> login(... store=keyringstore)
> login(... store=filestore)
>
> why not expose that instead.
> login(... store=None)
> """
> store: a store to request credentials from. If None, GnomeKeyringStore
> will be used, as the most reliable and secure store we have available
> today. Using a different store (see <...> for a list of stores) is
> needed when running scripts outside of a desktop context such as from
> cron.
> """

I'm fine with this. The main problem is that we have already staked out the namespace of generic names. login() and login_with() are existing methods that I hope to deprecate with this branch.

I think it wouldn't be _too_ bad to change the behavior of login() in 1.9.0, since everyone I know of uses login_with(). We could even call it 2.0.0. (But, I think a 3.0.0 would not be far behind--I was planning to make another big backward-incompatible change, though I don't remember what it was at the moment.)

Without an alias for cron usage, the code to run a cron script would look like this:

store = UnencryptedFileCredentialStore(credentials_file)
launchpad = Launchpad.login("my application" "production", credential_store=store)

Martin, are you OK with this?

[lp:~leonardr/launchpadlib/separate-login-for-cronjob]

129. By Leonard Richardson on 2011-01-03

Response to feedback: lint cleanup, (hackily) replace mktemp with mkstemp.

[Leonard Richardson (leonardr)]

I've addressed all of your comments except for [10]. I need to add
tests for the deprecation warning, which I will do with the
catch_warnings context manager. I also have some unknown test failures that
I don't remember having when I left for vacation last year, and I need
to look into those.

I got rid of most of the lint. launchpad.HostedFile and
launchpad.ScalarValue are re-imported from lazr.restfulclient for
client convenience, so I added comments to that effect.

Re: mkstemp. I deliberately avoided mkstemp because mkstemp creates an
empty file. lazr.restfulclient's Credentials.load_from_file() crashes
if you give it an empty file. Should I do a lazr.restfulclient release
and make load_from_path() treat an empty file the same as a
nonexistant file?

In the meantime, I have put a hack in place to make mkstemp work on
the launchpadlib level.

See my previous comment re: the method names. I'm okay with one
generic name, but we took the most generic name ("login") in the first
version of launchpadlib, and then we took another generic name
("login_with") in a later revision.

[Gavin Panella (allenap)]

> Re: mkstemp. I deliberately avoided mkstemp because mkstemp creates
> an empty file. lazr.restfulclient's Credentials.load_from_file()
> crashes if you give it an empty file. Should I do a
> lazr.restfulclient release and make load_from_path() treat an empty
> file the same as a nonexistant file?
>
> In the meantime, I have put a hack in place to make mkstemp work on
> the launchpadlib level.

An alternate approach might be to safely create a temporary directory
with tempfile.mkdtemp() then use static names within.

[13]

+ warnings.warn(
+ ("The Launchpad.%s() method is deprecated. You should use "
+ "Launchpad.login_anonymously() for anonymous access, "
+ "Launchpad.login_insecurely() for scripts that need to run "
+ "without any user interaction, and Launchpad.login_securely() "
+ "for all other purposes.") % name)

I haven't used warnings much before so I don't know if it works well
in practice, but you could pass DeprecationWarning as a second
argument to warn(). I think this makes it easier for users to filter
it out without missing other warnings.

[lp:~leonardr/launchpadlib/separate-login-for-cronjob]

130. By Leonard Richardson on 2011-01-03

Re-activated deprecation warning.

131. By Leonard Richardson on 2011-01-04

Fix test failures, which were caused by cached responses from actual usage being sent to the NoNetworkLaunchpad.

132. By Leonard Richardson on 2011-01-04

Finally got rid of all test failures and unxpected deprecation warnings.

133. By Leonard Richardson on 2011-01-04

Removed login_insecurely and login_securely. They are both now part of login_with(), which has been un-deprecated.

134. By Leonard Richardson on 2011-01-04

We don't need to test credential_save_failed twice now that there's only one method.

135. By Leonard Richardson on 2011-01-04

Updated the NEWS to reflect my most recent change.

[Leonard Richardson (leonardr)]

After evaluating my options I've decided to un-deprecate login_with(). This method acts like login_insecurely() when you specify a file to store the credential in, and acts like login_securely() when you don't. That's a perfect description of what I wanted login() to do.

Once we get rid of login(), we'll have space to come back to this problem and call the resulting method login(). At the very least, we need to rationalize the arguments to login_with(), which are out of control and presented in random order since we kept tacking arguments on to the end of the list.

=== modified file 'src/launchpadlib/NEWS.txt'
--- src/launchpadlib/NEWS.txt	2010-12-20 12:41:52 +0000
+++ src/launchpadlib/NEWS.txt	2011-01-04 19:04:27 +0000
@@ -11,6 +11,13 @@
 
 - The HTML generated by wadl-to-refhtml.xsl now validates.
 
+- Most of the helper login methods have been deprecated. There are now
+  only two helper methods:
+
+  * Launchpad.login_anonymously, for anonymous credential-free access.
+  * Launchpad.login_with, for programs that need a credential.
+
+
 1.8.0 (2010-11-15)
 ==================
 
 
=== modified file 'src/launchpadlib/credentials.py'
--- src/launchpadlib/credentials.py	2010-12-16 22:33:31 +0000
+++ src/launchpadlib/credentials.py	2011-01-04 19:04:27 +0000
@@ -21,19 +21,19 @@
     'AccessToken',
     'AnonymousAccessToken',
     'AuthorizeRequestTokenWithBrowser',
+    'CredentialStore',
     'RequestTokenAuthorizationEngine',
     'Consumer',
     'Credentials',
     ]
 
-import base64
 import cgi
 from cStringIO import StringIO
 import httplib2
-import sys
-import textwrap
+import os
+import stat
 import time
-from urllib import urlencode, quote
+from urllib import urlencode
 from urlparse import urljoin
 import webbrowser
 
@@ -212,6 +212,122 @@
         super(AnonymousAccessToken, self).__init__('','')
 
 
+class CredentialStore(object):
+    """Store OAuth credentials locally.
+
+    This is a generic superclass. To implement a specific way of
+    storing credentials locally you'll need to subclass this class,
+    and implement `do_save` and `do_load`.
+    """
+
+    def __init__(self, credential_save_failed=None):
+        """Constructor.
+
+        :param credential_save_failed: A callback to be invoked if the
+            save to local storage fails. You should never invoke this
+            callback yourself! Instead, you should raise an exception
+            from do_save().
+        """
+        self.credential_save_failed = credential_save_failed
+
+    def save(self, credentials, unique_consumer_id):
+        """Save the credentials and invoke the callback on failure."""
+        try:
+            self.do_save(credentials, unique_consumer_id)
+        except EXPLOSIVE_ERRORS:
+            raise
+        except:
+            if self.credential_save_failed is not None:
+                self.credential_save_failed()
+        return credentials
+
+    def do_save(self, credentials, unique_consumer_id):
+        """Store newly-authorized credentials locally for later use.
+
+        :param credentials: A Credentials object to save.
+        :param unique_consumer_id: A string uniquely identifying an
+            OAuth consumer on a Launchpad instance.
+        """
+        raise NotImplementedError()
+
+    def load(self, unique_key):
+        """Retrieve credentials from a local store.
+
+        This method is the inverse of `save`.
+
+        There's no special behavior in this method--it just calls
+        `do_load`. There _is_ special behavior in `save`, and this
+        way, developers can remember to implement `do_save` and
+        `do_load`, not `do_save` and `load`.
+
+        :param unique_key: A string uniquely identifying an OAuth consumer
+            on a Launchpad instance.
+
+        :return: A `Credentials` object if one is found in the local
+            store, and None otherise.
+        """
+        return self.do_load(unique_key)
+
+    def do_load(self, unique_key):
+        """Retrieve credentials from a local store.
+
+        This method is the inverse of `do_save`.
+
+        :param unique_key: A string uniquely identifying an OAuth consumer
+            on a Launchpad instance.
+
+        :return: A `Credentials` object if one is found in the local
+            store, and None otherise.
+        """
+        raise NotImplementedError()
+
+
+class KeyringCredentialStore(CredentialStore):
+    """Store credentials in the GNOME keyring or KDE wallet.
+
+    This is a good solution for desktop applications and interactive
+    scripts. It doesn't work for non-interactive scripts, or for
+    integrating third-party websites into Launchpad.
+    """
+
+    def do_save(self, credentials, unique_key):
+        """Store newly-authorized credentials in the keyring."""
+        keyring.set_password(
+            'launchpadlib', unique_key, credentials.serialize())
+
+    def do_load(self, unique_key):
+        """Retrieve credentials from the keyring."""
+        credential_string = keyring.get_password(
+            'launchpadlib', unique_key)
+        if credential_string is not None:
+            return Credentials.from_string(credential_string)
+        return None
+
+
+class UnencryptedFileCredentialStore(CredentialStore):
+    """Store credentials unencrypted in a file on disk.
+
+    This is a good solution for scripts that need to run without any
+    user interaction.
+    """
+
+    def __init__(self, filename, credential_save_failed=None):
+        super(UnencryptedFileCredentialStore, self).__init__(
+            credential_save_failed)
+        self.filename = filename
+
+    def do_save(self, credentials, unique_key):
+        """Save the credentials to disk."""
+        credentials.save_to_path(self.filename)
+
+    def do_load(self, unique_key):
+        """Load the credentials from disk."""
+        if (os.path.exists(self.filename)
+            and not os.stat(self.filename)[stat.ST_SIZE] == 0):
+            return Credentials.load_from_path(self.filename)
+        return None
+
+
 class RequestTokenAuthorizationEngine(object):
     """The superclass of all request token authorizers.
 
@@ -219,18 +335,12 @@
     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, service_root, application_name=None,
-                 consumer_name=None, credential_save_failed=None,
-                 allow_access_levels=None):
+                 consumer_name=None, allow_access_levels=None):
         """Base class initialization.
 
         :param service_root: The root of the Launchpad instance being
@@ -250,12 +360,6 @@
             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
@@ -272,7 +376,8 @@
         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.")
+                "consumer_name. (You provided %r and %r.)" % (
+                    application_name, consumer_name))
 
         if consumer_name is None:
             # System-wide integration. Create a system-wide consumer
@@ -290,7 +395,11 @@
         self.application_name = application_name
 
         self.allow_access_levels = allow_access_levels or []
-        self.credential_save_failed = credential_save_failed
+
+    @property
+    def unique_consumer_id(self):
+        """Return a string identifying this consumer on this host."""
+        return self.consumer.key + '@' + self.service_root
 
     def authorization_url(self, request_token):
         """Return the authorization URL for a request token.
@@ -307,17 +416,22 @@
                 + allow_permission.join(self.allow_access_levels))
         return urljoin(self.web_root, page)
 
-    def __call__(self, credentials):
+    def __call__(self, credentials, credential_store):
         """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.
+        If the credential store runs into a problem storing the
+        credential locally, the `credential_save_failed` callback will
+        be invoked. The callback 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.
 
+        :param credential_store: A `CredentialStore` object. If the
+            end-user authorizes the credentials, they will be
+            persisted locally using this object.
+
         :return: If the credentials are successfully authorized, the
             return value is the `Credentials` object originally passed
             in. Otherwise the return value is None.
@@ -328,13 +442,8 @@
         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()
+        # save() invokes the callback on failure.
+        credential_store.save(credentials, self.unique_consumer_id)
         return credentials
 
     def get_request_token(self, credentials):
@@ -363,43 +472,6 @@
         """
         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, the only) request token authorizer.
 
=== modified file 'src/launchpadlib/launchpad.py'
--- src/launchpadlib/launchpad.py	2010-12-16 21:47:02 +0000
+++ src/launchpadlib/launchpad.py	2011-01-04 19:04:27 +0000
@@ -22,24 +22,23 @@
     ]
 
 import os
-import socket
-import stat
 import urlparse
+import warnings
 
 from lazr.restfulclient.resource import (
     CollectionWithKeyBasedLookup,
-    HostedFile,
-    ScalarValue,
+    HostedFile,           # Re-import for client convenience
+    ScalarValue,          # Re-import for client convenience
     ServiceRoot,
     )
 from lazr.restfulclient._browser import RestfulHttp
 from launchpadlib.credentials import (
     AccessToken,
     AnonymousAccessToken,
-    AuthorizeRequestTokenWithBrowser,
     Consumer,
     Credentials,
-    SystemWideConsumer,
+    KeyringCredentialStore,
+    UnencryptedFileCredentialStore,
     )
 from launchpadlib import uris
 
@@ -135,7 +134,8 @@
             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)
+            self.authorization_engine(
+                self.launchpad.credentials, self.launchpad.credential_store)
             # Retry the request with the new credentials.
             return self._request(*args)
         return response, content
@@ -160,7 +160,7 @@
     RESOURCE_TYPE_CLASSES.update(ServiceRoot.RESOURCE_TYPE_CLASSES)
 
     def __init__(self, credentials, authorization_engine,
-                 service_root=uris.STAGING_SERVICE_ROOT,
+                 credential_store, service_root=uris.STAGING_SERVICE_ROOT,
                  cache=None, timeout=None, proxy_info=None,
                  version=DEFAULT_VERSION):
         """Root access to the Launchpad API.
@@ -186,6 +186,8 @@
                      "the version name from the root URI." % version)
             raise ValueError(error)
 
+        self.credential_store = credential_store
+
         # 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.
@@ -204,18 +206,27 @@
         return AuthorizeRequestTokenWithBrowser(*args)
 
     @classmethod
+    def credential_store_factory(cls, credential_save_failed):
+        return KeyringCredentialStore(credential_save_failed)
+
+    @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):
+              max_failed_attempts=None, credential_store=None,
+              credential_save_failed=None, version=DEFAULT_VERSION):
         """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
         method can be used to quickly log into the service root.
 
+        This method is deprecated as of launchpadlib version
+        1.9.0. You should use Launchpad.login_anonymously() for
+        anonymous access, and Launchpad.login_with() for all other
+        purposes.
+
         :param consumer_name: the application name.
         :type consumer_name: string
         :param token_string: the access token, as appropriate for the
@@ -237,36 +248,33 @@
         :return: The web service root
         :rtype: `Launchpad`
         """
+        cls._warn_of_deprecated_login_method("login")
         access_token = AccessToken(token_string, access_secret)
         credentials = Credentials(
             consumer_name=consumer_name, access_token=access_token)
         if authorization_engine is None:
             authorization_engine = cls.authorization_engine_factory(
-                service_root, None, consumer_name, allow_access_levels,
+                service_root, consumer_name, allow_access_levels)
+        if credential_store is None:
+            credential_store = cls.credential_store_factory(
                 credential_save_failed)
-        return cls(credentials, authorization_engine, service_root, cache,
-                   timeout, proxy_info, version)
+        return cls(credentials, authorization_engine, credential_store,
+                   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,
                             authorization_engine=None, allow_access_levels=[],
-                            max_failed_attempts=None,
+                            max_failed_attempts=None, credential_store=None,
                             credential_save_failed=None,
                             version=DEFAULT_VERSION):
         """Get credentials from Launchpad and log into the service root.
 
-        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()`.
+        This method is deprecated as of launchpadlib version
+        1.9.0. You should use Launchpad.login_anonymously() for
+        anonymous access and Launchpad.login_with() for all other
+        purposes.
 
         :param consumer_name: Either a consumer name, as appropriate for
             the `Consumer` constructor, or a premade Consumer object.
@@ -279,11 +287,28 @@
             `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`
         """
+        cls._warn_of_deprecated_login_method("get_token_and_login")
+        return cls._authorize_token_and_login(
+            consumer_name, service_root, cache, timeout, proxy_info,
+            authorization_engine, allow_access_levels,
+            credential_store, credential_save_failed, version)
+
+    @classmethod
+    def _authorize_token_and_login(
+        cls, consumer_name, service_root, cache, timeout, proxy_info,
+        authorization_engine, allow_access_levels, credential_store,
+        credential_save_failed, version):
+        """Authorize a request token. Log in with the resulting access token.
+
+        This is the private, non-deprecated implementation of the
+        deprecated method get_token_and_login(). Once
+        get_token_and_login() is removed, this code can be streamlined
+        and moved into its other call site, login_with().
+        """
+
         if isinstance(consumer_name, Consumer):
             # Create the credentials with no Consumer, then set its .consumer
             # property directly.
@@ -295,11 +320,23 @@
             credentials = Credentials(consumer_name)
         if authorization_engine is None:
             authorization_engine = cls.authorization_engine_factory(
-                service_root, None, consumer_name, allow_access_levels,
+                service_root, consumer_name, None, allow_access_levels)
+        if credential_store is None:
+            credential_store = cls.credential_store_factory(
                 credential_save_failed)
-        credentials = authorization_engine(credentials)
-        return cls(credentials, authorization_engine, service_root, cache,
-                   timeout, proxy_info, version)
+        else:
+            # A credential store was passed in, so we won't be using
+            # any provided value for credential_save_failed. But at
+            # least make sure we weren't given a conflicting value,
+            # since that makes the calling code look confusing.
+            cls._assert_login_argument_consistency(
+                "credential_save_failed", credential_save_failed,
+                credential_store.credential_save_failed,
+                "credential_store")
+
+        credentials = authorization_engine(credentials, credential_store)
+        return cls(credentials, authorization_engine, credential_store,
+                   service_root, cache, timeout, proxy_info, version)
 
     @classmethod
     def login_anonymously(
@@ -311,7 +348,7 @@
          service_root_dir) = cls._get_paths(service_root, launchpadlib_dir)
         token = AnonymousAccessToken()
         credentials = Credentials(consumer_name, access_token=token)
-        return cls(credentials, None, service_root=service_root,
+        return cls(credentials, None, None, service_root=service_root,
                    cache=cache_path, timeout=timeout, proxy_info=proxy_info,
                    version=version)
 
@@ -322,23 +359,36 @@
                    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.
+                   credential_save_failed=None, credential_store=None):
+        """Log in to Launchpad, possibly acquiring and storing credentials.
 
-        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
+        Use this method to get a `Launchpad` object. 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.
+        stored securely: in the GNOME keyring, the KDE Wallet, or in
+        an encrypted file on disk.
+
+        The next time your program (or any other program run by that
+        user on the same computer) invokes this method, the end-user
+        will be prompted to unlock their keyring (or equivalent), and
+        the credential will be retrieved from local storage and
+        reused.
+
+        You can customize this behavior in three ways:
+
+        1. Pass in a filename to `credentials_file`. The end-user's
+           credential will be written to that file, and on subsequent
+           runs read from that file.
+
+        2. Subclass `CredentialStore` and pass in an instance of the
+           subclass as `credential_store`. This lets you change how
+           the end-user's credential is stored and retrieved locally.
+
+        3. Subclass `RequestTokenAuthorizationEngine` and pass in an
+           instance of the subclass as `authorization_engine`. This
+           lets you change change what happens when the end-user needs
+           to authorize the Launchpad credential.
 
         :param application_name: The application name. This is *not*
             the OAuth consumer name. Unless a consumer_name is also
@@ -356,25 +406,14 @@
            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.
+        :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. By default, launchpadlib will open the end-user's
+            web browser to have them authorize the request token.
         :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.
 
@@ -386,8 +425,32 @@
 
         :type allow_access_levels: list of strings
 
-        :param credentials_file: ignored, only here for backward compatability
-        :type credentials_file: string
+        :param max_failed_attempts: Ignored; only present for
+            backwards compatibility.
+
+        :param credentials_file: The path to a file in which to store
+            this user's OAuth access token.
+
+        :param version: The version of the Launchpad web service to use.
+
+        :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 credential_save_failed: a callback that is called upon
+           a failure to save the credentials locally. This argument is
+           used to construct the default `credential_store`, so if
+           you pass in your own `credential_store` any value for
+           this argument will be ignored.
+        :type credential_save_failed: A callable
+
+        :param credential_store: A strategy for storing an OAuth
+            access token locally. By default, tokens are stored in the
+            GNOME keyring (or equivalent). If `credentials_file` is
+            provided, then tokens are stored unencrypted in that file.
+        :type credential_store: `CredentialStore`
 
         :return: A web service root authorized as the end-user.
         :rtype: `Launchpad`
@@ -402,69 +465,111 @@
                 "At least one of application_name, consumer_name, or "
                 "authorization_engine must be provided.")
 
+        if credentials_file is not None and credential_store is not None:
+            raise ValueError(
+                "At most one of credentials_file and credential_store "
+                "must be provided.")
+
+        if credential_store is None:
+            if credentials_file is not None:
+                # The end-user wants credentials stored in an
+                # unencrypted file.
+                credential_store = UnencryptedFileCredentialStore(
+                    credentials_file, credential_save_failed)
+            else:
+                credential_store = cls.credential_store_factory(
+                    credential_save_failed)
+        else:
+            # A credential store was passed in, so we won't be using
+            # any provided value for credential_save_failed. But at
+            # least make sure we weren't given a conflicting value,
+            # since that makes the calling code look confusing.
+            cls._assert_login_argument_consistency(
+                'credential_save_failed', credential_save_failed,
+                credential_store.credential_save_failed,
+                "credential_store")
+            credential_store = credential_store
+
         if authorization_engine is None:
             authorization_engine = cls.authorization_engine_factory(
                 service_root, application_name, consumer_name,
-                credential_save_failed, allow_access_levels)
+                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(
+            cls._assert_login_argument_consistency(
                 "application_name", application_name,
                 authorization_engine.application_name)
 
-            cls._assert_login_with_argument_consistency(
+            cls._assert_login_argument_consistency(
                 "consumer_name", consumer_name,
                 authorization_engine.consumer.key)
 
-            cls._assert_login_with_argument_consistency(
+            cls._assert_login_argument_consistency(
                 "allow_access_levels", allow_access_levels,
                 authorization_engine.allow_access_levels)
 
-        credentials = authorization_engine.retrieve_credentials_from_local_store()
+        credentials = credential_store.load(
+            authorization_engine.unique_consumer_id)
 
         if credentials is None:
             # Credentials were not found in the local store. Go
             # through the authorization process.
-            launchpad = cls.get_token_and_login(
-                authorization_engine.consumer, service_root=service_root,
-                cache=cache_path, timeout=timeout, proxy_info=proxy_info,
-                authorization_engine=authorization_engine,
-                version=version)
+            launchpad = cls._authorize_token_and_login(
+                authorization_engine.consumer, service_root,
+                cache_path, timeout, proxy_info, authorization_engine,
+                allow_access_levels, credential_store,
+                credential_save_failed, version)
         else:
+            # The application name wasn't stored locally, because in a
+            # desktop integration scenario, a single set of
+            # credentials may be shared by many applications. We need
+            # to set the application name for this specific instance
+            # of the credentials.
+            credentials.consumer.application_name = (
+                authorization_engine.application_name)
+
             # Credentials were found in the local store. Create a
             # Launchpad object.
             launchpad = cls(
-                credentials, authorization_engine, service_root=service_root,
-                cache=cache_path, timeout=timeout, proxy_info=proxy_info,
-                version=version)
+                credentials, authorization_engine, credential_store,
+                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.
+    def _warn_of_deprecated_login_method(cls, name):
+        warnings.warn(
+            ("The Launchpad.%s() method is deprecated. You should use "
+             "Launchpad.login_anonymous() for anonymous access and "
+             "Launchpad.login_with() for all other purposes.") % name,
+            DeprecationWarning)
+
+    @classmethod
+    def _assert_login_argument_consistency(
+        cls, argument_name, argument_value, object_value,
+        object_name="authorization engine"):
+        """Helper to find conflicting values passed into the login methods.
+
+        Many of the arguments to login_with are used to build other
+        objects--the authorization engine or the credential store. If
+        these objects are provided directly, many of the arguments
+        become redundant. We'll allow redundant arguments through, but
+        if a argument *conflicts* with the corresponding value in the
+        provided object, 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):
+            "(%r passed in, versus %r in %s). "
+            "You don't need to pass in %s if you pass in %s, "
+            "so just omit that argument.")
+        if (argument_value is not None and argument_value != object_value):
             raise ValueError(inconsistent_value_message % (
-                argument_name, argument_value, authorization_engine_value,
-                argument_name))
+                argument_name, argument_value, object_value,
+                object_name, argument_name, object_name))
 
 
     @classmethod
 
=== modified file 'src/launchpadlib/testing/helpers.py'
--- src/launchpadlib/testing/helpers.py	2010-12-16 21:47:02 +0000
+++ src/launchpadlib/testing/helpers.py	2011-01-04 19:04:27 +0000
@@ -21,6 +21,10 @@
 
 __metaclass__ = type
 __all__ = [
+    'BadSaveKeyring',
+    'fake_keyring',
+    'FauxSocketModule',
+    'InMemoryKeyring',
     'NoNetworkAuthorizationEngine',
     'NoNetworkLaunchpad',
     'TestableLaunchpad',
@@ -29,13 +33,12 @@
     'salgado_with_full_permissions',
     ]
 
-import simplejson
+from contextlib import contextmanager
 
+import launchpadlib
 from launchpadlib.launchpad import Launchpad
 from launchpadlib.credentials import (
     AccessToken,
-    AuthorizeRequestTokenWithBrowser,
-    Credentials,
     RequestTokenAuthorizationEngine,
     )
 
@@ -91,10 +94,11 @@
     It can't be used to interact with the API.
     """
 
-    def __init__(self, credentials, authorization_engine, service_root,
-                 cache, timeout, proxy_info, version):
+    def __init__(self, credentials, authorization_engine, credential_store,
+                 service_root, cache, timeout, proxy_info, version):
         self.credentials = credentials
         self.authorization_engine = authorization_engine
+        self.credential_store = credential_store
         self.passed_in_args = dict(
             service_root=service_root, cache=cache, timeout=timeout,
             proxy_info=proxy_info, version=version)
@@ -104,8 +108,51 @@
         return NoNetworkAuthorizationEngine(*args)
 
 
+@contextmanager
+def fake_keyring(fake):
+    original_keyring = launchpadlib.credentials.keyring
+    launchpadlib.credentials.keyring = fake
+    try:
+        yield
+    finally:
+        launchpadlib.credentials.keyring = original_keyring
+
+
+class FauxSocketModule:
+    """A socket module replacement that provides a fake hostname."""
+
+    def gethostname(self):
+        return 'HOSTNAME'
+
+
+class BadSaveKeyring:
+    """A keyring that generates errors when saving passwords."""
+
+    def get_password(self, service, username):
+        return None
+
+    def set_password(self, service, username, password):
+        raise RuntimeError
+
+
+class InMemoryKeyring:
+    """A keyring that saves passwords only in memory."""
+
+    def __init__(self):
+        self.data = {}
+
+    def set_password(self, service, username, password):
+        self.data[service, username] = password
+
+    def get_password(self, service, username):
+        return self.data.get((service, username))
+
+
 class KnownTokens:
-    """Known access token/secret combinations."""
+    """Known access token/secret combinations.
+
+    XXX This will need to be redone when integrating into Launchpad.
+    """
 
     def __init__(self, token_string, access_secret):
         self.token_string = token_string
 
=== added file 'src/launchpadlib/tests/test_credential_store.py'
--- src/launchpadlib/tests/test_credential_store.py	1970-01-01 00:00:00 +0000
+++ src/launchpadlib/tests/test_credential_store.py	2011-01-04 19:04:27 +0000
@@ -0,0 +1,138 @@
+# 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 credential store classes."""
+
+import os
+import tempfile
+import unittest
+
+from launchpadlib.testing.helpers import (
+    fake_keyring,
+    InMemoryKeyring,
+)
+
+from launchpadlib.credentials import (
+    AccessToken,
+    Credentials,
+    KeyringCredentialStore,
+    UnencryptedFileCredentialStore,
+)
+
+
+class CredentialStoreTestCase(unittest.TestCase):
+
+    def make_credential(self, consumer_key):
+        """Helper method to make a fake credential."""
+        return Credentials(
+            "app name", consumer_secret='consumer_secret:42',
+            access_token=AccessToken(consumer_key, 'access_secret:168'))
+
+
+class TestUnencryptedFileCredentialStore(CredentialStoreTestCase):
+    """Tests for the UnencryptedFileCredentialStore class."""
+
+    def setUp(self):
+        ignore, self.filename = tempfile.mkstemp()
+        self.store = UnencryptedFileCredentialStore(self.filename)
+
+    def tearDown(self):
+        if os.path.exists(self.filename):
+            os.remove(self.filename)
+
+    def test_save_and_load(self):
+        # Make sure you can save and load credentials to a file.
+        credential = self.make_credential("consumer key")
+        self.store.save(credential, "unique key")
+        credential2 = self.store.load("unique key")
+        self.assertEquals(credential.consumer.key, credential2.consumer.key)
+
+    def test_unique_id_doesnt_matter(self):
+        # If a file contains a credential, that credential will be
+        # accessed no matter what unique ID you specify.
+        credential = self.make_credential("consumer key")
+        self.store.save(credential, "some key")
+        credential2 = self.store.load("some other key")
+        self.assertEquals(credential.consumer.key, credential2.consumer.key)
+
+    def test_file_only_contains_one_credential(self):
+        # A credential file may contain only one credential. If you
+        # write two credentials with different unique IDs to the same
+        # file, the first credential will be overwritten with the
+        # second.
+        credential1 = self.make_credential("consumer key")
+        credential2 = self.make_credential("consumer key2")
+        self.store.save(credential1, "unique key 1")
+        self.store.save(credential1, "unique key 2")
+        loaded = self.store.load("unique key 1")
+        self.assertEquals(loaded.consumer.key, credential2.consumer.key)
+
+
+class TestKeyringCredentialStore(CredentialStoreTestCase):
+    """Tests for the KeyringCredentialStore class."""
+
+    def setUp(self):
+        self.keyring = InMemoryKeyring()
+        self.store = KeyringCredentialStore()
+
+    def test_save_and_load(self):
+        # Make sure you can save and load credentials to a keyring.
+        with fake_keyring(self.keyring):
+            credential = self.make_credential("consumer key")
+            self.store.save(credential, "unique key")
+            credential2 = self.store.load("unique key")
+            self.assertEquals(
+                credential.consumer.key, credential2.consumer.key)
+
+    def test_lookup_by_unique_key(self):
+        # Credentials in the keyring are looked up by the unique ID
+        # under which they were stored.
+        with fake_keyring(self.keyring):
+            credential1 = self.make_credential("consumer key1")
+            self.store.save(credential1, "key 1")
+
+            credential2 = self.make_credential("consumer key2")
+            self.store.save(credential2, "key 2")
+
+            loaded1 = self.store.load("key 1")
+            self.assertEquals(
+                credential1.consumer.key, loaded1.consumer.key)
+
+            loaded2 = self.store.load("key 2")
+            self.assertEquals(
+                credential2.consumer.key, loaded2.consumer.key)
+
+    def test_reused_unique_id_overwrites_old_credential(self):
+        # Writing a credential to the keyring with a given unique ID
+        # will overwrite any credential stored under that ID.
+
+        with fake_keyring(self.keyring):
+            credential1 = self.make_credential("consumer key1")
+            self.store.save(credential1, "the only key")
+
+            credential2 = self.make_credential("consumer key2")
+            self.store.save(credential2, "the only key")
+
+            loaded = self.store.load("the only key")
+            self.assertEquals(
+                credential2.consumer.key, loaded.consumer.key)
+
+    def test_bad_unique_id_returns_none(self):
+        # Trying to load a credential without providing a good unique
+        # ID will get you None.
+        with fake_keyring(self.keyring):
+            self.assertEquals(None, self.store.load("no such key"))
+
 
=== modified file 'src/launchpadlib/tests/test_http.py'
--- src/launchpadlib/tests/test_http.py	2010-12-20 17:44:36 +0000
+++ src/launchpadlib/tests/test_http.py	2011-01-04 19:04:27 +0000
@@ -71,14 +71,15 @@
         :param responses: A list of HttpResponse objects to use
             in response to requests.
         """
+        super(SimulatedResponsesHttp, self).__init__(*args)
         self.sent_responses = []
         self.unsent_responses = responses
-        super(SimulatedResponsesHttp, self).__init__(*args)
+        self.cache = None
 
     def _request(self, *args):
         response = self.unsent_responses.popleft()
         self.sent_responses.append(response)
-        return self.retry_on_bad_token(response, response.content)
+        return self.retry_on_bad_token(response, response.content, *args)
 
 
 class SimulatedResponsesLaunchpad(Launchpad):
 
=== modified file 'src/launchpadlib/tests/test_launchpad.py'
--- src/launchpadlib/tests/test_launchpad.py	2010-12-20 17:44:36 +0000
+++ src/launchpadlib/tests/test_launchpad.py	2011-01-04 19:04:27 +0000
@@ -23,9 +23,8 @@
 import socket
 import stat
 import tempfile
-import textwrap
 import unittest
-from contextlib import contextmanager
+import warnings
 
 from lazr.restfulclient.resource import ServiceRoot
 
@@ -38,9 +37,17 @@
 import launchpadlib.launchpad
 from launchpadlib.launchpad import Launchpad
 from launchpadlib.testing.helpers import (
+    BadSaveKeyring,
+    fake_keyring,
+    FauxSocketModule,
+    InMemoryKeyring,
     NoNetworkAuthorizationEngine,
     NoNetworkLaunchpad,
     )
+from launchpadlib.credentials import (
+    KeyringCredentialStore,
+    UnencryptedFileCredentialStore,
+    )
 
 # A dummy service root for use in tests
 SERVICE_ROOT = "http://api.example.com/"
@@ -112,7 +119,7 @@
         version = "version-foo"
         root = uris.service_roots['staging'] + version
         try:
-            Launchpad(None, None, service_root=root, version=version)
+            Launchpad(None, 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 "
@@ -124,30 +131,17 @@
         # Make sure the problematic URL is caught even if it has a
         # slash on the end.
         root += '/'
-        self.assertRaises(ValueError, Launchpad, None, None,
+        self.assertRaises(ValueError, Launchpad, None, 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, None,
+        self.assertRaises(ValueError, Launchpad, None, None, None,
                           service_root=root)
 
 
-class InMemoryKeyring:
-    """A keyring that saves passwords only in memory."""
-
-    def __init__(self):
-        self.data = {}
-
-    def set_password(self, service, username, password):
-        self.data[service, username] = password
-
-    def get_password(self, service, username):
-        return self.data.get((service, username))
-
-
 class TestRequestTokenAuthorizationEngine(unittest.TestCase):
     """Tests for the RequestTokenAuthorizationEngine class."""
 
@@ -174,8 +168,33 @@
             SERVICE_ROOT, application_name='name', consumer_name='name')
 
 
-class TestLaunchpadLoginWith(unittest.TestCase):
-    """Tests for Launchpad.login_with()."""
+class TestLaunchpadLoginWithCredentialsFile(unittest.TestCase):
+    """Tests for Launchpad.login_with() with a credentials file."""
+
+    def test_filename(self):
+        ignore, filename = tempfile.mkstemp()
+        launchpad = NoNetworkLaunchpad.login_with(
+            application_name='not important', credentials_file=filename)
+
+        # The credentials are stored unencrypted in the file you
+        # specify.
+        credentials = Credentials.load_from_path(filename)
+        self.assertEquals(credentials.consumer.key,
+                          launchpad.credentials.consumer.key)
+        os.remove(filename)
+
+    def test_cannot_specify_both_filename_and_store(self):
+        ignore, filename = tempfile.mkstemp()
+        store = KeyringCredentialStore()
+        self.assertRaises(
+            ValueError, NoNetworkLaunchpad.login_with,
+            application_name='not important', credentials_file=filename,
+            credential_store=store)
+        os.remove(filename)
+
+
+class KeyringTest(unittest.TestCase):
+    """Base class for tests that use the keyring."""
 
     def setUp(self):
         # For these tests we want to disable retrieving or storing credentials
@@ -183,30 +202,28 @@
         # instaed.
         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.credentials.keyring = self._saved_keyring
+
+
+class TestLaunchpadLoginWith(KeyringTest):
+    """Tests for Launchpad.login_with()."""
+
+    def setUp(self):
+        super(TestLaunchpadLoginWith, self).setUp()
+        self.temp_dir = tempfile.mkdtemp()
+
+    def tearDown(self):
+        super(TestLaunchpadLoginWith, self).tearDown()
         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.
+        # cache for all service roots are stored.
         launchpadlib_dir = os.path.join(self.temp_dir, 'launchpadlib')
-        launchpad = NoNetworkLaunchpad.login_with(
+        NoNetworkLaunchpad.login_with(
             'not important', service_root=SERVICE_ROOT,
             launchpadlib_dir=launchpadlib_dir)
         # The 'launchpadlib' dir got created.
@@ -233,7 +250,7 @@
         statinfo = os.stat(launchpadlib_dir)
         mode = stat.S_IMODE(statinfo.st_mode)
         self.assertNotEqual(mode, stat.S_IWRITE | stat.S_IREAD | stat.S_IEXEC)
-        launchpad = NoNetworkLaunchpad.login_with(
+        NoNetworkLaunchpad.login_with(
             'not important', service_root=SERVICE_ROOT,
             launchpadlib_dir=launchpadlib_dir)
         # Verify the mode has been changed to 0700
@@ -243,7 +260,7 @@
 
     def test_dirs_created_are_secure(self):
         launchpadlib_dir = os.path.join(self.temp_dir, 'launchpadlib')
-        launchpad = NoNetworkLaunchpad.login_with(
+        NoNetworkLaunchpad.login_with(
             'not important', service_root=SERVICE_ROOT,
             launchpadlib_dir=launchpadlib_dir)
         self.assertTrue(os.path.isdir(launchpadlib_dir))
@@ -300,8 +317,7 @@
         # token.
         engine = NoNetworkAuthorizationEngine(
             SERVICE_ROOT, 'application name')
-        launchpad = NoNetworkLaunchpad.login_with(
-            authorization_engine=engine)
+        NoNetworkLaunchpad.login_with(authorization_engine=engine)
         self.assertEquals(engine.request_tokens_obtained, 1)
         self.assertEquals(engine.access_tokens_obtained, 1)
 
@@ -311,9 +327,13 @@
         self.assertRaises(ValueError, NoNetworkLaunchpad.login_with)
 
     def test_application_name_identifies_app(self):
+        # If you pass in application_name, that's good enough to identify
+        # your application.
         NoNetworkLaunchpad.login_with(application_name="name")
 
     def test_consumer_name_identifies_app(self):
+        # If you pass in consumer_name, that's good enough to identify
+        # your application.
         NoNetworkLaunchpad.login_with(consumer_name="name")
 
     def test_inconsistent_application_name_rejected(self):
@@ -344,6 +364,19 @@
                           allow_access_levels=['BAR'],
                           authorization_engine=engine)
 
+    def test_inconsistent_credential_save_failed(self):
+        # Catch an attempt to specify inconsistent callbacks for
+        # credential save failure.
+        def callback1():
+            pass
+        store = KeyringCredentialStore(credential_save_failed=callback1)
+
+        def callback2():
+            pass
+        self.assertRaises(ValueError, NoNetworkLaunchpad.login_with,
+                          "app name", credential_store=store,
+                          credential_save_failed=callback2)
+
     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
@@ -472,30 +505,32 @@
         self.assertRaises(
             ValueError, NoNetworkLaunchpad.login_with, 'app name', 'foo')
 
-
-class FauxSocketModule:
-    """A socket module replacement that provides a fake hostname."""
-
-    def gethostname(self):
-        return 'HOSTNAME'
-
-
-class BadSaveKeyring:
-    """A keyring that generates errors when saving passwords."""
-
-    def get_password(self, service, username):
-        return None
-
-    def set_password(self, service, username, password):
-        raise RuntimeError
-
-
-@contextmanager
-def fake_keyring(fake):
-    original_keyring = launchpadlib.credentials.keyring
-    launchpadlib.credentials.keyring = fake
-    yield
-    launchpadlib.credentials.keyring = original_keyring
+    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.
+        NoNetworkLaunchpad.login_with(
+            'not important', max_failed_attempts=5)
+
+
+class TestDeprecatedLoginMethods(KeyringTest):
+    """Make sure the deprecated login methods still work."""
+
+    def test_login_is_deprecated(self):
+        # login() works but triggers a deprecation warning.
+        with warnings.catch_warnings(record=True) as caught:
+            warnings.simplefilter("always")
+            launchpad = NoNetworkLaunchpad.login(
+                'consumer', 'token', 'secret')
+            self.assertEquals(len(caught), 1)
+            self.assertEquals(caught[0].category, DeprecationWarning)
+
+    def test_get_token_and_login_is_deprecated(self):
+        # get_token_and_login() works but triggers a deprecation warning.
+        with warnings.catch_warnings(record=True) as caught:
+            warnings.simplefilter("always")
+            launchpad = NoNetworkLaunchpad.get_token_and_login('consumer')
+            self.assertEquals(len(caught), 1)
+            self.assertEquals(caught[0].category, DeprecationWarning)
 
 
 class TestCredenitialSaveFailedCallback(unittest.TestCase):
@@ -578,7 +613,6 @@
         keyring = InMemoryKeyring()
         # Be paranoid about the keyring starting out empty.
         assert not keyring.data, 'oops, a fresh keyring has data in it'
-        service_root = 'http://api.example.com/'
         with fake_keyring(keyring):
             # Create stored credentials for the same application but against
             # two different sites (service roots).