Merge lp:~dktrkranz/launchpadlib/bug-643699 into lp:launchpadlib

[Luca Falavigna]

Fix for LP: #643699

[Benji York (benji)]

Thanks for submitting this branch Luca. I have a couple of comments:

We really don't need a second string (TIMEOUT_MESSAGE). The two
strings should be combined into one. The string should end in something
like this:

    "Press any key to view the authentication page in a web browser."

There's no reason to have a timeout. If the intent is to avoid
censusing the user by suddenly showing them a web page, then we have no
guarantee that they'll read the message before the timeout ends. They
might be otherwise distracted when the message is shown, the message may
be generated at the end of a long shell pipeline or script that the user
has switched away from, the user may be a slow reader, etc.

Also, the code as-written won't actually proceed if they press any key,
only enter. Either the code should be changed or the prompt should be
re-worded to ask the user to press enter.

Thanks again for the branch. This bug certainly needs attention.

[Martin Pool (mbp)]

This code runs regardless of whether the user has a graphical or text web browser. If we pause indefinitely, and they are in the typical graphical environment, and the stdout of the process using launchpadlib is not visible to the user, it will apparently just hang, which will be bad.

As I said in <https://bugs.launchpad.net/launchpadlib/+bug/643699/comments/6> I'd strongly prefer guarding this by checking $DISPLAY (on X11) even though that's not 100% correlated with using a graphical browser.

You're right that readline will block until enter is pressed. In fact unless the code has already put stdin in uncooked mode (which it might have, but I don't see it) even the select will block until you press enter.

I do think the cleanest way to fix this (though perhaps less accessible to dktrkranz) is to just show an actually useful message in the web page and we should at least file a bug against <https://launchpad.net/canonical-identity-provider> or Launchpad or whatever is showing that page.

review: Needs Fixing

=== modified file 'src/launchpadlib/credentials.py'
--- src/launchpadlib/credentials.py	2011-01-10 14:34:03 +0000
+++ src/launchpadlib/credentials.py	2011-10-07 18:45:23 +0000
@@ -31,7 +31,9 @@
 from cStringIO import StringIO
 import httplib2
 import os
+from select import select
 import stat
+from sys import stdin
 import time
 from urllib import urlencode
 from urlparse import urljoin
@@ -500,7 +502,10 @@
     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..."
+    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."
+    TIMEOUT_MESSAGE = "Press any key to continue or wait (%d) seconds..."
+    TIMEOUT = 5
+    WAITING_FOR_LAUNCHPAD = "Waiting to hear from Launchpad about your decision..."
 
     def __init__(self, service_root, application_name, consumer_name=None,
                  credential_save_failed=None, allow_access_levels=None):
@@ -536,8 +541,15 @@
         """Have the end-user authorize the token in their browser."""
 
         authorization_url = self.authorization_url(request_token)
+        self.output(self.WAITING_FOR_USER % authorization_url)
+        self.output(self.TIMEOUT_MESSAGE % self.TIMEOUT)
+        # Wait a little time before attempting to launch browser,
+        # give users the chance to press a key to skip it anyway.
+        rlist, _, _ = select([stdin], [], [], self.TIMEOUT)
+        if rlist:
+            stdin.readline()
+        self.output(self.WAITING_FOR_LAUNCHPAD)
         webbrowser.open(authorization_url)
-        self.output(self.WAITING_FOR_USER % authorization_url)
         while credentials.access_token is None:
             time.sleep(access_token_poll_time)
             try: