Merge lp:~leonardr/launchpadlib/improve-workflow into lp:launchpadlib

[Leonard Richardson]

This branch improves the console-based workflow for exchanging a request token for an access token.

Here's the old workflow:
1. Tab into your browser app.
2. Authorize (or don't) the launchpadlib app.
3. Tab into the console.
4. Hit Enter.

My original plan was to eliminate all but step 2. If I could somehow do this, the current launchpadlib workflow would be more or less the same as the proposed expensive workflow involving a custom desktop application and a brand new OAuth access level.

Unfortunately, a combination of problems with the webbrowser module/Firefox/the standard Ubuntu window manager foiled my plan to eliminate step 1, and Javascript security issues foiled my plan to eliminate step 3.

But, I was able to eliminate step 4, with a minor change to Launchpad (https://code.edge.launchpad.net/~leonardr/launchpad/distinguish-between-unauthorized-and-forbidden). This branch eliminates the "hit Enter" step by periodically polling Launchpad, attempting to exchange the request token for an access token. If the user has decided to authorize the app, this request returns a 200 response code with the access token information. If the user has declined to authorize the app, this request returns a 403 response code (this is the minor change to Launchpad). If the user has yet to make a decision, this request returns a 401 response code.

Since it's now possible to distinguish "I don't want you to have access to my Launchpad account" from "I haven't made a decision about giving you access", I took the opportunity to improve launchpadlib's behavior when access is denied. Previously launchpadlib just raised an unhelpful HTTPError. Now it ensures that login_with() and the like return None instead of a Launchpad object. When you call login_with(), you can check the return value and know that if it's None, the user made a decision not to give you access--you can sys.exit() or complain or whatever.

Bonus: since the console-based workflow no longer reads from stdin, it should be now possible to use it from a GUI application.

Since this branch only works on a version of Launchpad that has my branch installed, I won't be releasing a version of launchpadlib that includes this branch until my Launchpad branch is fully deployed.

[Martin Pool (mbp)]

Nice cover letter.

> Since it's now possible to distinguish "I don't want you to have access to my Launchpad account" from "I haven't made a decision about giving you access", I took the opportunity to improve launchpadlib's behavior when access is denied. Previously launchpadlib just raised an unhelpful HTTPError. Now it ensures that login_with() and the like return None instead of a Launchpad object. When you call login_with(), you can check the return value and know that if it's None, the user made a decision not to give you access--you can sys.exit() or complain or whatever.

This of course needs to go into the docs and I would say ideally to get a brief post on the blog (even just a pointer to this mp.)

I think you should raise a specific exception if the user denies access, rather than returning None:

1- If the app ignores the return code, we don't want it to just blithely continue and then cause knock-on errors. The default should be for the app to stop.
2- Returning None for errors is just bad.
3- It gives you a space to eventually possibly pass back a sensible message. (I mean I doubt the user wants to explain why, but perhaps you can also in the future say "this app is banned" or "you're creating too many tokens" etc.) I would say you should probably put the error body in to the exception, even if at the moment it's not super helpful.

+ print " (%s)" % self.authorization_url
+ print "should be opening in your browser. Use your browser to"
+ print "to authorize this program to access Launchpad on your behalf."

It would be nice to not directly print but rather pass this to a callback function so guis can hook it. It could be a followon but perhaps you could just hoist it out while you're here.

- launchpad.credentials.save_to_path(consumer_credentials_path)
- os.chmod(consumer_credentials_path, stat.S_IREAD | stat.S_IWRITE)
+ if launchpad is not None:
+ launchpad.credentials.save_to_path(consumer_credentials_path)
+ os.chmod(
+ consumer_credentials_path, stat.S_IREAD | stat.S_IWRITE)

It's always much more secure to create the path secure (by passing a third parameter to os.open) than to chmod it later: the app may be interrupted, someone may grab it in the race window, etc.

Otherwise looks good.

review: Needs Fixing

[James Westby (james-w)]

Hi,

One trick I have seen that is a bit nicer than polling is to pass a localhost
url as the OAuth callback, and then set up a listening socket at that URL, and
respond to any requests made to it by checking the status of the token.

It's not that much different really, but gives you a more responsive application
for users without hitting LP frequently.

Thanks,

James

[Leonard Richardson (leonardr)]

I responded to your comments and also changed the code that creates directories so that they're chmod 0700. Take another look.

[lp:~leonardr/launchpadlib/improve-workflow]

94. By Leonard Richardson on 2010-07-14

Response to feedback.

[Robert Collins (lifeless)]

+1 on all of poolies points.

James - the local socket thing is very fragile. better to use long
poll on LP to have lp signal when its ready.

-Rob

[Martin Pool (mbp)]

+ self.output(self.WAITING_FOR_USER % self.authorization_url)
+ while credentials.access_token is None:
+ try:
+ credentials.exchange_request_token_for_access_token(web_root)
+ except HTTPError, e:
+ if e.response.status == 401:
+ # The user has not made their decision yet.
+ time.sleep(1)
+ elif e.response.status == 403:
+ # The user decided not to authorize this
+ # application.
+ raise EndUserDeclinedAuthorization(e.content)
+ else:
+ # The user decided to grant access.
+ # credentials.access_token is no longer None.
+ break

This treats a 500 error as deciding to grant access. Really? I think you want the 'break' inside the try block. Aside from that it looks good.

 review: tweak

I do wonder a bit about the load this will introduce but perhaps we can measure that. Can we find out from the logs how many times this is called at the moment?

review: Needs Fixing

[lp:~leonardr/launchpadlib/improve-workflow]

95. By Leonard Richardson on 2010-07-19

Response to feedback.

[Leonard Richardson (leonardr)]

Ok, take another look.

1. I made the poll time configurable with a module constant, and moved the sleep() call to before the request so it will always be called once. Since it would take superhuman reflexes to click a link before the sleep() call expires (for any reasonable value of the poll time), this will save at least one HTTP request every time.

2. If Launchpad goes down during the poll, the poll will continue instead of the program assuming the user granted access. (I tested this by killing launchpad.dev after opening token-authorized in my browser.)

3. I saw only 50 requests for +access-token for July 16th (a Friday), out of about 4.5 million requests total. Even a large number of additional POSTs should go unnoticed. But I'd be OK with increasing the poll time to 2 or 3 seconds.

[Martin Pool (mbp)]

+ else:
+ # The user has not made their decision yet,
+ # or there was an error accessing the server.
+ pass

So this seems to me to mean that if the server is giving some non-403 error, the client will silently keep polling indefinitely. I think that is a bit strange. I can see how you might want to keep polling even if you get a transient error, but I think you should at least print the error. That's what I meant in my comment earlier today.

btw I suggest if you're making substantive changes is response to feedback you actually describe them in the commit message. If someone annotates the code later just "response to feedback" doesn't explain a lot.

[Martin Pool (mbp)]

As a (imo wishlist-priority) follow on, https://bugs.edge.launchpad.net/launchpad-foundations/+bug/607418

[Leonard Richardson (leonardr)]

All right, I've changed the code so that if there's a problem communicating with Launchpad after the initial browser open, the exception will be printed to stdout.

[lp:~leonardr/launchpadlib/improve-workflow]

96. By Leonard Richardson on 2010-07-19

If there's a problem communicating with Launchpad, print out the exception before retrying.

97. By Leonard Richardson on 2010-07-20

Merge with trunk.

[Martin Pool (mbp)]

review: Approve

=== modified file 'src/launchpadlib/NEWS.txt'
--- src/launchpadlib/NEWS.txt	2010-07-19 18:38:21 +0000
+++ src/launchpadlib/NEWS.txt	2010-07-20 15:57:51 +0000
@@ -5,6 +5,15 @@
 1.6.3 (Unreleased)
 ==================
 
+- Instead of making the end-user hit Enter after authorizing an
+  application to access their Launchpad account, launchpadlib will
+  automatically poll Launchpad until the user makes a decision.
+
+- Previously, if the end-user explicitly denied access to a
+  launchpadlib application, launchpadlib raised an unhelpful
+  exception. Now, methods like login_with() will return None instead
+  of a Launchpad object.
+
 - Improved the XSLT stylesheet to reflect Launchpad's more complex
   top-level structure. [bug=286941]
 

=== modified file 'src/launchpadlib/credentials.py'
--- src/launchpadlib/credentials.py	2010-03-16 21:05:15 +0000
+++ src/launchpadlib/credentials.py	2010-07-20 15:57:51 +0000
@@ -30,6 +30,7 @@
 import httplib2
 import sys
 import textwrap
+import time
 from urllib import urlencode, quote
 from urlparse import urljoin
 import webbrowser
@@ -45,6 +46,7 @@
 request_token_page = '+request-token'
 access_token_page = '+access-token'
 authorize_token_page = '+authorize-token'
+access_token_poll_time = 1
 
 
 class Credentials(OAuthAuthorizer):
@@ -512,6 +514,8 @@
     themselves.
     """
 
+    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=3):
         web_root = uris.lookup_web_root(web_root)
@@ -525,20 +529,35 @@
             web_root, consumer_name, request_token,
             allow_access_levels, max_failed_attempts)
 
-    def __call__(self):
+    def output(self, message):
+        """Display a message.
+
+        By default, prints the message to standard output. The message
+        does not require any user interaction--it's solely
+        informative.
+        """
+        print message
+
+    def __call__(self, credentials, web_root):
         self.open_page_in_user_browser(self.authorization_url)
-        print "The authorization page:"
-        print "   (%s)" % self.authorization_url
-        print "should be opening in your browser. After you have authorized"
-        print "this program to access Launchpad on your behalf you should come"
-        print ("back here and press <Enter> to finish the authentication "
-               "process.")
-        self.wait_for_request_token_authorization()
-
-    def wait_for_request_token_authorization(self):
-        """Get the end-user to hit enter."""
-        sys.stdin.readline()
-
+        self.output(self.WAITING_FOR_USER % self.authorization_url)
+        while credentials.access_token is None:
+            time.sleep(access_token_poll_time)
+            try:
+                credentials.exchange_request_token_for_access_token(web_root)
+                break
+            except HTTPError, e:
+                if e.response.status == 403:
+                    # The user decided not to authorize this
+                    # application.
+                    raise EndUserDeclinedAuthorization(e.content)
+                elif e.response.status == 401:
+                    # The user has not made a decision yet.
+                    pass
+                else:
+                    # There was an error accessing the server.
+                    print "Unexpected response from Launchpad:"
+                    print e
 
 class TokenAuthorizationException(Exception):
     pass
@@ -548,6 +567,10 @@
     pass
 
 
+class EndUserDeclinedAuthorization(TokenAuthorizationException):
+    pass
+
+
 class ClientError(TokenAuthorizationException):
     pass
 

=== modified file 'src/launchpadlib/launchpad.py'
--- src/launchpadlib/launchpad.py	2010-06-21 15:16:28 +0000
+++ src/launchpadlib/launchpad.py	2010-07-20 15:57:51 +0000
@@ -210,8 +210,10 @@
             web_root, authorization_json['oauth_token_consumer'],
             authorization_json['oauth_token'], allow_access_levels,
             max_failed_attempts)
-        authorizer()
-        credentials.exchange_request_token_for_access_token(web_root)
+        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)
 
@@ -273,7 +275,7 @@
          service_root_dir) = cls._get_paths(service_root, launchpadlib_dir)
         credentials_path = os.path.join(service_root_dir, 'credentials')
         if not os.path.exists(credentials_path):
-            os.makedirs(credentials_path)
+            os.makedirs(credentials_path, 0700)
         if credentials_file is None:
             consumer_credentials_path = os.path.join(credentials_path,
                 consumer_name)
@@ -292,8 +294,10 @@
                 authorizer_class=authorizer_class,
                 allow_access_levels=allow_access_levels,
                 max_failed_attempts=max_failed_attempts, version=version)
-            launchpad.credentials.save_to_path(consumer_credentials_path)
-            os.chmod(consumer_credentials_path, stat.S_IREAD | stat.S_IWRITE)
+            if launchpad is not None:
+                launchpad.credentials.save_to_path(consumer_credentials_path)
+                os.chmod(
+                    consumer_credentials_path, stat.S_IREAD | stat.S_IWRITE)
         return launchpad
 
     @classmethod
@@ -320,8 +324,8 @@
             launchpadlib_dir = os.path.join(home_dir, '.launchpadlib')
         launchpadlib_dir = os.path.expanduser(launchpadlib_dir)
         if not os.path.exists(launchpadlib_dir):
-            os.makedirs(launchpadlib_dir,0700)
-        os.chmod(launchpadlib_dir,0700)
+            os.makedirs(launchpadlib_dir, 0700)
+        os.chmod(launchpadlib_dir, 0700)
         # Determine the real service root.
         service_root = uris.lookup_service_root(service_root)
         # Each service root has its own cache and credential dirs.
@@ -330,5 +334,5 @@
         service_root_dir = os.path.join(launchpadlib_dir, host_name)
         cache_path = os.path.join(service_root_dir, 'cache')
         if not os.path.exists(cache_path):
-            os.makedirs(cache_path)
+            os.makedirs(cache_path, 0700)
         return (service_root, launchpadlib_dir, cache_path, service_root_dir)

=== modified file 'src/launchpadlib/tests/test_launchpad.py'
--- src/launchpadlib/tests/test_launchpad.py	2010-07-15 00:49:57 +0000
+++ src/launchpadlib/tests/test_launchpad.py	2010-07-20 15:57:51 +0000
@@ -150,7 +150,7 @@
         # 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/beta',
+            'not important', service_root='http://api.example.com/',
             launchpadlib_dir=launchpadlib_dir)
         # The 'launchpadlib' dir got created.
         self.assertTrue(os.path.isdir(launchpadlib_dir))
@@ -174,7 +174,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/beta',
+            'not important', service_root='http://api.example.com/',
             launchpadlib_dir=launchpadlib_dir)
         # Verify the mode has been changed to 0700
         statinfo = os.stat(launchpadlib_dir)
@@ -184,7 +184,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/beta',
+            'not important', service_root='http://api.example.com/',
             launchpadlib_dir=launchpadlib_dir)
         self.assertTrue(os.path.isdir(launchpadlib_dir))
         # Verify the mode is safe
@@ -213,7 +213,7 @@
 
     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/beta'
+        service_root = 'http://api.example.com/'
         timeout = object()
         proxy_info = object()
         launchpad = NoNetworkLaunchpad.login_with(
@@ -235,7 +235,7 @@
         """Test the anonymous login helper function."""
         launchpad = NoNetworkLaunchpad.login_anonymously(
             'anonymous access', launchpadlib_dir=self.temp_dir,
-            service_root='http://api.example.com/beta')
+            service_root='http://api.example.com/')
         self.assertEqual(launchpad.credentials.access_token.key, '')
         self.assertEqual(launchpad.credentials.access_token.secret, '')
 
@@ -250,7 +250,7 @@
         # credentials are saved.
         launchpad = NoNetworkLaunchpad.login_with(
             'app name', launchpadlib_dir=self.temp_dir,
-            service_root='http://api.example.com/beta')
+            service_root='http://api.example.com/')
         credentials_path = os.path.join(
             self.temp_dir, 'api.example.com', 'credentials', 'app name')
         self.assertTrue(os.path.exists(credentials_path))
@@ -270,7 +270,7 @@
         # writable by the user.
         launchpad = NoNetworkLaunchpad.login_with(
             'app name', launchpadlib_dir=self.temp_dir,
-            service_root='http://api.example.com/beta')
+            service_root='http://api.example.com/')
         credentials_path = os.path.join(
             self.temp_dir, 'api.example.com', 'credentials', 'app name')
         statinfo = os.stat(credentials_path)
@@ -291,7 +291,7 @@
 
         launchpad = NoNetworkLaunchpad.login_with(
             'app name', launchpadlib_dir=self.temp_dir,
-            service_root='http://api.example.com/beta')
+            service_root='http://api.example.com/')
         self.assertFalse(launchpad.get_token_and_login_called)
         self.assertEqual(launchpad.credentials.consumer.key, 'app name')
         self.assertEqual(
@@ -335,7 +335,7 @@
         old_home = os.environ['HOME']
         os.environ['HOME'] = self.temp_dir
         launchpad = NoNetworkLaunchpad.login_with(
-            'app name', service_root='http://api.example.com/beta')
+            'app name', service_root='http://api.example.com/')
         # Reset the environment to the old value.
         os.environ['HOME'] = old_home
 
@@ -375,7 +375,7 @@
         launchpad = NoNetworkLaunchpad.login_with(
             'app name', launchpadlib_dir=self.temp_dir,
             credentials_file=my_credentials_path,
-            service_root='http://api.example.com/beta')
+            service_root='http://api.example.com/')
         default_credentials_path = os.path.join(
             self.temp_dir, 'api.example.com', 'credentials', 'app name')
         self.assertFalse(os.path.exists(default_credentials_path))
@@ -387,7 +387,7 @@
         launchpad = NoNetworkLaunchpad.login_with(
             'app name', launchpadlib_dir=self.temp_dir,
             credentials_file=my_credentials_path,
-            service_root='http://api.example.com/beta')
+            service_root='http://api.example.com/')
         self.assertFalse(os.path.exists(default_credentials_path))
         self.assertTrue(os.path.exists(my_credentials_path))
         self.assertFalse(launchpad.get_token_and_login_called)