diff --git a/gerrit_util.py b/gerrit_util.py index 4b074020d..a08ce1636 100644 --- a/gerrit_util.py +++ b/gerrit_util.py @@ -9,7 +9,7 @@ https://gerrit-review.googlesource.com/Documentation/rest-api.html import base64 import contextlib -from typing import List, Type +from typing import List, Optional, Tuple, Type import httplib2 import json import logging @@ -96,7 +96,17 @@ def _QueryString(params, first_param=None): class Authenticator(object): """Base authenticator class for authenticator implementations to subclass.""" - def get_auth_header(self, host): + def get_auth_info(self, host: str) -> Tuple[Optional[str], Optional[httplib2.ProxyInfo]]: + """Returns the Authorization header value, plus an optional ProxyInfo. + + TODO: Remove `host`. This is only needed for the deprecated + CookiesAuthenticator. If distinguishing between hosts is still needed + later, I would propose moving this parameter to + Authenticator.get/Authenticator.is_applicable/Authenticator.__init__ + instead. + + TODO: Make auth header non-optional. + """ raise NotImplementedError() def debug_summary_state(self) -> str: @@ -231,16 +241,16 @@ class CookiesAuthenticator(Authenticator): return (creds[0], None, creds[1]) return None - def get_auth_header(self, host): + def get_auth_info(self, host: str) -> Tuple[Optional[str], Optional[httplib2.ProxyInfo]]: a = self._get_auth_for_host(host) if a: if a[0]: secret = base64.b64encode( ('%s:%s' % (a[0], a[2])).encode('utf-8')) - return 'Basic %s' % secret.decode('utf-8') + return 'Basic %s' % secret.decode('utf-8'), None - return 'Bearer %s' % a[2] - return None + return 'Bearer %s' % a[2], None + return None, None # Used to redact the cookies from the gitcookies file. GITCOOKIES_REDACT_RE = re.compile(r'1/.*') @@ -333,11 +343,11 @@ class GceAuthenticator(Authenticator): cls._token_expiration = cls._token_cache['expires_in'] + time_time() return cls._token_cache - def get_auth_header(self, _host): + def get_auth_info(self, host: str) -> Tuple[Optional[str], Optional[httplib2.ProxyInfo]]: token_dict = self._get_token_dict() if not token_dict: - return None - return '%(token_type)s %(access_token)s' % token_dict + return None, None + return '%(token_type)s %(access_token)s' % token_dict, None def debug_summary_state(self) -> str: # TODO(b/343230702) - report ambient account name. @@ -355,8 +365,8 @@ class LuciContextAuthenticator(Authenticator): self._authenticator = auth.Authenticator(' '.join( [auth.OAUTH_SCOPE_EMAIL, auth.OAUTH_SCOPE_GERRIT])) - def get_auth_header(self, _host): - return 'Bearer %s' % self._authenticator.get_access_token().token + def get_auth_info(self, host: str) -> Tuple[Optional[str], Optional[httplib2.ProxyInfo]]: + return 'Bearer %s' % self._authenticator.get_access_token().token, None def debug_summary_state(self) -> str: # TODO(b/343230702) - report ambient account name. @@ -373,22 +383,22 @@ def CreateHttpConn(host, headers = headers or {} bare_host = host.partition(':')[0] - a = Authenticator.get() + authenticator = Authenticator.get() # TODO(crbug.com/1059384): Automatically detect when running on cloudtop. - if isinstance(a, GceAuthenticator): + if isinstance(authenticator, GceAuthenticator): print('If you\'re on a cloudtop instance, export ' 'SKIP_GCE_AUTH_FOR_GIT=1 in your env.') - a = a.get_auth_header(bare_host) - if a: - headers.setdefault('Authorization', a) + auth_header, proxy = authenticator.get_auth_info(bare_host) + if auth_header: + headers.setdefault('Authorization', auth_header) else: LOGGER.debug('No authorization found for %s.' % bare_host) url = path if not url.startswith('/'): url = '/' + url - if 'Authorization' in headers and not url.startswith('/a/'): + if auth_header and not url.startswith('/a/'): url = '/a%s' % url if body: @@ -402,7 +412,7 @@ def CreateHttpConn(host, LOGGER.debug('%s: %s' % (key, val)) if body: LOGGER.debug(body) - conn = httplib2.Http(timeout=timeout) + conn = httplib2.Http(timeout=timeout, proxy_info=proxy) # HACK: httplib2.Http has no such attribute; we store req_host here for # later use in ReadHttpResponse. conn.req_host = host diff --git a/git_cl.py b/git_cl.py index 28289832e..2d0ca89bc 100755 --- a/git_cl.py +++ b/git_cl.py @@ -2312,12 +2312,12 @@ class Changelist(object): git_host = self._GetGitHost() assert self._gerrit_server and self._gerrit_host and git_host - gerrit_auth = cookie_auth.get_auth_header(self._gerrit_host) - git_auth = cookie_auth.get_auth_header(git_host) + gerrit_auth, _ = cookie_auth.get_auth_info(self._gerrit_host) + git_auth, _ = cookie_auth.get_auth_info(git_host) if gerrit_auth and git_auth: if gerrit_auth == git_auth: return - all_gsrc = cookie_auth.get_auth_header( + all_gsrc, _ = cookie_auth.get_auth_info( 'd0esN0tEx1st.googlesource.com') print( 'WARNING: You have different credentials for Gerrit and git hosts:\n' diff --git a/tests/gerrit_util_test.py b/tests/gerrit_util_test.py index 107ee7851..94d7d4c57 100755 --- a/tests/gerrit_util_test.py +++ b/tests/gerrit_util_test.py @@ -151,13 +151,13 @@ class CookiesAuthenticatorTest(unittest.TestCase): 'Basic Z2l0LXVzZXIuY2hyb21pdW0ub3JnOjEvY2hyb21pdW0tc2VjcmV0') auth = gerrit_util.CookiesAuthenticator() - self.assertEqual(expected_chromium_header, - auth.get_auth_header('chromium.googlesource.com')) + self.assertEqual((expected_chromium_header, None), + auth.get_auth_info('chromium.googlesource.com')) self.assertEqual( - expected_chromium_header, - auth.get_auth_header('chromium-review.googlesource.com')) - self.assertEqual('Bearer example-bearer-token', - auth.get_auth_header('some-review.example.com')) + (expected_chromium_header, None), + auth.get_auth_info('chromium-review.googlesource.com')) + self.assertEqual(('Bearer example-bearer-token', None), + auth.get_auth_info('some-review.example.com')) def testGetAuthEmail(self): auth = gerrit_util.CookiesAuthenticator() @@ -226,15 +226,21 @@ class GceAuthenticatorTest(unittest.TestCase): def testGetAuthHeader_Error(self): httplib2.Http().request.side_effect = httplib2.HttpLib2Error - self.assertIsNone(self.GceAuthenticator().get_auth_header('')) + self.assertEqual( + (None, None), + self.GceAuthenticator().get_auth_info('')) def testGetAuthHeader_500(self): httplib2.Http().request.return_value = (mock.Mock(status=500), None) - self.assertIsNone(self.GceAuthenticator().get_auth_header('')) + self.assertEqual( + (None, None), + self.GceAuthenticator().get_auth_info('')) def testGetAuthHeader_Non200(self): httplib2.Http().request.return_value = (mock.Mock(status=403), None) - self.assertIsNone(self.GceAuthenticator().get_auth_header('')) + self.assertEqual( + (None, None), + self.GceAuthenticator().get_auth_info('')) def testGetAuthHeader_OK(self): httplib2.Http().request.return_value = ( @@ -242,8 +248,8 @@ class GceAuthenticatorTest(unittest.TestCase): '{"expires_in": 125, "token_type": "TYPE", "access_token": "TOKEN"}' ) gerrit_util.time_time.return_value = 0 - self.assertEqual('TYPE TOKEN', - self.GceAuthenticator().get_auth_header('')) + self.assertEqual(('TYPE TOKEN', None), + self.GceAuthenticator().get_auth_info('')) def testGetAuthHeader_Cache(self): httplib2.Http().request.return_value = ( @@ -251,10 +257,10 @@ class GceAuthenticatorTest(unittest.TestCase): '{"expires_in": 125, "token_type": "TYPE", "access_token": "TOKEN"}' ) gerrit_util.time_time.return_value = 0 - self.assertEqual('TYPE TOKEN', - self.GceAuthenticator().get_auth_header('')) - self.assertEqual('TYPE TOKEN', - self.GceAuthenticator().get_auth_header('')) + self.assertEqual(('TYPE TOKEN', None), + self.GceAuthenticator().get_auth_info('')) + self.assertEqual(('TYPE TOKEN', None), + self.GceAuthenticator().get_auth_info('')) httplib2.Http().request.assert_called_once() def testGetAuthHeader_CacheOld(self): @@ -263,10 +269,10 @@ class GceAuthenticatorTest(unittest.TestCase): '{"expires_in": 125, "token_type": "TYPE", "access_token": "TOKEN"}' ) gerrit_util.time_time.side_effect = [0, 100, 200] - self.assertEqual('TYPE TOKEN', - self.GceAuthenticator().get_auth_header('')) - self.assertEqual('TYPE TOKEN', - self.GceAuthenticator().get_auth_header('')) + self.assertEqual(('TYPE TOKEN', None), + self.GceAuthenticator().get_auth_info('')) + self.assertEqual(('TYPE TOKEN', None), + self.GceAuthenticator().get_auth_info('')) self.assertEqual(2, len(httplib2.Http().request.mock_calls)) @@ -294,7 +300,7 @@ class GerritUtilTest(unittest.TestCase): @mock.patch('gerrit_util.Authenticator') def testCreateHttpConn_Basic(self, mockAuth): - mockAuth.get().get_auth_header.return_value = None + mockAuth.get().get_auth_info.return_value = None, None conn = gerrit_util.CreateHttpConn('host.example.com', 'foo/bar') self.assertEqual('host.example.com', conn.req_host) self.assertEqual( @@ -307,7 +313,7 @@ class GerritUtilTest(unittest.TestCase): @mock.patch('gerrit_util.Authenticator') def testCreateHttpConn_Authenticated(self, mockAuth): - mockAuth.get().get_auth_header.return_value = 'Bearer token' + mockAuth.get().get_auth_info.return_value = 'Bearer token', None conn = gerrit_util.CreateHttpConn('host.example.com', 'foo/bar', headers={'header': 'value'}) @@ -325,7 +331,7 @@ class GerritUtilTest(unittest.TestCase): @mock.patch('gerrit_util.Authenticator') def testCreateHttpConn_Body(self, mockAuth): - mockAuth.get().get_auth_header.return_value = None + mockAuth.get().get_auth_info.return_value = None, None conn = gerrit_util.CreateHttpConn('host.example.com', 'foo/bar', body={ diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 2b4f29726..74f091407 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -2598,7 +2598,7 @@ class TestGitCl(unittest.TestCase): 'chromium-review.googlesource.com': ('', None, 'secret'), }) self.assertIsNone(cl.EnsureAuthenticated(force=False)) - header = gerrit_util.CookiesAuthenticator().get_auth_header( + header, _ = gerrit_util.CookiesAuthenticator().get_auth_info( 'chromium.googlesource.com') self.assertTrue('Bearer' in header)