diff --git a/gerrit_util.py b/gerrit_util.py index 579dea76b..a8c5440c8 100644 --- a/gerrit_util.py +++ b/gerrit_util.py @@ -230,6 +230,10 @@ class SSOAuthenticator(Authenticator): # cookies. _testing_load_expired_cookies = False + # How long we should wait for the sso helper to write and close stdout. + # Overridden in tests. + _timeout_secs = 5 + # Tri-state cache for sso helper command: # * None - no lookup yet # * () - lookup was performed, but no binary was found. @@ -322,60 +326,80 @@ class SSOAuthenticator(Authenticator): Raises an exception if something goes wrong. """ - tdir = tempfile.mkdtemp(suffix='gerrit_util') - tf = os.path.join(tdir, 'git-remote-sso.stderr') + cmd = cls._resolve_sso_cmd() with tempdir() as tdir: - cmd = cls._resolve_sso_cmd() - - stderr_file = open(tf, mode='w') - - # NOTE: The git-remote-sso helper does the following: - # - # 1. writes files to disk. - # 2. writes config to stdout, referencing those files. - # 3. closes stdout (thus sending EOF to us, allowing - # sys.stdout.read() to complete). - # 4. waits for stdin to close. - # 5. deletes files on disk (which is why we make sys.stdin a PIPE - # instead of closing it outright). - # - # NOTE: the http.proxy value in the emitted config points to - # a socket which is owned by a system service, not `proc` itself. - with subprocess2.Popen(cmd, - stdout=subprocess2.PIPE, - stderr=stderr_file, - stdin=subprocess2.PIPE, - encoding='utf-8') as proc: - timedout = False - - def _fire_timeout(): - nonlocal timedout - timedout = True - proc.kill() - - timer = threading.Timer(5, _fire_timeout) - timer.start() - try: - ret = cls._parse_config(proc.stdout.read()) - finally: - timer.cancel() - - if timedout: - LOGGER.error( - 'SSOAuthenticator: Timeout: %r: reading config.', cmd) - raise subprocess.TimeoutExpired(cmd=cmd, timeout=5) - - proc.poll() - if (retcode := proc.returncode) is not None: - # process failed - we should be able to read the tempfile. - stderr_file.close() - with open(tf, encoding='utf-8') as stderr: - sys.exit( - f'SSOAuthenticator: exit {retcode}: {stderr.read().strip()}' - ) - - return ret + tf = os.path.join(tdir, 'git-remote-sso.stderr') + + with open(tf, mode='w') as stderr_file: + # NOTE: The git-remote-sso helper does the following: + # + # 1. writes files to disk. + # 2. writes config to stdout, referencing those files. + # 3. closes stdout (thus sending EOF to us, allowing + # sys.stdout.read() to complete). + # 4. waits for stdin to close. + # 5. deletes files on disk (which is why we make sys.stdin a PIPE + # instead of closing it outright). + # + # NOTE: the http.proxy value in the emitted config points to + # a socket which is owned by a system service, not `proc` itself. + with subprocess2.Popen(cmd, + stdout=subprocess2.PIPE, + stderr=stderr_file, + stdin=subprocess2.PIPE, + encoding='utf-8') as proc: + stderr_file.close() # we can close after process starts. + timedout = False + + def _fire_timeout(): + nonlocal timedout + timedout = True + proc.kill() + + timer = threading.Timer(cls._timeout_secs, _fire_timeout) + timer.start() + try: + stdout_data = proc.stdout.read() + finally: + timer.cancel() + + if timedout: + LOGGER.error( + 'SSOAuthenticator: Timeout: %r: reading config.', + cmd) + raise subprocess.TimeoutExpired( + cmd=cmd, timeout=cls._timeout_secs) + + # if the process already ended, then something is wrong. + retcode = proc.poll() + # if stdout was closed without any data, we need to wait for + # end-of-process here and hope for an error message - the + # poll above is racy in this case (we could see stdout EOF + # but the process may not have quit yet). + if not retcode and not stdout_data: + retcode = proc.wait(timeout=cls._timeout_secs) + # We timed out while doing `wait` - we can't safely open + # stderr on windows, so just emit a generic timeout + # exception. + if retcode is None: + LOGGER.error( + 'SSOAuthenticator: Timeout: %r: waiting error output.', + cmd) + raise subprocess.TimeoutExpired( + cmd=cmd, timeout=cls._timeout_secs) + + # Finally, if the poll or wait ended up getting the retcode, + # it means the process failed, so we can read the stderr + # file and reflect it back to the user. + if retcode is not None: + # process failed - we should be able to read the tempfile. + with open(tf, encoding='utf-8') as stderr: + sys.exit( + f'SSOAuthenticator: exit {retcode}: {stderr.read().strip()}' + ) + + return cls._parse_config(stdout_data) @classmethod def _get_sso_info(cls) -> SSOInfo: diff --git a/tests/gerrit_util_test.inputs/testLaunchHelperFailQuick/git-remote-sso.py b/tests/gerrit_util_test.inputs/testLaunchHelperFailQuick/git-remote-sso.py new file mode 100755 index 000000000..d1c689abb --- /dev/null +++ b/tests/gerrit_util_test.inputs/testLaunchHelperFailQuick/git-remote-sso.py @@ -0,0 +1,10 @@ +#!/usr/bin/env python3 +# Copyright 2024 The Chromium Authors. All rights reserved. +# Use of this source code is governed by a BSD-style license that can be +# found in the LICENSE file. + +import os +import sys + +print("SSO Failure Message!!!", file=sys.stderr) +os.close(1) # signal that we've written all config diff --git a/tests/gerrit_util_test.inputs/testLaunchHelperFailSlow/git-remote-sso.py b/tests/gerrit_util_test.inputs/testLaunchHelperFailSlow/git-remote-sso.py new file mode 100755 index 000000000..ab3d4a8d7 --- /dev/null +++ b/tests/gerrit_util_test.inputs/testLaunchHelperFailSlow/git-remote-sso.py @@ -0,0 +1,10 @@ +#!/usr/bin/env python3 +# Copyright 2024 The Chromium Authors. All rights reserved. +# Use of this source code is governed by a BSD-style license that can be +# found in the LICENSE file. + +import sys +import time + +time.sleep(6) +print(f"I ran too long without writing output!!", file=sys.stderr) diff --git a/tests/gerrit_util_test.inputs/testLaunchHelperOK/cookiefile.txt b/tests/gerrit_util_test.inputs/testLaunchHelperOK/cookiefile.txt new file mode 100644 index 000000000..8fa963407 --- /dev/null +++ b/tests/gerrit_util_test.inputs/testLaunchHelperOK/cookiefile.txt @@ -0,0 +1,6 @@ +# Netscape HTTP Cookie File +# https://curl.se/docs/http-cookies.html +# This file was generated by libcurl! Edit at your own risk. + +#HttpOnly_login.example.com FALSE / FALSE 1718730497 SSO TUVFUE1PUlAK +#HttpOnly_.example.com TRUE / FALSE 1718730497 __CoolProxy QkxFRVBCTE9SUAo= diff --git a/tests/gerrit_util_test.inputs/testLaunchHelperOK/git-remote-sso.py b/tests/gerrit_util_test.inputs/testLaunchHelperOK/git-remote-sso.py new file mode 100755 index 000000000..085885623 --- /dev/null +++ b/tests/gerrit_util_test.inputs/testLaunchHelperOK/git-remote-sso.py @@ -0,0 +1,38 @@ +#!/usr/bin/env python3 +# Copyright 2024 The Chromium Authors. All rights reserved. +# Use of this source code is governed by a BSD-style license that can be +# found in the LICENSE file. + +import os +import shutil +import sys +import tempfile + +from pathlib import Path + +THIS_DIR = Path(__file__).parent.absolute() + +with tempfile.TemporaryDirectory() as tempdir: + tempdir = Path(tempdir) + + target_config = tempdir / "gitconfig" + target_cookies = tempdir / "cookiefile.txt" + + shutil.copyfile(THIS_DIR / "gitconfig", target_config) + shutil.copyfile(THIS_DIR / "cookiefile.txt", target_cookies) + + print('http.proxy=localhost:12345') + print(f'include.path={target_config}') + print(f'http.cookiefile={target_cookies}') + sys.stdout.flush() + # need to fully close file descriptor, sys.stdout.close() doesn't seem to cut + # it. + os.close(1) + + print("OK", file=sys.stderr) + + # block until stdin closes, then clean everything via TemporaryDirectory(). + # + # This emulates the behavior of the real git-remote-sso helper which just + # prints temporary configuration for a daemon running elsewhere. + sys.stdin.read() diff --git a/tests/gerrit_util_test.inputs/testLaunchHelperOK/gitconfig b/tests/gerrit_util_test.inputs/testLaunchHelperOK/gitconfig new file mode 100644 index 000000000..5a8add7eb --- /dev/null +++ b/tests/gerrit_util_test.inputs/testLaunchHelperOK/gitconfig @@ -0,0 +1,2 @@ +[http] + extraHeader = Authorization: Basic REALLY_COOL_TOKEN diff --git a/tests/gerrit_util_test.py b/tests/gerrit_util_test.py index e2e6705a5..29c43740e 100755 --- a/tests/gerrit_util_test.py +++ b/tests/gerrit_util_test.py @@ -7,6 +7,7 @@ import json import os import socket +import subprocess import sys import textwrap import unittest @@ -25,6 +26,8 @@ import git_common import metrics import subprocess2 +RUN_SUBPROC_TESTS = 'RUN_SUBPROC_TESTS' in os.environ + def makeConn(host: str) -> gerrit_util.HttpConn: """Makes an empty gerrit_util.HttpConn for the given host.""" @@ -590,10 +593,16 @@ class GerritUtilTest(unittest.TestCase): class SSOAuthenticatorTest(unittest.TestCase): + @classmethod + def setUpClass(cls) -> None: + cls._original_timeout_secs = gerrit_util.SSOAuthenticator._timeout_secs + return super().setUpClass() + def setUp(self) -> None: gerrit_util.SSOAuthenticator._sso_cmd = None gerrit_util.SSOAuthenticator._sso_info = None gerrit_util.SSOAuthenticator._testing_load_expired_cookies = True + gerrit_util.SSOAuthenticator._timeout_secs = self._original_timeout_secs self.sso = gerrit_util.SSOAuthenticator() return super().setUp() @@ -601,11 +610,14 @@ class SSOAuthenticatorTest(unittest.TestCase): gerrit_util.SSOAuthenticator._sso_cmd = None gerrit_util.SSOAuthenticator._sso_info = None gerrit_util.SSOAuthenticator._testing_load_expired_cookies = False + gerrit_util.SSOAuthenticator._timeout_secs = self._original_timeout_secs return super().tearDown() @property def _input_dir(self) -> Path: - return Path(__file__).with_suffix('.inputs') / self._testMethodName + base = Path(__file__).absolute().with_suffix('.inputs') + # Here _testMethodName would be a string like "testCmdAssemblyFound" + return base / self._testMethodName @mock.patch('shutil.which', return_value='/fake/git-remote-sso') def testCmdAssemblyFound(self, _): @@ -631,8 +643,8 @@ class SSOAuthenticatorTest(unittest.TestCase): somekey=a value with = in it novalue= http.proxy=localhost:12345 - http.cookiefile={(self._input_dir/'cookiefile.txt').absolute()} - include.path={(self._input_dir/'gitconfig').absolute()} + http.cookiefile={self._input_dir/'cookiefile.txt'} + include.path={self._input_dir/'gitconfig'} ''').strip()) self.assertDictEqual(parsed.headers, { 'Authorization': 'Basic REALLY_COOL_TOKEN', @@ -646,6 +658,43 @@ class SSOAuthenticatorTest(unittest.TestCase): self.assertEqual(c['.example.com']['/']['__CoolProxy'].value, 'QkxFRVBCTE9SUAo=') + @unittest.skipUnless(RUN_SUBPROC_TESTS, 'subprocess tests are flakey') + def testLaunchHelperOK(self): + gerrit_util.SSOAuthenticator._sso_cmd = ('python3', + str(self._input_dir / + 'git-remote-sso.py')) + + info = self.sso._get_sso_info() + self.assertDictEqual(info.headers, { + 'Authorization': 'Basic REALLY_COOL_TOKEN', + }) + self.assertEqual(info.proxy.proxy_host, b'localhost') + self.assertEqual(info.proxy.proxy_port, 12345) + c = info.cookies._cookies + self.assertEqual(c['login.example.com']['/']['SSO'].value, + 'TUVFUE1PUlAK') + self.assertEqual(c['.example.com']['/']['__CoolProxy'].value, + 'QkxFRVBCTE9SUAo=') + + @unittest.skipUnless(RUN_SUBPROC_TESTS, 'subprocess tests are flakey') + def testLaunchHelperFailQuick(self): + gerrit_util.SSOAuthenticator._sso_cmd = ('python3', + str(self._input_dir / + 'git-remote-sso.py')) + + with self.assertRaisesRegex(SystemExit, "SSO Failure Message!!!"): + self.sso._get_sso_info() + + @unittest.skipUnless(RUN_SUBPROC_TESTS, 'subprocess tests are flakey') + def testLaunchHelperFailSlow(self): + gerrit_util.SSOAuthenticator._timeout_secs = 0.2 + gerrit_util.SSOAuthenticator._sso_cmd = ('python3', + str(self._input_dir / + 'git-remote-sso.py')) + + with self.assertRaises(subprocess.TimeoutExpired): + self.sso._get_sso_info() + if __name__ == '__main__': unittest.main()