[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 <iannucci@chromium.org>
Reviewed-by: Yiwei Zhang <yiwzhang@google.com>
Commit-Queue: Yiwei Zhang <yiwzhang@google.com>

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.
         """
+        authenticators: List[Type[Authenticator]] = [
             # LUCI Context takes priority since it's normally present only on bots,
             # which then must use it.
-        if LuciContextAuthenticator.is_luci():
-            return LuciContextAuthenticator()
+            LuciContextAuthenticator,
+
             # TODO(crbug.com/1059384): Automatically detect when running on
             # cloudtop, and use CookiesAuthenticator instead.
-        if GceAuthenticator.is_gce():
-            return GceAuthenticator()
-        return CookiesAuthenticator()
+            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,

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)
+        authenticator = gerrit_util.Authenticator.get()
+        # Writes a file like CookiesAuthenticator.debug_summary_state
         gclient_utils.FileWrite(
-                    os.path.join(git_info_dir, 'gitcookies'),
-                    GITCOOKIES_REDACT_RE.sub('REDACTED', gitcookies))
+            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
 

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

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