Merge lp:~ddstreet/launchpadlib/nosudo into lp:launchpadlib

[Dan Streetman]

When running under sudo, credentials should not be stored nor should a browser be started.

LP: #1825014
LP: #1862948

[lp:~ddstreet/launchpadlib/nosudo]

178. By Dan Streetman on 2020-02-22

Do not use keyring or browser under sudo

[Colin Watson (cjwatson)]

review: Needs Information

[lp:~ddstreet/launchpadlib/nosudo]

179. By Dan Streetman on 2020-03-13

Fixes based on MR feedback

[Dan Streetman (ddstreet)]

updated based on your feedback; let me know if you have any more feedback with the updated code.

Sorry if I am not getting the bzr commits correct, I use git and have almost no experience with bzr :(

[lp:~ddstreet/launchpadlib/nosudo]

180. By Dan Streetman on 2020-03-30

Fallback to MemoryCredentialStore if NoKeyringError

Instead of passing NoKeyringError up to calling application, change
default behavior to fallback to MemoryCredentialStore.

LP: #1864204

[Dan Streetman (ddstreet)]

I meant to create a new branch with the last commit for a separate MR, but I don't know the bzr cmdline well enough so it seems I accidentally pushed it to this branch. I'll leave it in this MR since it does follow on the previous commits for this but if you would prefer I can try to move it into a separate MR.

[Dan Streetman (ddstreet)]

Also, it would be *super awesome* if launchpadlib could be moved into a git repo instead of bzr... ;-)

[Colin Watson (cjwatson)]

Looks mostly good, but tox reports that this fails tests on Python 2.7 and 3.5 like this:

Failure in test test_credentials_save_failed (launchpadlib.tests.test_launchpad.TestCredenitialSaveFailedCallback)
Traceback (most recent call last):
  File "/usr/lib/python2.7/unittest/case.py", line 329, in run
    testMethod()
  File "/home/cjwatson/src/canonical/launchpadlib/launchpadlib/.tox/py27/local/lib/python2.7/site-packages/launchpadlib/tests/test_launchpad.py", line 632, in test_credentials_save_failed
    self.assertEqual(len(callback_called), 1)
  File "/usr/lib/python2.7/unittest/case.py", line 513, in assertEqual
    assertion_func(first, second, msg=msg)
  File "/usr/lib/python2.7/unittest/case.py", line 506, in _baseAssertEqual
    raise self.failureException(msg)
AssertionError: 0 != 1

Failure in test test_default_credentials_save_failed_is_to_raise_exception (launchpadlib.tests.test_launchpad.TestCredenitialSaveFailedCallback)
Traceback (most recent call last):
  File "/usr/lib/python2.7/unittest/case.py", line 329, in run
    testMethod()
  File "/home/cjwatson/src/canonical/launchpadlib/launchpadlib/.tox/py27/local/lib/python2.7/site-packages/launchpadlib/tests/test_launchpad.py", line 644, in test_default_credentials_save_failed_is_to_raise_exception
    launchpadlib_dir=launchpadlib_dir)
  File "/usr/lib/python2.7/unittest/case.py", line 473, in assertRaises
    callableObj(*args, **kwargs)
  File "/usr/lib/python2.7/unittest/case.py", line 116, in __exit__
    "{0} not raised".format(exc_name))
AssertionError: RuntimeError not raised

This is because 2.7 and 3.5 are no longer supported by the version of keyring that has your patch to add the NoKeyringError exception, so tox picks an older version of keyring: in other words, this is indicating a slight incompatibility with the older keyring version.

Things seem to work well if I apply this patch on top:

=== modified file 'src/launchpadlib/credentials.py'
--- old/src/launchpadlib/credentials.py 2020-03-30 15:39:23 +0000
+++ new/src/launchpadlib/credentials.py 2020-04-08 15:38:53 +0000
@@ -393,7 +393,12 @@ class KeyringCredentialStore(CredentialS
         try:
             keyring.set_password(
                 'launchpadlib', unique_key, serialized.decode('utf-8'))
- except NoKeyringError:
+ except NoKeyringError as e:
+ # keyring < 21.2.0 raises RuntimeError rather than anything more
+ # specific. Make sure it's the exception we're interested in.
+ if (NoKeyringError == RuntimeError and
+ 'No recommended backend was available' not in str(e)):
+ raise
             if self._fallback:
                 self._fallback.save(credentials, unique_key)
             else:
@@ -405,7 +410,12 @@ class KeyringCredentialStore(CredentialS
         try:
             credential_string = keyring.get_password(
                 'launchpadlib', unique_key)
- except NoKeyringError:
+ except NoKeyringError as e:
+ # keyring < 21.2.0 raises RuntimeError rather than anything more
+ # specific. Make sure it's the exception we're interested in.
+ if (NoKeyringError == Runtime...

review: Needs Fixing

[lp:~ddstreet/launchpadlib/nosudo]

181. By Dan Streetman on 2020-04-13

Apply cjwatson patch to handle older Keyring code that may throw RuntimeError to indicate real error, instead of NoKeyringError

[Dan Streetman (ddstreet)]

Yep, I see what you're saying; I can't really think of any better way to handle older Keyring code that would throw RuntimeError to indicate a real error. I applied your patch, and should now be pushed here. Thanks!

[Colin Watson (cjwatson)]

review: Approve

=== modified file 'src/launchpadlib/credentials.py'
--- src/launchpadlib/credentials.py	2020-02-04 14:26:36 +0000
+++ src/launchpadlib/credentials.py	2020-04-13 20:43:34 +0000
@@ -41,6 +41,10 @@
 from sys import stdin
 import time
 try:
+    from keyring.errors import NoKeyringError
+except ImportError:
+    NoKeyringError = RuntimeError
+try:
     from urllib.parse import urlencode
 except ImportError:
     from urllib import urlencode
@@ -359,6 +363,12 @@
 
     B64MARKER = b"<B64>"
 
+    def __init__(self, credential_save_failed=None, fallback=False):
+        super(KeyringCredentialStore, self).__init__(credential_save_failed)
+        self._fallback = None
+        if fallback:
+            self._fallback = MemoryCredentialStore(credential_save_failed)
+
     @staticmethod
     def _ensure_keyring_imported():
         """Ensure the keyring module is imported (postponing side effects).
@@ -380,14 +390,36 @@
         # Gnome and KDE, when newlines are included in the password.  Avoid
         # this problem by base 64 encoding the serialized value.
         serialized = self.B64MARKER + b64encode(serialized)
-        keyring.set_password(
-            'launchpadlib', unique_key, serialized.decode('utf-8'))
+        try:
+            keyring.set_password(
+                'launchpadlib', unique_key, serialized.decode('utf-8'))
+        except NoKeyringError as e:
+            # keyring < 21.2.0 raises RuntimeError rather than anything more
+            # specific.  Make sure it's the exception we're interested in.
+            if (NoKeyringError == RuntimeError and
+                    'No recommended backend was available' not in str(e)):
+                raise
+            if self._fallback:
+                self._fallback.save(credentials, unique_key)
+            else:
+                raise
 
     def do_load(self, unique_key):
         """Retrieve credentials from the keyring."""
         self._ensure_keyring_imported()
-        credential_string = keyring.get_password(
-            'launchpadlib', unique_key)
+        try:
+            credential_string = keyring.get_password(
+                'launchpadlib', unique_key)
+        except NoKeyringError as e:
+            # keyring < 21.2.0 raises RuntimeError rather than anything more
+            # specific.  Make sure it's the exception we're interested in.
+            if (NoKeyringError == RuntimeError and
+                    'No recommended backend was available' not in str(e)):
+                raise
+            if self._fallback:
+                return self._fallback.load(unique_key)
+            else:
+                raise
         if credential_string is not None:
             if isinstance(credential_string, unicode_type):
                 credential_string = credential_string.encode('utf8')
@@ -434,6 +466,25 @@
         return None
 
 
+class MemoryCredentialStore(CredentialStore):
+    """CredentialStore that stores keys only in memory.
+
+    This can be used to provide a CredentialStore instance without
+    actually saving any key to persistent storage.
+    """
+    def __init__(self, credential_save_failed=None):
+        super(MemoryCredentialStore, self).__init__(credential_save_failed)
+        self._credentials = {}
+
+    def do_save(self, credentials, unique_key):
+        """Store the credentials in our dict"""
+        self._credentials[unique_key] = credentials
+
+    def do_load(self, unique_key):
+        """Retrieve the credentials from our dict"""
+        return self._credentials.get(unique_key)
+
+
 class RequestTokenAuthorizationEngine(object):
     """The superclass of all request token authorizers.
 
@@ -579,12 +630,75 @@
         raise NotImplementedError()
 
 
-class AuthorizeRequestTokenWithBrowser(RequestTokenAuthorizationEngine):
-    """The simplest (and, right now, the only) request token authorizer.
+class AuthorizeRequestTokenWithURL(RequestTokenAuthorizationEngine):
+    """Authorize using a URL.
+
+    This authorizer simply shows the URL for the user to open for
+    authorization, and waits until the server responds.
+    """
+
+    WAITING_FOR_USER = (
+        "Please open this authorization page:\n"
+        " (%s)\n"
+        "in your browser. Use your browser to authorize\n"
+        "this program to access Launchpad on your behalf.")
+    WAITING_FOR_LAUNCHPAD = (
+        "Press Enter after authorizing in your browser.")
+
+    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 notify_end_user_authorization_url(self, authorization_url):
+        """Notify the end-user of the URL."""
+        self.output(self.WAITING_FOR_USER % authorization_url)
+
+    def check_end_user_authorization(self, credentials):
+        """Check if the end-user authorized"""
+        try:
+            credentials.exchange_request_token_for_access_token(
+                self.web_root)
+        except HTTPError as e:
+            if e.response.status == 403:
+                # The user decided not to authorize this
+                # application.
+                raise EndUserDeclinedAuthorization(e.content)
+            else:
+                if e.response.status != 401:
+                    # There was an error accessing the server.
+                    print("Unexpected response from Launchpad:")
+                    print(e)
+                # The user has not made a decision yet.
+                raise EndUserNoAuthorization(e.content)
+        return credentials.access_token is not None
+
+    def wait_for_end_user_authorization(self, credentials):
+        """Wait for the end-user to authorize"""
+        self.output(self.WAITING_FOR_LAUNCHPAD)
+        stdin.readline()
+        self.check_end_user_authorization(credentials)
+
+    def make_end_user_authorize_token(self, credentials, request_token):
+        """Have the end-user authorize the token using a URL."""
+        authorization_url = self.authorization_url(request_token)
+        self.notify_end_user_authorization_url(authorization_url)
+        self.wait_for_end_user_authorization(credentials)
+
+
+class AuthorizeRequestTokenWithBrowser(AuthorizeRequestTokenWithURL):
+    """Authorize using a URL that pops-up automatically in a browser.
 
     This authorizer simply opens up the end-user's web browser to a
     Launchpad URL and lets the end-user authorize the request token
     themselves.
+
+    This is the same as its superclass, except this class also
+    performs the browser automatic opening of the URL.
     """
 
     WAITING_FOR_USER = (
@@ -594,10 +708,10 @@
         "this program to access Launchpad on your behalf.")
     TIMEOUT_MESSAGE = "Press Enter to continue or wait (%d) seconds..."
     TIMEOUT = 5
-    WAITING_FOR_LAUNCHPAD = (
-            "Waiting to hear from Launchpad about your decision...")
     TERMINAL_BROWSERS = ('www-browser', 'links', 'links2', 'lynx',
                          'elinks', 'elinks-lite', 'netrik', 'w3m')
+    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):
@@ -620,20 +734,10 @@
               service_root, application_name, None,
               credential_save_failed)
 
-    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 make_end_user_authorize_token(self, credentials, request_token):
-        """Have the end-user authorize the token in their browser."""
-
-        authorization_url = self.authorization_url(request_token)
-        self.output(self.WAITING_FOR_USER % authorization_url)
+    def notify_end_user_authorization_url(self, authorization_url):
+        """Notify the end-user of the URL."""
+        super(AuthorizeRequestTokenWithBrowser,
+              self).notify_end_user_authorization_url(authorization_url)
 
         try:
             browser_obj = webbrowser.get()
@@ -651,28 +755,20 @@
             if rlist:
                 stdin.readline()
 
-        self.output(self.WAITING_FOR_LAUNCHPAD)
         if browser_obj is not None:
             webbrowser.open(authorization_url)
+
+    def wait_for_end_user_authorization(self, credentials):
+        """Wait for the end-user to authorize"""
+        self.output(self.WAITING_FOR_LAUNCHPAD)
         start_time = time.time()
         while credentials.access_token is None:
             time.sleep(access_token_poll_time)
             try:
-                credentials.exchange_request_token_for_access_token(
-                    self.web_root)
-                break
-            except HTTPError as 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)
+                if self.check_end_user_authorization(credentials):
+                    break
+            except EndUserNoAuthorization:
+                pass
             if time.time() >= start_time + access_token_poll_timeout:
                 raise TokenAuthorizationTimedOut(
                     "Timed out after %d seconds." % access_token_poll_timeout)
@@ -686,11 +782,23 @@
     pass
 
 
-class EndUserDeclinedAuthorization(TokenAuthorizationException):
-    pass
-
-
-class TokenAuthorizationTimedOut(TokenAuthorizationException):
+class EndUserAuthorizationFailed(TokenAuthorizationException):
+    """Superclass exception for all failures of end-user authorization"""
+    pass
+
+
+class EndUserDeclinedAuthorization(EndUserAuthorizationFailed):
+    """End-user declined authorization"""
+    pass
+
+
+class EndUserNoAuthorization(EndUserAuthorizationFailed):
+    """End-user did not perform any authorization"""
+    pass
+
+
+class TokenAuthorizationTimedOut(EndUserNoAuthorization):
+    """End-user did not perform any authorization in timeout period"""
     pass
 
 
 
=== modified file 'src/launchpadlib/launchpad.py'
--- src/launchpadlib/launchpad.py	2015-11-17 18:23:24 +0000
+++ src/launchpadlib/launchpad.py	2020-04-13 20:43:34 +0000
@@ -47,8 +47,10 @@
     AccessToken,
     AnonymousAccessToken,
     AuthorizeRequestTokenWithBrowser,
+    AuthorizeRequestTokenWithURL,
     Consumer,
     Credentials,
+    MemoryCredentialStore,
     KeyringCredentialStore,
     UnencryptedFileCredentialStore,
     )
@@ -213,12 +215,29 @@
             proxy_info)
 
     @classmethod
+    def _is_sudo(cls):
+        return (set(['SUDO_USER', 'SUDO_UID', 'SUDO_GID']) &
+                set(os.environ.keys()))
+
+    @classmethod
     def authorization_engine_factory(cls, *args):
+        if cls._is_sudo():
+            # Do not try to open browser window under sudo;
+            # we probably don't have access to the X session,
+            # and some browsers (e.g. chromium) won't run as root
+            # LP: #1825014
+            return AuthorizeRequestTokenWithURL(*args)
         return AuthorizeRequestTokenWithBrowser(*args)
 
     @classmethod
     def credential_store_factory(cls, credential_save_failed):
-        return KeyringCredentialStore(credential_save_failed)
+        if cls._is_sudo():
+            # Do not try to store credentials under sudo;
+            # it can be problematic with shared sudo access,
+            # and we may not have access to the normal keyring provider
+            # LP: #1862948
+            return MemoryCredentialStore(credential_save_failed)
+        return KeyringCredentialStore(credential_save_failed, fallback=True)
 
     @classmethod
     def login(cls, consumer_name, token_string, access_secret,