From 779f70fd7cbeea89c93b876632c31bb573b7a6ac Mon Sep 17 00:00:00 2001
From: Robert Iannucci <iannucci@chromium.org>
Date: Mon, 15 Jul 2024 22:09:01 +0000
Subject: [PATCH] [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 | 68 ++++++++++++++++++++++++---------------
git_cl.py | 16 ++++-----
tests/gerrit_util_test.py | 6 ++--
tests/git_cl_test.py | 4 +--
4 files changed, 55 insertions(+), 39 deletions(-)
diff --git a/gerrit_util.py b/gerrit_util.py
index 09e8d57d0..e27c3f9b0 100644
--- a/gerrit_util.py
+++ b/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.
diff --git a/git_cl.py b/git_cl.py
index 11ed6af4d..07597d4d1 100755
--- a/git_cl.py
+++ b/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, '
diff --git a/tests/gerrit_util_test.py b/tests/gerrit_util_test.py
index d3d6d981e..3b94406f4 100755
--- a/tests/gerrit_util_test.py
+++ b/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
diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py
index 4b0706cf9..c61e2a1b7 100755
--- a/tests/git_cl_test.py
+++ b/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 = {