From a85dcffff40e537ce931ec7941400a8ef0c4e37a Mon Sep 17 00:00:00 2001 From: Robert Iannucci Date: Tue, 28 May 2024 18:55:32 +0000 Subject: [PATCH] [git_cl] Refactor away a use of isinstance. Previously when composing a debugging trace, it would use an isinstance check to special-case inclusion of a gitcookies file. This CL refactors this to be part of the Authenticator API, instead, which means that we will always get some sort of authenticator log file in a trace, even if it's empty. This also provides an affordance to add debugging information for the other authenticator types later. R=ddoman@chromium.org, gavinmak@google.com Bug: 336351842 Change-Id: Idd6f45ea60b089f9b2391b5527c5281f67421043 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/5571497 Auto-Submit: Robbie Iannucci Reviewed-by: Yiwei Zhang Commit-Queue: Yiwei Zhang --- gerrit_util.py | 74 +++++++++++++++++++++++++++++++-------- git_cl.py | 18 +++++----- tests/gerrit_util_test.py | 14 ++++---- tests/git_cl_test.py | 20 +++++++++-- 4 files changed, 92 insertions(+), 34 deletions(-) diff --git a/gerrit_util.py b/gerrit_util.py index 35c206f80..4b074020d 100644 --- a/gerrit_util.py +++ b/gerrit_util.py @@ -9,6 +9,7 @@ https://gerrit-review.googlesource.com/Documentation/rest-api.html import base64 import contextlib +from typing import List, Type import httplib2 import json import logging @@ -98,6 +99,21 @@ class Authenticator(object): def get_auth_header(self, host): raise NotImplementedError() + def debug_summary_state(self) -> str: + """If this Authenticator has any debugging information about its state, + _WriteGitPushTraces will call this to include in the git push traces. + + Return value is any relevant debugging information with all PII/secrets + redacted. + """ + raise NotImplementedError() + + @classmethod + def is_applicable(cls) -> bool: + """Must return True if this Authenticator is available in the current + environment.""" + raise NotImplementedError() + @staticmethod def get(): """Returns: (Authenticator) The identified Authenticator to use. @@ -105,15 +121,22 @@ class Authenticator(object): Probes the local system and its environment and identifies the Authenticator instance to use. """ - # LUCI Context takes priority since it's normally present only on bots, - # which then must use it. - if LuciContextAuthenticator.is_luci(): - return LuciContextAuthenticator() - # TODO(crbug.com/1059384): Automatically detect when running on - # cloudtop, and use CookiesAuthenticator instead. - if GceAuthenticator.is_gce(): - return GceAuthenticator() - return CookiesAuthenticator() + authenticators: List[Type[Authenticator]] = [ + # LUCI Context takes priority since it's normally present only on bots, + # which then must use it. + LuciContextAuthenticator, + + # TODO(crbug.com/1059384): Automatically detect when running on + # cloudtop, and use CookiesAuthenticator instead. + GceAuthenticator, + CookiesAuthenticator, + ] + for candidate in authenticators: + if candidate.is_applicable(): + return candidate() + + raise ValueError( + f"Could not find suitable authenticator, tried: {authenticators}") class CookiesAuthenticator(Authenticator): @@ -132,6 +155,11 @@ class CookiesAuthenticator(Authenticator): # error if they are wrong). self._gitcookies = self._EMPTY + @classmethod + def is_applicable(cls) -> bool: + # We consider CookiesAuthenticator always applicable for now. + return True + @property def gitcookies(self): if self._gitcookies is self._EMPTY: @@ -160,9 +188,9 @@ class CookiesAuthenticator(Authenticator): return 'You can (re)generate your credentials by visiting %s' % url @classmethod - def get_gitcookies_path(cls): - if os.getenv('GIT_COOKIES_PATH'): - return os.getenv('GIT_COOKIES_PATH') + def get_gitcookies_path(cls) -> str: + if envVal := os.getenv('GIT_COOKIES_PATH'): + return envVal return os.path.expanduser( scm.GIT.GetConfig(os.getcwd(), 'http.cookiefile', @@ -214,6 +242,16 @@ class CookiesAuthenticator(Authenticator): return 'Bearer %s' % a[2] return None + # Used to redact the cookies from the gitcookies file. + GITCOOKIES_REDACT_RE = re.compile(r'1/.*') + + def debug_summary_state(self) -> str: + gitcookies_path = self.get_gitcookies_path() + if os.path.isfile(gitcookies_path): + gitcookies = gclient_utils.FileRead(gitcookies_path) + return self.GITCOOKIES_REDACT_RE.sub('REDACTED', gitcookies) + return '' + def get_auth_email(self, host): """Best effort parsing of email to be used for auth for the given host.""" a = self._get_auth_for_host(host) @@ -241,7 +279,7 @@ class GceAuthenticator(Authenticator): _token_expiration = None @classmethod - def is_gce(cls): + def is_applicable(cls): if os.getenv('SKIP_GCE_AUTH_FOR_GIT'): return False if cls._cache_is_gce is None: @@ -301,12 +339,16 @@ class GceAuthenticator(Authenticator): return None return '%(token_type)s %(access_token)s' % token_dict + def debug_summary_state(self) -> str: + # TODO(b/343230702) - report ambient account name. + return '' + class LuciContextAuthenticator(Authenticator): """Authenticator implementation that uses LUCI_CONTEXT ambient local auth. """ @staticmethod - def is_luci(): + def is_applicable(): return auth.has_luci_context_local_auth() def __init__(self): @@ -316,6 +358,10 @@ class LuciContextAuthenticator(Authenticator): def get_auth_header(self, _host): return 'Bearer %s' % self._authenticator.get_access_token().token + def debug_summary_state(self) -> str: + # TODO(b/343230702) - report ambient account name. + return '' + def CreateHttpConn(host, path, diff --git a/git_cl.py b/git_cl.py index 1150e397e..2e5071f1d 100755 --- a/git_cl.py +++ b/git_cl.py @@ -2276,7 +2276,7 @@ class Changelist(object): # Fall back on still unique, but less efficient change number. return str(self.GetIssue()) - def EnsureAuthenticated(self, force): + def EnsureAuthenticated(self, force: bool) -> None | NoReturn: """Best effort check that user is authenticated with Gerrit server.""" if settings.GetGerritSkipEnsureAuthenticated(): # For projects with unusual authentication schemes. @@ -2890,14 +2890,12 @@ class Changelist(object): gclient_utils.FileWrite(os.path.join(git_info_dir, 'git-config'), git_config) - cookie_auth = gerrit_util.Authenticator.get() - if isinstance(cookie_auth, gerrit_util.CookiesAuthenticator): - gitcookies_path = cookie_auth.get_gitcookies_path() - if os.path.isfile(gitcookies_path): - gitcookies = gclient_utils.FileRead(gitcookies_path) - gclient_utils.FileWrite( - os.path.join(git_info_dir, 'gitcookies'), - GITCOOKIES_REDACT_RE.sub('REDACTED', gitcookies)) + authenticator = gerrit_util.Authenticator.get() + # Writes a file like CookiesAuthenticator.debug_summary_state + gclient_utils.FileWrite( + os.path.join(git_info_dir, + f'{type(authenticator).__name__}.debug_summary_state'), + authenticator.debug_summary_state()) shutil.make_archive(git_info_zip, 'zip', git_info_dir) gclient_utils.rmtree(git_info_dir) @@ -5312,7 +5310,7 @@ def _UploadAllPrecheck( DieWithError('Can\'t upload from detached HEAD state. Get on a branch!') branch_ref = None - cls = [] + cls: List[Changelist] = [] must_upload_upstream = False first_pass = True diff --git a/tests/gerrit_util_test.py b/tests/gerrit_util_test.py index 3d8c15a28..107ee7851 100755 --- a/tests/gerrit_util_test.py +++ b/tests/gerrit_util_test.py @@ -187,16 +187,16 @@ class GceAuthenticatorTest(unittest.TestCase): def testIsGce_EnvVarSkip(self, *_mocks): os.getenv.return_value = '1' - self.assertFalse(self.GceAuthenticator.is_gce()) + self.assertFalse(self.GceAuthenticator.is_applicable()) os.getenv.assert_called_once_with('SKIP_GCE_AUTH_FOR_GIT') def testIsGce_Error(self): httplib2.Http().request.side_effect = httplib2.HttpLib2Error - self.assertFalse(self.GceAuthenticator.is_gce()) + self.assertFalse(self.GceAuthenticator.is_applicable()) def testIsGce_500(self): httplib2.Http().request.return_value = (mock.Mock(status=500), None) - self.assertFalse(self.GceAuthenticator.is_gce()) + self.assertFalse(self.GceAuthenticator.is_applicable()) last_call = gerrit_util.time_sleep.mock_calls[-1] self.assertLessEqual(last_call, mock.call(43.0)) @@ -207,21 +207,21 @@ class GceAuthenticatorTest(unittest.TestCase): (mock.Mock(status=500), None), (response, 'who cares'), ] - self.assertTrue(self.GceAuthenticator.is_gce()) + self.assertTrue(self.GceAuthenticator.is_applicable()) def testIsGce_MetadataFlavorIsNotGoogle(self): response = mock.Mock(status=200) response.get.return_value = None httplib2.Http().request.return_value = (response, 'who cares') - self.assertFalse(self.GceAuthenticator.is_gce()) + self.assertFalse(self.GceAuthenticator.is_applicable()) response.get.assert_called_once_with('metadata-flavor') def testIsGce_ResultIsCached(self): response = mock.Mock(status=200) response.get.return_value = 'Google' httplib2.Http().request.side_effect = [(response, 'who cares')] - self.assertTrue(self.GceAuthenticator.is_gce()) - self.assertTrue(self.GceAuthenticator.is_gce()) + self.assertTrue(self.GceAuthenticator.is_applicable()) + self.assertTrue(self.GceAuthenticator.is_applicable()) httplib2.Http().request.assert_called_once() def testGetAuthHeader_Error(self): diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 44956c64a..2b4f29726 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -645,9 +645,9 @@ class TestGitCl(unittest.TestCase): lambda h, i, msg=None, labels=None, notify=None, ready=None: (self._mocked_call('SetReview', h, i, msg, labels, notify, ready))).start() - mock.patch('git_cl.gerrit_util.LuciContextAuthenticator.is_luci', + mock.patch('git_cl.gerrit_util.LuciContextAuthenticator.is_applicable', return_value=False).start() - mock.patch('git_cl.gerrit_util.GceAuthenticator.is_gce', + mock.patch('git_cl.gerrit_util.GceAuthenticator.is_applicable', return_value=False).start() mock.patch('git_cl.gerrit_util.ValidAccounts', lambda *a: self._mocked_call('ValidAccounts', *a)).start() @@ -1115,12 +1115,26 @@ class TestGitCl(unittest.TestCase): ( ([ 'FileWrite', - os.path.join('TEMP_DIR', 'gitcookies'), + os.path.join( + 'TEMP_DIR', + 'CookiesAuthenticatorMock.debug_summary_state'), 'gitcookies REDACTED' ], ), None, ), ] + else: + calls += [ + ( + ([ + 'FileWrite', + os.path.join( + 'TEMP_DIR', + 'CookiesAuthenticatorMock.debug_summary_state'), '' + ], ), + None, + ), + ] calls += [ # Make zip file for the git config and gitcookies. (