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 = {