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

Proposed by Dan Streetman on 2020-02-22
Status: Merged
Merged at revision: 178
Proposed branch: lp:~ddstreet/launchpadlib/nosudo
Merge into: lp:launchpadlib
Diff against target: 322 lines (+171/-44)
2 files modified
src/launchpadlib/credentials.py (+151/-43)
src/launchpadlib/launchpad.py (+20/-1)
To merge this branch: bzr merge lp:~ddstreet/launchpadlib/nosudo
Reviewer Review Type Date Requested Status
Colin Watson 2020-02-22 Approve on 2020-04-14
Review via email: mp+379694@code.launchpad.net

Commit message

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

LP: #1825014
LP: #1862948

To post a comment you must log in.
lp:~ddstreet/launchpadlib/nosudo updated on 2020-02-22
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 updated on 2020-03-13
179. By Dan Streetman on 2020-03-13

Fixes based on MR feedback

Dan Streetman (ddstreet) wrote :

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 updated on 2020-03-30
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) wrote :

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) wrote :

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

Colin Watson (cjwatson) wrote :
Download full text (3.6 KiB)

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...

Read more...

review: Needs Fixing
lp:~ddstreet/launchpadlib/nosudo updated on 2020-04-13
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) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/launchpadlib/credentials.py'
2--- src/launchpadlib/credentials.py 2020-02-04 14:26:36 +0000
3+++ src/launchpadlib/credentials.py 2020-04-13 20:43:34 +0000
4@@ -41,6 +41,10 @@
5 from sys import stdin
6 import time
7 try:
8+ from keyring.errors import NoKeyringError
9+except ImportError:
10+ NoKeyringError = RuntimeError
11+try:
12 from urllib.parse import urlencode
13 except ImportError:
14 from urllib import urlencode
15@@ -359,6 +363,12 @@
16
17 B64MARKER = b"<B64>"
18
19+ def __init__(self, credential_save_failed=None, fallback=False):
20+ super(KeyringCredentialStore, self).__init__(credential_save_failed)
21+ self._fallback = None
22+ if fallback:
23+ self._fallback = MemoryCredentialStore(credential_save_failed)
24+
25 @staticmethod
26 def _ensure_keyring_imported():
27 """Ensure the keyring module is imported (postponing side effects).
28@@ -380,14 +390,36 @@
29 # Gnome and KDE, when newlines are included in the password. Avoid
30 # this problem by base 64 encoding the serialized value.
31 serialized = self.B64MARKER + b64encode(serialized)
32- keyring.set_password(
33- 'launchpadlib', unique_key, serialized.decode('utf-8'))
34+ try:
35+ keyring.set_password(
36+ 'launchpadlib', unique_key, serialized.decode('utf-8'))
37+ except NoKeyringError as e:
38+ # keyring < 21.2.0 raises RuntimeError rather than anything more
39+ # specific. Make sure it's the exception we're interested in.
40+ if (NoKeyringError == RuntimeError and
41+ 'No recommended backend was available' not in str(e)):
42+ raise
43+ if self._fallback:
44+ self._fallback.save(credentials, unique_key)
45+ else:
46+ raise
47
48 def do_load(self, unique_key):
49 """Retrieve credentials from the keyring."""
50 self._ensure_keyring_imported()
51- credential_string = keyring.get_password(
52- 'launchpadlib', unique_key)
53+ try:
54+ credential_string = keyring.get_password(
55+ 'launchpadlib', unique_key)
56+ except NoKeyringError as e:
57+ # keyring < 21.2.0 raises RuntimeError rather than anything more
58+ # specific. Make sure it's the exception we're interested in.
59+ if (NoKeyringError == RuntimeError and
60+ 'No recommended backend was available' not in str(e)):
61+ raise
62+ if self._fallback:
63+ return self._fallback.load(unique_key)
64+ else:
65+ raise
66 if credential_string is not None:
67 if isinstance(credential_string, unicode_type):
68 credential_string = credential_string.encode('utf8')
69@@ -434,6 +466,25 @@
70 return None
71
72
73+class MemoryCredentialStore(CredentialStore):
74+ """CredentialStore that stores keys only in memory.
75+
76+ This can be used to provide a CredentialStore instance without
77+ actually saving any key to persistent storage.
78+ """
79+ def __init__(self, credential_save_failed=None):
80+ super(MemoryCredentialStore, self).__init__(credential_save_failed)
81+ self._credentials = {}
82+
83+ def do_save(self, credentials, unique_key):
84+ """Store the credentials in our dict"""
85+ self._credentials[unique_key] = credentials
86+
87+ def do_load(self, unique_key):
88+ """Retrieve the credentials from our dict"""
89+ return self._credentials.get(unique_key)
90+
91+
92 class RequestTokenAuthorizationEngine(object):
93 """The superclass of all request token authorizers.
94
95@@ -579,12 +630,75 @@
96 raise NotImplementedError()
97
98
99-class AuthorizeRequestTokenWithBrowser(RequestTokenAuthorizationEngine):
100- """The simplest (and, right now, the only) request token authorizer.
101+class AuthorizeRequestTokenWithURL(RequestTokenAuthorizationEngine):
102+ """Authorize using a URL.
103+
104+ This authorizer simply shows the URL for the user to open for
105+ authorization, and waits until the server responds.
106+ """
107+
108+ WAITING_FOR_USER = (
109+ "Please open this authorization page:\n"
110+ " (%s)\n"
111+ "in your browser. Use your browser to authorize\n"
112+ "this program to access Launchpad on your behalf.")
113+ WAITING_FOR_LAUNCHPAD = (
114+ "Press Enter after authorizing in your browser.")
115+
116+ def output(self, message):
117+ """Display a message.
118+
119+ By default, prints the message to standard output. The message
120+ does not require any user interaction--it's solely
121+ informative.
122+ """
123+ print(message)
124+
125+ def notify_end_user_authorization_url(self, authorization_url):
126+ """Notify the end-user of the URL."""
127+ self.output(self.WAITING_FOR_USER % authorization_url)
128+
129+ def check_end_user_authorization(self, credentials):
130+ """Check if the end-user authorized"""
131+ try:
132+ credentials.exchange_request_token_for_access_token(
133+ self.web_root)
134+ except HTTPError as e:
135+ if e.response.status == 403:
136+ # The user decided not to authorize this
137+ # application.
138+ raise EndUserDeclinedAuthorization(e.content)
139+ else:
140+ if e.response.status != 401:
141+ # There was an error accessing the server.
142+ print("Unexpected response from Launchpad:")
143+ print(e)
144+ # The user has not made a decision yet.
145+ raise EndUserNoAuthorization(e.content)
146+ return credentials.access_token is not None
147+
148+ def wait_for_end_user_authorization(self, credentials):
149+ """Wait for the end-user to authorize"""
150+ self.output(self.WAITING_FOR_LAUNCHPAD)
151+ stdin.readline()
152+ self.check_end_user_authorization(credentials)
153+
154+ def make_end_user_authorize_token(self, credentials, request_token):
155+ """Have the end-user authorize the token using a URL."""
156+ authorization_url = self.authorization_url(request_token)
157+ self.notify_end_user_authorization_url(authorization_url)
158+ self.wait_for_end_user_authorization(credentials)
159+
160+
161+class AuthorizeRequestTokenWithBrowser(AuthorizeRequestTokenWithURL):
162+ """Authorize using a URL that pops-up automatically in a browser.
163
164 This authorizer simply opens up the end-user's web browser to a
165 Launchpad URL and lets the end-user authorize the request token
166 themselves.
167+
168+ This is the same as its superclass, except this class also
169+ performs the browser automatic opening of the URL.
170 """
171
172 WAITING_FOR_USER = (
173@@ -594,10 +708,10 @@
174 "this program to access Launchpad on your behalf.")
175 TIMEOUT_MESSAGE = "Press Enter to continue or wait (%d) seconds..."
176 TIMEOUT = 5
177- WAITING_FOR_LAUNCHPAD = (
178- "Waiting to hear from Launchpad about your decision...")
179 TERMINAL_BROWSERS = ('www-browser', 'links', 'links2', 'lynx',
180 'elinks', 'elinks-lite', 'netrik', 'w3m')
181+ WAITING_FOR_LAUNCHPAD = (
182+ "Waiting to hear from Launchpad about your decision...")
183
184 def __init__(self, service_root, application_name, consumer_name=None,
185 credential_save_failed=None, allow_access_levels=None):
186@@ -620,20 +734,10 @@
187 service_root, application_name, None,
188 credential_save_failed)
189
190- def output(self, message):
191- """Display a message.
192-
193- By default, prints the message to standard output. The message
194- does not require any user interaction--it's solely
195- informative.
196- """
197- print(message)
198-
199- def make_end_user_authorize_token(self, credentials, request_token):
200- """Have the end-user authorize the token in their browser."""
201-
202- authorization_url = self.authorization_url(request_token)
203- self.output(self.WAITING_FOR_USER % authorization_url)
204+ def notify_end_user_authorization_url(self, authorization_url):
205+ """Notify the end-user of the URL."""
206+ super(AuthorizeRequestTokenWithBrowser,
207+ self).notify_end_user_authorization_url(authorization_url)
208
209 try:
210 browser_obj = webbrowser.get()
211@@ -651,28 +755,20 @@
212 if rlist:
213 stdin.readline()
214
215- self.output(self.WAITING_FOR_LAUNCHPAD)
216 if browser_obj is not None:
217 webbrowser.open(authorization_url)
218+
219+ def wait_for_end_user_authorization(self, credentials):
220+ """Wait for the end-user to authorize"""
221+ self.output(self.WAITING_FOR_LAUNCHPAD)
222 start_time = time.time()
223 while credentials.access_token is None:
224 time.sleep(access_token_poll_time)
225 try:
226- credentials.exchange_request_token_for_access_token(
227- self.web_root)
228- break
229- except HTTPError as e:
230- if e.response.status == 403:
231- # The user decided not to authorize this
232- # application.
233- raise EndUserDeclinedAuthorization(e.content)
234- elif e.response.status == 401:
235- # The user has not made a decision yet.
236- pass
237- else:
238- # There was an error accessing the server.
239- print("Unexpected response from Launchpad:")
240- print(e)
241+ if self.check_end_user_authorization(credentials):
242+ break
243+ except EndUserNoAuthorization:
244+ pass
245 if time.time() >= start_time + access_token_poll_timeout:
246 raise TokenAuthorizationTimedOut(
247 "Timed out after %d seconds." % access_token_poll_timeout)
248@@ -686,11 +782,23 @@
249 pass
250
251
252-class EndUserDeclinedAuthorization(TokenAuthorizationException):
253- pass
254-
255-
256-class TokenAuthorizationTimedOut(TokenAuthorizationException):
257+class EndUserAuthorizationFailed(TokenAuthorizationException):
258+ """Superclass exception for all failures of end-user authorization"""
259+ pass
260+
261+
262+class EndUserDeclinedAuthorization(EndUserAuthorizationFailed):
263+ """End-user declined authorization"""
264+ pass
265+
266+
267+class EndUserNoAuthorization(EndUserAuthorizationFailed):
268+ """End-user did not perform any authorization"""
269+ pass
270+
271+
272+class TokenAuthorizationTimedOut(EndUserNoAuthorization):
273+ """End-user did not perform any authorization in timeout period"""
274 pass
275
276
277
278=== modified file 'src/launchpadlib/launchpad.py'
279--- src/launchpadlib/launchpad.py 2015-11-17 18:23:24 +0000
280+++ src/launchpadlib/launchpad.py 2020-04-13 20:43:34 +0000
281@@ -47,8 +47,10 @@
282 AccessToken,
283 AnonymousAccessToken,
284 AuthorizeRequestTokenWithBrowser,
285+ AuthorizeRequestTokenWithURL,
286 Consumer,
287 Credentials,
288+ MemoryCredentialStore,
289 KeyringCredentialStore,
290 UnencryptedFileCredentialStore,
291 )
292@@ -213,12 +215,29 @@
293 proxy_info)
294
295 @classmethod
296+ def _is_sudo(cls):
297+ return (set(['SUDO_USER', 'SUDO_UID', 'SUDO_GID']) &
298+ set(os.environ.keys()))
299+
300+ @classmethod
301 def authorization_engine_factory(cls, *args):
302+ if cls._is_sudo():
303+ # Do not try to open browser window under sudo;
304+ # we probably don't have access to the X session,
305+ # and some browsers (e.g. chromium) won't run as root
306+ # LP: #1825014
307+ return AuthorizeRequestTokenWithURL(*args)
308 return AuthorizeRequestTokenWithBrowser(*args)
309
310 @classmethod
311 def credential_store_factory(cls, credential_save_failed):
312- return KeyringCredentialStore(credential_save_failed)
313+ if cls._is_sudo():
314+ # Do not try to store credentials under sudo;
315+ # it can be problematic with shared sudo access,
316+ # and we may not have access to the normal keyring provider
317+ # LP: #1862948
318+ return MemoryCredentialStore(credential_save_failed)
319+ return KeyringCredentialStore(credential_save_failed, fallback=True)
320
321 @classmethod
322 def login(cls, consumer_name, token_string, access_secret,

Subscribers

People subscribed via source and target branches