[gerrit_util] Move Authenticator to be private in gerrit_util.

This should help give additional confidence while refactoring in
gerrit_util.

R=ayatane, yiwzhang

Change-Id: I03927e072e62f6109571ab699f90db7c51ccc6c0
Bug: b/335483238
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/5665455
Auto-Submit: Robbie Iannucci <iannucci@chromium.org>
Reviewed-by: Allen Li <ayatane@chromium.org>
Reviewed-by: Yiwei Zhang <yiwzhang@google.com>
Commit-Queue: Yiwei Zhang <yiwzhang@google.com>

gerrit_util.py

@@ -200,11 +200,11 @@ def ShouldUseSSO(host: str) -> bool:
     return False
 
 
-class Authenticator(object):
+class _Authenticator(object):
     """Base authenticator class for authenticator implementations to subclass."""
 
-    # Cached Authenticator subclass instance, resolved via get().
-    _resolved: Optional[Authenticator] = None
+    # Cached _Authenticator subclass instance, resolved via get().
+    _resolved: Optional[_Authenticator] = None
     _resolved_lock = threading.Lock()
 
     def authenticate(self, conn: HttpConn):
@@ -212,7 +212,7 @@ class Authenticator(object):
         raise NotImplementedError()
 
     def debug_summary_state(self) -> str:
-        """If this Authenticator has any debugging information about its state,
+        """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
@@ -241,12 +241,12 @@ class Authenticator(object):
 
     @classmethod
     def get(cls):
-        """Returns: (Authenticator) The identified Authenticator to use.
+        """Returns: (_Authenticator) The identified _Authenticator to use.
 
         Probes the local system and its environment and identifies the
-        Authenticator instance to use.
+        _Authenticator instance to use.
 
-        The resolved Authenticator instance is cached as a class variable.
+        The resolved _Authenticator instance is cached as a class variable.
         """
         with cls._resolved_lock:
             if ret := cls._resolved:
@@ -258,14 +258,14 @@ class Authenticator(object):
             skip_sso = newauth.SkipSSO()
 
             if use_new_auth:
-                LOGGER.debug('Authenticator.get: using new auth stack')
+                LOGGER.debug('_Authenticator.get: using new auth stack')
                 if LuciContextAuthenticator.is_applicable():
                     LOGGER.debug(
-                        'Authenticator.get: using LUCI context authenticator')
+                        '_Authenticator.get: using LUCI context authenticator')
                     ret = LuciContextAuthenticator()
                 else:
                     LOGGER.debug(
-                        'Authenticator.get: using chained authenticator')
+                        '_Authenticator.get: using chained authenticator')
                     a = [
                         SSOAuthenticator(),
                         # GCE detection can't distinguish cloud workstations.
@@ -287,7 +287,7 @@ class Authenticator(object):
 
             for candidate in authenticators:
                 if candidate.is_applicable():
-                    LOGGER.debug('Authenticator.get: Selected %s.',
+                    LOGGER.debug('_Authenticator.get: Selected %s.',
                                  candidate.__name__)
                     ret = candidate()
                     cls._resolved = ret
@@ -299,7 +299,23 @@ class Authenticator(object):
             )
 
 
-class SSOAuthenticator(Authenticator):
+def debug_auth() -> Tuple[str, str]:
+    """Returns the name of the chosen auth scheme and any additional debugging
+    information for that authentication scheme. """
+    authn = _Authenticator.get()
+    return authn.__class__.__name__, authn.debug_summary_state()
+
+
+def ensure_authenticated(gerrit_host: str, git_host: str) -> Tuple[bool, str]:
+    """Returns (bypassable, error message).
+
+    If the error message is empty, there is no error to report. If bypassable is
+    true, the caller will allow the user to continue past the error.
+    """
+    return _Authenticator.get().ensure_authenticated(gerrit_host, git_host)
+
+
+class SSOAuthenticator(_Authenticator):
     """SSOAuthenticator implements a Google-internal authentication scheme.
 
     TEMPORARY configuration for Googlers (one `url` block for each Gerrit host):
@@ -515,8 +531,8 @@ class SSOAuthenticator(Authenticator):
         return ''
 
 
-class CookiesAuthenticator(Authenticator):
-    """Authenticator implementation that uses ".gitcookies" for token.
+class CookiesAuthenticator(_Authenticator):
+    """_Authenticator implementation that uses ".gitcookies" for token.
 
     Expected case for developer workstations.
     """
@@ -525,7 +541,7 @@ class CookiesAuthenticator(Authenticator):
 
     def __init__(self):
         # Credentials will be loaded lazily on first use. This ensures
-        # Authenticator get() can always construct an authenticator, even if
+        # _Authenticator get() can always construct an authenticator, even if
         # something is broken. This allows 'creds-check' to proceed to actually
         # checking creds later, rigorously (instead of blowing up with a cryptic
         # error if they are wrong).
@@ -677,8 +693,8 @@ class CookiesAuthenticator(Authenticator):
         return '%s@%s' % (username, domain)
 
 
-class GceAuthenticator(Authenticator):
-    """Authenticator implementation that uses GCE metadata service for token.
+class GceAuthenticator(_Authenticator):
+    """_Authenticator implementation that uses GCE metadata service for token.
     """
 
     _INFO_URL = 'http://metadata.google.internal'
@@ -757,8 +773,8 @@ class GceAuthenticator(Authenticator):
         return ''
 
 
-class LuciContextAuthenticator(Authenticator):
-    """Authenticator implementation that uses LUCI_CONTEXT ambient local auth.
+class LuciContextAuthenticator(_Authenticator):
+    """_Authenticator implementation that uses LUCI_CONTEXT ambient local auth.
     """
     @staticmethod
     def is_applicable(*, conn: Optional[HttpConn] = None):
@@ -778,7 +794,7 @@ class LuciContextAuthenticator(Authenticator):
 
 
 class LuciAuthAuthenticator(LuciContextAuthenticator):
-    """Authenticator implementation that uses `luci-auth` credentials.
+    """_Authenticator implementation that uses `luci-auth` credentials.
 
     This is the same as LuciContextAuthenticator, except that it is for local
     non-google.com developer credentials.
@@ -789,13 +805,13 @@ class LuciAuthAuthenticator(LuciContextAuthenticator):
         return True
 
 
-class ChainedAuthenticator(Authenticator):
-    """Authenticator that delegates to others in sequence.
+class ChainedAuthenticator(_Authenticator):
+    """_Authenticator that delegates to others in sequence.
 
     Authenticators should implement the method `is_applicable_for`.
     """
 
-    def __init__(self, authenticators: List[Authenticator]):
+    def __init__(self, authenticators: List[_Authenticator]):
         self.authenticators = list(authenticators)
 
     def is_applicable(self, *, conn: Optional[HttpConn] = None) -> bool:
@@ -889,7 +905,7 @@ def CreateHttpConn(host,
                    body: Optional[Dict] = None,
                    timeout=300,
                    *,
-                   authenticator: Optional[Authenticator] = None) -> HttpConn:
+                   authenticator: Optional[_Authenticator] = None) -> HttpConn:
     """Opens an HTTPS connection to a Gerrit service, and sends a request."""
     headers = headers or {}
     bare_host = host.partition(':')[0]
@@ -914,7 +930,7 @@ def CreateHttpConn(host,
                     req_body=rendered_body)
 
     if authenticator is None:
-        authenticator = Authenticator.get()
+        authenticator = _Authenticator.get()
     # TODO(crbug.com/1059384): Automatically detect when running on cloudtop.
     if isinstance(authenticator, GceAuthenticator):
         print('If you\'re on a cloudtop instance, export '
@@ -1728,7 +1744,7 @@ class EmailRecord(TypedDict):
 def GetAccountEmails(host,
                      account_id='self',
                      *,
-                     authenticator: Optional[Authenticator] = None
+                     authenticator: Optional[_Authenticator] = None
                      ) -> Optional[List[EmailRecord]]:
     """Returns all emails for this account, and an indication of which of these
     is preferred.

git_cl.py

@@ -2368,7 +2368,8 @@ class Changelist(object):
         git_host = self._GetGitHost()
         assert self._gerrit_server and self._gerrit_host and git_host
 
-        bypassable, msg = gerrit_util.Authenticator.get().ensure_authenticated(git_host, self._gerrit_host)
+        bypassable, msg = gerrit_util.ensure_authenticated(
+            git_host, self._gerrit_host)
         if not msg:
             return  # OK
         if bypassable:
@@ -2935,12 +2936,11 @@ class Changelist(object):
         gclient_utils.FileWrite(os.path.join(git_info_dir, 'git-config'),
                                 git_config)
 
-        authenticator = gerrit_util.Authenticator.get()
+        auth_name, debug_state = gerrit_util.debug_auth()
         # 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())
+            os.path.join(git_info_dir, f'{auth_name}.debug_summary_state'),
+            debug_state)
         shutil.make_archive(git_info_zip, 'zip', git_info_dir)
 
         gclient_utils.rmtree(git_info_dir)
@@ -4013,14 +4013,14 @@ def CMDcreds_check(parser, args):
         return 0
 
     # Code below checks .gitcookies. Abort if using something else.
-    authn = gerrit_util.Authenticator.get()
-    if not isinstance(authn, gerrit_util.CookiesAuthenticator):
+    auth_name, _ = gerrit_util.debug_auth()
+    if auth_name != "CookiesAuthenticator":
         message = (
             'This command is not designed for bot environment. It checks '
             '~/.gitcookies file not generally used on bots.')
         # TODO(crbug.com/1059384): Automatically detect when running on
         # cloudtop.
-        if isinstance(authn, gerrit_util.GceAuthenticator):
+        if auth_name == "GceAuthenticator":
             message += (
                 '\n'
                 'If you need to run this on GCE or a cloudtop instance, '

tests/gerrit_util_test.py

@@ -322,7 +322,7 @@ class GerritUtilTest(unittest.TestCase):
                                      'first param+'))
 
     @mock.patch('gerrit_util.CookiesAuthenticator._get_auth_for_host')
-    @mock.patch('gerrit_util.Authenticator.get')
+    @mock.patch('gerrit_util._Authenticator.get')
     def testCreateHttpConn_Basic(self, mockAuth, cookieAuth):
         mockAuth.return_value = gerrit_util.CookiesAuthenticator()
         cookieAuth.return_value = None
@@ -338,7 +338,7 @@ class GerritUtilTest(unittest.TestCase):
             }, conn.req_params)
 
     @mock.patch('gerrit_util.CookiesAuthenticator._get_auth_for_host')
-    @mock.patch('gerrit_util.Authenticator.get')
+    @mock.patch('gerrit_util._Authenticator.get')
     def testCreateHttpConn_Authenticated(self, mockAuth, cookieAuth):
         mockAuth.return_value = gerrit_util.CookiesAuthenticator()
         cookieAuth.return_value = (None, 'token')
@@ -359,7 +359,7 @@ class GerritUtilTest(unittest.TestCase):
             }, conn.req_params)
 
     @mock.patch('gerrit_util.CookiesAuthenticator._get_auth_for_host')
-    @mock.patch('gerrit_util.Authenticator')
+    @mock.patch('gerrit_util._Authenticator')
     def testCreateHttpConn_Body(self, mockAuth, cookieAuth):
         mockAuth.return_value = gerrit_util.CookiesAuthenticator()
         cookieAuth.return_value = None

tests/git_cl_test.py

@@ -669,7 +669,7 @@ class TestGitCl(unittest.TestCase):
         # It's important to reset settings to not have inter-tests interference.
         git_cl.settings = git_cl.Settings()
         self.addCleanup(mock.patch.stopall)
-        gerrit_util.Authenticator._resolved = None
+        gerrit_util._Authenticator._resolved = None
 
     def tearDown(self):
         try:
@@ -3646,7 +3646,7 @@ class ChangelistTest(unittest.TestCase):
         self.mockGit = GitMocks()
         mock.patch('scm.GIT.GetConfig', self.mockGit.GetConfig).start()
         mock.patch('scm.GIT.SetConfig', self.mockGit.SetConfig).start()
-        gerrit_util.Authenticator._resolved = None
+        gerrit_util._Authenticator._resolved = None
 
     def testRunHook(self):
         expected_results = {