diff --git a/gerrit_util.py b/gerrit_util.py index 8669b8cd0..dbf4b3a51 100644 --- a/gerrit_util.py +++ b/gerrit_util.py @@ -352,6 +352,11 @@ class SSOAuthenticator(_Authenticator): # Overridden in tests. _timeout_secs = 5 + # The required fields for the sso config, expected from the sso helper's + # output. + _required_fields = frozenset( + ['http.cookiefile', 'http.proxy', 'include.path']) + @dataclass class SSOInfo: proxy: httplib2.ProxyInfo @@ -387,24 +392,27 @@ class SSOAuthenticator(_Authenticator): return email.endswith('@google.com') @classmethod - def _parse_config(cls, config: str) -> SSOInfo: - parsed: Dict[str, str] = dict(line.strip().split('=', 1) - for line in config.splitlines()) + def _parse_config(cls, config: Dict[str, str]) -> SSOInfo: + """Parses the sso info from the given config. + Note: update cls._required_fields appropriately when making + changes to this method, to ensure the field values are captured + from the sso helper. + """ fullAuthHeader = cast( str, scm.GIT.Capture([ 'config', '-f', - parsed['include.path'], + config['include.path'], 'http.extraHeader', ])) headerKey, headerValue = fullAuthHeader.split(':', 1) headers = {headerKey.strip(): headerValue.strip()} - proxy_host, proxy_port = parsed['http.proxy'].split(':', 1) + proxy_host, proxy_port = config['http.proxy'].split(':', 1) - cfpath = parsed['http.cookiefile'] + cfpath = config['http.cookiefile'] cj = http.cookiejar.MozillaCookieJar(cfpath) # NOTE: python3.8 doesn't support httponly cookie lines, so we parse # this manually. Once we move to python3.10+, this hack can be removed. @@ -442,8 +450,9 @@ class SSOAuthenticator(_Authenticator): # # 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). + # 3. closes stdout (sending EOF to us, ending the stdout data). + # - Note: on Windows, stdout.read() times out anyway, so + # instead we process stdout line by line. # 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). @@ -466,24 +475,39 @@ class SSOAuthenticator(_Authenticator): timer = threading.Timer(cls._timeout_secs, _fire_timeout) timer.start() try: - stdout_data = proc.stdout.read() + config = {} + for line in proc.stdout: + if not line: + break + field, value = line.strip().split('=', 1) + config[field] = value + # Stop reading if we have all the required fields. + if cls._required_fields.issubset(config.keys()): + break finally: timer.cancel() + missing_fields = cls._required_fields - config.keys() if timedout: + # Within the time limit, at least one required field was + # still missing. Killing the process has been triggered, + # even if we now have all fields. + details = '' + if missing_fields: + details = ': missing fields [{names}]'.format( + names=', '.join(missing_fields)) LOGGER.error( - 'SSOAuthenticator: Timeout: %r: reading config.', - cmd) + 'SSOAuthenticator: Timeout: %r: reading config%s.', + cmd, details) 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: + # if stdout was closed without all required 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 missing_fields: 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 @@ -505,7 +529,7 @@ class SSOAuthenticator(_Authenticator): f'SSOAuthenticator: exit {retcode}: {stderr.read().strip()}' ) - return cls._parse_config(stdout_data) + return cls._parse_config(config) @classmethod def _get_sso_info(cls) -> SSOInfo: diff --git a/tests/gerrit_util_test.py b/tests/gerrit_util_test.py index 2e05cf3b9..894205616 100755 --- a/tests/gerrit_util_test.py +++ b/tests/gerrit_util_test.py @@ -647,14 +647,14 @@ class SSOAuthenticatorTest(unittest.TestCase): self.assertFalse(self.sso.is_applicable()) def testParseConfigOK(self): - parsed = self.sso._parse_config( - textwrap.dedent(f''' - somekey=a value with = in it - novalue= - http.proxy=localhost:12345 - http.cookiefile={self._input_dir/'cookiefile.txt'} - include.path={self._input_dir/'gitconfig'} - ''').strip()) + test_config = { + 'somekey': 'a value with = in it', + 'novalue': '', + 'http.proxy': 'localhost:12345', + 'http.cookiefile': str(self._input_dir / 'cookiefile.txt'), + 'include.path': str(self._input_dir / 'gitconfig'), + } + parsed = self.sso._parse_config(test_config) self.assertDictEqual(parsed.headers, { 'Authorization': 'Basic REALLY_COOL_TOKEN', })