[gerrit_util] Add some additional tests and fixes for SSOAuthenticator.

Unfortunately, the depot_tools presubmit builders are incredibly slow
which make the subprocess based tests fail flakily. I've marked them
all as `skip` with an optional way to run them locally.

R=ayatane, yiwzhang

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

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()
+            tf = os.path.join(tdir, 'git-remote-sso.stderr')
 
-            stderr_file = open(tf, mode='w')
+            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
 
-            # 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()
 
-                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()
 
-                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=cls._timeout_secs)
 
-                if timedout:
-                    LOGGER.error(
-                        'SSOAuthenticator: Timeout: %r: reading config.', cmd)
-                    raise subprocess.TimeoutExpired(cmd=cmd, timeout=5)
+                    # 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)
 
-                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()}'
-                        )
+                    # 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 ret
+                    return cls._parse_config(stdout_data)
 
     @classmethod
     def _get_sso_info(cls) -> SSOInfo:

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

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)

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=

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

tests/gerrit_util_test.inputs/testLaunchHelperOK/gitconfig

@@ -0,0 +1,2 @@
+[http]
+	extraHeader = Authorization: Basic REALLY_COOL_TOKEN

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