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

[Leonard Richardson]

This branch fixes bug 71403 with two little bits of code:

1. If you call login_with() without specifying a service_root, you get 'staging', but you also get a warning. This lets you avoid mistakes in your code, but makes it obvious what you need to do if you want your changes to persist.

2. If you call login_anonymously() without specifying a service_root, you get 'production'.

Martin, let me know if you like this solution.

[Martin Pool (mbp)]

Hi Leonard, thanks for picking this up.

Can we think of any case in which the user would thank us for defaulting to talking to staging? I think the only case would be if they start up a client and make a lot of changes just assuming it has no effect. I don't know why someone would think that; fairly obviously the library is for talking to Launchpad and has the effect of manipulating objects in Launchpad.

If somebody is randomly poking things, they'll be doing so under their own account and they'll suffer the consequences.

If we default to staging then the first thing people are likely to do is to override it to talk to the real site. People want to write code that actually manipulates Launchpad. If they have bugs, those bugs are going to affect the real lp.

Consistent behaviour between anonymous and authenticated access is desirable, all else being equal.

So, I think it should just default to production.

We can look for precedents in other libraries. I've never encountered one that doesn't talk to the production site by default.

[Leonard Richardson (leonardr)]

I guess it depends on the development style. When I write a launchpadlib script, I run it against staging until it's debugged, and the last thing I do is switch it to production. This branch would kind of annoy me, but I'd work around it by explicitly passing in service_root='staging' to suppress the warning. If someone else's style is to switching to production immediately, under this branch they'll at least be reminded that they need to do that.

"If somebody is randomly poking things, they'll be doing so under their own account and they'll suffer the consequences." "If they have bugs, those bugs are going to affect the real lp." You're saying these things in a very matter-of-fact way, but it doesn't have to be this way. These are reasons for not defaulting to staging.

I'm not aware of any service-specific clients that can talk to staging servers *at all*, but I'm not up to date on these things. Most web service providers don't publish their staging servers.

I'm happy to put this up for a vote on launchpad-users.

[Robert Collins (lifeless)]

I think gathering some user input would be good; perhaps a -users
thread as you suggest.

[Martin Pool (mbp)]

On 9 February 2011 08:19, Robert Collins <email address hidden> wrote:
> I think gathering some user input would be good; perhaps a -users
> thread as you suggest.

+1, maybe also forward it to canonical-tech.

[Martin Pool (mbp)]

We did have a thread, in which there was a mix of people agreeing it was confusing or annoying, and people saying they think it is nice to have a "safe" mode by default.

Also, since this was first proposed, staging has been down a lot (whereas lpnet has generally been quite reliable), and it seems to me it is definitely moving towards being a non-production-quality internal-use system, and not something users ought to rely on.

(In the interim, Leonard has moved on to other projects and I'm not sure he'll chase this up, but I'm going to comment here for other people who might look at the bug or mp. I may pick this up myself eventually.)

So my overall vote, for what it's worth, is to reject this and to make lplib actually default to the real service. Clients must write code that is adequately safe when it's actually used.

review: Disapprove

[Martin Pool (mbp)]

For the sake of getting this off my queue and making a decision, I'm going to reject it. If someone else wants to pick it up, or even repropose this branch, please do. I hope we still fix this bug.

Unmerged revisions

117. By Leonard Richardson on 2011-02-07

Added NEWS.

116. By Leonard Richardson on 2011-02-07

Initial implementation.

=== modified file 'src/launchpadlib/NEWS.txt'
--- src/launchpadlib/NEWS.txt	2011-01-18 21:57:09 +0000
+++ src/launchpadlib/NEWS.txt	2011-02-07 20:53:52 +0000
@@ -2,6 +2,19 @@
 NEWS for launchpadlib
 =====================
 
+1.9.5 (2010-02-07)
+==================
+
+- Fixed a bug that prevented the deprecated get_token_and_login code
+  from working, and required that users of get_token_and_login get a
+  new token on every usage.
+
+- Added a warning to users when their code runs against staging and
+  they might not realize it.
+
+- By default, anonymous read-only access runs against the production
+  server, since there's no chance of the client making mistakes.
+
 1.9.4 (2010-01-18)
 ==================
 
 
=== modified file 'src/launchpadlib/__init__.py'
--- src/launchpadlib/__init__.py	2011-01-18 22:33:53 +0000
+++ src/launchpadlib/__init__.py	2011-02-07 20:53:52 +0000
@@ -14,4 +14,4 @@
 # You should have received a copy of the GNU Lesser General Public License
 # along with launchpadlib. If not, see <http://www.gnu.org/licenses/>.
 
-__version__ = '1.9.4'
+__version__ = '1.9.5'
 
=== modified file 'src/launchpadlib/launchpad.py'
--- src/launchpadlib/launchpad.py	2011-01-18 21:57:09 +0000
+++ src/launchpadlib/launchpad.py	2011-02-07 20:53:52 +0000
@@ -31,6 +31,7 @@
     ScalarValue,          # Re-import for client convenience
     ServiceRoot,
     )
+from lazr.restfulclient.authorize.oauth import SystemWideConsumer
 from lazr.restfulclient._browser import RestfulHttp
 from launchpadlib.credentials import (
     AccessToken,
@@ -309,16 +310,18 @@
         get_token_and_login() is removed, this code can be streamlined
         and moved into its other call site, login_with().
         """
-
         if isinstance(consumer_name, Consumer):
-            # Create the credentials with no Consumer, then set its .consumer
-            # property directly.
-            credentials = Credentials(None)
-            credentials.consumer = consumer_name
+            consumer = consumer_name
         else:
-            # Have the Credentials constructor create the Consumer
-            # automatically.
-            credentials = Credentials(consumer_name)
+            # Create a system-wide consumer. lazr.restfulclient won't
+            # do this automatically, but launchpadlib's default is to
+            # do a desktop-wide integration.
+            consumer = SystemWideConsumer(consumer_name)
+
+        # Create the credentials with no Consumer, then set its .consumer
+        # property directly.
+        credentials = Credentials(None)
+        credentials.consumer = consumer
         if authorization_engine is None:
             authorization_engine = cls.authorization_engine_factory(
                 service_root, consumer_name, None, allow_access_levels)
@@ -335,13 +338,30 @@
                 credential_store.credential_save_failed,
                 "credential_store")
 
-        credentials = authorization_engine(credentials, credential_store)
+        # Try to get the credentials out of the credential store.
+        cached_credentials = credential_store.load(
+            authorization_engine.unique_consumer_id)
+        if cached_credentials is None:
+            # They're not there. Acquire new credentials using the
+            # authorization engine.
+            credentials = authorization_engine(credentials, credential_store)
+        else:
+            # We acquired credentials. But, the application name
+            # wasn't stored along with the credentials, because in a
+            # desktop integration scenario, a single set of
+            # credentials may be shared by many applications. We need
+            # to set the application name for this specific instance
+            # of the credentials.
+            credentials = cached_credentials
+            credentials.consumer.application_name = (
+                authorization_engine.application_name)
+
         return cls(credentials, authorization_engine, credential_store,
                    service_root, cache, timeout, proxy_info, version)
 
     @classmethod
     def login_anonymously(
-        cls, consumer_name, service_root=uris.STAGING_SERVICE_ROOT,
+        cls, consumer_name, service_root=uris.LPNET_SERVICE_ROOT,
         launchpadlib_dir=None, timeout=None, proxy_info=None,
         version=DEFAULT_VERSION):
         """Get access to Launchpad without providing any credentials."""
@@ -355,7 +375,7 @@
 
     @classmethod
     def login_with(cls, application_name=None,
-                   service_root=uris.STAGING_SERVICE_ROOT,
+                   service_root=None,
                    launchpadlib_dir=None, timeout=None, proxy_info=None,
                    authorization_engine=None, allow_access_levels=None,
                    max_failed_attempts=None, credentials_file=None,
@@ -457,6 +477,15 @@
         :rtype: `Launchpad`
 
         """
+        if service_root is None:
+            service_root = uris.STAGING_SERVICE_ROOT
+            staging_warning = (
+                "Running against the Launchpad staging server. Changes you "
+                "make will be reverted, and the data you read may be "
+                "slightly out of date. To run against production, pass "
+                'service_root="production" into login_with().')
+            warnings.warn(staging_warning)
+
         (service_root, launchpadlib_dir, cache_path,
          service_root_dir) = cls._get_paths(service_root, launchpadlib_dir)
 
@@ -513,33 +542,11 @@
                 "allow_access_levels", allow_access_levels,
                 authorization_engine.allow_access_levels)
 
-        credentials = credential_store.load(
-            authorization_engine.unique_consumer_id)
-
-        if credentials is None:
-            # Credentials were not found in the local store. Go
-            # through the authorization process.
-            launchpad = cls._authorize_token_and_login(
-                authorization_engine.consumer, service_root,
-                cache_path, timeout, proxy_info, authorization_engine,
-                allow_access_levels, credential_store,
-                credential_save_failed, version)
-        else:
-            # The application name wasn't stored locally, because in a
-            # desktop integration scenario, a single set of
-            # credentials may be shared by many applications. We need
-            # to set the application name for this specific instance
-            # of the credentials.
-            credentials.consumer.application_name = (
-                authorization_engine.application_name)
-
-            # Credentials were found in the local store. Create a
-            # Launchpad object.
-            launchpad = cls(
-                credentials, authorization_engine, credential_store,
-                service_root=service_root, cache=cache_path, timeout=timeout,
-                proxy_info=proxy_info, version=version)
-        return launchpad
+        return cls._authorize_token_and_login(
+            authorization_engine.consumer, service_root,
+            cache_path, timeout, proxy_info, authorization_engine,
+            allow_access_levels, credential_store,
+            credential_save_failed, version)
 
     @classmethod
     def _warn_of_deprecated_login_method(cls, name):