From a0d13c95494a0c36fe62c7dd9fb4dd21aa7d40fb Mon Sep 17 00:00:00 2001 From: Allen Li Date: Tue, 18 Mar 2025 14:38:56 -0700 Subject: [PATCH] [git_auth] Use pathless URL for cred helper The credential helper rules should not have a path, while the URL rewrite rules should. Reland of I79b52ab4af62367363617b2a9afa03a67f5ea0b9 Bug: b/401338175 Bug: b/403635929 Change-Id: Ib89d9e855ca5eba29cc67f8846bb7ca0cb3622ab Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6363080 Reviewed-by: Josip Sokcevic Commit-Queue: Allen Li --- git_auth.py | 30 +++++++++++++++++++++++------- tests/git_auth_test.py | 10 ++++------ 2 files changed, 27 insertions(+), 13 deletions(-) diff --git a/git_auth.py b/git_auth.py index b03f174f80..81311285e7 100644 --- a/git_auth.py +++ b/git_auth.py @@ -38,7 +38,7 @@ class ConfigChanger(object): # # Increment this when making changes to the config, so that reliant # code can determine whether the config needs to be re-applied. - VERSION: int = 4 + VERSION: int = 6 def __init__( self, @@ -69,12 +69,20 @@ class ConfigChanger(object): return _url_shortname(parts) @functools.cached_property - def _base_url(self) -> str: + def _host_url(self) -> str: + # Example: https://chromium.googlesource.com + # Example: https://chromium-review.googlesource.com + parts: urllib.parse.SplitResult = urllib.parse.urlsplit( + self._remote_url) + return _url_host_url(parts) + + @functools.cached_property + def _root_url(self) -> str: # Example: https://chromium.googlesource.com/ # Example: https://chromium-review.googlesource.com/ parts: urllib.parse.SplitResult = urllib.parse.urlsplit( self._remote_url) - return parts._replace(path='/', query='', fragment='').geturl() + return _url_root_url(parts) @classmethod def new_from_env(cls, cwd: str, cl: git_cl.Changelist) -> ConfigChanger: @@ -152,7 +160,7 @@ class ConfigChanger(object): def _apply_cred_helper(self, cwd: str) -> None: """Apply config changes relating to credential helper.""" - cred_key: str = f'credential.{self._base_url}.helper' + cred_key: str = f'credential.{self._host_url}.helper' if self.mode == ConfigMode.NEW_AUTH: self._set_config(cwd, cred_key, '', modify_all=True) self._set_config(cwd, cred_key, 'luci', append=True) @@ -163,6 +171,10 @@ class ConfigChanger(object): else: raise TypeError(f'Invalid mode {self.mode!r}') + # Cleanup old from version 4 + old_key: str = f'credential.{self._root_url}.helper' + self._set_config(cwd, old_key, None, modify_all=True) + def _apply_sso(self, cwd: str) -> None: """Apply config changes relating to SSO.""" sso_key: str = f'url.sso://{self._shortname}/.insteadOf' @@ -174,7 +186,7 @@ class ConfigChanger(object): self._set_config(cwd, http_key, self._remote_url, modify_all=True) elif self.mode == ConfigMode.NEW_AUTH_SSO: self._set_config(cwd, 'protocol.sso.allow', 'always') - self._set_config(cwd, sso_key, self._base_url, modify_all=True) + self._set_config(cwd, sso_key, self._root_url, modify_all=True) self._set_config(cwd, http_key, None, modify_all=True) elif self.mode == ConfigMode.NO_AUTH: self._set_config(cwd, 'protocol.sso.allow', None) @@ -198,7 +210,7 @@ class ConfigChanger(object): def _apply_global_cred_helper(self, cwd: str) -> None: """Apply config changes relating to credential helper.""" - cred_key: str = f'credential.{self._base_url}.helper' + cred_key: str = f'credential.{self._host_url}.helper' if self.mode == ConfigMode.NEW_AUTH: self._set_config(cwd, cred_key, '', scope='global', modify_all=True) self._set_config(cwd, cred_key, 'luci', scope='global', append=True) @@ -213,6 +225,10 @@ class ConfigChanger(object): else: raise TypeError(f'Invalid mode {self.mode!r}') + # Cleanup old from version 4 + old_key: str = f'credential.{self._root_url}.helper' + self._set_config(cwd, old_key, None, modify_all=True, scope='global') + def _apply_global_sso(self, cwd: str) -> None: """Apply config changes relating to SSO.""" sso_key: str = f'url.sso://{self._shortname}/.insteadOf' @@ -231,7 +247,7 @@ class ConfigChanger(object): scope='global') self._set_config(cwd, sso_key, - self._base_url, + self._root_url, scope='global', modify_all=True) elif self.mode == ConfigMode.NO_AUTH: diff --git a/tests/git_auth_test.py b/tests/git_auth_test.py index 184dadc533..286e36b006 100755 --- a/tests/git_auth_test.py +++ b/tests/git_auth_test.py @@ -44,7 +44,7 @@ class TestConfigChanger(unittest.TestCase): ).apply('/some/fake/dir') want = { '/some/fake/dir': { - 'credential.https://chromium.googlesource.com/.helper': + 'credential.https://chromium.googlesource.com.helper': ['', 'luci'], 'http.cookiefile': [''], 'url.https://chromium.googlesource.com/chromium/tools/depot_tools.git.insteadof': @@ -95,7 +95,7 @@ class TestConfigChanger(unittest.TestCase): ).apply('/some/fake/dir') want = { '/some/fake/dir': { - 'credential.https://chromium.googlesource.com/.helper': + 'credential.https://chromium.googlesource.com.helper': ['', 'luci'], 'http.cookiefile': [''], 'url.https://chromium.googlesource.com/chromium/tools/depot_tools.git.insteadof': @@ -166,8 +166,7 @@ class TestConfigChanger(unittest.TestCase): 'https://chromium.googlesource.com/chromium/tools/depot_tools.git', ).apply_global('/some/fake/dir') want = { - 'credential.https://chromium.googlesource.com/.helper': - ['', 'luci'], + 'credential.https://chromium.googlesource.com.helper': ['', 'luci'], } self.assertEqual(self.global_state, want) @@ -197,8 +196,7 @@ class TestConfigChanger(unittest.TestCase): ).apply_global('/some/fake/dir') want = { 'protocol.sso.allow': ['always'], - 'credential.https://chromium.googlesource.com/.helper': - ['', 'luci'], + 'credential.https://chromium.googlesource.com.helper': ['', 'luci'], } self.assertEqual(self.global_state, want)